Analysis: Add support for overriding variable/constant BPM on a per-track basis

[fwcd]

This PR, intended as a follow-up to #4806, wraps scheduled tracks in AnalyzerScheduledTrack and AnalyzerTrack, depending on whether it contains a TrackId or a resolved TrackPointer.

These wrappers add the ability to override analysis options on a per-track basis. This PR also introduced one such option, namely useFixedTempo, which can be used to override the constant/variable BPM setting. Additionally, it adds two new entries to WTrackMenu > Analyze:

• Reanalyze (constant BPM)
• Reanalyze (variable BPM)

Any feedback is of course welcome!

[ronso0]

Re track menu implementation:
Instead of 3 "Reanalyze" actions, how about reading the current setting (const/var BPM analyzer) and display these actions

• Analyze
• Reanalyze
• Reanalyze with [constant | variable] BPM

where [constant | variable] is the opposite of the current setting.

[daschuer]

Yes this would help to get rid of the redundant menu. The other issue is that it is not that clear what exactly is reanalyzed with "Reanalyze" when we have two other options where BPM is explicit mentioned.

Some ideas for discussion

• Analyze
• Reanalyze (constant BPM)
• Reanalyze (variable BPM)

Or:

• Analyze
• Reanalyze
  [✓] variable BPM

default [✓] is from the preferences option

[fwcd]

I think @ronso0's

• Analyze
• Reanalyze
• Reanalyze with [opposite of setting]

strikes a good balance. The checkbox one is intriguing too, but I like the above one because it saves a click (since, in my experience, a user that wants to reanalyze is likely to pick the [opposite of setting] option, especially if they already performed a batch analysis using the default setting).

[fwcd]

Since we'd need to update the menus whenever the setting changes, is there an elegant way to hook into/listen for settings changes? Or would we need to add a new signal e.g. in beatdetectionsettings?

[daschuer]

You may pipe the userSettings pointer into the class and recreate the menu just before showing.

The other option is to backup the preferences option with a ControlObject and consume the value change signal.

I think you will find templaes for both in the Mixxx code.

[fwcd]

Hm, I'm almost inclined to do that in a separate PR to keep this one focused if it requires a lot of wiring to make the menu dynamic.

[daschuer]

That would be OK for me.

[ronso0]

Hm, I'm almost inclined to do that in a separate PR to keep this one focused if it requires a lot of wiring to make the menu dynamic.

Both WTrackMenu::loadTrack and WTrackMenu::loadTrackModelIndices call WTrackMenu::updateMenus where you can easily read the config with

m_pConfig->getValue<bool>(
            ConfigKey("[BPM]", "BeatDetectionFixedTempoAssumption"),
            true);

and configure the Reanalyze actions accordingly.

[fwcd]

Ah thanks, I wasn't aware of the updateMenus method. The menu should now automatically update accordingly.

[daschuer]

We now have basically:
std::optional<std::not_null<std::shared_ptr<Track>>>

You introduce std::not_null to justify the use of std::optional and this comes with a lot of surprising magic and is slowing down the process.

I assume that you are coming from languages where the std::optional equivalent is the only solution to express a null value. This is not the case in C++ where a pointer is already an "optional". Also the used std::shared_ptr has the "optional semantic" https://en.cppreference.com/w/cpp/memory/shared_ptr/operator_bool.
So for a C++ developer is unusual to not use this feature, but use a not free workaround instead. Also in Qt many classes are optional. For instance https://doc.qt.io/qt-6/qvariant.html#isNull. In Mixxx we also use this pattern Bpm::isValid();

This is a well established pattern in C++ and I strongly recommend to use it here.

Let's move the Track not_null idea to a separate PR. I think we should create our own not_null track class that fixes the performance issues.

Apart form that, this branch works good. Thank you.

[fwcd]

I'd say it's more like

std::optional<std::pair<
  gsl::not_null<std::shared_ptr<Track>>,
  AnalyzerTrack::Options
