Repository navigation
fix(producer): an export over the peak ceiling loses only what it must - #7246
Conversation
The delivery limiter cut every peak over -1 dBTP down to -3, a fixed 2 dB margin for its detector. It now limits at -1, measures the encode, and lowers the ceiling only by what that encode measured over.
There was a problem hiding this comment.
Verdict: REQUEST CHANGES · 3 blocker · 0 important · 0 nits · head e937e600
The pass logic is sound. Ceilings step down from -1 and stop at -3, and switching between the a/b scratch files keeps the last good limited source if a later pass fails. Renaming encodedFrom also keeps constrainAacTruePeak's whole-mix cut reading the source the AAC was actually encoded from. CI is red on three counts, all caused by this diff:
- blocker:
audioDeliveryLimit.integration.test.tsis not listed inpackages/producer/scripts/integration-tests.json.Producer: unit testsandProducer: integration testsboth fail ontest-classification.test.mjs:75("add each to scripts/integration-tests.json"). Until it is listed, the test named on thefails without the fix:line never runs in CI, so nothing in CI protects against the regression. - blocker:
Formatand everyPreflight (lint + format)fail onoxfmt --check, which flagsaudioDeliveryLimit.ts(the body says oxfmt is clean). The Windows render/tests and regression failures are downstream of that preflight. ↓ inline - blocker:
Fallow auditfails:limitDeliveryPeakshas a cognitive complexity of 17, over the 15 threshold. ↓ inline
Scope
Read audioDeliveryLimit.ts end to end, its caller guardDeliveryPeaks in audioStage.ts, and both test files. No earlier reviews. CI run 42039904327: Format, Producer unit/integration and Fallow audit failed; CodeQL was still running.
There was a problem hiding this comment.
Verdict: REQUEST CHANGES · 3 resolved, 0 open, 1 new · e937e60 → c1fbba8
- blocker (new): registering the integration lane makes
Producer: integration testsrun, and two existing tests insrc/services/distributed/plan.test.ts("stitched mp4 plans") now fail. They fail the same way in both CI runs on this head (42042315743, 42042346040):fits the audio length and peak…(plan.test.ts:1075): the stitchedaudio.m4amax_volumeis now-1and the test expects< -1. This assertion counted on the old 2 dB margin.keeps assembled loudness within 0.1 LUFS of the frame-fitted float delivery(plan.test.ts:1287): the assembled file is 0.5 LUFS from the reference (limit is 0.1). The reference goes throughconstrainAacTruePeakonly, while delivery now goes through the -1 dBTP limiter first. Build the reference throughlimitDeliveryPeaksand thenconstrainAacTruePeak, asguardDeliveryPeaksdoes, or explain why distributed assembly should differ from the frame-fitted delivery. Loosen the peak assertion to<= -1only if -1.0 is the intended sample peak.
There was a problem hiding this comment.
Verdict: APPROVE · 1 resolved, 0 open, 0 new · c1fbba8 → 66654cc
CI note
The loudness reference now goes through the same steps as delivery: 16-bit read, float fit, limitDeliveryPeaks, then constrainAacTruePeak. The 16-bit step matches the mixer, whose prepared tracks are pcm_s16le (audioVolumeEnvelope.ts:13, audioMixer.ts:813). Producer: unit tests and Producer: integration tests pass, including both stitched mp4 plans tests. The remaining red has nothing to do with the code: Catalog payloads (ffmpeg-static download failed 3 times) and Test (runtime) (BtbN ffmpeg download returned 504), which also turns the Test aggregate red.
…view Adopts heygen-com#7250 (SVG stroke drawing is not a dead stretch), heygen-com#7248 (contract check reads a timeline registered in a local script), heygen-com#7252 (comma-list selectors hidden by authored CSS), heygen-com#7238 (snapshot and preview show a clip on the render frame), heygen-com#7237 (stalled cloud upload retries) and heygen-com#7251 (per-scene SVG id namespacing), with prerequisite heygen-com#6829. Defers heygen-com#7246 (needs the delivery limiter from heygen-com#6940) and heygen-com#7232 (needs the WebKit decode-first pipeline), with conditions in the ledger. Watermark advances to b108f85. Co-authored-by: Miguel Ángel <miguel.sierra@heygen.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
What
An export whose audio peaks pass the -1 dBTP delivery ceiling now loses only what it takes to get under it. Before, every peak over the ceiling was pulled down to -3 dBTP, 2 dB more than the rule asks, so a loud mix came out quieter on every transient.
Why
A report: a -13.3 LUFS source came out quieter, about -1.9 dB on transients and recovering in 100-150 ms. That is the delivery limiter (
audioDeliveryLimit.ts, #6940): when the encoded mix's true peak passes -1 dBTP, it re-encodes the source through a true-peak limiter at -3 dBTP, a fixed 2 dB under the target because the limiter's 4x detector and the AAC encode can land over its ceiling. Its 80 ms release is the 100-150 ms recovery.Measured with the CLI render of the
audio-mux-parityfixture, its track replaced by a drum hit every 0.5 s at -13.4 LUFS with a true peak of +0.1 dBTP. A source like this must lose about 1.1 dB at its peaks to meet -1 dBTP; anything past that is the margin."At each hit" is the gain over the source in the first 30 ms of each hit; plain AAC of the same source, with no limiting, reads -0.4 to -0.5 dB there. The first three rows were measured on the devbox with the CLI 0.9.13 bundle and its ceiling edited; this PR's row on a second Linux box with this branch's CLI built from source.
How
constrainAacTruePeakstays behind it for what still gets over.limitPass; the loop only picks the ceiling.Time it adds
On a 10-minute mix over the ceiling: main's one pass at -3 dBTP took 116.9 s; this branch took 122.0 s, one pass that landed at -1.00 dBTP plus the extra peak read. A mix whose first encode still lands over takes one or two more passes of about the same length each.
Test plan
Unit tests added/updated
Manual testing performed
Documentation updated (if applicable)
Comments follow CONTRIBUTING.md "Comments": they say why, not what, and a bug fix says what the code must do and how to reproduce the bug
fails without the fix:
limitDeliveryPeaks on a real mix > brings the encode under -1 dBTP losing no more than 0.2 dB past it, the same bytes every time(real limiter in the headless browser, real AAC encode, a mix at +0.1 dBTP) fails with main's one pass at -3 dBTP restored (expected -3 to be greater than or equal to -1.2) and passes here; two runs give the same bytes.Unit: a mix whose encode lands under after the first pass is limited once at -1; each later pass lowers the ceiling by the measured excess, from the original mix, never past -3; a later pass that cannot run keeps the last good one.
3 runs in a row at the earlier head:
audioDeliveryLimit.test.ts,audioDeliveryLimit.integration.test.ts,audioTruePeak.test.ts(14 tests);audioPadTrim.test.tsunder bun (45). The real-file test is listed inscripts/integration-tests.json, so CI's integration job runs it;test-classification.test.mjspasses.tsc,oxfmt --checkandfallow auditclean at the pushed head.plan.test.ts"keeps assembled loudness within 0.1 LUFS of the frame-fitted float delivery": its reference now goes through the same peak limit and whole-mix cut as delivery, from the track read as 16-bit, as the mixer reads it. Built from the float file instead, the first AAC encode of this white-noise fixture read +2.7 dBTP against the delivered mix's +3.8 (a 0.4 dB swing on the same audio rounded to 16 bits), so the limiter took different steps and the two landed 0.6 LU apart. The 16-bit reference is bit-identical to the mixer's PCM (md5 of the float samples).At this head, on the devbox (the shared test box was down):
plan.test.ts3 runs (74 pass each);audioDeliveryLimit.test.ts,audioDeliveryLimit.integration.test.ts,audioTruePeak.test.ts,audioTruePeak.fakeFfmpeg.test.ts,audioStage.test.ts3 runs (39 each);audioPadTrim.test.tsunder bun 3 runs (45 each); the producer's unit lane (1196 pass, 1 skip) and integration lane (278 + 18 under vitest, the bun files all pass);oxfmt --checkand the comment ratchet clean.Not covered: a mix needing three passes on a real file (the unit tests drive that path), and the real-file test skips where no headless browser is installed, as the engine's browser tests do.