docs(q-w0): T1 DSP audit — 11 triaged findings, SOLA engine sound; stereo splice decorrelation is the headline

docs/product/audit-notes/q-w0-t1-dsp.md

@@ -0,0 +1,279 @@
+# Q-W0 Track 1 — DSP / audio algorithm-quality audit (findings)
+
+Static analysis only; no code changed. Surfaces per the track brief: `src/vst/pitch_shift.{h,cpp}`
+(highest priority — GA correlation-aligned SOLA rewrite + GA2 prime + GA3 freeze, never audited),
+`src/vst/sampler_core.{h,cpp}`, `src/peaks.*`, `src/wav_trim.*`, the capture/tail paths
+(`src/capture.cpp`, `src/capture_realtime.cpp`, `src/realtime_record.h`), `src/vst/master_gain.*`,
+`src/vst/velocity_curve.*`.
+
+Dispositions follow Q-11 (SETTLED): default document-and-defer; a bounded SOLA fix is weighed
+before any technique replacement; technique replacement is Daniel's decision at triage. Note on
+"assigned wave": **no structural wave Q-W1..Q-W6 opens the `src/vst/` DSP files** (they target
+`core/json`, `bank_panel`, `main`, `actions`, `persist`, registration tables) — so any DSP finding
+triaged fix-now is remediated in Q-W0 itself or folded in as a new point before Q-W1 begins.
+
+I cannot listen; every artifact below is stated as mechanism + predicted audible consequence.
+Perceptual materiality is Daniel's call.
+
+---
+
+## Overall verdict on the pitch engine (Q-11 framing)
+
+The correlation-aligned SOLA in `pitch_shift` is **not a reinvented wheel in the pejorative
+sense** — single-tap SOLA with normalized cross-correlation splice alignment, parabolic sub-sample
+peak refinement, and amplitude-complementary raised-cosine fades *is* an established time-domain
+technique family (SOLA/TD-PSOLA lineage), and the implementation is unusually well-defended:
+normalized (not raw) correlation, ratio-scaled fade lengths with a drain-headroom derivation,
+prime-with-real-content onset, frozen-writer tail, filled-span clamping, and double-before-int64
+clamps at the overflow-prone spots. The RT discipline holds throughout: `process()` does no
+allocation, no locks; the splice burst is bounded and fires once per splice cadence, not per frame.
+**No technique replacement (phase-vocoder / WSOLA) is warranted on this evidence.** The findings
+below are bounded-fix candidates and documented limits within the existing approach, exactly the
+Q-11 escalation ladder's first rung.
+
+---
+
+## Findings
+
+### T1-01 — Stereo Preserve: per-channel independent splice alignment decorrelates L/R
+- **Location:** `src/vst/sampler_core.cpp` `Voice::advanceFrame` (Preserve branch, ~577–589) +
+  `src/vst/pitch_shift.cpp` `PitchShifter::splice`.
+- **Mechanism:** a stereo Preserve voice owns two `PitchShifter`s, each running its **own**
+  correlation search on its own channel's PCM. `bestLag + frac` differ per channel at every splice,
+  so after the first splice the two read taps sit at different ring positions — an inter-channel
+  time offset of up to ±`maxLag` (= window/4 ≈ **12.5 ms** at the 50 ms window), re-drawn at every
+  splice (cadence ≈ window/|ratio−1| frames). The splice *schedules* also diverge (delay drift
+  depends on tap position), so L and R fade at different times.
+- **Predicted artifact:** on genuinely stereo captures played through Preserve off-root: stereo
+  image wander / widening that changes at the splice cadence, and comb-filter coloration on any
+  mono sum. Correlated stereo content (the common case for a captured bus) is the worst case.
+  Dual-mono (mono sample in stereo bus) is unaffected — the code correctly mirrors one shifter.
+- **Severity:** **High** (Preserve is the product-default pitch engine and the output bus is
+  permanently stereo with channel mode auto-defaulting from the capture — this hits the flagship
+  path on stereo material).
+- **Disposition proposal:** this is the strongest candidate for a **bounded SOLA fix** (Q-11 rung
+  1): link the channels — run the correlation search once (on the L+R mid signal, or on L as
+  master) and apply the same `bestLag + frac` and splice schedule to both channels. Standard
+  practice for stereo SOLA. It reshapes the `PitchShifter` seam slightly (splice decision must be
+  computable once and applied to two rings — e.g. a lag-provider hook or a two-channel shifter),
+  but no technique change and no new dependency. Recommend **fix-now in Q-W0** pending Daniel's
+  triage call; if deferred, record it as the known stereo-Preserve limitation.
+
+### T1-02 — Ratio slew mid-fade can drain the outgoing tap past the writer
+- **Location:** `src/vst/pitch_shift.cpp` `splice()` fade-length cap (~301–311) + `process()` tap
+  advance (~360–366).
+- **Mechanism:** `fadeLen_` is capped from the drain headroom **at splice time** using the
+  then-current `ratio_`. The pitch envelope legitimately slews the ratio per frame
+  (`setShiftRatio` mid-fade). A ratio that **rises** after the splice (pitch-env attack toward a
+  positive peak, or attack from a negative dip back to base) drains tap B faster than the cap
+  assumed; the code comment claims the 2-frame margin covers "any realistic per-frame bias", but
+  the margin is absolute, not slew-proportional: e.g. a splice at ratio ≈ 1 sets
+  `fadeLen_ = window/4` with no cap (drainRate ≈ 0), and a pitch-env attack ramping to +24 st
+  (ratio 4) within those ~12 ms drains tap B ≈ 3·window/4 — far past the `dLow` ≈ window/4
+  headroom. Tap B laps the parked/advancing writer and reads ring-length-stale content at up to
+  ~half fade gain.
+- **Predicted artifact:** a periodic click/garble burst at the splice cadence during fast upward
+  pitch-envelope ramps on Preserve voices. Only reachable with the AD pitch envelope enabled and
+  steep; base transpositions (constant ratio) are correctly covered by the existing cap.
+- **Severity:** Med.
+- **Disposition proposal:** document-and-defer (needs pitch-env + Preserve + steep attack to
+  trigger), with a cheap bounded fix noted for whenever the file is opened: re-tighten `fadeLen_`
+  when the ratio increases mid-fade (the `freezeTail()` re-anchor block is the exact pattern to
+  reuse), or clamp tap B's delay to ≥ 2 during a fade.
+
+### T1-03 — Preserve prime ignores the Trigger play-end bound (short-span rings hold cut content)
+- **Location:** `src/vst/sampler_core.cpp` `Voice::start` prime block (~375–400) vs. the GA3
+  `feedBound` in `advanceFrame` (~568–572).
+- **Mechanism:** the per-frame feed treats `playEnd_` (Trigger) / `frameCount` as source
+  exhaustion and freezes the writer so "no padding ever enters the ring" (GA3). But `start()`
+  primes a **full window** from `pcm[q]` bounded only by `frameCount` — not by `playEnd_` — and
+  pads with zeros past the sample end while declaring the whole window `filled_`. Two
+  consequences for spans shorter than the 50 ms window: (a) a Trigger zone's ring holds real PCM
+  **past the user's chosen stop**, which an up-shifted tap can reach and play (transposed) before
+  `readPos_ ≥ playEnd_` frees the voice; (b) a sample shorter than the window gets zero padding
+  inside the ring as declared-valid history, so splices can land in silence — a bounded re-entry
+  of exactly the burst/gap onset artifact GA2/GA3 eliminated, scoped to sub-50 ms material (short
+  drum one-shots are realistic content).
+- **Severity:** Med (bounded to short spans / short samples in Preserve; inaudible for spans ≥ one
+  window).
+- **Disposition proposal:** bounded fix candidate — prime `min(window, span-to-feedBound)` frames
+  and call `freezeTail()` immediately after prime when the span is shorter than a window (the GA3
+  machinery then recycles the real short tail, which is its designed behavior). Small and
+  contained in `Voice::start`. Recommend fix-now in Q-W0 if Daniel agrees the short-one-shot case
+  matters; else document-and-defer with this note as the record.
+
+### T1-04 — No sustain-loop crossfade (hard loop seam)
+- **Location:** `src/vst/sampler_core.cpp` `Voice::advanceFrame` loop wrap (~493–498, 610–612).
+- **Mechanism:** the sustain loop wraps by subtracting the loop length (phase-preserving) and the
+  interpolation partner wraps `i1 → loop.start`, giving one-sample continuity only. There is no
+  crossfade region: unless the user's loop points sit at amplitude/slope-matched positions, every
+  loop pass produces a step discontinuity — a click at the loop rate. `waveform_view`'s
+  zero-crossing snap on the loop markers mitigates but does not remove it (zero crossings with
+  mismatched slopes still click). Established samplers crossfade the loop seam (equal-power over a
+  user- or fixed-length region).
+- **Severity:** Med (musically prominent when it hits, fully user-avoidable with careful loop
+  placement).
+- **Disposition proposal:** document-and-defer — a loop-crossfade is a *feature* (needs a
+  crossfade-length parameter and UI surface), not a bug fix; wrong scope for a reorg phase. Record
+  as a known limitation beside the zone-loop spec.
+
+### T1-05 — Linear interpolation + no band-limiting on repitch (both engines)
+- **Location:** `src/vst/sampler_core.cpp` Varispeed read (~607–624); `src/vst/pitch_shift.cpp`
+  `readTap` (~172–185).
+- **Mechanism:** all fractional reads are first-order (linear). Linear interpolation's frequency
+  response rolls off highs and leaks imaging sidebands (the interpolation image spectrum is
+  attenuated only ~12 dB/oct); Varispeed up-shifts additionally alias (reading faster than 1× with
+  no pre-filter folds source content above the post-shift Nyquist back into band). This is classic
+  hardware-sampler behavior — often accepted, sometimes desired — and both engines share it
+  consistently.
+- **Severity:** Low (quality ceiling, not a defect; deterministic and stable).
+- **Disposition proposal:** document-and-defer as a recorded trade-off. If a quality bump is ever
+  wanted, a 4-point cubic Hermite read is a drop-in bounded upgrade at both call sites (no
+  structural change); band-limited varispeed is a much bigger lift and not recommended.
+
+### T1-06 — Correlation search: coarse step 4 can mis-lock on very high fundamentals; maxLag bounds alignment to ≥ ~80 Hz
+- **Location:** `src/vst/pitch_shift.cpp` `splice()` search loops (~240–257) and `configure()`
+  geometry (~68–72).
+- **Mechanism:** two documented-by-construction limits. (a) The coarse search samples the
+  correlation every 4 lags and refines ±3 around the coarse best — full integer coverage only
+  *near* the coarse winner. For content whose correlation oscillates with period < ~8 samples
+  (fundamentals above ~5.5 kHz at 44.1k), the coarse grid can alias and lock a non-optimal region;
+  the splice then lands up to half a period misaligned. (b) `maxLag = window/4` (~12.5 ms) cannot
+  span a full period below ~80 Hz, so deep-bass fundamentals cannot be period-aligned and splices
+  degrade toward unaligned OLA there. Both are inherent range/cost trades every SOLA makes; the
+  in-code comment already states (b).
+- **Severity:** Low (edge content: pure tones > 5 kHz, fundamentals < 80 Hz).
+- **Disposition proposal:** document-and-defer; record both bounds as the engine's stated
+  operating range. No change recommended — widening either costs splice-burst CPU linearly.
+
+### T1-07 — `splice()` up-jump clamp comment contradicts the code (margin direction)
+- **Location:** `src/vst/pitch_shift.cpp` ~196–210.
+- **Mechanism:** the comment derives the "tight cap" as `filled_ - d - maxLag_ - 2`, then says the
+  code's `- 1` is "one sample of conservative margin" — but `-1` permits a *larger* jump than
+  `-2`, i.e. the code is *less* restrictive than the comment's own derivation; the sentence has
+  the direction backwards. Re-deriving: the deepest probe is the parabola's outer lag at
+  `d + jump + maxLag + 1` (the interpolator's `i1 = i0 + 1` read-ahead moves *younger*, not
+  deeper), so the code's `-1` is exactly tight and the comment's `-2` double-counts the
+  interpolator. No out-of-range read either way; the comment is wrong, not the code.
+- **Severity:** Low (doc-only; misleads the next maintainer of a safety-critical clamp).
+- **Disposition proposal:** fix-now (comment rewrite, zero behavior change) — fold into whichever
+  Q-W0 remediation touches `pitch_shift`; if none does, a standalone one-line doc fix in Q-W0.
+
+### T1-08 — Linear-in-amplitude ADSR decay/release segments
+- **Location:** `src/vst/sampler_core.cpp` `AdsrEnvelope::tick` (~131–170).
+- **Mechanism:** decay and release ramp linearly in amplitude. Constant-slope amplitude is
+  constant-dB-rate nowhere: a long release spends most of its wall-clock at perceptually loud
+  levels then collapses abruptly (in dB terms the curve is logarithmic-late). Classic samplers use
+  exponential (constant-ratio) segments for decay/release. The evaluator itself is correct and
+  well-tested (release-from-current-level, hold-0 byte-compat re-dispatch are both right).
+- **Severity:** Low (character, not correctness; the perceptual judgment is Daniel's).
+- **Disposition proposal:** document-and-defer. An exponential-segment option is a contained
+  evaluator change but alters every existing instrument's envelope feel — a product decision, not
+  a Q-W0 cleanup.
+
+### T1-09 — Takeover-declick: `declickR_` is dead state
+- **Location:** `src/vst/sampler_core.cpp` (~598–599, 639–646, 515–518).
+- **Mechanism:** both channels deliberately share one blend weight (`declickL_` — commented), but
+  `declickR_` is still seeded and decayed every frame and never read for output. Dead state that
+  invites a future L/R-weight divergence bug. The blend itself audits **clean**: `out' =
+  (1−w)·out + w·ref` is a convex combination for w ∈ [0,1], so `|out'| ≤ max(|out|,|ref|)` — the
+  rev-2 boundedness claim is mathematically sound, the boundary-frame identity holds, and the
+  ring-out path on voice end is handled (the peer-path symmetry is present).
+- **Severity:** Low (hygiene; no audio effect).
+- **Disposition proposal:** fix-now-trivial (delete the field or rename the shared weight) — fold
+  into any Q-W0 edit of `sampler_core`; not worth its own change otherwise.
+
+### T1-10 — `planWavTruncate` silently drops chunks located after `data`
+- **Location:** `src/wav_trim.cpp` `planWavTruncate` (~151), `capture_realtime.cpp`
+  `trimAutoTailInPlace`.
+- **Mechanism:** the plan truncates the file at `dataByteOffset + keptDataBytes`. Any RIFF chunk
+  REAPER wrote *after* the data chunk (bext/iXML/smpl orderings vary by writer) is discarded; the
+  RIFF size is patched consistently so the result is a valid WAV, but metadata is lost without a
+  trace. The PCM and the trim boundary math themselves audit clean (file-rate-authoritative frame
+  math, scan confined to the tail region, -72 dB threshold single-sourced from
+  `kAutoTrimThresholdDb`, one-frame-past-last-audible per spec, no-trim fallbacks total).
+- **Severity:** Low (metadata only; audio unaffected; trim is a convenience path).
+- **Disposition proposal:** document-and-defer — note the behavior in the header's FORMAT
+  ASSUMPTION block when the file is next touched. Preserving trailing chunks would complicate the
+  single-truncating-write design for no audio benefit.
+
+### T1-11 — `makeUniqueTag` has one-second resolution (collision window)
+- **Location:** `src/capture.cpp` (~224–227) and `src/capture_realtime.cpp` (~120–123).
+- **Mechanism:** the uniqueness tag is `std::time(nullptr)` — 1 s resolution. Two captures of the
+  same `baseName` within the same wall-clock second derive the same file stem: the offline path
+  would overwrite the first render's file and mint two Samples with colliding ids. Reachable in
+  practice via `batch_capture` driving several short renders back-to-back. The realtime path
+  can't self-collide (transport exclusivity) but shares the pattern. DSP-adjacent rather than
+  DSP; recorded here because the capture paths are this track's surface — Track 2 may claim it.
+- **Severity:** Low-Med (silent data loss on collision; narrow window).
+- **Disposition proposal:** fix-now candidate, trivial: append a per-session monotonic counter to
+  the tag (both call sites). Belongs wherever Track 2/triage routes capture-path hygiene; Q-W3
+  (main/orchestration split) is the nearest wave that opens the extension capture flow, else Q-W0.
+
+---
+
+## Surfaces that came back clean
+
+- **`src/peaks.*` — clean.** The bin partition `[b·frames/binCount, (b+1)·frames/binCount)` is
+  exact integer math, remainder-distributing, no dropped tail; overflow guarded; short-buffer
+  clamped; per-channel with no fold (invariant honored). `columnMinMax` mirrors the partition with
+  64-bit products and the enclosing-bin fallback. `lastFrameAboveThreshold` scans backward with a
+  correct strictly-greater test and no wrap hazard.
+- **`src/wav_trim.*` — clean** except T1-10 (metadata note). Chunk walk is bounds-checked and
+  total; even-byte padding honored; extensible-format float discrimination via the SubFormat GUID
+  leading tag is correct; LE reads via `memcpy` (no aliasing UB); truncate plan never grows.
+- **`src/capture.cpp` (offline) — clean** from the algorithm-quality lens except T1-11. Exact
+  unrounded bounds, dither forced off (bit-identical repeats), float32-only with ground-truth
+  format blob, surgical trim-end normalize only in Auto, full snapshot/restore RAII. The
+  precision-invariant plumbing is disciplined.
+- **`src/capture_realtime.cpp` + `src/realtime_record.h` — clean** except T1-11 (shared) and the
+  already-in-code DAW-verify flags (take/frame-0 alignment assumption for the trim; abort()'s
+  best-effort finalize racing the flush — both explicitly documented in place, correctly scoped).
+  The record state machine's decisions are pure and ceiling-bounded; the trim is best-effort and
+  never eats the range body.
+- **`src/vst/master_gain.*` — clean.** Taper endpoints single-sourced; norm-0 true-zero detent
+  with the finite −60 dB floor; unity at ≈ 0.714 as documented; inverse collapses sub-floor values
+  to the detent (documented); non-finite input clamped. The dB↔linear math is correct.
+- **`src/vst/velocity_curve.*` — clean.** The Fritsch–Carlson tangent is the standard
+  weighted-harmonic-mean form (w₁ = 2h₂ + h₁, w₂ = h₂ + 2h₁), which bounds m ≤ 3·min(d₁,d₂) —
+  monotonicity and no-overshoot inside [0,1] hold as claimed; sign-change/flat neighbors pin to 0;
+  zero-span steps and coincident-X knots are handled; deserialize repairs the invariant
+  defensively. `eval` once per note-on keeps it off the per-frame path.
+- **`sampler_core` voice/steal/mono machinery — clean** (beyond the findings above): the steal
+  policy is deterministic and as documented; the mono held-stack has correct range guards against
+  uint8 aliasing, order-preserving removal, per-note velocity for retrigger fallback, and CC 123
+  as its only reset path; `soundingNote()` correctly excludes ring-out tails from the Preserve cap
+  and legato predicates; the two-tier panic semantics are right; both render overloads share one
+  summation discipline with no allocation; envelope/keymap resolution honors the rate-free-seconds
+  invariant (frames resolved at keymap build against the live rate — the prior frame-domain
+  incident is not repeated here). The per-frame `std::pow` when the pitch envelope is active is
+  bounded and acceptable.
+- **`pitch_shift` core machinery — clean** (beyond the findings above): the safe-band geometry,
+  filled-span clamps, normalized correlation, parabolic sub-sample refinement, complementary
+  raised-cosine fade (correct for phase-aligned content; the −6 dB midpoint on uncorrelated
+  content is a documented, benign trade), `freezeTail`'s fade re-anchor continuity, and the
+  down-shift ring-lap margin all audit sound. Down-shift writer-lap is unreachable for any ratio
+  above ≈ −109 st; up-shift fade drain is covered to +24 st and beyond by the ratio-scaled cap
+  (T1-02 is the slew case only).
+
+---
+
+## Summary table
+
+| ID    | Surface                          | Finding                                                    | Severity | Disposition proposal                              |
+|-------|----------------------------------|------------------------------------------------------------|----------|---------------------------------------------------|
+| T1-01 | pitch_shift + sampler_core       | Stereo Preserve: independent L/R splice alignment          | High     | Bounded SOLA fix (linked lag) — recommend fix-now in Q-W0; Daniel's call |
+| T1-02 | pitch_shift                      | Ratio slew mid-fade can lap the outgoing tap               | Med      | Document-and-defer; bounded re-cap noted          |
+| T1-03 | sampler_core (Preserve prime)    | Prime ignores Trigger playEnd / pads short samples         | Med      | Bounded fix candidate in Q-W0; else defer w/ note |
+| T1-04 | sampler_core (loop)              | No sustain-loop crossfade (hard seam)                      | Med      | Document-and-defer (feature, not reorg scope)     |
+| T1-05 | sampler_core + pitch_shift       | Linear interp, no band-limiting on repitch                 | Low      | Document-and-defer (recorded trade-off)           |
+| T1-06 | pitch_shift                      | Coarse-search HF mis-lock; ≥ ~80 Hz alignment bound        | Low      | Document-and-defer (stated operating range)       |
+| T1-07 | pitch_shift                      | maxJump clamp comment contradicts code                     | Low      | Fix-now (comment-only), in Q-W0                   |
+| T1-08 | sampler_core (ADSR)              | Linear-amplitude decay/release segments                    | Low      | Document-and-defer (product decision)             |
+| T1-09 | sampler_core (declick)           | `declickR_` dead state                                     | Low      | Fix-now-trivial, fold into any Q-W0 edit          |
+| T1-10 | wav_trim                         | Truncate drops post-`data` chunks (metadata)               | Low      | Document-and-defer (header note)                  |
+| T1-11 | capture.cpp + capture_realtime   | 1 s-resolution unique tag → batch collision window         | Low-Med  | Fix-now candidate (monotonic counter); route at triage |
+
+Clean surfaces: `peaks`, `master_gain`, `velocity_curve` (fully); `wav_trim`, offline + realtime
+capture paths, and the non-flagged machinery of `sampler_core` / `pitch_shift` (clean with the
+noted exceptions above).