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

Swiftb0y · 2025-01-02T10:25:15Z

src/effects/backends/builtin/reverbeffect.cpp

+        // Function to determine if reverb tail is gone
+        // Calculate absolute difference between wet and dry buffers for the tail
+        float differenceSum = 0.0f;
+        const SINT tailCheckLength = engineParameters.samplesPerBuffer() / 4;
+        for (SINT i = engineParameters.samplesPerBuffer() - tailCheckLength;
+                i < engineParameters.samplesPerBuffer();
+                ++i) {
+            differenceSum += fabsf(pOutput[i] - pInput[i]);
+        }
+        // Calculate average of the differences
+        const float averageDifference = differenceSum / tailCheckLength;
+        constexpr float threshold = 0.002f;
+        const bool reverbTailFullyFaded = averageDifference < threshold;
+        if (reverbTailFullyFaded) {
+            m_isReadyForDisable = true;
+        }


Then lets make it a function

I made it into a function :--)

Swiftb0y · 2025-01-02T10:26:00Z

src/effects/backends/builtin/echoeffect.cpp

+        // Function to determine if echo tail is gone
+        // Calculate absolute if the delayline-buffer is approx. zero/empty.
        pGroupState->prev_send = 0;
-    } else {
-        pGroupState->prev_send = send_current;
+        float differenceSum = 0.0f;
+        for (SINT i = 0; i < pGroupState->delay_buf.size();
+                i = i + 16) { // We don't need to consider *all* samples,
+                              // assuming 44.1kHz sample-rate this sums every
+                              // ~1/3ms
+            differenceSum += fabsf(pGroupState->delay_buf[i]);
+        }
+        // Calculate average of the differences
+        constexpr float threshold = 0.00001f;
+        const bool echoTailFullyFaded = differenceSum < threshold;
+        if (echoTailFullyFaded) {
+            m_isReadyForDisable = true;
+
+            pGroupState->delay_buf.clear();
+        }


same here. lets just actually make a function.

I made it into a function :--)

Swiftb0y · 2025-01-04T10:46:34Z

src/effects/backends/effectprocessor.h

@@ -110,6 +110,9 @@ class EffectProcessor {
    /// the dry signal is delayed to overlap with the output wet signal
    /// after processing all effects in the effects chain.
    virtual SINT getGroupDelayFrames() = 0;
+    virtual bool isReadyForDisable(void) {


Style NIT:

Suggested change

virtual bool isReadyForDisable(void) {

virtual bool isReadyForDisable() {

Swiftb0y · 2025-01-04T10:47:42Z

src/effects/backends/builtin/reverbeffect.h

+float averageSampleDifferenceEnergy(const SINT samplesPerBuffer,
+        const CSAMPLE* buffer_in,
+        const CSAMPLE* buffer_out,
+        const SINT tailCheckLength);


If you don't need to call it from outside the class, don't declare it in the header. Move it into the anonymous namespace in the .cpp instead.

Suggested change

float averageSampleDifferenceEnergy(const SINT samplesPerBuffer,

const CSAMPLE* buffer_in,

const CSAMPLE* buffer_out,

const SINT tailCheckLength);

Swiftb0y · 2025-01-04T10:48:39Z

src/effects/backends/builtin/reverbeffect.cpp

+    for (SINT i = samplesPerBuffer - tailCheckLength;
+            i < samplesPerBuffer;
+            ++i) {
+        differenceSum += fabsf(buffer_out[i] - buffer_in[i]);


Suggested change

differenceSum += fabsf(buffer_out[i] - buffer_in[i]);

differenceSum += std::abs(buffer_out[i] - buffer_in[i]);

Swiftb0y · 2025-01-04T10:50:22Z

src/effects/backends/builtin/reverbeffect.cpp

+        const SINT tailCheckLength) {
+    float differenceSum = 0.0f;


avoid div-by-zero.

Suggested change

const SINT tailCheckLength) {

float differenceSum = 0.0f;

const SINT tailCheckLength) {

if (tailCheckLength == 0) {

return 0;

}

float differenceSum = 0.0f;

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.

Swiftb0y · 2025-01-04T10:55:16Z

src/effects/backends/builtin/echoeffect.cpp

+                          // ~1/3ms
+        differenceSum += fabsf(delay_buffer[i]);
+    }
+    return differenceSum;


This is not an average, so the function name is slightly confusing IMO. Lets just return the average instead or change the name.

Swiftb0y · 2025-01-04T11:55:27Z

src/effects/backends/builtin/echoeffect.cpp

+    for (SINT i = 0; i < delayBufferSize;
+            i = i + 16) { // We don't need to consider *all* samples,


Are you sure? This just seems like downsampling that would introduce artifacts and miss high frequency signals. Is this just an optimization?

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 :--)

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

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

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 2, 2025

mrnicegyu11 Jan 4, 2025

Swiftb0y Jan 2, 2025

mrnicegyu11 Jan 4, 2025

Swiftb0y Jan 4, 2025

Swiftb0y Jan 4, 2025

Swiftb0y Jan 4, 2025

Swiftb0y Jan 4, 2025

Swiftb0y Jan 4, 2025

Swiftb0y Jan 4, 2025

Swiftb0y Jan 4, 2025

mrnicegyu11 commented Jan 13, 2025

	virtual bool isReadyForDisable(void) {
	virtual bool isReadyForDisable() {

	differenceSum += fabsf(buffer_out[i] - buffer_in[i]);
	differenceSum += std::abs(buffer_out[i] - buffer_in[i]);

		for (SINT i = 0; i < delayBufferSize;
		i = i + 16) { // We don't need to consider all samples,

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

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

mrnicegyu11 commented Jan 13, 2025