cute_h264.h: place MP4 pictures by ranking order counts, not halving them - #603
Merged
Merged
Conversation
…them The muxer took a picture's order count divided by two as its display slot. The specification numbers fields, so most encoders step the count by two per frame, but VideoToolbox steps it by one and reorders, so every two frames shared a slot and clips played at twice speed. Order counts only have to sort pictures, so rank them within each keyframe group and give every picture its own slot; a step of one, a step of two, and gaps all come out right, and the software encoder's output is unchanged. Fixes RandyGaul#602 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NEv4Z3RZ5msceW87XGkqoB
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The newly added qsort comparator in test_video.cpp uses subtraction (x - y), which can overflow and violate comparator ordering requirements.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes MP4 muxer presentation timing when H.264 picture order counts (POC) don’t step by 2 per frame (e.g., VideoToolbox stepping by 1 with reordering), by ranking pictures within each IDR-delimited group instead of deriving display slots from POC/2.
Changes:
- Update MP4 muxer sample placement to treat POC as an ordering key and convert to contiguous display positions via per-GOP ranking.
- Add MP4 timeline tests that parse
stts/ctts/elstto validate contiguous presentation slots (including step=1, step=2, gaps, and wraparound cases). - Update macOS VideoToolbox path commentary to reflect verified behavior and the muxer fix.
File summaries
| File | Description |
|---|---|
| test/test_video.cpp | Adds timeline-parsing tests to validate MP4 presentation order/lead and introduces a fill helper for variable sizes. |
| src/cute_video.cpp | Updates documentation comment for the VideoToolbox backend and references the muxer’s ranking approach and new test. |
| libraries/cute/cute_h264.h | Implements per-keyframe-group ranking of POC values into contiguous display slots; improves error propagation in ch_mp4_wrap. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The qsort comparator in the timeline check returned x - y, which can overflow when the values are far apart. The times it sorts here are small, but the muxer's own comparator already uses the sign-of-difference form, so use it here too. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NEv4Z3RZ5msceW87XGkqoB
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #602.
The muxer took a picture's order count divided by two as its display slot. The specification numbers fields, so most encoders step the count by two per frame, but VideoToolbox steps it by one and reorders, so every two frames shared a slot and the clip played at twice speed. Order counts only have to sort pictures, so the muxer now ranks them within each keyframe group and gives every picture its own slot. A step of one, a step of two, and gaps all come out right, and the software encoder's output is unchanged.
Verified on an M3 Max with a 30-frame VideoToolbox clip: before, ffprobe showed 15 distinct presentation times with the last at 0.467 s of a 1.0 s file; after, 30 distinct times ending at 0.967 s.
CI notes: the muxer-only test feeds hand-built streams to ch_mp4_wrap, which shared builds do not export, so it runs in static builds only. The public-API test's hardware-size case is skipped on Windows, where Media Foundation writes its own container.
🤖 Generated with Claude Code
https://claude.ai/code/session_01NEv4Z3RZ5msceW87XGkqoB