Allow effect to continue processing if its buffer isn't empty (builtin reverb/delay) #14099

mrnicegyu11 · 2025-01-01T16:22:06Z

Summary

Hey guys, this imperfection of mixxx's default effects has frustrated me personally when using mixxx in the past, and now I have set out to potentially fix it ;) . This functionality feels much better than the current "hard-cutoff" for me. It enhances the perception of turning on/off reverb or delay effects (that have a buffer) by playing the effect until the buffer is empty. This is how the reverb/echo effects of e.g. Pioneer Digital DJ-Mixers behave.

Works smoothly in my local builds, all tests are passing for this code. I would be very happy about a review and comments on what would be needed to potentially get this in :--)

Cheers, all the best to you.

Related Issues

Notable Code changes

The EffectEnableState::Disabling state would previously be used only for one buffer-processing iteration, as far as I can see mainly to ramp volume. With this new code, the EffectEnableState::Disabling can be prolonged by effects responding to the call to isReadyForDisable() with false, and the effect can continue playing sound.
The EffectProcessor base-class gets the new function virtual bool isReadyForDisable(void) that defaults to true (current behaviour, hard cutoff of sound when effect is switched off). By overwriting this function effects can control weather they still have sound to play, even after the "shutoff" button has been pressed, or not.

When will effects then be disabled?

Reverb: One sample-buffer of the reverb's combined dry-wet output is identical to the input. More precisely, the energy (summed abs values) of one sample-buffer of the combined dry-wet output is identical to the input buffer's energy, up to a small threshold.
Echo (Feedback < 1): When the delay's ringbuffer is is approx. zero everywhere, all echos are faded out.
Echo (Feedback == 1): Turning the effect off would now never actually turn it off, since the delayline would never empty and the tail that continues playing is infinitely long. The echo would perpetually stay in EffectEnableState::Disabling. I find this not ideal, although one seldomly sets feedback==1 in practice I guess. Any ideas?
All other effects: Behavior is as before, volume is ramped down over one buffer-processing length and practically speaking effects are switched off immediately.

Ideas

At first I wanted to add a button to the echo&reverb effects that control the shut-off behavior, but after pondering this idea for a few days I convinced myself that a reverb or a delay emptying their buffers is the natural and expected user experience, and the commercial tools also implement the effects like this usually. An extra button I guess is not warranted, no such "shutoff-behaviour-control-button" would e.g. be found on hardware delay/reverb units. But this is I guess opinionated and maybe debatable :--)

JoergAtGithub · 2025-01-01T17:41:02Z

Happy New Year and Welcome at Mixxx!
As a first-time contributor we need you to sign the Mixxx Contributor Agreement and comment here when you have done so. It gives us permission to distribute your contribution under the GPL v2 or later license and the Apple Mac App Store. It is also helpful for us to have contact information for contributors in case we may need it in the future.

mrnicegyu11 · 2025-01-02T07:56:13Z

Hi @JoergAtGithub , happy new year as well to you and thanks for answering! I have just signed the Mixxx Contributor Agreement 💪

Swiftb0y

Thanks for your feature. Lets iterate on this a bit.

src/effects/backends/effectprocessor.h

src/effects/backends/builtin/reverbeffect.cpp

src/effects/backends/builtin/echoeffect.cpp

src/effects/backends/effectprocessor.h

src/effects/backends/builtin/reverbeffect.h

src/effects/backends/builtin/reverbeffect.cpp

Swiftb0y · 2025-01-04T10:54:08Z

src/effects/backends/builtin/echoeffect.cpp

@@ -118,6 +118,17 @@ void EchoEffect::loadEngineEffectParameters(
    m_pTripletParameter = parameters.value("triplet");
 }

+float averageSampleEnergy(SINT delayBufferSize, mixxx::SampleBuffer const& delay_buffer) {


consider taking a std::span<const CSAMPLE> instead. You can easily obtain one from the a mixxx::SampleBuffer using its span() member function.

done but I feel a bit insecure about this, please have a look if this is as you intended ;)

src/effects/backends/builtin/echoeffect.cpp

mrnicegyu11 · 2025-01-13T09:14:06Z

