Repository navigation
feat(rate)!: native publishers disable simulcast rungs the grant cannot fund - #6781
Conversation
Quest-Claim: 7d0f7f4d11c3477b
Policy::lower sets a band boundary below which a rendition disables, once a session probes for its reservation. Idle reservations read the share they would get back, so a disabled rung resumes after its viewers left. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
MoqVideoProducerOptions::lower names the next lower rendition: the rung takes the priority below it, stops encoding and advertises itself disabled below its band, and reopens on a keyframe once the grant recovers. MoqReservation::probed reports whether a session probes for it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
moq_encode_video takes the next lower rendition's handle, mirroring MoqVideoProducerOptions::lower. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Mirrors the Rust allocator: an unwatched track claims nothing from its siblings but its own reservation reads the slice it would get back. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Python, Swift, and Go take a lower producer; Kotlin and Dart alias the regenerated options. Docs and MIGRATE notes cover the rung and the idle grant, and grant tests wait for their subscription to land. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
changed the title
Replace the positional bandwidth and lower handles with a struct that holds both, validated before the encoder opens. NULL or zeroed keeps the unadapted behavior, and new fields append. The quest notes the BBR3 max_bw aging it depends on. 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
No actionable comments were generated in the recent review.
 |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 78.26% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 184 functions across 23 files. (8 skipped… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title names the main change: native publishers (Rust, FFI, C) disable simulcast renditions whose bandwidth grant cannot fund them. The changeset adds this rung-disable behavior in moq-mux rate con… Description check The description matches the changeset. It links the quest, explains the rate rule (Policy::lower, the boundary, and the 5% resume margin), the probing gate via Consumer::probed(), the idle reservation… 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. Full details: Title check
-
Explanation
The title names the main change: native publishers (Rust, FFI, C) disable simulcast renditions whose bandwidth grant cannot fund them. The changeset adds this rung-disable behavior in moq-mux rate control, moq-net probing, the FFI and C video producers, and the C options struct. The title is specific, concise, and matches the primary objective. The breaking marker (!) also matches the documented breaking API changes.
Full details: Description check
-
Explanation
The description matches the changeset. It links the quest, explains the rate rule (Policy::lower, the boundary, and the 5% resume margin), the probing gate via Consumer::probed(), the idle reservation change, the FFI and C producer changes, the moq_video_encode_options struct, binding updates, and breaking changes. It also records the maintainer decisions, checks run, and follow-ups.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 78.26% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 184 functions across 23 files. (8 skipped: 8 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
Finishing Touches
-
✨ Simplify code
-
- 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.
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: a82a81d598
ℹ️ 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".
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: a82a81d
Confirmed the existing P2 lower-ceiling target finding #6781 (comment) at rs/moq-ffi/src/video.rs:887 and rs/moq-c/src/video.rs:290. Lower-only updates reset the follower’s remembered target to max without retuning the encoder. If the resulting grant equals max, no update/deadline is emitted and the encoder stays below its intended rate. Preserve actual target/ramp or explicitly synchronize the reset with the encoder; cover a two-rung lower-ceiling change.
Direction: common-controller disable/resume and non-claiming idle reservations are coherent; policy updates need that state correction.
Verification: static controller, allocator, native/C publisher, bindings and tests inspection. No tests or shaped-uplink/OBS verification.
Follower::set_policy reset the target to the ceiling on any policy change, but a lower rendition moving or finishing never retuned this sender, so a grant at the ceiling read as already applied and the encoder stayed below it. Only a new ceiling or floor starts over now. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 30c4025942
ℹ️ 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".
When the policy moves between a follower's target and its apply, the target is dropped unapplied. Now that a moved lower rendition keeps the follower's state, it would count that target applied, so a disable could leave the encoder running. Roll it back before installing the policy. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Maintainer decision (2026-10-25): options struct. The hand-written moq-c ships at this cut with parity, so the C half belongs in this PR. (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: f8a24df, compared with a82a81d.
The previous lower-ceiling finding #6781 (comment) is fixed: lower-only policy changes preserve the applied target and ramp. The independent unapplied-target race #6781 (comment) is also addressed by rolling back before replacing the policy in both native bindings.
No new actionable bugs found in this delta. These focused state-preservation fixes are the right direction and include regression tests.
Verification: static inspection of both commits, controller/caller interactions and added tests; tests and shaped-uplink/OBS scenarios were not run.
|
Landing summary:
(Written by Claude Opus 5.5) |
# Conflicts: # rs/moq-c/src/video.rs # rs/moq-ffi/src/video.rs
Rate control holds a rung enabled unless a session probes for it, since nothing else could raise the estimate to resume it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: def2ee5, compared with the last published review at 80ab074 and accounting for intervening main merges.
The independent probing-prerequisite documentation finding is addressed consistently across the native binding guides. This correctly makes the existing probing gate explicit rather than promising disable/resume on every transport. Earlier controller, rollback and lower-ceiling notification fixes remain intact.
No new actionable bugs found in this delta. The focused documentation correction is the right direction; the other changes come from main integration, including broadcast-child lifecycle handling and its C test adaptation.
Verification: static diff and affected native lifecycle-path inspection. Tests, OBS and shaped-uplink scenarios were not independently run.
|
Re-landing after the queue dropped this PR for a
Validation:
(Written by Claude Opus 5.5) |
|
Re-landing after the merge queue dropped this PR on
(Written by Claude Opus 5.5) |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Merged origin/main (head 47f86d5) after the queue dropped this on a conflict. The only conflict was Checks on the merged head: (Written by Claude Opus 5.5) |
Resolve moq-c and moq-ffi MIGRATE.md Unreleased entries by keeping both, newest first, and regenerate the Dart bindings with both the MoqVideoProducer and MoqVideoRead optional converters. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Requeued after the
Checks: (Written by Claude Opus 5.5) |
Carry the renames into #6781 (simulcast rungs): MoqAllocator option types, Reservation::rate()/consume(), moq_allocator_* in the C tests, and keep both sides of the Unreleased MIGRATE entries. Rename main's new track::Consumer::query() and JS writeGroup callers, and regenerate the Dart bindings. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Quest: quest/m1/simulcast-rung-disable.md. This PR lands the Rust/FFI/C publishers and rewrites the quest to the remaining OBS work and the shaped-uplink proof.
Summary
moq_mux::rate):Policy::lowersets a band boundary at(max + 2 * lower) / 3. Below it,Control::update/Follower::nextyieldrate::Target::Disabled. The rung resumes once the grant reaches boundary + 5%, restarting at the boundary and ramping 25%/s. A policy withoutlower(the lowest rendition) never disables.moq_net::bandwidth): a rung disables only while a session probes for its reservation, via the newConsumer::probed(). A session counts only when its transport has a sender, i.e. it took the allocator's target, and none counts whileAllocator::set_target(Some(Rate::ZERO))turns probing off.js/net.lower: Option<MoqVideoProducer>and sits one track priority below it.enabled: false.lowerwithoutbandwidth, or one whose ceiling is not below its own.kiochannel that closes when it finishes. A rung watches its lower rendition's channel, so it notices that rendition moving or ending even if the lower one reserves nothing or uses another allocator. The C encoder does the same.moq_encode_videotakes amoq_video_encode_optionsholdingbandwidthandlower, in place of its positionalbandwidth. NULL or zeroed is the old unadapted behavior; new fields append. The options are validated before the encoder opens.lower. Kotlin and Dart alias the regenerated options record. Docs and MIGRATE notes are updated.cpp/moqandcpp/obsuse the generated moq-ffi package, not moq-c, so the C struct does not reach them.Follower::set_policystarts over at the ceiling only for a newmaxormin. A newloweralone (the lower rendition retuned or finished) keeps the applied target and ramp, since nothing retuned this sender. A target dropped because the policy moved before it was applied is taken back (Follower::failed) and asked for again (both from Codex review).rate::tests::modelreplays seeded proptest walks of grants, jitter, probing flips,lower/max/minchanges, refused changes, and time throughControlthe wayFollowerdrives it. After every wake it checks: no deadline is already due, the target stays within[min, max], a rung disables only while probed, under its boundary, and outside the deadband, a disabled rung resumes once probed with the grant at boundary + 5%, and a policy withoutlowernever disables. It catches both state holes above when either fix is reverted, and runs in under a second.Follower::set_policymoved ontoControlso the model drives the same code.moq-video's capture path is single-rendition, so it sets nolowerand never disables.update_max_bwnever ages an app-limitedmax_bw, measured in moq-dev/moq#6635); a separate quest fixes that.Impact
enabledalready exists in the catalog).moq-mux:rate::Follower::next/poll_nextreturnrate::Target. MatchTarget::Enabled(rate)where you took the rate.Control::updatereturnsOption<Target>.Policygainspub lower. Addlower: Noneto struct literals.moq-net/js/net/MoqReservation::grant/moq_reservation_grant: an unwatched reservation reports its would-be share instead ofNone.Nonenow means only "no estimate". A sender that relied onNoneto hold while unwatched should check its track's demand.moq-net:bandwidth::Consumer::probed().MoqVideoProducerOptions.lower(UniFFI defaultNone).moq_encode_video(broadcast, input, output, const moq_video_encode_options *options). PassNULLwhere you passed a0bandwidth, or&(moq_video_encode_options){.bandwidth = bandwidth}.Decisions
Maintainer picks (2026-10-25):
C signature for the lower rendition
uint32_t loweronmoq_encode_videomoq_video_encode_optionsstruct holdingbandwidthandlowerScope
Prompt: "Land #6781. (1) do not add a 5th positional argument to
moq_encode_video; add amoq_video_encode_optionsstruct holdingbandwidthandlower, following moq-c's struct conventions, and update every consumer. (2) Land the native part now, and rewrite the quest to what remains (the OBS per-encoder rung and the shaped-uplink resume proof). Also record in the quest's Plan that the BBR3 estimate it reads can stay high for 20 s or more after the path drops; a separate quest will fix that, so just note the dependency." ✅Design checkpoint after four review rounds
Control/Followerto catch this class of state bug in one passRungtypeChoices made
lowerreferences the lower producer, rather than taking a bitrate plus priority. The boundary therefore tracks the lower rung's ceiling, and the priority ordering can't be misconfigured.Control/Follower, which returnTarget, rather than living in a separateRungtype. Every follower gets the same hysteresis state.MoqReservation::probedwas dropped from the FFI because nothing consumes it yet. OBS adds it with its rung.moq_video_encode_optionsfollowsmoq_announce_config: the pointer may be NULL, a zeroed struct means the defaults, and new fields append.Follow-ups
moq_encode_audiostill takesbandwidthpositionally (its 6th argument). Amoq_audio_encode_optionswould match the video shape; left out of scope.Checks
just checkafter the options struct and the main merge: pass, except one Dart RSS leak test (a non-null optional argument is released, inresolve, untouched here) that flaked under the parallel load and passes alone. Swift skipped on Linux.just checkpasses, withjust rs testrun separately. Itscheck flakestep cannot build the github-runner image here (unshareis not permitted in the local sandbox), and CI covers it. Swift was not re-run; nothing it binds changed.just checkpasses except the samecheck flakestep.just rs test,cargo test -p moq-c --lib, andcargo test -p moq-ffi --lib --features openh264,testingpass. One exception isserver_cancel_releases_the_bound_port(port reuse, untouched here), which hitAddrInUseunder the shared host's load and passes alone.just test interop --all: passed on the previous head. Not re-run: the struct change is moq-c only, and no interop client callsmoq_encode_video.🤖 Generated with Claude Code
(Written by Claude Opus 5.5)