Fix typo in makeConstBpm() and improve BPM precison for long tracks #3794
Conversation
daschuer
changed the title
…atching region for a precise const BPM
…at two tootal number of beats are valid in between. This fixes a warping beat issue for long track with short const regions.
daschuer
force-pushed
the
makeConstBpmFix
branch
from
ef4cb02 to
3c60f0d
Compare
daschuer
changed the title
|
Done. |
src/track/beatutils.cpp
Outdated
| @@ -231,19 +242,31 @@ double BeatUtils::makeConstBpm( | |||
| ((kMaxSecsPhaseError * sampleRate) / numberOfBeats); | |||
| const double maxRoundSamples = constantRegions[i].beatLength + | |||
| ((kMaxSecsPhaseError * sampleRate) / numberOfBeats); | |||
| if (longestRegionLength > minRoundSamples && | |||
| longestRegionLength < maxRoundSamples) { | |||
| if (longestRegionBeatLenth > minRoundSamples && | |||
There was a problem hiding this comment.
If you rename the variable anyway, it would be good if you'd append the unit (Samples?) to the variable name.
There was a problem hiding this comment.
All variables are on a frame base.
I have added an initial comment to clarify this. And took this momentum to unify the variable naming inside the function.
There was a problem hiding this comment.
Code looks good so far. There are still implicit roundings using +0.5 in unchanged parts of the code, but we do not need to fix them now.
I am worried about invalidating analysis results once again, but I guess we don't have a choice. Are we sure that this will be the last change for 2.3?
|
I like to hear from @poelzi if this is sufficient for his tracks. |
|
@poelzi Please test and report. |
|
During my tests I don't see any regressions. The only concern is that this PR does not completely solve the issue and we need to advance the analysis version number once more. |
|
Not a big issue, so this is merge-able. |
|
@poelzi Please respond if you want to participate. Otherwise I'll merge this PR soon. |
|
RFC period is over. Merging this now. LGTM |
This fixes two issues in makeConstBpm()
An a obvious typo slipped through my original tests.
Now the https://sanatonrecords.bandcamp.com/track/procs-frigolitpuffens-magiska-trampdyna is detected correctly as 139,98 BPM instead of rounding to 140,00 BPM.
This track is a good example why too aggressive rounding is not always a good solution.
And an issues with long tracks where the number of beats is ambiguous.
Like:
https://sanatonrecords.bandcamp.com/track/already-maged-circle-dance-of-cold-constellations
Now we continue searching until a single number of beats is returned for the whole possible BPM range.