Merge pull request #4169 from ywwg/synclock-disablesyncnoseek

Sync Lock: Don't seek phase when disabling sync

src/engine/controls/bpmcontrol.h

@@ -176,4 +176,5 @@ class BpmControl : public EngineControl {
 
     FRIEND_TEST(EngineSyncTest, UserTweakPreservedInSeek);
     FRIEND_TEST(EngineSyncTest, FollowerUserTweakPreservedInLeaderChange);
+    FRIEND_TEST(EngineSyncTest, FollowerUserTweakPreservedInSyncDisable);
 };

src/engine/enginebuffer.cpp

@@ -1229,6 +1229,7 @@ void EngineBuffer::processSyncRequests() {
 }
 
 void EngineBuffer::processSeek(bool paused) {
+    m_previousBufferSeek = false;
     // Check if we are cloning another channel before doing any seeking.
     EngineChannel* pChannel = m_pChannelToCloneFrom.fetchAndStoreRelaxed(nullptr);
     if (pChannel) {
@@ -1298,6 +1299,7 @@ void EngineBuffer::processSeek(bool paused) {
             kLogger.trace() << "EngineBuffer::processSeek" << getGroup() << "Seek to" << position;
         }
         setNewPlaypos(position);
+        m_previousBufferSeek = true;
     }
     // Reset the m_queuedSeek value after it has been processed in
     // setNewPlaypos() so that the Engine Controls have always access to the

src/engine/enginebuffer.h

@@ -228,7 +228,11 @@ class EngineBuffer : public EngineObject {
 
     void processSyncRequests();
     void processSeek(bool paused);
-
+    // For debugging / testing -- returns true if the previous buffer call resulted in a seek.
+    FRIEND_TEST(EngineSyncTest, FollowerUserTweakPreservedInSyncDisable);
+    bool previousBufferSeek() const {
+        return m_previousBufferSeek;
+    }
     bool updateIndicatorsAndModifyPlay(bool newPlay, bool oldPlay);
     void verifyPlay();
     void notifyTrackLoaded(TrackPointer pNewTrack, TrackPointer pOldTrack);
@@ -389,6 +393,7 @@ class EngineBuffer : public EngineObject {
     QAtomicInt m_iEnableSyncQueued;
     QAtomicInt m_iSyncModeQueued;
     ControlValueAtomic<QueuedSeek> m_queuedSeek;
+    bool m_previousBufferSeek = false;
 
     /// Indicates that no seek is queued
     static constexpr QueuedSeek kNoQueuedSeek = {mixxx::audio::kInvalidFramePos, SEEK_NONE};

src/engine/sync/enginesync.cpp

@@ -85,7 +85,7 @@ void EngineSync::requestSyncMode(Syncable* pSyncable, SyncMode mode) {
     // Second, figure out what Syncable should be used to initialize the leader
     // parameters, if any. Usually this is the new leader. (Note, that pointer might be null!)
     Syncable* pParamsSyncable = m_pLeaderSyncable;
-    // But If we are newly soft leader, we need to match to some other deck.
+    // But if we are newly soft leader, we need to match to some other deck.
     if (pSyncable == m_pLeaderSyncable && pSyncable != oldLeader &&
             mode != SyncMode::LeaderExplicit) {
         pParamsSyncable = findBpmMatchTarget(pSyncable);
@@ -104,7 +104,7 @@ void EngineSync::requestSyncMode(Syncable* pSyncable, SyncMode mode) {
         }
         reinitLeaderParams(pParamsSyncable);
         pSyncable->updateInstantaneousBpm(pParamsSyncable->getBpm());
-        if (pParamsSyncable != pSyncable) {
+        if (pParamsSyncable != pSyncable && mode != SyncMode::None) {
             pSyncable->requestSync();
         }
     }

src/test/enginesynctest.cpp

@@ -2409,6 +2409,40 @@ TEST_F(EngineSyncTest, FollowerUserTweakPreservedInLeaderChange) {
     }
 }
 
+TEST_F(EngineSyncTest, FollowerUserTweakPreservedInSyncDisable) {
+    // Ensure that when a follower disables sync, a phase seek is not performed.
+    // This is about 128 bpm, but results in nice round numbers of samples.
+    const double kDivisibleBpm = 44100.0 / 344.0;
+    mixxx::BeatsPointer pBeats1 = BeatFactory::makeBeatGrid(
+            m_pTrack1->getSampleRate(), mixxx::Bpm(kDivisibleBpm), mixxx::audio::kStartFramePos);
+    m_pTrack1->trySetBeats(pBeats1);
+    mixxx::BeatsPointer pBeats2 = BeatFactory::makeBeatGrid(
+            m_pTrack2->getSampleRate(), mixxx::Bpm(130), mixxx::audio::kStartFramePos);
+    m_pTrack2->trySetBeats(pBeats2);
+
+    ControlObject::getControl(ConfigKey(m_sGroup1, "sync_leader"))->set(1);
+    ControlObject::getControl(ConfigKey(m_sGroup2, "sync_enabled"))->set(1);
+    ControlObject::set(ConfigKey(m_sGroup1, "quantize"), 1.0);
+    ControlObject::set(ConfigKey(m_sGroup2, "quantize"), 1.0);
+    ControlObject::set(ConfigKey(m_sGroup1, "play"), 1.0);
+    ControlObject::set(ConfigKey(m_sGroup2, "play"), 1.0);
+    ProcessBuffer();
+
+    // Apply user tweak offset to follower
+    m_pChannel2->getEngineBuffer()
+            ->m_pBpmControl->m_dUserOffset.setValue(0.3);
+
+    EXPECT_TRUE(isExplicitLeader(m_sGroup1));
+    EXPECT_TRUE(isFollower(m_sGroup2));
+
+    ProcessBuffer();
+
+    // Disable sync, no seek should occur
+    ControlObject::getControl(ConfigKey(m_sGroup2, "sync_enabled"))->set(0);
+    ProcessBuffer();
+    EXPECT_FALSE(m_pChannel2->getEngineBuffer()->previousBufferSeek());
+}
+
 TEST_F(EngineSyncTest, LeaderUserTweakPreservedInLeaderChange) {
     // Ensure that when the leader deck changes, the user offset is accounted for when
     // reinitializing the leader parameters..