>>

which is not isomorphic to the type you suggested:

std::pair<
  std::shared_ptr<Track>,
  AnalyzerTrack::Options
>

So I'd argue that even though the difference is subtle, they do have different semantics and I would also argue that the upper one is the more correct one (since it doesn't really make sense to specify options on a null track).

a lot of surprising magic and is slowing down the process.

I really don't think this is a fair assessment of the situation. std::optional is safer, more explicit, better signals intent and forces the programmer to stop for a moment and think about optionality. How is a nullable pointer where the programmer may or may not have intended to allow null better?

So for a C++ developer is unusual to not use this feature, but use a not free workaround instead. Also in Qt many classes are optional. For instance https://doc.qt.io/qt-6/qvariant.html#isNull. In Mixxx we also use this pattern Bpm::isValid();

I agree, it's a common pattern to write custom isValid-style methods. But this is an "appeal to tradition" argument: If we have the opportunity to modernize the codebase here in the face of a new feature, why not do so directly instead of implementing the legacy pattern and then refactoring later on?

I assume that you are coming from languages where the std::optional equivalent is the only solution to express a null value.

I have worked in a bunch of languages, including ones that implicitly permit null everywhere (Java, Python etc.) and languages that enforce strongly-typed optionals (Rust, Haskell, Kotlin, Swift, ...). Encoding invariants at the type-level is not only a general trend in modern, statically-typed languages, even C++ seems to move into the direction. It's the reason why we use smart pointers instead of managing memory manually using new and delete everywhere (aside from situations where tiny differences in memory usage are actually critical, but this isn't really the case here), etc.

fixes the performance issues.

We already do a null check, so there's no performance overhead. gls::not_null also has zero overhead as mentioned in the docs, std::optional maybe a byte. This is a minuscule space cost per AnalyzerThread that's not really worth bikeshedding over.

Let's move the Track not_null idea to a separate PR.

This only applies to AnalyzerTrack not all other TrackPointers. I don't think the general pattern of using null TrackPointers is bad, just that it makes sense for this particular use case to bundle a not-null track with it's options and then add optionality on top. This is because optionality is a property of the analyzer, not the track, since the analyzer queue can be empty.

[uklotzde]

@fwcd I appreciate your efforts to modernize the code base and fully agree with your arguments about "encoding invariants at the type-level".

[ywwg]

for what it's worth, using nullptr as an optional type has several shortcomings -- code can crash if it doesn't handle null correctly, and variables can end up being null even if they weren't intended to be so. std::optional is a good feature to make this use explicit and does not impose a lot of computational overhead. So I am in favor of using a more modern approach than using pointers-and-null

[daschuer]

I sounds like you did not get my point correctly.
I am not against using std::optional here.

The remaining issue in this case is wrapping std::not_null<std::shared_ptr<Track>>
Every access off the pointer is a deep copy of the shared pointer including all book keeping and memory fences for syncing.

That needs to be solved!

I and I have already recomended to move that out of this PR. Just to continue here ...

[daschuer]

@ywwg std::optional suffers exactly the same issue as plain pointer. If you access it without a check Mixxx crashes.

[daschuer]

Summary: I see two possible solutions:

• Implement an TrackPointerNotNull where operator-> and friends are raw pointer access. Than use it here warped in an std::optional
• Add the optional semantic to AnalyzerTrack, and reuse the null TrackPointer to express this.

[fwcd]

Implement an TrackPointerNotNull where operator-> and friends are raw pointer access. Than use it here warped in an std::optional

This is pretty much what we are doing right now. Since gsl::not_null is a zero-overhead wrapper, I don't see how a custom class would be necessary instead of just wrapping the shared_ptr. Any refcounting overhead here would already be present in the current Mixxx and optimizing this would IMO indeed be out of scope for this PR.

