fix(cli): select containing source frames in snapshots - #4809
Conversation
6450035 to
2751770
Compare
…l decode Probing decoded frames and selecting by index decoded the source from its start, so a sample 290 s into a 1080p clip took about 35 s and hit the 30 s extraction bound. Packet timestamps need only a demux, and an accurate -ss to the midpoint before the containing frame decodes from the nearest keyframe.
…e videos Fragmented MP4 seeks by dts and MPEG-TS seeks imprecisely, so an input seek to the frame midpoint returned the next frame before each keyframe (fMP4) or a corrupt or missing frame (TS). Seek to the keyframe that starts the frame's decode instead and pick the frame by decoded time with a select filter. A failed or unparsable ffprobe now returns null, so one unreadable video is blanked with a warning instead of aborting the whole snapshot.
c96ae32 to
1f8614e
Compare
The keyframe walk went by decode order, so an open-GOP leading frame seeked to the keyframe that follows it, the decoder dropped it and the next frame was returned. Only keyframes at or before the frame in presentation order count. Without ffprobe, extraction falls back to a plain seek instead of failing the snapshot, as it worked before frame selection needed ffprobe.
ffprobe prints format.start_time rounded to microseconds. When it rounds down, t=0 found no frame and every exact frame time picked the previous frame (24 fps fragmented MP4 with B-frames, many MPEG-TS files). Subtract the stream's integer start_pts in time-base ticks when the video sets the format start.
…eroll A stream-copy trim keeps preroll packets before the first visible frame. The packet probe read a window relative to the first packet, so it stopped before the requested time and picked an earlier frame. Probe the stream start first, then up to start + time + 1 s.
…e input The producer ffprobe argv contract reads each argv literal and requires -- right before the input; the probe helper spliced flags in from its callers, so the contract saw argv without it.
jrusso1020
left a comment
There was a problem hiding this comment.
Requesting changes at d61d4951. The frame math is right when the video stream starts where the container starts. It goes wrong when the video starts later than the container does, which happens with an MPEG-TS that carries audio, or any file where the audio leads the video. I reproduced this with real FFmpeg/ffprobe 8.0.1.
Repro
ffmpeg -f lavfi -i testsrc2=d=3:r=30:s=160x90 -f lavfi -i sine=d=3 \
-c:v libx264 -bf 3 -pix_fmt yuv420p -c:a aac av.ts
ffprobe reports the format start_time as 1.443444 and the video start_pts as 132000 (1.466667). So video frame n sits at 0.0232 + n/30 on the container timeline. I identified frames by comparing each output against select=eq(n,k) extractions:
| time | containing frame | this PR | main |
|---|---|---|---|
| 0, 0.01 | n0 (before the first frame) | null |
n0 |
| 0.05 | n0 | n1 | n1 |
| 0.1 | n2 | n3 | n3 |
I also made an MP4 with audio from 0 and the video shifted +0.5 s (setpts=PTS+0.5/TB). For times 0, 0.01 and 0.1 this PR returns null and main returns n0. From 0.5 s on, the PR is correct.
Two causes
- Before the first frame there's no frame (regression).
containingSourceFrameIndexreturns -1, so the probe returnsnull. In the snapshot path,if (!png) continuethen leaves the plate as whatever headless Chrome painted.--againstprints "has no frame" and skips the reference pair. Main extracted the first frame. The engine also holds the first frame here:videoFrameExtractor.tssamples withfps=…:start_time=0, and on the +0.5 s MP4 that path writes 105 frames from 90 source frames, with the gap filled by frame 0. I tried a local patch that clamps the index to 0 whentimestampsis non-empty. With it, the +0.5 s MP4 gives n0 at all three times, andsnapshot.test.tsstill passes 66/66. - The select threshold assumes FFmpeg's output timeline starts at the container start. On
av.tsit doesn't when the seek is 0. With-ss 0, or no-ssat all,showinfoputs the first video frame atpts_time:0, not 0.0232. With-ss 0.05the timeline does start at the container start (pts_time:0.00656= 0.0566 − 0.05). So wheneverseekcomes out as 0, the threshold is off by the video's lead and the next frame is chosen. Any time inside the first GOP gives a seek of 0, and that's the whole clip here. The clamp alone doesn't fix this: with it,av.tsat t=0 gives n1. The PR's MPEG-TS test passes because its fixture has no audio, so the container start equals the video start.
Suggested fix: add audio to the TS fixture and add a late-video MP4 case. Then pick the target frame in a way that doesn't depend on where FFmpeg puts zero. One option is select=eq(n,k), with k counted in presentation order from the seek keyframe. Another is to read the offset from the first decoded frame. I haven't tried either.
Everything else checks out
- Boundaries:
<=with a 16·ε tolerance selects the frame that starts exactly at the requested time. I confirmed that and frames 9/17/8 on the 24 fps grid, the irregular VFR interval, and the held last frame (with ffprobe) at head. - ffprobe argv:
runCancellableProcessspawns without a shell, and the argv is a fixed literal ending in--before the path. I checked that a relative path literally named-i.mp4still extracts. On the ffmpeg side the path is only ever the value of-i. Inputs are still project-local absolute paths or http(s) URLs, the same as main. - Minor, not blocking: without ffprobe, a held tail now returns
null, because main's-sseof -1fallback was removed. I checked this by stubbingHYPERFRAMES_FFPROBE_PATHto a missing binary: the same call gives a frame with ffprobe andnullwithout it. The body says ffprobe is required, but it doesn't mention this case. Either name it there or keep the tail fallback.
Tests at head: snapshot.test.ts 66/66.
— Rames
| } | ||
| // Packets arrive in decode order; B-frames make that differ from presentation order. | ||
| const timestamps = packets.map((packet) => packet.pts).sort((a, b) => a - b); | ||
| const index = containingSourceFrameIndex(timestamps, time); |
There was a problem hiding this comment.
When the requested time is before the first video frame (a video that starts after the container does), this is -1 and the extraction returns null. Main returned the first frame, and the engine's fps=…:start_time=0 path holds it too. I tried clamping to 0 when timestamps is non-empty: that fixes the late-video MP4, and snapshot.test.ts still passes 66/66.
| seek = Math.max(0, Math.min(packet.pts, packet.dts)); | ||
| break; | ||
| } | ||
| return { seek, select: (previous + frame) / 2 - seek }; |
There was a problem hiding this comment.
This threshold is measured from the container start, but FFmpeg's t isn't always measured from there. On an MPEG-TS with audio leading the video by 23 ms, -ss 0 puts the first video frame at t=0, so every seek-0 extraction picks the next frame (t=0.05 gives n1, t=0.1 gives n3). With -ss 0.05 the timeline does start at the container start. The table in the review body has the full repro.
… leads When the video starts after the container (TS with audio, late video), FFmpeg puts zero at the first video frame after a seek to 0, so the threshold picked the next frame, and times before the first frame gave none. Select on raw stream time (-copyts) and hold the first frame.
-copyts kept raw time in the filter but changed which frames survive the seek trim, so a file whose timestamps start below zero froze on a late frame. Seek to the keyframe display time and select relative to start_t; the no-ffprobe path keeps the plain seek.
…s on MPEG-TS seeks can land a whole keyframe group late, and an open-GOP keyframe is not a clean start, so a predicted start_t picked later frames. Read the landing keyframe with a stream copy, seek exactly to it so its leading frames are trimmed, and select relative to it.
|
@jrusso1020 thanks for the repro, both causes are fixed at 7c2ec71.
Your
Each test fails on the commit before its fix. On your fixtures: Still as you noted: without ffprobe, a held tail past the last frame returns null, because the |
jrusso1020
left a comment
There was a problem hiding this comment.
Approving at 7c2ec71c, which replaces my changes request at d61d4951. Both blockers are fixed.
The two blockers, re-run on the same clips with real FFmpeg/ffprobe 8.0.1
| clip | time | containing frame | d61d4951 |
7c2ec71c |
|---|---|---|---|---|
av.ts (TS, audio leads by 23 ms) |
0, 0.01 | n0 | null |
n0 |
av.ts |
0.05 | n0 | n1 | n0 |
av.ts |
0.1 | n2 | n3 | n2 |
late.mp4 (video +0.5 s) |
0, 0.01, 0.1 | n0 | null |
n0 |
- Before the first frame, the index now clamps to 0, as the engine does.
t-start_ttogether with the observed seek landing makes the threshold independent of where FFmpeg puts zero.
Wider sweep
I added five clips:
gop.ts: MPEG-TS, audio leading,-g 15 -bf 3.- The same TS with
open-gop=1. - An MP4 with the video late and
-g 15. - A fragmented open-GOP MP4.
- VP9 WebM.
Each clip was sampled at fixed early and mid times, then at ±2 ms around every 7th frame boundary across the whole clip. Frames were identified by rgb24 framemd5 against a passthrough decode. The result was 0 mismatches in 407 samples across the 7 clips, including the times where the TS seek lands on a later keyframe and the candidate loop has to walk back.
Other checks
snapshot.test.tspasses 70/70 andtsc --noEmitis clean for the CLI.- The new fixtures cover audio-leading TS, late-video MP4 and open GOP. Those are the cases from my repro.
Non-blocking:
- A held tail without ffprobe still gives
null. That's the limit the body already names, and it's unchanged. - Each extraction can now spawn up to six short
-c copyffmpeg runs: three candidates, each landing plus a confirm. Without ffmpeg, the code falls back to decoding from the file start. Both outcomes are correct. On a long file with many samples the extra runs could add up, but that's not a correctness issue.
— Rames
What
Snapshots select the source video frame whose presentation interval contains the resolved source time. For a 24 fps source sampled at 0.4, 22/30, and 10/30 seconds, that means frames 9, 17, and 8.
Why
Seeking FFmpeg directly to an off-grid time discards the containing frame and returns the next frame. This shifts a baked video plate by one source frame relative to other renderers.
Related work
Fixes #4763. Implements the source-presentation-timestamp approach approved in the issue discussion.
PR #4767 addresses a separate VFX post-injection capture issue (#4762); this change addresses source-frame selection.
How
-c copy, no decode). That probe reads which keyframe the seek actually lands on. The first candidate that lands at or before the frame, and lands on that same keyframe when sought again, is used.select=gte(t-start_t, …)), so the result does not depend on where FFmpeg puts zero for a given container. If no candidate qualifies, it decodes from the start of the file.Both rendered-video injection and
--againstreference extraction use this rule.Trim, playback-rate, automation, loop, and clip visibility time mapping remain in their existing shared runtime paths. A held tail samples the actual last containing frame.
Cost per sample: one ffprobe call for stream metadata and one for packets, up to six short stream-copy landing probes, and one decode from the chosen keyframe. On a 5-minute 1080p clip, a sample near the end took under 0.8 s in both MP4 and MPEG-TS. The 30-second extraction bound remains, and probing has its own 30-second/32 MiB output bound.
Without ffprobe, extraction falls back to a plain seek (the first frame at or after the time), as before. One case is not covered on that path: a held tail past the last frame returns no frame.
Test plan
Snapshot suite: 70 tests passed. They use real FFmpeg extraction, each compared against a full-decode
select=eq(n,k)frame. They cover:Each regression test was confirmed to fail on the commit before its fix.
The producer ffprobe argv contract passes (87 tests).
CLI typecheck passed.
Repository pre-commit checks passed: typechecks, lint/format, fallow (no new findings), tracked artifacts, large files, and commitlint.
No browser pixel reproduction is claimed for this source-extractor change.
Unit tests added/updated
Manual testing performed
Documentation updated (if applicable)
Comments follow CONTRIBUTING.md "Comments"