Skip to content

Commit

Permalink
Sync Lock: Differentiate track loading from beats updating
Browse files Browse the repository at this point in the history 
When we load a track, we want the deck to sync to other decks.
When we update beats of a Leader deck, we want it to remain at the same playback speed.
  • Loading branch information
@ywwg
ywwg committed Jun 16, 2021
1 parent 9e85796 commit eb3e428
Show file tree
Hide file tree
Showing 6 changed files with 77 additions and 50 deletions.
3 changes: 0 additions & 3 deletions src/engine/controls/bpmcontrol.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -302,9 +302,6 @@ void BpmControl::slotControlBeatSync(double value) {
}

bool BpmControl::syncTempo() {
if (getSyncMode() == SYNC_LEADER_EXPLICIT) {
return false;
}
EngineBuffer* pOtherEngineBuffer = pickSyncTarget();

if (!pOtherEngineBuffer) {
Expand Down
2 changes: 1 addition & 1 deletion src/engine/enginemaster.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,7 @@ void EngineMaster::processChannels(int iBufferSize) {
m_activeChannels.clear();

//ScopedTimer timer("EngineMaster::processChannels");
EngineChannel* pMasterChannel = m_pMasterSync->getLeader();
EngineChannel* pMasterChannel = m_pMasterSync->getLeaderChannel();
// Reserve the first place for the master channel which
// should be processed first
m_activeChannels.append(NULL);
Expand Down
11 changes: 5 additions & 6 deletions src/engine/sync/enginesync.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,10 +83,10 @@ void EngineSync::requestSyncMode(Syncable* pSyncable, SyncMode mode) {
}

// Second, figure out what Syncable should be used to initialize the leader
// parameters, if any. Usually this is the new leader. (Note, that pointer might be null!)
// parameters, if any. Usually this is the leader. (Note, that pointer might be null!)
Syncable* pParamsSyncable = m_pLeaderSyncable;
// But If we are newly soft leader, we need to match to some other deck.
if (pSyncable == m_pLeaderSyncable && pSyncable != oldLeader && mode != SYNC_LEADER_EXPLICIT) {
// But if we are newly leader, we need to match to some other deck.
if (pSyncable == m_pLeaderSyncable && pSyncable != oldLeader) {
pParamsSyncable = findBpmMatchTarget(pSyncable);
if (!pParamsSyncable) {
// We weren't able to find anything to match to, so set ourselves as the
Expand All @@ -107,7 +107,6 @@ void EngineSync::requestSyncMode(Syncable* pSyncable, SyncMode mode) {
pSyncable->requestSync();
}
}

}

void EngineSync::activateFollower(Syncable* pSyncable) {
Expand Down Expand Up @@ -208,7 +207,7 @@ Syncable* EngineSync::pickLeader(Syncable* enabling_syncable) {
return m_pLeaderSyncable;
}

// First preference: some other sync deck that is not playing.
// First preference: some other sync deck that is playing.
// Note, if we are using PREFER_LOCK_BPM we don't use this option.
Syncable* first_other_playing_deck = nullptr;
// Second preference: whatever the first playing sync deck is, even if it's us.
Expand Down Expand Up @@ -531,7 +530,7 @@ void EngineSync::onCallbackEnd(int sampleRate, int bufferSize) {
m_pInternalClock->onCallbackEnd(sampleRate, bufferSize);
}

EngineChannel* EngineSync::getLeader() const {
EngineChannel* EngineSync::getLeaderChannel() const {
return m_pLeaderSyncable ? m_pLeaderSyncable->getChannel() : nullptr;
}

Expand Down
2 changes: 1 addition & 1 deletion src/engine/sync/enginesync.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ class EngineSync : public SyncableListener {
bool otherSyncedPlaying(const QString& group);

void addSyncableDeck(Syncable* pSyncable);
EngineChannel* getLeader() const;
EngineChannel* getLeaderChannel() const;
void onCallbackStart(int sampleRate, int bufferSize);
void onCallbackEnd(int sampleRate, int bufferSize);

Expand Down
40 changes: 34 additions & 6 deletions src/engine/sync/synccontrol.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -332,21 +332,49 @@ void SyncControl::trackLoaded(TrackPointer pNewTrack) {
if (pNewTrack) {
pBeats = pNewTrack->getBeats();
}
trackBeatsUpdated(pBeats);
}

void SyncControl::trackBeatsUpdated(mixxx::BeatsPointer pBeats) {
// This slot is fired by a new file is loaded or if the user
// has adjusted the beatgrid.
if (kLogger.traceEnabled()) {
kLogger.trace() << getGroup() << "SyncControl::trackBeatsUpdated";
kLogger.trace() << getGroup() << "SyncControl::trackLoaded";
}

VERIFY_OR_DEBUG_ASSERT(m_pLocalBpm) {
// object not initialized
return;
}

bool hadBeats = m_pBeats != nullptr;
m_pBeats = pBeats;
m_leaderBpmAdjustFactor = kBpmUnity;

m_pBpmControl->updateLocalBpm();
if (isSynchronized()) {
if (!m_pBeats) {
// If we were soft leader and now we have no beats, go to follower.
// This is a bit of "enginesync" logic that has bled into this Syncable,
// is there a better way to handle "soft leaders no longer have bpm"?
if (getSyncMode() == SYNC_LEADER_SOFT) {
m_pChannel->getEngineBuffer()->requestSyncMode(SYNC_FOLLOWER);
}
return;
}

m_pBpmControl->syncTempo();
if (!hadBeats) {
// There is a chance we were beatless leader before, so we notify a basebpm change
// to possibly reinit leader params.
m_pChannel->getEngineBuffer()->requestSyncMode(getSyncMode());
m_pEngineSync->notifyBaseBpmChanged(this, getBaseBpm());
}
}
}

void SyncControl::trackBeatsUpdated(mixxx::BeatsPointer pBeats) {
// This slot is fired by if the user has adjusted the beatgrid.
if (kLogger.traceEnabled()) {
kLogger.trace() << getGroup() << "SyncControl::trackBeatsUpdated";
}

m_pBeats = pBeats;
m_leaderBpmAdjustFactor = kBpmUnity;

Expand All @@ -357,7 +385,7 @@ void SyncControl::trackBeatsUpdated(mixxx::BeatsPointer pBeats) {
// be leader.
m_pChannel->getEngineBuffer()->requestSyncMode(SYNC_FOLLOWER);
} else {
// We are remaining leader, so notify the engine with our update.
// We should not change playback speed.
m_pBpmControl->updateLocalBpm();
m_pEngineSync->notifyBaseBpmChanged(this, getBaseBpm());
}
Expand Down
69 changes: 36 additions & 33 deletions src/test/enginesynctest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ class EngineSyncTest : public MockedEngineBackendTest {
}

void assertNoLeader() {
EXPECT_EQ(NULL, m_pEngineSync->getLeader());
EXPECT_EQ(NULL, m_pEngineSync->getLeaderChannel());
EXPECT_EQ(NULL, m_pEngineSync->getLeaderSyncable());
}

Expand All @@ -106,7 +106,7 @@ class EngineSyncTest : public MockedEngineBackendTest {
qWarning() << "internal clock sync_leader should be 2.0, is" << leader;
return false;
}
if (m_pEngineSync->getLeader()) {
if (m_pEngineSync->getLeaderChannel()) {
qWarning() << "no current leader";
return false;
}
Expand All @@ -117,28 +117,28 @@ class EngineSyncTest : public MockedEngineBackendTest {
return true;
}
if (group == m_sGroup1) {
if (m_pEngineSync->getLeader() != m_pChannel1) {
if (m_pEngineSync->getLeaderChannel() != m_pChannel1) {
qWarning() << "leader pointer should be channel 1, is "
<< (m_pEngineSync->getLeader()
? m_pEngineSync->getLeader()
<< (m_pEngineSync->getLeaderChannel()
? m_pEngineSync->getLeaderChannel()
->getGroup()
: "null");
return false;
}
} else if (group == m_sGroup2) {
if (m_pEngineSync->getLeader() != m_pChannel2) {
if (m_pEngineSync->getLeaderChannel() != m_pChannel2) {
qWarning() << "leader pointer should be channel 2, is "
<< (m_pEngineSync->getLeader()
? m_pEngineSync->getLeader()
<< (m_pEngineSync->getLeaderChannel()
? m_pEngineSync->getLeaderChannel()
->getGroup()
: "null");
return false;
}
} else if (group == m_sGroup3) {
if (m_pEngineSync->getLeader() != m_pChannel3) {
if (m_pEngineSync->getLeaderChannel() != m_pChannel3) {
qWarning() << "leader pointer should be channel 3, is "
<< (m_pEngineSync->getLeader()
? m_pEngineSync->getLeader()
<< (m_pEngineSync->getLeaderChannel()
? m_pEngineSync->getLeaderChannel()
->getGroup()
: "null");
return false;
Expand Down Expand Up @@ -679,14 +679,13 @@ TEST_F(EngineSyncTest, RateChangeTestOrder3) {
EXPECT_DOUBLE_EQ(
120.0, ControlObject::get(ConfigKey(m_sGroup2, "file_bpm")));

// Turn on Leader. Setting explicit leader causes this track's rate to be adopted instead
// of matching against the other deck.
// Turn on Leader. Even though it is explict leader, it still matches the other deck.
auto pButtonLeaderSync1 =
std::make_unique<ControlProxy>(m_sGroup1, "sync_mode");
pButtonLeaderSync1->set(SYNC_LEADER_EXPLICIT);
ProcessBuffer();
EXPECT_TRUE(isExplicitLeader(m_sGroup1));
EXPECT_DOUBLE_EQ(160.0, ControlObject::get(ConfigKey(m_sGroup1, "bpm")));
EXPECT_DOUBLE_EQ(120.0, ControlObject::get(ConfigKey(m_sGroup1, "bpm")));

// Turn on follower.
auto pButtonLeaderSync2 =
Expand All @@ -695,12 +694,12 @@ TEST_F(EngineSyncTest, RateChangeTestOrder3) {
ProcessBuffer();

// Follower should immediately set its slider.
EXPECT_NEAR(getRateSliderValue(1.3333333333),
ControlObject::get(ConfigKey(m_sGroup2, "rate")),
EXPECT_NEAR(getRateSliderValue(0.75),
ControlObject::get(ConfigKey(m_sGroup1, "rate")),
kMaxFloatingPointErrorLowPrecision);
EXPECT_DOUBLE_EQ(160.0, ControlObject::get(ConfigKey(m_sGroup2, "bpm")));
EXPECT_DOUBLE_EQ(120.0, ControlObject::get(ConfigKey(m_sGroup2, "bpm")));
EXPECT_DOUBLE_EQ(
160.0, ControlObject::get(ConfigKey(m_sInternalClockGroup, "bpm")));
120.0, ControlObject::get(ConfigKey(m_sInternalClockGroup, "bpm")));
}

TEST_F(EngineSyncTest, FollowerRateChange) {
Expand Down Expand Up @@ -817,12 +816,12 @@ TEST_F(EngineSyncTest, LeaderStopSliderCheck) {
mixxx::BeatsPointer pBeats2 = BeatFactory::makeBeatGrid(m_pTrack2->getSampleRate(), 128, 0.0);
m_pTrack2->trySetBeats(pBeats2);

auto pButtonLeaderSync1 =
std::make_unique<ControlProxy>(m_sGroup1, "sync_mode");
pButtonLeaderSync1->slotSet(SYNC_LEADER_EXPLICIT);
auto pButtonLeaderSync2 =
std::make_unique<ControlProxy>(m_sGroup2, "sync_mode");
pButtonLeaderSync2->slotSet(SYNC_FOLLOWER);
auto pButtonLeaderSync1 =
std::make_unique<ControlProxy>(m_sGroup1, "sync_mode");
pButtonLeaderSync1->slotSet(SYNC_LEADER_EXPLICIT);
ProcessBuffer();

//EXPECT_TRUE(isExplicitLeader(m_sGroup1));
Expand Down Expand Up @@ -972,7 +971,7 @@ TEST_F(EngineSyncTest, LoadTrackInitializesLeader) {
pButtonSyncEnabled1->slotSet(1.0);

// No leader because this deck has no track.
EXPECT_EQ(NULL, m_pEngineSync->getLeader());
EXPECT_EQ(NULL, m_pEngineSync->getLeaderChannel());
EXPECT_DOUBLE_EQ(
0.0, ControlObject::getControl(ConfigKey(m_sGroup1, "bpm"))->get());

Expand All @@ -982,13 +981,12 @@ TEST_F(EngineSyncTest, LoadTrackInitializesLeader) {
EXPECT_DOUBLE_EQ(140.0,
ControlObject::getControl(ConfigKey(m_sGroup1, "bpm"))->get());

// But as soon as we play, deck 1 is leader
// Play button doesn't affect leader.
ControlObject::getControl(ConfigKey(m_sGroup1, "play"))->set(1.0);
EXPECT_TRUE(isSoftLeader(m_sGroup1));
ControlObject::getControl(ConfigKey(m_sGroup1, "play"))->set(0.0);

// If sync is on two decks and we load a track in only one of them, it will be
// leader.
// Ejecting the track has no effect on leader status
m_pChannel1->getEngineBuffer()->slotEjectTrack(1.0);
EXPECT_TRUE(isFollower(m_sGroup1));
// no relevant tempo available so internal clock is following
Expand Down Expand Up @@ -1035,7 +1033,8 @@ TEST_F(EngineSyncTest, LoadTrackResetTempoOption) {
std::make_unique<ControlProxy>(m_sGroup2, "sync_enabled");
pButtonSyncEnabled2->set(1.0);

// If sync is on and we load a track, that should initialize leader.
// If sync is on and we load a track, it should initialize leader because
// the other track has no bpm.
TrackPointer track1 = m_pMixerDeck1->loadFakeTrack(false, 140.0);

EXPECT_DOUBLE_EQ(
Expand Down Expand Up @@ -1068,12 +1067,13 @@ TEST_F(EngineSyncTest, LoadTrackResetTempoOption) {
EXPECT_DOUBLE_EQ(140.0, ControlObject::get(ConfigKey(m_sGroup2, "bpm")));

// Repeat with RESET_NONE
EXPECT_TRUE(isSoftLeader(m_sGroup1));
m_pConfig->set(ConfigKey("[Controls]", "SpeedAutoReset"),
ConfigValue(BaseTrackPlayer::RESET_NONE));
ControlObject::set(ConfigKey(m_sGroup1, "play"), 0.0);
ControlObject::set(ConfigKey(m_sGroup1, "rate"), getRateSliderValue(1.0));
ControlObject::set(ConfigKey(m_sGroup2, "rate"), getRateSliderValue(1.0));
track1 = m_pMixerDeck1->loadFakeTrack(false, 140.0);
track1 = m_pMixerDeck1->loadFakeTrack(false, 124.0);
ControlObject::set(ConfigKey(m_sGroup1, "play"), 1.0);
track2 = m_pMixerDeck2->loadFakeTrack(false, 128.0);
EXPECT_DOUBLE_EQ(
Expand Down Expand Up @@ -1188,7 +1188,7 @@ TEST_F(EngineSyncTest, SyncToNonSyncDeck) {
ConfigKey(m_sInternalClockGroup, "sync_leader")));
EXPECT_DOUBLE_EQ(
100.0, ControlObject::get(ConfigKey(m_sInternalClockGroup, "bpm")));
EXPECT_EQ(NULL, m_pEngineSync->getLeader());
EXPECT_EQ(NULL, m_pEngineSync->getLeaderChannel());
EXPECT_EQ(NULL, m_pEngineSync->getLeaderSyncable());
EXPECT_EQ(SYNC_NONE, ControlObject::get(ConfigKey(m_sGroup1, "sync_mode")));
EXPECT_EQ(0, ControlObject::get(ConfigKey(m_sGroup1, "sync_enabled")));
Expand All @@ -1213,7 +1213,7 @@ TEST_F(EngineSyncTest, SyncToNonSyncDeck) {
ConfigKey(m_sInternalClockGroup, "sync_leader")));
EXPECT_DOUBLE_EQ(
100.0, ControlObject::get(ConfigKey(m_sInternalClockGroup, "bpm")));
EXPECT_EQ(NULL, m_pEngineSync->getLeader());
EXPECT_EQ(NULL, m_pEngineSync->getLeaderChannel());
EXPECT_EQ(NULL, m_pEngineSync->getLeaderSyncable());
EXPECT_EQ(SYNC_NONE, ControlObject::get(ConfigKey(m_sGroup2, "sync_mode")));
EXPECT_EQ(0, ControlObject::get(ConfigKey(m_sGroup2, "sync_enabled")));
Expand Down Expand Up @@ -1350,7 +1350,7 @@ TEST_F(EngineSyncTest, EjectTrackSyncRemains) {
}

TEST_F(EngineSyncTest, FileBpmChangesDontAffectLeader) {
// If filebpm changes, don't treat it like a rate change unless it's the leader.
// If filebpm changes, don't treat it like a rate change.
mixxx::BeatsPointer pBeats1 = BeatFactory::makeBeatGrid(m_pTrack1->getSampleRate(), 100, 0.0);
m_pTrack1->trySetBeats(pBeats1);
auto pButtonSyncEnabled1 =
Expand All @@ -1371,6 +1371,8 @@ TEST_F(EngineSyncTest, FileBpmChangesDontAffectLeader) {
// Update the leader's beats -- update the internal clock
pBeats1 = BeatFactory::makeBeatGrid(m_pTrack1->getSampleRate(), 160, 0.0);
m_pTrack1->trySetBeats(pBeats1);
EXPECT_DOUBLE_EQ(
160.0, ControlObject::get(ConfigKey(m_sGroup1, "bpm")));
EXPECT_DOUBLE_EQ(
160.0, ControlObject::get(ConfigKey(m_sInternalClockGroup, "bpm")));

Expand Down Expand Up @@ -2287,9 +2289,10 @@ TEST_F(EngineSyncTest, FollowerUserTweakPreservedInLeaderChange) {
// This is about 128 bpm, but results in nice round numbers of samples.
const double kDivisibleBpm = 44100.0 / 344.0;
mixxx::BeatsPointer pBeats1 = BeatFactory::makeBeatGrid(
m_pTrack1->getSampleRate(), kDivisibleBpm, 0.0);
m_pTrack1->getSampleRate(), 130, 0.0);
m_pTrack1->trySetBeats(pBeats1);
mixxx::BeatsPointer pBeats2 = BeatFactory::makeBeatGrid(m_pTrack2->getSampleRate(), 130, 0.0);
mixxx::BeatsPointer pBeats2 = BeatFactory::makeBeatGrid(
m_pTrack2->getSampleRate(), kDivisibleBpm, 0.0);
m_pTrack2->trySetBeats(pBeats2);

ControlObject::getControl(ConfigKey(m_sGroup1, "sync_leader"))->set(1);
Expand Down Expand Up @@ -2344,8 +2347,8 @@ TEST_F(EngineSyncTest, LeaderUserTweakPreservedInLeaderChange) {
mixxx::BeatsPointer pBeats2 = BeatFactory::makeBeatGrid(m_pTrack2->getSampleRate(), 130, 0.0);
m_pTrack2->trySetBeats(pBeats2);

ControlObject::getControl(ConfigKey(m_sGroup1, "sync_leader"))->set(1);
ControlObject::getControl(ConfigKey(m_sGroup2, "sync_enabled"))->set(1);
ControlObject::getControl(ConfigKey(m_sGroup1, "sync_leader"))->set(1);
ControlObject::set(ConfigKey(m_sGroup1, "quantize"), 1.0);
ControlObject::set(ConfigKey(m_sGroup2, "quantize"), 1.0);
ControlObject::set(ConfigKey(m_sGroup1, "play"), 1.0);
Expand Down

0 comments on commit eb3e428

Please sign in to comment.