From 0518a100333799deb5bc1d9f46c806795b516883 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Sat, 8 Aug 2026 12:25:42 -0700 Subject: [PATCH] docs(plans): record the second session's ten fixes and what is left MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All four of section 4a and six of 4b are done. Adds section 3b with the commits and the three judgement calls behind them: 4a-1 changes rendered audio and its before/after pair is written but unheard; 4a-4's striding was measured, found to move the carve's band selection, and rejected; 4a-2's size ceiling has no test because reproducing it needs a 256 MB payload. Rewrites 4b around what remains — the chorus/phaser LFO phase, which needs a buffer-source rework and its own listen, and the per-element browser setup — and records why the over-broad media.ts health ignore was left alone. Refreshes the baselines (studio 3662, core 1669, engine 736) and adds producer, which was never in the set: 534 passing plus 50 pre-existing collect failures from files importing bun:test. Co-Authored-By: Claude Opus 5 (1M context) --- plans/audio-review-fixes-handoff.md | 172 +++++++++++++++++----------- 1 file changed, 104 insertions(+), 68 deletions(-) diff --git a/plans/audio-review-fixes-handoff.md b/plans/audio-review-fixes-handoff.md index 38182433fd..0015683e6c 100644 --- a/plans/audio-review-fixes-handoff.md +++ b/plans/audio-review-fixes-handoff.md @@ -4,9 +4,14 @@ Written 2026-08-08 so a fresh session can continue without re-deriving anything. Everything here was verified in this worktree unless it says otherwise. **Worktree:** `~/src/wt/hyperframes/webaudio-fx` -**Working branch:** `wa-19-review-fixes` — 11 commits, **local only, never pushed** +**Working branch:** `wa-19-review-fixes` — 21 commits, **local only, never pushed** **Branched from:** `wa-18-lane-stretch` @ `4bcc7e188` +> **Second session (2026-08-08 afternoon) added 10 commits.** All four of §4a is +> done, plus six of §4b. See §3b. Baselines moved: studio **3662**, core +> **1669**, engine **736** (src/services), producer **534** passing with 50 +> pre-existing collect failures (files importing `bun:test` under vitest). + --- ## 1. Where everything is @@ -14,7 +19,7 @@ Everything here was verified in this worktree unless it says otherwise. | Branch | State | | --- | --- | | `wa-18-lane-stretch` | **pushed**, matches remote. PR **#3058** open, ready for review, base `wa-17-lane-clipboard` | -| `wa-19-review-fixes` | 11 commits, **local only**. The review fixes. | +| `wa-19-review-fixes` | 21 commits, **local only**. The review fixes. | | `carve-reconcile-wip` | 1 commit (`7b57000a1`), **local only**. Working feature, but it breaks undo — see §5. | Every other `wa-*` branch is in sync with its remote. The handoff §6 warning about @@ -24,12 +29,17 @@ plain fast-forward. ### Test baselines (all green at `wa-19-review-fixes` HEAD) ``` -packages/studio 3661 passing, 18 todo, 1 file skipped -packages/core 1657 passing -packages/engine 733 passing, 3 skipped (src/services) +packages/studio 3662 passing, 18 todo, 1 file skipped +packages/core 1669 passing +packages/engine 736 passing, 3 skipped (src/services) +packages/producer 534 passing — plus 50 PRE-EXISTING collect failures ``` -`tsc`, `oxlint`, `oxfmt` clean across studio, core, engine, lint. +The producer collect failures are files importing `bun:test`, which `vitest run` +cannot load. Same 50 before and after this work; only the passing count moved +(533 → 534). Producer was never in the original baseline set, which is why. + +`tsc`, `oxlint`, `oxfmt` clean across studio, core, engine, producer, lint. --- @@ -56,7 +66,7 @@ upstack. --- -## 3. Fixed (11 commits on `wa-19-review-fixes`) +## 3. Fixed in the first session (11 commits) Each was verified against current code, fixed, given a test, and the test **falsified by deleting the fix**. Three ship without tests and say so in their @@ -88,72 +98,90 @@ Two judgement calls worth knowing: --- +## 3b. Fixed in the second session (10 more commits) + +Same method: verified against current code, fixed, tested, and every test +falsified by mutating the fix out. + +| Commit | What | +| --- | --- | +| `55f852a72` | §4a-1 FX output ducked before it is quantised, not after | +| `c9448aec0` | §4a-2 a track's PCM crosses the CDP wire in 8 MiB chunks | +| `5e166ad47` | §4a-3 `setRate` re-aims every FX automation lane | +| `71ca86456` | §4a-4 FFT scratch reused across carve windows | +| `85257571c` | §4b a failed worklet registration can be retried | +| `eb60133e3` | §4b a worklet processor retires on dispose | +| `eb0080dec` | §4b a blank FX parameter no longer reads as 0 | +| `b1faacb3a` | §4b the fatal audio failure gets classified | +| `7ecc99605` | §4b dead `ignoreExports` entry dropped | +| `fddd596e8` | §4b FX rows keyed by node id | + +Three judgement calls worth knowing: + +- **§4a-1 changes rendered audio** for every track with both a chain and a + lane. A before/after pair driving the real code path on real narration is at + `~/audio-fx-clip-ab/{before,after}-levelmatched.wav`, level-matched so the + only difference is what the clamp destroyed. **Nobody has listened to it + yet.** The numbers: the chain peaks at 1.346, the old order sheared 19,166 + samples (1.7% of the file) flat against ±1, and level-matched the two differ + by an error signal 26.3 dB down with a worst single-sample delta of 0.242. +- **§4a-4 striding was tried and rejected.** Capping Welch at N windows is 27x + faster on a 5-minute voiceover (427 ms → 15 ms) but *changes the carve*: at + strength 0.9 the selected band set moved (630 Hz → 160 Hz) and it did not + converge back to the full read even at 2048 windows. Only the allocation + hoist shipped; wall-clock is unchanged and the values are bit-identical. The + reason is recorded in the code so it is not re-attempted. +- **§4a-2's ceiling has no test.** Reproducing the 8.7-minute failure needs a + 256 MB payload. What is tested is chunk assembly, which is where chunking + actually goes wrong. + +`applyAudioFxChain` now returns `{ path, envelopeBaked }` rather than a bare +path — inferring the bake from `path !== inputWav` breaks on the two early +returns (empty chain, empty track). + ## 4. Remaining work -### 4a. The four still open from the top 15 - -**1. FX output clipped to ±1 before the volume lane ducks it.** -`packages/engine/src/services/audioFxRender.ts:~136` clamps and quantises to -int16 in `writeWav`; `audioMixer.ts:~936` then calls `applyVolumeEnvelopeToWav`, -which re-reads that file and multiplies. So a chain that overshoots full scale is -destructively clipped even when the envelope immediately pulls the track 12 dB -down — audible distortion in the render that preview does not have. - -*I started this one and stopped deliberately.* The fix is to apply the envelope -to the float samples inside `applyAudioFxChain` before `writeWav` quantises. -That means: -- hoisting `envelopeKeyframes` (currently computed AFTER the FX call, `audioMixer.ts:~933`) above `applyAudioFxChain` -- extending its options with the envelope + `trackStart` + base volume -- setting `bakedEnvelope = true` so the mixer skips the second pass -- extracting the incremental segment-cursor gain walk out of - `audioVolumeEnvelope.ts:~107-116` so both paths share one implementation -- keeping the ffmpeg-expression fallback intact for non-int16 WAVs - -It changes rendered audio for every track with both a chain and a lane, so it -wants its own session and a before/after listen. - -**2. Base64 PCM round-tripped whole through one `page.evaluate`.** -`audioFxRender.ts:~256`. ~184 MB per 3-minute stereo track across one CDP message -pair. Verified ceilings: puppeteer-core sets `maxPayload: 256 MB` and -`browserManager.ts` does not pass `pipe: true`, so stereo 48 kHz past **~8.7 min** -exceeds the cap; V8's max string length is a second wall at ~17.5 min. Both raise -`AudioFxRenderError`, fatal. Compounded by `processCompositionAudio`'s unbounded -`Promise.all` holding N payloads at once. Also `Array.from(u8.subarray(...))` -boxes each 32 KiB chunk for nothing — `apply` takes array-likes. - -**3. `setRate` does not reschedule FX automation.** -`webAudioTransport.ts:~263`. `attachElementFxChain` gets a frozen -`{scheduledAt, elapsed, rate}` and commits ramps to absolute context time; -`setRate` only assigns `playbackRate.value`. A lowpass sweeping over 10 clip- -seconds, switched to 2× mid-play, eats 20 s of material in 10 s of wall clock -while the sweep keeps its original schedule. The recovery path in -`runtime/init.ts` (`stopAll()` + reschedule) only fires when -`hasBoundedActiveSources()` is true — a project-level music bed with no -`data-duration` is unbounded, so it never recovers. Second consequence: -`timingNow()` advances `elapsed` with the stale rate, so any later chain edit -re-aims the envelope at the wrong clip position. - -**4. `powerSpectrum` allocates on the main thread.** -`audioCarve.ts:~100` — two `Float64Array(4096)` inside the per-hop loop. A -5-minute 48 kHz voiceover is ~461 MB of transient garbage and ~7030 FFTs in one -uninterruptible block, called synchronously from the panel. `re` is fully -overwritten each iteration and `im` only needs `.fill(0)`, so both hoist; Welch -converges after a few hundred frames, so striding the hops gives the same -spectrum for ~2% of the work. **Now copied into two more loops** — -`audioCarve.ts:~320` and `~558` — so each needs its own spectrum check. +### 4a. The four from the top 15 — ALL DONE, see §3b ### 4b. Confirmed but cut by the reviewer's 15-cap -Rough severity order, file:line at the review tip: +Six of the eight are done (§3b). Two remain: -- `audioFxGraph.ts:254`/`:316` — chorus + phaser `lfo.start()` at build time, not clip-relative → preview ≠ render phase -- `audioFxWorklets.ts:247` — a rejected `addModule` is cached per-context forever while `readyContexts` is only written on success → all worklet effects permanently silent, no retry -- `audioFxGraph.ts:159` — worklet `dispose` only disconnects while every `process()` returns `true` → abandoned processors never released -- `audioFx.ts:754` — `Number(null)`/`Number("")` === 0 passes `Number.isFinite`, so a null param clamps to 0 instead of its declared default (compressor threshold 0 dB = never engages). `numberOrNull` at `audioAutomation.ts:141` already guards exactly this -- `audioStage.ts:89` — `audioFailures: undefined` flattens the typed failure the stage comment says it exists to attach -- `audioFxRender.ts:204` — per-element mkdtemp + page + `goto` + runtime re-injection -- `.fallowrc.jsonc:662` — file-scoped health ignore also exempts `syncRuntimeMedia`; `:157` `ignoreExports` names two symbols that do not exist -- `propertyPanelFxSection.tsx:287` — `key={type-index}` (PLAUSIBLE; likely inert now that the `latest` ref issue is fixed) +**LFO phase — the one real piece of work left here.** `audioFxGraph.ts:274` +and `:336`: chorus and phaser call `lfo.start()` with no argument, so the +oscillator's phase is zero at *attach* time. Offline that IS clip-relative (the +graph is built at ctx time 0 and the source starts at 0), but in preview a chain +attached mid-play — a seek, a scrub, or any structural edit, which rebuilds the +graph — starts the LFO at phase 0 wherever the playhead happens to be. So +preview and render disagree on modulation phase, and preview disagrees with +itself across an edit. + +*This is not a small fix.* An OscillatorNode's phase cannot be set, and +`start(when)` clamps a past `when` to now. The real fix is an +AudioBufferSourceNode looping one cycle, started with +`offset = (elapsed * speed) % cycle` — which gives phase control but moves the +`speed` automation target from `frequency` to `playbackRate` with a +`v => v / BASE` map, for both effects. It changes the DSP for every chorus and +phaser render, so like §4a-1 it wants a before/after listen. Deliberately not +attempted in a session that was already changing rendered audio elsewhere. + +**Per-element browser setup.** `audioFxRender.ts:~240`: every track pays its own +`mkdtemp` + `newPage` + `goto` + runtime injection. The browser lease is already +pooled across tracks; the page is not. Untouched — it is a throughput +optimisation with no correctness claim behind it, and `applyAudioFxChain` was +already rewritten twice in this session. + +Also left, deliberately: the file-scoped `health.ignore` on +`packages/core/src/runtime/media.ts` in `.fallowrc.jsonc`. Its comment says it +is for `refreshRuntimeMediaCache`, but being file-scoped it also exempts +`syncRuntimeMedia`, which IS what this stack changed. `health.ignore` takes file +paths only, so it cannot be narrowed in config; removing it wants a fallow run +to see what it then reports, and fallow already fails on this stack (§6.7). The +dead `ignoreExports` entry beside it *was* removed (`7ecc99605`) — both symbols +it named were gone. A sweep found five more stale `ignoreExports` symbols +belonging to other stacks (`DeleteConfirm`, `GsapKeyframeFormat`, +`GSAP_CSS_FALLBACK_BLOCKED_MESSAGE`, and two glob-style `file` entries that +match nothing); they are inert and out of scope here. ### 4c. Cleanup list @@ -237,7 +265,15 @@ real store-resync gap could be observed. here and on `wa-18` used `--no-verify`, as the previous 49 did. 8. **Two vitest runs racing on this machine** produce spurious 5 s timeouts in unrelated PropertyPanel tests. Do not chase them; re-run alone. -9. **The timeline store does not refresh after a delete** — the deleted clip stays +9. **`git add -A ` sweeps up more than the fix you meant to commit.** Two + separate worklet fixes landed in one commit that way and had to be split back + out. Stage the files the fix touches, or check `git show --stat` before + moving on. +10. **`applyVolumeEnvelopeToWav` is not the only envelope baker any more.** The + FX pass bakes it too and reports `envelopeBaked`; the mixer's own pass runs + only for tracks the FX pass did not handle. Anything that reasons about + "where does the volume lane get applied" has to look at both. +11. **The timeline store does not refresh after a delete** — the deleted clip stays rendered until a page reload. Independent of this work, but it gates when store-driven reconciliation fires.