diff --git a/src/shell/view/fx_reclaim.cpp b/src/shell/view/fx_reclaim.cpp index b575c11..6d116e8 100644 --- a/src/shell/view/fx_reclaim.cpp +++ b/src/shell/view/fx_reclaim.cpp @@ -48,10 +48,15 @@ struct DrainLatch { DrainLatch& operator=(const DrainLatch&) = delete; }; -// Resolved fresh from the GUID on every step rather than held across a write, for the -// same pump premise: a plugin load can run arbitrary REAPER code, and a handle taken -// before it is not one this code may trust after it. +// Resolved fresh from the GUID rather than held across a write, for the same pump +// premise: a plugin load can run arbitrary REAPER code, and a handle taken before it +// is not one this code may trust after it. The tick's own project pointer is +// validated once at entry, before any write has run; every call here happens AFTER +// at least one prior pumping write might have, so the project itself — not just the +// track — needs its own re-check, or CountTracks/GetTrack below run on a project that +// may have closed mid-drain. MediaTrack* resolveFresh(ReaProject* proj, const std::string& guid) { + if (!ValidatePtr2(nullptr, proj, "ReaProject*")) return nullptr; TrackHandles handles; readFolderEntries(proj, handles); for (const auto& kv : handles) @@ -72,6 +77,24 @@ struct ReclaimHost { ReaProject* project = nullptr; ReaSamplerSession* session = nullptr; + // One item's liveChain call plus every writeOffline call it drives all name the + // SAME track, back to back, before drainTick can move to the next item — so the + // full O(T) resolveFresh scan is owed once per item, not once per call. Every + // call after the first re-validates the SAME pointer instead: cheap, and still + // catches a track (or its whole project) that a prior write's pump took down, + // which the scan-per-call version caught too, just at O(T) instead of O(1). + std::string cachedGuid_; + MediaTrack* cachedTrack_ = nullptr; + + MediaTrack* resolveForStep(const std::string& guid) { + if (guid == cachedGuid_ && cachedTrack_ && + ValidatePtr2(nullptr, cachedTrack_, "MediaTrack*")) + return cachedTrack_; + cachedTrack_ = resolveFresh(project, guid); + cachedGuid_ = guid; + return cachedTrack_; + } + // Monotonic and sub-millisecond: the budget below discriminates at 30ms, which // GetTickCount's ~16ms granularity cannot do. No wraparound to guard against, so // util::elapsedAtLeast (whose whole reason for existing is that wrap) does not apply. @@ -82,7 +105,7 @@ struct ReclaimHost { } bool liveChain(const std::string& guid, std::vector& out) { - MediaTrack* tr = resolveFresh(project, guid); + MediaTrack* tr = resolveForStep(guid); if (!tr) return false; const int count = TrackFX_GetCount(tr); out.clear(); @@ -94,7 +117,7 @@ struct ReclaimHost { } bool writeOffline(const std::string& guid, int fxIndex, bool offline) { - MediaTrack* tr = resolveFresh(project, guid); + MediaTrack* tr = resolveForStep(guid); if (!tr) return false; if (fxIndex < 0 || fxIndex >= TrackFX_GetCount(tr)) return false; // chain shrank if (TrackFX_GetOffline(tr, fxIndex) == offline) return false; // already there @@ -177,10 +200,14 @@ void fxReclaimTick(ReaSamplerSession& session, ReaProject* proj) { if (modeApplyInProgress()) return; DrainLatch latch; - // Both arms below are "the project this queue belongs to is no longer the one in - // front of us": no active project at all, or a tab switch / close. Drop the queue - // rather than write another project's chains; returning to it re-derives, because - // poll() reads that as a project load and the load glue reapplies, which invalidates. + // 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); diff --git a/src/shell/view/fx_reclaim.h b/src/shell/view/fx_reclaim.h index 3e571f9..f898e5d 100644 --- a/src/shell/view/fx_reclaim.h +++ b/src/shell/view/fx_reclaim.h @@ -31,13 +31,18 @@ inline constexpr double kParkHoldMs = 3000.0; // chain and cannot be split, so a budget decides only how many TRACKS a tick moves -- // and a single track with a long chain overruns it by as much as that chain costs. // -// ONLINE 120ms: onlining restores state the user owns and is waiting on, so a tick -// buys at least one typical load and two cheap ones. OFFLINE 30ms: under one typical -// unload, so opportunistic reclaim moves exactly ONE track per tick and yields -- -// background work must not make the UI feel worse than the switch already did. The -// budget is checked only AFTER a step, never before the first and never mid-step, so -// a track dearer than the whole budget still runs; that is what makes forward -// progress unconditional. +// ONLINE 120ms: onlining restores state the user owns and is waiting on. The check +// runs only AFTER a step, so against the cited 105ms typical load it admits TWO +// typical tracks per tick (105 < 120, so a second runs; 210 >= 120 stops a third), +// not one -- worst case a tick runs over budget by up to one track's whole cost. +// OFFLINE 30ms: background work must not make the UI feel worse than the switch +// already did. At the cited 36-42ms costs one track alone already clears the +// budget, so opportunistic reclaim moves ONE track and yields -- but that count is a +// property of those costs, not the budget: a cheap chain (a stock JS at ~1ms) stays +// under 30ms for many tracks in a row and all of them move in one tick. Checked only +// AFTER a step either way, never before the first and never mid-step, so a track +// dearer than the whole budget still runs; that is what makes forward progress +// unconditional. inline constexpr double kOnlineBudgetMs = 120.0; inline constexpr double kOfflineBudgetMs = 30.0;