fix(audio): preserve causes and use portable padding - #2769
Conversation
vanceingalls
left a comment
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
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. |
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
R2 exact-head re-review on .\n\nThe rebase is clean: shows the previously approved implementation commit is patch-equivalent ( → ), with one additive review-fix commit.\n\nVerified the addressed concerns:\n- the orphaned filter now uses portable ;\n- the stale comment is corrected;\n- probe abort/cancellation is user-owned and non-retryable, while timeout/deadline/inactivity is system-owned and retryable;\n- direct tests pin both classifications;\n- now has direct mutation-isolation coverage for every mutable detail array.\n\nFresh exact-head validation: 21/21 focused Vitest cases pass. All CI lanes are green, including all 8 regression shards, Windows render/tests, CodeQL, CLI smoke, and global install.\n\nNo new blocking findings. Approved.
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
R2 verification
Head b550865 vs R1 4b024ed: PR moved forward (branch also rebased through base drift). Every R1 finding checked at the new head — the actionable ones landed, the "skip if you'd rather keep it tight" nit was intentionally deferred. Verdict: APPROVE.
Prior-finding status
| Finding | Status | Evidence at head b550865 |
|---|---|---|
P2 (a) sibling apad=whole_dur landmine in audioExtractor.ts |
Addressed | packages/producer/src/services/audioExtractor.ts:210 — now atrim=0:${trimDuration},volume=…,adelay=…,apad,atrim=0:${totalDuration} (portable shape, matches the fix). |
P3 (a) stale apad=whole_dur reference in audioPadTrim.ts comment |
Addressed | packages/producer/src/services/render/audioPadTrim.ts:246 — comment now reads "…the module's arg shape vs a working manual pad/trim command"; no whole_dur mention. |
P3 (b) probeFailure misclassifies aborted as retryable |
Addressed | packages/engine/src/services/audioMixer.ts:220-244 — new cancelled = /(?:aborted|AbortError|cancelled|canceled)/i boolean; timedOut no longer contains aborted; retryable: !cancelled && !invalidMedia && (unavailable || timedOut); owner: cancelled || invalidMedia ? "user" : "system"; reason: cancelled ? "cancelled" : …. Sibling ffmpegFailure L266-269 mirrors via result.terminationReason === "abort" → reason: "cancelled", owner: "user", retryable stays false. Test lock in audioMixer.test.ts:69-84 (parametrized it.each — cancelled vs timeout with expected owner/retryable pins). |
| P3 (c) duplicate owner/retryable aggregation | Not addressed (acceptable) | Still duplicated at packages/producer/src/services/distributed/plan.ts:270-278 and packages/producer/src/services/renderOrchestrator.ts:2196-2205. R1 explicitly offered "Skip if you'd rather keep the PR tight" — deferring the extract is a legitimate choice, both sites are identical and behave the same, cross-mode parity is preserved. Ledger-only. |
Nice-to-have cloneCaptureWarning unit test |
Added | packages/engine/src/services/captureWarning.test.ts — mutates sources / failureReasons / failureStages on the clone and asserts source is untouched. Exact invariant I sketched. |
P3 boundedDetail POSIX path over-redaction |
Softened | audioMixer.ts:212-215 — regex now gated by (^|[\s"'(]) prefix, matching only paths at start-of-line or after whitespace/quote/paren. Doesn't fully close the lazy-match issue but narrows the false-positive surface; still an over-redaction (never a leak). |
Fresh light-touch adversarial pass on delta code
ffmpegFailuretermination-reason switch (audioMixer.ts:262-289):"abort" / "deadline" / "inactivity" / "spawn_error"values match theManagedProcessTerminationReasonunion atpackages/engine/src/utils/managedChildProcess.ts:3-9. No dangling case — the tail regex fallback (unrecognized option/invalid data found) handles stderr-only errors whenterminationReason === "exit". No new escape hatch that skips ownership/retryable initialization.cloneCaptureWarningnew shared helper: called from every legacy inline warning-clone site (renderEventPublisher.ts,frameCapture.ts,captureHdrStage.ts,renderOrchestrator.ts). Confirmed all four now delegate to the shared clone rather than re-implementing — closes the aliasing hole forfailureReasons/failureStages.- No net-new call sites of
apad=whole_dur:rg 'apad=whole_dur'at head returns zero live-code hits.
Verdict
APPROVE — every actionable R1 finding is addressed with test coverage, the intentionally-deferred duplicate-aggregation nit is a legitimate scope call, and the fresh delta doesn't introduce new correctness or coverage regressions.
— Via
* fix(audio): preserve causes and use portable padding * fix(audio): address failure taxonomy review
…hes (#3380) On FFmpeg 5.x through 8.0.x the samples `apad` appends carry timestamps the following `atrim` misreads. A delayed branch then sounds at t=0 instead of its offset and, once four or more branches are mixed, the last one disappears from the output entirely. No error is raised; the render succeeds with wrong audio. Reverting to `apad=whole_dur=` is not an option: #2769 moved off that form because some builds reject the option outright ("Error applying option 'whole_dur': Option not found"). Inserting `asetpts=N/SR/TB` between the pad and the trim rebuilds the timestamps from the sample count using only filters every build ships, so it fixes the misplacement without giving up the portability that change bought. Verified on FFmpeg 4.2.7, 7.0.2, an 8.x nightly and 8.1.1: the current form is wrong on the middle two, the new form is correct on all four. audioPadTrim.ts also pads with apad+atrim but has no adelay and is correct on every version tested, so it is left alone. Closes #3344
3 participants
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.