Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 26 additions & 9 deletions src/effects/backends/builtin/echoeffect.cpp
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
#include "effects/backends/builtin/echoeffect.h"

#include <span>

#include "effects/backends/effectmanifest.h"
#include "engine/effects/engineeffectparameter.h"
#include "util/math.h"
Expand Down Expand Up @@ -118,6 +120,14 @@
m_pTripletParameter = parameters.value("triplet");
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

float differenceSum = 0.0f;
for (const CSAMPLE sample : delay_buffer.span()) {

Check failure on line 125 in src/effects/backends/builtin/echoeffect.cpp

View workflow job for this annotation

GitHub Actions / clazy

cannot refer to type member 'span' in 'std::span<const CSAMPLE>' (aka 'span<const float>') with '.'

Check failure on line 125 in src/effects/backends/builtin/echoeffect.cpp

View workflow job for this annotation

GitHub Actions / Ubuntu 24.04

invalid use of ‘std::span<const float>::span’

Check failure on line 125 in src/effects/backends/builtin/echoeffect.cpp

View workflow job for this annotation

GitHub Actions / macOS 13 x64

cannot refer to type member 'span' in 'std::span<const CSAMPLE>' (aka 'span<const float>') with '.'

Check failure on line 125 in src/effects/backends/builtin/echoeffect.cpp

View workflow job for this annotation

GitHub Actions / clang-tidy

cannot refer to type member 'span' in 'std::span<const CSAMPLE>' (aka 'span<const float>') with '.' [clang-diagnostic-error]

Check failure on line 125 in src/effects/backends/builtin/echoeffect.cpp

View workflow job for this annotation

GitHub Actions / macOS 13 arm64

cannot refer to type member 'span' in 'std::span<const CSAMPLE>' (aka 'span<const float>') with '.'

Check failure on line 125 in src/effects/backends/builtin/echoeffect.cpp

View workflow job for this annotation

GitHub Actions / coverage

invalid use of ‘std::span<const float>::span’

Check failure on line 125 in src/effects/backends/builtin/echoeffect.cpp

View workflow job for this annotation

GitHub Actions / Windows 2019 (MSVC)

Type name 'std::span<const CSAMPLE,18446744073709551615>' cannot appear on the right side of a class member access expression

Check failure on line 125 in src/effects/backends/builtin/echoeffect.cpp

View workflow job for this annotation

GitHub Actions / Windows 2019 (MSVC)

'sample': 'const' object must be initialized if not 'extern'

Check failure on line 125 in src/effects/backends/builtin/echoeffect.cpp

View workflow job for this annotation

GitHub Actions / Windows 2019 (MSVC)

syntax error: missing ';' before ':'

Check failure on line 125 in src/effects/backends/builtin/echoeffect.cpp

View workflow job for this annotation

GitHub Actions / Windows 2019 (MSVC)

syntax error: missing ';' before ')'
differenceSum += std::abs(sample);
}
return differenceSum / delay_buffer.size();
}

void EchoEffect::processChannel(
EchoGroupState* pGroupState,
const CSAMPLE* pInput,
Expand All @@ -127,10 +137,16 @@
const GroupFeatureState& groupFeatures) {
// The minimum of the parameter is zero so the exact center of the knob is 1 beat.
double period = m_pDelayParameter->value();
const auto send_current = static_cast<CSAMPLE_GAIN>(m_pSendParameter->value());
const auto send_current = enableState == EffectEnableState::Disabling
? 0
: static_cast<CSAMPLE_GAIN>(m_pSendParameter->value());
const auto feedback_current = static_cast<CSAMPLE_GAIN>(m_pFeedbackParameter->value());
const auto pingpong_frac = static_cast<CSAMPLE_GAIN>(m_pPingPongParameter->value());

if (enableState == EffectEnableState::Enabling) {
m_isReadyForDisable = false;
}

int delay_frames;
if (groupFeatures.beat_length.has_value()) {
// period is a number of beats
Expand Down Expand Up @@ -236,16 +252,17 @@
pGroupState->ping_pong = 0;
}
}

