Fix issues with half/double and Sync Lock #3899

ywwg · 2021-05-23T18:33:29Z

One part of Sync Lock that isn't well-described by the code is the idea of which Syncable is responsible for initializing the Sync Leader values. This is not the same as the leader! For instance, when you push and hold sync, that deck becomes the leader but it initializes based on the values of some other deck. This concept was causing problems with half/double bpm calculation. The code is a little more explicit now, and doesn't just assume that the Sync Leader should be the source of sync params.

Also fixed some super edge cases

ywwg · 2021-05-23T18:33:49Z

src/engine/sync/synccontrol.cpp

@@ -289,7 +290,8 @@ void SyncControl::updateTargetBeatDistance() {
        targetDistance *= kBpmDouble;
    } else if (m_masterBpmAdjustFactor == kBpmHalve) {
        targetDistance *= kBpmHalve;
-        if (m_pBeatDistance->get() >= 0.5) {
+        // Our beat distance CO is still a buffer behind, so take the current value.
+        if (m_pBpmControl->getBeatDistance(getSampleOfTrack().current) >= 0.5) {


very proud of this fix. It was hard to track down.

ywwg · 2021-05-23T18:34:22Z

p.s.: I really, really hate half-double syncing and wish I had never done it :P.

ywwg · 2021-05-28T19:30:33Z

The PR after this will rename master->leader! So let's get this in yes? :)

Holzhaus · 2021-05-28T19:46:43Z

Clazy checks failed

ywwg · 2021-05-28T22:54:16Z

Clazy checks failed

fixed

Holzhaus

Thanks, I noticed no issues when testing this, but I can't really comment on the code changes because I'm not familiar enough with it. Would be good if someone else would have a look.

Holzhaus · 2021-05-29T16:45:12Z

src/test/enginesynctest.cpp

@@ -1445,6 +1445,23 @@ TEST_F(EngineSyncTest, ZeroBPMRateAdjustIgnored) {
            ControlObject::getControl(ConfigKey(m_sGroup2, "rate"))->get());
 }

+TEST_F(EngineSyncTest, DISABLED_BeatDistanceBeforeStart) {


Are you planning to fix this in this PR?

no -- this is a much more complex issue and I'm not even sure what's going wrong. This is a pretty rare circumstance, and shouldn't be a release-blocker, let alone a PR-blocker.

https://bugs.launchpad.net/mixxx/+bug/1930143

daschuer

I have a notable momentary pitch change in case of a implicit leader deck playing audible and a inaudible follower disables sync.

Play track A
Enable Sync A
Move x-fader to A
Play track B
Enable Sync B
A is implicit Leader
Nudge Track A
Disable Sync B
Hear a tempo dip in Track A -> wrong!

This can be noticed the best if Track A is a 440 Hz Sine wave analyzed with a const beat grid.

src/engine/sync/enginesync.cpp

daschuer · 2021-05-30T19:32:03Z

src/engine/sync/enginesync.cpp

            continue;
        }
        pSyncable->setMasterParams(beatDistance, baseBpm, bpm);
    }
 }

