From 0ab467388701eb5835a2f8fbbb40bc997ca70bef Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Sun, 2 Aug 2026 14:45:42 -0400 Subject: [PATCH] Fix eight review findings on the render-bounds diagnostics Corrects a false comment example, fixes two tests that couldn't detect their own regressions, adds two more read-back checkpoints around Main_OnCommand so a drift report self-locates, guards a spurious zero-vs-zero coincidence match, and softens two sentences that overclaimed cause or defect. --- src/core/capture/render_window.cpp | 6 ++++- src/core/capture/render_window.h | 6 +++-- src/shell/capture/capture.cpp | 28 ++++++++++++++++---- src/shell/capture/capture_realtime_shell.cpp | 5 ++++ src/shell/capture/render_bounds_gate.cpp | 15 +++++++---- tests/test_render_window.cpp | 27 ++++++++++++++----- 6 files changed, 68 insertions(+), 19 deletions(-) diff --git a/src/core/capture/render_window.cpp b/src/core/capture/render_window.cpp index 14dad44..40e2840 100644 --- a/src/core/capture/render_window.cpp +++ b/src/core/capture/render_window.cpp @@ -70,7 +70,11 @@ std::string describeBoundsDrift(double reqStart, double reqEnd, // straight back, so anything but the same bits is a value REAPER changed. if (storedStart == reqStart && storedEnd == reqEnd) return {}; - std::string s = "REAPER did not keep the render bounds it was handed -- asked for [" + + // Says only that the two differ, not why -- a legitimate clamp (negative start, + // end past project end) reads back differently for the same reason a precision + // defect would, and this sentence cannot tell those apart. + std::string s = "REAPER read back different render bounds than it was handed -- " + "asked for [" + exactly(reqStart) + "s, " + exactly(reqEnd) + "s), read back [" + exactly(storedStart) + "s, " + exactly(storedEnd) + "s)."; if (sampleRate > 0) { diff --git a/src/core/capture/render_window.h b/src/core/capture/render_window.h index 4c1113f..3f96e6f 100644 --- a/src/core/capture/render_window.h +++ b/src/core/capture/render_window.h @@ -2,7 +2,9 @@ // render_window — pure frame arithmetic for a capture's requested window: the // frame count a project-time range occupies, whether a render whose bounds come // from the selected items' own extent already prints that window, and the two -// diagnostics that say where a short render lost its frames. +// diagnostics that bound a short render without locating it: whether the stored +// RENDER_* bounds round-tripped, and whether the shortfall matches a millisecond- +// floor coincidence. // NO REAPER types; unit-tested by tests/test_render_window.cpp. #include @@ -54,7 +56,7 @@ bool itemExtentPrintsWindow(double reqStart, double reqEnd, // A COINCIDENCE OF COUNTS, not a claim about how anything resolved the end: nothing // renders from this number and no capture path asks for it. Whole-millisecond values // are recognized within a nanosecond, because a decimal millisecond is not always one -// in binary (0.029 * 1000 lands just below 29) and a bare floor would drop a +// in binary (1.007 * 1000 lands just below 1007) and a bare floor would drop a // millisecond from a window already on the grid. A nanosecond is far under one frame // at any rate we render, so a real sub-millisecond remainder still floors. long long msFlooredEndFrameCount(double startSeconds, double endSeconds, diff --git a/src/shell/capture/capture.cpp b/src/shell/capture/capture.cpp index b96a3c0..9c71758 100644 --- a/src/shell/capture/capture.cpp +++ b/src/shell/capture/capture.cpp @@ -463,14 +463,18 @@ CaptureResult OfflineRenderBackend::capture(const CaptureRequest& request) { // Silent unless a bound came back changed. Fires on EVERY tail mode on purpose: // only None is judged against its window after the render, so this is the sole - // signal an Auto/Manual capture was shortened before it ever started. - { + // signal an Auto/Manual capture was shortened before it ever started. Named by + // checkpoint so a DAW observation is self-locating: three reads bracket the two + // places REAPER could quantize — the store, and the render itself. + auto reportDrift = [&](const char* checkpoint, double atStart, double atEnd) { const std::string drift = describeBoundsDrift(request.startSeconds, request.endSeconds, - storedStart, storedEnd, effectiveSampleRate); + atStart, atEnd, effectiveSampleRate); if (!drift.empty()) - ShowConsoleMsg(("ReaSampler capture: " + drift + "\n").c_str()); - } + ShowConsoleMsg(("ReaSampler capture (" + std::string(checkpoint) + "): " + + drift + "\n").c_str()); + }; + reportDrift("at store", storedStart, storedEnd); GetSetProjectInfo(proj, "RENDER_CHANNELS", static_cast(request.channelCount), true); @@ -504,8 +508,22 @@ CaptureResult OfflineRenderBackend::capture(const CaptureRequest& request) { } setProjString(proj, "RENDER_FORMAT", fmtBase64); + // Read again right here: a mismatch against the store-time read-back above + // means something between the two writes and this line moved the bounds, + // before the render ever ran. + reportDrift("before render", + GetSetProjectInfo(proj, "RENDER_STARTPOS", 0.0, false), + GetSetProjectInfo(proj, "RENDER_ENDPOS", 0.0, false)); + Main_OnCommand(kActionRenderUsingMostRecentSettings, 0); + // And once more here, while the guard above is still live and before it restores + // anything: only the gap between this read and the one immediately above can be + // the render itself. + reportDrift("after render", + GetSetProjectInfo(proj, "RENDER_STARTPOS", 0.0, false), + GetSetProjectInfo(proj, "RENDER_ENDPOS", 0.0, false)); + // Main_OnCommand returns void, so a failed render is silent — stat the // expected output path to detect it. const std::string expectedPath = paths.absoluteDir + "/" + paths.fileName; diff --git a/src/shell/capture/capture_realtime_shell.cpp b/src/shell/capture/capture_realtime_shell.cpp index 41a307b..e596091 100644 --- a/src/shell/capture/capture_realtime_shell.cpp +++ b/src/shell/capture/capture_realtime_shell.cpp @@ -369,6 +369,11 @@ RealtimeRecordBackend::begin(const CaptureRequest& request, // recordWindowEnd extends past the range end for a tail mode so the // transport captures the decay; cursor + time selection are restored by restore(). + // `[verify — DAW]` whether rs/re come back changed on this isSet=true call: the SDK + // header names both `double*` but documents no read-back semantics for either + // direction, and nothing here reads rs/re again after the call to notice. Lower + // stakes than the offline RENDER_* store: completion is driven by the play cursor + // reaching the range end (tick(), below), not by re-reading this pair. double rs = request.startSeconds, re = st->recordWindowEnd_; GetSet_LoopTimeRange(true, false, &rs, &re, false); SetEditCurPos(request.startSeconds, false, false); diff --git a/src/shell/capture/render_bounds_gate.cpp b/src/shell/capture/render_bounds_gate.cpp index 7528ec2..c0b7311 100644 --- a/src/shell/capture/render_bounds_gate.cpp +++ b/src/shell/capture/render_bounds_gate.cpp @@ -100,12 +100,17 @@ BoundsVerdict checkRenderedBounds(const std::string& renderedPath, if (renderHonoredBounds(expectedFrames, actualFrames)) return v; // Says whether this shortfall has the one shape two live short renders already - // matched to the frame, so every refusal from here on adds to (or breaks) that - // evidence instead of needing the arithmetic done by hand. A count coincidence - // only — it does not establish how the render resolved anything. + // matched to the frame: the END alone floored to the millisecond. Checked against + // the END only -- a refusal whose START is also off-grid and independently floored + // would not match this shape, and this note's silence on that refusal is this + // check not covering it, not the coincidence breaking. Excludes 0, which every + // sub-millisecond window (a legitimate day-one capture) also floors to, and which + // would otherwise match a render that produced nothing. A count coincidence only — + // it does not establish how the render resolved anything. + const long long msFlooredEnd = + msFlooredEndFrameCount(request.startSeconds, request.endSeconds, rate); const std::string msNote = - actualFrames == msFlooredEndFrameCount(request.startSeconds, - request.endSeconds, rate) + (msFlooredEnd > 0 && actualFrames == msFlooredEnd) ? " Those are exactly the frames this window holds with its end floored to" " the millisecond -- a match on the count, not a measured cause." : std::string(); diff --git a/tests/test_render_window.cpp b/tests/test_render_window.cpp index 563c73e..9e0f0b6 100644 --- a/tests/test_render_window.cpp +++ b/tests/test_render_window.cpp @@ -278,11 +278,16 @@ static void testWindowAlreadyOnTheMillisecondGridLosesNothing() { static void testOneFrameOfRemainderStillFloors() { // The whole-millisecond tolerance must sit far below a frame, or it would swallow - // the very remainder this diagnostic exists to find. One frame at 48 kHz is 20.8 us - // — four orders of magnitude above the nanosecond tolerance. + // the very remainder this diagnostic exists to find. A remainder JUST BELOW a + // millisecond boundary is the discriminating case: one frame short of 1.0 s is + // 999.979166 ms, only ~0.0208 ms off the next whole millisecond. The shipped + // nanosecond tolerance still floors it down; a tolerance any wider than ~0.021 ms + // would snap it up to the millisecond instead and this test would then see 48000, + // not 47952 — which is what would fail if the tolerance regressed to something + // that wide. const double oneFrame = 1.0 / 48000.0; - CHECK(frameCountFor(0.0, 1.0 + oneFrame, 48000) == 48001); - CHECK(msFlooredEndFrameCount(0.0, 1.0 + oneFrame, 48000) == 48000); + CHECK(frameCountFor(0.0, 1.0 - oneFrame, 48000) == 47999); + CHECK(msFlooredEndFrameCount(0.0, 1.0 - oneFrame, 48000) == 47952); } static void testMillisecondFloorAt44100WhereAMillisecondIsNotWholeFrames() { @@ -333,12 +338,22 @@ static void testADriftedEndNamesBothWindowsAndBothCounts() { static void testTheReportPrintsEnoughDigitsToShowTheDrift() { // A report whose two numbers print identically is evidence of nothing. Two ends a // single ULP apart — far under the sixth decimal a shorter rendering would stop at - // — must still read as two different numbers. + // — must still read as two different numbers. Pinned as the actual %.17g literals + // (not the needle the two ends share, "s)", which occurs at every precision and so + // proves nothing): a report that regressed to a shorter format like %.6g would + // print the same six significant digits for both ends, and these two `contains` + // checks would then fail. const double asked = 4.067797; const double stored = std::nextafter(asked, 5.0); + char askedBuf[32], storedBuf[32]; + std::snprintf(askedBuf, sizeof(askedBuf), "%.17g", asked); + std::snprintf(storedBuf, sizeof(storedBuf), "%.17g", stored); + CHECK(std::string(askedBuf) != std::string(storedBuf)); + const std::string s = describeBoundsDrift(0.0, asked, 0.0, stored, 48000); CHECK(!s.empty()); - CHECK(!contains(s, "4.067797s, read back [0s, 4.067797s)")); + CHECK(contains(s, askedBuf)); + CHECK(contains(s, storedBuf)); } static void testADriftedStartIsCaughtToo() {