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 6 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
36 changes: 27 additions & 9 deletions src/effects/backends/builtin/echoeffect.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,17 @@ void EchoEffect::loadEngineEffectParameters(
m_pTripletParameter = parameters.value("triplet");
}

float averageSampleEnergy(SINT delayBufferSize, mixxx::SampleBuffer const& 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.

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

Copy link
Author

Choose a reason for hiding this comment

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

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

float differenceSum = 0.0f;
for (SINT i = 0; i < delayBufferSize;
i = i + 16) { // We don't need to consider *all* samples,
mrnicegyu11 marked this conversation as resolved.
Show resolved Hide resolved
// assuming 44.1kHz sample-rate this sums every
// ~1/3ms
differenceSum += fabsf(delay_buffer[i]);
}
return differenceSum;
mrnicegyu11 marked this conversation as resolved.
Show resolved Hide resolved
}

void EchoEffect::processChannel(
EchoGroupState* pGroupState,
const CSAMPLE* pInput,
Expand All @@ -127,10 +138,16 @@ void EchoEffect::processChannel(
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 +253,17 @@ void EchoEffect::processChannel(
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;
// Calculate if the delayline-buffer is approx. zero/empty.
const float avgSampleEnergy = averageSampleEnergy(
pGroupState->delay_buf.size(), pGroupState->delay_buf);
// If echo tail fully faded
if (avgSampleEnergy < 0.00001f) {
m_isReadyForDisable = true;
pGroupState->delay_buf.clear();
}
}

pGroupState->prev_feedback = feedback_current;
Expand Down
8 changes: 6 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,8 @@ class EchoEffect : public EffectProcessorImpl<EchoGroupState> {
EngineEffectParameterPointer m_pPingPongParameter;
EngineEffectParameterPointer m_pQuantizeParameter;
EngineEffectParameterPointer m_pTripletParameter;

bool m_isReadyForDisable = false;
DISALLOW_COPY_AND_ASSIGN(EchoEffect);
};

float averageSampleEnergy(SINT delayBufferSize, mixxx::SampleBuffer const& delay_buffer);
37 changes: 29 additions & 8 deletions src/effects/backends/builtin/reverbeffect.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,21 @@ 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) {
float differenceSum = 0.0f;
mrnicegyu11 marked this conversation as resolved.
Show resolved Hide resolved
for (SINT i = samplesPerBuffer - tailCheckLength;
i < samplesPerBuffer;
++i) {
differenceSum += fabsf(buffer_out[i] - buffer_in[i]);
mrnicegyu11 marked this conversation as resolved.
Show resolved Hide resolved
}
// Calculate average of the differences
const float averageDifference = differenceSum / tailCheckLength;
return averageDifference;
}

void ReverbEffect::processChannel(
ReverbGroupState* pState,
const CSAMPLE* pInput,
Expand All @@ -89,7 +104,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 +115,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 +127,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(void) {
mrnicegyu11 marked this conversation as resolved.
Show resolved Hide resolved
return true;
};
Swiftb0y marked this conversation as resolved.
Show resolved Hide resolved
};

/// 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