SyncLock: Fix issues with single-playing syncables. #4155
Conversation
https://mixxx.discourse.group/t/vestax-tr-1-weird-behaviour-quantizing-drops-tempo/22715/7 Fixes two issues: * Do not seek phase when audible status changes * When only one syncable is playing, use its beat distance to update the leader, so that seeks are properly handled
|
I was unable to write a test to cover these cases :( |
Why? What was the issue? |
I tried to create the same set of circumstances but the test always passed -- I couldn't get it to fail in the test environment. |
|
The likely cause, PR #3772, was merged into |
|
Race condition ;-) you answered my question by opening #4156. |
Pull Request Test Coverage Report for Build 1078757626
💛 - Coveralls |
|
How about merging #4156 to 2.3 first, merge 2.3 to main and rebase this on top of it to get rid of two commits doing the same? |
|
yes that's a good idea |
|
main has the other PR, so this one is ready to go too |
src/engine/sync/enginesync.cpp
Outdated
| @@ -371,7 +371,7 @@ void EngineSync::notifyPlayingAudible(Syncable* pSyncable, bool playingAudible) | |||
| // Even if we didn't change leader, if there is only one player (us), then we should | |||
There was a problem hiding this comment.
The comment doesn't match the new method invocation.
|
Unfortunately #4156 does not completely fix the issue in 2.3 see my comments there. The same issue is already fixed here, unfortunately here it is shifted to another point.
|
|
hm ok I'll take a look |
|
Might be the same issue that @daschuer mentioned, but I'm not sure: https://mixxx.discourse.group/t/vestax-tr-1-weird-behaviour-quantizing-drops-tempo/22715/14?u=hlzhs |
|
that feels like a bad beatgrid issue. I'm looking in to daniel's issue now. |
Ah I see what is going on. This is a separate issue, I'd prefer to address it in a separate PR. (Otherwise this becomes "let's fix absolutely every issue with sync lock in one PR" which is not reasonable) |
|
#4169 for the other issue |
|
anything more needed here? |
|
There is still an issue, when the explicit master is paused, quantize play doe not longer work, see: There is also an issue that the double detection is applied twice when repeated clicking on sync twice or using the right click: Can you also take a look at the issue https://bugs.launchpad.net/mixxx/+bug/1939103 to backport the fix in this PR completely? |
|
do those issues only affect this branch, or do they affect main as well? I have three outstanding synclock PRs and each fixes a different issue. If those issues are new after these fixes, then they are bugs here that should be addressed. If they also exist in main then they are out of scope. These things will never get fixed if we allow every sync lock PR to expand to encompass all synclock bugs. |
Yes of cause. I hope I will find time to check that soon with main. If you find time before to test it, it would be a good step forward. |
|
I can confirm that the first issue happens with plain master as well. I have filed a bug: |
|
I no longer reproduce the second issue with this branch and with main. |
|
Ups, I have merged this with a broken test. I will take a look. |
|
thanks again for all your reviews |
Discovered while investigating https://mixxx.discourse.group/t/vestax-tr-1-weird-behaviour-quantizing-drops-tempo/22715/7
When only one syncable is playing, use its beat distance to update the leader, so that seeks are properly handled