diff --git a/src/engine/sync/enginesync.cpp b/src/engine/sync/enginesync.cpp index c918b922418..5f6b6f51812 100644 --- a/src/engine/sync/enginesync.cpp +++ b/src/engine/sync/enginesync.cpp @@ -65,13 +65,13 @@ void EngineSync::requestSyncMode(Syncable* pSyncable, SyncMode mode) { [[fallthrough]]; } case SyncMode::Follower: { - // This request is also used to verifies and moves a soft leader + // This request is also used to verify and move a soft leader if (!m_pLeaderSyncable || m_pLeaderSyncable == pSyncable || m_pLeaderSyncable->getSyncMode() != SyncMode::LeaderExplicit) { // Pick a new leader, in case we would have none after becoming follower - Syncable* pNewLeader = pickNewLeader(pSyncable); - // Note: A request for follower mode may have been converted into - // enabling of soft leader mode if this still be best choice + Syncable* pNewLeader = pickLeader(pSyncable, false); + // Note: A request for follower mode may have been converted into enabling of soft + // leader mode if this syncable is still the best choice. if (pNewLeader) { activateLeader(pNewLeader, SyncMode::LeaderSoft); } @@ -209,14 +209,17 @@ void EngineSync::deactivateSync(Syncable* pSyncable) { } if (wasLeader) { - Syncable* newLeader = pickNewLeader(nullptr); + Syncable* newLeader = pickLeader(nullptr, false); if (newLeader != nullptr) { activateLeader(newLeader, SyncMode::LeaderSoft); } } } -Syncable* EngineSync::pickLeader(Syncable* pEnablingSyncable) { +Syncable* EngineSync::pickLeader(Syncable* triggering_syncable, bool newStatus) { + if (kLogger.traceEnabled()) { + kLogger.trace() << "EngineSync::pickLeader"; + } if (m_pLeaderSyncable && m_pLeaderSyncable->getSyncMode() == SyncMode::LeaderExplicit && m_pLeaderSyncable->getBaseBpm().isValid()) { @@ -225,21 +228,18 @@ Syncable* EngineSync::pickLeader(Syncable* pEnablingSyncable) { } return m_pLeaderSyncable; } - return pickNewLeader(pEnablingSyncable); -} -Syncable* EngineSync::pickNewLeader(Syncable* pEnablingSyncable) { - if (kLogger.traceEnabled()) { - kLogger.trace() << "EngineSync::pickNewLeader"; - } - // First preference: some other sync deck that is playing. - // Note, if we are using PREFER_LOCK_BPM we don't use this option. + // TODO: We should probably convert this function to use a ranking system and then pick the + // deck with the top rank, rather than this hacky series of conditionals. + bool leaderIsValid = (m_pLeaderSyncable + // The current leader is not valid if it's the triggering_syncable + // and it's being turned off. + && (m_pLeaderSyncable != triggering_syncable || newStatus) && + m_pLeaderSyncable->isPlaying() && + m_pLeaderSyncable->getBaseBpm().isValid()); Syncable* first_other_playing_deck = nullptr; - // Second preference: whatever the first playing sync deck is, even if it's us. Syncable* first_playing_deck = nullptr; - // Third preference: the first stopped sync deck. Syncable* first_stopped_deck = nullptr; - // Last resorts: Internal Clock or nullptr. int stopped_deck_count = 0; int playing_deck_count = 0; @@ -249,7 +249,7 @@ Syncable* EngineSync::pickNewLeader(Syncable* pEnablingSyncable) { continue; } - if (pSyncable != pEnablingSyncable) { + if (pSyncable != triggering_syncable) { if (!pSyncable->getChannel()->isPrimaryDeck()) { continue; } @@ -262,7 +262,7 @@ Syncable* EngineSync::pickNewLeader(Syncable* pEnablingSyncable) { if (playing_deck_count == 0) { first_playing_deck = pSyncable; } - if (!first_other_playing_deck && pSyncable != pEnablingSyncable) { + if (!first_other_playing_deck && pSyncable != triggering_syncable) { first_other_playing_deck = pSyncable; } playing_deck_count++; @@ -283,7 +283,13 @@ Syncable* EngineSync::pickNewLeader(Syncable* pEnablingSyncable) { if (playing_deck_count == 1) { return first_playing_deck; } else if (playing_deck_count > 1) { - return first_other_playing_deck; + // Prefer keeping the current leader rather than switching it with the first playing + // deck. + if (leaderIsValid) { + return m_pLeaderSyncable; + } else { + return first_other_playing_deck; + } } if (stopped_deck_count >= 1) { @@ -382,7 +388,7 @@ void EngineSync::notifyPlayingAudible(Syncable* pSyncable, bool playingAudible) } // similar to enablesync -- we pick a new leader and maybe reinit. - Syncable* newLeader = pickLeader(pSyncable); + Syncable* newLeader = pickLeader(pSyncable, playingAudible); if (newLeader != nullptr && newLeader != m_pLeaderSyncable) { activateLeader(newLeader, SyncMode::LeaderSoft); @@ -574,7 +580,6 @@ bool EngineSync::otherSyncedPlaying(const QString& group) { void EngineSync::addSyncableDeck(Syncable* pSyncable) { if (m_syncables.contains(pSyncable)) { - qDebug() << "EngineSync: already has" << pSyncable; return; } m_syncables.append(pSyncable); @@ -632,7 +637,6 @@ mixxx::Bpm EngineSync::leaderBaseBpm() const { } void EngineSync::updateLeaderBpm(Syncable* pSource, mixxx::Bpm bpm) { - //qDebug() << "EngineSync::updateLeaderBpm" << pSource << bpm; if (pSource != m_pInternalClock) { m_pInternalClock->updateLeaderBpm(bpm); } diff --git a/src/engine/sync/enginesync.h b/src/engine/sync/enginesync.h index 6b9ec5db0ea..c5c4a5ae9b9 100644 --- a/src/engine/sync/enginesync.h +++ b/src/engine/sync/enginesync.h @@ -75,14 +75,10 @@ class EngineSync : public SyncableListener { void onCallbackEnd(mixxx::audio::SampleRate sampleRate, int bufferSize); private: - /// Iterate over decks, and based on sync and play status, pick a new Leader. - /// if enabling_syncable is not null, we treat it as if it were enabled because we may - /// be in the process of enabling it. - Syncable* pickNewLeader(Syncable* enabling_syncable); - - /// Return the explicit leader if the one has been selected or picks a new leader using - /// pickNewLeader(); - Syncable* pickLeader(Syncable* enabling_syncable); + /// Iterate over decks, and based on sync and play status, pick a new Leader, or return the + /// explicit leader if the one has been selected. If triggering_syncable is not null, we treat + /// it as if it had newStatus because we may be in the process of enabling or disabling it. + Syncable* pickLeader(Syncable* triggering_syncable, bool newStatus); /// Find a deck to match against, used in the case where there is no sync Leader. /// Looks first for a playing deck, and falls back to the first non-playing deck. diff --git a/src/engine/sync/synccontrol.cpp b/src/engine/sync/synccontrol.cpp index 100125d6967..4cdc3693c02 100644 --- a/src/engine/sync/synccontrol.cpp +++ b/src/engine/sync/synccontrol.cpp @@ -471,7 +471,7 @@ void SyncControl::slotControlBeatSync(double value) { void SyncControl::slotControlPlay(double play) { if (kLogger.traceEnabled()) { - kLogger.trace() << "SyncControl::slotControlPlay" << getSyncMode() << play; + kLogger.trace() << "SyncControl::slotControlPlay" << getGroup() << getSyncMode() << play; } m_pEngineSync->notifyPlayingAudible(this, play > 0.0 && m_audible); } @@ -512,7 +512,11 @@ void SyncControl::slotSyncLeaderEnabledChangeRequest(double state) { qDebug() << "Disallowing enabling of sync mode when passthrough active"; return; } - m_pChannel->getEngineBuffer()->requestSyncMode(SyncMode::LeaderExplicit); + // NOTE: This branch would normally activate Explicit Leader mode. Due to the large number + // of side effects and bugs, this mode is disabled. For now, requesting explicit leader mode + // only activates the chosen deck as the soft leader. See: + // https://github.com/mixxxdj/mixxx/issues/11788 + m_pChannel->getEngineBuffer()->requestSyncMode(SyncMode::LeaderSoft); } else { // Turning off leader goes back to follower mode. switch (mode) { diff --git a/src/test/enginesynctest.cpp b/src/test/enginesynctest.cpp index b4f3773b3bb..c60f60500ca 100644 --- a/src/test/enginesynctest.cpp +++ b/src/test/enginesynctest.cpp @@ -356,8 +356,8 @@ TEST_F(EngineSyncTest, DisableInternalLeaderWhilePlaying) { ProcessBuffer(); // This is not allowed, Internal should still be leader. - EXPECT_TRUE(isFollower(m_sInternalClockGroup)); - EXPECT_EQ(0, pButtonLeaderSync->get()); + EXPECT_TRUE(isSoftLeader(m_sInternalClockGroup)); + EXPECT_EQ(1, pButtonLeaderSync->get()); } TEST_F(EngineSyncTest, DisableSyncOnLeader) { @@ -372,20 +372,20 @@ TEST_F(EngineSyncTest, DisableSyncOnLeader) { mixxx::BeatsPointer pBeats2 = mixxx::Beats::fromConstTempo( m_pTrack2->getSampleRate(), mixxx::audio::kStartFramePos, mixxx::Bpm(130)); m_pTrack2->trySetBeats(pBeats2); - // Set deck two to explicit leader. + // Set deck two to leader, but this is not allowed because it's stopped. auto pButtonSyncLeader2 = std::make_unique(m_sGroup2, "sync_leader"); pButtonSyncLeader2->set(1.0); ProcessBuffer(); - EXPECT_TRUE(isFollower(m_sGroup1)); - EXPECT_TRUE(isExplicitLeader(m_sGroup2)); + EXPECT_TRUE(isSoftLeader(m_sGroup1)); + EXPECT_TRUE(isFollower(m_sGroup2)); // Set deck 2 to playing, now it becomes explicit leader. ControlObject::getControl(ConfigKey(m_sGroup2, "play"))->set(1.0); // The request to become leader is queued, so we have to process a buffer. ProcessBuffer(); EXPECT_TRUE(isFollower(m_sGroup1)); - EXPECT_TRUE(isExplicitLeader(m_sGroup2)); + EXPECT_TRUE(isSoftLeader(m_sGroup2)); // Unset enabled on channel2, it should work. auto pButtonSyncEnabled2 = @@ -557,7 +557,7 @@ TEST_F(EngineSyncTest, SetExplicitLeaderByLights) { ProcessBuffer(); // The sync lock should now be channel 1. - EXPECT_TRUE(isExplicitLeader(m_sGroup1)); + EXPECT_TRUE(isSoftLeader(m_sGroup1)); // Set channel 2 to be follower. pButtonSyncEnabled2->set(1); @@ -571,31 +571,30 @@ TEST_F(EngineSyncTest, SetExplicitLeaderByLights) { // Now channel 2 should be leader, and channel 1 should be a follower. EXPECT_TRUE(isFollower(m_sGroup1)); - EXPECT_TRUE(isExplicitLeader(m_sGroup2)); + EXPECT_TRUE(isSoftLeader(m_sGroup2)); // Now back again. pButtonSyncLeader1->set(1); ProcessBuffer(); // Now channel 1 should be leader, and channel 2 should be a follower. - EXPECT_TRUE(isExplicitLeader(m_sGroup1)); + EXPECT_TRUE(isSoftLeader(m_sGroup1)); EXPECT_TRUE(isFollower(m_sGroup2)); // Now set channel 1 to not-leader. - // This will choose automatically a Soft Leader, but prefers the old - // explicit leader it is still playing with a valid BPM + // This will choose automatically a Soft Leader, so it chooses the other deck. pButtonSyncLeader1->set(0); ProcessBuffer(); EXPECT_TRUE(isFollower(m_sInternalClockGroup)); - EXPECT_TRUE(isSoftLeader(m_sGroup1)); - EXPECT_TRUE(isFollower(m_sGroup2)); + EXPECT_TRUE(isFollower(m_sGroup1)); + EXPECT_TRUE(isSoftLeader(m_sGroup2)); // Try again without playing pButtonSyncLeader1->set(1); ProcessBuffer(); - EXPECT_TRUE(isExplicitLeader(m_sGroup1)); + EXPECT_TRUE(isSoftLeader(m_sGroup1)); EXPECT_TRUE(isFollower(m_sGroup2)); ControlObject::set(ConfigKey(m_sGroup1, "play"), 0.0); @@ -2399,7 +2398,7 @@ TEST_F(EngineSyncTest, FollowerUserTweakPreservedInLeaderChange) { ControlObject::set(ConfigKey(m_sGroup1, "play"), 1.0); ControlObject::set(ConfigKey(m_sGroup2, "play"), 1.0); - EXPECT_TRUE(isExplicitLeader(m_sGroup1)); + EXPECT_TRUE(isSoftLeader(m_sGroup1)); EXPECT_TRUE(isFollower(m_sGroup2)); ProcessBuffer(); @@ -2419,7 +2418,7 @@ TEST_F(EngineSyncTest, FollowerUserTweakPreservedInLeaderChange) { ControlObject::getControl(ConfigKey(m_sGroup2, "sync_leader"))->set(1); ProcessBuffer(); EXPECT_TRUE(isFollower(m_sGroup1)); - EXPECT_TRUE(isExplicitLeader(m_sGroup2)); + EXPECT_TRUE(isSoftLeader(m_sGroup2)); for (int i = 0; i < 10; ++i) { ProcessBuffer(); @@ -2452,7 +2451,7 @@ TEST_F(EngineSyncTest, FollowerUserTweakPreservedInSyncDisable) { m_pChannel2->getEngineBuffer() ->m_pBpmControl->m_dUserOffset.setValue(0.3); - EXPECT_TRUE(isExplicitLeader(m_sGroup1)); + EXPECT_TRUE(isSoftLeader(m_sGroup1)); EXPECT_TRUE(isFollower(m_sGroup2)); ProcessBuffer(); @@ -2483,7 +2482,7 @@ TEST_F(EngineSyncTest, LeaderUserTweakPreservedInLeaderChange) { ControlObject::set(ConfigKey(m_sGroup1, "play"), 1.0); ControlObject::set(ConfigKey(m_sGroup2, "play"), 1.0); - EXPECT_TRUE(isExplicitLeader(m_sGroup1)); + EXPECT_TRUE(isSoftLeader(m_sGroup1)); EXPECT_TRUE(isFollower(m_sGroup2)); ProcessBuffer(); @@ -2507,7 +2506,7 @@ TEST_F(EngineSyncTest, LeaderUserTweakPreservedInLeaderChange) { ControlObject::getControl(ConfigKey(m_sGroup2, "sync_leader"))->set(1); ProcessBuffer(); EXPECT_TRUE(isFollower(m_sGroup1)); - EXPECT_TRUE(isExplicitLeader(m_sGroup2)); + EXPECT_TRUE(isSoftLeader(m_sGroup2)); for (int i = 0; i < 10; ++i) { ProcessBuffer();