diff --git a/docs/COMPLETED.md b/docs/COMPLETED.md index 4bd0834..72644ba 100644 --- a/docs/COMPLETED.md +++ b/docs/COMPLETED.md @@ -6,6 +6,24 @@ original Goal, Verify, and checklist points with boxes marked done. This file holds the current (1.x) cycle's landed milestones only. For all pre-1.0 (version-0) history, see `docs/ARCHIVE.md`. +### Decouple the instrument reload from VST3 activation (filed follow-up, discharged in Γ-W3) + +`ReaSamplerProcessor::setActive` meant two things at once — "the audio thread may run" and "the +decoded `SampleData` is (re)built" — so every host-driven activation cycle paid a bridge read +and a full WAV decode that nothing about activation required. The two lifetimes are now +separate: `setActive(false)` parks the decoded sample and destroys the voice state, +`setActive(true)` rebuilds the voices around the parked sample through the drain-slot swap +`rebuildVoiceEngine` already used for voice-count edits. A cycle costs no disk I/O and no +decode; sounding voices are still destroyed across it (a surviving `live_` would be displaced +into the drain slot and resurrect stale sustained voices as ghosts); an instance with nothing +decoded still routes through the full reload, which is where the pre-v10 legacy lift lives; and +`getLatencySamples()` still answers from the persisted enable, untouched by the cycle. The +build shared by the reload, the voice-param rebuild and the reactivation was factored to one +site so the three cannot drift on the generation stamp or the ring size. Daniel reversed the +deferral (*"I thought we agreed to decouple the unnecessary functions from the reactivation +path"*); Γ-F2 and Γ-F6 are untouched — dynamic latency ships, the deactivate/reactivate is +still the accepted cost of the toggle, just a much cheaper one. + ### Comment-reduction pass (tree-wide, twelve parallel tracks) Cut source comment volume tree-wide: 209 files changed, net **−6,493** lines. diff --git a/docs/TODO.md b/docs/TODO.md index 40cdf4f..922f0ce 100644 --- a/docs/TODO.md +++ b/docs/TODO.md @@ -240,57 +240,33 @@ alpha and this entry is re-filed against the new value. **Nothing here is actionable as a TODO.** Delete this entry when Γ-W1-T1 lands. -## Decouple the instrument reload from VST3 activation +## The editor's drag state machine has no seam, and `reasampler_editor.h` is near the ceiling -**Context (Daniel, 2026-08-01 — Phase Γ fork Γ-F6, ruled closed).** Γ-W1-T2 ships the plugin's -first latency reporting: `getLatencySamples()` returns 0 with the limiter off and the lookahead -with it on, and the toggle calls `IComponentHandler::restartComponent(kLatencyChanged)`. The -vendored SDK defines that flag as a host **deactivate/reactivate** -(`pluginterfaces/vst/ivsteditcontroller.h:105-108`). **Dynamic latency reporting is routine for -VST3 instruments and REAPER handles it as a matter of course** — the deactivate/reactivate is -the normal contract, and for a typical plugin `setActive` only allocates and frees buffers. -Γ-F6 was originally posed as "is this SDK cost acceptable?"; Daniel's answer relocated it: -*"you have to have missed something, I used plenty of VST3s inside of REAPER that report PDC -dynamically... Toggling the limiter killing the voices isn't a deal breaker though, the limiter -will either be on or off on its instance, toggling during playback is not a use case."* +**Context (Γ-W3, meter re-review).** `reasampler_editor.h` stands at **563 lines** against the +~600-line ceiling — 37 lines of margin — and it keeps growing because every new surface on the +Sample face adds its transient state there. The obvious seam is the drag state machine: `drag_` +plus the per-gesture anchors it is read against. -**The wart — and it is ours, not the SDK's.** `ReaSamplerProcessor::setActive(true)` calls -`reloadInstrument()` (`src/shell/instrument/reasampler_processor.cpp:89-97`) — a bridge read -plus a **full WAV re-decode** plus a fresh engine. `setActive(false)` frees `live_`, -`draining_` and the graveyard (`:98-107`). So every host-driven activation cycle — a -latency-change restart, an offline-render bracket, any host that deactivates around transport -state — pays a disk read and a decode that nothing about activation requires. **Activation -currently means two things at once**: "the audio thread may run" and "the decoded `SampleData` -is (re)built." Dynamic latency is simply the first feature that makes the cycle -user-triggerable. +**Why it was declined rather than taken.** `drag_` has **42 references across 13 shell TUs** +(measured over `src/shell/instrument/*.cpp`; the declaration in the header is additional), and +every input TU both writes it and branches on it. Extracting it is a real refactor of the +editor's input half, not a header move — and doing it inside a wave whose subject is the MASTER +deck would have put an unrelated high-blast-radius change in the same diff. Declining was right; +leaving it unrecorded was not. -**Intended fix.** Separate the two lifetimes: keep the decoded `SampleData` alive across a -deactivate and rebuild only the voice state on reactivate. The mechanism already exists in this -file — `rebuildVoiceEngine` performs exactly that shape (drain-slot swap around the -already-decoded `SampleData`, no bank re-read, no WAV re-decode) for voice-count and voice-mode -edits. This is a lifetime split, not a new mechanism. +**The shape a fix would take.** A `DragState` type owning the kind plus its anchor payload, +with the input TUs mutating it through named transitions rather than assigning `drag_` and its +anchors independently — which is also what would let the invariant "an anchor is only readable +while its own `DragKind` is in flight" be enforced rather than observed. `editor_interaction.h` +already holds the `DragKind` vocabulary and is the natural home. -**The constraint the fix MUST handle.** The deactivate's destruction is deliberate and its -reason is documented at the call site: a surviving `live_` would be displaced into the drain -slot on reactivate and *"resurrect stale sustained voices as ghosts."* **Voice state must still -die across the cycle** — only the decoded PCM survives, and those are two different lifetimes -currently collapsed into one. Second constraint: `setActive(true)` is also the non-editor -legacy-lift trigger for a pre-v10 blob (its opportunistic `refreshRefsFromBank` copies refs in -once the bank blob is readable), so a path that skips the bridge read must keep that lift -reachable — the comment at `:90-96` records the residual load-order race it exists to cover. +**Priority / risk.** Low, but the margin is the clock: the next surface that adds two members to +the header takes it over the ceiling, and at that point the seam gets chosen under time pressure +by whoever is unlucky. Take it before that, not after. -**Priority / risk.** Low; deferred by ruling. Nothing is incorrect today, only wasteful, and -Daniel has explicitly accepted the user-visible consequence (held notes cut on a limiter -toggle). **Trigger conditions — revisit when any one of these holds:** (a) a second -latency-changing control appears, so the cycle stops being a once-per-patch event; (b) the -limiter enable is ever wanted automatable, which `docs/product/parameter-automation.md` §3.8 -currently forbids *because* of this cost; or (c) the re-decode is observed to be perceptible in -REAPER — Γ-W1-T2's review records that observation for exactly this purpose. - -**Done looks like.** A host-driven deactivate/reactivate cycle costs no disk I/O and no WAV -decode; sounding voices are still destroyed across it, with no ghost-resurrection regression; -a pre-v10 blob still lifts; and `getLatencySamples()` still derives from persisted state rather -than from a transient the deactivate cleared. +**Done looks like.** `reasampler_editor.h` is back under the ceiling with room; no TU assigns +`drag_` and an anchor as two independent writes; and the transitions are named where the +`DragKind` catalogue already lives. ## `Sample::sourceMode` has no value meaning "produced by the instrument" diff --git a/docs/product/instrument-control-surface.md b/docs/product/instrument-control-surface.md index a2fdb58..2a93607 100644 --- a/docs/product/instrument-control-surface.md +++ b/docs/product/instrument-control-surface.md @@ -50,9 +50,8 @@ own width formula, not carried over from a prior measurement. The stale geometry when off, the lookahead when on, reported to the host's PDC. This is **routine VST3 behaviour**; the `restartComponent(kLatencyChanged)` it costs is the normal contract, and the deactivate/reactivate the flag mandates is **accepted** — the toggle is a patch-design - gesture. The only reason the cycle is expensive at all is that **our** `setActive` re-decodes - the WAV, which is a latent improvement filed in `docs/TODO.md`, not a design constraint. - §3.1.1. + gesture. The cycle used to be expensive only because **our** `setActive` re-decoded the WAV; + Γ-W3 decoupled the two lifetimes, so it no longer does. §3.1.1. - **The cortex limiter does not clear the bar** — §3.5. Read it, take nothing. - **Loop gets an explicit enable on the chrome row** (Γ-F4), and the four-mark grammar sits under it. The core finding behind the re-approach: three identical bars draw a @@ -560,31 +559,28 @@ plugins — lookahead limiters, linear-phase EQs and oversampling processors all REAPER handles it as a matter of course. The deactivate/reactivate is the *normal* cost of the flag, and for a typical plugin it is cheap: `setActive` allocates and frees buffers. -**What makes it expensive here is entirely our own design, in one line.** -`ReaSamplerProcessor::setActive` is deliberately destructive in both directions -(`reasampler_processor.cpp:85-109`): +**What made it expensive here was entirely our own design, in one line** — and Γ-W3 removed +that line. `ReaSamplerProcessor::setActive` was deliberately destructive in both directions: -- `setActive(true)` calls `reloadInstrument()` (`:89-97`) — **a bridge read and a full WAV - re-decode**, plus a fresh engine. This is the expensive half, and no part of it is required - by the SDK: it is there because activation was the convenient trigger for a reload, not - because activation implies one. -- `setActive(false)` frees `live_`, `draining_` **and** the graveyard (`:98-107`), so every - sounding voice dies. The comment there explains why that is correct and must not be - softened casually: a surviving `live_` would be displaced into the drain slot on reactivate - and *"resurrect stale sustained voices as ghosts."* +- `setActive(true)` called `reloadInstrument()` — **a bridge read and a full WAV re-decode**, + plus a fresh engine. That was the expensive half, and no part of it was required by the SDK: + it was there because activation was the convenient trigger for a reload, not because + activation implies one. +- `setActive(false)` frees `live_`, `draining_` **and** the graveyard, so every sounding voice + dies. That half is correct and must not be softened casually: a surviving `live_` would be + displaced into the drain slot on reactivate and *"resurrect stale sustained voices as + ghosts."* -**So the cost is ours, and it is ours to reduce.** The reduction is **decoupling the reload -from activation** — keeping the decoded `SampleData` alive across a deactivate while still -destroying voice state, which is exactly the shape `rebuildVoiceEngine`'s drain-slot swap -already implements for voice-count edits. **That is a latent improvement with a clear trigger -condition, filed in `docs/TODO.md` ("Decouple the instrument reload from VST3 activation") — -not a reason to abandon dynamic latency, and not scheduled in this phase.** +**The cost was ours, and it has been reduced (Γ-W3 — see §7.11).** The deactivate now parks the +decoded `SampleData` and the reactivate rebuilds only the voice state around it, through the +same drain-slot swap `rebuildVoiceEngine` uses for voice-count edits. An activation cycle costs +no disk read and no decode; an instance with nothing decoded still takes the full reload, which +is where the pre-v10 legacy lift lives. -**The honest cost of the toggle today, stated plainly:** every sounding note stops and the -sample is re-decoded from disk. **Daniel has accepted it** (Γ-F6): *"Toggling the limiter -killing the voices isn't a deal breaker though, the limiter will either be on or off on its -instance, toggling during playback is not a use case."* There is no fallback design and no -measurement gate. +**The honest cost of the toggle, stated plainly:** every sounding note stops. **Daniel has +accepted it** (Γ-F6): *"Toggling the limiter killing the voices isn't a deal breaker though, +the limiter will either be on or off on its instance, toggling during playback is not a use +case."* There is no fallback design and no measurement gate. #### The standing scar, and why this is nonetheless not the forbidden change @@ -660,10 +656,9 @@ What is in scope alongside it — and what each is actually for: in the **not-automatable** class, and it is emphatically not the plugin's `kIsBypass` parameter either. - **Observe what REAPER does, and record it — as evidence, not as a gate.** Whether notes - cut, whether the re-decode is perceptible, whether transport hiccups, is DAW-observable - only. Record it in Γ-W1-T2's review because it is the trigger-condition evidence for the - `docs/TODO.md` decoupling entry. **No outcome changes the design**; Γ-F6 is closed either - way. + cut and whether transport hiccups is DAW-observable only. The re-decode half of that + question is gone (§7.11), so what remains to observe is the voice cut alone. **No outcome + changes the design**; Γ-F6 is closed either way. ### 3.2 The meter @@ -1401,13 +1396,18 @@ squarely on `ReaSamplerProcessor::setActive`, which is deliberately destructive directions. **Those four are hygiene against the `kIoChanged` scar (§3.1.1), not a hedge against the flag itself** — Γ-F6 is ruled and the restart ships. -**7.11 — `setActive` conflates two lifetimes, and dynamic latency is the first feature that -makes a user notice.** Activation currently means both "the audio thread may run" and "the -decoded `SampleData` is (re)built" (`reasampler_processor.cpp:89-97`). Phase Γ does **not** -separate them — Γ-F6 accepts the cost — but the conflation is now a named, filed improvement -(`docs/TODO.md`, "Decouple the instrument reload from VST3 activation") rather than an -unremarked property. **Do not restructure `setActive` inside this phase**; its destructive -shape is deliberate and its reasoning is documented at the call site. +**7.11 — `setActive` conflated two lifetimes; it no longer does (LANDED, Γ-W3).** Activation +used to mean both "the audio thread may run" and "the decoded `SampleData` is (re)built", so +every host-driven cycle paid a bridge read and a full WAV decode. The two are now separate: +`setActive(false)` parks the decoded sample and destroys the voice state, `setActive(true)` +rebuilds the voices around the parked sample through the drain-slot swap `rebuildVoiceEngine` +already used. **This section's earlier instruction — "do not restructure `setActive` inside +this phase" — was superseded by Daniel's ruling that this track does it**; the deactivate's +destruction of voice state is still deliberate (a surviving `live_` would resurrect stale +sustained voices as ghosts) and only the PCM survives. Nothing parked routes the activation +back through the full reload, which is what keeps the pre-v10 legacy lift reachable. Γ-F6 is +untouched: dynamic latency ships and the deactivate/reactivate is still the accepted cost — +it is simply a much cheaper one. --- @@ -1427,7 +1427,7 @@ ceiling. | **Γ-F3** | Does the log taper raise the 2 s stage-time ceiling? | **REVERSED, same day. Ruled first "not in this phase — stays 2.0 s"; then Daniel: _"extend the stage lengths to 10s."_ The ceiling moves 2.0 → 10.0 in Γ-W1-T1.** The reversal's cause is Ruling 1: parameters now ship in-phase, so the ceiling is a one-way door that has to be walked through *before* them. | **§4.3.1** (new), §4.3; `docs/TODO.md` entry discharged | | **Γ-F4** | Explicit loop enable? | **Yes — on the CHROME ROW.** Not a deck cell; loop is a waveform-overlay concept and has no deck. | **§6.4** (new), §6.5, §7.9 | | **Γ-F5** | MASTER's reserved slot: one cell or two? | **One cell.** Two would spend 60 of the 82 px headroom on an unnamed control and freeze row 1 forever. | **§1.6** (new), §1.4 | -| **Γ-F6** | Is the `kLatencyChanged` deactivate/reactivate acceptable as the cost of the toggle? | **Yes — ship dynamic latency as ruled.** No constant-latency fallback, no measurement gate. *Corrected this doc's analysis: the cost is self-inflicted, not SDK-imposed.* | **§3.1.1** (rewritten), §7.10, §7.11, `docs/TODO.md` | +| **Γ-F6** | Is the `kLatencyChanged` deactivate/reactivate acceptable as the cost of the toggle? | **Yes — ship dynamic latency as ruled.** No constant-latency fallback, no measurement gate. *Corrected this doc's analysis: the cost is self-inflicted, not SDK-imposed.* | **§3.1.1** (rewritten), §7.10, §7.11; `docs/TODO.md` decoupling entry discharged in Γ-W3 | | **Γ-F7** | VST3 parameter ORDER: signal flow, or the editor's visual rows? | **Signal flow** — *"signal flow order."* The frozen id numbering and the presentation index both follow the deck's own rule; the visual layout is too mobile to freeze against. | **§8.3**; `parameter-automation.md` §6.4 (argument) and §6.2 (the 44-id table) | Three of these corrected this doc rather than confirming it, and all three corrections are @@ -1473,7 +1473,8 @@ reintroduced: than just counting: 1. **§3.1.1 was rewritten, not annotated.** Its prior framing — dynamic latency as exotic and - expensive — was wrong. Dynamic PDC is routine; the expense is our reload-on-activate. + expensive — was wrong. Dynamic PDC is routine; the expense was our reload-on-activate, and + Γ-W3 removed it (§7.11). 2. **The measurement gate was dropped.** Γ-W1-T2's first deliverable is the limiter, not a spike. What remains is an *observation* recorded in review as evidence for the deferred improvement — it gates nothing. diff --git a/docs/product/parameter-automation.md b/docs/product/parameter-automation.md index 2f056b2..0fed5e7 100644 --- a/docs/product/parameter-automation.md +++ b/docs/product/parameter-automation.md @@ -202,8 +202,8 @@ The consequence for this doc is concrete and it is a **subtraction from the para > latency, and the vendored SDK defines `restartComponent(kLatencyChanged)` as *"the host > has to deactivate and reactivate the plug-in"* > (`pluginterfaces/vst/ivsteditcontroller.h:105-108`). In this plugin a deactivate frees -> every sounding voice and a reactivate re-decodes the WAV. **An automation lane toggling -> that parameter would deactivate the plugin on every flip.** +> every sounding voice. **An automation lane toggling that parameter would deactivate the +> plugin on every flip.** Two corollaries the parameter work must carry rather than rediscover: @@ -211,7 +211,7 @@ Two corollaries the parameter work must carry rather than rediscover: binding it to `kIsBypass` would hand the host a control that restarts the component. - **Latency reporting must be derived from persisted state, not from a transient.** The SDK states the new latency is what `getLatencySamples` returns *after* `setActive(true)` — and - this plugin's `setActive(false)` frees essentially everything. Whatever holds the limiter + this plugin's `setActive(false)` destroys the whole voice state. Whatever holds the limiter flag must survive that cycle. Full reasoning, the SDK quotes, and the required verification steps are in @@ -221,12 +221,13 @@ There is no constant-reported-latency fallback — that option is closed, not sh **this section does not shrink to a footnote and the limiter enable does not become automatable.** Plan against the not-automatable classification; it is settled. -**One future condition could reopen it, and it is worth knowing about.** The restart is only -expensive because *this plugin's* `setActive(true)` re-decodes the WAV — not because the SDK -requires it. `docs/TODO.md` ("Decouple the instrument reload from VST3 activation") files that -reduction, and **"the limiter enable is wanted automatable" is one of its named trigger -conditions.** If the parameter work genuinely needs that lane, the answer is to do the -decoupling first, not to re-litigate the classification. +**The decoupling that was filed against this section has LANDED (Γ-W3), and it changes the +cost but not the classification.** `setActive(true)` no longer re-decodes the WAV: the decoded +sample now survives a deactivate and only the voice state is rebuilt +(`instrument-control-surface.md` §7.11). So a flip costs a voice rebuild rather than a disk +read plus a decode — but **the deactivate still frees every sounding voice**, which is the +ground the not-automatable classification actually rests on. Plan against not-automatable; if +the parameter work wants that lane, the question to answer is the voice cut, not the decode. --- diff --git a/src/core/instrument/engine/CMakeLists.txt b/src/core/instrument/engine/CMakeLists.txt index 4954cab..9c917b0 100644 --- a/src/core/instrument/engine/CMakeLists.txt +++ b/src/core/instrument/engine/CMakeLists.txt @@ -87,3 +87,9 @@ reasampler_test(limiter LINK limiter) reasampler_pure_library(meter_ballistics SOURCES meter_ballistics.cpp) reasampler_test(meter_ballistics LINK meter_ballistics) + +# The meter's ACCUMULATE half, beside the ballistics that consume it. Header-only (the folds +# sit on the audio thread's per-block path), hence INTERFACE. +add_library(meter_accumulate INTERFACE) +target_include_directories(meter_accumulate INTERFACE ${REASAMPLER_SRC_DIR}) +reasampler_test(meter_accumulate LINK meter_accumulate) diff --git a/src/core/instrument/engine/meter_accumulate.h b/src/core/instrument/engine/meter_accumulate.h new file mode 100644 index 0000000..27de0b0 --- /dev/null +++ b/src/core/instrument/engine/meter_accumulate.h @@ -0,0 +1,66 @@ +// meter_accumulate.h — the master meter's ACCUMULATE half: the audio thread's block-rate fold +// into the two windows the UI drains, and the drain that starts the next window. The ballistics +// that run on what comes out are meter_ballistics'. Header-only — the folds sit on the audio +// thread's per-block path. Templated on the accumulator ONLY so the drain-inside-the-fold +// interleave below can be pinned deterministically instead of raced for. + +#pragma once + +#include + +namespace reasampler::instrument::engine { + +// A lock-backed std::atomic would put a mutex on the audio thread; assert the freedom +// rather than assume it. +static_assert(std::atomic::is_always_lock_free, + "the meter folds run on the audio thread and must be lock-free"); + +// The two windows' identity elements: a peak window that has seen nothing reports silence, a +// gain window that has seen nothing reports no reduction. They are what a consume reinstalls, +// so they live beside the folds rather than at the reader. +inline constexpr float kMeterPeakIdentity = 0.f; +inline constexpr float kMeterGainIdentity = 1.f; + +// Folds one block's reading into its accumulator — a running max for a peak, a running min for +// the limiter's gain — so the ~47 blocks that elapse between two 500 ms UI frames at 48 kHz/512 +// all reach the meter instead of the one it happened to sample. +// +// An UNCONDITIONAL read-modify-write, and that is the whole point. The UI's consume is an +// exchange that can land between a plain load and its store, and a load-compare-store fold +// would then drop the block outright: it decided against storing by comparing with a window the +// UI has since taken, so that block's reading enters neither the old window nor the new one. +// The CAS retries against whatever the consume left, which makes `acc >= blockPeak` hold on +// exit however the two interleave. Bounded — the audio thread is this accumulator's only other +// writer, so one interfering consume costs one retry. Relaxed throughout: the accumulators are +// advisory and order no other state. Block rate, never per frame. +template +inline void foldPeak(Accumulator& acc, float blockPeak) { + float seen = acc.load(std::memory_order_relaxed); + while (!acc.compare_exchange_weak(seen, seen > blockPeak ? seen : blockPeak, + std::memory_order_relaxed, + std::memory_order_relaxed)) { + } +} + +template +inline void foldMinGain(Accumulator& acc, float blockMinGain) { + float seen = acc.load(std::memory_order_relaxed); + while (!acc.compare_exchange_weak(seen, seen < blockMinGain ? seen : blockMinGain, + std::memory_order_relaxed, + std::memory_order_relaxed)) { + } +} + +// Takes what the window accumulated and reinstalls the identity element, which IS what starts +// the next window — so exactly one reader may consume (the shell's MasterBusMeter states who). +template +inline float consumePeak(Accumulator& acc) { + return acc.exchange(kMeterPeakIdentity, std::memory_order_relaxed); +} + +template +inline float consumeMinGain(Accumulator& acc) { + return acc.exchange(kMeterGainIdentity, std::memory_order_relaxed); +} + +} // namespace reasampler::instrument::engine diff --git a/src/core/instrument/ui/master_meter.h b/src/core/instrument/ui/master_meter.h index 5b1378e..a746af4 100644 --- a/src/core/instrument/ui/master_meter.h +++ b/src/core/instrument/ui/master_meter.h @@ -58,7 +58,7 @@ struct MasterMeterUi { double reductionDb = 0.0; // how far the limiter is pulling gain down; 0 = not working // The lamp's hold, on the SAME principle (and the same window) as the peak tick's: without // it a catch smaller than kMeterFallDbPerSecond x the UI period is fully decayed by the - // next frame and the lamp never draws lit at all. + // next frame, so the lamp is dark again after the single repaint the catch landed on. double reductionHoldSeconds = 0.0; }; @@ -91,8 +91,8 @@ bool meterDrawEqual(const MasterMeterUi& a, const MasterMeterUi& b); // The lamp reports "the limiter is working", not "a sample grazed the threshold", so it needs // a floor rather than a bare non-zero test. 0.5 dB is a CHOSEN floor, not a measurement — the -// spec asks only for "a small floor". Lowering it makes the lamp flicker on limiting too slight -// to hear; raising it hides genuine catches, since the limiter's ceiling is only −0.3 dBTP. +// spec asks only for "a small floor". Raising it hides genuine catches, since the limiter's +// ceiling is only −0.3 dBTP. inline constexpr double kGrLampFloorDb = 0.5; bool grLampLit(const MasterMeterUi& m); diff --git a/src/shell/instrument/CLAUDE.md b/src/shell/instrument/CLAUDE.md index bdef065..a828667 100644 --- a/src/shell/instrument/CLAUDE.md +++ b/src/shell/instrument/CLAUDE.md @@ -107,7 +107,7 @@ declared ahead of the instrument slots at that member in `reasampler_processor.h ## Modules - `reaper_bridge` — READ-ONLY bank consumer: receives bank snapshots from the extension and exposes them as a read-only view. **Never writes to the extension's bank** — this is a load-bearing invariant; no mutation path exists in this module. It owns TWO prefix-guarded ext-state write entry points, `writeUsageExtState` (`rsusage_`) and `writeBakeExtState` (`rsbake_`), each refusing every other key; neither weakens the read-only-*bank* invariant, because neither payload is bank state and `banks`/`view`/`tail`/`assign` stay structurally unwritable. Both PROVE the write by reading the key back (`wire::extStateWriteLanded`) — `SetProjExtState`'s own return cannot speak for one key, so testing it was a guard that could never fire, and the bake's "could not publish" refusal was consequently unreachable. It also owns the bake crossing — `extensionActionAvailable` / `invokeExtensionAction` (`NamedCommandLookup` + `Main_OnCommandEx` with `getReaperParent(3)`, the instance's OWN project tab, as `proj` — a request, not a DAW-verified guarantee; see the header) and `projectTempoBpm`. -- `reasampler_processor` (`shell/instrument/`: `reasampler_processor.cpp` lifecycle + `process()`, `processor_state.cpp` component-state I/O + UI-thread parameter accessors, `processor_reload.cpp` the off-audio-thread `reloadInstrument`/publish family — Q-W2v, T4-12 split; `process()` and its per-block work stay ONE TU on purpose, no cross-TU call on the per-sample path) — VST3 `SingleComponentEffect` shell: declares event-input bus + **permanently stereo** output (GA fix: dynamic mono↔stereo bus renegotiation deleted; `ChannelMode` is now decode-only), marshals MIDI note-on/off into the VoiceEngine, renders audio; owns off-audio-thread `reloadInstrument` + atomic pointer swap so `process()` does no allocation, no file I/O, no bridge calls. The instance state is `{loaded capture id, one InstrumentParams}`, and `reloadInstrument` resolves + decodes exactly that one capture into the `SampleData` the engine plays. **Self-contained playback (pS):** `ComponentState` v10 adds a `SampleRefs` table — per referenced sample, a project-relative path + decode intrinsics (root, loop, channels, displayName); `reloadInstrument` decodes directly from `SampleRefs`, bank-free (plays with the extension absent). The bank/bridge is a browser source: loading a capture copies its reference in; the reopen-heal timer + poll-to-play apparatus are removed. `retireIdleDrain()` retires fully-idle drain snapshots on the UI-timer cadence. Voice-param edits (`setVoiceCount`/`setVoiceMode`/`setMonoTrigger`) rebuild the engine from the already-decoded `SampleData` via the drain-slot swap — no bank re-read, no WAV re-decode, no audible cut to ringing tails. **FB1:** applies the post-mixer `masterGainLinear` (from `ComponentState` v8) as a per-sample ramp over the summed output — no zipper noise. **GA v9:** `channelModeExplicit_` flag persisted; `channelModeFor()` auto-defaults the mode from the loaded capture's channel count when the flag is not set. **pS:** `ComponentState` bumped v9→v10 (`SampleRefs` table); pre-v10 blobs lift to empty refs and re-save self-contained. **pS-usage:** publishes instance usage (held `SampleRefs` paths) to `rsusage_` at the tail of `reloadInstrument` (off audio thread) via `reaper_bridge::writeUsageExtState`; `ComponentState` bumped v10→**v11** (`instanceGuid` field); pre-v11 blobs mint guid on first publish. **The master bus:** the summed output runs `voice mixer → master gain → limiter (core/instrument/engine/limiter) → output bus`, with the meter tapped at the bus output POST-limiter and published as relaxed atomics — per-channel peak and smallest limiter gain ACCUMULATED across every block since the UI last read, plus the latched clip (the meter Gotcha below owns why). The limiter's enable is persisted in the parameter set (params payload v15) and mirrored onto the audio thread by `setInstrumentParams`, the single funnel every writer already goes through. That mirror is also what `getLatencySamples()` answers from — the plugin's FIRST latency reporting: 0 bypassed, the lookahead engaged. The host's `restartComponent(kLatencyChanged)` is issued by `flushLatencyRestart` alone, UI thread only and never from `process()`; it is a LATENCY restart with the bus untouched, NOT the retired per-mode `kIoChanged` bus renegotiation the invariant above forbids. Why it is split from the commit is the Gotcha below. +- `reasampler_processor` (`shell/instrument/`: `reasampler_processor.cpp` lifecycle + `process()`, `processor_state.cpp` component-state I/O + UI-thread parameter accessors, `processor_reload.cpp` the off-audio-thread `reloadInstrument`/publish family — Q-W2v, T4-12 split; `process()` and its per-block work stay ONE TU on purpose, no cross-TU call on the per-sample path) — VST3 `SingleComponentEffect` shell: declares event-input bus + **permanently stereo** output (GA fix: dynamic mono↔stereo bus renegotiation deleted; `ChannelMode` is now decode-only), marshals MIDI note-on/off into the VoiceEngine, renders audio; owns off-audio-thread `reloadInstrument` + atomic pointer swap so `process()` does no allocation, no file I/O, no bridge calls. The instance state is `{loaded capture id, one InstrumentParams}`, and `reloadInstrument` resolves + decodes exactly that one capture into the `SampleData` the engine plays. **Self-contained playback (pS):** `ComponentState` v10 adds a `SampleRefs` table — per referenced sample, a project-relative path + decode intrinsics (root, loop, channels, displayName); `reloadInstrument` decodes directly from `SampleRefs`, bank-free (plays with the extension absent). The bank/bridge is a browser source: loading a capture copies its reference in; the reopen-heal timer + poll-to-play apparatus are removed. `retireIdleDrain()` retires fully-idle drain snapshots on the UI-timer cadence. Voice-param edits (`setVoiceCount`/`setVoiceMode`/`setMonoTrigger`) rebuild the engine from the already-decoded `SampleData` via the drain-slot swap — no bank re-read, no WAV re-decode, no cut to ringing tails. **Activation and decoding are separate lifetimes:** `setActive(false)` parks the decoded `SampleData` and destroys the voice state (a surviving `live_` would be displaced into the drain slot and resurrect stale sustained voices), and `setActive(true)` rebuilds the voices around the parked sample through that same swap — so a host-driven cycle costs no disk read and no decode. Nothing parked means nothing was decoded, which routes the activation back through the full reload; that is also where the pre-v10 legacy lift lives. **FB1:** applies the post-mixer `masterGainLinear` (from `ComponentState` v8) as a per-sample ramp over the summed output — no zipper noise. **GA v9:** `channelModeExplicit_` flag persisted; `channelModeFor()` auto-defaults the mode from the loaded capture's channel count when the flag is not set. **pS:** `ComponentState` bumped v9→v10 (`SampleRefs` table); pre-v10 blobs lift to empty refs and re-save self-contained. **pS-usage:** publishes instance usage (held `SampleRefs` paths) to `rsusage_` at the tail of `reloadInstrument` (off audio thread) via `reaper_bridge::writeUsageExtState`; `ComponentState` bumped v10→**v11** (`instanceGuid` field); pre-v11 blobs mint guid on first publish. **The master bus:** the summed output runs `voice mixer → master gain → limiter (core/instrument/engine/limiter) → output bus`, with the meter tapped at the bus output POST-limiter and published as relaxed atomics — per-channel peak and smallest limiter gain ACCUMULATED across every block since the UI last read, plus the latched clip (the meter Gotcha below owns why). The limiter's enable is persisted in the parameter set (params payload v15) and mirrored onto the audio thread by `setInstrumentParams`, the single funnel every writer already goes through. That mirror is also what `getLatencySamples()` answers from — the plugin's FIRST latency reporting: 0 bypassed, the lookahead engaged. The host's `restartComponent(kLatencyChanged)` is issued by `flushLatencyRestart` alone, UI thread only and never from `process()`; it is a LATENCY restart with the bus untouched, NOT the retired per-mode `kIoChanged` bus renegotiation the invariant above forbids. Why it is split from the commit is the Gotcha below. - `reasampler_editor` — VST3 `IPlugView` LICE editor shell: hosts a LICE-drawn child window; the Sample face is home and Browse is a modal picker over it. Split on the Sample face's BAND axis, mirroring the pure `sample_bands` allocator: `editor_session` (session/bridge state, caches, commit-and-reload), `editor_controls` (the ONE `faceLayout` band resolve every paint and hit-test path shares, the node-drag bounds, the value labels, and the per-instance controls the parameter set does not carry — the parameter-set binding itself is the pure `core/instrument/ui/deck_values` module this only adapts int ids onto), `editor_models` (the orthogonal half: which stored struct each transient editor selection names — the staged-envelope pack/unpack, the drawn contour, and the three velocity curves), then matching paint and input sets — `editor_paint`/`editor_input` (dispatch + drag router + hover dispatch), `_chrome`, `_waveform`, `_deck` — plus the two band-independent surfaces (`_browse` for the modal picker, `_curve` for the velocity-curve popup) and `editor_platform` (IPlugView/Win32 window plumbing). Shared internals in `editor_internal.h`, no TU of its own. Drop-onto-editor ingest is NOT shipped (deferred). - `reasampler_embed` — implements `IReaperUIEmbedInterface` so the instrument draws inline in the TCP/MCP without a plugin-owned HWND; delegates layout to `embed_strip`. A read-only readout: the loaded capture across the keyboard span with its root marked, plus the activity level. It takes no mouse input (there is nothing on the strip to select). - `editor_stroke` — the editor's LICE side of the analytic stroker: builds a coverage mask with the pure `core/ui/stroke_aa` and blends it into the bitmap ONCE, writing straight to the bitmap's bits (the arithmetic matches LICE's own mode-0 combine, so a stroke composites identically to every other kit draw). Every radial and spline stroke on the editor routes through `strokeArcAA` / `strokePolylineAA` / `strokeLineAA`. Holds the draw-thread-only scratch mask and arc point list — reuse, not a hidden dependency: threading a canvas through the eight paint sites would grow those signatures to carry an allocation detail. Deliberately does NOT touch `shell/panel/draw_kit`: the waveform stroke, the docked bank panel and the browse cards are out of this seam's blast radius. @@ -127,15 +127,16 @@ declared ahead of the instrument slots at that member in `reasampler_processor.h one. - **The limiter toggle splits its commit: the sound is inline, the HOST NOTIFICATION arms.** Its commit needs the host's `restartComponent(kLatencyChanged)`; a host that services that - synchronously runs `setActive(false)`/`setActive(true)`, and OUR `setActive(true)` calls - `reloadInstrument()` — a WAV re-decode plus disk I/O, which inline from `WM_LBUTTONDOWN` - would run nested in a mouse handler. So the click commits the parameter set, the audio-thread - mirror and the latency reader at once, and `setInstrumentParams` only ARMS a pending restart - that `flushLatencyRestart` delivers. The editor's sync tick is the general drain and sits - AFTER the drag guard with the bake (the restart rebuilds the instance, which mid-drag would - yank the edit surface exactly as a reload would); `setState` and the bake's adopt flush at - their own tails, because they can commit with no editor open. The arm is a sticky bool, so - toggling twice inside one tick still costs exactly one restart. + synchronously runs `setActive(false)`/`setActive(true)`, which rebuilds this instance's voice + state — running that inline from `WM_LBUTTONDOWN` would nest it in a mouse handler. So the + click commits the parameter set, the audio-thread mirror and the latency reader at once, and + `setInstrumentParams` only ARMS a pending restart that `flushLatencyRestart` delivers. The + editor's sync tick is the general drain and sits AFTER the drag guard with the bake (the + restart rebuilds the instance, which mid-drag would yank the edit surface exactly as a reload + would); `setState` flushes at its own tail because it can commit with no editor open, and the + bake's adopt does so only to save a tick — its chain runs from that same tick. The arm is + judged against the LAST ANNOUNCED enable, so toggling back to it inside one tick costs no + restart at all. **The residual:** between the commit and the flush the host's delay compensation is out of step with the plugin by `limiterLookaheadSamples` (2 ms — `round(0.002 · rate)`, the detector's 4-sample group delay INSIDE that budget, not on top), bounded by one 500 ms tick. @@ -151,7 +152,11 @@ declared ahead of the instrument slots at that member in `reasampler_processor.h store displayed one block in ~47 and lost the rest — the specified "a peak displays on the first UI frame after it occurs" is what the fold restores. `masterBusMeter()` is CONSUMING, so exactly one caller may hold it; the embed strip reads its own non-consuming - `embedActivityLevel()`. A meter-rate timer remains a separate change and is not in. + `embedActivityLevel()`. **The tick's FIRST read is discarded**, because that caller is the + only consumer: with no editor open the accumulators hold everything since the instance was + created, and advancing off them would open the meter at the session's loudest peak. The clip + latch is not discarded with them — it is a latch the user clears. A meter-rate timer remains a + separate change and is not in. - The bake's availability probe runs on the SAME tick that paints the button, so the control can never be enabled on one tick and refuse on the next. The bake Hold control's applicability (`resolveBakeHoldNeeded`) rides the same tick for the same reason, and diff --git a/src/shell/instrument/CMakeLists.txt b/src/shell/instrument/CMakeLists.txt index cb025e4..a59836b 100644 --- a/src/shell/instrument/CMakeLists.txt +++ b/src/shell/instrument/CMakeLists.txt @@ -89,7 +89,7 @@ if(WIN32 AND EXISTS "${VST3_SDK}/public.sdk/source/main/pluginfactory.cpp") waveform_view loop_marks bank_sync browser_scroll param_slider tooltip theme component_geometry bank_grid trigger_seam envelope_overlay envelope_edit knob_deck deck_groups deck_values curve_popup spline_edit master_gain sample_usage - limiter meter_ballistics master_meter bake_hold + limiter meter_accumulate meter_ballistics master_meter bake_hold file_bytes curve_law stroke_aa curve_tessellate bake_plan bake_render bake_reset bake_wire wav_codec) diff --git a/src/shell/instrument/editor_paint_deck.cpp b/src/shell/instrument/editor_paint_deck.cpp index c849c4c..6878753 100644 --- a/src/shell/instrument/editor_paint_deck.cpp +++ b/src/shell/instrument/editor_paint_deck.cpp @@ -37,11 +37,14 @@ std::string tickLabel(int db) { } // The MASTER column: dB scale in the label gutter, one or two bars, the held peak tick, and -// the latched clip cap. `split` is waveformSurface's own lane decision — see master_meter.h. +// the latched clip cap. `split` is the RESOLVED lane decision — see master_meter.h. void paintMeterColumn(LICE_IBitmap* bmp, const Rect& column, const MasterMeterUi& state, LaneSplit split) { if (column.width <= 0 || column.height <= 0) return; const MeterRects m = meterRects(column, split); + // A column narrower than the interior needs yields all-empty rects, which under rect.h's + // contract means suppressed — not a zero-height field to fill, tick twelve times and cap. + if (m.field.empty()) return; fillSurface(bmp, toKitBox(m.field), Role::BgCell, InteractionState::Rest); // Scale: a rule every 6 dB, numeralled every 12 with 0 dB heavier — the reference the @@ -60,8 +63,9 @@ void paintMeterColumn(LICE_IBitmap* bmp, const Rect& column, const MasterMeterUi } } - // The bars. A single-lane surface shows ONE bar off the louder channel: the two are the - // same signal there (dual-mono), so two bars would be a duplicate rather than a reading. + // The bars. A single-lane surface shows ONE bar folding both channels per field + // (meterSingleLaneState) — the two are the same signal there (dual-mono), so two bars would + // be a duplicate rather than a reading. const LICE_pixel barInk = toLice(roleColor(Role::AccentPrimary)); const LICE_pixel holdInk = toLice(roleColor(Role::TextPrimary)); const auto drawBar = [&](const Rect& bar, const instrument::engine::MeterState& ch) { diff --git a/src/shell/instrument/editor_session.cpp b/src/shell/instrument/editor_session.cpp index 5b543fa..bca7a84 100644 --- a/src/shell/instrument/editor_session.cpp +++ b/src/shell/instrument/editor_session.cpp @@ -112,18 +112,25 @@ void ReaSamplerEditor::onSyncTimer() { // keeps sounding and a frozen bar would misreport it. { const unsigned long long now = GetTickCount64(); - const double elapsed = meterTickMs_ == 0 - ? 0.0 - : static_cast(now - meterTickMs_) / 1000.0; + const unsigned long long previous = meterTickMs_; meterTickMs_ = now; const MasterBusMeter bus = processor_->masterBusMeter(); - const instrument::ui::MasterMeterUi advanced = instrument::ui::advanceMasterMeter( - masterMeter_, - {bus.peakL, bus.peakR, bus.minGain, bus.clip}, - elapsed); - const bool changed = !instrument::ui::meterDrawEqual(advanced, masterMeter_); - masterMeter_ = advanced; - if (changed) invalidate(); + // The accumulators have exactly one consumer — this tick — so with no editor open they + // hold everything since the instance was created. The first read is therefore session + // history, not a window: showing it would put the bar at the loudest peak of the + // session (instantaneous rise, then a 1.5 s hold) and light the GR lamp off a catch + // minutes old, with the limiter possibly off since. Discard it and start the window + // here. The CLIP survives, because it is a latch the user clears rather than a window — + // it is still set in the processor and the next tick reports it. + if (previous != 0) { + const instrument::ui::MasterMeterUi advanced = instrument::ui::advanceMasterMeter( + masterMeter_, + {bus.peakL, bus.peakR, bus.minGain, bus.clip}, + static_cast(now - previous) / 1000.0); + const bool changed = !instrument::ui::meterDrawEqual(advanced, masterMeter_); + masterMeter_ = advanced; + if (changed) invalidate(); + } } if (drag_ != DragKind::kNone) return; // defer past the in-flight edit diff --git a/src/shell/instrument/processor_reload.cpp b/src/shell/instrument/processor_reload.cpp index d032f0d..8bd0f20 100644 --- a/src/shell/instrument/processor_reload.cpp +++ b/src/shell/instrument/processor_reload.cpp @@ -96,10 +96,6 @@ std::string ReaSamplerProcessor::reloadInstrument() { // retired-slot free is single-writer; never taken on the audio thread. std::lock_guard lock(reloadMutex_); - // Mint this reload's generation number first so the built instrument is stamped - // before publishing. - const std::uint64_t gen = reloadGeneration_.fetch_add(1, std::memory_order_relaxed) + 1; - // 1. Self-contained resolution: the instance-owned refs table is the source of truth. // The live bank blob, when readable, is folded in first (refreshRefsFromBank — the // browser's copy-the-ref-in + recapture-sync mechanism), but its absence changes @@ -124,17 +120,6 @@ std::string ReaSamplerProcessor::reloadInstrument() { // Governs how the WAV decodes (mono downmix vs 2-channel); auto-defaulted from the // capture's own channel count below, before the decode. ChannelMode mode = channelMode(); - // Snapshot the voice-system parameters once — baked into the built engine's - // construction (immutable config; a later change rebuilds). - int builtVoiceCount = kDefaultVoiceCount; - VoiceMode builtVoiceMode = VoiceMode::Poly; - MonoTrigger builtMonoTrigger = MonoTrigger::Retrigger; - { - std::lock_guard vp(voiceParamsMutex_); - builtVoiceCount = voiceCount_; - builtVoiceMode = voiceMode_; - builtMonoTrigger = monoTrigger_; - } std::string resolvedId; std::unique_ptr built; @@ -177,17 +162,7 @@ std::string ReaSamplerProcessor::reloadInstrument() { } } - if (havePlayable) { - // Preserve OLA window in output frames from the host rate (kPreserveWindowMs), - // pre-sized here so process()-time note-on never allocates. Floored at 2 so a - // valid window is always a real ring, covering a pathological host rate <= 0 too. - std::int64_t preserveWindow = static_cast( - kPreserveWindowMs * sampleRate_ / 1000.0 + 0.5); - if (preserveWindow < 2) preserveWindow = 2; - built = std::make_unique( - std::move(sample), static_cast(builtVoiceCount), gen, - kPreserveVoiceCap, preserveWindow, builtVoiceMode, builtMonoTrigger); - } + if (havePlayable) built = buildInstrumentLocked(std::move(sample)); // 3. Publish: atomically install the new instrument via the drain-slot swap (see the // header). A null `built` (no ref / unreadable WAV) installs silence while any @@ -246,9 +221,10 @@ void ReaSamplerProcessor::adoptBakedCapture(const SampleRefEntry& entry, // ONE reload for the re-point and the reset together: it decodes the new file and // publishes the neutral parameters in the same swap. reloadInstrument(); - // The bake's reset may have flipped the limiter; deliver the host's latency restart here - // rather than leaving it to the editor's tick, so an adopt is correct with no editor open. - // At the tail for the same reason setState's is (see there). + // The bake's reset may have flipped the limiter; delivering the restart here rather than + // leaving it to the editor's tick is a LATENCY improvement, not a correctness one — the + // bake chain only ever runs from that tick, so the arm would be drained on the next one + // anyway. At the tail for the same reason setState's is (see there). flushLatencyRestart(); } @@ -301,6 +277,10 @@ void ReaSamplerProcessor::publishBuiltLocked(std::unique_ptr b return e->installedAt < seen; }), graveyard_.end()); + // Any publish supersedes the deactivate's park: whatever is installed here is the newer + // truth, and a park surviving it would be reinstalled over this instrument at the next + // activation (the setState-while-inactive case). + dormantSample_.reset(); LoadedInstrument* prev = live_.exchange(built.release()); // A bake's reset gain lands here rather than at its call site, so the gain and the // capture it belongs to become audible to process() within one block of each other. @@ -317,6 +297,34 @@ void ReaSamplerProcessor::publishBuiltLocked(std::unique_ptr b if (evicted) graveyard_.push_back(std::unique_ptr(evicted)); } +std::unique_ptr +ReaSamplerProcessor::buildInstrumentLocked(SampleData sample) { + // REQUIRES reloadMutex_ held. The ONE construction of a playable snapshot, so the three + // callers (full reload, voice-param rebuild, reactivation) cannot drift on the generation + // stamp, the voice-system snapshot or the ring size. + int builtVoiceCount = kDefaultVoiceCount; + VoiceMode builtVoiceMode = VoiceMode::Poly; + MonoTrigger builtMonoTrigger = MonoTrigger::Retrigger; + { + // Baked into the engine's construction (immutable config; a later change rebuilds). + std::lock_guard vp(voiceParamsMutex_); + builtVoiceCount = voiceCount_; + builtVoiceMode = voiceMode_; + builtMonoTrigger = monoTrigger_; + } + // Preserve OLA window in output frames from the host rate (kPreserveWindowMs), pre-sized + // here so process()-time note-on never allocates. Floored at 2 so a valid window is always + // a real ring, covering a pathological host rate <= 0 too. Re-derived per build, so a + // reactivation after the host changed its rate gets a ring sized for the new one. + std::int64_t preserveWindow = static_cast( + kPreserveWindowMs * sampleRate_ / 1000.0 + 0.5); + if (preserveWindow < 2) preserveWindow = 2; + const std::uint64_t gen = reloadGeneration_.fetch_add(1, std::memory_order_relaxed) + 1; + return std::make_unique( + std::move(sample), static_cast(builtVoiceCount), gen, + kPreserveVoiceCap, preserveWindow, builtVoiceMode, builtMonoTrigger); +} + void ReaSamplerProcessor::rebuildVoiceEngine() { // Off the audio thread. A voice-param change touches no audio data, so this rebuilds // the engine around a copy of the live instrument's already-decoded SampleData — no @@ -324,31 +332,25 @@ void ReaSamplerProcessor::rebuildVoiceEngine() { std::lock_guard lock(reloadMutex_); LoadedInstrument* cur = live_.load(std::memory_order_acquire); if (!cur) return; // nothing loaded: the new params bake into the next real reload. - - int builtVoiceCount = kDefaultVoiceCount; - VoiceMode builtVoiceMode = VoiceMode::Poly; - MonoTrigger builtMonoTrigger = MonoTrigger::Retrigger; - { - std::lock_guard vp(voiceParamsMutex_); - builtVoiceCount = voiceCount_; - builtVoiceMode = voiceMode_; - builtMonoTrigger = monoTrigger_; - } - - const std::uint64_t gen = reloadGeneration_.fetch_add(1, std::memory_order_relaxed) + 1; - // Same Preserve-window derivation as reloadInstrument. - std::int64_t preserveWindow = static_cast( - kPreserveWindowMs * sampleRate_ / 1000.0 + 0.5); - if (preserveWindow < 2) preserveWindow = 2; - // Deep-copy the decoded sample: safe to read concurrently with process() because the // SampleData is immutable after construction and reloadMutex_ prevents `cur` from being // freed. - SampleData sample = cur->sample; - auto built = std::make_unique( - std::move(sample), static_cast(builtVoiceCount), gen, - kPreserveVoiceCap, preserveWindow, builtVoiceMode, builtMonoTrigger); - publishBuiltLocked(std::move(built)); + publishBuiltLocked(buildInstrumentLocked(cur->sample)); +} + +bool ReaSamplerProcessor::resumeDormantInstrument() { + // Off the audio thread (setActive only). The reactivation half of the lifetime split: the + // voice state the deactivate destroyed is rebuilt, the PCM it parked is reused as-is. + std::lock_guard lock(reloadMutex_); + // A publish that landed while inactive (setState's reload, a bake's adopt) IS the + // activation state — its voices have never rendered, and it has already superseded the + // park. Rebuilding here would displace a correct instrument into the drain slot. + if (live_.load(std::memory_order_acquire)) return true; + if (!dormantSample_) return false; + // MOVED, not copied: the park exists for this one handoff, and keeping it would hold a + // second copy of the PCM for the whole active lifetime. + publishBuiltLocked(buildInstrumentLocked(std::move(*dormantSample_))); + return true; } void ReaSamplerProcessor::retireIdleDrain() { diff --git a/src/shell/instrument/processor_state.cpp b/src/shell/instrument/processor_state.cpp index 784a258..ac7056c 100644 --- a/src/shell/instrument/processor_state.cpp +++ b/src/shell/instrument/processor_state.cpp @@ -153,10 +153,8 @@ InstrumentParams ReaSamplerProcessor::instrumentParams() { } void ReaSamplerProcessor::setInstrumentParams(const InstrumentParams& params) { - bool limiterFlagChanged = false; { std::lock_guard lock(paramsMutex_); - limiterFlagChanged = (params_.limiterEnabled != params.limiterEnabled); params_ = params; } // Every writer of the parameter set — setState, the editor's commits, the bake's adopt — @@ -167,11 +165,14 @@ void ReaSamplerProcessor::setInstrumentParams(const InstrumentParams& params) { // safe there (see this directory's CLAUDE.md). publishLimiterEnabled(params.limiterEnabled); // Armed AFTER the mirror, so getLatencySamples already answers the new value for the whole - // window the arm stays outstanding. Sticky and idempotent: any number of changes before one - // flush cost one restart, and the flush is the only thing that clears it. - if (limiterFlagChanged) { - latencyRestartPending_.store(true, std::memory_order_release); - } + // window the arm stays outstanding. Compared against the last ANNOUNCED enable rather than + // against the previous parameter set: off->on->off inside one tick ends at the latency the + // host already knows, and a restart rebuilds the instance, so announcing a latency that + // never changed is pure cost. Any number of changes before one flush still cost at most one + // restart, and this store is the only one that raises OR lowers the arm. + latencyRestartPending_.store( + params.limiterEnabled != latencyAnnounced_.load(std::memory_order_relaxed), + std::memory_order_release); } void ReaSamplerProcessor::flushLatencyRestart() { @@ -179,6 +180,11 @@ void ReaSamplerProcessor::flushLatencyRestart() { // its handler waits for a later flush instead of evaporating. if (!componentHandler) return; if (!latencyRestartPending_.exchange(false, std::memory_order_acquire)) return; + // Latched BEFORE the call: a host that services the restart synchronously re-enters this + // object inside it, so the next commit must compare against the value the host is about to + // read, not against the one it held before. + latencyAnnounced_.store(limiterEnabled_.load(std::memory_order_relaxed), + std::memory_order_relaxed); // The SDK requires this on the UI thread and answers getLatencySamples only after the host's // own deactivate/reactivate — so the flag is long committed by the time the host asks. This // is a kLatencyChanged restart with the bus untouched, NOT the retired per-mode kIoChanged @@ -202,14 +208,14 @@ void ReaSamplerProcessor::setLimiterEnabled(bool on) { MasterBusMeter ReaSamplerProcessor::masterBusMeter() { MasterBusMeter m; - // Exchange, not load: the accumulators hold the window since this was last called, and - // clearing them here is what starts the next window. The audio thread's own fold is a - // load-max-store, so a store landing between this exchange and that store can retain one - // window's peak for one extra frame — it can never LOSE one, which is the property that - // matters for a peak meter. - m.peakL = meterPeakL_.exchange(0.f, std::memory_order_relaxed); - m.peakR = meterPeakR_.exchange(0.f, std::memory_order_relaxed); - m.minGain = meterMinGain_.exchange(1.f, std::memory_order_relaxed); + // Consuming: each read takes the window and reinstalls its identity element, which is what + // starts the next one. The audio thread's fold is an unconditional CAS against exactly that + // (meter_accumulate.h owns the argument), so a fold interleaved with these exchanges lands + // in one window or the other and is never dropped between them. + m.peakL = instrument::engine::consumePeak(meterPeakL_); + m.peakR = instrument::engine::consumePeak(meterPeakR_); + m.minGain = instrument::engine::consumeMinGain(meterMinGain_); + // NOT consumed: the clip is a latch the user clears, not a window. m.clip = meterClip_.load(std::memory_order_relaxed); return m; } diff --git a/src/shell/instrument/reasampler_editor.h b/src/shell/instrument/reasampler_editor.h index f67fc69..f27d642 100644 --- a/src/shell/instrument/reasampler_editor.h +++ b/src/shell/instrument/reasampler_editor.h @@ -457,7 +457,8 @@ private: bool searchFocused_ = false; // whether the search box has keyboard focus // The MASTER deck's meter, advanced from the published block magnitudes on the sync tick - // (see onSyncTimer for why it runs mid-drag too). meterTickMs_ 0 = never advanced. + // (see onSyncTimer for why it runs mid-drag too, and why the first read is discarded). + // meterTickMs_ 0 = never ticked. instrument::ui::MasterMeterUi masterMeter_; unsigned long long meterTickMs_ = 0; diff --git a/src/shell/instrument/reasampler_processor.cpp b/src/shell/instrument/reasampler_processor.cpp index 2d4721b..d95f193 100644 --- a/src/shell/instrument/reasampler_processor.cpp +++ b/src/shell/instrument/reasampler_processor.cpp @@ -79,34 +79,45 @@ tresult PLUGIN_API ReaSamplerProcessor::terminate() { delete live_.exchange(nullptr); delete draining_.exchange(nullptr); graveyard_.clear(); + dormantSample_.reset(); return SingleComponentEffect::terminate(); } tresult PLUGIN_API ReaSamplerProcessor::setActive(TBool state) { - // Activating: build from the currently-selected sample so the first block after - // activation can play. Deactivating: process is now guaranteed stopped, so this is - // the safe point to reclaim the graveyard. Main/UI-thread call. + // Activation governs ONE thing — whether the audio thread may run. The decoded + // SampleData has its own lifetime and survives the cycle (dormantSample_); only the + // voice state is built and destroyed here. Main/UI-thread call. if (state) { - // Resolves + decodes from the instance-owned refs — no bank read needed, so it - // plays regardless of PROJEXTSTATE parse state. Also doubles as the non-editor - // legacy-lift trigger for a pre-v10 blob: reloadInstrument's opportunistic - // refreshRefsFromBank copies refs in when the bank blob is readable by now. + // Rebuild the voices around the sample the deactivate parked — no bridge read, no WAV + // decode — so a host-driven cycle (a kLatencyChanged restart, an offline-render + // bracket) costs no I/O. With nothing to activate from, the full reload runs: it + // resolves + decodes from the instance-owned refs (no bank read, so it plays regardless + // of PROJEXTSTATE parse state) and doubles as the non-editor legacy-lift trigger for a + // pre-v10 blob, whose opportunistic refreshRefsFromBank copies refs in when the bank + // blob is readable by now. Nothing can shadow that lift: a pre-v10 blob resolves + // nothing, so it has neither a parked sample nor a published instrument. // Residual load-order race (DAW-verifiable only): if the host activates before the // project's ext-state parses, nothing retries until the next activation or editor // tick — open a pre-v10 instrument once after upgrading if it restores silent. - reloadInstrument(); + if (!resumeDormantInstrument()) reloadInstrument(); // The host performs this deactivate/reactivate whenever it acts on a kLatencyChanged // request, so the limiter starts each activation with an empty delay line and snapped - // to its persisted state — no transition mute, because there is nothing sounding to be - // continuous with once the block above has destroyed every voice. + // to its persisted state — no transition mute, because the deactivate destroyed every + // voice and the rebuild above starts with none sounding. limiter_.reset(); } else { std::lock_guard lock(reloadMutex_); - // Free EVERYTHING, including live_: its voices are frozen mid-flight, and if it - // survived deactivation the reactivate reload would displace it into the drain - // slot, resurrecting stale sustained voices as ghosts. Reactivation rebuilds from - // scratch above, so nothing is lost. - delete live_.exchange(nullptr); + // Park the decoded PCM; free EVERYTHING else, live_ included. Its voices are frozen + // mid-flight, and if it survived deactivation the reactivate would displace it into + // the drain slot, resurrecting stale sustained voices as ghosts. + std::unique_ptr dying(live_.exchange(nullptr)); + // Moved out ahead of the destruction: `sample` is declared before `engine`, so the + // engine — the only holder of a reference to it — dies first and never reads the + // moved-from value. Empty when nothing was loaded, which is what routes the next + // activation back through the full reload. + dormantSample_ = dying ? std::optional(std::move(dying->sample)) + : std::nullopt; + dying.reset(); delete draining_.exchange(nullptr); graveyard_.clear(); } @@ -309,7 +320,7 @@ tresult PLUGIN_API ReaSamplerProcessor::process(ProcessData& data) { } // The chain's last stage before the bus, after the gain above. const float minGain = limiter_.process(ch0, ch1, frames); - foldMinGain(meterMinGain_, minGain); + instrument::engine::foldMinGain(meterMinGain_, minGain); // Channels beyond the first two mirror ch0 (defensive — REAPER negotiates 1 or 2). for (int32 ch = 2; ch < out.numChannels; ++ch) { if (float* buf = out.channelBuffers32[ch]) { @@ -324,8 +335,8 @@ tresult PLUGIN_API ReaSamplerProcessor::process(ProcessData& data) { if (a0 > peakL) peakL = a0; if (a1 > peakR) peakR = a1; } - foldPeak(meterPeakL_, peakL); - foldPeak(meterPeakR_, peakR); + instrument::engine::foldPeak(meterPeakL_, peakL); + instrument::engine::foldPeak(meterPeakR_, peakR); advisoryPeak_.store(peakL > peakR ? peakL : peakR, std::memory_order_relaxed); if (peakL >= 1.f || peakR >= 1.f) meterClip_.store(true, std::memory_order_relaxed); } else if (ch0) { @@ -355,14 +366,14 @@ tresult PLUGIN_API ReaSamplerProcessor::process(ProcessData& data) { } } const float minGain = limiter_.process(ch0, nullptr, frames); - foldMinGain(meterMinGain_, minGain); + instrument::engine::foldMinGain(meterMinGain_, minGain); float peak = 0.f; for (int32 i = 0; i < frames; ++i) { const float a = ch0[i] < 0.f ? -ch0[i] : ch0[i]; if (a > peak) peak = a; } - foldPeak(meterPeakL_, peak); - foldPeak(meterPeakR_, peak); + instrument::engine::foldPeak(meterPeakL_, peak); + instrument::engine::foldPeak(meterPeakR_, peak); advisoryPeak_.store(peak, std::memory_order_relaxed); if (peak >= 1.f) meterClip_.store(true, std::memory_order_relaxed); for (int32 ch = 1; ch < out.numChannels; ++ch) { diff --git a/src/shell/instrument/reasampler_processor.h b/src/shell/instrument/reasampler_processor.h index d6f93fd..e5f9dd0 100644 --- a/src/shell/instrument/reasampler_processor.h +++ b/src/shell/instrument/reasampler_processor.h @@ -22,6 +22,7 @@ #include "core/instrument/map/component_state_io.h" // ComponentState codec #include "core/instrument/engine/limiter.h" // the master bus's post-gain limiter #include "core/instrument/engine/live_params.h" // LiveParams (the live-parameter block) +#include "core/instrument/engine/meter_accumulate.h" // the meter's block-rate folds + consume #include "core/instrument/engine/voice_engine.h" namespace reasampler::vst { @@ -137,7 +138,10 @@ public: // What the audio thread published since the LAST call: peaks maxed and minGain minimised // across every block in that window. CONSUMING — it resets the accumulators as it reads, so - // exactly one reader may call it, and that reader is the editor's meter tick. UI thread. + // exactly one reader may call it, and that reader is the editor's meter tick. Two live + // editors would each consume half the windows and both meters would read low; what makes + // that unreachable is the HOST calling createView once per instance, not anything this + // plugin enforces — createView allocates a new editor on every call. UI thread. MasterBusMeter masterBusMeter(); void clearMasterBusClip(); @@ -248,9 +252,10 @@ public: // Delivers the armed kLatencyChanged restart, at most once per armed window, and does // nothing when none is armed. Split off the commit because the SDK requires this on the UI // thread AND a host may service it synchronously — deactivate/reactivate, which reaches our - // setActive(true) and its reloadInstrument — so it must never run nested inside a mouse - // handler. The editor's sync tick is the general drain; the two callers that can commit with - // no editor open (setState, adoptBakedCapture) flush themselves at their own tails. + // setActive(true) — so it must never run nested inside a mouse handler. The editor's sync + // tick is the general drain; setState flushes at its own tail because it can commit with no + // editor open, and adoptBakedCapture does so only to save a tick (its chain runs from that + // same tick, so its arm would drain on the next one regardless). void flushLatencyRestart(); // Fires a one-shot preview note-on/off through the live VoiceEngine — the same @@ -283,6 +288,18 @@ private: // swap as a full reload. No-op when nothing is loaded. Off the audio thread only. void rebuildVoiceEngine(); + // The reactivation half of the activation/decode lifetime split (see dormantSample_): + // rebuilds the voice state around the parked sample and publishes it through the same + // drain-slot swap. True also when a publish landed while inactive, which needs no rebuild. + // False when there is nothing to activate from — the caller then falls back to a full + // reload, which is where the pre-v10 legacy lift lives. Off the audio thread only. + bool resumeDormantInstrument(); + + // Builds a playable snapshot around `sample` at the current voice-system parameters and + // host rate, stamped with a fresh generation. Requires reloadMutex_ held; the ONE + // construction site shared by the reload, the voice-param rebuild and the reactivation. + std::unique_ptr buildInstrumentLocked(SampleData sample); + // Pre-v10 legacy-lift gate: true when a lift attempt this tick could make progress // (see legacyLiftConcluded_). Off the audio thread only (bridge read + bank parse). bool legacyLiftShouldRun(); @@ -293,22 +310,6 @@ private: // Requires reloadMutex_ held — shared by reloadInstrument and rebuildVoiceEngine. void publishBuiltLocked(std::unique_ptr built); - // Folds one block's reading into the accumulator it belongs to — a running max for a peak, - // a running min for the limiter's gain. Read-modify-write rather than a bare store, so the - // ~47 blocks that elapse between two 500 ms UI frames at 48 kHz/512 all reach the meter - // instead of the one it happened to sample. Relaxed throughout: the accumulators are - // advisory, and no other state is ordered against them. Block rate, never per frame. - static void foldPeak(std::atomic& acc, float blockPeak) { - if (blockPeak > acc.load(std::memory_order_relaxed)) { - acc.store(blockPeak, std::memory_order_relaxed); - } - } - static void foldMinGain(std::atomic& acc, float blockMinGain) { - if (blockMinGain < acc.load(std::memory_order_relaxed)) { - acc.store(blockMinGain, std::memory_order_relaxed); - } - } - // Publishes a silent block. EVERY process() path that emits no audio calls this. It clears // only the ADVISORY level, which is a last-block reading: the meter accumulators need // nothing here, because a block that emitted no audio contributes no peak and no gain @@ -383,6 +384,13 @@ private: // Guarded by reloadMutex_, consumed by publishBuiltLocked. std::optional gainAtNextPublish_; + // The decoded PCM parked across a deactivate, so an activation cycle costs no disk read + // and no WAV decode: activation is "the audio thread may run", not "the sample is + // rebuilt". Holds a value only while inactive, and any publish drops it (publishBuiltLocked + // owns why). Voice state is deliberately NOT parked with it; see setActive. Guarded by + // reloadMutex_. + std::optional dormantSample_; + // The loaded capture's id ("" = no pick -> silence). Off-thread only, not read on the // audio thread. std::mutex selectionMutex_; @@ -479,20 +487,25 @@ private: // getLatencySamples answers from. instrument::engine::Limiter limiter_; std::atomic limiterEnabled_{false}; - // Set by the commit funnel when the enable actually changed, cleared only by - // flushLatencyRestart. A sticky bool and not a count on purpose: the host is being told to - // re-ASK, so N changes before one flush need exactly one restart, and whatever - // getLatencySamples answers at that moment is the truth being announced. + // Raised (and lowered) by the commit funnel from the difference between the enable and + // latencyAnnounced_, cleared by flushLatencyRestart on delivery. A bool and not a count on + // purpose: the host is being told to re-ASK, so N changes before one flush need exactly one + // restart, and whatever getLatencySamples answers at that moment is the truth announced. std::atomic latencyRestartPending_{false}; + // The enable the host was last told about — false initially, which is what an instance + // that has announced nothing reports. Every arm is judged against this, so a change that + // returns to the announced state costs no restart. + std::atomic latencyAnnounced_{false}; // What the audio thread publishes about the output bus each block, relaxed. The peaks and // minGain ACCUMULATE (max / min) across every block since the UI last read, and - // masterBusMeter() resets them as it reads — the fix for a bar that displayed roughly one - // block in fifty. No dB, no ballistics, no hold timer here; - // the UI runs those off these values and its own elapsed time. - std::atomic meterPeakL_{0.f}; - std::atomic meterPeakR_{0.f}; - std::atomic meterMinGain_{1.f}; + // masterBusMeter() consumes them as it reads — the fix for a bar that displayed roughly one + // block in fifty. The folds and the identity elements below are meter_accumulate's; no dB, + // no ballistics, no hold timer here — the UI runs those off these values and its own + // elapsed time. + std::atomic meterPeakL_{instrument::engine::kMeterPeakIdentity}; + std::atomic meterPeakR_{instrument::engine::kMeterPeakIdentity}; + std::atomic meterMinGain_{instrument::engine::kMeterGainIdentity}; std::atomic meterClip_{false}; // The embed strip's activity level: the LAST block's loudest channel, plainly overwritten. diff --git a/tests/test_meter_accumulate.cpp b/tests/test_meter_accumulate.cpp new file mode 100644 index 0000000..436475a --- /dev/null +++ b/tests/test_meter_accumulate.cpp @@ -0,0 +1,106 @@ +// Standalone tests for reasampler::instrument::engine::meter_accumulate — no VST3, no REAPER, +// no framework. Assert: +// +// * the window semantics — max for a peak, min for the limiter gain, and a consume that both +// reports the window and reinstalls the identity element that starts the next one. +// * the INTERLEAVE the CAS exists for: a consume landing inside a fold must not swallow that +// block. Driven by an accumulator that performs the consume from inside its first CAS, so +// the ordering is pinned rather than raced for. + +#include "../src/core/instrument/engine/meter_accumulate.h" + +#include +#include + +using namespace reasampler::instrument::engine; + +static int g_fail = 0; +#define CHECK(cond) do { if(!(cond)) { \ + std::printf("FAIL line %d: %s\n", __LINE__, #cond); ++g_fail; } } while(0) + +// Stands in for std::atomic with ONE scripted interference: the first compare-exchange +// runs the UI's consume (identity reinstalled, the window taken) and reports failure exactly as +// the real CAS does — expected updated to what the consume left. Everything after is ordinary. +struct ConsumingAccumulator { + float value; + float identity; + float consumed = -1.f; // what the injected consume took + int casCount = 0; + + float load(std::memory_order) const { return value; } + + bool compare_exchange_weak(float& expected, float desired, std::memory_order, + std::memory_order) { + if (casCount++ == 0) { + consumed = value; + value = identity; + expected = value; + return false; + } + value = desired; + return true; + } +}; + +static void testPeakWindowKeepsTheLoudestBlock() { + std::atomic acc{kMeterPeakIdentity}; + foldPeak(acc, 0.25f); + foldPeak(acc, 0.90f); + foldPeak(acc, 0.40f); // quieter than the window's max: must not lower it + CHECK(acc.load() == 0.90f); + CHECK(consumePeak(acc) == 0.90f); + // Consumed means a NEW window, not a carried-over one. + CHECK(acc.load() == kMeterPeakIdentity); + foldPeak(acc, 0.10f); + CHECK(consumePeak(acc) == 0.10f); +} + +static void testGainWindowKeepsTheDeepestReduction() { + std::atomic acc{kMeterGainIdentity}; + foldMinGain(acc, 0.80f); + foldMinGain(acc, 0.55f); + foldMinGain(acc, 0.95f); // shallower: must not raise the window + CHECK(acc.load() == 0.55f); + CHECK(consumeMinGain(acc) == 0.55f); + // 1.0, not 0.0 — an untouched gain window means "no reduction", and a 0 identity would + // report a total mute on every idle frame. + CHECK(acc.load() == kMeterGainIdentity); +} + +static void testBlocksAtOrBelowTheWindowLeaveItAlone() { + std::atomic acc{kMeterPeakIdentity}; + foldPeak(acc, 0.50f); + foldPeak(acc, 0.50f); + CHECK(acc.load() == 0.50f); + // The post-condition every fold owes, whichever way the comparison went. + foldPeak(acc, 0.20f); + CHECK(acc.load() >= 0.20f); +} + +static void testConsumeInsideAFoldStillLandsTheBlockInTheNewWindow() { + // The window holds a LOUDER peak than the block being folded — the exact case a + // load-compare-store fold skips, so the block would be lost when the consume lands + // between that load and the store it decided not to make. + ConsumingAccumulator acc{0.90f, kMeterPeakIdentity}; + foldPeak(acc, 0.40f); + CHECK(acc.consumed == 0.90f); // the UI got the window it was owed + CHECK(acc.value == 0.40f); // and the block reached the NEW window rather than vanishing + CHECK(acc.casCount == 2); // one interfering consume, exactly one retry + + // Same for the gain window: a block reducing LESS than the window's minimum is the one a + // skipping fold drops, and losing it reports "no reduction" over a block that had some. + ConsumingAccumulator gain{0.60f, kMeterGainIdentity}; + foldMinGain(gain, 0.90f); + CHECK(gain.consumed == 0.60f); + CHECK(gain.value == 0.90f); + CHECK(gain.casCount == 2); +} + +int main() { + testPeakWindowKeepsTheLoudestBlock(); + testGainWindowKeepsTheDeepestReduction(); + testBlocksAtOrBelowTheWindowLeaveItAlone(); + testConsumeInsideAFoldStillLandsTheBlockInTheNewWindow(); + if (g_fail == 0) std::printf("meter_accumulate tests passed\n"); + return g_fail == 0 ? 0 : 1; +}