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

SyncLock: Do not seek phase when audible status changes #4156

Merged
merged 1 commit into from
Jul 29, 2021

Conversation

ywwg
Copy link
Member

@ywwg ywwg commented Jul 27, 2021

backport of #4155 -- the other issue fixed in that PR does not affect 2.3

https://mixxx.discourse.group/t/vestax-tr-1-weird-behaviour-quantizing-drops-tempo/22715/7
Do not seek phase when audible status changes

(the syncrequest is totally unnecessary, enginebuffer already puts in a sync request when the user hits Play)

@Holzhaus Holzhaus added major bug engine changelog This PR should be included in the changelog labels Jul 27, 2021
@Holzhaus Holzhaus added this to the 2.3.1 milestone Jul 27, 2021
@ywwg ywwg changed the title SyncLock: Fix issue with single-playing syncables. SyncLock: Do not seek phase when audible status changes Jul 27, 2021
@Holzhaus
Copy link
Member

I cannot reproduce the original issue, does someone else want to have a look? Otherwise LGTM.

@ywwg
Copy link
Member Author

ywwg commented Jul 28, 2021

Steps to reproduce:

  • load two tracks
  • enable sync only on one of them, and also enable quantize on it
  • make them different bpms
  • play both
  • fade the slider for the track with sync up and down

In the old code, when the fader hit the bottom, the track would jump and/or change speed to fix what it thought was a quantize error

@uklotzde
Copy link
Contributor

I suppose that @ywwg has tested the fix properly. LGTM

@uklotzde uklotzde merged commit 0ebbc60 into mixxxdj:2.3 Jul 29, 2021
@daschuer
Copy link
Member

Oh, unfortunately this does not completely fix the issue, Instead of a jump, we now have the tempo dip stretched over some seconds.
This can be reproduced the best with two 440 Hz tracks and:

  • Playing two tracks with sync enabled the second one silenced by the line fader
  • Nudge the second track
  • Bring the second track in
  • Silence the first track.
  • Now the nudging is reverted gradually. (Without this branch it was reverted immediately)
  • This can be noted by listening the silenced track in headphone
  • When you bring the track in, the phase adjustment is gone.

@daschuer
Copy link
Member

@ywwg

when I remove

    if (isMaster(mode)) {
        m_pBpmControl->resetSyncAdjustment();

from void SyncControl::setSyncMode(SyncMode mode) the issue is gone.

Unfortunately the issue is then shifted to the sync enable case because the leader adjustment is not reset when playing as single leader. The code is in place, but somehow bypassed. Hopefully you know already how to fix it right from the main code.

@ywwg
Copy link
Member Author

ywwg commented Jul 30, 2021

The sync code is ever evolving and getting simpler as time goes on :)

@daschuer
Copy link
Member

daschuer commented Aug 6, 2021

I have filed https://bugs.launchpad.net/mixxx/+bug/1939103 to not forget the issue.

@ywwg
Copy link
Member Author

ywwg commented Aug 6, 2021

got it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog This PR should be included in the changelog engine major bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants