Skip to content

fix(producer): mix audio into a container that can record encoder delay - #3200

Merged
miguel-heygen merged 5 commits into
mainfrom
fix/audio-mix-container
Aug 11, 2026
Merged

fix(producer): mix audio into a container that can record encoder delay#3200
miguel-heygen merged 5 commits into
mainfrom
fix/audio-mix-container

Conversation

@miguel-heygen

Copy link
Copy Markdown
Collaborator

What

Every rendered composition's audio lands 1024 samples (21.33 ms at 48 kHz) after its authored data-start, against a frame-accurate video track. This moves the mixed-audio artifact into a container that can record the AAC encoder's priming delay, so it lands on time.

Why

An <audio data-start="2" data-duration="3"> rendered with its onset at 2.0220s and its tail at 5.0200s instead of 2.000 / 5.000. It is under one frame at 30fps so it hides well, but it is deterministic, it affects every render with audio, and it matters for anything beat-synced or lip-synced.

Measuring the envelope of each intermediate localises it exactly:

Stage Onset
source tone.wav 0.0000s exact
mixer output 2.0220s offset introduced here
audio.duration-normalized.m4a 2.0220s inherited
final output.mp4 2.0220s inherited

The mix is AAC-encoded, and AAC encoders emit ~1024 priming samples. The mix was written to a raw ADTS .aac file, which has nowhere to record that delay, so it decoded as real leading silence and every stage downstream preserved it faithfully.

Three things rule out the obvious alternatives:

  • Not the filter graph. Running the mixer's own graph by hand to PCM (atrim=0:3,volume=1,adelay=2000|2000,apad,atrim=0:6) lands on 2.0000s.
  • Not the assemble stage. The pad/trim and mux stages inherit the offset unchanged, as the table shows.
  • Not a measurement artifact. Encoding one reference PCM (silence, then a tone starting at exactly 2.000s) twice with the same ffmpeg, same encoder, same bitrate: into a raw .aac the first audible sample comes back at 2021.35 ms, exactly 1024 samples late; into an .m4a it comes back at 2000.02 ms, 0 samples late.

How

Switch the artifact to an MP4-family container, which stores the delay as an edit list that decoders strip. Same codec, same bitrate, so no size or quality change. .m4a is also a better fit than raw ADTS for the PNG-sequence sidecar, which is handed to users for After Effects / Nuke / Fusion ingest.

The filename is a contract shared by three consumers: the mux input, the distributed plan artifact, and that user-facing sidecar. Its extension is what selects the muxer, so a disagreement between them is a silently wrong container rather than a missing file. It now has one owner in the engine instead of five literals across two packages.

Reviewer note on the plan contract

This renames the distributed plan's audio artifact. That is an on-disk contract between the plan writer and the assembler. Both move together in this PR, but a plan written by an older build would not be found by a newer assembler. I don't know whether that mixed-version window is reachable in how these are deployed, so flagging it rather than deciding it: if it is, the reader should accept the legacy name for a release.

Test plan

  • Unit tests added/updated
  • Manual testing performed
  • Documentation updated (if applicable)

New behavioural test in audioMixer.level.test.ts, alongside the existing real-ffmpeg level test and skipped the same way when ffmpeg is absent: mix a tone at start: 2 and assert the first audible sample is within 5 ms of 2.000s. Verified it fails on main with expected 2.021354166666667 to be close to 2 — that is 2 + 1024/48000 exactly, which is the fingerprint of the bug rather than an approximation of it.

  • packages/engine: 1482 passed, 3 skipped, 60 files.
  • packages/producer unit lane: 579 passed, 0 failed. Integration lane: 37 passed, 2 failed — both crossWorkerIdempotency capture-mode assertions that fail identically on main on this machine (the browser probe selects screenshot capture), unrelated to audio.
  • End-to-end on the original repro, the rendered onset and tail move from 2.0200 / 5.0200 to 2.0000 / 4.9980, matching a hand-built reference that is exact by construction.
  • Rendered a PNG sequence and confirmed the sidecar is written as audio.m4a and probes as AAC.

Not covered

The authored 0.05s fade still measures ~62 ms wide in the encoded output. That is the mixer's volume=...:eval=frame expression being evaluated once per ~21 ms audio frame, which is a separate, smaller inaccuracy in the envelope's shape rather than in its placement. Left alone here.

Every rendered composition's audio landed 1024 samples (21.33 ms at 48 kHz)
after its authored `data-start`, against a frame-accurate video track.