@Swiftb0y thanks for the thorough review, definitely some valid points. January is a bit hectic for me, I will address them and ask for re-review when I have some quiet time :--)

acolombier · 2025-02-05T02:25:03Z

Thanks for looking into this feature @mrnicegyu11! I am definitely interested in getting this in 2.6 beta, planned for the end of the month. Do you need some help to complete this PR?

mrnicegyu11 · 2025-02-07T08:01:29Z

@acolombier Thanks for your kind words, I will try to address the comments in the PR this weekend. If I dont manage till then please feel free to jump in.

Just as a disclaimer: I've worked with juce and such in the past but I don't know the details of the mixxx codebase, so my codestyle is probably off. Nevertheless, to the best of my judgement, my codechanges should just work. Actually I use my local build for DJ'ing since I made the changes, and it works like a charm on my archlinux laptop :--)

mrnicegyu11 · 2025-02-07T08:02:30Z

Looking again I agree with the reviewer fully though that I have some ambiguous naming and some premature optimizations in there, probably best to clean that up once more

Co-authored-by: Swiftb0y <[email protected]>

…gy() from outside the class, don't declare it in the header

…ate averageSampleEnergy over whole delay ringbuffer, actually return avergae

mrnicegyu11 · 2025-02-08T07:57:51Z

Alright, change requests are addressed 🦾 hope this is ok, let me know, thanks for the very friendly communication to you guys, very welcoming :--)

mrnicegyu11 · 2025-02-08T07:58:04Z

CC @acolombier ;)

Swiftb0y

couple more nits

src/effects/backends/builtin/echoeffect.cpp

src/effects/backends/builtin/echoeffect.h

Swiftb0y · 2025-02-08T09:54:22Z

src/effects/backends/builtin/echoeffect.cpp

+        // Calculate if the delayline-buffer is approx. zero/empty.
+        const float avgSampleEnergy = averageSampleEnergy(pGroupState->delay_buf);
+        // If echo tail fully faded
+        if (avgSampleEnergy < (0.00001f / delayBufferSize)) {


why does the energy depend on the buffer size? The average shouldn't be influenced by the sample size of the dataset.

If the concern is that the fading tail influences the current average, simply don't sample it. Only sample as much as you need:

The average sample energy depends on the buffer-size since it is computed from the buffer (average over buffer length, the average over the next buffer as well, or only half the buffer, would be numerically different). Thus, avgSampleEnergy implicitly depends on the buffer length.

Before your previous review, the averageSampleEnergy would not return an average but the summed Sample Energy over the buffer, this was simply bad code and is now fixed. To retain the exactly same threshold which was test-driven by me as before, the 0.00001f magic number now needs to be divided by the delayBufferSize. Of course I can also shift the 0.00001f a bit by making it 100x smaller or so and call it a day :--) let me know

Thus, avgSampleEnergy implicitly depends on the buffer length.

Well, now that we're only looking at the samples at the beginning of the delay buffer (exactly the samples that feedback into the effect to be exact) the size shouldn't anymore, right? Am I making sense here?

Besides I think .first(engineParameters.sampleRate()) is probably wrong, my mistake. I meant .first(<how big the current playback buffer is>)...

Mhm I am not sure I follow sorry, I think the average is calculated over the whole delay buffer. But we can in any case remove the division by the delay buffer length in the treshhold check. This is just numerics:--)

It looks like the solution is not indepenedent of single clicks vs pink noise.
The noise value will contribute way more than a single click.

If we assume that the last added sample was above the threshold, we can alternatively just consume the number samples according to the current picked delay value. Not sure if the earlay disabling has benefits. Does it?

Did you consicder to loop through the whole buffer and continue once you have found a single semale above a certain thresshold?

Maybe copy/reuse the approch that we took here:

mixxx/src/analyzer/analyzersilence.cpp

Line 27 in ae2dc10

Iterator first_sound(Iterator begin, Iterator end) {

src/effects/backends/builtin/echoeffect.cpp

Co-authored-by: Swiftb0y <[email protected]>

mrnicegyu11 · 2025-02-10T16:26:36Z

@Swiftb0y please check again :--)

daschuer · 2025-02-11T08:01:28Z

src/effects/backends/builtin/echoeffect.cpp

