From 761125d0fe175d84b08d39b641ef59458371fde2 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Mon, 3 Aug 2026 15:50:07 -0400 Subject: [PATCH] view: assert the FX-park coalescing delay's debounce; correct four overclaiming doc/comment claims MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extracts parkReadyAt/parkIsReady as a tested pure fold per PLAN.md's phase criterion; the rest is wording fixes — hitch bound, hazard width, forced-drain scope, progressive CPU reclaim. --- src/shell/view/CLAUDE.md | 37 +++++++++----- src/shell/view/view_fx_park.cpp | 24 +++++---- src/shell/view/view_fx_park.h | 86 +++++++++++++++------------------ tests/test_view_fx_park.cpp | 36 ++++++++++++-- 4 files changed, 114 insertions(+), 69 deletions(-) diff --git a/src/shell/view/CLAUDE.md b/src/shell/view/CLAUDE.md index 7541c7b..6e6457d 100644 --- a/src/shell/view/CLAUDE.md +++ b/src/shell/view/CLAUDE.md @@ -57,17 +57,32 @@ decide membership or mode rules. `[verify — DAW]`. **The drain is SPLIT BY INTENT KIND, because only one kind is unsafe to persist over.** A pending RESTORE is: flags restored, snapshot already dropped, FX still offline — a save inside that window records offline FX beside a - model that can no longer replan them, unrecoverable on reopen. So any path that - serializes the view model drains every restore synchronously first - (`persistViewState`, `render_in_place`), and an action-driven switch still pays - the RELOAD hitch before it returns — its repaint and undo block have both closed - by then, which is what the deferral was for. Any new caller that reapplies a mode - and then persists inherits this obligation. A pending PARK is safe to save over: - flags parked, snapshot stored, FX still online, so a reopen replans the park and - the drain offlines it — it converges. Parks therefore go lazy, ONE FX per idle - tick behind a one-second coalescing delay, so the unload half never blocks the - UI at all. The delay is also the width of the live-enumeration hazard at - `tickDeferredFxParks` — do not lengthen it casually. + model that can no longer replan them, unrecoverable on reopen. `persistViewState` + and `render_in_place` are the only two paths that force the restore drain before + they serialize; roughly ten other `saveToActiveProject` callers (`ingest.cpp`, + `bank_ops.cpp`, `bake_land.cpp`, `capture_batch.cpp`, `capture_orchestrator.cpp`, + `realtime_lifecycle.cpp`, `panel_input.cpp`) persist without forcing it first — a + pre-existing, roughly one-tick-wide gap (the idle tick drains restores too), + not known to be exploitable in practice. A new caller that reapplies a mode and + then persists inherits the forced-drain obligation; one that merely persists does + not. `drainDeferredFxRestores` is also a silent no-op while a drain is already in + flight (`g_draining`), so a restore forced from inside another drain's + message-pump window can itself be skipped — same order of window, same + pre-existing status. An action-driven switch still pays the RELOAD hitch before + it returns — its repaint and undo block have both closed by then, which is what + the deferral was for. A pending PARK is safe to save over: flags parked, snapshot + stored, FX still online, so a reopen replans the park and the drain offlines it — + it converges. Parks therefore go lazy, ONE FX per idle tick behind a one-second + coalescing delay, which bounds each tick's hitch to one FX's unload rather than + eliminating it — see `view_fx_park.h` for why per-track chunking would not help. + CPU reclaim is therefore PROGRESSIVE, completing over FX-count ticks rather than + in one step: a user watching the CPU meter after a switch sees a ramp, not a + drop — in tension with `design-view.md`'s full-CPU-park rationale ("a heavy FX + bench you don't want taxing the CPU while you arrange"). The delay is only ONE term + of the live-enumeration hazard's width: `firstOnlineFx` re-enumerates the chain + every tick, not just the first, so the actual exposure is delay + (FX count × + tick interval), and for a large chain the second term dominates — see + `tickDeferredFxParks`. Do not lengthen the delay casually. `[verify — DAW]` whether the switch's `UpdateArrange` paints before the first drain tick: nothing pumps the message loop between them, so "the switch paints first" is an assumption, not an SDK guarantee. diff --git a/src/shell/view/view_fx_park.cpp b/src/shell/view/view_fx_park.cpp index fc30552..c1bb0b9 100644 --- a/src/shell/view/view_fx_park.cpp +++ b/src/shell/view/view_fx_park.cpp @@ -33,14 +33,19 @@ namespace { // swallow a rapid A→B→A flip whole — that flip's restore then cancels the park // outright and the plugins never move at all — and short enough that the window // in which a user could add an FX to the track the switch just hid, and so miss -// the pre-park snapshot, stays implausible. Seconds, not minutes: this delay is -// the width of the hazard documented at tickDeferredFxParks. +// the pre-park snapshot, stays implausible. Seconds, not minutes; this is ONE +// term of the hazard's width — see tickDeferredFxParks for the other. constexpr double kParkCoalesceSeconds = 1.0; FxParkQueue g_queue; ReaProject* g_owner = nullptr; // the project the pending intents were enqueued against bool g_draining = false; -double g_parkReadyAt = 0.0; // time_precise() stamp; re-armed by every enqueued park +// The gate is GLOBAL, not per-track: a park enqueued for track Z re-arms this +// same stamp even though it does nothing to an already-partial park on track A. +// Benign — that track's FX are already I_FXEN=0/B_MAINSEND=0 from the +// synchronous half, so only its CPU reclaim (not correctness) is delayed — but +// this is not a per-intent debounce. +double g_parkReadyAt = 0.0; // parkReadyAt() stamp; re-armed by every enqueued park // Makes "one drain at a time" explicit rather than implied by the call sites. // RAII because an apply can throw and a stuck flag would silence the queue for @@ -81,10 +86,11 @@ int firstOnlineFx(MediaTrack* tr) { return -1; } -// One track by GUID. The park tick resolves ONE track and does so afresh every -// tick — a whole-project map would be rebuilt per tick for a single lookup, and -// caching the handle across ticks is exactly the dangle ValidatePtr2 is here to -// prevent. +// One track by GUID, resolved afresh every tick — caching the handle across +// ticks is exactly the dangle ValidatePtr2 is here to prevent. Still O(tracks), +// still one guidString() per track per tick; a per-tick unordered_map keyed by +// GUID would cost the same enumeration to build for a single lookup, so the only +// thing avoided by NOT building one is that allocation. MediaTrack* findTrack(ReaProject* proj, const std::string& guid) { const int count = CountTracks(proj); for (int i = 0; i < count; ++i) { @@ -169,7 +175,7 @@ std::vector deferFxPark(ReaProject* proj, const std::string& guid) adoptOwner(proj); // Re-armed, not set once: while switches keep arriving the park keeps // waiting, which is the whole point of coalescing them. - g_parkReadyAt = time_precise() + kParkCoalesceSeconds; + g_parkReadyAt = parkReadyAt(time_precise(), kParkCoalesceSeconds); return g_queue.park(guid); } @@ -239,7 +245,7 @@ void drainDeferredFxRestores() { void tickDeferredFxParks() { if (!drainGateOpen()) return; - if (time_precise() < g_parkReadyAt) return; // still coalescing + if (!parkIsReady(time_precise(), g_parkReadyAt)) return; // still coalescing const std::string guid = g_queue.nextParkGuid(); if (guid.empty()) return; // restores only — not this half's work diff --git a/src/shell/view/view_fx_park.h b/src/shell/view/view_fx_park.h index 15a62f7..954b67f 100644 --- a/src/shell/view/view_fx_park.h +++ b/src/shell/view/view_fx_park.h @@ -16,6 +16,15 @@ class ReaProject; namespace reasampler { +// The coalescing delay's gate, pure so the debounce can be asserted without a +// DAW clock (test_view_fx_park.cpp). parkReadyAt is called on EVERY enqueue, +// not just the first — that re-arming from each call's own `now` is what makes +// it a debounce rather than a one-shot timer, and is what lets a rapid A→B→A +// flip (three enqueues inside one delay window) cost a single wait measured +// from the last flip rather than three, or one anchored to the first. +inline double parkReadyAt(double now, double coalesceSeconds) { return now + coalesceSeconds; } +inline bool parkIsReady(double now, double readyAt) { return now >= readyAt; } + // The chain as it stands now: identity by current slot. Snapshot, park and // restore all address FX through this one plain 0..TrackFX_GetCount-1 // enumeration — never the 0x1000000/0x2000000 input-FX or container forms — so @@ -27,14 +36,11 @@ std::vector liveFxGuids(MediaTrack* tr); struct FxParkIntent { std::string guid; bool park = false; - // A park write is landing or has landed on this track, so the live chain - // matches NEITHER endpoint. Parks apply one FX per idle tick, so this is a - // state a park genuinely sits in — and while it holds, the cancel rule - // below is unsafe in both directions: only the restore's own ops can put - // back what a half-finished park offlined, and a park arriving on that - // restore must RESUME rather than annihilate or it strands every FX the - // first pass had not reached. Restores carry no such state: they are - // detached whole and applied in one drain. + // Set once a park write has landed on this track — the live chain then + // matches NEITHER endpoint, so the cancel-on-inverse rule below is unsafe + // until the intent resumes/supersedes instead of annihilating. Restores + // carry no such state: they are detached whole and applied in one drain. + // Full contract: src/shell/view/CLAUDE.md's `view_fx_park` entry. bool partial = false; std::vector restoreOps; }; @@ -158,22 +164,16 @@ struct PreParkFx { // The FX half a fresh pre-park snapshot must carry when the park CANCELLED a // pending restore: those ops are the only surviving record of the pre-park -// state, because the chain still reads the parked values until that restore -// drains — and it never will, the cancel dropped it. Empty in (nothing was -// cancelled) means the caller reads the live chain instead. The restore's own -// keying travels with it so a slot-keyed snapshot lifted from a legacy -// view_state does not silently become an identity-keyed one with no identities. -// -// FAITHFUL TO THE SNAPSHOT, NOT THE CHAIN. The ops describe the chain as it was -// at the ORIGINAL park; applyPark enumerates it again at drain time. So an FX -// added while the track was parked (a floating FX-chain window, ReaScript) is -// absent from this reconstruction yet IS offlined by the cancelling park's -// drain — and so never comes back online. Rare, and recoverable by hand in the -// FX chain, but specific to the deferral. -// -// `offline` is already boolean by the time it arrives: makeRestorePlan narrowed -// FxOfflineState's defensive int to FxOfflineOp's bool, so this widening back to -// int restores the type, not lost information. +// state, because the chain still reads the parked values and the cancel means +// no drain will ever put them back. Empty (nothing cancelled) means the caller +// reads the live chain instead; the restore's own keying travels with it so a +// slot-keyed snapshot does not silently become identity-keyed with no +// identities. The ops describe the chain as of the ORIGINAL park, not the live +// chain applyPark re-enumerates at drain time — so an FX added while parked (a +// floating FX-chain window, ReaScript) is absent here yet still offlined by the +// cancelling park's drain, and never comes back online. Rare and hand-recoverable. +// `offline` widens back from FxOfflineOp's bool to FxOfflineState's defensive int +// — restoring the type, not adding information. inline PreParkFx preParkFxFromCancelledRestore(const std::vector& cancelled) { PreParkFx out; if (cancelled.empty()) return out; @@ -202,33 +202,27 @@ void deferFxRestore(ReaProject* proj, const std::string& guid, std::vector