Merge pull request #3745 from daschuer/lp1920084

Fix quantized play from the pre-roll in beatmap tracks 

3 files changed, 52 insertions, 33 deletions

diff --git a/src/engine/controls/bpmcontrol.cpp b/src/engine/controls/bpmcontrol.cpp
index 617c973654..1914b4c82d 100644
--- a/src/engine/controls/bpmcontrol.cpp
+++ b/src/engine/controls/bpmcontrol.cpp
@@ -39,6 +39,11 @@ constexpr int kBpmTapFilterLength = 5;
 // the actual number of beats is this x2.
 constexpr int kLocalBpmSpan = 4;
 constexpr SINT kSamplesPerFrame = 2;
+
+// If we are 1 / 8.0 beat fraction near the previous beat we match that instead
+// of the next beat.
+constexpr double kPastBeatMatchThreshold = 1 / 8.0;
+
 } // namespace
 
 BpmControl::BpmControl(const QString& group,
@@ -46,8 +51,7 @@ BpmControl::BpmControl(const QString& group,
         : EngineControl(group, pConfig),
           m_tapFilter(this, kBpmTapFilterLength, kBpmTapMaxInterval),
           m_dSyncInstantaneousBpm(0.0),
-          m_dLastSyncAdjustment(1.0),
-          m_dUserTweakingSync(false) {
+          m_dLastSyncAdjustment(1.0) {
     m_dSyncTargetBeatDistance.setValue(0.0);
     m_dUserOffset.setValue(0.0);
 
@@ -407,7 +411,6 @@ double BpmControl::calcSyncedRate(double userTweak) {
     if (kLogger.traceEnabled()) {
         kLogger.trace() << getGroup() << "BpmControl::calcSyncedRate, tweak " << userTweak;
     }
-    m_dUserTweakingSync = userTweak != 0.0;
     double rate = 1.0;
     // Don't know what to do if there's no bpm.
     if (m_pLocalBpm->toBool()) {
@@ -444,7 +447,7 @@ double BpmControl::calcSyncedRate(double userTweak) {
     }
 
     // Now we have all we need to calculate the sync adjustment if any.
-    double adjustment = calcSyncAdjustment(m_dUserTweakingSync);
+    double adjustment = calcSyncAdjustment(userTweak != 0.0);
     return (rate + userTweak) * adjustment;
 }
 
@@ -859,7 +862,20 @@ double BpmControl::getBeatMatchPosition(
         return dThisPosition;
     }
 
-    double dOtherPosition = pOtherEngineBuffer->getExactPlayPos();
+    const double dOtherPosition = pOtherEngineBuffer->getExactPlayPos();
+    const double dThisSampleRate = m_pBeats->getSampleRate();
+    const double dThisRateRatio = m_pRateRatio->get();
+
+    // Seek our next beat to the other next beat near our beat.
+    // This is the only thing we can do if the track has different BPM,
+    // playing the next beat together.
+
+    // First calculate the position in the other track where this next beat will be.
+    const double thisSecs2ToNextBeat = (dThisNextBeat - dThisPosition) /
+            dThisSampleRate / dThisRateRatio;
+    const double dOtherPositionOfThisNextBeat =
+            thisSecs2ToNextBeat * otherBeats->getSampleRate() * pOtherEngineBuffer->getRateRatio() +
+            dOtherPosition;
 
     double dOtherPrevBeat = -1;
     double dOtherNextBeat = -1;
@@ -867,7 +883,7 @@ double BpmControl::getBeatMatchPosition(
     double dOtherBeatFraction = -1;
     if (!BpmControl::getBeatContext(
                 otherBeats,
-                dOtherPosition,
+                dOtherPositionOfThisNextBeat,
                 &dOtherPrevBeat,
                 &dOtherNextBeat,
                 &dOtherBeatLength,
@@ -880,29 +896,24 @@ double BpmControl::getBeatMatchPosition(
         return dThisPosition;
     }
 
-    double dThisSampleRate = m_pBeats->getSampleRate();
-    double dThisRateRatio = m_pRateRatio->get();
-
-    // Seek our next beat to the other next beat
-    // This is the only thing we can do if the track has different BPM,
-    // playing the next beat together.
-    double thisDivSec = (dThisNextBeat - dThisPosition) /
-            dThisSampleRate / dThisRateRatio;
-
-    if (dOtherBeatFraction < 1.0 / 8) {
-        // the user has probably pressed play too late, sync the previous beat
-        dOtherBeatFraction += 1.0;
+    // We can either match the past beat with dOtherBeatFraction 1.0
+    // or the next beat with dOtherBeatFraction 0.0
+    // We prefer the next because this is what will be played,
+    // unless we are close to the previous.
+    // This happens if the user presses play too late.
+    if (dOtherBeatFraction > 1.0 - kPastBeatMatchThreshold) {
+        // match the past beat
+        dOtherBeatFraction -= 1.0;
     }
 
-    dOtherBeatFraction += m_dUserOffset.getValue();
-    double otherDivSec = (1 - dOtherBeatFraction) *
+    double otherDivSec2 = dOtherBeatFraction *
             dOtherBeatLength / otherBeats->getSampleRate() / pOtherEngineBuffer->getRateRatio();
-
-    // This matches the next beat in of both tracks.
-    double seekMatch = (thisDivSec - otherDivSec) *
-            dThisSampleRate * dThisRateRatio;
+    // Transform for this track
+    double seekMatch = otherDivSec2 * dThisSampleRate * dThisRateRatio;
 
     if (dThisBeatLength > 0) {
+        // restore phase adjustment
+        seekMatch += (dThisBeatLength * m_dUserOffset.getValue());
         if (dThisBeatLength / 2 < seekMatch) {
             // seek to previous beat, because of shorter distance
             seekMatch -= dThisBeatLength;
diff --git a/src/engine/controls/bpmcontrol.h b/src/engine/controls/bpmcontrol.h
index 156fdca700..62b2fe8367 100644
--- a/src/engine/controls/bpmcontrol.h
+++ b/src/engine/controls/bpmcontrol.h
@@ -161,7 +161,6 @@ class BpmControl : public EngineControl {
     // used in the engine thread only
     double m_dSyncInstantaneousBpm;
     double m_dLastSyncAdjustment;
-    bool m_dUserTweakingSync;
 
     // m_pBeats is written from an engine worker thread
     mixxx::BeatsPointer m_pBeats;
diff --git a/src/test/enginesynctest.cpp b/src/test/enginesynctest.cpp
index a1ace0478c..128dc0f481 100644
--- a/src/test/enginesynctest.cpp
+++ b/src/test/enginesynctest.cpp
@@ -2264,9 +2264,10 @@ TEST_F(EngineSyncTest, QuantizeImpliesSyncPhase) {
     EXPECT_DOUBLE_EQ(100, ControlObject::get(ConfigKey(m_sGroup2, "bpm")));
 
     // we align here to the past beat, because beat_distance < 1.0/8
-    EXPECT_DOUBLE_EQ(
+    EXPECT_NEAR(
             ControlObject::get(ConfigKey(m_sGroup1, "beat_distance")) / 130 * 100,
-            ControlObject::get(ConfigKey(m_sGroup2, "beat_distance")));
+            ControlObject::get(ConfigKey(m_sGroup2, "beat_distance")),
+            1e-15);
 }
 
 TEST_F(EngineSyncTest, SeekStayInPhase) {
@@ -2285,7 +2286,8 @@ TEST_F(EngineSyncTest, SeekStayInPhase) {
     ProcessBuffer();
 
     // We expect to be two buffers ahead in a beat near 0.2
-    EXPECT_DOUBLE_EQ(0.050309901738473183, ControlObject::get(ConfigKey(m_sGroup1, "beat_distance")));
+    EXPECT_DOUBLE_EQ(0.050309901738473162,
+            ControlObject::get(ConfigKey(m_sGroup1, "beat_distance")));
     EXPECT_DOUBLE_EQ(0.18925937554508981, ControlObject::get(ConfigKey(m_sGroup1, "playposition")));
 
     // The same again with a stopped track loaded in Channel 2
@@ -2308,7 +2310,7 @@ TEST_F(EngineSyncTest, SeekStayInPhase) {
     ProcessBuffer();
 
     // We expect to be two buffers ahead in a beat near 0.2
-    EXPECT_DOUBLE_EQ(0.050309901738473183,
+    EXPECT_DOUBLE_EQ(0.050309901738473162,
             ControlObject::get(ConfigKey(m_sGroup1, "beat_distance")));
     EXPECT_DOUBLE_EQ(0.18925937554508981, ControlObject::get(ConfigKey(m_sGroup1, "playposition")));
 }
@@ -2374,9 +2376,12 @@ TEST_F(EngineSyncTest, QuantizeHotCueActivate) {
     pHotCue2Activate->set(1.0);
     ProcessBuffer();
 
+    // Beat_distance is the distance to the previous beat wich has already passed.
+    // We compare here the distance to the next beat (1 - beat_distance) and
+    // scale it by the different tempos.
     EXPECT_NEAR(
-            ControlObject::get(ConfigKey(m_sGroup1, "beat_distance")) / 130 * 100,
-            ControlObject::get(ConfigKey(m_sGroup2, "beat_distance")),
+            (1 - ControlObject::get(ConfigKey(m_sGroup1, "beat_distance"))) / 130 * 100,
+            (1 - ControlObject::get(ConfigKey(m_sGroup2, "beat_distance"))),
             1e-15);
 
     pHotCue2Activate->set(0.0);
@@ -2491,8 +2496,12 @@ TEST_F(EngineSyncTest, BeatMapQantizePlay) {
 
     ProcessBuffer();
 
-    EXPECT_DOUBLE_EQ(m_pChannel1->getEngineBuffer()->m_pSyncControl->getBeatDistance(),
-            m_pChannel2->getEngineBuffer()->m_pSyncControl->getBeatDistance());
+    // Beat Distance shall be still 0, because we are before the first beat.
+    // This was fixed in https://bugs.launchpad.net/mixxx/+bug/1920084
+    EXPECT_DOUBLE_EQ(m_pChannel2->getEngineBuffer()->m_pSyncControl->getBeatDistance(), 0);
+    EXPECT_DOUBLE_EQ(
+            ControlObject::get(ConfigKey(m_sGroup1, "playposition")),
+            ControlObject::get(ConfigKey(m_sGroup2, "playposition")));
 }
 
 TEST_F(EngineSyncTest, BpmAdjustFactor) {