Repository navigation
@remotion/media: Avoid rounding seeks below fractional keyframes - #12132
alec-watts wants to merge 4 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Exact seek timestamp for the frame bank —
makeKeyframeBanknow passesinitialTimestampRequeststraight toVideoSampleSink.samples()instead ofroundTo4Digits(...), so a fractional keyframe boundary like61061 / 30000is no longer rounded down below itself. This is sound: Mediabunny'sgetKeyPacketreturns the last key packet with start timestamp<=the request, so an unrounded timestamp can only select the same or a later keyframe — never the previous GOP — and the sample sink's predecessor handling still yields the requested frame first. - Fractional-boundary regression test — the new
starts at a fractional keyframe without reading the preceding framecase drives the real bank with a boundary-aware mock sink and asserts only the requested frame is consumed and each emitted sample is closed exactly once. It correctly fails against the pre-fix bank (which also consumes the2.002predecessor).
I traced the changed path through extract-frame.ts → keyframe-manager.ts → keyframe-bank.ts and confirmed the timestamp handed to samples() is the requested frame's presentation time, and that the existing rounded comparisons used for in-bank frame matching are untouched.
deepseek-v4.1-flash ｜ 𝕏
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a394985c2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const sampleIterator = videoSampleSink.samples( | ||
| roundTo4Digits(initialTimestampRequest), | ||
| ); | ||
| const sampleIterator = videoSampleSink.samples(initialTimestampRequest); |
There was a problem hiding this comment.
When a keyframe's stored timestamp is rounded upward by a millisecond-timescale container, the raw composition timestamp falls just before that keyframe. For example, a 30 fps WebM keyframe stored at 0.067 is requested as 2 / 30 (0.06666…); passing that value to samples() starts decoding from the previous GOP, while the previous roundTo4Digits() call landed on 0.067. The bank still returns the right frame, but each new bank/seek can now decode an entire unnecessary GOP for these tracks, so this needs a one-sided or timescale-aware tolerance rather than removing the rounding unconditionally.
Useful? React with 👍 / 👎.
JonnyBurger
left a comment
JonnyBurger
left a comment
There was a problem hiding this comment.
The fractional MP4 improvement is valid, but removing seek rounding unconditionally introduces the opposite performance regression for millisecond-quantized WebM timestamps.
I independently reproduced the counterexample with a real 30 fps VP9 WebM, 130 frames, and GOP length 62:
ffmpeg -f lavfi -i testsrc2=size=128x72:rate=30 -frames:v 130 \
-c:v libvpx-vp9 -g 62 -keyint_min 62 quantized.webmFrame 62's composition timestamp is 62 / 30 = 2.066666666666667, but its stored keyframe timestamp is 2.067. Querying Mediabunny's EncodedPacketSink.getKeyPacket() with verifyKeyPackets: true gives:
| Opening seek | Selected keyframe |
|---|---|
Current rounded seek: 2.067 |
2.067 |
PR's raw seek: 2.066666666666667 |
0 |
The raw seek therefore starts in the preceding GOP and requires decoding through 62 earlier frames. This verifies keyframe selection on an actual file; it is not a native decoder-call or latency measurement. The bank's existing rounded comparisons should still select the intended output frame, but they cannot recover the decoder work already spent reaching it.
Please preserve upward seek tolerance while preventing downward rounding, and add a regression covering this WebM case alongside the fractional MP4 case. A candidate is Math.max(initialTimestampRequest, roundTo4Digits(initialTimestampRequest)); validate it for both boundaries and requests just before a keyframe before choosing the final implementation.
alec-watts
changed the title
@remotion/media: Avoid rounding seeks below fractional keyframes
This branch was successfully deployed
3 participants
Rounding the opening timestamp downward can move a seek just before a fractional MP4 keyframe. Passing the raw timestamp avoids that extra GOP, but regresses WebM when its millisecond-quantized keyframe timestamp lies slightly above the composition timestamp.
Use
Math.max(initialTimestampRequest, roundTo4Digits(initialTimestampRequest)): preserve the existing upward tolerance, while never rounding the opening seek below the exact request. Later bank comparisons and frame-matching tolerance remain unchanged. This addresses the WebM counterexample in the maintainer review.Native decoder verification
Fresh comparison against this branch's source, public Mediabunny 1.61.3, and native WebCodecs in hidden Electron. Each variant ran three trials for each of eight requests: the MP4 and WebM boundaries, requests 0.1 ms before their composition boundaries, requests 2 ms before the stored keyframes, and zero-time controls. 72 delivered frames matched full RGBA hashes from independent
VideoSampleSink.getSample()references. All measured decoders were closed after bank/input disposal.VideoDecoder.decode()calls at the boundary61061 / 3000062 / 30, stored at2.067Counts were identical across all three trials per case. The MP4 improvement remains 61 avoided decode calls; the WebM regression is removed. The revised WebM seek also preserves 42 calls just before the boundary. MP4 requests just before its exact keyframe still require the earlier GOP, and requests 2 ms before either stored keyframe still deliver the preceding frame. Zero-time controls are unchanged. This measures decoder work for these fixtures, not editor latency or a general playback speedup.
Fixture generation:
Validation
VideoSampleobjects and a boundary-aware sink, checks consumed frames, and checks samples close exactly once.git diff --checkpass.Why each file changes
packages/media/src/video-extraction/keyframe-bank.tspackages/media/src/test/keyframe-bank.test.tsDiff cleanup found no unrelated formatting, dependencies, parallel decoder mechanism, or unused exports.
Shared CI test repair
Includes the one-line caption-inspector track selector fix from #12179. Current main fails this test before it can exercise caption persistence. Both caption-inspector E2E cases pass in the actual Studio with this repair; restoring the old selector reproduces the failure.
packages/example/e2e/captions-inspector.test.mts