Macros: Migrate to FramePos

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,26 @@ 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 +218,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 +323,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,20 @@ 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());
  }
 };