fix(render): normalize local AAC duration before mux

[miguel-heygen]

Summary\n\nNormalize local AAC audio to the encoded video frame duration before final muxing, and make AAC duration metadata reliable for VBR ADTS inputs.\n\nThis is an adjacent parity fix, not a claimed root-cause closure for the reported 615-second render that failed while opening audio.aac with an OS timeout. That field failure remains a separate concurrency or filesystem symptom; its workaround changed drawElement parallelism. This PR closes the independently verified AAC duration-drift class shared by local assembly, distributed assembly, and composition metadata consumers.\n\n## Changes\n\n- Run fail-closed pad or trim normalization before local final mux.\n- Derive AAC-LC duration from packet count and sample rate in the shared engine extractAudioMetadata path, so htmlCompiler and all other consumers receive the same VBR-safe duration.\n- Keep non-AAC and invalid or missing packet-count inputs on container metadata.\n- Always create a distinct extension-agnostic duration-normalized AAC tempfile.\n- Document the AAC-LC 1024-sample packet invariant.\n\n## Tests\n\n- Shared engine metadata: table-driven coverage for non-AAC, valid AAC packets, missing packets, zero packets, and invalid packets.\n- Assemble stage: normalization before mux, fail-closed behavior, and non-.aac input path isolation.\n- Full ffprobe utility suite: 19 passing.\n- Focused assemble suite: 3 passing.\n- Producer audioPadTrim execution is blocked locally by a missing sparse-worktree dependency; current-head CI provides the full package run.\n\n## Scope\n\nThe original field timeout is intentionally not marked closed by this PR. This change fixes the duration-parity sibling class and prevents composition metadata from retaining the same VBR AAC undercount.

[james-russo-rames-d-jusso]

Reviewed at c2c4e6eb1a57484cd4c4b007d8ee2d0720be85b1.

What this does

Two coordinated changes to stop mixed-source AAC audio from drifting the muxed output:

