From d888acdf92603fc6071e21415b243d29fe19ac99 Mon Sep 17 00:00:00 2001 From: Jan Holthuis Date: Wed, 7 Jul 2021 21:58:42 +0200 Subject: [PATCH 1/3] Sync: Use mixxx::Bpm class --- src/engine/enginebuffer.cpp | 6 +-- src/engine/sync/clock.h | 5 ++- src/engine/sync/enginesync.cpp | 46 ++++++++++--------- src/engine/sync/enginesync.h | 16 +++---- src/engine/sync/internalclock.cpp | 51 ++++++++++----------- src/engine/sync/internalclock.h | 16 +++---- src/engine/sync/syncable.h | 20 +++++---- src/engine/sync/synccontrol.cpp | 75 +++++++++++++++++++------------ src/engine/sync/synccontrol.h | 19 ++++---- src/test/enginesynctest.cpp | 4 +- src/test/synccontroltest.cpp | 12 +++-- 11 files changed, 152 insertions(+), 118 deletions(-) diff --git a/src/engine/enginebuffer.cpp b/src/engine/enginebuffer.cpp index 2158b165fe7..bd0c0824ab0 100644 --- a/src/engine/enginebuffer.cpp +++ b/src/engine/enginebuffer.cpp @@ -1317,11 +1317,11 @@ void EngineBuffer::postProcess(const int iBufferSize) { const mixxx::Bpm localBpm = m_pBpmControl->updateLocalBpm(); double beatDistance = m_pBpmControl->updateBeatDistance(); // FIXME: Double check if calling setLocalBpm with an invalid value is correct and intended. - double localBpmValue = mixxx::Bpm::kValueUndefined; + mixxx::Bpm newLocalBpm; if (localBpm.isValid()) { - localBpmValue = localBpm.value(); + newLocalBpm = localBpm; } - m_pSyncControl->setLocalBpm(localBpmValue); + m_pSyncControl->setLocalBpm(newLocalBpm); m_pSyncControl->updateAudible(); SyncMode mode = m_pSyncControl->getSyncMode(); if (isLeader(mode)) { diff --git a/src/engine/sync/clock.h b/src/engine/sync/clock.h index a0e8731fe51..06e1371b2b9 100644 --- a/src/engine/sync/clock.h +++ b/src/engine/sync/clock.h @@ -1,4 +1,5 @@ #pragma once +#include "track/bpm.h" class Clock { public: @@ -7,6 +8,6 @@ class Clock { virtual double getBeatDistance() const = 0; virtual void updateLeaderBeatDistance(double beatDistance) = 0; - virtual double getBpm() const = 0; - virtual void updateLeaderBpm(double bpm) = 0; + virtual mixxx::Bpm getBpm() const = 0; + virtual void updateLeaderBpm(mixxx::Bpm bpm) = 0; }; diff --git a/src/engine/sync/enginesync.cpp b/src/engine/sync/enginesync.cpp index d82dfb28668..6b90ab05661 100644 --- a/src/engine/sync/enginesync.cpp +++ b/src/engine/sync/enginesync.cpp @@ -12,6 +12,7 @@ namespace { const mixxx::Logger kLogger("EngineSync"); const QString kInternalClockGroup = QStringLiteral("[InternalClock]"); +constexpr mixxx::Bpm kDefaultBpm = mixxx::Bpm(124.0); } // anonymous namespace EngineSync::EngineSync(UserSettingsPointer pConfig) @@ -19,12 +20,15 @@ EngineSync::EngineSync(UserSettingsPointer pConfig) m_pInternalClock(new InternalClock(kInternalClockGroup, this)), m_pLeaderSyncable(nullptr) { qRegisterMetaType("SyncMode"); - m_pInternalClock->updateLeaderBpm(124.0); + m_pInternalClock->updateLeaderBpm(kDefaultBpm); } EngineSync::~EngineSync() { // We use the slider value because that is never set to 0.0. - m_pConfig->set(ConfigKey(kInternalClockGroup, "bpm"), ConfigValue(m_pInternalClock->getBpm())); + const mixxx::Bpm bpm = m_pInternalClock->getBpm(); + m_pConfig->set(ConfigKey(kInternalClockGroup, "bpm"), + ConfigValue( + bpm.isValid() ? bpm.value() : mixxx::Bpm::kValueUndefined)); delete m_pInternalClock; } @@ -45,7 +49,7 @@ void EngineSync::requestSyncMode(Syncable* pSyncable, SyncMode mode) { switch (mode) { case SyncMode::LeaderExplicit: case SyncMode::LeaderSoft: { - if (pSyncable->getBaseBpm() > 0) { + if (pSyncable->getBaseBpm().isValid()) { activateLeader(pSyncable, mode); } else { // Because we don't have a valid bpm, we can't be the leader @@ -206,7 +210,7 @@ Syncable* EngineSync::pickLeader(Syncable* enabling_syncable) { } if (m_pLeaderSyncable && m_pLeaderSyncable->getSyncMode() == SyncMode::LeaderExplicit && - m_pLeaderSyncable->getBaseBpm() != 0.0) { + m_pLeaderSyncable->getBaseBpm().isValid()) { return m_pLeaderSyncable; } @@ -223,7 +227,7 @@ Syncable* EngineSync::pickLeader(Syncable* enabling_syncable) { int playing_deck_count = 0; for (const auto& pSyncable : qAsConst(m_syncables)) { - if (pSyncable->getBaseBpm() <= 0.0) { + if (!pSyncable->getBaseBpm().isValid()) { continue; } @@ -309,7 +313,7 @@ Syncable* EngineSync::findBpmMatchTarget(Syncable* requester) { if (!pOtherSyncable->getChannel()->isPrimaryDeck()) { continue; } - if (pOtherSyncable->getBaseBpm() == 0.0) { + if (!pOtherSyncable->getBaseBpm().isValid()) { continue; } @@ -382,7 +386,7 @@ void EngineSync::notifyScratching(Syncable* pSyncable, bool scratching) { Q_UNUSED(scratching); } -void EngineSync::notifyBaseBpmChanged(Syncable* pSyncable, double bpm) { +void EngineSync::notifyBaseBpmChanged(Syncable* pSyncable, mixxx::Bpm bpm) { if (kLogger.traceEnabled()) { kLogger.trace() << "EngineSync::notifyBaseBpmChanged" << pSyncable->getGroup() << bpm; } @@ -392,7 +396,7 @@ void EngineSync::notifyBaseBpmChanged(Syncable* pSyncable, double bpm) { } } -void EngineSync::notifyRateChanged(Syncable* pSyncable, double bpm) { +void EngineSync::notifyRateChanged(Syncable* pSyncable, mixxx::Bpm bpm) { if (kLogger.traceEnabled()) { kLogger.trace() << "EngineSync::notifyRateChanged" << pSyncable->getGroup() << bpm; } @@ -400,13 +404,13 @@ void EngineSync::notifyRateChanged(Syncable* pSyncable, double bpm) { updateLeaderBpm(pSyncable, bpm); } -void EngineSync::requestBpmUpdate(Syncable* pSyncable, double bpm) { +void EngineSync::requestBpmUpdate(Syncable* pSyncable, mixxx::Bpm bpm) { if (kLogger.traceEnabled()) { kLogger.trace() << "EngineSync::requestBpmUpdate" << pSyncable->getGroup() << bpm; } - double mbaseBpm = 0.0; - double mbpm = 0.0; + mixxx::Bpm mbaseBpm; + mixxx::Bpm mbpm; double beatDistance = 0.0; if (m_pLeaderSyncable) { mbaseBpm = m_pLeaderSyncable->getBaseBpm(); @@ -414,7 +418,7 @@ void EngineSync::requestBpmUpdate(Syncable* pSyncable, double bpm) { beatDistance = m_pLeaderSyncable->getBeatDistance(); } - if (mbaseBpm != 0.0) { + if (mbaseBpm.isValid()) { // update from current leader pSyncable->updateLeaderBeatDistance(beatDistance); pSyncable->updateLeaderBpm(mbpm); @@ -425,7 +429,7 @@ void EngineSync::requestBpmUpdate(Syncable* pSyncable, double bpm) { } } -void EngineSync::notifyInstantaneousBpmChanged(Syncable* pSyncable, double bpm) { +void EngineSync::notifyInstantaneousBpmChanged(Syncable* pSyncable, mixxx::Bpm bpm) { if (kLogger.traceEnabled()) { kLogger.trace() << "EngineSync::notifyInstantaneousBpmChanged" << pSyncable->getGroup() << bpm; } @@ -546,14 +550,14 @@ Syncable* EngineSync::getSyncableForGroup(const QString& group) { bool EngineSync::syncDeckExists() const { for (const auto& pSyncable : qAsConst(m_syncables)) { - if (pSyncable->isSynchronized() && pSyncable->getBaseBpm() > 0) { + if (pSyncable->isSynchronized() && pSyncable->getBaseBpm().isValid()) { return true; } } return false; } -double EngineSync::leaderBpm() const { +mixxx::Bpm EngineSync::leaderBpm() const { if (m_pLeaderSyncable) { return m_pLeaderSyncable->getBpm(); } @@ -567,14 +571,14 @@ double EngineSync::leaderBeatDistance() const { return m_pInternalClock->getBeatDistance(); } -double EngineSync::leaderBaseBpm() const { +mixxx::Bpm EngineSync::leaderBaseBpm() const { if (m_pLeaderSyncable) { return m_pLeaderSyncable->getBaseBpm(); } return m_pInternalClock->getBaseBpm(); } -void EngineSync::updateLeaderBpm(Syncable* pSource, double bpm) { +void EngineSync::updateLeaderBpm(Syncable* pSource, mixxx::Bpm bpm) { //qDebug() << "EngineSync::updateLeaderBpm" << pSource << bpm; if (pSource != m_pInternalClock) { m_pInternalClock->updateLeaderBpm(bpm); @@ -588,7 +592,7 @@ void EngineSync::updateLeaderBpm(Syncable* pSource, double bpm) { } } -void EngineSync::updateLeaderInstantaneousBpm(Syncable* pSource, double bpm) { +void EngineSync::updateLeaderInstantaneousBpm(Syncable* pSource, mixxx::Bpm bpm) { if (pSource != m_pInternalClock) { m_pInternalClock->updateInstantaneousBpm(bpm); } @@ -648,9 +652,9 @@ void EngineSync::reinitLeaderParams(Syncable* pSource) { beatDistance = m_pInternalClock->getBeatDistance(); } } - const double baseBpm = pSource->getBaseBpm(); - double bpm = pSource->getBpm(); - if (bpm <= 0) { + const mixxx::Bpm baseBpm = pSource->getBaseBpm(); + mixxx::Bpm bpm = pSource->getBpm(); + if (!bpm.isValid()) { bpm = baseBpm; } if (kLogger.traceEnabled()) { diff --git a/src/engine/sync/enginesync.h b/src/engine/sync/enginesync.h index 22018ae7727..b656aa12a75 100644 --- a/src/engine/sync/enginesync.h +++ b/src/engine/sync/enginesync.h @@ -25,13 +25,13 @@ class EngineSync : public SyncableListener { /// Syncables notify EngineSync directly about various events. EngineSync /// does not have a say in whether these succeed or not, they are simply /// notifications. - void notifyBaseBpmChanged(Syncable* pSyncable, double bpm) override; - void notifyRateChanged(Syncable* pSyncable, double bpm) override; - void requestBpmUpdate(Syncable* pSyncable, double bpm) override; + void notifyBaseBpmChanged(Syncable* pSyncable, mixxx::Bpm bpm) override; + void notifyRateChanged(Syncable* pSyncable, mixxx::Bpm bpm) override; + void requestBpmUpdate(Syncable* pSyncable, mixxx::Bpm bpm) override; /// Instantaneous BPM refers to the actual, honest-to-god speed of playback /// at any moment, including any scratching that may be happening. - void notifyInstantaneousBpmChanged(Syncable* pSyncable, double bpm) override; + void notifyInstantaneousBpmChanged(Syncable* pSyncable, mixxx::Bpm bpm) override; /// the beat distance is updated on every callback. void notifyBeatDistanceChanged(Syncable* pSyncable, double beatDistance) override; @@ -91,7 +91,7 @@ class EngineSync : public SyncableListener { /// Return the current BPM of the Leader Syncable. If no Leader syncable is /// set then returns the BPM of the internal clock. - double leaderBpm() const; + mixxx::Bpm leaderBpm() const; /// Returns the current beat distance of the Leader Syncable. If no Leader /// Syncable is set, then returns the beat distance of the internal clock. @@ -100,14 +100,14 @@ class EngineSync : public SyncableListener { /// Returns the overall average BPM of the Leader Syncable if it were playing /// at 1.0 rate. This is used to calculate half/double multipliers and whether /// the Leader has a bpm at all. - double leaderBaseBpm() const; + mixxx::Bpm leaderBaseBpm() const; /// Set the BPM on every sync-enabled Syncable except pSource. - void updateLeaderBpm(Syncable* pSource, double bpm); + void updateLeaderBpm(Syncable* pSource, mixxx::Bpm bpm); /// Set the Leader instantaneous BPM on every sync-enabled Syncable except /// pSource. - void updateLeaderInstantaneousBpm(Syncable* pSource, double bpm); + void updateLeaderInstantaneousBpm(Syncable* pSource, mixxx::Bpm bpm); /// Set the Leader beat distance on every sync-enabled Syncable except /// pSource. diff --git a/src/engine/sync/internalclock.cpp b/src/engine/sync/internalclock.cpp index 9ef2355e7b8..aed7e8f210a 100644 --- a/src/engine/sync/internalclock.cpp +++ b/src/engine/sync/internalclock.cpp @@ -12,6 +12,7 @@ namespace { const mixxx::Logger kLogger("InternalClock"); +constexpr mixxx::Bpm kDefaultBpm(124.0); } // namespace InternalClock::InternalClock(const QString& group, SyncableListener* pEngineSync) @@ -19,9 +20,9 @@ InternalClock::InternalClock(const QString& group, SyncableListener* pEngineSync m_pEngineSync(pEngineSync), m_mode(SyncMode::None), m_iOldSampleRate(44100), - m_dOldBpm(124.0), - m_dBaseBpm(124.0), - m_dBeatLength(m_iOldSampleRate * 60.0 / m_dOldBpm), + m_oldBpm(kDefaultBpm), + m_baseBpm(kDefaultBpm), + m_dBeatLength(m_iOldSampleRate * 60.0 / m_oldBpm.value()), m_dClockPosition(0) { // Pick a wide range (1 to 200) and allow out of bounds sets. This lets you // map a soft-takeover MIDI knob to the leader BPM. This also creates bpm_up @@ -84,7 +85,7 @@ void InternalClock::slotSyncLeaderEnabledChangeRequest(double state) { return; } if (mode == SyncMode::None) { - m_dBaseBpm = m_dOldBpm; + m_baseBpm = m_oldBpm; } m_pEngineSync->requestSyncMode(this, SyncMode::LeaderExplicit); } else { @@ -115,26 +116,26 @@ void InternalClock::updateLeaderBeatDistance(double beatDistance) { m_pEngineSync->notifyBeatDistanceChanged(this, beatDistance); } -double InternalClock::getBaseBpm() const { - return m_dBaseBpm; +mixxx::Bpm InternalClock::getBaseBpm() const { + return m_baseBpm; } -double InternalClock::getBpm() const { - return m_pClockBpm->get(); +mixxx::Bpm InternalClock::getBpm() const { + return mixxx::Bpm(m_pClockBpm->get()); } -void InternalClock::updateLeaderBpm(double bpm) { +void InternalClock::updateLeaderBpm(mixxx::Bpm bpm) { if (kLogger.traceEnabled()) { kLogger.trace() << "InternalClock::setBpm" << bpm; } - if (bpm == 0) { + if (!bpm.isValid()) { return; } - m_pClockBpm->set(bpm); + m_pClockBpm->set(bpm.value()); updateBeatLength(m_iOldSampleRate, bpm); } -void InternalClock::updateInstantaneousBpm(double bpm) { +void InternalClock::updateInstantaneousBpm(mixxx::Bpm bpm) { if (kLogger.traceEnabled()) { kLogger.trace() << "InternalClock::setInstantaneousBpm" << bpm; } @@ -145,28 +146,28 @@ void InternalClock::updateInstantaneousBpm(double bpm) { void InternalClock::notifyLeaderParamSource() { } -void InternalClock::reinitLeaderParams(double beatDistance, double baseBpm, double bpm) { +void InternalClock::reinitLeaderParams(double beatDistance, mixxx::Bpm baseBpm, mixxx::Bpm bpm) { if (kLogger.traceEnabled()) { kLogger.trace() << "InternalClock::reinitLeaderParams" << beatDistance << baseBpm << bpm; } - if (bpm <= 0.0 || baseBpm <= 0.0) { + if (!bpm.isValid() || !baseBpm.isValid()) { return; } - m_dBaseBpm = baseBpm; + m_baseBpm = baseBpm; updateLeaderBpm(bpm); updateLeaderBeatDistance(beatDistance); } void InternalClock::slotBpmChanged(double bpm) { - m_dBaseBpm = bpm; - updateBeatLength(m_iOldSampleRate, m_dBaseBpm); + m_baseBpm = mixxx::Bpm(bpm); + updateBeatLength(m_iOldSampleRate, m_baseBpm); if (!isSynchronized()) { return; } // The internal clock doesn't have a rate slider, so treat // "base" bpm changes as rate changes -- this means the change will be // reflected in all synced decks. - m_pEngineSync->notifyRateChanged(this, m_dBaseBpm); + m_pEngineSync->notifyRateChanged(this, m_baseBpm); } void InternalClock::slotBeatDistanceChanged(double beatDistance) { @@ -176,8 +177,8 @@ void InternalClock::slotBeatDistanceChanged(double beatDistance) { updateLeaderBeatDistance(beatDistance); } -void InternalClock::updateBeatLength(int sampleRate, double bpm) { - if (m_iOldSampleRate == sampleRate && bpm == m_dOldBpm) { +void InternalClock::updateBeatLength(int sampleRate, mixxx::Bpm bpm) { + if (m_iOldSampleRate == sampleRate && bpm == m_oldBpm) { return; } @@ -193,13 +194,13 @@ void InternalClock::updateBeatLength(int sampleRate, double bpm) { // that last term is 1 over bpm. - if (qFuzzyCompare(bpm, 0)) { + if (!bpm.isValid()) { qDebug() << "WARNING: Leader bpm reported to be zero, internal clock guessing 124bpm"; m_dBeatLength = (sampleRate * 60.0) / 124.0; - m_dOldBpm = 124.0; + m_oldBpm = kDefaultBpm; } else { - m_dOldBpm = bpm; - m_dBeatLength = (sampleRate * 60.0) / bpm; + m_oldBpm = bpm; + m_dBeatLength = (sampleRate * 60.0) / bpm.value(); if (m_dBeatLength <= 0) { qDebug() << "WARNING: Tried to set samples per beat <=0"; m_dBeatLength = sampleRate; @@ -219,7 +220,7 @@ void InternalClock::onCallbackStart(int sampleRate, int bufferSize) { } void InternalClock::onCallbackEnd(int sampleRate, int bufferSize) { - updateBeatLength(sampleRate, m_pClockBpm->get()); + updateBeatLength(sampleRate, getBpm()); // 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 6c82a95d84b..c6150866697 100644 --- a/src/engine/sync/internalclock.h +++ b/src/engine/sync/internalclock.h @@ -50,12 +50,12 @@ class InternalClock : public QObject, public Clock, public Syncable { double getBeatDistance() const override; void updateLeaderBeatDistance(double beatDistance) override; - double getBaseBpm() const override; - void updateLeaderBpm(double bpm) override; + mixxx::Bpm getBaseBpm() const override; + void updateLeaderBpm(mixxx::Bpm bpm) override; void notifyLeaderParamSource() override; - double getBpm() const override; - void updateInstantaneousBpm(double bpm) override; - void reinitLeaderParams(double beatDistance, double baseBpm, double bpm) override; + mixxx::Bpm getBpm() const override; + void updateInstantaneousBpm(mixxx::Bpm bpm) override; + void reinitLeaderParams(double beatDistance, mixxx::Bpm baseBpm, mixxx::Bpm bpm) override; void onCallbackStart(int sampleRate, int bufferSize); void onCallbackEnd(int sampleRate, int bufferSize); @@ -66,7 +66,7 @@ class InternalClock : public QObject, public Clock, public Syncable { void slotSyncLeaderEnabledChangeRequest(double state); private: - void updateBeatLength(int sampleRate, double bpm); + void updateBeatLength(int sampleRate, mixxx::Bpm bpm); const QString m_group; SyncableListener* m_pEngineSync; @@ -76,12 +76,12 @@ class InternalClock : public QObject, public Clock, public Syncable { SyncMode m_mode; int m_iOldSampleRate; - double m_dOldBpm; + mixxx::Bpm m_oldBpm; // This is the BPM value at unity adopted when sync is enabled. // It is used to relate the followers and must not change when // the bpm is adjusted to avoid sudden double/half rate changes. - double m_dBaseBpm; + mixxx::Bpm m_baseBpm; // The internal clock rate is stored in terms of samples per beat. // Fractional values are allowed. diff --git a/src/engine/sync/syncable.h b/src/engine/sync/syncable.h index 65d43d3135d..06b04acb44e 100644 --- a/src/engine/sync/syncable.h +++ b/src/engine/sync/syncable.h @@ -3,6 +3,8 @@ #include #include +#include "track/bpm.h" + class EngineChannel; enum class SyncMode { @@ -113,12 +115,12 @@ class Syncable { // Gets the current speed of the syncable in bpm (bpm * rate slider), doesn't // include scratch or FF/REW values. - virtual double getBpm() const = 0; + virtual mixxx::Bpm getBpm() const = 0; // Gets the beat distance as a fraction from 0 to 1 virtual double getBeatDistance() const = 0; // Gets the speed of the syncable if it was playing at 1.0 rate. - virtual double getBaseBpm() const = 0; + virtual mixxx::Bpm getBaseBpm() const = 0; // The following functions are used to tell syncables about the state of the // current Sync Leader. @@ -130,7 +132,7 @@ class Syncable { // of the current leader. // Must never result in a call to SyncableListener::notifyBpmChanged or // signal loops could occur. - virtual void updateLeaderBpm(double bpm) = 0; + virtual void updateLeaderBpm(mixxx::Bpm bpm) = 0; // Tells a Syncable that it's going to be used as a source for leader // params. This is a gross hack so that the SyncControl can undo its @@ -139,13 +141,13 @@ class Syncable { // Perform a reset of Leader parameters. This function also triggers recalculation // of half-double multiplier. - virtual void reinitLeaderParams(double beatDistance, double baseBpm, double bpm) = 0; + virtual void reinitLeaderParams(double beatDistance, mixxx::Bpm baseBpm, mixxx::Bpm bpm) = 0; // Update the playback speed of the leader, including scratch values. // Must never result in a call to // SyncableListener::notifyInstantaneousBpmChanged or signal loops could // occur. - virtual void updateInstantaneousBpm(double bpm) = 0; + virtual void updateInstantaneousBpm(mixxx::Bpm bpm) = 0; }; /// SyncableListener is an interface class used by EngineSync to receive @@ -161,14 +163,14 @@ class SyncableListener { // A Syncable must never call notifyBpmChanged in response to a updateLeaderBpm() // call. - virtual void notifyBaseBpmChanged(Syncable* pSyncable, double bpm) = 0; - virtual void notifyRateChanged(Syncable* pSyncable, double bpm) = 0; - virtual void requestBpmUpdate(Syncable* pSyncable, double bpm) = 0; + virtual void notifyBaseBpmChanged(Syncable* pSyncable, mixxx::Bpm bpm) = 0; + virtual void notifyRateChanged(Syncable* pSyncable, mixxx::Bpm bpm) = 0; + virtual void requestBpmUpdate(Syncable* pSyncable, mixxx::Bpm bpm) = 0; // Syncables notify EngineSync directly about various events. EngineSync // does not have a say in whether these succeed or not, they are simply // notifications. - virtual void notifyInstantaneousBpmChanged(Syncable* pSyncable, double bpm) = 0; + virtual void notifyInstantaneousBpmChanged(Syncable* pSyncable, mixxx::Bpm bpm) = 0; // Notify Syncable that the Syncable's scratching state changed. virtual void notifyScratching(Syncable* pSyncable, bool scratching) = 0; diff --git a/src/engine/sync/synccontrol.cpp b/src/engine/sync/synccontrol.cpp index 5e766381c7c..05f56e4ac7b 100644 --- a/src/engine/sync/synccontrol.cpp +++ b/src/engine/sync/synccontrol.cpp @@ -77,9 +77,6 @@ SyncControl::SyncControl(const QString& group, m_pQuantize = new ControlProxy(group, "quantize", this); - // Adopt an invalid to not ignore the first call setLocalBpm() - m_prevLocalBpm.setValue(-1); - // BPMControl and RateControl will be initialized later. } @@ -204,8 +201,12 @@ double SyncControl::getBeatDistance() const { return adjustSyncBeatDistance(beatDistance); } -double SyncControl::getBaseBpm() const { - return m_pLocalBpm->get() / m_leaderBpmAdjustFactor; +mixxx::Bpm SyncControl::getBaseBpm() const { + const mixxx::Bpm bpm = getLocalBpm(); + if (!bpm.isValid()) { + return {}; + } + return mixxx::Bpm(bpm.value() / m_leaderBpmAdjustFactor); } void SyncControl::updateLeaderBeatDistance(double beatDistance) { @@ -221,7 +222,7 @@ void SyncControl::updateLeaderBeatDistance(double beatDistance) { updateTargetBeatDistance(); } -void SyncControl::updateLeaderBpm(double bpm) { +void SyncControl::updateLeaderBpm(mixxx::Bpm bpm) { if (kLogger.traceEnabled()) { kLogger.trace() << getGroup() << "SyncControl::updateLeaderBpm" << bpm; } @@ -236,8 +237,8 @@ void SyncControl::updateLeaderBpm(double bpm) { return; } - double localBpm = m_pLocalBpm->get(); - if (localBpm > 0.0) { + const auto localBpm = getLocalBpm(); + if (localBpm.isValid()) { m_pRateRatio->set(bpm * m_leaderBpmAdjustFactor / localBpm); } } @@ -247,7 +248,7 @@ void SyncControl::notifyLeaderParamSource() { } void SyncControl::reinitLeaderParams( - double beatDistance, double baseBpm, double bpm) { + double beatDistance, mixxx::Bpm baseBpm, mixxx::Bpm bpm) { if (kLogger.traceEnabled()) { kLogger.trace() << "SyncControl::reinitLeaderParams" << getGroup() << beatDistance << baseBpm << bpm; @@ -257,8 +258,8 @@ void SyncControl::reinitLeaderParams( updateLeaderBeatDistance(beatDistance); } -double SyncControl::determineBpmMultiplier(double myBpm, double targetBpm) const { - if (myBpm == 0.0 || targetBpm == 0.0) { +double SyncControl::determineBpmMultiplier(mixxx::Bpm myBpm, mixxx::Bpm targetBpm) const { + if (!myBpm.isValid() || !targetBpm.isValid()) { return kBpmUnity; } double unityRatio = myBpm / targetBpm; @@ -305,17 +306,23 @@ void SyncControl::updateTargetBeatDistance() { m_pBpmControl->setTargetBeatDistance(targetDistance); } -double SyncControl::getBpm() const { +mixxx::Bpm SyncControl::getBpm() const { + const auto bpm = mixxx::Bpm(m_pBpm->get()); if (kLogger.traceEnabled()) { kLogger.trace() << getGroup() << "SyncControl::getBpm()" - << m_pBpm->get() << "/" << m_leaderBpmAdjustFactor; + << bpm << "/" << m_leaderBpmAdjustFactor; + } + if (!bpm.isValid()) { + return {}; } - return m_pBpm->get() / m_leaderBpmAdjustFactor; + + return bpm / m_leaderBpmAdjustFactor; } -void SyncControl::updateInstantaneousBpm(double bpm) { +void SyncControl::updateInstantaneousBpm(mixxx::Bpm bpm) { // Adjust the incoming bpm by the multiplier. - m_pBpmControl->updateInstantaneousBpm(bpm * m_leaderBpmAdjustFactor); + const double bpmValue = bpm.valueOr(0.0) * m_leaderBpmAdjustFactor; + m_pBpmControl->updateInstantaneousBpm(bpmValue); } void SyncControl::reportTrackPosition(double fractionalPlaypos) { @@ -444,18 +451,18 @@ void SyncControl::slotSyncEnabledChangeRequest(double enabled) { m_pChannel->getEngineBuffer()->requestEnableSync(bEnabled); } -void SyncControl::setLocalBpm(double local_bpm) { - if (local_bpm == m_prevLocalBpm.getValue()) { +void SyncControl::setLocalBpm(mixxx::Bpm localBpm) { + if (localBpm == m_prevLocalBpm.getValue()) { return; } - m_prevLocalBpm.setValue(local_bpm); + m_prevLocalBpm.setValue(localBpm); SyncMode syncMode = getSyncMode(); if (syncMode <= SyncMode::None) { return; } - double bpm = local_bpm * m_pRateRatio->get(); + const mixxx::Bpm bpm = localBpm * m_pRateRatio->get(); if (isFollower(syncMode)) { // In this case we need an update from the current leader to adjust @@ -484,11 +491,14 @@ void SyncControl::updateAudible() { } void SyncControl::slotRateChanged() { - double bpm = m_pLocalBpm ? m_pLocalBpm->get() * m_pRateRatio->get() : 0.0; + mixxx::Bpm bpm = getLocalBpm(); + if (bpm.isValid()) { + bpm *= m_pRateRatio->get(); + } if (kLogger.traceEnabled()) { kLogger.trace() << getGroup() << "SyncControl::slotRateChanged" << m_pRateRatio->get() << bpm; } - if (bpm > 0 && isSynchronized()) { + if (bpm.isValid() && isSynchronized()) { // When reporting our bpm, remove the multiplier so the leaders all // think the followers have the same bpm. m_pEngineSync->notifyRateChanged(this, bpm / m_leaderBpmAdjustFactor); @@ -504,15 +514,24 @@ void SyncControl::reportPlayerSpeed(double speed, bool scratching) { } // When reporting our speed, remove the multiplier so the leaders all // think the followers have the same bpm. - double instantaneous_bpm = m_pLocalBpm->get() * speed / m_leaderBpmAdjustFactor; - m_pEngineSync->notifyInstantaneousBpmChanged(this, instantaneous_bpm); + mixxx::Bpm localBpm = getLocalBpm(); + if (localBpm.isValid()) { + const mixxx::Bpm instantaneousBpm = localBpm * (speed / m_leaderBpmAdjustFactor); + m_pEngineSync->notifyInstantaneousBpmChanged(this, instantaneousBpm); + } } -double SyncControl::fileBpm() const { +mixxx::Bpm SyncControl::fileBpm() const { mixxx::BeatsPointer pBeats = m_pBeats; if (pBeats) { - // FIXME: calling bpm.value() without checking bpm.isValid() - return pBeats->getBpm().value(); + return pBeats->getBpm(); + } + return {}; +} + +mixxx::Bpm SyncControl::getLocalBpm() const { + if (m_pLocalBpm) { + return mixxx::Bpm(m_pLocalBpm->get()); } - return mixxx::Bpm::kValueUndefined; + return {}; } diff --git a/src/engine/sync/synccontrol.h b/src/engine/sync/synccontrol.h index f74a8a9fff3..c9f2e170f7b 100644 --- a/src/engine/sync/synccontrol.h +++ b/src/engine/sync/synccontrol.h @@ -26,7 +26,7 @@ class SyncControl : public EngineControl, public Syncable { const QString& getGroup() const override { return m_sGroup; } EngineChannel* getChannel() const override { return m_pChannel; } - double getBpm() const override; + mixxx::Bpm getBpm() const override; SyncMode getSyncMode() const override; void setSyncMode(SyncMode mode) override; @@ -38,11 +38,11 @@ class SyncControl : public EngineControl, public Syncable { double adjustSyncBeatDistance(double beatDistance) const; double getBeatDistance() const override; void updateTargetBeatDistance(); - double getBaseBpm() const override; + mixxx::Bpm getBaseBpm() const override; // The local bpm is the base bpm of the track around the current position. // For beatmap tracks, this can change with every beat. - void setLocalBpm(double local_bpm); + void setLocalBpm(mixxx::Bpm localBpm); void updateAudible(); // Must never result in a call to @@ -50,14 +50,14 @@ class SyncControl : public EngineControl, public Syncable { void updateLeaderBeatDistance(double beatDistance) override; // Must never result in a call to // SyncableListener::notifyBpmChanged or signal loops could occur. - void updateLeaderBpm(double bpm) override; + void updateLeaderBpm(mixxx::Bpm bpm) override; void notifyLeaderParamSource() override; - void reinitLeaderParams(double beatDistance, double baseBpm, double bpm) override; + void reinitLeaderParams(double beatDistance, mixxx::Bpm baseBpm, mixxx::Bpm bpm) override; // Must never result in a call to // SyncableListener::notifyInstantaneousBpmChanged or signal loops could // occur. - void updateInstantaneousBpm(double bpm) override; + void updateInstantaneousBpm(mixxx::Bpm bpm) override; void setEngineControls(RateControl* pRateControl, BpmControl* pBpmControl); @@ -90,8 +90,9 @@ class SyncControl : public EngineControl, public Syncable { // bpm. e.g. 70 matches better with 140/2. This function returns the // best factor for multiplying the leader bpm to get a bpm this syncable // should match against. - double determineBpmMultiplier(double myBpm, double targetBpm) const; - double fileBpm() const; + double determineBpmMultiplier(mixxx::Bpm myBpm, mixxx::Bpm targetBpm) const; + mixxx::Bpm fileBpm() const; + mixxx::Bpm getLocalBpm() const; QString m_sGroup; // The only reason we have this pointer is an optimzation so that the @@ -113,7 +114,7 @@ class SyncControl : public EngineControl, public Syncable { // It is handy to store the raw reported target beat distance in case the // multiplier changes and we need to recalculate the target distance. double m_unmultipliedTargetBeatDistance; - ControlValueAtomic m_prevLocalBpm; + ControlValueAtomic m_prevLocalBpm; QAtomicInt m_audible; QScopedPointer m_pSyncMode; diff --git a/src/test/enginesynctest.cpp b/src/test/enginesynctest.cpp index 0ba03ae08b9..1f8a06a8343 100644 --- a/src/test/enginesynctest.cpp +++ b/src/test/enginesynctest.cpp @@ -2081,7 +2081,7 @@ TEST_F(EngineSyncTest, SetFileBpmUpdatesLocalBpm) { m_pTrack1->getSampleRate(), mixxx::Bpm(130), mixxx::audio::kStartFramePos); m_pTrack1->trySetBeats(pBeats1); EXPECT_EQ( - 130.0, m_pEngineSync->getSyncableForGroup(m_sGroup1)->getBaseBpm()); + mixxx::Bpm(130.0), m_pEngineSync->getSyncableForGroup(m_sGroup1)->getBaseBpm()); } TEST_F(EngineSyncTest, SyncPhaseToPlayingNonSyncDeck) { @@ -2905,7 +2905,7 @@ TEST_F(EngineSyncTest, BpmAdjustFactor) { ProcessBuffer(); EXPECT_TRUE(isSoftLeader(m_sGroup2)); // Pretend a changing beatgrid - static_cast(m_pEngineSync->getLeaderSyncable())->setLocalBpm(152); + static_cast(m_pEngineSync->getLeaderSyncable())->setLocalBpm(mixxx::Bpm{152}); ProcessBuffer(); ControlObject::set(ConfigKey(m_sGroup1, "sync_enabled"), 1.0); diff --git a/src/test/synccontroltest.cpp b/src/test/synccontroltest.cpp index 59a62d88a29..3a87a4ad609 100644 --- a/src/test/synccontroltest.cpp +++ b/src/test/synccontroltest.cpp @@ -19,9 +19,15 @@ class SyncControlTest : public MockedEngineBackendTest { TEST_F(SyncControlTest, TestDetermineBpmMultiplier) { EXPECT_EQ(SyncControl::kBpmUnity, - m_pChannel1->getEngineBuffer()->m_pSyncControl->determineBpmMultiplier(70, 80)); + m_pChannel1->getEngineBuffer() + ->m_pSyncControl->determineBpmMultiplier( + mixxx::Bpm(70), mixxx::Bpm(80))); EXPECT_EQ(SyncControl::kBpmHalve, - m_pChannel1->getEngineBuffer()->m_pSyncControl->determineBpmMultiplier(70, 160)); + m_pChannel1->getEngineBuffer() + ->m_pSyncControl->determineBpmMultiplier( + mixxx::Bpm(70), mixxx::Bpm(160))); EXPECT_EQ(SyncControl::kBpmDouble, - m_pChannel1->getEngineBuffer()->m_pSyncControl->determineBpmMultiplier(70, 40)); + m_pChannel1->getEngineBuffer() + ->m_pSyncControl->determineBpmMultiplier( + mixxx::Bpm(70), mixxx::Bpm(40))); } From e99bf9d4ddb620955c92ef0d87476c2951b63f0e Mon Sep 17 00:00:00 2001 From: Jan Holthuis Date: Fri, 6 Aug 2021 13:32:21 +0200 Subject: [PATCH 2/3] EngineSync: Rebname local leader bpm variables (leftover from #3921) --- src/engine/sync/enginesync.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/engine/sync/enginesync.cpp b/src/engine/sync/enginesync.cpp index 6b90ab05661..f10debccb6d 100644 --- a/src/engine/sync/enginesync.cpp +++ b/src/engine/sync/enginesync.cpp @@ -409,19 +409,19 @@ void EngineSync::requestBpmUpdate(Syncable* pSyncable, mixxx::Bpm bpm) { kLogger.trace() << "EngineSync::requestBpmUpdate" << pSyncable->getGroup() << bpm; } - mixxx::Bpm mbaseBpm; - mixxx::Bpm mbpm; + mixxx::Bpm leaderBaseBpm; + mixxx::Bpm leaderBpm; double beatDistance = 0.0; if (m_pLeaderSyncable) { - mbaseBpm = m_pLeaderSyncable->getBaseBpm(); - mbpm = m_pLeaderSyncable->getBpm(); + leaderBaseBpm = m_pLeaderSyncable->getBaseBpm(); + leaderBpm = m_pLeaderSyncable->getBpm(); beatDistance = m_pLeaderSyncable->getBeatDistance(); } - if (mbaseBpm.isValid()) { + if (leaderBaseBpm.isValid()) { // update from current leader pSyncable->updateLeaderBeatDistance(beatDistance); - pSyncable->updateLeaderBpm(mbpm); + pSyncable->updateLeaderBpm(leaderBpm); } else { // There is no leader, adopt this bpm as leader value pSyncable->updateLeaderBeatDistance(0.0); From 6b495a322fa3638a45d0164f352a265dcade1761 Mon Sep 17 00:00:00 2001 From: Jan Holthuis Date: Fri, 6 Aug 2021 19:14:43 +0200 Subject: [PATCH 3/3] SyncControl: Simplify slotRateChanged() implementation --- src/engine/sync/synccontrol.cpp | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/src/engine/sync/synccontrol.cpp b/src/engine/sync/synccontrol.cpp index 05f56e4ac7b..cc898a0db55 100644 --- a/src/engine/sync/synccontrol.cpp +++ b/src/engine/sync/synccontrol.cpp @@ -492,17 +492,24 @@ void SyncControl::updateAudible() { void SyncControl::slotRateChanged() { mixxx::Bpm bpm = getLocalBpm(); - if (bpm.isValid()) { - bpm *= m_pRateRatio->get(); + if (!bpm.isValid()) { + return; } + + const double rateRatio = m_pRateRatio->get(); + bpm *= rateRatio; if (kLogger.traceEnabled()) { - kLogger.trace() << getGroup() << "SyncControl::slotRateChanged" << m_pRateRatio->get() << bpm; + kLogger.trace() << getGroup() << "SyncControl::slotRateChanged" << rateRatio << bpm; } - if (bpm.isValid() && isSynchronized()) { - // When reporting our bpm, remove the multiplier so the leaders all - // think the followers have the same bpm. - m_pEngineSync->notifyRateChanged(this, bpm / m_leaderBpmAdjustFactor); + + // BPM may be invalid if rateRatio is NaN or infinity. + if (!bpm.isValid() || !isSynchronized()) { + return; } + + // When reporting our bpm, remove the multiplier so the leaders all + // think the followers have the same bpm. + m_pEngineSync->notifyRateChanged(this, bpm / m_leaderBpmAdjustFactor); } void SyncControl::reportPlayerSpeed(double speed, bool scratching) {