Repository navigation
quest: Opus holds its codec floor instead of disabling its rendition - #8135
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
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 3 minutes. 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. |
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: f6ec3d5
Base: 3eb044f
No actionable findings in this plan-only change.
Direction: keeping Opus enabled at its codec floor removes the planned demand-loss feedback loop without adding passive subscriptions or a separate recovery mechanism. This fits the existing shared rate policy: its lowest/only rendition holds the minimum, while rung disabling requires a lower rendition. The plan consistently updates the goal, decision and below-floor test expectation, retains a fixed reservation ceiling, and leaves PCM reserve-only.
Verification: GitHub-only static review of the complete changed quest and the relevant shared rate policy and Opus bitrate setter. No implementation changes are included. I did not run quest context/check, tests, or network impairment experiments; encoder adaptation and recovery remain future implementation work. This is a COMMENT, not an approval.
(Written by review (OpenAI))
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ Minor: the new decision's recorded reason conflicts with the tree's recovery design. The decision itself is sound.
Reviewed changes
- Below-floor Opus behavior: the Goal, Plan and Tests now hold the encoder at the codec floor and leave the excess to the transport, keeping the rendition enabled instead of disabling it.
- Recorded decision: a new 2026-11-07 maintainer decision block replaces the 2026-10-04 disable-and-re-enable rule, listing all four options with the chosen one marked ✅.
- Removed stale detail: the "viewers with no enabled alternative hold it" sentence and its push-to-talk link are gone, along with the hypothetical-share recovery sentence.
deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) ｜ 𝕏
…d rendition Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Corrected decision rationale: the 2026-11-07 block now records the catalog flip as the reason to stop disabling, citing
an_idle_track_claims_nothingto show recovery alone would work, and ties the cost to theenabled-toggle latency #7942 removed.
deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) ｜ 𝕏
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 54d25ae
Base: 3eb044f
Incremental review since f6ec3d5; the base is unchanged.
No actionable new findings.
The corrected rationale is supported by the source. An idle reservation receives its hypothetical rejoin share (rs/moq-net/src/model/bandwidth.rs:1070–1083 and the test at 1442–1462), so my earlier review's demand-loss feedback-loop explanation was incorrect. Disabled audio is deselected by the browser player (js/watch/src/audio/source.ts:103–118), and native playback stops disabled tracks (rs/moq-cli/src/play/media.rs:189–203). Keeping the rendition enabled therefore avoids subscription/playback churn rather than repairing bandwidth recovery.
Direction: retain the chosen codec-floor behavior and the existing shared rate policy; no extra reservation or passive-subscription recovery mechanism is needed for this rationale. The implementation and its adaptation proof remain future work.
Verification limits: GitHub-only static review of the complete quest, the incremental correction, and relevant allocator/player paths. No code changes are part of this PR; I did not run tests, quest validation, or playback/network experiments. This is a COMMENT, not an approval.
(Written by review (OpenAI))
Follow-up from moq-dev/moq#8123, planned through
/quest-planin the 2026-11-07/quest-completerun.Problem
quest/m5/2848-follow-the-bandwidth-grant-in-moq-audio-instead-of.mdhad a grant below the Opus codec floor disable the rendition (catalogenabled: false) until bandwidth recovered. After the push-to-talk work, players deselect a disabled rendition, so each flip makes every listener drop the track and resubscribe, losing what it was playing. That is theenabled-toggle cost moq-dev/moq#7942 removed. Recovery itself would still work, since an unwatched reservation still reads its slice (pullfrog's finding, now recorded).Change
moq_mux::rate::Control's rung-disable rule no longer applies to audio.Decisions
Decided 2026-11-07 (maintainer): how a below-floor Opus grant behaves.
Public API: none. Wire: none.
Review
Codex (
quest review --base origin/main), gpt-6.1-sol, medium effort, session01a1b4a8-36e1-72f3-b0cc-03bbe1fda81e, round 2 on 54d25ae603: no findings. Round 1 on f6ec3d5 also had none. Pullfrog's finding on the recorded reason was fixed in 54d25ae.quest checkpasses.(Written by Claude Opus 5.5 with high effort)
🤖 Generated with Claude Code