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

* fix(audio): never feed chain processors blocks larger than prepared size 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> * fix(audio): prefer same-backend duplex routing * fix(audio): centre mono input; limit duplex to same-endpoint devices 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 * fix(audio): probe same-endpoint duplex the same way apply routes it 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 * fix(audio): close stale-format race when processors are added mid-reconfigure 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> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: ChrisBeWithYou <chris@rifflarr.local>
2026-08-11 03:09:56 +00:00 · 2026-07-08 22:44:26 +02:00
parent 56e929da4e
commit 3e3f1f868c
6 changed files with 279 additions and 66 deletions
@@ -242,13 +242,6 @@ AudioEngine::DeviceOptions AudioEngine::probeDeviceOptionsDual(const juce::Strin
        options.input = inputName;
        options.output = outputName;

-        // userIntendsDuplex matches setAudioDevices's classification:
-        // identical names on both sides (typically both empty = OS default)
-        // means we'll go duplex regardless of which specific devices the
-        // first-enumerated lookup would have produced.
-        const bool userIntendsDuplex = (options.inputType == options.outputType
-                                        && options.input == options.output);
-
        // For probing we still need a concrete device to instantiate.
        // Resolve empty names to first-enumerated ONLY for the probe-device
        // creation below — DON'T write back into options.input/options.output;
@@ -261,25 +254,37 @@ AudioEngine::DeviceOptions AudioEngine::probeDeviceOptionsDual(const juce::Strin
        const juce::String probeOutputName =
            options.output.isEmpty() && outputs.size() > 0 ? outputs[0] : options.output;

-        const bool isDuplex = userIntendsDuplex
-                              || (options.inputType == options.outputType
-                                  && options.input == options.output
-                                  && options.input.isNotEmpty());
+        // Probe the SAME way setAudioDevices() will actually apply, or the
+        // startup auto-apply mis-fires: init() fail-closes on this probe's
+        // `compatible` verdict, so if the probe measures a combined duplex device
+        // but apply then opens split (or vice-versa), the verdict describes a
+        // config that won't be the one used — the classic symptom being "no audio
+        // until I press Apply". Duplex is only attempted for the SAME physical
+        // endpoint (a true single-clock device); two different endpoints of the
+        // same backend (USB cable in + separate speakers out) are two clocks and
+        // go split. Mirror setAudioDevices()'s sameEndpointIntent exactly.
+        bool isDuplex = (options.inputType == options.outputType)
+                        && (options.input == options.output);

        if (isDuplex)
        {
            std::unique_ptr<juce::AudioIODevice> dev(
                inputType->createDevice(probeOutputName, probeInputName));
-            if (!dev) { options.error = "Could not create probe device"; options.compatible = false; return options; }
-
-            options.inputChannels = dev->getInputChannelNames();
-            options.outputChannels = dev->getOutputChannelNames();
-            for (auto rate : dev->getAvailableSampleRates())
-                options.sampleRates.addIfNotAlreadyThere(rate);
-            for (auto size : dev->getAvailableBufferSizes())
-                options.bufferSizes.addIfNotAlreadyThere(size);
+            if (dev)
+            {
+                options.inputChannels = dev->getInputChannelNames();
+                options.outputChannels = dev->getOutputChannelNames();
+                for (auto rate : dev->getAvailableSampleRates())
+                    options.sampleRates.addIfNotAlreadyThere(rate);
+                for (auto size : dev->getAvailableBufferSizes())
+                    options.bufferSizes.addIfNotAlreadyThere(size);
+            }
+            else
+            {
+                isDuplex = false;
+            }
        }
-        else
+        if (!isDuplex)
        {
            std::unique_ptr<juce::AudioIODevice> inDev(
                inputType->createDevice({}, probeInputName));
@@ -665,18 +670,6 @@ AudioEngine::DeviceConfigResult AudioEngine::setAudioDevices(const DeviceConfig&
        return res;
    }

-    // User-intent duplex: both sides came in identical (typically both
-    // empty = "system default", or both naming the same explicit device).
-    // Capture before we resolve names, otherwise the resolve loop below
-    // fills empty-input with first-input-device and empty-output with
-    // first-output-device — those usually differ (especially on macOS
-    // where defaults are separate input/output devices), and the engine
-    // would silently route into split mode with ~85ms of ring-buffer
-    // latency for a config the user expected to be duplex. Legacy
-    // pre-PR settings commonly use empty names; preserve their behavior.
-    const bool userIntendsDuplex = (resolvedInputType == resolvedOutputType
-                                    && config.inputDevice == config.outputDevice);
-
    // Don't resolve empty names to first-device-of-each-type. Pre-PR
    // behavior — and Copilot's fail-closed concern — treat empty names
    // as "OS default" per side. Filling them with inputs[0] / outputs[0]
@@ -687,10 +680,9 @@ AudioEngine::DeviceConfigResult AudioEngine::setAudioDevices(const DeviceConfig&
    const juce::String& resolvedInput  = config.inputDevice;
    const juce::String& resolvedOutput = config.outputDevice;

-    const bool isDuplex = userIntendsDuplex
-                          || (resolvedInputType == resolvedOutputType
-                              && resolvedInput == resolvedOutput
-                              && resolvedInput.isNotEmpty());
+    const bool sameBackendType = (resolvedInputType == resolvedOutputType);
+    const bool sameEndpointIntent = sameBackendType
+                                    && config.inputDevice == config.outputDevice;

    // Normalize before branching — applyDuplexSetup() only checks `> 0` and
    // would otherwise let Infinity (or NaN slipping past N-API) reach JUCE
@@ -705,30 +697,45 @@ AudioEngine::DeviceConfigResult AudioEngine::setAudioDevices(const DeviceConfig&
    // (Extra input devices were closed by the stopAudio() above with their intent
    // kept; startAudio() below re-opens them at the new config — split mode only.)

-    if (isDuplex)
+    // Only attempt the low-latency COMBINED (duplex) device when input and output
+    // are the SAME physical endpoint — a true single-clock duplex device. Two
+    // DIFFERENT endpoints of the same backend (e.g. a USB guitar cable in + separate
+    // speakers out) are independent hardware clocks; forcing them through one duplex
+    // device proved unstable across the app lifecycle (no audio until an explicit
+    // Apply, then distortion / dropouts / silent-in-song on navigation). Those route
+    // through the split path, whose ring buffer bridges the two clocks. Cross-backend
+    // pairs split too. (Low-latency for the two-device case is a separate follow-up —
+    // it needs the device-lifecycle work: startup restore + reconfigure-on-nav.)
+    if (sameEndpointIntent)
    {
        teardownSplitMode();

        const juce::String err = applyDuplexSetup(resolvedInput, resolvedOutput,
                                                  requestedSampleRate, requestedBufferSize);
-        if (err.isNotEmpty())
+        if (err.isEmpty())
        {
+            duplexMode.store(true, std::memory_order_relaxed);
+
+            if (auto* dev = inputDeviceManager.getCurrentAudioDevice())
+            {
+                res.sampleRate = dev->getCurrentSampleRate();
+                res.inputBlockSize = dev->getCurrentBufferSizeSamples();
+                res.outputBlockSize = res.inputBlockSize;
+            }
+            res.ok = true;
+            res.duplex = true;
+        }
+        else
+        {
+            // A same-device config that can't open combined is a real error, not a
+            // reason to silently fall to split (which would misrepresent the intent).
            res.error = err;
            res.duplex = true;
            return res;
        }
-        duplexMode.store(true, std::memory_order_relaxed);
-
-        if (auto* dev = inputDeviceManager.getCurrentAudioDevice())
-        {
-            res.sampleRate = dev->getCurrentSampleRate();
-            res.inputBlockSize = dev->getCurrentBufferSizeSamples();
-            res.outputBlockSize = res.inputBlockSize;
-        }
-        res.ok = true;
-        res.duplex = true;
    }
-    else
+
+    if (!res.ok)
    {
        DeviceConfig resolved = config;
        resolved.inputType = resolvedInputType;
@@ -96,12 +96,23 @@ void NAMProcessor::processBlock(juce::AudioBuffer<float>& buffer, juce::MidiBuff
        inputBuf[(size_t)i] = (double)((sum / (float)numChannels) * inLevel);
    }

-    // Process through NAM model (double** in, double** out)
-    double* inPtr = inputBuf.data();
-    double* outPtr = outputBuf.data();
-    double** inPtrs = &inPtr;
-    double** outPtrs = &outPtr;
-    model->process(inPtrs, outPtrs, numSamples);
+    // 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 mono result to all output channels with output level
    float outLevel = outputLevel.load();
@@ -216,9 +216,6 @@ SignalChain::~SignalChain()

 void SignalChain::prepare(double sampleRate, int blockSize)
 {
-    currentSampleRate = sampleRate;
-    currentBlockSize = blockSize;
-
    // Size the parallel-branch scratch once, off the audio thread. Stereo, the
    // chain's fixed channel layout. avoidReallocating=true keeps the storage
    // stable so the RT path never allocates.
@@ -227,6 +224,10 @@ void SignalChain::prepare(double sampleRate, int blockSize)
    accumScratch.setSize(2, blockSize, false, false, true);

    const juce::ScopedLock sl(lock);
+    // Published under the lock so addProcessor/replaceProcessor's under-lock
+    // stale-format check can't tear against a concurrent prepare.
+    currentSampleRate = sampleRate;
+    currentBlockSize = blockSize;
    for (auto* slot : slots)
    {
        invokePlugin(*slot, [&](juce::AudioProcessor& p)
@@ -259,6 +260,46 @@ void SignalChain::process(juce::AudioBuffer<float>& buffer, juce::MidiBuffer& mi
    const juce::ScopedTryLock sl(lock);
    if (!sl.isLocked()) return;

+    // Never hand a slot a block larger than the one it was prepared for.
+    // prepareToPlay's samplesPerBlock is a hard contract for VST3s, and the NAM
+    // core sizes its conv ring/output buffers to it with only a release-no-op
+    // assert guarding overruns — one oversized block (WASAPI shared mode
+    // delivers them right after a device start) permanently garbles its state.
+    // Slice the block into prepared-size chunks instead; each slot processes
+    // each chunk in sequence, preserving slot ordering per sample.
+    const int totalSamples = buffer.getNumSamples();
+    const int maxChunk = currentBlockSize > 0 ? currentBlockSize : totalSamples;
+    if (totalSamples <= maxChunk)
+    {
+        processLocked(buffer, midi);
+        return;
+    }
+
+    constexpr int kMaxSliceChannels = 8;
+    const int numChannels = buffer.getNumChannels();
+    if (numChannels > kMaxSliceChannels)
+    {
+        // Shouldn't happen (the chain runs stereo) — keep the legacy whole-block
+        // behaviour rather than dropping channels.
+        processLocked(buffer, midi);
+        return;
+    }
+
+    juce::MidiBuffer emptyMidi;
+    float* slicePtrs[kMaxSliceChannels];
+    for (int offset = 0; offset < totalSamples; offset += maxChunk)
+    {
+        const int chunk = juce::jmin(maxChunk, totalSamples - offset);
+        for (int ch = 0; ch < numChannels; ++ch)
+            slicePtrs[ch] = buffer.getWritePointer(ch) + offset;
+        juce::AudioBuffer<float> slice(slicePtrs, numChannels, chunk);
+        // MIDI (all stamped at sample 0) goes to the first slice only.
+        processLocked(slice, offset == 0 ? midi : emptyMidi);
+    }
+}
+
+void SignalChain::processLocked(juce::AudioBuffer<float>& buffer, juce::MidiBuffer& midi)
+{
    // Drain pending MIDI messages from the lock-free queue
    struct DrainedMsg { int slotId; juce::MidiMessage msg; };
    DrainedMsg drained[kMidiQueueSize];
@@ -412,15 +453,37 @@ int SignalChain::addProcessor(std::unique_ptr<juce::AudioProcessor> processor,

    // Prepare under the SEH-catching helper so a plugin that faults during
    // prepareToPlay is blocklisted (next load routes to the sandbox) and the
-    // slot is dropped, rather than taking the app down.
+    // slot is dropped, rather than taking the app down. Snapshot the playback
+    // format we prepare against: this runs OFF the audio lock on an N-API
+    // worker thread, and a device reconfigure can run prepare() concurrently —
+    // its slot loop won't see this slot (not added yet), so if the format
+    // moved we must re-prepare under the lock below or the slot stays at a
+    // stale sample rate / block size until the next device restart (heard as
+    // pitch-shifted/garbled monitoring after first-open races).
+    const double prepSr = currentSampleRate;
+    const int prepBs = currentBlockSize;
    invokePlugin(*slot, [&](juce::AudioProcessor& p)
    {
-        prepareForPlayback(p, currentSampleRate, currentBlockSize);
+        prepareForPlayback(p, prepSr, prepBs);
    });
    if (! slot->processor) return -1;

    int id = slot->id;
    const juce::ScopedLock sl(lock);
+    if (currentSampleRate != prepSr || currentBlockSize != prepBs)
+    {
+        fprintf(stderr,
+                "[SignalChain] addProcessor: device format changed during prepare "
+                "(%.0f/%d -> %.0f/%d) — re-preparing '%s'\n",
+                prepSr, prepBs, currentSampleRate, currentBlockSize,
+                slot->name.toRawUTF8());
+        invokePlugin(*slot, [&](juce::AudioProcessor& p)
+        {
+            p.releaseResources();
+            prepareForPlayback(p, currentSampleRate, currentBlockSize);
+        });
+        if (! slot->processor) return -1;
+    }
    slots.add(slot.release());
    return id;
 }
@@ -456,9 +519,11 @@ bool SignalChain::replaceProcessor(int slotId, std::unique_ptr<juce::AudioProces
    // does — under invokePlugin's SEH/signal guard so a fault in prepareToPlay is
    // contained (the processor is dropped) rather than taking the app down.
    staging.processor = std::move(processor);
+    const double prepSr = currentSampleRate;
+    const int prepBs = currentBlockSize;
    invokePlugin(staging, [&](juce::AudioProcessor& p)
    {
-        prepareForPlayback(p, currentSampleRate, currentBlockSize);
+        prepareForPlayback(p, prepSr, prepBs);
    });
    if (! staging.processor) return false;   // faulted during prepare → leave the slot as-is

@@ -467,6 +532,24 @@ bool SignalChain::replaceProcessor(int slotId, std::unique_ptr<juce::AudioProces
        const juce::ScopedLock sl(lock);
        const int idx = findSlotIndex(slotId);
        if (idx < 0) return false;           // slot was removed underneath us
+        // Same off-lock prepare race as addProcessor: a concurrent device
+        // reconfigure's prepare() couldn't have re-prepared the staging
+        // processor (it isn't in a slot yet). Re-prepare at the current format
+        // before it goes live.
+        if (currentSampleRate != prepSr || currentBlockSize != prepBs)
+        {
+            fprintf(stderr,
+                    "[SignalChain] replaceProcessor: device format changed during prepare "
+                    "(%.0f/%d -> %.0f/%d) — re-preparing '%s'\n",
+                    prepSr, prepBs, currentSampleRate, currentBlockSize,
+                    staging.name.toRawUTF8());
+            invokePlugin(staging, [&](juce::AudioProcessor& p)
+            {
+                p.releaseResources();
+                prepareForPlayback(p, currentSampleRate, currentBlockSize);
+            });
+            if (! staging.processor) return false;
+        }
        auto* slot = slots[idx];
        old = std::move(slot->processor);
        slot->processor = std::move(staging.processor);
@@ -127,6 +127,11 @@ public:

 private:
    int findSlotIndex(int slotId) const;
+    // process() body for one block of at most currentBlockSize samples. Caller
+    // holds `lock`. Split out so process() can slice an oversized device block
+    // (WASAPI shared mode delivers them after a device start) into chunks the
+    // slots were actually prepared for.
+    void processLocked(juce::AudioBuffer<float>& buffer, juce::MidiBuffer& midi);

    juce::OwnedArray<ProcessorSlot> slots;
    juce::CriticalSection lock;
@@ -61,15 +61,30 @@ void SourceChain::processBlock(const float* const* inputData, int numInputChanne
    // range — the broadcast branches fill all of them, the pass-through branch
    // only fills the overlap.
    int filledOutputChannels = 0;
-    if (numInputChannels >= 2 && selectedCh >= 0 && selectedCh < numInputChannels)
+    if (selectedCh >= 0 && selectedCh < numInputChannels)
    {
-        // Single-channel mode (e.g. dry from Valeton GP-5 left channel).
-        // Broadcast the selected input across all output channels.
+        // Explicit single-channel pick (e.g. dry from a Valeton GP-5 left
+        // channel, or a USB guitar cable whose guitar is on a known channel).
+        // Broadcast the selected input across all output channels. Works for a
+        // mono device too (selectedCh 0 on a 1-channel input).
        for (int outCh = 0; outCh < effectiveOutputChannels; ++outCh)
            for (int i = 0; i < numSamples; ++i)
                buffer.setSample(outCh, i, inputData[selectedCh][i] * inGain);
        filledOutputChannels = effectiveOutputChannels;
    }
+    else if (numInputChannels == 1)
+    {
+        // Mono input device — the common USB guitar cable enumerates as a single
+        // capture channel. Broadcast that one channel across EVERY output channel
+        // so the guitar is centred. The old pass-through branch below only filled
+        // min(numInputChannels, outputChannels) = 1 channel and zeroed the rest,
+        // which put the guitar in the left speaker only on a stereo duplex device
+        // (cable-in + speakers-out). This restores mono-in / centred-out.
+        for (int outCh = 0; outCh < effectiveOutputChannels; ++outCh)
+            for (int i = 0; i < numSamples; ++i)
+                buffer.setSample(outCh, i, inputData[0][i] * inGain);
+        filledOutputChannels = effectiveOutputChannels;
+    }
    else if (selectedCh < 0 && numInputChannels > 1)
    {
        // Default pair mono mix: average the first two input channels and
@@ -93,8 +108,9 @@ void SourceChain::processBlock(const float* const* inputData, int numInputChanne
    }
    else
    {
-        // Pass-through: single-input device, or stereo in/out with no explicit
-        // channel selection and no need to mix.
+        // Pass-through: genuine multi-channel in/out with an out-of-range
+        // explicit selection, or other configs that map channels 1:1. (The mono
+        // and default-pair cases are handled above and always broadcast.)
        const int passThroughChannels = juce::jmin(numInputChannels, effectiveOutputChannels);
        for (int ch = 0; ch < passThroughChannels; ++ch)
            for (int i = 0; i < numSamples; ++i)