Repository navigation
fix(engine): late-starting videos play to their last frame - #6736
Conversation
A video whose first frame is after timestamp 0 could end early (frozen tail) or late: ffprobe's stream duration does not run from the first frame in every container (fragmented MP4, Matroska). Such a video's end is now read from its last packet, once, where its facts are probed.
Edit accuracy: accurate 271 (base branch 271), smooth 151 of thoseThe gate passes. Quarantined, measured but not gated (0) Unstable (2)
|
![heygengenesis[bot]](https://avatars.githubusercontent.com/in/2913576?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: REQUEST CHANGES · 1 blocker · 0 important · 2 nits · head 9ad31920
The fix itself holds up: I ran the new test at the merge base and the edit-list, fMP4 and WebM-with-audio rows fail there (3 of 24), as the fails without the fix: line says. The blocker is the new fixture row, which turns the required Tests on windows-latest red (engine-cli lane). Details inline.
- nit (
Comments, not required):ffprobe.tscomment share went from 13.5% to 13.7% (+4 lines).
Scope
Read all 4 files in full. CI, regression, CodeQL and the Windows render are green. Locally (Debian ffmpeg 5.1) the fixed code passes 23 of 24; late-with-audio.webm gives 3.513.
| { source: "late.webm", firstFrameAt: 0.52, seconds: 3 }, | ||
| { source: "edit-list.mp4", firstFrameAt: 1.5, seconds: 3.125 }, | ||
| // Opus starts 7 ms before 0, so the browser's timeline starts there too. | ||
| { source: "late-with-audio.webm", firstFrameAt: 0.507, seconds: 3 }, |
There was a problem hiding this comment.
blocker: 0.507 depends on the encoder's Opus pre-skip, so this row is tied to one FFmpeg build. Windows CI (FFmpeg N-124278) measures 3.499, Debian FFmpeg 5.1 measures 3.513, and Linux CI happens to match. Both miss the toBeCloseTo(…, 2) tolerance of 0.005, which fails the required Tests on windows-latest. Derive the expected end from the fixture itself, for example the video stream's first-packet pts_time from ffprobe plus 3 s, instead of hard-coding 0.507.
There was a problem hiding this comment.
✅ Fixed in f6fa77c: the row now measures the first video packet from the fixture. Passes 24/24 here on ffmpeg 5.1, and the Windows render job is green.
| videoStreamStartSeconds: streamStart, | ||
| videoStreamDurationSeconds: effectiveStreamDuration, | ||
| }).catch(() => undefined); | ||
| if (last) effectiveStreamDuration = last.ptsSeconds + last.durationSeconds - streamStart; |
There was a problem hiding this comment.
nit: when this read fails, resolvePlayableVideoDuration now adds mediaTimeOffsetSeconds to ffprobe's own duration. That is the overshoot the PR description's table describes for fMP4 (0.25 + 3.29), so a failed read now expects frames that don't exist, where before it froze one frame early. Consider leaving the start offset out of that fallback.
| const BOTH_LATE_SECONDS = 2; | ||
|
|
||
| function ffprobeValue(args: string[]): number { | ||
| const result = spawnSync(getFfprobeBinary(), ["-v", "error", ...args, "-of", "csv=p=0"], { |
There was a problem hiding this comment.
blocker: callers pass path inside args, so the argv ends ..., path, "-of", "csv=p=0". That fails the producer ffprobeArgvContract pin ("-- must be immediately before the input"), and with it the required Test. Take the path separately so the argv ends "--", path:
function ffprobeValue(args: string[], path: string): number {
const result = spawnSync(getFfprobeBinary(), ["-v", "error", ...args, "-of", "csv=p=0", "--", path], {Then update the two call sites. The contract regex reads the literal tail, so check it with vitest run src/utils/ffprobeArgvContract.test.ts in packages/producer.
There was a problem hiding this comment.
✅ Fixed in 688389b: the argv now ends "--", path. ffprobeArgvContract passes 118/118 here, and CI is green.
…-com#6746) Reverts heygen-com#6736. Its end-of-clip read fails intermittently on Windows, and the fallback then expects frames past the clip's end.
What
A video whose first frame sits after timestamp 0 now plays to its real last frame in the render, the way the browser preview shows it. Before, such a clip could freeze on a frame before its end, or be expected to run past its end.
Why
The engine took a video's end on the browser timeline as "where it starts + ffprobe's stream duration". That duration does not run from the first frame in every container:
Adding the start to ffprobe's duration fixes the first and overshoots the second, and an overshoot makes the frame-coverage check expect frames that do not exist. So the end is measured instead: for a video that starts after 0,
extractMediaMetadatareads its last packet (presentation time + duration) once, where the rest of its facts are probed, and every end-of-clip path reads it from there throughresolvePlayableVideoDuration. Videos that start at 0 take no extra probe and behave exactly as before.Related work
No issue. Found while checking frame-accurate output on cloud renders.
How
Cost: one extra ffprobe packet read of the last second, only for videos that start after 0: 22-26 ms on a 30-minute, 900 MB H.264 file shifted to start at 0.5 s (the existing metadata probe on the same file: 18-22 ms). The read result is cached with the rest of the metadata.
Test plan
fails without the fix:
streamStartOffset.test.tsasserts the playable end for four late-start shapes; on main the edit-list MP4, fragmented MP4 and WebM-with-audio-from-0 cases fail (3 of 24).