The mix is AAC-encoded, and AAC encoders emit ~1024 priming samples. The mix
was written to a raw ADTS `.aac` file, which has nowhere to record that delay,
so it decoded as real leading silence and every stage downstream preserved it
faithfully. Measuring each intermediate localises it precisely: the source WAV
is exact, the mixer's own output is already 21.33 ms late, and the pad/trim and
mux stages inherit it unchanged. The filter graph itself is correct - run by
hand to PCM it lands on the authored start.

Switch the artifact to an MP4-family container, which stores the delay as an
edit list that decoders strip. Same codec, same bitrate, so no size or quality
change.

The filename is a contract shared by three consumers - the mux input, the
distributed plan artifact, and the PNG-sequence sidecar handed to users for
NLE ingest - and its extension is what selects the muxer. Give it one owner in
the engine rather than five literals, so those consumers cannot drift onto
different containers.

Note for reviewers: this renames the distributed plan's audio artifact, which
is an on-disk contract between the plan writer and the assembler. Both move
together here, but a plan written by an older build would not be found by a
newer assembler. Flagging in case that mixed-version window matters for how
these are deployed.
The aws-lambda and gcp-cloud-run adapters each restated the plan's audio
filename in five places, so renaming it in the producer left them looking for a
file that is no longer written. CI caught it: the gcp dispatch test asserting a
plan has no audio artifact started seeing one.

Export the name from `@hyperframes/producer/distributed` and consume it in both
adapters. This is the same failure the constant exists to prevent, one package
boundary further out: a literal that drifts from the writer's is a silently
missing audio track rather than a loud error, because both call sites only ever
ask whether the file exists.
@miguel-heygen

Copy link
Copy Markdown
Collaborator Author

Pushed 927f44e: CI caught that the aws-lambda and gcp-cloud-run adapters each restated audio.aac in five places, so the rename left them looking for a file that is no longer written. Both now read the name from @hyperframes/producer/distributed.

That is the same failure the constant exists to prevent, one package boundary further out, and it sharpens the reviewer note above: these two adapters locate the artifact by existence check only, so a name mismatch is a silently missing audio track rather than an error. Worth weighing when deciding whether the assembler should accept the legacy name for a release.

@vanceingalls vanceingalls left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

1. [BLOCKER] CI shard-6 audio regression is real, and the PR body's "unrelated to audio" note refers to a different lane. regression-shards (shard-6, …) (job 93648211745) fails missing-host-comp-id at the audio check: ✗ Audio quality: FAILED (correlation: 0.394, threshold: 0.9), with "lagWindows":-12 at 2026-08-11T01:18:16Z. That suite is the only one in the shard with real (non-silent) audio; every other audio check in the same run passes at correlation 1.0 (they're silence-vs-silence). The PR body's "the 2 failing tests are unrelated to audio" refers to the integration lane's crossWorkerIdempotency capture-mode asserts — this is the regression lane, and the failing suite is exactly the one that renders real audio. Almost certainly the pinned reference for missing-host-comp-id was baked with the 21.33 ms bug and now the correct audio is ahead of it (negative lag) — the "golden encodes the bug" trap that any correctness fix on a baked medium hits. Please regen the reference on this branch, or if the divergence isn't the encoder-delay fingerprint, explain what the correlator is seeing. Either way, this needs a decision in-PR.

2. [BLOCKER] Rolling-deploy asymmetry can silently drop audio on in-flight plans. handlePlan writes PLAN_AUDIO_RELATIVE_PATH (audio.m4a); handleAssemble / handleAssembleV2 in packages/aws-lambda/src/handler.ts and packages/gcp-cloud-run/src/server.ts now read only that name. handlePlan and handleAssemble are separate Lambda invocations bridged by S3/GCS — during rollout, a pre-rollout planner writes audio.aac, a post-rollout assembler reads it, finds nothing, produces a video with muted audio. Your own "Reviewer note on the plan contract" flags this direction and defers the reachability call; I'm arguing it is reachable given the separated invocation model, and the mitigation is cheap: for one release, both readers accept audio.aac as a fallback when the primary name is missing, with a deprecation log. The GCP path already keeps a similar back-compat fallback for the old AudioGcsUri shape (server.ts:307 "only for backward compatibility with an older Plan that uploaded it standalone"), so the precedent is right there.

