Repository navigation
fix(audio): fit PCM to video frames before AAC delivery - #6089
Conversation
jrusso1020
left a comment
There was a problem hiding this comment.
Reviewed 990a09d. The shared frame clock and the retained-PCM peak correction remove the extra lossy generation. One caller was missed in the input migration.
[P2] Migrate the CLI diagnostic fixture to the frame clock. packages/producer/src/services/render/stages/audioStage.ts:37-40,91 changes the input from duration to the required totalFrames/fps. packages/cli/src/ui/renderDiagnostic.test.ts:259-263 still passes duration: 1 and neither new field.
What breaks:
- The "triggering seventeenth interruption" case now throws a
TypeErrorreadingfps.denbefore it reaches the mixer. - Its
catchreceives that ordinary error, sorenderFailureDiagnostichas noaudioFailures. - The 16-record and interruption assertions then fail. The CLI tsconfig includes this test under
src.
Fix: replace the old argument with the equivalent totalFrames: 30, fps: { num: 30, den: 1 } and keep the existing triggering-interruption assertions. Both production callers are migrated; only this test caller still needs the new contract.
Evidence:
- A probe running the actual transpiled stage reproduces the early
TypeErrorwith the old shape. The migrated-clock control passes. - An isolated TypeScript program using the actual interface and caller reports
TS2353: duration does not exist in AudioStageInput. - I did not run the whole CLI test suite or typecheck.
Other paths: no further production audio defect turned up in the local, plan or deferred audio handoffs, silence, assembly, or subprocess flag scope.
Native checks:
- The targeted scalar-AAC and fractional-duration tests passed (2 selected).
- A separate bundle of the actual encoder and runner produced identical repeated M4A bytes on x86.
- No full-suite pass is claimed.
- The ARM comparisons, the 13-minute project and the per-chapter loudness numbers are the author's evidence.
Review by Rames (hf-review)
Edit accuracy: accurate 271 (base branch 271), smooth 176 of thoseThe gate passes. Quarantined, measured but not gated (0) |
jrusso1020
left a comment
There was a problem hiding this comment.
Re-reviewed 17929a5. My previous P2 fixture finding is resolved, and I found no other concrete blocker. This supersedes review 5620605527.
The new changes are only the missed CLI fixture's migration from duration to 30 frames at 30/1 fps, plus a deleted encoder comment. Production behaviour is unchanged.
The real caller now satisfies AudioStageInput:
- Replaying the actual caller and stage with the real diagnostic helpers yields
ENCODER_INTERRUPTED, 16 bounded records, and the triggering SIGTERM first. - The old input shape still fails, which serves as the negative control.
- Isolated TypeScript validation reports no diagnostics.
- Hosted runtime logs at this head show
renderDiagnostic.test.ts's 13 tests passing.
Speed-ramp golden comparison: still red, not a rate bug. The check reports envelope correlation 0.59394043, with zero lag, an exact 4-second duration and passing visuals.
- A browser-free audio reconstruction reproduces that number exactly.
- Against the unchanged ramp PCM, the new output has sample cosine 0.9999925 and a fitted residual of −50.14 dBFS. The golden has 0.9998215 and −37.44 dBFS, so the new output is closer to the source.
- Measured true peak is −1.4 dBFS, and the roughly 1.07 dB gain increase over the golden is real.
- This fits the intended PCM-source correction. The golden's envelope varies by only about 1%, so Pearson correlation amplifies the small changes. A constant gain change alone wouldn't explain it.
- An approximate replay of the old correction restores the correlation to about 0.9985. That probe used whole-file peak measurement rather than the production fine-peak probe, so it isn't an exact replay of the old pipeline.
The golden baseline or metric needs updating before CI is green; I'm not presenting it as green.
The two native AAC tests are baseline evidence for unchanged production code, not reruns at this head. ARM byte matching and the 13-minute chapter comparisons are the author's evidence.
Review by Rames (hf-review)
8940fa4 to
1b9292c
Compare
jrusso1020
left a comment
There was a problem hiding this comment.
Re-reviewed exact 1b9292cb824b2c84d88c860b1f5976f34b0a1075 after approved 17929a5.
Need · Size · Reuse · Simpler: The three-file follow-up corrects the regression measurement itself and refreshes one speed-ramp golden. No production audio code changed in this successor.
computeAudioResidualRmsDb now disables amix normalization, so it measures actual subtraction rather than halving the difference, and keeps astats accumulation for the whole track instead of resetting per block. The new half-signal/half-silence control correctly expects -15.0515 dBFS; the old filter returns -inf and misses the mismatch. Subprocess/probe error handling is unchanged.
Validation: all five audioRegression tests passed locally with real FFmpeg. An independent old/new filter control confirms the silent-tail measurement. Both MP4 goldens decode as four-second stereo 48 kHz audio with 120 video frames; decoded video hashes match exactly, the new file matches its LFS pointer, and audio is not clipped. Only speed-ramp's MP4 changed among golden paths; the correlation threshold is unchanged. The CLI frame-clock finding remains repaired in unchanged code.
The previous round's PCM-source investigation supported the intended loudness correction and explained the old golden's poorer residual. This round does not independently establish ARM reproducibility, a fresh 13-minute project render or full native producer-harness acceptance. Speed-ramp still uses envelope correlation without opting into the optional residual gate; no defect in the changed helper was demonstrated.
— hf-review
Verdict: APPROVE
Reasoning: The measurement now detects the analytically known silent-tail mismatch, and the narrowly refreshed golden preserves video content. No new blocker in this delta.
What
Local and distributed renders now use one audio stage to fit the float mix to the video's exact frame duration, apply the encoded true-peak check from that mix, and deliver AAC for packet-copy assembly. AAC encoding uses scalar DSP for consistent output across CPU architectures. Regression residual measurements now cover the whole unscaled difference, including before a silent ending.
Why
The declared composition duration can differ from the duration of its whole video frames. Mixing to the declared seconds forced assembly to decode AAC and encode it again. That extra lossy generation changed peak correction and reduced integrated loudness by 1.5 LUFS in the native regression fixture.
Related work
This corrects audio delivery shared by local and distributed renders. It does not change video capture or video encoding.
How
runAudioStageowns the frame-count/rational-fps duration, retained float mix, encoded peak check, and PCM cleanup.Both plan entrypoints and local orchestration call that stage. The plan-only peak helper is removed; assembly receives fitted AAC.
Existing attenuation warnings and render metadata retain the measured gain reduction.
-cpuflags 0is set only on the canonical audio AAC encode command. Packet-copy muxing and video encode commands retain their existing flags. Existing frame-aligned parallel segmentation remains enabled; each segment starts from PCM.The regression residual checker disables mixer normalization and accumulates statistics over the whole track. A silent last block can no longer erase an earlier mismatch.
Test plan
Native duration witness failed before the fix: the default plan was 1,120 samples short of the required frame duration.
Native loudness witness failed on main with a 1.5 LUFS difference against a float-source reference. It must stay within 0.1 LUFS after assembly and below the delivery peak ceiling.
Scalar-argument witness failed before the flag change. It runs real FFmpeg, pins the audio encoder arguments, checks repeated output bytes, and checks the copy mux has no CPU flag.
Changed suites passed three serial runs on Linux. Neighbor suites, engine and producer typechecks, formatting, lint, fallow audit, package build and all six package budgets passed.
The real-project check runs final compile, probe, audio and assembly stages against a frozen project. Its picture comes from the corresponding cloud export and is copied without video encoding. Reusing those packets is valid because this diff changes only audio delivery. The measured candidate preceded the latest clip-gain change on main; the patch rebases cleanly onto that change and its native plan suite passed three further runs with rebuilt dependencies. The final post-rebase Vitest stage and local/distributed handoff checks also passed. The CLI diagnostic fixture uses the same frame clock; its 13 tests passed three serial runs, and the CLI typecheck passed. Both comment checks and all commit hooks passed. All seven chapters measured 0.0 LUFS difference at the meter's 0.1 LUFS reporting precision, within the 0.1 LUFS limit. Compressed video packet hashes matched.
Actual built-package checks passed on x86 and ARM through the default segmented caption route: two scalar AAC segment encodes followed by packet-copy assembly. With identical input PCM, the entire M4A was byte-identical across architectures. Native upstream PCM differs slightly between architectures, so this does not claim identical bytes for independently mixed projects.
The existing
speed-rampgolden predates the PCM correction. At the unchanged 16 kHz gate, scalar/golden correlation was 0.594, while scalar/non-scalar correlation was 0.9996. Gain-matched decoded RMS error against the retained float mix improved from -37.44 dBFS (old golden) to -50.14 dBFS (new delivery). Replaying correction from AAC reproduced the old golden's envelope at 0.9985 correlation. Only that fixture's MP4 golden is refreshed through the CI harness entrypoint, run natively with Node 22.23.3, Chrome 148.0.7778.167 and the exact pinned FFmpeg n8.0.1-66-g27b8d1a017 build. Three serial harness checks passed at 1.000 audio correlation, zero lag and zero failed visual frames. SHA-256 checks confirmed all 217 other fixture golden files stayed byte-identical. The original compiled HTML object is retained too, bringing the total of unchanged golden files to 218; the public compilation validator passed against that original snapshot without errors or warnings. The 16 kHz correlation threshold remains unchanged. Temporary Node and Chrome downloads were removed.The residual witness uses a 0.25 stereo signal for one second followed by one second of silence against silence. The old helper incorrectly reported negative infinity instead of the analytical -15.0515 dBFS. A deliberate averaging mutant reported -21.0721 dBFS and failed. The corrected native suite passed three serial runs.
Unit and native integration tests added/updated
Final package and real-project verification completed
Encoder test pins the audio-only scalar choice