fix(audio): renumber timestamps between apad and atrim in mixed branches - #3380
Conversation
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
miguel-heygen
left a comment
There was a problem hiding this comment.
Reviewed exact head 9055942deec84cb956b1cfbfa9a77513965db944.
The filter placement is correct. audioMixer.ts:724 and the legacy producer mixer both reset timestamps after adelay + indefinite apad and before the duration-bounding atrim, preserving the inserted silence while removing the padded-sample PTS ambiguity. A repository-wide search finds no other delayed apad → atrim branch; audioPadTrim.ts is the only untouched sibling and has no adelay, matching the measured carve-out.
Coverage pins every engine mix branch to apad,asetpts=N/SR/TB,atrim, retains the whole_dur portability prohibition, and updates the tail/default-gain assertions. CI is fully green at this head.
I independently executed the new graph on the repo host's FFmpeg 4.2.7 with two staggered tones: 0–0.1s stayed silent (-91 dB floor), while both authored windows measured audible (-30.1 dB mean), so the reset neither erases adelay nor loses the later branch on the oldest claimed build.
I cannot cast a formal APPROVE because this review uses miguel-heygen, the PR author's account; GitHub forbids self-approval. The code verdict is clean.
Verdict: COMMENT
Reasoning: The timestamp reset is placed on every delayed mix branch, preserves the older portability constraint, and holds under an independent FFmpeg execution; no code blocker remains, but a different GitHub identity must supply the approval stamp.
— Magi
jrusso1020
left a comment
There was a problem hiding this comment.
Reviewed exact head 9055942deec84cb956b1cfbfa9a77513965db944 through a separate GitHub identity.
packages/engine/src/services/audioMixer.ts:724resets timestamps after the sample-producingadelay + apadchain and before the duration-boundingatrim, preserving authored silence and clip duration.packages/producer/src/services/audioExtractor.ts:216applies the identical fix to the legacy mixer. A repository-wide search found no other delayedapad → atrimbranches; the untouchedaudioPadTrim.tssibling has no delay.- Independent FFmpeg 7.1.1 execution reproduced the old failure (audible pre-roll at -24.1 dB and the late tone absent) and verified this head's graph (digital-silent pre-roll and both authored tones present at -21.1 dB).
- The required checks and exact-head check runs are all green. A fresh adversarial pass found no blocker or important issue and independently passed 61 focused engine mixer tests.
Optional follow-up: add a four-input behavioral regression for the exact disappearance scenario and exercise the legacy producer graph directly. The current structural coverage pins both production branches, so this is not merge-blocking.
Verdict: APPROVE
Reasoning: The timestamp reset is placed at every affected delayed mixer boundary, preserves gain/tail/duration semantics, and fixes the failure on an affected FFmpeg family without regressing the portability constraint.
— Codex
Twelve of the stack's feature commits landed on main as squashes (#3274 through #3292, plus #3401's canary removal); the 96 review-and-fix commits that followed them here did not, and main moved 64 commits on in the meantime. This reconciles the two. 58 files conflicted. 44 were audio-only — main's side there is the squashed form of commits this branch already carries and has since superseded, so the branch side stands. The rest needed real work, in both directions: **Taken from main, absent here.** - `ensureAudioGroupInertStyle` (#3278's review). An `<hf-audio-group>` is an unknown custom element, so it still takes a flex/grid slot and can open a line box — adding a group shifted authored layout. The helper and its `init.ts` call never came back to the branch, and this branch is what emits the element. - `#3383`'s ended-audio replay: `canSeekEndedMediaBackward` and its five siblings in `media.ts`, with all six tests. Not present here in any form. - `#3380`'s `asetpts=N/SR/TB` between `apad` and `atrim`. Also applied to `mixGroupMembers`, the group submix, which is new on this branch and so had the same bug in a path main's fix could not reach: delayed members padded then amix'd, where a group of four or more silently loses one. - `#3401`'s `displayNumber` thread. The header derives its row from the group-aware order and the undo label from ascending element keys, so once a group exists the same click said "Hide track 2" and recorded "Hide track 1". - `#3413`/`#3421`'s viewport handling — the popover's height cap and `inset()`, and `resolveFloatingPanelPosition` for the grouping dialog, which lives in a track header at the bottom of the window. - Two extractions this branch had inline and at exactly the 600-line cap: `useTimelineDeleteOps` and `editingModeSlice`. Bodies were identical. **Kept from the branch, against main.** Mute and solo are gone by deliberate breaking change (`remove mute and solo from tracks and groups`, `remove the group volume slider and level meter`), so eight files main still carries are deleted again, `PlayerControls` keeps no `previewIframeRef` (it existed only to feed `SoloBanner`), the group-levels branch comes out of main's new `previewMessageRouter`, and `STRIP_H` goes with the bus strip it sized. Main's `TimelineTrackPlainHeader.test.tsx` is rewritten against the control that actually exists — the visibility eye, withheld from an audible audio row and offered back once hidden, which is the only way out of `data-hidden`. **Unioned.** `TimelineFxPopover` — main's positioning, this branch's audition telemetry (`auditionPresetChain`, `storedChain`, `onAuditionTracked`); `SKILL.md` — main's #3416 "keep the carve group a voice group" beside this branch's bus section, with the canary paragraph dropped since the canaries no longer exist. Every port is mutation-checked. core 2508, studio 4460, lint 528, engine 1630, cli 2813, sdk 549, producer green; tsc, oxlint, oxfmt, fallow and the 600-line cap clean.
What
Insert
asetpts=N/SR/TBbetweenapadandatrimon every mixed audio branch.Closes #3344.
Why
Each mixed track is built as
atrim → volume → adelay → apad → atrim. On FFmpeg 5.xthrough 8.0.x the samples
apadappends carry timestamps the followingatrimmisreads,so a delayed branch sounds at t=0 instead of at its offset and, once four or more
branches are mixed, the last one disappears from the output entirely.
Nothing errors. The render succeeds and the mp4 has wrong audio.
Three independent reports, including one traced from the composition side: "the first
<audio>element whosedata-startis > 0 and comes after a time gap gets scheduled atrender time 0". That reporter thought composition size was the trigger and could not
reproduce it small — size was a proxy for branch count, which is why it needs four.
How
Not by reverting to
apad=whole_dur=. #2769 deliberately moved off that form becausesome builds reject the option outright:
asetpts=N/SR/TBrebuilds the timestamps from the sample count before the trim reads them.It uses only filters every build ships, so it fixes the misplacement without giving up the
portability #2769 bought. The existing
expect(filter).not.toContain("whole_dur")guardstill passes unchanged.
audioPadTrim.tspads with the sameapad,atrimpair but has noadelay, and is correcton every version tested, so it is left alone rather than changed on suspicion.
Test plan
Measured across four FFmpeg builds
Five clips at staggered delays, mixed to 43.3s.
t=0.0smust be silent (the earliest clipstarts at 0.3s) and every clip must sound at its own offset.
-infat a clip's offset meansthat clip is absent from the mix.
Current form,
apad,atrim=0:TOTAL:-inf✅-30.1✅-31.0❌-inf❌-31.0❌-inf❌-inf✅-30.1✅This PR,
apad,asetpts=N/SR/TB,atrim=0:TOTAL:-inf✅-30.1✅-inf✅-30.1✅-inf✅-30.1✅-inf✅-30.1✅Two other candidates were tested and rejected:
apad,atrim,asetpts(asetpts after thetrim) is still broken on 7.0.2, and
apad=pad_dur=works but is a relative duration, so itsays something different from what the code means.
Regression test
renumbers timestamps between apad and atrim on every mixed branchasserts every branchmatches
/^apad,asetpts=N\/SR\/TB,atrim=0:/and re-asserts thewhole_durportabilityconstraint. Verified it fails without the fix:
Two existing assertions that pinned the literal
apad,atrim=0:Nstring were updated to thenew shape.
Other checks
vitest packages/engine/src/services/audioMixer.test.ts— 59 passedoxlint+oxfmton all three touched files — cleanWorth knowing
The bug appears to have been fixed upstream somewhere between 8.0.x and 8.1.1, which is why
it is invisible on a current toolchain. It still matters, because we do not control which
FFmpeg a user has, and 7.x is common.