fix: resolve review findings (audio-thread safety, robustness)

CHANGELOG.md

@@ -7,6 +7,11 @@ 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.
+- **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
 
 ### Added

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()
@@ -201,6 +202,15 @@ void CabConvolutionEngine::setImpulseResponse (juce::AudioBuffer<float> 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()
@@ -221,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<float> 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
@@ -254,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<float>& block)
@@ -308,8 +339,23 @@ void CabConvolutionEngine::process (juce::dsp::AudioBlock<float>& 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)

src/dsp/CabConvolutionEngine.h

@@ -104,26 +104,35 @@ 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<float> 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
     // Blend). Same off-audio-thread contract as setImpulseResponse(). Before
     // 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<float> 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.
@@ -216,11 +225,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
@@ -229,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<float> lastIrBRawBuffer;
+    double lastIrBRawSampleRate = 44100.0;
+    bool irBNeedsAlignment = false;
+
     JUCE_DECLARE_NON_COPYABLE_WITH_LEAK_DETECTOR (CabConvolutionEngine)
 };

tests/EngineTests.cpp

@@ -486,6 +486,163 @@ 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<float> irB (1, irLength);
+    for (int i = 0; i < irLength; ++i)
+        irB.setSample (0, i, 0.8f * std::pow (0.995f, static_cast<float> (i)));
+
+    engine.setImpulseResponseB (std::move (irB), testSampleRate);
+    engine.prepare (spec); // guarantee the async load is drained/active
+
+    juce::AudioBuffer<float> 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<float> 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<float> 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<float> reengagedBlock (buffer);
+    engine.process (reengagedBlock);
+
+    CHECK (TestHelpers::allSamplesFinite (buffer));
+    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<float> buffer (1, totalSamples);
+        buffer.clear();
+
+        for (int i = onsetSample; i < totalSamples; ++i)
+            buffer.setSample (0, i, std::pow (0.7f, static_cast<float> (i - onsetSample)));
+
+        return buffer;
+    };
+
+    const auto renderIrBOnly = [&] (const std::function<void (CabConvolutionEngine&, const juce::dsp::ProcessSpec&)>& 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<float> impulse (2, totalSamples);
+        impulse.clear();
+        impulse.setSample (0, 0, 1.0f);
+        impulse.setSample (1, 0, 1.0f);
+
+        juce::dsp::AudioBlock<float> 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;