Repository navigation
fix(engine): a 29.97 fps camera clip exports smoothly at 30 fps - #5849
Conversation
Extraction samples each output frame from the last source frame starting at or before it. Millisecond container ticks move each frame by up to half a millisecond, while 29.97 fps drifts against 30 fps by 0.033 ms per frame, so the rounding decided the slot and the export alternated repeats and skips. Constant-rate frames are now re-spaced on the stream's own frame grid from the first decoded frame before sampling. The cache schema moves to v8 so exports stop reusing frames extracted the old way.
Edit accuracy: accurate 271 (base branch 271), smooth 169 of thoseThe gate passes. Quarantined, measured but not gated (0) |
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Reviewed at aca46340. Approving: it fixes the P0 and leaves every other source byte-identical. One residual is worth a follow-up; details below.
The math: after the existing half-tick shift, a CFR source (!isVFR) has its timestamps re-snapped to its own frame grid: settb=AVTB, then setpts = STARTPTS + round((PTS−STARTPTS)·TB·FR)/(FR·TB). Rounding to the nearest grid point is monotonic, so frames can't reorder. Up to ±0.5 ms of container rounding becomes one constant offset carried by the anchor frame, and the 29.97 → 30 conversion then repeats once per ~1001 slots instead of alternating. FR is the link rate (r_frame_rate, 30000/1001 on the fixture; avg_frame_rate is 599500/20003 there, which is why the nominal rate is the right one). isnan(FR) passes timestamps through unchanged. VFR sources (ffprobe.ts:761, r vs avg > 10%) skip the new filters entirely.
Checked with real ffmpeg (n8.0.1), main's chain vs this head's chain, PNG indices decoded as in your test:
- 29.97 fps, 1/1000 tick, 1.5 s at 30 fps from 0 s: main repeats at slots 7, 10, 13, 15, 18, 21, 24 and skips at 8, 11, 14, 17, 20, 23. This head: one repeat (slot 16), no skips. From 0.5 s: main has 3 repeats and 3 skips; this head has 1 repeat and none. So the PR's sequence reproduces and the fix holds.
- Unchanged for already-correct clips: I compared md5 of 10 s windows at seeks 0, 0.5 and 1.7 s. These are byte-identical to main: 29.97 fps with exact 1/30000 ticks, 30 fps with 1/1000 ticks, 30 fps with exact 1/15360 ticks, and 25 fps with 1/1000 ticks. Only a rounded-rate millisecond-tick source changes (23.976 fps on 1/1000 ticks), which is the intended case. The schema bump to
hfcache-v8-follows the rule for that. - First frame: the anchor is the first decoded frame, so a seek keeps its offset. With
-noaccurate_seekthat is the keyframe before the seek point. At a 0.5 s start, slot 0 shows frame 15 (true start 500.5 ms), not 14. That falls inside the existing half-tick tolerance, which treats a frame within half a tick as starting at the slot. Main shows 14 there. Not blocking.
Residual (follow-up, not blocking): the anchor is per ffmpeg process, so segments are no longer byte-identical to a single pass for these sources. The comment above EXTRACTION_SEGMENT_SECONDS says they are. Each segment's STARTPTS is a different keyframe, each with its own ±0.5 ms millisecond rounding, so the grid shifts by up to about 1 ms per segment, which moves the periodic repeat by up to about 30 slots. On a 40 s 29.97 fixture with 1/1000 ticks, a segment starting at 33 s put the repeat 4 slots from where the single pass put it. I spliced a single pass at frame N with a segment starting at N/30, for 11 values of N:
- At N = 1025 the seam itself skips a frame and the window carries 2 repeats. That's the original glitch, once.
- At the other 10 seams there's no skip.
This hits runSegmentedExtraction (ranges > 120 s, frameRanges cache fills), and distributed chunks, which extract their own ranges, so a chunk seam can do the same. It's far rarer than main's per-second stutter, which is why I'm not blocking. A fix would anchor every process on the same absolute grid, for example by passing each process's -ss (plus the stream's first-frame time) into the setpts expression instead of using STARTPTS. A splice test like the one above would pin it down.
Tests: the new test decodes each extracted frame's index and requires no skip and ≤ 1 repeat over 1.5 s. My main-chain run above shows that fails on main, as the PR body says. The commit aca46340 is signed.
CI is green at this head (CI run 39141918404, Windows 39141918429). All required checks pass, except Skills: manifest in sync, which was skipped because no skill files changed. In Test (runtime), videoFrameExtractor.test.ts ran 202 tests in 17.8 s, and extractionCache.test.ts ran 41.
No live run beyond the ffmpeg filter-chain comparisons above.
— Rames
The grid was anchored on each process's first decoded frame, which carries its own millisecond rounding, so a segment or distributed chunk could place the 29.97 -> 30 repeat elsewhere and skip a frame at its seam. Frames are now snapped to the stream's own grid in file time (this process's seek plus the video's lead), the same in every process.
|
Folded in your residual at New test: "samples a millisecond-tick 29.97 fps source the same in segments as in one pass" extracts 12-frame segments at 14 starts (4 … 201) and requires every frame to match a single pass. At |
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Re-review at cef94496. The anchor residual from my approval at aca46340 is fixed, so I'm approving again.
What changed: the snap no longer anchors on each process's STARTPTS. Every process now snaps time since file start, PTS·TB + seek, to lead + k/FR, where lead is the existing videoStreamLeadSeconds and seek is the process's own -ss. Segments get it through filterAndEncodeArgs(seek), and the single pass gets filterArgsFor(startTime). The snap now runs before the half-tick shift, in AVTB. After snapping, the shift is 0.5 µs, which still covers the ≤ 0.5 µs rounding of k/FR to whole µs, so an exact grid frame stays at or before its slot. VFR sources and an unknown FR skip the snap as before. finalFrameOnly never samples (sampleAtOutputFps = !finalFrameOnly), so the output-seek path isn't affected.
Re-checked with real ffmpeg (n8.0.1) using this chain:
- Segments vs single pass: on the 40 s 29.97 fps fixture with 1/1000 ticks, I compared 16 segment starts (3, 4.4, 7.4, 9.9, 11, 13.9, 15.2, 17, 19.3, 21.7, 23, 26.1, 27.3, 29.5, 31.1 and 33 s) against one pass. All 16 had 0 differing slots. At
aca46340, the 33 s segment differed in 4. - Already-correct clips: 29.97 fps with exact ticks, 30 fps with millisecond and exact ticks, and 25 fps with millisecond ticks are all still byte-identical to main at seeks 0, 0.5 and 1.7 s.
- The fix: 29.97 fps on 1/1000 ticks from 0 s shows one repeat (frame 0 at slots 0 and 1, since frame 1 truly starts at 33.37 ms, after the 33.33 ms slot) and no skips. From 0.5 s there are no repeats and no skips, and frame 14 sits at slot 0, which is its true time.
One edge, non-blocking: for a seam at frame 1025 (seek 34.1667 s), the keyframe is stored at 34 167 ms, 0.33 ms after the seek. ffmpeg's seek lands on it, so frame 1023, which owns that slot, is never decoded, and the segment's slot 0 shows 1024. It's a seek-geometry case, not the snap: it needs a keyframe within about 1 ms after a segment start. Main hides it only because its half-tick shift moves 1024 back to the slot. Seeking each segment one frame early (seek − 1/fps) and letting -frames:v/start_time keep its own slots would close it, but I don't think it needs to block this P0.
The new test compares segmented extraction against one pass slot by slot on a 29.97 fixture with millisecond ticks. Both commits are signed.
CI: the check list shows "skipping" because duplicate runs 39144409507 and 39144409517 skipped every job. The real runs at this head passed: CI 39144394989 (Test, Test (runtime), Typecheck, Build, Test: runtime contract, Studio: edit accuracy gate) and Windows 39144395001 (Render on windows-latest, Tests on windows-latest). videoFrameExtractor.test.ts ran 203 tests, which includes the new segment-vs-single test.
No live run beyond the ffmpeg filter-chain comparisons above.
— Rames
What
A 30 fps export of a camera clip shot at 29.97 fps plays smoothly. Before this, when the clip's container stores timestamps in milliseconds (common for phone and camera files), the export alternated repeated frames with skipped ones for the first second or so. On a 1.5 s test clip the source frames came out as
0,1,2,3,4,5,6,6,8,9,9,11,12,12,14,14,15,17,…. Later exports of the same clip kept the bad frames because extracted frames are cached.Why
Extraction fills each output slot with the last source frame starting at or before it (
fps=…:round=up, after a half-tick shift that absorbs whole-ms rounding when source and output rates match). Millisecond ticks place each frame up to ±0.5 ms from its true time. A 29.97 fps source drifts against 30 fps slots by only 0.033 ms per frame, so for the first ~15–25 frames that rounding noise decides which side of a slot each frame lands on: repeat, skip, repeat, skip. With exact timestamps the same clip samples correctly.How
isVFR), extraction snaps frame times to the stream's own frame grid in file time before sampling:settb=AVTB, setpts = (lead + round((PTS·TB + seek − lead) × FR) / FR − seek) / TB. Hereseekis this ffmpeg process's-ssandleadis where the video starts in the file (videoStreamLeadSeconds).PTS·TB + seekis the frame's time since file start in every process, so single passes, segments and distributed chunks all snap to the same grid.FRis ffmpeg's nominal stream rate (30000/1001), not the averaged rate, which drifts on long millisecond-tick files. When ffmpeg has no constant rate (FRundefined), timestamps pass through unchanged. The existing half-tick shift runs after the snap.SCHEMA_PREFIXmoves tohfcache-v8-, as its rule requires when extraction writes different frames for the same key, so warm caches stop serving the old frames.Test plan
videoFrameExtractor.test.ts"samples a millisecond-tick 29.97 fps source the same in segments as in one pass" compares 14 segment starts against a single pass. It failed when each process anchored on its own first frame (15 slots differed), and passes now.videoFrameExtractor.test.ts"samples a 29.97 fps source with millisecond timestamps at 30 fps without skipping a frame" synthesizes a clip whose frames carry their own index as binary blocks, extracts 1.5 s at 30 fps, and requires no skipped frame and at most one repeat. On main it fails with the sequence above. With the fix it passes 3 runs in a row, and the extractor and cache suites pass (268).render --fps 30from this branch.data-media-start="0.5": one repeat.