fix: sync rate using the current BPM instead of the file one #13671
Conversation
|
@mixxxdj/developers this is potentially a breaking change, although IMHO this improves the existing behaviour. Let me know what you think, I'd be happy to put that behaviour change behind a setting. |
|
I agree this is a fix. I hardly ever use sync so I can't really tell whether this would break use cases. One could be too going wild and play a few chipmunk'ed tracks in a row (eg. with 90% rate range). Either way, if we decide this significantly breaks use cases it should go to 2.5, otherwise it should go to 2.4, no? |
I was even tempted to put it in 2.6 since we are already in the beta process of 2.5. This way, if we do break some user's routines, this give use more lead way to gather the feedback. Happy to release in previous version otherwise! |
There was a problem hiding this comment.
The intent of the original behavior was to handle cases where drum and bass tracks were detected at 1/2 the correct bpm. (that's what happened to Grimes at her famously disastrous Coachella set). In that situation, the file bpms are a better hint as to whether the tracks themselves, at native rate, are 1/2 or 2x from each other.
As long as the beatmatch behavior still works for the 70/140 (dubstep) or 87/174 (drum and bass) scenario at reasonable rate slider settings (+/- 8), then this fix is fine. Can you do some quick checking to see if that's the case?
src/engine/sync/synccontrol.cpp
Outdated
| updateLeaderBpm(bpm); | ||
| updateLeaderBeatDistance(beatDistance); | ||
| } | ||
|
|
||
| double SyncControl::determineBpmMultiplier(mixxx::Bpm myBpm, mixxx::Bpm targetBpm) const { | ||
| if (!myBpm.isValid() || !targetBpm.isValid()) { | ||
| if (!myBpm.isValid() || !targetBpm.isValid() || myBpm.isNull() || targetBpm.isNull()) { |
There was a problem hiding this comment.
is null ever a valid bpm? seems like isValid could check for null value rather than having a separate function.
There was a problem hiding this comment.
As part of my routine, I sometime makes breaks where I slide the tempo fader all the way to zeo (with a 100% range), meaning the final bpm will be 0.00, on potentially the leader deck. Technically, 0.00 is a valid BPM, thus why the isNull introduction
There was a problem hiding this comment.
isValidValue only returns true if kValueMin < value;, so 0.0 is not a valid bpm by that function, so the original check should be fine.
There was a problem hiding this comment.
Ah indeed. I thought I had seen the min value being included in the valid range.
I removed the above
Yes, this was part of my testing already, and it worked fine on my end. But it would be very appreciated if someone could also try, just to make sure I didn't have a "lucky" case |
|
I'll give it a shot |
|
hm, ok try this: enable sync
Although, I think it's doing that in HEAD as well! This is why I occasionally suggest that we should drop the half/double feature... it's too hard to get right without these edge cases. People just need to make sure their tracks are detected right. |
|
I will try and fix this usecase. Just to confirm, we have
|
|
probably when the rate slider signal is being processed, the sync code is not calling the bpm multiplier code somewhere |
|
Yeah, testcases are the way to go here! |
|
@ywwg I tried really hard the usecase and more stress test (disable and re-enable sync, adjust leader slider again, then follower, change leader, ...) and I couldn't get it to behave as you suggested. I have added a unit test that capture that reproducing steps but it is passing. Could you confirm I translated it correctly? Is that 100% reproducible btw? |
|
Here, the code does not do the correct multiplier on first load, but it does on the second. And then, the rate slider adjustment shows the bug as I described mixxx-bug.mp4 |
|
Right, it took me some times, but I finally got the source of the bug. There is a race condition when you load a track, where the BPM multiplier is recalculated by the audio thread before the file has loaded (some BPM is invalid -> multiplier get's the value of 1). then later the CachingReader completes the file load, and reinit the multiplier to one. Honestly, it doesn't look trivial to fix, so since this is another bug that already exists in main, how would you feel with merging this bugfix already? I have added the test case that reproduce this issue so if that's okay with you, I will create an issue to address that separately. |
|
that's ok by me. Thanks for looking in to it. |
src/engine/sync/synccontrol.cpp
Outdated
| updateLeaderBpm(bpm); | ||
| updateLeaderBeatDistance(beatDistance); | ||
| } | ||
|
|
||
| double SyncControl::determineBpmMultiplier(mixxx::Bpm myBpm, mixxx::Bpm targetBpm) const { | ||
| if (!myBpm.isValid() || !targetBpm.isValid()) { | ||
| if (!myBpm.isValid() || !targetBpm.isValid() || myBpm.isNull() || targetBpm.isNull()) { |
There was a problem hiding this comment.
isValidValue only returns true if kValueMin < value;, so 0.0 is not a valid bpm by that function, so the original check should be fine.
|
Created #13689 to address the issue above |
Fixes #12738
It doesn't make much sense to use the original file BPM as it is irrelevant to the user. In case it is, they could always reset the fader to zero before syncing, in which case sync behaviour will have the same effect as today