fix(core): keep authored gain above unity off el.volume in the sandbox bridge - #3349
Conversation
…x bridge Raising the authoring ceiling to 12 dB made `data-volume` legal up to ~3.98, but the sandbox runtime's volume bridge assigned `clipVolume * volume` straight to `el.volume`, which the spec pins to [0,1] and which THROWS IndexSizeError outside it. Verified in Chrome and reproduced in the suite: the throw aborts the loop, so every media element after the boosted one keeps the volume it already had while the bridge's own state says otherwise. The element carries the legal part. The boost above unity belongs to Web Audio, which already receives it — the two tests here pin that half, since it is the reason clamping the element is safe rather than lossy: `syncRuntimeMedia` hands the transport the authored gain alongside the element's clamped one, and the per-element gain node keeps it. The fix was written when the ceiling was, and got stranded in a PR whose other work landed in pieces around it.
d250db7 to
4d43a89
Compare
terencecho
left a comment
There was a problem hiding this comment.
Fix is right and the tests pin it.
Bug is live: verified on main at packages/core/src/runtime/init.ts:3159 — the raw el.volume = clipVolume * volume inside the onSetVolume for loop throws IndexSizeError for data-volume > 1, aborting the loop and stranding every element after the boosted one. MAX_AUDIO_GAIN_DB = 12 → data-volume legal up to ~3.98, so the throw is one authored boost away.
Fix is scoped correctly: clamp only the DOM assignment. The author boost above unity was already preserved on the Web Audio path since #3328 — media.ts already has the 3-arg onElementVolume?: (el, effectiveVolume, authorVolume) signature and init.ts:2042 already passes authorVolume into webAudio.setElementVolume. So this PR's two "transport pinning" tests (media.test.ts + webAudioTransport.test.ts) codify existing-but-untested behavior, and the description's "clamping the element is safe rather than lossy" claim holds.
Discriminating test: the init.test.ts case seeds boosted=0.2 / quiet=0.1 sentinels before dispatching set-volume: 1, then asserts both boosted→1 and quiet→0.5. Without the clamp, the boosted assignment throws and quiet.volume stays at 0.1 (not 0.5) — the assertion pair is what makes it the mid-loop-throw discriminator.
Adjacent-site check: swept el.volume = sites across packages/** — every remaining unclamped assignment is either downstream of an existing clamp (media.ts:313 uses effectiveVolume from clampVolume; mediaVolumeEnvelope.ts:140 uses staticVolume clamped in resolveVolumeProbeWindow) or in unrelated packages (player/parent-media.ts, cli/present.ts, studio/timelineIframeHelpers.ts). The sandbox bridge was the sole exposed leg.
Tiny nit, non-blocking: the clamp doesn't guard NaN — if volume ever arrives NaN, Math.max(0, Math.min(1, NaN)) = NaN and el.volume = NaN throws TypeError. Sibling hyperframes-slideshow.ts:761 wraps with finiteMediaNumber(msg.volume, 1) for defense in depth. Pre-existing latent risk (main is worse), not scope for this PR.
CI: all required contexts green including the format cascade (Preflight, Test, preview-regression, regression shards 1–9, Windows render, Producer integration).
— Review by tai (pr-review)
jrusso1020
left a comment
There was a problem hiding this comment.
Post-merge — this landed at 4d43a89b while I was still reading, which is the exact head I read, so everything below still applies to what is on main. Nothing here would have blocked it.
Read the four changed files whole at 4d43a89b, plus media.ts, webAudioTransport.ts, bridge.ts and audioGain.ts at the same head. The bug is real and the fix is the right shape. tai's review already covers the live throw on main, the el.volume = adjacent-site sweep, and why the init.test.ts case discriminates the mid-loop throw, so this is additive only — four things I did not see covered.
1. The new test cannot distinguish the shipped formula from the obvious alternative, because volume: 1 is the identity element
init.test.ts:165 dispatches set-volume with volume: 1. At that value the shipped clamp and a mutant that moves the fader outside it produce identical numbers:
boosted (data-volume="3.98") |
quiet (data-volume="0.5") |
|
|---|---|---|
shipped max(0, min(1, clip * v)) |
min(1, 3.98) → 1 |
min(1, 0.5) → 0.5 |
mutant max(0, min(1, clip)) * v |
1 * 1 → 1 |
0.5 * 1 → 0.5 |
Both assertions stay green under the mutant. The two forms disagree at every other fader position — at volume: 0.5 the shipped form leaves the boosted element at 1 (min(1, 1.99)) while the mutant gives 0.5 — and that difference is the master-fader semantics for a boosted clip, so it is the half worth pinning.
Stated in the form I verified: this is the only test in the repo that drives that line. init.test.ts contains exactly one set-volume dispatch (the new one), and bridge.test.ts:90-110 asserts the callback argument against a vi.fn(), so it never reaches the element loop. One extra dispatch at a non-unity volume closes it.
2. The NaN nit is reachable, and the first thing to throw is not the element
The boundary already clamps, and it has the identical gap — bridge.ts:62-63:
"set-volume": (data, deps) =>
deps.onSetVolume(Math.max(0, Math.min(1, Number(data.volume ?? 1)))),Number("loud") is NaN, and Math.max(0, Math.min(1, NaN)) is NaN, so a non-numeric volume from the parent frame arrives at onSetVolume unchanged. bridge.test.ts:97-103 pins 1.5 → 1 and -0.5 → 0; no test passes a non-numeric.
Then the order inside onSetVolume matters: webAudio.setVolume(volume) runs at init.ts:3156, before the element loop, and lands on applyMasterGain() → _masterGain.gain.value = NaN. AudioParam.value is a Web IDL float, so that is a TypeError too — the handler dies at the master gain and never reaches the line this PR fixes. So clamping harder at the element site cannot close this path; bridge.ts:63 can.
Worth doing there because the helper already exists in this package: clampNativeMediaVolume in packages/core/src/audioGain.ts is exactly Math.max(0, Math.min(1, v)) with a Number.isFinite guard in front. Today its only callers are inside audioGain.ts itself (withUnclampedVolume) — code search finds no other importer — while its sibling clampAudioGain from the same module is used in five files. That is probably why the gap keeps recurring: bridge.ts:63, init.ts:1828 and the new init.ts:3167 each hand-roll the clamp without the finite guard, and media.ts:153 hand-rolls a fourth copy with it. Not this PR's job to unify them, but the new site is the cheapest place to start using the named one.
3. The two paths do now agree — and they share a consequence worth writing down
Confirming the PR body's "clamping the element is the right half to clamp", from the other side: media.ts:318-319 already computes clampVolume(authorVolume * userVol) and assigns that to el.volume, and init.ts:1828 already clamps on the metadata-bind path. This PR's line is the same formula, so all three sites now match. Matching media.ts is the right call, and it is the reason I would not take the mutant in (1) — it would fix one site into disagreement with the other two.
The consequence, shared by both paths and not introduced here: on the HTMLMedia fallback path — element neither owned nor routed by the transport (webAudioTransport.ts:492-499), which includes any time the transport is paused — el.volume is the entire audio chain, so a 3.98 clip sits pinned at 1 for every fader position at or above 0.251 and the fader only starts moving below that. On the Web Audio path this does not happen: setElementVolume forces source.el.volume = 1 for media-element sources and puts the author gain on the per-element node, with the fader on _masterGain, so the response stays linear. Both behaviours are correct for their path; the fallback one is just non-obvious, and it is the thing a test at a non-unity volume would document.
4. The bridge writes el.volume without touching the runtime's own bookkeeping
syncRuntimeMedia tracks what it last wrote (lastRuntimeAppliedVolume, media.ts:151 and :320) and treats any drift as an authored edit (media.ts:303-312, "GSAP (or user code) changed el.volume between ticks — track it"). onSetVolume writes el.volume directly and does not update that map, so the very next sync tick sees a difference that the bridge itself caused.
Preconditions for it to bite, all readable at head: no volume lane and no probed keyframes (both branches sit above it in the ladder), and isWebAudioRouted(el) false (that branch, media.ts:290, short-circuits to fallbackAuthorVolume and absorbs this entirely). Under those, authorVolume becomes the fader-scaled value and effectiveVolume applies the fader a second time — while a fader drag is in flight the element sits at clip * v² instead of clip * v (6 dB low at v = 0.5), snapping back one tick after the messages stop, since the unchanged-since-last-tick branch then restores data-volume as the baseline.
Pre-existing — the bridge has always written el.volume — but this PR is what makes it reachable for boosted clips, which previously threw before the write landed. Not asking for it here; flagging it because it is invisible to the new test, which asserts synchronously after dispatchEvent with no sync tick in between.
None of the four is a blocker — each describes behaviour that is the same as or better than what main had before this landed, and the throw it fixes was live. (1) and (2) are the two I would actually pick up: one extra set-volume dispatch at a non-unity volume, and moving bridge.ts:63 onto clampNativeMediaVolume.
— Rames
…x bridge (heygen-com#3349) Authoring a clip above unity gain throws at runtime today. ## What breaks `MAX_AUDIO_GAIN_DB = 12` makes `data-volume` legal up to ~3.98. The sandbox runtime's volume bridge assigns the product straight to the element: ```ts el.volume = clipVolume * volume; // init.ts, onSetVolume ``` `HTMLMediaElement.volume` is spec-pinned to [0,1] and **throws `IndexSizeError`** outside it — verified in Chrome, and the test DOM agrees: ``` el.volume = 2 → IndexSizeError: Failed to set the 'volume' property... ``` The throw lands inside a `for` loop over every media element, so it takes the rest of the loop with it: every clip after the boosted one keeps whatever volume it already had, while `state.bridgeVolume` says the change was applied. A composition with one boosted clip stops responding to the volume control for every clip authored after it. ## The fix Clamp what the element receives. That is not lossy, because the element was never where the boost lived — the transport gets the authored gain unclamped, and this PR pins that half too: - `syncRuntimeMedia` hands `onElementVolume` both the element's clamped volume **and** the authored gain, so the transport can have the boost the element cannot hold. - `setElementVolume` keeps that gain on the per-element node, clamped only to `MAX_AUDIO_GAIN`. Those two paths already worked; they were untested, and they are the reason clamping the element is the right half to clamp. ## Tests - `init.test.ts` — a boosted clip followed by a quieter one, both seeded with sentinels, then the real `set-volume` control message. Asserts the boosted element lands at 1 **and** that the clip after it still gets its own volume, which is what a throw mid-loop strands. - `media.test.ts` — the transport receives the authored gain while the element stays legal. - `webAudioTransport.test.ts` — the per-element gain node keeps a boost above unity. All three mutation-checked: removing the clamp reds the first, and clamping the gain at either transport seam reds the others. ## Provenance This is the last unlanded piece of heygen-com#3280. That PR was rebased onto current `main` and collapsed from +3050 to +944, of which everything except these lines is either already merged (heygen-com#3308, heygen-com#3309, heygen-com#3333, heygen-com#3339) or duplicated by the open heygen-com#3306 and heygen-com#3310. Cutting it out separately because the throw is live on `main` now and shouldn't wait behind a PR that is otherwise redundant. (cherry picked from commit 9140c0e)
Authoring a clip above unity gain throws at runtime today.
What breaks
MAX_AUDIO_GAIN_DB = 12makesdata-volumelegal up to ~3.98. The sandbox runtime's volume bridge assigns the product straight to the element:HTMLMediaElement.volumeis spec-pinned to [0,1] and throwsIndexSizeErroroutside it — verified in Chrome, and the test DOM agrees:The throw lands inside a
forloop over every media element, so it takes the rest of the loop with it: every clip after the boosted one keeps whatever volume it already had, whilestate.bridgeVolumesays the change was applied. A composition with one boosted clip stops responding to the volume control for every clip authored after it.The fix
Clamp what the element receives. That is not lossy, because the element was never where the boost lived — the transport gets the authored gain unclamped, and this PR pins that half too:
syncRuntimeMediahandsonElementVolumeboth the element's clamped volume and the authored gain, so the transport can have the boost the element cannot hold.setElementVolumekeeps that gain on the per-element node, clamped only toMAX_AUDIO_GAIN.Those two paths already worked; they were untested, and they are the reason clamping the element is the right half to clamp.
Tests
init.test.ts— a boosted clip followed by a quieter one, both seeded with sentinels, then the realset-volumecontrol message. Asserts the boosted element lands at 1 and that the clip after it still gets its own volume, which is what a throw mid-loop strands.media.test.ts— the transport receives the authored gain while the element stays legal.webAudioTransport.test.ts— the per-element gain node keeps a boost above unity.All three mutation-checked: removing the clamp reds the first, and clamping the gain at either transport seam reds the others.
Provenance
This is the last unlanded piece of #3280. That PR was rebased onto current
mainand collapsed from +3050 to +944, of which everything except these lines is either already merged (#3308, #3309, #3333, #3339) or duplicated by the open #3306 and #3310. Cutting it out separately because the throw is live onmainnow and shouldn't wait behind a PR that is otherwise redundant.