[POC] Beats: Add editing controls and show downbeats on scrolling waveforms

[Holzhaus]

Based on #4411.

To Do:

• Change Beatmap import to ensure that beatsTillNextMarker is always a multiple of beatsPerBar to fix downbeats support in beatmaps
• ...

[ywwg]

If I read this right, this code assumes the "first beat" as recorded in the beat grid is a down beat. I have a lot of tracks where this isn't true, unfortunately. And since the rewrite, our beat detector currently makes some very strange choices for the first beat (often it's many beats into the track). Would it be possible to count downbeats from the primary CUE position? We don't need 100% flexibility yet, but some control would be nice.

[daschuer]

Isn't this this only the visual POC?

I think before we activate this in a build by default, we need also a data model for measures and a basic editor. Else this will be useless clutter, that makes the mixing experience worse IMHO.

[Swiftb0y]

I disagree, hardcoded 4/4 is more than sufficient even for shipping in the next stable feature. As a replacement for a basic editor we can also hardcode that the "beatgrid setter" (whatever gets called for the beats_translate_curpos CO) always sets the downbeat. As a migration strategy, aligning the downbeat with the cue or earliest hotcue should also be sufficient.

[daschuer]

If it is default off, OK. Than one can enable it for every track where this basic assumption is correct.
Unreliable annotation is IMHO worse than none.

[Swiftb0y]

Opting-in for each track is not feasible for libraries containing more than lets say one hundred tracks IMO.

[poelzi]

I want the default setting to be on for the music I listen to, so a setting in the preferences would be appropriate in my opinion.
Since downbeats are only half the thing and we actually need to have root beats as well. Most modern music is organized in 16 or 32 bars. In most cases, the first stable beat is the root beat and if every 16/32 bar is colored differently, it helps a lot to see the next segment starting even when it's visually not easy.

[Be-ing]

Unreliable annotation is IMHO worse than none.

I agree. I thought we had planned to hide this feature behind a hidden preference that isn't in the preferences GUI until it has matured to a state that it is useful. I agree that this cannot be useful without the ability to edit the downbeat data.

hardcoded 4/4 is more than sufficient even for shipping in the next stable feature

I disagree. Hardcoding 4/4 is sufficient for a temporary proof-of-concept hack but not for a release. We need to ensure that the data model works well for all planned features before committing to it.

[Holzhaus]

Okay, I should probably explained the motivation behind this PR better, sorry😅. First of all, I've put "Request for comments" in the PR title because I don't plan to get this merged as-is.

Isn't this this only the visual POC?

Nope, it's actually an API design experiment. The visuals are not that nice (i just tried to implement these Downbeat visuals in the laziest way possible). What I actually wanted to test here is how to access beat properties.

Before the refactoring, every beat has a single property (the position). But for Downbeat support, we need more (such as the beat index in the bar, whether a beat is a downbeat, or, as @poelzi pointed out, also a way to check if a beat is a phrase start).

Right now, our legacy API like Beats::findNthBeat(position, n) only work with positions. Instead of somehow adding support for these extra properties to those methods, I think accessing them via the new iterator API is much more convenient. This is what this PR is supposed to show and what I'd like to get your opinions on. 🙂

I think before we activate this in a build by default, we need also a data model for measures and a basic editor. Else this will be useless clutter, that makes the mixing experience worse IMHO.

I agree that we need some kind of editing workflow, but that is out of scope for this PR. I can check if I can make a PoC for that.

In the meantime, I'd prefer a opt-in approach via a hidden config option (like we did for QML). That lowers the barrier to test it out while keeping it disabled for the regular user, prevents merge conflicts and ensure everything builds via CI (in contrast to ifdef'ing it). We could even implement the beats editor that way to edit beats in-memory, and only change the persistent data model as the last step when everything else is done.

hardcoded 4/4 is more than sufficient even for shipping in the next stable feature

I disagree. Hardcoding 4/4 is sufficient for a temporary proof-of-concept hack but not for a release. We need to ensure that the data model works well for all planned features before committing to it.

Hardcoding 4/4 makes it possible to not change the data model at all and be fully backwards-compatible with beatmaps/beatgrids in Mixxx 2.2/2.3. If you look at the diff, the protobuf format is unchanged.

Regarding 4/4 being just a "temporary proof-of-concept hack": let's not get too overambitious here. Major DJ software such a Serato hardcodes 4/4 too and does not support other time signatures. So I think we could consider it sufficient for a stable feature, and refine it later. Not everything has to bee 100% perfect from the start.

[daschuer]

Thank you for pushing this forward. I have left some comments.

[daschuer]

Why do you use here toLowerFrameBoundary()?

What is auto here? The type is out of sight.

[Holzhaus]

Why do you use here toLowerFrameBoundary()?

