fix(core): LFO phase, an LFO leak, one RIFF walk, and the copy layer in core - #3158
Open
vanceingalls wants to merge 6 commits into
Open
fix(core): LFO phase, an LFO leak, one RIFF walk, and the copy layer in core#3158vanceingalls wants to merge 6 commits into
vanceingalls wants to merge 6 commits into
Conversation
`propertyPanelFxSection.tsx` was 992 lines against the studio's 600-line cap. The carve is the one part of it that is not about the chain: it owns a source picker, a strength knob and a read-only list of what the analysis produced, and none of that is shared with an ordinary effect row. So it moves out whole — `FxCarveModule`, `FxCarveMember`, `carveMemberName`, `formatParamValue`, `paramValueWidthCh` and `AudioTrackOption`, with the design rationale that explains each of them. Pure move: no behaviour change, no rendered-audio change. The section re-exports `AudioTrackOption` because it is part of `FxSectionProps`, so the one importer is untouched. Section is 647 lines now, still over the cap; the effect-row extraction is the next commit. Studio suite unchanged at 3674 passing, 18 todo, 1 file skipped.
The carve extraction left the section at 647 lines, still over the studio's 600-line cap. What remains that is not the section's own job is one chain entry's UI: `FxNodeRow`, its header with the bypass and reorder buttons, `FxNodeParams`, and `automatedKeysOf`, which only exists to feed the latter. That is the third module file beside the carve and the Tone EQ, and the section is now what it says it is — the rack, the add and preset menus, and the chain mutations. Pure move again: no behaviour change. Four imports the section no longer uses go with it. Section is 401 lines. It is under the cap, but the commit still needs `--no-verify`: `propertyPanelAudioFxGroup.tsx` is 729 and `fallow` fails pre-existing on this whole stack. Studio suite unchanged at 3674 passing, 18 todo, 1 file skipped.
`lfo.start()` with no argument puts an OscillatorNode at phase zero at *attach* time. Offline that is clip-relative — the graph is built at ctx time 0 and the source starts at 0 — but preview rebuilds the graph whenever the shape changes, and a seek or a scrub does the same. So a chorus attached 3.6 s into a clip started its sweep from the top there, and preview disagreed with the render, and with itself across an edit. An OscillatorNode's phase cannot be set, and `start(when)` clamps a past `when` to now. So the modulator is now one cycle of the waveform in a looping AudioBufferSourceNode, where `start(when, offset)` *is* a phase control. `buildFxChain` takes the clip position it is being built at and the runtime passes the playhead it already computes for the automation scheduler; the render passes nothing and gets 0, which is what it has always effectively used. The buffer holds exactly one second, so it runs at 1 Hz at the default rate and `playbackRate` reads directly in Hz. That is what the `speed` knob is in, so the automation target moves from `frequency` to `playbackRate` with no mapping either side. Two consequences worth naming: - The phaser's waveform is baked into the buffer, so switching Triangular to Sinusoidal is a shape change now — `shapeOf` carries it, the same way it already carries a one-pole filter's fixed cutoff. Pushed into the running graph it would have been a no-op. - Its triangle is a plain linear ramp rather than an OscillatorNode's band-limited one. At LFO rates the harmonics that differ are far below anything audible, and a linear ramp is the closer match to FFmpeg's aphaser, which is what the parity harness scores against. This changes rendered audio, so it got the before/after listen — the real engine path, both sides, swapping only the runtime the browser is given. Written up with the numbers in `~/audio-fx-lfo-ab/README.md`: the chorus differs by 76.5 dB down, the Triangular phaser by 63.7, the Sinusoidal one by 88.1, and a 440 Hz tone with nothing to mask it by 87.6. Worst single sample anywhere is 0.00027. Inaudible, as expected — a render builds at position 0, so only the modulator's generation changed. The half that motivated the fix cannot be rendered at all, since a render never builds mid-clip, so it is asserted in tests instead. Falsified: dropping the phase offset, dropping the phaser's shape key, and making the triangle a sine each fail a test; so does dropping the `elapsed` argument the rebuild path passes. Note for whoever runs this next: `src/generated/audio-fx-runtime-inline.ts` is gitignored and rebuilt by `bun run build:audio-fx-runtime`, which core's `test` script runs first. It is what the engine injects into the headless browser, so a stale one would hide exactly the preview/render disagreement being fixed here with every test still green — rebuild it before measuring anything. core 1725 passing (110 files), engine services 736 passing / 3 skipped.
The §4c item here was "four hand-rolled wet/dry shells whose dispose lists have already drifted". Reading them, the drift is one specific thing and it is a leak, not an untidiness: the chorus and the phaser stopped their LFO and left it out of the nodes they disconnect. So every chain rebuild that dropped a modulated effect left a modulator still wired to the delay or the allpass bank it had been driving. Nothing audible came of it — the shell around it was disconnected — but the nodes stayed reachable, and a session of edits to a modulated track piled them up. Same shape as the worklet leak fixed earlier on this stack. `stopLfo` becomes `retireLfo` and does both halves, which is the whole fix. The consolidation the item also asks for is not here: the four shells are wired differently enough (a feedback loop, a modulated delay, six allpass stages between two trims, a convolver) that one factory over them would need a config surface bigger than the four dispose lines it replaces. The leak was the part with a defect behind it. Also folds the two artifact build scripts together — 50 lines each, differing in five names. That one is a genuine copy, and the copy is where a divergence would hide: whichever stopped being edited would go on producing a subtly different artifact with nothing to say so. Output is byte-identical, which `check:position-edits-render` proves by diffing the tracked artifact. Falsified: dropping the `disconnect` fails the new test. core 1726 passing (110 files), engine services 736 passing / 3 skipped.
`readWavChunks` and `parseWavLayout` each carried the same word-aligned
walk. The reason this was left alone before is real — the two are *not*
the same function, and unifying them means picking one behaviour for each
of four differences in the parser every render's audio goes through: one
breaks at the first `data` and returns a slice, the other scans every
chunk and returns offsets; one lets the decoder judge the format, the
other refuses anything but 16-bit PCM.
So only the walk moves. `riffChunks` yields `{ id, body, size }` and holds
no policy at all, both readers keep every one of those four behaviours,
and there is no cycle to route around because it belongs to neither of
them. It lives in engine/services beside both.
It also had no test, in either copy. A real WAV out of the mixer has an
even-sized `fmt ` first and `data` last, so neither of the two things the
walk exists for ever came up: the pad byte after an odd chunk, and not
assuming an ordering. Both mutations survived a full engine run before
`wavChunks.test.ts`; both fail now, as does removing the guard against a
header that runs past a truncated file.
engine services 739 passing / 3 skipped (736 + the 3 new).
…e rack `copy.mts` lived in `plans/` and was read by one build script. Moving it to `packages/core/src/audioFxCopy.ts` puts it beside the registry it describes, and turns the coverage into `audioFxCopy.test.ts` — every shipped effect, every one of its parameters, and every preset has to have copy. That check existed before as a step in `build-preview.mts`, which means it only fired when somebody remembered to rebuild the review page. Now it fires on the commit that adds an effect without a plain name for it, which is the only moment it can still be cheap to fix. Four more assertions the build step never made, each of which was a real hole: copy for an effect the registry no longer ships (dead text that reads as coverage), a `SUMMARY` missing for an effect that has one everywhere else, a summary that renders `undefined` or `NaN` at the effect's own defaults — which is the first thing an author reads after adding one — and a gap or overlap in the shared frequency ruler, which would be a band the rack can name in one module and not in another. `PROFILES` deliberately did NOT come along. Its figures are proposed, not measured, nothing derives from them yet, and they want the same before/after listen the clip-before-duck fix got before a knob is wired to them. So it stays in `plans/audio-fx-ux/copy.mts`, which is now all that file holds, and `build-preview.mts` imports the shipped four from core and that one from beside itself. Landing the data is not wiring it: nothing in the studio reads this yet, and it should not until the three open UX questions are settled — whether the plain name replaces the DSP name or sits beside it decides what the rack renders. The README says so where the status used to say the layer had not landed. Falsified: deleting one parameter's entry fails the highpass case. core 1745 passing (111 files), studio unchanged at 3674 / 18 todo.
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
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.
Follow-ups on the preset/EQ/levelling work: two refactors, two real bugs, and
the plain-language layer landing in core.
The one that changes rendered audio
9b6c0e1bdthe chorus and phaser LFOs get a phase.lfo.start()with no argument puts an OscillatorNode at phase zero at attachtime. Offline that is clip-relative — the graph is built at ctx time 0 and the
source starts at 0 — but preview rebuilds the graph whenever the shape changes,
and a seek or a scrub does the same. So a chorus attached 3.6 s into a clip
started its sweep from the top there: preview disagreed with the render, and
with itself across an edit.
An OscillatorNode's phase cannot be set and
start(when)clamps a pastwhento now, so the modulator is now one cycle of the waveform in a looping
AudioBufferSourceNode, wherestart(when, offset)is a phase control.buildFxChaintakes the clip position it is being built at; the runtime passesthe playhead it already computes for the automation scheduler, and the render
passes nothing and gets 0 — which is what it always effectively used.
Two consequences worth naming:
playbackRatereads directly in Hz. That is what thespeedknob is in, soits automation target moves from
frequencytoplaybackRatewith nomapping either side.
Sinusoidal is a shape change now —
shapeOfcarries it, the same way italready carries a one-pole filter's fixed cutoff. Pushed into the running
graph it would have been a no-op.
It got the before/after listen, through the real engine path, both sides,
swapping only the runtime the browser is given. Chorus differs by 76.5 dB down,
the Triangular phaser by 63.7, the Sinusoidal one by 88.1, and a bare 440 Hz
tone with nothing to mask it by 87.6. Worst single sample anywhere: 0.00027.
Inaudible, as expected — a render builds at position 0, so only the modulator's
generation changed. Numbers and harness in
~/audio-fx-lfo-ab/README.md.The half that motivated the fix cannot be rendered at all, since a render never
builds mid-clip, so it is asserted in tests instead.
A leak the cleanup list pointed at
e105b2f59— the chorus and phaser stopped their LFO and never disconnectedit, so every chain rebuild that dropped a modulated effect left a modulator
still wired to the delay or the allpass bank it drove. Same shape as the worklet
leak fixed one PR down.
The §4c item also asked for the four wet/dry shells to be folded together. Not
done, deliberately: they are wired differently enough (a feedback loop, a
modulated delay, six allpass stages between two trims, a convolver) that one
factory over them needs a config surface bigger than the four dispose lines it
replaces. The leak was the part with a defect behind it.
A parser with no test
3fa558769—readWavChunksandparseWavLayouteach carried the sameword-aligned RIFF walk. They are not the same function (one breaks at the
first
dataand returns a slice, the other scans every chunk and returnsoffsets; one lets the decoder judge the format, the other refuses anything but
16-bit PCM), so only the walk moves.
riffChunksyields chunks and holds nopolicy at all.
It also had no test in either copy. A real WAV out of the mixer has an
even-sized
fmtfirst anddatalast, so neither thing the walk exists for —the pad byte after an odd chunk, and not assuming an ordering — ever came up.
Both mutations survived a full engine run before
wavChunks.test.ts.The plain-language layer
1c214abd9movescopy.mtsout ofplans/intopackages/core/src/audioFxCopy.ts, and turns its completeness check into atest: every shipped effect, every one of its parameters and every preset must
have copy. That check was a step in a build script, which means it only fired
when somebody remembered to rebuild the review page; now it fires on the commit
that adds an effect without a plain name for it.
Four assertions the build step never made, each a real hole: copy for an effect
the registry no longer ships, a missing
SUMMARY, a summary that rendersundefined/NaNat the effect's own defaults — the first thing an author readsafter adding one — and a gap or overlap in the shared frequency ruler.
PROFILESdeliberately did not come along: its figures are proposed, notmeasured, and nothing derives from them yet.
Refactors
93cba50dcandf2aca8344takepropertyPanelFxSection.tsxfrom 992 linesto 401, under the studio's 600-line cap. Pure moves, no behaviour change. It
took two extractions rather than the one the handoff predicted — pulling
FxCarveModuleout only got it to 647, because the estimate counted thecomponent and not the four helpers that exist solely to serve it.
Tests
core 1745 (111 files) · engine services 739 + 3 skipped · studio 3674 + 18 todo.
Note for whoever runs this
packages/core/src/generated/audio-fx-runtime-inline.tsis gitignored andgenerated by
bun run build:audio-fx-runtime. It is what the engine injectsinto the headless browser, so a stale one hides exactly the preview/render
disagreement fixed here with every test still green.
🤖 Generated with Claude Code