Macros: Migrate to FramePos

Author: xeruf

src/engine/controls/enginecontrol.h

@@ -41,17 +41,17 @@ class EngineControl : public QObject {
             UserSettingsPointer pConfig);
     ~EngineControl() override;
 
-    // Called by EngineBuffer::process every latency period. See the above
-    // comments for information about guarantees that hold during this call. An
-    // EngineControl can perform any upkeep operations that are necessary during
-    // this call.
+    /// Called by EngineBuffer::process every latency period.
+    /// See the above comments for information about guarantees that hold during this call.
+    /// An EngineControl can perform any upkeep operations necessary here.
+    /// @param dRate current playback rate in audio frames per second
     virtual void process(const double dRate,
             mixxx::audio::FramePos currentPosition,
             const int iBufferSize);
 
-    // hintReader allows the EngineControl to provide hints to the reader to
-    // indicate that the given portion of a song is a potential imminent seek
-    // target.
+    /// hintReader allows the EngineControl to provide hints to the reader
+    /// to indicate that the given portion of a song
+    /// is a potential imminent seek target.
     virtual void hintReader(HintVector* pHintList);
 
     virtual void setEngineMaster(EngineMaster* pEngineMaster);
@@ -66,7 +66,7 @@ class EngineControl : public QObject {
             mixxx::audio::FramePos endPosition,
             bool enabled);
 
-    // Called to collect player features for effects processing.
+    /// Collect player features for effects processing.
     virtual void collectFeatureState(GroupFeatureState* pGroupFeatures) const {
         Q_UNUSED(pGroupFeatures);
     }
@@ -96,10 +96,10 @@ class EngineControl : public QObject {
     }
     void seek(double fractionalPosition);
     void seekAbs(mixxx::audio::FramePos position);
-    /// Seek to an exact sample, no quantizing
-    /// virtual only for testing!
+    /// Seek to an exact frame, no quantizing
+    /// virtual only for tests!
     virtual void seekExact(mixxx::audio::FramePos position);
-    // Returns an EngineBuffer to target for syncing. Returns nullptr if none found
+    /// Return an EngineBuffer to target for syncing. Returns nullptr if none found.
     EngineBuffer* pickSyncTarget();
 
     UserSettingsPointer getConfig();

src/engine/controls/macrocontrol.cpp

@@ -76,28 +76,28 @@ MacroControl::MacroControl(const QString& group, UserSettingsPointer pConfig, in
             &MacroControl::slotClear);
 }
 