I removed it here, and moved the rounding to the later part where this position is used for inserting a beat marker.

What is auto here? The type is out of sight.

It's a frame position, which should be clear from the variable names.

[daschuer]

do you actually want std::lower_bound() ?

[Holzhaus]

No. I added comments to clarify that.

[daschuer]

This condition can make use of a comment.
Can this ever be true, because if markerIt-- above?

[daschuer]

Is this not already "markerIt" ?

[Holzhaus]

No, one is a std::vector<BeatMarker>::iterastor and one is a Beats::ConstIterator. I modified the code so that this second iterator isn't necessary anymore.

[daschuer]

This assumes that markerIt is ahead of the new marker position, which is not guaranteed.

[daschuer]

You use the same lambda more often I think it makes sense to make a named function from it. The name will also improve readability.

I just had to look up how std::upper_bound() works exactly in edge cases. Given that its implementation is not has not more lines than the call here, I consider a normal loop even more readable ... just my two cents.

[daschuer]

"it" can be anything. Can we give it a reasonable name? I do not understand the code. Maybe some comments will also help.

[daschuer]

isn't it = markerIt? Why you look it up again?

[daschuer]

I think it make sense to cache the next iterator. Than we can give theme reasonable names to improve the readability as well.

[Holzhaus]

Thank you for pushing this forward. I have left some comments.

Thanks for the review, but I'd rather get some feedback on the editing workflow first, before delving into the code. If the workflow is considered insufficient, we will probably need another implementation anyway.

[ywwg]

I think this is a good approach. The editing functions are good and this provides a solid benefit for the vast majority of tracks. I do think we need some sort of behavior for migration / first-use because the current algorithm makes bad choices for the first beat in many cases. Using the CUE position as a downbeat is a sufficient solution for me.

[Be-ing]

I tested this with a jazz track which is 140 BPM for most of the track and gradually accelerates to 185 BPM by the end. It was really cool to use beatjumping and looping and actually have it be accurate.

get some feedback on the editing workflow first

In general, I think the approach of manually adjusting downbeat positions works well. However I think there is a lot of room for improvement here.

First, showing tempo markers in a special color is odd. I want to see that when editing the beatgrid, but after I've done that, I don't want to be bothered by that information when mixing. I am usually against the idea of creating separate modes, but I think creating a beatgrid editing mode that shows where the manual edits have been made would be appropriate.

I tested using a mouse and keyboard. The workflow of seeking to an exact point and pressing a button is quite difficult to use with this input method. This design may work okay with a controller with a jog wheel and a button, but I think we could do better with a design that works well for any input method. I propose to instead use a click-and-drag workflow to adjust beat positions. This would work well with both mice and touchscreens. On controllers it would be similar to the manual loop adjustment workflow, holding a button down while moving a jog wheel. For controllers this would have the advantage that you could see how the beats within the measure line up with the waveform as you drag the marker. This could also allow for fine tuning the position of any beat, not just downbeats.

It is not obvious to me what is supposed to happen to surrounding regions when placing a new tempo marker. I didn't look at the behavior too closely but it seemed like it didn't always behave the same way? I also ran into an issue twice where setting a new marker somehow made the tempo way slower? I think the downbeats became the only beats or something like that?

It was quite tedious to manually place every downbeat. I wish I could adjust the tempo for the beginning and end of an accelerating section and have Mixxx try to interpolate the tempo changes in between, then manually adjust as needed.

My understanding, without looking at the code, is that you intend for Mixxx to interpolate evenly spaced beats between downbeats. But when I set tempo markers, the tempo is shown as varying within the bar and I don't know what's happening.

[daschuer]

I have just experienced an endless loop when trying to remove a beat marker:

critical [Main] DEBUG ASSERT: "isValid()" in function mixxx::Beats::ConstIterator mixxx::Beats::iteratorFrom(mixxx::audio::FramePos) const at /home/sperry/workspace/2.3/src/track/beats.cpp:455

This message is printed over and over again.

[daschuer]

I get also this when trying to set beats in a row:

critical [Main] DEBUG ASSERT: "isValid()" in function mixxx::Beats::ConstIterator mixxx::Beats::iteratorFrom(mixxx::audio::FramePos) const at /home/sperry/workspace/2.3/src/track/beats.cpp:455
critical [Main] DEBUG ASSERT: "it == cbegin() || it == cend() || *it - position < std::prev(it).beatLengthFrames()" in function mixxx::Beats::ConstIterator mixxx::Beats::iteratorFrom(mixxx::audio::FramePos) const at /home/sperry/workspace/2.3/src/track/beats.cpp:485
terminate called after throwing an instance of 'std::bad_alloc'
  what():  std::bad_alloc