// The ramping of the send parameter handles ramping when enabling, so
// this effect must handle ramping to dry when disabling itself (instead
// of being handled by EngineEffect::process).
pGroupState->prev_send = send_current;
if (enableState == EffectEnableState::Disabling) {
SampleUtil::applyRampingGain(pOutput, 1.0, 0.0, engineParameters.samplesPerBuffer());
pGroupState->delay_buf.clear();
pGroupState->prev_send = 0;
} else {
pGroupState->prev_send = send_current;
const SINT delayBufferSize = pGroupState->delay_buf.size();
// Calculate if the delayline-buffer is approx. zero/empty.
const float avgSampleEnergy = averageSampleEnergy(pGroupState->delay_buf.span().first(engineParameters.sampleRate()));
// If echo tail fully faded
if (avgSampleEnergy < (0.00001f / delayBufferSize)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Copy link
Author

@mrnicegyu11 mrnicegyu11 Feb 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Iterator first_sound(Iterator begin, Iterator end) {

m_isReadyForDisable = true;
pGroupState->delay_buf.clear();
}
}

pGroupState->prev_feedback = feedback_current;
Expand Down
6 changes: 4 additions & 2 deletions src/effects/backends/builtin/echoeffect.h
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,9 @@ class EchoEffect : public EffectProcessorImpl<EchoGroupState> {

void loadEngineEffectParameters(
const QMap<QString, EngineEffectParameterPointer>& parameters) override;

bool isReadyForDisable() override {
return m_isReadyForDisable;
};
void processChannel(
EchoGroupState* pState,
const CSAMPLE* pInput,
Expand All @@ -73,6 +75,6 @@ class EchoEffect : public EffectProcessorImpl<EchoGroupState> {
EngineEffectParameterPointer m_pPingPongParameter;
EngineEffectParameterPointer m_pQuantizeParameter;
EngineEffectParameterPointer m_pTripletParameter;

bool m_isReadyForDisable = false;
DISALLOW_COPY_AND_ASSIGN(EchoEffect);
};
40 changes: 32 additions & 8 deletions src/effects/backends/builtin/reverbeffect.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,24 @@ void ReverbEffect::loadEngineEffectParameters(
m_pSendParameter = parameters.value("send_amount");
}

float averageSampleDifferenceEnergy(const SINT samplesPerBuffer,
const CSAMPLE* buffer_in,
const CSAMPLE* buffer_out,
const SINT tailCheckLength) {
if (tailCheckLength == 0) {
return 0;
}
float differenceSum = 0.0f;
for (SINT i = samplesPerBuffer - tailCheckLength;
i < samplesPerBuffer;
++i) {
differenceSum += std::abs(buffer_out[i] - buffer_in[i]);
}
// Calculate average of the differences
const float averageDifference = differenceSum / tailCheckLength;
return averageDifference;
}

void ReverbEffect::processChannel(
ReverbGroupState* pState,
const CSAMPLE* pInput,
Expand All @@ -89,7 +107,9 @@ void ReverbEffect::processChannel(
const auto decay = static_cast<sample_t>(m_pDecayParameter->value());
const auto bandwidth = static_cast<sample_t>(m_pBandWidthParameter->value());
const auto damping = static_cast<sample_t>(m_pDampingParameter->value());
const auto sendCurrent = static_cast<sample_t>(m_pSendParameter->value());
const auto sendCurrent = enableState == EffectEnableState::Disabling
? 0
: static_cast<sample_t>(m_pSendParameter->value());

// Reinitialize the effect when turning it on to prevent replaying the old buffer
// from the last time the effect was enabled.
Expand All @@ -98,6 +118,7 @@ void ReverbEffect::processChannel(
pState->sampleRate != engineParameters.sampleRate()) {
pState->reverb.init(engineParameters.sampleRate());
pState->sampleRate = engineParameters.sampleRate();
m_isReadyForDisable = false;
}

pState->reverb.processBuffer(pInput,
Expand All @@ -109,13 +130,16 @@ void ReverbEffect::processChannel(
sendCurrent,
pState->sendPrevious);

// The ramping of the send parameter handles ramping when enabling, so
// this effect must handle ramping to dry when disabling itself (instead
// of being handled by EngineEffect::process).
if (enableState == EffectEnableState::Disabling) {
SampleUtil::applyRampingGain(pOutput, 1.0, 0.0, engineParameters.samplesPerBuffer());
pState->sendPrevious = 0;
} else {
pState->sendPrevious = sendCurrent;
// Calculate absolute difference between wet and dry buffers for the tail
const SINT tailCheckLength = engineParameters.samplesPerBuffer() / 4;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have here two delay filters per channel:

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?

const float averageDifference = averageSampleDifferenceEnergy(
engineParameters.samplesPerBuffer(),
pInput,
pOutput,
tailCheckLength);
if (averageDifference < 0.002f) {
m_isReadyForDisable = true;
}
}
}
10 changes: 10 additions & 0 deletions src/effects/backends/builtin/reverbeffect.h
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@ class ReverbEffect : public EffectProcessorImpl<ReverbGroupState> {

static QString getId();
static EffectManifestPointer getManifest();
bool isReadyForDisable() override {
return m_isReadyForDisable;
};

void loadEngineEffectParameters(
const QMap<QString, EngineEffectParameterPointer>& parameters) override;
Expand All @@ -54,10 +57,17 @@ class ReverbEffect : public EffectProcessorImpl<ReverbGroupState> {
return getId();
}

bool m_isReadyForDisable = false;

EngineEffectParameterPointer m_pDecayParameter;
EngineEffectParameterPointer m_pBandWidthParameter;
EngineEffectParameterPointer m_pDampingParameter;
EngineEffectParameterPointer m_pSendParameter;

DISALLOW_COPY_AND_ASSIGN(ReverbEffect);
};

float averageSampleDifferenceEnergy(const SINT samplesPerBuffer,
const CSAMPLE* buffer_in,
const CSAMPLE* buffer_out,
const SINT tailCheckLength);
mrnicegyu11 marked this conversation as resolved.
Show resolved Hide resolved
3 changes: 3 additions & 0 deletions src/effects/backends/effectprocessor.h
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
return true;
};
};

/// EffectProcessorImpl manages a separate EffectState for every combination of
Expand Down
2 changes: 1 addition & 1 deletion src/engine/effects/engineeffect.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ bool EngineEffect::process(const ChannelHandle& inputHandle,
// Now that the EffectProcessor has been sent the intermediate enabling/disabling
// signal, set the channel state to fully enabled/disabled for the next engine callback.
EffectEnableState& effectOnChannelState = m_effectEnableStateForChannelMatrix[inputHandle][outputHandle];
if (effectOnChannelState == EffectEnableState::Disabling) {
if (effectOnChannelState == EffectEnableState::Disabling && m_pProcessor->isReadyForDisable()) {
effectOnChannelState = EffectEnableState::Disabled;
} else if (effectOnChannelState == EffectEnableState::Enabling) {
effectOnChannelState = EffectEnableState::Enabled;
Expand Down
Loading