Skip to content

fix(audio): never feed chain processors blocks larger than prepared size - #85

Merged
byrongamatos merged 5 commits into
mainfrom
fix/nam-chain-blocksize-overrun
Jul 8, 2026
Merged

byrongamatos merged 5 commits into
mainfrom
fix/nam-chain-blocksize-overrun

Conversation

@OmikronApex

@OmikronApex OmikronApex commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Fixes the 'heavily distorted mic monitoring / garbled audio' tester reports (USB interface + Windows Audio shared/exclusive). Investigation notes in docs/audio-distortion-first-start-investigation.md; native song playback follow-up plan in docs/native-song-playback-plan.md.

Engine fixes

  1. Never feed chain processors blocks larger than the prepared size — the NAM core sizes its conv ring/output buffers to Reset()'s maxBufferSize with only a release-no-op assert guarding overruns; one oversized WASAPI block corrupted its ring state and garbled all audio until the next Reset(). NAMProcessor::processBlock now chunks, SignalChain::process slices oversized device blocks for every slot type.
  2. Stale-format raceaddProcessor/replaceProcessor prepare off the audio lock; a concurrent device reconfigure couldn't re-prepare the incoming processor. Format re-checked under the lock at insert/swap time.
  3. Input-callback double registration (exclusive-mode killer) — a transient audioDeviceStopped() during the slow exclusive open cleared audioRunning while the callback stayed attached; the next startAudio() registered it twice → every block processed and ring-pushed twice (half-speed, octave-down garble), stopAudio() left a live registration (engine wedged, exclusive device held open after app close). Input side now has the same guard the output callback already had.
  4. Audio-effects executor schema aligned with the rebranded capability layer + legacy alias (rebrand had split the pipeline three ways; every chain load fell back to legacy full-rebuild — see fix(audio-effects): accept pre-rebrand chain plan schema as alias feedBack#816 and fix(effects): rebranded plan schema id + stop poll/stop-handler rebuild ping-pong feedBack-plugin-rig-builder#52).

Also includes PR #82 (merged in: same-endpoint duplex routing + mono-input broadcast) and anomaly-only [diag] instrumentation (callback re-entrancy, oversized blocks, device lifecycle) behind --verbose.

Verification

  • Native unit tests 32/32 + sanitize suite.
  • Tester-verified on two machines: distortion gone in shared and exclusive mode, no rebuild storms, engine releases the device on close.
  • Remaining known issue (out of scope, planned): song audio is silent with exclusive output because song playback runs through the renderer — see docs/native-song-playback-plan.md Phase 0-4.

🤖 Generated with Claude Code

@coderabbitai

coderabbitai Bot commented Jul 8, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Updates duplex device selection, SignalChain preparation and block chunking, mono input broadcasting, and adds an investigation document describing the first-start distortion hypothesis and verification notes.

Changes

Audio startup and processing flow

Layer / File(s) Summary
Duplex device selection
src/audio/AudioEngine.cpp
Replaces legacy duplex intent handling with same-backend and same-endpoint checks in device probing and audio device setup, and returns duplex errors immediately when combined-device setup fails.
SignalChain prepare and processor preparation
src/audio/SignalChain.cpp
Moves format publication under lock, snapshots format before off-lock processor preparation, and re-prepares processors when device format changes during add or replace flows.
Chunked audio processing
src/audio/SignalChain.h, src/audio/NAMProcessor.cpp
Adds prepared-size chunking in SignalChain and NAMProcessor so oversized callback buffers are split before model processing, with MIDI delivered only on the first slice.
Mono input broadcast
src/audio/SourceChain.cpp
Updates SourceChain channel selection so explicit single-channel selection and mono input are broadcast across all effective output channels.
Distortion investigation document
docs/audio-distortion-first-start-investigation.md
Adds a markdown investigation note describing the distortion hypothesis, alternative suspects, remediation options, verification steps, and file references.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant AudioDevice
  participant AudioEngine
  participant SignalChain
  participant NAMProcessor
  participant NAMModel

  AudioDevice->>AudioEngine: setAudioDevices()
  AudioEngine->>AudioEngine: choose duplex or split path
  AudioEngine->>SignalChain: prepare()
  AudioDevice->>SignalChain: process(buffer, midi)
  alt buffer exceeds prepared block size
    loop chunk through currentBlockSize slices
      SignalChain->>NAMProcessor: processBlock(chunk)
      NAMProcessor->>NAMModel: process(slice)
    end
  else buffer fits prepared block size
    SignalChain->>NAMProcessor: processBlock(buffer)
    NAMProcessor->>NAMModel: process(buffer)
  end
Loading
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: preventing chain processors from receiving oversized blocks.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/nam-chain-blocksize-overrun

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/audio/NAMProcessor.cpp`:
- Around line 99-115: `processBlock()` now relies on `currentBlockSize` to cap
chunk size, but `prepareToPlay()` updates that value outside `modelLock`, so
another thread can see the new block size before `model->Reset()` has completed.
Move the `currentBlockSize` assignment into the same `modelLock`-protected
section in `prepareToPlay()`, alongside the `model->Reset()` call, so
`NAMProcessor::processBlock()` only reads a block size that matches the model
state.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 42743d31-24d4-4f0d-b5be-6a7367b974d5

📥 Commits

Reviewing files that changed from the base of the PR and between a53fd38 and 3b98e5a.

📒 Files selected for processing (4)
  • docs/audio-distortion-first-start-investigation.md
  • src/audio/NAMProcessor.cpp
  • src/audio/SignalChain.cpp
  • src/audio/SignalChain.h

Comment on lines +99 to +115
// Process through NAM model (double** in, double** out), in slices no larger
// than the block size the model was Reset() with. The NAM core pre-allocates
// its conv ring/output buffers to that maxBufferSize and only asserts (a
// release-build no-op) on larger blocks — one oversized block (WASAPI shared
// mode delivers them right after a device start) writes past those buffers
// and leaves the conv ring misaligned, garbling ALL subsequent audio until
// the next Reset().
const int maxChunk = currentBlockSize > 0 ? currentBlockSize : numSamples;
for (int offset = 0; offset < numSamples; offset += maxChunk)
{
const int chunk = juce::jmin(maxChunk, numSamples - offset);
double* inPtr = inputBuf.data() + offset;
double* outPtr = outputBuf.data() + offset;
double** inPtrs = &inPtr;
double** outPtrs = &outPtr;
model->process(inPtrs, outPtrs, chunk);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the relevant processor implementation and surrounding locking.
ast-grep outline src/audio/NAMProcessor.cpp --view expanded || true
sed -n '1,220p' src/audio/NAMProcessor.cpp | cat -n

# Find all uses of currentBlockSize/currentSampleRate/modelLock in this file.
rg -n "currentBlockSize|currentSampleRate|modelLock|prepareToPlay|processBlock|Reset\\(" src/audio/NAMProcessor.cpp

Repository: got-feedBack/feedBack-desktop

Length of output: 7458


🌐 Web query:

JUCE AudioProcessor prepareToPlay processBlock thread safety concurrent audio callback host contract documentation

💡 Result:

In JUCE, the interaction between AudioProcessor::prepareToPlay, processBlock, and the audio host involves critical thread-safety considerations. There is no universal host contract guaranteeing thread affinity; while processBlock is typically called on a high-priority real-time audio thread, behavior can vary by host [1][2]. Key threading and safety principles include: 1. Real-Time Constraints: The processBlock callback must be real-time safe [3]. You must never use locks (e.g., std::mutex), perform memory allocations (e.g., new, malloc), invoke system calls, or perform any operations that can block the audio thread, as these cause audio dropouts (pops, clicks) [4][3][5]. 2. Threading Context: While processBlock is generally called by a dedicated audio thread, some hosts may execute it on the message thread during offline operations (e.g., offline rendering/bouncing) [2]. prepareToPlay is often called from the message thread, but can also be invoked from the audio thread [6][7]. Consequently, you should not assume specific thread affinity for these methods [2]. 3. Data Sharing and Synchronization: Because the message thread (GUI) and audio thread run concurrently, you must use thread-safe techniques to share data [3]. - Use std::atomic for simple flags or values [3][5]. - Use lock-free structures, such as lock-free FIFOs (e.g., juce::AbstractFifo or juce::AudioProcessorValueTreeState), for passing complex data between threads [4][8]. - Avoid manual locking whenever possible to prevent priority inversion, where a low-priority thread (like the UI thread) holds a lock required by the high-priority audio thread [9]. 4. Defensive Programming: Hosts may provide variable-sized blocks that deviate from the maximumExpectedSamplesPerBlock passed to prepareToPlay [1][10]. Your processBlock implementation must be robust enough to handle varying block sizes without crashing or glitching [1][10]. 5. UI Interaction: Interaction with the UI from processBlock is prohibited [1]. Use asynchronous communication mechanisms like juce::ChangeBroadcaster, juce::AsyncUpdater, or thread-safe messaging to signal updates to the UI thread [1][4]. For complex state management, it is recommended to prepare or preallocate resources outside of the real-time path and use atomic pointer swaps or similar lock-free techniques to update the processing state while it is running [3]. Always assume the host may call prepareToPlay and processBlock in ways that require strict thread-safe state management [11].

Citations:


Move currentBlockSize under modelLock. processBlock() now depends on currentBlockSize, but prepareToPlay() writes it before acquiring the same lock used for model->Reset(). Hosts can call these callbacks from different threads, so processBlock() can observe the new block size before the model is reset to match, reintroducing the oversized-chunk corruption this change is meant to prevent. Set currentBlockSize while holding modelLock alongside the reset.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/audio/NAMProcessor.cpp` around lines 99 - 115, `processBlock()` now
relies on `currentBlockSize` to cap chunk size, but `prepareToPlay()` updates
that value outside `modelLock`, so another thread can see the new block size
before `model->Reset()` has completed. Move the `currentBlockSize` assignment
into the same `modelLock`-protected section in `prepareToPlay()`, alongside the
`model->Reset()` call, so `NAMProcessor::processBlock()` only reads a block size
that matches the model state.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
src/audio/SourceChain.cpp (1)

64-87: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Broadcast fix looks correct.

The removal of the numInputChannels >= 2 guard plus the new dedicated mono branch correctly handles both explicit single-channel picks and true mono devices, broadcasting to all effectiveOutputChannels per the documented SourceChain::processBlock contract (mono-in → all output channels).

Minor: the two branches (Lines 70-72 and 83-85) duplicate the identical "broadcast one source channel to all outputs, scaled by gain" loop, differing only in the source channel index (selectedCh vs 0). Consider factoring into a small local helper to avoid the duplication going forward.

♻️ Optional consolidation
+    auto broadcastChannel = [&](int srcCh)
+    {
+        for (int outCh = 0; outCh < effectiveOutputChannels; ++outCh)
+            for (int i = 0; i < numSamples; ++i)
+                buffer.setSample(outCh, i, inputData[srcCh][i] * inGain);
+    };
+
     if (selectedCh >= 0 && selectedCh < numInputChannels)
     {
-        for (int outCh = 0; outCh < effectiveOutputChannels; ++outCh)
-            for (int i = 0; i < numSamples; ++i)
-                buffer.setSample(outCh, i, inputData[selectedCh][i] * inGain);
+        broadcastChannel(selectedCh);
         filledOutputChannels = effectiveOutputChannels;
     }
     else if (numInputChannels == 1)
     {
-        for (int outCh = 0; outCh < effectiveOutputChannels; ++outCh)
-            for (int i = 0; i < numSamples; ++i)
-                buffer.setSample(outCh, i, inputData[0][i] * inGain);
+        broadcastChannel(0);
         filledOutputChannels = effectiveOutputChannels;
     }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/audio/SourceChain.cpp` around lines 64 - 87, The two broadcast branches
in SourceChain::processBlock duplicate the same nested loop that copies one
input channel to every effective output channel with gain applied. Refactor that
repeated logic into a small local helper or shared lambda within processBlock,
parameterized by the source channel index, and call it from both the selectedCh
path and the mono-input path so the broadcast behavior stays identical without
duplicated code.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/audio/AudioEngine.cpp`:
- Around line 266-267: The duplex detection in AudioEngine::setAudioDevices() /
the isDuplex check currently treats matching empty input and output names as the
same device, which incorrectly routes OS-default per-side WASAPI configs into
the duplex path. Update the condition so duplex is only selected when both names
are non-empty and equal, and keep empty names on the split-device path.

---

Nitpick comments:
In `@src/audio/SourceChain.cpp`:
- Around line 64-87: The two broadcast branches in SourceChain::processBlock
duplicate the same nested loop that copies one input channel to every effective
output channel with gain applied. Refactor that repeated logic into a small
local helper or shared lambda within processBlock, parameterized by the source
channel index, and call it from both the selectedCh path and the mono-input path
so the broadcast behavior stays identical without duplicated code.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 790f84bf-5e98-4887-90e9-830d786d551e

📥 Commits

Reviewing files that changed from the base of the PR and between 3b98e5a and eeca0c0.

📒 Files selected for processing (2)
  • src/audio/AudioEngine.cpp
  • src/audio/SourceChain.cpp

Comment thread src/audio/AudioEngine.cpp
Comment on lines +266 to +267
bool isDuplex = (options.inputType == options.outputType)
&& (options.input == options.output);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Where setAudioDevices / probeDeviceOptionsDual are invoked and with what names
rg -nP 'setAudioDevices|probeDeviceOptionsDual' src --type cpp -C2
# applyDuplexSetup handling of empty names (useDefault* / default endpoint selection)
ast-grep outline src/audio/AudioEngine.cpp --match applyDuplexSetup --view expanded

Repository: got-feedBack/feedBack-desktop

Length of output: 8465


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the exact logic around the probe, apply, and duplex branches.
sed -n '240,290p' src/audio/AudioEngine.cpp
printf '\n---\n'
sed -n '670,760p' src/audio/AudioEngine.cpp
printf '\n---\n'
sed -n '930,1010p' src/audio/AudioEngine.cpp

Repository: got-feedBack/feedBack-desktop

Length of output: 10530


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Map applyDuplexSetup and any helper it calls.
ast-grep outline src/audio/AudioEngine.cpp --match applyDuplexSetup --view expanded

Repository: got-feedBack/feedBack-desktop

Length of output: 272


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Read applyDuplexSetup and its empty-name handling.
sed -n '760,860p' src/audio/AudioEngine.cpp

Repository: got-feedBack/feedBack-desktop

Length of output: 4470


Treat empty names as split, not duplex. Empty input/output names mean “OS default per side,” not the same physical endpoint. This == check makes the probe and setAudioDevices() pick duplex for default-in/default-out WASAPI configs even though they’re usually separate clocks. Require non-empty, equal names before taking the duplex branch.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/audio/AudioEngine.cpp` around lines 266 - 267, The duplex detection in
AudioEngine::setAudioDevices() / the isDuplex check currently treats matching
empty input and output names as the same device, which incorrectly routes
OS-default per-side WASAPI configs into the duplex path. Update the condition so
duplex is only selected when both names are non-empty and equal, and keep empty
names on the split-device path.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/audio/SignalChain.cpp`:
- Around line 463-468: The playback-format snapshot in SignalChain::prepare() is
being read from currentSampleRate and currentBlockSize without holding the same
mutex used by the publisher, so those values should be captured under lock
before invoking the plugin. Update the prepare() path in SignalChain.cpp to take
a brief locked read of the two members, store them in local variables, and then
pass those locals into prepareForPlayback via invokePlugin so the snapshot is
consistent with the synchronized write path.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 95faaffa-6877-47f8-a761-11767ad457d4

📥 Commits

Reviewing files that changed from the base of the PR and between eeca0c0 and ca8b0c9.

📒 Files selected for processing (1)
  • src/audio/SignalChain.cpp

Comment thread src/audio/SignalChain.cpp
OmikronApex and others added 5 commits July 8, 2026 22:25
WASAPI shared mode can deliver oversized blocks right after a device
start. The NAM core pre-allocates its conv ring/output buffers to the
Reset() maxBufferSize and only asserts (release no-op) on larger blocks;
one oversized block corrupts the conv ring state and garbles all
subsequent audio until the next Reset() — the 'first start heavily
distorted until tone reset / engine restart' bug.

- NAMProcessor::processBlock: process in slices of at most the prepared
  block size.
- SignalChain::process: slice oversized device blocks into prepared-size
  chunks before any slot (VST/NAM/IR) sees them.

See docs/audio-distortion-first-start-investigation.md.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two issues found while testing the USB-guitar-cable path on Windows:

1. Centre a mono input. SourceChain::processBlock fell into the
   pass-through branch for a 1-channel input, filling only
   min(inputChannels, outputChannels) = 1 output channel and zeroing the
   rest, so a mono USB guitar cable played out of the left speaker only.
   A single-channel input is now broadcast across every output channel.

2. Only attempt the combined (duplex) device when input and output are
   the SAME physical endpoint. Two different endpoints of the same
   backend (USB cable in + separate speakers out) are independent
   hardware clocks; routing them through one duplex device was unstable
   across the app lifecycle (no audio until an explicit Apply, then
   distortion / dropouts / silent-in-song on navigation). Different
   endpoints now use the split path, whose ring bridges the two clocks.
   Same-endpoint duplex (one interface for in and out) keeps the
   low-latency win. Low latency for the two-device case is a follow-up
   that needs the device-lifecycle work (startup restore + reconfigure
   on navigation).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JEoFeTPSnz4NpwwCG52hnu
Startup auto-apply (renderer init) fail-closes on probeDeviceOptionsDual's
`compatible` verdict, but the probe still measured a COMBINED duplex device
for any same-backend pair while setAudioDevices now opens split for
different endpoints. That mismatch made the startup probe describe a config
that isn't the one applied — surfacing as "no audio until I press Apply" for
a USB cable + separate speakers. Gate the probe's duplex path on the same
sameEndpointIntent (same type AND same device) the apply path uses, so a
two-device pair is probed via the split path it will actually run on.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JEoFeTPSnz4NpwwCG52hnu
…onfigure

addProcessor/replaceProcessor prepare the incoming processor off the
audio lock on N-API worker threads. A concurrent device reconfigure's
SignalChain::prepare() can't see that processor (not slotted yet), so a
slot could go live prepared at a stale sample rate / block size and stay
wrong until the next device restart — heard as pitch-shifted/garbled
monitoring when a chain loads while the device is being (re)opened
(widest window: WASAPI exclusive mode's slower open).

Re-check the chain's current format under the lock at insert/swap time
and re-prepare if it moved; log the transition to stderr so tester logs
show when the race fired. prepare() now publishes the format under the
lock so the check can't tear.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@byrongamatos
byrongamatos force-pushed the fix/nam-chain-blocksize-overrun branch from ca8b0c9 to fc39e42 Compare July 8, 2026 20:29

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/audio/SignalChain.cpp (1)

509-523: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Snapshot replace format while already holding lock.

currentSampleRate / currentBlockSize are written under prepare()’s lock, but Lines 522-523 read them unlocked. Capture them in the existing locked block that copies the staging identity.

Proposed fix
     ProcessorSlot staging;
+    double prepSr = 0.0;
+    int prepBs = 0;
     {
         const juce::ScopedLock sl(lock);
         const int idx = findSlotIndex(slotId);
         if (idx < 0) return false;           // nothing to replace
         staging.type = slots[idx]->type;
         staging.name = slots[idx]->name;
         staging.path = slots[idx]->path;
+        prepSr = currentSampleRate;
+        prepBs = currentBlockSize;
     }
@@
     // contained (the processor is dropped) rather than taking the app down.
     staging.processor = std::move(processor);
-    const double prepSr = currentSampleRate;
-    const int prepBs = currentBlockSize;
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/audio/SignalChain.cpp` around lines 509 - 523, The snapshot replace path
in SignalChain::prepare() reads currentSampleRate and currentBlockSize after
releasing lock, even though those values are written under the same mutex. Move
the sampling of both fields into the existing juce::ScopedLock block where
staging.type/name/path are copied, so the replacement snapshot is fully captured
while holding lock and then used afterward without unlocked access.
♻️ Duplicate comments (1)
src/audio/SignalChain.cpp (1)

463-464: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Take the add-processor format snapshot under lock.

This still reads plain members concurrently with prepare()’s locked write. The later locked stale-format check does not make the initial snapshot race-free.

Proposed fix
-    const double prepSr = currentSampleRate;
-    const int prepBs = currentBlockSize;
+    double prepSr = 0.0;
+    int prepBs = 0;
+    {
+        const juce::ScopedLock sl(lock);
+        prepSr = currentSampleRate;
+        prepBs = currentBlockSize;
+    }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/audio/SignalChain.cpp` around lines 463 - 464, The add-processor format
snapshot is taken outside the mutex, so it can race with prepare() updating the
same members. Move the initial reads of currentSampleRate and currentBlockSize
in SignalChain::addProcessor under the same lock used by the rest of the format
handling, so the snapshot and later stale-format check are both based on a
consistent state.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/audio/SignalChain.cpp`:
- Around line 296-297: The MIDI handling in SignalChain::process() is draining
queued MIDI on every oversized slice, which can let pending events be consumed
by later slices in the same callback. Update processLocked() (or its caller in
the slice loop) so midiQueueFifo is drained only for the first slice when offset
== 0, and pass empty MIDI for subsequent slices to preserve the intended
sample-0 behavior.

---

Outside diff comments:
In `@src/audio/SignalChain.cpp`:
- Around line 509-523: The snapshot replace path in SignalChain::prepare() reads
currentSampleRate and currentBlockSize after releasing lock, even though those
values are written under the same mutex. Move the sampling of both fields into
the existing juce::ScopedLock block where staging.type/name/path are copied, so
the replacement snapshot is fully captured while holding lock and then used
afterward without unlocked access.

---

Duplicate comments:
In `@src/audio/SignalChain.cpp`:
- Around line 463-464: The add-processor format snapshot is taken outside the
mutex, so it can race with prepare() updating the same members. Move the initial
reads of currentSampleRate and currentBlockSize in SignalChain::addProcessor
under the same lock used by the rest of the format handling, so the snapshot and
later stale-format check are both based on a consistent state.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 82c1ecee-517b-4b4a-92ca-91c888137c10

📥 Commits

Reviewing files that changed from the base of the PR and between ca8b0c9 and fc39e42.

📒 Files selected for processing (6)
  • docs/audio-distortion-first-start-investigation.md
  • src/audio/AudioEngine.cpp
  • src/audio/NAMProcessor.cpp
  • src/audio/SignalChain.cpp
  • src/audio/SignalChain.h
  • src/audio/SourceChain.cpp
🚧 Files skipped from review as they are similar to previous changes (4)
  • src/audio/NAMProcessor.cpp
  • src/audio/SignalChain.h
  • src/audio/SourceChain.cpp
  • src/audio/AudioEngine.cpp

Comment thread src/audio/SignalChain.cpp
Comment on lines +296 to +297
// MIDI (all stamped at sample 0) goes to the first slice only.
processLocked(slice, offset == 0 ? midi : emptyMidi);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Drain queued MIDI only once per oversized device block.

processLocked() drains midiQueueFifo on every slice, so queued MIDI can be consumed on a later slice of the same oversized callback even though this path intends sample-0 MIDI to hit only the first slice. Consider making pending-MIDI draining explicit and enabled only for the first slice.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/audio/SignalChain.cpp` around lines 296 - 297, The MIDI handling in
SignalChain::process() is draining queued MIDI on every oversized slice, which
can let pending events be consumed by later slices in the same callback. Update
processLocked() (or its caller in the slice loop) so midiQueueFifo is drained
only for the first slice when offset == 0, and pass empty MIDI for subsequent
slices to preserve the intended sample-0 behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants