From 16b2a1b8ca134ccc28755a83c841eab06af25d28 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Tue, 28 Jul 2026 19:27:12 -0400 Subject: [PATCH] Q-W0 code-review remediation: pitch_shift follower self-heal, T3-03 rate bail, doc/comment riders --- docs/product/code-quality-audit.md | 8 ++++++++ src/provenance.cpp | 3 +++ src/vst/pitch_shift.cpp | 28 +++++++++++++++++++++++--- src/vst/reasampler_editor.cpp | 22 +++++++++++++------- tests/test_pitch_shift.cpp | 32 ++++++++++++++++++++++++++---- 5 files changed, 79 insertions(+), 14 deletions(-) diff --git a/docs/product/code-quality-audit.md b/docs/product/code-quality-audit.md index df59c85..196df62 100644 --- a/docs/product/code-quality-audit.md +++ b/docs/product/code-quality-audit.md @@ -315,6 +315,14 @@ the clean bills) are deliberately absent. fallback): document-and-defer with the comment amended to name the 44.1 k assumption, if zero UI-feel change is preferred. (Folding either into Q-W2v instead is *not* recommended — it would put behavior changes inside a mechanical-split wave; §2.5(8).) + + **Recorded deviation (Q-W0 remediation, code review):** T3-03 as implemented resolves + `fadeMaxFrames()` against `liveSampleRate()` (the host/project rate), not the per-file rate + this section's text literally suggests ("the loaded source's rate"). Reviewer verified this + is the more correct choice: no resample path exists anywhere in `src/`, the engine advances + one source frame per host frame, and this matches the time base `paintEnvelopeOverlay` + already uses for the same fades (`totalSeconds = frames / liveSampleRate()`). No further + action — recorded here so the audit text and the shipped behavior don't read as diverged. - **(e) WAV/RIFF consolidation moment — the one true track disagreement (T2-08 vs T4-23/T4-10).** T2 prefers the `core/wav` homing moment (the relocation wave); T4 prefers "the wave that opens `ingest.cpp`" — which does not exist, and T4-10 points back circularly. *Recommend:* record diff --git a/src/provenance.cpp b/src/provenance.cpp index 5423af7..89cd816 100644 --- a/src/provenance.cpp +++ b/src/provenance.cpp @@ -140,6 +140,9 @@ public: private: bool fail() { ok_ = false; return false; } + // TODO(Q-W1): strtol does not check errno/range here, so an out-of-range field narrows + // silently to LONG_MAX (then truncates into `int`) instead of failing parse. Flagged for + // the Q-W1 wire-codec collapse rather than fixed in place. static bool toInt(const std::string& f, int& out) { const char* b = f.c_str(); char* end = nullptr; diff --git a/src/vst/pitch_shift.cpp b/src/vst/pitch_shift.cpp index 59aa73d..dd73d16 100644 --- a/src/vst/pitch_shift.cpp +++ b/src/vst/pitch_shift.cpp @@ -201,8 +201,9 @@ void PitchShifter::splice(std::int64_t nominalJump, double delay) { // search (and the +/-1-lag parabolic refinement calls at bestLag ± 1, and the interpolator's // read-ahead) can touch is delay d + jump + maxLag + 2 (maxLag from the coarse/fine search, // +1 for the parabola's outer ± 1 probe, +1 for the interpolator's i1 = i0+1 read-ahead), - // so the tight cap is filled_ - d - maxLag_ - 2. The code uses - 1 here — one sample of - // conservative margin, never out-of-range. In steady state (filled_ == ringLen_) this is + // so the tight cap is filled_ - d - maxLag_ - 2. The code uses - 1 here — one sample LOOSER + // than that derived cap (not extra margin); ring indexing wraps via modulo everywhere, so + // this never runs off the physical ring_ array. In steady state (filled_ == ringLen_) this is // > window_ and the nominal jump is untouched; near a primed onset it shrinks the jump to // what real history exists (still many source periods with a full-window prime). The floor of // 1 is only reachable on the documented degenerate reset-without-prime path — garbage-tolerant. @@ -375,7 +376,28 @@ AudioSample PitchShifter::processImpl(AudioSample in, const SpliceEvent* linked) // master channel did this frame. Lockstep state means our own trigger would have // fired on the same frame; applying the master's decision keeps the two rings // sample-aligned (one shared lag, one shared schedule). - if (linkedEv.fired) applySplice(linkedEv); + if (linkedEv.fired) { + applySplice(linkedEv); + } else { + // Self-healing fallback (review rider): the master not firing normally means this + // channel's own trigger wouldn't fire either (lockstep). But if the processor ever + // renders a mono block mid-note, this follower channel is skipped for that block + // while the master keeps advancing — its writePos_/filled_ falls behind and, with + // only the `if (linkedEv.fired)` path above, could never resync. So check this + // follower's OWN tap distance against the safe band and splice via its own search + // when it has left [dLow_, dHigh_], exactly as the master would. Reuses splice() — + // no allocation, no new RT cost. In the normal (non-mono-block) case this branch + // never triggers: the master's trigger fires first and this whole `if` is false. + double d = static_cast(writePos_) - posA_; + const double len = static_cast(ringLen_); + while (d < 0.0) d += len; + while (d >= len) d -= len; + if (d <= static_cast(dLow_)) { + splice(+window_, d); + } else if (d >= static_cast(dHigh_)) { + splice(-window_, d); + } + } } else { // 3. Splice scheduling: relocate when the active tap's delay leaves the safe band. // Up-shifts (ratio > 1) drain the delay toward 0 -> jump one window OLDER; down- diff --git a/src/vst/reasampler_editor.cpp b/src/vst/reasampler_editor.cpp index 744f4c1..13f8a6c 100644 --- a/src/vst/reasampler_editor.cpp +++ b/src/vst/reasampler_editor.cpp @@ -405,10 +405,14 @@ double clamp01(double v) { return v < 0.0 ? 0.0 : (v > 1.0 ? 1.0 : v); } double ReaSamplerEditor::controlValue(int id, const ZonePlaySeconds& play) const { // Wall-clock seconds -> normalized over the seconds ceiling; source frames -> normalized over // the rate-resolved frames ceiling (T3-03). Two domains, kept explicit so neither leaks a rate. + // A stored fade exceeding fadeMaxFrames() at the current host rate reads as norm 1.0 (clamp01 + // pins it) and gets rewritten down on the next knob touch — deliberate, matching the old + // fixed-ceiling clamp behavior in kind, just rate-dependent now instead of fixed at 88200. const double fadeMax = fadeMaxFrames(); const auto secToNorm = [](double s) { return clamp01(s / kEnvTimeMaxSeconds); }; const auto framesToNorm = [fadeMax](std::int64_t f) { - return clamp01(static_cast(f) / fadeMax); + // Ceiling unavailable (rate not yet known): inert until fadeMaxFrames() resolves. + return fadeMax > 0.0 ? clamp01(static_cast(f) / fadeMax) : 0.0; }; switch (static_cast(id)) { case ParamControl::kPlayMode: return play.playMode == PlayMode::Trigger ? 1.0 : 0.0; @@ -435,7 +439,9 @@ void ReaSamplerEditor::applyControl(int id, ZonePlaySeconds& play, double value, int segment) const { const double fadeMax = fadeMaxFrames(); // T3-03: rate-resolved knob full-scale const auto normToSec = [](double v) { return clamp01(v) * kEnvTimeMaxSeconds; }; - const auto normToFrames = [fadeMax](double v) { + const auto normToFrames = [fadeMax](double v) -> std::int64_t { + // Ceiling unavailable (rate not yet known): inert until fadeMaxFrames() resolves. + if (fadeMax <= 0.0) return 0; return static_cast(clamp01(v) * fadeMax + 0.5); }; switch (static_cast(id)) { @@ -476,12 +482,14 @@ double ReaSamplerEditor::fadeMaxFrames() const { // T3-03: the Trigger-fade knob's full-scale throw is kFadeMaxSeconds (2 s wall-clock) // resolved against the live rate — the SAME time base the envelope overlay already uses // to place these source-frame fades on screen (totalSeconds = frames / liveSampleRate()), - // and the rate captures are made at (the capture path renders at the project rate). The - // 44.1 kHz fallback covers the pre-setupProcessing window (rate still 0) and reproduces - // the legacy 88200-frame ceiling there. Storage stays SOURCE FRAMES — this resolves the - // UI ceiling only. + // and the rate captures are made at (the capture path renders at the project rate). + // Pre-setupProcessing the rate is still 0: rather than substitute a literal rate (the + // exact residue T3-03 removed), bail the same way paintEnvelopeOverlay does (~line 1396) — + // callers treat a <= 0 return as "ceiling unavailable yet" and degrade the knob to inert + // rather than guess a rate. Storage stays SOURCE FRAMES — this resolves the UI ceiling only. const double rate = liveSampleRate(); - return kFadeMaxSeconds * (rate > 0.0 ? rate : 44100.0); + if (rate <= 0.0) return 0.0; + return kFadeMaxSeconds * rate; } double ReaSamplerEditor::previewVelocity01() const { diff --git a/tests/test_pitch_shift.cpp b/tests/test_pitch_shift.cpp index 255a5aa..a92c2fe 100644 --- a/tests/test_pitch_shift.cpp +++ b/tests/test_pitch_shift.cpp @@ -501,7 +501,19 @@ static void testFreezeTailContinuousTone() { // mono-sum-combing mechanism). The divergence witness: an INDEPENDENT shifter fed the // follower's content picks a different lag on the same schedule, proving the mirror // assertion is not vacuous (the two channels' contents genuinely disagree on the best -// alignment). --- +// alignment). A third shifter (`mirror`), primed with the SAME content as the master +// and driven via processLinked() with the master's own decisions, must reproduce the +// master's output BIT-IDENTICALLY every frame — this is the review-rider strengthening: +// the `ef == em` mirror check above only proves lastSplice_ was copied verbatim (which +// applySplice() always does), not that applySplice() actually reproduces splice()'s +// effect on posA_/fadeLen_/audio output; a same-content bit-identical check catches a +// real divergence there (e.g. an asymmetry between applySplice()'s unconditional +// `max(1, ev.fadeLen)` and splice()'s own fadeLen_ assignment). This driven-every-frame +// setup keeps both master and follower in lockstep the whole run (posA_/writePos_ stay +// identical since jumps are geometric, not content-dependent), so it exercises +// applySplice() on every splice — never the Q-W0 remediation self-healing fallback +// (own-search splice on a stale follower), which only fires when a follower has been +// skipped a block relative to the master (mono-render-block starvation). --- static void testStereoLinkedLagSharedSchedule() { const std::int64_t w = 2205; // the product window const std::size_t n = 40000; // ~17 splice cycles at ratio 2 @@ -515,28 +527,34 @@ static void testStereoLinkedLagSharedSchedule() { srcL[i] = static_cast(std::sin(2.0 * kPi * fL * static_cast(i))); srcR[i] = static_cast(std::sin(2.0 * kPi * fR * static_cast(i))); } - PitchShifter master, follower, independent; + PitchShifter master, follower, independent, mirror; master.configure(w); follower.configure(w); independent.configure(w); + mirror.configure(w); master.prime(srcL.data(), w); follower.prime(srcR.data(), w); // linked: R content, master's decisions independent.prime(srcR.data(), w); // control: R content, OWN search (pre-fix behavior) + mirror.prime(srcL.data(), w); // SAME content as master: bit-identical witness master.setShiftRatio(2.0); follower.setShiftRatio(2.0); independent.setShiftRatio(2.0); + mirror.setShiftRatio(2.0); int spliceCount = 0; bool followerDiverged = false; bool independentDiverged = false; + bool mirrorDiverged = false; for (std::size_t i = 0; i < n; ++i) { const std::size_t si = i + static_cast(w); - (void)master.process(srcL[si]); + const AudioSample oM = master.process(srcL[si]); const SpliceEvent& em = master.lastSplice(); const AudioSample oR = follower.processLinked(srcR[si], em); CHECK(std::isfinite(oR)); // The follower mirrors the master's decision EXACTLY, every frame (fired == false - // frames included — a follower must never splice on its own). + // frames included). In this driven-every-frame lockstep run the follower never falls + // behind, so it never reaches the Q-W0 self-healing fallback — every splice here goes + // through applySplice(), same as the mirror check below. const SpliceEvent& ef = follower.lastSplice(); if (ef.fired != em.fired || ef.jump != em.jump || ef.lag != em.lag || ef.frac != em.frac || ef.fadeLen != em.fadeLen) { @@ -550,10 +568,16 @@ static void testStereoLinkedLagSharedSchedule() { if (ei.fired != em.fired || ei.lag != em.lag || ei.frac != em.frac) { independentDiverged = true; } + // The bit-identical witness: same content as the master, master's decisions applied + // via applySplice() instead of computed via splice() — the two code paths must produce + // the exact same sample stream. + const AudioSample oMirror = mirror.processLinked(srcL[si], em); + if (oMirror != oM) mirrorDiverged = true; } CHECK(spliceCount >= 3); // the run actually exercised several splices CHECK(!followerDiverged); // linked lag: one decision, one schedule, both channels CHECK(independentDiverged); // non-tautology witness: unlinked channels DO disagree + CHECK(!mirrorDiverged); // applySplice() reproduces splice() bit-identically } int main() {