Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

AutoDJ: don't use removed Intro end and outro start makers, use transition time instead #11830

Merged
merged 5 commits into from
Dec 19, 2023
Merged

AutoDJ: don't use removed Intro end and outro start makers, use transition time instead #11830

merged 5 commits into from
Dec 19, 2023

Conversation

daschuer
Copy link
Member

This is one part of #11648

@daschuer
Auto-DJ: Don't adopt last sound as outro end, when user has explicit …
dd608bf 
…deleted it.

Use transition time instead if possible, else fall back to last sound.
@daschuer
Auto-DJ: Don't adopt first sound as intro start, when user has explic…
430a8f8 
…it deleted it.

Use transition time instead if possible, else fall back to first sound.
@daschuer daschuer mentioned this pull request Aug 14, 2023
Closed
@ronso0 ronso0 changed the title Don't use removed Into end and outro start makers, use transition time instead. AutoDJ: don't use removed Intro end and outro start makers, use transition time instead Aug 14, 2023
@ronso0 ronso0 added autodj changelog This PR should be included in the changelog and removed library labels Aug 14, 2023
@daschuer daschuer added this to the 2.4.0 milestone Aug 15, 2023
@daschuer
Copy link
Member Author

@ikr83 has confirmed this is working good in #11648

@daschuer
Copy link
Member Author

daschuer commented Sep 9, 2023

This is the first step fixing #11648 entirely.
Original I have considered to fix this in 2.3.6 but since it is a slight a behavior change it does not suite a minor version.
Is this still a 2.4 candidate?

Swiftb0y
Swiftb0y previously requested changes Sep 10, 2023
View reviewed changes
src/library/autodj/autodjprocessor.cpp
Comment on lines +1157 to +1158
double outroEndFromTime = outroStartSecond + m_transitionTime;
if (outroEndFromTime < lastSoundSecond) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you'll also need to check for outroEndFromTime < trackEndPosition IIUC.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lastSoundSecond has been clamped to be <= trackEndPosition
I will add a note.

src/library/autodj/autodjprocessor.cpp
Comment on lines +1152 to +1153
// No outro start and outro end set, use Last Sound.
return lastSoundSecond;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wasn't this dependent on transition mode?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we calculate a reliable OutroEnd position, as an input to the transition mode dependent code.
The change here takes place in case we have an OutroStart. Than we try to use the transition time to calulate a better OutroEnd than just the lastSoundSecond as the last resort.

@daschuer
Copy link
Member Author

daschuer commented Oct 5, 2023

Done

@ronso0
Copy link
Member

ronso0 commented Nov 13, 2023

LGTM
@Swiftb0y any more comments?

@ikr83 @Bacadam Can you confirm AutoDJ behaves as expected for your use cases? You shared your feedback in #11648, though I'd like to make sure you tested this PR.
Instructions how to test the PR builds (as well as how to backup! your data) are here https://github.com/mixxxdj/mixxx/wiki/Testing

@Swiftb0y Swiftb0y dismissed their stale review November 13, 2023 22:00

sorry, no time&energy.

ronso0
ronso0 requested changes Dec 7, 2023
View reviewed changes
Copy link
Member

@ronso0 ronso0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes LGTM, just a question and comment fixes.

Didn't do a manual test yet.
Some other parts of the processor are still a mystery to me.

src/library/autodj/autodjprocessor.cpp Outdated Show resolved Hide resolved
src/library/autodj/autodjprocessor.cpp Outdated Show resolved Hide resolved
src/library/autodj/autodjprocessor.cpp Outdated
double introEndSecond = framePositionToSeconds(introEndPosition, pDeck);
if (m_transitionTime >= 0 && firstSoundSecond < introEndSecond) {
double introStartFromTime = introEndSecond - m_transitionTime;
if (introStartFromTime > firstSoundSecond) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would exclude introStart in the preroll if m_transitionTime > introEndSecond.
Is this intended?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think originally it was intended. But now I see no strong reason for it. So let's allow it.

@daschuer
Copy link
Member Author

Done

@ronso0
Copy link
Member

ronso0 commented Dec 19, 2023

Thank you!
I still didn't do manual tests so I rely on your tests and the previuos feedback from @ikr83 and @Bacadam

@ronso0 ronso0 merged commit 61b529f into mixxxdj:2.4 Dec 19, 2023
13 checks passed
@daschuer daschuer mentioned this pull request Oct 9, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ronso0 ronso0 ronso0 approved these changes

@Swiftb0y Swiftb0y Swiftb0y left review comments

Assignees
No one assigned
Labels
autodj changelog This PR should be included in the changelog library
Projects
None yet
Milestone
2.4.0
Development

Successfully merging this pull request may close these issues.
3 participants
@daschuer @ronso0 @Swiftb0y