Repository navigation
feat(audio): hear the FX chain while previewing - #3014
Conversation
800b82b to
626093d
Compare
f5bd5eb to
8f08d3b
Compare
626093d to
879687a
Compare
8f08d3b to
1745f66
Compare
879687a to
78ada29
Compare
1745f66 to
d5f62c8
Compare
78ada29 to
1031190
Compare
d5f62c8 to
15dfbda
Compare
terencecho
left a comment
There was a problem hiding this comment.
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:195callsattachElementFxChain(this._ctx, el, sourceNode, gainNode)beforegainNode.connect(this._masterGain). - Splice sits between the decoded source and the volume gain (matches offline order):
packages/core/src/runtime/audioFx.ts:81-84wiressource.connect(handle.input)andhandle.output.connect(destination). - Worklet-not-ready plays dry then hot-swaps:
runtime/audioFx.ts:66-96(verified viaWeakMap+WeakSetper-context registration inaudio/audioFxWorklets.ts:229-249). - Live re-parameterisation via
MutationObserveronHF_AUDIO_FX_ATTR:runtime/audioFx.ts:113-121. - Dispose on
stopAll():webAudioTransport.ts:290—source.fx?.dispose().
Foundation-cohort seed audit:
- LFO dispose leak (#3183 later fixes with
retireLfo) — PRESENT.chorusLfoandallpassPhaserinaudio/audioFxGraph.tscalllfo.stop()but the LFO is NOT in the[input, out, dl, depth, wet, dry, …].forEach(disconnect)list. This PR activates the leak by wiringbuildFxChaininto preview. Seed the retire helper here. - Worklet lifecycle (#3175) — PRESENT.
HfCompressor/HfLimiter/HfGate/HfBitcrushinaudio/audioFxWorklets.tsallreturn trueunconditionally inprocess().workletBuilder's dispose only callsnode.disconnect()— noport.postMessagestop signal, noprocess(): falseself-retire. Preview accumulates processors across clip lifetimes. - Chunked PCM (#3174) — N/A. Preview decodes via
AudioContext.decodeAudioData(arrayBuffer); the singledata:base64URL (audioFxWorklets.ts:243) carries the worklet JS source, not PCM. - 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.tsuniquely pins the splice (dry path must no longer exist), pins graceful degradation (empty / unreadable / unknown effect / no-getAttribute), pins dispose. Uses a fakeCtx/Node— cannot exercise worklet late-swap or the MutationObserver path.propertyPanelFxSection.test.tsxpins the drag-preview-vs-commit regression the PR body calls out (onChainPreview≥3,onChainChange0 during drag; 1 on pointerup). Good.- Missing pins: the worklet-not-ready → dry → late-swap branch, and the observer callback ignoring
handle.update()'sfalsereturn 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);dropshandle.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.tsonePoleBuilder'ssimple(node, () => {})returnsdispose: () => node.disconnect()— fine, but ties into the shape-signature carryingfrequencyfor 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/engineaudioFxRender.test.ts:95,spawnSync ffmpeg ENOENT): missingffmpegon 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)
`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.
15dfbda to
b0fb4de
Compare
`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.
`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.
8eac905 to
a636a07
Compare
b0fb4de to
e36ba93
Compare
`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.
a636a07 to
e3e3a41
Compare
e36ba93 to
fcf1a88
Compare
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.
e3e3a41 to
732fe3d
Compare
`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.
fcf1a88 to
d6bf7a9
Compare
d6bf7a9 to
c2846a9
Compare
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.
# 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.
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
AudioBufferand mutes the<audio>element to avoid doubling, socreateMediaElementSourcewould have processed a stream nothing is listeningto. Effects go between the decoded source and its gain — the same order the
offline render uses.
Two fixes found by using it:
attribute path on every
inputevent, so each pixel of a knob drag triggereda preview refresh, a composition reload, and an audio restart. Dragging now
writes live and commits once on release.
AudioWorkletNodethrows in that case and the whole chain is lost. Worse, amodule-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 growninto 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