chore(quest): fold in the review findings the m0 replans merged past - #3350
Conversation
Address the remaining review findings on #3346: the open-GOP tests now include a latency skip into a later GOP, and the recovery_frame_cnt > 0 case gets its own quest instead of a sentence that would vanish with the parent. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…e encoder epoch Address the remaining review findings on #3347: a half-frame tolerance cannot detect a lost 2.5 ms Opus packet after a 20 ms one, so the gap policy keys on timestamp quantization; the encoder already anchors to its first input and the resampler drops its own startup delay, so the skipped() correction is replaced by a pin; and the ladder test now proves moq play reselects a rung. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…s on flush Address the remaining review findings on #3349: catalog finality is expressed by the hang publisher rejecting with NotFound rather than by moq-net folding it into the drop path; the smoke client drains every callback registration before returning; and a pad joining a new moqsink generation replays its sticky caps and rebuilds its producer before the first buffer. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 3 minutes. View limit detailsLimit details: You’ve used all 4 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (8)
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 |
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. |
…play Codex review on #3350: a gradual refresh needs every picture from the recovery point decoded to build the recovery picture's references, so the consumer suppresses presentation rather than skipping input, and finds the recovery picture by frame_num progression per H.264 D.2.8 and RFC 6184. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
![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: 1ae934eb50
ℹ️ 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".
| - On the first group after a non-continuous transition, the consumer skips | ||
| every picture until the recovery picture, in JS and in the Rust decode path, |
There was a problem hiding this comment.
When recovery_frame_cnt > 0, recovery depends on decoding the access unit carrying the SEI and the intervening pictures so they can rebuild reference state. In the inspected JS path, the container consumer is upstream of VideoDecoder, so implementing this instruction by skipping those encoded pictures prevents the decoder from reaching the advertised recovery point. Feed them into the decoder and suppress their decoded output until recovery instead, retaining the recovery picture itself as the first safe output. Because this quest's Plan is implementation guidance, the distinction needs to be explicit here.
AGENTS.md reference: quest/AGENTS.md:L43-L45
Useful? React with 👍 / 👎.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 133263b7c4
ℹ️ 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".
| unsafe pictures sit at or after the keyframe timestamp. Recovery points with | ||
| `broken_link_flag` set are out of scope: there even a continuous viewer must | ||
| withhold display (H.264 D.2.8), which is a different rule with its own plan. |
There was a problem hiding this comment.
When an H.264 stream sets broken_link_flag, this text identifies a separate correctness bug affecting continuous viewers but leaves it out of scope while claiming it has its own plan. A search of quest/ at this commit finds no other mention of the flag, so completing and deleting the two open-GOP quests would lose this known work. Add a separate related quest, or include the flag in this quest's recovery metadata and display rules. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L98-L104
Useful? React with 👍 / 👎.
1 participant
#3346, #3347, and #3349 merged while their last review findings were still being addressed. These are the fixes for the findings that stand; each was verified against the code before editing.
recovery_frame_cnt > 0gets its own quest,quest/m0/open-gop-gradual-recovery.md, instead of a sentence that vanishes when the parent is deleted.encode::Producerskipped()correction is replaced by a pin: the producer already anchors to its first input andResampler::processdrops the startup delay, so subtracting it would shift PTS early. The ladder test now provesmoq playreselects a rung.NotFound, keeping moq-net's documentedDroppedsemantics for requests lost to failures; the C smoke client drains every callback registration before returning, since the consume registrations retain the stack context past session close; and a pad joining a new moqsink generation replays its sticky caps and rebuilds its producer before the first buffer, orpush_bufferdrops it.quest checkpasses.🤖 Generated with Claude Code
(written by Claude Fable 5.1)