AutoDJProcessorTest.FadeToDeck2_Long_Transition fix by daschuer · Pull Request #34 · Be-ing/mixxx

daschuer · 2019-10-27T22:43:43Z

No description provided.

…-slot

…on time

…also a stalled autoDJ in caseof transition time exceedes track duration.

Be-ing · 2019-10-27T23:13:53Z

    } else {
        pFromDeck->fadeBeginPos = endPoint;
-        pFromDeck->fadeEndPos = endPoint - m_transitionTime;
+        pFromDeck->fadeEndPos = endPoint;


This breaks fading with a negative transition time. AutoDJ stops in this case.

This was causes elsewhere, but fixed now.

Be-ing · 2019-10-28T12:21:08Z

When the transition time is 0, tracks are loaded at the end. AutoDJ keeps playing by seeking back to the start when it is time to transition, but this is really confusing.

Be-ing · 2019-10-28T12:23:05Z

        if (m_eState == ADJ_IDLE) {
            if (thisDeckPlaying || thisPlayPosition >= 1.0) {
+                // cache this before calculating the new transition in otherDeck.play();
+                bool hasFadeTransition = thisDeck.fadeBeginPos < thisDeck.fadeEndPos;


When could this be false? It is not obvious. At first glance it seems like this should be a debug assert.

This is always false, if you use a negative or zero transition time.
I will add a comment.

daschuer · 2019-10-28T22:43:08Z

When the transition time is 0, tracks are loaded at the end. AutoDJ keeps playing by seeking back to the start when it is time to transition, but this is really confusing.

Does this seek to the end happens automatically? I cannot confirm that. I have only found and fix an issue using n invalid cue point of -1.

Or do you mean manual seeking to the end? The track should not seek or unload to allow the user to adjust the outro, right? Do you now have a different opinion?

Be-ing · 2019-10-29T03:10:55Z

Does this seek to the end happens automatically?

Yes, the second track and every track after that loaded in Full Track mode with a transition transition time of 0 loads at the end of the track.

daschuer · 2019-10-29T07:21:19Z

I cannot reproduce this and I have no idea where the seek is coming from.
We have two concurrent seeks when loading the track. First seek to cue, second seek to auto DJ calculated position. The cue load should not seek to end though.
Which load mode do you use without Auto DJ?
Does it also happen with other modes than full track mode?
Does it still happen after my last changes here?

daschuer · 2019-10-29T07:28:32Z

Could it be the deck clone feature?
Did you use always the same track?

It looks like the new code tricks the detection.

Be-ing · 2019-10-29T23:11:56Z

This seeking to the end bug is new with this PR and 100% reproducible for me. I am not sure what commit introduced the bug because AutoDJ has not worked at all with a transition time of 0 since before any of your recent PRs were merged. ee9cfcc is the most recent commit before this PR where Full Track mode works with a transition time of 0. That commit does not have the loading at end of track bug.

Does it also happen with other modes than full track mode?

No. Maybe it would happen with Skip Silence mode if the outro end was at the very end of the track but I have not tested.

Does it still happen after my last changes here?

Yes.

daschuer · 2019-10-30T07:46:11Z

I have a bug in rare cases where the seek to 0:0 does not happen. Instead it is seeked to the main cue as set in the deck preferences.

I guess here the seek is omitted:
https://github.com/daschuer/mixxx/blob/70214dfbd27a0915eb51406d49f827f801586659/src/library/autodj/autodjprocessor.cpp#L1305

Maybe in you case there is a stray 1 stored as seek point and never overridden due to an early return in calculateTransition()

Can you verify this?

I have tried to remove the is playing dependency, but this has messed up the tests.

Be-ing · 2019-10-30T11:17:40Z

I tested your latest changes and the bug is still there. I will try debugging tonight.

I have a bug in rare cases where the seek to 0:0 does not happen. Instead it is seeked to the main cue as set in the deck preferences.

The main cue is not at the end of the track, so I don't think this bug is related.

daschuer · 2019-10-30T12:11:56Z

Being at the main cue maybe only a side effect.
The issue is if to or from deck is nullptr, the we load position is not witten.
For some reason it could be 1 in you case an -1 in my case.

Maybe we should force it to -1 in case of early exit.

Be-ing · 2019-10-31T06:07:25Z

I will try debugging tonight.

Actually I won't have time until this weekend.

…ixed deck index orientation asumption

… a new one.

daschuer · 2019-10-31T12:08:11Z

My issue is gone now. Yours as well?

daschuer · 2019-11-03T22:12:23Z

The random seek issue still happens sometimes. I am currently digging into it.

daschuer · 2019-11-03T23:19:53Z

Fixed now. @Be-ing: Is your issue fixed as well?

Be-ing · 2019-11-03T23:45:40Z

No, the seeking to the end bug is still there. I am puzzled why you cannot reproduce it. I can reproduce it 100% of the time by choosing Full Track mode and setting the transition time to 0. The bug appears as soon as the first transition is made after enabling AutoDJ.

daschuer · 2019-11-04T01:03:20Z

Strange, I have added some debug. Can you create a mixxx.log file. I am curious which path in the code seeks to 1.

Be-ing · 2019-11-04T02:02:13Z

I think the bug is actually in WOverview, not AutoDJProcessor. I just noticed that when the bug occurs, the scrolling waveform is in the correct position, it is only the overview that is in the wrong position. I suspect this was caused by merging master after mixxxdj#2238 was merged to master.

Be-ing · 2019-11-04T02:04:02Z

This does seem to be somehow related to the track load preference. When I have the preference set to load at the beginning of the track, the bug occurs with every track. When I set it to load at intro start, the bug only occurs when the intro start is at the very beginning of the track.

Be-ing · 2019-11-04T02:38:09Z

Yes, the problem is in WOverview. Adding a call to onConnectedControlChanged in slotWaveformSummaryUpdated hacks around the bug. However, I am not sure why onConnectedControlChanged is not getting called on track load. Do you want to use that quick hack or investigate further why WOverview::onConnectedControlChanged is not getting called?

daschuer · 2019-11-04T23:01:03Z

After merging master, I can reproduce the issue.

Be-ing · 2019-11-05T00:20:10Z

Okay, so how should we solve it?

daschuer · 2019-11-07T00:21:56Z

@Be-ing: Now it should be solved. Please test again.

Be-ing · 2019-11-07T00:47:43Z

It's fixed!

Be-ing · 2019-11-07T01:04:26Z

    // We require ADJ_IDLE to prevent changing the thresholds in the middle of a
    // fade.
-    if (m_eState != ADJ_IDLE) {
+    VERIFY_OR_DEBUG_ASSERT(m_eState == ADJ_IDLE) {


I am confused why you changed this to a debug assertion. There is now code scattered throughout AutoDJProcessor repeating this check before calling calculateTransition. Why is it not sufficient to check this here at the top of calculateTransition? Aren't those other checks redundant?

I have moved the check outside to clearly mark all calling functions as "guard for manual user changes only".
I had a knot in my head when debugging because of all these slots.
This change helped me finally for a better understanding. I have kept the change, because it also saved some cpu cycles.

Be-ing · 2019-11-07T01:05:33Z

What was the root cause of the bug? I don't understand how the changes you made fixed it.

daschuer · 2019-11-07T07:15:22Z

It was actually a combination of two bugs. One was a recursive call of playerPositionChanged(). A stop() causes finally another playerPositionChanged() to 0 before the fist value was passed to the woverview. So first 1 was passed after the following 0.

The other issue was in engine buffer, that a seek of an old track to a position prevents a new track to seek to the same position.

The later was the original bug where the fix inside AutoDj leads to the first bug. I think I now have catched the route cause. I guess the behaviour has changed in master lately due to the engine track loading refactoring.

Be-ing · 2019-11-07T12:42:51Z

Okay, thank you for digging deep to solve those tricky bugs. I am glad we did not go with the quick hack in WOverview.

daschuer added 4 commits October 27, 2019 15:49

make emitLoadTrackToPlayer and emitAutoDJStateChanged a protected non…

c1010a6

…-slot

move no outro start fallback code into getOutroStartPosition()

cf658a9

consider current position of the from deck after chaning the transiti…

df29633

…on time

fix AutoDJProcessorTest.FadeToDeck2_Long_Transition test. This fixes …

2b1f525

…also a stalled autoDJ in caseof transition time exceedes track duration.

daschuer mentioned this pull request Oct 27, 2019

make better use of intro/outro markers with AutoDJ mixxxdj/mixxx#2103

Merged

Be-ing reviewed Oct 27, 2019

View reviewed changes

daschuer added 2 commits October 28, 2019 08:15

Cache hasFadeTransition before possible override

5e6988e

Adjust FadeToDeck2_Pause_Transition for no fading transitions

74fdcb8

Be-ing reviewed Oct 28, 2019

View reviewed changes

daschuer added 2 commits October 28, 2019 23:20

improve comments

277f5da

improve error handing in case of -1 cue positions.

96a5a45

improve crossfaderChanged()

873e313

daschuer added 2 commits October 30, 2019 07:41

Adjust short track workaround in outro start mode

d6e6cf4

Adjust intro and outro change slots

70214df

daschuer added 4 commits October 31, 2019 09:31

don't bother with playing state when calculating a new transition

e1c45e5

Don't use non cons references in playerPositionChanged() and remove f…

6dac3a6

…ixed deck index orientation asumption

Don't use non const references in toggleAutoDJ()

fd36927

Reset startPos, just in case calculateTransition() is not able to set…

2f88f54

… a new one.

Set the just loaded deck as toDeck

7036b74

daschuer added 2 commits November 4, 2019 01:37

respect the rate slider when calculating auto-DJ transitions

9951daa

Whatch rate changes

db49654

daschuer force-pushed the autodj_intro_outro branch from c5528d0 to db49654 Compare November 4, 2019 22:06

fix recursive positionChanged call. Fix omitting fist seek.

efa6c00

Be-ing reviewed Nov 7, 2019

View reviewed changes

Be-ing merged commit 7658b51 into Be-ing:autodj_intro_outro Nov 7, 2019

Conversation

daschuer commented Oct 27, 2019

Uh oh!

Be-ing Oct 27, 2019

Choose a reason for hiding this comment

Uh oh!

daschuer Oct 28, 2019

Choose a reason for hiding this comment

Uh oh!

Be-ing commented Oct 28, 2019

Uh oh!

Be-ing Oct 28, 2019

Choose a reason for hiding this comment

Uh oh!

daschuer Oct 28, 2019

Choose a reason for hiding this comment

Uh oh!

daschuer commented Oct 28, 2019

Uh oh!

Be-ing commented Oct 29, 2019

Uh oh!

daschuer commented Oct 29, 2019

Uh oh!

daschuer commented Oct 29, 2019

Uh oh!

Be-ing commented Oct 29, 2019

Uh oh!

daschuer commented Oct 30, 2019

Uh oh!

Be-ing commented Oct 30, 2019

Uh oh!

daschuer commented Oct 30, 2019

Uh oh!

Be-ing commented Oct 31, 2019

Uh oh!

daschuer commented Oct 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

daschuer commented Nov 3, 2019

Uh oh!

daschuer commented Nov 3, 2019

Uh oh!

Be-ing commented Nov 3, 2019

Uh oh!

daschuer commented Nov 4, 2019

Uh oh!

Be-ing commented Nov 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Be-ing commented Nov 4, 2019

Uh oh!

Be-ing commented Nov 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

daschuer commented Nov 4, 2019

Uh oh!

Be-ing commented Nov 5, 2019

Uh oh!

daschuer commented Nov 7, 2019

Uh oh!

Be-ing commented Nov 7, 2019

Uh oh!

Be-ing Nov 7, 2019

Choose a reason for hiding this comment

Uh oh!

daschuer Nov 7, 2019

Choose a reason for hiding this comment

Uh oh!

Be-ing commented Nov 7, 2019

Uh oh!

daschuer commented Nov 7, 2019

Uh oh!

Be-ing commented Nov 7, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

daschuer commented Oct 31, 2019 •

edited

Loading

Be-ing commented Nov 4, 2019 •

edited

Loading

Be-ing commented Nov 4, 2019 •

edited

Loading