Repository navigation
fix(core): a clip's volume lane stays smooth in preview while the page is busy - #6057
Conversation
vanceingalls
left a comment
There was a problem hiding this comment.
Reviewed at c9254348 (two commits on f5873b80). The diagnosis is right, and moving the lane onto a gain of its own fixes the choppiness. One writer per param is the correct invariant. But the new lane gain is only ever scheduled once, when the clip starts. Until this PR, a routed clip's lane was also applied per tick, which kept it in line with edits and speed changes during playback. Without that per tick path, those two cases now regress in preview.
What holds
- Both routes build the lane stage ahead of
gainNode, and FX feed into it (webAudioTransport.ts:436-437,:765-766). All three teardowns disconnect it (:711,:789,:890). routesElementis true only while an active source exists for the element, sotransportOwnsLaneand the lane node are tied to the same source. Hidden and mute state, and fades, still go throughgainNodeper tick.- The new tests do what they say:
setElementVolumeleaves the lane gain alone, and a routed clip's per tick volume excludes the lane.
Blocker: the clip's lane gain doesn't follow a rate change or an edit during playback
clipVolumeLaneStage (:119-130) books the envelope against the start-time frame and never touches it again. The FX lanes on the same clip are re-aimed in both cases, and setRate's own doc comment (:828-839) describes this exact bug for them.
- Preview speed change mid-play.
setRate(:841-858) re-aimssource.fx, notlaneNode. The runtime'sstopAll()and reschedule (init.ts,applyWebAudioRate) runs only when a bounded source is active.- So an unbounded clip keeps its 1× plan at 2× for the rest of the pass, for example a music bed with no
data-durationcarrying a volume lane: a fade-out lands at twice its clip time. - Before this PR, the per tick write resampled the lane at composition time on every frame, so preview stayed close to the render.
- So an unbounded clip keeps its 1× plan at 2× for the rest of the pass, for example a music bed with no
- A lane edit during playback. The FX handle's observer (
audioFx.ts:280-297) reschedules FX lanes on adata-automationedit. Nothing rescheduleslaneNode, so a moved point isn't heard until the next seek or play.- Deleting the lane mid-play is worse. The old envelope keeps running on
laneNode, and the per tick path now sendsdata-volumeas well. A deleted fade-out still fades to silence. - Adding a lane mid-play gives
laneGain !== nullwith nolaneNode, andtransportOwnsLaneis true. So the clip plays at unity and ignores both the new lane anddata-volume.
- Deleting the lane mid-play is worse. The old envelope keeps running on
- Repro at
c9254348: a throwaway test inwebAudioTransport.test.ts(not committed) scheduled an unbounded media-element clip with a 0 → 1 → 0 lane, then calledsetRate(2), then edited the lane. The lane gain'slinearRamp/setValue/cancelcall counts stayed2/1/1after both, andhasBoundedActiveSources()wasfalse.
Suggested fix
- Keep the lane's frame on the scheduled source. In
setRate, re-booklaneNodethe wayfx.setRatedoes: clear the lane, thenscheduleVolumeLaneat the live elapsed time and the new rate. - On a
data-automationedit, re-book the lane as well. The simplest route may be to let the FX handle's existing observer andsetRatecarry the volume lane as one more automated target. - In
media.ts:460, gate on whether the active source actually has a lane node, not onroutesElement. A lane added mid-play would then fall back to the per tick path instead of unity. - Tests:
setRate(2)on an unbounded source re-books the lane, and adata-automationedit re-books it, or clears it when the lane is removed.
The group fader that this PR mirrors has the same rate and edit gap. That predates this PR, but the same fix would cover it.
CI: still running at c9254348. Test (support) failed in verify-packed-manifests.test.mjs "installs a same-day dependency bump whose only matching version is an hour old" (bun minimum-release-age). It passes on the base, and this diff doesn't touch it, so it looks time-dependent and is worth a re-run.
Verdict: REQUEST CHANGES
Reasoning: Moving the lane to a gain that is only scheduled at clip start drops the per tick path that kept routed volume lanes correct across speed changes and live edits. The FX lanes in this file already guard against that bug class. A small re-booking on setRate and on edits keeps the smoothness fix without the regression.
— Via
Edit accuracy: accurate 271 (base branch 271), smooth 153 of thoseThe gate passes. Quarantined, measured but not gated (0) |
|
Thanks, the blocker was real. Fixed at Clip lane gain now follows the transport. The clip's FX handle books the lane gain as one more target (
Group fader. It rides the same handle (
Tests. A new Budget. Core unpacked goes to 3936240 (+290 over the previous head), reason in the body. CI. |
…e is busy A clip with a volume automation lane had two writers on one gain in preview: the Web Audio transport scheduled the lane as ramps on the clip's gain, and the per-tick media sync wrote the same lane's value onto that gain about 60 times a second. Each write landed whenever the main thread got to it, so a busy page stepped the gain and the clip sounded choppy. The lane now has its own gain node, like a group's fader beside its mute gain, and the per-tick volume of a routed clip no longer folds the lane in.
…tead of commenting it
… gain The preview fix gives a clip's volume lane its own gain node and tears it down with the clip, which takes core to 3935950 bytes unpacked, 283 over the budget main set at its exact size.
37b16ca to
bdcbdc4
Compare
vanceingalls
left a comment
There was a problem hiding this comment.
Re-reviewed at bdcbdc48. That is 37b16cac (Miguel's pin) rebased onto 2c1d9d50, with every changed file byte-identical to 37b16cac, and main touched none of them since f5873b80. This closes my change request at c9254348 (5612701093).
Since c9254348
- The lane follows the transport.
attachClipStagesbuildslaneNodefor every routed clip and handslaneNode.gaintoattachElementFxChainas the volume target, soscheduleForbooks the lane alongside the FX lanes. That puts it on every path that re-aims them:setRaterebases the frame at the old rate and replays at the new one;reanchorcovers seek and play;- the
data-automationobserver reschedules on an edit, or clears withcancelScheduledValues(0)when the lane is removed, solaneNodereturns to unity.
- The mid-play gate is right again.
laneNodenow always exists for a routed clip, so themedia.ts:460isWebAudioRoutedgate is correct. A lane added mid-play is booked by the observer instead of leaving the clip at unity. - The group fader uses the same path. It is booked through its own handle (
groupEl ? fader.gain : undefined).reanchorre-books from the live element only when it differs from the one the bus was built with. - The tests cover the regressions. The five new "volume lanes follow the transport mid-play" tests cover rate, edit, removal, mid-play add, and the group fader at a new rate and on an edit, all on an unbounded media-element clip. Dropping the
scheduleVolumeline inscheduleForturns those five and the two existing lane tests red. - Locally at
5ca2f88c: thewebAudioTransport,audioFxandmediatests pass 284/284, andtscis clean. 5ca2f88c..37b16cac(= this head's content) is comments only.audioAutomationVolume.ts:6now citesattachClipStages, and thestartBoundeddoc namesremaininginstead of the nonexistentclipSourceLen. That clears theCommentsfailure.
Nit (non-blocking)
- A group bus built before its
<hf-audio-group>exists gets the stub element and no fader target, so its lane follows onlyreanchor, not a rate change or an edit until the next seek or play. That predates this PR (it was reanchor-only before) and is rare.
CI: green at bdcbdc48 (and before that at 37b16cac). All 8 required checks pass (Build, Typecheck, Test, Test: runtime contract, Tests on windows-latest, Render on windows-latest, Semantic PR title, regression), and so does Comments. No check is red. GitHub still lists Test (runtime) as in progress, but its job (119880579937) finished every step with success at 14:28Z, including bun run --filter '@hyperframes/{cli,core,engine}' test. Its CI run 39795109640 concluded success, and the Test aggregate that waits on it went green at 14:31Z. The check-run status is stale.
Verdict: APPROVE
Reasoning: The clip's lane stays on its own gain, so the choppiness fix holds. Booking it through the FX handle puts it on the same rate, edit and reanchor paths as the FX lanes, which closes the regression I flagged. Tests pin each case, and a mutation shows they bite.
— Via
What
A clip with a volume automation lane now plays its lane on a gain node of its own in preview, so the per-tick volume write can no longer step it. Preview of such a clip stays smooth while the page is busy.
Why
In preview, a clip with a
data-automationvolume lane had two writers on oneGainNode:WebAudioTransportscheduled the lane as ramps on the clip'sgainNodewhen the clip started.syncRuntimeMediasampled the same lane every tick and sent it throughsetElementVolume, which wrotegainNode.gain.value, about 60 times a second.Setting
.valueon a param with scheduled ramps inserts a step at the context's current time. Each step carries the lane value from whenever the main thread ran the tick, so on a busy page (videos decoding, heavy UI) the gain jumps and the clip sounds choppy. The more the main thread lags, the larger the steps.Repro, headless Chrome on Linux, synthetic 1 kHz tone through highpass, gain, compressor and true-peak limiter with a 0 → 1 → 0 volume lane over 6 s, 48 kHz context, main thread blocked 30-60 ms every 100 ms:
The same clip without a lane measures the same before and after. On an idle page, before and after are both smooth (the steps are small when ticks are on time), but before still writes ~360 times per 6 s onto the scheduled param; after, none.
Related work
Refs the earlier change that fed the lane through the per-tick path so a scheduled envelope was not erased by the per-tick
data-volumewrite (comment inmedia.test.ts, "volume automation lane"). That fix stays for clips that sound throughel.volume; this PR gives routed clips the scheduled lane without a second writer.How
Invariant: one writer per gain param. A clip's volume lane owns a gain node of its own; the per-tick volume of a routed clip never folds the lane in. This mirrors the group bus in the same file, where the lane rides
faderand per-tick and hidden state ridemuteGain.webAudioTransport.ts: every routed clip, decoded or media-element, gets a lane gain of its own between its FX and itsgainNode(attachClipStages), disconnected at every teardown. It is unity while the clip has no lane.audioFx.ts: the clip's FX handle books that lane gain alongside the clip's other envelopes. The lane is re-aimed onsetRateandreanchor, re-booked whendata-automationchanges, cleared when the lane is removed, and booked when a lane is added mid-play. The group fader rides the same handle, so a group's lane follows rate changes and edits too.media.ts: while the transport routes the element, the per-tick volume no longer folds the lane in (it carries only what multiplies the lane, the clip fades). A routed element always has a lane gain the handle books, so routing and lane ownership are the same condition. Unrouted clips keep drivingel.volumefrom the lane as before.audioAutomationVolume.ts: doc comment updated; it said a volume lane is never scheduled on a gain.scripts/package-size-budgets.json: core's unpacked budget goes from 3935667 to 3936240 bytes (+573). The budget sits at main's exact size; the lane gain, its teardown and the handle booking it are what this fix adds to the runtime bundles.Render is unchanged: it already bakes lane × fades into the samples.
Test plan
webAudioTransport.test.ts, "volume lanes follow the transport mid-play"): rate change,data-automationedit, lane removal, lane added mid-play, and the group fader's rate change and edit. All 5 fail on the previous head and pass now; transport, media, FX and idle suites 300/300, 3 runs in a row; 24 neighbouring suites 467/467.webAudioTransport.test.ts(decoded and media-element routes keep the lane's gain untouched bysetElementVolume) andmedia.test.ts(a routed clip's per-tick volume excludes the lane). All 3 fail on main (expected 0.5 to be 1; lane value re-sent) and pass with the fix; both files 242/242, 3 runs in a row.tsc --noEmitclean;oxfmt --checkandoxlintclean.