mirror of
https://github.com/got-feedBack/feedBack-desktop.git
synced 2026-08-11 19:29:32 +00:00
docs: commit the audio-engine TLC findings + plan this branch implements
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Fable 5
parent
48d7e68a91
commit
d887c68014
@@ -0,0 +1,940 @@
|
||||
> **Execution status (2026-07-14, branch `refactor/audio-engine-tlc`):** implemented.
|
||||
> Phases 0-8 of Part IV/V are complete on this branch, including the chain-mutation
|
||||
> serializer + chainGeneration (native + executor), the monitor-mute arbiter, the
|
||||
> single persistence store, and getLatencyBreakdown. Two sequencing changes vs. the
|
||||
> roadmap below: the gain-sanitization fix shipped first (before phase 1), and the
|
||||
> Part II ownership work that needs the rig_builder repo (single chain owner,
|
||||
> legacy-path deletion, alias removal) is NOT on this branch. Drift note: the
|
||||
> "21 clearChain call sites" count in Part II grew to 30 by execution time.
|
||||
|
||||
# Audio Engine TLC — Consolidated Findings & Refactor Plan
|
||||
|
||||
TLC pass on the feedBack-desktop audio engine (2026-07-12, branch `fix/loopback-capture-permission`).
|
||||
Single consolidated document; supersedes the four separate docs
|
||||
(overview / collisions / deep-read / refactor-plan).
|
||||
|
||||
Contents:
|
||||
- **Part I** — how the engine works and integrates with the app, effects (NAM/VST/IR), plugins.
|
||||
- **Part II** — redundant code, settings with multiple writers, control collisions.
|
||||
- **Part III** — line-level deep read of `AudioEngine.cpp` and `NodeAddon.cpp`.
|
||||
- **Part IV** — decomposition plan for the monolithic files.
|
||||
- **Part V** — merged priority roadmap.
|
||||
|
||||
---
|
||||
|
||||
# Part I — Architecture Overview
|
||||
Covers: engine architecture, desktop-app integration, input/output paths, effects (NAM / VST / IR),
|
||||
detection pipeline, and which bundled plugins touch the engine and how.
|
||||
|
||||
---
|
||||
|
||||
### 1. Layer map
|
||||
|
||||
```
|
||||
Renderer plugins (rig_builder, note_detect, stems, …)
|
||||
│ window.feedBackDesktop.audio.* / .audioEffects.* (aliased: slopsmithDesktop)
|
||||
▼
|
||||
preload.ts ── contextBridge, ~99 audio methods + audioEffects methods
|
||||
│ ipcRenderer.invoke / send
|
||||
▼
|
||||
Main process
|
||||
audio-bridge.ts 102 ipcMain.handle channels (audio:*, audio-effects:*)
|
||||
audio-effects-executor.ts validates chain plans, drives native chain
|
||||
vst-crash-guard.ts sentinel files → blocklist crashy VSTs across restarts
|
||||
plugin-manager.ts git-based plugin install/update (server plugins)
|
||||
│ require('slopsmith_audio.node')
|
||||
▼
|
||||
NodeAddon.cpp (N-API, ~160 KB) ── marshals JS ⇄ C++, async workers for VST loads
|
||||
▼
|
||||
AudioEngine (JUCE, C++) src/audio/
|
||||
├── SourceChain ×8 (pooled) per-input capture + detect + tone chain
|
||||
│ └── SignalChain ordered ProcessorSlots (VST | NAM | IR)
|
||||
├── Backing-track transport + signalsmith-stretch
|
||||
├── Stream sink (2nd output device for OBS/Discord)
|
||||
└── Renderer bus (WebAudio master → engine output)
|
||||
▼
|
||||
JUCE AudioDeviceManager(s) → WASAPI / ASIO / DirectSound / CoreAudio / ALSA / JACK
|
||||
|
||||
Out-of-process: slopsmith-vst-host.exe (src/vst-host/main.cpp) — VST3 sandbox child.
|
||||
```
|
||||
|
||||
Key files:
|
||||
|
||||
| Area | File |
|
||||
|---|---|
|
||||
| Engine core | `src/audio/AudioEngine.{h,cpp}` (47K header / 156K impl) |
|
||||
| Per-input chain | `src/audio/SourceChain.{h,cpp}` |
|
||||
| Effects chain | `src/audio/SignalChain.{h,cpp}` |
|
||||
| NAM | `src/audio/NAMProcessor.{h,cpp}` (wraps NeuralAmpModelerCore, `src/audio/third_party/NAM`) |
|
||||
| IR / cab | `src/audio/IRLoader.{h,cpp}` (juce::dsp::Convolution) |
|
||||
| VST hosting | `src/audio/VSTHost.{h,cpp}` |
|
||||
| VST sandbox | `src/audio/Sandbox/*` + `src/vst-host/main.cpp` |
|
||||
| Detection | `PitchDetector` (YIN), `MlNoteDetector` (Basic Pitch ONNX), `ChordScorer`, `NoteVerifier`, `OnsetDetector` |
|
||||
| Utility DSP | `NoiseGate`, `TonePolish`, `BackingLeveler`, `AudioSanitize` |
|
||||
| JS bridge | `src/audio/NodeAddon.cpp`, `src/main/audio-bridge.ts`, `src/main/preload.ts` |
|
||||
|
||||
---
|
||||
|
||||
### 2. AudioEngine core
|
||||
|
||||
`AudioEngine` is a `juce::AudioIODeviceCallback` owning **two** `AudioDeviceManager`s:
|
||||
|
||||
- **Duplex mode** (default): `inputDeviceManager` owns both directions; one callback
|
||||
(`audioDeviceIOCallbackWithContext`) reads input, processes, writes output directly.
|
||||
- **Split mode**: input-only on `inputDeviceManager`, output-only on `outputDeviceManager`
|
||||
(separate device types possible, e.g. ASIO in + WASAPI out). Processed stereo crosses via
|
||||
`outputPendingRing` — a lock-free SPSC ring of 4096 frames where each stereo frame is packed
|
||||
into one `atomic<uint64_t>` (bit_cast L|R) so reads are tear-free. ~85 ms of drift absorption.
|
||||
Input and output block sizes may differ; the ring absorbs asymmetry.
|
||||
|
||||
Device management surface: enumerate types/devices, dual-type probing
|
||||
(`probeDeviceOptionsDual` — sample-rate intersection, `compatible` flag), `setAudioDevices`,
|
||||
metrics (overflow/underflow counters, ring fill). Config persisted by the renderer via
|
||||
`audio:saveDeviceSettings` / `loadDeviceSettings` (legacy single-`type` settings are mirrored to
|
||||
input+output type).
|
||||
|
||||
Threading model (recurring pattern throughout): audio thread never locks — atomics everywhere,
|
||||
lock-free SPSC rings, `static_assert(is_always_lock_free)`, control-thread mutation via
|
||||
pending-flag handoff (e.g. `backingPendingSpeed`), and drop-oldest on overflow with counters.
|
||||
|
||||
---
|
||||
|
||||
### 3. Audio input
|
||||
|
||||
#### Sources (multi-input)
|
||||
|
||||
Per-input state lives on **SourceChain**, a fixed pool of `kMaxSources = 8` constructed up front
|
||||
(pointers never reassigned → no race with audio thread; add/remove only flips an atomic `active`
|
||||
flag). `sources[0]` is the permanent legacy default input; the engine facade forwards the
|
||||
single-source API to it so NodeAddon/renderer needed no change.
|
||||
|
||||
- `addSource(inputChannel, deviceKey)` — bind another channel of the current device
|
||||
(multi-channel interfaces, e.g. Valeton GP-5) or of an **additional physical input device**
|
||||
(`bindInputDevice(deviceKey, name)`, up to 3 extras, each at its own clock; forces split mode).
|
||||
- Removal uses a per-deviceKey `callbacksInFlight` counter handshake; wedged callbacks defer the
|
||||
release (`pendingRelease[]`) instead of blocking.
|
||||
- Per-source: input gain, channel select (-1 = mono mix), monitor mute/kill, meters, verifier
|
||||
offsets (auto device-latency delta + user fine-tune, summed).
|
||||
|
||||
#### Per-source capture path (SourceChain::processBlock, audio thread)
|
||||
|
||||
```
|
||||
device input → channel select / mono mix → inputGain
|
||||
├─→ MlNoteDetector feed + pre-gate inputFrameRing (8192, SPSC) [getInputFrame/getInputSince]
|
||||
→ NoiseGate (post-gain, pre-FX; pitch detector sees ungated signal)
|
||||
├─→ YIN PitchDetector feed + post-gate rawAudioRing (16384) [getRawAudioFrame → tuner]
|
||||
→ SignalChain (VST/NAM/IR tone chain)
|
||||
→ sanitize (non-finite/runaway scrub, counted — issue #403)
|
||||
→ monitor mute / monitor kill / chainOutputGain → TonePolish (fixed 3-band EQ, guitar bus only)
|
||||
→ summed into output mix (sourceMonitorScratch, pre-sized)
|
||||
```
|
||||
|
||||
Monitor semantics: `monitorMute` mutes dry pass-through only when chain empty (suppressible
|
||||
around song-load chain rebuilds); `monitorKill` silences the guitar bus unconditionally
|
||||
(external-rig users), applied to every pooled source.
|
||||
|
||||
---
|
||||
|
||||
### 4. Effects — SignalChain
|
||||
|
||||
Ordered `ProcessorSlot`s, each `Type::{VST, NAM, IR, Empty}` holding a
|
||||
`unique_ptr<juce::AudioProcessor>`. Features:
|
||||
|
||||
- **Routing**: per-slot `pan` (constant-power), `branch` (0 = serial trunk, ≥1 = parallel branch,
|
||||
branches read the pre-split signal, panned outputs summed at merge), `branchSrc` (branch reads
|
||||
L / R / both), `postGain` (per-amp loudness trim). Pre-allocated scratch buffers; all-trunk path
|
||||
pays nothing.
|
||||
- **State**: per-slot base64 VST state; whole-chain JSON preset save/load (`savePreset` /
|
||||
`loadPreset`); `replaceProcessor` swaps a slot in place (same id/position) — used for sandbox
|
||||
promotion and `replaceIR` cab swaps; `setSlotState` for the tone-switcher's incremental rebuild.
|
||||
- **MIDI**: lock-free SPSC queue (64 msgs), `queueMidiMessage(slotId, msg)` from the N-API thread
|
||||
→ drained on audio thread (`audio:sendMidiToSlot`).
|
||||
- Oversized device blocks (WASAPI shared after device start) are sliced to the prepared block size.
|
||||
- SEH/signal guards around plugin prepare/state calls; faults blocklist the plugin path.
|
||||
|
||||
#### Processor types
|
||||
|
||||
- **NAMProcessor** — mono in/out neural amp model (`.nam`), NeuralAmpModelerCore backend
|
||||
(`SLOPSMITH_NAM_SUPPORT`). Async-safe model load: staged `pendingModel`, atomic swap.
|
||||
Input/output level params. No editor.
|
||||
- **IRLoader** — cab IR convolution (`.wav/.aif/.ir`) via `juce::dsp::Convolution`.
|
||||
- **VST3** via **VSTHost**: background directory scanning (per-file subprocess probe through
|
||||
`slopsmith-vst-host --scan-plugin` → XML merge, so crashy plugins can't kill the app),
|
||||
known-plugin persistence, sync `loadPlugin` + async `loadPluginAsync` (message-thread pumping
|
||||
required for AmpliTube-class plugins that post messages to themselves during init).
|
||||
|
||||
#### VST sandbox (out-of-process)
|
||||
|
||||
`SandboxedProcessor` (src/audio/Sandbox) is a `juce::AudioProcessor` façade that forwards
|
||||
everything to a spawned `slopsmith-vst-host.exe` child over a control pipe + shared-memory audio
|
||||
channel (`Protocol.h`; platform impls `_win` / `_posix` / `_shared`). Properties:
|
||||
|
||||
- One child per plugin; child dies with the processor. Crash → `isAlive()` false → audio thread
|
||||
inserts silence; `CrashCallback` + `CrashAttribution` report which plugin died.
|
||||
- Child owns the plugin editor as its own top-level window (Reaper-style; cross-process HWND
|
||||
reparenting broke D3D/GL plugins like Neural DSP Archetypes).
|
||||
- **Promotion path**: an in-process VST3 is promoted to the sandbox when its editor is opened
|
||||
(in-process editors are the Windows WndProc/Qt crash path). `captureVstStateForPromotion`
|
||||
snapshots state under lock + SEH guard, then `replaceProcessor` swaps in the sandboxed twin.
|
||||
- Child guarantees JUCE MessageManager on the OS main thread (impossible in the Node addon where
|
||||
V8 owns it); audio runs on a dedicated ring-drain worker.
|
||||
- Known v1 gaps (documented in header): no `AudioProcessorParameter` proxies; bus layout
|
||||
hard-coded stereo↔stereo.
|
||||
- **vst-crash-guard.ts** (main process): arms a sentinel file before risky load/editor operations;
|
||||
a crash leaves the sentinel behind → next launch blocklists that plugin.
|
||||
|
||||
---
|
||||
|
||||
### 5. Audio output
|
||||
|
||||
Three output paths, mixed in the engine:
|
||||
|
||||
1. **Monitor output** (primary device): sum of active sources' processed guitar buses
|
||||
+ backing-track mix (`backingVolume` fader, `BackingLeveler` per-song loudness normalizer)
|
||||
+ master `outputGain`. Duplex writes in-callback; split drains `outputPendingRing`.
|
||||
2. **Stream sink** — an ADDITIONAL output device carrying an independent submix
|
||||
(backing/game and/or guitar monitor, own gain, sanitized 0..8) for OBS/Discord capture.
|
||||
`setStreamOutputDevice` / `setStreamBus`; underflow/overflow counters + level meter exposed.
|
||||
3. **Renderer bus** (Phase 2, exclusive-mode support): the renderer's WebAudio master mix is
|
||||
pushed over IPC (`audio:pushRendererAudio`, fire-and-forget `ipcRenderer.send`) into a large
|
||||
SPSC ring (65536 frames ≈ 1.5 s; producer is the jittery IPC thread), linear-resampled
|
||||
producer-side to device rate, mixed into engine output. Keeps song/stem audio audible when the
|
||||
output device is exclusive-style (ASIO / WASAPI exclusive) and the OS mixer path is silent.
|
||||
Fed renderer-side by a whole-app `getDisplayMedia({audio})` loopback capture —
|
||||
`setDisplayMediaRequestHandler` in `main.ts` grants it (audio-only, own-app loopback; other
|
||||
apps' audio not captured). Current branch (`fix/loopback-capture-permission`) fixes the media
|
||||
permission handler to allow this getDisplayMedia request.
|
||||
|
||||
**Backing track**: JUCE `AudioFormatReaderSource` → `AudioTransportSource` buffered by a
|
||||
`TimeSliceThread` read-ahead → optional signalsmith-stretch phase vocoder for speed change
|
||||
(1x bypass path; lock-free speed handoff via pending atomic so slider drags never block the RT
|
||||
tryLock). Playhead = accumulated heard frames minus stretcher latency; non-blocking cached
|
||||
position/duration getters. Used for local audio-file playback; sloppak/HTML5-routed songs play
|
||||
through the renderer's WebAudio instead (engine playhead frozen; the verifier is fed the
|
||||
renderer's corrected playhead via `setPlayhead`).
|
||||
|
||||
---
|
||||
|
||||
### 6. Detection & scoring (engine-side)
|
||||
|
||||
- **PitchDetector** — monophonic YIN, sub-Hz parabolic interpolation, reads post-gate signal
|
||||
(silent when gate closed). Backs the always-on home tuner (`audio:getRawPitch`).
|
||||
- **MlNoteDetector** — polyphonic Basic Pitch ONNX model (`loadNoteModel`); armed only while a
|
||||
consumer actually reads ML notes (`setMlNoteDetectionEnabled`) so ONNX inference isn't paid
|
||||
otherwise. `getActiveDetection()` prefers ML when loaded, else YIN — same shape either way.
|
||||
- **ChordScorer** — scores a renderer-supplied chord context against the input ring
|
||||
(`audio:scoreChord`); ML-backed variant when the ML detector is live.
|
||||
- **NoteVerifier** — background thread per source; renderer pushes the chart once (`setChart`),
|
||||
verifier scores each note's timing window against the live playhead + input ring, renderer
|
||||
drains verdicts (`getNoteVerdicts`). Replaced the per-tick scoreChord IPC loop that starved on
|
||||
dense passages. Playhead offset = auto device-latency delta + user fine-tune.
|
||||
- **OnsetDetector** — consumes the input ring gaplessly via `getInputSince`.
|
||||
|
||||
---
|
||||
|
||||
### 7. Main-process integration
|
||||
|
||||
- **audio-bridge.ts** (65K): loads `slopsmith_audio.node`, registers all 102 `audio:*` /
|
||||
`audio-effects:*` IPC handlers, normalizes/persists device settings, wires vst-crash-guard
|
||||
sentinels around VST loads and editor opens, forwards renderer-bus audio.
|
||||
- **audio-effects-executor.ts**: the capability-pipeline backend for the `audio-effects`
|
||||
capability. Accepts validated **chain plans** (`feedBack.audio_effects.chain_plan.v1`, legacy
|
||||
`slopsmith.…` schema accepted): up to 24 stages of kind `nam | ir | vst | utility | bypass`
|
||||
with roles (pedal/amp/cab/…), route keys (default `desktop-main`), gain sets, authorization
|
||||
gating (`user-action` / `restore-selection` / `playback-session`). Translates plans into native
|
||||
calls (loadPreset/clearChain/setBypass/setParameter/setGain/…) and reports structured outcomes
|
||||
(`handled | degraded | failed | unavailable | no-target | user-action-required`).
|
||||
- **preload.ts**: exposes the whole surface as `window.feedBackDesktop` (alias
|
||||
`window.slopsmithDesktop`) — `audio.*` (~99 methods) + `audioEffects.*`.
|
||||
- **NodeAddon.cpp**: N-API glue; libuv async workers for plugin loads (message thread keeps
|
||||
pumping); sandbox-aware VST loading (`loadVstSandboxAware`); shutdown cancels pending loads.
|
||||
|
||||
---
|
||||
|
||||
### 8. Bundled plugins that touch audio
|
||||
|
||||
Plugins are renderer-side (screen.js + plugin.json manifest, capability-pipelines.v1). The ones
|
||||
interacting with the engine:
|
||||
|
||||
| Plugin | Interaction |
|
||||
|---|---|
|
||||
| **audio_engine** (bundled in this repo, `src/renderer/`) | The engine's own UI: device setup, chain editor, meters. Declares provider capabilities `audio-input`, `audio-mix`, `audio-monitoring`; observes `playback` lifecycle to rebuild tone automation / tear down native chain state. |
|
||||
| **rig_builder** | Biggest consumer. Builds amp/cab/pedal rigs; declares `audio-effects` capability and submits chain plans (NAM stages, IRs, VSTs, per-stage gain/bypass/params) through audio-effects-executor. Also privileged-capabilities, jobs, library. Own repo also contains a VST (`vst/`) and tone-curation tooling. |
|
||||
| **nam_tone** | NAM tone library (server-side): manages `nam_models/`, `nam_irs/`, `nam_tone.db` on the Python backend; models/IRs are what `audio:loadNAMModel` / `loadIR` consume. |
|
||||
| **note_detect** | Real-time detection/scoring: arms ML detection, pushes charts (`setChart`), drains verdicts, reads pitch/raw frames, drives per-source scoring APIs. |
|
||||
| **stems** / **stem_mixer** | WebAudio-side stem mix (`audio-mix` capability, mute/volume commands). Their master mix reaches the engine only via the renderer bus on exclusive-mode outputs. |
|
||||
| **midi_amp** | Sends MIDI Program Change to external amps/modelers on tone switches (external gear path; engine-side per-slot MIDI exists via `audio:sendMidiToSlot`). |
|
||||
| **tuner (built-in home tuner)** | Always-on YIN readout via `getRawPitch` / `getRawAudioFrame` — deliberately never pays ONNX cost. |
|
||||
| **virtuoso / practice / minigames** | Consume detection results (verdicts/pitch) rather than driving the chain. |
|
||||
|
||||
Plugin lifecycle: `plugin-manager.ts` installs/updates plugins as git checkouts under the
|
||||
plugins dir (https-only remotes, path-safe names); restart of the Python backend activates them.
|
||||
|
||||
---
|
||||
|
||||
### 9. Observations for the TLC pass (starting points)
|
||||
|
||||
- `AudioEngine.cpp` (156K) and `NodeAddon.cpp` (160K) are monoliths; SourceChain extraction
|
||||
("Phase 0/2" comments) is mid-flight — multi-source fan-out phases still landing.
|
||||
- Duplicated facade surface: engine forwards ~40 single-source methods to `source0()` while a
|
||||
parallel `getSource(id)`-indexed API grows alongside (`audio:*` vs `audio:setSource*`).
|
||||
- Sandbox v1 gaps documented in `SandboxedProcessor.h` (no parameter proxies, fixed stereo buses).
|
||||
- Three separate SPSC ring implementations (outputPendingRing, renderer bus, stream sink) share
|
||||
the packed-LR pattern — candidate for one templated ring.
|
||||
- Backing-track transport is legacy for sloppak songs (renderer WebAudio does playback); the
|
||||
frozen-playhead special case leaks into NoteVerifier via `setPlayhead`.
|
||||
- Naming drift: slopsmith → feedBack rebrand half-done (addon name `slopsmith_audio.node`,
|
||||
`slopsmith-vst-host.exe`, legacy schema ids, `window.slopsmithDesktop` alias).
|
||||
|
||||
---
|
||||
|
||||
# Part II — Redundancy & Control Collisions
|
||||
Every finding below was
|
||||
verified in source; file references point at the current tree.
|
||||
|
||||
Severity legend: 🔴 active conflict (two writers fight at runtime) · 🟠 dual ownership
|
||||
(same setting settable from two places, last-writer-wins, no arbitration) · 🟡 redundancy
|
||||
(duplicate surface/code, no runtime conflict yet).
|
||||
|
||||
---
|
||||
|
||||
### 1. 🔴 Signal chain has three independent writers
|
||||
|
||||
The native `SignalChain` is a single global resource, but three parties load/clear it:
|
||||
|
||||
1. **audio_engine bundle** (`src/renderer/screen.js`): direct `api.loadVST` / `loadNAMModel`
|
||||
/ `loadIR` / `loadPreset` / `clearChain` (21 `clearChain` call sites), plus its own tone
|
||||
auto-switch/automation (`applyToneMappingsNow`, `applyToneAutomationFor`,
|
||||
`_restorePresetBlob` → `clearChain` + `loadPreset`).
|
||||
2. **rig_builder via capability pipeline**: `audioEffects.loadPlan` → main-process
|
||||
`audio-effects-executor.ts` → `nativeAudio.loadPreset`.
|
||||
3. **rig_builder legacy direct path**: `feedBackDesktop.audio.loadPreset` (tracked in its own
|
||||
telemetry as `audio-effects.legacy-native-load`).
|
||||
|
||||
Concrete evidence of the fight (rig_builder `screen.js`):
|
||||
|
||||
> "PROACTIVE TRANSIENT KILL: the bundle calls loadPreset ~1ms after we return this response.
|
||||
> We can't monkey-patch `feedBackDesktop.audio.loadPreset` … the object is frozen by
|
||||
> contextBridge"
|
||||
|
||||
rig_builder ships timing hacks (`_rbUnmuteTimer`, transient kill, fallback unmute) purely to
|
||||
survive the bundle re-loading the chain right after it did. That is two plugins racing on the
|
||||
same native chain with wall-clock heuristics as the arbiter.
|
||||
|
||||
**Additional executor-state hazard**: the executor keeps a `routes` map with
|
||||
`stageSlots` (stageId → native slotId). Any direct `loadPreset` / `clearChain` /
|
||||
`removeProcessor` / `moveProcessor` from path 1 or 3 invalidates those slot ids silently —
|
||||
subsequent `setStageBypass` / `setStageParameter` / `activateSegment` then flip
|
||||
bypass/params on the **wrong slots** (slot ids are reused sequentially by `nextSlotId`) or
|
||||
return `no-target`. Nothing detects the divergence.
|
||||
|
||||
### 2. 🔴 Monitor mute: five writers, one atomic, persisted preference gets clobbered
|
||||
|
||||
`SourceChain::monitorMuted` writers:
|
||||
|
||||
| Writer | Where | When |
|
||||
|---|---|---|
|
||||
| audio_engine settings UI checkbox | `screen.js` (`ae-monitor-mute`) | user toggle; persisted in device settings |
|
||||
| startup restore | `screen.js` ~901 | pushes saved value into engine on boot |
|
||||
| executor preload-mute | `audio-effects-executor.ts:479-489` | saves `previousMonitorMute`, forces mute/unmute during chain load, restores on a `setTimeout` ramp |
|
||||
| executor `releaseRoute` | `:616` | **unconditionally** `setMonitorMute(true)` + `setMonitorMuteSuppressed(false)` |
|
||||
| renderer song-load suppression | `screen.js` (2 sites) + `audio:setMonitorMuteSuppressed` | temporary override around chain rebuild |
|
||||
|
||||
Collisions:
|
||||
|
||||
- `releaseRoute` forces mute=true regardless of the user's persisted `monitorMute:false`
|
||||
preference — the checkbox UI and the engine now disagree until the next toggle/restart.
|
||||
- The executor's read-modify-restore (`previousMonitorMute` + delayed `schedulePreloadRestore`)
|
||||
races a user toggling the checkbox during the hold window: the restore overwrites the fresh
|
||||
user choice with the stale snapshot. `preloadRestoreVersion` guards against *newer executor
|
||||
loads*, not against other writers.
|
||||
- `monitorMuteSuppressed` is set by both the renderer (song load) and executor flows with no
|
||||
refcount — whoever clears last wins; overlapping windows un-suppress early.
|
||||
|
||||
### 3. 🔴/🟠 Gain: four knobs, three surfaces, inconsistent clamping
|
||||
|
||||
Native gains: per-source `inputGain`, per-source `chainOutputGain`, global `outputGain`
|
||||
(master), `backingVolume` — all reachable through `audio:setGain(which, value)`
|
||||
(`NodeAddon.cpp SetGain`), and `input`/`chain` also through
|
||||
`audio-effects:setRouteGain` + chain-plan `options.gains` + `preloadMute.targetGain`.
|
||||
|
||||
- **Dual ownership of `chain` gain**: audio_engine screen sets it (9 `setGain` sites);
|
||||
the executor zeroes it (`trySetGain('chain', 0)` on load and on `releaseRoute`) and later
|
||||
ramps it to `targetGain` (default **1**, or plan-supplied) on a timer. If the user (or tone
|
||||
automation) set chain gain meanwhile, the ramp silently overwrites it. Same
|
||||
stale-snapshot race as monitor mute.
|
||||
- **Clamp inconsistency**: executor clamps to `0..32` (`clampGain`); `NodeAddon::SetGain` does
|
||||
**no** validation — `NaN`/`Infinity` from any direct `audio:setGain` caller reaches
|
||||
`outputGain.store()` / `inputGain.store()` raw. The engine sanitizes only the *stream* and
|
||||
*renderer-bus* gains (`sanitizeStreamGain`, 0..8, explicitly "so a NaN/Inf from JS can never
|
||||
reach the ring") — the exact same hazard is unguarded for master/input/chain/backing.
|
||||
A NaN master gain silences output and poisons the peak meters.
|
||||
- Per-slot `postGain` overlaps conceptually with `chainOutputGain` (both are "level after the
|
||||
amp"): rig plans carry per-stage loudness trims while the screen's chain gain scales the
|
||||
same signal — two normalization layers, no documented ownership.
|
||||
|
||||
### 4. 🟠 Device settings: two persistence stores, newest-timestamp arbitration
|
||||
|
||||
`screen.js loadDeviceSettings()` merges **file-backed** settings (main process,
|
||||
`audio:saveDeviceSettings`) with **`localStorage['slopsmith-audio-device']`**, picking
|
||||
whichever has the newer `savedAt`. Two stores for one setting means:
|
||||
|
||||
- A main-side migration/reset (`config-reset.ts` territory) leaves stale localStorage that can
|
||||
win the timestamp race and resurrect wiped settings.
|
||||
- `monitorMute` / `monitorKill` ride inside the *device* settings blob, so a device re-save
|
||||
from one path re-persists mute flags captured from checkbox state at that moment —
|
||||
interleaving with §2's runtime writers.
|
||||
- Renderer keeps 10 `slopsmith-*` localStorage keys total (`slopsmith-signal-chain`,
|
||||
`slopsmith-chain-presets`, `slopsmith-tone-automation`, …) — the chain is *also* persisted
|
||||
renderer-side while rig_builder persists rigs server-side (`routes.py` / DB): two saved
|
||||
descriptions of the same chain that can disagree on restore.
|
||||
|
||||
### 5. 🟡 Legacy alias surfaces (three layers deep)
|
||||
|
||||
Same setting, multiple entry points kept for back-compat — each a place for behavior to drift:
|
||||
|
||||
- **Engine facade**: `getDeviceManager()` ≡ `getInputDeviceManager()`;
|
||||
`setInputDeviceType()` ≡ `setDeviceType()`; `DeviceOptions.type` ≡ `inputType`;
|
||||
single-source methods (`setInputGain`, `setMonitorMute`, `setChart`, `scoreChord`, ~40 of
|
||||
them) forward to `source0()` while a parallel indexed API (`getSource(id)` →
|
||||
`audio:setSource*`) does the same thing for id 0. Two IPC routes mutate the same atomic
|
||||
(`audio:setMonitorMute` vs `audio:setSourceMonitorMute(0, …)`).
|
||||
- **Settings shape**: legacy `{type}` vs `{inputType, outputType}` normalized in **two
|
||||
places** — `audio-bridge.ts normalizeDeviceSettings` *and* `screen.js
|
||||
normalizeDeviceSettings` (duplicated logic, must stay in sync by hand).
|
||||
- **Schema/branding**: `feedBack.audio_effects.chain_plan.v1` + accepted legacy
|
||||
`slopsmith.…` id; `window.feedBackDesktop` + `window.slopsmithDesktop`; localStorage keys
|
||||
still `slopsmith-*`. Each alias doubles the grep surface for every future change.
|
||||
|
||||
### 6. 🟡 Duplicated implementation code
|
||||
|
||||
- **Three packed-LR SPSC rings** in `AudioEngine.h` (split-mode `outputPendingRing`, renderer
|
||||
bus, stream sink) — same pack/unpack, same power-of-two asserts, same drop-oldest logic,
|
||||
three hand-maintained copies. One templated ring kills ~2/3 of the index math.
|
||||
- **Two fail-soft wrappers per method** in the JS layer: audio-bridge's typeof-guarded
|
||||
handlers and the executor's `trySetGain`/`trySetMonitorMute`/… re-wrap the same native
|
||||
calls with slightly different error policy (bridge: silent no-op; executor: outcome
|
||||
strings). A single native-call helper with one policy would remove a class of divergence.
|
||||
- **`normalizeLoadResult` tolerance duplicated**: both rig_builder (`screen.js`: "Some JUCE
|
||||
bridges return {success:false} or bare …") and the executor normalize loadPreset results
|
||||
independently.
|
||||
- **Chain-restore logic**: executor rollback (`rollbackPreset` + `restorePreset`) vs
|
||||
screen.js `_restorePresetBlob` — two snapshot/rollback implementations for the same chain.
|
||||
|
||||
### 7. 🟠 `startAudio` / route lifecycle from two sides
|
||||
|
||||
`audio:startAudio` is invoked by the renderer UI **and** best-effort by the executor when a
|
||||
chain plan carries `startAudio: true` (`:574`). Neither side knows the other's intent; there's
|
||||
no matching stop ownership — `releaseRoute` clears the chain and mutes but leaves the device
|
||||
running or not depending on who started it.
|
||||
|
||||
---
|
||||
|
||||
### Recommended direction (for TLC scoping, not yet implemented)
|
||||
|
||||
1. **Single chain owner**: make the audio-effects executor the *only* writer of the native
|
||||
chain; migrate the audio_engine screen's direct loadVST/loadPreset/tone-switch calls onto
|
||||
route-scoped executor operations; then delete rig_builder's transient-kill hacks and the
|
||||
legacy direct `loadPreset` path. Executor should reject/re-sync when
|
||||
`getChainState` disagrees with its `stageSlots` map (generation counter on the native chain).
|
||||
2. **Arbitrated monitor state**: replace raw `setMonitorMute` writes with a small state owner
|
||||
(user preference + N stackable suppressions/overrides, refcounted). `releaseRoute` releases
|
||||
its override instead of forcing `true`.
|
||||
3. **Sanitize all gains natively**: extend `sanitizeStreamGain`-style clamping to
|
||||
input/chain/output/backing in `AudioEngine` setters (single choke point) and drop the
|
||||
JS-side clamp divergence.
|
||||
4. **One persistence store per setting**: file-backed settings as the single source; treat
|
||||
localStorage as a migration source only, delete after import. Move mute flags out of the
|
||||
device blob.
|
||||
5. **Deprecation plan for aliases**: freeze `slopsmith*` surfaces, log-once on use, remove on
|
||||
next major.
|
||||
|
||||
---
|
||||
|
||||
# Part III — Deep Read: AudioEngine.cpp + NodeAddon.cpp
|
||||
|
||||
Full read of `src/audio/AudioEngine.cpp` (3223 lines) and the load-bearing regions of
|
||||
`src/audio/NodeAddon.cpp` (3699 lines). Line refs current as of
|
||||
`fix/loopback-capture-permission`.
|
||||
|
||||
**Overall verdict first**: the RT core is in much better shape than its size suggests —
|
||||
disciplined lock-free SPSC rings, no allocation on the audio thread, denormal flushing on every
|
||||
callback clock, a correct per-deviceKey quiescence handshake for source removal, and unusually
|
||||
good comments that cite the bug each guard fixes. The problems live at the *edges*: the JS↔native
|
||||
boundary, concurrency between async workers, and inconsistent input sanitization.
|
||||
|
||||
---
|
||||
|
||||
### 1. 🔴 Chain-mutating async workers are not serialized (NodeAddon)
|
||||
|
||||
`LoadPresetWorker`, `LoadVSTWorker`, `LoadNAMWorker`, `LoadIRWorker`, `ReplaceIRWorker` all queue
|
||||
on the libuv threadpool (default 4 threads) with **no mutual exclusion between workers**.
|
||||
`SignalChain` locks per-operation only, so the sequence `clear() → addProcessor() × N`
|
||||
(`NodeAddon.cpp:3312-3408`) is not atomic.
|
||||
|
||||
Two `loadPreset` calls in flight — which is precisely the documented rig_builder-vs-bundle
|
||||
"~1ms later" race from the collisions doc — can interleave as:
|
||||
|
||||
```
|
||||
worker A: clear() worker B: clear()
|
||||
worker A: add(ampA) worker B: add(ampB)
|
||||
worker A: add(irA) → final chain: [ampA, ampB, irA, irB] (merged garbage)
|
||||
```
|
||||
|
||||
Both report `success:true` with wrong `slotsLoaded` semantics; the executor's stageId→slotId map
|
||||
is then built against a chain that neither caller described. A `loadVST` concurrent with a
|
||||
`loadPreset` similarly lands a slot into (or after) someone else's rebuild.
|
||||
|
||||
**Fix shape**: one native "chain mutation" mutex (or a serial dispatch queue) around
|
||||
clear+rebuild and single-slot adds; alternatively a chain generation counter returned to JS so
|
||||
callers detect they lost the race. This is the single highest-value fix of the whole pass —
|
||||
it converts the plugin-vs-plugin fight from corruption to last-writer-wins.
|
||||
|
||||
### 2. 🔴 Argument sanitization is inconsistent across the N-API surface
|
||||
|
||||
The addon knows the hazard — `getValidatedSource` (`NodeAddon.cpp:89-103`) documents that
|
||||
`Int32Value()` coerces NaN→0, and `setAudioDevices` normalizes sampleRate against "NaN slipping
|
||||
past N-API" (`AudioEngine.cpp:700-708`). But that rigor is only applied to the *newer* bindings:
|
||||
|
||||
| Guarded (fail-soft) | Unguarded (blind `As<>()` coercion) |
|
||||
|---|---|
|
||||
| `getValidatedSource` (all `*Source*` methods) | `SetGain` — NaN/Inf reaches `outputGain.store()` raw |
|
||||
| `SetSlotState` (IsNumber/IsString checks) | `SetParameter`, `SetBypass`, `RemoveProcessor`, `MoveProcessor` — NaN slotId → **slot 0** |
|
||||
| `SetMonitorMuteSuppressed`, `SetMonitorKill` (bridge-side Boolean coercion) | `SetMultiBypass` (per-item `As<Number>` uncheck) |
|
||||
| `setBackingSpeed` (isfinite + clamp, engine-side) | `SendMidiToSlot` (channel/program unclamped → JUCE assertions) |
|
||||
|
||||
Consequences of the worst one: a NaN master gain via `audio:setGain('output', NaN)` multiplies
|
||||
the entire device output to NaN (`buffer.applyGain(outputGain.load())`,
|
||||
`AudioEngine.cpp:2407/3083`) — full silence plus poisoned peak meters, and nothing scrubs it
|
||||
(the per-source NaN scrub runs *before* the master gain). Engine-side clamps at the four gain
|
||||
setters (mirroring `sanitizeStreamGain`) fix every caller at once.
|
||||
|
||||
### 3. 🔴 `wasRunning` race in `setAudioDevices` (AudioEngine.cpp:658)
|
||||
|
||||
`audioDeviceStopped()` clears `audioRunning` on **transient** stops, and the code's own comment
|
||||
says WASAPI exclusive opens "routinely fire one mid-start". `setAudioDevices` captures
|
||||
`wasRunning = audioRunning.load()` and only calls `startAudio()` at the end when it was true.
|
||||
The comment above it (`:651-657`) fixes the *detach* half of this race (stopAudio is now
|
||||
unconditional) but the *restart* half still reads the racy flag: a reconfigure landing inside a
|
||||
transient-stop window sees `wasRunning == false` and leaves the engine configured but stopped —
|
||||
"no audio until user presses Start/Apply again". The intent flag it should read is "did the user
|
||||
want audio running", which currently doesn't exist separately from device state (see §6).
|
||||
|
||||
### 4. 🟠 `setRendererBus(false)` violates the ring's own SPSC discipline
|
||||
|
||||
`AudioEngine.h` (`setRendererBus`) drops buffered audio on disable by writing
|
||||
`rendererBusReadIndex` from the **control thread**, while `pullRendererBus`
|
||||
(`AudioEngine.cpp:3143-3208`) is the designated single consumer-side writer of that index (the
|
||||
file's comments elsewhere are explicit that "only the consumer ever moves readIndex"). A
|
||||
concurrent output callback mid-`pullRendererBus` can overwrite the control thread's store with
|
||||
`r + pull`, replaying a stale tail after re-enable — exactly what the drop was meant to prevent.
|
||||
Low probability, audible-blip severity; fix by setting a "flush requested" atomic the consumer
|
||||
honors instead of writing its index.
|
||||
|
||||
### 5. 🟠 Latency accounting has three unreconciled truths
|
||||
|
||||
- `getLatencyMs()` (`:469-496`): device latencies + (split only) a static `kOutputRingFrames/2`
|
||||
≈ 42.7 ms ring-residency guess. The actual ring fill is measurable (`getDeviceMetrics` reports
|
||||
it) but not used.
|
||||
- Verifier auto-offset (`extraInputAboutToStart`, `:2554-2568`): per-device *input-latency
|
||||
delta* only, 0 on JACK/PipeWire (documented), user offset summed on top.
|
||||
- Renderer bus: adds `kRendererBusPrimeFrames` (~10 ms) prime + fill drift + producer-side
|
||||
resample, none of it surfaced in any latency figure; stems audio through the bus is delayed by
|
||||
an amount the UI never reports and the verifier never compensates.
|
||||
|
||||
For a TLC pass: one `getLatencyBreakdown()` that owns all terms would replace three ad-hoc sums.
|
||||
|
||||
### 6. 🟠 `audioRunning` conflates user intent with device state
|
||||
|
||||
Writers: `startAudio`/`stopAudio` (user intent), `audioDeviceAboutToStart` (device came up —
|
||||
including JUCE auto-restarts the user never asked for, `:1802`), `audioDeviceStopped` (device
|
||||
went down — including transient stops the user didn't ask for). Readers assume different
|
||||
meanings: `setAudioDevices` reads it as intent (§3), detection guards read it as device state
|
||||
(correct), the bridge's `isAudioRunning` surfaces it to the UI as intent. Two booleans
|
||||
(`userWantsAudio`, `deviceRunning`) would kill the §3 race and make the auto-restart paths
|
||||
self-explanatory. Related: `stopAudio()` does not stop the backing transport — `backingPlaying`
|
||||
stays true and playback resumes on the next start, which is intentional for unplug-recovery but
|
||||
surprising for an explicit user stop.
|
||||
|
||||
### 7. 🟡 Probe/apply duplication — three copies of the rate-tolerance logic
|
||||
|
||||
The `|r - r2| <= 0.5` sample-rate matching + round-to-nominal logic exists in
|
||||
`probeDeviceOptionsDual` (`:316-350`), `applySplitSetup::rateSupportedBy` (`:961-982`), and the
|
||||
post-open verify (`:1146`). The comments at each site narrate keeping the three in sync by hand
|
||||
("<= 0.5 (not <) to match…", "Tolerance matches the probe-side rounding…") — i.e. they've
|
||||
already been bitten. Same story for empty-name→first-enumerated resolution (probe, preflight,
|
||||
apply must agree; three sites). One shared helper each.
|
||||
|
||||
### 8. 🟡 Device identity is display-name only (documented limitation)
|
||||
|
||||
`getBindableInputDevices` (`:120-161`) and `bindInputDevice`'s duplicate/primary checks compare
|
||||
`juce::String` names. Two identical interfaces collapse to one entry; a device exposed under two
|
||||
backends may bind the wrong one. The comment block is honest about it; flagging here because the
|
||||
fix ((typeName, name) identity threaded through bind/reopen/persistence) also touches the
|
||||
renderer's saved settings — a cross-layer change worth scheduling deliberately.
|
||||
|
||||
### 9. 🟡 NodeAddon miscellany
|
||||
|
||||
- **`LoadPresetWorker` state restore bypasses `setSlotState`**: it `const_cast`s the slot from
|
||||
`getSlot()` and calls `slot->setState(state)` directly (`:3402-3404`) — outside whatever
|
||||
synchronization `SignalChain::setSlotState` provides against a concurrently-processing audio
|
||||
thread. During a preset load the chain was just rebuilt so the window is small, but it's the
|
||||
only chain mutation in the file that dodges the class's own API.
|
||||
- **Every preset load closes every editor window** (`LoadPreset:3452`, `ClearChain:2668` —
|
||||
required by the #56 use-after-free). Combined with the tone auto-switch calling loadPreset on
|
||||
song events, a user tweaking an amp editor mid-song has the window yanked. A single-slot
|
||||
replace path (`replaceProcessor` exists) for tone switches would avoid the nuke.
|
||||
- **macOS is a second-class citizen by design**: no JUCE dispatch loop (`startJuceMessageThread`
|
||||
JUCE_MAC branch), `dispatchOnMessageThread` runs inline, VST/AU instantiation "given up until
|
||||
a proper libuv-based pump lands". Every load path carries a divergent `#if JUCE_MAC` branch —
|
||||
a large, mostly-untested platform fork woven through the file.
|
||||
- **`loadVstSandboxAware` holds a libuv worker for the whole plugin init** (documented tradeoff,
|
||||
`:2241-2248`); concurrent slow loads can starve fs/crypto AsyncWorkers.
|
||||
- **Misnomer**: `inputOverflowCount` is incremented by the *output consumer's* catch-up on the
|
||||
primary split ring (`:2941`) — it counts ring overruns, not input overflows; the metric name
|
||||
leaks into `DeviceMetrics`/diagnostics.
|
||||
|
||||
### 10. What is genuinely solid (don't "fix")
|
||||
|
||||
- The per-deviceKey `callbacksInFlight` handshake + `pendingRelease` deferral for source removal
|
||||
(`:1554-1618`) — correct, well-reasoned, and the 200 ms bounded wait is the right call.
|
||||
- Ring discipline: packed-LR single-atomic frames, consumer-side drop-oldest, `w < r` resync
|
||||
after index resets, consume-vs-pull split to avoid clock skew after clamps (`:2944-2974`).
|
||||
- RT allocation hygiene: every scratch pre-sized in about-to-start, every hot path clamps to
|
||||
capacity instead of resizing; stream scratches deliberately fixed at ring capacity so a
|
||||
hotplug about-to-start can't realloc under a live producer (`:1830-1841`).
|
||||
- `ScopedNoDenormals` on *both* callback clocks with the explanation of why (`:2268-2273,2898`).
|
||||
- Backing speed hand-off (pending atomic + same-block stretcher reset, `:1670-1691`) and the
|
||||
read-ahead thread rationale, including the honest note about `BufferingAudioSource`'s residual
|
||||
lock (`:1334-1341`).
|
||||
- `bindInputDevice`'s failure hygiene: every abort path closes the half-open device; validate-
|
||||
eagerly-then-close when the engine is stopped (`:2774-2783`).
|
||||
|
||||
---
|
||||
|
||||
### Priority order for the TLC pass
|
||||
|
||||
1. **Serialize chain mutations** (§1) — prerequisite for any single-chain-owner work from the
|
||||
collisions doc; without it the executor can't even trust its own load result.
|
||||
2. **Sanitize gains + slot ids natively** (§2) — small, mechanical, kills a user-visible
|
||||
silence-the-app bug class.
|
||||
3. **Split `audioRunning` into intent + state** (§6, fixes §3) — unlocks correct
|
||||
reconfigure-under-transient-stop and clarifies every auto-restart path.
|
||||
4. **Renderer-bus flush flag** (§4) — one-line-ish, closes the last SPSC discipline hole.
|
||||
5. **Latency breakdown API** (§5) and **probe/apply shared helpers** (§7) — quality-of-life,
|
||||
schedule with the settings-ownership work.
|
||||
|
||||
---
|
||||
|
||||
# Part IV — Decomposition / Refactor Plan
|
||||
Targets the two monoliths:
|
||||
`AudioEngine.{h,cpp}` (819 + 3223 lines) and `NodeAddon.cpp` (3699 lines).
|
||||
Builds on the findings in Part II and Part III —
|
||||
several fixes there (chain-mutation serialization, gain sanitization, intent/state split)
|
||||
get a natural home in the new units instead of being bolted onto the monolith.
|
||||
|
||||
**Precedent**: the SourceChain extraction already proved the working method on this codebase —
|
||||
move a cohesive member cluster verbatim into a class, bind shared engine atomics by reference,
|
||||
keep the facade byte-identical, land in phases. This plan repeats that recipe seven more times.
|
||||
|
||||
**Prime directive**: no behavior change per phase. Every phase is a pure code move that
|
||||
compiles + passes the existing tests (`tests/audio_sanitize`, `tests/sandbox/*`, e2e) before
|
||||
the next starts. Bug fixes ride in separate commits on top of the phase that creates their home.
|
||||
|
||||
---
|
||||
|
||||
### 1. Target layout
|
||||
|
||||
```
|
||||
src/audio/
|
||||
engine/
|
||||
AudioEngine.{h,cpp} facade + callback orchestration only (~500 lines total)
|
||||
DeviceSetup.{h,cpp} probe/apply/teardown for duplex + split (§2.1)
|
||||
SourcePool.{h,cpp} source add/remove/reclaim + in-flight counts (§2.2)
|
||||
ExtraInputs.{h,cpp} InputDeviceSlot registry, bind/unbind/reopen (§2.3)
|
||||
BackingPlayer.{h,cpp} transport + stretch + leveler + playhead (§2.4)
|
||||
StreamSink.{h,cpp} 2nd output device + submix compose (§2.5)
|
||||
RendererBus.{h,cpp} WebAudio→engine ring, push/pull/metrics (§2.6)
|
||||
PackedStereoRing.h the one SPSC ring template (§2.7)
|
||||
EngineState.h shared atomics: rates, block sizes, run state(§2.8)
|
||||
addon/
|
||||
NodeAddon.cpp module init + binding registration only
|
||||
AddonContext.{h,cpp} engine/vstHost lifetime, message thread, shutdown latch
|
||||
NapiHelpers.h arg validation (the getValidatedSource pattern, generalized)
|
||||
ChainOps.{h,cpp} chain workers (LoadPreset/VST/NAM/IR) + mutation queue
|
||||
DeviceBindings.cpp device enumeration/config/metrics bindings
|
||||
ControlBindings.cpp gain/mute/gate/stream/renderer-bus bindings
|
||||
DetectionBindings.cpp pitch/chord/chart/verdict/source-indexed bindings
|
||||
BackingBindings.cpp backing-track bindings
|
||||
EditorWindows.{h,cpp} PluginEditorWindow + open/close/promotion
|
||||
(existing DSP files stay where they are)
|
||||
```
|
||||
|
||||
CMake: append the new files to the existing source list in `src/audio/CMakeLists.txt`; no
|
||||
target restructuring needed.
|
||||
|
||||
---
|
||||
|
||||
### 2. AudioEngine decomposition (one phase per unit)
|
||||
|
||||
Ordering is by extraction risk, lowest first. Each unit lists what moves, its boundary, and
|
||||
which known bug lands in it afterwards.
|
||||
|
||||
#### 2.1 `PackedStereoRing<Frames>` — first, everything else builds on it
|
||||
|
||||
Template over capacity; owns `array<atomic<uint64_t>>`, write/read indices, the pack/unpack
|
||||
helpers, and the three ritual moves currently copy-pasted at six sites: producer publish
|
||||
(`packStereoIntoRing`), consumer `w < r` resync, lapped catch-up with overflow counter, and the
|
||||
pull-vs-consume split. Replaces: `outputPendingRing`, each `InputDeviceSlot::ring`,
|
||||
`streamSink.ring`, `rendererBusRing`. The static_asserts move inside the template.
|
||||
**Bug fixed here after the move**: none — but §2.6's flush fix becomes a one-method addition
|
||||
(`requestFlush()` honored by the consumer) instead of index surgery.
|
||||
|
||||
#### 2.2 `SourcePool`
|
||||
|
||||
Moves: `sources[]` array, `sourcesMutex`, `callbacksInFlight[]`, `pendingRelease[]`,
|
||||
`addSource/removeSource/reclaimPendingReleases/listSources/getSource`, the fan-out helpers
|
||||
(`setMlNoteDetectionEnabled`, `setMonitorKill` loop, `resetPeaks` loop), and
|
||||
`mixSourcesForDevice`. Boundary: callbacks call `pool.enterCallback(deviceKey)` /
|
||||
`pool.exitCallback(deviceKey)` (RAII guard) and `pool.mixForDevice(...)`.
|
||||
This is the most delicate move (RT-shared state) but it is also the best-commented, most
|
||||
self-contained cluster — the handshake logic doesn't touch any other member.
|
||||
|
||||
#### 2.3 `ExtraInputs`
|
||||
|
||||
Moves: `InputDeviceSlot` + `extraInputs[]`, `bindInputDevice/unbindInputDevice/
|
||||
closeExtraInputDevice/reopenDesiredExtraInputs/activeExtraInputCount`, the extra callback
|
||||
trio (`extraInputCallback/AboutToStart/Stopped`). Depends on SourcePool (prepares/releases
|
||||
sources by deviceKey) and PackedStereoRing. The (typeName, name) device-identity fix
|
||||
(deep-read §8) lands here later without touching the engine again.
|
||||
|
||||
#### 2.4 `BackingPlayer`
|
||||
|
||||
Moves: transport + reader + read-ahead thread, signalsmith stretch state, `backingLock`,
|
||||
speed hand-off atomics, `BackingLeveler`, `renderBackingBlockLocked`, all playhead caches,
|
||||
load/start/stop/seek/speed. Boundary: `backing.renderInto(buffer, numSamples)` returning frames
|
||||
(caller mixes + meters), `backing.prepare(sr, bs)` from the about-to-start hooks. The duplex and
|
||||
split callbacks already share `renderBackingBlockLocked`, so the seam exists.
|
||||
**Lands here later**: the "stop engine ≠ stop backing" intent decision (deep-read §6 note).
|
||||
|
||||
#### 2.5 `StreamSink`
|
||||
|
||||
Already 80% a struct — promote to a class owning its manager, callback, ring, scratches,
|
||||
`composeAndPushStreamMix`, `set/clear/reopen/close`. Bus flags (`includeBacking/includeGuitar/
|
||||
gain`) move in. The engine's callbacks call `sink.publish(guitarMix, backing, renderer, n)`.
|
||||
|
||||
#### 2.6 `RendererBus`
|
||||
|
||||
Moves: ring + indices + resampler carry-state (`rendererBusSrcPos/PrevL/PrevR`), prime/fill
|
||||
constants, `pushRendererAudio/pullRendererBus/getRendererBusMetrics/setRendererBus`.
|
||||
**Bug fixed here after the move**: the control-thread readIndex write on disable (deep-read §4)
|
||||
becomes an atomic `flushRequested` flag consumed in `pull()`.
|
||||
|
||||
#### 2.7 `DeviceSetup`
|
||||
|
||||
Moves: `probeDeviceOptions[Dual]`, `applyDuplexSetup`, `applySplitSetup`, `teardownSplitMode`,
|
||||
type resolution/preference tables, and the three hand-synced helpers extracted once:
|
||||
`rateIntersection()`, `bufferIntersection()`, `resolveDeviceName()` (deep-read §7). Stateless
|
||||
apart from references to the two managers + EngineState; takes managers by reference so it owns
|
||||
no lifetime. `setAudioDevices` stays on the facade as the orchestrator (stop → resolve →
|
||||
duplex-or-split → restart) but shrinks to ~40 lines.
|
||||
|
||||
#### 2.8 `EngineState`
|
||||
|
||||
Tiny header: `currentSampleRate`, `inputBlockSize`, `outputBlockSize`, `duplexMode`, and —
|
||||
**the deliberate fix from deep-read §3/§6** — `userWantsAudio` (intent, written only by
|
||||
start/stopAudio) split from `deviceRunning` (state, written by the device callbacks).
|
||||
SourceChain already binds engine atomics by reference; it re-binds to this struct unchanged.
|
||||
Every unit above takes `EngineState&`, which is what keeps them unit-testable without JUCE
|
||||
devices (hand them a state struct + a fake ring).
|
||||
|
||||
#### What remains on `AudioEngine`
|
||||
|
||||
The `AudioIODeviceCallback` implementations (now ~60 lines each: enter pool guard → mix →
|
||||
backing → renderer bus → sink publish → master gain → meters), the facade forwarding to
|
||||
`source0()` (unchanged for NodeAddon compatibility), device enumeration getters, and
|
||||
construction/destruction ordering. Header drops from 819 lines to roughly 250.
|
||||
|
||||
---
|
||||
|
||||
### 3. NodeAddon decomposition
|
||||
|
||||
The file is 100+ bindings sharing four bits of infrastructure. Split infrastructure first,
|
||||
then the bindings become mechanical moves.
|
||||
|
||||
#### 3.1 `AddonContext` — lifetime + threading
|
||||
|
||||
Moves: `engine/vstHost` globals + mutexes + `snapshotEngine/snapshotVstHost`, the JUCE message
|
||||
thread (`startJuceMessageThread/stop/dispatchOnMessageThread` with the macOS fork in ONE place),
|
||||
`alreadyShutDown`, `doShutdown`, `registerPendingLoad/cancelAllPendingLoads`. Everything else
|
||||
receives `AddonContext&`. This quarantines the `#if JUCE_MAC` platform fork (deep-read §9) into
|
||||
a single file instead of a branch inside every load path.
|
||||
|
||||
#### 3.2 `NapiHelpers.h` — kill the validation inconsistency structurally
|
||||
|
||||
Generalize the `getValidatedSource` pattern into typed extractors:
|
||||
|
||||
```cpp
|
||||
std::optional<int> argSlotId(info, i); // finite integer, >= 0
|
||||
std::optional<float> argGain(info, i); // finite, clamped 0..8 (one policy)
|
||||
std::optional<float> argParamValue(info, i); // finite, clamped 0..1
|
||||
std::optional<bool> argBool(info, i);
|
||||
```
|
||||
|
||||
Then rewriting `SetGain/SetParameter/SetBypass/SendMidiToSlot/SetMultiBypass` onto them is the
|
||||
deep-read §2 fix, done once, enforced by convention (new bindings have no raw `As<>()` path to
|
||||
copy). Engine-side clamps in the gain setters stay as the second belt.
|
||||
|
||||
#### 3.3 `ChainOps` — the serialization point
|
||||
|
||||
Moves: all five chain workers + `loadVstSandboxAware` + `decodeStateBlob`. Adds the
|
||||
**chain-mutation serializer** (deep-read §1): a single `std::mutex chainMutationMutex` acquired
|
||||
for the full Execute() of every worker, plus a monotonic `chainGeneration` bumped on every
|
||||
mutation and returned in load results — the executor and renderer can then detect a lost race
|
||||
instead of trusting a corrupted merge. The `const_cast` slot-state bypass (deep-read §9) is
|
||||
replaced with `setSlotState()` during this move.
|
||||
|
||||
#### 3.4 `EditorWindows`
|
||||
|
||||
Moves: `PluginEditorWindow`, the window map, open/close/destroy-on-message-thread, and the
|
||||
sandbox-promotion flow inside `OpenPluginEditor`. Later improvement lands here: tone-switch
|
||||
single-slot replace instead of close-all-editors (collisions/deep-read editor-nuke issue).
|
||||
|
||||
#### 3.5 Binding files
|
||||
|
||||
`DeviceBindings/ControlBindings/DetectionBindings/BackingBindings` — pure moves, grouped to
|
||||
match the preload API sections, each ~400-600 lines. `NodeAddon.cpp` keeps only `Init/Shutdown`
|
||||
and the `exports.Set(...)` table (which doubles as the API index the current file lacks).
|
||||
|
||||
---
|
||||
|
||||
### 4. Phase 0 — Compatibility contract & test scaffolding
|
||||
|
||||
Runs BEFORE any code moves. Purpose: turn "no public API change" from a review rule into a
|
||||
failing CI check, and codify the compat decisions consumers (core screen, rig_builder,
|
||||
note_detect, stems) depend on. Test infrastructure follows the repo's existing two-track
|
||||
convention: native `tests/<name>/test.cpp` targets registered in `tests/CMakeLists.txt`, and
|
||||
Node `tests/*.test.js` for the JS/addon boundary.
|
||||
|
||||
**0.a Contract snapshots (`tests/contracts/`)**
|
||||
|
||||
| Snapshot | Source | How |
|
||||
|---|---|---|
|
||||
| `addon-exports.json` | `slopsmith_audio.node` export table | Node script: `Object.keys(require(addon)).sort()` |
|
||||
| `preload-audio-api.json` | `window.feedBackDesktop.audio.*` + `audioEffects.*` key lists | static extraction from `preload.ts` |
|
||||
| `ipc-channels.json` | every `ipcMain.handle`/`ipcMain.on` name in `audio-bridge.ts` | static extraction |
|
||||
| `result-shapes.json` | golden key/type shapes (not values) for `loadPreset`, `loadVST`, `loadNAM/IR`, `getChainState`, `savePreset`, `getDeviceMetrics`, `getRendererBusMetrics`, and every executor outcome (`loadChainPlan`/`releaseRoute`/`setRouteGain`/…) | run against the real addon (null audio device) + executor with a stubbed native |
|
||||
|
||||
CI job `contract-check` regenerates all four and diffs against the committed snapshots.
|
||||
Additive keys require a deliberate snapshot update in the same PR; removals/renames fail.
|
||||
|
||||
**0.b Compat decisions codified as tests**
|
||||
|
||||
- `isAudioRunning` reports **device state** (current semantics) — pinned by a test across a
|
||||
simulated transient stop, so the phase-1 intent/state split can't silently change it.
|
||||
- Native gain clamp bounds = **0..32** (matching the executor's `clampGain`), NaN/Inf rejected
|
||||
universally; only stream/renderer-bus gains keep the tighter 0..8 `sanitizeStreamGain`.
|
||||
Pinned by a table-driven test so phase 8's clamps can't under-shoot a legit rig gain.
|
||||
- Concurrent `loadPreset` storm test written NOW (expected-fail / quarantined): two overlapping
|
||||
loads must end with the chain equal to exactly one caller's preset. Documents today's
|
||||
corruption, flips to expected-pass at phase 7, and doubles as the rig_builder timing smoke
|
||||
(its transient-kill/unmute heuristics tolerate serialized latency — assert its fallback
|
||||
unmute path still fires).
|
||||
|
||||
**0.c Native unit-test harness**
|
||||
|
||||
Add a `tests/engine_units/` CMake target (same pattern as `tests/audio_sanitize`) that links the
|
||||
audio sources without a real device — the home for every per-unit test below. Add a tiny
|
||||
`FakeClock`/`NullDevice` helper pair here once; all later phases reuse them.
|
||||
|
||||
### 5. Bespoke tests per phase (unit + integration)
|
||||
|
||||
Each extraction phase ships WITH its tests in the same PR — the unit tests pin the moved logic,
|
||||
the integration gate proves the seam. "U" = `tests/engine_units` C++ test, "I" = Node/e2e.
|
||||
|
||||
| Phase | Unit tests (new) | Integration tests |
|
||||
|---|---|---|
|
||||
| 1 `PackedStereoRing` | U: SPSC threaded stress (producer/consumer at different block sizes); wrap + drop-oldest lap; `w < r` resync after index reset; L/R tear check under lap (packed-atomic invariant); pull-vs-consume skew accounting; overflow/underflow counters | I: existing audio smoke (duplex, split, stream, renderer bus pass audio); contract-check green |
|
||||
| 1 `EngineState` | U: intent/state transition table — user start/stop × device aboutToStart/stopped × transient stop; `isAudioRunning` compat pin from 0.b stays green | I: reconfigure-during-transient-stop scenario (documents deep-read §3; expected-fail until phase 8) |
|
||||
| 2 `RendererBus` | U: resampler continuity across pushes (fractional pos + carried frame → no discontinuity at chunk seams); equal-rate degenerate path bit-exact; prime gate (no output until ~10 ms); underflow → silence + re-prime; fill clamp trims to prime target; flush-on-disable drops tail (expected-fail until phase 8 flag fix); metrics arithmetic | I: `getRendererBusMetrics` shape + push/consume accounting via addon against null device |
|
||||
| 2 `StreamSink` | U: submix compose matrix (guitar/backing/renderer × include flags × gain); oversized-block skipped AND counted; scratch-not-sized skip is silent-safe | I: OBS-capture manual smoke; stream under/overflow counters via IPC |
|
||||
| 3 `BackingPlayer` | U (synthetic reader source): speed change adopts rate + stretch reset in same block; EOF short-read playhead clamp; stretch-latency compensation vs 1× bypass; leveler re-prepare on SR change; tryLock-miss drops block without state damage | I: existing backing play/seek/speed e2e, duplex + split; `audio-chain-persistence.test.js` green |
|
||||
| 4 `DeviceSetup` | U: `rateIntersection`/`bufferIntersection`/`resolveDeviceName` helpers — incl. the 0.5 Hz boundary cases the three duplicated sites hand-narrate today, midpoint-rounding fail-closed cases, empty-name resolution parity | I: manual device matrix (WASAPI shared/exclusive, ASIO, dual-type split); probe verdict == apply outcome assertion in a scripted run |
|
||||
| 5 `SourcePool` | U: threaded add/remove storm under a fake callback loop — per-deviceKey quiescence handshake, deferred release + reclaim, no release while in-flight (TSAN job on this target); active-snapshot consistency in `mixForDevice` | I: `multi-source.test.js` + `tests/sandbox` e2e (GP-5 scenario), remove-under-load |
|
||||
| 5 `ExtraInputs` | U: bind rejection matrix (duplicate name, primary device, duplex mode, out-of-range key); transient close keeps intent, permanent unbind clears + deactivates; reopen-failure ghost-source cleanup | I: second-interface e2e; meters zeroed while device gone |
|
||||
| 6 `NapiHelpers` | I (Node, real addon): table-driven arg fuzz per extractor — NaN/Inf/negative/string/missing/object for slot ids, gains, params, midi bytes → no crash, documented no-op or clamp; pins the 0.b clamp decisions | I: addon init→shutdown→init cycle; pending-load cancellation on shutdown |
|
||||
| 7 `ChainOps` | U: serializer — N threads × (loadPreset/loadVST/clearChain) → final chain equals exactly one caller's request, `chainGeneration` strictly monotonic, per-caller result reports the generation it produced | I: 0.b storm test flips to expected-pass; `audio-effects-executor.test.js` extended — executor detects stale generation; rig_builder legacy-path telemetry smoke; editor open/close + sandbox promotion e2e |
|
||||
| 8 bug fixes | U: gain clamp tables (0..32 native, NaN reject); renderer flush flag; `wasRunning` intent read (phase-1 expected-fail flips to pass); latency breakdown terms sum | I: full regression: all snapshots + all suites green |
|
||||
|
||||
Cross-cutting:
|
||||
|
||||
- **TSAN/ASAN lane** for `tests/engine_units` in CI (the ring/pool tests are exactly what
|
||||
sanitizers are for; the RT code has never had one).
|
||||
- **Expected-fail discipline**: known bugs get their test at the phase that creates the home,
|
||||
marked expected-fail with the deep-read § reference; the fix commit flips the mark. No fix
|
||||
lands without its test having existed first.
|
||||
- Existing suites (`audio_sanitize`, `chordscorer`, `mlnotedetector`, `sandbox/*`,
|
||||
`*.test.js`) run on every phase — they are the behavior-freeze net.
|
||||
|
||||
### 6. Phasing & verification
|
||||
|
||||
| Phase | Content | Risk | Gate |
|
||||
|---|---|---|---|
|
||||
| **0** | **Contract snapshots + compat pins + `tests/engine_units` harness + storm test (expected-fail)** | **none (test-only)** | **`contract-check` job green; snapshots committed; harness builds on all 3 platforms** |
|
||||
| 1 | `PackedStereoRing` + `EngineState` (incl. intent/state split behind a facade-compatible `isAudioRunning`) | low | phase-1 unit tests + audio smoke: duplex, split, stream sink, renderer bus each pass audio |
|
||||
| 2 | `RendererBus`, `StreamSink` | low | phase-2 unit tests; renderer-bus metrics unchanged in diag build; OBS capture works |
|
||||
| 3 | `BackingPlayer` | low-med | phase-3 unit tests; backing play/seek/speed e2e; split + duplex |
|
||||
| 4 | `DeviceSetup` | med | helper unit tests; device matrix: WASAPI shared/exclusive, ASIO, dual-type split, probe==apply verdicts |
|
||||
| 5 | `SourcePool` + `ExtraInputs` | med-high | TSAN-clean pool stress; multi-source + second-interface tests (`tests/sandbox` e2e, GP-5 scenario), remove-under-load |
|
||||
| 6 | `AddonContext` + `NapiHelpers` | low | arg-fuzz suite; addon init/shutdown cycles, macOS build |
|
||||
| 7 | `ChainOps` (with serializer) + `EditorWindows` + binding split | med | storm test flips to pass; serializer unit tests; editor open/close, sandbox promotion |
|
||||
| 8 | Bug-fix commits now homed: gain clamps, renderer flush flag, `wasRunning` intent read, latency breakdown | — | each fix flips its pre-existing expected-fail test |
|
||||
|
||||
Every phase additionally requires: `contract-check` green (public surface unchanged) and the
|
||||
full pre-existing suite green.
|
||||
|
||||
Rules that keep this safe:
|
||||
|
||||
- **Move, don't edit**: each phase's diff should be reviewable as "same lines, new file" plus a
|
||||
thin call seam. The excellent existing comments move with their code.
|
||||
- **Reference-bind shared state** (the SourceChain trick) rather than adding getters — keeps the
|
||||
RT paths free of indirection changes.
|
||||
- **No public API change**: NodeAddon exports, IPC channel names, and preload surface stay
|
||||
identical throughout; the collisions-doc ownership work (single chain owner, monitor-state
|
||||
arbiter) is a separate track that starts after phase 7 gives it `chainGeneration`.
|
||||
- Tester diag counters (`audiodiag`, `[asio-diag]`) must survive verbatim — they're how field
|
||||
regressions get caught.
|
||||
|
||||
### 7. Explicit non-goals
|
||||
|
||||
- Rewriting the JS layer (`audio-bridge.ts` 65K / `preload.ts`) — separate track; its shape
|
||||
already mirrors the binding groups this plan creates.
|
||||
- Replacing JUCE transport/BufferingAudioSource, adaptive resampling for split mode, sandbox
|
||||
parameter proxies — feature work, not decomposition.
|
||||
- Renaming slopsmith→feedBack artifacts — orthogonal, and renaming during a move-refactor
|
||||
destroys diff reviewability.
|
||||
|
||||
---
|
||||
|
||||
# Part V — Merged Priority Roadmap
|
||||
|
||||
One ordered list combining the collision remediation (Part II), the deep-read fixes (Part III),
|
||||
and the decomposition phases (Part IV). Decomposition and fixes interleave: each fix lands as a
|
||||
separate commit in the unit that becomes its home.
|
||||
|
||||
1. **Phase 0** — contract snapshots, compat pins (gain bounds, `isAudioRunning` semantics),
|
||||
unit-test harness, concurrency storm test (expected-fail).
|
||||
2. **Refactor phases 1–2** (`PackedStereoRing`, `EngineState` with intent/state split,
|
||||
`RendererBus`, `StreamSink`) — low risk, creates the homes.
|
||||
3. **Fix: renderer-bus flush flag** (III §4) and **gain/slot-id sanitization** (III §2) —
|
||||
small, kills the NaN-master-gain and stale-tail bug classes.
|
||||
4. **Refactor phases 3–5** (`BackingPlayer`, `DeviceSetup`, `SourcePool` + `ExtraInputs`).
|
||||
5. **Fix: `wasRunning` intent read in setAudioDevices** (III §3) — now trivial on the split
|
||||
intent/state atomics.
|
||||
6. **Refactor phases 6–7** (`AddonContext`, `NapiHelpers`, `ChainOps` + serializer +
|
||||
`chainGeneration`, `EditorWindows`, binding split).
|
||||
7. **Ownership work (Part II)** — single chain owner via executor (needs `chainGeneration`),
|
||||
refcounted monitor-state arbiter, one persistence store per setting, tone-switch
|
||||
single-slot replace instead of editor nuke.
|
||||
8. **Long tail** — latency breakdown API, (typeName, name) device identity,
|
||||
slopsmith→feedBack alias deprecation.
|
||||
Reference in New Issue
Block a user