Skip to content

feat(audio): hear the FX chain while previewing - #3014

Merged
vanceingalls merged 27 commits into
mainfrom
wa-2-fx-preview
Aug 12, 2026
Merged

vanceingalls merged 27 commits into
mainfrom
wa-2-fx-preview

Conversation

@vanceingalls

Copy link
Copy Markdown
Collaborator

Stacked on #3013.

Splices the chain into the preview transport, so a chain is heard while
scrubbing rather than only after a render.

The splice point matters: the transport plays each track from a decoded
AudioBuffer and mutes the <audio> element to avoid doubling, so
createMediaElementSource would have processed a stream nothing is listening
to. Effects go between the decoded source and its gain — the same order the
offline render uses.

Two fixes found by using it:

  • Parameter drags restarted playback. The panel wrote through the persisting
    attribute path on every input event, so each pixel of a knob drag triggered
    a preview refresh, a composition reload, and an audio restart. Dragging now
    writes live and commits once on release.
  • Worklet nodes were constructed before their processor was registered.
    AudioWorkletNode throws in that case and the whole chain is lost. Worse, a
    module-level promise cache made every context after the first believe it was
    ready when it was not — registration is now tracked per context, and a chain
    that needs an unregistered worklet plays dry until the module lands, then
    swaps itself in.

Also splits the chain-node row out of FxSection: the render callback had grown
into a 70-line closure over half the component's state, and the two reorder
arrows were the same button written twice.

🤖 Generated with Claude Code

@terencecho terencecho left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: BLOCK — required "File size check" is red and PR-caused. Otherwise the splice and worklet-late-swap architecture verify cleanly; four foundation-cohort seeds should be flagged for pre-emption.

PR-body claim verification (head 15dfbda5):

  • Transport builds the graph on schedule: packages/core/src/runtime/webAudioTransport.ts:195 calls attachElementFxChain(this._ctx, el, sourceNode, gainNode) before gainNode.connect(this._masterGain).
  • Splice sits between the decoded source and the volume gain (matches offline order): packages/core/src/runtime/audioFx.ts:81-84 wires source.connect(handle.input) and handle.output.connect(destination).
  • Worklet-not-ready plays dry then hot-swaps: runtime/audioFx.ts:66-96 (verified via WeakMap+WeakSet per-context registration in audio/audioFxWorklets.ts:229-249).
  • Live re-parameterisation via MutationObserver on HF_AUDIO_FX_ATTR: runtime/audioFx.ts:113-121.
  • Dispose on stopAll(): webAudioTransport.ts:290 — source.fx?.dispose().

