Merge pq-w0-audit: Q-W0 pre-restructure audit — 4 track notes + committed code-quality audit (59 findings triaged); docs-only
This commit is contained in:
@@ -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).
|
||||
@@ -0,0 +1,333 @@
|
||||
# Q-W0 Track 2 — architecture-smell audit (functional lens)
|
||||
|
||||
Static analysis of the whole `src/` tree (extension + `src/vst/`), 2026-07-28, branch
|
||||
`pq-w0-audit`. Complement to the grep-verified SOLID audit (§2) and naming audit (§2b) in
|
||||
`docs/product/code-organization.md` — this track reports the **functional** smells those did not
|
||||
target: duplicated *algorithms* (not merely duplicated responsibilities), reinvented wheels,
|
||||
poor abstractions, and leaky pure/shell boundaries. Findings already catalogued there (the four
|
||||
god-modules, the 4× JSON `Parser`, fat headers, `promptText`/`mintBankId` duplication, namespace
|
||||
flatness, naming families) are **not restated**; where a finding below touches the same file it
|
||||
is because the functional mechanism is new.
|
||||
|
||||
Every claim below was verified by grep/read of the actual tree. Line numbers are as of this
|
||||
audit's snapshot. Wave assignments reference PLAN.md §Q-W1..Q-W6.
|
||||
|
||||
---
|
||||
|
||||
## Findings
|
||||
|
||||
### T2-01 — 3× copy-pasted length-prefixed wire `Cursor`, with security-hardening drift
|
||||
**Location:** `src/provenance.cpp:56–137`, `src/assignment_request.cpp:25–121`,
|
||||
`src/sample_usage.cpp:25–89` (plus a fourth sibling: `src/vst/bank_sync.cpp:11–28`
|
||||
`parseBankGeneration` re-rolls the same guarded decimal accumulate).
|
||||
|
||||
**Mechanism.** The `<len>':'<bytes>` ext-state wire idiom ("one grammar across every ext-state
|
||||
seam", per sample_usage's own comment) is implemented as three near-identical `putField` +
|
||||
`Cursor` copies — and they have **drifted on the hardening**. The two newer copies
|
||||
(`assignment_request`, `sample_usage`) carry a 20-digit length cap and an overflow guard
|
||||
(`len > (SIZE_MAX - digit) / 10 → fail`) plus the subtraction-first bounds check
|
||||
(`len > s_.size() - start`). The oldest copy (`provenance.cpp:65–82`) has **neither**: a crafted
|
||||
long digit run wraps `len` silently, and the additive bounds check `start + len > s_.size()`
|
||||
can itself wrap, letting a wrapped length pass. Downstream, `parseFingerprint`
|
||||
(`provenance.cpp:197–199`) calls `r.trackGuids.reserve(guidCount)` on an **unbounded** count
|
||||
parsed by the equally unguarded `fieldSizeT` — a corrupt/crafted `Sample.provenance` string in
|
||||
the bank JSON can drive `reserve(huge)` into `std::length_error`/`bad_alloc` thrown through the
|
||||
shell. (`sample_usage` fixed exactly this with its `count > wire.size()/4 + 1` sanity bound,
|
||||
`sample_usage.cpp:121`; the fix was never backported.) `std::string::assign` clamping keeps the
|
||||
wrap short of UB, but the parse-integrity promise ("never UB, never a partial value") is upheld
|
||||
in two copies and eroded in the third — the textbook cost of a duplicated algorithm.
|
||||
|
||||
**Severity:** High (the drift already produced a concrete robustness gap on a persisted,
|
||||
user-editable input; the class of bug will recur with every new wire seam).
|
||||
**Disposition:** **fix-now, split:** (a) backport the hardened `field()` + a count sanity bound
|
||||
to `provenance.cpp` **in Q-W0** — small, pure, existing `provenance_tests` covers round-trip and
|
||||
malformed-input paths; (b) the structural collapse (one shared `wire` codec module beside
|
||||
`core/json`, all three seams + `parseBankGeneration` consuming it) belongs to **Q-W1**, which is
|
||||
already the serialization-extraction wave. Rationale: the hazard is cheap to close now; the
|
||||
dedup is a relocation-adjacent move that should ride the wave already creating `core/`.
|
||||
|
||||
### T2-02 — a FIFTH hand-rolled JSON decoder the §2 audit did not count
|
||||
**Location:** `src/tail_control.cpp:88–130` (`valueAfterKey` + `deserializeTailSetting`).
|
||||
|
||||
**Mechanism.** The catalogued DRY violation is "JSON `Parser` duplicated 4×" (`bank_model`,
|
||||
`bank_book`, `view_mode_model`, `owned_manifest`). `tail_control` carries a fifth, structurally
|
||||
different JSON decode: a substring-scan reader (`json.find("\"key\"")` → skip ws → parse token).
|
||||
It is correct for the flat single-object payload it reads (the file argues this honestly), but
|
||||
it is a fifth place JSON-reading behavior is defined, with different tolerance semantics (a key
|
||||
found inside a *string value* would match — impossible today only because the writer is its own
|
||||
sole producer). If Q-W1 extracts `core/json` from the four `Parser`s and misses this site, the
|
||||
"one JSON path" goal is silently not achieved.
|
||||
|
||||
**Severity:** Med (no live bug; a completeness gap in the already-planned fix).
|
||||
**Disposition:** **fix-now, folded into Q-W1** — add `tail_control` to the Q-W1 consumer list
|
||||
explicitly. Rationale: zero extra cost when `core/json` lands; a stray fifth decoder afterward
|
||||
would be a defect of the wave.
|
||||
|
||||
### T2-03 — `readFileBytes` hand-rolled five times, both sides of the artifact split
|
||||
**Location:** `src/capture.cpp:232`, `src/capture_realtime.cpp:329` (as `readAllBytes`),
|
||||
`src/ingest.cpp:86` (comment admits: "of capture.cpp's readFileBytes"),
|
||||
`src/vst/reasampler_processor.cpp:72`, and inline in `src/vst/reasampler_editor.cpp:749–756`.
|
||||
|
||||
**Mechanism.** The identical ifstream-binary-ate/tellg/read whole-file loader exists five times
|
||||
(two spellings, one anonymous inline). Well past extract-on-third-occurrence, and the copies
|
||||
already disagree cosmetically (name, empty-on-failure comment placement) — the next divergence
|
||||
will be behavioral (e.g. one copy gaining a size ceiling the others lack).
|
||||
|
||||
**Severity:** Med.
|
||||
**Disposition:** **fix-now, folded into Q-W1** — a trivial pure `readFileBytes` helper in the
|
||||
`core/` utility home Q-W1 creates; both CMake targets link it. Rationale: five occurrences of a
|
||||
ten-line function is pure debt with a zero-risk fix, but creating its home is exactly Q-W1's
|
||||
job — doing it days earlier in the flat tree would just move the file twice.
|
||||
|
||||
### T2-04 — the growing `GetProjExtState` read loop, three copies, pure half only half-used
|
||||
**Location:** `src/persist.cpp:140–157` (`getProjExtStateString`),
|
||||
`src/vst/reaper_bridge.cpp:93–107` (self-described "mirrors persist.cpp's growing strategy"),
|
||||
`src/usage_scan.cpp:168–181` (self-described "the persist.cpp idiom").
|
||||
|
||||
**Mechanism.** The grow-buffer-until-it-fits retry loop over `GetProjExtState` is implemented
|
||||
three times, in three TUs, on both sides of the split. The fiddly part — interpreting the int
|
||||
return against the filled buffer — is *already extracted pure* as
|
||||
`bridge_marshal::decodeGetProjExtState`, but only `reaper_bridge` consumes it; `persist` and
|
||||
`usage_scan` interpret `rv` inline with their own conventions (persist: `rv <= 0` → absent;
|
||||
bridge: return AND non-empty buffer). The absent-vs-truncated-vs-empty semantics are precisely
|
||||
the kind of edge that drifts when defined thrice. `usage_scan`'s copy is prune-safety-adjacent
|
||||
(an unreadable usage record must abort the prune) — its read loop deserves the tested pure
|
||||
decode, not an inline reimplementation.
|
||||
|
||||
**Severity:** Med.
|
||||
**Disposition:** **fix-now, assigned to the downstream wave that opens `persist`**
|
||||
(Q-W4 per the current wave map; whichever wave splits `persist.cpp` is the moment). Generalize
|
||||
the retry policy (next-capacity/done decision) into `bridge_marshal` (or its `core/` successor)
|
||||
and make all three loops consume it. Rationale: touching persist's session machinery outside
|
||||
its own wave risks the highest-traffic shell for a dedup that has no live bug today.
|
||||
|
||||
### T2-05 — 19 rect structs + ~15 inline point-in-rect predicates across the pure UI family
|
||||
**Location (structs):** `action_bar.h:61`, `bank_grid.h:23`, `card_drag.h:109`,
|
||||
`component_geometry.h:28,53,106`, `drag_out.h:40`, `footer_bar.h:41`, `mode_switch.h:21,35`,
|
||||
`overflow_menu.h:23,39`, `prune_button.h:32,46`, `tab_strip.h:24,51`, `tooltip.h:20`,
|
||||
`vst/editor_geometry.h:19`, `vst/velocity_curve.h:124`.
|
||||
**Location (predicates):** inline half-open `px >= r.x && px < r.x + r.width && …` re-typed in
|
||||
`action_bar.cpp`, `bank_grid.cpp`, `card_drag.cpp` (×2), `component_geometry.cpp` (×2),
|
||||
`drag_out.cpp`, `footer_bar.cpp`, `mode_switch.cpp`, `overflow_menu.cpp`, `prune_button.cpp`,
|
||||
`tab_strip.cpp` (×2), `bank_panel.cpp:1049`, plus `vst/editor_geometry.cpp:20`.
|
||||
|
||||
**Mechanism.** §2b.2 catalogued the *naming/collision* half of this (the `footer_bar.h` "NAME
|
||||
NOTE" hand-checking smell). The functional half is uncatalogued: nineteen structurally identical
|
||||
axis-aligned `{x, y, w, h}` record types, each with its own hand-typed containment predicate.
|
||||
Every new pure-UI module re-mints both. This is Daniel's heuristic (b) verbatim: N near-identical
|
||||
concrete implementations that one shared type collapses at compile time — one `ui::Rect` + one
|
||||
`contains(Rect, x, y)` free function (both already exist in embryo as `vst/editor_geometry`'s
|
||||
`Rect`/`contains`), with per-module aliases or thin wrappers only where a struct carries extra
|
||||
fields (e.g. `TabRect::index`). Zero runtime cost; deletes ~15 chances for the next half-open/
|
||||
closed-interval inconsistency to slip in.
|
||||
|
||||
**Severity:** Med.
|
||||
**Disposition:** **fix-now, folded into Q-W2** (the ui/ relocation wave) — collapsing the type
|
||||
zoo is nearly free precisely when every one of these files is being moved and re-namespaced;
|
||||
doing it pre-reorg would churn 19 headers twice. Rationale: same-moment-as-relocation is the
|
||||
stated principle for renames (§2b intro); it holds identically for type unification.
|
||||
|
||||
### T2-06 — pure-computable layout math stranded in the VST editor shell (the §2 scope gap)
|
||||
**Location:** `src/vst/reasampler_editor.cpp` — `SampleFaceLayout` (~line 922) and its builder,
|
||||
`ClusterLayout` (~line 964), `zonesStripArea`/`noteEntryFieldsArea`/`noteEntryFieldRect`/
|
||||
`zonesControlPanel`/`zonesDeckArea`/`zonesCurveButton` (lines 992–1042), the channel-toggle
|
||||
segment rects (~line 1065), banner rect math (~line 1195), among ~49 inline geometry
|
||||
computations across the 3,065-LOC TU.
|
||||
|
||||
**Mechanism.** The codebase's own grammar homes exactly this class of math in pure modules
|
||||
(`editor_geometry`, `knob_deck`, `curve_popup`, `capture_browser`, …), yet the editor shell has
|
||||
accreted a second, untested layout layer: whole named layout structs and pure `Rect → Rect`
|
||||
functions that take only ints and rects, compiled into the one TU that cannot be unit-tested
|
||||
without a host window. This is the mirrored form of the pure/shell leak (algorithm math living
|
||||
untestable in a shell). Note also the audit-scope gap this exposes: §2's god-module catalogue
|
||||
covered the extension tree only — `reasampler_editor.cpp` (3,065 LOC) and
|
||||
`reasampler_processor.cpp` (1,164 LOC) repeat the bank_panel pattern on the VST side and appear
|
||||
in no existing finding.
|
||||
|
||||
**Severity:** Med (no correctness bug found in the stranded math; the cost is untestability and
|
||||
the growth trajectory — the editor gained ~500 LOC/phase through r11).
|
||||
**Disposition:** **document-and-defer, with a named reshape:** Q-W0 should surface a downstream
|
||||
point (the wave that opens `src/vst/`, or a new one) hoisting the Sample-face/Zone-panel layout
|
||||
into the existing pure homes (`editor_geometry` is the natural owner). Rationale: a hoist is a
|
||||
behavior-preserving mechanical move best done under the reorg's test discipline, not pre-reorg;
|
||||
but it must be a recorded point or the layer keeps growing.
|
||||
|
||||
### T2-07 — the extension links the entire voice engine to serialize one preset blob
|
||||
**Location:** `CMakeLists.txt:383–385` (`instrument_drop` → PUBLIC `sample_map`);
|
||||
`src/instrument_drop.cpp` includes `vst/sample_map.h`, which pulls `sampler_core.h` →
|
||||
`pitch_shift.h` + `velocity_curve.h`.
|
||||
|
||||
**Mechanism.** `instrument_drop` (extension side) deliberately reuses
|
||||
`sample_map::serializeComponentState` so the `.vstpreset` payload and the instrument's own
|
||||
reader cannot drift — the right DRY call, explicitly documented in CMake. But the shared writer
|
||||
lives *inside* the module that also owns zone resolution, WAV decode plumbing, and (via header
|
||||
fan-in) the whole voice engine — so `reaper_reasampler` compiles and links `sampler_core`,
|
||||
`pitch_shift`, and `velocity_curve` object code it never executes. The abstraction is right; its
|
||||
*granularity* is wrong: the ComponentState codec is not separable from the engine stack today.
|
||||
|
||||
**Severity:** Low (dead weight in the binary and a misleading dependency edge; no runtime cost —
|
||||
heuristic (c) is about call chains, which this does not add).
|
||||
**Disposition:** **document-and-defer to Q-W1/Q-W2 module-homing:** when serialization gets its
|
||||
`core/` home, split a `component_state` codec module (types + serialize/deserialize only) out of
|
||||
`sample_map`; both artifacts link the codec, only the VST links the engine. Rationale: purely
|
||||
structural, zero behavior change, and exactly the kind of module-boundary decision the reorg
|
||||
waves exist to make once, deliberately.
|
||||
|
||||
### T2-08 — WAV/RIFF byte-format knowledge spread across four modules, two chunk walkers
|
||||
**Location:** `src/wav_trim.cpp` (canonical parse: `parseWavLayout`/`extractFloatFrames`),
|
||||
`src/capture_paths.cpp:31–115` (a second, independent RIFF chunk walker for content hashing),
|
||||
`src/ingest.cpp:108–160` (hand-built 32f WAV writer), `src/capture_realtime.cpp:423` (in-place
|
||||
RIFF/data size patch).
|
||||
|
||||
**Mechanism.** The tree is disciplined about *decoding* ("no third WAV reader" — sample_map,
|
||||
editor, processor all route through `wav_trim`), but RIFF *container* knowledge is still minted
|
||||
per site: `capture_paths` walks chunks with its own tag/size/pad-byte logic to hash `fmt `+`data`
|
||||
while skipping metadata; `wav_trim` walks the same container shape for layout; `ingest` writes
|
||||
headers by hand; `capture_realtime` patches sizes by offset. Four places know the RIFF framing
|
||||
rules (even-byte padding, chunk-header arithmetic); a drift in any one (e.g. pad-byte handling)
|
||||
would desynchronize hashing from decoding — the dedup-by-hash and null-test invariants both sit
|
||||
on this.
|
||||
|
||||
**Severity:** Low (all four are currently correct against each other by inspection; the smell is
|
||||
the maintenance surface, not a live divergence).
|
||||
**Disposition:** **document-and-defer** — consolidate into a `core/wav` home (walker + layout +
|
||||
writer + patch) when the reorg assigns module homes. Rationale: pre-reorg consolidation churns
|
||||
the capture hot path (§3 guardrail) for no functional gain; the reorg wave that relocates
|
||||
`wav_trim` is the natural moment.
|
||||
|
||||
### T2-09 — the two capture backends' Sample-stamping epilogue is copy-paste with silent divergences
|
||||
**Location:** `src/capture.cpp:490–537` vs `src/capture_realtime.cpp:505–540`.
|
||||
|
||||
**Mechanism.** The finished-capture metadata stamp — `trackGuids`, `channelCount`, `sampleRate`,
|
||||
`Master_GetTempo`, the `TimeMap_GetTimeSigAtTime` block, the WAV-aware `hashWavContent` content
|
||||
hash (comment block duplicated verbatim, ~10 lines), `createdTimestamp` — is written twice, once
|
||||
per backend. The copies have already diverged in quiet ways: offline passes `proj = nullptr`
|
||||
(active project) to `TimeMap_GetTimeSigAtTime` while realtime pins `st.proj_`; the sampleRate
|
||||
fallback logic differs in shape; realtime overrides `lengthSeconds` post-hoc. Some divergence is
|
||||
semantic (realtime's tail-trim length), but the shared stamp is one concept — a future field
|
||||
(e.g. a new provenance stamp) must currently be added in two places, and the time-sig
|
||||
active-project vs pinned-project asymmetry is exactly the kind of drift that produces a
|
||||
wrong-project stamp during a background-project capture.
|
||||
|
||||
**Severity:** Med.
|
||||
**Disposition:** **fix-now, folded into Q-W3** (the wave already hoisting capture orchestration
|
||||
out of `main.cpp` / right-sizing `capture.h`). Extract a `stampCaptureSample(Sample&, const
|
||||
CaptureRequest&, ReaProject*)` shared helper; the divergent bits (length override) stay in the
|
||||
realtime caller. Rationale: the fix touches both backend TUs, which Q-W3 opens anyway; doing it
|
||||
there keeps one review of the precision-invariant-adjacent code.
|
||||
|
||||
### T2-10 — the two thumbnail pipelines' cache-invalidation strategies have drifted
|
||||
**Location:** `src/bank_panel.cpp:422–483` (extension: `computeThumbnail` + pure
|
||||
`ThumbnailKey{id, width, generation}` via `bank_grid::thumbnailKeyString`) vs
|
||||
`src/vst/reasampler_editor.cpp:715–789` (VST: `monoPcmFor` keyed by bare `sampleId`,
|
||||
`thumbnailFor` keyed by ad-hoc `sampleId + "|" + binCount`, invalidated by wholesale
|
||||
`clear()` at lines 159–160/894).
|
||||
|
||||
**Mechanism.** The dock panel and the VST browser render the same thumbnails through the shared
|
||||
`peaks::computeEnvelope`, but the caching layer around it was re-designed independently on each
|
||||
side: the extension bakes the bank generation into a *pure, tested* key type; the editor
|
||||
hand-concats a string key with no generation and relies on call-site `clear()`s (bank-refresh,
|
||||
resize). Both are correct **today** — but correctness on the editor side is distributed across
|
||||
remembering every clear site, and the bin-clamp guard comment ("computeEnvelope pads binCount >
|
||||
frameCount…") is duplicated verbatim in both TUs (`bank_panel.cpp:462`,
|
||||
`reasampler_editor.cpp:780`), marking the copied design. A future refresh path that forgets the
|
||||
clear shows stale waveforms with no test to catch it.
|
||||
|
||||
**Severity:** Low.
|
||||
**Disposition:** **document-and-defer** — when T2-06's layout hoist opens the editor, adopt the
|
||||
pure `ThumbnailKey` (or a shared `thumb_cache` helper) on the VST side. Rationale: no live bug;
|
||||
unifying cache policy is a natural rider on the editor wave, pointless as standalone churn.
|
||||
|
||||
### T2-11 — ComponentState v1→v11 deserialize chain: sound, but the legacy branches triplicate the shared read
|
||||
**Location:** `src/vst/sample_map.cpp:775–945` (`deserializeComponentState`).
|
||||
|
||||
**Mechanism.** Audited the full lift chain for functional soundness: the bounded `ByteReader`
|
||||
latches on truncation, every version's tail fields carry per-field corrupt fallbacks
|
||||
(previewVelocity → mid default, voiceCount → default-not-clamp, gain → unity, refs → keep-parsed
|
||||
prefix), and the strict-prefix envelope discipline is honest. **No correctness finding.** The
|
||||
smell is shape: the v3, v4, and v5 branches each re-implement the mode-byte → marker → idLen/id
|
||||
→ zones read sequence that the v6+ shared path also implements (three near-copies of the same
|
||||
cursor walk, lines 806–841 vs 851+), and each new envelope version adds another
|
||||
`version >= kꞏꞏꞏV*Version` stanza to a function already ~170 lines long.
|
||||
|
||||
**Severity:** Low.
|
||||
**Disposition:** **document-and-defer, explicitly.** The legacy branches are frozen back-compat
|
||||
contract code with saved-project blobs as their only callers; rewriting them into a table-driven
|
||||
lift risks the one thing they must never break, for zero user-visible gain. Record the pattern
|
||||
so the *next* envelope bump (v12) prefers extending the shared path over minting another branch.
|
||||
(The unbounded-suffix version-constant naming is §2b territory; not restated.)
|
||||
|
||||
---
|
||||
|
||||
## Surfaces checked and found clean
|
||||
|
||||
Recorded per the wave's no-silent-omission rule; each was read/grepped this audit.
|
||||
|
||||
- **Pure-module include hygiene, both trees.** Every module CLAUDE.md claims pure was scanned
|
||||
for REAPER/SWELL/WDL/LICE/VST3-SDK includes: all clean, `src/` and `src/vst/` both. The one
|
||||
grep hit in `pitch_shift.h` is a comment (the S16 WDL-exclusion note), not an include. The one
|
||||
cross-tree include (`instrument_drop` → `vst/sample_map.h`) is pure-to-pure — see T2-07 for
|
||||
the granularity concern; it is not a boundary violation.
|
||||
- **`bank_sync`** — the generation/consume decision rules are pure, explicit, and exhaustively
|
||||
commented (rules 1–4); `parseBankGeneration` is overflow-guarded (its duplication is rolled
|
||||
into T2-01's family, not a separate defect).
|
||||
- **`bridge_marshal`** — one honest job, done pure, with the S1 string-scan JSON reader
|
||||
documented as retired (verified: no second JSON parser on the VST side; `sample_map` routes
|
||||
through `BankBook::deserialize`).
|
||||
- **The realtime record lifecycle** — *not* an implicit state machine: `RecordPhase` is an
|
||||
explicit enum with pure per-tick transitions in `realtime_record.h`; `main.cpp` holds only the
|
||||
handle + project pointer. (Its *residence* in main.cpp is catalogued §2.1; nothing functional
|
||||
to add.)
|
||||
- **Project-identity transitions** — `classifyProjectTransition` is pure (capture_paths), the
|
||||
shell passes `sameProjectObject` as a bool to keep it so; the GUID-primary layering is
|
||||
decision-tabled in one place.
|
||||
- **`usage_scan`** — every decision delegated to pure `sample_usage`; container recursion is
|
||||
depth-bounded with a protect-on-truncation fail-safe; the `std::function` parm-getter
|
||||
indirection is prune-scan-cold (heuristic (c) satisfied — no hot-path chain).
|
||||
- **Exception boundaries** — the three `catch (...)` sites (`bank_book.cpp:794` stol guard,
|
||||
`instrument_drop_win.cpp:74` REAPER-callback boundary, `render_settings.cpp:180` stod guard)
|
||||
are all documented, narrow, and non-swallowing in intent (each converts to an explicit
|
||||
failure value). No silent error swallowing found.
|
||||
- **`FxBypassGuard` (main.cpp) vs `view.cpp` park/restore** — both are snapshot-mutate-restore
|
||||
over track flags and *look* like a dedup candidate; they are deliberately not one. Different
|
||||
flag sets, different invariants (precision-neutralization vs Design-View parking), different
|
||||
failure postures. Duplication of shape, not of concept — correctly left separate.
|
||||
- **Path resolution** — `capture_paths::resolveBankFile` is the single resolver on both sides
|
||||
of the split (panel, insert, drag_out, editor, processor). No parallel path logic.
|
||||
- **WAV decode on the play path** — `wav_trim` is genuinely the only decoder (T2-08 concerns
|
||||
the *container* knowledge spread, not a second decoder).
|
||||
- **Draw layer** — the VST editor/embed compile the same `draw_kit`/`theme`/`component_geometry`
|
||||
the extension uses (verified in CMake + includes); no parallel draw vocabulary grew on the
|
||||
VST side.
|
||||
- **Interface cost audit (heuristic (c))** — `ICaptureBackend` is the tree's only virtual
|
||||
interface; two real implementations, dispatched once per capture (cold). No hot-path virtual
|
||||
or std::function chain found in `sampler_core`/`pitch_shift`/`sample_map` (all static calls).
|
||||
No interface-with-one-implementation found anywhere.
|
||||
- **Boolean-parameter proliferation** — swept `src/` headers for multi-bool signatures; the only
|
||||
hit is `bank_grid::applyClick(…, bool ctrl, bool shift, …)`, which mirrors physical modifier
|
||||
keys and reads fine at call sites. Not a finding.
|
||||
|
||||
## Cross-checks against the §2/§2b audits (gaps noted, not restated)
|
||||
|
||||
- §2's evidence base is scoped to the extension tree ("45 files / ~19,800 LOC"); the full tree
|
||||
is now ~39,000 LOC. The VST shells repeat the god-module pattern uncatalogued
|
||||
(`reasampler_editor.cpp` 3,065 LOC, `reasampler_processor.cpp` 1,164 LOC) — carried here as
|
||||
T2-06's scope note so the reorg waves size the `src/vst/` work realistically.
|
||||
- §2.1's "4× JSON Parser" undercounts by one — T2-02 (`tail_control`).
|
||||
- §2b.2's shared-rect naming hazard has an uncatalogued functional twin — T2-05.
|
||||
|
||||
## Summary table
|
||||
|
||||
| ID | Finding | Severity | Disposition | Where |
|
||||
|-------|---------------------------------------------------------------|----------|-------------------|--------------|
|
||||
| T2-01 | Wire `Cursor` ×3 with hardening drift; provenance unguarded | High | fix-now (split) | Q-W0 backport + Q-W1 dedup |
|
||||
| T2-02 | Fifth JSON decoder in `tail_control` | Med | fix-now | Q-W1 |
|
||||
| T2-03 | `readFileBytes` ×5 across both artifacts | Med | fix-now | Q-W1 |
|
||||
| T2-04 | Growing ext-state read loop ×3; pure decode half-adopted | Med | fix-now | persist's wave (Q-W4) |
|
||||
| T2-05 | 19 rect structs + ~15 inline point-in-rect predicates | Med | fix-now | Q-W2 |
|
||||
| T2-06 | Layout math stranded in VST editor shell (+§2 scope gap) | Med | document-and-defer (named reshape) | src/vst wave |
|
||||
| T2-07 | Extension links voice engine to share the preset serializer | Low | document-and-defer | Q-W1/Q-W2 homing |
|
||||
| T2-08 | RIFF container knowledge in 4 modules / 2 chunk walkers | Low | document-and-defer | core/wav homing |
|
||||
| T2-09 | Capture backends' Sample-stamp epilogue copy-paste w/ drift | Med | fix-now | Q-W3 |
|
||||
| T2-10 | Thumbnail cache-invalidation strategies drifted across split | Low | document-and-defer | editor wave |
|
||||
| T2-11 | ComponentState legacy lift branches triplicate the shared read | Low | document-and-defer | (pattern note for v12) |
|
||||
@@ -0,0 +1,220 @@
|
||||
# Q-W0 Track 3 — env-coupled-constant domain-modeling audit
|
||||
|
||||
Static analysis, 2026-07-28, branch `pq-w0-audit`. Scope: any value stored in an
|
||||
environment-coupled domain — frames, sample rate, DPI, pixels, tick cadence — that should be
|
||||
stored **rate-free / device-free and resolved at the point of use** (PLAN.md §Q-W0 env-coupled
|
||||
bullet; `docs/product/code-organization.md` §2c.3; the load-bearing `sample_map` seconds
|
||||
invariant). Findings are domain-modeling calls, not "rescale by rate" patches. The judgment bar
|
||||
applied: a finding requires (a) an env-coupled *stored* domain AND (b) an environment that can
|
||||
actually change under it. Frame counts computed transiently from seconds at the use site are
|
||||
correct and are not reported.
|
||||
|
||||
Waves referenced for disposition: Q-W1..Q-W6 open `bank_panel.cpp`, `main.cpp`, `actions.cpp`,
|
||||
`persist.cpp`, and relocate the clean pure libs — **no downstream wave opens
|
||||
`reasampler_processor.cpp` / `sampler_core.h` / `reasampler_editor.cpp` for logic change**, so
|
||||
fix-now findings in those files must be remediated in Q-W0 itself.
|
||||
|
||||
---
|
||||
|
||||
## Findings
|
||||
|
||||
### T3-01 — master-gain ramp step is a frame-domain constant anchored to 48 kHz
|
||||
|
||||
- **Location:** `src/vst/reasampler_processor.cpp:46-51` (`kGainRampRate = 1.0f / 960.0f`,
|
||||
`kGainRampSnap`), applied per-sample at `:1069-1085` (stereo) and `:1114-1126` (mono).
|
||||
- **Stored vs. correct domain:** stored as a **per-sample linear step** — `960` is literally
|
||||
20 ms × 48 000 Hz, and the comment says so ("960 samples @ 48 kHz ≈ 20 ms"). The intended
|
||||
quantity is a **wall-clock ramp time** (~20 ms); the correct model is a seconds/ms constant
|
||||
with the per-sample step derived from `sampleRate_` at `setupProcessing` — exactly the
|
||||
pattern the same file already uses two paragraphs away for `kPreserveWindowMs`
|
||||
(`:618-623`, `:733-735`).
|
||||
- **What breaks when the environment shifts:** the ramp's wall-clock length halves at 96 kHz
|
||||
(~10 ms) and quarters at 192 kHz (~5 ms); at 44.1 kHz it stretches to ~21.8 ms. The FB1
|
||||
"no zipper" contract degrades silently as the host rate rises. Not persisted, so no on-disk
|
||||
breakage — but it is a live hardcoded-rate assumption in `src/`, which Daniel's standing
|
||||
ruling forbids.
|
||||
- **Severity:** Med.
|
||||
- **Disposition:** **fix-now, remediated in Q-W0.** Rationale: trivial, isolated,
|
||||
behavior-identical at 48 kHz; no downstream wave opens this file, so deferring means keeping
|
||||
a named violation of the no-hardcoded-rate ruling through the whole reorg. Store
|
||||
`kGainRampSeconds = 0.020`, derive the step from the live rate where `sampleRate_` is set.
|
||||
|
||||
### T3-02 — takeover-declick decay is a per-frame coefficient (documented deliberate)
|
||||
|
||||
- **Location:** `src/vst/sampler_core.h:409-410` (`kDeclickDecay = 0.95`,
|
||||
`kDeclickFloor = 1e-4`), applied per output frame in `sampler_core.cpp:514-519`, `:643-645`.
|
||||
- **Stored vs. correct domain:** a per-output-frame exponential coefficient; the implied
|
||||
wall-clock decay-to-floor is ~4.2 ms at 44.1 kHz and ~1.9 ms at 96 kHz. The strict
|
||||
domain-model form would be a time constant in seconds resolved to a coefficient at engine
|
||||
build.
|
||||
- **What breaks when the environment shifts:** the takeover-blend residue fades ~2× faster at
|
||||
96 kHz. Audibly negligible for a declick micro-ramp — and the in-code comment
|
||||
(`sampler_core.h:397-401`) **already documents this as a deliberate per-frame DSP micro-ramp,
|
||||
"not a stored wall-clock quantity"**, with the 44.1–96 kHz variance stated and accepted.
|
||||
- **Severity:** Low.
|
||||
- **Disposition:** **document-and-defer.** Rationale: the coupling is already an explicit,
|
||||
written, bounded design decision in the code; converting it buys no audible improvement.
|
||||
Triage should ratify the in-code note as the record.
|
||||
|
||||
### T3-03 — Trigger-fade UI throw ceiling hardcodes 2 s × 44 100 as `88200.0` frames
|
||||
|
||||
- **Location:** `src/vst/reasampler_editor.cpp:395`
|
||||
(`constexpr double kFadeMaxFrames = 88200.0`), used by `controlValue`/the commit path to
|
||||
normalize the Trigger fade-in/out knobs.
|
||||
- **Stored vs. correct domain:** the fade **storage** domain (int64 SOURCE frames, persisted in
|
||||
the zones payload) is settled and correct — a source-timeline fact, invariant under project-
|
||||
rate change (PLAN.md §S15). The *UI ceiling*, however, encodes a wall-clock intent ("2-second
|
||||
max fade throw") as a frame count at an assumed 44.1 kHz source. `88200` is a rate-derived
|
||||
literal in `src/`, brushing the no-hardcoded-rate ruling even though it never touches disk.
|
||||
- **What breaks when the environment shifts:** the environment here is the **source file's
|
||||
rate**: a 96 kHz capture's maximum fade throw is ~0.92 s; a 22.05 kHz file gets 4 s. The knob's
|
||||
full-scale meaning silently varies per loaded sample.
|
||||
- **Severity:** Low (UI-only, not persisted, comment flags it as a "build-time residual — one
|
||||
place to retune").
|
||||
- **Disposition:** **fix-now, remediated in Q-W0.** Rationale: small and contained — replace
|
||||
with `kFadeMaxSeconds = 2.0` resolved against the loaded source's rate at the two normalize
|
||||
sites (the editor already threads `frameCount + rate` through the pack/unpack path,
|
||||
`envelope_edit.cpp:148`); storage domain unchanged. If triage prefers zero UI-feel change,
|
||||
the fallback is document-and-defer with the comment amended to name the 44.1 k assumption.
|
||||
|
||||
### T3-04 — drop-hint banner duration stored in sync-timer ticks
|
||||
|
||||
- **Location:** `src/vst/reasampler_editor.cpp:2920-2923` (`dropHintTicks_ = 6`), decayed in
|
||||
`onSyncTimer` (`:244-245`); field at `reasampler_editor.h:457`.
|
||||
- **Stored vs. correct domain:** a wall-clock intent ("a few seconds of banner") stored as a
|
||||
**count of `kSyncTimerIntervalMs` ticks** (6 × 500 ms). Correct model: a duration in ms,
|
||||
ticks derived — or a `GetTickCount`-style deadline like the bank panel's tooltip already
|
||||
uses.
|
||||
- **What breaks when the environment shifts:** retuning the sync cadence (a plausible perf
|
||||
tweak — the 500 ms value is itself a tuning constant) silently changes the banner duration.
|
||||
The comment does state the coupling.
|
||||
- **Severity:** Low.
|
||||
- **Disposition:** **document-and-defer.** Rationale: cosmetic, self-documenting at the single
|
||||
site, and the cadence and hint decay live three lines apart; a fix is fine to fold in
|
||||
opportunistically if the file is ever opened, but does not justify a Q-W0 edit on its own.
|
||||
|
||||
### T3-05 — systemic: no DPI/content-scale support in either UI surface
|
||||
|
||||
- **Location:** systemic. VST3 editor: `reasampler_editor.cpp:150-153` (`ViewRect(0,0,840,620)`
|
||||
default and the size floor at `:816`), all `editor_geometry` / `knob_deck` / `curve_popup` /
|
||||
`envelope_overlay` px constants (e.g. the 8 px node min-separation, the 28×28 curve button),
|
||||
cached font sizes in `draw_kit`. Extension side: the LICE-drawn `bank_panel` dock and its
|
||||
geometry modules. No implementation of VST3's `IPlugViewContentScaleSupport` anywhere in
|
||||
`src/vst/` (grep: zero hits for content-scale/DPI), no scale factor threaded through the
|
||||
pure geometry modules.
|
||||
- **Stored vs. correct domain:** every layout constant is a **physical device pixel** that
|
||||
silently assumes ~96 DPI. Correct model: logical units × one scale factor resolved at draw
|
||||
time (the pure geometry modules take widths/heights as parameters already, so a scale factor
|
||||
threads through cleanly — the constants are centralized, which is the good news).
|
||||
- **What breaks when the environment shifts:** on a 150–200 % Windows display the editor and
|
||||
dock render physically small (or get bitmap-stretched by the host, blurring text); hit
|
||||
targets like the 8 px min node separation shrink below comfortable pointer accuracy.
|
||||
Usability, not correctness — nothing mis-plays and nothing persisted is wrong.
|
||||
- **Severity:** Med (usability on modern displays; Windows-only product makes high-DPI common).
|
||||
- **Disposition:** **document-and-defer.** Rationale: a proper UI-scaling pass is a feature
|
||||
wave of its own (scale plumbing through ~15 geometry modules + font cache + both shells),
|
||||
far outside Q-W0's remediation budget; deferral should be recorded as a named future phase,
|
||||
and Q-W1's relocation of the geometry modules should keep the constants centralized so the
|
||||
eventual scale factor lands in one place.
|
||||
|
||||
### T3-06 — legacy v3 zone-payload lift divides by the *current* project rate
|
||||
|
||||
- **Location:** `src/vst/sample_map.cpp:601-605` (v3 lift inside `readZonesPayload`), format
|
||||
note at `sample_map.h:439-456`, `:510-513`.
|
||||
- **Stored vs. correct domain:** the v3 blobs (Daniel's beta projects) stored wall-clock times
|
||||
as frames — **the prior incident itself**. The lift converts frames → seconds by dividing by
|
||||
the live `projectRate` threaded in at read time. That is exact only if the project rate today
|
||||
equals the rate in effect when the S15/S16 editor wrote the frames; the write-era rate was
|
||||
never recorded, so a project whose rate changed since lifts skewed times (old/new ratio,
|
||||
e.g. ~8.8 % for 44.1→48 k).
|
||||
- **What breaks when the environment shifts:** already broken by construction for
|
||||
rate-changed-since-write projects; a one-time lift residue, after which v5+ re-saves in
|
||||
seconds and the skew is frozen in, silently.
|
||||
- **Severity:** Low (legacy-only, beta-project blobs, envelope-time magnitudes; unrecoverable
|
||||
in principle — the missing datum was never written).
|
||||
- **Disposition:** **document-and-defer.** Rationale: no better conversion exists; this is the
|
||||
documented residue of the incident that motivated the seconds invariant. Worth one sentence
|
||||
in the code-quality-audit report so the skew is a recorded known, not a mystery bug later.
|
||||
|
||||
### T3-07 — SOLA correlation-segment cap of 512 frames (deliberate CPU bound; cross-ref T1)
|
||||
|
||||
- **Location:** `src/vst/pitch_shift.cpp:72`
|
||||
(`corrFrames_ = max(1, min(dLow_ - 1, 512))`; rationale comment at `:64-67`).
|
||||
- **Stored vs. correct domain:** borderline by design. The quantity being bounded is **work per
|
||||
splice** (multiply-accumulates), which is genuinely frame-domain — a CPU bound *should* be in
|
||||
frames. The side effect is that the correlation segment's wall-clock span halves at 96 kHz
|
||||
(512 frames ≈ 11.6 ms at 44.1 k, ≈ 5.3 ms at 96 k), raising the lowest frequency the
|
||||
alignment search can lock onto at high rates. All other shifter geometry correctly derives
|
||||
from `kPreserveWindowMs` resolved at the live rate.
|
||||
- **What breaks when the environment shifts:** alignment quality for low-frequency content
|
||||
degrades somewhat at high host rates; no correctness or persistence impact.
|
||||
- **Severity:** Low.
|
||||
- **Disposition:** **document-and-defer**, and hand to the T1 DSP audit for the quality call.
|
||||
Rationale: the frame domain is arguably correct for a compute bound; whether 512 is the right
|
||||
*number* is an algorithm-quality question (T1's territory), not a domain-modeling one.
|
||||
|
||||
---
|
||||
|
||||
## Surfaces checked clean
|
||||
|
||||
- **`sample_map` v5+ persistence (the reference implementation):** AHDSR + pitch-env times as
|
||||
SECONDS doubles; no rate constant anywhere in the read/write paths (`kLegacyV3NominalRate`
|
||||
deliberately does not exist); legacy v3 lift takes the rate as a parameter. Clean.
|
||||
- **ComponentState envelope v6–v11 fields:** channel mode, assign generation, preview velocity,
|
||||
voice count/mode/trigger, `masterGainLinear` (dimensionless linear), explicit flag,
|
||||
`SampleRefs` (paths + root/loop/channels intrinsics), `instanceGuid` — all rate-free or
|
||||
file-fact domains. Clean.
|
||||
- **Trigger `fadeInFrames`/`fadeOutFrames`/`startPoint`/`SampleLoop.start/end` persisted as
|
||||
int64 SOURCE frames:** deliberate, settled source-timeline facts (PLAN.md §S15;
|
||||
`bank_model.h:66-72` documents the loop rationale) — frames *of the file* are invariant under
|
||||
project-rate change; the file's own rate is stored alongside and resolved at decode. Correct
|
||||
domain, not a finding.
|
||||
- **`trigger_seam`:** frames↔fraction with `startFrame` threaded both directions; the overlay's
|
||||
fraction domain is expressly rate-invariant. Clean.
|
||||
- **`kPreserveWindowMs` (50 ms):** resolved to frames against the live host rate at both call
|
||||
sites (`reasampler_processor.cpp:618-623`, `:733-735`) — the correct pattern, cited here as
|
||||
the model T3-01 should copy.
|
||||
- **`pitch_shift` internal geometry:** ring length, fade, lag band, delay band all derived from
|
||||
the rate-resolved `window_`; ratio-scaled live fade length. Clean (T3-07 cap noted above).
|
||||
- **Tail system:** `TailSetting.manualMs` persisted in **ms**; `kMaxTailSeconds`/`kMaxTailMs`
|
||||
wall-clock; trim threshold in **dB** with the linear ratio derived
|
||||
(`render_settings.h:59-86`); the realtime decay scan resolves frames against the **file's own
|
||||
authoritative rate** (`capture_realtime.cpp:384-408`). Clean.
|
||||
- **`wav_trim`:** frame counts are parsed file facts and transient truncate plans. Clean.
|
||||
- **`bank_model` persisted metadata:** source bounds in seconds + PPQ (both stored, each for
|
||||
its consumer); loudness in dB; `sampleRate`/`channelCount` are *recorded facts about the
|
||||
file*, not assumptions; capture tempo + meter stamped at capture time deliberately so the
|
||||
bars.beats read-out is stable under later project meter changes (`card_meta`). Clean.
|
||||
- **Ext-state wires** (`banks` JSON, view-mode model, owned manifest, `assignment_request`,
|
||||
`rsusage_*`): no frame-domain values; the rate field in the assign wire is a recorded fact.
|
||||
Clean.
|
||||
- **Envelope schematic (`envelope_overlay`/`envelope_edit`):** param-domain px↔seconds scale
|
||||
derived from the live rect (`gatePxPerSecond`), sample-length-free; editor time-slider
|
||||
ceiling is `kEnvTimeMaxSeconds = 2.0` (seconds). Clean (pixel constants themselves fall under
|
||||
the systemic T3-05).
|
||||
- **Timers:** bank_panel tooltip delay uses `GetTickCount()` ms against `kTooltipDelayMs = 500`
|
||||
(wall-clock — the pattern T3-04 should copy); editor sync timer is a 500 ms `SetTimer`
|
||||
interval (ms, not ticks); the new-content detector is an event diff per tick with no
|
||||
wall-clock meaning encoded in tick counts; `retireIdleDrain` is idleness-driven, not
|
||||
time-driven. Clean.
|
||||
- **`peaks` / `waveform_view` / `master_gain` / `velocity_curve` / `keyboard_strip`:** bins and
|
||||
columns derived from rects at use; dB↔linear taper and curve math dimensionless; key rects
|
||||
from the passed strip rect. Clean.
|
||||
|
||||
## Summary
|
||||
|
||||
| ID | Location | Stored domain | Severity | Disposition |
|
||||
|----|----------|---------------|----------|-------------|
|
||||
| T3-01 | `reasampler_processor.cpp:46-51` gain-ramp step | per-sample step (20 ms @ 48 k baked in) | Med | **Fix-now (Q-W0)** — store seconds, derive step from `sampleRate_` |
|
||||
| T3-02 | `sampler_core.h:409-410` declick decay | per-frame coefficient | Low | Document-and-defer — deliberate, already documented in-code |
|
||||
| T3-03 | `reasampler_editor.cpp:395` fade throw ceiling | 88200 source frames (2 s @ 44.1 k) | Low | **Fix-now (Q-W0)** — seconds ceiling resolved vs. source rate at use |
|
||||
| T3-04 | `reasampler_editor.cpp:2923` drop-hint duration | sync-timer ticks | Low | Document-and-defer — cosmetic, coupling stated in-code |
|
||||
| T3-05 | systemic (both UI surfaces) | physical px, ~96 DPI assumed; no content-scale | Med | Document-and-defer — a UI-scaling phase of its own; keep geometry constants centralized through Q-W1 |
|
||||
| T3-06 | `sample_map.cpp:601-605` v3 legacy lift | frames ÷ *current* project rate | Low | Document-and-defer — unrecoverable legacy residue; record as known skew |
|
||||
| T3-07 | `pitch_shift.cpp:72` correlation cap | 512 frames (CPU bound) | Low | Document-and-defer — frame domain arguably correct for a compute bound; hand to T1 for the quality call |
|
||||
|
||||
Two fix-now findings (T3-01, T3-03), both assigned to **Q-W0 itself** — no downstream wave
|
||||
opens those files for logic change. Five deferrals, each with a recorded rationale. The
|
||||
persistence surfaces — the highest-stakes case — are clean: every wall-clock quantity written
|
||||
to disk since the S12 remediation is in seconds or ms, and every frame-domain persisted value
|
||||
is a source-file fact whose rate travels with it.
|
||||
@@ -0,0 +1,399 @@
|
||||
# Q-W0 Track 4 — structural sizing + placement audit
|
||||
|
||||
Date: 2026-07-28 · Branch: `pq-w0-audit` · READ-ONLY static analysis (no build, no code edits)
|
||||
|
||||
**Method.** Line counts measured with `wc -l` on the worktree; seams derived from function-definition
|
||||
skeletons (`grep` for top-level definitions + section markers) plus targeted reads. Acceptance bar =
|
||||
Daniel's three heuristics: (a) more directories a must, files ≤ ~600 lines, SRP applies to files and
|
||||
namespaces; (b) templates are good where they dedup at compile time; (c) saved CPU beats abstraction —
|
||||
no dispatch-stack blowouts, prefer static polymorphism where types are compile-time-known.
|
||||
|
||||
**Measured sizes differ from the wave brief in several places** (the tree moved after the brief was
|
||||
drafted — GA/pS/pS-usage landed): `reasampler_editor.cpp` 3065 (brief said 3035),
|
||||
`reasampler_processor.cpp` 1164 (1004), `sampler_core.cpp` 968 (1049), `sample_map.cpp` 970 (807),
|
||||
`sample_map.h` 708 (600), `sampler_core.h` 762 (820), `actions.cpp` 1016 (996), `persist.cpp` 852
|
||||
(812). All numbers below are the measured ones.
|
||||
|
||||
---
|
||||
|
||||
## 1. Oversize census + seams
|
||||
|
||||
Every `.cpp`/`.h` in `src/` (both sides) over ~600 lines, with the *real* responsibility clusters.
|
||||
Where a file is genuinely one responsibility, I say so and recommend leaving it.
|
||||
|
||||
### 1.1 The four planned splits — do the plan's seams still land sub-600?
|
||||
|
||||
**T4-01 — `src/bank_panel.cpp` (3459; plan assumed 2424).**
|
||||
The plan's six seams (`panel_render` / `panel_thumbnails` / `panel_audition` / `panel_input` /
|
||||
`panel_bank_ops` / `panel_window`) no longer all land sub-600 at current size. Tally against the
|
||||
skeleton:
|
||||
|
||||
| Planned TU | Functions (line spans) | Est. LOC | Verdict |
|
||||
|---|---|---|---|
|
||||
| `panel_thumbnails` | `computeThumbnail`/`thumbnailFor` (420–487) | ~130 | fine |
|
||||
| `panel_render` | `drawCardMeta`/`drawThumbnail` (487–547), kit adapters (547–570), `drawFooter` (685–780), `drawToolbar`/`drawMoreButton`/`drawTooltip` (992–1155), `drawRegionGrid`/`drawCardDropTarget`/`drawRegionHeader`/`drawTabStrip`/`paintPanel` (1334–1646) | ~700 | **over — needs the layout cut below** |
|
||||
| *(unplanned)* **`panel_layout`** | toolbar/footer/menu rect + row/cluster builders (571–684, 785–991), split geometry + region rects + L7 slot-order display bridge (1156–1333) | ~500 | **new TU required** — this is pure-ish geometry glue, distinct from LICE drawing; extracting it puts `panel_render` at ~550 |
|
||||
| `panel_audition` | `initPreview`/`startAudition`/`stopAudition`/`deinitPreview` (1874–1965) | ~90 | fine (keep the direct call-through guardrail) |
|
||||
| `panel_input` | input helpers + `regionAt` (1965–2010), click routing `handleBanksChromeClick`…`handleKey`/accelerator (2384–2710), plus new-content detection (1647–1874, ~230) | ~800 | **over — needs the drag cut below** |
|
||||
| *(unplanned)* **`panel_drag`** | `updateDropTarget`/`dropTargetBankId`/`classifyCardDrag`/`applyDragCursor`/`resolveHover`/`updateHover`/`maybeShowTooltip`/`onMouseMove`/`doReorderDrop`/`doReplaceDrop`/`resetDragState`/`onLBtnUp`/`handleRightClick` (2711–3185) | ~475 | **new TU required** — the card-drag/hover state machine is a cohesive cluster of its own (it already has a pure mirror, `card_drag`); extracting it puts `panel_input` at ~550 |
|
||||
| `panel_bank_ops` | `promptText`/`mintBankId`/`doCreateBank`…`removeSamples`/`focusedSelectionIds`/`resolveDragPathsForOs` (2006–2231) + popup menus (2231–2384) | ~375 | fine (menus ride with bank_ops or input — either works; they invoke the ops) |
|
||||
| `panel_window` | `handleDropFiles`/`dlgProc`/`openPanel`/`closePanel` (3185–3336) + public API (3336–3459) | ~275 | fine |
|
||||
|
||||
**Proposal:** eight TUs, not six — add `panel_layout` and `panel_drag`.
|
||||
**Severity:** high (it is the biggest file in the repo). **Disposition: reshapes wave Q-W2** —
|
||||
the wave brief must name eight seams, or two of its six TUs ship >600 on day one.
|
||||
|
||||
**T4-02 — `src/main.cpp` (1897; plan assumed 1762).**
|
||||
The plan's three hoists are the right seams, but `capture_orchestrator` as specced lands **~885
|
||||
lines** — over by half again. Tally: `FxBypassGuard` (627–731, ~105), `renderOffline` +
|
||||
`captureAndIndexOne` + `RunCapture` + `RunCaptureItemAssign` (731–909, ~180), batch family
|
||||
(`ItemSelectionGuard`/`selectOnlyItem`/`RunBatchCaptureItems`/`collectRazorAreas`/
|
||||
`TrackSelectionGuard`/`RunBatchCaptureRazor`, 909–1181, ~270), `RunRecaptureFromSource` (1200–1398,
|
||||
~200), realtime + insert actions (1398–1512, ~115). `scope_resolve` (361–590) ≈ 230 ✓;
|
||||
`realtime_lifecycle` (186–360) ≈ 175 ✓; registration/entry residue (1512–1897) ≈ 385 ✓ (shrinks
|
||||
further under Q-W6's table).
|
||||
**Proposal:** split the orchestrator seam once more: `capture_orchestrator` (FxBypassGuard +
|
||||
single-capture path + realtime/insert action bodies, ~450) and **`capture_batch`** (batch family +
|
||||
`RunRecaptureFromSource` + the two selection guards, ~470). Recapture is planner-driven like batch
|
||||
and shares the selection-guard machinery — it belongs with batch, not the single-shot path.
|
||||
**Severity:** high. **Disposition: reshapes wave Q-W3** — add the fourth TU to the brief.
|
||||
|
||||
**T4-03 — `src/actions.cpp` (1016; plan assumed 981).**
|
||||
Plan's seams still land: `design_view_actions` (59–421, ~360), `bank_actions` (422–1016 minus prune,
|
||||
~490), `prune_action` (`doBankPruneFolder` 821–916 + registration share, ~130). All sub-600.
|
||||
**Disposition: no change to Q-W4.**
|
||||
|
||||
**T4-04 — `src/persist.cpp` (852; plan assumed 766).**
|
||||
Plan's seams still land: `ext_state_io` (helpers + `saveToActiveProject` + `writeAssignmentRequest`,
|
||||
112–288, ~180), `prune_fs` (`scanPruneOrphans`/`pruneDryRun`/`pruneOrphanSet`/`deleteOrphanFile`/
|
||||
`pruneReclaim`, 288–539, ~250 — pS-usage growth landed here, exactly where the plan isolates it),
|
||||
`session` (load/guid/poll/reload, 539–852, ~315). All sub-600.
|
||||
**Disposition: no change to Q-W5.**
|
||||
|
||||
### 1.2 Known offenders beyond the planned four — extension side
|
||||
|
||||
**T4-05 — `src/bank_book.cpp` (1109) + `bank_book.h` (457).**
|
||||
Three genuine seams: **`SlotMap`** (27–143, ~115 — a self-contained ordered-slot container with its
|
||||
own serialize at 614), **`BankBook`** registry/CRUD/transfer/slot-reconcile (145–545, ~400), and
|
||||
**JSON serialize + `Parser`** (547–1109, ~560). Q-W1 deletes the Parser + `ObjWriter`/`writeEscaped`
|
||||
copies; what remains of serialization rewired onto `core/json` is ~150.
|
||||
**Proposal:** after Q-W1, split `slot_map` into its own TU/header pair (it is a distinct type with
|
||||
its own tests-worthy invariants); `bank_book.cpp` lands ~550. **Severity:** medium.
|
||||
**Disposition:** fold into Q-W1 (the JSON rewire already opens this file; the `slot_map` file split
|
||||
is one `git mv`-shaped extraction on top).
|
||||
|
||||
**T4-06 — `src/view_mode_model.cpp` (1049) + `view_mode_model.h` (748).**
|
||||
Four seams: **indexes** (`ModeRegistry`/`MembershipIndex`/`LaneOwnershipIndex`, 29–140), **pure
|
||||
planners** (`autoTagNewContent`/`planItemRetag`/`planLaneMinting`/`makeParkPlan`/`makeRestorePlan`/
|
||||
`nextModeId`, 143–326, ~185), **`ViewModeModel`** state + visibility/toggle planning (332–490), and
|
||||
**JSON serialize + `Parser`** (494–1049, ~555). Q-W1 deletes the Parser (~390); remainder ~660.
|
||||
**Proposal:** split planners (`view_plan.cpp`) from model+indexes (`view_mode_model.cpp`, ~450 after
|
||||
JSON extraction). The header's 26 structs split the same way: mode/membership/lane types + model
|
||||
class vs. the plan-record structs (`TrackPlan`/`TogglePlan`/`AutoTag`/`ItemRetagOp`/`LaneMint*`).
|
||||
**Severity:** medium. **Disposition:** fold into Q-W1 (JSON rewire opens the file; planner split
|
||||
rides it). If the wave wants to stay minimal, the planner split can defer — post-extraction ~660 is
|
||||
marginal, not pathological.
|
||||
|
||||
**T4-07 — `src/bank_model.cpp` (767).**
|
||||
Two seams only: the model (`Sample` equality + `BankIndex` verbs, 25–145, ~120) and JSON
|
||||
(`ObjWriter`/`writeSample`/`Parser::parseSample`/`parseIndex`, 148–767, ~620). This file is the
|
||||
poster child for Q-W1: after the extraction it is ~250 total (model + thin serialize using
|
||||
`core/json`). **Disposition: already owned by Q-W1; no new seam needed.**
|
||||
|
||||
**T4-08 — `src/capture_realtime.cpp` (867).**
|
||||
Two seams: the **async record lifecycle** (`RealtimeCaptureState` snapshot/restore, `begin`/`tick`/
|
||||
`abort`, 177–324 + 569–867, ~450) and the **file-side finalize** (`readAllBytes`/`writeU32LE`/
|
||||
`trimAutoTailInPlace`/`finalizeRecording`, 324–566, ~240). The lifecycle is genuinely one
|
||||
responsibility (the header itself documents why the restore lives on the state object). The finalize
|
||||
half — WAV byte-patching, decay-scan trim, move-into-bank — is a distinct concern that talks to
|
||||
`wav_trim`, not to the transport.
|
||||
**Proposal:** split `capture_realtime_finalize.cpp` (~240); lifecycle TU lands ~600 with the file
|
||||
banner. Both stay in `shell/capture/`. **Severity:** low-medium. **Disposition:** ride Q-W3 (the
|
||||
wave already renames this family per the Q-9 naming rider — same-wave file surgery is free).
|
||||
|
||||
**T4-09 — `src/view.cpp` (677).**
|
||||
Two halves: **park/restore + flag application** (`snapshotTrack`/`applyFlags`/`parkFxOffline`/
|
||||
`restoreFxOffline`/`applyMode`, ~350) and **lane management** (`laneName`/`managedLaneOrdinals`/
|
||||
`applyLanePlays`/`applyLaneOps`/`readLaneTracks`/`assignItemToLane`/`applyMintPlan`/
|
||||
`mintManagedLanes`/`reconcileManagedLanes`, ~330). The D2 lane machinery arrived after the file's
|
||||
original charter and is a separable concern.
|
||||
**Proposal:** split `view_lanes.cpp`. **Severity:** low (677 is barely over). **Disposition:**
|
||||
document-and-defer unless Q-W1's relocation is already touching it — then take the free split.
|
||||
|
||||
**T4-10 — `src/ingest.cpp` (636).**
|
||||
Seams: **pure-ish WAV/PCM helpers** (`buildFloat32Wav`/`decodePcmSource` + byte I/O, 86–217, ~130 —
|
||||
see T4-22: `buildFloat32Wav` is a pure function trapped in a shell TU), **`importFileIntoActiveBank`**
|
||||
(249–420, ~170), and the three ingest surfaces + registration (420–636). Extracting the pure WAV
|
||||
build into a testable core module (`wav_write` beside `wav_trim`, or one `wav_codec`) drops the shell
|
||||
to ~500 and gains a test target.
|
||||
**Severity:** low-medium. **Disposition:** fix in whichever wave lands the WAV consolidation
|
||||
(T4-22); the file split itself is a rider.
|
||||
|
||||
### 1.3 Known offenders — VST side (entirely absent from the current plan)
|
||||
|
||||
**T4-11 — `src/vst/reasampler_editor.cpp` (3065) + `reasampler_editor.h` (539).**
|
||||
The single biggest unplanned file — it grew past `main.cpp` in the r11 recomposition, *after* the
|
||||
plan was written. It is now the `bank_panel.cpp` of the VST artifact, with the same god-module
|
||||
profile. Real seams, from the skeleton:
|
||||
|
||||
| Proposed TU | Functions (line spans) | Est. LOC |
|
||||
|---|---|---|
|
||||
| `editor_session` | ctor/dtor, `refreshFromBank`/`rebuildVisible`/`onSyncTimer`/`commitAndReload`/`loadSelection`/`upsertPickedOverride`/effective-zone helpers (146–400), PCM + thumbnail caches `monoPcmFor`/`thumbnailFor` (719–800) | ~420 |
|
||||
| `editor_controls` | control-value map `controlValue`/`applyControl` (399–475), knob-deck plumbing `zoneDeckGroupDescs`/`deckGroupDescs`/`deckControlNorm`/`applyDeckKnob`/`deckValueLabel` (475–649), envelope pack/unpack + `commitPickedMarkers` (649–719), `applyZoneControl` (2576–2587) | ~470 |
|
||||
| `editor_layout` (pure candidate — see T4-23) | anon-ns geometry: `computeSampleBands`/`clusterRects`/zone-panel areas/`channelToggleRects` (904–1076), `computeBrowseModal` (1756–1785), `zoneContentArea` (1917–1924) | ~250 |
|
||||
| `editor_paint_sample` | `paint` dispatch (1169), `drawTitleBand`, `paintSample`/`paintEnvelopeOverlay`/`paintVelocityCurve`/`paintKnobDeck`/`paintCurveButton`/`paintCurvePopup`/`paintEmptyState` + `drawKnobFace`/`drawSpectralStrip`/`drawRootMarker` (1076–1756) | ~590 |
|
||||
| `editor_paint_browse_zone` | `paintBrowse` (1785–1917), `paintZone` (1924–2041) | ~260 |
|
||||
| `editor_input` | `resolveHover` (2041–2145), `onMouseDown` (2147–2576 — a 430-line per-face dispatch), popup/curve mouse (1637–1735), `onMouseMove`/`onMouseUp`/`onMouseRDown`/`onMouseWheel`/`onSearchChar`/`onFilesDropped` (2587–2929) | ~880 → **split by face**: `editor_input_sample` (~500: sample-face hit branches + drag state + curve popup) and `editor_input_browse_zone` (~380: browser cards/scroll/search + zone strip/note entry) |
|
||||
| `editor_platform` | IPlugView overrides `isPlatformTypeSupported`/`canResize`/`checkSizeConstraint`/`attachedToParent`/`removedFromParent`/`onSize`/`invalidate` (800–904), `wndProc` + non-Windows stubs (2929–3065) | ~280 |
|
||||
|
||||
Seven-to-eight TUs, all sub-600, each a real cohesive cluster (session/bridge state · param
|
||||
plumbing · layout · paint × 2 · input × 2 · platform). The face structure (Sample / Browse / Zone)
|
||||
is the natural input/paint split axis — it mirrors how the code already dispatches.
|
||||
**Severity: highest of the audit** — this is the largest unowned file in the repo.
|
||||
**Disposition: reshapes the wave plan — needs a new owning wave** (see §1.5 / summary table).
|
||||
|
||||
**T4-12 — `src/vst/reasampler_processor.cpp` (1164) + `reasampler_processor.h` (502).**
|
||||
Four seams: **VST3 lifecycle + bus boilerplate** (`queryInterface`…`setupProcessing`,
|
||||
`setBusArrangements`, 118–209 + 492–506, ~120), **component-state I/O** (`setState`/`getState` +
|
||||
`legacyLiftShouldRun`, 209–349 + 774–904, ~270), **accessors/param setters** (349–492, ~140), and
|
||||
**reload + drain + usage-publish + render** (`reloadInstrument`/`publishUsage`/
|
||||
`publishBuiltLocked`/`rebuildVoiceEngine`/`retireIdleDrain` 506–774 ~270, `process` 904–1164 ~260).
|
||||
**Proposal:** three TUs — `processor_state` (state I/O + accessors, ~410), `processor_reload`
|
||||
(reload/drain/usage, ~290), `reasampler_processor.cpp` (lifecycle + `process()`, ~400). Keep
|
||||
`process()` and its block-render helpers in one TU (heuristic c — see T4-27). All are member
|
||||
functions of one class; partial-class-across-TUs is the same pattern the Q-W2 panel split uses.
|
||||
**Severity:** medium-high. **Disposition:** new VST wave (see §1.5).
|
||||
|
||||
**T4-13 — `src/vst/sample_map.cpp` (970) + `sample_map.h` (708).**
|
||||
Two clean halves plus a small third: **resolution core** (bank-JSON distill/select/list, SampleRefs
|
||||
management, PCM downmix/extract/decode, `resolvePlay`, keymap builders, performance-map resolution +
|
||||
`reconcileSingleCaptureZones`, 14–406, ~390) and **binary component-state codec** (`putU32le`/
|
||||
`putU64le`/`ByteReader`, zones payload put/read, `serializePerformance`/`deserializePerformance`,
|
||||
`serializeComponentState`/`deserializeComponentState` v3→v11 lift ladder, selection codec,
|
||||
406–970, ~565).
|
||||
**Proposal:** split `component_state_io.cpp` (the codec, ~565) from `sample_map.cpp` (resolution,
|
||||
~400). The header splits the same way: wire/state structs (`ComponentState`/`SampleRefEntry`/codec
|
||||
decls) vs. resolution API (`SelectedSample`/`PerformanceMap`/`ZonePlaySeconds`/resolvers). This is
|
||||
the same shape as the extension's model-vs-JSON split and directly reduces rebuild fan-out — the
|
||||
editor and processor both include `sample_map.h` today and recompile on every codec tweak.
|
||||
**Severity:** medium-high (the codec grows every ComponentState version bump — v6→v11 in one
|
||||
quarter; it will cross 600 on its own soon). **Disposition:** new VST wave.
|
||||
|
||||
**T4-14 — `src/vst/sampler_core.cpp` (968) + `sampler_core.h` (762).**
|
||||
Contents: pitch math (15–30), `Keymap` (34–58), `AdsrEnvelope`/`TriggerEnvelope`/`PitchEnvelope`
|
||||
(62–251, ~190), `Voice` (255–680 — `advanceFrame` alone is ~200), `VoiceEngine` (680–968, ~290).
|
||||
**This TU is a genuine single responsibility — the realtime voice engine — and it is the hottest
|
||||
code in the repo.** Every function in it sits on the per-sample render path; the envelope `tick()`s
|
||||
and `Voice::advanceFrame` benefit from same-TU inlining (no LTO assumption in the build). Splitting
|
||||
the .cpp along class lines would put per-sample calls across TU boundaries — precisely the
|
||||
heuristic-(c) violation the phase forbids.
|
||||
**Proposal:** **leave the TU whole at 968** (documented exception to the 600 bar, justified by the
|
||||
hot path), but **split the header**, which is where the pain actually is: `zone_params.h` (the enums
|
||||
+ `AdsrParams`/`TriggerParams`/`PitchEnvParams`/`ZonePlayParams`/`SampleLoop`/`SampleData` — what
|
||||
`sample_map`, the editor, and the codec actually need, ~250) vs. `sampler_core.h` (Keymap + the
|
||||
engine classes, ~500). Today every UI TU that reads a param struct recompiles when a `Voice` member
|
||||
changes. **Severity:** medium. **Disposition:** header split in the new VST wave; TU stays —
|
||||
recommend recording the exception in the wave brief so nobody "fixes" it later.
|
||||
|
||||
**T4-15 — `src/view_mode_model.h` (748)** — covered under T4-06 (splits with its TU).
|
||||
**T4-16 — `src/vst/sample_map.h` (708)** — covered under T4-13.
|
||||
**T4-17 — `src/vst/sampler_core.h` (762)** — covered under T4-14.
|
||||
|
||||
### 1.4 Borderline (no action, for the record)
|
||||
|
||||
`capture.cpp` (549), `reasampler_editor.h` (539 — shrinks when the editor splits move private
|
||||
helpers into their TUs), `reasampler_processor.h` (502 — same), `bank_book.h` (457), `pitch_shift.cpp`
|
||||
(371 — single responsibility, hot, leave), `persist.h`/`capture.h` (341/234 — Q-W6 fat-header pass
|
||||
already owns them). None need action beyond what their TU splits imply.
|
||||
|
||||
### 1.5 The structural conclusion for the wave plan
|
||||
|
||||
The existing plan splits 4 files, all extension-side. The census says **9 files need splitting and 2
|
||||
need header-only splits — 5 of them VST-side, which currently have no owning wave.** The VST work is
|
||||
the same kind and size as Q-W2 (the editor alone ≈ the old bank_panel). Recommendation: add one VST
|
||||
god-module wave (call it **Q-W2v**, runnable in parallel with Q-W2 — different artifact, zero file
|
||||
overlap; or sequence after W5 as Q-W7 if Daniel wants serial waves). Q-W1's relocation scope also
|
||||
grows: the ~20 clean VST pure libs relocate + namespace in W1 alongside the extension's 30.
|
||||
|
||||
---
|
||||
|
||||
## 2. `src/vst/` placement in the Q-3 directory map
|
||||
|
||||
The settled Q-3 map (`core/{model,view,capture,audio,ui,reclaim,version,json}`,
|
||||
`shell/{capture,panel,view,persist,actions}`, `app/`) covers only the extension. Two viable shapes
|
||||
for the VST artifact; **this is Daniel's fork to call at triage.**
|
||||
|
||||
**T4-18 — Leading recommendation: integrate into the same `core/`/`shell/` top split, with
|
||||
`instrument/` subsystem dirs beneath.**
|
||||
|
||||
```
|
||||
core/instrument/engine/ sampler_core, pitch_shift, velocity_curve, master_gain
|
||||
core/instrument/map/ sample_map (+component_state_io), bank_sync, bridge_marshal, note_entry, trigger_seam
|
||||
core/instrument/ui/ editor_geometry, keyboard_strip, waveform_view, capture_browser,
|
||||
browser_scroll, param_slider, knob_deck, curve_popup,
|
||||
envelope_overlay, envelope_edit, embed_strip
|
||||
shell/instrument/ reaper_bridge, processor TUs, editor TUs, reasampler_embed, vst_entry,
|
||||
reasampler_vst.h / reasampler_uid.h
|
||||
```
|
||||
|
||||
Rationale:
|
||||
1. **One rule, no special case.** Q-3's settled reasoning is "top-level by the load-bearing
|
||||
discipline, because the pure/shell split is the invariant worth making structural." That reasoning
|
||||
is artifact-agnostic — a file's directory should tell you whether it may touch a *host* type
|
||||
(REAPER or VST3 SDK), and `shell/instrument/` says exactly that.
|
||||
2. **The artifact boundary is a link-graph fact, not a source-layout fact — and the sources already
|
||||
straddle it.** Verified cross-artifact consumers: `sample_map` links `bank_book` + `wav_trim` +
|
||||
`sampler_core`; the editor includes `draw_kit`/`theme`/`component_geometry`/`capture_paths`/
|
||||
`peaks`/`wav_trim`/`app_version`/`ext_keys` (extension-side modules); the extension's pure
|
||||
`instrument_drop` includes `vst/reasampler_uid.h`. An artifact-first subtree would either
|
||||
duplicate these or still reach across — the boundary it draws is already false.
|
||||
3. **Namespace map falls out:** `reasampler::instrument::{engine,map,ui}` beside
|
||||
`reasampler::model` etc. — the Q-4 rule applied uniformly.
|
||||
4. **CMake impact: path edits only.** Targets, links, and test executables are unchanged; the
|
||||
VST3-gate (`EXISTS pluginfactory.cpp`) already guards targets, not directories.
|
||||
|
||||
Cost to name: the VST3-gated targets stay interleaved through the top-level `CMakeLists.txt` rather
|
||||
than being isolatable behind one `add_subdirectory`. Mitigable by grouping the instrument targets
|
||||
into one guarded block (or one `include()`d .cmake file) without moving sources.
|
||||
|
||||
**T4-19 — Alternative: parallel artifact-first subtree** — `src/vst/core/{engine,map,ui}` +
|
||||
`src/vst/shell/`, extension keeps `src/core|shell|app`. Pros: the artifact boundary is visible at
|
||||
top level; the whole VST tree (sources *and* a dedicated `src/vst/CMakeLists.txt`) can sit behind
|
||||
one SDK-gated `add_subdirectory`, which is the cleanest possible expression of "this half only
|
||||
exists on Windows with the submodule slice". Cons: two parallel `core/` trees dilute the "directory
|
||||
= may it touch a host type" invariant into "check which subtree first"; the shared-module reality
|
||||
(point 2 above) means the subtree is not actually self-contained — its purity is cosmetic; and the
|
||||
gated-`add_subdirectory` win is achievable under T4-18 with an `include()` anyway. **Recommend
|
||||
T4-18; T4-19 is defensible if Daniel weighs artifact legibility above discipline uniformity.**
|
||||
**Disposition: reshapes Q-W1** (the relocation wave executes whichever shape is chosen).
|
||||
|
||||
---
|
||||
|
||||
## 3. Template-collapse opportunities
|
||||
|
||||
Judged per heuristic (b) — proposed only where duplication is real and the template earns it; two
|
||||
anti-recommendations included, because a forced template is the worse smell.
|
||||
|
||||
**T4-20 — Little-endian byte codec: real template win.**
|
||||
Five hand-rolled copies, verified: `putU32le`/`putU64le` + `ByteReader` (`vst/sample_map.cpp`
|
||||
406–500), `writeU32LE` (`capture_realtime.cpp` 342), `readU32LE` lambda (`capture_paths.cpp` 49),
|
||||
`putU32` lambda (`ingest.cpp` 134), `appendU32LE` (`instrument_drop.cpp` 18). One header —
|
||||
`core/wire/bytes.h` (or beside `core/json`): `template <class T> void putLE(std::vector<uint8_t>&,
|
||||
T)` / `template <class T> bool readLE(ByteReader&, T&)` with the double↔bits helpers — replaces all
|
||||
five, compile-time dispatched, zero runtime cost, and gives the ComponentState codec (T4-13) a
|
||||
tested primitive. Entirely off hot paths (serialization/file I/O only).
|
||||
**Severity:** medium (each new ComponentState version re-duplicates today). **Disposition:** fix in
|
||||
the wave that lands `component_state_io` (the biggest consumer); consumers rewire opportunistically.
|
||||
|
||||
**T4-21 — Rect family: unify, but with a concrete type, NOT a template.**
|
||||
Verified 12+ byte-identical `{int x,y,width,height}` structs (`ActionBarRect`/`CellRect`/
|
||||
`PanelClientRect`/`FooterBarRect`/`HeaderRect`/`SegmentRect`/`MenuBarRect`/`MenuButtonRect`/
|
||||
`FooterRect`/`ButtonRect`/`TabStripRect`/`KitBox`…) plus a *second grammar* on the VST side
|
||||
(`editor_geometry`'s `Rect` is LTRB with `left/top/right/bottom` + `height()`). The right tool is
|
||||
one concrete `ui::Rect` + `contains()` with per-role type aliases (`using ButtonRect = ui::Rect;`)
|
||||
so call sites keep their semantic names — Q-W1's "one `ui::` owner" note already points here; this
|
||||
finding extends it: (a) retire the XYWH-vs-LTRB fork by picking one grammar (LTRB has the live
|
||||
`contains`/`height` users; either works — pick once), and (b) the per-type `contains`/`hitTest*`
|
||||
one-liners collapse for free. A template rect would model nothing — the types differ in name only.
|
||||
**Watch:** cross-lib name collisions (extension `Rect` vs vst `Rect`) surface only when both headers
|
||||
meet in one TU — `sample_map` and the editor are exactly such TUs; the Q-4 sub-namespaces are the fix.
|
||||
**Severity:** medium. **Disposition:** ride Q-W1 (it is the settled `ui::` unification, widened to
|
||||
include `editor_geometry::Rect`).
|
||||
|
||||
**T4-22 — Linear rect-scan hit-tests: small template, real but modest.**
|
||||
`hitTestCell` (`bank_grid`), `hitTestSlot` (`card_drag`), and the tab/segment scans are the same
|
||||
first-rect-containing-point loop over records that carry a rect plus extra fields (`SlotCellRect`
|
||||
adds `slot`). After T4-21, a single `template <class R> int hitIndex(int px, int py,
|
||||
span<const R>)` (requiring `r.rect.contains(px,py)` or a rect accessor) collapses them. Earns its
|
||||
keep only if T4-21 lands first; alone it would be a forced template.
|
||||
**Severity:** low. **Disposition:** document-and-defer; opportunistic rider on Q-W1.
|
||||
|
||||
**T4-23 — WAV build/parse consolidation (dedup, not template).**
|
||||
`buildFloat32Wav` (ingest, 116–179) hand-writes the float32 header that `wav_trim` hand-parses and
|
||||
`capture_paths`/`capture_realtime` chunk-scan/byte-patch. One pure `wav_codec` (or fold build into
|
||||
`wav_trim`, renamed) gives one tested owner of the RIFF layout. Concrete functions; nothing to
|
||||
template. **Severity:** low-medium. **Disposition:** fix-now-sized, but assign to the wave that
|
||||
opens `ingest.cpp` (T4-10) to avoid a standalone churn commit.
|
||||
|
||||
**T4-24 — `clamp01`: dedup with one inline, anti-template.**
|
||||
Six verified copies (`envelope_overlay`, `master_gain`, `param_slider`, `reasampler_editor`,
|
||||
`velocity_curve` + `envelope_edit`'s `clamp`). One `constexpr inline double clamp01(double)` in a
|
||||
shared core header (or just `std::clamp` at call sites). Not a template candidate — `std::clamp`
|
||||
already is one. **Severity:** trivial. **Disposition:** rider on Q-W1 relocation.
|
||||
|
||||
**T4-25 — JSON `Parser`/`ObjWriter`/`writeEscaped`/`intToStr` ×4 — already owned by Q-W1;
|
||||
confirmed still accurate** (verified in `bank_model`/`bank_book`/`view_mode_model`/
|
||||
`owned_manifest`; `sample_usage` uses its own `rsusage` k/v wire, *not* a fifth JSON parser — no
|
||||
scope growth). Concrete class, not a template. **Disposition:** no change.
|
||||
|
||||
---
|
||||
|
||||
## 4. Indirection audit (heuristic c)
|
||||
|
||||
**T4-26 — `ICaptureBackend` is now a dead abstraction: one implementation, zero polymorphic call
|
||||
sites. The brief's "two implementations — earning its keep" assumption is FALSE at current state.**
|
||||
Verified: `capture.h` itself documents (lines 139–148, the "SEAM CHOICE" comment) that
|
||||
`RealtimeRecordBackend` **deliberately does not implement** `ICaptureBackend` — it has a bespoke
|
||||
async `begin/tick/abort` seam. `OfflineRenderBackend` is the sole deriver, and the only
|
||||
construction site (`main.cpp:738`) instantiates the concrete type; nobody anywhere holds an
|
||||
`ICaptureBackend*`/`&`. The interface costs a vtable and models nothing.
|
||||
**Proposal:** delete `ICaptureBackend`; `OfflineRenderBackend` becomes a plain concrete class. Note
|
||||
CLAUDE.md/CONTEXT still describe the module as "`ICaptureBackend` interface; two backends" — the doc
|
||||
should be corrected in the same commit. **Severity:** low runtime, medium hygiene (it misleads —
|
||||
this audit's own brief was misled). **Disposition:** fix in Q-W3 (the wave that rehomes the capture
|
||||
orchestration and touches every call site).
|
||||
|
||||
**T4-27 — Warning to the Q-W2v brief (T4-14): do not split `sampler_core.cpp` along class lines.**
|
||||
`AdsrEnvelope::tick`/`TriggerEnvelope::amplitudeAt`/`PitchEnvelope::tick` are called per-voice
|
||||
per-sample from `Voice::advanceFrame`, which is called per-sample from `VoiceEngine::render`.
|
||||
Same-TU definition is what lets the compiler inline this stack today (no LTO configured). A
|
||||
by-class TU split converts the hottest inner loop into cross-TU calls — the exact dispatch-stack
|
||||
blowout heuristic (c) forbids. If a split is ever wanted, the envelopes must move as
|
||||
header-defined (inline) classes, not to a TU. The engine TU staying whole at 968 is the correct
|
||||
trade.
|
||||
|
||||
**T4-28 — Warning to the Q-W2 brief (reaffirming the plan's own guardrail).** `panel_audition` and
|
||||
the preview idle path must stay direct call-throughs after the 8-TU split — the plan already says
|
||||
this; the two *added* TUs (T4-01: `panel_layout`, `panel_drag`) introduce no new risk (layout is
|
||||
paint-time, drag is input-time), but the split of `onMouseMove` (which calls hover + drag + tooltip)
|
||||
should keep per-mouse-move work as plain free-function calls, no interface.
|
||||
|
||||
**T4-29 — Warning to the processor split (T4-12).** Keep `process()` and any per-block helpers it
|
||||
calls in one TU. The state/reload/accessor TUs are UI-thread or setup-time — safe to move freely.
|
||||
The atomic-pointer-swap pattern (`publishBuiltLocked`) must not gain a virtual seam.
|
||||
|
||||
**T4-30 — No other gratuitous indirection found (verified, not assumed).** The only extension-side
|
||||
`virtual` is T4-26. VST-side virtuals are all VST3-SDK-mandated overrides (`SingleComponentEffect`,
|
||||
`CPluginView`, `IReaperUIEmbedInterface`) — not ours to remove. The layered pure→shell pairs
|
||||
(`card_drag`→`bank_panel`, `realtime_record`→`capture_realtime`, `drag_out`→`drag_out_win`,
|
||||
`prune_reconcile`→`persist`) are the load-bearing discipline, not forwarding waste — each layer
|
||||
adds the decision/side-effect split, and all are direct calls. `bank_panel`'s ~20-function free-API
|
||||
is a module boundary, not a dispatch chain; Q-W2's header segmentation thins it.
|
||||
|
||||
---
|
||||
|
||||
## Summary table — every oversize file → proposed seams → owning wave
|
||||
|
||||
| File (LOC) | Proposed TUs/headers | Owning wave |
|
||||
|---|---|---|
|
||||
| `bank_panel.cpp` (3459) | 8 TUs: render / **layout (new)** / thumbnails / audition / input / **drag (new)** / bank_ops / window | **Q-W2 (reshaped: 6→8 seams)** |
|
||||
| `vst/reasampler_editor.cpp` (3065) | 8 TUs: session / controls / layout (pure candidate) / paint_sample / paint_browse_zone / input_sample / input_browse_zone / platform | **NEW wave Q-W2v** |
|
||||
| `main.cpp` (1897) | orchestrator / **batch+recapture (new)** / scope_resolve / realtime_lifecycle / app-entry residue | **Q-W3 (reshaped: 3→4 hoists)** + Q-W6 |
|
||||
| `vst/reasampler_processor.cpp` (1164) | processor_state / processor_reload / lifecycle+process (whole) | **NEW wave Q-W2v** |
|
||||
| `bank_book.cpp` (1109) | slot_map / bank_book / JSON→`core/json` | Q-W1 (+slot_map rider) |
|
||||
| `view_mode_model.cpp` (1049) + `.h` (748) | indexes+model / planners / JSON→`core/json`; header splits likewise | Q-W1 (+planner rider) |
|
||||
| `actions.cpp` (1016) | design_view_actions / bank_actions / prune_action (unchanged) | Q-W4 (no change) |
|
||||
| `vst/sample_map.cpp` (970) + `.h` (708) | sample_map (resolution) / component_state_io (codec); header splits likewise | **NEW wave Q-W2v** |
|
||||
| `vst/sampler_core.cpp` (968) + `.h` (762) | **TU stays whole (hot-path exception, T4-27)**; header → zone_params.h + sampler_core.h | **NEW wave Q-W2v** (header only) |
|
||||
| `capture_realtime.cpp` (867) | lifecycle / finalize | Q-W3 (rides the Q-9 rename) |
|
||||
| `persist.cpp` (852) | session / ext_state_io / prune_fs (unchanged) | Q-W5 (no change) |
|
||||
| `bank_model.cpp` (767) | model / JSON→`core/json` (unchanged) | Q-W1 (no change) |
|
||||
| `view.cpp` (677) | apply / lanes | defer, or Q-W1 rider |
|
||||
| `ingest.cpp` (636) | wav helpers→core / import / surfaces | wave owning T4-23 |
|
||||
|
||||
**Wave-plan deltas requested of triage:** (1) Q-W2 grows to 8 seams; (2) Q-W3 grows to 4 hoists +
|
||||
the `ICaptureBackend` deletion; (3) a **new VST god-module wave (Q-W2v)** owns the editor /
|
||||
processor / sample_map splits + the sampler_core header split — parallel-safe with Q-W2 (zero file
|
||||
overlap); (4) Q-W1's relocation scope includes the VST pure libs under the placement shape chosen
|
||||
at the T4-18/T4-19 fork; (5) the `core/wire/bytes.h` LE-codec template (T4-20) lands with
|
||||
`component_state_io`.
|
||||
@@ -0,0 +1,370 @@
|
||||
# ReaSampler code-quality audit — Q-W0 findings report and triage
|
||||
|
||||
Date: 2026-07-28 · Branch: `pq-w0-audit` · Static analysis only; no code changed by the audit.
|
||||
|
||||
This is the committed Q-W0 findings report (Q-10 SETTLED: a committed doc beside the SOLID/naming
|
||||
audit — `docs/product/code-organization.md` §2c.3; deliverable contract in PLAN.md §Q-W0). It
|
||||
synthesizes four parallel audit tracks; the full track notes remain in the tree as appendices and
|
||||
are the evidence base for every claim here — this report cites finding IDs and does not restate
|
||||
mechanisms in full:
|
||||
|
||||
- **Track 1 — DSP / audio algorithm quality:** [`audit-notes/q-w0-t1-dsp.md`](audit-notes/q-w0-t1-dsp.md) (T1-01…T1-11)
|
||||
- **Track 2 — architecture smells (functional lens):** [`audit-notes/q-w0-t2-architecture.md`](audit-notes/q-w0-t2-architecture.md) (T2-01…T2-11)
|
||||
- **Track 3 — env-coupled-constant domain modeling:** [`audit-notes/q-w0-t3-env-constants.md`](audit-notes/q-w0-t3-env-constants.md) (T3-01…T3-07)
|
||||
- **Track 4 — structural sizing + placement:** [`audit-notes/q-w0-t4-sizing.md`](audit-notes/q-w0-t4-sizing.md) (T4-01…T4-30)
|
||||
|
||||
Dispositions below are **proposals**. Per the Q-W0 sign-off gate, Q-W1 does not begin until Daniel
|
||||
has signed off on every disposition; the open calls are collected in §4.
|
||||
|
||||
---
|
||||
|
||||
## 1. Verdicts up front
|
||||
|
||||
**DSP / pitch engine (the Q-11 question).** The correlation-aligned SOLA in `pitch_shift` is
|
||||
**sound — no technique replacement (phase-vocoder / WSOLA) is warranted on this evidence** (T1
|
||||
overall verdict). Track 1 finds the implementation "unusually well-defended" (normalized
|
||||
correlation, ratio-scaled fades with drain-headroom derivation, prime-with-real-content onset,
|
||||
frozen-writer tail, filled-span clamping) with RT discipline intact throughout. Every T1 finding
|
||||
sits on the Q-11 escalation ladder's first rungs — bounded fixes within the existing technique, or
|
||||
documented operating limits — exactly the settled default. The one High finding (T1-01, stereo
|
||||
splice decorrelation) is a bounded fix inside the current design (link the per-channel lag
|
||||
search), not a technique change. Track 1 states plainly that it cannot listen: every artifact is
|
||||
mechanism + predicted audible consequence, and perceptual materiality is Daniel's call.
|
||||
|
||||
**Architecture (functional smells).** The load-bearing boundaries hold: no pure module includes a
|
||||
host type, the VST bridge is genuinely read-only, WAV *decoding* has exactly one owner, the prune
|
||||
and exception boundaries audit clean (T2 clean list). What Track 2 found instead is the classic
|
||||
cost of duplicated *algorithms*: the length-prefixed wire `Cursor` copy-pasted 3× **with
|
||||
security-hardening drift** — the oldest copy (`provenance`) missing the overflow guards its
|
||||
siblings gained (T2-01, High, with a cheap Q-W0 backport); a fifth hand-rolled JSON decoder the
|
||||
§2 audit did not count (T2-02); `readFileBytes` ×5 (T2-03); the ext-state grow-loop ×3 with its
|
||||
pure decode half-adopted (T2-04); a 19-struct rect zoo (T2-05); and drifting copy-paste in the
|
||||
capture-stamp epilogue (T2-09). All are dedup/relocation-shaped and route onto the reorg waves;
|
||||
none is a live user-facing bug except the T2-01 robustness gap.
|
||||
|
||||
**Env-coupled constants (the prior-incident category).** The persistence surfaces — the
|
||||
highest-stakes case — are **clean**: every wall-clock quantity written to disk since the S12
|
||||
remediation is in seconds or ms, and every persisted frame-domain value is a source-file fact
|
||||
whose rate travels with it (T3 clean list). Seven findings, only two fix-now: the master-gain
|
||||
ramp step hard-codes 20 ms × 48 kHz (T3-01) and the Trigger-fade UI ceiling hard-codes
|
||||
2 s × 44.1 kHz (T3-03) — both live hardcoded-rate residues in `src/`, both trivial, both in files
|
||||
no then-planned downstream wave opens. The rest are deliberate-and-documented couplings or
|
||||
recorded legacy residue, plus one systemic usability note (no DPI/content-scale support, T3-05)
|
||||
that is a future phase of its own.
|
||||
|
||||
**Sizing + placement.** The wave plan's four planned splits are necessary but no longer
|
||||
sufficient: the census measures **9 files needing TU splits and 2 needing header-only splits — 5
|
||||
of them VST-side, which currently have no owning wave** (T4 §1.5). `reasampler_editor.cpp`
|
||||
(3,065 LOC) is now the largest unowned file in the repo. Track 4's structural answer is a new
|
||||
VST god-module wave (**Q-W2v**), reshaped seam lists for Q-W2 (6→8) and Q-W3 (3→4 hoists), one
|
||||
documented exception (`sampler_core.cpp` stays whole — hot path, T4-14/T4-27), and one dead
|
||||
abstraction to delete (`ICaptureBackend`, T4-26 — the brief's "two implementations" assumption is
|
||||
false at current state). The `src/vst/` placement question is a genuine fork for Daniel
|
||||
(T4-18/T4-19, §4a).
|
||||
|
||||
---
|
||||
|
||||
## 2. Unified findings register
|
||||
|
||||
Every finding from all four tracks, exactly once, with proposed disposition. Severities are the
|
||||
tracks' own. "Q-W0" as a destination means remediated in this wave before it closes (post
|
||||
sign-off). Cross-track overlaps are reconciled in §2.5.
|
||||
|
||||
### 2.1 Track 1 — DSP (appendix: `q-w0-t1-dsp.md`)
|
||||
|
||||
| ID | Finding (one line) | Sev | Proposed disposition | Rationale |
|
||||
|----|--------------------|-----|----------------------|-----------|
|
||||
| T1-01 | Stereo Preserve: per-channel independent splice alignment decorrelates L/R (image wander + mono-sum combing on stereo captures) | High | **Fix-now in Q-W0** (bounded SOLA fix: linked lag/schedule across channels) — **Daniel's call, §4b** | Hits the flagship path (Preserve default + permanently stereo bus); standard stereo-SOLA practice; no technique change |
|
||||
| T1-02 | Ratio slew mid-fade can drain the outgoing tap past the writer (pitch-env attack case) | Med | Document-and-defer; bounded re-cap noted for when the file is next opened | Needs pitch-env + Preserve + steep attack to trigger; constant-ratio case already covered |
|
||||
| T1-03 | Preserve prime ignores Trigger `playEnd_` bound; zero-pads sub-window samples as declared-valid ring content | Med | **Fix-now in Q-W0** (prime to feedBound + immediate `freezeTail()`) **if Daniel agrees short one-shots matter, else defer with note — §4b** | Small, contained in `Voice::start`; re-uses designed GA3 machinery |
|
||||
| T1-04 | No sustain-loop crossfade — hard loop seam clicks unless loop points amplitude-matched | Med | Document-and-defer (record beside the zone-loop spec) | A crossfade is a feature (parameter + UI), wrong scope for a reorg phase |
|
||||
| T1-05 | Linear interpolation + no band-limiting on repitch (both engines) | Low | Document-and-defer (recorded trade-off; cubic Hermite noted as drop-in if ever wanted) | Classic sampler behavior, deterministic and consistent across engines |
|
||||
| T1-06 | Correlation search: coarse step-4 can mis-lock above ~5 kHz; maxLag bounds alignment to ≥ ~80 Hz | Low | Document-and-defer (record as the engine's stated operating range) | Inherent SOLA range/cost trades; widening costs splice-burst CPU linearly |
|
||||
| T1-07 | `splice()` up-jump clamp comment contradicts the code (code is exactly tight; comment's margin direction is backwards) | Low | **Fix-now in Q-W0** (comment rewrite, zero behavior change) | Misleads the next maintainer of a safety-critical clamp; one line |
|
||||
| T1-08 | Linear-in-amplitude ADSR decay/release (constant-dB nowhere; abrupt-late releases) | Low | Document-and-defer | Character-vs-correctness product decision; changing it alters every existing instrument's feel |
|
||||
| T1-09 | `declickR_` is dead state (blend correctly shares one weight; R is seeded/decayed, never read) | Low | Fix-now-trivial **as a rider on any Q-W0 `sampler_core` edit** (T1-01/T1-03); else defer | Hygiene only, no audio effect; not worth a standalone change |
|
||||
| T1-10 | `planWavTruncate` silently drops RIFF chunks after `data` (metadata loss on trim) | Low | Document-and-defer (note in the header's FORMAT ASSUMPTION block when next touched) | Metadata-only; preserving trailing chunks complicates the single-truncating-write design for no audio benefit |
|
||||
| T1-11 | `makeUniqueTag` 1 s resolution → same-second batch captures collide (silent overwrite) | Low-Med | **Fix-now, assigned to Q-W3** (per-session monotonic counter, both call sites); Q-W0 fallback if triage prefers | T1 explicitly routes at triage; Q-W3 is the nearest wave opening the extension capture flow |
|
||||
|
||||
### 2.2 Track 2 — architecture (appendix: `q-w0-t2-architecture.md`)
|
||||
|
||||
| ID | Finding (one line) | Sev | Proposed disposition | Rationale |
|
||||
|----|--------------------|-----|----------------------|-----------|
|
||||
| T2-01 | Wire `Cursor` ×3 with hardening drift — `provenance` lacks the overflow/length guards its siblings have; unbounded `reserve` reachable from persisted input | High | **Fix-now, split:** (a) backport hardened `field()` + count sanity bound to `provenance.cpp` **in Q-W0** (§4c); (b) structural collapse to one shared wire codec **in Q-W1** | Hazard is cheap to close now with existing `provenance_tests`; the dedup should ride the wave already creating `core/` |
|
||||
| T2-02 | Fifth hand-rolled JSON decoder in `tail_control` (the §2 "4× Parser" undercount) | Med | **Fix-now, folded into Q-W1** — add `tail_control` to the `core/json` consumer list explicitly | Zero extra cost when `core/json` lands; a stray fifth decoder afterward would be a defect of the wave |
|
||||
| T2-03 | `readFileBytes` hand-rolled ×5 across both artifacts | Med | **Fix-now, folded into Q-W1** — one pure helper in the `core/` utility home; both targets link it | Five copies of a ten-line function; creating its home is exactly Q-W1's job |
|
||||
| T2-04 | `GetProjExtState` grow-loop ×3; pure `bridge_marshal` decode only half-adopted (`usage_scan`'s copy is prune-safety-adjacent) | Med | **Fix-now, assigned to Q-W5** (the wave that splits `persist.cpp` — T2's own rule; its "Q-W4" label predates the plan's persist=W5 numbering) | Touching persist's session machinery outside its own wave risks the highest-traffic shell for a dedup with no live bug |
|
||||
| T2-05 | 19 rect structs + ~15 inline point-in-rect predicates across the pure UI family | Med | **Fix-now, folded into Q-W1** — reconciled with T4-21 into one disposition, see §2.5(1) | Same-moment-as-relocation principle; PLAN Q-W1 already owns the `ui::` rect unification |
|
||||
| T2-06 | Pure-computable layout math stranded in the VST editor shell (~49 inline geometry computations; §2's VST scope gap) | Med | Document-and-defer **with named reshape — satisfied by Q-W2v's `editor_layout` pure-candidate TU** (see §2.5(4), §3) | Behavior-preserving hoist best done under the reorg's test discipline; must be a recorded point or the layer keeps growing |
|
||||
| T2-07 | Extension links the entire voice engine to serialize one preset blob (codec not separable from engine) | Low | Document-and-defer → **executed by Q-W2v's `component_state_io` split** (same split as T4-13, see §2.5(3)) | Right abstraction, wrong granularity; a module-homing decision the reorg waves exist to make once |
|
||||
| T2-08 | WAV/RIFF container knowledge in 4 modules / 2 chunk walkers (dedup-by-hash + null-test invariants sit on their agreement) | Low | Document-and-defer → **consolidation moment is a triage question, §4e** (T2 prefers the `core/wav` homing moment; T4-23 prefers the wave that opens `ingest.cpp`) | All four currently correct against each other; pre-reorg consolidation churns the capture hot path for no functional gain |
|
||||
| T2-09 | Capture backends' Sample-stamping epilogue copy-paste with silent divergences (active-project vs pinned-project time-sig read) | Med | **Fix-now, folded into Q-W3** — extract shared `stampCaptureSample` helper; divergent bits stay in the realtime caller | Q-W3 opens both backend TUs anyway; keeps one review of precision-invariant-adjacent code |
|
||||
| T2-10 | Thumbnail cache-invalidation drifted across the split (extension: pure generation-baked key; editor: ad-hoc string key + call-site `clear()`s) | Low | Document-and-defer — adopt the pure `ThumbnailKey` on the VST side **as a rider on Q-W2v's editor work** | No live bug; pointless as standalone churn, natural rider on the editor wave |
|
||||
| T2-11 | ComponentState v1→v11 deserialize chain sound, but v3/v4/v5 legacy branches triplicate the shared read | Low | Document-and-defer, explicitly — record that the next envelope bump (v12) extends the shared path rather than minting another branch | Legacy branches are frozen back-compat contract; rewriting them risks the one thing they must never break |
|
||||
|
||||
### 2.3 Track 3 — env-coupled constants (appendix: `q-w0-t3-env-constants.md`)
|
||||
|
||||
| ID | Finding (one line) | Sev | Proposed disposition | Rationale |
|
||||
|----|--------------------|-----|----------------------|-----------|
|
||||
| T3-01 | Master-gain ramp step is a per-sample constant baking in 20 ms × 48 kHz (`kGainRampRate = 1/960`); FB1 no-zipper contract degrades silently at higher rates | Med | **Fix-now in Q-W0** — store `kGainRampSeconds`, derive step from `sampleRate_` (the file's own `kPreserveWindowMs` pattern) — **§4d** | Trivial, isolated, behavior-identical at 48 kHz; a live violation of the no-hardcoded-rate ruling in a file no then-planned wave opens |
|
||||
| T3-02 | Takeover-declick decay is a per-frame coefficient (~2× faster at 96 kHz) — documented deliberate in-code | Low | Document-and-defer — triage ratifies the in-code note as the record | Already an explicit, written, bounded design decision; converting buys no audible improvement |
|
||||
| T3-03 | Trigger-fade UI throw ceiling hardcodes 2 s × 44 100 as `88200.0` frames (knob full-scale varies per source rate) | Low | **Fix-now in Q-W0** — `kFadeMaxSeconds = 2.0` resolved against the loaded source's rate; T3's stated fallback (defer, amend comment) if zero UI-feel change is preferred — **§4d** | Small and contained; storage domain unchanged; the editor already threads `frameCount + rate` through pack/unpack |
|
||||
| T3-04 | Drop-hint banner duration stored in sync-timer ticks (6 × 500 ms) | Low | Document-and-defer; fold in opportunistically if the file is opened | Cosmetic, self-documenting, cadence and decay live three lines apart |
|
||||
| T3-05 | Systemic: no DPI/content-scale support in either UI surface (all layout constants are physical px at ~96 DPI) | Med | Document-and-defer — record as a named future phase; Q-W1's geometry relocation keeps constants centralized so the eventual scale factor lands in one place | A proper UI-scaling pass is a feature wave of its own, far outside Q-W0's remediation budget |
|
||||
| T3-06 | Legacy v3 zone-payload lift divides by the *current* project rate (skewed times if the rate changed since write; write-era rate never recorded) | Low | Document-and-defer — this report is the record; the skew is a known, not a future mystery bug | Unrecoverable in principle; the documented residue of the incident that motivated the seconds invariant |
|
||||
| T3-07 | SOLA correlation-segment cap of 512 frames — frame-domain by design (CPU bound); wall-clock span halves at 96 kHz | Low | Document-and-defer — **resolved against Track 1's verdict, see §2.5(2)** | T3 judged the frame domain arguably correct for a compute bound and handed the quality call to T1 |
|
||||
|
||||
### 2.4 Track 4 — sizing + placement (appendix: `q-w0-t4-sizing.md`)
|
||||
|
||||
| ID | Finding (one line) | Sev | Proposed disposition | Rationale |
|
||||
|----|--------------------|-----|----------------------|-----------|
|
||||
| T4-01 | `bank_panel.cpp` (3459): the plan's six seams no longer land sub-600 — `panel_render` ~700, `panel_input` ~800 | High | **Fix-now → reshapes Q-W2:** eight TUs, adding `panel_layout` and `panel_drag` | Without the two new seams, two of six TUs ship >600 on day one |
|
||||
| T4-02 | `main.cpp` (1897): `capture_orchestrator` as specced lands ~885 | High | **Fix-now → reshapes Q-W3:** fourth hoist `capture_batch` (batch family + recapture + selection guards) | Recapture is planner-driven like batch and shares the guard machinery — it belongs with batch |
|
||||
| T4-03 | `actions.cpp` (1016): plan's seams still land sub-600 | — | No change to Q-W4 (confirmation) | Measured against the current tree |
|
||||
| T4-04 | `persist.cpp` (852): plan's seams still land sub-600; pS-usage growth landed exactly where the plan isolates it | — | No change to Q-W5 (confirmation) | Measured against the current tree |
|
||||
| T4-05 | `bank_book.cpp` (1109): `SlotMap` is a self-contained type; post-JSON-extraction remainder splits cleanly | Med | Fix-now, fold into Q-W1 (`slot_map` extraction rides the JSON rewire already opening this file) | One `git mv`-shaped extraction on top of owned work |
|
||||
| T4-06 | `view_mode_model.cpp` (1049) + `.h` (748): planners separable from model+indexes after JSON extraction | Med | Fix-now, fold into Q-W1 (planner split rides the JSON rewire); T4 allows deferring the planner split if the wave wants to stay minimal (~660 post-extraction is marginal) | JSON rewire opens the file; header splits the same way |
|
||||
| T4-07 | `bank_model.cpp` (767): model vs JSON — the Q-W1 poster child, ~250 after extraction | — | Already owned by Q-W1; no new seam (confirmation) | — |
|
||||
| T4-08 | `capture_realtime.cpp` (867): async lifecycle vs file-side finalize are distinct concerns | Low-Med | Fix-now, ride Q-W3 (`capture_realtime_finalize.cpp` split rides the Q-9 naming rider) | Same-wave file surgery is free |
|
||||
| T4-09 | `view.cpp` (677): park/restore vs D2 lane machinery are separable halves | Low | Document-and-defer unless Q-W1's relocation touches it — then take the free `view_lanes` split | 677 is barely over the bar |
|
||||
| T4-10 | `ingest.cpp` (636): pure WAV/PCM build helpers trapped in a shell TU | Low-Med | Fix in whichever wave lands the WAV consolidation — **moment is the §4e triage question** (circular with T4-23; no current wave opens `ingest.cpp`) | Extracting the pure WAV build gains a test target and drops the shell to ~500 |
|
||||
| T4-11 | `vst/reasampler_editor.cpp` (3065): the largest unowned file — the `bank_panel` of the VST artifact | Highest | **Fix-now → new wave Q-W2v:** eight TUs (session / controls / layout / paint ×2 / input ×2 / platform), split axis = the face structure | Grew past `main.cpp` after the plan was written; same god-module profile |
|
||||
| T4-12 | `vst/reasampler_processor.cpp` (1164): four seams | Med-High | **Fix-now → Q-W2v:** three TUs (`processor_state` / `processor_reload` / lifecycle+`process()` whole) | Partial-class-across-TUs is the same pattern as the Q-W2 panel split |
|
||||
| T4-13 | `vst/sample_map.cpp` (970) + `.h` (708): resolution core vs binary ComponentState codec | Med-High | **Fix-now → Q-W2v:** split `component_state_io.cpp` + matching header split — one disposition with T2-07, see §2.5(3) | The codec grows every envelope bump (v6→v11 in one quarter); reduces editor/processor rebuild fan-out |
|
||||
| T4-14 | `vst/sampler_core.cpp` (968) + `.h` (762): TU is a genuine single responsibility on the hottest path | Med | **Fix-now → Q-W2v, header only:** split `zone_params.h` from `sampler_core.h`; **TU stays whole — documented exception to the 600 bar** (record in the wave brief so nobody "fixes" it later) | Same-TU inlining on the per-sample path; no LTO in the build (see T4-27) |
|
||||
| T4-15 | `view_mode_model.h` (748) | — | Covered under T4-06 (splits with its TU) | — |
|
||||
| T4-16 | `vst/sample_map.h` (708) | — | Covered under T4-13 | — |
|
||||
| T4-17 | `vst/sampler_core.h` (762) | — | Covered under T4-14 | — |
|
||||
| T4-18 | VST placement, leading recommendation: integrate into the one `core/`/`shell/` split with `instrument/` subsystem dirs | — | **Daniel's fork — §4a** (T4 recommends T4-18) | One rule, no special case; the artifact boundary is a link-graph fact the sources already straddle |
|
||||
| T4-19 | VST placement, alternative: parallel artifact-first subtree (`src/vst/core|shell` behind one SDK-gated `add_subdirectory`) | — | **Daniel's fork — §4a** | Defensible if artifact legibility outweighs discipline uniformity; T4 notes its self-containment is cosmetic |
|
||||
| T4-20 | Little-endian byte codec hand-rolled ×5 — real template win (`putLE`/`readLE`) | Med | Fix-now, lands with `component_state_io` in Q-W2v (its biggest consumer); other consumers rewire opportunistically — relationship to T2-03/T2-04 noted in §2.5(5) | Compile-time dispatch, zero runtime cost, entirely off hot paths; gives the codec a tested primitive |
|
||||
| T4-21 | Rect family: unify with one **concrete** `ui::Rect` + `contains()` + per-role aliases — NOT a template; retire the XYWH-vs-LTRB fork | Med | **Fix-now, ride Q-W1** — reconciled with T2-05 into one disposition, see §2.5(1) | The types differ in name only; a template would model nothing; cross-lib `Rect` collision is fixed by the Q-4 sub-namespaces |
|
||||
| T4-22 | Linear rect-scan hit-tests: one small `hitIndex` template collapses them — only after T4-21 | Low | Document-and-defer; opportunistic rider on Q-W1 once the rect unification lands | Alone it would be a forced template |
|
||||
| T4-23 | WAV build/parse consolidation into one pure `wav_codec` owner (dedup, not template) | Low-Med | Fix-now-sized, but the owning moment is the **§4e triage question** (T4 assigns it to the wave opening `ingest.cpp`; T2-08 prefers the `core/wav` homing moment; no current wave opens ingest) | Avoid a standalone churn commit; one tested owner of the RIFF layout |
|
||||
| T4-24 | `clamp01` ×6 — one `constexpr inline` (or `std::clamp` at sites); anti-template | Trivial | Fix-now, rider on Q-W1 relocation | — |
|
||||
| T4-25 | JSON `Parser` ×4 confirmed still accurate; `sample_usage` is **not** a fifth JSON parser (no scope growth from this track) | — | No change to Q-W1 (confirmation; T2-02's fifth decoder is a different file and does grow the consumer list) | — |
|
||||
| T4-26 | `ICaptureBackend` is a dead abstraction: one deriver, zero polymorphic call sites; the brief's "two implementations" assumption is FALSE (realtime deliberately has a bespoke seam) | Low (runtime) / Med (hygiene) | **Fix-now, in Q-W3:** delete the interface, `OfflineRenderBackend` becomes concrete; **correct CLAUDE.md/CONTEXT ("ICaptureBackend interface; two backends") in the same commit** | It misleads — this audit's own brief was misled; Q-W3 touches every call site |
|
||||
| T4-27 | Warning: do **not** split `sampler_core.cpp` along class lines — envelope `tick()`s are per-voice-per-sample; a by-class split is the exact heuristic-(c) dispatch blowout | — | Record as a guardrail in the Q-W2v brief (pairs with T4-14's whole-TU exception) | Same-TU definition is what lets the compiler inline the stack today |
|
||||
| T4-28 | Warning to Q-W2: audition/preview stays a direct call-through across the 8-TU split; per-mouse-move work stays plain free-function calls | — | Record as a guardrail in the (reshaped) Q-W2 brief — reaffirms the plan's own guardrail; the two added TUs introduce no new risk | — |
|
||||
| T4-29 | Warning to the processor split: `process()` + per-block helpers stay one TU; the atomic-pointer-swap pattern must not gain a virtual seam | — | Record as a guardrail in the Q-W2v brief | — |
|
||||
| T4-30 | No other gratuitous indirection found (verified: only extension-side `virtual` is T4-26; VST virtuals are SDK-mandated; layered pure→shell pairs are the discipline, all direct calls) | — | Clean verification — see §5 | — |
|
||||
|
||||
### 2.5 Cross-track dedupes and reconciliations
|
||||
|
||||
1. **T2-05 ≡ T4-21 (+ T4-22 rider) — the rect zoo. Reconciled into ONE disposition: fix-now,
|
||||
ride Q-W1.** Both tracks found the same duplication (19 XYWH structs + inline predicates;
|
||||
T4-21 adds the VST side's second LTRB grammar) and both prescribe the same mechanism — one
|
||||
concrete `ui::Rect` + `contains()` with per-role aliases, explicitly **not** a template
|
||||
(T4-21's ruling). The only divergence was the wave label: T2-05 said "Q-W2 (the ui/
|
||||
relocation wave)", but in the plan the relocation wave — and the settled `ui::` rect
|
||||
unification (PLAN §Q-W1, "shared pure-UI rect types … get one `ui::` owner") — is **Q-W1**.
|
||||
T2-05's own rationale ("same-moment-as-relocation") therefore points at Q-W1; reconciled
|
||||
there. T4-22's hit-test template stays a deferred opportunistic rider behind it.
|
||||
2. **T3-07 → T1 — the 512-frame correlation cap. Resolved: document-and-defer.** T3 handed the
|
||||
"is 512 the right number" quality call to Track 1. Track 1's verdict on the correlation
|
||||
search's bounds (T1-06) is document-and-defer — record the alignment limits as the engine's
|
||||
stated operating range; "widening either costs splice-burst CPU linearly." Note for accuracy:
|
||||
T1-06 adjudicates the coarse-step and `maxLag` bounds specifically and does not name the
|
||||
512-frame `corrFrames_` cap; the resolution here rests on T1's general verdict (bounded
|
||||
limits recorded, no change recommended) applied to the same search-cost family. If triage
|
||||
wants the 512 value separately adjudicated, that is a residual question — flagged rather than
|
||||
silently absorbed.
|
||||
3. **T2-07 ≡ T4-13 — the ComponentState codec split. One disposition: Q-W2v's
|
||||
`component_state_io`.** T2-07's complaint (extension links the whole voice engine to share
|
||||
the preset serializer) is *solved by* T4-13's proposed split; T2-07's link-weight rationale
|
||||
rides the T4-13 seam. T4-14's `zone_params.h` header split completes the extension-side
|
||||
decoupling. T2 had provisionally pointed at "Q-W1/Q-W2 module-homing"; with Q-W2v now
|
||||
proposed as the wave that opens `sample_map`, that is the owning wave.
|
||||
4. **T2-06 ≡ T4-11's `editor_layout` — the stranded editor layout math.** T2-06's
|
||||
document-and-defer explicitly demanded a *named* downstream reshape; Q-W2v's `editor_layout`
|
||||
TU (T4-11, marked "pure candidate") is that reshape. One disposition: assigned to Q-W2v, with
|
||||
the hoist targeting the existing pure homes (`editor_geometry` is T2's named natural owner).
|
||||
T2-06's scope note (the §2 god-module catalogue missed the VST side) is also the evidence
|
||||
base for Q-W2v existing at all.
|
||||
5. **T4-20 / T2-03 / T2-04 — related but distinct dedup family, three separate dispositions.**
|
||||
All three converge on shared `core/` utility homes but touch disjoint code: T4-20 (LE byte
|
||||
codec ×5) lands with `component_state_io` in Q-W2v; T2-03 (`readFileBytes` ×5) lands in
|
||||
Q-W1's utility home; T2-04 (ext-state grow-loop ×3, generalizing `bridge_marshal`) lands in
|
||||
Q-W5 with the persist split. Marked here so triage sees them as one family and no wave
|
||||
assumes another already covered its slice.
|
||||
6. **T2-08 / T4-23 / T4-10 — WAV/RIFF consolidation. Genuine track disagreement on the moment;
|
||||
surfaced as §4e** rather than silently picked. T2-08 defers to "the reorg wave that
|
||||
relocates `wav_trim`" (Q-W1's relocation); T4-23 assigns to "the wave that opens
|
||||
`ingest.cpp`" — and T4-10 assigns the ingest split to "whichever wave lands the WAV
|
||||
consolidation," which is circular: **no current wave opens `ingest.cpp`.**
|
||||
7. **T1-11 — capture-tag collision.** T1 offered it to Track 2 ("Track 2 may claim it");
|
||||
Track 2 did not. It remains a single T1 finding, routed per T1's own suggestion to Q-W3.
|
||||
8. **Cross-track interaction on the DSP/VST fix-nows.** T1 and T3 both justified
|
||||
fix-now-in-Q-W0 partly by "no downstream wave opens these files" — written before T4
|
||||
proposed Q-W2v, which *does* open `reasampler_processor.cpp` / `reasampler_editor.cpp` /
|
||||
`sampler_core.h`. The recommendation stands unchanged: Q-W2v is a behavior-preserving
|
||||
mechanical-split wave, and folding behavior-changing DSP/domain fixes into it would break the
|
||||
wave discipline (CTest-green mechanical moves, no logic change). Fix-now items stay in Q-W0;
|
||||
noted so the rationale reads correctly against the reshaped plan.
|
||||
|
||||
---
|
||||
|
||||
## 3. Plan reshape — what Q-W0 proposes for Q-W1..Q-W6
|
||||
|
||||
The concrete deltas to the PLAN §Phase Q wave graph. Every wave is named; "no change"
|
||||
confirmations included deliberately.
|
||||
|
||||
**Q-W0 (this wave, post sign-off) — remediations before close:** T2-01(a) provenance cursor
|
||||
hardening backport; T3-01 gain-ramp seconds; T3-03 fade-ceiling seconds (or its stated fallback);
|
||||
T1-07 comment fix; pending §4b — T1-01 linked-lag stereo fix and T1-03 prime bound, with T1-09
|
||||
riding any `sampler_core` edit. Each behavior-touching remediation lands with its module's CTest
|
||||
target green per the Q-W0 verify contract.
|
||||
|
||||
**Q-W1 (safe opener — scope grows):**
|
||||
- Add `tail_control` to the `core/json` consumer list (T2-02) — the wave's "one JSON path" goal
|
||||
is not met without it. (T4-25 confirms the original 4× scope is otherwise accurate.)
|
||||
- Add the shared `readFileBytes` pure helper to the `core/` utility home (T2-03).
|
||||
- Rect unification: one concrete `ui::Rect` + `contains()` + per-role aliases, including
|
||||
retiring the XYWH-vs-LTRB fork and folding in `editor_geometry`'s `Rect` (T2-05 ≡ T4-21);
|
||||
`clamp01` dedup rider (T4-24); `hitIndex` template only as an opportunistic follow-on (T4-22).
|
||||
- Structural collapse of the wire `Cursor` family into one shared `wire` codec module beside
|
||||
`core/json`, consumed by `provenance` / `assignment_request` / `sample_usage` /
|
||||
`parseBankGeneration` (T2-01(b)).
|
||||
- `slot_map` extraction rider on the `bank_book` JSON rewire (T4-05); `view_mode_model` planner
|
||||
split rider (T4-06, optional per T4); `bank_model` unchanged-as-planned (T4-07); `view_lanes`
|
||||
split only if relocation touches `view.cpp` anyway (T4-09).
|
||||
- Relocation scope grows to include the ~20 clean VST pure libs, under whichever placement shape
|
||||
§4a settles (T4 §1.5, T4-18/T4-19).
|
||||
|
||||
**Q-W2 (bank_panel split — 6→8 seams):** the wave brief must name **eight** TUs — the planned
|
||||
six plus `panel_layout` and `panel_drag` (T4-01) — or two of its TUs ship >600 on day one.
|
||||
Guardrails reaffirmed: audition direct call-through; per-mouse-move work stays plain free-function
|
||||
calls (T4-28).
|
||||
|
||||
**Q-W2v (NEW — VST god-module wave; the T2-06/T4 §1.5 scope gap made structural):**
|
||||
- `reasampler_editor.cpp` → eight TUs: session / controls / **layout (pure-candidate hoist into
|
||||
the existing pure homes — this discharges T2-06)** / paint_sample / paint_browse_zone /
|
||||
input_sample / input_browse_zone / platform (T4-11).
|
||||
- `reasampler_processor.cpp` → three TUs: `processor_state` / `processor_reload` /
|
||||
lifecycle+`process()` kept whole (T4-12), with the T4-29 guardrail (no virtual seam on the
|
||||
atomic-swap pattern).
|
||||
- `sample_map` → `component_state_io` codec split + matching header split (T4-13 ≡ T2-07).
|
||||
- `sampler_core`: **TU stays whole at 968 — documented hot-path exception** recorded in the wave
|
||||
brief (T4-14, T4-27); header splits into `zone_params.h` + `sampler_core.h`.
|
||||
- The `core/wire/bytes.h` LE-codec template lands here with its biggest consumer (T4-20).
|
||||
- Rider: adopt the pure `ThumbnailKey` on the VST side while the editor is open (T2-10).
|
||||
- **Scheduling:** parallel-safe with Q-W2 (different artifact, zero file overlap) — T4 also
|
||||
offers serial-after-W5 as "Q-W7" if Daniel prefers serial waves (§4f).
|
||||
|
||||
**Q-W3 (main.cpp split — 3→4 hoists):** add **`capture_batch`** (batch family +
|
||||
`RunRecaptureFromSource` + the two selection guards) as a fourth TU so `capture_orchestrator`
|
||||
lands ~450 (T4-02). Also owned here: **delete `ICaptureBackend`** and correct the
|
||||
CLAUDE.md/CONTEXT description in the same commit (T4-26); the shared `stampCaptureSample`
|
||||
epilogue dedup (T2-09); the `capture_realtime_finalize` split riding the Q-9 naming rider
|
||||
(T4-08); the `makeUniqueTag` monotonic-counter fix (T1-11).
|
||||
|
||||
**Q-W4 (actions split): no change** — the planned seams still land sub-600 (T4-03).
|
||||
|
||||
**Q-W5 (persist split): seams unchanged** (T4-04); **add** the ext-state grow-loop dedup —
|
||||
generalize the retry policy into `bridge_marshal` (or its `core/` successor) and rewire all three
|
||||
loops, `usage_scan`'s prune-safety-adjacent copy included (T2-04).
|
||||
|
||||
**Q-W6 (registration table): no change**; T4-02 notes the `main.cpp` registration residue (~385)
|
||||
shrinks further under its table.
|
||||
|
||||
**Unassigned pending §4e:** the WAV/RIFF consolidation family (T2-08 / T4-23 / T4-10) — no
|
||||
current wave opens `ingest.cpp`; the owning moment is Daniel's call.
|
||||
|
||||
Updated dependency sketch:
|
||||
|
||||
```
|
||||
Q-W0 (this report + remediations) ── SUB-GATE: Daniel signs off every disposition (§4) ──
|
||||
▼
|
||||
Q-W1 (core/json + wire codec + relocation incl. VST pure libs + rect unification + riders)
|
||||
├─► Q-W2 (bank_panel split, 8 seams) ──► Q-W4 (actions; unchanged)
|
||||
├─► Q-W2v (NEW: VST god-modules — editor/processor/sample_map splits, sampler_core header,
|
||||
│ LE codec) [parallel-safe with Q-W2; serial "Q-W7" alternative — §4f]
|
||||
├─► Q-W3 (main split, 4 hoists; ICaptureBackend deletion + doc fix; stamp dedup; T1-11)
|
||||
│ └──► Q-W6 (registration table; unchanged)
|
||||
└─► Q-W5 (persist; seams unchanged; + ext-state-loop dedup) [best after Q-W4]
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## 4. Daniel's decision list (sign-off gate)
|
||||
|
||||
Each item: leading recommendation first, alternative second. Settled matters (Q-10, Q-11 framing,
|
||||
the clean bills) are deliberately absent.
|
||||
|
||||
- **(a) VST placement fork — T4-18 vs T4-19.** *Recommend T4-18:* integrate `src/vst/` into the
|
||||
one `core/`/`shell/` top split with `instrument/{engine,map,ui}` subsystem dirs — one rule
|
||||
("directory = may it touch a host type"), the artifact boundary is a link-graph fact the
|
||||
sources already straddle, CMake impact is path-edits only. *Alternative T4-19:* artifact-first
|
||||
subtree (`src/vst/core|shell` behind one SDK-gated `add_subdirectory`) — cleanest expression
|
||||
of the Windows-only gate, at the cost of two parallel `core/` trees and a subtree whose
|
||||
self-containment is cosmetic. Q-W1 executes whichever shape is chosen.
|
||||
- **(b) DSP bounded fixes in Q-W0 — T1-01 and T1-03.** *Recommend fix-now for T1-01* (linked
|
||||
L/R lag + splice schedule): High severity on the flagship stereo-Preserve path, standard
|
||||
stereo-SOLA practice, no technique change. *Alternative:* defer and record as the known
|
||||
stereo-Preserve limitation. *For T1-03* (prime bound + immediate `freezeTail()` on
|
||||
sub-window spans): T1's own framing — fix-now **if the short-one-shot case matters** to you
|
||||
(short drum one-shots are realistic content); else document-and-defer with the T1 note as the
|
||||
record. T1-09 (`declickR_` dead state) rides whichever `sampler_core` edit happens.
|
||||
- **(c) T2-01(a) provenance wire-cursor hardening backport in Q-W0.** *Recommend yes:* small,
|
||||
pure, closes a concrete robustness gap on persisted user-editable input, covered by existing
|
||||
`provenance_tests`. *Alternative:* wait for the Q-W1 wire-codec collapse to fix it
|
||||
structurally — leaves the gap open through the gate for no saving.
|
||||
- **(d) Env-constant fix-nows in Q-W0 — T3-01 and T3-03.** *Recommend fix-now for both:* each
|
||||
is trivial, isolated, and a live hardcoded-rate residue Daniel's standing ruling forbids;
|
||||
behavior-identical at the baked-in rates. *Alternative for T3-03 only* (T3's stated
|
||||
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).)
|
||||
- **(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
|
||||
the consolidation (one pure `wav_codec` owner: walker + layout + build + patch, absorbing
|
||||
T4-10's ingest extraction) as a named rider on **Q-W3** — the wave already opening the capture
|
||||
family (`capture_realtime` finalize, T4-08) — with Q-W1-relocation as the alternative moment
|
||||
if Daniel prefers T2's framing. Either way it must land somewhere named, or the
|
||||
dedup-by-hash/null-test maintenance surface stays quadruplicated.
|
||||
- **(f) Q-W2v scheduling.** *Recommend parallel with Q-W2* (different artifact, zero file
|
||||
overlap — T4 §1.5). *Alternative:* sequence it serially after W5 as "Q-W7" if you want serial
|
||||
waves throughout.
|
||||
|
||||
---
|
||||
|
||||
## 5. Clean bills — surfaces audited and found clean
|
||||
|
||||
Consolidated coverage evidence; details in the appendices' clean-surface sections.
|
||||
|
||||
**DSP (T1):** `peaks` (bin partition exact, overflow-guarded, per-channel no-fold), `master_gain`
|
||||
(taper math correct end to end), `velocity_curve` (Fritsch–Carlson monotonicity/no-overshoot
|
||||
claims hold) — fully clean. Clean with only the noted findings: `wav_trim` (T1-10 metadata note),
|
||||
offline capture path (T1-11; precision-invariant plumbing "disciplined"), realtime capture path
|
||||
(T1-11 + already-in-code DAW-verify flags), `sampler_core`'s voice/steal/mono/panic machinery
|
||||
(rate-free-seconds invariant honored; takeover blend mathematically sound), `pitch_shift`'s core
|
||||
machinery (safe-band geometry, clamps, correlation, fades, `freezeTail` continuity all audit
|
||||
sound; down-shift writer-lap unreachable above ≈ −109 st).
|
||||
|
||||
**Architecture (T2):** pure-module include hygiene across both trees (zero host-type includes in
|
||||
any claimed-pure module); `bank_sync`; `bridge_marshal`; the realtime record lifecycle (explicit
|
||||
enum state machine, not implicit); project-identity transitions; `usage_scan` (decisions
|
||||
delegated pure, depth-bounded recursion, protect-on-truncation); the three `catch (...)` sites
|
||||
(documented, narrow, non-swallowing); `FxBypassGuard` vs `view.cpp` park/restore (duplication of
|
||||
shape, not concept — correctly separate); path resolution (`resolveBankFile` is the single
|
||||
resolver both sides); WAV decode (one decoder); the draw layer (no parallel vocabulary on the VST
|
||||
side); interface cost (no hot-path virtual/`std::function` chains); boolean parameters (no
|
||||
smell). Also T2-11's headline: the ComponentState v1→v11 lift chain is **functionally sound** —
|
||||
the finding is shape, not correctness.
|
||||
|
||||
**Env-coupled constants (T3):** `sample_map` v5+ persistence (the reference implementation);
|
||||
ComponentState v6–v11 fields; Trigger fade/loop frames as source-file facts with the rate stored
|
||||
alongside; `trigger_seam`; `kPreserveWindowMs` (the model pattern); `pitch_shift` internal
|
||||
geometry; the tail system; `wav_trim`; `bank_model` persisted metadata; all ext-state wires; the
|
||||
envelope schematic's param-domain scaling; every timer surveyed; `peaks` / `waveform_view` /
|
||||
`master_gain` / `velocity_curve` / `keyboard_strip`. The persistence layer — the category's
|
||||
highest-stakes surface — is clean end to end.
|
||||
|
||||
**Sizing/indirection (T4):** borderline files needing no action: `capture.cpp` (549),
|
||||
`reasampler_editor.h`/`reasampler_processor.h` (shrink with their TU splits), `bank_book.h`,
|
||||
`pitch_shift.cpp` (371 — single responsibility, hot, leave), `persist.h`/`capture.h` (owned by
|
||||
Q-W6). T4-30: no gratuitous indirection anywhere beyond the dead `ICaptureBackend` — VST-side
|
||||
virtuals are SDK-mandated, and the layered pure→shell pairs are the load-bearing discipline, all
|
||||
direct calls. Plan-confirmations: Q-W4 and Q-W5 seams land as planned (T4-03/T4-04); Q-W1's JSON
|
||||
scope confirmed accurate modulo T2-02 (T4-25).
|
||||
Reference in New Issue
Block a user