fix(render): trim AAC packet padding exactly#2531
Conversation
|
Pushed |
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at b82a21dd. Traced the padding-detection → trim-length → mux carrier boundaries end-to-end, checked the -avoid_negative_ts make_zero decoupling for the copy-M4A mux path, and dug into the failing regression-shards (shard-1 … style-3-prod) output to reason about whether this PR's mechanism actually addresses the observed failure.
Blockers
(none on code shape)
Concerns
🟠 The failing style-3-prod shard shows a 606ms audio-over-video drift, which is 30× larger than any single AAC packet (~21ms at 48 kHz / 1024 samples-per-frame). The regression-shards (shard-1 … style-3-prod) job at HEAD b82a21dd reports:
✗ Stream duration parity: FAILED (video: 16.07s, audio: 16.67s, drift: 0.606s > 0.5s)
❌ Test FAILED: style-3-prod
This PR's mechanism trace — "packet-copy -t target snaps to AAC frame boundaries, ~20ms over" → "atrim filter + PTS reset + re-encode to M4A gives exact presentation duration" — cleanly explains the ~21ms class of drift documented in the PR body (15.018667s → 15.000000s in the manual fixture). It does NOT explain a 606ms drift. Whatever is producing 606ms of extra audio for style-3 must be either:
- A path that doesn't invoke
padOrTrimAudioToVideoFrameCountat all. The failing job's assemble phase log showsdurationMs: 399for a 16.04s composition withhasAudio: true, audioCount: 5, workerCount: 1, forceScreenshot: true. 399ms is likely too short for a full decode /atrim=duration=16.04/ re-encode of 16 seconds of audio at 48 kHz (which typically wall-clocks in the 1–3s range on this class of runner). If the trim branch ran, its wall-clock should be visible. - A path routing mismatch — the PR changes both writer (
padOrTrimAudioToVideoFrameCount's outputPath) and every reader I could find (assembleStage.ts:64,distributed/assemble.ts:294,assembleStage.test.ts) from.aacto.m4a. If a third caller (streaming-encode assemble path? cloud renderer? chunkEncoder inline mux?) still reads a.duration-normalized.aacpath, or the atrim'd.m4anever actually gets fed into the mux command, the muxed output would carry the pre-normalized (untrimmed) audio — which would explain 5 audio tracks × their concat producing ~600ms of AAC-packet-padding cumulative drift rather than the ~21ms of a single trim boundary. - A pre-existing style-3 issue unrelated to this PR that has been latent-then-tripped by the
-avoid_negative_tschange or the M4A edit-list preservation. This is worth ruling out by running style-3-prod against fresh main first — if main is also >0.5s drift on this fixture today, then this PR is not the regression cause and the merge gate can shift to "PSNR / audio-correlation still pass on the failing head, drift is a pre-existing baseline miss." (Recent main regression CI runs aresuccess, so pre-existing is unlikely — but worth the 30-second empirical check before assuming this PR broke it.)
Recommended follow-up: dump ffmpeg's atrim invocation stderr on the style-3-prod render — if the filter didn't actually run (path routing / assemble-branch skip), that log will be silent for the trim step. If the filter DID run and still produced 16.67s output, then atrim=duration=X isn't behaving the way the PR body assumes on this input shape (likely candidate: multi-input concat / mixed sample rates producing a filter graph where atrim receives already-mixed input past the target). Either finding sharpens the fix.
Nits
🟡 buildPadTrimAudioPlan trim branch loses sourceDurationSeconds context in the emitted command. The new args are:
[
"-i", audioPath,
"-af", `atrim=duration=${targetSec},asetpts=PTS-STARTPTS`,
"-c:a", "aac",
"-b:a", "192k",
"-y", outputPath,
]If a caller passes sourceDurationSeconds < targetDurationSeconds by accident (should be caught by the delta check upstream, but defense-in-depth), atrim=duration=X where X > source would silently succeed and produce a track SHORTER than X (only source-duration long). Not a blocker — the upstream delta > tolerance check should prevent this and the pad branch handles source-shorter cases correctly — but a follow-up assertion in the tests (sourceDuration >= targetDuration when operation === "trim") would pin the invariant.
🟡 copiesContainerizedAac gate keys on extension string, not container type. extname(audioPath).toLowerCase() === ".m4a" — if a caller ever produces .mp4 output from padOrTrim (mp4 containers can carry AAC and edit lists just like m4a), the make_zero suppression won't fire. Today no caller emits .mp4 from padOrTrim, so this is theoretical. If the container-choice ever expands (e.g. streaming-encode also containerizes AAC), the gate needs to broaden. Consider [".m4a", ".mp4"].includes(...) or an explicit carriesEditList: boolean field on the padOrTrim result.
Questions
Where does style-3's 5-track audio pipeline concat / mix? Before or after padOrTrimAudioToVideoFrameCount? The renderJob b619fc56's audio_process phase completed with audioCount: 5, durationMs: 989. If those 5 tracks are extracted individually and each padded/trimmed to per-track duration BEFORE mixing, the mix step's -shortest / -longest behavior determines the final duration — and post-mix, padOrTrim on the final track is the last chance to hit the video target. If padOrTrim runs on the mix (as the code path I traced suggests), then the observed 16.67s must come from AFTER padOrTrim ran — either its output wasn't muxed, or the target-duration probe hit a different frame count than 16.04s × 30. Worth a data-dump on targetDurationSeconds / sourceDurationSeconds for style-3 to know which branch fired.
Green notes
🟢 The atrim-then-re-encode shape is the correct fix for the ~21ms packet-copy class. atrim=duration=X operates on decoded PCM samples inside the filter graph — sample-accurate cut at exact PTS, not packet-accurate. asetpts=PTS-STARTPTS resets the timeline so downstream containers record from t=0 with no leading gap. Re-encoding to M4A gives ffmpeg an exact presentation duration in the container's edit list. Every step of that chain contributes to sample-exact duration.
🟢 -avoid_negative_ts make_zero suppression for containerized-AAC copy is well-reasoned. The docstring on the guard captures the mechanism: M4A carries an edit list that hides the AAC priming packet (typically 2112 samples ≈ 44ms at 48 kHz, but often 1024 samples ≈ 21ms). make_zero discards that edit and shifts copied video forward by exactly the priming duration, producing a visible first-frame offset. Suppressing the flag for this specific copy path preserves the edit list intent. The gate is narrow (!isWebm && shouldCopyAudio && ext === ".m4a"), so other mux paths keep the flag.
🟢 Regression test in chunkEncoder.test.ts pins the -avoid_negative_ts invariant. it("preserves M4A priming edit lists instead of shifting copied video", ...) explicitly asserts .not.toContain("-avoid_negative_ts") in the emitted args. Clean guard against a "just always add the flag" refactor. Symmetric with the existing "uses caller-provided AAC codec contract" test at line 424.
🟢 Audio-mux parity harness test at packages/producer/tests/audio-mux-parity/output/output.mp4 passed (321645 bytes recorded). That's the narrow case the PR was designed to fix (a single-track mixed audio going through the trim branch). So the PR does what it says on the tin for the shape captured by that fixture — the style-3 failure is a wider class the fixture didn't cover.
🟢 buildPadTrimAudioPlan docstring / mechanism trace is honest. The updated block documents WHY packet-copy is being replaced ("AAC frames are independently decodable [old]; packet-copy trimming snaps to AAC frame boundaries [new]") and calls out the ~20ms bound. Future maintainers can find the reasoning from the code without digging in git-log for the PR body.
🟢 Path renames in test fixtures are consistent. assembleStage.test.ts updates "/tmp/audio.duration-normalized.aac" → .m4a in every position (mock resolve value, expect.toHaveBeenCalledWith for both padOrTrim and muxVideoWithAudio, and the fail-path fixture). No stale .aac reference could pass a mock check that the writer/reader have diverged.
What I didn't verify
- Didn't stand up a full local render of style-3-prod to reproduce the 606ms drift and empirically split the three hypotheses (unrun trim / path routing miss / pre-existing baseline). This is the highest-leverage next step; the CI log gives the drift number but not the atrim-invocation stderr.
- Didn't grep for a third caller of
padOrTrimAudioToVideoFrameCountor the.duration-normalizednaming convention across the whole repo. The two callers I updated in the PR (assembleStage.ts,distributed/assemble.ts) match every hit I saw, but I didn't do a byte-exhaustive sweep. - Didn't check whether streaming-encode / chunked-encode audio paths go through
runAssembleStageOR through a separate assemble routine that bypassespadOrTrimAudioToVideoFrameCountentirely. style-3-prod's log showsstreaming-encode gate {enabled:true, ...}+workerCount:1+chunkedEncode:false— the interplay between streaming-encode-enabled and chunkedEncode-false is worth confirming; if streaming-encode has its own assemble path that doesn't call padOrTrim, the entire fix is unreachable for streaming-encode compositions (which is basically every high-res prod render). - Didn't try to reason about whether the atrim filter changes the sample rate / channel-layout of the output — if the input has an unusual layout,
atrim+192kAAC re-encode might not preserve it perfectly, but the audio-correlation check (which passed at 1.000 with lag 0 for the two other jobs in this shard) suggests decode-side is fine.
Merge gate — CI is red
regression-shards (shard-1, hdr-regression style-5-prod style-3-prod mov-prores) is FAILURE at HEAD b82a21dd; approval is correctly held per Magi's own framing. The stream-duration-parity threshold (0.5s) is what fires. The other two jobs in that shard (hdr-regression, style-5-prod) both pass audio correlation and visual quality — only style-3-prod hits the stream-duration check red. Sibling shards (shard-2…shard-8) were cancelled after shard-1's failure; they need to complete on the corrective head to prove no wider regression.
Do NOT merge until regression-shards is green. The code shape is defensible for the class it targets, but the concrete failing counterexample is not addressed. Recommend either (a) a follow-up commit that shows an atrim invocation log for style-3-prod and either narrows the root cause or exposes a third path that also needs the .m4a routing, or (b) landing this PR gated on a second PR that addresses style-3 specifically.
Verdict framing
The mechanism trace and code shape are correct for the ~21ms packet-copy class of drift. The M4A copy-mux -avoid_negative_ts decoupling is a valuable adjacent fix documented clearly. But the empirical failure at HEAD reports drift 30× larger than the class this PR targets — the fix likely doesn't reach style-3's failure mode, and the merge gate correctly enforces that. Ready-from-my-side on the code delta; blocked on the concrete counterexample being resolved either here or via a follow-up.
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Adversarial R1 review. Verdict: CHANGES_REQUESTED. No prior reviews on the PR — this is the first pass.
Summary
Root-cause approach is right (filter-trim + re-encode into M4A + preserve edit list at mux) — not a band-aid on a downstream muxer step. On style-5-prod the mechanism visibly works (drift dropped 0.005s → 0.001s across the two commits). But on the same fixture the PR body claims went RED→PASS, style-3-prod is now RED with a much larger drift than base main showed:
| video | audio | drift | status | |
|---|---|---|---|---|
Base main 21cb722 |
16.07s | 16.11s | 0.040s | PASSED |
Head b82a21dd (this) |
16.07s | 16.67s | 0.606s | FAILED |
Audio_correlation is identical on both runs (0.9539), so it's the same signal — the pipeline is emitting 560ms more audio than before the fix. That is not "the assertion is tightened"; the output is objectively longer. This won't be resolved by CI turning green — the head reproduces 0.606s drift deterministically on both 1ca01819 and b82a21dd, and the second commit only moved style-5 (5ms→1ms) and had no effect on style-3.
Findings
blocker — style-3-prod: 40ms → 606ms drift (audio +560ms)
Run on head b82a21dd, shard-1, packages/producer/tests/style-3-prod:
✗ Stream duration parity: FAILED (video: 16.07s, audio: 16.67s, drift: 0.606s > 0.5s)
(log: actions/runs/29482258384/job/87568690623 line 2239)
Base 21cb722 same fixture:
✓ Stream duration parity: PASSED (video: 16.07s, audio: 16.11s, drift: 0.040s)
(log: actions/runs/29477616277/job/87554113154 line 1818)
packages/producer/src/services/render/audioPadTrim.ts:186-207 (new trim branch) is expected to bound output at targetSec. On style-3 with source ≈16.11s and target ≈16.07s, delta = -40ms → outside the 1ms tolerance at audioPadTrim.ts:132, so the trim plan should fire. Yet the produced audio is 16.67s, i.e. ~600ms past the atrim duration=. Some interaction of atrim=duration=16.070000,asetpts=PTS-STARTPTS + -c:a aac -b:a 192k + M4A container is producing extra audio on this fixture. Manual FFmpeg reproduction against the style-3 audio.aac before merge is needed. Do not resolve by widening the parity threshold.
blocker — PR base is stale w.r.t. #2525
#2525 (merged 2026-07-16T07:35Z, ~19 min after this PR was opened) rewrote concatFileLine to emit bare paths + materialized concat scripts, killing the Windows file:///C:/… failure and the Linux pipe:/tmp/… failure. packages/producer/src/services/render/audioPadTrim.ts at head b82a21dd still carries the pre-#2525 shape:
audioPadTrim.ts:24—import { pathToFileURL } from "node:url";audioPadTrim.ts:170-178— pad-concat step still-i pipe:0with astdinconcat scriptaudioPadTrim.ts:248-250—concatFileLinestillpathToFileURL(path).href
Git 3-way merge will likely keep main's concatFileLine (no overlap with the trim-branch changes), but CI on this PR was run against pre-#2525 world — style-3's failure is on a base that main no longer looks like. Rebase onto latest main and re-run regression before any verdict-worthy read of the CI signal. If style-3 clears post-rebase, we still need to understand why the pre-rebase run failed — a silent fix by rebase points to a pipeline sensitivity the trim path shouldn't have.
blocker — extension-based muxer gate applies to pad + no-op branches too, without an edit list to preserve
packages/engine/src/services/chunkEncoder.ts:789-793:
const copiesContainerizedAac =
!isWebm && shouldCopyAudio && extname(audioPath).toLowerCase() === ".m4a";
...
if (!copiesContainerizedAac) args.push("-avoid_negative_ts", "make_zero");
The .m4a extension is now written by all three operations in buildPadTrimAudioPlan:
- trim (
audioPadTrim.ts:186-207) — real M4A edit list, gatingmake_zeroOFF is correct - pad (
audioPadTrim.ts:139-182) — concat-copies AAC packets into.m4a(no re-encode → no priming edit) - no-op copy (
audioPadTrim.ts:132-138) — bare-c:a copyfrom source AAC into.m4a(no edit)
For the pad and no-op paths the gate flips OFF make_zero on the muxer even though there is no edit list to preserve. That's the exact opposite of the safety -avoid_negative_ts make_zero provides against negative-DTS after concat-copy. This is a plausible contributor to the style-3 regression (if style-3 hit pad or copy rather than trim under any code path) and is a live footgun regardless. Options: (a) gate on the trim operation actually having fired (thread operation through, not the extension), or (b) keep pad/no-op writing to .aac and only trim to .m4a.
The comment at chunkEncoder.ts:790-792 claims the edit list "already accounted for AAC priming" — that assumption only holds for the trim branch. extname === ".m4a" is a decorative proxy for "was just re-encoded" that will lie for pad and copy outputs.
important — regression test locks argv shape, not output duration
packages/producer/src/services/render/audioPadTrim.test.ts:69-82 asserts the ffmpeg argv (atrim=duration=15.000000,asetpts=PTS-STARTPTS, -c:a aac, -b:a 192k, .m4a). It doesn't run the plan against a real audio fixture and doesn't assert on the resulting file's ffprobe duration. That's the failure mode the CI regression is surfacing (argv looks correct; output is 600ms too long). Add a fixture-level integration test that pins ffprobe(out.m4a).duration ≤ targetSec + AUDIO_DURATION_TOLERANCE_SECONDS.
important — PR body claim contradicts CI
"Focused regression observed RED before implementation and PASS after implementation."
If the "focused regression" was style-3-prod, it's currently RED at head. If it was something else, that should be named in the PR body — the failing check name is regression-shards (shard-1, hdr-regression style-5-prod style-3-prod mov-prores), and reviewers need to know whether the intended-fix regression is one of those or lives elsewhere.
nit — 5-surface consumer audit is clean
padOrTrimAudioToVideoFrameCount is called from packages/producer/src/services/render/stages/assembleStage.ts:65 (local) and packages/producer/src/services/distributed/assemble.ts:295 (distributed). Both updated to .m4a. Lambda + cloudrun surfaces route through distributed/assemble.ts, so the change reaches all render surfaces via those two entrypoints. No dead half-plumbed surfaces here. The #2525 file://-vs-bare-path concern is orthogonal to this fix's intent and only surfaces because of the stale base (above).
Recommendation
Do not stamp until:
style-3-prodregression is diagnosed and closed on the fresh head — not by threshold-widening.- Base is rebased onto latest main so #2525's
concatFileLinereconciliation is explicit, and regression is re-run. extname === ".m4a"gate atchunkEncoder.ts:789is either narrowed to "trim operation ran" or the pad/no-op branches keep the.aacextension so the gate stays precise.- Argv-only regression test is complemented by an actual-duration assertion against an ffprobed output.
Miguel — root-cause direction is right, and I'd expect this to converge quickly once style-3 is reproduced locally. The 606ms delta is much too large to be encoder priming; there's a real mechanism to find.
Verdict: REQUEST CHANGES
Reasoning: Head produces a 606ms audio-lengthening regression on style-3-prod (base was 40ms), and the extension-based copiesContainerizedAac gate at chunkEncoder.ts:789 skips negative-DTS repair on pad/no-op branches that have no edit list to preserve. Both blockers need addressing before green CI would authorize a stamp.
— Via
|
Cross-posted with @james-russo-rames-d-jusso — my review went up 2 minutes after his and I hadn't fetched. Manual peer-lens split so Miguel doesn't have to reconcile: Where we overlap: 606ms drift on style-3-prod is 30× the packet-copy class this PR targets; the argv-shape regression test doesn't cover the actual failure; the Adds from my pass (not in Rames's review):
Adds from Rames's pass (not in mine): the 399ms Position stays CHANGES_REQUESTED on my side vs. Rames's COMMENTED — divergence axis is the extension-based gate on pad/no-op branches. If pad/no-op stay writing to — Via |
b82a21d to
d92c236
Compare
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Delta review at d92c236e (from R1 head b82a21dd — GC'd, structurally compared against the current diff). Scoped to the two follow-up commits 82537eb0 (edit-timing preservation) and d92c236e (explicit trim-only scoping).
Blockers
(none on code)
Concerns
(none)
Nits
(none)
Green notes
🟢 The preserveAudioPrimingEditList scoping is exactly the right fix and directly resolves my R1 nit about the extension-string gate. R1 flagged that extname(audioPath).toLowerCase() === ".m4a" was a fragile signal for "this is a freshly-encoded AAC file whose priming edit list must be preserved." The old shape swallowed ALL .m4a outputs — including pad-branch concat outputs and copy-branch containerized AAC — regardless of whether they carried a fresh priming edit. The new shape lifts the decision out of the muxer's file-name inspection and into the caller, which knows for certain whether the audio it's handing over is a fresh trim/re-encode or something else. Explicit contract, no extension guessing, type-safe.
🟢 This likely also closes the style-3-prod 606ms drift concern I raised in R1. My R1 hypothesis (b) was "a third caller routes to a path that gets the wrong suppression." The failure mode I was reaching for was subtler than I realized: padOrTrimAudioToVideoFrameCount writes ALL three operation branches (copy / pad / trim) to a caller-provided outputPath, and both callers I traced (assembleStage.ts line 76's normalizeResult.outputPath → .m4a, distributed/assemble.ts:294's paddedAudioPath = join(workDir, "audio-padded.m4a")) hand back .m4a regardless of operation. That means the R1 head was suppressing -avoid_negative_ts make_zero on pad-branch outputs too — including the pad branch's concat of a silence tail with the source audio, which does NOT carry a fresh AAC priming edit and needs the DTS repair. Any small PTS discontinuity from the silence-concat then propagates through the mux unrepaired, and on a 5-track composition (style-3-prod's audioCount: 5) can accumulate to hundreds of ms of drift. The d92c236e gate flips exactly this: pad-branch → preserveAudioPrimingEditList: false → mux keeps -avoid_negative_ts make_zero → concat drift suppressed. Mechanism-plausible, but empirical proof waits on regression-shards green.
🟢 Test coverage is a clean RED→GREEN pair. The old single test (preserves M4A priming edit lists instead of shifting copied video) is split into two symmetric tests:
keeps negative-timestamp repair for an M4A without a known priming edit list— asserts-avoid_negative_tsIS in args when the caller does not opt in (default false).preserves a known M4A priming edit list instead of shifting copied video— asserts-avoid_negative_tsis NOT in args when the caller setspreserveAudioPrimingEditList: true.
Two-sided coverage locks the invariant on both sides of the branch. A future refactor that always sets or always drops the flag would fail one test or the other.
🟢 Caller-side pairing is correct at both invocation sites.
assembleStage.ts:76-82:preserveAudioPrimingEditList: normalizeResult.operation === "trim"— only the trim operation carries the flag.distributed/assemble.ts:293-325: same pattern (preserveAudioPrimingEditList = padTrimResult.operation === "trim"), threaded through to the mux call. Neither caller synthesizes the value from.m4aextension or naming convention; it flows directly from the pad-or-trim result. Correctly decoupled.
🟢 Interface is scoped to what the muxer actually needs to decide. MuxVideoWithAudioOptions.preserveAudioPrimingEditList?: boolean with a clear docstring ("Preserve a priming edit list known to have been created by AAC re-encoding."). Optional with implicit default of false — safe on all existing callers (preserveAudioPrimingEditList !== true picks up the negative-TS repair). No behavioral change for any caller that doesn't opt in.
🟢 Comment on the copy-M4A guard is preserved from R1 through the refactor. The 82537eb0 commit's docblock ("A freshly encoded M4A is the exception: its edit list already hides the AAC priming packet. \make_zero` discards that edit and shifts copied video forward by one AAC frame (~21ms), creating a visible first-frame offset.") survives into d92c236above the gate. Future maintainer looking atif (!copiesContainerizedAac)gets the mechanism trace inline; the newpreserveAudioPrimingEditList` variable name reinforces the intent.
What I didn't verify
- Didn't re-run
regression-shardslocally to confirm the 606ms drift on style-3-prod is empirically gone atd92c236e. CI is currentlyIN_PROGRESSonregression-shardsat time of review; that job's outcome is the definitive test of the mechanism trace above. - Didn't audit whether the streaming-encode / chunkedEncode assemble path (surfaced as R1 hypothesis (a)) routes through the same
padOrTrimAudioToVideoFrameCountand thus benefits from this fix.style-3-prod's log linestreaming-encode gate {enabled:true, workerCount:1, chunkedEncode:false}suggests it does share the assemble stage, but I didn't grep exhaustively for a bypass path.
Merge gate — CI still running
Required checks in progress at d92c236e:
Test,Typecheck,Producer: unit tests,Producer: integration tests,SDK: unit + contract + smoke,Studio: load smoke,Test: runtime contract,CLI smoke (required),Build,Fallow audit— allIN_PROGRESSon the CI workflow.regression-shards(the shard that failed at R1 head with the 606ms drift) is not yet visible in the rollup, but is expected to be re-triggered by the new head.
reviewDecision: CHANGES_REQUESTED is stale from the R1-era peer review; that will need to be dismissed or re-reviewed on this head before the merge state clears.
Do NOT merge until regression-shards reports green on d92c236e. The mechanism trace is now defensible on both sides (the extension-check-driven over-suppression → pad-branch drift theory closes the loop cleanly), but the concrete counterexample from R1 needs to actually be shown resolved.
Verdict framing
Delta LGTM from my side. d92c236e is a well-targeted scope narrowing that both closes the R1 nit and plausibly reaches the R1 concern's failure mode. Waiting on regression-shards green to confirm the 606ms drift is gone at HEAD.
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
R2 verification at d92c236e — re-verified each of my R1 blockers independently against the new head, ran the full adversarial pass on the three follow-up commits, and cross-checked against Rames's delta R1 + delta R2. Two of three blockers are cleanly resolved in code; the third — style-3-prod parity — is still failing at HEAD with byte-identical drift numbers to R1.
Per-blocker verdict
B2 (stale base w.r.t. #2525) — ✅ resolved
gh api /compare/main...d92c236ereportsbehind_by: 0, ahead_by: 3. Rebase caught up cleanly.packages/producer/src/services/render/audioPadTrim.tsatd92c236e: nopathToFileURL, nopipe:0-fed concat.-i concatListPathataudioPadTrim.ts:174-175,-i audioPathat:197-198,concatFileLine(audioPath)writing bare paths at:182. The R1 head'spathToFileURL/pipe:0shape is absorbed away by the rebase over #2525.
B3 (copiesContainerizedAac .m4a extname footgun) — ✅ resolved
packages/engine/src/services/chunkEncoder.ts:794-795: gate is nowThe extname-only signal is gone. Default isconst copiesContainerizedAac = !isWebm && shouldCopyAudio && config?.preserveAudioPrimingEditList === true;
false.chunkEncoder.ts:801:if (!copiesContainerizedAac) args.push("-avoid_negative_ts", "make_zero");— negative-DTS safety fires on every path that doesn't explicitly opt in.- Callers thread the flag correctly from the pad-or-trim operation only:
packages/producer/src/services/render/stages/assembleStage.ts:81—preserveAudioPrimingEditList: normalizeResult.operation === "trim".packages/producer/src/services/distributed/assemble.ts:305,325—preserveAudioPrimingEditList = padTrimResult.operation === "trim", threaded through to the mux at:325.
- Pad and copy branches →
false→-avoid_negative_ts make_zerostill applied. Fresh trim/re-encode →true→ priming edit list preserved. Test coverage inchunkEncoder.test.tsis a tight RED→GREEN pair on both sides of the branch (line-435 asserts-avoid_negative_tsIS in args without the flag, line-460 asserts it is NOT in args with the flag). This is exactly the shape I asked for at R1.
B1 (style-3-prod parity regression) — ❌ still open (blocker)
regression-shards (shard-1, hdr-regression style-5-prod style-3-prod mov-prores) completed at 15:16:08Z on d92c236e with conclusion: failure. From the job log:
✗ Stream duration parity: FAILED (video: 16.07s, audio: 16.67s, drift: 0.606s > 0.5s)
{"event":"audio_comparison_complete","suite":"style-3-prod","passed":true,
"correlation":0.9539530103009182,"lagWindows":0,"residualRmsDb":null}
This is byte-identical to the R1 head b82a21dd signature: same video 16.07s, same audio 16.67s, same 606ms drift, same correlation 0.9539530103. Neither the M4A trim mechanism refactor (82537eb0) nor the extname → explicit-flag scoping (d92c236e) moved the numbers.
That empirically falsifies Rames's plausible mechanism trace — the theory was "extname gate over-suppressed -avoid_negative_ts on the pad-branch concat; scoping it to trim only should recover the DTS repair on pad." The scoping is now correct in code (verified in B3 above), pad-branch on style-3-prod's assemble now flows preserveAudioPrimingEditList: false → -avoid_negative_ts make_zero fires — and the drift is unchanged. The 560ms audio surplus is originating somewhere else: probably the pad or trim step itself, or the pre-assembly audio mixer that emits audio.aac, not the final mux flag.
For reference, base main at 21cb722 passed the same shard at 40ms drift with correlation 0.954. So the regression started with something in this PR's diff, but the B3 fix is not the mechanism.
Adversarial notes on what to look at next:
padOrTrimAudioToVideoFrameCountin the new head can now emit.m4awhen the pad branch fires (assembleStage.test.tsfixtures moved/tmp/audio.duration-normalized.aac→.m4aat every callsite). But the pad branch still does an AAC-copy concat (audioPadTrim.ts:168-183,kind: "pad-concat",-c:a copy), not a re-encode. Copying AAC packets into an.m4ashell without a fresh encoder-priming interval risks the same class of duration-recording drift the atrim-then-re-encode fix was designed to close for the trim branch. Worth probing whether style-3-prod is hitting the pad branch and whether the.m4acontainer is recording the sum-of-packet duration instead of the concat script's target.audioPadTrim.ts:139— the no-opcopybranch also now writes intooutputPathwhich the callers made.m4a. Same concern:-c:a copyinto.m4awithout a priming edit could produce a container whosedurationmetadata is packet-derived rather than sample-exact.- Rames's R1 hypothesis (a) — streaming-encode / chunkedEncode bypass — is closed by inspection:
renderOrchestrator.ts:3157-3170dispatches Stage 6 unconditionally throughrunAssembleStage, and the style-3-prod trace showsstreaming-encode gate {enabled:true, ...}on the same job that then hits the assemble stage. Both encode strategies funnel through the fixed callers. Not the miss. - Rames's R1 hypothesis on 399ms
durationMsevidence trim not running — the R2 fix DID change-t <sec>packet-copy trim to-af atrim=duration=<sec>,asetpts=PTS-STARTPTSwith-c:a aac -b:a 192kre-encode into.m4a(audioPadTrim.ts:191-210, verified against the test patch inaudioPadTrim.test.tsandassembleStage.test.ts). That's the right structural change for AAC packet-padding elimination. But it didn't move style-3-prod's numbers, so either that path isn't the one exercised by style-3-prod, or the mechanism is downstream.
Rames's concerns roll-up
- Extension-string gate → ✅ closed by B3.
- Streaming-encode / chunkedEncode bypass → ✅ closed by inspection (both funnel through the fixed callers).
- 399ms trim evidence / atrim re-encode → ✅ mechanism correctly refactored (audioPadTrim.ts:191-210), but empirically not the fix for style-3-prod.
- Mixed-sample-rate atrim → not verified this round; parking as follow-up since the parity failure is the dominant signal.
Non-blocker green notes
chunkEncoder.test.tsRED→GREEN pair is exactly the shape B3 asked for. Both value-side (flag presence/absence) and reachability-side (audioCodec: "aac"→shouldCopyAcasSidecarreturns true → gate active) are locked.assembleStage.test.tsnow assertspreserveAudioPrimingEditList: trueis passed on the trim path. Good coverage on the invariant.- Docstring at
chunkEncoder.ts:798-800preserving the mechanism trace inline is a durable win for the next maintainer.
Verdict
Rebase-resolution correctness: ✅ clean (per rebase-clean-ci-red-new-base discipline — the byte-level file audit at both SHAs shows the resolution introduced no drift beyond the intended fix commits).
Merged-tree CI health: ❌ red on regression-shards shard-1 (style-3-prod). The one shard that mattered to B1 completed with the exact same failure signature as R1.
Verdict: REQUEST CHANGES
Reasoning: B2 and B3 are cleanly resolved in code, but B1's style-3-prod 606ms parity regression reproduced byte-identically at d92c236e. This isn't a mitigation-accepting case per the r2-verdict-mitigation-vs-full-resolution discipline — the blast radius is unchanged, and the mechanism theory driving the code changes was empirically falsified by the CI shard.
— Via
|
New Windows field signal maps directly to this open audioPadTrim/AAC-padding work:
Source: https://heygen.slack.com/archives/C0BGC335AQY/p1784220443405999 This is additional evidence for probing the pad-concat/no-op branches rather than only the trim re-encode branch. No duplicate PR created; #2531 remains the owning fix and is still blocked by the existing style-3 parity regression. |
d92c236 to
0096018
Compare
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
35fe1e7 to
b6f53ba
Compare
|
Resolved the outstanding Root cause was not a trim miss or the final mux threshold. The pad path concat-copied raw ADTS AAC with a tiny generated silence segment. FFmpeg's concat demuxer estimated the raw ADTS duration from bitrate and timestamped the silence segment about 606ms late; the final MP4 therefore carried an oversized terminal AAC packet/timeline gap. The corrective delta:
Verification:
The prior CHANGES_REQUESTED reviews were against |
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
R2 exact-head verify at 8aa09a8e2. Re-read audioPadTrim.ts, both test files, assembleStage.ts, assemble.ts, and chunkEncoder.ts at this SHA; cross-checked each of the four R2-fix claims against the actual bytes. Verdict: APPROVED — root-cause fix, no band-aid, regression-shards shard-1 (style-3-prod) green at this SHA.
Verified
Root cause identified and fixed. The prior head's pad branch generated a tiny raw-ADTS silence tail via anullsrc and stitched it via concat + -c:a copy. The concat demuxer's terminal-packet PTS is bitrate-estimated on raw ADTS input, not sample-counted — with a ~26ms pad ask and 192 kbps AAC, that estimator can round the last packet's presentation up by ~600ms in the muxed MP4. Verified by reading the old pad-silence + pad-concat step pair in the pre-fix source and confirming both are gone at head.
Fix is on the correct axis. Both branches at head use filter-graph, sample-counted duration + a container-boundary -t cap + AAC re-encode:
- Pad (
audioPadTrim.tsL139-155):-af apad=whole_dur=<targetSec>+-t <targetSec>+-c:a aac -b:a 192k.apad=whole_durcounts samples through the filter graph, and-tbounds the muxer at the container boundary. Belt + suspenders in the right places. - Trim (L165-186):
-af atrim=duration=<targetSec>,asetpts=PTS-STARTPTS+-t <targetSec>+-c:a aac -b:a 192k. Comment in-source correctly notes thatatrimalone doesn't cap muxer timestamps (encoder delay/flush can persist beyond samples), so the-tat the container boundary is the second lock.
Dead concat mechanism removed — the old pad-silence / pad-concat step kinds, writeFileSync for the concat script, sampleRateForFilter / channelLayoutForChannels helpers, and both concatListPath / concatListContent fields are all gone. The PadTrimAudioStepKind union narrowed from four kinds to three (copy | trim | normalize). No compensating post-hoc offset anywhere — this is a root-cause replacement, not a band-aid on the drift.
Edit-list preservation is the right complement, not a workaround. chunkEncoder.ts L693-703 conditionally drops -avoid_negative_ts make_zero when the mux is copy-audio on freshly re-encoded M4A. The comment nails the reason: the M4A edit list already hides AAC priming; make_zero would discard that edit and shift copied video by ~21ms. The preserveAudioPrimingEditList flag is threaded through assembleStage.ts L79 and assemble.ts L306-329 gated on padTrimResult.operation !== "copy". This is a real correctness constraint of the re-encode approach, not a hack.
Real-media regression covers the failure mode. audioPadTrim.integration.test.ts synthesizes a 16.04s raw-ADTS AAC input, drives the pad branch to a 16.0667s target (~26ms delta — the exact shape of the prior style-3-prod failure), and asserts (a) output duration ∈ [16.0, 16.1] s (would catch a 606ms overrun) and (b) packet.duration_time on the terminal packet < 0.1s (would catch a bitrate-PTS-estimated fat tail). CI shows regression-shards (shard-1, hdr-regression style-5-prod style-3-prod mov-prores) PASS at this head.
Non-blocking follow-ups (not gating)
audioPadTrim.tsL217-238 — theconcatFileLinefunction body was retained as a bare/* … */block comment after the function itself was deleted. Now-orphaned documentation for a mechanism the file no longer has; safe to remove in the next janitor pass.audioPadTrim.test.tsL28 — the firstbuildPadTrimAudioPlan(...)call still passes a 5th arg{ sampleRate: 48000, channels: 2 }. The function signature at head only takes four params; the extra positional is silently dropped at runtime (Typecheck CI passes, so this file's tsconfig must be lenient enough not to flag it). Dead argument — trim for hygiene.- PR body drift — the
## Howsection says "Pad and no-op branches continue to stream-copy audio," but the pad branch at head re-encodes AAC. Description reads like it was written before the pad branch was also converted; the code is correct, the copy is stale. Worth fixing for future spelunkers.
CI at head: Build, Test, Typecheck, Lint, Format, all Windows lanes, CodeQL, all 8 regression shards, CLI smoke, and Fallow audit — all green. Ship it.
— Via
|
Addressed the three non-blocking review follow-ups at exact head
Verification: |
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Delta re-review at head d4c77a8a4b5080fbde38ea36bb1b4eb767921359. All 7 required checks pass (Build, Test, Test: runtime contract, Typecheck, Render on windows-latest, Tests on windows-latest, Semantic PR title). Compare vs prior approved head 8aa09a8e28: 1 commit ahead, 0 behind, only 2 files touched (audioPadTrim.ts -23, audioPadTrim.test.ts +2/-5) — matches the hygiene-only scope.
Three hygiene follow-ups verified:
-
Orphan
concatFileLineblock comment —packages/producer/src/services/render/audioPadTrim.tsL217-238 in the prior head is gone. New file goes cleanly fromformatSecondsat L212-215 straight into thepadOrTrimAudioToVideoFrameCountdoc block at L217. No new orphan blocks introduced. -
Stale 5th arg in test —
packages/producer/src/services/render/audioPadTrim.test.tsL28 is nowbuildPadTrimAudioPlan("/tmp/in.aac", "/tmp/out.aac", 4.0, 5.0)— 4 args, matching the current public signature. The sibling win32-path call at L102-107 is also 4-arg. -
PR body
## Howstale copy —## Hownow states "Pad withapad=whole_dur, cap the output at the target duration, and AAC-encode the continuous sample timeline into M4A." The old "Pad and no-op branches continue to stream-copy audio" phrasing is gone. Remainingstream-copymentions in## Whatand the closing paragraph refer only to the no-op audio path and final video mux — both accurate.
No regression vs prior R2 approval: pad-silence / pad-concat step kinds remain absent (step kinds are copy / normalize / trim), apad=whole_dur=${targetSec} + -c:a aac -b:a 192k re-encode present at L145-151, sampleRateForFilter / channelLayoutForChannels helpers still deleted, and audioPadTrim.integration.test.ts still present alongside the unit test.
LGTM.
— Via
3 participants
What
Normalize both padded and trimmed render audio on the decoded sample timeline, encode the result as AAC in M4A, and keep the no-op path on stream copy. Final video muxing remains stream-copy only.
Why
AAC packet-copy operations cannot guarantee the requested presentation duration. Trimming could retain a packet-boundary tail, while padding by concatenating a separately encoded raw-ADTS silence segment could create a timestamp/bitrate discontinuity that surfaced as roughly 606 ms of apparent audio drift in the production-style regression.
How
atrimplusasetpts, then AAC-encode into M4A.apad=whole_dur, cap the output at the target duration, and AAC-encode the continuous sample timeline into M4A.Test plan
style-3-prodregression: video/audio approximately 16.07 s, 1 ms drift, zero visual failuresThe regression was reproduced before the fix with 0.563416 s of reported audio/video drift. The sample-timeline normalization removes the malformed tail while preserving stream-copy final muxing and preview behavior.