Sync Lock: Differentiate track loading from beats updating #3944

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

When we load a track, we want the deck to sync to other decks.
When we update beats of a Leader deck, we want it to remain at the same playback speed.

ywwg · 2021-06-16T03:11:22Z

fix this commit history as well

coveralls · 2021-07-19T22:34:47Z

Pull Request Test Coverage Report for Build 1156225558

25 of 39 (64.1%) changed or added relevant lines in 4 files are covered.
3 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.02%) to 25.985%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/engine/sync/enginesync.cpp	2	4	50.0%
src/engine/sync/synccontrol.cpp	15	19	78.95%
src/engine/controls/bpmcontrol.cpp	2	10	20.0%

Files with Coverage Reduction	New Missed Lines	%
src/engine/controls/bpmcontrol.cpp	1	65.02%
src/engine/cachingreader/cachingreaderworker.cpp	2	63.48%

Totals
Change from base Build 1154301022:	0.02%
Covered Lines:	20023
Relevant Lines:	77056

💛 - Coveralls

ywwg · 2021-07-23T00:52:45Z

ugh this commit history is now borked, fixing...

ywwg · 2021-07-23T00:59:17Z

ok I manually squashed this down to one simple commit.

Holzhaus · 2021-07-23T11:09:43Z

ok I manually squashed this down to one simple commit.

For such PRs I think it would be good to add the PR description to the commit message as well. No need to amend the commit, but I think it would be good practice in the future ;-)

Holzhaus

Thanks. Didn't test this yet but code look good.

src/engine/sync/synccontrol.cpp

src/test/enginesynctest.cpp

Holzhaus · 2021-07-23T14:01:43Z

src/test/enginesynctest.cpp

    auto pButtonLeaderSync1 =
            std::make_unique<ControlProxy>(m_sGroup1, "sync_mode");
    pButtonLeaderSync1->set(static_cast<double>(SyncMode::LeaderExplicit));
    ProcessBuffer();
    EXPECT_TRUE(isExplicitLeader(m_sGroup1));
-    EXPECT_DOUBLE_EQ(160.0, ControlObject::get(ConfigKey(m_sGroup1, "bpm")));
+    EXPECT_DOUBLE_EQ(120.0, ControlObject::get(ConfigKey(m_sGroup1, "bpm")));


These test value changes are just for easier calculation by hand, right?

Because the behavior of mixxx changed, the test now acts differently than it did before. So, I had to update some of the values. (Where appropriate, I also updated comments)

Holzhaus · 2021-07-23T14:02:25Z

src/engine/sync/enginesync.cpp

@@ -533,7 +531,7 @@ void EngineSync::onCallbackEnd(int sampleRate, int bufferSize) {
    m_pInternalClock->onCallbackEnd(sampleRate, bufferSize);
 }