Also while I agree that it's important to optimize, going with battle-tested (and optimized) standard library types like shared_ptr is IMO usually better than rolling our own with all of the pitfalls that writing your own wrappers comes with (e.g. making sure all copy/move constructors/assignments are implemented correctly, just to name one example). Another advantage is that using types from the standard library makes the codebase less obscure and more accessible to new contributors, since they are likely to be familiar with these types.

[daschuer]

Since gsl::not_null is a zero-overhead wrapper ...

I have just realized that since:
microsoft/GSL@4a4bb3c

This is actually the case. My concerns where regarding the previous version, which has the described overhead.

[daschuer]

Wait, I have misread the code
std::is_copy_constructible<std::shared_ptr<T>>::value
is true this mean we do a deep copy, which the opposite of zero overhead.

The changes where introduced to support std::unique_ptr()

So my conclusion is still stands:
The introduced overhead is not worth the benefits we have by using std::optional, because the alternative traditional solution offers the same safety and semantics and is much more performant.

We may add a wrapper header that defines is_copy_constructible as is_pod.
Or inherit from std::shared_ptr a new one with deleted copy constructor.
We may just copy and edit the not_null template ...

Ideas?

[fwcd]

The docs explicitly state that there's no (space) overhead: https://github.com/microsoft/GSL/blob/1683d87a3f45dff1817e858dbaf006e5ff84e784/include/gsl/pointers#L74 (this is a permalink, but points to the current main branch). With "deep copy" I assume you mean "copying the pointer to a track and maybe updating its reference count"? But even if there were some overhead, I think vendoring our own version is not a good idea, for reasons mentioned in my previous comments:

going with battle-tested (and optimized) standard library types like shared_ptr is IMO usually better than rolling our own with all of the pitfalls that writing your own wrappers comes with (e.g. making sure all copy/move constructors/assignments are implemented correctly, just to name one example). Another advantage is that using types from the standard library makes the codebase less obscure and more accessible to new contributors, since they are likely to be familiar with these types.

This is really not a case worth micro-optimizing. Tracks added to the analyzer queue change maybe once every few seconds and std::optional adds so little overhead that, unless we have evidence that it actually is a space/performance issue (e.g. by profiling it), I think the tradeoff in productivity/readability is simply not worth it.

[daschuer]

I have found a related bug microsoft/GSL#550

The docs explicitly state that there's no overhead: https://github.com/microsoft/GSL/blob/1683d87a3f45dff1817e858dbaf006e5ff84e784/include/gsl/pointers#L74

This is regarding size. My concerns are regarding runtime.

With "deep copy" I assume you mean "copying the pointer to a track and maybe updating its reference count"?

Yes, it counts always fore and back and may empty the CPU pipeline because of memory fences.

This is really not a case worth micro-optimizing.

Look, we compare here the traditional solution with a solution that aims to give a better semantic. Both are ways of optimization. I am happy with the later if it actually has close to zero overhead as assumed, but since this is not yet the case, let's go for the traditional solution.

It would be also a good solution if the we remove the deep copy somehow.

[fwcd]

Hm, that's a fair point, I wasn't aware that not_null<shared_ptr<...>> actually performed a reference increment/decrement on every access. I still think that it wouldn't really be worth optimizing, since infrequent reference copies are comparatively cheap. But maybe we can find a solution that works for all of us.

What do you think about adding an assertion or check to AnalyzerTrack's constructor that the passed TrackPointer is not null and then perhaps a doc comment that AnalyzerTrack always wraps a not-null pointer? Since the wrapped TrackPointer field is not accessible from the outside, this should guarantee that the invariant (the pointer not being null) is always upheld. To avoid unnecessary reference copies we could make getTrack return the shared_ptr by const reference and anyone interested in retaining the TrackPointer could then make a reference copy themselves.

[daschuer]

Good idea. That works for me.

[fwcd]

Done!

[daschuer]

Thank you. I have just added some code style comments.

[daschuer]

Thank you for this nice improvement. LGTM