diff --git a/src/app/main.cpp b/src/app/main.cpp index 3f616aa..4dd9a05 100644 --- a/src/app/main.cpp +++ b/src/app/main.cpp @@ -206,12 +206,9 @@ 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 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(); + // 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(); } 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 f525145..77b24e7 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" // drainDeferredFxRestores — see persistViewState +#include "shell/view/view_fx_park.h" // drainDeferredFxParks — see persistViewState #define REAPERAPI_MINIMAL #define REAPERAPI_WANT_CountSelectedTracks @@ -148,11 +148,9 @@ 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 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(); + // unrecoverable on reopen is at drainDeferredFxParks. Runs for BOTH scopes — + // the narrowed save still writes the key the half-applied park contradicts. + drainDeferredFxParks(); 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 bd7f8bf..e0434aa 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" // drainDeferredFxRestores +#include "shell/view/view_fx_park.h" // drainDeferredFxParks #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. - drainDeferredFxRestores(); // the reapply above may have deferred a restore — contract there + drainDeferredFxParks(); // the reapply above may have deferred a restore — see the contract there session.saveToActiveProject(); if (!placed) { diff --git a/src/shell/view/CLAUDE.md b/src/shell/view/CLAUDE.md index 6e6457d..caa8743 100644 --- a/src/shell/view/CLAUDE.md +++ b/src/shell/view/CLAUDE.md @@ -54,38 +54,14 @@ 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 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. `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. + `[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. - **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 @@ -131,7 +107,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 — 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_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_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 c1bb0b9..467a80e 100644 --- a/src/shell/view/view_fx_park.cpp +++ b/src/shell/view/view_fx_park.cpp @@ -22,30 +22,15 @@ #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 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; -// 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 @@ -76,43 +61,9 @@ void setOfflineIfChanged(MediaTrack* tr, int fx, bool offline) { TrackFX_SetOffline(tr, fx, offline); } -// 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) { +void applyPark(MediaTrack* tr) { const int fxCount = TrackFX_GetCount(tr); - for (int fx = 0; fx < fxCount; ++fx) - if (!TrackFX_GetOffline(tr, fx)) return fx; - return -1; -} - -// 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) { - 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; + for (int fx = 0; fx < fxCount; ++fx) setOfflineIfChanged(tr, fx, true); } // Restores per-FX offline from the plan verbatim — never a blanket "online". @@ -173,9 +124,6 @@ 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 = parkReadyAt(time_precise(), kParkCoalesceSeconds); return g_queue.park(guid); } @@ -189,8 +137,19 @@ void discardDeferredFxParks() { g_owner = nullptr; } -void drainDeferredFxRestores() { - if (!drainGateOpen()) return; +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; + } ReaProject* const proj = g_owner; const DrainScope scope; @@ -199,8 +158,7 @@ void drainDeferredFxRestores() { // 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.takeRestores(); - if (draining.empty()) return; // parks only — not this half's work + const std::vector draining = g_queue.take(); std::unordered_map byGuid; const int count = CountTracks(proj); @@ -227,6 +185,10 @@ void drainDeferredFxRestores() { 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); @@ -243,39 +205,4 @@ void drainDeferredFxRestores() { if (!fxDropMsg.empty()) ShowConsoleMsg(("!SHOW:" + fxDropMsg).c_str()); } -void tickDeferredFxParks() { - if (!drainGateOpen()) return; - 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 - - 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 954b67f..aebbf6d 100644 --- a/src/shell/view/view_fx_park.h +++ b/src/shell/view/view_fx_park.h @@ -1,9 +1,8 @@ #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 two drains -// that apply it — restores whole, parks one FX per idle tick. See -// src/shell/view/CLAUDE.md's FX-parking caveat. +// 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. #include #include @@ -16,15 +15,6 @@ 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 @@ -34,24 +24,17 @@ 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; - // 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::string guid; + bool park = 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 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. +// 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. class FxParkQueue { public: // Returns the ops of a pending restore this park CANCELLED, empty otherwise. @@ -66,83 +49,47 @@ public: std::vector park(const std::string& guid) { FxParkIntent* held = find(guid); if (!held) { - pending_.push_back(FxParkIntent{guid, true, false, {}}); + pending_.push_back(FxParkIntent{guid, true, {}}); return {}; } if (held->park) return {}; std::vector cancelled = std::move(held->restoreOps); - if (held->partial) *held = FxParkIntent{guid, true, true, {}}; - else erase(held); + erase(held); return cancelled; } - // 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. + // A restore cancelling a pending park is COMPLETE at that point — the park + // never ran, so no drain will ever come for this GUID. void restore(const std::string& guid, std::vector ops) { FxParkIntent* held = find(guid); if (!held) { - pending_.push_back(FxParkIntent{guid, false, false, std::move(ops)}); - } else if (held->park && !held->partial) { - erase(held); + pending_.push_back(FxParkIntent{guid, false, std::move(ops)}); } else if (held->park) { - *held = FxParkIntent{guid, false, true, std::move(ops)}; + erase(held); } else { held->restoreOps = std::move(ops); } } - // Enqueue order, which is apply order WITHIN a kind — the two kinds drain - // separately and on different schedules (see the two drains below). + // Enqueue order, which is apply order: park before restore within one + // switch, as the synchronous body already orders them. const std::vector& pending() const { return pending_; } bool empty() const { return pending_.empty(); } void clear() { pending_.clear(); } - // 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() { + // 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() { std::vector taken; - 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); + taken.swap(pending_); 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_) @@ -164,16 +111,22 @@ 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 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. +// 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. inline PreParkFx preParkFxFromCancelledRestore(const std::vector& cancelled) { PreParkFx out; if (cancelled.empty()) return out; @@ -194,36 +147,22 @@ 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); -// 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. +// 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. // -// RESTORES, applied whole, synchronously. Called on the idle tick and forced by -// a subset of the paths that serialize the view model. WHICH paths force it, and -// why forcing is required, is src/shell/view/CLAUDE.md's Documented-caveat entry -// — kept there only, not restated here, so the two cannot drift apart. -void drainDeferredFxRestores(); - -// PARKS, one FX per idle tick behind a short coalescing delay (parkReadyAt / -// parkIsReady above). Unlike a restore it is safe to persist over — it -// converges on its own; see CLAUDE.md for why, and for the CPU-reclaim-is- -// progressive consequence. -// -// 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 — only the -// per-FX split does, and only to ONE FX's unload per tick, not to zero. -// -// The live-enumeration hazard's width is delay + (FX count × tick interval), NOT -// the delay alone: firstOnlineFx re-enumerates the chain LIVE every tick, not -// just the first, so an FX added at any point before the park retires is -// offlined carrying no snapshot entry — resolveFxRestore then has no op for it -// and it stays offline. Same failure as the cancelled-restore case above, over a -// longer window. Do not lengthen the delay casually; it is only one term. -void tickDeferredFxParks(); +// 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(); // 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 9dca8bd..8dec3ac 100644 --- a/tests/test_view_fx_park.cpp +++ b/tests/test_view_fx_park.cpp @@ -4,11 +4,7 @@ // 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; -// a cancel must not strand the pre-park FX state it was the last record of; 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; and the coalescing -// delay debounces from each enqueue's own time rather than firing once per the -// first. +// and a cancel must not strand the pre-park FX state it was the last record of. #include "../src/shell/view/view_fx_park.h" @@ -37,32 +33,6 @@ static const FxParkIntent* intentFor(const FxParkQueue& q, const std::string& gu return nullptr; } -// -- the coalescing gate ------------------------------------------------------ - -static void testCoalesceDebouncesFromEachEnqueuesOwnTimeNotTheFirst() { - double readyAt = parkReadyAt(0.0, 1.0); - CHECK(!parkIsReady(0.9, readyAt)); - - // A second enqueue arrives before the first's delay elapsed. A true debounce - // re-arms from THIS call's time; a one-shot would leave readyAt at 1.0 and - // this enqueue would have no effect. - readyAt = parkReadyAt(0.5, 1.0); - CHECK(!parkIsReady(1.0, readyAt)); // a one-shot would already be ready here - CHECK(parkIsReady(1.5, readyAt)); -} - -static void testRapidAToBToAStillCostsOneWaitFromTheLastFlip() { - // Three enqueues inside one delay window (A→B→A) still produce exactly one - // wait, measured from the LAST enqueue — not three separate timers and not - // one anchored to the first. - double readyAt = parkReadyAt(0.0, 1.0); - readyAt = parkReadyAt(0.3, 1.0); - readyAt = parkReadyAt(0.6, 1.0); - - CHECK(!parkIsReady(1.5, readyAt)); - CHECK(parkIsReady(1.6, readyAt)); -} - // -- tests ------------------------------------------------------------------- static void testParkEnqueuesOneIntentCarryingNoOps() { @@ -263,126 +233,30 @@ static void testRestorePlanOpsRebuildTheSlotKeyedSnapshotVerbatim() { CHECK(rebuilt.states == snap.fxOffline); } -// -- the kind split ---------------------------------------------------------- +// -- re-entrancy ------------------------------------------------------------- -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. +static void testTakeDetachesEverythingAndLeavesTheQueueEmpty() { 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.takeRestores(); + const std::vector taken = q.take(); CHECK(taken.size() == 2); - 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(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(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.restore("{A}", ops("{FX}", true)); + q.park("{A}"); - const std::vector draining = q.takeRestores(); + const std::vector draining = q.take(); q.restore("{B}", ops("{FX}", false)); // arrives while {A} is being applied CHECK(draining.size() == 1); @@ -397,7 +271,7 @@ static void testAReEntrantParkCancelsOnlyWhatIsStillPending() { // silently annihilate against an intent that has already been applied. FxParkQueue q; q.restore("{A}", ops("{FX}", false)); - q.takeRestores(); + q.take(); const std::vector cancelled = q.park("{A}"); @@ -407,8 +281,6 @@ static void testAReEntrantParkCancelsOnlyWhatIsStillPending() { } int main() { - testCoalesceDebouncesFromEachEnqueuesOwnTimeNotTheFirst(); - testRapidAToBToAStillCostsOneWaitFromTheLastFlip(); testParkEnqueuesOneIntentCarryingNoOps(); testParkReportsNothingCancelledWhenNoIntentWasPending(); testParkOnItsOwnPendingParkReportsNothingCancelled(); @@ -425,13 +297,7 @@ int main() { testSlotKeyedRestoreDoesNotBecomeIdentityKeyedWithNoIdentities(); testRestorePlanOpsRebuildTheIdentityKeyedSnapshotVerbatim(); testRestorePlanOpsRebuildTheSlotKeyedSnapshotVerbatim(); - testTakeRestoresDetachesRestoresAndLeavesParksQueued(); - testTakeRestoresWithOnlyParksQueuedTakesNothing(); - testNextParkGuidWalksParksInEnqueueOrderAndSkipsRestores(); - testFinishParkNeverRetiresARestoreStandingAtThatGuid(); - testAnUnstartedParkCancelledByItsRestoreCostsZeroWork(); - testAParkThatAlreadyWroteIsSupersededByItsRestoreNotCancelled(); - testAParkOnASupersededRestoreResumesRatherThanAnnihilates(); + testTakeDetachesEverythingAndLeavesTheQueueEmpty(); testIntentsArrivingDuringADrainSurviveIt(); testAReEntrantParkCancelsOnlyWhatIsStillPending();