fix(audio): preserve causes and use portable padding#2769
Conversation
vanceingalls
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
Portability fix + typed failure taxonomy — well-executed, well-tested, additive. The apad,atrim=0:X shape is semantically equivalent to apad=whole_dur=X for the pipeline's needs (delay + trim to composition duration), and the taxonomy propagation preserves diagnostic content without introducing new metric tags or unbounded telemetry. Windows CI (Render on windows-latest 8m18s, Tests on windows-latest 11m5s — both pass) is direct end-to-end validation of the very platform this fix targets.
Empirical premises verified against main @ base 3b3d4f5:
RunFfmpegResult.terminationReason—packages/engine/src/utils/runFfmpeg.ts:28, exercised across streaming/chunk encoder / videoFrameExtractor already. ✓formatFfmpegError—packages/engine/src/utils/runFfmpeg.ts:71, re-exported from engine index. ✓formatFilterNumber—packages/engine/src/services/audioMixer.ts:27, existing helper. ✓applyDistributedAudioWarningPolicyruntime callers: onlypackages/producer/src/services/distributed/plan.ts:946, which the PR updates. No positional-log breakage risk. ✓- Legacy inline warning-clone sites that the PR replaces (
frameCapture.ts:3776,renderEventPublisher.ts:8,captureHdrStage.ts:131,renderOrchestrator.ts:618) all only clonedsources; the new sharedcloneCaptureWarningalso deep-clones the newfailureReasons/failureStagesfields, closing the aliasing hole that would otherwise ship with the taxonomy. ✓ - No other user of
apad=whole_durin the live code path (see P2 below for the dead-code sibling). ✓
End-to-end validation
- User-facing exercise path:
plan()(distributed) andexecuteRenderPipeline(in-process) →runAudioStage→processCompositionAudio→ filter emission atpackages/engine/src/services/audioMixer.ts(post-PR L522). Downstream:audioResult.audioFailures→applyDistributedAudioWarningPolicy/ orchestrator's inline aggregation →applyRenderWarningPolicy→ structuredlog.warnwithwarningReasons/warningStages/warningOwners/warningRetryable. - Filter-string test-lock:
packages/engine/src/services/audioMixer.test.tsL79-83 assertsapad,atrim=0:2present andwhole_durabsent — the exact shape swap. Multi-track filter test at L603-609 pinsatrim=count totrackCount * 2andapad,count totrackCount, catching any silent revert to the single-atrimwhole_durshape. Golden churn on the multi-track test is bounded and semantic — checking that the new shape emits both trims and one apad per track, not merely a text-match. - Taxonomy propagation coverage:
plan.test.ts— typed-details, mixed-cause retryability rule (every retryableonly), legacy-untyped preservation (does not invent ownership).renderEventPublisher.test.ts— deep-clone-across-sink-boundary (mutatingjob.warnings[0].details.failureReasonsafterpublish()does not affect the delivered snapshot; sink mutation does not bleed back), plus explicit taxonomy log-shape assertions. - Windows-native run:
Render on windows-latest(8m18s pass) andTests on windows-latest(11m5s pass) both green on head4b024ed. That is the direct field of failure this PR targets. - Producer-lane check: the one non-green matrix job (
Producer: ${{ matrix.lane }} tests, job 89645115967) iscancelled, not failed. Not blocking; not a real signal.
P2 (address before merge or acknowledge)
packages/producer/src/services/audioExtractor.ts:210 — same portability bug, dead code.
The filter builder there emits the identical broken shape:
[${i}:a]atrim=0:${trimDuration},volume=${track.volume},adelay=${delayMs}|${delayMs},apad=whole_dur=${totalDuration}[a${i}]
Zero runtime callers today (rg processAudio\|from.*audioExtractor outside the file itself returns nothing) — the module is orphaned. So it does not block this PR's correctness on Windows. But because the whole thesis is "the portable filter shape is apad,atrim, apad=whole_dur is not supported by the bundled Windows FFmpeg", leaving the identical broken shape in a sibling is a re-materialization landmine the moment anyone revives or copies from that module. Suggest either:
- align the shape here (mirror the change, no other refactor), or
- delete
audioExtractor.tsin a follow-up (there is a scoped ticket-worthy cleanup here regardless of this PR).
Not blocking — flag with a one-line commit or a TODO(#…).
P3 nits
packages/producer/src/services/render/audioPadTrim.ts:249— comment referencesapad=whole_duras the shape used in the codebase (paraphrased: "vs a working manualapad=whole_durcommand"). After merge, the module the comment contrasts against no longer emitswhole_dur. Consider updating the comment or noting the migration.packages/engine/src/services/audioMixer.tsprobeFailure, L292 — thetimedOutregex is/(?:timed?\s*out|timeout|deadline|inactivity|aborted)/i.abortedinretryable: !invalidMedia && (unavailable || timedOut)yieldsretryable: true, but user-initiated aborts should beretryable: false. Low-frequency becauseextractAudioMetadatais called guarded by a signal check, but the semantics are wrong. Consider splittingabortedoff — either into its own branch (withowner: "user",retryable: false,reason: "cancelled") or dropping it from thetimedOutset.- Duplicate ownership/retryability aggregation — the same 8-line
failureOwner/retryablederivation appears in bothpackages/producer/src/services/distributed/plan.tsL273-279 andpackages/producer/src/services/renderOrchestrator.tsL2196-2203. Not a bug (they must stay identical for cross-mode parity), but it's ripe for extraction to a helper (e.g.deriveAudioFailureAggregate(audioFailures)). Skip if you'd rather keep the PR tight. boundedDetailregex fallback — the POSIX path regex ataudioMixer.tsL307-311 lazy-matches\/.+?(?=:\s[A-Z]|\s(?:ENOENT|EACCES|EPERM)\b|\r?$). On an error string like/var/log/render.log then something else happened(path with no terminator marker), the lazy match will extend to end-of-line and swallow the trailing prose. That is over-redaction, not a leak, so it's safe; just noting so you're not surprised by aggressive redaction in unusual FFmpeg outputs.
Nice-to-have (non-nit)
- The
cloneCaptureWarninghelper is now the canonical clone. Consider adding a tiny unit test atpackages/engine/src/services/captureWarning.test.tsthat mutates each ofsources,failureReasons,failureStageson the clone and asserts the source array is untouched. Currently the invariant is only exercised transitively via therenderEventPublishersink-boundary test.
— Via
miguel-heygen
left a comment
There was a problem hiding this comment.
Independent exact-head review on 4b024ed0e1a50e6871ccd5e40d255123c200e978: no additional blocking findings beyond Via’s documented non-blocking follow-ups.
Fresh validation:
- 76/76 focused tests passed in their native lanes (33 Vitest + 43 Bun).
- Full
audio-mux-parityrender passed end to end: compile, 300-frame Chrome capture, FFmpeg audio processing/mux, 100/100 visual checkpoints, stream drift 0.005s, audio correlation 0.99997 with zero lag. - All required GitHub checks are green on this head, including Windows render/tests.
Approved.
4b024ed to
b550865
Compare
|
Addressed the actionable review follow-ups in
I intentionally left the duplicate distributed/in-process audio aggregation in place to keep this correctness PR narrow; the two copies remain parity-tested. The bounded-detail over-redaction note is safe and unchanged. |

What
apad=whole_dur=...with the portable, finiteapad,atrim=0:<compositionDuration>filter chainWhy
Production
audio_processing_failedevents currently discard the underlying FFmpeg/ffprobe cause. A reproduced Windows failure on the bundled FFmpeg rejectsapad=whole_durwithOption 'whole_dur' not found, even though the same composition can render video and be muxed locally.The missing causes also make deterministic input failures indistinguishable from transient infrastructure failures. This change fixes the known portability bug and makes future failures actionable without weakening the existing fail-closed audio correctness policy.
How
amix, preserving authored clip/delay timing while avoiding the unsupportedwhole_duroptionThis is additive/backward-compatible telemetry and does not change retry or fallback policy. It is safe to canary independently; any later policy change can key off the new bounded taxonomy.
Test plan
Independent review: approved after timing, privacy, propagation, retry, and snapshot-isolation findings were addressed.