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

Auto DJ skip fix lp1941989 #4319

Merged
merged 8 commits into from Sep 23, 2021
Merged

Auto DJ skip fix lp1941989 #4319

merged 8 commits into from Sep 23, 2021

Conversation

daschuer
Copy link
Member

This should fix https://bugs.launchpad.net/mixxx/+bug/1941989 where tracks are skipped in AutoDJ.

This is hard to test, because skipping happens only under rare conditions. I was able to reproduce it unreliable with the "Full Track Mode" and a slow spinning USB HDD. With this PR I cannot reproduce it anymore.

The fix is implemented in two places:

  • Fix the symptom: Remove isFromDeck flag before loading a new track, to avoid doing transition twice.
  • The guessed Root case: The engine was able to request new chunks of an already replaced track.

In rare conditions, we receive a stray position update which leads to a repeated loadNextTrackFromQueue() call, discarding the track currently loading.
This effectively skips a track. Fixing lp:941989
@daschuer daschuer changed the base branch from main to 2.3 September 21, 2021 23:24
TrackPointer pLoadTrack;
{ // locking scope
QMutexLocker locker(&m_newTrackMutex);
pLoadTrack = m_pNewTrack;
m_pNewTrack.reset();
m_newTrackAvailable = false;
m_newTrackAvailable = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

m_newTrackAvailable.storeRelease(0)

src/engine/cachingreader/cachingreaderworker.cpp Outdated Show resolved Hide resolved
src/engine/enginebuffer.cpp Outdated Show resolved Hide resolved
src/engine/cachingreader/cachingreaderworker.cpp Outdated Show resolved Hide resolved
src/engine/cachingreader/cachingreaderworker.cpp Outdated Show resolved Hide resolved
@uklotzde
Copy link
Contributor

Probably related? https://bugs.launchpad.net/mixxx/+bug/1859374

src/engine/cachingreader/cachingreaderworker.cpp Outdated Show resolved Hide resolved
src/engine/cachingreader/cachingreaderworker.cpp Outdated Show resolved Hide resolved
src/engine/cachingreader/cachingreaderworker.cpp Outdated Show resolved Hide resolved
src/engine/cachingreader/cachingreaderworker.cpp Outdated Show resolved Hide resolved
src/engine/cachingreader/cachingreaderworker.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@uklotzde uklotzde left a comment

Choose a reason for hiding this comment

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

Thanks for stepping in! Let's see if the AutoDJ bug is fixed.

The interaction between CachingReader and the Engine is still complicated and hard to reason about. I expect that the code contains more race conditions and multi-threading bugs. Hopefully we don't wake some one of those hidden dragons by applying these changes ;) Eventually the code has to be rewritten. In Rust.

LGTM

@uklotzde uklotzde merged commit 77fafc5 into mixxxdj:2.3 Sep 23, 2021
@uklotzde
Copy link
Contributor

I will merge to main.

@ronso0
Copy link
Member

ronso0 commented Oct 5, 2021

Seems like this or #4267 also fixed the Repeat Playlist issue
https://mixxx.discourse.group/t/mixxx-autodj-stopped-repeating-playlist/23151/6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants