fix(audio): raise the authoring gain ceiling and carry it through the probes - #3333
Conversation
terencecho
left a comment
There was a problem hiding this comment.
Concur-on the load-bearing pieces of the #3304 round — they closed cleanly at head 891560f10:
- Clamp pinned at the ceiling.
audioMixer.test.ts:289-319(volume: 99→toContain("volume=3.981072")) plusaudioGain.test.ts:58(formatAudioGain(99).toBe(formatAudioGain(MAX_AUDIO_GAIN))). The comment at :316 spells out exactly the failure mode the earliervolume=3.98case would have masked — "changing MAX_AUDIO_GAIN's effect leaves this suite green" without a> MAXcase. Nailed. - Percent slider locked above unity.
propertyPanelMediaSection.tsx:261getsdisabled={volume > 1}andpropertyPanelFlatMediaSection.tsxgets one at both readouts (+35 lines of new coverage). A boosted clip can no longer be silently capped by touching the 0–100% control. audioLeveller.ts:14-22rationale rewritten off "0..1 clamp" onto ownership (author's fader vs leveller's gain node) — the reason that survives the ceiling change.
Chain LGTM otherwise:
- Shared module
audioGain.ts— single owner:MAX_AUDIO_GAIN = 10 ** (12/20) ≈ 3.981,clampAudioGainfor authoring,clampNativeMediaVolumefor the spec-clampedHTMLMediaElement.volume. - Probes updated on both sides — preview (
mediaVolumeEnvelope.ts:withUnclampedVolume) and render (htmlCompiler.ts:2280mirror,maxGain: MAX_AUDIO_GAINserialized into the page-evaluate) shadow.volumeso the authored gain survives sampling while the native setter still receives the clamped value. - Runtime split is clean —
media.tscomputesauthorVolume = clampAudioGain(...)for the WebAudio path andeffectiveVolume = clampVolume(...)forel.volume; render mixer widensclampVolume → clampAudioGainso bothstaticVolumeand every keyframe travel unclamped up to the ceiling into ffmpeg'svolumeexpression. - The LUFS numbers are self-consistent — +5.8 dB = 20·log10(1.949845). Boost measurably lands.
One question and two nits, none blocking (draft):
Q — webAudioTransport.setElementVolume:453 still clamps to [0,1]. The runtime tick (init.ts:2034) hands authorVolume in unclamped, but setElementVolume does Math.max(0, Math.min(1, volume)) before gainNode.gain.value = safeVolume. Every tick a boosted clip is active, the transport re-caps its per-element gain at unity. Automation lanes scheduled via scheduleParamLane route around this — they book ramps directly and VOLUME_RANGE.scale is only used for the shape — but for a static data-volume="1.949845" with no envelope I don't see how the preview retains the boost past the first tick. If the intent is that this is a user-volume stage (fader semantics) and the authored boost lives elsewhere, the name reads misleadingly; if it's meant to carry the authored gain, the clamp is the same shape the probes just retired. Worth a look — the PR body's measurement is coherent with the render path but doesn't distinguish preview vs render.
Nit — VOLUME_RANGE.max = 1 at audioAutomation.ts:156. Struct's .scale is what scheduleParamLane consumes for the volume lane, and I don't see .max clamping lane .v values on the write path (volumeLaneKeyframes passes through, buildVolumeExpression re-clamps via clampAudioGain), so it looks harmless. But leaving the range advertising 0..1 alongside the leveller comment that specifically calls out "0..+12 dB, not 0..1" reads inconsistently — widening or a documentation-only note would remove the double-read.
Nit — GSAP-tracking branch at media.ts:296-302. The change-detection compares clampVolume(el.volume) (0..1) against previousRuntimeVolume. Since withUnclampedVolume shadows the accessor only during the probe — not runtime sync — el.volume reads via the spec-clamped native getter, so any GSAP tween running above unity between ticks reads back ≤1 and the branch either misses it or under-tracks it. Practically fine because the volumeKeyframes path at :278 supersedes it whenever automation was probed, but a one-line comment noting the branch is unity-capped by design would prevent the future rediscovery.
Bridge, not destination — the percent slider stays disabled here and gets replaced outright by the dB fader in #3305 per your inline note at propertyPanelMediaSection.tsx:249-252.
CI green across all shards. Not stamping while draft.
— Review by tai (pr-review)
jrusso1020
left a comment
There was a problem hiding this comment.
Reviewed at 891560f1. All 8 required contexts are green, so nothing below comes from CI — it comes from diffing this branch against its own merge base.
Audited end-to-end: core/src/audioGain.ts, core/src/audioLeveller.ts, core/src/runtime/media.ts, engine/src/services/audioMixer.ts, producer/src/services/htmlCompiler.ts, both Studio panels, parsers/src/types.ts, and all four test files against their merge-base versions. Read for context: core/src/runtime/webAudioTransport.ts at this head (not in the diff — see the preview finding). Not executed: no suites run locally; every claim below is a source or content-API read.
Strengths
- My two #3304 nits came back better than I filed them. The fader hold is the exact one-liner with the in-file precedent —
disabled={volumeAutomated || volume > 1}(propertyPanelFlatMediaSection.tsx:229) — and the comment above it says plainly that it is "A hold, not a fix". AndaudioLeveller.ts:14-22didn't just delete the false "volume is 0..1" claim, it replaced it with a rationale that is actually true and stronger: the lane is owned by the author, so a leveller writing into it would silently redraw their envelope. Correcting a stale comment by finding the real reason is the harder and better move. - The upper bound is now pinned, with the reasoning in the test.
audioMixer.test.tsfeedsvolume: 99and assertstoContain("volume=3.981072"), and the comment states why the3.98case alone was insufficient. That is the rule-15 shape closed properly rather than patched. - The ceiling still crosses the process boundary as a parameter, not a literal —
MAX_AUDIO_GAINis passed into thepage.evaluatecontext asmaxGain(htmlCompiler.ts:2257+), so the one value that would drift is not part of the duplicated probe body. The duplication is annotated as such. parsers/src/types.ts:163— thevolume?: numbercomment goes from0-1 (0% to 100%)tolinear gain; 0 is silent, 1 is 0 dB, values above 1 boost. Small, and it is the field's contract.
blocker — three files are stale copies from the abandoned #3304 branch, and they revert live work
This is one mechanism with three consequences, and CI cannot see any of it, because deleted tests do not fail.
packages/engine/src/services/audioMixer.test.ts at this head is byte-identical to #3304's final head (6a5267ab, diff -q → identical, 1186 lines). #3304's merge base is 67edb01b (Aug 16 18:01). This PR's merge base is 3e4b08cd (Aug 18 15:11), where that file is 1353 lines. So the file is a two-day-old snapshot laid over a newer base, and everything that landed on it in between is silently undone. Confirmed at all three refs — merge base 1353, this head 1186, current main 1353 — and nothing is re-added anywhere in the diff (grep for each removed block: added 0, removed 1+).
What goes, none of it gain-related:
data-playback-startfallthrough (valueless / empty / whitespace / invalid →data-media-start), from #3322, merged Aug 18 14:17 — 54 minutes before this branch's own merge base. Those cases pin a fix forgetAttributereturning""rather thannull, where a??reader and a truthiness reader diverged and surfaced the whole trim offset as A/V desync. Deleting them un-guards a bug fixed hours earlier.- All retiming coverage:
playbackRate7 → 0 andatempo5 → 0. Gone:builds a bounded atempo chain for normalized rate(0.1 and 5),trims the consumed source span and applies pitch-preserving tempo at 2x(its-ss/-t/atempoassertions replaced by a gain test),keeps automation on authored timeline time after constant retiming. parseAudioElements strict literal timing(9 hand-authoreddata-durationliterals:""," ","0s","0abc","0px","-1s","Infinity","NaN","0x10") anddrops a known zero timeline window without audio prep or mix(3 cases).it.eachblocks drop 5 → 1.
audioMixer.ts itself is only +2/-2 here, so the retiming behaviour stays and only its guard leaves — the failure mode is a future change to atempo or the playback-start fallthrough landing with a green suite.
Same mechanism, two more files:
packages/core/package.jsonrolls the package version backwards:0.8.3→0.7.109. The./audio-gainsubpath additions in that file are correct and wanted; theversionline is carryover.packages/core/src/runtime/mediaVolumeEnvelope.test.tsdropstreats trailing-garbage duration as unknown instead of truncating preview sampling(data-duration = "5s"— theparseFloat("5s") === 5vsNumber("5s") === NaNsplit) andretains the last plateau sample before a short volume change, while adding its gain cases.
Fix: restore those three files from main and re-apply only the additions this PR needs — the two mixer gain tests, the envelope gain tests, and the ./audio-gain export block. The four source files that were genuinely re-edited on the new base (media.ts, mediaVolumeEnvelope.ts, audioMixer.ts, htmlCompiler.ts) all differ from #3304 and are fine, which is what makes this a slip on three copied files rather than a branch-wide problem. Note #3305 is stacked on audio-gain-ceiling, so it restacks onto whatever this becomes.
important — preview still caps at 0 dB while render now goes to +12 dB
This also answers the setElementVolume question raised on this PR: the clamp there is real, but it is not the only one, and it is not the reason.
media.ts now lets an above-unity value reach the write — the three clampVolume → clampAudioGain swaps at :259, :277, :287 do exactly that. But the terminus is unchanged: const effectiveVolume = clampVolume(authorVolume * userVol); el.volume = effectiveVolume; (:304-305), and clampVolume in that file is still Math.min(1, …). So the boost is clamped back to unity at the last step. The only other writer that could carry it, webAudioTransport.setElementVolume:453, also clamps to [0,1].
The mechanism that carried it on #3304 — applyElementGain, returning a boolean so the element stays at unity when the graph owns the gain — exists in neither this PR nor main (verified: 0 occurrences at both refs; webAudioTransport.ts is not in this diff). #3304 closed unmerged, so that half never landed and was not re-cut here.
Net: a clip at data-volume="1.95" renders ~5.8 dB hotter than it previews. The title scopes this PR to the probes, so I am filing it as important rather than a blocker — but it inverts the usual direction of the problem. On #3304 the fix applied and the reports lied; here the render applies and the thing the author is listening to does not. If #3333 is the last rung before users can author above unity, this is the blocker in the stack, and it is worth saying which rung owns it.
nit — the master bus still has no limiter, and now that is the only headroom question left
Correcting my own #3304 wording: I wrote "no limiter anywhere", which was wrong. HfLimiter exists (core/src/audio/audioFxWorklets.ts:104, registered hf-limiter) and it does reach render, because engine/services/audioFxRender.ts runs the real Web Audio graph offline in headless Chrome rather than reimplementing effects as FFmpeg filters. What is absent is a limiter in the default master chain (amix → [mixed]volume=master × N, audioMixer.ts:660-665), which is where a boosted clip near full scale now hard-clips. Still a question about which rung owns it, not a request.
Verdict: REQUEST CHANGES
Reasoning: The gain work itself is good and closes every #3304 nit, but three files are stale copies from the abandoned branch and silently revert live coverage — including a regression guard merged 54 minutes before this branch's own merge base — plus a package version rollback. The fix is mechanical, and I will re-review promptly.
— Rames Jusso
763caa1 to
8fb332f
Compare
|
The stale-files blocker was exactly right, and it's the kind I'd never have caught myself — deleted tests don't fail. Fixed at How it happened, since the mechanism matters more than the three files: when #3304 was superseded I rebuilt this branch by All three are restored from I also moved my two gain cases into On the preview/render split — you were right, and my earlier
I only found the second because I wrote the test before believing the first fix — it asserted the author gain reaching the transport and failed at 1 instead of 1.949845. Both are pinned now, both verified by mutation. The The limiter note — agreed, and I'd rather not fold it in here. A default master limiter is a mix-quality decision with its own blast radius, and it's the first thing that should land once above-unity authoring is actually reachable by users. Happy to open it as its own issue. Coming out of draft now — CI is green and the branch is rebased onto |
jrusso1020
left a comment
There was a problem hiding this comment.
Re-reviewed at 8fb332f3. All three blockers resolved, and the important finding was fixed too, which I had not asked for.
Verified the restore at source rather than from the diff, at three refs:
| merge base | this head | main |
|
|---|---|---|---|
audioMixer.test.ts |
1353 | 1414 | 1353 |
It now adds 61 lines over main instead of deleting 167. All six previously-dropped blocks are back — strict literal timing, the zero-window drop cases, bounded atempo chain, keeps automation on authored timeline time, and the valueless/whitespace data-playback-start fallthrough from #3322 — with playbackRate restored 0 → 7, atempo 0 → 5, and it.each 1 → 5. The gain additions survived the restore intact (volume: 99 → volume=3.981072 still pinned). core/package.json is back to 0.8.3, and both mediaVolumeEnvelope.test.ts guards are present again (the data-duration="5s" trailing-garbage case and the plateau-retention case). The branch also rebased onto current main, so the two-day-stale base is gone as well as its symptoms.
The preview cap is fixed properly, with the gain on the right node. setElementVolume now clamps with clampAudioGain against the shared ceiling instead of min(1, …), and holds source.el.volume = 1 for media-element sources while the boost rides gainNode.gain.value. That is the correct shape — el.volume staying spec-clamped is not the bug, it is the contract, and the gain belongs on the node. Pinned by three new cases in webAudioTransport.test.ts: an above-unity author gain reaching the node (toBeCloseTo(1.949845, 6)), a refusal beyond the ceiling (toBeCloseTo(MAX_AUDIO_GAIN, 6)), and the master fader staying spec-clamped because it is a fader, not a gain. That last one is the distinction I would have wanted a test for and did not think to ask for.
note — Tests on windows-latest is red here, and it is not this PR
Flagging so nobody chases it into the audio code. The failing test is packages/cli/src/utils/orphanCleanup.test.ts — CLI, and this PR touches no CLI file. It is processIdentity(process.pid) returning null for the live test process and reaching .toMatch(), after 2050 ms against a 2000 ms PowerShell budget.
Provenance: that test arrived in c1c70f44 (#3307). main's windows job was green at 3e4b08cd — the older base this branch used before the rebase, which is exactly why the previous head passed it — and is red at b31dde35, current main. So this head inherited a main-side flake by rebasing onto it, and the gate will keep holding the merge until main is fixed. Nothing to change here; details on the root cause are in my #3308 note.
Verdict: APPROVE
Reasoning: Every blocker is verified fixed at source across three refs with the gain work intact, and the preview/render divergence was fixed and tested beyond what I flagged. The one red required context is an inherited main-side flake in an unrelated package, and the branch-protection gate — not my review — is the right thing holding the merge until that clears.
— Rames Jusso
terencecho
left a comment
There was a problem hiding this comment.
Re-reviewed at 8fb332f3a (was 891560f10 when I last looked — PR is out of draft now). The two fixup commits close both open items from that round.
The open question is resolved — and the mechanism is the right one
I asked whether a static above-unity data-volume survived the runtime tick, given setElementVolume re-capped the per-element gain at unity every tick. It did not, and 1aa969e2e fixes it at the terminus rather than routing around it:
webAudioTransport.ts:464—Math.max(0, Math.min(1, volume))→clampAudioGain(volume). The per-element gain now clamps againstMAX_AUDIO_GAIN, not the spec's[0,1].setVolume:448deliberately stays spec-clamped, and the new docstring says exactly why: the master is a fader, not a gain. That is the distinction that makes the split correct instead of an inconsistency, and it is pinned by a test (setVolume(99)→1).- No double-application.
media.ts:304-305still writesel.volume = clampVolume(authorVolume * userVol), butsetElementVolume:469resetssource.el.volume = 1formedia-elementsources on the same tick, so the element passes unity-flat into the graph and the gain node carries the whole author gain. That is functionally whatapplyElementGaindid on the abandoned #3304 — re-cut here rather than copied, and it lands in the same place.
Traced the full path for a static data-volume="1.949845": isWebAudioRouted → authorVolume = clampAudioGain(clip.volume) = 1.949845 → onElementVolume (init.ts:2034) → setElementVolume → gainNode.gain.value = 1.949845. Holds on every tick, not just the first. Preview and render now agree.
The first-tick branch got the other half of the same bug: authorVolume = fallbackAuthorVolume > 1 ? fallbackAuthorVolume : currentElementVolume (media.ts:305). Reading back through el.volume on tick one would have opened a boosted clip at 0 dB and jumped once the unchanged-since-last-tick branch took over. The comment names that failure mode and names why no single-tick test would have caught it.
Coverage is the shape I'd want: media.test.ts asserts the author gain leaves uncapped while el.volume is 1 (both halves, one test), and webAudioTransport.test.ts pins the ceiling with a 99 input rather than a value at the bound — same rule the mixer test closed last round, applied without being asked.
James's blocker is mechanically closed — verified against main, not just re-read
All three files are now strictly additive against main (git diff origin/main..head → 0 removed lines on each):
| file | main | head | removals |
|---|---|---|---|
engine/services/audioMixer.test.ts |
1353 | 1414 | 0 |
core/runtime/mediaVolumeEnvelope.test.ts |
204 | 260 | 0 |
core/package.json |
668 | 678 | 0 |
packages/core/package.json version is back to 0.8.3, matching main. Every test he named by name is present at head: data-playback-start fallthrough (7 hits), builds a bounded atempo chain, pitch-preserving tempo at 2x, keeps automation on authored timeline time after constant retiming, strict literal timing, drops a known zero timeline window, trailing-garbage duration, retains the last plateau sample. The GitHub file list confirms it at the API level too — audioMixer.test.ts is now +61/-0 where it was a whole-file replacement.
Both prior nits addressed — one of them by documenting the split rather than closing it
- GSAP-tracking branch (
media.ts:296-302) — got the one-line note I asked for, and it is better than the note: it explains the branch is unity-capped becauseel.volumeis spec-pinned and cannot observe an above-unity value however wide the clamp gets, then points at the probed-keyframes branch that does carry the gain. That is the rediscovery prevented rather than deferred. VOLUME_RANGE.max = 1— still 1, and correctly so. I under-called this last round: it is load-bearing on the write path, not just advisory.cleanPoint(audioAutomation.ts:295) clamps every volume-lane point throughrange.max, anduseAutomationLaneGestures.ts:193/automationLaneDragMath.ts:125clamp drag and typed entry the same way. So a drawn volume lane is capped at 0 dB while a staticdata-volumereaches +12 dB.audioLeveller.ts:17-19now states that split precisely — "normaliseEnvelopenow clamps to 0..+12 dB, whileVOLUME_RANGE, which bounds the lane itself, still stops at unity until the dB fader lands" — and I verified both halves are true in code. Documented staged asymmetry with the landing rung named beats a widening that outruns the fader UI.
nit — two comments the clamp swap left behind
Small, and the same class of stale-doc that #3304 spent a round correcting in this exact file's neighbour.
mediaVolumeEnvelope.ts:20-22— the docstring onnormaliseEnvelopestill says it will "subtracttrackStart, clamp to [0,1], sort, de-duplicate". Both clamps in the body becameclampAudioGainin this diff (:36,:52), so[0,1]is now the one range it specifically does not use. It reads as the contract for the function whose contract this PR changed.audioLeveller.test.ts:151—// VOLUME_RANGE is 0..1 and normaliseEnvelope clamps into it. That coupling is exactly what this PR decouples, and it sits in the file whose header comment now documents the two as deliberately different. Not in the diff, so CI has no reason to surface it.
CI — green except one failure that main already has
37 checks green at head. Tests on windows-latest is red, and it is not this PR. The failure is packages/cli/src/utils/orphanCleanup.test.ts:22 — TypeError: .toMatch() expects to receive a string, but got object in process-tree ownership > captures a stable birth token for the current process. That file is not among this PR's 18 changed files; it arrived via base commit c1c70f44b (#3307, "signal only processes the OS says own the port"). I confirmed main at b31dde35b fails the same check with the same job. processIdentity(process.pid) returns an object rather than a linux|posix|windows: string on win32 — worth a follow-up against #3307, but it does not belong to #3333 and it is not a required context.
Everything I raised is closed, the blocker is closed against main rather than by assertion, and the preview/render divergence this PR exists to remove is actually removed. Approving.
— Review by tai (pr-review)
… probes Builds on #3328, which made the preview graph apply author gain and user volume exactly once each. That ownership is now correct but everything is still clamped to 1.0, so a clip authored above unity cannot be heard or rendered. `HTMLMediaElement.volume` is spec-clamped to [0,1], so both timeline probes lost a clip's authored gain the moment it also carried a fade: the probe seeded the element at the clamped value and every sample read back at or below 0 dB, and the mixer prefers probed keyframes over the static volume. Both probes now shadow the accessor for their own duration and forward the clamped value to the native setter, so the authored gain survives while nothing outside the probe ever sees an illegal volume. Measured on one 6 s composition, first 4 s: unity -32.8 LUFS, boosted-with-fade -32.8 before and -27.0 after — +5.8 dB, exactly the gain the clip was authored at. One ceiling, defined once in `audioGain.ts` and reachable from both sides: the render mixer imports it, and the page-serialized probe takes it as a parameter rather than re-literalling it. User volume stays spec-clamped — it is a fader, not a gain. Also holds the percent volume slider above unity in both property panels. That control tops out at 100%, so one touch would cap a boosted clip and drop up to 12 dB that now genuinely renders; the dB fader that can represent these levels replaces it in the next PR.
Review follow-up. `setElementVolume` receives the clip's author gain and clamped it to [0,1], so a static `data-volume` above unity was capped on the WebAudio preview path while the render honoured it — the exact preview/render divergence this ceiling exists to close. Automation lanes hid it: they schedule ramps onto the param directly and never pass through here. The master volume beside it stays spec-clamped, because a user fader is not a gain. Verified by mutation: restoring the [0,1] clamp reds the new case. Also scope the leveller's rationale to this rung — `VOLUME_RANGE` still stops at unity until the dB fader lands, so "both now span the same range" was premature — and say why the GSAP-tracking fallback is unity-capped: it reads back through `el.volume`, which the spec pins to [0,1], so it cannot observe an above-unity value however wide the clamp gets.
…cap preview Review blocker: three files were wholesale copies from the abandoned #3304 branch laid over a two-day-newer base, so they silently reverted work that had landed in between. CI could not see it — deleted tests do not fail. - `audioMixer.test.ts` was byte-identical to #3304's head: 1186 lines against a base of 1353. Gone with it were the `data-playback-start` fallthrough cases from #3322 — merged 54 minutes before this branch's own merge base — and all retiming coverage (`playbackRate` 7 to 0, `atempo` 5 to 0), the strict literal-timing table, and the zero-window cases. - `mediaVolumeEnvelope.test.ts` dropped the trailing-garbage duration case and the plateau-retention case. - `packages/core/package.json` rolled the package version back 0.8.3 to 0.7.109. All three are restored from `main` with only this PR's additions re-applied on top, and the subpath export is regenerated by the repo's own script rather than hand-edited. Also closes the preview/render split the same review raised. Two clamps had to go, not one: `setElementVolume` capped the author gain at the transport, and the first-tick branch in `syncRuntimeMedia` trusted `el.volume` — which is spec-bound to [0,1] and so cannot represent a boost, opening a boosted clip at 0 dB for one tick before the steady-state branch took over. Both pinned by tests, both verified by mutation.
8fb332f to
834c52e
Compare
…x bridge (#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 #3280. That PR was rebased onto current `main` and 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 on `main` now and shouldn't wait behind a PR that is otherwise redundant.
…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)
Supersedes #3304. Rebuilt on top of #3328 rather than merged into it — see below.
HTMLMediaElement.volumeis spec-clamped to[0,1]. Both timeline volume probes — the preview one and the copy the renderer serializes into the page — seeded the element fromdata-volumeand sampled it back, so a clip authored above unity lost its whole boost the moment it also carried a fade: every sample read at or below 0 dB, and the mixer prefers probed keyframes over the static volume.Both probes now shadow the accessor for the duration of the probe, forwarding the clamped value to the native setter, so the authored gain survives while nothing outside the probe ever observes an illegal volume.
Measured
One 6 s composition, integrated loudness over the first 4 s:
data-volume="1.949845"+ fade-out — before+5.8 dB, exactly the gain the clip was authored at. Same composition, same asset, only the probe changed.
Why this replaced #3304
#3304 was opened before #3328 landed, and the two overlapped: #3328 already gives the preview graph correct ownership — master gain owns user volume and mute, per-element gain owns authored volume, and media-element sources are routed explicitly. #3304 solved those same three things its own way, so merging it would have left two owners for one decision.
What #3328 does not do is raise the ceiling: every path still clamps to 1.0. That is all this PR does, on top of #3328's structure rather than beside it — the shared
audioGainmodule, the unclamped probes, and widening the author-gain clamps in the runtime and the render mixer. User volume stays spec-clamped, because it is a fader rather than a gain.The result is ~680 lines instead of ~840, and it keeps
isWebAudioRoutedandsourceKindas the single answer to "is the graph carrying this element".Carried over from the #3304 review
value={195}on it and one touch would silently cap the clip — only damaging once this PR makes the boost real.propertyPanelMediaSectionhad nodisabledat all, so both are held, not just the one raised in review.audioLeveller's rationale corrected — it justified riding againnode with "volume is 0..1 andnormaliseEnvelopeclamps every keyframe into it", which this PR makes false. The reason that survives is ownership: the volume lane is the author's envelope.Stack
First of three on the audio side.
#3305(the dB fader) and#3306(normalize-audio) both import the shared gain module and will be retargeted onto this.