-void MacroControl::process(const double dRate, const double dCurrentSample, const int iBufferSize) {
+void MacroControl::process(const double dRate,
+        mixxx::audio::FramePos currentPosition,
+        const int iBufferSize) {
     Q_UNUSED(dRate);
-    if (m_queuedJumpTarget >= 0) {
+    if (m_queuedJumpTarget.value() >= 0) {
         // if a cue press doesn't change the position, notifySeek isn't called, thus m_queuedJumpTarget isn't reset
         if (getStatus() == Status::Armed) {
             // start the recording on a cue press even when there is no jump
-            notifySeek(dCurrentSample);
+            notifySeek(currentPosition);
         }
-        m_queuedJumpTarget = -1;
+        m_queuedJumpTarget = mixxx::audio::FramePos(-1);
     }
     if (getStatus() != Status::Playing) {
         return;
     }
-    double framePos = dCurrentSample / mixxx::kEngineChannelCount;
     const MacroAction& nextAction = m_pMacro->getActions().at(m_iNextAction);
-    double nextActionPos = nextAction.getSourcePosition();
-    int bufFrames = iBufferSize / 2;
-    // The process method is called roughly every iBufferSize/2 samples, the
+    // The process method is called roughly every iBufferSize frames, the
     // tolerance range is double that to be safe. It is ahead of the position
     // because the seek is executed in the next EngineBuffer process cycle.
-    if (framePos > nextActionPos - bufFrames && framePos < nextActionPos + bufFrames) {
-        seekExact(nextAction.getTargetPositionSample());
+    if (currentPosition > nextAction.getSourcePosition() - iBufferSize &&
+            currentPosition < nextAction.getSourcePosition() + iBufferSize) {
+        seekExact(nextAction.getTargetPosition());
         m_iNextAction++;
         if (m_iNextAction == m_pMacro->size()) {
             if (m_pMacro->isLooped()) {
@@ -134,26 +134,24 @@ void MacroControl::trackLoaded(TrackPointer pNewTrack) {
     }
 }
 
-void MacroControl::notifySeek(double dNewPlaypos) {
-    if (m_queuedJumpTarget < 0) {
+void MacroControl::notifySeek(mixxx::audio::FramePos position) {
+    if (m_queuedJumpTarget.value() < 0) {
         return;
     }
-    double originalDestFramePos = m_queuedJumpTarget;
-    m_queuedJumpTarget = -1;
+    auto queuedTarget = m_queuedJumpTarget;
+    m_queuedJumpTarget = mixxx::audio::FramePos(-1);
     if (getStatus() == Status::Armed) {
         setStatus(Status::Recording);
     }
     if (getStatus() != Status::Recording) {
         return;
     }
-    double sourceFramePos = getSampleOfTrack().current / mixxx::kEngineChannelCount;
-    double destFramePos = dNewPlaypos / mixxx::kEngineChannelCount;
-    double diff = originalDestFramePos - destFramePos;
-    m_recordedActions.try_emplace(sourceFramePos + diff, originalDestFramePos);
+    // Account for quantization so the jump stays in-phase but snaps exactly to the original target
+    m_recordedActions.try_emplace(frameInfo().currentPosition + (queuedTarget - position), queuedTarget);
 }
 
-void MacroControl::slotJumpQueued(double samplePos) {
-    m_queuedJumpTarget = samplePos / mixxx::kEngineChannelCount;
+void MacroControl::slotJumpQueued(mixxx::audio::FramePos samplePos) {
+    m_queuedJumpTarget = samplePos;
 }
 
 MacroControl::Status MacroControl::getStatus() const {
@@ -218,13 +216,12 @@ bool MacroControl::stopRecording() {
     } else {
         // This will still be the position of the previous track when called from trackLoaded
         // since trackLoaded is invoked before the SampleOfTrack of the controls is updated.
-        m_pMacro->setEnd(getSampleOfTrack().current / mixxx::kEngineChannelCount);
+        m_pMacro->setEnd(frameInfo().currentPosition);
         if (m_pMacro->getLabel().isEmpty()) {
-            // Automatically set the start position in seconds as label if there
-            // is no user-defined one
-            double secPos = m_pMacro->getStartSamplePos() /
-                    mixxx::kEngineChannelCount / getSampleOfTrack().rate;
-            m_pMacro->setLabel(QString::number(secPos, 'f', 2));
+            // Automatically set the start position in seconds as label
+            // if there is no user-defined one
+            auto secPos = m_pMacro->getStart() / frameInfo().sampleRate;
+            m_pMacro->setLabel(QString::number(secPos.value(), 'f', 2));
         }
         setStatus(Status::Recorded);
         if (m_pMacro->isEnabled()) {
@@ -324,7 +321,7 @@ void MacroControl::slotToggle(double value) {
 
 void MacroControl::slotGotoPlay(double value) {
     if (value > 0 && getStatus() > Status::Recording) {
-        seekExact(m_pMacro->getStartSamplePos());
+        seekExact(m_pMacro->getStart());
         play();
     }
 }

src/engine/controls/macrocontrol.h

@@ -14,8 +14,10 @@ class MacroControl : public EngineControl {
     MacroControl(const QString& group, UserSettingsPointer pConfig, int slot);
 
     void trackLoaded(TrackPointer pNewTrack) override;
-    void process(const double dRate, const double dCurrentSample, const int iBufferSize) override;
-    void notifySeek(double dNewPlaypos) override;
+    void process(const double dRate,
+            mixxx::audio::FramePos currentPosition,
+            const int iBufferSize) override;
+    void notifySeek(mixxx::audio::FramePos position) override;
 
     bool isRecording() const;
 
@@ -42,7 +44,7 @@ class MacroControl : public EngineControl {
     void slotToggle(double value = 1);
     void slotClear(double value = 1);
 
-    void slotJumpQueued(double samplePos);
+    void slotJumpQueued(mixxx::audio::FramePos samplePos);
 
   private:
     /// Returns whether a new action was recorded
@@ -60,7 +62,7 @@ class MacroControl : public EngineControl {
     QString m_controlPattern;
 
     /// The unquantized FramePos of a jump that is yet to be processed
-    double m_queuedJumpTarget;
+    mixxx::audio::FramePos m_queuedJumpTarget;
 
     rigtorp::SPSCQueue<MacroAction> m_recordedActions;
     QTimer m_updateRecordingTimer;

src/engine/enginebuffer.cpp

@@ -44,20 +44,20 @@
 namespace {
 const mixxx::Logger kLogger("EngineBuffer");
 
-// This value is used to make sure the initial seek after loading a track is
-// not omitted. Therefore this value must be different for 0.0 or any likely
-// value for the main cue
+/// Used to ensure the initial seek after loading a track is not omitted.
+/// Therefore this value must stray away from 0.0 or any probably value for the main cue.
 constexpr auto kInitialPlayPosition =
         mixxx::audio::FramePos::fromEngineSamplePos(
                 std::numeric_limits<double>::lowest());
 
-constexpr double kLinearScalerElipsis =
-        1.00058; // 2^(0.01/12): changes < 1 cent allows a linear scaler
+/// 2^(0.01/12): changes < 1 cent allows a linear scaler
+constexpr double kLinearScalerElipsis = 1.00058;
 
-constexpr SINT kSamplesPerFrame = 2; // Engine buffer uses Stereo frames only
+/// Engine buffer uses Stereo frames only
+constexpr SINT kSamplesPerFrame = 2;
 
-// Rate at which the playpos slider is updated
-constexpr int kPlaypositionUpdateRate = 15; // updates per second
+/// Updates per second of the playpos slider
+constexpr int kPlaypositionUpdateRate = 15;
 
 } // anonymous namespace
 
@@ -999,8 +999,8 @@ void EngineBuffer::processTrackLocked(
         // The way we treat rate inside of EngineBuffer is actually a
         // description of "sample consumption rate" or percentage of samples
         // consumed relative to playing back the track at its native sample
-        // rate and normal speed. pitch_adjust does not change the playback
-        // rate.
+        // rate and normal speed.
+        // pitch_adjust does not change the playback rate.
         rate = baserate * speed;
 
         // Scaler is up to date now.

src/engine/enginebuffer.h

@@ -64,13 +64,13 @@ class EngineBuffer : public EngineObject {
         SEEK_PHASE = 1u,
         /// Bypass Quantization
         SEEK_EXACT = 2u,
-        /// This is an artificial state that happens if an exact seek and a
-        /// phase seek are scheduled at the same time.
+        /// This artificial state occurs when an exact seek and a phase seek
+        /// are scheduled at the same time.
         SEEK_EXACT_PHASE = SEEK_PHASE | SEEK_EXACT,
-        /// #SEEK_PHASE if Quantize enables, otherwise SEEK_EXACT
+        /// SEEK_PHASE if Quantization is enabled, otherwise SEEK_EXACT
         SEEK_STANDARD = 4u,
-        /// This is an artificial state that happens if a standard seek and a
-        /// phase seek are scheduled at the same time.
+        /// This artificial state occurs when a standard seek and a phase seek
+        /// are scheduled at the same time.
         SEEK_STANDARD_PHASE = SEEK_STANDARD | SEEK_PHASE,
     };
     Q_DECLARE_FLAGS(SeekRequests, SeekRequest);
@@ -185,7 +185,7 @@ class EngineBuffer : public EngineObject {
   signals:
     void trackLoaded(TrackPointer pNewTrack, TrackPointer pOldTrack);
     void trackLoadFailed(TrackPointer pTrack, const QString& reason);
-    void cueJumpQueued(double samplePos);
+    void cueJumpQueued(mixxx::audio::FramePos targetPosition);
 
   private slots:
     void slotTrackLoading();

src/test/macros/macro_test.cpp

@@ -3,7 +3,7 @@
 #include <gtest/gtest.h>
 
 TEST(MacroTest, SerializeMacroActions) {
-    QList<MacroAction> actions{MacroAction(0, 1)};
+    QList<MacroAction> actions{MacroAction(mixxx::audio::FramePos(0), mixxx::audio::FramePos(1))};
 
     QString filename(QDir::currentPath() % "/src/test/macros/macro_proto");
     ASSERT_TRUE(QFile::exists(filename));

src/test/macros/macro_test.h

@@ -11,16 +11,16 @@ struct TestMacro {
     MacroAction action;
 
     TestMacro(double sourceFramePos = 25'000, double targetFramePos = 7'500)
-            : action(sourceFramePos, targetFramePos) {
+            : action(mixxx::audio::FramePos(sourceFramePos), mixxx::audio::FramePos(targetFramePos)) {
     }
 
     /// Checks that the recorded Macro corresponds to the parameters,
     /// including the extra first loop action.
     void checkMacroAction(MacroPointer macro) {
         ASSERT_EQ(macro->size(), 2);
-        EXPECT_EQ(macro->getActions().last().getSourcePosition(), action.getSourcePosition());
-        EXPECT_EQ(macro->getActions().last().getTargetPosition(), action.getTargetPosition());
+        EXPECT_EQ(macro->getActions().last().getSourcePosition().value(), action.getSourcePosition().value());
+        EXPECT_EQ(macro->getActions().last().getTargetPosition().value(), action.getTargetPosition().value());
         // Loopback action
-        EXPECT_EQ(macro->getActions().first().getSourcePosition(), action.getTargetPosition());
+        EXPECT_EQ(macro->getActions().first().getSourcePosition().value(), action.getTargetPosition().value());
     }
 };

src/test/macros/macrocontrol_test.cpp

@@ -20,7 +20,7 @@ class MacroControlTest : public MacroControl, public testing::Test {
         EXPECT_FALSE(isRecording());
     }
 
-    MOCK_METHOD1(seekExact, void(double));
+    MOCK_METHOD1(seekExact, void(mixxx::audio::FramePos));
 
     const TrackPointer pLoadedTrack = Track::newTemporary();
 };
@@ -39,37 +39,37 @@ TEST_F(MacroControlTest, RecordSeek) {
     EXPECT_TRUE(isRecording());
     EXPECT_EQ(getMacro()->getLabel(), " [Recording]");
     // Prepare recording
-    int frameRate = 1'000;
-    auto seek = [this, frameRate](double position) {
+    mixxx::audio::SampleRate frameRate(1'000);
+    auto seek = [this, frameRate](mixxx::audio::FramePos position) {
         notifySeek(position);
-        setCurrentSample(position, 99'000, frameRate);
-        process(0, position, 2);
+        setFrameInfo(position, mixxx::audio::FramePos(99'000), frameRate);
+        process(frameRate, position, 1);
     };
-    seek(0);
+    seek(mixxx::audio::FramePos(0));
     ASSERT_EQ(getStatus(), MacroControl::Status::Armed);
 
     // Initial jump
-    double startPos = 1'008;
+    mixxx::audio::FramePos startPos(504);
     slotJumpQueued(startPos);
     seek(startPos);
     // Jump with quantization adjustment
     TestMacro testMacro;
-    double diff = 20;
-    seek(testMacro.action.getSourcePositionSample() - diff);
-    slotJumpQueued(testMacro.action.getTargetPositionSample());
-    seek(testMacro.action.getTargetPositionSample() - diff);
+    mixxx::audio::FrameDiff_t diff(10);
+    seek(testMacro.action.getSourcePosition() - diff);
+    slotJumpQueued(testMacro.action.getTargetPosition());
+    seek(testMacro.action.getTargetPosition() - diff);
 
     // Stop recording
-    seek(testMacro.action.getTargetPositionSample());
+    seek(testMacro.action.getTargetPosition());
     EXPECT_CALL(*this, seekExact(startPos));
     slotRecord(0);
     EXPECT_EQ(getStatus(), MacroControl::Status::Playing);
     // Check recording result
     testMacro.checkMacroAction(getMacro());
-    EXPECT_EQ(getMacro()->getActions().first().getTargetPositionSample(), startPos);
+    EXPECT_EQ(getMacro()->getActions().first().getTargetPosition(), startPos);
     EXPECT_TRUE(pLoadedTrack->isDirty());
     // Check generated label
-    EXPECT_EQ(startPos / mixxx::kEngineChannelCount / frameRate, 0.504);
+    EXPECT_EQ((startPos / frameRate).value(), 0.504);
     EXPECT_EQ(getMacro()->getLabel().toStdString(), "0.50");
     // Activate
     EXPECT_CALL(*this, seekExact(startPos));
@@ -107,16 +107,16 @@ TEST_F(MacroControlTest, ControlObjects) {
     ControlProxy(kChannelGroup, "macro_2_enable").set(0);
     ControlProxy(kChannelGroup, "macro_2_loop").set(1);
     EXPECT_TRUE(getMacro()->isLooped());
-    slotJumpQueued(0);
-    notifySeek(0);
+    slotJumpQueued(mixxx::audio::FramePos(0));
+    notifySeek(mixxx::audio::FramePos(0));
     activate.set(1);
     ASSERT_STATUS(MacroControl::Status::Recorded);
     EXPECT_EQ(getMacro()->getLabel(), label);
     activate.set(1);
     ASSERT_STATUS(MacroControl::Status::Playing);
 
     // Restart
-    EXPECT_CALL(*this, seekExact(0));
+    EXPECT_CALL(*this, seekExact(mixxx::audio::FramePos(0)));
     activate.set(1);
     ASSERT_STATUS(MacroControl::Status::Playing);
 
@@ -131,7 +131,7 @@ TEST_F(MacroControlTest, ControlObjects) {
 }
 
 TEST_F(MacroControlTest, LoadTrackAndPlayAndClear) {
-    MacroAction jumpAction(40'000, 0);
+    MacroAction jumpAction(mixxx::audio::FramePos(40'000), mixxx::audio::FramePos(0));
     TestMacro testMacro;
 
     QString label = QStringLiteral("test");
@@ -146,23 +146,23 @@ TEST_F(MacroControlTest, LoadTrackAndPlayAndClear) {
     slotActivate();
     EXPECT_EQ(getStatus(), MacroControl::Status::Playing);
     EXPECT_CALL(*this, seekExact(jumpAction.getTargetPosition()));
-    process(0, jumpAction.getSourcePositionSample(), 2);
+    process(0, jumpAction.getSourcePosition(), 2);
     EXPECT_EQ(getStatus(), MacroControl::Status::Recorded);
 
     // LOOP
     slotLoop(1);
-    EXPECT_CALL(*this, seekExact(testMacro.action.getTargetPositionSample()));
+    EXPECT_CALL(*this, seekExact(testMacro.action.getTargetPosition()));
     slotActivate();
     slotActivate();
     // Jump
-    EXPECT_CALL(*this, seekExact(jumpAction.getTargetPositionSample()));
-    process(0, jumpAction.getSourcePositionSample(), 2);
+    EXPECT_CALL(*this, seekExact(jumpAction.getTargetPosition()));
+    process(0, jumpAction.getSourcePosition(), 2);
     // Loop back
-    EXPECT_CALL(*this, seekExact(testMacro.action.getTargetPositionSample()));
-    process(0, testMacro.action.getSourcePositionSample(), 2);
+    EXPECT_CALL(*this, seekExact(testMacro.action.getTargetPosition()));
+    process(0, testMacro.action.getSourcePosition(), 2);
     // Jump again
-    EXPECT_CALL(*this, seekExact(jumpAction.getTargetPositionSample()));
-    process(0, jumpAction.getSourcePositionSample(), 2);
+    EXPECT_CALL(*this, seekExact(jumpAction.getTargetPosition()));
+    process(0, jumpAction.getSourcePosition(), 2);
     EXPECT_EQ(getStatus(), MacroControl::Status::Playing);
 
     // Stop playing