fix(audio): eleven small correctness fixes across engine, core and the panel - #3173
Open
vanceingalls wants to merge 11 commits into
Open
fix(audio): eleven small correctness fixes across engine, core and the panel#3173vanceingalls wants to merge 11 commits into
vanceingalls wants to merge 11 commits into
Conversation
This was referenced Aug 10, 2026
…nder ffmpeg exits 0 and writes a structurally valid but EMPTY wav whenever a clip's trim starts past the end of its source — `-ss 10 -t 5` on a 2 s file. That track reached `new OfflineAudioContext(channels, 0, rate)`, which throws NotSupportedError, and the error travels past the mixer's per-track failure collector: one mis-set `data-media-start` on one clip with one enabled effect took the WHOLE render down with a RenderQualityError. An empty track has nothing to process, so applyAudioFxChain now hands the input back the way it already does for a chain with nothing enabled — before paying for a browser to decide it. The runtime stub guards the same case at the point the constructor would actually blow up, since it is reachable by any other caller of the injected script. The test falsifies against the real browser path: removing the guard reproduces `NotSupportedError: The number of frames provided (0) is less than the minimum bound (1)`.
The lease was acquired above the try, with `mkdtempSync` between the two — so a failure there (a full disk, a read-only tmpdir) returned without ever reaching the `finally` that releases it. A leaked pooled browser is worse than the error that caused it: a pool with no leases left hangs every later render instead of failing one. The temp dir is now taken first and the lease inside the try, with the release made conditional. The abort check moves above the acquire too, so an already-cancelled track no longer takes a browser out of the pool just to hand it straight back. No test: the fix is the ordering of two resource acquisitions, and the only way to observe it is to make mkdtempSync throw, which needs an injection seam that does not exist here and would be more risk than the three lines it guards.
…d wet/dry `in_gain` and `out_gain` trim the signal entering and leaving the phaser, and apply() drives them through inTrim/outTrim while pinning wet and dry to 1. The automation map aimed both lanes at wet.gain and dry.gain instead — so an envelope on either knob modulated a constant, the trim it was supposed to move stayed wherever apply() last left it, and any values-only edit re-ran apply() and slammed wet/dry back to 1 over the running ramp. Heard as: "fade the phaser to silence" ends at full dry signal. Wrong in preview and in the render, since both share this builder. The builder's own comment records that wiring these knobs to wet/dry was already found wrong once and moved to the trims; that refactor updated apply() and missed the map. The existing exposure test only asserted that SOME param was present under each key, which a mis-aimed target passes — the new test asserts by value, which is the only thing a lane actually cares about.
…lace Reordering two effects of the same type leaves `shapeOf` identical — it carries type, poles and the one-pole frequency, but no id — so the chain updates in place instead of rebuilding. That is right for the audio: the params move with the position. The ids did not move with them. They were captured once at build time, so after a reorder each handle named whichever effect used to sit in that slot, and scheduleChainAutomation built its byId map from those stale names: a lane on `fx.n2.frequency` drove the band that is now n1. This is the invariant HfAudioFxNode.id documents itself as protecting — "reordering the chain never re-points a lane at a different effect" — and it is easiest to hit on the all-peaking chains the voiceover carve produces. Preview only, since the render rebuilds from scratch, so the symptom is preview quietly ceasing to predict the render. Fixed by following the id, not by adding it to the shape: a same-type reorder genuinely does not need a rebuild, only a correct id map.
…der bakes it A lane's `t` is "seconds from the start of the clip" — HfAutomationPoint says so, and the render honours it: prepareAudioTrack cuts the wav with `-ss mediaStart`, so its t=0 IS the clip's start, and normaliseEnvelope subtracts trackStart. Preview fed `relTime` instead, which is MEDIA time: it carries mediaStart, scales by playbackRate and wraps on a loop. So the same envelope played somewhere else than it rendered. `data-media-start` past the last point held the final value from the first frame and never faded; `playbackRate: 2` ran the envelope at double speed in preview only; a looping clip restarted the envelope every lap in preview while the render baked it once. The FX lanes shipped in this same stack already use clip-local elapsed. There is one time base for automation, and this makes the volume lane use it.
…rsisting it
Two failures with one root: the row never tracked whether a gesture actually
edited anything.
The number field could not be typed into. It was bound to the committed value,
while every keystroke was clamped into range and written live — and a live
write does not refresh the prop, so React put the old number straight back.
Typing 5000 into a 20..20000 knob wrote 20 on the first keystroke and got no
further; the knob could only be dragged. Worse, `Number("") === 0` passes
Number.isFinite, so select-all + Delete before retyping instantly live-wrote the
parameter MINIMUM — 20 Hz on a cutoff, -40 dB on a gain. The field now holds
what is being typed as text, and an empty field is a state on the way to a
number rather than a number to clamp and persist.
`commit()` hangs off pointerup, keyup and blur, all reachable with no edit —
clicking a slider thumb without moving it, or tabbing through the field. It
unconditionally wrote `latest`, which was seeded at mount and only ever written
by an edit. So: open an EQ row, run the carve or press undo, then click the
slider and release without moving it, and the row silently re-persisted its
mount-time number over the change. It also fired a full persisting write —
source patch, selection resync, preview reload, audio restart — for a gesture
that moved nothing.
Each fix is falsified by deletion. A fourth guard (resyncing `latest` on every
inbound value) was dropped rather than kept: the edit flag subsumes it, and no
test could be made to fail without it.
The pattern allowed `[^;]{0,200}` between an element's selector and `volume:`,
and `[^;]` crosses `)`, `,` and newlines. A chained GSAP timeline has no
semicolon until the end of the whole chain, so
gsap.timeline().to("#bgm", { x: 10 }).to("#vo", { volume: 1 });
matched for `#bgm` — an element whose only automation is the lane. The warning
then told the author "the lane wins, the tween is ignored" about a tween that
is on a different track, and its fixHint invited them to delete the lane that
was actually working.
Refusing to cross the closing paren keeps the match inside the call the
selector belongs to. It costs a false negative when another value in the same
object is a call result, which is the safe direction for a rule that only
warns and, by its own comment, already only guesses. The `s` flag went with it:
inert, since the character class already matched newlines.
`src/generated/audio-fx-runtime-inline.ts` is what the engine injects into the headless page to do the actual DSP, and nothing kept it in step with its sources. Its sibling has `check:position-edits-render`, which rebuilds and then `git diff --exit-code`s the artifact — but that form needs the file tracked, and this one is gitignored. Measured tonight while fixing the zero-frame guard: editing the stub and re-running the engine's render tests injected the PREVIOUS bundle, so the fix appeared not to work and the old DSP ran with nothing reporting the drift. That is the exact preview/render divergence this stack exists to prevent, aimed at whoever is developing it. Making `test` regenerate it is the version of the gate that works without touching .gitignore: the artifact cannot be stale when the tests read it. Not covered: a fresh clone still cannot resolve `@hyperframes/core/audio-fx-runtime` from `packages/engine` until core has been built at least once. CI survives on build ordering. Tracking the artifact, or a pretest that builds core from the engine, are both bigger calls than this one.
…s stopped The `ended` listener spliced the source out of `_activeSources` and restored the element's muted flag, but never disposed the FX handle — and the splice is what made it unreachable, because `stopAll()` disposes by walking that same array. So every clip that finished naturally leaked its graph for the rest of the session. The leak is not just memory. Each stale handle keeps a MutationObserver bound to `data-fx-chain`/`data-automation`, so every later panel edit runs rebuild() against a dead source: a fresh graph per abandoned clip, including a ~576 KB reverb impulse and chorus/phaser oscillators that are started and never stopped. Play a five-clip composition three times and fifteen observers answer every subsequent knob turn. At this point in the stack `attachElementFxChain` has no chainless early return — it watches every audio clip, chain or not — so this leaked for all of them, not only the ones carrying effects. Disposal is skipped when the entry was already gone: `stopAll()` disposes its own, and `stop()` is what fires this event.
`analyse` captures the chain and the automation before its fetch and decode, then rewrites the whole `data-fx-chain` from that snapshot. Anything committed during those seconds — adding an effect, moving a knob — landed first and was silently discarded when the analysis returned. Only the Analyse control was gated on `analysing`; every other control in the rack stayed live throughout. FxSection already accepts `disabled` and threads it to every row; the carve panel simply never passed it. Refusing the edit for the duration is the honest answer — merging it into a measurement that did not account for it is not, and last-write-wins across an async gap is what this was. The test had to stub OfflineAudioContext as well as fetch: without a constructor, `analyse` returns before the decode and the window under test never opens, which is why the existing carve tests never saw this. Does not cover a write arriving from somewhere other than this panel — a timeline lane edit, or an undo — during the same gap. That needs the write to re-read rather than the reader to lock.
… them
The internal AbortController exists so a fatal FX error can cancel in-flight
ffmpeg before the finally-block deletes workDir. It reached the entry guard and
applyAudioFxChain, and nothing else: the trim, the video extract and the
download were all still handed the CALLER's signal. So `internalController
.abort()` cancelled nothing, and the `rmSync(workDir, { recursive: true })`
immediately after it ran while those children were still writing into the
directory.
Concretely: track A's FX render fails while track B is mid-trim, with no
external cancellation. B's ffmpeg keeps writing `${id}-trimmed.wav` into an
unlinked directory until it finishes. The comment above the controller
describes a mechanism that was never wired to the processes it names.
The download also had no signal at all, though downloadToTemp accepts one.
No test: the three changes are which variable is passed to a child process
helper, and observing the difference means driving real ffmpeg to the point of
cancellation. The old wiring's inertness is provable by reading — the caller's
signal is never aborted by this function — and the new wiring is the same three
calls with the controller the function already built.
vanceingalls
force-pushed
the
wa-19a-audio-fixes
branch
from
August 11, 2026 08:33
7a1adbc to
834dcb6
Compare
vanceingalls
changed the base branch from
wa-18-lane-stretch
to
wa-18i-row-fixes
August 11, 2026 08:40
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
First of four slices of the old #3156, cut to stay under 1000 lines.
Small, independent fixes with no audible change:
Each commit body carries its falsification note — the test was broken on purpose and confirmed to fail.
🤖 Generated with Claude Code