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.

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

Fix issues with half/double and Sync Lock #3899

Merged
merged 27 commits into from
Jun 15, 2021
Merged
Changes from 1 commit
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
507bb03
Fix half-double initialization issues
ywwg May 23, 2021
0718058
Sync Lock: Fix more half/double issues and clean up for PR
ywwg May 23, 2021
c521601
Sync Lock: Fix clazy issue
ywwg May 28, 2021
15cd3d4
sync lock: add bug link for disabled / broken test
ywwg May 29, 2021
35ed9c9
Sync Lock: don't use c++17 inline conditional assignment (1)
ywwg Jun 1, 2021
ec01df8
Sync Lock: Fix issue where single playing sync deck was syncing again…
ywwg Jun 1, 2021
b041a6e
Sync Lock: better name for function
ywwg Jun 3, 2021
d8c843a
SyncLock: Increase deck size to make room for Leader button
ywwg Jun 3, 2021
8984df9
Merge branch 'main' into stopped-explicit
ywwg Jun 3, 2021
ae18e4e
SyncLock: Fix leader button in latenight classic
ywwg Jun 3, 2021
0e03653
SyncLock: Skin fixes
ywwg Jun 3, 2021
58d0e4b
Sync Lock: Fix Leader button colors
ywwg Jun 4, 2021
534eacf
Fix half-double initialization issues
ywwg May 23, 2021
bd560c0
Sync Lock: Fix more half/double issues and clean up for PR
ywwg May 23, 2021
4e9f8f4
Sync Lock: Fix clazy issue
ywwg May 28, 2021
4ccdebe
sync lock: add bug link for disabled / broken test
ywwg May 29, 2021
259cfa5
Sync Lock: don't use c++17 inline conditional assignment (1)
ywwg Jun 1, 2021
d450bca
Sync Lock: Fix issue where single playing sync deck was syncing again…
ywwg Jun 1, 2021
b0c51f0
Sync Lock: better name for function
ywwg Jun 3, 2021
372d9e4
SyncLock: Increase deck size to make room for Leader button
ywwg Jun 3, 2021
08cead7
SyncLock: Fix leader button in latenight classic
ywwg Jun 3, 2021
7efe916
SyncLock: Skin fixes
ywwg Jun 3, 2021
fc5813f
Sync Lock: Fix Leader button colors
ywwg Jun 4, 2021
257b943
Merge branch 'main' into stopped-explicit
ywwg Jun 5, 2021
daba011
Merge remote-tracking branch 'origin/stopped-explicit' into stopped-e…
ywwg Jun 5, 2021
7b56e59
Merge branch 'main' into stopped-explicit
ywwg Jun 6, 2021
3e331c9
Merge branch 'main' into stopped-explicit
ywwg Jun 11, 2021
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
Prev Previous commit
Next Next commit
Sync Lock: Fix more half/double issues and clean up for PR
ywwg committed Jun 1, 2021
commit 0718058bfc098070bd6323d0e64d9d9a505b7418
6 changes: 4 additions & 2 deletions src/engine/controls/bpmcontrol.cpp
Original file line number Diff line number Diff line change
@@ -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());
1 change: 0 additions & 1 deletion src/engine/enginebuffer.cpp
Original file line number Diff line number Diff line change
@@ -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();
8 changes: 3 additions & 5 deletions src/engine/enginemaster.cpp
Original file line number Diff line number Diff line change
@@ -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
83 changes: 29 additions & 54 deletions src/engine/sync/enginesync.cpp
Original file line number Diff line number Diff line change
@@ -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) {
ywwg marked this conversation as resolved.
Show resolved Hide resolved
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) {
ywwg marked this conversation as resolved.
Show resolved Hide resolved
// 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,72 +603,68 @@ 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.
ywwg marked this conversation as resolved.
Show resolved Hide resolved
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;
}
pSyncable->setMasterParams(beatDistance, baseBpm, bpm);
}
}

QList<Syncable*> EngineSync::getPlayingSyncables() const {
QList<Syncable*> players;
Syncable* EngineSync::getOnlyPlayingSyncable() const {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the function name can be improved.
Only playing can also mean that it is playing without being synced or without being audible.
Something like:
getLonlyPlayingSyncronizedSyncable();
getLonlyPlayingSyncable();

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you mean "lonely" ? :) I tried "getUniquePlayingSyncedDeck"

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;
}
5 changes: 3 additions & 2 deletions src/engine/sync/enginesync.h
Original file line number Diff line number Diff line change
@@ -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<Syncable*> 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);
2 changes: 1 addition & 1 deletion src/engine/sync/syncable.h
Original file line number Diff line number Diff line change
@@ -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
31 changes: 4 additions & 27 deletions src/engine/sync/synccontrol.cpp
Original file line number Diff line number Diff line change
@@ -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) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very proud of this fix. It was hard to track down.

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();
1 change: 0 additions & 1 deletion src/engine/sync/synccontrol.h
Original file line number Diff line number Diff line change
@@ -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<double> m_prevLocalBpm;
QAtomicInt m_audible;

21 changes: 17 additions & 4 deletions src/test/enginesynctest.cpp
Original file line number Diff line number Diff line change
@@ -1445,6 +1445,23 @@ TEST_F(EngineSyncTest, ZeroBPMRateAdjustIgnored) {
ControlObject::getControl(ConfigKey(m_sGroup2, "rate"))->get());
}

TEST_F(EngineSyncTest, DISABLED_BeatDistanceBeforeStart) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you planning to fix this in this PR?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no -- this is a much more complex issue and I'm not even sure what's going wrong. This is a pretty rare circumstance, and shouldn't be a release-blocker, let alone a PR-blocker.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// 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