Repository navigation
quest(m1): plan failing an export on a source it cannot follow - #5437
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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. |
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 1 minute. View limit details
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 |
![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: b75eb1dcfb
ℹ️ 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 check lives in the shared jitter buffer, so TS, FLV, and MKV export | ||
| and audio decode fail the same way. |
There was a problem hiding this comment.
Implementing this check in rs/moq-mux/src/jitter.rs will affect the TS, FLV, and MKV exporters, but not audio decoding: rs/moq-audio/src/decode/consumer.rs uses its own crate::jitter::{self, Target}, while the mux buffer is private to moq-mux. Either remove audio decode from the promised behavior or explicitly plan a corresponding implementation and regression test in moq-audio; otherwise this quest can be marked complete while one stated consumer still silently behaves differently.
AGENTS.md reference: AGENTS.md:L73-L74
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. moq-audio decode uses its own delay estimator (rs/moq-audio/src/jitter.rs), not moq_mux::jitter::Buffer, which only the TS, FLV, and MKV exporters use. Fixed in 7bbe94e by scoping the quest to the mux exporters and naming audio decode as out of scope, matching the decision to put the check in the shared buffer.
(Written by Claude Opus 5.5)
Audio decode uses its own delay estimator in moq-audio, not the shared mux jitter buffer. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Merge summary
Public API: none. Wire: none. (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 7bbe94e against base 4a6304a, including the full plan and audio-scope correction. The narrower exporter scope resolves the existing finding (#5437 (comment)). No outstanding actionable code bug established.
One non-blocking clarification remains at quest/m1/slow-source.md:47: the -1382.60 ppm result is the external grader's PCR-versus-arrival fit, not a confirmed jitter-buffer source-rate measurement. The linked ts-live-clock investigation still leaves source timing, pacing, backpressure and measurement effects unresolved. Make the predicted loud failure conditional on reproducing out-of-tolerance source measurements and the half-delay condition, rather than assuming that diagnosis.
Fail-fast behavior fits the existing buffer error contract; keep the early-stall regressions before removing the steered-drift gate. Static GitHub review of both changed documents, related jitter/exporter/audio code and discussion. No tests run.
Plans a follow-up from #5435. Before #5292, the TS export measured a source running -222,000 ppm, dropped 1,273 frames as late, and exited clean.
jitter::Bufferalready bails on an out-of-tolerance source, but only once its steered clock reaches the 30 ppm limit, which takes about 3 hours at the spec's slew rate.Adds
quest/m1/slow-source.mdand links it from the m1 README.Decisions
Goal: an export whose source arrives too far off real time for the output to follow ends promptly with an error naming the measured rate.
Where does the check live?
What trips the error?
Which act?
Relation to m1 ts-live-clock (a local -1382 ppm run that exited clean)?
Landing
Docs beyond inline fixes?
Public API: none. Wire: none.
(Written by Claude Opus 5.5)
🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need.