Fix issues with half/double and Sync Lock

src/engine/controls/bpmcontrol.cpp

@@ -536,7 +536,9 @@ double BpmControl::getBeatDistance(double dThisPosition) const {
     // we don't adjust the reported distance the track will try to adjust
     // sync against itself.
     if (kLogger.traceEnabled()) {
-        kLogger.trace() << getGroup() << "BpmControl::getBeatDistance" << dThisPosition;
+        kLogger.trace() << getGroup()
+                        << "BpmControl::getBeatDistance. dThisPosition:"
+                        << dThisPosition;
     }
     double dPrevBeat = m_pPrevBeat->get();
     double dNextBeat = m_pNextBeat->get();
@@ -560,7 +562,7 @@ double BpmControl::getBeatDistance(double dThisPosition) const {
     }
 
     if (kLogger.traceEnabled()) {
-        kLogger.trace() << getGroup() << "BpmControl::getBeatDistance"
+        kLogger.trace() << getGroup() << "BpmControl::getBeatDistance. dBeatPercentage:"
                         << dBeatPercentage << "-  offset "
                         << m_dUserOffset.getValue() << " =  "
                         << (dBeatPercentage - m_dUserOffset.getValue());

src/engine/enginemaster.cpp

@@ -369,6 +369,9 @@ void EngineMaster::processChannels(int iBufferSize) {
 
     // Do internal master sync post-processing before the other
     // channels.
+    // Note, because we call this on the internal clock first,
+    // it will have an up-to-date beatDistance, whereas the other
+    // Syncables will not.
     m_pMasterSync->onCallbackEnd(m_iSampleRate, m_iBufferSize);
 
     // After all the engines have been processed, trigger post-processing

src/engine/sync/enginesync.cpp

@@ -75,6 +75,13 @@ void EngineSync::requestSyncMode(Syncable* pSyncable, SyncMode mode) {
     default:;
     }
 
+    Syncable* pOnlyPlayer = getUniquePlayingSyncedDeck();
+    if (pOnlyPlayer) {
+        // This resets the user offset, so that if this deck gets used as the params syncable
+        // it will have that offset removed.
+        pOnlyPlayer->notifyOnlyPlayingSyncable();
+    }
+
     // Second, figure out what Syncable should be used to initialize the master
     // parameters, if any. Usually this is the new master. (Note, that pointer might be null!)
     Syncable* pParamsSyncable = m_pMasterSyncable;
@@ -96,9 +103,6 @@ void EngineSync::requestSyncMode(Syncable* pSyncable, SyncMode mode) {
         }
     }
 
-    if (noPlayingFollowers() && m_pMasterSyncable) {
-        m_pMasterSyncable->notifyOnlyPlayingSyncable();
-    }
 }
 
 void EngineSync::activateFollower(Syncable* pSyncable) {
@@ -350,11 +354,15 @@ void EngineSync::notifyPlayingAudible(Syncable* pSyncable, bool playingAudible)
 
     if (newMaster != nullptr && newMaster != m_pMasterSyncable) {
         activateMaster(newMaster, SYNC_MASTER_SOFT);
-    }
-
-    if (noPlayingFollowers() && m_pMasterSyncable) {
-        m_pMasterSyncable->notifyOnlyPlayingSyncable();
-        setMasterParams(m_pMasterSyncable);
+        setMasterParams(newMaster);
+    } else {
+        Syncable* pOnlyPlayer = getUniquePlayingSyncedDeck();
+        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();
+            setMasterBeatDistance(pOnlyPlayer, pOnlyPlayer->getBeatDistance());
+        }
     }
 
     pSyncable->requestSync();
@@ -584,6 +592,11 @@ void EngineSync::setMasterInstantaneousBpm(Syncable* pSource, double bpm) {
 }
 
 void EngineSync::setMasterBeatDistance(Syncable* pSource, double beatDistance) {
+    if (kLogger.traceEnabled()) {
+        kLogger.trace() << "EngineSync::setMasterBeatDistance"
+                        << (pSource ? pSource->getGroup() : "null")
+                        << beatDistance;
+    }
     if (pSource != m_pInternalClock) {
         m_pInternalClock->setMasterBeatDistance(beatDistance);
     }
@@ -597,35 +610,68 @@ void EngineSync::setMasterBeatDistance(Syncable* pSource, double beatDistance) {
 }
 
 void EngineSync::setMasterParams(Syncable* pSource) {
-    const double beatDistance = pSource->getBeatDistance();
+    // Important note! Because of the way sync works, the new master is usually not the same
+    // as the Syncable setting the master parameters (here, pSource). Notify the proper Syncable
+    // so it can prepare itself.  (This is a hack to undo half/double math so that we initialize
+    // based on un-multiplied bpm values).
+    pSource->notifyMasterParamSource();
+
+    double beatDistance = pSource->getBeatDistance();
+    if (!pSource->isPlaying()) {
+        // If the params source is not playing, but other syncables are, then we are a stopped
+        // explicit Master and we should not initialize the beat distance.  Take it from the
+        // internal clock instead, because that will be up to date with the playing deck(s).
+        bool playingSyncables = false;
+        for (Syncable* pSyncable : qAsConst(m_syncables)) {
+            if (pSyncable == pSource) {
+                continue;
+            }
+            if (!pSyncable->isSynchronized()) {
+                continue;
+            }
+            if (pSyncable->isPlaying()) {
+                playingSyncables = true;
+                break;
+            }
+        }
+        if (playingSyncables) {
+            beatDistance = m_pInternalClock->getBeatDistance();
+        }
+    }
     const double baseBpm = pSource->getBaseBpm();
     double bpm = pSource->getBpm();
     if (bpm <= 0) {
         bpm = baseBpm;
     }
-    // qDebug() << "BaseSyncableListener::setMasterParams, source is"
-    //          << pSource->getGroup() << beatDistance << baseBpm << bpm;
+    if (kLogger.traceEnabled()) {
+        kLogger.trace() << "BaseSyncableListener::setMasterParams, source is"
+                        << pSource->getGroup() << beatDistance << baseBpm << bpm;
+    }
     if (pSource != m_pInternalClock) {
         m_pInternalClock->setMasterParams(beatDistance, baseBpm, bpm);
     }
     foreach (Syncable* pSyncable, m_syncables) {
-        if (pSyncable == pSource || !pSyncable->isSynchronized()) {
+        if (!pSyncable->isSynchronized()) {
             continue;
         }
         pSyncable->setMasterParams(beatDistance, baseBpm, bpm);
     }
 }
 
-bool EngineSync::noPlayingFollowers() const {
+Syncable* EngineSync::getUniquePlayingSyncedDeck() const {
+    Syncable* onlyPlaying = nullptr;
     for (Syncable* pSyncable : m_syncables) {
         if (!pSyncable->isSynchronized()) {
             continue;
         }
 
-        if (pSyncable->isPlaying() && pSyncable->isAudible() &&
-                isFollower(pSyncable->getSyncMode())) {
-            return false;
+        if (pSyncable->isPlaying()) {
+            if (!onlyPlaying) {
+                onlyPlaying = pSyncable;
+            } else {
+                return nullptr;
+            }
         }
     }
-    return true;
+    return onlyPlaying;
 }

src/engine/sync/enginesync.h

@@ -115,8 +115,9 @@ class EngineSync : public SyncableListener {
 
     void setMasterParams(Syncable* pSource);
 
-    /// Check if there are no playing followers left.
-    bool noPlayingFollowers() const;
+    /// Iff there is a single playing syncable in sync mode, return it.
+    /// This is used to initialize master params.
+    Syncable* getUniquePlayingSyncedDeck() const;
 
     /// Only for testing. Do not use.
     Syncable* getSyncableForGroup(const QString& group);

src/engine/sync/internalclock.cpp

@@ -140,6 +140,9 @@ void InternalClock::setInstantaneousBpm(double bpm) {
     Q_UNUSED(bpm);
 }
 
+void InternalClock::notifyMasterParamSource() {
+}
+
 void InternalClock::setMasterParams(double beatDistance, double baseBpm, double bpm) {
     if (kLogger.traceEnabled()) {
         kLogger.trace() << "InternalClock::setMasterParams" << beatDistance << baseBpm << bpm;

src/engine/sync/internalclock.h

@@ -52,6 +52,7 @@ class InternalClock : public QObject, public Clock, public Syncable {
 
     double getBaseBpm() const override;
     void setMasterBpm(double bpm) override;
+    void notifyMasterParamSource() override;
     double getBpm() const override;
     void setInstantaneousBpm(double bpm) override;
     void setMasterParams(double beatDistance, double baseBpm, double bpm) override;

src/engine/sync/syncable.h

@@ -112,6 +112,11 @@ class Syncable {
     // signal loops could occur.
     virtual void setMasterBpm(double bpm) = 0;
 
+    // Tells a Syncable that it's going to be used as a source for master
+    // params. This is a gross hack so that the SyncControl can undo its
+    // half/double adjustment so bpms are initialized correctly.
+    virtual void notifyMasterParamSource() = 0;
+
     // Combines the above three calls into one, since they are often set
     // simultaneously.  Avoids redundant recalculation that would occur by
     // using the three calls separately.

src/engine/sync/synccontrol.cpp

@@ -238,19 +238,17 @@ void SyncControl::setMasterBpm(double bpm) {
     }
 }
 
+void SyncControl::notifyMasterParamSource() {
+    m_masterBpmAdjustFactor = kBpmUnity;
+}
+
 void SyncControl::setMasterParams(
         double beatDistance, double baseBpm, double bpm) {
-    // Calculate the factor for the file bpm. That gives the best
-    // result at any rate slider position.
-    double masterBpmAdjustFactor = determineBpmMultiplier(fileBpm(), baseBpm);
-    if (isMaster(getSyncMode())) {
-        // In Master mode we adjust the incoming Bpm for the initial sync.
-        bpm *= masterBpmAdjustFactor;
-        m_masterBpmAdjustFactor = kBpmUnity;
-    } else {
-        // in Follower mode we keep the factor when reporting our BPM
-        m_masterBpmAdjustFactor = masterBpmAdjustFactor;
+    if (kLogger.traceEnabled()) {
+        kLogger.trace() << "SyncControl::setMasterParams" << getGroup()
+                        << beatDistance << baseBpm << bpm;
     }
+    m_masterBpmAdjustFactor = determineBpmMultiplier(fileBpm(), baseBpm);
     setMasterBpm(bpm);
     setMasterBeatDistance(beatDistance);
 }
@@ -274,7 +272,10 @@ double SyncControl::determineBpmMultiplier(double myBpm, double targetBpm) const
 void SyncControl::updateTargetBeatDistance() {
     double targetDistance = m_unmultipliedTargetBeatDistance;
     if (kLogger.traceEnabled()) {
-        kLogger.trace() << getGroup() << "SyncControl::updateTargetBeatDistance, unmult distance" << targetDistance;
+        kLogger.trace()
+                << getGroup()
+                << "SyncControl::updateTargetBeatDistance, unmult distance"
+                << targetDistance << m_masterBpmAdjustFactor;
     }
 
     // Determining the target distance is not as simple as x2 or /2.  Since one
@@ -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) {
             targetDistance += 0.5;
         }
     }
@@ -301,7 +303,8 @@ void SyncControl::updateTargetBeatDistance() {
 
 double SyncControl::getBpm() const {
     if (kLogger.traceEnabled()) {
-        kLogger.trace() << getGroup() << "SyncControl::getBpm()";
+        kLogger.trace() << getGroup() << "SyncControl::getBpm()"
+                        << m_pBpm->get() << "/" << m_masterBpmAdjustFactor;
     }
     return m_pBpm->get() / m_masterBpmAdjustFactor;
 }

src/engine/sync/synccontrol.h

@@ -48,10 +48,10 @@ class SyncControl : public EngineControl, public Syncable {
     // Must never result in a call to
     // SyncableListener::notifyBeatDistanceChanged or signal loops could occur.
     void setMasterBeatDistance(double beatDistance) override;
-
     // Must never result in a call to
     // SyncableListener::notifyBpmChanged or signal loops could occur.
     void setMasterBpm(double bpm) override;
+    void notifyMasterParamSource() override;
     void setMasterParams(double beatDistance, double baseBpm, double bpm) override;
 
     // Must never result in a call to

src/test/enginesynctest.cpp

@@ -1445,6 +1445,24 @@ TEST_F(EngineSyncTest, ZeroBPMRateAdjustIgnored) {
             ControlObject::getControl(ConfigKey(m_sGroup2, "rate"))->get());
 }
 
+TEST_F(EngineSyncTest, DISABLED_BeatDistanceBeforeStart) {
+    // https://bugs.launchpad.net/mixxx/+bug/1930143
+    // If the start position is before zero, we should still initialize the beat distance
+    // correctly.  Unfortunately, this currently doesn't work.
+
+    mixxx::BeatsPointer pBeats1 = BeatFactory::makeBeatGrid(m_pTrack1->getSampleRate(), 128, 0.0);
+    m_pTrack1->trySetBeats(pBeats1);
+    ControlObject::set(ConfigKey(m_sGroup1, "playposition"), -.05);
+    ControlObject::getControl(ConfigKey(m_sGroup1, "sync_mode"))
+            ->set(SYNC_MASTER_SOFT);
+    ControlObject::getControl(ConfigKey(m_sGroup1, "play"))->set(1.0);
+    ProcessBuffer();
+    EXPECT_NEAR(
+            ControlObject::getControl(ConfigKey(m_sGroup1, "beat_distance"))->get(),
+            ControlObject::getControl(ConfigKey(m_sInternalClockGroup, "beat_distance"))->get(),
+            kMaxBeatDistanceEpsilon);
+}
+
 TEST_F(EngineSyncTest, ZeroLatencyRateChangeNoQuant) {
     // Confirm that a rate change in an explicit master is instantly communicated
     // to followers.
@@ -1664,10 +1682,10 @@ TEST_F(EngineSyncTest, HalfDoubleBpmTest) {
     ControlObject::getControl(ConfigKey(m_sGroup1, "play"))->set(1.0);
     ProcessBuffer();
 
-    EXPECT_EQ(1.0,
+    EXPECT_EQ(0.5,
             m_pChannel1->getEngineBuffer()
                     ->m_pSyncControl->m_masterBpmAdjustFactor);
-    EXPECT_EQ(2.0,
+    EXPECT_EQ(1.0,
             m_pChannel2->getEngineBuffer()
                     ->m_pSyncControl->m_masterBpmAdjustFactor);
     EXPECT_DOUBLE_EQ(
@@ -1789,7 +1807,7 @@ TEST_F(EngineSyncTest, HalfDoubleThenPlay) {
 
     ProcessBuffer();
 
-    EXPECT_DOUBLE_EQ(87.5,
+    EXPECT_DOUBLE_EQ(175,
             ControlObject::getControl(ConfigKey(m_sInternalClockGroup, "bpm"))
                     ->get());
 
@@ -2021,12 +2039,13 @@ TEST_F(EngineSyncTest, SyncPhaseToPlayingNonSyncDeck) {
 
     ProcessBuffer();
 
-    // we expect that Deck 1 distance has not changed and the internal clock follows it exactly.
+    // we expect that Deck 1 distance has not changed but the internal clock keeps going, because
+    // the internal clock should continue playing even if the leader is stopped.
     EXPECT_TRUE(isSoftMaster(m_sGroup1));
     EXPECT_NEAR(0.019349962,
             ControlObject::getControl(ConfigKey(m_sGroup1, "beat_distance"))->get(),
             kMaxFloatingPointErrorLowPrecision);
-    EXPECT_NEAR(0.019349962,
+    EXPECT_NEAR(0.038699924,
             ControlObject::getControl(ConfigKey(m_sInternalClockGroup, "beat_distance"))->get(),
             kMaxFloatingPointErrorLowPrecision);