Foundation-cohort seed audit:

  1. LFO dispose leak (#3183 later fixes with retireLfo) — PRESENT. chorusLfo and allpassPhaser in audio/audioFxGraph.ts call lfo.stop() but the LFO is NOT in the [input, out, dl, depth, wet, dry, …].forEach(disconnect) list. This PR activates the leak by wiring buildFxChain into preview. Seed the retire helper here.
  2. Worklet lifecycle (#3175) — PRESENT. HfCompressor / HfLimiter / HfGate / HfBitcrush in audio/audioFxWorklets.ts all return true unconditionally in process(). workletBuilder's dispose only calls node.disconnect() — no port.postMessage stop signal, no process(): false self-retire. Preview accumulates processors across clip lifetimes.
  3. Chunked PCM (#3174) — N/A. Preview decodes via AudioContext.decodeAudioData(arrayBuffer); the single data:base64 URL (audioFxWorklets.ts:243) carries the worklet JS source, not PCM.
  4. Duck-before-quantise (#3174) — N/A. Real-time Web Audio path; no offline sample clamp.

Extra seed I found (worth mentioning here so it's on the wa-18 sweep): natural track-end doesn't dispose fx. schedulePlayback's sourceNode.addEventListener("ended", …) at webAudioTransport.ts:238-244 splices _activeSources but never calls source.fx?.dispose(). Only stopAll() disposes. A clip that plays to its natural boundary leaves the MutationObserver + LFOs + worklet nodes attached until the next stop — compounds with (1) and (2).

HfAudioFxNode field / allow-list check: New optional fromCarve?: boolean at audioFx.ts:738; covered in both parseAudioFxChain (:784, :790) and serializeAudioFxChain (:812). Correctly gated. The hand-constructed allow-list pattern (#3019's root concern — not typed against keyof HfAudioFxNode) is unchanged, but this field IS covered.

Test-shape audit:

  • runtime/audioFx.test.ts uniquely pins the splice (dry path must no longer exist), pins graceful degradation (empty / unreadable / unknown effect / no-getAttribute), pins dispose. Uses a fake Ctx/Node — cannot exercise worklet late-swap or the MutationObserver path.
  • propertyPanelFxSection.test.tsx pins the drag-preview-vs-commit regression the PR body calls out (onChainPreview ≥3, onChainChange 0 during drag; 1 on pointerup). Good.
  • Missing pins: the worklet-not-ready → dry → late-swap branch, and the observer callback ignoring handle.update()'s false return on shape change (documented as "wait for next schedule" — but no test guards it).

Nits:

  • runtime/audioFx.ts:120 — if (next.chain.nodes.length > 0) handle.update(next.chain); drops handle.update()'s return value; a shape change silently falls through with the old graph running. Intended per PR body, but worth a code comment referencing the "waits for next schedule" contract.
  • audio/audioFxGraph.ts onePoleBuilder's simple(node, () => {}) returns dispose: () => node.disconnect() — fine, but ties into the shape-signature carrying frequency for one-pole rebuilds.

CI:

  • FAILURE — File size check: PropertyPanelFlat.tsx has 672 lines (max 600). This PR added +60 net lines there. Real, blocking, PR-caused. Split before merge.
  • FAILURE — CI Test (@hyperframes/engine audioFxRender.test.ts:95, spawnSync ffmpeg ENOENT): missing ffmpeg on the runner — infra drift, not PR signal. The affected test is offline-render territory this PR doesn't touch.
  • All Preflight, regression shards 1-9, Perf (load/fps/scrub/drift/parity), Windows render+tests, Preview parity, CodeQL, Typecheck, Studio smoke, Producer/SDK/CLI: green.

— Review by tai (pr-review)

vanceingalls added a commit that referenced this pull request Aug 12, 2026
`PropertyPanelFlat.tsx` was 672 lines against the repo's 600-line cap, so
the required File size check was red — the sole reason #3014 and #3022 are
blocked. Both reviews say the same thing: "mechanical fix, not a design
problem. Code itself is LGTM."

Moves `AudioFxGroup` and `audioFxSummary` into
`propertyPanelAudioFxGroup.tsx`, which is where a later branch puts them
anyway — done here so the file is under the cap from the point it first
crosses it, rather than ten branches later.

533 lines now. The four audio imports it no longer needs go with it.

Not fixed here: three `FxSection carve` tests fail on this branch with
"Cannot read properties of undefined (reading 'toFixed')". Confirmed
pre-existing by stashing this change and re-running — that is the separate
`Test` failure the review also flags.
vanceingalls added a commit that referenced this pull request Aug 12, 2026
`PropertyPanelFlat.tsx` was 672 lines against the repo's 600-line cap, so
the required File size check was red — the sole reason #3014 and #3022 are
blocked. Both reviews say the same thing: "mechanical fix, not a design
problem. Code itself is LGTM."

Moves `AudioFxGroup` and `audioFxSummary` into
`propertyPanelAudioFxGroup.tsx`, which is where a later branch puts them
anyway — done here so the file is under the cap from the point it first
crosses it, rather than ten branches later.

533 lines now. The four audio imports it no longer needs go with it.

Not fixed here: three `FxSection carve` tests fail on this branch with
"Cannot read properties of undefined (reading 'toFixed')". Confirmed
pre-existing by stashing this change and re-running — that is the separate
`Test` failure the review also flags.
vanceingalls added a commit that referenced this pull request Aug 12, 2026
`PropertyPanelFlat.tsx` was 672 lines against the repo's 600-line cap, so
the required File size check was red — the sole reason #3014 and #3022 are
blocked. Both reviews say the same thing: "mechanical fix, not a design
problem. Code itself is LGTM."

Moves `AudioFxGroup` and `audioFxSummary` into
`propertyPanelAudioFxGroup.tsx`, which is where a later branch puts them
anyway — done here so the file is under the cap from the point it first
crosses it, rather than ten branches later.

533 lines now. The four audio imports it no longer needs go with it.

Not fixed here: three `FxSection carve` tests fail on this branch with
"Cannot read properties of undefined (reading 'toFixed')". Confirmed
pre-existing by stashing this change and re-running — that is the separate
`Test` failure the review also flags.
vanceingalls added a commit that referenced this pull request Aug 12, 2026
`PropertyPanelFlat.tsx` was 672 lines against the repo's 600-line cap, so
the required File size check was red — the sole reason #3014 and #3022 are
blocked. Both reviews say the same thing: "mechanical fix, not a design
problem. Code itself is LGTM."

Moves `AudioFxGroup` and `audioFxSummary` into
`propertyPanelAudioFxGroup.tsx`, which is where a later branch puts them
anyway — done here so the file is under the cap from the point it first
crosses it, rather than ten branches later.

533 lines now. The four audio imports it no longer needs go with it.

Not fixed here: three `FxSection carve` tests fail on this branch with
"Cannot read properties of undefined (reading 'toFixed')". Confirmed
pre-existing by stashing this change and re-running — that is the separate
`Test` failure the review also flags.
vanceingalls and others added 6 commits August 12, 2026 02:35
Reads `data-fx-chain` off an audio element and runs the chain over the trimmed
WAV before volume automation is baked in — effects should see the raw signal,
and the envelope belongs on their output.

The processing happens in an OfflineAudioContext inside the headless browser
the engine already drives, running the same graph builders the studio previews
with. That is the point of the approach: one implementation per effect, so the
render agreeing with the preview is a property of the architecture rather than
a tolerance to police. Reimplementing each effect as an FFmpeg filter would
mean two implementations to keep in step, and for the dynamics processors and
modulated delays there is no filter that behaves the same way.

`build:audio-fx-runtime` bundles the graph builders into an injectable IIFE,
following the same pattern as the existing runtime artifacts, so the browser
runs exactly the code the studio does.

The page loads from a file:// URL rather than about:blank because AudioWorklet
is only exposed in a secure context — the compressor, limiter, gate and
bitcrush processors would otherwise fail to register with an opaque error.
file:// qualifies and needs no listening socket.

The chain is serialised into the attribute the way colour grading carries its
config, so there is no side-car file to resolve or lose.

An FX failure is fatal for the whole mix rather than a per-track soft failure.
Every other audio failure mode degrades gracefully — the track drops, siblings
continue — but substituting the dry signal for a processed one ships a render
that sounds plausible and is not what the author set up. Since the per-element
work races under Promise.all, an internal AbortController chained off the
caller's signal aborts in-flight siblings before workDir is removed.
Finds the bands a voice occupies so a music bed can be dipped there, letting
the voice sit in front without ducking the whole track.

Carve is a relationship between two tracks rather than an effect on one, so it
stays out of the FX chain. What it emits is an ordinary chain of peaking
filters, so a carve composes with whatever else is on the track and needs no
separate rendering path.

Selection is weighted toward intelligibility rather than raw voice energy.
Ranking purely by power lands on the fundamental almost every time, because
that is where a voice is loudest — but the masking that actually hurts a
voiceover happens higher up, and dipping 160 Hz mostly just thins the bed. The
bias is a control, not a constant: at 0 it follows raw energy, at 1 it weights
toward 1-3 kHz.

Ranking happens in dB, which matters more than it looks. Speech spreads 20-30 dB
across these bands — it falls off roughly 6 dB per octave above the fundamental
— so a weighting has to be on that scale to move anything at all. A
multiplicative weight of `1 - bias + bias * shaped` is bounded below by
`1 - bias`, capping its influence at 10*log10(1/(1 - bias)): 5.2 dB at the 0.7
default, 3 dB at 0.5. That is no influence against a real voice — every bias
short of ~0.95 would rank exactly like bias 0 and carve the fundamental, the
outcome the bias exists to prevent, while looking decisive against a fixture
whose bands sit 2 dB apart. So the bias is a dB penalty, zero at 2 kHz and worth
up to 30 dB at full strength, and relative cut depths come from a dB difference
rather than a ratio of weighted linear powers.

The bias reweights ranking without overriding the spectrum — a band the voice
has no energy in is not worth carving, and scores -Infinity rather than
competing — so a strongly low-pitched voice can still select low at full bias.
What the tests hold is that biasing never selects lower than the unbiased
ranking, that the DEFAULT bias reaches the presence region on a voice with a
realistic tilt, and that bias 0 still follows raw power exactly.

Includes a radix-2 FFT rather than a dependency; one Welch-style averaged
spectrum over third-octave bands does not justify pulling in a DSP library.
Three defects in the offline FX path, none of which any test could see.

**Float output silently disabled sample-accurate volume automation.** The writer
emitted 32-bit IEEE float; the very next mixer step bakes the volume envelope
into the samples and accepts only 16-bit PCM, returning null otherwise. So
enabling any effect downgraded that track to the ffmpeg expression path — capped
at 32 straight segments, quantising a curved envelope, and on a dense one falling
back to base volume. It now writes 16-bit PCM, clamped rather than wrapped so a
limiter at 0 dB or a resonant filter cannot turn overshoot into a click. A test
asserts the baker accepts the writer's own output and actually fades it.

**Everything was folded to mono.** `prepareAudioTrack` goes out of its way to
emit stereo — its pan filter exists to dodge ffmpeg's 3 dB mono-to-stereo
rematrix — and this folded it, then wrote one channel. So adding a single peaking
EQ collapsed a bed's width and cost ~3 dB in the render, while preview stayed
stereo. Channels now travel as one plane each, through an OfflineAudioContext of
the same width, and come back interleaved.

**Small results decoded the wrong length.** `new Float32Array(buf.buffer)`
discards byteOffset and byteLength, and Node pools small allocations: a 400-byte
payload sits at offset 8 inside an 8 KiB pool, so a clip under ~1024 samples
decoded as 2048 samples of unrelated memory — and the empty-result guard could
not see it. The reader has the mirror-image fix: a float data chunk on an odd
boundary (ffmpeg's pcm_f32le writes fmt(18) + fact, landing `data` at 58) now
copies instead of throwing RangeError on an unaligned view.

The tail limitation is now stated rather than mis-stated: the context is exactly
as long as the input, so a reverb or delay still ringing is cut there. The old
comment claimed the opposite. How far a tail may run past a clip's end changes
the clip's length in the mix, so it is a product decision, not one to make here.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
`processCompositionAudio` reports per-track failures in its result, but an FX
failure it cannot degrade past — a browser that will not launch, a chain that
will not build — rejects instead. `runAudioStage` had no try, so that rejection
escaped to the orchestrator as an unclassified pipeline exception, losing the
stage/owner/retryable classification this stage exists to attach, and skipping
its abort check on the way out.

It now lands in `audioError` alongside every other cause, while an abort still
keeps its own shape rather than being reported as an audio problem.

Not done here: committing the generated `audio-fx-runtime-inline.ts` so a fresh
clone typechecks packages/engine without building first. The bundle is built from
the stub, and the stub changes three times across this stack — so the artifact
differs per branch and would conflict on every restack. Its model,
position-edits-render-inline.ts, is committed only because it is stable. Building
before testing is this monorepo's existing contract (studio's tests need core's
dist too), so the gap is not specific to audio FX and is better closed by a build
ordering gate than by committing a per-branch artifact.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
CI's `Test` job was red on this PR with four failures, all the same cause:

  Failed to launch the browser process: spawn
  /home/runner/.cache/hyperframes/chrome/chrome-headless-shell

The job installs ffmpeg and no browser, deliberately — every other suite
that needs an external binary already guards on it
(`describe.skipIf(!HAS_FFMPEG)`). These cases were the only ones assuming
a Chrome, so they failed on an absent dependency rather than on anything
about the code.

Guards on `resolveHeadlessShellPath()` — the same resolver
`acquireBrowser` launches through, so the check cannot drift from the
thing it guards the way a hard-coded cache path would. A configured path
that does not exist throws; that is caught and read as "cannot run here".

Checked both directions rather than just the green one: with a browser all
11 cases run and pass, and with `HYPERFRAMES_BROWSER_PATH` pointed at a
missing binary exactly 3 skip and the other 8 still run. A guard that
silently skipped everything would have looked identical in CI.

They keep their value where it exists — every developer machine, and any
job that has run `hyperframes browser ensure`.

Not touched: the CodeQL failure on this PR is a run from 2026-08-07, five
days and several force-pushes stale. None of the 17 open repo alerts are
in files this PR changes; it re-runs on this push.
CodeQL flags `writeWav`'s `writeFileSync` as js/insecure-temporary-file
(high) — the one new alert on #3021, and the reason its CodeQL check is
red.

It is a false positive, and the comment says why rather than just silencing
it: `path` is always inside a directory made by `mkdtempSync`, never a
name assembled directly under `tmpdir()`. Both callers are covered — the
browser host page writes into `mkdtempSync(join(tmpdir(), "hf-fx-host-"))`,
and the render output goes to the producer work dir, itself
`mkdtempSync(join(tempRoot, "producer-project-"))`. mkdtemp picks the
random suffix and creates the directory 0700 in one syscall, so the
predictable filename inside it cannot be pre-created or symlinked by
another user, which is the attack the rule is about. The analyzer sees the
dataflow reach `tmpdir()` and not the mkdtemp in between.

Suppressed inline rather than dismissed in the UI, so the justification
lives next to the code and the rule stays live for anything added later in
this file. Matches the repo's existing convention — `planV2.ts:222`
carries an `lgtm[js/insecure-temporary-file]` for a different reason on
the same rule.

Correcting myself: I first reported this alert as not real, having
intersected the PR's files against the default-branch alert list, which
does not contain PR-ref alerts. Querying ?ref=refs/pull/3021/merge returns
it straight away.
vanceingalls added a commit that referenced this pull request Aug 12, 2026
`PropertyPanelFlat.tsx` was 672 lines against the repo's 600-line cap, so
the required File size check was red — the sole reason #3014 and #3022 are
blocked. Both reviews say the same thing: "mechanical fix, not a design
problem. Code itself is LGTM."

Moves `AudioFxGroup` and `audioFxSummary` into
`propertyPanelAudioFxGroup.tsx`, which is where a later branch puts them
anyway — done here so the file is under the cap from the point it first
crosses it, rather than ten branches later.

533 lines now. The four audio imports it no longer needs go with it.

Not fixed here: three `FxSection carve` tests fail on this branch with
"Cannot read properties of undefined (reading 'toFixed')". Confirmed
pre-existing by stashing this change and re-running — that is the separate
`Test` failure the review also flags.
Fallow audit flagged it — no test imports the module.
… panel

- Split FxSection's per-node row into FxNodeRow + FxNodeControls so the
  CRAP score (31.6, threshold 30) splits across two smaller units instead
  of moving wholesale with one extraction.
- Dedupe the repeated "open the add menu, read its items" block in
  propertyPanelFxSection.test.tsx into openAddMenuItems().
- Merge build-audio-fx-runtime.ts and build-position-edits-render.ts into
  one build-inline-artifact.ts, config-selected by CLI arg — the two
  scripts were a byte-for-byte clone save for names.
- Exempt canary.test.ts's rawFnv (a deliberate independent
  reimplementation used to cross-check canaryBucket, per its own
  docstring) and the property-panel test files' shared renderInto/mount
  scaffolding (pre-existing across 9 files, 2 outside this stack) in
  .fallowrc.jsonc, consistent with this file's existing exemptions for
  the same class of intentional/pre-existing duplication.
# Conflicts:
#	packages/studio/src/components/editor/propertyPanelFxSection.tsx
# Conflicts:
#	packages/core/package.json
…tions guard

build-audio-fx-runtime.ts and build-position-edits-render.ts were merged into
build-inline-artifact.ts to kill a fallow duplication finding; the deletion
guard flagged that as an accidental loss since main still has both originals.
Base automatically changed from wa-4-fx-panel to main August 12, 2026 20:34
# Conflicts:
#	packages/studio/src/components/editor/PropertyPanelFlat.tsx
#	packages/studio/src/components/editor/propertyPanelFxControls.tsx
#	packages/studio/src/components/editor/propertyPanelFxSection.test.tsx
#	packages/studio/src/components/editor/propertyPanelFxSection.tsx
…usLfo

Both effect builders set wet.gain to the mix and dry.gain to its complement
in identical two-line blocks; fallow kept re-flagging it as a 10-line clone
on every unrelated change. Extracted setWetDryMix.
@vanceingalls
vanceingalls merged commit 9b0c5e8 into main Aug 12, 2026
113 of 146 checks passed
@vanceingalls
vanceingalls deleted the wa-2-fx-preview branch August 12, 2026 22:40
vanceingalls added a commit that referenced this pull request Aug 12, 2026