diff --git a/src/app/main.cpp b/src/app/main.cpp index f7cb606..4d9457d 100644 --- a/src/app/main.cpp +++ b/src/app/main.cpp @@ -222,11 +222,11 @@ static void OnTimer() // Converge the FX chains toward what the view state says they should be — the // 1897ms of TrackFX_SetOffline a mode switch no longer pays synchronously. Same - // idle discipline as the realtime fast path above: a SINGLE BOOL TEST when there is + // idle discipline as the realtime fast path above: ONE bool read when there is // nothing to converge. Ordered AFTER the load block deliberately — a reload replaces // the view model wholesale, and the applyMode above is what re-derives the queue // from it, so draining first would spend a tick against the outgoing model. - if (reasampler::g_fxReclaimPending) + if (reasampler::fxReclaimPending()) reasampler::fxReclaimTick(g_session, nullptr); reasampler::bankPanelRefresh(); // cheap fingerprint compare; no-op when unchanged/closed diff --git a/src/shell/view/fx_reclaim.cpp b/src/shell/view/fx_reclaim.cpp index 6d116e8..091cffd 100644 --- a/src/shell/view/fx_reclaim.cpp +++ b/src/shell/view/fx_reclaim.cpp @@ -30,12 +30,9 @@ namespace reasampler { namespace { -// The derived queue and the project it was derived against. The pointer is -// COMPARE-ONLY and never dereferenced beyond the ValidatePtr2 the tick already makes — -// a tab switch has to be detectable without following a pointer that may name a closed -// project. +// The derived queue, the project it was derived against, and the timer's wakeup — see +// FxReclaimQueue, which owns all three. FxReclaimQueue g_queue; -ReaProject* g_project = nullptr; // TrackFX_SetOffline is ASSUMED to pump the message loop, so a plugin load can re-enter // the timer and land back here mid-drain. Fail closed: a re-entered drain would derive a @@ -86,9 +83,11 @@ struct ReclaimHost { std::string cachedGuid_; MediaTrack* cachedTrack_ = nullptr; + // A track is validated IN a project (reaper_plugin_functions.h:7898); the null form + // is defined only where the pointer is itself a project. MediaTrack* resolveForStep(const std::string& guid) { if (guid == cachedGuid_ && cachedTrack_ && - ValidatePtr2(nullptr, cachedTrack_, "MediaTrack*")) + ValidatePtr2(project, cachedTrack_, "MediaTrack*")) return cachedTrack_; cachedTrack_ = resolveFresh(project, guid); cachedGuid_ = guid; @@ -171,24 +170,15 @@ void reportDrops(ReaProject* proj, const std::vector& drops) { if (!msg.empty()) ShowConsoleMsg(("!SHOW:" + msg).c_str()); } -// Every path that stops scheduling ticks leaves the same state behind: no queue, no pin, -// nothing pending. The debounce clock is NOT part of it — see fx_reclaim.h. -void goIdle() { - g_queue.discard(); - g_project = nullptr; - g_fxReclaimPending = false; -} - } // namespace -bool g_fxReclaimPending = false; +bool fxReclaimPending() { return g_queue.pending(); } void invalidateFxReclaim() { // A generation bump and a flag, and deliberately nothing more. Discarding the queue // here is what let a re-entered apply free the element the outer drain was writing // through, and clear the very wakeup this call had just raised. g_queue.invalidate(); - g_fxReclaimPending = true; } bool fxReclaimDrainInProgress() { return g_draining; } @@ -200,27 +190,17 @@ void fxReclaimTick(ReaSamplerSession& session, ReaProject* proj) { if (modeApplyInProgress()) return; DrainLatch latch; - // Belt-and-braces, not the tab-switch mechanism: OnTimer runs session.poll() before - // this tick, and poll() already classifies a tab switch as a Load whose reapply - // invalidates the queue — so by the time this test runs, a switch has normally - // already been handled and g_project already matches. This still catches it when - // poll() was itself skipped this tick (modeApplyInProgress/drain-in-progress - // deferral) and the two go stale together. Both arms below mean the same thing — - // no active project at all, or a project this queue no longer belongs to — so drop - // the queue rather than write another project's chains; returning to it re-derives. - // The clock goes too — a hold measured in a project we stopped watching is not - // evidence about the one we come back to. - ReaProject* const project = proj ? proj : EnumProjects(-1, nullptr, 0); - if (!ValidatePtr2(nullptr, project, "ReaProject*") || (g_project && g_project != project)) { - goIdle(); - g_queue.forgetHolds(); - return; - } + // beginTick owns what a pin/project mismatch means. What is local to here: the model + // the re-derivation below reads is the ACTIVE project's, because reaching this line + // means OnTimer ran session.poll() this tick — the two guards above are the same pair + // that gate that poll. + ReaProject* project = proj ? proj : EnumProjects(-1, nullptr, 0); + if (!ValidatePtr2(nullptr, project, "ReaProject*")) project = nullptr; + if (!g_queue.beginTick(project)) return; // DERIVED, never persisted: recomputed from view state every time the queue was // invalidated, so a REAPER quit mid-drain simply re-derives and finishes on reopen. if (g_queue.stale()) { - g_project = project; const ViewModeModel& model = session.view(); g_queue.adopt(makeQueue( view::desiredFxState(model, model.activeModeId(), liveTrackGuids(project)))); @@ -229,7 +209,7 @@ void fxReclaimTick(ReaSamplerSession& session, ReaProject* proj) { ReclaimHost host{project, &session}; const ReclaimRunResult run = g_queue.run(host); reportDrops(project, run.drops); - if (!run.aborted && run.settled) goIdle(); + if (!run.aborted && run.settled) g_queue.goIdle(); } } // namespace reasampler diff --git a/src/shell/view/fx_reclaim.h b/src/shell/view/fx_reclaim.h index f898e5d..55bd25c 100644 --- a/src/shell/view/fx_reclaim.h +++ b/src/shell/view/fx_reclaim.h @@ -137,8 +137,14 @@ struct TrackFxDrops { // True when a drop set is worth printing: any change of owner or of the set itself. // The one-per-changed-set memo this gates is view_park's `shouldReport`/ // `reportExposedTracks`, which states the reason; the shell holds the memo. +// +// A tick that lost nothing is never worth printing AND must leave the memo alone — it +// is not evidence about the last set that DID lose something, and letting it overwrite +// the memo re-arms the identical report for the next re-derivation to print again. +// Answering false is what holds the memo back, so this is the one gate for both. inline bool shouldReportDrops(bool sameOwnerAsLast, const std::vector& last, const std::vector& now) { + if (now.empty()) return false; return !(sameOwnerAsLast && last == now); } @@ -256,18 +262,64 @@ struct ReclaimRunResult { std::vector drops; }; -// Owns the derived queue and the debounce clock across a tick that can be re-entered: -// TrackFX_SetOffline is ASSUMED to pump the message loop, so an invalidation can arrive -// with a drain half-way through a track. +// Owns the derived queue, the debounce clock, the project both were derived against, and +// the timer's wakeup — together, because it is their interaction that decides whether a +// tick may happen at all. A `ReaProject*` here is COMPARE-ONLY and never dereferenced, so +// the whole state machine is exercisable without REAPER (tests/test_fx_reclaim.cpp). // -// The two halves of that safety are structural, not guarded. `invalidate()` bumps a -// counter and touches NOTHING else, so there is nothing for it to clear under a drain. -// `run()` moves the queue and the clock into locals for the drain's whole duration, so -// the object holds nothing the drain is reading — and re-checks the counter before it -// commits either the surviving queue or a single release. +// The queue must also survive being re-entered: TrackFX_SetOffline is ASSUMED to pump the +// message loop, so an invalidation can arrive with a drain half-way through a track. The +// two halves of that safety are structural, not guarded. `invalidate()` bumps a counter +// and raises the wakeup and touches NOTHING else, so there is nothing for it to clear +// under a drain. `run()` moves the queue and the clock into locals for the drain's whole +// duration, so the object holds nothing the drain is reading — and re-checks the counter +// before it commits either the surviving queue or a single release. class FxReclaimQueue { public: - void invalidate() { ++generation_; } + void invalidate() { + ++generation_; + pending_ = true; + } + + // The ONE test the timer makes per tick. False means settled: nothing to converge + // until the next invalidation. + bool pending() const { return pending_; } + + // The project the current queue and holds were derived against, or nullptr when idle. + ReaProject* project() const { return project_; } + + // The tick's entry transition, and the ONE place the pin moves. `project` is the + // validated active project, or nullptr when there is none; false means the caller + // must return without touching REAPER. + // + // A project that does not match the pin is a TAB SWITCH — the ordinary path, not a + // corner. Neither the derived work nor the holds are evidence about the project we + // arrived in, so both go, and the tick proceeds to re-derive against the new one. The + // WAKEUP STANDS: clearing it here (an earlier goIdle in this branch did) stranded the + // incoming project's whole reclaim, because the timer gates every tick on it and + // nothing re-raises it short of a mode switch or a membership edit there. Only the + // no-project arm may clear it — nothing to converge, and nothing to come back to. + bool beginTick(ReaProject* project) { + if (!project) { + goIdle(); + clock_.forget(); + return false; + } + if (project_ && project_ != project) { + discard(); + clock_.forget(); + } + project_ = project; + return true; + } + + // Settled: drop the queue and the pin and stop the timer's ticks. The debounce clock + // deliberately survives — it is keyed on the track, not on the queue. + void goIdle() { + discard(); + project_ = nullptr; + pending_ = false; + } // No queue at all, or one derived before the last invalidation. bool stale() const { return !derived_ || derivedAt_ != generation_; } @@ -283,8 +335,6 @@ public: derived_ = false; } - void forgetHolds() { clock_.forget(); } - const std::vector& items() const { return items_; } template @@ -327,19 +377,22 @@ public: private: std::vector items_; ParkClock clock_; + ReaProject* project_ = nullptr; unsigned long long generation_ = 0; unsigned long long derivedAt_ = 0; bool derived_ = false; + bool pending_ = false; }; // --- shell entry points -------------------------------------------------------- // The ONE test the timer makes per tick, mirroring the realtime capture's single // pointer test. False means the queue is settled and there is nothing to converge. -extern bool g_fxReclaimPending; +bool fxReclaimPending(); -// Marks the derived queue for re-derivation by the NEXT tick; discards nothing here, -// because this is reachable from inside a drain. Called from applyMode and nothing else +// Raises the wakeup and marks the derived queue for re-derivation by the NEXT tick; +// discards nothing here, because this is reachable from inside a drain. Called from +// applyMode and nothing else // — every membership, snapshot and active-mode mutation reapplies through it — on the // refusal path as well as the success one (see view.cpp for why the refusal is the // load-bearing half). The debounce clock deliberately survives: it is keyed on the @@ -351,11 +404,12 @@ void invalidateFxReclaim(); bool fxReclaimDrainInProgress(); // Drains one tick against `proj` (nullptr = current project), re-deriving the queue from -// the session's view model first if it was invalidated. Returns without touching the -// project while an applyMode is on the stack, while already draining, or when the active -// project is not the one the queue was derived against. Takes the SESSION rather than the -// model alone because completing an online spends the park record's FX half, and that -// spend has to reach the persisted view state — see FxReclaimQueue::run. +// the session's view model first if it was invalidated or if the active project moved +// (FxReclaimQueue::beginTick). Returns without touching the project while an applyMode is +// on the stack, while already draining, or when there is no valid active project at all. +// Takes the SESSION rather than the model alone because completing an online spends the +// park record's FX half, and that spend has to reach the persisted view state — see +// FxReclaimQueue::run. void fxReclaimTick(ReaSamplerSession& session, ReaProject* proj); } // namespace reasampler diff --git a/tests/test_fx_reclaim.cpp b/tests/test_fx_reclaim.cpp index 1dfa6cf..ecd70b2 100644 --- a/tests/test_fx_reclaim.cpp +++ b/tests/test_fx_reclaim.cpp @@ -4,10 +4,12 @@ // yield, the queue being a cache of intent that a model change discards, the user's own // offline plugins surviving an online, the release reaching the persisted model, and — // via a host that invalidates from inside a write — that a re-entered invalidation -// reaches nothing the drain is using. +// reaches nothing the drain is using. The queue owns the project pin and the timer's +// wakeup for the same reason: a compare-only pointer makes the tab switch assertable. #include "../src/shell/view/fx_reclaim.h" +#include #include #include #include @@ -176,6 +178,11 @@ FxReclaimQueue queueOf(const ViewModeModel& vm, const std::set& liv return q; } +// Two project handles. The queue treats a project pointer as COMPARE-ONLY and never +// dereferences it, so a fabricated address is a faithful stand-in for REAPER's. +ReaProject* const kProjA = reinterpret_cast(static_cast(0x1)); +ReaProject* const kProjB = reinterpret_cast(static_cast(0x2)); + } // namespace // -- 1. debounce --------------------------------------------------------------- @@ -633,6 +640,138 @@ static void testAnOfflineTickReportsNoDrops() { CHECK(r.drops.empty()); } +// -- 8. the pin and the wakeup ------------------------------------------------------ + +static void testATabSwitchRetargetsTheQueueAndLeavesTheWakeupStanding() { + // Project A, one parked leaf owed offline work, derived against A and pinned to it. + ViewModeModel a = parkedDesignLeaf({"{FX-A}"}); + FakeHost hostA; + hostA.chains["{T}"] = onlineChain({"{FX-A}"}); + + FxReclaimQueue q; + q.invalidate(); // what applyMode does on its way out of the switch that parked {T} + CHECK(q.pending()); + CHECK(q.beginTick(kProjA)); + CHECK(q.project() == kProjA); + q.adopt(derive(a, {"{T}"})); + CHECK(!q.run(hostA).settled); // observes the hold at t=0; still owed + CHECK(!q.stale()); + + // The tab switch. poll()'s Load reapply invalidates on the way in, and NOTHING there + // moves the pin, so the tick arrives holding A's work with B active. + q.invalidate(); + CHECK(q.beginTick(kProjB)); // the tick proceeds — it does not bail on the mismatch + CHECK(q.pending()); // and the timer will still call it: the strand's one bit + CHECK(q.project() == kProjB); + CHECK(q.stale()); // A's work is gone, B's is owed + CHECK(q.items().empty()); + + // B's own parked leaf converges on this very tick and every one after it. + ViewModeModel b = parkedDesignLeaves({{"{T2}", {"{FX-B}"}}}); + FakeHost hostB; + hostB.chains["{T2}"] = onlineChain({"{FX-B}"}); + q.adopt(derive(b, {"{T2}"})); + CHECK(!q.run(hostB).settled); // B's own hold starts here + hostB.clock += kParkHoldMs; + const ReclaimRunResult done = q.run(hostB); + CHECK(done.settled); + CHECK(hostB.writeLog == std::vector{"{T2}:0=off"}); + CHECK(hostB.offlineAt("{T2}", 0)); + + // ...and A's chain was never touched from B: the queue was dropped, not run. + CHECK(hostA.writeLog.empty()); + + // Only now, settled, does the shell stop the ticks. + q.goIdle(); + CHECK(!q.pending()); + CHECK(q.project() == nullptr); +} + +static void testGoingIdleKeepsTheParkHoldWhileARetargetDropsIt() { + // A hold is evidence about a track in ONE project, so a tab switch drops it. An + // ordinary settle does not: restarting every 3s hold on the next unrelated membership + // edit would defer an unchanged track's reclaim indefinitely. + ViewModeModel vm = parkedDesignLeaf({"{FX-A}"}); + FakeHost host; + host.chains["{T}"] = onlineChain({"{FX-A}"}); + + FxReclaimQueue q; + CHECK(q.beginTick(kProjA)); + q.adopt(derive(vm, {"{T}"})); + q.run(host); // hold starts at t=0 + host.clock = kParkHoldMs; + CHECK(q.run(host).settled); + CHECK(host.writeLog.size() == 1); + + // Settled: the queue and the pin go, the hold stays. A membership edit elsewhere + // re-derives the SAME offline item, which is matured on arrival — one tick, no + // second 3s wait, and nothing to write because the chain is already there. + q.goIdle(); + q.invalidate(); + CHECK(q.beginTick(kProjA)); + q.adopt(derive(vm, {"{T}"})); + const ReclaimRunResult resumed = q.run(host); + CHECK(resumed.settled); + CHECK(resumed.writes == 0); + CHECK(host.writeLog.size() == 1); + + // The mirror: away to B and back to A, and the same item must wait a fresh hold. + q.invalidate(); + CHECK(q.beginTick(kProjB)); + CHECK(q.beginTick(kProjA)); + q.adopt(derive(vm, {"{T}"})); + const double back = host.clock; + CHECK(!q.run(host).settled); // the hold restarts here, at `back` + host.clock = back + kParkHoldMs - 1.0; + CHECK(!q.run(host).settled); + host.clock = back + kParkHoldMs; + CHECK(q.run(host).settled); +} + +static void testOnlyANoProjectTickClearsTheWakeup() { + ViewModeModel vm = parkedDesignLeaf({"{FX-A}"}); + FakeHost host; + host.chains["{T}"] = onlineChain({"{FX-A}"}); + + FxReclaimQueue q; + q.invalidate(); + CHECK(q.beginTick(kProjA)); + q.adopt(derive(vm, {"{T}"})); + q.run(host); + host.clock += kParkHoldMs; + + // An abort leaves the wakeup up: the work is re-owed, and the shell's settle test + // never runs on this path. + host.reentrant = &q; + host.invalidateAfterWrite = 1; + const ReclaimRunResult r = q.run(host); + CHECK(r.aborted); + CHECK(q.pending()); + + // No active project at all — the one arm with nothing to converge and nothing to + // come back to. Everything goes, wakeup included. + CHECK(!q.beginTick(nullptr)); + CHECK(!q.pending()); + CHECK(q.project() == nullptr); + CHECK(q.stale()); + CHECK(q.items().empty()); +} + +static void testATickThatLostNothingNeitherPrintsNorDisturbsTheDropMemo() { + const std::vector none; + const std::vector lost{TrackFxDrops{"{T}", FxRestoreDrops{1, 0, 0}}}; + + CHECK(shouldReportDrops(/*sameOwnerAsLast=*/false, none, lost)); // first sighting + CHECK(!shouldReportDrops(/*sameOwnerAsLast=*/true, lost, lost)); // the re-derivation + + // The regression: every ordinary tick carries no drops at all. Treating that as a + // change would overwrite the memo and re-arm the identical report for the next + // re-derivation — the shell holds the memo back on exactly this answer. + CHECK(!shouldReportDrops(/*sameOwnerAsLast=*/true, lost, none)); + CHECK(!shouldReportDrops(/*sameOwnerAsLast=*/false, lost, none)); + CHECK(!shouldReportDrops(/*sameOwnerAsLast=*/true, lost, lost)); +} + int main() { testFastFlipMovesZeroPluginsButAHeldParkStillReclaims(); @@ -657,6 +796,11 @@ int main() { testADropIsReportedOnceAcrossRepeatedRederivations(); testAnOfflineTickReportsNoDrops(); + testATabSwitchRetargetsTheQueueAndLeavesTheWakeupStanding(); + testGoingIdleKeepsTheParkHoldWhileARetargetDropsIt(); + testOnlyANoProjectTickClearsTheWakeup(); + testATickThatLostNothingNeitherPrintsNorDisturbsTheDropMemo(); + if (g_fail == 0) std::printf("All tests passed.\n"); return g_fail ? 1 : 0; }