fix: resolve review findings (audio-thread safety, robustness)#18
Merged
Conversation
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
convolutionB's internal overlap-add tail was never reset on Blend's disengaged->engaged transition (unlike LoCut/HiCut/Distance), so a frozen, time-decoupled tail could leak into the output the moment Blend re-engaged.CabConvolutionEnginenow tracksblendEngagedPreviouslyand callsconvolutionB.reset()on that transition, mirroring the existing pattern.CabConvolutionEnginenow retains IR B's raw pre-alignment buffer and automatically re-runs alignment whenever IR A's reference onset changes.Both fixes were already present as an uncommitted working-tree change at the start of this session; this PR verifies them against JUCE 8.0.14 source (
juce_Convolution.cpp/.h, confirmingreset()isnoexcept/RT-safe and the citedoverlapData/bufferOverlapbehaviour), adds regression tests for each, and splits the change into one commit per issue.Test plan
tests/EngineTests.cpp, one per issue; both independently verified to fail against the pre-fix code and pass with the fix (viagit stashbisection during development).cmake --build build --target Tests Nave_Standalone --parallel 3— clean build, no new warnings from changed files.ctest --test-dir build --output-on-failure— 53/53 tests green (51 pre-existing + 2 new).CHANGELOG.mdupdated under## [Unreleased]with aFixedsection for both issues.🤖 Generated with Claude Code