From 2a9ab659440747d297c0af9eca44e755c33c0471 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Sat, 1 Aug 2026 23:01:50 -0400 Subject: [PATCH] =?UTF-8?q?=CE=A8-W3=20remediation:=20fix=20the=20verify?= =?UTF-8?q?=20doc's=20wrong=20render=20source,=20add=20missing=20content?= =?UTF-8?q?=20checks,=20soften=20unverified=20claims?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Corrected the Render-dialog source name the refusal's evidence depends on, added a by-ear content check and a file-size channel proxy, and stopped two comments from overclaiming. --- docs/TODO.md | 32 ++++++++++++--- docs/verify-track-scope-multitrack.md | 39 +++++++++++++------ src/core/capture/render_settings.cpp | 5 +++ src/core/capture/render_settings.h | 5 ++- src/core/capture/wav_codec.h | 6 ++- src/shell/capture/capture.cpp | 34 +++++++++------- src/shell/capture/capture.h | 21 +++++----- .../capture/capture_realtime_finalize.cpp | 3 +- src/shell/capture/capture_realtime_shell.cpp | 3 +- tests/test_render_settings.cpp | 17 ++++++++ tests/test_wav_codec.cpp | 6 ++- 11 files changed, 125 insertions(+), 46 deletions(-) diff --git a/docs/TODO.md b/docs/TODO.md index ade44c9..fe32b04 100644 --- a/docs/TODO.md +++ b/docs/TODO.md @@ -527,11 +527,16 @@ message where a correct summed file used to land. **Intended fix.** Run the observation in `docs/verify-track-scope-multitrack.md` §3 (a hand-driven Render dialog, source "selected tracks via master", one literal filename, two -tracks selected — then count the files REAPER writes). If it comes back "one summed -file", the refusal is over-strict for the TRACK scope and should be narrowed back; the -ITEM-scope half stays regardless, since a per-item-track render is not the sum the user -asked for either way. If it comes back "one file per track", nothing to do and the -inference is retired into fact. +tracks selected — then count the files REAPER writes). If it comes back "one file per +track", nothing to do and the inference is retired into fact. If it comes back "one +summed file", the refusal is over-strict for the TRACK scope and should be narrowed back +— and the ITEM-scope half is then an OPEN question, not settled: a full-extent item +capture already sums a multi-track item selection via `&32|single-file` +(`tests/test_render_settings.cpp:262`), so if `&128` also sums, a ranged item capture +routed through it sums too, and keeping the item refusal in that branch would make item +scope inconsistent with itself across the range boundary (full-extent sums, ranged +refuses, same scope). Whether that inconsistency is acceptable or the item refusal should +narrow too needs its own look at that point — not decided here. **The constraint the fix MUST handle.** Narrowing the refusal must keep the ITEM scope refusing, must keep `renderOffline` the single seam (so a recipe replay cannot diverge @@ -645,3 +650,20 @@ stereo file today. **Done looks like.** A dead-center instrument bake lands as a 1-channel file with `Sample::channelCount` matching, the same way an offline dead-center capture does; a true-stereo bake is byte-identical to today's output. + +## A 0-byte render can pass every gate and land as `Ok` (pre-existing, not a Ψ-W3 regression) + +**Context (surfaced by Ψ-W3 review).** `OfflineRenderBackend::capture`'s exists-check +(`capture.cpp:489`) passes for a 0-byte file, and the bounds gate (`:507-546`) only fires +when `expectedFrames > 0` — an invalid/empty layout reads `expectedFrames == 0` and skips +the gate rather than refusing. A 0-byte render can therefore reach `stampCaptureSample` +and land as `CaptureStatus::Ok` with an empty `contentHash` and `channelCount == 0`. + +**Not introduced by Ψ-W3.** The exists-check and the `expectedFrames > 0` guard both +predate this track; Ψ-W3 only added the mono-collapse failure report that sits downstream +of this hole and was careful not to assert bytes it never verified (see +`reportCollapseFailure` in `capture.cpp`). + +**Intended fix.** After the exists-check, also reject a 0-byte file explicitly (its own +status, not folded into `BoundsMismatch`, since a 0-byte file was never bounds-checked at +all) before anything downstream reads it. diff --git a/docs/verify-track-scope-multitrack.md b/docs/verify-track-scope-multitrack.md index c29890c..3ef819f 100644 --- a/docs/verify-track-scope-multitrack.md +++ b/docs/verify-track-scope-multitrack.md @@ -30,9 +30,15 @@ Read off: to ±1 — what you are confirming here is that it landed at all. - The REAPER console shows **no** `ReaSampler capture failed:` line. - Track `A` is still the only selected track afterwards. - -Repeat with a **razor area** on `A` over the same 10–12 s span and no time selection: same -three readings. +- **Content, not just length.** Listen to the landed file. `A` and `B` carry *audibly + different* content by the project setup above (tone vs. drum loop), so this is a by-ear + check, not a null test: the capture must be the tone alone, with **no** drum-loop bleed. + Expected: pure tone, matching `A` soloed. Failing: any trace of `B`'s drum loop audible + in the file. This is not a tautology check — the SDK header's own `RENDER_SETTINGS` line + admits a second reading, `(&(1|2)==0)=master mix`, under which a single-track track + capture could render the **whole master mix** (both `A` and `B`) rather than `A` alone; + drum-loop bleed here is exactly what that misreading would produce, and this is the + cheapest place in the whole doc to catch it. **This is the byte-identical floor.** If either cell now refuses, the change is wrong — the refusal must fire only above one track. @@ -69,7 +75,10 @@ are believed to produce N files. That has never been observed. Drive REAPER's own Render dialog by hand, with the extension out of the loop: 1. Select `A` and `B`. -2. File → Render. **Source:** *Selected tracks (stems)* — the dialog wording for `&128`. +2. File → Render. **Source:** *Selected tracks via master* — the dialog wording for `&128` + (SDK header ~3041). Do **not** pick *Stems (selected tracks)* — that is `&2`, a + different source bit that unambiguously writes one file per track and would confirm + nothing about `&128`. **Bounds:** *Custom time range*, 10.000 to 12.000 s. 3. **File name:** a literal stem with **no wildcards at all** — e.g. `stemprobe`. Clear `$track` / `$item` / anything else from the pattern; the extension writes exactly one @@ -122,16 +131,24 @@ the divergence from §2 is deliberate and this cell is what confirms it is real. ## 7. Mono collapse — what is and is not reachable Capture a range on a track whose content is dead-center (a mono source panned center, or -a duplicated-channel file). Read off: +a duplicated-channel file), using time selection **10.000 s to 12.000 s** (2.000 s, 96000 +frames at 48000 Hz — the §1 convention, so the resulting file size is exact). The panel has +no channel-count readout anywhere (`Sample::channelCount` is not drawn by +`src/shell/panel/panel_render.cpp`), so read the proxy instead: -- The panel card reports **1 channel**. -- The console shows **no** `the lossless mono collapse ... landed intact, as captured.` - line. +- Check the landed `.wav`'s size on disk (Explorer → Properties, or a directory listing). A + successful collapse is the extension's own rebuild — canonical 44-byte header + 96000 × + 4 bytes = **384,044 bytes**. A file near double that (~768,044 bytes, plus whatever + REAPER's own render adds for `bext`/metadata chunks) means the collapse did not fire — + recheck the source is genuinely dead-center before treating this as a defect. +- The console shows **no** `the lossless mono collapse ... already reached the bank; only + the size win from the collapse was lost.` line. **Not DAW-reachable:** the collapse's *failure* branch. It fires only if the captured file cannot be read, or its temporary rewrite cannot be written or renamed, inside the same call that just rendered the file — there is no manual way to inject that fault between the render and the rename. The branch is covered only at its reporting seam -(`tests/test_wav_codec.cpp`, `testCollapseOutcomesReportDistinctly`), and its console line -has never been seen in a running REAPER. If you ever do see it, the capture in question is -intact and correctly measured — only the size win was lost. +(`tests/test_wav_codec.cpp`, `testCollapseOutcomeSuffixesAreDistinctStrings`), and its console line +has never been seen in a running REAPER. If you ever do see it, the render already reached +the bank — the report only tells you the collapse's size win was lost, not that the bytes +were verified (see `docs/TODO.md`'s 0-byte-render entry). diff --git a/src/core/capture/render_settings.cpp b/src/core/capture/render_settings.cpp index bdd5e90..4939940 100644 --- a/src/core/capture/render_settings.cpp +++ b/src/core/capture/render_settings.cpp @@ -117,6 +117,11 @@ bool isMultiTrackStemRender(SourceMode mode, int sourceTrackCount) { } std::string multiTrackRefusalMessage(CaptureScope scope) { + // Deliberately does not name realtime capture as a way out, though it is the one + // action that sums correctly here: realtime is non-deterministic (hardware/performed + // FX, no bit-identical-repeats guarantee), so pointing an offline refusal at it would + // trade one invariant for another rather than just naming a substitute. A stated + // choice, not an oversight. switch (scope) { case CaptureScope::Item: return "This range is narrower than the selected items, so it renders " diff --git a/src/core/capture/render_settings.h b/src/core/capture/render_settings.h index b5472b9..e3c120d 100644 --- a/src/core/capture/render_settings.h +++ b/src/core/capture/render_settings.h @@ -1,8 +1,9 @@ #pragma once // render_settings — the REAPER-free logic behind the capture action family: // sourceMode -> RENDER_SETTINGS bits, P_RAZOREDITS parsing + range union, -// razor-else-time inference, the FX-scope bypass plan, and the capture-action -// table main.cpp iterates. Bit MEANINGS below are transcribed verbatim from +// razor-else-time inference, the FX-scope bypass plan, the capture-action +// table main.cpp iterates, and the multi-track-stem refusal + its user-facing +// message text. Bit MEANINGS below are transcribed verbatim from // reaper_plugin_functions.h; the CHOICE of which bits each mode sets is tested. #include diff --git a/src/core/capture/wav_codec.h b/src/core/capture/wav_codec.h index d58d6f7..88ab15d 100644 --- a/src/core/capture/wav_codec.h +++ b/src/core/capture/wav_codec.h @@ -129,7 +129,11 @@ enum class MonoCollapseOutcome { // The capture message's collapse suffix — empty for Declined, so a capture that had // nothing to collapse reads exactly as it did before the collapse existed. Shared by -// both backends so one outcome cannot be reported two ways. +// both backends so one outcome cannot be reported two ways. NOT user-observable on its +// own: CaptureResult::message on a successful capture is never printed by any caller, so +// the Collapsed/Failed text this returns reaches no one today — the one observable +// channel for a genuine Failed outcome is the backends' own reportCollapseFailure +// console line. std::string monoCollapseSuffix(MonoCollapseOutcome outcome); // --- Content identity (dedup hashes) ----------------------------------------- diff --git a/src/shell/capture/capture.cpp b/src/shell/capture/capture.cpp index de43225..3999df2 100644 --- a/src/shell/capture/capture.cpp +++ b/src/shell/capture/capture.cpp @@ -231,23 +231,28 @@ CaptureName captureNameFor(const std::vector& sourceNames, namespace { -// One console line per genuine collapse failure. The capture itself is intact and was -// measured after this step, so the failure costs only the size win — but silence here is -// what made a failed rewrite read exactly like a legitimately stereo capture. -void reportCollapseFailure(const std::string& absolutePath, const char* what) { - ShowConsoleMsg(("ReaSampler capture: the lossless mono collapse " + std::string(what) + - " -- " + absolutePath + - " landed intact, as captured.\n").c_str()); +// One console line per genuine collapse failure — silence here is what made a failed +// rewrite read exactly like a legitimately stereo capture. Deliberately does NOT claim +// the captured bytes are intact: a 0-byte render can reach this branch too (it passes the +// exists/bounds gates upstream; see docs/TODO.md), and this path never verified the bytes +// it's reporting on. +void reportCollapseFailure(const std::string& absolutePath, const char* what, + const char* consoleLabel) { + ShowConsoleMsg((std::string(consoleLabel) + ": the lossless mono collapse " + + std::string(what) + " -- " + absolutePath + + " already reached the bank; only the size win from the collapse " + "was lost.\n").c_str()); } } // namespace -MonoCollapseOutcome collapseCapturedFileToMono(const std::string& absolutePath) { +MonoCollapseOutcome collapseCapturedFileToMono(const std::string& absolutePath, + const char* consoleLabel) { const std::vector bytes = util::readFileBytes(absolutePath); if (bytes.empty()) { // Failed, not Declined: the read that would have decided never happened, so // "the channels differ" is a claim this path cannot make. - reportCollapseFailure(absolutePath, "could not read the captured file"); + reportCollapseFailure(absolutePath, "could not read the captured file", consoleLabel); return MonoCollapseOutcome::Failed; } @@ -265,7 +270,7 @@ MonoCollapseOutcome collapseCapturedFileToMono(const std::string& absolutePath) { std::ofstream out(tempPath, std::ios::binary | std::ios::trunc); if (!out) { - reportCollapseFailure(absolutePath, "could not open its temporary file"); + reportCollapseFailure(absolutePath, "could not open its temporary file", consoleLabel); return MonoCollapseOutcome::Failed; } out.write(reinterpret_cast(collapse.bytes.data()), @@ -275,7 +280,7 @@ MonoCollapseOutcome collapseCapturedFileToMono(const std::string& absolutePath) if (!wroteOk) { std::error_code ec; std::filesystem::remove(tempPath, ec); - reportCollapseFailure(absolutePath, "could not write the rebuilt file"); + reportCollapseFailure(absolutePath, "could not write the rebuilt file", consoleLabel); return MonoCollapseOutcome::Failed; } } @@ -283,7 +288,7 @@ MonoCollapseOutcome collapseCapturedFileToMono(const std::string& absolutePath) std::filesystem::rename(tempPath, absolutePath, ec); if (ec) { std::filesystem::remove(tempPath, ec); // don't leave litter on a failed rename - reportCollapseFailure(absolutePath, "could not replace the captured file"); + reportCollapseFailure(absolutePath, "could not replace the captured file", consoleLabel); return MonoCollapseOutcome::Failed; } return MonoCollapseOutcome::Collapsed; @@ -497,8 +502,9 @@ CaptureResult OfflineRenderBackend::capture(const CaptureRequest& request) { // (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 landed file is read three times on this path — this gate, the mono collapse, - // and stampCaptureSample — plus one rewrite when the collapse fires; a + // (On TailMode::None the landed file is read three times on this path — this gate, + // the mono collapse, and stampCaptureSample — plus one rewrite when the collapse + // fires; Auto/Manual skip this gate entirely, so they read it twice. A // once-per-capture cost on an already-warm file, judged acceptable.) 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 diff --git a/src/shell/capture/capture.h b/src/shell/capture/capture.h index afe80a5..37209f7 100644 --- a/src/shell/capture/capture.h +++ b/src/shell/capture/capture.h @@ -80,14 +80,14 @@ struct CaptureRequest { // Every failure is an explicit code, never a thrown exception across the REAPER boundary. enum class CaptureStatus { Ok, - NoProject, // no active project to render / resolve a bank folder - EmptyRange, // start >= end: nothing to render - UnsupportedMode, // backend does not implement this source mode - UnsupportedFormat, // requested bit depth has no known REAPER blob (Float32 only) - RenderFailed, // the render action ran but produced no output file - TransportBusy, // realtime backend: transport already playing/recording — refused - MultiTrackSelection,// a selected-tracks render over >1 track — would render N files - BoundsMismatch, // the rendered file's frame count is not the requested window's + NoProject, // no active project to render / resolve a bank folder + EmptyRange, // start >= end: nothing to render + UnsupportedMode, // backend does not implement this source mode + UnsupportedFormat, // requested bit depth has no known REAPER blob (Float32 only) + RenderFailed, // the render action ran but produced no output file + TransportBusy, // realtime backend: transport already playing/recording — refused + MultiTrackSelection, // a selected-tracks render over >1 track — would render N files + BoundsMismatch, // the rendered file's frame count is not the requested window's }; struct CaptureResult { @@ -130,7 +130,10 @@ CaptureName captureNameFor(const std::vector& sourceNames, // A Failed outcome is ALSO logged to the console here, because a successful capture's // CaptureResult::message is not printed by any caller — the return value alone would // leave a genuine I/O failure indistinguishable from a legitimately stereo capture. -MonoCollapseOutcome collapseCapturedFileToMono(const std::string& absolutePath); +// `consoleLabel` matches each caller's own console-prefix convention (offline: +// "ReaSampler capture"; realtime: "ReaSampler realtime capture"). +MonoCollapseOutcome collapseCapturedFileToMono(const std::string& absolutePath, + const char* consoleLabel = "ReaSampler capture"); // Stamps the metadata shared by both backends onto `s`: trackGuids (echoed from the // request) + channelCount (measured from the produced file's `fmt`; 0/unknown as the diff --git a/src/shell/capture/capture_realtime_finalize.cpp b/src/shell/capture/capture_realtime_finalize.cpp index b9e2a2d..2cc68ec 100644 --- a/src/shell/capture/capture_realtime_finalize.cpp +++ b/src/shell/capture/capture_realtime_finalize.cpp @@ -186,7 +186,8 @@ CaptureResult finalizeRecording(ReaProject* proj, MediaTrack* temp, // Channel-domain rewrite, after the frame-domain trim so it acts on the final // frame set; it preserves the frame count, so the trimmed length above still holds. - const MonoCollapseOutcome collapseOutcome = collapseCapturedFileToMono(destPath); + const MonoCollapseOutcome collapseOutcome = + collapseCapturedFileToMono(destPath, "ReaSampler realtime capture"); // Pure recorded-capture -> Sample mapping (identity, bounds echo, tier). RecordedCapture cap; diff --git a/src/shell/capture/capture_realtime_shell.cpp b/src/shell/capture/capture_realtime_shell.cpp index 2c82306..41a307b 100644 --- a/src/shell/capture/capture_realtime_shell.cpp +++ b/src/shell/capture/capture_realtime_shell.cpp @@ -32,7 +32,8 @@ // it never sums back into the master — no feedback, no monitoring double). // Multiple selected tracks sum in the one temp track — a real mix, which is why // realtime accepts a multi-track selection where the offline track scope refuses -// it (that render source cannot express a sum; see shell/capture/CLAUDE.md). +// it (the offline render source is read as emitting one file per track, DAW- +// unverified — see src/shell/capture/CLAUDE.md §Gotchas). // // Why this needs no FxBypassGuard: CreateTrackSend defaults to I_SENDMODE=0 // (post-fader), which taps the source track after its own FX/fader/pan — its diff --git a/tests/test_render_settings.cpp b/tests/test_render_settings.cpp index 6986c76..3db957f 100644 --- a/tests/test_render_settings.cpp +++ b/tests/test_render_settings.cpp @@ -297,6 +297,22 @@ static void testRefusalMessagesAreSiblingsWithDistinctExits() { CHECK(track.find("selected items") == std::string::npos); } +// Golden literals: docs/verify-track-scope-multitrack.md §2 quotes the track message as +// an exact console match. A substring check alone leaves that doc free to drift from +// whatever ships, so pin both strings byte-for-byte here. +static void testRefusalMessagesMatchGoldenLiterals() { + CHECK(multiTrackRefusalMessage(CaptureScope::Item) == + "This range is narrower than the selected items, so it renders " + "through 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."); + CHECK(multiTrackRefusalMessage(CaptureScope::Track) == + "A track capture renders the selected tracks through the master, " + "and more than one track cannot land as a single file. Capture one " + "track at a time, or route them into a folder/bus track and capture " + "that (a folder's own output is its children summed)."); +} + // --- inferRangeSource: razor-else-time (orthogonal to scope) ----------------- static void testRangeInference() { @@ -396,6 +412,7 @@ int main() { testRangedItemScopeRendersTimeBounded(); testMultiTrackStemRenderIsNamedForRefusal(); testRefusalMessagesAreSiblingsWithDistinctExits(); + testRefusalMessagesMatchGoldenLiterals(); testRangeInference(); testItemScopeBypassesEverythingButTake(); testTrackScopeKeepsSelfBypassesAncestorsAndMaster(); diff --git a/tests/test_wav_codec.cpp b/tests/test_wav_codec.cpp index 925f6d2..ec39a03 100644 --- a/tests/test_wav_codec.cpp +++ b/tests/test_wav_codec.cpp @@ -779,7 +779,9 @@ static void testCollapsePreservesQuietNaNBitPattern() { // identically to "the channels differ" — a stereo file landing under a plain Ok. The // report is where they must part: Declined stays silent (a capture with nothing to // collapse reads as it always did), and the other two say different things. -static void testCollapseOutcomesReportDistinctly() { +// Pins the three suffix STRINGS, not user-observable behavior — see wav_codec.h's +// monoCollapseSuffix doc: this text reaches no one on a successful capture. +static void testCollapseOutcomeSuffixesAreDistinctStrings() { const std::string declined = monoCollapseSuffix(MonoCollapseOutcome::Declined); const std::string collapsed = monoCollapseSuffix(MonoCollapseOutcome::Collapsed); const std::string failed = monoCollapseSuffix(MonoCollapseOutcome::Failed); @@ -828,7 +830,7 @@ int main() { testCollapseDeclinesOnUnparseableBytes(); testCollapseChangesContentHash(); testCollapsePreservesQuietNaNBitPattern(); - testCollapseOutcomesReportDistinctly(); + testCollapseOutcomeSuffixesAreDistinctStrings(); if (g_fail == 0) std::printf("wav_codec: all tests passed\n"); else std::printf("wav_codec: %d CHECK(s) FAILED\n", g_fail);