-bool EngineSync::noPlayingFollowers() const {
+Syncable* EngineSync::getOnlyPlayingSyncable() const {


I think the function name can be improved.
Only playing can also mean that it is playing without being synced or without being audible.
Something like:
getLonlyPlayingSyncronizedSyncable();
getLonlyPlayingSyncable();

do you mean "lonely" ? :) I tried "getUniquePlayingSyncedDeck"

Co-authored-by: Daniel Schürmann <daschuer@mixxx.org>

…st its nudge value

ywwg · 2021-06-01T03:33:04Z

I have a notable momentary pitch change in case of a implicit leader deck playing audible and a inaudible follower disables sync.

fixed.

daschuer · 2021-06-03T07:22:54Z

I can confirm the pitch dip issue is fixed with a nudged leader deck.

Somehow the leader buttons are now broken. Is this a visual skin issue? The plain main branch is also affected.

When I load a new track to a stopped explicit master deck the follower becomes high pitched.
I don't think that this is a valid use case.

daschuer · 2021-06-03T07:27:13Z

src/engine/sync/enginesync.cpp

+        if (pOnlyPlayer) {
+            // Even if we didn't change master, if there is only one player (us), then we should
+            // reinit the beat distance.
+            pOnlyPlayer->notifyOnlyPlayingSyncable();


can you rename this to something like:
notifyUniquePlaying() or such?

ywwg · 2021-06-03T18:19:14Z

When I load a new track to a stopped explicit master deck the follower becomes high pitched.
I don't think that this is a valid use case.

this is one of those edge cases. It's an explicit leader, so it's supposed to be in charge of the rate. Should we instead say that loading a new track causes it to relinquish explicit leader state? What's the right behavior?

ronso0 · 2021-06-03T18:37:19Z

Somehow the leader buttons are now broken. Is this a visual skin issue? The plain main branch is also affected.

fixed with 52786c9
text outlines stack order is not respected Qt SVG, thus the same path with an outline needs be behind the text :
we need to polish and add the Leader control to all skins anyway.

ywwg · 2021-06-03T19:05:49Z

Yes the leader button was always meant to be temp. Is it ok if I leave it for now?

ywwg · 2021-06-04T03:52:01Z

I made the decks a few pixels taller to make room for the button. Is that ok for now?

ywwg · 2021-06-04T03:53:03Z

Would it be ok if I fix the track load change in the next PR? That's a separate bug from this one and this PR has been hanging around a while. I have a good track record of fixing these things :)

Co-authored-by: Daniel Schürmann <daschuer@mixxx.org>

…st its nudge value

ywwg · 2021-06-04T05:13:08Z

sync the leader from a follower during load

This conflcts with our conclusion in https://bugs.launchpad.net/mixxx/+bug/1808698. There, we decided that "the second deck is still playing at unity -- it was the leader and it should not change speed just because it reloaded and the bpm changed." The test we wrote has the other decks all changing speed to match the new value.

ywwg · 2021-06-04T14:44:28Z

I suppose the difference here is between when the beats are updated due to a track load (should resync to the playing bpm) vs beats are updated due to rescan (should keep the slider where it is).

ywwg · 2021-06-04T19:51:06Z

OK I have a new PR that fixes the track load behavior. It is very far downstream of this PR so I'd really like to get this in so we can move ahead

…xplicit

ywwg · 2021-06-14T18:10:31Z

friendly ping -- I'd really like to start getting these in

daschuer · 2021-06-15T07:59:03Z

I have noticed an inconsistency:
1:

Play Deck A with Sync enabled (Implicit Leader)
Nudge deck A
Sync Deck B
Play Deck B
-> Beat markers are in sync

2:

Play Deck A with Sync enabled (Implicit Leader)
Sync Deck B (paused)
Nudge deck A
Play Deck B
-> Beat markers are out of sync

I can imagine arguments for both behaviors, but both should behave the same.
For now, I would prefer the first. Nudging is used to make two decks sound good, when playing together.
When playing allown this feature is pointless.

daschuer · 2021-06-15T08:21:00Z

The behavior of nudging is IMHO correct, when playing with three decks.
I can nudge the leader of two decks. When I now add play a third synced deck it is in sync with the other follower, which is perfect.

After rethinking the whole, my conclusion is: The current behavior is OK.

daschuer · 2021-06-15T08:25:54Z

LGTM, Thank you.

ywwg · 2021-06-16T01:59:27Z

ok cool. There's still a lot to fix with Sync Lock so we'll keep working on it

github-actions bot added the code quality label May 23, 2021

ywwg commented May 23, 2021

View reviewed changes

ywwg requested review from Holzhaus and daschuer May 23, 2021 18:34

Holzhaus reviewed May 29, 2021

View reviewed changes

daschuer requested changes May 30, 2021

View reviewed changes

ywwg added 4 commits May 31, 2021 23:11

Fix half-double initialization issues

507bb03

Sync Lock: Fix more half/double issues and clean up for PR

0718058

Sync Lock: Fix clazy issue

c521601

sync lock: add bug link for disabled / broken test

15cd3d4

ywwg force-pushed the stopped-explicit branch from adbf37e to 85bbc63 Compare June 1, 2021 03:12

Sync Lock: don't use c++17 inline conditional assignment (1)

35ed9c9

Co-authored-by: Daniel Schürmann <daschuer@mixxx.org>

ywwg force-pushed the stopped-explicit branch from 85bbc63 to 35ed9c9 Compare June 1, 2021 03:13

Sync Lock: Fix issue where single playing sync deck was syncing again…

ec01df8

…st its nudge value

This was referenced Jun 1, 2021

SyncLock: Don't recalc half/double multiplier on every callback #3706

Merged

Rename Sync instances of "master" to "leader" #3921

Closed

daschuer reviewed Jun 3, 2021

View reviewed changes

Sync Lock: better name for function

b041a6e

ywwg added 2 commits June 3, 2021 15:05

SyncLock: Increase deck size to make room for Leader button

d8c843a

Merge branch 'main' into stopped-explicit

8984df9

github-actions bot added the skins label Jun 3, 2021

ywwg and others added 11 commits June 3, 2021 23:54

Fix half-double initialization issues

534eacf

Sync Lock: Fix more half/double issues and clean up for PR

bd560c0

Sync Lock: Fix clazy issue

4e9f8f4

sync lock: add bug link for disabled / broken test

4ccdebe

Sync Lock: don't use c++17 inline conditional assignment (1)

259cfa5

Co-authored-by: Daniel Schürmann <daschuer@mixxx.org>

Sync Lock: Fix issue where single playing sync deck was syncing again…

d450bca

…st its nudge value

Sync Lock: better name for function

b0c51f0

SyncLock: Increase deck size to make room for Leader button

372d9e4

SyncLock: Fix leader button in latenight classic

08cead7

SyncLock: Skin fixes

7efe916

Sync Lock: Fix Leader button colors

fc5813f

ywwg mentioned this pull request Jun 4, 2021

Sync Lock: Differentiate track loading from beats updating #3944

Merged

ywwg requested review from daschuer and Holzhaus June 4, 2021 19:51

ywwg added 4 commits June 5, 2021 09:31

Merge branch 'main' into stopped-explicit

257b943

Merge remote-tracking branch 'origin/stopped-explicit' into stopped-e…

daba011

…xplicit

Merge branch 'main' into stopped-explicit

7b56e59

Merge branch 'main' into stopped-explicit

3e331c9

daschuer approved these changes Jun 15, 2021

View reviewed changes

daschuer merged commit d8a9fd3 into mixxxdj:main Jun 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issues with half/double and Sync Lock #3899

Fix issues with half/double and Sync Lock #3899

ywwg commented May 23, 2021

ywwg May 23, 2021

ywwg commented May 23, 2021

ywwg commented May 28, 2021

Holzhaus commented May 28, 2021

ywwg commented May 28, 2021

Holzhaus left a comment

Holzhaus May 29, 2021

ywwg May 29, 2021

ywwg May 29, 2021

daschuer left a comment

daschuer May 30, 2021

ywwg Jun 1, 2021

ywwg commented Jun 1, 2021

daschuer commented Jun 3, 2021

daschuer Jun 3, 2021

ywwg Jun 3, 2021

ywwg commented Jun 3, 2021

ronso0 commented Jun 3, 2021 •

edited

Loading

ywwg commented Jun 3, 2021

ywwg commented Jun 4, 2021

ywwg commented Jun 4, 2021

ywwg commented Jun 4, 2021

ywwg commented Jun 4, 2021

ywwg commented Jun 4, 2021

ywwg commented Jun 14, 2021

daschuer commented Jun 15, 2021

daschuer commented Jun 15, 2021

daschuer commented Jun 15, 2021

ywwg commented Jun 16, 2021

Fix issues with half/double and Sync Lock #3899

Fix issues with half/double and Sync Lock #3899

Conversation

ywwg commented May 23, 2021

Choose a reason for hiding this comment

ywwg commented May 23, 2021

ywwg commented May 28, 2021

Holzhaus commented May 28, 2021

ywwg commented May 28, 2021

Holzhaus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ywwg commented Jun 1, 2021

daschuer commented Jun 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ywwg commented Jun 3, 2021

ronso0 commented Jun 3, 2021 • edited Loading

ywwg commented Jun 3, 2021

ywwg commented Jun 4, 2021

ywwg commented Jun 4, 2021

ywwg commented Jun 4, 2021

ywwg commented Jun 4, 2021

ywwg commented Jun 4, 2021

ywwg commented Jun 14, 2021

daschuer commented Jun 15, 2021

daschuer commented Jun 15, 2021

daschuer commented Jun 15, 2021

ywwg commented Jun 16, 2021

ronso0 commented Jun 3, 2021 •

edited

Loading