Skip to content

Commit

Permalink
Sync Lock: Differentiate track loading from beats updating
Browse files Browse the repository at this point in the history
  • Loading branch information
@ywwg
ywwg committed Jul 23, 2021
1 parent 0fb764a commit 0d1eb3f
Show file tree
Hide file tree
Showing 6 changed files with 75 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() == SyncMode::LeaderExplicit) {
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
10 changes: 4 additions & 6 deletions src/engine/sync/enginesync.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,9 +85,8 @@ 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!)
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 != SyncMode::LeaderExplicit) {
// 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 @@ -108,7 +107,6 @@ void EngineSync::requestSyncMode(Syncable* pSyncable, SyncMode mode) {
pSyncable->requestSync();
}
}

}

void EngineSync::activateFollower(Syncable* pSyncable) {
Expand Down Expand Up @@ -210,7 +208,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 @@ -533,7 +531,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() == SyncMode::LeaderSoft) {
m_pChannel->getEngineBuffer()->requestSyncMode(SyncMode::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(SyncMode::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
68 changes: 35 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 @@ -702,14 +702,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 explicit leader, it still matches the other deck.
auto pButtonLeaderSync1 =
std::make_unique<ControlProxy>(m_sGroup1, "sync_mode");
pButtonLeaderSync1->set(static_cast<double>(SyncMode::LeaderExplicit));
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 @@ -718,12 +717,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 @@ -846,12 +845,12 @@ TEST_F(EngineSyncTest, LeaderStopSliderCheck) {
m_pTrack2->getSampleRate(), mixxx::Bpm(128), mixxx::audio::kStartFramePos);
m_pTrack2->trySetBeats(pBeats2);

auto pButtonLeaderSync1 =
std::make_unique<ControlProxy>(m_sGroup1, "sync_mode");
pButtonLeaderSync1->set(static_cast<double>(SyncMode::LeaderExplicit));
auto pButtonLeaderSync2 =
std::make_unique<ControlProxy>(m_sGroup2, "sync_mode");
pButtonLeaderSync2->set(static_cast<double>(SyncMode::Follower));
auto pButtonLeaderSync1 =
std::make_unique<ControlProxy>(m_sGroup1, "sync_mode");
pButtonLeaderSync1->set(static_cast<double>(SyncMode::LeaderExplicit));
ProcessBuffer();

//EXPECT_TRUE(isExplicitLeader(m_sGroup1));
Expand Down Expand Up @@ -1006,7 +1005,7 @@ TEST_F(EngineSyncTest, LoadTrackInitializesLeader) {
pButtonSyncEnabled1->set(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 @@ -1016,13 +1015,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 @@ -1069,7 +1067,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 @@ -1102,12 +1101,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 @@ -1225,7 +1225,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(static_cast<double>(SyncMode::None),
ControlObject::get(ConfigKey(m_sGroup1, "sync_mode")));
Expand All @@ -1251,7 +1251,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(static_cast<double>(SyncMode::None),
ControlObject::get(ConfigKey(m_sGroup2, "sync_mode")));
Expand Down Expand Up @@ -1393,7 +1393,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(), mixxx::Bpm(100), mixxx::audio::kStartFramePos);
m_pTrack1->trySetBeats(pBeats1);
Expand All @@ -1417,6 +1417,8 @@ TEST_F(EngineSyncTest, FileBpmChangesDontAffectLeader) {
pBeats1 = BeatFactory::makeBeatGrid(
m_pTrack1->getSampleRate(), mixxx::Bpm(160), mixxx::audio::kStartFramePos);
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 @@ -2365,10 +2367,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(), mixxx::Bpm(kDivisibleBpm), mixxx::audio::kStartFramePos);
m_pTrack1->getSampleRate(), mixxx::Bpm(130), mixxx::audio::kStartFramePos);
m_pTrack1->trySetBeats(pBeats1);
mixxx::BeatsPointer pBeats2 = BeatFactory::makeBeatGrid(
m_pTrack2->getSampleRate(), mixxx::Bpm(130), mixxx::audio::kStartFramePos);
m_pTrack2->getSampleRate(), mixxx::Bpm(kDivisibleBpm), mixxx::audio::kStartFramePos);
m_pTrack2->trySetBeats(pBeats2);

ControlObject::getControl(ConfigKey(m_sGroup1, "sync_leader"))->set(1);
Expand Down Expand Up @@ -2424,8 +2426,8 @@ TEST_F(EngineSyncTest, LeaderUserTweakPreservedInLeaderChange) {
m_pTrack2->getSampleRate(), mixxx::Bpm(130), mixxx::audio::kStartFramePos);
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 0d1eb3f

Please sign in to comment.