3. Docstring drift in packages/producer/src/services/render/audioPadTrim.ts:91-99. The buildPadTrimAudioPlan docstring still says the pad branch will "concat-copy the source AAC plus that tail. This avoids re-encoding the already mixed audio." The pad branch at :128-143 decodes/filters/re-encodes AAC (-af apad,atrim=0:{targetSec} … -c:a aac -b:a 192k) — matching the corrected top-of-file docstring at :19-22. Fix the mid-function comment so future readers don't act on the stale invariant.

4. Stale .aac output extension in audioPadTrim.test.ts:100 (Windows fixture). Input path was updated to audio.m4a but the output extension is still audio-padded.aac. runAssembleStage in-tree writes to audio.duration-normalized.m4a, so nothing production-facing is broken today — but this fixture models a Windows-invocation shape and the pad branch re-encodes AAC. If a future caller ever landed on .aac output the priming-delay bug returns wholesale in that consumer. Rename the fixture's output to .m4a.

5. Test asserts on mixer output only, not the assembled MP4. audioMixer.level.test.ts (diff :166-217) probes the mixer's direct MIXED_AUDIO_FILENAME output and asserts the first audible sample there. The PR body claims the final output.mp4 moves from 2.020 s → 2.000 s end-to-end, but that assertion is manual only. A full-chain fixture (mix → pad → mux → probe final mp4) would have caught the regression #1 surfaces. Not blocking this PR, but the automated coverage gap is what let the golden trap through.

6. CLI-feedback fingerprint not cited. PR body describes the author-reproduced case but doesn't include a Slack ts= / fingerprint for the CLI-feedback signal (or the internal repro origin). Convention is to link it so future readers can rebuild the causal chain. Nit — the mechanism trace is otherwise exemplary.

Verdict

REQUEST_CHANGES. Mechanism trace is watertight (source 0 s → mixer 2.022 s → 1024/48 000 = 21.33 ms fingerprint), single-owner constant is the right refactor shape, blast radius is fully swept, and numeric verification is sample-precise (s16le decode, |sample| > 512, toBeCloseTo(2, 2) = ±5 ms is comfortably above the fingerprint). Two blockers: a real audio-regression in the regression shard that the PR body's "unrelated to audio" line misdirects past — almost certainly a golden-baked-with-bug case that needs regen or explanation — and an assembler-side rolling-deploy asymmetry where an old-planner + new-assembler window silently drops audio. Both addressable in-PR (regen the golden or explain the correlator; two-line fallback to audio.aac on read for one release). Once those land this is a clean approve — the underlying fix is right.

— Via

@james-russo-rames-d-jusso james-russo-rames-d-jusso left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed at 927f44e9 (head).

The container swap is the right fix and the shape of the change (single owner in engine, exports propagated to every reader, filename derived from the constant) is exactly what "the extension is a load-bearing contract" wants. The audioMixer.level.test.ts:108+ test locks the fingerprint (firstAudibleSeconds within 5 ms of 2.000) and the existing chunkEncoder.ts mux path already carried audioCodec: "aac" + preserveAudioPrimingEditList explicitly, so the codec-copy fast-path survives even though the sidecar filename changed.

Blocker on CI. regression-shards (shard-6, style-11-prod style-10-prod style-1-prod webm-transparency css-spinne...) failed with:

✗ Audio quality: FAILED (correlation: 0.394, threshold: 0.9)
Total: 6 | Passed: 5 | Failed: 1 | Skipped: 0

That's the direct consequence of the fix — the goldens for whichever audio-carrying fixture is in this shard were captured when the mixed audio was 21.33 ms late. Sliding the audio 21.33 ms earlier drops the cross-correlation with the stale golden into the floor. The regeneration is expected as part of the fix; the PR just needs the affected golden(s) refreshed before merge, otherwise the audio-quality regression check is neutered from the moment this lands. See the inline note on handler.ts for the backward-compat concern that pairs with this — both need to land before merge.

The other line in that log, ❌ Test FAILED: Missing Host Composition Id, is emitted before the audio-quality summary but doesn't cause a shard fail on its own — treating it as either a flake or a pre-existing symptom unrelated to this PR. Worth spot-checking if the goldens don't fully close the shard.

Concern — filename-based codec-copy short-circuit. packages/engine/src/services/chunkEncoder.ts:57-58 still says:

function isAacSidecar(audioPath: string): boolean {
  return extname(audioPath).toLowerCase() === ".aac";
}

