diff --git a/docs/TODO.md b/docs/TODO.md index 8b5348a..6b89ac9 100644 --- a/docs/TODO.md +++ b/docs/TODO.md @@ -512,3 +512,31 @@ wrong when they do not. **Done looks like.** A `SelectedItems` recapture either reproduces its recorded audio from the recipe alone, or refuses with a message naming what the recipe cannot pin down. + +## An overlapping item on the source track itself is not isolated from a ranged item capture — DECIDED, not deferred + +**Context (surfaced by Ψ-W1-T1, capture-range-exactness).** The re-source to the +selected-tracks render (`&128`) needed transient upstream silencing so an item capture +did not also print folder children and receives; `render_isolation` (`UpstreamIsolation`) +covers both. A third widening exists in the same shape: a non-selected item on the +SAME track that overlaps the requested range is now audible in the render, where the +pre-fix `&32` selected-items source excluded it by construction (that source only ever +prints the selected items). + +**This is a decision, not a gap.** `src/shell/capture/CLAUDE.md` states the reasoning in +full and it is not repeated here: `UpstreamIsolation`/`render_selection` silence and +select TRACKS because the recipe that replays a capture stores tracks and a range, never +item GUIDs — a mute plan keyed to today's overlapping item could not be recomputed at +replay time, so muting items would make the capture stop reproducing itself. The named +candidate (a) in `docs/PLAN.md` §Ψ-W1-T1 carried exactly this semantic edge; it was +weighed against candidate (b) (an item-bounds render with a derived start time) and (a) +shipped with the edge accepted rather than closed. + +**Priority / risk.** Low in the common case (one item per track over the captured range is +the normal shape); a project with deliberately overlapping items on one track is the one +that surfaces it, and the practical mitigation is unchanged from before this track: +select/move the neighbour, or capture at track scope instead. + +**Done looks like.** Nothing to do — recorded so a future reviewer does not read the +non-isolation as an oversight and re-propose closing it against the recipe's stated +tracks-and-range-only shape. diff --git a/src/shell/capture/capture.cpp b/src/shell/capture/capture.cpp index a881ea3..ca79649 100644 --- a/src/shell/capture/capture.cpp +++ b/src/shell/capture/capture.cpp @@ -21,11 +21,13 @@ #include "shell/capture/capture.h" #include +#include #include #include #include #include #include +#include #include #include "core/capture/capture_paths.h" @@ -396,13 +398,14 @@ CaptureResult OfflineRenderBackend::capture(const CaptureRequest& request) { } // Exact bounds, made structural: with no tail requested the file must contain - // exactly the requested window's frames, so a source mode that silently widened - // the render fails loudly here instead of landing as a successful capture. Auto - // and Manual add frames by design and are skipped. UNVERIFIED: that REAPER - // resolves the window's two edges to frame indices the same way frameCountFor - // does — a DAW pass decides whether this equality is exact or off by a frame. + // (within a tolerance, see below) the requested window's frames, so a source + // mode that silently widened the render fails loudly here instead of landing as + // a successful capture. Auto and Manual add frames by design and are skipped. // (The file is read again by stampCaptureSample below; the duplicate read is a - // once-per-capture cost on an already-warm file.) + // once-per-capture cost on an already-warm file.) A bounded/header-only read is + // not a clean substitute: parseWavLayout only marks the data chunk valid when + // the buffer holds the chunk's FULL declared body (bodyInBounds), so a truncated + // read would read as invalid here on every real capture, not just malformed ones. if (request.tailMode == TailMode::None) { const WavLayout layout = parseWavLayout(util::readFileBytes(expectedPath)); @@ -411,15 +414,35 @@ CaptureResult OfflineRenderBackend::capture(const CaptureRequest& request) { static_cast(layout.sampleRate)) : 0; const long long actualFrames = static_cast(layout.frameCount()); - if (expectedFrames > 0 && actualFrames != expectedFrames) { + // frameCountFor is a difference of frame indices, not a rounded duration + // (see render_window.h) — REAPER's own edge-rounding can legitimately land + // one frame off that, so the gate tolerates +/-1 rather than exact equality. + // The defect this refuses is a whole-item widening (seconds of extra audio, + // thousands of frames), which a 1-frame tolerance still catches with + // certainty. Tightening to exact equality needs a DAW pass confirming REAPER + // resolves the window's two edges to frame indices the same way this does. + const long long frameDelta = actualFrames > expectedFrames + ? actualFrames - expectedFrames + : expectedFrames - actualFrames; + if (expectedFrames > 0 && frameDelta > 1) { result.status = CaptureStatus::BoundsMismatch; result.message = "Render produced " + std::to_string(actualFrames) + " frames but the requested range is " + std::to_string(expectedFrames) + " at " + std::to_string(layout.sampleRate) + " Hz -- the render did not honor the requested bounds. " - "Nothing was added to the bank; the file is at: " + - expectedPath; + "Requested [" + std::to_string(request.startSeconds) + + "s, " + std::to_string(request.endSeconds) + + "s) -> frame indices [" + + std::to_string(std::llround(request.startSeconds * + layout.sampleRate)) + + ", " + + std::to_string(std::llround(request.endSeconds * + layout.sampleRate)) + + "). Nothing was added to the bank; the render at " + + expectedPath + " was never indexed and has been cleaned up."; + std::error_code ec; + std::filesystem::remove(expectedPath, ec); return result; } } diff --git a/src/shell/capture/capture_orchestrator.cpp b/src/shell/capture/capture_orchestrator.cpp index 09c5c1f..91cc47c 100644 --- a/src/shell/capture/capture_orchestrator.cpp +++ b/src/shell/capture/capture_orchestrator.cpp @@ -192,9 +192,9 @@ CaptureResult renderOffline(CaptureScope scope, refused.status = CaptureStatus::MultiTrackRange; refused.message = "This range is narrower than the selected items, so it renders through " - "their tracks -- and those items span more than one track, which renders " - "one file per track. Capture one track's items at a time, or make the " - "range match the items' extent."; + "their tracks -- and those items span more than one track, which this " + "shape cannot land as a single file. Capture one track's items at a " + "time, or make the range match the items' extent."; return refused; } diff --git a/src/shell/capture/render_isolation.h b/src/shell/capture/render_isolation.h index 04d817d..1edb7eb 100644 --- a/src/shell/capture/render_isolation.h +++ b/src/shell/capture/render_isolation.h @@ -2,7 +2,7 @@ // The transient silencing a ranged ITEM render needs: REAPER's selected-tracks // source prints everything upstream of the track — its folder children and its // receives — which an item capture must not hear. Stack RAII, restored on every -// path. The .cpp includes reaper_plugin_functions.h WITHOUT REAPERAPI_IMPLEMENT. +// path. #include diff --git a/src/shell/capture/scope_resolve.cpp b/src/shell/capture/scope_resolve.cpp index 628ef54..d68bac0 100644 --- a/src/shell/capture/scope_resolve.cpp +++ b/src/shell/capture/scope_resolve.cpp @@ -54,8 +54,9 @@ model::ProvenanceScope provenanceScopeFor(CaptureScope scope) // out.sourceTracks (deduped) — these are the tracks whose FX must be bypassed so an // item capture hears take/item FX only. GUIDs recorded for provenance. // extentStart/extentEnd come back as the union of the selected items' own extents: -// the window REAPER's selected-items render source would print (SDK ~1990: -// D_POSITION/D_LENGTH in seconds). +// INFERRED to be the window REAPER's selected-items render source would print +// (SDK ~1990: D_POSITION/D_LENGTH in seconds) — unverified; see +// src/core/capture/CLAUDE.md §Gotchas for what that inference rests on. bool collectSelectedItemTracks(ResolvedSource& out, double& extentStart, double& extentEnd) { diff --git a/tests/test_render_settings.cpp b/tests/test_render_settings.cpp index fdb4292..9143197 100644 --- a/tests/test_render_settings.cpp +++ b/tests/test_render_settings.cpp @@ -246,9 +246,10 @@ static void testRangedItemScopeRendersTimeBounded() { static void testMultiTrackRangedItemRenderIsNamedForRefusal() { // The one shape that cannot land: a ranged item capture (item scope re-sourced to // the selected-tracks render) whose items span more than one track. That source - // renders one file per track with no single-file bit available, so N stems would - // collapse onto one render pattern and one track's audio would land as a - // successful capture. + // is INFERRED to render one file per track with no single-file bit available + // (unverified; see src/shell/capture/CLAUDE.md §Gotchas for what that inference + // rests on), so N stems would collapse onto one render pattern and one track's + // audio would land as a successful capture. CHECK(isMultiTrackRangedItemRender(CaptureScope::Item, SourceMode::SelectedTracks, 2)); CHECK(isMultiTrackRangedItemRender(CaptureScope::Item, diff --git a/tests/test_track_topology.cpp b/tests/test_track_topology.cpp index 45bf323..3963927 100644 --- a/tests/test_track_topology.cpp +++ b/tests/test_track_topology.cpp @@ -70,6 +70,15 @@ static void testUnterminatedFolderSwallowsTheRest() { CHECK(sameIndices(directChildIndices(depths, 0), {1, 2})); } +static void testSiblingFolderAfterParentClosesIsNotIncluded() { + // 0: folder parent, 1: child, 2: last child (closes it). 3: an unrelated sibling + // folder that opens AFTER track 0's folder has already closed, 4: its child, 5: + // its last child. A walk that fails to stop at track 0's own close would keep + // consuming and wrongly pull the sibling's children in too. + const std::vector depths{1, 0, -1, 1, 0, -1}; + CHECK(sameIndices(directChildIndices(depths, 0), {1, 2})); +} + int main() { testFlatProjectHasNoChildren(); testFolderParentReturnsItsDirectChildren(); @@ -77,6 +86,7 @@ int main() { testMultiLevelCloseEndsTheOuterFolderToo(); testNonFolderAndOutOfRangeReturnEmpty(); testUnterminatedFolderSwallowsTheRest(); + testSiblingFolderAfterParentClosesIsNotIncluded(); if (g_fail == 0) std::printf("track_topology: all tests passed\n"); return g_fail == 0 ? 0 : 1;