From eb40b87deaf7e408188c916f0d4a3ec5e04e5991 Mon Sep 17 00:00:00 2001 From: OmikronApex Date: Mon, 13 Jul 2026 23:42:29 +0200 Subject: [PATCH] refactor(audio): extract EngineState with intent/state split (phase 1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Moves the shared run-state atomics (currentSampleRate, block sizes, duplexMode, run flags) into slopsmith::EngineState (src/audio/engine/) so later extracted units take EngineState& and stay unit-testable without JUCE devices. AudioEngine binds the members back by reference under their historical names — zero call-site churn, behavior-identical. The old audioRunning conflated user intent with device state (deep-read §3/§6); it is now state.deviceRunning (same semantics, isAudioRunning compat pinned) plus a new state.userWantsAudio written only by startAudio/stopAudio. Nothing reads the intent flag yet — phase 8 flips setAudioDevices' restart decision onto it. Co-Authored-By: Claude Fable 5 --- src/audio/AudioEngine.cpp | 5 +++ src/audio/AudioEngine.h | 31 ++++++------- src/audio/engine/EngineState.h | 55 ++++++++++++++++++++++++ tests/engine_units/CMakeLists.txt | 4 ++ tests/engine_units/engine_state_test.cpp | 53 +++++++++++++++++++++++ 5 files changed, 130 insertions(+), 18 deletions(-) create mode 100644 src/audio/engine/EngineState.h create mode 100644 tests/engine_units/engine_state_test.cpp diff --git a/src/audio/AudioEngine.cpp b/src/audio/AudioEngine.cpp index 8100f15..d0fba8f 100644 --- a/src/audio/AudioEngine.cpp +++ b/src/audio/AudioEngine.cpp @@ -1188,6 +1188,10 @@ void AudioEngine::teardownSplitMode() void AudioEngine::startAudio() { + // Intent flag first — even if the device open below fails or a transient + // stop races in, "the user wants audio" survives (read by phase 8's + // setAudioDevices restart fix; see EngineState.h). + state.userWantsAudio.store(true, std::memory_order_relaxed); if (audioRunning.load(std::memory_order_relaxed)) { fprintf(stderr, "[AudioEngine] startAudio: already running\n"); @@ -1244,6 +1248,7 @@ void AudioEngine::startAudio() void AudioEngine::stopAudio() { + state.userWantsAudio.store(false, std::memory_order_relaxed); if (slopsmith_vst_trace::isEnabled()) fprintf(stderr, "[diag] stopAudio: audioRunning=%d inputCbReg=%d outputCbReg=%d\n", (int) audioRunning.load(std::memory_order_relaxed), diff --git a/src/audio/AudioEngine.h b/src/audio/AudioEngine.h index 3382ade..4933777 100644 --- a/src/audio/AudioEngine.h +++ b/src/audio/AudioEngine.h @@ -2,6 +2,7 @@ #include "SourceChain.h" #include "GainSanitize.h" #include "engine/PackedStereoRing.h" +#include "engine/EngineState.h" #include "BackingLeveler.h" #include "signalsmith-stretch.h" #include @@ -444,7 +445,12 @@ private: // with an SPSC ring between them. juce::AudioDeviceManager inputDeviceManager; juce::AudioDeviceManager outputDeviceManager; - std::atomic duplexMode{true}; + + // Shared run-state atomics (TLC phase 1) — the members below are + // reference aliases under their historical names so call sites are + // untouched; extracted units take `state` (EngineState&) directly. + slopsmith::EngineState state; + std::atomic& duplexMode = state.duplexMode; // Per-input capture+detect+monitor chains. A FIXED pool, all constructed up // front, so adding/removing a source never reassigns a pointer the audio @@ -538,23 +544,12 @@ private: std::atomic backingSpeedChangePending{false}; juce::CriticalSection backingLock; - // Toggled from startAudio()/stopAudio() (main / device-management - // threads) and read from isAudioRunning() on the JS thread via the - // audio-bridge dispatch loop. Plain bool would be a data race; - // relaxed-atomic is well-defined and compiles to a plain MOV. - std::atomic audioRunning{false}; - // Sample rate is written from the JUCE device callbacks (audio - // thread / device-management thread) and read from arbitrary - // callers including the JS thread via getCurrentSampleRate(), - // so a plain double would be a C++ data race. std::atomic - // is well-defined and lock-free on the platforms we ship; the - // hot reads use relaxed since the consumer just wants the latest - // observable value, not a synchronization point. - std::atomic currentSampleRate{48000.0}; - // Split mode allows different input vs output block sizes; the ring absorbs - // the asymmetry. DSP prepares against input; backing resampler against output. - std::atomic inputBlockSize{256}; - std::atomic outputBlockSize{256}; + // audioRunning keeps its historical DEVICE-STATE semantics (isAudioRunning + // compat pin); the intent half is state.userWantsAudio — see EngineState.h. + std::atomic& audioRunning = state.deviceRunning; + std::atomic& currentSampleRate = state.currentSampleRate; + std::atomic& inputBlockSize = state.inputBlockSize; + std::atomic& outputBlockSize = state.outputBlockSize; // The per-input lock-free SPSC rings (pre-gate getInputFrame ring + post-gate // getRawAudioFrame ring), the YIN/ML detectors, and the zero-output capture diff --git a/src/audio/engine/EngineState.h b/src/audio/engine/EngineState.h new file mode 100644 index 0000000..7dcc1ef --- /dev/null +++ b/src/audio/engine/EngineState.h @@ -0,0 +1,55 @@ +#pragma once + +// EngineState — the audio engine's shared run-state atomics (TLC plan +// phase 1 / §2.8). Every extracted engine unit takes an EngineState& instead +// of reaching back into AudioEngine, which is what keeps them unit-testable +// without JUCE devices. AudioEngine itself binds these members by reference +// under their historical names, so existing call sites are untouched. +// +// The deliberate fix homed here (deep-read §3/§6): the old single +// `audioRunning` flag conflated USER INTENT ("the user pressed Start") with +// DEVICE STATE ("a device callback is live") — audioDeviceStopped() clears it +// on transient stops (WASAPI exclusive opens routinely fire one mid-start), +// so setAudioDevices' restart decision read a racy answer. The two are now +// separate atomics: +// +// userWantsAudio — intent. Written ONLY by startAudio()/stopAudio(). +// deviceRunning — state. Written by startAudio()/stopAudio() AND the +// device callbacks (aboutToStart/stopped), i.e. the exact +// semantics the old audioRunning had. isAudioRunning() +// keeps reporting THIS one (Phase 0.b compat pin). +// +// Until the phase-8 fix, nothing reads userWantsAudio — writing it here first +// keeps that later commit a one-line read-side change in setAudioDevices. + +#include + +namespace slopsmith { + +struct EngineState +{ + // Sample rate is written from the JUCE device callbacks (audio thread / + // device-management thread) and read from arbitrary callers including the + // JS thread, so a plain double would be a C++ data race. atomic + // is lock-free on the platforms we ship; hot reads use relaxed since the + // consumer just wants the latest observable value, not a sync point. + std::atomic currentSampleRate{48000.0}; + // Split mode allows different input vs output block sizes; the ring + // absorbs the asymmetry. DSP prepares against input; backing resampler + // against output. + std::atomic inputBlockSize{256}; + std::atomic outputBlockSize{256}; + // Duplex: one device manager owns both directions. Split: input-only + + // output-only managers with an SPSC ring between them. + std::atomic duplexMode{true}; + + // Intent: the user asked for audio to run. start/stopAudio only. + std::atomic userWantsAudio{false}; + // State: toggled from startAudio()/stopAudio() (main/device-management + // threads) and the device callbacks, read from isAudioRunning() on the JS + // thread. Plain bool would be a data race; relaxed-atomic compiles to a + // plain MOV. + std::atomic deviceRunning{false}; +}; + +} // namespace slopsmith diff --git a/tests/engine_units/CMakeLists.txt b/tests/engine_units/CMakeLists.txt index 88447f1..b56b047 100644 --- a/tests/engine_units/CMakeLists.txt +++ b/tests/engine_units/CMakeLists.txt @@ -12,3 +12,7 @@ target_compile_features(packed_stereo_ring_test PRIVATE cxx_std_20) find_package(Threads REQUIRED) target_link_libraries(packed_stereo_ring_test PRIVATE Threads::Threads) add_test(NAME packed_stereo_ring COMMAND packed_stereo_ring_test) + +add_executable(engine_state_test engine_state_test.cpp) +target_compile_features(engine_state_test PRIVATE cxx_std_17) +add_test(NAME engine_state COMMAND engine_state_test) diff --git a/tests/engine_units/engine_state_test.cpp b/tests/engine_units/engine_state_test.cpp new file mode 100644 index 0000000..4b5e882 --- /dev/null +++ b/tests/engine_units/engine_state_test.cpp @@ -0,0 +1,53 @@ +// Phase 1 unit test for EngineState (docs/audio-engine-tlc.md §5): the +// intent/state transition table. Mirrors how AudioEngine drives the two +// flags — startAudio/stopAudio write BOTH (intent + state), the device +// callbacks write deviceRunning ONLY — and pins the Phase 0.b compat +// decision that isAudioRunning() reports DEVICE STATE: a transient device +// stop flips it false even though the user never pressed Stop. + +#include "../../src/audio/engine/EngineState.h" + +#include +#include + +using slopsmith::EngineState; + +// The write sets, as AudioEngine performs them. +static void userStart(EngineState& s) { s.userWantsAudio.store(true); s.deviceRunning.store(true); } +static void userStop(EngineState& s) { s.userWantsAudio.store(false); s.deviceRunning.store(false); } +static void deviceAboutToStart(EngineState& s) { s.deviceRunning.store(true); } +static void deviceStopped(EngineState& s) { s.deviceRunning.store(false); } +// isAudioRunning() facade == deviceRunning (compat pin). +static bool isAudioRunning(const EngineState& s) { return s.deviceRunning.load(); } + +int main() +{ + EngineState s; + assert(!s.userWantsAudio.load() && !isAudioRunning(s)); + + // User starts audio. + userStart(s); + assert(s.userWantsAudio.load() && isAudioRunning(s)); + + // Transient device stop (WASAPI exclusive mid-start hiccup): device state + // drops, intent survives — this is the split that fixes deep-read §3. + deviceStopped(s); + assert(s.userWantsAudio.load() && "transient stop must not erase user intent"); + assert(!isAudioRunning(s) && "compat pin: isAudioRunning reports device state"); + + // JUCE auto-restart brings the device back without user action. + deviceAboutToStart(s); + assert(s.userWantsAudio.load() && isAudioRunning(s)); + + // Explicit user stop clears both. + userStop(s); + assert(!s.userWantsAudio.load() && !isAudioRunning(s)); + + // A stray device start (auto-restart after user stop) must not fabricate + // intent: device state true, intent still false. + deviceAboutToStart(s); + assert(!s.userWantsAudio.load() && isAudioRunning(s)); + + std::puts("engine_state: all transitions passed"); + return 0; +}