Called at :89 via shouldCopyAacSidecar. Every in-tree caller passes audioCodec: "aac" explicitly (assembleStage.ts:80, distributed/assemble.ts:341), so those short-circuit correctly. But the semantic name says "this file's audio can be stream-copied" and the extension .m4a is now the canonical mixed-audio sidecar too — a third-party consumer of muxVideoWithAudio that passes an .m4a sidecar without audioCodec: "aac" falls through to the ffprobe fallback (chunkEncoder.ts:94-96), which is still correct but pays a probe process it didn't used to. Cheap cleanup: extend to .aac OR .m4a (both are AAC-in-container in this codebase) or rename to canCopyAudioStream to reflect the actual predicate. Not a blocker — in-tree behavior is preserved.

Nit. audioPadTrim.test.ts:626 and :632-641 still hardcode .aac in test-supplied output-path fixtures (audio-padded.aac, /tmp/out.aac). Test-behavior-neutral — the fixtures are strings, no ffmpeg actually runs on them — but reads oddly against the rest of the PR now that .m4a is the canonical name. Zero-cost rename.

Nit. encodeStage.test.ts:35 inlines MIXED_AUDIO_FILENAME: "audio.m4a" in the @hyperframes/engine mock. If the constant ever moves again, this mock silently drifts. Importing the real constant (or referencing it via vi.importActual) keeps them locked.

What I like. The MIXED_AUDIO_FILENAME docstring at audioMixer.ts:31-45 explains exactly WHY the extension is load-bearing, WHY MP4-family is the fix, and points at the three consumers that would silently disagree. Future readers won't need to re-derive.

What I didn't verify.

  • Did not reproduce the audio-quality correlation locally, or check whether every audio-carrying fixture in shard 6 needs its golden refreshed or just one. PROD_FIXTURES layout in that shard would tell you at a glance.
  • Did not walk the lambda / cloud-run deployment topology to prove the mixed-version window Miguel called out is actually reachable — the inline note on handler.ts:574 argues it IS reachable given plan-store-and-forward, but the concrete deployment cadence is yours to know.
  • Did not audit KNOWN_NON_AAC_AUDIO_EXTENSIONS in chunkEncoder.ts:61-69.m4a isn't there, so the fallback path correctly probes rather than short-returning false. Good in the current shape, but ties into the isAacSidecar cleanup above.

Otherwise the shape is right. Once the goldens are refreshed (and the backward-compat call is made — inline), this is ready. Stamp routing per standing rule.

Review by Rames D Jusso

