feat(net)!: divide one connection's bandwidth estimate among its tracks - #2854
feat(net)!: divide one connection's bandwidth estimate among its tracks#2854kixelated wants to merge 5 commits into
Conversation
![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: ae42c849f3
ℹ️ 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".
| let settling = next > self.target && next == desired; | ||
| let hysteresis = self.policy.hysteresis.max(0.0); | ||
| if (next.abs_diff(self.target) as f64) < self.target as f64 * hysteresis { | ||
| if !settling && (next.abs_diff(self.target) as f64) < self.target as f64 * hysteresis { |
There was a problem hiding this comment.
When an estimate rises only slightly, next immediately equals desired, so settling is true and bypasses hysteresis. For example, after targeting 2 Mbps, a rise to 2.01 Mbps is applied despite the documented 5% deadband, allowing ordinary upward estimate jitter to repeatedly reconfigure the encoder. Restrict this exemption to recovery at the policy ceiling and cover upward jitter with a regression test. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L121-L121
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f117cf447a
ℹ️ 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".
| /// track": leaving it at zero is what puts audio and video in one undifferentiated | ||
| /// tier, both on the wire and in | ||
| /// [`bandwidth::Allocator`](moq_net::bandwidth::Allocator). | ||
| pub fn track_info(priority: u8) -> moq_net::track::Info { |
There was a problem hiding this comment.
When a broadcast is published from JavaScript, js/publish/src/broadcast.ts:238-262 still calls request.accept() for catalogs and Container.trackInfo(...) for media, while js/hang/src/container/track.ts:29-30 supplies no priority. Consequently every browser-published track continues to advertise priority 0, so under congestion relays cannot keep catalog and audio ahead of video as this change now does for Rust publishers. Mirror the priority API and call-site changes in the JS implementation.
AGENTS.md reference: AGENTS.md:L170-L176
Useful? React with 👍 / 👎.
| "aac" | "opus" => hang::catalog::PRIORITY.audio, | ||
| _ => hang::catalog::PRIORITY.video, |
There was a problem hiding this comment.
When Track::new imports its supported flac or mp3 formats, matched at lines 236-243, this helper falls through to the video priority. Their TRACK_INFO therefore advertises priority 60 instead of 80, so these audio tracks lose the publisher-priority ordering that this change preserves for AAC and Opus. Include every supported audio format in this classification.
Useful? React with 👍 / 👎.
…ority sweep Three defects from the adversarial review of #2854. `rate::Policy`'s ceiling exemption was keyed on `next == desired`, but every raise eventually lands on `desired`, so it fired on ordinary upward jitter instead of only on the recovery it was written for. A 0.5% rise applied immediately, and a slowly-rising estimate retuned the encoder on every 100ms tick, which is exactly what the deadband exists to prevent. Keyed on `Policy::max` instead: stalling a few percent below a merely estimate-limited ceiling is the deadband working, while stalling below the rate the caller configured is the bug that exemption exists to fix. `format_priority` classified only `aac` and `opus` as audio, so the `flac` and `mp3` tracks `Track::new` also accepts advertised the video priority and lost the ordering the priorities exist for. The JavaScript publisher stamped nothing at all: `Container.trackInfo` carried only timescale and retention, and the catalog track was accepted bare. Every browser-published track therefore still advertised 0, so a relay could not order a browser broadcast the way it now orders a Rust one, even though `track.ts` describes itself as the twin of `hang::container::track_info`. `trackInfo` now requires a priority, mirroring the Rust signature and for the same reason: there is no correct value for "some media track". BREAKING CHANGE: `@moq/hang`'s `container.trackInfo` requires an options object carrying `priority`. Refs #2815 Co-Authored-By: Claude Opus 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: 3c76111491
ℹ️ 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".
| if share.is_none() | ||
| && let Some(allocator) = &encode.bandwidth | ||
| { | ||
| share = Some(allocator.register(demand, ceiling)); |
There was a problem hiding this comment.
When an on-demand capture reopens with a different negotiated resolution or frame rate, ceiling is recomputed but share.is_none() preserves the reservation created for the first encoder. This is reachable for screen or window sources resized while no viewer is subscribed. A larger mode remains capped at the old reservation, while a smaller mode continues claiming the old amount and deprives sibling tracks. Update or replace the existing reservation whenever the reopened encoder's ceiling changes. (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
Adversarial review (Codex), triagedRan an adversarial pass with Codex as a second opinion and verified every finding against the code. Verdict was HOLD on five findings: four fixed in FixedThe hysteresis exemption defeated the deadband it was carved out of. The The distinction that matters: stalling a few percent below a merely estimate-limited ceiling is the deadband doing its job, while stalling below the rate the caller configured is the defect the exemption exists to fix. Regression test walks a sub-deadband rise and pins both sides of the 5% boundary.
The browser publisher stamped nothing. Plus a bug in one of this PR's own new tests, which asserted the 5% boundary was inside the deadband when it is exactly at it. Rejected"The allocator leaves no capacity for transport overhead" (flagged high). The physics is right: encoder ceilings describe encoded payload while MoQ framing and QUIC packet overhead consume additional capacity, so senders following their grants exactly can still exceed the estimate. This is a deliberate decision rather than an oversight. Overshoot costs tail-dropped groups, the allocator is advisory by design, and with #2847 open the estimate on the quinn backend is "Video priming sends at full bitrate while claiming no allocation" (flagged medium). The mechanism is real: before The suggested fix would make it worse: representing priming as active demand would have a non-transmitting encoder claim budget away from the siblings that are transmitting. Verification
(written by Opus 5) |
`bandwidth::Consumer` carries a per-connection send estimate, but rate control was applied per encoder: every sender targeted the whole estimate, so N senders on one connection targeted N times the uplink. A capture publish already does this today, since audio and video share a connection and only video's encoder was counted at all. `bandwidth::Allocator` divides one estimate among the tracks registered against it. Registration is keyed on a `track::Demand`, which supplies all three things a share needs: its priority (`track::Info::priority`, so allocation and transmission order can't disagree), whether it's demanded right now (an unwatched track claims nothing, and `publish_capture` stops encoding entirely while unwatched), and its lifetime (a Demand is weak, so registering never keeps a track alive). Reservations are ceilings, never observed rates. A VBR encoder on a black screen can jump from 1 to 6 Mbps between frames, and a reservation that had followed it down would already have handed that room to someone else. The split is strict priority, max-min fair within a tier, with surplus above the total reserved left unclaimed. Publishers leave `priority` at its default today, so the max-min half is what carries the common case: one tier of audio and video still serves audio's small reservation in full before video takes the remainder. Advisory, not enforced: a track that ignores its share, or can't follow one at all (PCM audio's bitrate is fixed), still sends what it sends, and the transport sheds the excess. Bandwidth estimation isn't exact enough for the difference to be worth policing. Two follow-on changes fall out: `rate::Policy::headroom` is removed. Its documented job was reserving room "for the other tracks sharing this connection (audio)", which is now the allocator's, and holding back a further 10% per encoder for one connection's overhead would double-count it. Removing it surfaced a latent bug in the same policy. `next` stops growing once the ramp reaches the ceiling, so a target that lands within `hysteresis` of it has no move left that can clear the threshold: the walk stalls at 3_866_256 of a 4 Mbps ceiling and stays there. It was invisible before because the ramp chased 0.9 * max and the residual gap hid inside the headroom. A raise that lands exactly on `desired` is now exempt from hysteresis, since that's the last step of the ramp rather than a twitch. Drops keep the deadband. BREAKING CHANGE: `moq_video::encode::Options::bandwidth` and `moq_audio::encode::Options::bandwidth` take a `bandwidth::Allocator` rather than a `bandwidth::Consumer`, and `rate::Policy::headroom` is gone. Refs #2815, #2847, #2848 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`hang::catalog::PRIORITY` (catalog 100, text 90, audio 80, video 60) was only ever read on the subscribe side. `hang::container::track_info()` left `track::Info::priority` at its default, so every published media track went out at 0 and TRACK_INFO advertised one undifferentiated tier. That makes the publisher priority the draft describes unusable in practice. draft-lcurley-moq-lite says a publisher "advertises a fixed publisher priority (here audio at 2 and video at 1) used only to break ties", and a relay SHOULD prefer it upstream precisely because it cannot pick between its subscribers' priorities. With every track at 0 there is nothing to prefer, so an audio group and a video group arrive at a congested upstream leg indistinguishable. `track_info` now takes the priority rather than defaulting it. There is no correct value for "some media track", and a default is what let this sit unnoticed: making it a parameter turns every new publisher into a decision the compiler asks for. Each call site passes the kind it already knows. Undecoded MPEG-TS elementary streams (SCTE-35, teletext, DVB subtitles) follow `text`: they are tiny and timing-critical, so they should not queue behind a media backlog and are too small to starve one by sitting above it. This also un-flattens `bandwidth::Allocator`'s tiers, which read the same field. The split was already correct at a single tier (max-min fair serves audio's small reservation before video's large one either way), so this changes what happens once a tier's smaller claims outgrow an even split rather than fixing a live misallocation. No wire format change: TRACK_INFO has carried this field since Lite05 and the draft already specifies these semantics, so no draft update is due. BREAKING CHANGE: `hang::container::track_info`, `moq_mux::catalog::Producer::track_info`, and `moq_mux::catalog::Reserved::track_info` take a `priority: u8`. Refs #2815 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both were minted with no `Info` at all, so they went out at priority 0 while the hang catalog tracks now carry `PRIORITY.catalog`. One broadcast therefore advertised its catalog at 100 and the same catalog, in MSF, at 0, and a subscriber reading MSF was ranked below every media track on a relay's upstream leg. Both are the index a player reads before any media is useful to it, and both are far too small to starve the media they index by sitting above it. Also corrects the claim on `Allocator::register` that allocation and transmission order can't disagree. They can: allocation ranks by the publisher's priority, while the local send queue ranks by each subscription's own (`lite::publisher` seeds it from the SUBSCRIBE message, and `info.priority` only ever reaches the TRACK_INFO message). The publisher's is still the right one to divide by, since allocation decides what to *produce* and there is no single subscriber priority to read when several are watching, but the reason is not the one the doc gave. Drops the note that allocators nest. The property is real and falls out of `Allocator::new` taking a `Consumer`, but nothing in tree uses it and nothing tests it, so it shouldn't be advertised as a promise. Refs #2815, #2858 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ority sweep Three defects from the adversarial review of #2854. `rate::Policy`'s ceiling exemption was keyed on `next == desired`, but every raise eventually lands on `desired`, so it fired on ordinary upward jitter instead of only on the recovery it was written for. A 0.5% rise applied immediately, and a slowly-rising estimate retuned the encoder on every 100ms tick, which is exactly what the deadband exists to prevent. Keyed on `Policy::max` instead: stalling a few percent below a merely estimate-limited ceiling is the deadband working, while stalling below the rate the caller configured is the bug that exemption exists to fix. `format_priority` classified only `aac` and `opus` as audio, so the `flac` and `mp3` tracks `Track::new` also accepts advertised the video priority and lost the ordering the priorities exist for. The JavaScript publisher stamped nothing at all: `Container.trackInfo` carried only timescale and retention, and the catalog track was accepted bare. Every browser-published track therefore still advertised 0, so a relay could not order a browser broadcast the way it now orders a Rust one, even though `track.ts` describes itself as the twin of `hang::container::track_info`. `trackInfo` now requires a priority, mirroring the Rust signature and for the same reason: there is no correct value for "some media track". BREAKING CHANGE: `@moq/hang`'s `container.trackInfo` requires an options object carrying `priority`. Refs #2815 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
#2809 landed multi-stage support on dev, so a single process can now run several import stages over one connection. `spawn_import` was handed a bandwidth `Consumer` per stage and each capture stage built its own `Allocator` from it, which divides that stage's own video and audio correctly and leaves every stage oversubscribing the others. That is the exact shape of the bug the allocator exists to fix, one level up. The allocator is now minted once, where the connection is, and stages receive the allocator rather than the estimate. Making `spawn_moq` return it is what keeps this from regressing: there is no longer an estimate in scope for a stage to build its own from. Also stamps the two `.avc1` / `.aac` export-test tracks the rebase brought in, and resolves the conflict with #2768: the catalog rendition is now probed up front, so `capture_loop` no longer has a priming phase that ran the encoder before any subscriber existed. Refs #2815 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
3c76111 to
f72c50a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f72c50ab16
ℹ️ 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".
| import: Import, | ||
| name: String, | ||
| bandwidth: Option<moq_net::bandwidth::Consumer>, | ||
| bandwidth: Option<moq_net::bandwidth::Allocator>, |
There was a problem hiding this comment.
When one invocation combines a capture import with an HLS, RTMP, SRT, RTC, or stdin import, this shared allocator reaches every spawn_import, but only the ImportSource::Capture arm at lines 479-485 uses it. The fixed-rate imports still consume the same outbound connection without registering reservations, so the capture encoder can target the entire estimate on top of their traffic and recreate the cross-stage oversubscription this change is intended to prevent. Plumb the allocator into those import pipelines and reserve each known or advertised track ceiling. (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
| moq_net::track::Info::default() | ||
| .with_timescale(TIMESCALE) | ||
| .with_latency_max(LATENCY_MAX) | ||
| .with_priority(priority) |
There was a problem hiding this comment.
When a session negotiates IETF moq-transport, this newly assigned track priority never reaches the wire: rs/moq-net/src/ietf/publisher.rs:1424-1429 and js/net/src/ietf/publisher.ts:240-250 still hardcode every group header's publisher priority to 0. Consequently catalog, audio, and video remain in one priority tier for IETF peers, while only moq-lite observes this priority sweep. Pass the track's priority through both IETF publishers, converting from the model's higher-first convention to the IETF wire convention. (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
1 participant
Closes #2815.
Problem
moq_net::bandwidth::Consumercarries a per-connection send estimate, but rate control is applied per encoder. Nothing divided it, so N senders on one connection each targeted the whole uplink.This is reachable today without #2809: a capture publish runs an audio encoder and a video encoder over one connection, and only video's was counted at all.
rate::Policy::headroom(0.9) was the stand-in, and its doc said so outright: it reserved room "for the other tracks sharing this connection (audio)", i.e. one video encoder per connection, with audio's real bitrate guessed at a flat 10%.bandwidth::AllocatorDivides one estimate among the tracks registered against it. Registration is keyed on a
track::Demand, which supplies all three things a share needs:track::Info::priority;publish_capturestops encoding entirely while nothing is subscribed);Demandis weak, so registering never keeps a track alive.registerreturns an ordinarybandwidth::Consumer, sorate::Policyand every other downstream consumer are untouched.Reservations are ceilings, never observed rates. A VBR encoder sitting on a black screen at 1 Mbps can jump to 6 Mbps between one frame and the next; a reservation that had followed it down would already have handed that room to someone else.
The split is strict priority, max-min fair within a tier, with surplus above the total reserved left unclaimed (capping at the reservation is what keeps an uncongested link encoding at exactly the configured rate).
It is advisory. A track that ignores its share, or can't follow one at all (PCM audio's bitrate is fixed by sample rate and channel count), still sends what it sends and the transport sheds the excess. Bandwidth estimation isn't exact enough for the difference to be worth policing.
Publishers stamp
PRIORITYhang::catalog::PRIORITY(catalog 100, text 90, audio 80, video 60) was only ever read on the subscribe side.hang::container::track_info()leftInfo::priorityat 0, so every published track advertised one undifferentiated tier and the allocator's tiers were vacuous.That also makes the publisher priority the draft describes unusable: draft-lcurley-moq-lite says a publisher "advertises a fixed publisher priority (here audio at 2 and video at 1) used only to break ties", and a relay SHOULD prefer it upstream precisely because it cannot pick between its subscribers' priorities. With everything at 0 there is nothing to prefer.
track_infonow takes the priority instead of defaulting it (and so does its TypeScript twin,@moq/hang'scontainer.trackInfo). There is no correct value for "some media track", and the default is what let this sit unnoticed; a parameter turns every new publisher into a decision the compiler asks for.Two non-media tracks were caught by the same sweep: the MSF catalog and the timeline were both minted with no
Infoat all, so a broadcast advertised its hang catalog at 100 and the identical catalog in MSF at 0. Both now rank with the catalog.What this does not do: it doesn't align allocation with transmission order. Allocation ranks by the publisher's priority; the local send queue ranks by each subscription's own (
lite::publisherseeds it from the SUBSCRIBE message, andinfo.priorityonly ever reaches the TRACK_INFO message). A subscriber asking for video ahead of audio still gets it sent that way. The publisher's priority is the right one to divide by, because allocation decides what to produce and there is no single subscriber priority to read when several are watching one track, but the two are genuinely separate rankings.The allocator was already correct at a single tier (max-min fair serves audio's small reservation before video's large one either way), so this changes what happens once a tier's smaller claims outgrow an even split rather than fixing a live misallocation.
Two bugs found on the way
rate::Policystranded the target below its ceiling, permanently.nextstops growing once the ramp reachesdesired, so a target landing withinhysteresis(5%) of the ceiling has no move left that can ever clear the threshold. A 4 Mbps encoder recovering from one congestion event stalls at 3,866,256 and stays there. It was invisible before because the ramp chased0.9 * maxand the residual gap hid inside the headroom. A raise that lands exactly ondesiredis now exempt: that's the last step of the ramp, not a twitch. Drops keep the deadband, which is what it's for.Two lost wakeups in the new poll paths. A share whose slice didn't move (its reservation caps it, so most estimate changes don't reach it) parked without the estimate's waker armed, because a poll returning
Readyregisters nothing;Demand::poll_statehad the same hole when demand flipped between its two internal reads. Both now loop until something is armed. These are hangs rather than failures, so they're covered by tests that assert on a realWakerbeing woken rather than on a re-read, and I verified each fails with the fix reverted.Also
rate::Policy::headroomis removed. Its job was reserving room for audio, which is now the allocator's, and holding back a further 10% per encoder for one connection's overhead would double-count it. Going over budget is fine: the transport tail-drops groups.Caveat
The estimate being divided is only as good as its input, and on the quinn backend that input is
cwnd * 8 / rtt, which is a window over a latency rather than a rate. Filed as #2847 (quinn already computes the BBR pacing rate and exposes it onController::metrics(); it just never reachesPathStats). This PR makes the arithmetic honest; it does not make the number good.Audio reserves but does not follow its share yet: #2848. Three more gaps this surfaced, all filed rather than scoped in: transcode ladders (#2858, the sharpest remaining instance, and one with a catalog-honesty question of its own), passthrough imports (#2859), and bindings, which have no encoder rate control at all (#2857).
Testing
just checkandjust testclean (2737 pass). Also ran, since PR CI doesn't:just rs doctest,just rs macos(the only thing that ever compiles moq-audio'scapturefeature), andcargo check -p moq-ffi -p libmoq(just checknever compiles either).New coverage: the non-media tracks ranking with the catalog (mutation-checked against reverting the fix), the split itself (strict priority, max-min fairness, starved tiers, unclaimed surplus, and the one-tier case that is what actually runs today), two concurrent encoders splitting an estimate, idle and closed tracks, estimate lifecycle, both wakeup regressions, the hysteresis stall, and
track_infocarrying the priority.No wire format change, so no draft update: TRACK_INFO has carried
prioritysince Lite05 and the draft already specifies these semantics.js/hangis mirrored for the priority half:container.trackInfonow requires apriority, the browser publisher stamps each rendition's kind, and its catalog track takes the catalog priority. Without it a browser broadcast would keep advertising 0 while a Rust one advertised 80, which is the inconsistency this PR exists to remove. The allocator half has nothing to mirror:js/publishhas no bandwidth-driven encoder rate control to divide.(Written by Opus 5)