1. Duration derivation switches from ffprobe metadata to packet-count math for AAC-LC.
Pre-PR, defaultProbeAudioInfo in audioPadTrim.ts:420-424 trusted metadata.durationSeconds from extractAudioMetadata (ffprobe format tag). For raw ADTS AAC, FFmpeg 5.1 estimates that field from bitrate, and a 16.704s synthetic track probed as 16.955s (~250-708ms over-estimate depending on bitrate stats). That fed a false pad decision downstream. Post-PR at audioPadTrim.ts:423-446, if audioCodec === "aac" and sampleRate > 0, a second ffprobe call with -count_packets -show_entries stream=nb_read_packets returns the exact ADTS frame count; durationSecondsFromAacPacketCount(packetCount, sampleRate) computes packetCount * 1024 / sampleRate (AAC-LC's fixed 1024-sample frame) as the authoritative duration.

2. runAssembleStage now runs padOrTrimAudioToVideoFrameCount unconditionally before mux at assembleStage.ts:58-68, writes to <name>.duration-normalized.aac, and hard-fails the render if normalization fails. Was previously an optional/conditional step on the mux path; now always-on so any residual drift after the fixed probe is closed by an explicit trim/pad rather than surviving into the muxed MP4.

Test coverage: pure math pinned at audioPadTrim.test.ts:26-30 (durationSecondsFromAacPacketCount(783, 48_000) === 16.704); orchestration pinned at assembleStage.test.ts:56-94 (happy path passes the normalized path through to mux; failure path throws before the mux call).

Verification

• Packet-math arithmetic. AAC-LC: 1024 samples per raw-frame packet. 783 * 1024 / 48_000 = 802,032 / 48,000 = 16.704. ✓ Also holds for the CI-reproducer numbers Miguel cited: 501 video frames at 30fps → 16.700s video; audio at 16.704s → 4ms drift → within the existing AUDIO_DURATION_TOLERANCE_SECONDS = 0.001 s trim envelope. ✓
• Codec gating. The -count_packets branch fires only when metadata.audioCodec === "aac" && metadata.sampleRate > 0. Non-AAC codecs preserve pre-PR behavior (metadata.durationSeconds). ✓
• Assemble-stage failure mode is fail-closed. if (!normalizeResult.success) throw before the mux call — no silent fallback to unnormalized audio. Test at assembleStage.test.ts:79-93 pins this: mux not called on normalization failure. ✓
• extractAudioMetadata caching + fresh packet probe — the packet-count call goes through runFfprobeJson (not the cached extractor), so the packet number reflects the current file even if metadata was cached from an earlier read. ✓
• CI green on the current head after Miguel's dockerized reproducer confirmed the root cause. ✓

Concerns — most-severe first

• 🟠 audioOutputPath.replace(/\.aac$/i, ".duration-normalized.aac") silently no-ops when the input path doesn't end in .aac. String.prototype.replace on a regex that doesn't match returns the original string. So a non-.aac audioOutputPath (e.g. an intermediate wav/m4a fallback path) produces normalizedAudioPath === audioOutputPath, and the subsequent padOrTrimAudioToVideoFrameCount({audioPath, outputPath: <same path>}) writes to its own input — undefined behavior at the FFmpeg layer (can succeed with a copy, can truncate mid-write depending on backend). Two shapes fix this cleanly: (a) assert audioOutputPath.endsWith(".aac") before the replace (fail fast if the assumption breaks in a refactor), or (b) derive the tempfile as ${dirname}/${basename-without-ext}.duration-normalized${ext} so any extension is handled. Given the AAC-gate on the whole path, (a) is the smaller change. Worth closing before merge if the assumption is load-bearing.

• 🟡 -count_packets walks every packet. For a 16-second AAC (~800 packets) the cost is milliseconds — negligible. But this now runs on every render's audio probe path. Long recordings (podcast-length inputs, multi-hour audio) will pay proportionally. Correctness-over-speed is the right trade for the drift bug, but worth documenting so a future perf regression on long inputs traces back to this deliberate choice. Non-blocking.

• 🟡 Assemble-stage normalization is now always-on. Every render with audio now writes a second .duration-normalized.aac intermediate plus the mux input. Extra ffmpeg pass per render (typically fast — trim/pad without re-encode), extra disk write on each job. For batch renders in throughput-sensitive environments this compounds. If the normalization is a no-op (source already matches video frame count), an early return on "no work to do" would keep the tempfile write off the disk. Non-blocking; small optimization worth as a follow-up if profiling flags it.

• 🟢 AAC_SAMPLES_PER_PACKET = 1024 is correct for AAC-LC (the codec the audio path emits at ffmpeg default settings). AAC-HEv1/v2 (SBR) uses 2048 samples/packet at the output rate. If the codec detection ever widens beyond metadata.audioCodec === "aac" to include HE-AAC variants, the constant would need to become codec-profile-aware. Note-only.

• 🟢 assembleStage.test.ts uses vitest but the sibling audioPadTrim.test.ts uses bun:test. Different runners in the same directory tree — probably intentional (assemble uses vi.hoisted mocks) but worth confirming the CI runs both. Both suites appear to pass in Magi's summary.

Verdict framing

Two-part fix on a real, reproduced root cause; math checks out; failure mode is fail-closed. The replace(/\.aac$/, ...) fallback is the one thing I'd like to see hardened before merge. Otherwise LGTM from my side once the AAC-path assumption is either asserted or made extension-agnostic.

— Review by Rames D Jusso

[james-russo-rames-d-jusso]

Reviewed at d8e8477eb9a4f616100ead178ca04190d6b2a39d — R1 (c2c4e6eb) → R2 delta is +16 / -1 across two files.

R2 delta

Closes the R1 🟠 on the extension-agnostic path derivation. assembleStage.ts:59-64:

const audioExtension = extname(audioOutputPath);
const audioStem = audioExtension
  ? audioOutputPath.slice(0, -audioExtension.length)
  : audioOutputPath;
const normalizedAudioPath = `${audioStem}.duration-normalized.aac`;

The pad-trim output is now derived from the stem regardless of input extension, so a non-.aac input (e.g. mixed-audio .m4a intermediates that would have silently no-op'd through the old .replace(/\.aac$/i, …)) gets a distinct output path.

Regression at assembleStage.test.ts:77-85 pins the .m4a case: audioOutputPath: "/tmp/audio.m4a" → outputPath: "/tmp/audio.duration-normalized.aac". The existing default-.aac test at :56-74 continues to assert the same /tmp/audio.duration-normalized.aac output — backwards-compat with the R1 behavior for the AAC path is preserved.

Verification

• Node's extname semantics.
  • "/tmp/audio.m4a" → .m4a, stem /tmp/audio, normalized /tmp/audio.duration-normalized.aac. ✓
  • "/tmp/audio.aac" → .aac, stem /tmp/audio, normalized /tmp/audio.duration-normalized.aac (identical to R1 output). ✓
  • No extension ("/tmp/audio") → "", stem /tmp/audio, normalized /tmp/audio.duration-normalized.aac. ✓
  • Leading-dot file ("/tmp/.audio") → extname returns "", so stem is the whole path → /tmp/.audio.duration-normalized.aac. Edge case but sensible. ✓
• No input/output path collision. For any input extension, the derived normalized path differs from the input path (different suffix). Fixes the R1 silent no-op. ✓
• Repeated normalization is idempotent-safe. If a caller feeds a .duration-normalized.aac input (e.g. a re-run against the tempfile), the derived output is <...>.duration-normalized.duration-normalized.aac — distinct, so no self-overwrite. ✓
• CI: 9 SUCCESS + 22 pending + 4 skipped at read time; fresh restack still settling, no FAILURES on completed jobs. ✓

R1 concerns status

• 🟠 Path collision on non-.aac inputs → CLOSED via the extname/stem derivation. ✓
• 🟡 -count_packets walking every packet on long inputs — still unchanged (was note-only, not blocking).
• 🟡 Always-on assemble-stage pad-trim on every render — still unchanged (was note-only, not blocking).
• 🟢 AAC_SAMPLES_PER_PACKET = 1024 is AAC-LC only, HE-AAC would need 2048 — still unchanged (was note-only, only load-bearing if codec detection widens).

Verdict framing

Small, targeted follow-up that closes the one blocker cleanly, with a matching regression that pins the previously-broken .m4a case. LGTM from my side once CI settles.

— Review by Rames D Jusso

[vanceingalls]

Reviewed at d8e8477eb9a4f616100ead178ca04190d6b2a39d. Additive to @james-russo-rames-d-jusso's review — filling gaps; one explicit divergence on verdict.

Strengths

• Packet-count math is authoritative for AAC-LC. packetCount * 1024 / sampleRate (audioPadTrim.ts:40-42, :445) bypasses the FFmpeg bitrate estimator that undercounts VBR ADTS containers. Right primitive at the right layer.
• Fail-closed on normalization error (assembleStage.ts:68-71 at d8e8477eb) — throws before the mux call rather than muxing an unnormalized tail. Failure test at assembleStage.test.ts:79-93 pins this behavior.
• Codec + sampleRate gate (audioPadTrim.ts:425) — non-AAC and zero-sampleRate paths preserve pre-PR behavior. Narrow blast radius on the new branch.
• Push at d8e8477eb resolves the .replace(/\.aac$/i, …) self-write hazard Rames flagged — extname() + stem derivation is extension-agnostic, and the new .m4a regression test (assembleStage.test.ts:77-86) pins the behavior. Nothing to add there.

Additive concerns (gaps in prior review)

🔴 Field-report attribution unproven; PR body still empty (important). This PR was tagged as targeting the 615s field render that failed with Audio muxing failed: FFmpeg exited with code 196; Error opening input file audio.aac: Operation timed out, with the workaround HF_DE_PARALLEL_ROUTER=false HF_STATIC_DEDUP=false. That failure signature reads as an OS-level open timeout under concurrency contention, and the workaround touches worker parallelism — not obviously the same failure class as "container VBR duration undercount → false silence tail." The causal chain from the fixed root cause to the specific errno=110 on file open is not established anywhere: PR body is blank at d8e8477eb, no repro in the PR, no attribution comment. The fix is defensible on its own merits as parity with distributed/assemble.ts:296-311, but the "closes the field report" thesis is unsupported. Please either (a) explain the causal chain in the body (why ADTS drift produced the observed Operation timed out, not just why it produced a wrong duration), or (b) reframe as "adjacent — mux-drift parity with distributed" and file the observed field failure as a separate open ticket.

🔴 Sibling site with the same VBR-drift precondition — fix locality is one layer too deep (important / Rule 2 extrapolation). The fix patched the leaf (defaultProbeAudioInfo), not the shared root (packages/engine/src/utils/ffprobe.ts:351-390 extractAudioMetadata). packages/producer/src/services/htmlCompiler.ts:434,443 calls extractAudioMetadata(filePath) directly on user-provided AAC composition media, then uses metadata.durationSeconds as the audio element's effective composition duration. Same precondition (VBR ADTS container duration under-reports), different consumer (composition timeline duration, not mux drift). A user-uploaded VBR AAC will still have its composition length undercounted. Two shapes: (a) lift the packet-count fallback into extractAudioMetadata itself so callers can't miss it, or (b) file a follow-up ticket for htmlCompiler + document that the fix is intentionally leaf-only. Extrapolation of the fix pattern is what Rule 2 wants — one site cured, one sibling inheriting the same bug is the classic manifestation.

🔴 Regression test doesn't parameterize the new branch (important — Miguel R1 directive #4). The only new unit test at audioPadTrim.test.ts:26-30 is durationSecondsFromAacPacketCount(783, 48_000) === 16.704 — a math tautology of the exported helper. The real added logic is the four-branch decision inside defaultProbeAudioInfo (audioPadTrim.ts:423-446):

1. audioCodec !== "aac" → skip packet probe, keep metadata.durationSeconds
2. audioCodec === "aac", sampleRate === 0 → skip packet probe, keep metadata.durationSeconds
3. audioCodec === "aac", nb_read_packets NaN/0/missing → keep metadata.durationSeconds
4. audioCodec === "aac", valid packet count → override with derived

Zero of those branches are covered. The d8e8477eb push added an outer-wrapper .m4a regression test (correct fix for Rames's finding) but did not cover the packet-count branch itself. assembleStage.test.ts exercises the outer wrapping (padOrTrim wired before mux) — it never touches the packet-count branch. Options: export defaultProbeAudioInfo (or a small internal helper) for direct testing, or mock runFfprobeJson at the module level and parameterize with .each across the four cases. Miguel's bar on parameterizing test across every branch reads as blocking on its own.

Notes

• 🟢 CI runs both test runners. Rames flagged bun:test vs vitest as needing confirmation; verified packages/producer/scripts/run-test-lane.mjs:28-33 explicitly spawns bunx vitest run on vitest-classified files before running bun tests. Both suites execute in Producer: unit tests.
• 🟢 AAC_SAMPLES_PER_PACKET = 1024 is AAC-LC-only, and the mixer emits AAC-LC (packages/engine/src/services/audioMixer.ts:440 -acodec aac = ffmpeg's built-in AAC-LC encoder). Today the constant is safe. If future work introduces HE-AAC v1/v2 (2048 output samples/frame after SBR) or AAC-LD (480/960), the derivation silently produces wrong durations. Inline comment declaring the invariant would prevent silent breakage; not blocking at current codec scope.
• CI state at review time. mergeStateStatus=BLOCKED, checks in-flight on d8e8477eb after the extension-agnostic push; the earlier c2c4e6e head was green. Assuming re-run stays green.

Position

RIGHT direction, CLOSED thesis rejected. The packet-count derivation is the correct primitive, the fail-closed shape is right, and parity with distributed/assemble.ts is a solid architectural argument for the always-on normalization. The extension-agnostic push at d8e8477eb was the right response to Rames's finding. But the fix is one layer too deep (leaf, not shared root), the new branch is untested at parameterization discipline, and the field-report closure claim has no attribution evidence.

Explicit divergence from Rames: he framed this as LGTM-once-.replace-hardened (now done). I'd hold on merge until (a) PR body explains the causal chain or reframes as adjacent, (b) defaultProbeAudioInfo branches are parameterized, and (c) htmlCompiler.ts sibling is either fixed or ticketed.

Verdict: REQUEST CHANGES
Reasoning: Right fix direction, but Rule 2 extrapolation to htmlCompiler.ts sibling site + missing branch parameterization on defaultProbeAudioInfo + no field-report causal chain in the body add up to should-not-merge-as-is.

— Via

[james-russo-rames-d-jusso]

Reviewed at 9fb6f8b3f — R2 (d8e8477e) → R3 delta is +82 / -36 across 4 files.

R3 delta

Restructures the AAC duration derivation from producer-local to engine-shared, so every consumer of extractAudioMetadata sees the VBR-safe duration transparently.

1. AAC packet-duration logic moved into packages/engine/src/utils/ffprobe.ts:372-390 — extractAudioMetadata now runs the -count_packets -show_entries stream=nb_read_packets probe internally for audioCodec === "aac" && sampleRate > 0, and overrides format.duration with (packetCount * AAC_LC_SAMPLES_PER_PACKET) / sampleRate when the packet count is a positive finite number.
2. AAC_LC_SAMPLES_PER_PACKET = 1024 with an explicit comment ("FFmpeg's built-in AAC encoder emits AAC-LC, which has 1024 samples per packet.") — closes my original 🟢 about codec-profile specificity by documenting the AAC-LC assumption at the constant.
3. FFProbeStream.nb_read_packets?: string added to the type schema — properly typed, not stringly-typed.
4. durationSecondsFromAacPacketCount removed as an exported helper from audioPadTrim.ts. defaultProbeAudioInfo now trusts metadata.durationSeconds directly (the packet-derived value is baked in at the extractor).
5. 5-branch parametrized ffprobe test at ffprobe.test.ts:206-255:
   • non-AAC metadata (mp3) → uses format.duration = 1.25, only 1 ffprobe spawn.
   • valid AAC packet count (783 @ 48kHz) → 16.704, 2 spawns.
   • missing AAC packet count → falls back to format.duration = 1.25, 2 spawns.
   • zero AAC packet count → fallback, 2 spawns.
   • invalid AAC packet count (non-numeric) → fallback, 2 spawns.

Verification

• Move-into-engine is the right refactor. Every caller of extractAudioMetadata (there are several across producer, cli, and analysis paths) inherits the fix — no more risk of a sibling caller wired to raw format.duration regressing to the ADTS bitrate-estimate. ✓
• Cache reuse. extractAudioMetadata uses audioMetadataCache — first probe pays for both the metadata and packet-count calls; subsequent probes on the same path return the promise from cache. So the amortized cost of the -count_packets widening is one packet-walk per file per process lifetime. Addresses my R1 🟡 about the cost concern as a widening trade. ✓
• Fallback contract is fail-safe on all four failure modes — missing / zero / non-numeric / non-AAC — every non-happy path preserves the pre-PR value of output.format.duration. Test matrix pins each. ✓
• sampleRate fallback of 44100 when the stream reports no sample rate matches the pre-PR default, so the packet-count division has a defined divisor in the (unlikely) missing-rate case. ✓
• R2 🟠 stays closed. The extname/stem tempfile derivation from R2 is unchanged in R3. ✓
• CI: 25 SUCCESS + Preflight complete at head per read; still settling. No FAILURES. ✓

R1/R2 concerns status

• 🟠 (R1) Silent no-op on non-.aac inputs → CLOSED at R2 (unchanged).
• 🟡 (R1) -count_packets walking every packet on long inputs → NOW WIDENED to all AAC probes but each file pays only once via the audioMetadataCache. Amortized cost is bounded per file.
• 🟡 (R1) Always-on assemble-stage pad-trim → unchanged (still note-only, not blocking).
• 🟢 (R1) AAC_SAMPLES_PER_PACKET = 1024 is AAC-LC-only → DOCUMENTED via the rename to AAC_LC_SAMPLES_PER_PACKET and the inline comment. ✓

Verdict framing

R3 is the right level for this fix — the packet-count derivation now lives where the metadata invariant is enforced. Test matrix covers the fallback surface. LGTM from my side.

— Review by Rames D Jusso

[vanceingalls]

Reviewed at 9fb6f8b3f30a40cb19470fe98f4fc2b37517c52b — R1 (d8e8477e) → R2 delta is +82 / -36 across 4 files. Additive to @james-russo-rames-d-jusso's R3.

R1 findings status

• ✅ F1 (blocker, Rule 2 extrapolation) — RESOLVED. Packet-count derivation moved from defaultProbeAudioInfo (producer leaf) into the shared engine root at packages/engine/src/utils/ffprobe.ts:376-394. extractAudioMetadata now runs the -count_packets -show_entries stream=nb_read_packets probe internally when audioCodec === "aac" && sampleRate > 0, and overrides output.format.duration with (packetCount * AAC_LC_SAMPLES_PER_PACKET) / sampleRate. Verified the htmlCompiler.ts sibling I called out — htmlCompiler.ts:434 (await extractAudioMetadata(filePath)) → htmlCompiler.ts:443 (const fileDuration = metadata.durationSeconds) — now inherits the VBR-safe duration transparently. Cross-checked every other extractAudioMetadata consumer: packages/engine/src/services/audioMixer.ts:557-560 (composition mixdown), packages/engine/src/services/chunkEncoder.ts:95 (codec-only read, unaffected by duration), packages/producer/src/regression-harness.ts:840, packages/producer/src/services/render/audioPadTrim.ts:419-423 (now trusts metadata.durationSeconds directly, no duplicate probe). No sibling bypasses remain. The AAC_SAMPLES_PER_PACKET export was correctly removed from audioPadTrim.ts:39-43 and its tautological helper test at audioPadTrim.test.ts:26-30 was removed with it. ✓

• ✅ F2 (important, regression parameterization) — RESOLVED. packages/engine/src/utils/ffprobe.test.ts:206-257 adds an it.each matrix pinning all 5 branches of the new decision:

  1. non-AAC metadata (mp3) → keeps format.duration = 1.25, 1 ffprobe spawn ✓
  2. valid AAC packet count (783 @ 48kHz) → 16.704, 2 spawns ✓ (math checks: 783 * 1024 / 48000 = 16.704)
  3. missing AAC packet count → fallback to format.duration, 2 spawns ✓
  4. zero AAC packet count → fallback, 2 spawns ✓
  5. invalid AAC packet count (non-numeric string) → fallback, 2 spawns ✓

  Each row asserts both the derived duration AND the expected ffprobe call count, so the codec-gate is pinned in both directions (non-AAC skips the packet probe, AAC always makes it). This is stricter than my R1 ask — I only requested the 4 defaultProbeAudioInfo branches; the R2 matrix additionally distinguishes missing/zero/invalid fallback shapes. ✓

• ✅ F3 (important, PR body causal-chain) — RESOLVED. PR body now opens with explicit adjacent-not-closing framing: "This is an adjacent parity fix, not a claimed root-cause closure for the reported 615-second render that failed while opening audio.aac with an OS timeout. That field failure remains a separate concurrency or filesystem symptom; its workaround changed drawElement parallelism." and closes with a ## Scope section: "The original field timeout is intentionally not marked closed by this PR. This change fixes the duration-parity sibling class...". My R1 offered (a) explain the causal chain or (b) reframe as adjacent — option (b) taken cleanly. ✓

Additional verification

• Cache amortization. audioMetadataCache (ffprobe.ts:406-410) holds the promise so a single probe pays both the base metadata and packet-count calls; subsequent same-path lookups return the memoized value. My R1 didn't flag this but Rames R1 did — good that R2 keeps the cache boundary intact rather than duplicating the packet probe outside the extractor. ✓
• Fallback contract is fail-safe on every non-happy branch. Missing / zero / non-numeric / non-AAC all preserve pre-PR output.format.duration. Test matrix pins each. No new terminal failure paths introduced. ✓
• Type schema. FFProbeStream.nb_read_packets?: string added at ffprobe.ts:103 — properly typed at the schema, not stringly-typed at the read site. ✓
• Constant rename to AAC_LC_SAMPLES_PER_PACKET with inline comment (ffprobe.ts:56-57) documents the AAC-LC 1024-sample invariant at the definition. Codec-profile widening (HE-AACv1/v2 → 2048, AAC-LD → 480/960) would now naturally scope into the same block. Closes my R1 🟢 note on the constant. ✓
• CI at 9fb6f8b3f. mergeStateStatus=BLOCKED on required-reviewer gate; all required check-runs green (Test, Producer: unit + integration tests, Typecheck, Lint, Format, Preflight, Windows Render + Tests, Preview parity, SDK unit/contract/smoke, CLI smoke, CodeQL, Analyze × 3). Only pending: 3 regression-shards (non-blocking optionals still running). No FAILUREs. ✓
• Adversarial pass for other duration-drift consumers. packages/producer/src/utils/audioRegression.ts:302 probeAudioDuration reads stream=duration directly via a raw spawnSync ffprobe — but this is the regression comparison harness (compares two rendered outputs post-hoc), not a production render path. Out of scope for the drift fix. Not a bypass.

Position

RIGHT, thesis CLOSED. All three R1 blockers/importants land cleanly:

1. Fix locality lifted to the shared engine root — sibling htmlCompiler.ts (and every other extractAudioMetadata caller) transparently inherits the VBR-safe duration.
2. New decision surface is fully parameterized at 5 branches with call-count assertions.
3. PR body reframes as adjacent-parity, not field-report closure.

Explicit alignment with @james-russo-rames-d-jusso's R3 verdict at the same commit. Nothing to hold on my side.

Verdict: APPROVE

— Via

[vanceingalls]

R3 verification (head d26f3e889)

Narrow re-stamp on the golden refresh only.

Delta scope. gh api compare/9fb6f8b3f...d26f3e889 returns exactly one file, one line:

• packages/producer/tests/style-7-prod/output/compiled.html

  -      <audio id="aroll-audio" ... data-duration="16.043" ... data-end="16.043"></audio>
  +      <audio id="aroll-audio" ... data-duration="16.064" ... data-end="16.064"></audio>
  

Checks against the claim.

1. Confined to test golden — ✅. Path lives under packages/producer/tests/style-7-prod/output/, a snapshot artifact. No production source, no config, no schema.
2. Numeric delta 16.043 → 16.064 (+21ms) — ✅. Consistent with the AAC packet-count / 1024-sample-frame normalization derivation introduced upstream in the PR body (ceil(samples / 1024) * 1024 / sample_rate rounds up to the nearest full AAC frame, and MP4 container mdhd.duration is quantized on the same boundary; sub-frame residues manifest as low-tens-of-ms drift).
3. No test deletion / skip — ✅. Golden value updated in place; no .skip, no removed assertion, no test file removed. additions:1 deletions:1 changes:2 on one file.
4. CI health at this head — Format ✅, Lint ✅, Fallow ✅, CodeQL analyze (python) ✅, semantic-title ✅, CLI shims (ubuntu/macos) ✅, SDK unit+contract+smoke ✅, player-perf ✅. Producer unit/integration/runtime-contract, regression shards (shard-3 covers style-7-prod), Windows render, and CodeQL (javascript-typescript) still in-flight — none failing yet.

R3 verdict: APPROVE (re-stamp). Prior R2 logic clearance at 9fb6f8b3f (pullrequestreview-4703663981) carries; this head only updates the numeric golden to match the normalized derivation, no re-verification of runtime behavior required beyond the still-running style-7-prod regression shard.

— Via

[james-russo-rames-d-jusso]

Reviewed at d26f3e88 — R3 (9fb6f8b3) → R4 delta is +1 / -1 across 1 file (golden refresh scoped to a single test).

R4 delta

Single-line golden update at packages/producer/tests/style-7-prod/output/compiled.html:1051:

- <audio id="aroll-audio" src="_remote_media/download_054a544691a0.mp4" data-start="0" data-duration="16.043" data-track-index="1" data-end="16.043"></audio>
+ <audio id="aroll-audio" src="_remote_media/download_054a544691a0.mp4" data-start="0" data-duration="16.064" data-track-index="1" data-end="16.064"></audio>

Verification

• Math is consistent with the R3 packet-count formula. 16.064s * 48000 Hz / 1024 samples/packet = 753 packets (exact) — so this audio stream has 753 AAC packets @ 48kHz, and R3's extractAudioMetadata now derives the true 753 * 1024 / 48000 = 16.064s instead of the ADTS bitrate estimate (16.043s). The 0.021s = 21ms discrepancy is exactly what the R3 normalization was meant to close. ✓
• Scope is correct — no other stale goldens. Grepped packages/producer/tests/*/output/*.html for separate <audio> tags with matching pattern:
  • style-7-prod is the ONLY test with a separate <audio id="aroll-audio"> whose src is the ~16s AAC-in-MP4 file that hits the packet-count code path.
  • style-1/2/4-prod each expose their a-roll as <video ... data-has-audio="true" data-duration="16.043"> — the data-duration there reflects the video-stream duration (unaffected by extractAudioMetadata), and audio-vs-video reconciliation happens later at mux time via padOrTrimAudioToVideoFrameCount. Correctly not touched. ✓
  • Style-7-prod's own MUTED <video> sibling on download_054a544691a0.mp4 still shows data-duration="16.043" — same file, video-stream duration ≠ audio-stream duration by 21ms, both reflected correctly (video track = 16.043s frame-count derivation, audio track = 16.064s packet-count derivation). Container-level asymmetry is normal for MP4/M4A. ✓
• CI evidence. Per the message: full style-7 regression passed compilation + 100 visual checkpoints + 0.950 audio correlation + 4ms stream drift; audioPadTrim 16/16 and assembleStage 3/3 passed. New CI queued but not yet green — worth watching before merge.

R3 concerns status

• All R3 findings remained closed; R4 only touches the golden. ✓

Verdict framing

Scoped golden refresh reflecting the R3 code change, mathematically consistent with the packet-count derivation, and correctly limited to the one test where the separate <audio> track exposes AAC-packet duration. No other producer-test goldens are stale by this metric. Ready from where I sit once the new CI settles green.

— Review by Rames D Jusso