Comment thread packages/aws-lambda/src/handler.ts Outdated
let audioPath: string | null = null;
if (event.AudioS3Uri) {
audioPath = join(planDir, "audio.aac");
audioPath = join(planDir, PLAN_AUDIO_RELATIVE_PATH);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 Backward-compat concern — plan-writer/assembler mixed-version window

Miguel raised this himself in the PR body ("a plan written by an older build would not be found by a newer assembler"). This is the primary read site for the S3 v1 assemble path, and I think the window IS reachable given the plan-store-and-forward shape here — the plan is uploaded to S3 as a tarball, and there's no coordination that forbids a NEWER handleAssemble from picking up a plan tarball whose audio.aac was written by the pre-fix build (in-flight batches during a deploy, replay-on-failure of an older event, cross-region propagation lag, whatever). When that happens, existsSync(join(planDir, PLAN_AUDIO_RELATIVE_PATH)) at the sibling v2 site (:622-624) is false, handleAssemble here reads a non-existent path and the assembler proceeds without audio — silent no-audio render, not a loud failure.

A one-release compat window closes it cheaply. Something like:

const audioCandidates = [PLAN_AUDIO_RELATIVE_PATH, "audio.aac"]; // legacy fallback — remove after one release cycle
const audioPath = audioCandidates
  .map((name) => join(planDir, name))
  .find((p) => existsSync(p)) ?? null;

Or, for the S3 v1 path here, just OR the legacy name into the existing branch. Same treatment needed at:

  • handler.ts:622-624 (S3 v2 assemble)
  • packages/gcp-cloud-run/src/server.ts:571-577 (GCS v1)
  • packages/gcp-cloud-run/src/server.ts:622-624 (GCS v2)

Delete the legacy name in the next release. If the plan-store-and-forward mixed-version window ISN'T reachable in practice (all planners + assemblers deploy atomically together, no in-flight replay of older plans), then this reduces to a no-op comment saying so. Either resolution is fine — the current state (no fallback, no note documenting the reasoning) is what leaves someone else in the dark on the deploy day.

— Rames D Jusso

Review raised a rolling-deploy window I had flagged but left undecided: `plan`
and `assemble` are separate invocations bridged by object storage, so a
pre-rollout planner can be paired with a post-rollout assembler. Both readers
locate the artifact by existence alone, which makes that pairing a silently
muted video rather than an error. That is reachable enough to be worth two
lines, so reads now accept the old name while writes only ever emit the new one.

Give the fallback one owner (`resolvePlanAudioPath` / `isPlanAudioArtifactPath`)
rather than four call sites, marked for deletion one release out.

Also fixes a hole in the first pass of this: the plan-v2 materializer matched
either name but then joined the CURRENT one, so a legacy plan resolved to a path
that was never written. It now joins the artifact's own name.

Review nits in the same pass: correct the pad-branch docstring, which still
described a concat-copy shape the pad branch stopped using when it moved to
apad + re-encode, and fix the Windows fixture's stale `.aac` output extension so
it cannot model a shape that reintroduces the priming delay.
@miguel-heygen

Copy link
Copy Markdown
Collaborator Author

Thanks, this was a good catch list. Addressed 2, 3, 4 in c171e1c; 1 is diagnosed below with the golden regen in progress; 5 and 6 answered at the end.

1. The shard-6 failure: confirmed, and it is the encoder-delay fingerprint

You were right that my "unrelated to audio" line pointed at the wrong lane, and right about the cause. I reproduced it rather than reasoned about it, and the numbers are cleaner than "almost certainly":

Rendering missing-host-comp-id twice from the same source, once at origin/main and once on this branch, then cross-correlating the two final MP4s at native 48 kHz with sample-precision:

best lag 1024 samples @48k = 21.33 ms, corr 0.99994
level delta 0.02 dB

Exactly 1024 samples, which is the AAC priming fingerprint, at a correlation that says the content is otherwise bit-for-bit the same audio. So the golden was baked with the bug and the correct audio is now ahead of it, as you called.

Two corrections to the record while I was in there:

  • The lag number in the log is unstable, the correlation is the real signal. CI reported lagWindows: -12; my local run of the same fixture reported -6. The harness correlates 64 ms RMS envelopes (1024-sample hop at 16 kHz), so a 21.33 ms shift is a third of one window: it smears energy across window boundaries and the reported lag is the argmax of an already-degraded correlation, not a measurement of the shift. Worth knowing before anyone reads -12 as 768 ms of drift.
  • Not every other audio check in that shard is silence-vs-silence. style-10-prod passed at correlation 0.965 and style-11-prod at 0.954 in the same run, both with real audio. They survive because their content is dense and continuous, where a sub-window shift barely moves the envelope; missing-host-comp-id does not. (Its source is named silence.wav but peaks at -18.1 dBFS, which is what makes it sensitive.)

Unrelated staleness the regen will also absorb, flagging so it is not a surprise in the diff: the committed golden is 3.0 dB quieter than what both main and this branch render today. Since the correlator is scale-invariant it cannot see gain, so this has been sitting under a passing check. I am regenerating in the CI container (Dockerfile.test, ffmpeg 5.1.9) rather than natively, because the devbox host ffmpeg is 4.2.7 and would bake a different encoder's output into the golden.

5. Full-chain coverage

Agreed, and it is the gap that let this through. The unit test asserts the mixer's own output because that is where the defect lives, but you are right that nothing automated walks mix → pad → mux → probe. The regenerated missing-host-comp-id golden becomes exactly that assertion for the timing, since the harness now pins a correctly-placed track through the full chain. I would rather let that be the coverage than add a second fixture that renders the same thing.

6. Fingerprint link

Declining this one on purpose. There is no ticket or Slack thread behind it: this came out of a local reproduction, so there is no causal chain to rebuild. Separately, this repo is public, and internal tracker keys and Slack timestamps are exactly what should not go into permanent history here. If an internal reference is wanted, it belongs on the internal issue that links out to this PR, not in the PR body.

…dio delay

The pinned reference was rendered before this branch, so it carries the 1024
sample encoder-priming delay in its audio. With the delay gone the correct audio
now sits ahead of the reference and the harness's envelope correlation drops
below its floor.

Cross-correlating the old and new references at native 48 kHz gives a lag of
exactly 1024 samples (21.33 ms) at a correlation of 0.99985: same audio, moved
by exactly the amount this branch removes. Regenerated inside the CI container
(Dockerfile.test, ffmpeg 5.1.9) rather than natively, so the reference matches
the encoder CI will compare against - the container reproduced CI's failure to
the digit (correlation 0.3938764027803616, lagWindows -12) before the rebake and
passes at correlation 1.0 after it.

Note for archaeology: the new reference is also 3 dB louder than the old one.
That gap is not from this branch - `main` and this branch render the fixture at
the same level - it is pre-existing drift the reference had accumulated, which a
scale-invariant correlator could never see. The rebake absorbs it.

Only output.mp4 is updated. `--update` also rewrites compiled.html, but that
diff is embedded-font churn with no bearing on the comparison, which reports
"Failed at compilation: 0" either way.

@james-russo-rames-d-jusso james-russo-rames-d-jusso left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed delta 927f44e9..4d8eb93f.

Both R1 blockers addressed cleanly, plus Vance's audioPadTrim docstring drift and Windows-fixture extension nits. Reads through the delta with fresh eyes:

Compat shim is the right shape. resolvePlanAudioPath / isPlanAudioArtifactPath live in packages/producer/src/services/distributed/shared.ts with the one-release-lifetime note inline, and every read site takes them: handler.ts:576 (S3 v1 assemble), handler.ts:624 (S3 v2), server.ts:573 (GCS v1), server.ts:625 (GCS v2), plus HasAudio detection at handler.ts:398 / server.ts:403 and the plan-v2 artifact classification at planV2.ts:286. All writes still only emit PLAN_AUDIO_RELATIVE_PATH — the "reads accept legacy, writes only ever emit current" invariant holds. planAudioCompat.test.ts covers the four cases + the write-only-current asymmetry directly. Single owner, single removal point.

Nice self-catch at planV2.ts:898-908. Miguel's own first pass into the plan-v2 materializer would have matched either name at the classification step but then joined the CURRENT name for the audioPath return — a legacy plan flowing through v2 materialization would have resolved to a path that was never written, silently muting. Fixed by looking up the artifact by predicate and joining its OWN name (audioArtifact.path). Not a finding on this pass because it's already fixed — flagging as good work.

Golden rebake is textbook. The commit message for 4d8eb93f walks the causal chain end-to-end: rebaked inside the CI container (Dockerfile.test, ffmpeg 5.1.9) so the encoder matches CI's own, and the container reproduced the failure to the digit — correlation 0.3938764027803616, lagWindows -12 — before the rebake and passes at 1.0 after. Independently, cross-correlating the old vs new references at native 48 kHz gives 1024 samples of lag at 0.99985 correlation — the same audio, moved by exactly the fingerprint this branch removes. The 3 dB level drift note is the right archaeological callout — main and this branch render at the same level, so a scale-invariant correlator wouldn't have exposed it and it's pre-existing drift the reference had accumulated. Good taste.

Docstring correction at audioPadTrim.ts:112-115 is accurate. Reading :132-158, the pad branch is apad,atrim=0:{target} … -c:a aac -b:a 192k — decode + filter + re-encode, exactly as the corrected comment describes. The parenthetical about the concat-copy shape not producing portable output on bundled Windows FFmpeg builds is the right level of "why" to leave for a future reader.

What I didn't verify.

  • CI on the new SHA. Regression shard-6 is still IN_PROGRESS as of write. Miguel's in-container reproduction is strong evidence but the CI-green confirmation is what closes the R1 blocker on my side; my LGTM below is conditional on it.
  • Did not read every audio-fixture golden in regression beyond the missing-host-comp-id one Miguel rebaked. If any other suite in shard-6 has real (non-silent) audio, the same fingerprint would blow it up — Vance's callout that this is the only real-audio suite in the shard is the load-bearing assumption there; trusting it.

Carried forward from R1 (unchanged, unresolved, still non-blocking). chunkEncoder.ts:57-58's isAacSidecar is still .aac-only. Every in-tree caller passes audioCodec: "aac" explicitly at assembleStage.ts:80 / distributed/assemble.ts:341, so the codec-copy fast-path survives for in-tree behaviour. Third-party consumers of muxVideoWithAudio handing an .m4a sidecar without the codec option fall through to the ffprobe fallback at chunkEncoder.ts:94-96 — still correct, just pays a probe process. Cheap cleanup for a future PR: extend the predicate to .aac || .m4a or rename to canCopyAudioStream to match the actual invariant.

LGTM once CI comes back green. Stamp routing per standing rule.

Review by Rames D Jusso

@vanceingalls vanceingalls left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed delta 927f44e9..4d8eb93f.

Blocker 1 (shard-6 golden) — addressed

Golden was regenerated on this branch and verified by mechanism, not vibes. Commit 4d8eb93 (test(producer): rebake the missing-host-comp-id golden without the audio delay) touches packages/producer/tests/missing-host-comp-id/output/output.mp4 and its commit message walks the causal chain end-to-end:

  • Cross-correlating old vs new references at native 48 kHz: lag of exactly 1024 samples (21.33 ms) at correlation 0.99985 — same audio, moved by exactly the encoder-priming delay this branch removes.
  • Rebake done inside the CI container (Dockerfile.test, ffmpeg 5.1.9), which reproduced CI's failure to the digit: correlation 0.3938764027803616, lagWindows -12 before, 1.0 after.
  • Note on the 3 dB reference-drift is the right archaeological callout — orthogonal, invisible to a scale-invariant correlator, pre-existing on main.

CI caveat: regression-shards (shard-6, …) is still IN_PROGRESS at review time. Mechanism-tracing evidence is stronger than a CI green (digit-matched failure + digit-matched pass in the same container), but required-check gating on shard-6 remains the merge-time confirmation.

Blocker 2 (rolling-deploy fallback) — addressed

Fallback is symmetric across every reader. New primitives in packages/producer/src/services/distributed/shared.ts:740-758:

  • PLAN_AUDIO_LEGACY_RELATIVE_PATH = "audio.aac"
  • resolvePlanAudioPath(planDir) — tries current, falls back to legacy, returns null if neither.
  • isPlanAudioArtifactPath(path) — recognizes either name.

Read-side wired at every entry point (writer-side unchanged: audio.m4a only):

  • packages/aws-lambda/src/handler.ts:576 (handleAssemble), :624 (handleAssembleV2)
  • packages/gcp-cloud-run/src/server.ts:573, :625
  • HasAudio manifest at handler.ts:398 / server.ts:403 uses isPlanAudioArtifactPath so pre-rollout plan-v2 manifests still surface audio.
  • packages/producer/src/services/distributed/planV2.ts:898-908materializePlanV2Target joins the artifact's own path (not the constant), so a legacy-named manifest materializes under its legacy name. Nice defensive detail beyond the ask.
  • regression-harness-distributed.ts intentionally NOT wrapped: in-process, plan+assemble same version, no rolling exposure. Correct call.

Test coverage: planAudioCompat.test.ts (5 cases) locks prefer-current / fallback-to-legacy / null-when-neither / both-recognized / writer-only-current. Pinned removal comment: "Delete this file, the legacy constant, and the fallback branch one release after the container change ships."

New findings

  • Nit, not a blocker. No deprecation log emitted when the legacy path is taken. R1 ask specified "with a deprecation log" so the team knows when the fallback goes quiet in prod. You took a comment-plus-test approach instead. Tolerable; consider a one-shot logger.warn({ event: "plan_audio_legacy_artifact_path" }) inside resolvePlanAudioPath when the legacy branch fires — cheap signal, one line, deletes with the shim.
  • Grepped the diff for surviving audio.aac string references outside the LEGACY constant / compat test / removed lines — zero survivors. Docstring hygiene clean across planV2.ts, assemble.ts, plan.ts, audioPadTrim.ts, regression-harness-distributed.ts.
  • PLAN_AUDIO_RELATIVE_PATH is now derived from MIXED_AUDIO_FILENAME (shared.ts:728) rather than restated — writer/muxer disagreement can no longer land silently. Good.
  • Merge state: MERGEABLE, BLOCKED on required checks only. No rebase conflict.

Verdict

APPROVE. Both R1 blockers addressed with high-confidence evidence. Blocker 1's rebake is not just a regenerate — the encoder-container repro of failure + pass to matching decimals is the strongest possible discriminator for "golden-encodes-the-bug" vs a real regression. Blocker 2's fallback is symmetric across all four assemble entry points plus the manifest predicate + the plan-v2 materializer, with a compat test file and pinned removal comment. Deprecation-log ask is a follow-up nit, not a merge blocker. Merge on shard-6 green.

— Via

Same cause as the missing-host-comp-id rebake, caught by shard-8 once the
earlier shard stopped failing and the rest of the matrix could run: this
reference also carries the encoder-priming delay this branch removes.

Reproduced in the CI container to the digit (correlation 0.42704173048439215,
lagWindows -12), rebaked there, and it now passes at correlation 1.0.

Worth recording: the shift here is 2048 samples (42.67 ms) at correlation
0.99983, exactly twice the 1024 of the other fixture. The delay compounds once
per un-compensated AAC generation, and this fixture's audio needs its duration
normalized, so it takes the pad/trim branch's re-encode and picks up a second
frame of priming on top of the mixer's. So the pre-fix error was not a fixed
21 ms - it grew with the number of times the audio was re-encoded.

All nine shards ran in that CI round with only this one failing, so the matrix
has now covered every fixture against this change.
@miguel-heygen

Copy link
Copy Markdown
Collaborator Author

Pushed 5f7ef6d: shard-8 caught a second reference with the same cause, variables-prod. Rebaked in the CI container the same way (reproduced at correlation 0.42704173048439215 / lagWindows -12, passes at 1.0 after).

One detail worth recording, because it changes how the bug should be described. The shift on this fixture is 2048 samples (42.67 ms) at correlation 0.99983, exactly twice the 1024 on missing-host-comp-id. The delay compounds once per un-compensated AAC generation: this fixture's audio needs its duration normalized, so it takes the pad/trim branch's re-encode and picks up a second frame of priming on top of the mixer's. The pre-fix error was therefore not a fixed 21.33 ms, it grew with the number of times a track was re-encoded, which also explains why some fixtures drifted further than others.

All nine shards ran in the round that surfaced this, with only shard-8 failing, so the matrix has now covered every fixture against this change. Holding the merge until this round reports green rather than assuming it.

@miguel-heygen

Copy link
Copy Markdown
Collaborator Author

Re-review please. Your approval at 03:42 was on 927f44e9; branch protection dismissed it when I pushed the two golden rebakes after it, so this is back to REVIEW_REQUIRED.

All 57 checks now pass, 0 failures, including all nine regression shards and the Windows lane.

Everything since your stamp is test fixtures, no production code changed:

  • 4d8eb93f rebake missing-host-comp-id (1024 samples, 21.33 ms)
  • 5f7ef6d4 rebake variables-prod (2048 samples, 42.67 ms, the compounding case)

The interesting bit is in the second one: the delay is not a constant 21.33 ms, it adds one frame per un-compensated AAC generation, so a fixture that also goes through the pad/trim re-encode drifts twice as far. That is in the commit message and the earlier comment.

@vanceingalls vanceingalls left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-stamp at head 5f7ef6d4 — delta 4d8eb93f..5f7ef6d4 is one commit: test(producer): rebake the variables-prod golden without the audio delay.

Same class as the missing-host-comp-id rebake I approved at R2 — a second fixture also carrying the pre-fix encoder-priming delay, surfaced once shard-6 stopped failing and the rest of the matrix could complete. Diff is 1 file (packages/producer/tests/variables-prod/output/output.mp4, +2/-2 LFS pointer).

The commit message applies the same digit-matched CI-container discriminator that closed R2's blocker 1:

  • Failure reproduced in the CI container: correlation 0.42704173048439215, lagWindows -12 before rebake, 1.0 after.
  • Cross-correlation old vs new = 2048 samples (42.67 ms) at correlation 0.99983 — exactly twice the missing-host-comp-id fixture's 1024-sample shift.
  • Mechanism captured in the commit body: this fixture takes the pad/trim branch's re-encode path, picking up a second frame of AAC priming on top of the mixer's. The pre-fix error was NOT a fixed 21.33 ms — it compounded once per un-compensated AAC generation. That's a nice archaeological finding — the fingerprint scales with re-encode count.

Matrix coverage: all 9 shards ran in that CI round with only this one failing → matrix has now covered every fixture against the container change.

CI at head: 57 SUCCESS / 3 SKIPPED / 0 FAILURE / 0 IN_PROGRESS. regression-shards (shard-6, ...) SUCCESS. regression-shards (shard-8, ...) (which surfaced this fixture) SUCCESS. MERGEABLE / BLOCKED on required-review only. Merge cleanly.

— Via

@miguel-heygen
miguel-heygen merged commit dc43831 into main Aug 11, 2026
60 checks passed
@miguel-heygen
miguel-heygen deleted the fix/audio-mix-container branch August 11, 2026 04:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants