Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sync Lock: disallow selection of Explicit Leader #12431

Merged
merged 3 commits into from
Dec 15, 2023
Merged

Sync Lock: disallow selection of Explicit Leader #12431

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b077e55
Sync Lock: disallow selection of Explicit Leader
ywwg Dec 12, 2023
0dc81b4
Sync Lock: prefer an existing soft leader if it is valid
ywwg Dec 13, 2023
85d3d96
Sync Lock: revert ability to set soft leader on stopped decks.
ywwg Dec 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 27 additions & 23 deletions src/engine/sync/enginesync.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down Expand Up @@ -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()) {
Expand All @@ -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;
Expand All @@ -249,7 +249,7 @@ Syncable* EngineSync::pickNewLeader(Syncable* pEnablingSyncable) {
continue;
}

if (pSyncable != pEnablingSyncable) {
if (pSyncable != triggering_syncable) {
if (!pSyncable->getChannel()->isPrimaryDeck()) {
continue;
}
Expand All @@ -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++;
Expand All @@ -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) {
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
}
Expand Down
12 changes: 4 additions & 8 deletions src/engine/sync/enginesync.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
8 changes: 6 additions & 2 deletions src/engine/sync/synccontrol.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down Expand Up @@ -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) {
Expand Down
37 changes: 18 additions & 19 deletions src/test/enginesynctest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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<ControlProxy>(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 =
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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();
Expand All @@ -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();
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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();
Expand All @@ -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();
Expand Down
Loading