refactor: use higher-level std::span based logic
#13654
Conversation
Swiftb0y
force-pushed
the
review/daschauer/gh13611-metronome-effect-fix
branch
from
8db09c9
to
b5c78c6
Compare
|
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. |
|
Unfortunately the non synced mode does not produce a stady click. It sounds more like as rhythm. |
|
|
||
| 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I can't follow you. There is no ill-formed span (any operation that would produce one is already UB).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
force-pushed
the
review/daschauer/gh13611-metronome-effect-fix
branch
from
01b2a5f
to
b10d4b1
Compare
Fixed. mistakenly used |
| 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) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Oh, are you sure? I wasn't aware of that.
There was a problem hiding this comment.
yup, confirmed. fixed in fixup
| std::size_t samplesSinceLastClick = | ||
| gs->framesSinceLastClick * mixxx::kEngineChannelOutputCount; | ||
| std::size_t offset = samplesSinceLastClick % | ||
| samplesPerBeat(engineParameters.sampleRate(), | ||
| m_pBpmParameter->value()); |
| playMonoSamplesWithGain(subspan_clamped(click, gs->framesSinceLastClick), output, gain); | ||
| gs->framesSinceLastClick += engineParameters.framesPerBuffer(); | ||
|
|
||
| std::span<CSAMPLE> outputBufferOffset = [&] { |
There was a problem hiding this comment.
Can't we keep the original logic? It is hard to check if all conditions are still considered.
There was a problem hiding this comment.
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.
This is daschuer#97 rebased on main with the primary goal of making the logic easier to understand.