Abgebrochen (Speicherabzug geschrieben)

[ronso0]

Oooffps, clicked the __ button three times or more in a row and got std::bad_alloc
first warning

critical [Main] DEBUG ASSERT: "m_beatsTillNextMarker > 0" in function mixxx::BeatMarker::BeatMarker(mixxx::audio::FramePos, int) at /home/ronso/Downloads/mixxx_source/src/master/src/track/beats.h:32
debug [Main] BaseTrackCache(0x558275145320) updateIndexWithQuery took 0 ms
critical [Main] DEBUG ASSERT: "isValid()" in function mixxx::Beats::ConstIterator mixxx::Beats::iteratorFrom(mixxx::audio::FramePos) const at /home/ronso/Downloads/mixxx_source/src/master/src/track/beats.cpp:458
critical [Main] DEBUG ASSERT: "it == cbegin() || it == cend() || *it - position < std::prev(it).beatLengthFrames()" in function mixxx::Beats::ConstIterator mixxx::Beats::iteratorFrom(mixxx::audio::FramePos) const at /home/ronso/Downloads/mixxx_source/src/master/src/track/beats.cpp:488

the L488 assert is repeated for like... very often, then it's a lot of L458, then x__x

That happened after I was confused by the pink markers and result of 'set beat' didn't match my expectation, then I clicked it again a few times : )

[ronso0]

Was just giving this a shot with some random tracks, and I didn't find UI instructions in the first post, and I didn't read the other 64 posts to look for them .)

@Holzhaus Could you please add a basic introduction to the first post, about what the existing controls should do, what the different marker colors mean etc. Thanks!

[ronso0]

Another test with a track that I know has 160 BPM

• load track, analysis is right BPM-wise
• in the vocal intro I set a beat marker where I'd like the track to be synced
• I moved the playpos into a region with beats and set a downbeat marker
• "Const BPM" checkbox was active and unchecked afterwards
• I ticked "Const BPM" checkbox, hit Apply
• BPM is now 160 for the entire track
• except: everything before the first manually set marker has some insane BPM now:
  

Trying to move the first const beat by setting another marker gives me

critical [Main] DEBUG ASSERT: "it == cbegin() || it == cend() || *it - position < std::prev(it).beatLengthFrames()" in function mixxx::Beats::ConstIterator mixxx::Beats::iteratorFrom(mixxx::audio::FramePos) const at /src/master/src/track/beats.cpp:488

and std::bad_alloc, core dumped afterwards

[github-actions]

This PR is marked as stale because it has been open 90 days with no activity.

[prez]

Any updates on this?

[Holzhaus]

I'm pretty busy right now, can't make any promises.

[Holzhaus]

Related feature request: #10788

[v-byte-cpu]

Hi there! I really hope 🙏 that downbeat support will be added soon, as it is a very useful feature for setting cue points.

[fwcd]

Here's a rebase of the branch onto main: https://github.com/fwcd/mixxx/tree/newbeats-downbeats-rebase

After playing with it a bit, I gotta say, this is really nice! I know it's a POC, but think the downbeat visualization and editors are already nice improvements over the status quo.

The only thing I did find a bit distracting was the fixed 4/4 though. I know, this has been discussed already, but I have to agree with @daschuer und @Be-ing on that one, wrong annotations are IMO worse than no annotations.

Consider e.g. Ed Sheeran - Perfect, which is 6/8:

The detected downbeats being all over the place and misaligned with the actual ones is more distracting than having no downbeat annotations on that track. Same goes for tracks that have e.g. measures with an extra beat, which is surprisingly not that uncommon even in EDM tracks. Yes, other DJ software does this too (hardcoding 4/4), but I don't think that's a good reason for us not to do better here.

A lot of tracks are 4/4, so I don't think making the common case convenient is necessarily bad, but when given the choice I believe erring on the side of not setting/showing downbeats if the analyzer isn't "sure" the track is 4/4 would be nice.

[ywwg]

wow thanks for bringing this back -- @daschuer what do you think about closing this PR and having @fwcd opening a new one? I would really really like this feature in mixxx and if this is a good start let's build on it

[khimaros]

this is fantastic to see and i'm sure it was no small effort to rebase given how much has changed since the feature was initially developed. thank you!

[mxmilkiib]

Nice one!

Fwiw, here's a dancefloor orientated 6/8 track, and a broken beat/bruk (ctrl-f "6/8") style one at that; Distractions (Bugs In The Attic Remix) (see also the dub; Distractions (Bugz Co-operative Dub)).

[daschuer]

The way forward decision is up @Holzhaus and @fwcd. Any progress is welcome :-)

[fwcd]

I'll close this in favor of the rebased version:

• Beats: Add editing controls and show downbeats on scrolling waveforms [v2] #12343