From ca086f40093a4c0f6212fdca19127608cfff3b1b Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Tue, 28 Jul 2026 16:50:38 -0400 Subject: [PATCH] =?UTF-8?q?docs(q-w0):=20T3=20env-coupled-constant=20audit?= =?UTF-8?q?=20=E2=80=94=207=20findings=20(2=20fix-now:=20gain-ramp=20step,?= =?UTF-8?q?=20fade-ceiling=20frames);=20persistence=20surfaces=20clean?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../audit-notes/q-w0-t3-env-constants.md | 220 ++++++++++++++++++ 1 file changed, 220 insertions(+) create mode 100644 docs/product/audit-notes/q-w0-t3-env-constants.md diff --git a/docs/product/audit-notes/q-w0-t3-env-constants.md b/docs/product/audit-notes/q-w0-t3-env-constants.md new file mode 100644 index 0000000..993aa15 --- /dev/null +++ b/docs/product/audit-notes/q-w0-t3-env-constants.md @@ -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.