From 6e2128e9370fd1aaf66aa027fbb608a6e090aac3 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Mon, 3 Aug 2026 15:00:40 -0400 Subject: [PATCH 1/2] =?UTF-8?q?view:=20split=20the=20deferred=20FX-park=20?= =?UTF-8?q?drain=20by=20kind=20=E2=80=94=20restores=20stay=20forced,=20par?= =?UTF-8?q?ks=20go=20one=20FX=20per=20idle=20tick=20behind=20a=201s=20coal?= =?UTF-8?q?escing=20delay?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A rapid A→B→A flip now costs no plugin work: the restore cancels the still-pending park outright. A park that has already written one FX carries a `partial` flag and is superseded by its inverse rather than cancelled, so a half-parked chain is never stranded. --- src/app/main.cpp | 9 +- src/shell/actions/design_view_actions.cpp | 10 +- src/shell/capture/render_in_place.cpp | 4 +- src/shell/view/CLAUDE.md | 27 ++-- src/shell/view/view_fx_park.cpp | 107 +++++++++++++--- src/shell/view/view_fx_park.h | 147 ++++++++++++++++------ tests/test_view_fx_park.cpp | 126 +++++++++++++++++-- 7 files changed, 341 insertions(+), 89 deletions(-) diff --git a/src/app/main.cpp b/src/app/main.cpp index 140ca94..8800002 100644 --- a/src/app/main.cpp +++ b/src/app/main.cpp @@ -196,9 +196,12 @@ static void OnTimer() } else { // A mode switch applies its visibility/routing writes synchronously and // leaves the per-FX offline work here, so the new mode paints before the - // plugins unload. Idle cost is one empty test. Skipped on a load tick so - // the reapply's own intents defer one tick like any other switch's. - reasampler::drainDeferredFxParks(); + // plugins move. Split by intent kind: restores whole (nothing may persist + // over a half-applied one), parks one FX per tick. Idle cost is one empty + // test each. Skipped on a load tick so the reapply's own intents defer one + // tick like any other switch's. + reasampler::drainDeferredFxRestores(); + reasampler::tickDeferredFxParks(); } reasampler::bankPanelRefresh(); // cheap fingerprint compare; no-op when unchanged/closed diff --git a/src/shell/actions/design_view_actions.cpp b/src/shell/actions/design_view_actions.cpp index 77b24e7..f525145 100644 --- a/src/shell/actions/design_view_actions.cpp +++ b/src/shell/actions/design_view_actions.cpp @@ -26,7 +26,7 @@ #include "shell/capture/track_guid.h" // shared MediaTrack* -> canonical GUID key #include "shell/panel/panel_window.h" // bankPanelInvalidate — footer toggle repaint #include "shell/view/view.h" // applyMode + mintManagedLanes (D2 shell) -#include "shell/view/view_fx_park.h" // drainDeferredFxParks — see persistViewState +#include "shell/view/view_fx_park.h" // drainDeferredFxRestores — see persistViewState #define REAPERAPI_MINIMAL #define REAPERAPI_WANT_CountSelectedTracks @@ -148,9 +148,11 @@ void persistViewState(PersistScope scope) { // BEFORE the model is serialized, and before the Save-As below can write a // .rpp: a deferred FX restore leaves the chain offline while the model has // already dropped the snapshot that would replan it. Why that combination is - // unrecoverable on reopen is at drainDeferredFxParks. Runs for BOTH scopes — - // the narrowed save still writes the key the half-applied park contradicts. - drainDeferredFxParks(); + // unrecoverable on reopen is at drainDeferredFxRestores. Restores only — a + // pending PARK is safe to save over and converges on reopen. Runs for BOTH + // scopes: the narrowed save still writes the key the half-applied restore + // contradicts. + drainDeferredFxRestores(); if (!g_session->view().membership().empty()) { ReaProject* proj = EnumProjects(-1, nullptr, 0); diff --git a/src/shell/capture/render_in_place.cpp b/src/shell/capture/render_in_place.cpp index e0434aa..bd7f8bf 100644 --- a/src/shell/capture/render_in_place.cpp +++ b/src/shell/capture/render_in_place.cpp @@ -22,7 +22,7 @@ #include "shell/panel/panel_input.h" // bankPanelTailSetting #include "shell/persist/session.h" #include "shell/view/view.h" // applyMode / mintManagedLanes -#include "shell/view/view_fx_park.h" // drainDeferredFxParks +#include "shell/view/view_fx_park.h" // drainDeferredFxRestores #define REAPERAPI_MINIMAL #define REAPERAPI_WANT_CountTrackMediaItems @@ -211,7 +211,7 @@ void RunRenderTrackInPlace(ReaSamplerSession& session) { // Persist outside the block. The offline render's own save gate already forced a // saved project, so the Save-As-guarded persist the Design View actions need // cannot have anything to prompt for here. - drainDeferredFxParks(); // the reapply above may have deferred a restore — see the contract there + drainDeferredFxRestores(); // the reapply above may have deferred a restore — contract there session.saveToActiveProject(); if (!placed) { diff --git a/src/shell/view/CLAUDE.md b/src/shell/view/CLAUDE.md index caa8743..7541c7b 100644 --- a/src/shell/view/CLAUDE.md +++ b/src/shell/view/CLAUDE.md @@ -54,14 +54,23 @@ decide membership or mode rules. lost. What it does change is the undo record: the offline writes land outside the switch's undo block, so the tool no longer re-drives them on an undo or a redo — what a Ctrl-Z then leaves the chain at is REAPER's own FX-state record, - `[verify — DAW]`. **The idle tick is not the only drain point.** Any path that - serializes the view model drains synchronously first (`persistViewState`, - `render_in_place`), because a save landing between a restore's synchronous flag - writes and its drain would record offline FX beside a model that no longer - carries the snapshot to replan them — unrecoverable on reopen. So an - action-driven switch does pay the FX hitch before it returns; the repaint and - the 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. + `[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. + `[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. - **Stated DEVIATION — the undo mask does not keep FX out of a real switch.** The apply mask (`kApplyUndoMask`) drops `UNDO_STATE_FX` and ORs it back in when a driven flag in that domain moved; the only such flag is `I_FXEN`, which every @@ -107,7 +116,7 @@ applies the resulting lane state to live tracks. ## Modules - `view` — Design View shell: snapshots flag values before parking, drives hide + CPU-park on inactive-mode leaves (`B_SHOWINTCP`/`B_SHOWINMIXER`/`B_MAINSEND`/`I_FXEN`, with per-FX offline deferred to `view_fx_park`), restores from snapshot. Owns the one discriminator (`target != active`) that separates a real switch from a reapply, and with it both the playback gate (`transportBlocksModeSwitch`) and the solo cache/clear/restore seams. **Never touches master or `B_MUTE`.** -- `view_fx_park` — the per-FX offline surface: the `TrackFX_GetFXGUID` identity read snapshot/park/restore share, the deferred intent queue that keeps `TrackFX_SetOffline` off the switch's synchronous path (at most one intent per track GUID, latest wins, an intent landing on its own pending inverse cancels it), and the idle-tick drain `main.cpp`'s `OnTimer` calls. **The drain owns no model state.** A snapshot is dropped where the restore is PLANNED — the flags are back at their captured values from that moment, and a model that still described the track as parked would let a persist or a reapply inside the drain window replan a restore over whatever the user changed since. What the deferral costs instead is that the live FX chain stops being a trustworthy snapshot source while an intent is pending: a park that CANCELS a pending restore takes the pre-park FX states from that restore's ops (`preParkFxFromCancelledRestore`), because the chain still reads the parked values and the cancel means no drain will ever fix them. +- `view_fx_park` — the per-FX offline surface: the `TrackFX_GetFXGUID` identity read snapshot/park/restore share, the deferred intent queue that keeps `TrackFX_SetOffline` off the switch's synchronous path (at most one intent per track GUID, latest wins, an intent landing on its own pending inverse cancels it — which is what makes a rapid A→B→A flip cost zero plugin work), and the two drains `main.cpp`'s `OnTimer` calls: `drainDeferredFxRestores` (whole, also forced by every persist path) and `tickDeferredFxParks` (one FX per tick, after a coalescing delay). A park that has already written one FX can no longer be cancelled outright in either direction — the live chain then matches neither endpoint, so the intent carries a `partial` flag and its inverse SUPERSEDES it instead. **The drains own no model state.** A snapshot is dropped where the restore is PLANNED — the flags are back at their captured values from that moment, and a model that still described the track as parked would let a persist or a reapply inside the drain window replan a restore over whatever the user changed since. What the deferral costs instead is that the live FX chain stops being a trustworthy snapshot source while an intent is pending: a park that CANCELS a pending restore takes the pre-park FX states from that restore's ops (`preParkFxFromCancelledRestore`), because the chain still reads the parked values and the cancel means no drain will ever fix them. - `view_solo` — the `I_SOLO` read/write pair behind the per-mode solo surface, plus `clearTrackSolos`/`restoreTrackSolos`, the outgoing-clear and incoming-replay entry points `view` drives them through. Holds no policy: what to cache, clear, or replay is `core/view/solo_cache`. ## Gotchas diff --git a/src/shell/view/view_fx_park.cpp b/src/shell/view/view_fx_park.cpp index 467a80e..fc30552 100644 --- a/src/shell/view/view_fx_park.cpp +++ b/src/shell/view/view_fx_park.cpp @@ -22,15 +22,25 @@ #define REAPERAPI_WANT_TrackFX_SetOffline #define REAPERAPI_WANT_ValidatePtr2 #define REAPERAPI_WANT_guidToString +#define REAPERAPI_WANT_time_precise #include "reaper_plugin_functions.h" namespace reasampler { namespace { +// How long a fresh park intent waits before its first FX moves. Long enough to +// 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. +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 // 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 @@ -61,9 +71,42 @@ void setOfflineIfChanged(MediaTrack* tr, int fx, bool offline) { TrackFX_SetOffline(tr, fx, offline); } -void applyPark(MediaTrack* tr) { +// The park's next unit of work, or -1 when the track is fully parked. Derived +// from the live chain rather than a stored cursor so a chain edited between two +// ticks cannot leave the park writing past the end of it or skipping a slot. +int firstOnlineFx(MediaTrack* tr) { const int fxCount = TrackFX_GetCount(tr); - for (int fx = 0; fx < fxCount; ++fx) setOfflineIfChanged(tr, fx, true); + for (int fx = 0; fx < fxCount; ++fx) + if (!TrackFX_GetOffline(tr, fx)) return fx; + 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. +MediaTrack* findTrack(ReaProject* proj, const std::string& guid) { + const int count = CountTracks(proj); + for (int i = 0; i < count; ++i) { + MediaTrack* tr = GetTrack(proj, i); + if (tr && guidString(tr) == guid) return tr; + } + return nullptr; +} + +// Shared entry gate for both drains: nothing pending, someone already draining, +// or a queue belonging to a project that is no longer current — the last of +// which discards. Catches only a DIFFERENT live pointer; a REAPER-recycled +// address is caught upstream, by the caller discarding on the session's own load +// transition. +bool drainGateOpen() { + if (g_queue.empty()) return false; + if (g_draining) return false; // re-entered mid-apply: those intents are the next pass's + if (g_owner != currentProject()) { + discardDeferredFxParks(); + return false; + } + return true; } // Restores per-FX offline from the plan verbatim — never a blanket "online". @@ -124,6 +167,9 @@ PreParkFx snapshotFxOffline(MediaTrack* tr, const std::vector& canc 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; return g_queue.park(guid); } @@ -137,19 +183,8 @@ void discardDeferredFxParks() { g_owner = nullptr; } -void drainDeferredFxParks() { - if (g_queue.empty()) return; - if (g_draining) return; // re-entered mid-apply: those intents are the outer drain's next pass - - if (g_owner != currentProject()) { - // The project the intents were planned against was closed or switched - // away from: its tracks are not ours to write and its handles may be - // gone. Discard rather than apply. This catches only a DIFFERENT live - // pointer; a REAPER-recycled address is caught upstream, by the caller - // discarding on the session's own load transition. - discardDeferredFxParks(); - return; - } +void drainDeferredFxRestores() { + if (!drainGateOpen()) return; ReaProject* const proj = g_owner; const DrainScope scope; @@ -158,7 +193,8 @@ void drainDeferredFxParks() { // the message loop (plugins load and unload), so a re-entrant switch can // enqueue while this runs. Everything defensive below rests on that one // assumption; each piece is correct regardless of whether it holds. - const std::vector draining = g_queue.take(); + const std::vector draining = g_queue.takeRestores(); + if (draining.empty()) return; // parks only — not this half's work std::unordered_map byGuid; const int count = CountTracks(proj); @@ -185,10 +221,6 @@ void drainDeferredFxParks() { if (!ValidatePtr2(nullptr, proj, "ReaProject*")) return; if (!ValidatePtr2(proj, it->second, "MediaTrack*")) continue; - if (intent.park) { - applyPark(it->second); - continue; - } const FxRestoreDrops d = applyRestore(it->second, intent.restoreOps); if (d.total() > 0) { drops.add(d); @@ -205,4 +237,39 @@ void drainDeferredFxParks() { if (!fxDropMsg.empty()) ShowConsoleMsg(("!SHOW:" + fxDropMsg).c_str()); } +void tickDeferredFxParks() { + if (!drainGateOpen()) return; + if (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 + + ReaProject* const proj = g_owner; + const DrainScope scope; + + // Validated before the enumeration below reads the project, and again for the + // track it resolves — same gate, and the same reason, as the restore drain's + // per-intent re-validation: a longer park window makes a deleted track + // likelier, not less. + if (!ValidatePtr2(nullptr, proj, "ReaProject*")) return; + MediaTrack* tr = findTrack(proj, guid); + if (!tr || !ValidatePtr2(proj, tr, "MediaTrack*")) { + g_queue.finishPark(guid); // track deleted since the switch — prune + return; + } + + const int fx = firstOnlineFx(tr); + if (fx < 0) { + g_queue.finishPark(guid); // fully parked + return; + } + + // BEFORE the write, not after: the write may pump the message loop, and a + // restore arriving in that window has to supersede this park rather than + // cancel it outright — the FX about to go offline would otherwise be left + // there with the only record of its prior state dropped. + g_queue.markPartial(guid); + TrackFX_SetOffline(tr, fx, true); +} + } // namespace reasampler diff --git a/src/shell/view/view_fx_park.h b/src/shell/view/view_fx_park.h index aebbf6d..15a62f7 100644 --- a/src/shell/view/view_fx_park.h +++ b/src/shell/view/view_fx_park.h @@ -1,8 +1,9 @@ #pragma once // Design View's per-FX offline surface: the FX-identity read that snapshot, // park and restore all address FX through, the deferred intent queue that keeps -// TrackFX_SetOffline off the mode switch's synchronous path, and the idle-tick -// drain that applies it. See src/shell/view/CLAUDE.md's FX-parking caveat. +// TrackFX_SetOffline off the mode switch's synchronous path, and the two drains +// that apply it — restores whole, parks one FX per idle tick. See +// src/shell/view/CLAUDE.md's FX-parking caveat. #include #include @@ -24,17 +25,27 @@ std::vector liveFxGuids(MediaTrack* tr); // One deferred per-FX intent for one track. A park carries no ops (every live // slot goes offline); a restore carries the planned ops verbatim. struct FxParkIntent { - std::string guid; - bool park = false; + 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. + bool partial = false; std::vector restoreOps; }; // The queue's re-entrancy rule, pure so it can be asserted without a DAW: at // most ONE intent per track GUID, and the latest one wins. Park and restore are -// inverses, so an intent landing on its own pending inverse CANCELS it rather -// than stacking — the queued work never ran, so the track already holds the -// state the newcomer asks for, and replaying both would be both slower and -// observably wrong. +// inverses, so an UNSTARTED intent landing on its own pending inverse CANCELS it +// rather than stacking — the queued work never ran, so the track already holds +// the state the newcomer asks for, and replaying both would be both slower and +// observably wrong. That cancel is what makes a rapid A→B→A mode flip cost zero +// plugin work: B→A's restore annihilates A's park before a single FX moved. class FxParkQueue { public: // Returns the ops of a pending restore this park CANCELLED, empty otherwise. @@ -49,47 +60,83 @@ public: std::vector park(const std::string& guid) { FxParkIntent* held = find(guid); if (!held) { - pending_.push_back(FxParkIntent{guid, true, {}}); + pending_.push_back(FxParkIntent{guid, true, false, {}}); return {}; } if (held->park) return {}; std::vector cancelled = std::move(held->restoreOps); - erase(held); + if (held->partial) *held = FxParkIntent{guid, true, true, {}}; + else erase(held); return cancelled; } - // A restore cancelling a pending park is COMPLETE at that point — the park - // never ran, so no drain will ever come for this GUID. + // A restore cancelling an UNSTARTED park is COMPLETE at that point — the + // park never ran, so no drain will ever come for this GUID. Against a park + // that has already written, the restore supersedes instead: its ops are the + // only thing that can put the offlined FX back. void restore(const std::string& guid, std::vector ops) { FxParkIntent* held = find(guid); if (!held) { - pending_.push_back(FxParkIntent{guid, false, std::move(ops)}); - } else if (held->park) { + pending_.push_back(FxParkIntent{guid, false, false, std::move(ops)}); + } else if (held->park && !held->partial) { erase(held); + } else if (held->park) { + *held = FxParkIntent{guid, false, true, std::move(ops)}; } else { held->restoreOps = std::move(ops); } } - // Enqueue order, which is apply order: park before restore within one - // switch, as the synchronous body already orders them. + // Enqueue order, which is apply order WITHIN a kind — the two kinds drain + // separately and on different schedules (see the two drains below). const std::vector& pending() const { return pending_; } bool empty() const { return pending_.empty(); } void clear() { pending_.clear(); } - // Detaches everything pending, leaving the queue able to accept intents - // enqueued WHILE the caller applies what it took. [verify — DAW] applying - // loads/unloads plugins, which is ASSUMED to pump the message loop, so a - // re-entrant switch can enqueue mid-apply: iterating the live queue would - // dangle on the push_back, and clearing it afterwards would discard - // whatever arrived during the apply. The detach is correct either way; only - // the need for it is unconfirmed. - std::vector take() { + // Detaches every pending RESTORE and leaves the parks behind, in order. The + // detach — not a live iteration, not a clear afterwards — is what lets the + // queue accept intents enqueued WHILE the caller applies what it took: + // [verify — DAW] applying loads/unloads plugins, which is ASSUMED to pump + // the message loop, so a re-entrant switch can enqueue mid-apply. Iterating + // the live queue would dangle on the push_back and clearing it afterwards + // would discard whatever arrived. Correct either way; only the need for it + // is unconfirmed. + std::vector takeRestores() { std::vector taken; - taken.swap(pending_); + std::vector kept; + for (FxParkIntent& i : pending_) { + if (i.park) kept.push_back(std::move(i)); + else taken.push_back(std::move(i)); + } + pending_.swap(kept); return taken; } + // The park the drain should work next, in enqueue order; empty when none is + // pending. A PEEK, by value — an apply can reshape the queue under a pointer + // — and it marks nothing: a park still cancellable for free stays that way + // until markPartial says a write is imminent. + std::string nextParkGuid() const { + for (const FxParkIntent& i : pending_) + if (i.park) return i.guid; + return {}; + } + + // Records that a park write is ABOUT TO land on `guid` — set before the + // write, because the write may pump the message loop and an intent arriving + // in that window must already see a chain that matches neither endpoint. + void markPartial(const std::string& guid) { + if (FxParkIntent* held = find(guid)) held->partial = true; + } + + // Retires a park with nothing left to offline. A no-op when a restore + // replaced it mid-apply — that restore is the newer intent and owns the + // chain from here. + void finishPark(const std::string& guid) { + FxParkIntent* held = find(guid); + if (held && held->park) erase(held); + } + private: FxParkIntent* find(const std::string& guid) { for (FxParkIntent& i : pending_) @@ -147,22 +194,42 @@ PreParkFx snapshotFxOffline(MediaTrack* tr, const std::vector& canc std::vector deferFxPark(ReaProject* proj, const std::string& guid); void deferFxRestore(ReaProject* proj, const std::string& guid, std::vector ops); -// Applies every pending intent. Touches NO model state — a restore's snapshot is -// dropped where the restore is planned (applyMode). Discards the queue unapplied -// if the project it was enqueued against is no longer current (close / switch); -// an intent whose track is gone is pruned. Idle cost is one empty-queue test. -// Re-entrant calls early-out: one drain at a time, the outer one owns the queue. +// The two halves of the drain, split because only ONE of them is unsafe to +// persist over. Both touch NO model state — a restore's snapshot is dropped +// where the restore is planned (applyMode) — both discard the queue unapplied if +// the project it was enqueued against is no longer current (close / switch), +// both prune an intent whose track is gone, both cost one empty-queue test when +// idle, and both early-out re-entrantly: one drain at a time, the outer one owns +// the queue. // -// Called on the idle tick AND synchronously before the view model is serialized -// (persistViewState, render_in_place). The second call is not an optimization: -// between a restore's synchronous flag writes and its drain the FX are still -// offline while the model has already dropped the snapshot that would replan -// them, so a save inside that window — deterministic under a custom action chain -// like "toggle mode; save project" — records offline FX beside a snapshot-free -// model, and nothing on reopen brings them back online. The cost is that an -// action-driven switch pays the FX hitch before it returns; its repaint and undo -// block have both closed by then, which is what the deferral was for. -void drainDeferredFxParks(); +// RESTORES, applied whole, synchronously. Called on the idle tick AND before the +// view model is serialized (persistViewState, render_in_place). The second call +// is not an optimization: between a restore's synchronous flag writes and its +// drain the FX are still offline while the model has already dropped the +// snapshot that would replan them, so a save inside that window — deterministic +// under a custom action chain like "toggle mode; save project" — records offline +// FX beside a snapshot-free model, and nothing on reopen brings them back +// online. The cost is that an action-driven switch pays the RELOAD hitch before +// it returns; its repaint and undo block have both closed by then, which is what +// the deferral was for. +void drainDeferredFxRestores(); + +// PARKS, one FX per idle tick behind a short coalescing delay. Nothing waits on +// a park, and unlike a restore it is safe to persist over: the flags are already +// parked and the snapshot already stored, so a .rpp saved mid-window reopens with +// the FX online, applyMode replans a park for that inactive leaf, and the drain +// offlines it. It converges on its own, which is why this half does not force. +// +// One FX, not one track: a single convolution reverb or loaded sampler is the +// unit of cost, so per-track chunking would not bound the hitch. +// +// The delay's cost is the width of a documented hazard, not a new one. applyPark +// enumerates the chain LIVE at drain time while the pre-park snapshot was taken +// at switch time, so an FX added inside the window is offlined carrying no +// snapshot entry — resolveFxRestore then has no op for it and it stays offline. +// Same failure as the cancelled-restore case below, over a longer window; keep +// the delay short. +void tickDeferredFxParks(); // Drops every pending intent without applying it. Called when the model the // intents were planned against has been replaced (project load/switch, undo/redo diff --git a/tests/test_view_fx_park.cpp b/tests/test_view_fx_park.cpp index 8dec3ac..4e10791 100644 --- a/tests/test_view_fx_park.cpp +++ b/tests/test_view_fx_park.cpp @@ -4,7 +4,9 @@ // The properties under test: a mode switch leaves its per-FX offline work here, // so a second switch arriving before the first drained must leave every track in // the state the SECOND switch specifies — never the first's, never both replayed; -// and a cancel must not strand the pre-park FX state it was the last record of. +// a cancel must not strand the pre-park FX state it was the last record of; and +// the restore/park kind split must hold, restores detaching whole while parks +// stay queued across the ticks that apply them one FX at a time. #include "../src/shell/view/view_fx_park.h" @@ -233,30 +235,126 @@ static void testRestorePlanOpsRebuildTheSlotKeyedSnapshotVerbatim() { CHECK(rebuilt.states == snap.fxOffline); } -// -- re-entrancy ------------------------------------------------------------- +// -- the kind split ---------------------------------------------------------- -static void testTakeDetachesEverythingAndLeavesTheQueueEmpty() { +static void testTakeRestoresDetachesRestoresAndLeavesParksQueued() { + // The forced drain's slice: a restore is unsafe to persist over and goes now, + // a park is safe and stays for the idle tick to work one FX at a time. FxParkQueue q; q.park("{A}"); q.restore("{B}", ops("{FX}", true)); + q.park("{C}"); + q.restore("{D}", ops("{FX}", false)); - const std::vector taken = q.take(); + const std::vector taken = q.takeRestores(); CHECK(taken.size() == 2); - CHECK(taken.size() == 2 && taken[0].guid == "{A}" && taken[0].park); - CHECK(taken.size() == 2 && taken[1].guid == "{B}" && !taken[1].park); - CHECK(q.empty()); + CHECK(taken.size() == 2 && taken[0].guid == "{B}" && !taken[0].park); + CHECK(taken.size() == 2 && taken[1].guid == "{D}" && !taken[1].park); + // Enqueue order is apply order on BOTH sides of the slice. + CHECK(q.pending().size() == 2); + CHECK(q.pending().size() == 2 && q.pending()[0].guid == "{A}" && q.pending()[0].park); + CHECK(q.pending().size() == 2 && q.pending()[1].guid == "{C}" && q.pending()[1].park); } +static void testTakeRestoresWithOnlyParksQueuedTakesNothing() { + FxParkQueue q; + q.park("{A}"); + + CHECK(q.takeRestores().empty()); + CHECK(q.pending().size() == 1); // the park is not this drain's to consume +} + +static void testNextParkGuidWalksParksInEnqueueOrderAndSkipsRestores() { + FxParkQueue q; + q.restore("{R}", ops("{FX}", false)); + q.park("{A}"); + q.park("{B}"); + + // A PEEK: repeated reads answer the same until the park is retired, and a + // restore is never handed to the park tick. + CHECK(q.nextParkGuid() == "{A}"); + CHECK(q.nextParkGuid() == "{A}"); + q.finishPark("{A}"); + CHECK(q.nextParkGuid() == "{B}"); + q.finishPark("{B}"); + CHECK(q.nextParkGuid().empty()); + CHECK(q.pending().size() == 1); // park progress never consumed the restore + CHECK(intentFor(q, "{R}") != nullptr); +} + +static void testFinishParkNeverRetiresARestoreStandingAtThatGuid() { + FxParkQueue q; + q.park("{A}"); + q.markPartial("{A}"); + q.restore("{A}", ops("{FX}", false)); + + q.finishPark("{A}"); + + CHECK(q.pending().size() == 1); + CHECK(intentFor(q, "{A}") && !intentFor(q, "{A}")->park); +} + +// -- the lazy park's cancel semantics ---------------------------------------- + +static void testAnUnstartedParkCancelledByItsRestoreCostsZeroWork() { + // The headline property of deferring the park: A→B parks {A}, B→A restores it + // before the coalescing delay let one FX move, and the flip costs no plugin + // load or unload at all — neither half of the drain has anything left to do. + FxParkQueue q; + q.park("{A}"); + q.restore("{A}", ops("{FX}", false)); + + CHECK(q.empty()); + CHECK(q.nextParkGuid().empty()); + CHECK(q.takeRestores().empty()); +} + +static void testAParkThatAlreadyWroteIsSupersededByItsRestoreNotCancelled() { + // One FX is offline, so the chain matches NEITHER endpoint. Annihilating here + // would leave it offline with the ops that describe its prior state dropped. + FxParkQueue q; + q.park("{A}"); + CHECK(q.nextParkGuid() == "{A}"); + q.markPartial("{A}"); // the tick is about to write this track's first FX + + q.restore("{A}", ops("{FX}", false)); + + CHECK(q.pending().size() == 1); + const FxParkIntent* held = intentFor(q, "{A}"); + CHECK(held && !held->park); + CHECK(held && held->restoreOps.size() == 1 && held->restoreOps.front().fxGuid == "{FX}"); + CHECK(q.nextParkGuid().empty()); // no park work left — the restore owns the chain +} + +static void testAParkOnASupersededRestoreResumesRatherThanAnnihilates() { + // The mirror hazard: the restore also never ran, so the chain is still half + // parked. Cancelling outright would strand every FX the first pass had not + // reached yet — the park must resume, and still hand back the pre-park ops. + FxParkQueue q; + q.park("{A}"); + q.markPartial("{A}"); + q.restore("{A}", ops("{FX}", false)); + + const std::vector cancelled = q.park("{A}"); + + CHECK(cancelled.size() == 1); + CHECK(cancelled.size() == 1 && cancelled.front().fxGuid == "{FX}"); + CHECK(q.nextParkGuid() == "{A}"); + CHECK(intentFor(q, "{A}") && intentFor(q, "{A}")->park); +} + +// -- re-entrancy ------------------------------------------------------------- + static void testIntentsArrivingDuringADrainSurviveIt() { // [verify — DAW] applying an intent loads/unloads plugins, which is ASSUMED to // pump the message loop, so a switch can re-enter and enqueue mid-drain. Those // intents belong to the NEXT drain — the one in progress must neither see them // nor discard them. FxParkQueue q; - q.park("{A}"); + q.restore("{A}", ops("{FX}", true)); - const std::vector draining = q.take(); + const std::vector draining = q.takeRestores(); q.restore("{B}", ops("{FX}", false)); // arrives while {A} is being applied CHECK(draining.size() == 1); @@ -271,7 +369,7 @@ static void testAReEntrantParkCancelsOnlyWhatIsStillPending() { // silently annihilate against an intent that has already been applied. FxParkQueue q; q.restore("{A}", ops("{FX}", false)); - q.take(); + q.takeRestores(); const std::vector cancelled = q.park("{A}"); @@ -297,7 +395,13 @@ int main() { testSlotKeyedRestoreDoesNotBecomeIdentityKeyedWithNoIdentities(); testRestorePlanOpsRebuildTheIdentityKeyedSnapshotVerbatim(); testRestorePlanOpsRebuildTheSlotKeyedSnapshotVerbatim(); - testTakeDetachesEverythingAndLeavesTheQueueEmpty(); + testTakeRestoresDetachesRestoresAndLeavesParksQueued(); + testTakeRestoresWithOnlyParksQueuedTakesNothing(); + testNextParkGuidWalksParksInEnqueueOrderAndSkipsRestores(); + testFinishParkNeverRetiresARestoreStandingAtThatGuid(); + testAnUnstartedParkCancelledByItsRestoreCostsZeroWork(); + testAParkThatAlreadyWroteIsSupersededByItsRestoreNotCancelled(); + testAParkOnASupersededRestoreResumesRatherThanAnnihilates(); testIntentsArrivingDuringADrainSurviveIt(); testAReEntrantParkCancelsOnlyWhatIsStillPending(); From 761125d0fe175d84b08d39b641ef59458371fde2 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Mon, 3 Aug 2026 15:50:07 -0400 Subject: [PATCH 2/2] 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