feat(cli): add normalize-audio to match one clip's loudness to another - #3306
Conversation
63ffe8d to
295bd09
Compare
295bd09 to
f2a02bb
Compare
763caa1 to
8fb332f
Compare
f2a02bb to
1f2c5bb
Compare
8fb332f to
834c52e
Compare
1f2c5bb to
9e52967
Compare
9e52967 to
585ede9
Compare
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
jrusso1020
left a comment
There was a problem hiding this comment.
Reviewed at 585ede92, full pass — nothing else was posted when I started. Read normalize-audio.ts end to end plus its tests, and resolved the attribute question against the schema doc rather than against the diff.
Strengths
- The
-ss/-t-before--inote is the good kind of comment (normalize-audio.ts:390-398): it states the wrong behaviour, the right behaviour, and the measurement that separates them (-33.8 vs -61.8 LUFS on the same window). That is a comment the next person cannot accidentally undo. data-endas a first-class trim (authoredDuration,:209-218) is the non-obvious half of "measure what the composition plays", and it is covered in both directions plus the degenerate case (normalize-audio.test.ts:45-68, includingdata-endnot outlasting its start).- The write path takes concurrency seriously and says why (
:494-513): re-reading before patching because two ffmpeg passes have elapsed, temp-file-plus-rename so a crash cannot truncate a composition, and a one-attribute patch so scripts and comments stay byte-stable. --jsonfailures are documents too (:404-409). A command built for agents that emits a bare error line on stderr breaks every caller doingJSON.parse(stdout); getting this right up front is cheaper than retrofitting it.
Nit
--tolerance is validated after the expensive work. normalizeAudio (:496-499) calls measuredPlan(...) and only then parsedTolerance(options.tolerance). So --tolerance nonsense spends up to two 120-second ffmpeg passes before failing on an argument that could have been rejected immediately. Hoisting the parsedTolerance call above measuredPlan is the whole fix.
Notes (checked, no action needed — published so nobody re-derives them)
- Reading only
data-media-startis correct here, and I nearly filed it as a bug.readMediaStartinpackages/core/src/runtime/playbackRate.ts:21-30givesdata-playback-startprecedence, which reads like a supported alias this command ignores. It is not the right ceiling for this path:docs/reference/html-schema.mdx:125states that the engine's audio mixer — the thing that feeds ffmpeg-ss— reads onlydata-media-start, and that<audio>should use exactly that name. Since the point is matching loudness in the render, agreeing with the mixer is right. Flagging it because it is the first thing a reviewer working fromplaybackRate.tswill report. - Silence and clipping are both already guarded, so the usual two failure modes for a loudness matcher are closed: muted input is rejected (
:307-309, and!(volume > 0)catchesNaNas well as0), the boost ceiling is refused with the remedy named rather than silently clamped (:313-320), and the projected true peak is checked before anything is written (:323-328). A near-silent reference still resolves to a large attenuation rather than an error, which is arguably what "match this" means — and it cannot reach a hard mute, sinceebur128floors around -70 LUFS andformatAudioGainkeeps six decimals, so the written value stays above zero. resolveLocalAudioPath(:263-283) rejects absolute paths, URL schemes, protocol-relative sources and..traversal, and handles the malformed-percent-encoding case explicitly.- The body says to retarget to
main. Already done — base ismain, one commit ahead, and the gain module it imports is present there.
Merge order — worth coordinating with #3305
This PR and #3305 both rewrite the hyperframes-audio entry in skills-manifest.json, both starting from c96ccac4b1127b8c. Same line, different values, so whichever merges second hits a real conflict.
Regenerate rather than picking a side: your references/diagnosis.md and #3305's SKILL.md are both inside that one bundle, so once both are on main the correct hash is a third value neither PR carries. Picking a side fails quietly — the sync check runs on pull requests, so nothing on main's tip goes red, and the drift only surfaces on the next PR that touches the skill.
Verdict: APPROVE
Reasoning: The measurement window, the refusal cases and the write path are all correct and independently checked; the only finding is a one-line reordering of argument validation.
— Rames Jusso
terencecho
left a comment
There was a problem hiding this comment.
Well-scoped LUFS matcher; math and edge-handling both hold up.
- Effective-loudness math (
audioNormalizationPlan):wantedGainDb = referenceLufs - target.integratedLufscorrectly makes the target's effective LUFS equal the reference's effective LUFS (accounting for both clips' currentdata-volume). - Windowing (
loudnessMeasureArgs):-ss/-tbefore-i— critical when using-f null, and the unit test pins that ordering; also honorsdata-endviaauthoredDuration, so a trimmed clip is measured on the window that actually plays. - Clip guard: rejects gains whose true-peak projection exceeds 0 dBFS, using
ebur128=peak=true(true peak, not sample peak) — the right thing. - Ceiling:
wantedGainDb > MAX_AUDIO_GAIN_DBis on the absolute authored gain (not the delta), which matches the shared Studio/render ceiling, and error copy correctly points at source-file preprocessing rather than trying to save it in the mixer. - Locale:
parseEbur128SummaryscrapesI:/Peak:from the ebur128 C-format summary — locale-independent (unlike, e.g., loudnorm state strings), so no fail-open on non-English Windows. - Path safety (
resolveLocalAudioPath): remote, absolute, and..traversal all rejected; Windows separator is handled. - Concurrency: re-reads
index.htmlafter the ~2 min measurement window before applying the one-attribute patch, then atomic temp-file + rename — a Studio edit made during measurement is preserved. - Agent contract:
--jsonmode also emits errors as parseable documents, and command is registered in the curated--helpgrouping so agents enumerating capabilities discover it.
CI green including Windows render / tests / CLI-npx smoke.
|
Nit fixed — Being straight about coverage: it isn't pinned by a test. The ordering is internal to Manifest collision, confirmed: this PR and #3305 both rewrite the Thanks for going after the failure modes rather than the diff — muted input, the boost ceiling refusing with a remedy instead of clamping, and the true-peak check before any write were the three I most wanted a second pair of eyes on. |
somanshreddy
left a comment
There was a problem hiding this comment.
Independent pass — verified the two headline correctness points by re-execution. Reads clean; nits + one test gap, no blockers.
What I verified at the execution layer
-ss/-tbefore-i.loudnessMeasureArgspushes-ss/-t(input-side) then-i file … -af ebur128=peak=true -f null -— soebur128integrates only the played window, not the whole file. Pinned by theindexOf("-ss") < indexOf("-i")test.- Window = data-duration, else data-end−data-start, seeking data-media-start.
authoredDurationprefersdata-duration, falls back toend − start;-ssusesmediaStart. I confirmed this matches the render mixer's own windowing inproducer/src/services/audioExtractor.ts(extractAudioTrackderives duration identically and seeks byelement.mediaStart) — so the measured window is exactly what renders. This parity is the subtle thing that makes the gain correct, and it holds. - Gain math.
wantedGainDb = referenceLufs − target.integratedLufs(absolute, measured from the target's raw integrated loudness → the writtendata-volumeis absolute, not compounded),volume = 10^(wantedGainDb/20). I re-ran the plan in Node for the test vectors and a boosted-input case; results match (-3.8 dB → 0.645654, projected peak/LUFS correct). The +12 dB ceiling (dB) andformatAudioGain's linear clamp (MAX_AUDIO_GAIN = 3.981) agree exactly, so a passing plan is never silently clamped; muted input and projected-clip are refused before any write with the remedy named — matching Rames's read. - Dry-run never mutates;
--writegates onwrite && !withinToleranceand writes atomically (temp + rename).
Nits (non-blocking)
- Agree with Rames: hoist
--toleranceparsing. ConfirmedmeasuredPlan()runs both ~120s FFmpeg passes beforeparsedTolerance()validates the argument — a bad--tolerancewastes up to ~4 min before failing on a pure arg error. Validate it first. - Orchestration is untested. The pure helpers (
loudnessMeasureArgs,audioNormalizationPlan,authoredDuration) are well covered, butnormalizeAudio()itself — dry-run-does-not-write,--writewrites,withinToleranceskips, the deliberate re-read-of-index.html-before-write (so a Studio edit during the two FFmpeg passes isn't clobbered), and the--jsonfailure envelope — has no test. Those are the riskiest lines; a fake-ffmpeg orchestration test would pin the safety contract. - Docs artifact: the
cli.mdxinsertion re-emits## Look at it, producing a duplicated heading, and filesnormalize-audiounder it while the summary table correctly lists it under "Bring in source material".
Solid PR — the measurement correctness (the easy thing to get subtly wrong) is right and, importantly, matches the render path.
Measures two authored `<audio>` clips with FFmpeg's integrated EBU R128 loudness and writes the target's matching `data-volume`, leaving the reference untouched. The measurement is bounded to the window the composition actually plays. `data-end` bounds a clip's timeline window just as `data-duration` does, and `-ss`/`-t` belong before `-i`: after it they bound the OUTPUT, and with `-f null` there is none, so ebur128 keeps integrating past the clip. On a fixture whose played window is -61.8 LUFS inside a file that measures -27.9 whole, either mistake reports a loudness the composition never plays and "corrects" an already-matched clip by tens of dB. Two EBU R128 passes run between reading the composition and writing it, each bounded only by a two-minute timeout, and the skill docs tell agents to keep Studio open meanwhile — so the attribute patch is re-applied to a fresh read and written through a temp file and a rename. Under `--json` the failures are documents too: an agent doing `JSON.parse(stdout)` on a bare error line throws. A pair needing more than the +12 dB ceiling has a source-file problem rather than a mixer one — mixer gain raises the noise floor with the signal — so the refusal names the remedy.
Each EBU R128 pass is bounded at 120s and normalize-audio runs two, so parsing the argument afterwards made a typo'd --tolerance cost both of them before failing on something that was wrong from the start. Not pinned by a test: the ordering is internal to the command and neither it nor the parser is exported, so covering it would mean restructuring for a spy rather than asserting the behaviour.
00c40eb to
5cd1d5b
Compare
jrusso1020
left a comment
There was a problem hiding this comment.
Re-review at 5cd1d5b8. My earlier APPROVED sits at 585ede92 and no longer speaks to this head. The --tolerance hoist is correct; the reason I'm not re-approving yet is CI, and it's one file.
The nit is closed, and the hoist is safe
parsedTolerance (packages/cli/src/commands/normalize-audio.ts:486) is a pure validator — it reads options.tolerance, throws on non-finite or negative, and touches nothing the measurement produces. So moving it above measuredPlan has no dependency to get wrong, and a typo'd --tolerance now fails before the two 120s EBU R128 passes instead of after them. Exactly the change I asked for.
On your coverage note: I wouldn't restructure for a spy. The ordering isn't user-observable except as elapsed time, so a spy asserting call order would pin the implementation rather than the behaviour, and it'd be the kind of test that breaks on the next refactor while proving nothing. The coverage worth having is the one @somanshreddy named — the normalizeAudio orchestration paths (dry-run vs --write, within-tolerance short-circuit, the re-read before write, --json on failure). Those are user-observable, and a fixture project driven through the command end-to-end reaches them without exporting anything or introducing a spy. That's an important-tier gap, not a blocker.
Why three checks are red — one file, one command (net-new)
Format, Preflight (lint + format) (both runs) and preview-regression are all red, and they are all the same cause:
$ oxfmt --check .
packages/cli/README.md (45ms)
Format issues found in above 1 files. Run without `--check` to fix.
One file. The cascade from it is worth spelling out because preview-regression reads like a preview bug and isn't one:
packages/cli/README.mdisn't oxfmt-formatted →FormatandPreflight (lint + format)fail.preview-paritydeclaresneeds: [changes, preflight](.github/workflows/preview-regression.yml:64), so the failed preflight skips it.- The
preview-regressiongate then runs withPREVIEW_FILTER_RESULT: trueandPREVIEW_PARITY_RESULT: skipped, and its test is!= "success"→ exit 1. It's failing closed on a check that never ran, which is the right behaviour for a gate — it just isn't a preview defect.
The part that actually matters: the same needs chain means the required Test context was never created at this head. Not failing — absent. I diffed the required set against what exists:
required contexts: 8
--- 3306 @ 5cd1d5b8 --- NEVER RAN: Test
--- 3310 @ dfca6186 --- all required contexts present
So there is currently no test signal on 5cd1d5b8 at all, and the merge gate can't clear on reviews alone while a required context is missing. regression did run and passed; Test is the hole. bun run format (or oxfmt packages/cli/README.md) fixes the file, and preflight, Test, preview-parity and preview-regression all follow from it.
nit — the same coercion that caused the #3305 blocker, benignly
parsedTolerance uses Number(raw), and Number("") is 0, which is finite and not negative. So --tolerance "" is accepted as "require an exact match" rather than rejected. It's the same Number()-on-empty family as the data-volume bug on #3305, just harmless here since nobody passes an empty string on purpose. A raw.trim() === "" guard closes it if you're touching the function anyway.
Confirmed, not re-litigated
- Input-side
-ss/-t: verified at source myself —extractAudioTrackpushes both before-i(packages/producer/src/services/audioExtractor.ts:130-138), so the render path seeks rather than decode-and-discards. - @somanshreddy separately verified that the measurement window equals the mixer's window, and re-ran the gain plan against the test vectors. I'm citing that rather than restating it — it's their verification, not mine.
- The muted-input, +12 dB ceiling and true-peak-before-write guards I checked last round are unchanged by this commit.
Merge order
Your plan is the right one, and it's still live on the audio bundle: main has hyperframes-audio at c96ccac4b1127b8c, this PR moves it to 6bdb36e157..., #3305 moves it to 1c01419737.... #3310's half already resolved itself — it landed, and #3305 regenerated on top of it in bc757a8d rather than hand-picking. Same treatment here for whichever of the two goes second.
Verdict: COMMENT — the code change is right and my nit is closed, but I'm not stamping a head where the required Test context never ran. Format the one file and I'll re-review; I expect to approve.
— Rames Jusso
…udio sections Lost when I resolved the rebase conflict against the background-preview docs by hand instead of letting the formatter near it. oxfmt --check failed on the one file, which fails Preflight — and because preview-parity needs Preflight it skipped, and the preview-regression gate fails closed on a skip, so a missing newline read as a preview defect. The quieter half: the same needs chain meant the required Test context was never created at that head. Not failing — absent, so there was no test signal at all on the PR.
|
The format failure was mine and the trace is exactly right — thank you for following the chain rather than stopping at the red check.
Two things from your read I want to acknowledge rather than gloss:
The absent Coverage gap — agreed, and it's the right one. The |
…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)
Measures two authored
<audio>clips with FFmpeg's integrated EBU R128 loudness and writes the target's matchingdata-volume, leaving the reference untouched. Dry run by default;--writepersists.Measuring the window the composition actually plays
Two ways to get this wrong, both of which produce a plausible-but-wrong gain:
data-endbounds a clip's timeline window just asdata-durationdoes. The parser, the runtime and the render mixer all honour it.-ss/-tbelong before-i. After it they bound the output, and with-f nullthere is no output worth bounding — ffmpeg keeps feeding the filter graph andebur128integrates audio the clip never plays.On a fixture whose played window is −61.8 LUFS inside a file that measures −27.9 LUFS whole, either mistake reports the wrong number and
--write"corrects" an already-matched clip by tens of dB.Writing safely
Two EBU R128 passes run between reading the composition and writing it, each bounded only by a two-minute timeout, and the skill docs tell agents to keep Studio open on the project meanwhile. The attribute patch is re-applied to a fresh read and written through a temp file and a rename, so a concurrent Studio edit is not reverted and a crash cannot leave a half-written composition.
Agent contract
Under
--jsonthe failures are documents too — an agent doingJSON.parse(stdout)on a bare error line throws. The command is registered in the curated--helplisting so an agent enumerating capabilities can discover it.A pair needing more than the +12 dB authoring ceiling has a source-file problem rather than a mixer one — mixer gain raises the noise floor with the signal — so the refusal names the remedy instead of just declining.
Stack
Based on
u1-audio-gain-core, whose shared gain module it imports. Retarget tomainbefore merging. Independent ofu2-studio-gain-surface; the two may land in either order.