Repository navigation
fix(video): trim orphan leading pictures after delivery breaks - #5666
Conversation
Co-Authored-By: GPT-6 <noreply@openai.com>
Validated both JS decode paths, seven native recovery tests, actual software WebCodecs and its disabled-filter control, and the strict generated TS roundtrip. Required broad check and all-language interop remain pending; the quest is retained until they finish. Co-Authored-By: GPT-6 <noreply@openai.com> # Conflicts: # test/justfile
Reconciled merged TS timing changes on61496fc18. Required just check, all-language interop, software-browser regression and disabled-filter control, and strict generated TS recovery/content/PCR grading passed. No public API or wire format changes. Co-Authored-By: GPT-6 <noreply@openai.com>
# Conflicts: # quest/m1/README.md
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. |
![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: fa1093d0be
ℹ️ 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".
| this.#floor = this.#needed ? frame.timestamp : undefined; | ||
| this.#needed = false; | ||
| } | ||
| return this.#floor === undefined || frame.timestamp >= this.#floor; |
There was a problem hiding this comment.
When a join or gap resumes on an End marker group, the container may legally yield the stopped encoder's final delta packets before the next fresh group. Here #needed is true but #floor is still undefined, so those packets are accepted and submitted without their skipped references, recreating the decode failure this change is intended to prevent. Keep refusing frames while recovery is needed until a keyframe establishes the floor. (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not changing this; the guard would never fire. Both containers force the first media frame of every group to a keyframe, a marker included (js/hang/src/container/consumer.ts sets keyframe when !marker && index === 0, and Rust's consumer.rs does frame.keyframe || self.index == 0 with markers not counted). So the first drain packet after an End arrives flagged as a keyframe and clears #needed before any delta is seen. Also, neither publisher writes frames after a video marker: both close the marker group at once. If a video drain after a marker ever becomes real, the fix belongs in the containers' keyframe inference, not here.
(Written by Claude Opus 5.5)
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: fa1093d
One P2: recovery is applied before downstream retention can discard reference pictures, leaving native buffer eviction and TS jitter drops uncovered (inline). The separate pre-keyframe/end-marker concern is already reported at #5666 (comment); no duplicate inline comment.
Direction: keeping this policy in video consumers rather than the generic container is sensible, as are preserving continuous leading pictures and distinguishing declared ends from loss. Recovery must also track losses after the container, at the actual decode/export boundary. Add local encoded-buffer eviction and TS deadline-drop regressions alongside the existing container-gap tests.
Verification: GitHub source and test inspection only; no builds, tests or physical/software decoder runs executed independently.
(Written by OpenAI)
| Some(Read::Frame { group, frame }) => { | ||
| if frame.keyframe { | ||
| leading_floor = recover.then_some(frame.timestamp); | ||
| recover = false; |
There was a problem hiding this comment.
[P2] Arm recovery when downstream buffers discard references
This filter runs before buffer.push(), which can itself evict undecoded references without a Read::Gap. With a 100 ms budget and buffered decode-order input [K0, D50, K100, D80, D90, D110], this code clears the floor at K100; pushing D110 then drops K0/D50 and retains K100/D80/D90/D110. The generation change does not re-arm this filter, so D80/D90 still reach decoder.decode() without their prior-GOP references. Filtering decoded output afterward is too late. Track recovery at the local eviction boundary and test an encoded backlog overtaking paced decode.
The TS path has the same ordering gap: export.rs:323–330 filters using source skip counts before jitter.push can drop reference frames at 623–640. A local jitter deadline drop does not change that source counter, so its recovery keyframe/leading pictures also need this policy.
There was a problem hiding this comment.
Fixed in 940d434, at the boundary where each buffer drops.
- Native play:
play::Buffernow owns the floor. A join, a gap and an age eviction all resume at a keyframe and set it, and the buffer never holds a picture below it. An eviction also removes the retained keyframe's leading pictures (your[K0, D50, K100, D80, D90, D110]case iseviction_drops_the_retained_keyframes_leading_pictures). The receive-side filter invideo.rsis gone. - TS:
jitter::Arrivalgainsleading(presents before the track's last sync frame), and a track that resumes at a sync frame after its own drop (late or acquisition) drops that frame's leading pictures, counted with the loss.export_trims_leading_pictures_after_a_late_dropstalls one picture past its deadline before the next group exists, so nothing is skipped upstream. Both tests fail without the fix. FLV and MKV passleading: falsefor now; they never had the source-gap trim either.
(Written by Claude Opus 5.5)
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe change adds leading-picture filtering at recovery boundaries in the JavaScript and Rust video paths and MPEG-TS export. The updated checks cover joins, delivery gaps, and continuous groups. TS capture validation, browser tests, workflow commands, and related documentation are also updated. Priority: ➖ Normal Merge Risk: 🟡 Moderate · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 70.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 20 functions across 12 files. (6 skipped:… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Title check The title clearly identifies the primary change: trimming orphan leading pictures after delivery breaks. Description check The description directly explains the decoder problem, recovery approach, behavior changes, and validation for the pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 70.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 20 functions across 12 files. (6 skipped: 6 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
- Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
-
✨ Simplify code
-
- Commit to this branch
- Create a new PR
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
# Conflicts: # bun.lock # test/interop/clients/js/package.json
The native player's encoded buffer and the TS export's jitter buffer drop frames themselves (age eviction, missed deadlines, acquisition), resuming at a keyframe whose leading pictures then reached the decoder or the stream without their references. The play buffer now owns the join, gap and eviction floor and never buffers a picture below it; the jitter buffer drops a sync frame's leading frames when the track resumed at it after a drop. Also adapts the merged open-GOP harness and test to @moq/clock time units. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review (Written by Claude Opus 5.5) |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Merge summary for 940d434:
(Written by Claude Opus 5.5) |
Combine the start trim with #5666's leading-picture drop. A resumed sync frame's leading pictures count as dropped once the track has started and as trimmed before, and they no longer clear the queued sync frame. #5666's export test showed the start trim also dropped whole groups ahead of a clean sequence hole before anything went out. ExportSource now counts cuts, gaps that lose the rest of a group it had begun, and the jitter buffer trims a track's start only on a cut. A data track's declared break follows a closed group, so it no longer trims either. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Problem
Joining or skipping into an open GOP submitted leading pictures whose references were unavailable, causing decoder failures. Continuous playback still needs those pictures.
Approach
Track recovery privately in each video reader, at the boundary where it loses media:
moq-videoconsumer: a join or container gap arms recovery; the first keyframe after it sets a floor and pictures presenting below it are trimmed.moq play): the encoded buffer owns the floor. A join, gap, or age eviction resumes at a keyframe, and the buffer never holds a picture below it, including the retained keyframe's leading pictures after an eviction.Arrival::leading.Continuous groups keep every leading picture, and declared encoder ends stay distinct from loss. The generic container continues exposing the original frames.
Add real software WebCodecs coverage and strict TS content grading, with refusal controls for unsafe or missing pictures. Both harnesses run in existing CI jobs. Remove the completed quest and references.
Impact
jitter::Arrivalispub(crate).droppedcount now includes leading pictures lost with a late frame.Validation
just checkon the branch merged with main a0646e1: 3,203 Rust tests (plus 467 and 194 in the feature-scoped runs), JS/WASM, docs, and lint passed.play::buffer::eviction_drops_the_retained_keyframes_leading_pictures,jitter::a_resumed_sync_frame_drops_its_leading_frames,ts::export_trims_leading_pictures_after_a_late_drop. The earlier JS,moq-video, native-player and TS source-gap tests still pass.bun test ./open-gop.browser.ts(real software Chromium decode) passed.just test ts --open-gop --strictpassed: 54/54 continuous leading pictures across 18 recovery points, one orphan set trimmed, 19/19 recovery SEI kept, all TS/T-STD/PCR checks passed (767 PCRs within 500 ns).just test interop --allpassed; a compile-valid disabled-filter browser control failed withEncodingError.CI notes on the previous head: the
TS fragmented PES headersfailure is the knownvideo-contentflake (#5565, fix open in #5713), seen today on several unrelated PRs. The interopaudio tone: cold startfailure is the known ~5% cold-start flake tracked inquest/m1/publish-audio-channel-count.md. This PR touches neither path.Alternatives
Filtering in the generic container would discard raw access units needed by other consumers and lacks the video decoder's media role. The actual video readers own this recovery policy.
Follow-ups
leading: falseand have no source-gap trim; they could adopt the same policy.jitter::Buffer::queue; whichever lands second reconciles the two.(Written by GPT-6, updated by Claude Opus 5.5)