chore(quest): plan the transcode ladder and audio gap quests - #3347
Conversation
|
Warning Review limit reachedNext included review available in 9 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 (4)
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. |
![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: ce062b0b18
ℹ️ 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".
| the new ones, keep the rest. A subscriber on a retired rung sees its track | ||
| end and picks another rendition, the same as any rendition change. |
There was a problem hiding this comment.
When the retired rung is being consumed by native moq play, the subscriber does not pick another rendition: rs/moq-cli/src/play.rs:248 stops polling catalog updates after video and audio have started, and video_started is never cleared when play_video exits. Ending the rung therefore ends playback or leaves an audio-only session instead of switching, so the plan must include consumer reselection or use a retirement mechanism that preserves the subscribed track.
AGENTS.md reference: quest/AGENTS.md:L39-L45
Useful? React with 👍 / 👎.
| - One stated policy in `decode::Consumer`: a jump counts as a gap when it | ||
| exceeds one codec frame duration at the track timescale, derived from the | ||
| frame size and timescale rather than a constant. |
There was a problem hiding this comment.
Because the preceding text proposes comparing an incoming timestamp with the tracked tail, one missing 1024-sample AAC packet produces a discrepancy of exactly one codec-frame duration, not more than one. The stated exceeds threshold would therefore splice the planned 0 then 2048 regression case instead of detecting its hole, directly contradicting the test at lines 44-45; define the rounding tolerance separately so a one-frame loss still counts as a gap.
AGENTS.md reference: quest/AGENTS.md:L39-L45
Useful? React with 👍 / 👎.
…the bootstrap one Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
ce062b0 to
c28f63c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c28f63ca46
ℹ️ 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".
| - `encode::Producer` subtracts the resampler's skipped frames from its epoch | ||
| so the first published frame is stamped where its input was. |
There was a problem hiding this comment.
Resampler::process already removes the artificial startup silence, and flush restores the delayed tail, while encode::Producer anchors once to the first input timestamp and advances by emitted sample count. There is therefore no fixed delay to subtract from the producer epoch; skipped() is needed by decode::Consumer only because it reconstructs each output batch's time from a later packet timestamp. Implementing this step would shift every nonzero epoch early, and the proposed first-PTS test already passes without the change.
AGENTS.md reference: quest/AGENTS.md:L43-L45
Useful? React with 👍 / 👎.
| - One stated policy in `decode::Consumer`: a packet counts as discontinuous | ||
| when its timestamp differs from the tracked tail by more than half a codec | ||
| frame at the track timescale, derived from the frame size and timescale |
There was a problem hiding this comment.
The fresh half-frame correction still misses valid variable-duration audio: after a 20 ms packet, losing a 2.5 ms Opus packet puts the next timestamp only 2.5 ms past the tracked tail, below a 10 ms tolerance derived from either neighboring 20 ms packet, so the resampler still splices across the loss. Opus permits multiple packet durations and PCM packets can also vary, with no fixed frame duration in the catalog, so base the tolerance on timestamp quantization or otherwise ensure the shortest supported missing packet is detected.
AGENTS.md reference: quest/AGENTS.md:L43-L45
Useful? React with 👍 / 👎.
| - Test: a source that republishes its rendition at a new size; assert the | ||
| ladder changes, passthrough follows, and a subscriber on a retired rung is | ||
| ended cleanly. |
There was a problem hiding this comment.
Fresh evidence in this revision is the new requirement to re-arm moq play, but this test still verifies only ladder mutation, passthrough, and termination of the retired subscription. Those assertions would pass while video_started remains set and native playback never subscribes to the replacement rung, so the quest needs a test that completes the active native video task and confirms a later catalog snapshot starts the replacement track.
AGENTS.md reference: AGENTS.md:L152-L152
Useful? React with 👍 / 👎.
1 participant
Replans the capture/transcode/audio quests in m0 and retires one.
moq_transcode::runbootstraps from it. Closes moq-transcode: run() can't bootstrap from a demand-gated source that doesn't advertise its geometry #2788.decode::Consumer(tolerance of one codec frame at the track timescale), resampler reset, silence inplay_audio, and theencode::ProducerPTS offset from the resampler's skipped delay folded in, sincedecode::Consumeralready compensates and the encoder does not.Part of a per-cluster replan of
quest/m0: every issue-imported quest had the boilerplate "implement what the issue says" goal with the issue body pasted as the plan. Each quest now states its observable outcome and the decisions taken, and drops the pasted body.🤖 Generated with Claude Code
(written by Claude Fable 5.1)