Sync Lock: Differentiate track loading from beats updating

src/engine/controls/bpmcontrol.cpp

@@ -302,9 +302,6 @@ void BpmControl::slotControlBeatSync(double value) {
 }
 
 bool BpmControl::syncTempo() {
-    if (getSyncMode() == SyncMode::LeaderExplicit) {
-        return false;
-    }
     EngineBuffer* pOtherEngineBuffer = pickSyncTarget();
 
     if (!pOtherEngineBuffer) {

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();
     // Reserve the first place for the master channel which
     // should be processed first
     m_activeChannels.append(NULL);

src/engine/sync/enginesync.cpp

@@ -85,9 +85,8 @@ 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.
-    if (pSyncable == m_pLeaderSyncable && pSyncable != oldLeader &&
-            mode != SyncMode::LeaderExplicit) {
+    // But if we are newly leader, we need to match to some other deck.
+    if (pSyncable == m_pLeaderSyncable && pSyncable != oldLeader) {
         pParamsSyncable = findBpmMatchTarget(pSyncable);
         if (!pParamsSyncable) {
             // We weren't able to find anything to match to, so set ourselves as the
@@ -108,7 +107,6 @@ void EngineSync::requestSyncMode(Syncable* pSyncable, SyncMode mode) {
             pSyncable->requestSync();
         }
     }
-
 }
 
 void EngineSync::activateFollower(Syncable* pSyncable) {
@@ -210,7 +208,7 @@ Syncable* EngineSync::pickLeader(Syncable* enabling_syncable) {
         return m_pLeaderSyncable;
     }
 
-    // First preference: some other sync deck that is not playing.
+    // First preference: some other sync deck that is playing.
     // Note, if we are using PREFER_LOCK_BPM we don't use this option.
     Syncable* first_other_playing_deck = nullptr;
     // Second preference: whatever the first playing sync deck is, even if it's us.
@@ -533,7 +531,7 @@ void EngineSync::onCallbackEnd(int sampleRate, int bufferSize) {
     m_pInternalClock->onCallbackEnd(sampleRate, bufferSize);
 }
 
-EngineChannel* EngineSync::getLeader() const {
+EngineChannel* EngineSync::getLeaderChannel() const {
     return m_pLeaderSyncable ? m_pLeaderSyncable->getChannel() : nullptr;
 }
 

src/engine/sync/enginesync.h

@@ -56,7 +56,7 @@ class EngineSync : public SyncableListener {
     bool otherSyncedPlaying(const QString& group);
 
     void addSyncableDeck(Syncable* pSyncable);
-    EngineChannel* getLeader() const;
+    EngineChannel* getLeaderChannel() const;
     void onCallbackStart(int sampleRate, int bufferSize);
     void onCallbackEnd(int sampleRate, int bufferSize);
 

src/engine/sync/synccontrol.cpp

@@ -332,21 +332,49 @@ void SyncControl::trackLoaded(TrackPointer pNewTrack) {
     if (pNewTrack) {
         pBeats = pNewTrack->getBeats();
     }
-    trackBeatsUpdated(pBeats);
-}
-
-void SyncControl::trackBeatsUpdated(mixxx::BeatsPointer pBeats) {
     // This slot is fired by a new file is loaded or if the user
     // has adjusted the beatgrid.
     if (kLogger.traceEnabled()) {
-        kLogger.trace() << getGroup() << "SyncControl::trackBeatsUpdated";
+        kLogger.trace() << getGroup() << "SyncControl::trackLoaded";
     }
 
     VERIFY_OR_DEBUG_ASSERT(m_pLocalBpm) {
         // object not initialized
         return;
     }
 
+    bool hadBeats = m_pBeats != nullptr;
+    m_pBeats = pBeats;
+    m_leaderBpmAdjustFactor = kBpmUnity;
+
+    m_pBpmControl->updateLocalBpm();
+    if (isSynchronized()) {
+        if (!m_pBeats) {
+            // If we were soft leader and now we have no beats, go to follower.
+            // This is a bit of "enginesync" logic that has bled into this Syncable,
+            // is there a better way to handle "soft leaders no longer have bpm"?
+            if (getSyncMode() == SyncMode::LeaderSoft) {
+                m_pChannel->getEngineBuffer()->requestSyncMode(SyncMode::Follower);
+            }
+            return;
+        }
+
+        m_pBpmControl->syncTempo();
+        if (!hadBeats) {
+            // There is a chance we were beatless leader before, so we notify a basebpm change
+            // to possibly reinit leader params.
+            m_pChannel->getEngineBuffer()->requestSyncMode(getSyncMode());
+            m_pEngineSync->notifyBaseBpmChanged(this, getBaseBpm());
+        }
+    }
+}
+
+void SyncControl::trackBeatsUpdated(mixxx::BeatsPointer pBeats) {
+    // This slot is fired by if the user has adjusted the beatgrid.
+    if (kLogger.traceEnabled()) {
+        kLogger.trace() << getGroup() << "SyncControl::trackBeatsUpdated";
+    }
+
     m_pBeats = pBeats;
     m_leaderBpmAdjustFactor = kBpmUnity;
 
@@ -357,7 +385,7 @@ void SyncControl::trackBeatsUpdated(mixxx::BeatsPointer pBeats) {
             // be leader.
             m_pChannel->getEngineBuffer()->requestSyncMode(SyncMode::Follower);
         } else {
-            // We are remaining leader, so notify the engine with our update.
+            // We should not change playback speed.
             m_pBpmControl->updateLocalBpm();
             m_pEngineSync->notifyBaseBpmChanged(this, getBaseBpm());
         }

src/test/enginesynctest.cpp

@@ -89,7 +89,7 @@ class EngineSyncTest : public MockedEngineBackendTest {
     }
 
     void assertNoLeader() {
-        EXPECT_EQ(NULL, m_pEngineSync->getLeader());
+        EXPECT_EQ(NULL, m_pEngineSync->getLeaderChannel());
         EXPECT_EQ(NULL, m_pEngineSync->getLeaderSyncable());
     }
 
@@ -106,7 +106,7 @@ class EngineSyncTest : public MockedEngineBackendTest {
                 qWarning() << "internal clock sync_leader should be 2.0, is" << leader;
                 return false;
             }
-            if (m_pEngineSync->getLeader()) {
+            if (m_pEngineSync->getLeaderChannel()) {
                 qWarning() << "no current leader";
                 return false;
             }
@@ -117,28 +117,28 @@ class EngineSyncTest : public MockedEngineBackendTest {
             return true;
         }
         if (group == m_sGroup1) {
-            if (m_pEngineSync->getLeader() != m_pChannel1) {
+            if (m_pEngineSync->getLeaderChannel() != m_pChannel1) {
                 qWarning() << "leader pointer should be channel 1, is "
-                           << (m_pEngineSync->getLeader()
-                                              ? m_pEngineSync->getLeader()
+                           << (m_pEngineSync->getLeaderChannel()
+                                              ? m_pEngineSync->getLeaderChannel()
                                                         ->getGroup()
                                               : "null");
                 return false;
             }
         } else if (group == m_sGroup2) {
-            if (m_pEngineSync->getLeader() != m_pChannel2) {
+            if (m_pEngineSync->getLeaderChannel() != m_pChannel2) {
                 qWarning() << "leader pointer should be channel 2, is "
-                           << (m_pEngineSync->getLeader()
-                                              ? m_pEngineSync->getLeader()
+                           << (m_pEngineSync->getLeaderChannel()
+                                              ? m_pEngineSync->getLeaderChannel()
                                                         ->getGroup()
                                               : "null");
                 return false;
             }
         } else if (group == m_sGroup3) {
-            if (m_pEngineSync->getLeader() != m_pChannel3) {
+            if (m_pEngineSync->getLeaderChannel() != m_pChannel3) {
                 qWarning() << "leader pointer should be channel 3, is "
-                           << (m_pEngineSync->getLeader()
-                                              ? m_pEngineSync->getLeader()
+                           << (m_pEngineSync->getLeaderChannel()
+                                              ? m_pEngineSync->getLeaderChannel()
                                                         ->getGroup()
                                               : "null");
                 return false;
@@ -702,14 +702,13 @@ TEST_F(EngineSyncTest, RateChangeTestOrder3) {
     EXPECT_DOUBLE_EQ(
             120.0, ControlObject::get(ConfigKey(m_sGroup2, "file_bpm")));
 
-    // Turn on Leader. Setting explicit leader causes this track's rate to be adopted instead
-    // of matching against the other deck.
+    // Turn on Leader. Even though it is explicit leader, it still matches the other deck.
     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")));
 
     // Turn on follower.
     auto pButtonLeaderSync2 =
@@ -718,12 +717,12 @@ TEST_F(EngineSyncTest, RateChangeTestOrder3) {
     ProcessBuffer();
 
     // Follower should immediately set its slider.
-    EXPECT_NEAR(getRateSliderValue(1.3333333333),
-            ControlObject::get(ConfigKey(m_sGroup2, "rate")),
+    EXPECT_NEAR(getRateSliderValue(0.75),
+            ControlObject::get(ConfigKey(m_sGroup1, "rate")),
             kMaxFloatingPointErrorLowPrecision);
-    EXPECT_DOUBLE_EQ(160.0, ControlObject::get(ConfigKey(m_sGroup2, "bpm")));
+    EXPECT_DOUBLE_EQ(120.0, ControlObject::get(ConfigKey(m_sGroup2, "bpm")));
     EXPECT_DOUBLE_EQ(
-            160.0, ControlObject::get(ConfigKey(m_sInternalClockGroup, "bpm")));
+            120.0, ControlObject::get(ConfigKey(m_sInternalClockGroup, "bpm")));
 }
 
 TEST_F(EngineSyncTest, FollowerRateChange) {
@@ -846,12 +845,12 @@ TEST_F(EngineSyncTest, LeaderStopSliderCheck) {
             m_pTrack2->getSampleRate(), mixxx::Bpm(128), mixxx::audio::kStartFramePos);
     m_pTrack2->trySetBeats(pBeats2);
 
-    auto pButtonLeaderSync1 =
-            std::make_unique<ControlProxy>(m_sGroup1, "sync_mode");
-    pButtonLeaderSync1->set(static_cast<double>(SyncMode::LeaderExplicit));
     auto pButtonLeaderSync2 =
             std::make_unique<ControlProxy>(m_sGroup2, "sync_mode");
     pButtonLeaderSync2->set(static_cast<double>(SyncMode::Follower));
+    auto pButtonLeaderSync1 =
+            std::make_unique<ControlProxy>(m_sGroup1, "sync_mode");
+    pButtonLeaderSync1->set(static_cast<double>(SyncMode::LeaderExplicit));
     ProcessBuffer();
 
     //EXPECT_TRUE(isExplicitLeader(m_sGroup1));
@@ -1006,7 +1005,7 @@ TEST_F(EngineSyncTest, LoadTrackInitializesLeader) {
     pButtonSyncEnabled1->set(1.0);
 
     // No leader because this deck has no track.
-    EXPECT_EQ(NULL, m_pEngineSync->getLeader());
+    EXPECT_EQ(NULL, m_pEngineSync->getLeaderChannel());
     EXPECT_DOUBLE_EQ(
             0.0, ControlObject::getControl(ConfigKey(m_sGroup1, "bpm"))->get());
 
@@ -1016,13 +1015,12 @@ TEST_F(EngineSyncTest, LoadTrackInitializesLeader) {
     EXPECT_DOUBLE_EQ(140.0,
             ControlObject::getControl(ConfigKey(m_sGroup1, "bpm"))->get());
 
-    // But as soon as we play, deck 1 is leader
+    // Play button doesn't affect leader.
     ControlObject::getControl(ConfigKey(m_sGroup1, "play"))->set(1.0);
     EXPECT_TRUE(isSoftLeader(m_sGroup1));
     ControlObject::getControl(ConfigKey(m_sGroup1, "play"))->set(0.0);
 
-    // If sync is on two decks and we load a track in only one of them, it will be
-    // leader.
+    // Ejecting the track has no effect on leader status
     m_pChannel1->getEngineBuffer()->slotEjectTrack(1.0);
     EXPECT_TRUE(isFollower(m_sGroup1));
     // no relevant tempo available so internal clock is following
@@ -1069,7 +1067,8 @@ TEST_F(EngineSyncTest, LoadTrackResetTempoOption) {
             std::make_unique<ControlProxy>(m_sGroup2, "sync_enabled");
     pButtonSyncEnabled2->set(1.0);
 
-    // If sync is on and we load a track, that should initialize leader.
+    // If sync is on and we load a track, it should initialize leader because
+    // the other track has no bpm.
     TrackPointer track1 = m_pMixerDeck1->loadFakeTrack(false, 140.0);
 
     EXPECT_DOUBLE_EQ(
@@ -1102,12 +1101,13 @@ TEST_F(EngineSyncTest, LoadTrackResetTempoOption) {
     EXPECT_DOUBLE_EQ(140.0, ControlObject::get(ConfigKey(m_sGroup2, "bpm")));
 
     // Repeat with RESET_NONE
+    EXPECT_TRUE(isSoftLeader(m_sGroup1));
     m_pConfig->set(ConfigKey("[Controls]", "SpeedAutoReset"),
             ConfigValue(BaseTrackPlayer::RESET_NONE));
     ControlObject::set(ConfigKey(m_sGroup1, "play"), 0.0);
     ControlObject::set(ConfigKey(m_sGroup1, "rate"), getRateSliderValue(1.0));
     ControlObject::set(ConfigKey(m_sGroup2, "rate"), getRateSliderValue(1.0));
-    track1 = m_pMixerDeck1->loadFakeTrack(false, 140.0);
+    track1 = m_pMixerDeck1->loadFakeTrack(false, 124.0);
     ControlObject::set(ConfigKey(m_sGroup1, "play"), 1.0);
     track2 = m_pMixerDeck2->loadFakeTrack(false, 128.0);
     EXPECT_DOUBLE_EQ(
@@ -1225,7 +1225,7 @@ TEST_F(EngineSyncTest, SyncToNonSyncDeck) {
                     ConfigKey(m_sInternalClockGroup, "sync_leader")));
     EXPECT_DOUBLE_EQ(
             100.0, ControlObject::get(ConfigKey(m_sInternalClockGroup, "bpm")));
-    EXPECT_EQ(NULL, m_pEngineSync->getLeader());
+    EXPECT_EQ(NULL, m_pEngineSync->getLeaderChannel());
     EXPECT_EQ(NULL, m_pEngineSync->getLeaderSyncable());
     EXPECT_EQ(static_cast<double>(SyncMode::None),
             ControlObject::get(ConfigKey(m_sGroup1, "sync_mode")));
@@ -1251,7 +1251,7 @@ TEST_F(EngineSyncTest, SyncToNonSyncDeck) {
                     ConfigKey(m_sInternalClockGroup, "sync_leader")));
     EXPECT_DOUBLE_EQ(
             100.0, ControlObject::get(ConfigKey(m_sInternalClockGroup, "bpm")));
-    EXPECT_EQ(NULL, m_pEngineSync->getLeader());
+    EXPECT_EQ(NULL, m_pEngineSync->getLeaderChannel());
     EXPECT_EQ(NULL, m_pEngineSync->getLeaderSyncable());
     EXPECT_EQ(static_cast<double>(SyncMode::None),
             ControlObject::get(ConfigKey(m_sGroup2, "sync_mode")));
@@ -1393,7 +1393,7 @@ TEST_F(EngineSyncTest, EjectTrackSyncRemains) {
 }
 
 TEST_F(EngineSyncTest, FileBpmChangesDontAffectLeader) {
-    // If filebpm changes, don't treat it like a rate change unless it's the leader.
+    // If filebpm changes, don't treat it like a rate change.
     mixxx::BeatsPointer pBeats1 = BeatFactory::makeBeatGrid(
             m_pTrack1->getSampleRate(), mixxx::Bpm(100), mixxx::audio::kStartFramePos);
     m_pTrack1->trySetBeats(pBeats1);
@@ -1417,6 +1417,8 @@ TEST_F(EngineSyncTest, FileBpmChangesDontAffectLeader) {
     pBeats1 = BeatFactory::makeBeatGrid(
             m_pTrack1->getSampleRate(), mixxx::Bpm(160), mixxx::audio::kStartFramePos);
     m_pTrack1->trySetBeats(pBeats1);
+    EXPECT_DOUBLE_EQ(
+            160.0, ControlObject::get(ConfigKey(m_sGroup1, "bpm")));
     EXPECT_DOUBLE_EQ(
             160.0, ControlObject::get(ConfigKey(m_sInternalClockGroup, "bpm")));
 
@@ -2365,10 +2367,10 @@ TEST_F(EngineSyncTest, FollowerUserTweakPreservedInLeaderChange) {
     // 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->getSampleRate(), mixxx::Bpm(130), mixxx::audio::kStartFramePos);
     m_pTrack1->trySetBeats(pBeats1);
     mixxx::BeatsPointer pBeats2 = BeatFactory::makeBeatGrid(
-            m_pTrack2->getSampleRate(), mixxx::Bpm(130), mixxx::audio::kStartFramePos);
+            m_pTrack2->getSampleRate(), mixxx::Bpm(kDivisibleBpm), mixxx::audio::kStartFramePos);
     m_pTrack2->trySetBeats(pBeats2);
 
     ControlObject::getControl(ConfigKey(m_sGroup1, "sync_leader"))->set(1);
@@ -2424,8 +2426,8 @@ TEST_F(EngineSyncTest, LeaderUserTweakPreservedInLeaderChange) {
             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::getControl(ConfigKey(m_sGroup1, "sync_leader"))->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);