-EngineChannel* EngineSync::getLeader() const {
+EngineChannel* EngineSync::getLeaderChannel() const {


Looks unrelated, but but probably makes sense.

yeah it helped clarify -- "getLeader" would seemingly return a Syncable*

Use nullptr instead of NULL (reviewer suggestion) Co-authored-by: Jan Holthuis <holthuis.jan@googlemail.com>

constify bool (reviewer suggestion) Co-authored-by: Jan Holthuis <holthuis.jan@googlemail.com>

src/test/enginesynctest.cpp

Holzhaus · 2021-07-28T09:14:11Z

src/engine/enginemaster.cpp

@@ -281,7 +281,7 @@ void EngineMaster::processChannels(int iBufferSize) {
    m_activeChannels.clear();

    //ScopedTimer timer("EngineMaster::processChannels");
-    EngineChannel* pMasterChannel = m_pMasterSync->getLeader();
+    EngineChannel* pMasterChannel = m_pMasterSync->getLeaderChannel();


I'm a bit confused about the Master term here. I should rename it anyway (probably in a dedicated PR), but pMasterChannel does refer to the sync leader channel not the main output channel, right? This is a bit confusing.

yes, it should be LeaderChannel. I will file a PR to update the language

eh, just fixed it anyway

review note: NULL -> nullptr Co-authored-by: Jan Holthuis <holthuis.jan@googlemail.com>

…ynclock-05-explicitload

daschuer · 2021-08-01T20:39:25Z

Normal loading of tracks now work like expected.
However there is an issue with instant doubles.
If I dag the follower track to the leader deck, both decks receive a tempo change.
It also happens if I drag the follower track to the leader deck.
The rate shall not change IMHO in this case.

daschuer

I think this needs also some love for the instant double case.

ywwg · 2021-08-04T19:47:31Z

ah ok. Yeah I'll take a look at that

ywwg · 2021-08-06T00:58:48Z

I can't reproduce the issue with doubles you describe. Also in your description you twice describe dragging the follower track onto the leader deck as if they are different cases. Can you please describe this case in detail, like: which decks have sync on? Which decks have tracks? What are the track bpms? Where are the rate sliders? etc etc.

daschuer · 2021-08-06T07:50:29Z

I was not able to reproduce this issue with const bpm track but it works with a beat map track see:

simplescreenrecorder-2021-08-06_09.48.11.mp4

ywwg · 2021-08-22T17:21:47Z

Can you test with this branch? https://github.com/ywwg/mixxx/tree/synclock-05-explicitload-debug

ywwg · 2021-08-22T17:22:41Z

(the bug is that bpmcontrol contains an obsolete function called syncTempo which attempts to duplicate the synclock logic but does a very poor job of it. I have removed the function and replace the few calls to it with proper calls to synclock code)

ywwg · 2021-09-12T14:44:12Z

fixed conflict

daschuer · 2021-09-12T20:57:18Z

src/engine/controls/bpmcontrol.cpp

-    }
-
-    syncTempo();
+    m_pSyncEnabled->set(value != 0);


This is the momentary sync of tempo only when right clicking the sync button.
Mapping this to sync enabled will also sync the phase = seeks the track.
The original use case is syncing the tempo without seeking. This is now no longer possible.

moved these controls to the synccontrol and reimplemented using the correct code.

src/engine/controls/bpmcontrol.cpp

daschuer · 2021-09-12T21:21:57Z

Unfortunately the tempo change issue when doing instant double still exists:

simplescreenrecorder-2021-09-12_23.17.05_edit.mp4

ywwg · 2021-09-25T19:32:55Z

This is a bug with deck cloning, not Sync. And this only happens with beatmap tracks. I believe this is happening because the order of the clone events is wrong:

track is loaded
bpm is synced
track seek takes place

The sync operation is taking place before the cloned track has performed its seek -- and at the beginning of the track, the local bpm may be very different. So Mixxx is taking the actual un-seeked local bpm and applying the rate slider, getting a new bpm, and then setting that as the new bpm. So the bpm error will compound in the direction of the rate slider. I don't know a lot about deck cloning so I don't really know how to fix this. But what we should do is do the seek first and then initiate the rate matching.

I'd like to consider this issue "mostly fixed" except for this edge case. Let's file a bug for this new issue and get these other fixes in.

daschuer · 2021-09-27T06:29:11Z

src/engine/sync/synccontrol.cpp

+    m_pRateRatio->set(target->getBpm() * multiplier / localBpm);
+}
+
+void SyncControl::slotControlBeatSync(double value) {


This changes the behavior from a momentary control to a permanent switch.
I think we must not change it silently.

daschuer · 2021-10-07T17:59:28Z

I can confirm that the instant double issue is no regression from this PR. I have filed https://bugs.launchpad.net/mixxx/+bug/1946385

daschuer

LGTM, Thank you.

ywwg · 2021-10-08T02:55:35Z

thanks for your patience <3

ywwg marked this pull request as draft June 4, 2021 19:50

github-actions bot added code quality controller mappings skins labels Jun 4, 2021

ywwg force-pushed the synclock-05-explicitload branch from 35d217c to eb3e428 Compare June 16, 2021 03:11

daschuer mentioned this pull request Jun 20, 2021

Sync Lock: make sure to reinit leader params after scratching ends #4005

Merged

ywwg marked this pull request as ready for review July 22, 2021 22:34

Sync Lock: Differentiate track loading from beats updating

0d1eb3f

ywwg force-pushed the synclock-05-explicitload branch from a5b0969 to 0d1eb3f Compare July 23, 2021 00:57

ywwg requested a review from Holzhaus July 23, 2021 00:59

Holzhaus reviewed Jul 23, 2021

View reviewed changes

ywwg and others added 3 commits July 23, 2021 11:24

Update src/test/enginesynctest.cpp

0f15a65

Use nullptr instead of NULL (reviewer suggestion) Co-authored-by: Jan Holthuis <holthuis.jan@googlemail.com>

Update src/engine/sync/synccontrol.cpp

e506d79

constify bool (reviewer suggestion) Co-authored-by: Jan Holthuis <holthuis.jan@googlemail.com>

Merge branch 'main' into synclock-05-explicitload

2c7c706

Holzhaus reviewed Jul 28, 2021

View reviewed changes

ywwg and others added 4 commits July 28, 2021 08:48

Update src/test/enginesynctest.cpp

7ea1c8c

review note: NULL -> nullptr Co-authored-by: Jan Holthuis <holthuis.jan@googlemail.com>

Update src/test/enginesynctest.cpp

1410ff3

review note: NULL -> nullptr Co-authored-by: Jan Holthuis <holthuis.jan@googlemail.com>

Sync Lock: correct naming in enginemaster

f76defe

Merge remote-tracking branch 'origin/synclock-05-explicitload' into s…

b0868a1

…ynclock-05-explicitload

daschuer requested changes Aug 1, 2021

View reviewed changes

Merge branch 'main' into synclock-05-explicitload

00f7544

ywwg added 2 commits August 8, 2021 15:31

Merge branch 'main' into synclock-05-explicitload

d432fe9

Merge branch 'main' into synclock-05-explicitload

f4680af

ywwg added 2 commits August 22, 2021 13:39

Sync Lock: remove obsolete and broken BpmControler::syncTempo function

719bd08

Merge branch 'main' into synclock-05-explicitload

ffcfcbf

daschuer requested changes Sep 12, 2021

View reviewed changes

daschuer reviewed Sep 12, 2021

View reviewed changes

src/engine/controls/bpmcontrol.cpp Outdated Show resolved Hide resolved

daschuer reviewed Sep 12, 2021

View reviewed changes

src/engine/controls/bpmcontrol.cpp Outdated Show resolved Hide resolved

Remove processbuffer calls to keep positions at zero

b836148

ywwg added 3 commits September 25, 2021 13:38

Merge branch 'main' into synclock-05-explicitload

31b7463

Sync Lock: move obsolete beatsync_* controls to synccontrol

1a31030

Revert debug edit

90132fd

daschuer requested changes Sep 27, 2021

View reviewed changes

ywwg added 2 commits October 6, 2021 20:09

Sync Lock: make sure beatsync control is momentary

665c03d

Merge remote-tracking branch 'origin/main' into oops

2a66e45

ywwg force-pushed the synclock-05-explicitload branch from 028e849 to 2a66e45 Compare October 7, 2021 00:10

SyncLock: clang-format files

5d6dbc2

daschuer approved these changes Oct 7, 2021

View reviewed changes

daschuer merged commit bc3d041 into mixxxdj:main Oct 7, 2021

mixxxbot mentioned this pull request Aug 24, 2022

Changing tempo when using instant double and sync #10550

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sync Lock: Differentiate track loading from beats updating #3944

Sync Lock: Differentiate track loading from beats updating #3944

ywwg commented Jun 4, 2021 •

edited

Loading

ywwg commented Jun 16, 2021

coveralls commented Jul 19, 2021 •

edited

Loading

ywwg commented Jul 23, 2021

ywwg commented Jul 23, 2021

Holzhaus commented Jul 23, 2021

Holzhaus left a comment

Holzhaus Jul 23, 2021

ywwg Jul 23, 2021

Holzhaus Jul 23, 2021

ywwg Jul 26, 2021

Holzhaus Jul 28, 2021

ywwg Jul 28, 2021

ywwg Jul 28, 2021

daschuer commented Aug 1, 2021

daschuer left a comment

ywwg commented Aug 4, 2021

ywwg commented Aug 6, 2021

daschuer commented Aug 6, 2021

ywwg commented Aug 22, 2021

ywwg commented Aug 22, 2021

ywwg commented Sep 12, 2021

daschuer Sep 12, 2021

ywwg Sep 25, 2021

daschuer commented Sep 12, 2021

ywwg commented Sep 25, 2021

daschuer Sep 27, 2021

ywwg Oct 7, 2021

daschuer commented Oct 7, 2021

daschuer left a comment

ywwg commented Oct 8, 2021

Sync Lock: Differentiate track loading from beats updating #3944

Sync Lock: Differentiate track loading from beats updating #3944

Conversation

ywwg commented Jun 4, 2021 • edited Loading

ywwg commented Jun 16, 2021

coveralls commented Jul 19, 2021 • edited Loading

Pull Request Test Coverage Report for Build 1156225558

💛 - Coveralls

ywwg commented Jul 23, 2021

ywwg commented Jul 23, 2021

Holzhaus commented Jul 23, 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

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 commented Aug 1, 2021

daschuer left a comment

Choose a reason for hiding this comment

ywwg commented Aug 4, 2021

ywwg commented Aug 6, 2021

daschuer commented Aug 6, 2021

ywwg commented Aug 22, 2021

ywwg commented Aug 22, 2021

ywwg commented Sep 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer commented Sep 12, 2021

ywwg commented Sep 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer commented Oct 7, 2021

daschuer left a comment

Choose a reason for hiding this comment

ywwg commented Oct 8, 2021

ywwg commented Jun 4, 2021 •

edited

Loading

coveralls commented Jul 19, 2021 •

edited

Loading