From 507bb03bea5c141cf1d52566f4fa89f149339af6 Mon Sep 17 00:00:00 2001 From: Owen Williams Date: Sun, 23 May 2021 13:44:10 -0400 Subject: [PATCH 01/22] Fix half-double initialization issues --- src/engine/enginebuffer.cpp | 1 + src/engine/enginemaster.cpp | 5 ++ src/engine/sync/enginesync.cpp | 96 +++++++++++++++++++++++++------ src/engine/sync/enginesync.h | 4 +- src/engine/sync/internalclock.cpp | 3 + src/engine/sync/internalclock.h | 1 + src/engine/sync/syncable.h | 5 ++ src/engine/sync/synccontrol.cpp | 50 ++++++++++++---- src/engine/sync/synccontrol.h | 3 +- src/test/enginesynctest.cpp | 15 +++-- 10 files changed, 147 insertions(+), 36 deletions(-) diff --git a/src/engine/enginebuffer.cpp b/src/engine/enginebuffer.cpp index 80065ba2b25..4a5d011d40b 100644 --- a/src/engine/enginebuffer.cpp +++ b/src/engine/enginebuffer.cpp @@ -1297,6 +1297,7 @@ void EngineBuffer::postProcess(const int iBufferSize) { } double localBpm = m_pBpmControl->updateLocalBpm(); double beatDistance = m_pBpmControl->updateBeatDistance(); + qDebug() << getGroup() << "beat distance updated" << beatDistance; m_pSyncControl->setLocalBpm(localBpm); m_pSyncControl->updateAudible(); SyncMode mode = m_pSyncControl->getSyncMode(); diff --git a/src/engine/enginemaster.cpp b/src/engine/enginemaster.cpp index 33cd29d5e42..cda9a1dbb51 100644 --- a/src/engine/enginemaster.cpp +++ b/src/engine/enginemaster.cpp @@ -369,6 +369,11 @@ void EngineMaster::processChannels(int iBufferSize) { // Do internal master sync post-processing before the other // channels. + // OK, so this means the internal clock knows that it has crossed a beat + // boundary before the channels do. This means when we call this, we + // notify all the channels that the master beat distance has changed. + // They try to update the target beat distance, and guess the + // wrong number because they can't do half/double calculation correctly. m_pMasterSync->onCallbackEnd(m_iSampleRate, m_iBufferSize); // After all the engines have been processed, trigger post-processing diff --git a/src/engine/sync/enginesync.cpp b/src/engine/sync/enginesync.cpp index 7602283d22d..f913eb8c73d 100644 --- a/src/engine/sync/enginesync.cpp +++ b/src/engine/sync/enginesync.cpp @@ -96,8 +96,15 @@ void EngineSync::requestSyncMode(Syncable* pSyncable, SyncMode mode) { } } - if (noPlayingFollowers() && m_pMasterSyncable) { - m_pMasterSyncable->notifyOnlyPlayingSyncable(); + auto players = getPlayingSyncables(); + if (players.size() == 1) { + qDebug() << "!!!!!!!!!!!!!!!!!!!!!1there is only one playing syncable, tell it so!"; + players.at(0)->notifyOnlyPlayingSyncable(); + } else { + qDebug() << "players!"; + for (auto* play : players) { + qDebug() << play->getGroup(); + } } } @@ -350,11 +357,31 @@ void EngineSync::notifyPlayingAudible(Syncable* pSyncable, bool playingAudible) if (newMaster != nullptr && newMaster != m_pMasterSyncable) { activateMaster(newMaster, SYNC_MASTER_SOFT); - } - - if (noPlayingFollowers() && m_pMasterSyncable) { - m_pMasterSyncable->notifyOnlyPlayingSyncable(); - setMasterParams(m_pMasterSyncable); + setMasterParams(newMaster); + } + + // we know for a fact that pSyncable is playing and audible. + // We want to init master params + // cases: + // just we are playing: set master params with our settings + // NO. just because we are master, doesn't mean we win! we might be copying someone else. + // if anything we should JUST init beat distance. + // another deck is playing and it's master: don't touch it + // another deck is playing and it's not master: don't touch it + auto players = getPlayingSyncables(); + if (players.size() == 1) { + Syncable* onlyPlayer = players.at(0); + qDebug() << "2222222222222222222222222222 there is only one playing " + "syncable, it gets to set beat dist" + << onlyPlayer->getGroup(); + onlyPlayer->notifyOnlyPlayingSyncable(); + // setMasterParams(onlyPlayer); + setMasterBeatDistance(onlyPlayer, onlyPlayer->getBeatDistance()); + } else { + qDebug() << "players!"; + for (auto* play : players) { + qDebug() << play->getGroup(); + } } pSyncable->requestSync(); @@ -597,35 +624,72 @@ void EngineSync::setMasterBeatDistance(Syncable* pSource, double beatDistance) { } void EngineSync::setMasterParams(Syncable* pSource) { - const double beatDistance = pSource->getBeatDistance(); + // so the bug here is that we are setting the master parameters from some source, but + // that source may not be the master! but when we update the multipliers, we update + // based on who is master. We used to try to force master to be multiplier 1.0, but that + // causes problems. + + pSource->notifyMasterParamSource(); + + double beatDistance = pSource->getBeatDistance(); + // // If the params source is not playing, but other syncables are, then we are a stopped + // // explicit Master and we should not initialize the beat distance. Take it from the + // // internal clock instead. + if (!pSource->isPlaying()) { + bool playingSyncables = false; + for (Syncable* pSyncable : m_syncables) { + qDebug() << "checking" << pSyncable->getGroup(); + if (pSyncable == pSource) { + continue; + } + if (!pSyncable->isSynchronized()) { + continue; + } + qDebug() << "it's valid"; + if (pSyncable->isPlaying()) { + qDebug() << "found a playing syncable" << pSyncable->getGroup(); + playingSyncables = true; + break; + } + } + if (playingSyncables) { + qDebug() << "~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~" + "BaseSyncableListener::setMasterParams, not playing, " + "playing followers, pull from internal clock" + << pSource->getGroup(); + beatDistance = m_pInternalClock->getBeatDistance(); + } + } + // Do we want the RAW values here, not multiplied by adjust factor?? const double baseBpm = pSource->getBaseBpm(); double bpm = pSource->getBpm(); if (bpm <= 0) { bpm = baseBpm; } - // qDebug() << "BaseSyncableListener::setMasterParams, source is" - // << pSource->getGroup() << beatDistance << baseBpm << bpm; + qDebug() << "BaseSyncableListener::setMasterParams, source is" + << pSource->getGroup() << beatDistance << baseBpm << bpm; if (pSource != m_pInternalClock) { m_pInternalClock->setMasterParams(beatDistance, baseBpm, bpm); } foreach (Syncable* pSyncable, m_syncables) { - if (pSyncable == pSource || !pSyncable->isSynchronized()) { + // not sure if we should skip the pSource here. + if (!pSyncable->isSynchronized()) { continue; } pSyncable->setMasterParams(beatDistance, baseBpm, bpm); } } -bool EngineSync::noPlayingFollowers() const { +QList EngineSync::getPlayingSyncables() const { + QList players; for (Syncable* pSyncable : m_syncables) { if (!pSyncable->isSynchronized()) { continue; } - if (pSyncable->isPlaying() && pSyncable->isAudible() && - isFollower(pSyncable->getSyncMode())) { - return false; + if (pSyncable->isPlaying()) { + players.append(pSyncable); } } - return true; + return players; } diff --git a/src/engine/sync/enginesync.h b/src/engine/sync/enginesync.h index 426c5256f88..8e306e14a91 100644 --- a/src/engine/sync/enginesync.h +++ b/src/engine/sync/enginesync.h @@ -115,8 +115,8 @@ class EngineSync : public SyncableListener { void setMasterParams(Syncable* pSource); - /// Check if there are no playing followers left. - bool noPlayingFollowers() const; + /// Return the playing syncables, if any. This is used to initialize beat distance settings. + QList getPlayingSyncables() const; /// Only for testing. Do not use. Syncable* getSyncableForGroup(const QString& group); diff --git a/src/engine/sync/internalclock.cpp b/src/engine/sync/internalclock.cpp index 8ecfe8b27ad..8c3731f2397 100644 --- a/src/engine/sync/internalclock.cpp +++ b/src/engine/sync/internalclock.cpp @@ -140,6 +140,9 @@ void InternalClock::setInstantaneousBpm(double bpm) { Q_UNUSED(bpm); } +void InternalClock::notifyMasterParamSource() { +} + void InternalClock::setMasterParams(double beatDistance, double baseBpm, double bpm) { if (kLogger.traceEnabled()) { kLogger.trace() << "InternalClock::setMasterParams" << beatDistance << baseBpm << bpm; diff --git a/src/engine/sync/internalclock.h b/src/engine/sync/internalclock.h index 88ecfb38c6f..dea03a490d7 100644 --- a/src/engine/sync/internalclock.h +++ b/src/engine/sync/internalclock.h @@ -52,6 +52,7 @@ class InternalClock : public QObject, public Clock, public Syncable { double getBaseBpm() const override; void setMasterBpm(double bpm) override; + void notifyMasterParamSource() override; double getBpm() const override; void setInstantaneousBpm(double bpm) override; void setMasterParams(double beatDistance, double baseBpm, double bpm) override; diff --git a/src/engine/sync/syncable.h b/src/engine/sync/syncable.h index d25e54d0a09..19678be6229 100644 --- a/src/engine/sync/syncable.h +++ b/src/engine/sync/syncable.h @@ -112,6 +112,11 @@ class Syncable { // signal loops could occur. virtual void setMasterBpm(double bpm) = 0; + // Tells a Syncable that it's going to be used as a source for master + // params. This is a gross hack so that the SyncControl can undo its + // half/double adjustment. + virtual void notifyMasterParamSource() = 0; + // Combines the above three calls into one, since they are often set // simultaneously. Avoids redundant recalculation that would occur by // using the three calls separately. diff --git a/src/engine/sync/synccontrol.cpp b/src/engine/sync/synccontrol.cpp index 060cab53257..9c9a3c0b9d0 100644 --- a/src/engine/sync/synccontrol.cpp +++ b/src/engine/sync/synccontrol.cpp @@ -212,6 +212,7 @@ void SyncControl::setMasterBeatDistance(double beatDistance) { // Set the BpmControl target beat distance to beatDistance, adjusted by // the multiplier if in effect. This way all of the multiplier logic // is contained in this single class. + m_lastTargetBeatDistance = m_unmultipliedTargetBeatDistance; m_unmultipliedTargetBeatDistance = beatDistance; // Update the target beat distance based on the multiplier. updateTargetBeatDistance(); @@ -238,19 +239,37 @@ void SyncControl::setMasterBpm(double bpm) { } } +void SyncControl::notifyMasterParamSource() { + m_masterBpmAdjustFactor = kBpmUnity; +} + void SyncControl::setMasterParams( double beatDistance, double baseBpm, double bpm) { + qDebug() << "SyncControl::setMasterParams" << getGroup() << beatDistance << baseBpm << bpm; + // if (paramSource == this) { + // baseBpm *= m_masterBpmAdjustFactor; + // bpm *= m_masterBpmAdjustFactor; + // qDebug() << "we were param source, undo adjust factor:" << baseBpm << bpm; + // } // Calculate the factor for the file bpm. That gives the best // result at any rate slider position. - double masterBpmAdjustFactor = determineBpmMultiplier(fileBpm(), baseBpm); - if (isMaster(getSyncMode())) { - // In Master mode we adjust the incoming Bpm for the initial sync. - bpm *= masterBpmAdjustFactor; - m_masterBpmAdjustFactor = kBpmUnity; - } else { - // in Follower mode we keep the factor when reporting our BPM - m_masterBpmAdjustFactor = masterBpmAdjustFactor; - } + qDebug() << "file bpm: " << fileBpm(); + + // masterBpmAdjustFactor = determineBpmMultiplier(fileBpm(), baseBpm); + // double masterBpmAdjustFactor = determineBpmMultiplier(fileBpm(), baseBpm); + m_masterBpmAdjustFactor = determineBpmMultiplier(fileBpm(), baseBpm); + // if (paramSource == this) { + // // qDebug() << "We were param source!"; + // // // If we are the param source, we need to undo the adjustment factor + // // // or else we start piling multipliers on top of each other. + // // bpm *= masterBpmAdjustFactor; + // m_masterBpmAdjustFactor = kBpmUnity; + // } else { + // m_masterBpmAdjustFactor = masterBpmAdjustFactor; + // } + + // m_masterBpmAdjustFactor = determineBpmMultiplier(fileBpm(), baseBpm); + qDebug() << "ADJUST FACTOR?!" << m_masterBpmAdjustFactor; setMasterBpm(bpm); setMasterBeatDistance(beatDistance); } @@ -274,7 +293,10 @@ double SyncControl::determineBpmMultiplier(double myBpm, double targetBpm) const void SyncControl::updateTargetBeatDistance() { double targetDistance = m_unmultipliedTargetBeatDistance; if (kLogger.traceEnabled()) { - kLogger.trace() << getGroup() << "SyncControl::updateTargetBeatDistance, unmult distance" << targetDistance; + kLogger.trace() + << getGroup() + << "SyncControl::updateTargetBeatDistance, unmult distance" + << targetDistance << m_masterBpmAdjustFactor; } // Determining the target distance is not as simple as x2 or /2. Since one @@ -289,7 +311,9 @@ void SyncControl::updateTargetBeatDistance() { targetDistance *= kBpmDouble; } else if (m_masterBpmAdjustFactor == kBpmHalve) { targetDistance *= kBpmHalve; - if (m_pBeatDistance->get() >= 0.5) { + qDebug() << "my beat dist" << m_pBeatDistance->get(); + // Our beat distance CO is still a buffer behind, so take the current value. + if (m_pBpmControl->getBeatDistance(getSampleOfTrack().current) >= 0.5) { targetDistance += 0.5; } } @@ -301,7 +325,8 @@ void SyncControl::updateTargetBeatDistance() { double SyncControl::getBpm() const { if (kLogger.traceEnabled()) { - kLogger.trace() << getGroup() << "SyncControl::getBpm()"; + kLogger.trace() << getGroup() << "SyncControl::getBpm()" + << m_pBpm->get() << "/" << m_masterBpmAdjustFactor; } return m_pBpm->get() / m_masterBpmAdjustFactor; } @@ -441,6 +466,7 @@ void SyncControl::setLocalBpm(double local_bpm) { if (local_bpm == m_prevLocalBpm.getValue()) { return; } + qDebug() << "SyncControl::setLocalBpm" << getGroup() << local_bpm; m_prevLocalBpm.setValue(local_bpm); SyncMode syncMode = getSyncMode(); diff --git a/src/engine/sync/synccontrol.h b/src/engine/sync/synccontrol.h index ccdff1e4f54..32781b42ff3 100644 --- a/src/engine/sync/synccontrol.h +++ b/src/engine/sync/synccontrol.h @@ -48,10 +48,10 @@ class SyncControl : public EngineControl, public Syncable { // Must never result in a call to // SyncableListener::notifyBeatDistanceChanged or signal loops could occur. void setMasterBeatDistance(double beatDistance) override; - // Must never result in a call to // SyncableListener::notifyBpmChanged or signal loops could occur. void setMasterBpm(double bpm) override; + void notifyMasterParamSource() override; void setMasterParams(double beatDistance, double baseBpm, double bpm) override; // Must never result in a call to @@ -113,6 +113,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; + double m_lastTargetBeatDistance; ControlValueAtomic m_prevLocalBpm; QAtomicInt m_audible; diff --git a/src/test/enginesynctest.cpp b/src/test/enginesynctest.cpp index 7d00c4a7e2e..39a216db188 100644 --- a/src/test/enginesynctest.cpp +++ b/src/test/enginesynctest.cpp @@ -1660,14 +1660,16 @@ TEST_F(EngineSyncTest, HalfDoubleBpmTest) { ->set(SYNC_FOLLOWER); // Mixxx will choose the first playing deck to be master. Let's start deck 2 first. + qDebug() << "~~~~~~~~~~~~~~~~~~~~ ch2 play"; ControlObject::getControl(ConfigKey(m_sGroup2, "play"))->set(1.0); + qDebug() << "~~~~~~~~~~~~~~~~~~~~ ch1 play"; ControlObject::getControl(ConfigKey(m_sGroup1, "play"))->set(1.0); ProcessBuffer(); - EXPECT_EQ(1.0, + EXPECT_EQ(0.5, m_pChannel1->getEngineBuffer() ->m_pSyncControl->m_masterBpmAdjustFactor); - EXPECT_EQ(2.0, + EXPECT_EQ(1.0, m_pChannel2->getEngineBuffer() ->m_pSyncControl->m_masterBpmAdjustFactor); EXPECT_DOUBLE_EQ( @@ -1789,7 +1791,7 @@ TEST_F(EngineSyncTest, HalfDoubleThenPlay) { ProcessBuffer(); - EXPECT_DOUBLE_EQ(87.5, + EXPECT_DOUBLE_EQ(175, ControlObject::getControl(ConfigKey(m_sInternalClockGroup, "bpm")) ->get()); @@ -1997,7 +1999,9 @@ TEST_F(EngineSyncTest, SyncPhaseToPlayingNonSyncDeck) { // Next Deck becomes master and the Master clock is set to 100 BPM // The 130 BPM Track should be played at 100 BPM, rate = 0.769230769 pButtonSyncEnabled1->set(1.0); + qDebug() << "PUSH PLAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAY"; ControlObject::getControl(ConfigKey(m_sGroup1, "play"))->set(1.0); + // Beat length: 40707.692307692305 // Buffer size is 1024 at 44.1 kHz at the given rate = 787.692307692 Samples // Expected beat_distance = 0.019349962 @@ -2021,12 +2025,13 @@ TEST_F(EngineSyncTest, SyncPhaseToPlayingNonSyncDeck) { ProcessBuffer(); - // we expect that Deck 1 distance has not changed and the internal clock follows it exactly. + // we expect that Deck 1 distance has not changed but the internal clock keeps going, because + // the internal clock should continue playing even if the leader is stopped. EXPECT_TRUE(isSoftMaster(m_sGroup1)); EXPECT_NEAR(0.019349962, ControlObject::getControl(ConfigKey(m_sGroup1, "beat_distance"))->get(), kMaxFloatingPointErrorLowPrecision); - EXPECT_NEAR(0.019349962, + EXPECT_NEAR(0.038699924, ControlObject::getControl(ConfigKey(m_sInternalClockGroup, "beat_distance"))->get(), kMaxFloatingPointErrorLowPrecision); From 0718058bfc098070bd6323d0e64d9d9a505b7418 Mon Sep 17 00:00:00 2001 From: Owen Williams Date: Sun, 23 May 2021 14:29:36 -0400 Subject: [PATCH 02/22] Sync Lock: Fix more half/double issues and clean up for PR --- src/engine/controls/bpmcontrol.cpp | 6 ++- src/engine/enginebuffer.cpp | 1 - src/engine/enginemaster.cpp | 8 ++- src/engine/sync/enginesync.cpp | 83 +++++++++++------------------- src/engine/sync/enginesync.h | 5 +- src/engine/sync/syncable.h | 2 +- src/engine/sync/synccontrol.cpp | 31 ++--------- src/engine/sync/synccontrol.h | 1 - src/test/enginesynctest.cpp | 21 ++++++-- 9 files changed, 61 insertions(+), 97 deletions(-) diff --git a/src/engine/controls/bpmcontrol.cpp b/src/engine/controls/bpmcontrol.cpp index 3dae2ae609a..fac2e68e87d 100644 --- a/src/engine/controls/bpmcontrol.cpp +++ b/src/engine/controls/bpmcontrol.cpp @@ -536,7 +536,9 @@ double BpmControl::getBeatDistance(double dThisPosition) const { // we don't adjust the reported distance the track will try to adjust // sync against itself. if (kLogger.traceEnabled()) { - kLogger.trace() << getGroup() << "BpmControl::getBeatDistance" << dThisPosition; + kLogger.trace() << getGroup() + << "BpmControl::getBeatDistance. dThisPosition:" + << dThisPosition; } double dPrevBeat = m_pPrevBeat->get(); double dNextBeat = m_pNextBeat->get(); @@ -560,7 +562,7 @@ double BpmControl::getBeatDistance(double dThisPosition) const { } if (kLogger.traceEnabled()) { - kLogger.trace() << getGroup() << "BpmControl::getBeatDistance" + kLogger.trace() << getGroup() << "BpmControl::getBeatDistance. dBeatPercentage:" << dBeatPercentage << "- offset " << m_dUserOffset.getValue() << " = " << (dBeatPercentage - m_dUserOffset.getValue()); diff --git a/src/engine/enginebuffer.cpp b/src/engine/enginebuffer.cpp index 4a5d011d40b..80065ba2b25 100644 --- a/src/engine/enginebuffer.cpp +++ b/src/engine/enginebuffer.cpp @@ -1297,7 +1297,6 @@ void EngineBuffer::postProcess(const int iBufferSize) { } double localBpm = m_pBpmControl->updateLocalBpm(); double beatDistance = m_pBpmControl->updateBeatDistance(); - qDebug() << getGroup() << "beat distance updated" << beatDistance; m_pSyncControl->setLocalBpm(localBpm); m_pSyncControl->updateAudible(); SyncMode mode = m_pSyncControl->getSyncMode(); diff --git a/src/engine/enginemaster.cpp b/src/engine/enginemaster.cpp index cda9a1dbb51..202fe0b1341 100644 --- a/src/engine/enginemaster.cpp +++ b/src/engine/enginemaster.cpp @@ -369,11 +369,9 @@ void EngineMaster::processChannels(int iBufferSize) { // Do internal master sync post-processing before the other // channels. - // OK, so this means the internal clock knows that it has crossed a beat - // boundary before the channels do. This means when we call this, we - // notify all the channels that the master beat distance has changed. - // They try to update the target beat distance, and guess the - // wrong number because they can't do half/double calculation correctly. + // Note, because we call this on the internal clock first, + // it will have an up-to-date beatDistance, whereas the other + // Syncables will not. m_pMasterSync->onCallbackEnd(m_iSampleRate, m_iBufferSize); // After all the engines have been processed, trigger post-processing diff --git a/src/engine/sync/enginesync.cpp b/src/engine/sync/enginesync.cpp index f913eb8c73d..fad3c6ed696 100644 --- a/src/engine/sync/enginesync.cpp +++ b/src/engine/sync/enginesync.cpp @@ -96,15 +96,8 @@ void EngineSync::requestSyncMode(Syncable* pSyncable, SyncMode mode) { } } - auto players = getPlayingSyncables(); - if (players.size() == 1) { - qDebug() << "!!!!!!!!!!!!!!!!!!!!!1there is only one playing syncable, tell it so!"; - players.at(0)->notifyOnlyPlayingSyncable(); - } else { - qDebug() << "players!"; - for (auto* play : players) { - qDebug() << play->getGroup(); - } + if (auto onlyPlayer = getOnlyPlayingSyncable(); onlyPlayer != nullptr) { + onlyPlayer->notifyOnlyPlayingSyncable(); } } @@ -358,30 +351,11 @@ void EngineSync::notifyPlayingAudible(Syncable* pSyncable, bool playingAudible) if (newMaster != nullptr && newMaster != m_pMasterSyncable) { activateMaster(newMaster, SYNC_MASTER_SOFT); setMasterParams(newMaster); - } - - // we know for a fact that pSyncable is playing and audible. - // We want to init master params - // cases: - // just we are playing: set master params with our settings - // NO. just because we are master, doesn't mean we win! we might be copying someone else. - // if anything we should JUST init beat distance. - // another deck is playing and it's master: don't touch it - // another deck is playing and it's not master: don't touch it - auto players = getPlayingSyncables(); - if (players.size() == 1) { - Syncable* onlyPlayer = players.at(0); - qDebug() << "2222222222222222222222222222 there is only one playing " - "syncable, it gets to set beat dist" - << onlyPlayer->getGroup(); + } else if (auto onlyPlayer = getOnlyPlayingSyncable(); onlyPlayer != nullptr) { + // Even if we didn't change master, if there is only one player (us), then we should + // reinit the beat distance. onlyPlayer->notifyOnlyPlayingSyncable(); - // setMasterParams(onlyPlayer); setMasterBeatDistance(onlyPlayer, onlyPlayer->getBeatDistance()); - } else { - qDebug() << "players!"; - for (auto* play : players) { - qDebug() << play->getGroup(); - } } pSyncable->requestSync(); @@ -611,6 +585,11 @@ void EngineSync::setMasterInstantaneousBpm(Syncable* pSource, double bpm) { } void EngineSync::setMasterBeatDistance(Syncable* pSource, double beatDistance) { + if (kLogger.traceEnabled()) { + kLogger.trace() << "EngineSync::setMasterBeatDistance" + << (pSource ? pSource->getGroup() : "null") + << beatDistance; + } if (pSource != m_pInternalClock) { m_pInternalClock->setMasterBeatDistance(beatDistance); } @@ -624,55 +603,47 @@ void EngineSync::setMasterBeatDistance(Syncable* pSource, double beatDistance) { } void EngineSync::setMasterParams(Syncable* pSource) { - // so the bug here is that we are setting the master parameters from some source, but - // that source may not be the master! but when we update the multipliers, we update - // based on who is master. We used to try to force master to be multiplier 1.0, but that - // causes problems. - + // Important note! Because of the way sync works, the new master is usually not the same + // as the Syncable setting the master parameters (here, pSource). Notify the proper Syncable + // so it can prepare itself. (This is a hack to undo half/double math so that we initialize + // based on un-multiplied bpm values). pSource->notifyMasterParamSource(); double beatDistance = pSource->getBeatDistance(); - // // If the params source is not playing, but other syncables are, then we are a stopped - // // explicit Master and we should not initialize the beat distance. Take it from the - // // internal clock instead. if (!pSource->isPlaying()) { + // If the params source is not playing, but other syncables are, then we are a stopped + // explicit Master and we should not initialize the beat distance. Take it from the + // internal clock instead. bool playingSyncables = false; for (Syncable* pSyncable : m_syncables) { - qDebug() << "checking" << pSyncable->getGroup(); if (pSyncable == pSource) { continue; } if (!pSyncable->isSynchronized()) { continue; } - qDebug() << "it's valid"; if (pSyncable->isPlaying()) { - qDebug() << "found a playing syncable" << pSyncable->getGroup(); playingSyncables = true; break; } } if (playingSyncables) { - qDebug() << "~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~" - "BaseSyncableListener::setMasterParams, not playing, " - "playing followers, pull from internal clock" - << pSource->getGroup(); beatDistance = m_pInternalClock->getBeatDistance(); } } - // Do we want the RAW values here, not multiplied by adjust factor?? const double baseBpm = pSource->getBaseBpm(); double bpm = pSource->getBpm(); if (bpm <= 0) { bpm = baseBpm; } - qDebug() << "BaseSyncableListener::setMasterParams, source is" - << pSource->getGroup() << beatDistance << baseBpm << bpm; + if (kLogger.traceEnabled()) { + kLogger.trace() << "BaseSyncableListener::setMasterParams, source is" + << pSource->getGroup() << beatDistance << baseBpm << bpm; + } if (pSource != m_pInternalClock) { m_pInternalClock->setMasterParams(beatDistance, baseBpm, bpm); } foreach (Syncable* pSyncable, m_syncables) { - // not sure if we should skip the pSource here. if (!pSyncable->isSynchronized()) { continue; } @@ -680,16 +651,20 @@ void EngineSync::setMasterParams(Syncable* pSource) { } } -QList EngineSync::getPlayingSyncables() const { - QList players; +Syncable* EngineSync::getOnlyPlayingSyncable() const { + Syncable* onlyPlaying = nullptr; for (Syncable* pSyncable : m_syncables) { if (!pSyncable->isSynchronized()) { continue; } if (pSyncable->isPlaying()) { - players.append(pSyncable); + if (!onlyPlaying) { + onlyPlaying = pSyncable; + } else { + return nullptr; + } } } - return players; + return onlyPlaying; } diff --git a/src/engine/sync/enginesync.h b/src/engine/sync/enginesync.h index 8e306e14a91..4988a14c422 100644 --- a/src/engine/sync/enginesync.h +++ b/src/engine/sync/enginesync.h @@ -115,8 +115,9 @@ class EngineSync : public SyncableListener { void setMasterParams(Syncable* pSource); - /// Return the playing syncables, if any. This is used to initialize beat distance settings. - QList getPlayingSyncables() const; + /// Iff there is a single playing syncable, return it. This is used to initialize master + /// params. + Syncable* getOnlyPlayingSyncable() const; /// Only for testing. Do not use. Syncable* getSyncableForGroup(const QString& group); diff --git a/src/engine/sync/syncable.h b/src/engine/sync/syncable.h index 19678be6229..fa29b1da915 100644 --- a/src/engine/sync/syncable.h +++ b/src/engine/sync/syncable.h @@ -114,7 +114,7 @@ class Syncable { // Tells a Syncable that it's going to be used as a source for master // params. This is a gross hack so that the SyncControl can undo its - // half/double adjustment. + // half/double adjustment so bpms are initialized correctly. virtual void notifyMasterParamSource() = 0; // Combines the above three calls into one, since they are often set diff --git a/src/engine/sync/synccontrol.cpp b/src/engine/sync/synccontrol.cpp index 9c9a3c0b9d0..d387a22488c 100644 --- a/src/engine/sync/synccontrol.cpp +++ b/src/engine/sync/synccontrol.cpp @@ -212,7 +212,6 @@ void SyncControl::setMasterBeatDistance(double beatDistance) { // Set the BpmControl target beat distance to beatDistance, adjusted by // the multiplier if in effect. This way all of the multiplier logic // is contained in this single class. - m_lastTargetBeatDistance = m_unmultipliedTargetBeatDistance; m_unmultipliedTargetBeatDistance = beatDistance; // Update the target beat distance based on the multiplier. updateTargetBeatDistance(); @@ -245,31 +244,11 @@ void SyncControl::notifyMasterParamSource() { void SyncControl::setMasterParams( double beatDistance, double baseBpm, double bpm) { - qDebug() << "SyncControl::setMasterParams" << getGroup() << beatDistance << baseBpm << bpm; - // if (paramSource == this) { - // baseBpm *= m_masterBpmAdjustFactor; - // bpm *= m_masterBpmAdjustFactor; - // qDebug() << "we were param source, undo adjust factor:" << baseBpm << bpm; - // } - // Calculate the factor for the file bpm. That gives the best - // result at any rate slider position. - qDebug() << "file bpm: " << fileBpm(); - - // masterBpmAdjustFactor = determineBpmMultiplier(fileBpm(), baseBpm); - // double masterBpmAdjustFactor = determineBpmMultiplier(fileBpm(), baseBpm); + if (kLogger.traceEnabled()) { + kLogger.trace() << "SyncControl::setMasterParams" << getGroup() + << beatDistance << baseBpm << bpm; + } m_masterBpmAdjustFactor = determineBpmMultiplier(fileBpm(), baseBpm); - // if (paramSource == this) { - // // qDebug() << "We were param source!"; - // // // If we are the param source, we need to undo the adjustment factor - // // // or else we start piling multipliers on top of each other. - // // bpm *= masterBpmAdjustFactor; - // m_masterBpmAdjustFactor = kBpmUnity; - // } else { - // m_masterBpmAdjustFactor = masterBpmAdjustFactor; - // } - - // m_masterBpmAdjustFactor = determineBpmMultiplier(fileBpm(), baseBpm); - qDebug() << "ADJUST FACTOR?!" << m_masterBpmAdjustFactor; setMasterBpm(bpm); setMasterBeatDistance(beatDistance); } @@ -311,7 +290,6 @@ void SyncControl::updateTargetBeatDistance() { targetDistance *= kBpmDouble; } else if (m_masterBpmAdjustFactor == kBpmHalve) { targetDistance *= kBpmHalve; - qDebug() << "my beat dist" << m_pBeatDistance->get(); // Our beat distance CO is still a buffer behind, so take the current value. if (m_pBpmControl->getBeatDistance(getSampleOfTrack().current) >= 0.5) { targetDistance += 0.5; @@ -466,7 +444,6 @@ void SyncControl::setLocalBpm(double local_bpm) { if (local_bpm == m_prevLocalBpm.getValue()) { return; } - qDebug() << "SyncControl::setLocalBpm" << getGroup() << local_bpm; m_prevLocalBpm.setValue(local_bpm); SyncMode syncMode = getSyncMode(); diff --git a/src/engine/sync/synccontrol.h b/src/engine/sync/synccontrol.h index 32781b42ff3..357106fca94 100644 --- a/src/engine/sync/synccontrol.h +++ b/src/engine/sync/synccontrol.h @@ -113,7 +113,6 @@ 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; - double m_lastTargetBeatDistance; ControlValueAtomic m_prevLocalBpm; QAtomicInt m_audible; diff --git a/src/test/enginesynctest.cpp b/src/test/enginesynctest.cpp index 39a216db188..66351320754 100644 --- a/src/test/enginesynctest.cpp +++ b/src/test/enginesynctest.cpp @@ -1445,6 +1445,23 @@ TEST_F(EngineSyncTest, ZeroBPMRateAdjustIgnored) { ControlObject::getControl(ConfigKey(m_sGroup2, "rate"))->get()); } +TEST_F(EngineSyncTest, DISABLED_BeatDistanceBeforeStart) { + // If the start position is before zero, we should still initialize the beat distance + // corretly. Unfortunately, this currently doesn't work. + + mixxx::BeatsPointer pBeats1 = BeatFactory::makeBeatGrid(m_pTrack1->getSampleRate(), 128, 0.0); + m_pTrack1->trySetBeats(pBeats1); + ControlObject::set(ConfigKey(m_sGroup1, "playposition"), -.05); + ControlObject::getControl(ConfigKey(m_sGroup1, "sync_mode")) + ->set(SYNC_MASTER_SOFT); + ControlObject::getControl(ConfigKey(m_sGroup1, "play"))->set(1.0); + ProcessBuffer(); + EXPECT_NEAR( + ControlObject::getControl(ConfigKey(m_sGroup1, "beat_distance"))->get(), + ControlObject::getControl(ConfigKey(m_sInternalClockGroup, "beat_distance"))->get(), + kMaxBeatDistanceEpsilon); +} + TEST_F(EngineSyncTest, ZeroLatencyRateChangeNoQuant) { // Confirm that a rate change in an explicit master is instantly communicated // to followers. @@ -1660,9 +1677,7 @@ TEST_F(EngineSyncTest, HalfDoubleBpmTest) { ->set(SYNC_FOLLOWER); // Mixxx will choose the first playing deck to be master. Let's start deck 2 first. - qDebug() << "~~~~~~~~~~~~~~~~~~~~ ch2 play"; ControlObject::getControl(ConfigKey(m_sGroup2, "play"))->set(1.0); - qDebug() << "~~~~~~~~~~~~~~~~~~~~ ch1 play"; ControlObject::getControl(ConfigKey(m_sGroup1, "play"))->set(1.0); ProcessBuffer(); @@ -1999,9 +2014,7 @@ TEST_F(EngineSyncTest, SyncPhaseToPlayingNonSyncDeck) { // Next Deck becomes master and the Master clock is set to 100 BPM // The 130 BPM Track should be played at 100 BPM, rate = 0.769230769 pButtonSyncEnabled1->set(1.0); - qDebug() << "PUSH PLAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAY"; ControlObject::getControl(ConfigKey(m_sGroup1, "play"))->set(1.0); - // Beat length: 40707.692307692305 // Buffer size is 1024 at 44.1 kHz at the given rate = 787.692307692 Samples // Expected beat_distance = 0.019349962 From c52160167664dca0f08e4734b62bd44546004daa Mon Sep 17 00:00:00 2001 From: Owen Williams Date: Fri, 28 May 2021 18:53:48 -0400 Subject: [PATCH 03/22] Sync Lock: Fix clazy issue --- src/engine/sync/enginesync.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/engine/sync/enginesync.cpp b/src/engine/sync/enginesync.cpp index fad3c6ed696..1b8e876e010 100644 --- a/src/engine/sync/enginesync.cpp +++ b/src/engine/sync/enginesync.cpp @@ -615,7 +615,7 @@ void EngineSync::setMasterParams(Syncable* pSource) { // explicit Master and we should not initialize the beat distance. Take it from the // internal clock instead. bool playingSyncables = false; - for (Syncable* pSyncable : m_syncables) { + for (Syncable* pSyncable : qAsConst(m_syncables)) { if (pSyncable == pSource) { continue; } From 15cd3d402e0037a7a9221d45431841645ce53cf3 Mon Sep 17 00:00:00 2001 From: Owen Williams Date: Sat, 29 May 2021 13:35:39 -0400 Subject: [PATCH 04/22] sync lock: add bug link for disabled / broken test --- src/test/enginesynctest.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/test/enginesynctest.cpp b/src/test/enginesynctest.cpp index 66351320754..953c9405e62 100644 --- a/src/test/enginesynctest.cpp +++ b/src/test/enginesynctest.cpp @@ -1446,8 +1446,9 @@ TEST_F(EngineSyncTest, ZeroBPMRateAdjustIgnored) { } TEST_F(EngineSyncTest, DISABLED_BeatDistanceBeforeStart) { + // https://bugs.launchpad.net/mixxx/+bug/1930143 // If the start position is before zero, we should still initialize the beat distance - // corretly. Unfortunately, this currently doesn't work. + // correctly. Unfortunately, this currently doesn't work. mixxx::BeatsPointer pBeats1 = BeatFactory::makeBeatGrid(m_pTrack1->getSampleRate(), 128, 0.0); m_pTrack1->trySetBeats(pBeats1); From 35ed9c9470ac4711776fb03c2b0c98a3e319d545 Mon Sep 17 00:00:00 2001 From: Owen Williams Date: Mon, 31 May 2021 22:58:19 -0400 Subject: [PATCH 05/22] Sync Lock: don't use c++17 inline conditional assignment (1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Daniel Schürmann --- src/engine/sync/enginesync.cpp | 22 +++++++++++++--------- src/engine/sync/enginesync.h | 6 +++--- 2 files changed, 16 insertions(+), 12 deletions(-) diff --git a/src/engine/sync/enginesync.cpp b/src/engine/sync/enginesync.cpp index 1b8e876e010..f187d8b20bd 100644 --- a/src/engine/sync/enginesync.cpp +++ b/src/engine/sync/enginesync.cpp @@ -96,8 +96,9 @@ void EngineSync::requestSyncMode(Syncable* pSyncable, SyncMode mode) { } } - if (auto onlyPlayer = getOnlyPlayingSyncable(); onlyPlayer != nullptr) { - onlyPlayer->notifyOnlyPlayingSyncable(); + Syncable* pOnlyPlayer = getUniquePlayingSyncedDeck(); + if (pOnlyPlayer) { + pOnlyPlayer->notifyOnlyPlayingSyncable(); } } @@ -351,11 +352,14 @@ void EngineSync::notifyPlayingAudible(Syncable* pSyncable, bool playingAudible) if (newMaster != nullptr && newMaster != m_pMasterSyncable) { activateMaster(newMaster, SYNC_MASTER_SOFT); setMasterParams(newMaster); - } else if (auto onlyPlayer = getOnlyPlayingSyncable(); onlyPlayer != nullptr) { - // Even if we didn't change master, if there is only one player (us), then we should - // reinit the beat distance. - onlyPlayer->notifyOnlyPlayingSyncable(); - setMasterBeatDistance(onlyPlayer, onlyPlayer->getBeatDistance()); + } else { + Syncable* pOnlyPlayer = getUniquePlayingSyncedDeck(); + if (pOnlyPlayer) { + // Even if we didn't change master, if there is only one player (us), then we should + // reinit the beat distance. + pOnlyPlayer->notifyOnlyPlayingSyncable(); + setMasterBeatDistance(pOnlyPlayer, pOnlyPlayer->getBeatDistance()); + } } pSyncable->requestSync(); @@ -613,7 +617,7 @@ void EngineSync::setMasterParams(Syncable* pSource) { if (!pSource->isPlaying()) { // If the params source is not playing, but other syncables are, then we are a stopped // explicit Master and we should not initialize the beat distance. Take it from the - // internal clock instead. + // internal clock instead, because that will be up to date with the playing deck(s). bool playingSyncables = false; for (Syncable* pSyncable : qAsConst(m_syncables)) { if (pSyncable == pSource) { @@ -651,7 +655,7 @@ void EngineSync::setMasterParams(Syncable* pSource) { } } -Syncable* EngineSync::getOnlyPlayingSyncable() const { +Syncable* EngineSync::getUniquePlayingSyncedDeck() const { Syncable* onlyPlaying = nullptr; for (Syncable* pSyncable : m_syncables) { if (!pSyncable->isSynchronized()) { diff --git a/src/engine/sync/enginesync.h b/src/engine/sync/enginesync.h index 4988a14c422..5ce7574b520 100644 --- a/src/engine/sync/enginesync.h +++ b/src/engine/sync/enginesync.h @@ -115,9 +115,9 @@ class EngineSync : public SyncableListener { void setMasterParams(Syncable* pSource); - /// Iff there is a single playing syncable, return it. This is used to initialize master - /// params. - Syncable* getOnlyPlayingSyncable() const; + /// Iff there is a single playing syncable in sync mode, return it. + /// This is used to initialize master params. + Syncable* getUniquePlayingSyncedDeck() const; /// Only for testing. Do not use. Syncable* getSyncableForGroup(const QString& group); From ec01df82b8b8324014dd02a663470229f5965328 Mon Sep 17 00:00:00 2001 From: Owen Williams Date: Mon, 31 May 2021 23:32:29 -0400 Subject: [PATCH 06/22] Sync Lock: Fix issue where single playing sync deck was syncing against its nudge value --- src/engine/sync/enginesync.cpp | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/engine/sync/enginesync.cpp b/src/engine/sync/enginesync.cpp index f187d8b20bd..2e7a5bec784 100644 --- a/src/engine/sync/enginesync.cpp +++ b/src/engine/sync/enginesync.cpp @@ -75,6 +75,13 @@ void EngineSync::requestSyncMode(Syncable* pSyncable, SyncMode mode) { default:; } + Syncable* pOnlyPlayer = getUniquePlayingSyncedDeck(); + if (pOnlyPlayer) { + // This resets the user offset, so that if this deck gets used as the params syncable + // it will have that offset removed. + pOnlyPlayer->notifyOnlyPlayingSyncable(); + } + // Second, figure out what Syncable should be used to initialize the master // parameters, if any. Usually this is the new master. (Note, that pointer might be null!) Syncable* pParamsSyncable = m_pMasterSyncable; @@ -96,10 +103,6 @@ void EngineSync::requestSyncMode(Syncable* pSyncable, SyncMode mode) { } } - Syncable* pOnlyPlayer = getUniquePlayingSyncedDeck(); - if (pOnlyPlayer) { - pOnlyPlayer->notifyOnlyPlayingSyncable(); - } } void EngineSync::activateFollower(Syncable* pSyncable) { From b041a6ea33f52a13bded6c70e9c9ba8514a2f82e Mon Sep 17 00:00:00 2001 From: Owen Williams Date: Thu, 3 Jun 2021 14:20:25 -0400 Subject: [PATCH 07/22] Sync Lock: better name for function --- src/engine/sync/enginesync.cpp | 4 ++-- src/engine/sync/internalclock.cpp | 2 +- src/engine/sync/internalclock.h | 2 +- src/engine/sync/syncable.h | 2 +- src/engine/sync/synccontrol.cpp | 2 +- src/engine/sync/synccontrol.h | 2 +- 6 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/engine/sync/enginesync.cpp b/src/engine/sync/enginesync.cpp index 2e7a5bec784..474ff418e99 100644 --- a/src/engine/sync/enginesync.cpp +++ b/src/engine/sync/enginesync.cpp @@ -79,7 +79,7 @@ void EngineSync::requestSyncMode(Syncable* pSyncable, SyncMode mode) { if (pOnlyPlayer) { // This resets the user offset, so that if this deck gets used as the params syncable // it will have that offset removed. - pOnlyPlayer->notifyOnlyPlayingSyncable(); + pOnlyPlayer->notifyUniquePlaying(); } // Second, figure out what Syncable should be used to initialize the master @@ -360,7 +360,7 @@ void EngineSync::notifyPlayingAudible(Syncable* pSyncable, bool playingAudible) if (pOnlyPlayer) { // Even if we didn't change master, if there is only one player (us), then we should // reinit the beat distance. - pOnlyPlayer->notifyOnlyPlayingSyncable(); + pOnlyPlayer->notifyUniquePlaying(); setMasterBeatDistance(pOnlyPlayer, pOnlyPlayer->getBeatDistance()); } } diff --git a/src/engine/sync/internalclock.cpp b/src/engine/sync/internalclock.cpp index 8c3731f2397..734523d7e83 100644 --- a/src/engine/sync/internalclock.cpp +++ b/src/engine/sync/internalclock.cpp @@ -60,7 +60,7 @@ void InternalClock::setSyncMode(SyncMode mode) { m_pSyncMasterEnabled->setAndConfirm(SyncModeToMasterLight(mode)); } -void InternalClock::notifyOnlyPlayingSyncable() { +void InternalClock::notifyUniquePlaying() { // No action necessary. } diff --git a/src/engine/sync/internalclock.h b/src/engine/sync/internalclock.h index dea03a490d7..da0ddde7509 100644 --- a/src/engine/sync/internalclock.h +++ b/src/engine/sync/internalclock.h @@ -32,7 +32,7 @@ class InternalClock : public QObject, public Clock, public Syncable { } void setSyncMode(SyncMode mode) override; - void notifyOnlyPlayingSyncable() override; + void notifyUniquePlaying() override; void requestSync() override; SyncMode getSyncMode() const override { return m_mode; diff --git a/src/engine/sync/syncable.h b/src/engine/sync/syncable.h index fa29b1da915..1596b378cbb 100644 --- a/src/engine/sync/syncable.h +++ b/src/engine/sync/syncable.h @@ -76,7 +76,7 @@ class Syncable { virtual void setSyncMode(SyncMode mode) = 0; // Notify a Syncable that it is now the only currently-playing syncable. - virtual void notifyOnlyPlayingSyncable() = 0; + virtual void notifyUniquePlaying() = 0; // Notify a Syncable that they should sync phase. virtual void requestSync() = 0; diff --git a/src/engine/sync/synccontrol.cpp b/src/engine/sync/synccontrol.cpp index d387a22488c..728a51d9b39 100644 --- a/src/engine/sync/synccontrol.cpp +++ b/src/engine/sync/synccontrol.cpp @@ -145,7 +145,7 @@ void SyncControl::setSyncMode(SyncMode mode) { } } -void SyncControl::notifyOnlyPlayingSyncable() { +void SyncControl::notifyUniquePlaying() { // If we are the only remaining playing sync deck, we can reset the user // tweak info. m_pBpmControl->resetSyncAdjustment(); diff --git a/src/engine/sync/synccontrol.h b/src/engine/sync/synccontrol.h index 357106fca94..69af12e6c65 100644 --- a/src/engine/sync/synccontrol.h +++ b/src/engine/sync/synccontrol.h @@ -30,7 +30,7 @@ class SyncControl : public EngineControl, public Syncable { SyncMode getSyncMode() const override; void setSyncMode(SyncMode mode) override; - void notifyOnlyPlayingSyncable() override; + void notifyUniquePlaying() override; void requestSync() override; bool isPlaying() const override; bool isAudible() const override; From d8c843a676391f2f7d405dc19c81e7d9c6da74ae Mon Sep 17 00:00:00 2001 From: Owen Williams Date: Thu, 3 Jun 2021 15:05:54 -0400 Subject: [PATCH 08/22] SyncLock: Increase deck size to make room for Leader button --- res/skins/LateNight/decks/deck.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/res/skins/LateNight/decks/deck.xml b/res/skins/LateNight/decks/deck.xml index 9c42c500528..7f6d1228641 100644 --- a/res/skins/LateNight/decks/deck.xml +++ b/res/skins/LateNight/decks/deck.xml @@ -2,7 +2,7 @@ vertical - ,208 + ,230