Merge pull request #4370 from ywwg/cue-seek-fix

Update main cue position before seeking to it
mixxxdj · Oct 12, 2021 · c8c1855 · c8c1855
2 parents 58572fb + 8ec0a44
commit c8c1855
Show file tree

Hide file tree

Showing 4 changed files with 79 additions and 4 deletions.
diff --git a/src/engine/controls/cuecontrol.cpp b/src/engine/controls/cuecontrol.cpp
@@ -1342,8 +1342,13 @@ void CueControl::cueCDJ(double value) {
             // If quantize is enabled, jump to the cue point since it's not
             // necessarily where we currently are
             if (m_pQuantizeEnabled->toBool()) {
-                // Enginebuffer will quantize more exactly than we can.
-                seekAbs(mainCuePosition);
+                // We need to re-get the cue point since it changed.
+                const auto newCuePosition = mixxx::audio::FramePos::fromEngineSamplePosMaybeInvalid(
+                        m_pCuePoint->get());
+                if (newCuePosition.isValid()) {
+                    // Enginebuffer will quantize more exactly than we can.
+                    seekAbs(newCuePosition);
+                }
             }
         }
     } else if (m_currentlyPreviewingIndex == kMainCueIndex) {
@@ -1434,8 +1439,12 @@ void CueControl::cuePlay(double value) {
             // If quantize is enabled, jump to the cue point since it's not
             // necessarily where we currently are
             if (m_pQuantizeEnabled->toBool()) {
-                // Enginebuffer will quantize more exactly than we can.
-                seekAbs(mainCuePosition);
+                const auto newCuePosition = mixxx::audio::FramePos::fromEngineSamplePosMaybeInvalid(
+                        m_pCuePoint->get());
+                if (newCuePosition.isValid()) {
+                    // Enginebuffer will quantize more exactly than we can.
+                    seekAbs(newCuePosition);
+                }
             }
         }
     } else if (trackAt == TrackAt::Cue) {

diff --git a/src/engine/controls/cuecontrol.h b/src/engine/controls/cuecontrol.h
@@ -1,5 +1,7 @@
 #pragma once
 
+#include <gtest/gtest_prod.h>
+
 #include <QAtomicInt>
 #include <QAtomicPointer>
 #include <QList>
@@ -236,8 +238,10 @@ class CueControl : public EngineControl {
     void cueGotoAndPlay(double v);
     void cueGotoAndStop(double v);
     void cuePreview(double v);
+    FRIEND_TEST(CueControlTest, SeekOnSetCueCDJ);
     void cueCDJ(double v);
     void cueDenon(double v);
+    FRIEND_TEST(CueControlTest, SeekOnSetCuePlay);
     void cuePlay(double v);
     void cueDefault(double v);
     void pause(double v);

diff --git a/src/engine/enginebuffer.h b/src/engine/enginebuffer.h
@@ -266,6 +266,8 @@ class EngineBuffer : public EngineObject {
     BpmControl* m_pBpmControl;
     KeyControl* m_pKeyControl;
     ClockControl* m_pClockControl;
+    FRIEND_TEST(CueControlTest, SeekOnSetCueCDJ);
+    FRIEND_TEST(CueControlTest, SeekOnSetCuePlay);
     CueControl* m_pCueControl;
 
     QList<EngineControl*> m_engineControls;

diff --git a/src/test/cuecontrol_test.cpp b/src/test/cuecontrol_test.cpp
@@ -402,6 +402,66 @@ TEST_F(CueControlTest, FollowCueOnQuantize) {
     EXPECT_FRAMEPOS_EQ(mixxx::audio::kStartFramePos, getCurrentFramePos());
 }
 
+TEST_F(CueControlTest, SeekOnSetCueCDJ) {
+    // Regression test for https://bugs.launchpad.net/mixxx/+bug/1946415
+    config()->set(ConfigKey("[Controls]", "CueRecall"),
+            ConfigValue(static_cast<int>(SeekOnLoadMode::MainCue)));
+    m_pQuantizeEnabled->set(1);
+    TrackPointer pTrack = createTestTrack();
+    pTrack->trySetBpm(120.0);
+
+    const int sampleRate = pTrack->getSampleRate();
+    const double bpm = pTrack->getBpm();
+    const mixxx::audio::FrameDiff_t beatLengthFrames = (60.0 * sampleRate / bpm);
+    const auto cuePos = mixxx::audio::FramePos(10 * beatLengthFrames);
+    pTrack->setMainCuePosition(cuePos);
+
+    loadTrack(pTrack);
+
+    EXPECT_FRAMEPOS_EQ_CONTROL(cuePos, m_pCuePoint);
+    EXPECT_FRAMEPOS_EQ(cuePos, getCurrentFramePos());
+
+    // Change the playpos and move the cue point there.
+    const auto newCuePos = mixxx::audio::FramePos(2.0 * beatLengthFrames);
+    setCurrentFramePos(newCuePos);
+    m_pChannel1->getEngineBuffer()->m_pCueControl->cueCDJ(1.0);
+    ProcessBuffer();
+
+    // Cue point and playpos should be at the same position.
+    EXPECT_FRAMEPOS_EQ_CONTROL(newCuePos, m_pCuePoint);
+    EXPECT_FRAMEPOS_EQ(newCuePos, getCurrentFramePos());
+}
+
+TEST_F(CueControlTest, SeekOnSetCuePlay) {
+    // Regression test for https://bugs.launchpad.net/mixxx/+bug/1946415
+    config()->set(ConfigKey("[Controls]", "CueRecall"),
+            ConfigValue(static_cast<int>(SeekOnLoadMode::MainCue)));
+    m_pQuantizeEnabled->set(1);
+    TrackPointer pTrack = createTestTrack();
+    pTrack->trySetBpm(120.0);
+
+    const int sampleRate = pTrack->getSampleRate();
+    const double bpm = pTrack->getBpm();
+    const mixxx::audio::FrameDiff_t beatLengthFrames = (60.0 * sampleRate / bpm);
+    const auto cuePos = mixxx::audio::FramePos(10 * beatLengthFrames);
+    pTrack->setMainCuePosition(cuePos);
+
+    loadTrack(pTrack);
+
+    EXPECT_FRAMEPOS_EQ_CONTROL(cuePos, m_pCuePoint);
+    EXPECT_FRAMEPOS_EQ(cuePos, getCurrentFramePos());
+
+    // Change the playpos and do a cuePlay.
+    const auto newCuePos = mixxx::audio::FramePos(2.0 * beatLengthFrames);
+    setCurrentFramePos(newCuePos);
+    m_pChannel1->getEngineBuffer()->m_pCueControl->cuePlay(1.0);
+    ProcessBuffer();
+
+    // Cue point and playpos should be at the same position.
+    EXPECT_FRAMEPOS_EQ_CONTROL(newCuePos, m_pCuePoint);
+    EXPECT_FRAMEPOS_EQ(newCuePos, getCurrentFramePos());
+}
+
 TEST_F(CueControlTest, IntroCue_SetStartEnd_ClearStartEnd) {
     TrackPointer pTrack = createAndLoadFakeTrack();