From efd1e41f461299de593d37a623922d57163cad2e Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Thu, 30 Jul 2026 21:54:20 -0400 Subject: [PATCH] instrument: narrow the live-param publish lock to its own mutex, off the reload's decode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Knob-drag publishes no longer block behind a full reload's WAV decode — a dedicated livePublishMutex_ replaces reloadMutex_ for the seqlock's single-writer contract. Also fixes an editor comment overclaim and two doc restatements. --- src/core/instrument/CLAUDE.md | 9 ++------- src/shell/instrument/editor_platform.cpp | 10 +++++++--- src/shell/instrument/processor_reload.cpp | 7 ++++++- src/shell/instrument/processor_state.cpp | 15 ++++++--------- src/shell/instrument/reasampler_processor.h | 5 +++++ tests/test_live_delivery.cpp | 1 - 6 files changed, 26 insertions(+), 21 deletions(-) diff --git a/src/core/instrument/CLAUDE.md b/src/core/instrument/CLAUDE.md index 98e4a70..9665d47 100644 --- a/src/core/instrument/CLAUDE.md +++ b/src/core/instrument/CLAUDE.md @@ -161,13 +161,8 @@ must live compute, latching the parameters at note on is not acceptable. long te automatable parameters."* It rejects the precedent, not one instance of it. - **Which controls are live is ONE decision, recorded in ONE place** — `isLiveDeckParam` and - `liveCommitFor` (`ui/deck_groups`), whose header is the home for why each excluded control is - excluded. Continuous playback controls go live: the six filter tone/modulation knobs, and - every stage time and stage level on all three envelopes. Everything else reloads or drops to - the voice-param rebuild. **FIVE continuous controls are outside the live set** — pitch - key-track, velocity→cutoff depth, and Trigger's %-length, fade-in and fade-out — so a - Trigger-mode instance gets no live delivery on its amplitude controls at all; only the filter - and pitch-envelope knobs move a sounding one-shot. + `liveCommitFor` (`ui/deck_groups`), whose header is THE home for which controls are live and + why each exclusion is excluded — see there rather than restating the list here. - **Ownership sits ABOVE every snapshot.** `SampleData::live` is a NON-OWNING pointer to the one block the shell owns per instance. The member-ordering constraint that enforces it, and why, are recorded at `liveParams_` in `shell/instrument/reasampler_processor.h`. A drain voice diff --git a/src/shell/instrument/editor_platform.cpp b/src/shell/instrument/editor_platform.cpp index 9f0c391..7c2e077 100644 --- a/src/shell/instrument/editor_platform.cpp +++ b/src/shell/instrument/editor_platform.cpp @@ -219,9 +219,13 @@ LRESULT CALLBACK ReaSamplerEditor::wndProc(HWND hwnd, UINT msg, WPARAM wParam, } if (self->drag_ != DragKind::kNone) { // A scrollbar drag + the processor-side deck knobs (preview velocity -2 / - // voice count / master gain) mutate no parameter, so dragStartParams_ is - // not a rollback target for them — reset drag state only. Every - // parameter-editing drag restores the pre-grab snapshot. + // voice count / master gain) mutate no params_ field, so dragStartParams_ + // is not a rollback target for them — reset drag state only. Every + // params_-editing drag restores the pre-grab snapshot. Voice count and + // master gain are pre-existing exceptions to that: both write straight + // through on every move (editor voiceCount_ / processor masterGain_) + // rather than through params_, so an abandoned drag leaves them at the + // abandoned value indefinitely instead of rolling back. const bool transient = self->drag_ == DragKind::kScrollThumb || (self->drag_ == DragKind::kDeckKnob && (self->dragParamId_ == -2 || diff --git a/src/shell/instrument/processor_reload.cpp b/src/shell/instrument/processor_reload.cpp index 57ee09a..84559e4 100644 --- a/src/shell/instrument/processor_reload.cpp +++ b/src/shell/instrument/processor_reload.cpp @@ -154,7 +154,12 @@ std::string ReaSamplerProcessor::reloadInstrument() { // frames the build resolved and a note-on with a live block sounds identical // to one without. sample.live = &liveParams_; - liveParams_.publish(instrument::engine::foldLive(sample.play)); + { + // reloadMutex_ (held for this whole function) nests livePublishMutex_ here; + // publishLiveParams never holds reloadMutex_, so this is the only nesting. + std::lock_guard lp(livePublishMutex_); + liveParams_.publish(instrument::engine::foldLive(sample.play)); + } builtSampleRate_.store(sample.sampleRate, std::memory_order_relaxed); resolvedId = selId; // the concrete pick that resolved } diff --git a/src/shell/instrument/processor_state.cpp b/src/shell/instrument/processor_state.cpp index b08648e..112decf 100644 --- a/src/shell/instrument/processor_state.cpp +++ b/src/shell/instrument/processor_state.cpp @@ -153,17 +153,14 @@ void ReaSamplerProcessor::setInstrumentParams(const InstrumentParams& params) { } void ReaSamplerProcessor::publishLiveParams() { - // reloadMutex_ enforces the block's SINGLE-WRITER contract (live_params.h), not the - // reload's slot bookkeeping: reloadInstrument publishes the block too, and two concurrent - // seqlock writers can leave the generation even mid-write, which a reader would accept as - // a coherent — but torn — block. The audio thread never takes this mutex, so the cost is - // an off-thread wait behind a reload. Lock order matches reloadInstrument's - // (reloadMutex_ then paramsMutex_, taken by instrumentParams below). - std::lock_guard lock(reloadMutex_); const int rate = builtSampleRate_.load(std::memory_order_relaxed); if (rate <= 0) return; - liveParams_.publish( - instrument::engine::foldLive(resolvePlay(instrumentParams().play, rate))); + const instrument::engine::LiveValues block = + instrument::engine::foldLive(resolvePlay(instrumentParams().play, rate)); + // livePublishMutex_ enforces the seqlock's single-writer contract (live_params.h) against + // reloadInstrument's publish — held for the publish call only, not the fold above. + std::lock_guard lock(livePublishMutex_); + liveParams_.publish(block); } SampleRefs ReaSamplerProcessor::sampleRefs() { diff --git a/src/shell/instrument/reasampler_processor.h b/src/shell/instrument/reasampler_processor.h index 7524b64..3e0f7b9 100644 --- a/src/shell/instrument/reasampler_processor.h +++ b/src/shell/instrument/reasampler_processor.h @@ -239,6 +239,11 @@ private: // Both live_ and draining_ observe this same block — a block owned by a snapshot would // leave the drain's still-sounding voices deaf to the knob under them. instrument::engine::LiveParams liveParams_; + // Serializes liveParams_.publish's two writer sites (reloadInstrument, publishLiveParams) + // only — separate from reloadMutex_ so a knob drag's publish never blocks behind a + // reload's WAV decode. The audio thread never takes this; process() only reads via + // LiveParams::read's lock-free seqlock retry. + std::mutex livePublishMutex_; // The rate the loaded capture was decoded/built at, so a live republish resolves the // stored wall-clock seconds to exactly the frames the built SampleData carries. 0 = nothing // built yet. diff --git a/tests/test_live_delivery.cpp b/tests/test_live_delivery.cpp index 89b7527..2f5b30c 100644 --- a/tests/test_live_delivery.cpp +++ b/tests/test_live_delivery.cpp @@ -63,7 +63,6 @@ static SampleData filteredSine() { s.play.filter.settings.cutoffNorm = 0.8f; s.play.filter.settings.resonanceNorm = 0.9f; s.play.filter.settings.morphNorm = 1.0f; - s.play.filter.env.sustainLevel = 1.0; return s; }