@@ -118,6 +120,14 @@ void EchoEffect::loadEngineEffectParameters(
    m_pTripletParameter = parameters.value("triplet");
 }

+float averageSampleEnergy(std::span<const CSAMPLE> delay_buffer) {


Energy is voltage * current. Here you use only voltage.

Suggested change

float averageSampleEnergy(std::span<const CSAMPLE> delay_buffer) {

float averageSampleLevel(std::span<const CSAMPLE> delay_buffer) {

We collect here such sample functions that are reusable together with a hint that indicates if the function is auto vectorized.

This is probably already the case here, but worth to double check.

mixxx/src/util/sample.cpp

Line 463 in ae2dc10

CSAMPLE SampleUtil::maxAbsAmplitude(const CSAMPLE* pBuffer, SINT numSamples) {

daschuer

Thank you for this nice PR and sorry for juming in late.
I have left some comments.

daschuer · 2025-02-11T09:36:42Z

src/effects/backends/builtin/echoeffect.cpp

+        // Calculate if the delayline-buffer is approx. zero/empty.
+        const float avgSampleEnergy = averageSampleEnergy(pGroupState->delay_buf);
+        // If echo tail fully faded
+        if (avgSampleEnergy < (0.00001f / delayBufferSize)) {


It looks like the solution is not indepenedent of single clicks vs pink noise.
The noise value will contribute way more than a single click.

If we assume that the last added sample was above the threshold, we can alternatively just consume the number samples according to the current picked delay value. Not sure if the earlay disabling has benefits. Does it?

Did you consicder to loop through the whole buffer and continue once you have found a single semale above a certain thresshold?

Maybe copy/reuse the approch that we took here:

mixxx/src/analyzer/analyzersilence.cpp

Line 27 in ae2dc10

Iterator first_sound(Iterator begin, Iterator end) {

daschuer · 2025-02-11T09:38:19Z

src/effects/backends/builtin/reverbeffect.cpp

-    } else {
-        pState->sendPrevious = sendCurrent;
+        // Calculate absolute difference between wet and dry buffers for the tail
+        const SINT tailCheckLength = engineParameters.samplesPerBuffer() / 4;


Why you /4 ?
It would be ideal if the feature works the same independent form samplesPerBuffer().

We have here two delay filters per channel:

mixxx/lib/reverb/Reverb.cc

Line 216 in ae2dc10

tank.delay[0].init (L(6));

Initalized with 2^6 and 2^8 buffer size. This means the maximum delay is 256 frames.
The issue here in addition is that you are checking the current buffer and not was has added to the reverb.

EffectEnableState::Disabling means that no new samples are added, right?
Cant we just always process additional 256 Samples maybe a few more because of band filters, and than disable?

mrnicegyu11 added 5 commits January 1, 2025 16:20

Add fade-out to reverb

ac70a9a

Add fade-out to echo

8fe86ec

Merge remote-tracking branch 'upstream/main' into 2025/add/reverbTail

b3938ea

minor: better comment

c8e5222

minor

38f1858

github-actions bot added effects engine labels Jan 1, 2025

Swiftb0y requested changes Jan 2, 2025

View reviewed changes

src/effects/backends/effectprocessor.h Outdated Show resolved Hide resolved

src/effects/backends/builtin/reverbeffect.cpp Outdated Show resolved Hide resolved

src/effects/backends/builtin/echoeffect.cpp Outdated Show resolved Hide resolved

daschuer changed the title ~~✨ Allow effect to continue processing if its buffer isn't empty (builtin reverb/delay)~~ Allow effect to continue processing if its buffer isn't empty (builtin reverb/delay) Jan 2, 2025

Factor tail processing into function

e3cd6bb

mrnicegyu11 requested a review from Swiftb0y January 4, 2025 10:29

Swiftb0y reviewed Jan 4, 2025

View reviewed changes

Merge remote-tracking branch 'upstream/main' into 2025/add/reverbTail

29f5317

acolombier added this to the 2.6-beta milestone Feb 5, 2025

mrnicegyu11 closed this Feb 7, 2025

mrnicegyu11 reopened this Feb 7, 2025

mrnicegyu11 and others added 7 commits February 8, 2025 07:46

Update src/effects/backends/effectprocessor.h

c1ba962

Co-authored-by: Swiftb0y <[email protected]>

Update src/effects/backends/builtin/reverbeffect.cpp

2153a91

Co-authored-by: Swiftb0y <[email protected]>

Update src/effects/backends/builtin/reverbeffect.cpp

9f2fd6f

Co-authored-by: Swiftb0y <[email protected]>

Change Request @Swiftb0y: If you don't need to call averageSampleEner…

a9a679c

…gy() from outside the class, don't declare it in the header

Merge remote-tracking branch 'upstream' into 2025/add/reverbTail

1920ffc

Change Request @Swiftb0y: Remove premature/wrong optimization, calcul…

29b6fa2

…ate averageSampleEnergy over whole delay ringbuffer, actually return avergae

Use std span - change request @Swiftb0y

aaf6d09

mrnicegyu11 requested a review from Swiftb0y February 8, 2025 07:57

Swiftb0y reviewed Feb 8, 2025

View reviewed changes

mrnicegyu11 and others added 4 commits February 10, 2025 16:42

Update src/effects/backends/builtin/echoeffect.cpp

3654121

Co-authored-by: Swiftb0y <[email protected]>

Update src/effects/backends/builtin/echoeffect.cpp

ebba883

Co-authored-by: Swiftb0y <[email protected]>

Move include span from .h to .cpp - change request @Swiftb0y

3f1f385

Merge remote-tracking branch 'upstream/main' into 2025/add/reverbTail

fe00dbf

mrnicegyu11 requested a review from Swiftb0y February 10, 2025 16:25

daschuer reviewed Feb 11, 2025

View reviewed changes

daschuer requested changes Feb 11, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow effect to continue processing if its buffer isn't empty (builtin reverb/delay) #14099

Allow effect to continue processing if its buffer isn't empty (builtin reverb/delay) #14099

mrnicegyu11 commented Jan 1, 2025

JoergAtGithub commented Jan 1, 2025

mrnicegyu11 commented Jan 2, 2025

Swiftb0y left a comment

Swiftb0y Jan 4, 2025

mrnicegyu11 Feb 8, 2025

mrnicegyu11 commented Jan 13, 2025

acolombier commented Feb 5, 2025

mrnicegyu11 commented Feb 7, 2025

mrnicegyu11 commented Feb 7, 2025

mrnicegyu11 commented Feb 8, 2025

mrnicegyu11 commented Feb 8, 2025

Swiftb0y left a comment

Swiftb0y Feb 8, 2025

mrnicegyu11 Feb 10, 2025 •

edited

Loading

Swiftb0y Feb 10, 2025

mrnicegyu11 Feb 11, 2025

daschuer Feb 11, 2025

mrnicegyu11 commented Feb 10, 2025

daschuer Feb 11, 2025

daschuer Feb 11, 2025

daschuer left a comment

daschuer Feb 11, 2025

daschuer Feb 11, 2025

daschuer Feb 11, 2025

	float averageSampleEnergy(std::span<const CSAMPLE> delay_buffer) {
	float averageSampleLevel(std::span<const CSAMPLE> delay_buffer) {

Allow effect to continue processing if its buffer isn't empty (builtin reverb/delay) #14099

Are you sure you want to change the base?

Allow effect to continue processing if its buffer isn't empty (builtin reverb/delay) #14099

Conversation

mrnicegyu11 commented Jan 1, 2025

Summary

Related Issues

Notable Code changes

When will effects then be disabled?

Ideas

JoergAtGithub commented Jan 1, 2025

mrnicegyu11 commented Jan 2, 2025

Swiftb0y left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrnicegyu11 commented Jan 13, 2025

acolombier commented Feb 5, 2025

mrnicegyu11 commented Feb 7, 2025

mrnicegyu11 commented Feb 7, 2025

mrnicegyu11 commented Feb 8, 2025

mrnicegyu11 commented Feb 8, 2025

Swiftb0y left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrnicegyu11 Feb 10, 2025 • edited Loading

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

mrnicegyu11 commented Feb 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer left a 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

mrnicegyu11 Feb 10, 2025 •

edited

Loading