From 6e2128e9370fd1aaf66aa027fbb608a6e090aac3 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Mon, 3 Aug 2026 15:00:40 -0400 Subject: [PATCH] =?UTF-8?q?view:=20split=20the=20deferred=20FX-park=20drai?= =?UTF-8?q?n=20by=20kind=20=E2=80=94=20restores=20stay=20forced,=20parks?= =?UTF-8?q?=20go=20one=20FX=20per=20idle=20tick=20behind=20a=201s=20coales?= =?UTF-8?q?cing=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();