-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Lock free beats #3668
Lock free beats #3668
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. Some minor code nits: I think we can make a bunch of variables const, use the SampleRate
type instead of SINT
and remove the d
prefix from variable names. I'll test this soon.
src/analyzer/analyzerbeats.cpp
Outdated
@@ -287,7 +286,7 @@ void AnalyzerBeats::storeResults(TrackPointer tio) { | |||
double currentFirstBeat = pCurrentBeats->findNextBeat(0); | |||
double newFirstBeat = pBeats->findNextBeat(0); | |||
if (currentFirstBeat == 0.0 && newFirstBeat > 0) { | |||
pCurrentBeats->translate(newFirstBeat); | |||
tio->setBeats(pCurrentBeats->translate(newFirstBeat)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't want you to make large renamings, but if I just look at the diff I have no idea what tio
is.
src/engine/controls/bpmcontrol.cpp
Outdated
if (v <= 0) { | ||
return; | ||
} | ||
TrackPointer pTrack = getEngineBuffer()->getLoadedTrack(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TrackPointer pTrack = getEngineBuffer()->getLoadedTrack(); | |
const TrackPointer pTrack = getEngineBuffer()->getLoadedTrack(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have changed this, but note that it makes only the address held by the TrackPointer const not the whole track.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know. I think you change the data that is pointed to, so making the data also const wouldn't work anyway.
src/engine/controls/bpmcontrol.cpp
Outdated
if (v <= 0) { | ||
return; | ||
} | ||
TrackPointer pTrack = getEngineBuffer()->getLoadedTrack(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TrackPointer pTrack = getEngineBuffer()->getLoadedTrack(); | |
const TrackPointer pTrack = getEngineBuffer()->getLoadedTrack(); |
src/engine/controls/bpmcontrol.cpp
Outdated
if (!pTrack) { | ||
return; | ||
} | ||
mixxx::BeatsPointer pBeats = pTrack->getBeats(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mixxx::BeatsPointer pBeats = pTrack->getBeats(); | |
const mixxx::BeatsPointer pBeats = pTrack->getBeats(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... and in the rest of this PR.
auto* pBeats = new mixxx::BeatMap(*track, static_cast<SINT>(sampleRate), beats); | ||
pBeats->setSubVersion(mixxx::rekordboxconstants::beatsSubversion); | ||
track->setBeats(mixxx::BeatsPointer(pBeats)); | ||
auto pBeats = mixxx::BeatMap::makeBeatMap( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
auto pBeats = mixxx::BeatMap::makeBeatMap( | |
const auto pBeats = mixxx::BeatMap::makeBeatMap( |
mixxx::BeatGrid* pGrid = new mixxx::BeatGrid(track, 0); | ||
pGrid->setGrid(dBpm, dFirstBeatSample); | ||
return mixxx::BeatsPointer(pGrid, &BeatFactory::deleteBeats); | ||
SINT sampleRate, double dBpm, double dFirstBeatSample) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you modify this function signature anyway, maybe we should use mixxx::audio::SampleRate
instead of SINT
? If this would make this PR much larger, then let's just leave it as it is.
Also, we don't use the d
prefix anymore.
: m_mutex(QMutex::Recursive), | ||
m_iSampleRate(iSampleRate > 0 ? iSampleRate : track.getSampleRate()), | ||
m_dBeatLength(0.0) { | ||
SINT iSampleRate, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, we should ideally use the samplerate type unless it makes this PR much larger.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have intentionaly excluded unrelated refactoring. Because it will of cause makes this PR bigger.
I don't mind to add it here or in a follow up. What do you prefer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then don't do it here. I just wanted to point it out because we should switch eventually.
Done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works fine, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Holzhaus Thanks for the review.
Sharing only immutable data is the preferred way to go. Unfortunately, this approach invalidates a lot of assumptions in the current code. Nevertheless I support this move!
if (!isValid()) { | ||
return; | ||
return BeatsPointer(new BeatGrid(*this)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This clone is redundant, theoretically. But it is the only safe solution now.
https://en.cppreference.com/w/cpp/memory/enable_shared_from_this/shared_from_this
Unfortunately this operation is not safe and could cause undefined behavior.
Maybe use immer::box
instead of std::shared_ptr
for sharing immutable objects?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may add a static wrapper around this function to do the isValid() check.
This removed some locking calls from the engine thread. These locks may cause a priority inversion when enable sync and using quantize. As a side effect the QObject base class has become useless and was also removed, along with the thread affinity code.
The lock was original introduced to protect the engine from reading half way edited data when the beat grid is changed. As a side effect also concurrent read calls are locking each other.
The solution implemented here, makes the beat classes const. This way they can't be edited and there is no reason for locking anymore. If changes are required they are introduced to a copy of the original object. Due to the wrapping shared pointer the old copy is valid until the last reader is finished. Changing to the new beats object is a atomic pointer replace.
This PR heavily conflicts with the new betas object #2961. This is luckily no issue at all, because the beat classes changed in this PR were removed. In a merge we can just discard the changes here.
The new beats class still has locking. This can be removed using this PR as a template.
This PR has grown larger than I have initially expected due to many similar changes in the code using the beats interface. That's hopefully no problem during review.