Merge pull request #2940 from ywwg/more-rate-change-tests

Fix drifting sync when unquantized

4 files changed, 165 insertions, 17 deletions

diff --git a/src/engine/controls/bpmcontrol.cpp b/src/engine/controls/bpmcontrol.cpp
index 48ab0453cc..049a9e791b 100644
--- a/src/engine/controls/bpmcontrol.cpp
+++ b/src/engine/controls/bpmcontrol.cpp
@@ -436,6 +436,7 @@ double BpmControl::calcSyncAdjustment(bool userTweakingSync) {
         kLogger.trace() << m_group << "****************";
         kLogger.trace() << "master beat distance:" << syncTargetBeatDistance;
         kLogger.trace() << "my     beat distance:" << thisBeatDistance;
+        kLogger.trace() << "user offset distance:" << m_dUserOffset.getValue();
         kLogger.trace() << "error               :"
                         << (shortest_distance - m_dUserOffset.getValue());
         kLogger.trace() << "user offset         :" << m_dUserOffset.getValue();
@@ -1011,6 +1012,9 @@ double BpmControl::updateBeatDistance() {
 }
 
 void BpmControl::setTargetBeatDistance(double beatDistance) {
+    if (kLogger.traceEnabled()) {
+        qDebug() << getGroup() << "BpmControl::setTargetBeatDistance:" << beatDistance;
+    }
     m_dSyncTargetBeatDistance.setValue(beatDistance);
 }
 
diff --git a/src/engine/sync/internalclock.cpp b/src/engine/sync/internalclock.cpp
index 0f598743c0..9b1b1a819d 100644
--- a/src/engine/sync/internalclock.cpp
+++ b/src/engine/sync/internalclock.cpp
@@ -20,7 +20,6 @@ InternalClock::InternalClock(const QString& group, SyncableListener* pEngineSync
           m_iOldSampleRate(44100),
           m_dOldBpm(124.0),
           m_dBaseBpm(124.0),
-          m_bClockUpdated(false),
           m_dBeatLength(m_iOldSampleRate * 60.0 / m_dOldBpm),
           m_dClockPosition(0) {
     // Pick a wide range (1 to 200) and allow out of bounds sets. This lets you
@@ -101,7 +100,6 @@ void InternalClock::setMasterBeatDistance(double beatDistance) {
     if (kLogger.traceEnabled()) {
         kLogger.trace() << "InternalClock::setMasterBeatDistance" << beatDistance;
     }
-    m_bClockUpdated.fetchAndStoreAcquire(true);
     m_dClockPosition = beatDistance * m_dBeatLength;
     m_pClockBeatDistance->set(beatDistance);
     // Make sure followers have an up-to-date beat distance.
@@ -202,19 +200,12 @@ void InternalClock::updateBeatLength(int sampleRate, double bpm) {
 void InternalClock::onCallbackStart(int sampleRate, int bufferSize) {
     Q_UNUSED(sampleRate)
     Q_UNUSED(bufferSize)
-    m_bClockUpdated.fetchAndStoreAcquire(false);
     m_pEngineSync->notifyInstantaneousBpmChanged(this, getBpm());
 }
 
 void InternalClock::onCallbackEnd(int sampleRate, int bufferSize) {
     updateBeatLength(sampleRate, m_pClockBpm->get());
 
-    // If the clock was updated mid-call due to seeks or other updates from other decks, skip the
-    // incrementing of the clock.
-    if (m_bClockUpdated.fetchAndStoreRelaxed(false)) {
-        return;
-    }
-
     // stereo samples, so divide by 2
     m_dClockPosition += bufferSize / 2;
 
diff --git a/src/engine/sync/internalclock.h b/src/engine/sync/internalclock.h
index c5093e32c2..aa2e6b989c 100644
--- a/src/engine/sync/internalclock.h
+++ b/src/engine/sync/internalclock.h
@@ -75,7 +75,6 @@ class InternalClock : public QObject, public Clock, public Syncable {
     int m_iOldSampleRate;
     double m_dOldBpm;
     double m_dBaseBpm;
-    QAtomicInteger<bool> m_bClockUpdated;
 
     // The internal clock rate is stored in terms of samples per beat.
     // Fractional values are allowed.
diff --git a/src/test/enginesynctest.cpp b/src/test/enginesynctest.cpp
index 78352c2f55..39a25a6c77 100644
--- a/src/test/enginesynctest.cpp
+++ b/src/test/enginesynctest.cpp
@@ -1,7 +1,6 @@
 #include <gmock/gmock.h>
 #include <gtest/gtest.h>
 
-#include <QtDebug>
 #include <string>
 
 #include "control/controlobject.h"
@@ -1358,19 +1357,68 @@ TEST_F(EngineSyncTest, ZeroBPMRateAdjustIgnored) {
             ControlObject::getControl(ConfigKey(m_sGroup2, "rate"))->get());
 }
 
-TEST_F(EngineSyncTest, ZeroLatencyRateChange) {
+TEST_F(EngineSyncTest, ZeroLatencyRateChangeNoQuant) {
     // Confirm that a rate change in an explicit master is instantly communicated
     // to followers.
     mixxx::BeatsPointer pBeats1 = BeatFactory::makeBeatGrid(*m_pTrack1, 128, 0.0);
     m_pTrack1->setBeats(pBeats1);
-    mixxx::BeatsPointer pBeats2 = BeatFactory::makeBeatGrid(*m_pTrack2, 128, 0.0);
+    mixxx::BeatsPointer pBeats2 = BeatFactory::makeBeatGrid(*m_pTrack2, 160, 0.0);
+    m_pTrack2->setBeats(pBeats2);
+
+    // Make Channel2 master to weed out any channel ordering issues.
+    ControlObject::getControl(ConfigKey(m_sGroup2, "sync_mode"))
+            ->set(SYNC_FOLLOWER);
+    ControlObject::getControl(ConfigKey(m_sGroup1, "sync_mode"))
+            ->set(SYNC_FOLLOWER);
+    // Exaggerate the effect with a high rate.
+    ControlObject::set(ConfigKey(m_sGroup2, "rate_ratio"), 10.0);
+
+    ControlObject::getControl(ConfigKey(m_sGroup1, "play"))->set(1.0);
+    ControlObject::getControl(ConfigKey(m_sGroup2, "play"))->set(1.0);
+
+    EXPECT_EQ(ControlObject::getControl(ConfigKey(m_sGroup2, "beat_distance"))
+                      ->get(),
+            ControlObject::getControl(ConfigKey(m_sGroup1, "beat_distance"))
+                    ->get());
+
+    for (int i = 0; i < 50; ++i) {
+        ProcessBuffer();
+        // Keep messing with the rate
+        float rate = i % 2 == 0 ? i / 10.0 : i / -10.0;
+        ControlObject::set(ConfigKey(m_sGroup2, "rate_ratio"), rate);
+
+        // Buffers should be in sync.
+        ASSERT_FLOAT_EQ(ControlObject::getControl(ConfigKey(m_sGroup2, "beat_distance"))
+                                ->get(),
+                ControlObject::getControl(ConfigKey(m_sGroup1, "beat_distance"))
+                        ->get());
+    }
+
+    // Make sure we're actually going somewhere!
+    EXPECT_GT(ControlObject::getControl(ConfigKey(m_sGroup1, "beat_distance"))
+                      ->get(),
+            0);
+    // Buffers should be in sync.
+    EXPECT_EQ(ControlObject::getControl(ConfigKey(m_sGroup2, "beat_distance"))
+                      ->get(),
+            ControlObject::getControl(ConfigKey(m_sGroup1, "beat_distance"))
+                    ->get());
+}
+
+TEST_F(EngineSyncTest, ZeroLatencyRateChangeQuant) {
+    // Confirm that a rate change in an explicit master is instantly communicated
+    // to followers.
+    mixxx::BeatsPointer pBeats1 = BeatFactory::makeBeatGrid(*m_pTrack1, 128, 0.0);
+    m_pTrack1->setBeats(pBeats1);
+    mixxx::BeatsPointer pBeats2 = BeatFactory::makeBeatGrid(*m_pTrack2, 160, 0.0);
     m_pTrack2->setBeats(pBeats2);
 
     ControlObject::getControl(ConfigKey(m_sGroup1, "quantize"))->set(1.0);
     ControlObject::getControl(ConfigKey(m_sGroup2, "quantize"))->set(1.0);
+
     // Make Channel2 master to weed out any channel ordering issues.
     ControlObject::getControl(ConfigKey(m_sGroup2, "sync_mode"))
-            ->set(SYNC_MASTER_EXPLICIT);
+            ->set(SYNC_FOLLOWER);
     ControlObject::getControl(ConfigKey(m_sGroup1, "sync_mode"))
             ->set(SYNC_FOLLOWER);
     // Exaggerate the effect with a high rate.
@@ -1384,14 +1432,121 @@ TEST_F(EngineSyncTest, ZeroLatencyRateChange) {
             ControlObject::getControl(ConfigKey(m_sGroup1, "beat_distance"))
                     ->get());
 
-    ProcessBuffer();
-    ProcessBuffer();
+    for (int i = 0; i < 50; ++i) {
+        ProcessBuffer();
+        // Keep messing with the rate
+        float rate = i % 2 == 0 ? i / 10.0 : i / -10.0;
+        ControlObject::set(ConfigKey(m_sGroup2, "rate_ratio"), rate);
+
+        // Buffers should be in sync.
+        ASSERT_FLOAT_EQ(ControlObject::getControl(ConfigKey(m_sGroup2, "beat_distance"))
+                                ->get(),
+                ControlObject::getControl(ConfigKey(m_sGroup1, "beat_distance"))
+                        ->get());
+    }
+
+    // Make sure we're actually going somewhere!
+    EXPECT_GT(ControlObject::getControl(ConfigKey(m_sGroup1, "beat_distance"))
+                      ->get(),
+            0);
+    // Buffers should be in sync.
+    EXPECT_EQ(ControlObject::getControl(ConfigKey(m_sGroup2, "beat_distance"))
+                      ->get(),
+            ControlObject::getControl(ConfigKey(m_sGroup1, "beat_distance"))
+                    ->get());
+}
+
+TEST_F(EngineSyncTest, ZeroLatencyRateDiffQuant) {
+    // Confirm that a rate change in an explicit master is instantly communicated
+    // to followers.
+    mixxx::BeatsPointer pBeats1 = BeatFactory::makeBeatGrid(*m_pTrack1, 128, 0.0);
+    m_pTrack1->setBeats(pBeats1);
+    mixxx::BeatsPointer pBeats2 = BeatFactory::makeBeatGrid(*m_pTrack2, 160, 0.0);
+    m_pTrack2->setBeats(pBeats2);
+
+    ControlObject::getControl(ConfigKey(m_sGroup2, "quantize"))->set(1.0);
+
+    // Make Channel2 master to weed out any channel ordering issues.
+    ControlObject::getControl(ConfigKey(m_sGroup2, "sync_mode"))
+            ->set(SYNC_FOLLOWER);
+    ControlObject::getControl(ConfigKey(m_sGroup1, "sync_mode"))
+            ->set(SYNC_FOLLOWER);
+    // Exaggerate the effect with a high rate.
+    ControlObject::set(ConfigKey(m_sGroup2, "rate_ratio"), 10.0);
+
+    ControlObject::getControl(ConfigKey(m_sGroup1, "play"))->set(1.0);
+    ControlObject::getControl(ConfigKey(m_sGroup2, "play"))->set(1.0);
+
+    EXPECT_EQ(ControlObject::getControl(ConfigKey(m_sGroup2, "beat_distance"))
+                      ->get(),
+            ControlObject::getControl(ConfigKey(m_sGroup1, "beat_distance"))
+                    ->get());
+
+    for (int i = 0; i < 50; ++i) {
+        ProcessBuffer();
+        // Keep messing with the rate
+        float rate = i % 2 == 0 ? i / 10.0 : i / -10.0;
+        ControlObject::set(ConfigKey(m_sGroup2, "rate_ratio"), rate);
+
+        // Buffers should be in sync.
+        ASSERT_FLOAT_EQ(ControlObject::getControl(ConfigKey(m_sGroup2, "beat_distance"))
+                                ->get(),
+                ControlObject::getControl(ConfigKey(m_sGroup1, "beat_distance"))
+                        ->get());
+    }
+
+    // Make sure we're actually going somewhere!
+    EXPECT_GT(ControlObject::getControl(ConfigKey(m_sGroup1, "beat_distance"))
+                      ->get(),
+            0);
+    // Buffers should be in sync.
+    EXPECT_EQ(ControlObject::getControl(ConfigKey(m_sGroup2, "beat_distance"))
+                      ->get(),
+            ControlObject::getControl(ConfigKey(m_sGroup1, "beat_distance"))
+                    ->get());
+}
+
+// In this test, we set play *first* and then turn on master sync.
+// This exercises a slightly different ordering of signals that we
+// need to check. The Sync feature is unfortunately brittle.
+// This test exercises https://bugs.launchpad.net/mixxx/+bug/1884324
+TEST_F(EngineSyncTest, ActivatingSyncDoesNotCauseDrifting) {
+    mixxx::BeatsPointer pBeats1 = BeatFactory::makeBeatGrid(*m_pTrack1, 150, 0.0);
+    m_pTrack1->setBeats(pBeats1);
+    mixxx::BeatsPointer pBeats2 = BeatFactory::makeBeatGrid(*m_pTrack2, 150, 0.0);
+    m_pTrack2->setBeats(pBeats2);
+
+    ControlObject::getControl(ConfigKey(m_sGroup1, "quantize"))->set(0.0);
+    ControlObject::getControl(ConfigKey(m_sGroup2, "quantize"))->set(1.0);
+
+    ControlObject::getControl(ConfigKey(m_sGroup1, "play"))->set(1.0);
+    ControlObject::getControl(ConfigKey(m_sGroup2, "play"))->set(1.0);
+
     ProcessBuffer();
 
+    // make sure we aren't out-of-sync from the start
+    EXPECT_EQ(ControlObject::getControl(ConfigKey(m_sGroup2, "beat_distance"))
+                      ->get(),
+            ControlObject::getControl(ConfigKey(m_sGroup1, "beat_distance"))
+                    ->get());
+
+    // engage first sync-master
+    ControlObject::getControl(ConfigKey(m_sGroup1, "sync_mode"))
+            ->set(SYNC_FOLLOWER);
+
+    // engage second Sync-master
+    ControlObject::getControl(ConfigKey(m_sGroup2, "sync_mode"))
+            ->set(SYNC_FOLLOWER);
+
+    // Run for a number of buffers
+    for (int i = 0; i < 25; ++i) {
+        ProcessBuffer();
+    }
     // Make sure we're actually going somewhere!
     EXPECT_GT(ControlObject::getControl(ConfigKey(m_sGroup1, "beat_distance"))
                       ->get(),
             0);
+
     // Buffers should be in sync.
     EXPECT_EQ(ControlObject::getControl(ConfigKey(m_sGroup2, "beat_distance"))
                       ->get(),
@@ -1433,7 +1588,6 @@ TEST_F(EngineSyncTest, HalfDoubleBpmTest) {
 
     // Do lots of processing to make sure we get over the 0.5 beat_distance barrier.
     for (int i = 0; i < 50; ++i) {
-        qDebug() << "bpm test loop iter" << i;
         ProcessBuffer();
         // The beat distances are NOT as simple as x2 or /2.  Use the built-in functions
         // to do the proper conversion.