refactor: use higher-level `std::span` based logic #13654

Swiftb0y · 2024-09-15T13:49:45Z

This is daschuer#97 rebased on main with the primary goal of making the logic easier to understand.

Swiftb0y · 2024-10-04T16:08:28Z

noticed recently that the there was no use of the metaparameter within the metronome (and the gain always having the same volume being annoying), so I added a little simple gain knob in the newest commit.

daschuer · 2024-10-09T17:00:23Z

Unfortunately the non synced mode does not produce a stady click. It sounds more like as rhythm.

daschuer · 2024-10-09T17:14:15Z

src/effects/backends/builtin/metronomeeffect.cpp

+
+template<class T>
+std::span<T> subspan_clamped(std::span<T> in, typename std::span<T>::size_type offset) {
+    // TODO (Swiftb0y): should we instead create a wrapper type that implements


I think we should remove this function altogether.
It does not make sense to replace here an illformated span with an empty one and than check for it in the outer scope. Avoid the empty span as well.

I can't follow you. There is no ill-formed span (any operation that would produce one is already UB).

I see UB as anything can happen, but most likely there are no boundary checks to keep subspan() blazing fast, leading to a size_t overflow or such.

In our case not using the subspan_clamped() will speed up the code, because we can skip the empty span creation and the later isEmpty() check, if we will check for offset < size() in our business logic.

right, but offset < size() should not be part of the businesslogic. Explicit bounds checks are about as low level you can get so it doesn't make sense to have in high-level business logic. Thats explicitly why I put it into a utility function. From a high level perspective there is now only span. Either it has some sample content to play, or its empty and has none. This is a clear and intuitive API separation thats well worth paying two instructions for.

Swiftb0y · 2024-10-10T14:05:30Z

Unfortunately the non synced mode does not produce a stady click. It sounds more like as rhythm.

Fixed. mistakenly used kMaxEngineChannelInputCount instead of kEngineChannelOutputCount.

daschuer · 2024-10-10T22:40:55Z

src/effects/backends/builtin/metronomeeffect.cpp

-double framesPerBeat(mixxx::audio::SampleRate sampleRate, double bpm) {
-    double framesPerMinute = sampleRate * 60;
-    return framesPerMinute / bpm;
+std::size_t samplesPerBeat(mixxx::audio::SampleRate sampleRate, double bpm) {


The old name was better, because mixxx::audio::SampleRate is actually frame rate. We used the name sample rate because this is more common for audio and also used in external libraries. In a second step it can be than converted to monoSmaples whith the same value.

Oh, are you sure? I wasn't aware of that.

yup, confirmed. fixed in fixup

daschuer · 2024-10-10T22:44:41Z

src/effects/backends/builtin/metronomeeffect.cpp

+            std::size_t samplesSinceLastClick =
+                    gs->framesSinceLastClick * mixxx::kEngineChannelOutputCount;
+            std::size_t offset = samplesSinceLastClick %
+                    samplesPerBeat(engineParameters.sampleRate(),
+                            m_pBpmParameter->value());


Here is somewhere a 2x issue.

fixed in fixup.

daschuer · 2024-10-10T22:46:54Z

src/effects/backends/builtin/metronomeeffect.cpp

+    playMonoSamplesWithGain(subspan_clamped(click, gs->framesSinceLastClick), output, gain);
+    gs->framesSinceLastClick += engineParameters.framesPerBuffer();
+
+    std::span<CSAMPLE> outputBufferOffset = [&] {


Can't we keep the original logic? It is hard to check if all conditions are still considered.

No. That is the point of this PR. Independent of the std::span usage, this was the part I wanted to improve because I very much struggled with understanding the logic you came up with here. So I rewrote it in a way I found more intuitive.
I'm confident that all the logic is still there (except #13733 because I couldn't reproduce that here). It just has been disentangled into smaller functions.

We're at an impasse now because we simply disagree about what is more readable. I don't understand your code, you don't understand mine. So I suggest we ask other team members to evaluate what solution they prefer and decide based on that.

The new code looks much cleaner, but I can also not see, if it's functional identical.
May I suggest to implement a unit test for this code first, and than apply the refactured code in a second commit?

Sure, do we already test other effect code or do I need to come up with infrastructure for that first?

Regardless, It'll probably take some time until I come up with reasonable test code...

so I experimented a bit with this, but coming up with tests is incredibly tedious and brittle.

yeah, so spinning this a bit further. I think verifying the correctness by inspecting the buffer output is not sensible. At least not more than doing some QA testing by ear or by code review.

Swiftb0y · 2024-11-01T22:27:39Z

Heres my attempt at walking through the diff and explaining it along the way (text is always about the diff above)

The check is not needed anymore because subspan_clamped already takes care of it. calling playMonoSamples on an empty buffer doesn't do anything as you might expect.
Since this continued playing the buffer from a previous click (even if those zero because we already finished playing the sample), some time has passed so store that into gs->framesSinceLastClick accordingly.

these are equivalent. Only this time the entire body is encapsulated in its own function.

Here the preconditions are not satisfied so we can simply return with an empty span, which means nothing needs to be done. The else case handles #13727 which is not necessary because we already advance gs->framesSinceLastClick directly after (potentially) playing the buffer.

These two chunks are mathematically equivalent just rewritten in a way that I found a little easier to understand/meaningful.

These are also equivalent, only that this time I didn't make the assumption that gs->framesSinceLastClick was sufficiently small. This was an issue when playing a slow track and having the metronome synced and then switching to an unsynced click with sufficiently high speed (more than twice the track speed IIRC).

At this point we know that if we need to play new click we know where to play it in the buffer so just play it and record how far into the buffer that was. No need to shift framesSinceLastClick around "due to seeking" (honestly I don't understand why this is needed even today).

In summary:

I highly prefer this solution because it clearly breaks the problem down into subproblems (do we need to play the (remaining) click? where?) that are easily composed back together while hiding many of the uglier details about buffer management in the lower-level functions. I'm fairly confident that this refactor does not introduce regressions while raising maintainability significantly.

ChasonDeshotel

Agree fully with @Swiftb0y. This is a solid improvement, nicely done

I see UB as anything can happen, but most likely there are no boundary checks to keep subspan() blazing fast, leading to a size_t overflow or such.

This is what subspan_clamped does. It effectively mitigates the risk of undefined behavior by ensuring that the offset passed to subspan() never exceeds the size of the span.

As for the performance impact:

Cost of subspan_clamped

The subspan_clamped function adds a single std::min operation, which is a straightforward comparison between two size_t values. This operation:
Takes constant time: It’s an O(1) operation, meaning it doesn’t depend on the size of the data.
Is negligible in CPU cycles: On modern processors, a comparison and assignment generally take only a few nanoseconds, which is practically negligible, even for audio processing.

Some performance improvements that would be nice to add:

Make subspan_clamped inline
Store engineParameters.samplesPerBuffer(), engineParameters.framesPerBuffer(), and engineParameters.sampleRate() as local variables -- this would negate the additional cycles introduced from the extra function calls added with this PR. Not sure how/when metronomeeffect is instantiated or how we handle the case when the audio device changes elsewhere in the codebase

However, I wouldn't consider those a blocker. I imagine there's a lot better stuff to spend our time benchmarking if performance is an issue. Overall, these changes make the code more robust and maintainable without significant performance sacrifices.

Marked 'request changes' for inlining subspan_clamped. On the other, maybe @Swiftb0y has time to dig in or someone knows the answer already, if not, nbd.

Swiftb0y · 2024-11-06T17:21:36Z

Thank you @ChasonDeshotel for chiming in here. As for your suggestions.

Make subspan_clamped inline

It already is if the compiler decide its worth it (likely is).

Store engineParameters.samplesPerBuffer(), engineParameters.framesPerBuffer(), and engineParameters.sampleRate() as local variables

Not needed as those accessors implementations are already visible to the compiler, thus also likely inlined.

Feel free to verify these claims in compiler explorer.

daschuer · 2024-11-08T11:35:57Z

After a bit of testing I can confirm that this change works without regressions. I also confirm that the full use of span and the better separation of tasks improve the readability.

However there is still an issue with unnecessary playMonoSamplesWithGain() which impairs legibility. I have just proposed Swiftb0y#19

This avoids to call playMonoSamplesWithGain() when not necessary and solves some other issue in one go. It makes the code better traceable, because the cases where actually a new click is started can be better identified, it removes useless calls from the flow saving some ns and avoids the creation of empty spans entirely.

The patch also made the handling of the static bpm and quatized mode along with calling span::subspan() and span::last() symmetric and better comparable.

What do you think?

daschuer · 2024-11-08T11:39:00Z

The other issue is that the gain knob does not look right:

Did you consider to make it from 0 ... 4 ratio like the isolator gain knobs. This what unity is at center and it allows to go to full zero.

daschuer

.

Swiftb0y · 2024-11-08T12:18:15Z

The other issue is that the gain knob does not look right

Thats somewhat on purpose. The center of the knob is on 0db but since the sample is already quite close to 0db absolute I didn't want to allow for much range above 0db either, thats why its so asymmetrical. -24db is basically quiet even though its technically 0, its quiet enough. I figured a db readout was more informative than a unitless ratio. Do you disagree?

After a bit of testing I can confirm that this change works without regressions. I also confirm that the full use of span and the better separation of tasks improve the readability.

Thank you. Lets continue the code quality discussion on Swiftb0y#19

daschuer · 2024-11-08T14:08:05Z

I figured a db readout was more informative than a unitless ratio. Do you disagree?

I agree to that. That is a general issue with many effects.

Swiftb0y · 2024-11-08T14:12:12Z

great. so then keep the gain knob as is?

daschuer · 2024-11-08T14:23:00Z

If you insist. I still consider that ugly and prefer the solution we did for the idolators, to adopt the same pattern everywhere and than we can fix the missing unit issue in one go in a later PR.
Using ration is faster as a side effect.

Swiftb0y · 2024-11-08T15:00:20Z

I don't insist, though its not gonna be less work to do the ratio now and later in one go. Can you explain the ratio approach a little more to me? I assume that the 12 o'clock position is ${1 \over 1}$ and minimum is ${0 \over 1}$? What would the maximum be? Or do we just want this to be a level fader with $1.0$ as the maximum?

daschuer · 2024-11-08T22:22:35Z

The maximum number is what you like. 3 dB is ~1.4.
Like here:

mixxx/src/effects/backends/builtin/equalizer_util.h

Line 43 in 9381a84

mid->setRange(0, 1.0, maximum);

Swiftb0y · 2024-11-08T22:37:29Z

Right, but that means that the entire $[0, 1]$ range is on the left 50% of the pot and $[1, 1.4]$ on the right. So the pot is not continuous ("stetig" in german) anymore (in the mathematical sense). Thats not pretty either...

daschuer

Since this is already an improvement let's merge it and continue iterating from here.

github-actions bot added the effects label Sep 15, 2024

Swiftb0y force-pushed the review/daschauer/gh13611-metronome-effect-fix branch from 8db09c9 to b5c78c6 Compare September 15, 2024 14:30

Swiftb0y mentioned this pull request Oct 8, 2024

Metronome: Fix noise when pausing track in in the middle of the click sound. #13733

Merged

daschuer reviewed Oct 9, 2024

View reviewed changes

Swiftb0y force-pushed the review/daschauer/gh13611-metronome-effect-fix branch from 01b2a5f to b10d4b1 Compare October 10, 2024 14:04

daschuer reviewed Oct 10, 2024

View reviewed changes

Swiftb0y requested a review from daschuer October 14, 2024 20:04

Swiftb0y force-pushed the review/daschauer/gh13611-metronome-effect-fix branch from fe134f4 to 4850f4e Compare October 30, 2024 19:59

daschuer mentioned this pull request Nov 1, 2024

Improved comments in enginecontrol and use of std::size_t for bufferSize accross the codebase #13819

Merged

ChasonDeshotel suggested changes Nov 6, 2024

View reviewed changes

daschuer reviewed Nov 8, 2024

View reviewed changes

Swiftb0y mentioned this pull request Nov 8, 2024

Avoid unneccessary playMonoSamplesWithGain with empty spans Swiftb0y/mixxx#19

Open

Swiftb0y added 2 commits November 9, 2024 17:06

refactor: use higher-level std::span based logic

9d7b053

feat: add SampleUtil::addMonoToStereoWithGain

736034c

Swiftb0y added 3 commits November 9, 2024 17:06

refactor: wrap literals in QStringLiteral as appropriate

ce990e6

feat: add linked gain knob to metronome effect

ecababe

temp: incorporate style feedback

a092e8f

Swiftb0y force-pushed the review/daschauer/gh13611-metronome-effect-fix branch from f9f9da6 to a092e8f Compare November 9, 2024 16:06

daschuer approved these changes Nov 10, 2024

View reviewed changes

daschuer merged commit 2397016 into mixxxdj:main Nov 10, 2024
8 of 11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: use higher-level `std::span` based logic #13654

refactor: use higher-level `std::span` based logic #13654

Swiftb0y commented Sep 15, 2024

Swiftb0y commented Oct 4, 2024

daschuer commented Oct 9, 2024

daschuer Oct 9, 2024

Swiftb0y Oct 10, 2024

daschuer Oct 10, 2024

Swiftb0y Oct 11, 2024

Swiftb0y commented Oct 10, 2024

daschuer Oct 10, 2024

Swiftb0y Oct 11, 2024

Swiftb0y Oct 11, 2024

daschuer Oct 10, 2024

Swiftb0y Oct 11, 2024

daschuer Oct 10, 2024

Swiftb0y Oct 11, 2024

JoergAtGithub Oct 30, 2024

Swiftb0y Oct 30, 2024

Swiftb0y Oct 30, 2024

Swiftb0y Nov 1, 2024

Swiftb0y Nov 1, 2024

Swiftb0y commented Nov 1, 2024 •

edited

Loading

ChasonDeshotel left a comment

Swiftb0y commented Nov 6, 2024 •

edited

Loading

daschuer commented Nov 8, 2024

daschuer commented Nov 8, 2024

daschuer left a comment

Swiftb0y commented Nov 8, 2024

daschuer commented Nov 8, 2024

Swiftb0y commented Nov 8, 2024

daschuer commented Nov 8, 2024

Swiftb0y commented Nov 8, 2024

daschuer commented Nov 8, 2024

Swiftb0y commented Nov 8, 2024

daschuer left a comment

refactor: use higher-level std::span based logic #13654

refactor: use higher-level std::span based logic #13654

Conversation

Swiftb0y commented Sep 15, 2024

Swiftb0y commented Oct 4, 2024

daschuer commented Oct 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Swiftb0y commented Oct 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Swiftb0y commented Nov 1, 2024 • edited Loading

ChasonDeshotel left a comment

Choose a reason for hiding this comment

Swiftb0y commented Nov 6, 2024 • edited Loading

daschuer commented Nov 8, 2024

daschuer commented Nov 8, 2024

daschuer left a comment

Choose a reason for hiding this comment

Swiftb0y commented Nov 8, 2024

daschuer commented Nov 8, 2024

Swiftb0y commented Nov 8, 2024

daschuer commented Nov 8, 2024

Swiftb0y commented Nov 8, 2024

daschuer commented Nov 8, 2024

Swiftb0y commented Nov 8, 2024

daschuer left a comment

Choose a reason for hiding this comment

refactor: use higher-level `std::span` based logic #13654

refactor: use higher-level `std::span` based logic #13654

Swiftb0y commented Nov 1, 2024 •

edited

Loading

Swiftb0y commented Nov 6, 2024 •

edited

Loading