Ensure that tracks with an invalid bpm are re-analyzed #2776
Conversation
|
Something is quite bizarre with my database. I cleared the BPM and beatgrid of tracks via the track context menu that had no BPM detected before. With this branch, when I load them to a deck, they analyze, but every track is detected as 129.3 BPM... I cannot reproduce these issues with a fresh database. |
| if (!pBeats) { | ||
| return true; | ||
| } | ||
| if (!mixxx::Bpm::isValidValue(pBeats->getBpm())) { |
There was a problem hiding this comment.
This is a logical change. Bpm::isValidValue checks if the BPM is < 0. The old code checks if the BPM == 0. Do we want <= 0?
There was a problem hiding this comment.
No, that's correct. It checks for value > kValueMin = 0.0, i.e. re-analyze if bpm <= 0.
|
I have not digged into this, but what happens with short samples where the analyzer returns 0? |
|
I don't expect many tracks with bpm = 0. Repeatedly re-analyzing those very rare cases is a only minor issue compared to fixing tracks with wrong/missing analysis results for other users. |
|
Ok, but please add a comment about this into the code |
|
Thank you. LGTM |
|
Oh, just noticed this was targeted to master instead of 2.3. Cherry pick the commits? |
|
Oh sorry, I thought this was intended because of:
This fixes a bug, right? So a cherry-pick is appropriate ... |
Backport of #2776: Ensure that tracks with an invalid bpm are re-analyzed
I didn't want to publish these changes to avoid breaking the analysis again. But this change could fix the issues that occurred when resetting the bpm while the database column update (fixed in #2773) was missing.
By removing the nested if-else blocks the code is now hopefully easier to follow.