From 61a7ffc29aa72551b77055daa7bfbc7c51072516 Mon Sep 17 00:00:00 2001 From: Yves Vogl Date: Thu, 16 Jul 2026 02:03:32 +0200 Subject: [PATCH 1/2] fix(dsp): reset convolutionB on Blend's disengaged->engaged transition juce::dsp::Convolution keeps an internal overlap-add tail that is only ever mutated inside processSamples(); process() already skips convolutionB.process() entirely while Blend is disengaged, so that tail was left frozen (not decaying) and got added back into the output the moment Blend re-engaged, unlike the LoCut/HiCut/Distance filters which already reset on their own bypassed->engaged transition. CabConvolutionEngine now tracks blendEngagedPreviously and calls convolutionB.reset() on the same transition, mirroring the existing pattern for the other stages. Fixes #12 Co-Authored-By: Claude Fable 5 --- CHANGELOG.md | 4 ++ src/dsp/CabConvolutionEngine.cpp | 16 +++++++ src/dsp/CabConvolutionEngine.h | 12 +++++- tests/EngineTests.cpp | 72 ++++++++++++++++++++++++++++++++ 4 files changed, 102 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2ad5f36..e829e49 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- **`convolutionB` not reset on Blend's disengaged->engaged transition** (#12): unlike LoCut/HiCut/Distance, IR B's convolution engine kept no history of its own bypass state, so its internal overlap-add tail could go stale (frozen, not decaying) while Blend was disengaged and then leak into the output the moment Blend re-engaged. `CabConvolutionEngine` now tracks `blendEngagedPreviously` and calls `convolutionB.reset()` on the same disengaged->engaged transition the other stages already handle. + ## [0.1.0] - 2026-07-14 ### Added diff --git a/src/dsp/CabConvolutionEngine.cpp b/src/dsp/CabConvolutionEngine.cpp index 82cc2a8..458f3c3 100644 --- a/src/dsp/CabConvolutionEngine.cpp +++ b/src/dsp/CabConvolutionEngine.cpp @@ -129,6 +129,7 @@ void CabConvolutionEngine::prepare (const juce::dsp::ProcessSpec& spec) loCutEngagedPreviously = lastLoCutHz > loCutMinHz + bypassEpsilonHz; hiCutEngagedPreviously = lastHiCutHz < hiCutMaxHz - bypassEpsilonHz; distanceEngagedPreviously = lastDistancePercent > distanceMinPercent + distanceBypassEpsilonPercent; + blendEngagedPreviously = lastBlendProportion > blendBypassEpsilon; } void CabConvolutionEngine::reset() @@ -308,8 +309,23 @@ void CabConvolutionEngine::process (juce::dsp::AudioBlock& block) distanceHighShelfFilter.reset(); } + // Same idea for convolutionB: it keeps no history of its own bypass + // state, so without this it's the one exception in this function that + // never gets reset on a disengaged->engaged transition (see #12). + // convolutionB.process() is skipped entirely for every block Blend is + // disengaged (below), which freezes its internal overlap-add buffer + // rather than decaying it - left unreset, that stale, time-decoupled + // tail would be added back into the output the moment Blend re-engages. + // juce::dsp::Convolution::reset() is documented noexcept/real-time safe + // (JUCE 8.0.14 juce_Convolution.h) and this engine already calls it from + // the audio thread via CabConvolutionEngine::reset(), so this is safe + // here too. + if (blendEngaged && ! blendEngagedPreviously) + convolutionB.reset(); + loCutEngagedPreviously = ! loCutBypassed; hiCutEngagedPreviously = ! hiCutBypassed; + blendEngagedPreviously = blendEngaged; distanceEngagedPreviously = ! distanceBypassed; if (! loCutBypassed) diff --git a/src/dsp/CabConvolutionEngine.h b/src/dsp/CabConvolutionEngine.h index 02e9232..49dad97 100644 --- a/src/dsp/CabConvolutionEngine.h +++ b/src/dsp/CabConvolutionEngine.h @@ -216,11 +216,19 @@ class CabConvolutionEngine bool anyImpulseResponseBLoaded = false; // Previous block's engaged (i.e. not bypassed) state for LoCut/HiCut/ - // Distance, used to detect bypassed->engaged transitions so the - // filter(s) can be reset to a clean state exactly then (see process()). + // Distance/Blend, used to detect bypassed->engaged transitions so the + // filter(s)/convolution engine can be reset to a clean state exactly + // then (see process()). Blend's counterpart is convolutionB: unlike + // LoCut/HiCut/Distance's IIR filters, convolutionB is a stateful + // juce::dsp::Convolution whose internal overlap-add buffer keeps + // accumulating output for future blocks even after the engine stops + // calling its process() (see the Blend disengaged-branch below) - left + // unreset, that stale, time-decoupled tail gets added back into the + // output on re-engagement (see #12). bool loCutEngagedPreviously = false; bool hiCutEngagedPreviously = false; bool distanceEngagedPreviously = false; + bool blendEngagedPreviously = false; // IR A's most recently loaded onset sample/rate, recorded by // setImpulseResponse()/loadDefaultImpulseResponse() and used as the diff --git a/tests/EngineTests.cpp b/tests/EngineTests.cpp index 348a5fd..2602123 100644 --- a/tests/EngineTests.cpp +++ b/tests/EngineTests.cpp @@ -486,6 +486,78 @@ TEST_CASE ("IR Blend with two distinct, non-identity IRs in both slots blends th } } +TEST_CASE ("convolutionB is reset on Blend's disengaged->engaged transition, not left carrying a stale overlap tail", + "[dsp][engine][blend]") +{ + // Regression coverage for #12. juce::dsp::Convolution's zero-latency + // uniformly-partitioned algorithm carries an internal overlap-add tail + // ("bufferOverlap"/"overlapData" in JUCE 8.0.14's juce_Convolution.cpp) + // that is only ever mutated inside processSamples() - process() below + // skips convolutionB.process() entirely for every block Blend is + // disengaged, so that tail is frozen (not decaying) for as long as + // Blend stays disengaged. + // + // Each block here is a single full-size (testBlockSize) call rather than + // many small ones deliberately: testBlockSize's duration (~170 ms at + // 48 kHz) comfortably exceeds the engine's internal parameter-smoothing + // ramp (50 ms, see CabConvolutionEngine::smoothingTimeSeconds), so + // blendSmoothed.skip() settles fully to its new target *within* the very + // next block - Blend's disengaged/engaged state is then clean and + // block-atomic, with no partially-ramped blocks in between to muddy + // which state was actually in effect when. + constexpr int irLength = 512; // several times shorter than testBlockSize, + // so its full ring-down tail fits inside + // one process() call's overlap-add state + + CabConvolutionEngine engine; + engine.setMixProportion (1.0f); + engine.setLevelDb (0.0f); + engine.setBlendProportion (1.0f); // engaged from the start + + const auto spec = makeTestSpec (2); + engine.prepare (spec); + + // A long, high-energy, slowly-decaying IR B, so any leaked overlap tail + // is unmistakable rather than lost in floating-point noise. + juce::AudioBuffer irB (1, irLength); + for (int i = 0; i < irLength; ++i) + irB.setSample (0, i, 0.8f * std::pow (0.995f, static_cast (i))); + + engine.setImpulseResponseB (std::move (irB), testSampleRate); + engine.prepare (spec); // guarantee the async load is drained/active + + juce::AudioBuffer buffer (2, testBlockSize); + + // One loud, engaged block: builds real, non-trivial state in + // convolutionB's internal overlap-add buffer (its ring-down tail + // extends past this block's end, into what would be the next call). + TestHelpers::fillWithSine (buffer, testSampleRate, testFrequencyHz, 0.9f); + juce::dsp::AudioBlock engagedBlock (buffer); + engine.process (engagedBlock); + CHECK (TestHelpers::allSamplesFinite (buffer)); + + // Disengage Blend for one full block. convolutionB.process() is skipped + // entirely for it, so its internal state - including that ring-down + // tail - is frozen exactly where the block above left it, not decayed. + engine.setBlendProportion (0.0f); + buffer.clear(); + juce::dsp::AudioBlock disengagedBlock (buffer); + engine.process (disengagedBlock); + + // Re-engage Blend and feed pure silence. With convolutionB correctly + // reset on the disengaged->engaged transition, silence in must produce + // silence out. Without the reset (the bug), the frozen overlap-add tail + // from the loud block above gets added back into this block's output. + engine.setBlendProportion (1.0f); + buffer.clear(); + + juce::dsp::AudioBlock reengagedBlock (buffer); + engine.process (reengagedBlock); + + CHECK (TestHelpers::allSamplesFinite (buffer)); + CHECK (TestHelpers::peakAbsolute (buffer) < nullTestTolerance); +} + TEST_CASE ("Distance at 0% (default) is a bit-exact passthrough", "[dsp][engine][distance]") { CabConvolutionEngine engine; From d09162a07d0e98761e91c3f2c38e8ea8ec8173dc Mon Sep 17 00:00:00 2001 From: Yves Vogl Date: Thu, 16 Jul 2026 02:05:32 +0200 Subject: [PATCH 2/2] fix(dsp): re-align IR B when IR A is reloaded setImpulseResponse()/loadDefaultImpulseResponse() record IR A's onset as the reference that setImpulseResponseB() phase-aligns IR B against, but never re-ran that alignment for an already-loaded IR B when IR A was reloaded with a different onset - leaving IR B silently aligned to a stale, overwritten reference and reintroducing comb-filtering the next time Blend crosses back into an engaged range. CabConvolutionEngine now retains a copy of IR B's raw, pre-alignment buffer/sample rate and automatically re-runs setImpulseResponseB() against the new reference whenever IR A's onset changes (both from a loaded IR and from the default-delta reset path), clearing that bookkeeping again if IR B is later reset to its own default. Fixes #13 Co-Authored-By: Claude Fable 5 --- CHANGELOG.md | 1 + src/dsp/CabConvolutionEngine.cpp | 30 +++++++++++ src/dsp/CabConvolutionEngine.h | 29 +++++++++-- tests/EngineTests.cpp | 85 ++++++++++++++++++++++++++++++++ 4 files changed, 141 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e829e49..73e33f7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - **`convolutionB` not reset on Blend's disengaged->engaged transition** (#12): unlike LoCut/HiCut/Distance, IR B's convolution engine kept no history of its own bypass state, so its internal overlap-add tail could go stale (frozen, not decaying) while Blend was disengaged and then leak into the output the moment Blend re-engaged. `CabConvolutionEngine` now tracks `blendEngagedPreviously` and calls `convolutionB.reset()` on the same disengaged->engaged transition the other stages already handle. +- **Reloading IR A after IR B silently invalidated IR B's phase alignment** (#13): `setImpulseResponse()`/`loadDefaultImpulseResponse()` recorded IR A's new onset as the phase-alignment reference but never re-ran IR B's alignment against it, leaving an already-loaded IR B aligned to a stale, overwritten onset (reintroducing comb-filtering on the next Blend crossfade). `CabConvolutionEngine` now retains a copy of IR B's raw, pre-alignment buffer and automatically re-aligns it whenever IR A's reference onset changes. ## [0.1.0] - 2026-07-14 diff --git a/src/dsp/CabConvolutionEngine.cpp b/src/dsp/CabConvolutionEngine.cpp index 458f3c3..731dfc9 100644 --- a/src/dsp/CabConvolutionEngine.cpp +++ b/src/dsp/CabConvolutionEngine.cpp @@ -202,6 +202,15 @@ void CabConvolutionEngine::setImpulseResponse (juce::AudioBuffer irBuffer juce::dsp::Convolution::Normalise::yes); anyImpulseResponseLoaded = true; + + // IR A's onset (the alignment reference recorded above) just changed. If + // a real IR B is already loaded, its alignment was computed against the + // *previous* reference and is now stale - silently reintroducing + // comb-filtering the next time Blend crosses back into an engaged range + // (see #13). Re-run alignment from IR B's retained raw buffer against + // the new reference so it never goes stale like this. + if (irBNeedsAlignment) + setImpulseResponseB (lastIrBRawBuffer, lastIrBRawSampleRate); } void CabConvolutionEngine::loadDefaultImpulseResponse() @@ -222,10 +231,25 @@ void CabConvolutionEngine::loadDefaultImpulseResponse() juce::dsp::Convolution::Normalise::no); anyImpulseResponseLoaded = true; + + // Same rationale as the end of setImpulseResponse() above: this changed + // the alignment reference, so an already-loaded real IR B must be + // re-aligned against it rather than left pointing at a stale onset (#13). + if (irBNeedsAlignment) + setImpulseResponseB (lastIrBRawBuffer, lastIrBRawSampleRate); } void CabConvolutionEngine::setImpulseResponseB (juce::AudioBuffer irBuffer, double irSampleRate) { + // Retain a copy of the raw, pre-alignment buffer/rate so a later IR A + // reload can re-run this alignment on its own (see setImpulseResponse()/ + // loadDefaultImpulseResponse() above and #13), without requiring the + // caller to reload IR B. Copied (not moved) - `irBuffer` below still + // needs its original content for the alignment call that follows. + lastIrBRawBuffer.makeCopyOf (irBuffer); + lastIrBRawSampleRate = irSampleRate; + irBNeedsAlignment = true; + // Inter-IR phase alignment: shift IR B's onset to match IR A's most // recently recorded onset, so blending the two convolution outputs // doesn't introduce comb-filtering from a timing mismatch between their @@ -255,6 +279,12 @@ void CabConvolutionEngine::loadDefaultImpulseResponseB() juce::dsp::Convolution::Normalise::no); anyImpulseResponseBLoaded = true; + + // The default delta IR has no meaningful onset to keep aligned - clear + // the retained-raw-buffer bookkeeping so a subsequent IR A reload + // doesn't try to re-align it (see setImpulseResponse() above and #13). + irBNeedsAlignment = false; + lastIrBRawBuffer.setSize (0, 0); } void CabConvolutionEngine::process (juce::dsp::AudioBlock& block) diff --git a/src/dsp/CabConvolutionEngine.h b/src/dsp/CabConvolutionEngine.h index 49dad97..0a68197 100644 --- a/src/dsp/CabConvolutionEngine.h +++ b/src/dsp/CabConvolutionEngine.h @@ -104,14 +104,19 @@ class CabConvolutionEngine // (Convolution takes ownership to avoid an audio-thread copy). Also // records this IR's onset sample/rate as the reference that a // subsequently loaded IR B is phase-aligned against (see - // setImpulseResponseB()). + // setImpulseResponseB()). If a real (non-default) IR B is already + // loaded, its alignment is re-run against this new reference before + // returning, so a later A reload never leaves IR B silently aligned to a + // stale onset (see #13). void setImpulseResponse (juce::AudioBuffer irBuffer, double irSampleRate); // Resets slot A to the default unit-impulse (delta) IR - a mathematical // passthrough. Used both for the plugin's out-of-the-box default and to // let the user explicitly clear a loaded IR. Same off-audio-thread // contract as setImpulseResponse(). Also resets the phase-alignment - // reference back to the delta IR's trivial (sample 0) onset. + // reference back to the delta IR's trivial (sample 0) onset, and + // (like setImpulseResponse() above) re-aligns an already-loaded IR B + // against it (see #13). void loadDefaultImpulseResponse(); // Loads a new impulse response into slot B (the secondary IR used for IR @@ -119,11 +124,15 @@ class CabConvolutionEngine // loading, `irBuffer` is time-shifted ("inter-IR phase alignment", see // IrAlignment.h) so its detected onset lines up with slot A's most // recently recorded onset - this prevents comb-filtering when Blend - // crossfades the two convolution outputs together. + // crossfades the two convolution outputs together. A copy of the raw, + // pre-alignment buffer is retained so a later IR A reload can re-run + // this alignment automatically (see setImpulseResponse() and #13). void setImpulseResponseB (juce::AudioBuffer irBuffer, double irSampleRate); // Resets slot B to the default unit-impulse (delta) IR. Same - // off-audio-thread contract as loadDefaultImpulseResponse(). + // off-audio-thread contract as loadDefaultImpulseResponse(). Also clears + // the retained raw-IR-B/alignment bookkeeping above, since the default + // delta IR is never re-aligned on a subsequent IR A reload. void loadDefaultImpulseResponseB(); // Convolution engine latency in samples, valid after prepare() has run. @@ -237,5 +246,17 @@ class CabConvolutionEngine int lastIrAOnsetSample = 0; double lastIrASampleRate = 44100.0; + // A copy of IR B's raw, pre-alignment buffer/sample rate, retained so + // that a later reload of IR A (which changes the alignment reference + // above) can re-run alignment for the *already-loaded* IR B without + // requiring the caller to reload it - see setImpulseResponse()/ + // loadDefaultImpulseResponse() and #13. irBNeedsAlignment is true once a + // real (non-default) IR B has been loaded via setImpulseResponseB(); + // loadDefaultImpulseResponseB() clears it, since the default delta IR + // has no meaningful onset to keep aligned. + juce::AudioBuffer lastIrBRawBuffer; + double lastIrBRawSampleRate = 44100.0; + bool irBNeedsAlignment = false; + JUCE_DECLARE_NON_COPYABLE_WITH_LEAK_DETECTOR (CabConvolutionEngine) }; diff --git a/tests/EngineTests.cpp b/tests/EngineTests.cpp index 2602123..94d4cb2 100644 --- a/tests/EngineTests.cpp +++ b/tests/EngineTests.cpp @@ -558,6 +558,91 @@ TEST_CASE ("convolutionB is reset on Blend's disengaged->engaged transition, not CHECK (TestHelpers::peakAbsolute (buffer) < nullTestTolerance); } +TEST_CASE ("Reloading IR A after IR B re-aligns IR B against the new reference, not the stale one", + "[dsp][engine][blend][ir-alignment]") +{ + // Regression coverage for #13. setImpulseResponseB() aligns IR B + // against whatever IR A onset is current *at the moment it's called*; + // reloading IR A afterwards must re-run that alignment against the new + // reference, or IR B silently stays aligned to the stale, overwritten + // onset. Compares an engine that reloads IR A after IR B (the buggy + // sequence) against a reference engine that loads IR A with the same + // final onset *before* IR B is ever loaded (so IR B's alignment there + // is, by construction, never stale) - a correct implementation must + // make both produce the same IR-B-driven output. + constexpr int totalSamples = 96; + + const auto makeTransientAt = [] (int onsetSample) + { + juce::AudioBuffer buffer (1, totalSamples); + buffer.clear(); + + for (int i = onsetSample; i < totalSamples; ++i) + buffer.setSample (0, i, std::pow (0.7f, static_cast (i - onsetSample))); + + return buffer; + }; + + const auto renderIrBOnly = [&] (const std::function& loadSequence) + { + CabConvolutionEngine engine; + engine.setMixProportion (1.0f); + engine.setLevelDb (0.0f); + engine.setBlendProportion (1.0f); // IR B only, so IR A's final content is irrelevant to the output + + const auto spec = makeTestSpec (2); + engine.prepare (spec); + + loadSequence (engine, spec); + + juce::AudioBuffer impulse (2, totalSamples); + impulse.clear(); + impulse.setSample (0, 0, 1.0f); + impulse.setSample (1, 0, 1.0f); + + juce::dsp::AudioBlock block (impulse); + engine.process (block); + + REQUIRE (TestHelpers::allSamplesFinite (impulse)); + return impulse; + }; + + // Reference: IR A goes straight to its final onset (10) before IR B is + // ever loaded - IR B's alignment here can never be stale. + const auto expected = renderIrBOnly ([&] (CabConvolutionEngine& engine, const juce::dsp::ProcessSpec& spec) + { + engine.setImpulseResponse (makeTransientAt (10), testSampleRate); + engine.prepare (spec); + engine.setImpulseResponseB (makeTransientAt (30), testSampleRate); + engine.prepare (spec); + }); + + // Actual: IR A first loaded with onset 0 (so IR B aligns against that), + // then reloaded with onset 10 - the sequence #13 describes. + const auto actual = renderIrBOnly ([&] (CabConvolutionEngine& engine, const juce::dsp::ProcessSpec& spec) + { + engine.setImpulseResponse (makeTransientAt (0), testSampleRate); + engine.prepare (spec); + engine.setImpulseResponseB (makeTransientAt (30), testSampleRate); + engine.prepare (spec); + engine.setImpulseResponse (makeTransientAt (10), testSampleRate); + engine.prepare (spec); + }); + + for (int channel = 0; channel < expected.getNumChannels(); ++channel) + { + const auto* expectedData = expected.getReadPointer (channel); + const auto* actualData = actual.getReadPointer (channel); + + float maxResidual = 0.0f; + + for (int i = 0; i < totalSamples; ++i) + maxResidual = std::max (maxResidual, std::abs (actualData[i] - expectedData[i])); + + CHECK (maxResidual < nullTestTolerance); + } +} + TEST_CASE ("Distance at 0% (default) is a bit-exact passthrough", "[dsp][engine][distance]") { CabConvolutionEngine engine;