Beats: Refactor getBpm() into getBpmInRange(startPos, endPos) #4361

Holzhaus · 2021-10-06T13:32:10Z

This lays some groundwork for a future, sparse representation of beats
in a track.

At the moment, we have two kinds of beats implementations:

The BeatMap representation is simply a list of every single beat
position in a track. Hence, changing tempos are supported, but memory
intensive to work with.
The BeatGrid representation is actually sparse (only stores the
offset and bpm instead of actual beat positions), but only supports a
single constant tempo.

Both are not sufficient. A future implementation should be both sparse
and support multiple tempos per track. To make this possible, we need to
know the track duration for calculating the average BPM of a track.

There are two ways to handle this: We could either store the track
duration in a beats object, or we change the getBpm() method signature
to require passing the position range of the track that we want to know
the BPM for.

This commit implements the latter solution, because it's fully
backwards-compatible and - in contrast to the former - can be
implemented without modifying the protobuf format of the BeatGrid
class (which currently doesn't store the track end position).

Related Zulip discussion:
https://mixxx.zulipchat.com/#narrow/stream/109171-development/topic/beat.20grid.20revamp.20re.3A.20pr.20.232961/near/251702242

Be-ing · 2021-10-07T18:16:09Z

getBpm() has always been conceptually odd. This way to fix it in a backwards compatible way is clever. Thanks for working on it.

daschuer · 2021-10-07T19:53:30Z

src/analyzer/analyzerbeats.cpp

@@ -145,7 +145,10 @@ bool AnalyzerBeats::shouldAnalyze(TrackPointer pTrack) const {
    if (!pBeats) {
        return true;
    }
-    if (!pBeats->getBpm().isValid()) {
+    if (!pBeats->getBpmInRange(mixxx::audio::kStartFramePos,


This is now a very expensive check for a valid BPM. Maybe caching the nominal bpm is not such a bad idea.
What are the alternatives?

src/analyzer/analyzerbeats.cpp

daschuer · 2021-10-07T20:13:18Z

Can we add a getNominalBpm() for the whole track?
Do we have a use case for the nominal bpm of track sections?

We also have getBpmAroundPosition() which is the average, I can imagine confusion that on in the nominal BPM and the other the average.

Holzhaus · 2021-10-13T21:14:07Z

Can we add a getNominalBpm() for the whole track?

No, for the reasons I explained in the commit message. We simply don't know how long the track is.

Do we have a use case for the nominal bpm of track sections?

I don't know. Right now, we always use the start/end position.

We also have getBpmAroundPosition() which is the average, I can imagine confusion that on in the nominal BPM and the other the average.

I don't really know what you mean. Both essentially do the same, it's just that:

getBpmInRange retrieves the Bpm for a range defined by start and end position
getBpmAroundPosition retrieves the bpm for a range defined by a center point and a size in beats

daschuer · 2021-10-13T22:48:25Z

Can we add a getNominalBpm() for the whole track?

No, for the reasons I explained in the commit message. We simply don't know how long the track is.

I don't understand that. We have currently a well working nominal value. It is independednt from the total track length.

Do we have a use case for the nominal bpm of track sections?

I don't know. Right now, we always use the start/end position.

I cant think of one as well, so we can remove this feature and simplify the API.

I propose to keep getBpmAroundPosition() and rename getBpm() to getNominalBpm().
This should understandable ... hopfuly.

Be-ing · 2021-10-13T22:50:37Z

I am doubtful that "nominal BPM" is a concept that should exist in Mixxx.

daschuer · 2021-10-13T22:59:05Z

We need it for the value shown in the library.

uklotzde · 2021-10-13T23:48:40Z

I am doubtful that "nominal BPM" is a concept that should exist in Mixxx.

It's my primary sorting criteria for search results when DJing.

Swiftb0y · 2021-10-14T07:16:21Z

I think a nominal bpm is not accurate and not that helpful in the library. A BPM range of the min-max bpms in the track would make more sense. Representing a track with changing bpm the same as a track with constant bpm in the library is also misleading IMO.

Holzhaus · 2021-10-14T07:18:30Z

Do we have a use case for the nominal bpm of track sections?

I don't know. Right now, we always use the start/end position.

I cant think of one as well, so we can remove this feature and simplify the API.

I propose to keep getBpmAroundPosition() and rename getBpm() to getNominalBpm().
This should understandable ... hopfuly.

I don't understand what you're proposing if you don't want to remove the overall Bpm entirely. Should I remove support for legacy beatgrids/beatmaps in #4255 and always force a reanalysis instead? Or do you have another solution?

Holzhaus · 2021-10-14T07:26:34Z

I think a nominal bpm is not accurate and not that helpful in the library. A BPM range of the min-max bpms in the track would make more sense. Representing a track with changing bpm the same as a track with constant bpm in the library is also misleading IMO.

I suppose there are better ways to handle this, but it is out of scope for this PR. I just wanted to make our current bpm retrieval work with #4255, not redesign the BPM column in the library ;-)

uklotzde · 2021-10-14T07:35:44Z

I think a nominal bpm is not accurate and not that helpful in the library. A BPM range of the min-max bpms in the track would make more sense. Representing a track with changing bpm the same as a track with constant bpm in the library is also misleading IMO.

Do not only consider use cases of playing a single genre with a very limited tempo range. A single value is perfectly accurate if you play a broad range of music between 60 and 180 bpm.

I don't want to see 2 values and decide which one to choose for sorting and display both of them, information overload. Not needed for 99.9% of all the tracks. Min/max is useless if you don't know to which part of the track they apply to. If a track starts with a lower or higher bpm I could take a note. KISS!

Swiftb0y · 2021-10-14T08:59:44Z

I suppose there are better ways to handle this, but it is out of scope for this PR. I just wanted to make our current bpm retrieval work with #4255, not redesign the BPM column in the library ;-)

I suppose a nominal bpm value is fine as a stop gap solution then.

I don't want to see 2 values and decide which one to choose for sorting and display both of them, information overload. Not needed for 99.9% of all the tracks.

You misunderstood, but we can discuss this more when we actually decide to redesign this.

This lays some groundwork for a future, sparse representation of beats in a track. At the moment, we have two kinds of beats implementations: 1. The `BeatMap` representation is simply a list of every single beat position in a track. Hence, changing tempos are supported, but memory intensive to work with. 2. The `BeatGrid` representation is actually sparse (only stores the offset and bpm instead of actual beat positions), but only supports a single constant tempo. Both are not sufficient. A future implementation should be both sparse and support multiple tempos per track. To make this possible, we need to know the track duration for calculating the average BPM of a track. There are two ways to handle this: We could either store the track duration in a beats object, or we change the `getBpm()` method signature to require passing the position range of the track that we want to know the BPM for. This commit implements the latter solution, because it's fully backwards-compatible and - in contrast to the former - can be implemented *without* modifying the protobuf format of the `BeatGrid` class (which currently doesn't store the track end position). Related Zulip discussion: https://mixxx.zulipchat.com/#narrow/stream/109171-development/topic/beat.20grid.20revamp.20re.3A.20pr.20.232961/near/251702242

Holzhaus · 2021-10-20T10:20:57Z

Do we have a use case for the nominal bpm of track sections?

I don't know. Right now, we always use the start/end position.

I cant think of one as well, so we can remove this feature and simplify the API.
I propose to keep getBpmAroundPosition() and rename getBpm() to getNominalBpm().
This should understandable ... hopfuly.

I don't understand what you're proposing if you don't want to remove the overall Bpm entirely. Should I remove support for legacy beatgrids/beatmaps in #4255 and always force a reanalysis instead? Or do you have another solution?

@daschuer To clarify, let's consider a non-const track with 3 different tempo sections:

|     Tempo Section 1    |   Tempo Section 2   |    Tempo Section 3     |
Position A           Position B            Position C          Position D
(start of track)                                           (end of track)

The corresponding beat object would have the following data structure:

There are n beats in the section between positions A and B
There are m beats in section between positions B and C
After position C, the remainder of the track has BPM x

Here, position A is the start of the track (or rather the position of the
first beat in the track), but the end of track (or the position of the last
beat in the track) is unknown. As you can see, if you only look at entry 3,
this is actually the how a legacy BeatGrid for a const-tempo-track would look
like.

Now to the problem: To compute the average BPM of the track, I need to know how
long tempo section 3 is in relation to the other two tempo sections. For this,
the end position is needed, which requires the knowledge of the track end
position. That information is not stored in the beats object.

Hence the two options:

Pass the track end position when retrieving the Bpm (this is what this PR does and the best option by far IMHO)
Store the track end position in the beats object. This makes the implementation more complex in the const-tempo case and essentially drops backwards compatibility with legacy beatgrids because those do no contain that information.
Remove all ways to retrieve an average BPM (multiple people already argued against this)

uklotzde · 2021-10-20T11:20:32Z

Passing the duration of the track as a parameter from the outer context instead of storing it redundantly and needlessly inside the grid is correct, imho.

If the first section always starts at position 0 then storing the end instead of the start position for each section could be an alternative representation. This representation would then be self-contained.

I didn't check the code yet: Does the representation support sections with unknown tempo, i.e. 0 bpm? These sections should be skipped when calculating an average bpm.

Holzhaus · 2021-10-20T13:36:02Z

If the first section always starts at position 0 then storing the end instead of the start position for each section could be an alternative representation. This representation would then be self-contained.

No, the first section may start anywhere (the track could begin with silence). The first section should start at the first Downbeat.

I didn't check the code yet: Does the representation support sections with unknown tempo, i.e. 0 bpm? These sections should be skipped when calculating an average bpm.

No. This isn't possible because internally, because the new beats implementation uses a random access iterator to navigate between beats. If there were unknown sections, this wouldn't work.

daschuer · 2021-10-20T16:26:28Z

First of all, I think we need to define two terms to understand each other's.

Average BPM: the math correct average of a certain number of beats.

Nominal BPM: the single number displayed in the library, significant for sorting and comparing

When importing const BeatGrids, both values are the same, there is no need for the track length.

When importing BeatMaps let's say a ~120 BPM track with a speed up section of 160 BPM.
The average will be 125,2 BPM and the nominal BPM will be 120 BPM. The nominal BPM us currently calculated by creating const BeatGid that is as close as possible to the most Beats.
In many cases the BPM of the longest section.
It can also calculated without the beat length.

daschuer · 2021-10-20T16:29:47Z

For me it is mandatory to have the nominal value "somewhere" in Mixxx. We need it only for display purpose, in the library and the track metadata, where we are limited to a single value.

daschuer · 2021-10-20T16:36:27Z

The issue you mention raises because of the missing last beat in your model.

The problem does not exist during the initial import, but will come up whenever you edit the BeatGrid, because you cannot recreate the nominal value.

Introducing a final beat is not possible in case of BeatMaps, because of the missing track length in the current data. So we need something tricky around this dead lock. I have an Idea!

daschuer · 2021-10-20T16:44:16Z

For a nominal BPM you need at least two beats.
So you may import the two fist beats of a const BeatMap. This allows to calculate the nominal BPM even after edits. In case of manual edits we can allways add a Tempo Section 3 AFTER the last beat not considered for the nominal BPM but allows looping after the last beat.

I have no use case for the Average overall BPM.
Do you have one?

JoergAtGithub · 2021-10-20T18:21:26Z

Isn't this a drop in replacement, which covers all use cases currently implemented in Mixxx 1:1?
If everything works, which worked before, there is IMHO no reason to request additional functionality in this PR.

Be-ing · 2021-10-20T18:22:14Z

If everything works, which worked before, there is IMHO no reason to request additional functionality in this PR.

I agree. It is important to keep the scope of these refactoring PRs as small as possible without introducing regressions.

daschuer · 2021-10-20T21:52:57Z

The current solution looks a bit cluttered to me, but OK we can change that at any time.

daschuer

LGTM.
Is this ready to merge?
Did you consider my idea?

Holzhaus · 2021-10-20T22:08:12Z

Yes, it's ready to merge.

I'm not sure i understood your idea correctly, but I suppose it's easier to discuss this over at #4255 where we can discuss the code.

daschuer · 2021-10-20T22:13:18Z

Thank you.

Holzhaus added this to the 2.4.0 milestone Oct 6, 2021

github-actions bot added code quality library labels Oct 6, 2021

Holzhaus force-pushed the getbpminrange branch 2 times, most recently from 50dcde0 to 1ab61f5 Compare October 6, 2021 16:48

JoergAtGithub mentioned this pull request Oct 7, 2021

Add new beats implementation #4255

Merged

7 tasks

daschuer reviewed Oct 7, 2021

View reviewed changes

src/analyzer/analyzerbeats.cpp Outdated Show resolved Hide resolved

Holzhaus force-pushed the getbpminrange branch from 1ab61f5 to 883cea1 Compare October 15, 2021 10:26

Holzhaus added 3 commits October 20, 2021 20:55

AnalyzerBeats: Fix wrong log message about detected BPM

300c160

Beats: Fix wrong comment stating that getBpmInRage returns average BPM

9638b4f

Beats: Clarify that getBpmAroundPosition returns the arithmetic average

8decc52

daschuer approved these changes Oct 20, 2021

View reviewed changes

daschuer merged commit b74f015 into mixxxdj:main Oct 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Beats: Refactor getBpm() into getBpmInRange(startPos, endPos) #4361

Beats: Refactor getBpm() into getBpmInRange(startPos, endPos) #4361

Holzhaus commented Oct 6, 2021

Be-ing commented Oct 7, 2021

daschuer Oct 7, 2021

daschuer commented Oct 7, 2021

Holzhaus commented Oct 13, 2021

daschuer commented Oct 13, 2021

Be-ing commented Oct 13, 2021

daschuer commented Oct 13, 2021

uklotzde commented Oct 13, 2021

Swiftb0y commented Oct 14, 2021

Holzhaus commented Oct 14, 2021 •

edited

Loading

Holzhaus commented Oct 14, 2021

uklotzde commented Oct 14, 2021 •

edited

Loading

Swiftb0y commented Oct 14, 2021 •

edited

Loading

Holzhaus commented Oct 20, 2021

uklotzde commented Oct 20, 2021

Holzhaus commented Oct 20, 2021

daschuer commented Oct 20, 2021

daschuer commented Oct 20, 2021

daschuer commented Oct 20, 2021

daschuer commented Oct 20, 2021

JoergAtGithub commented Oct 20, 2021

Be-ing commented Oct 20, 2021

daschuer commented Oct 20, 2021

daschuer left a comment

Holzhaus commented Oct 20, 2021

daschuer commented Oct 20, 2021

Beats: Refactor getBpm() into getBpmInRange(startPos, endPos) #4361

Beats: Refactor getBpm() into getBpmInRange(startPos, endPos) #4361

Conversation

Holzhaus commented Oct 6, 2021

Be-ing commented Oct 7, 2021

daschuer Oct 7, 2021

Choose a reason for hiding this comment

daschuer commented Oct 7, 2021

Holzhaus commented Oct 13, 2021

daschuer commented Oct 13, 2021

Be-ing commented Oct 13, 2021

daschuer commented Oct 13, 2021

uklotzde commented Oct 13, 2021

Swiftb0y commented Oct 14, 2021

Holzhaus commented Oct 14, 2021 • edited Loading

Holzhaus commented Oct 14, 2021

uklotzde commented Oct 14, 2021 • edited Loading

Swiftb0y commented Oct 14, 2021 • edited Loading

Holzhaus commented Oct 20, 2021

uklotzde commented Oct 20, 2021

Holzhaus commented Oct 20, 2021

daschuer commented Oct 20, 2021

daschuer commented Oct 20, 2021

daschuer commented Oct 20, 2021

daschuer commented Oct 20, 2021

JoergAtGithub commented Oct 20, 2021

Be-ing commented Oct 20, 2021

daschuer commented Oct 20, 2021

daschuer left a comment

Choose a reason for hiding this comment

Holzhaus commented Oct 20, 2021

daschuer commented Oct 20, 2021

Holzhaus commented Oct 14, 2021 •

edited

Loading

uklotzde commented Oct 14, 2021 •

edited

Loading

Swiftb0y commented Oct 14, 2021 •

edited

Loading