Write the video start bitrate hint as one connection-level value, once - #2102
Merged
xianshijing-lk merged 3 commits intoSep 22, 2026
Merged
Conversation
x-google-start-bitrate is connection-scoped in libwebrtc: ApplyChangedParams reads it per m-section but pushes it into the shared Call via SetSdpBitrateParameters, where RtpBitrateConfigurator holds one config for the whole peer connection. Differing per-section values were last-writer-wins on m-section order, so a camera plus a screen share could seed the estimator from either one depending on SDP layout. Every video section now carries the same value: the largest hint among the sections that map to a published track. Write it only on the first offer that carries local video. libwebrtc retains start_bitrate_bps and re-applies it on network route changes, so rewriting it later is at best a no-op and at worst a restart of a converged bandwidth estimator. The latch is set only once the offer carrying the hint is accepted locally, so a rejected munge retries on the next offer. Add a 300 kbps target floor, matching the Rust SDK: below that, seeding above the real capacity costs more than the ramp it saves. applyVideoStartBitrate no longer matches the section to a track; that moves to findTrackCodecPayload so the dependent DD extension munging still runs on every offer. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 04f3556 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
size-limit report 📦
|
xianshijing-lk
marked this pull request as ready for review
September 14, 2026 17:31
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
`trackBitrates` is append-only and an unpublished section keeps its `a=msid`, so matching a section to a track by msid alone still paired a stale entry with the section it used to occupy. Two consequences, both reachable while `hasAppliedVideoStartBitrate` is still unset (a superseded offer, or a munge the browser rejected): an uncapped screen-share target could seed a connection that now carries only a camera, and the unpublish renegotiation itself could consume the one-shot hint on a section that sends nothing, leaving every later publish with no hint at all. `a=sendonly` separates a live send from an unpublished or pre-populated section: every publish creates its transceiver with `direction: 'sendonly'`, `unpublishTrack` sets it to `inactive` (and `removeTrack` does the same transition for the simulcast senders), and the pre-populated placeholders are `recvonly`. The matching in the munge loop is left as it was: it now writes the correct connection-level value, and suppressing it on reverted sections could leave them without an fmtp line the live section has, which is the bundled payload type collision `conformBundledCodecFmtp` works around. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Matching `a=sendonly` exactly dropped the legacy `addTrack` fallback: `createSender` uses it when `addTransceiver` is unavailable, and `addTrack` reuses a transceiver rather than creating a sendonly one, so a published camera lands on a `sendrecv` section. A section with no direction attribute has the same problem, since SDP defaults it to sendrecv. Those clients got no start bitrate hint at all. Invert the test: skip `recvonly` and `inactive`, the only two directions that cannot carry local media, and count everything else. That still excludes both shapes a dead section takes -- `inactive` from a sendonly transceiver, `recvonly` from the sendrecv one `addTrack` reuses -- and the pre-populated placeholders, which are `recvonly`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
xianshijing-lk
deleted the
sxian/CLT-3068/video-start-bitrate-connection-level
branch
September 22, 2026 00:17
This was referenced Sep 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #1987, bringing the JS start-bitrate munging in line with what came out of the review on client-sdk-android#973. No change to the formula: still 90% of the target, capped at 1 Mbps for camera, uncapped for screen share.
x-google-start-bitrateis connection-scoped, not per-sectionlibwebrtc reads it per m-section but doesn't apply it per m-section:
That lands in
RtpBitrateConfigurator, which holds oneBitrateConstraintsfor the entireRTCPeerConnection. Every video m-section writes the same slot, last writer wins, and the order is just m-section order.Today each track writes its own value. The camera/screen-share split is what makes that bite: camera capped at 1000, screen share uncapped at e.g. 2700, both in the same offer. Whichever section applies last seeds the single estimator — so either the screen share overrides the camera cap for the whole connection, or the camera clobbers the screen share and the exemption does nothing. It depends on SDP layout, so it looks stable in testing and flips in the field.
Fix:
computeConnectionStartBitratetakes the largest hint among the video m-sections that map to a published track, and every video section gets that same number. Only sections present in the current SDP count, sincetrackBitratesis append-only and outlives an unpublish.Written once per connection, not on every offer
libwebrtc guards re-application three ways: it only re-reads the fmtp when the send codec changes (
webrtc_video_engine.cc:1338-1342), ignores a value equal to the stored one (rtp_bitrate_configurator.cc:66-73— "setting the same remote description twice shouldn't restart bandwidth estimation"), and returns-1for "no change" while retaining the real value.The gap: when the value changes between offers and the codec changed too, it really does restart a converged estimator. With per-track values, publishing a screen share mid-session changes the last-writer value.
The reason once is sufficient, not just safe: the seed persists.
RtpBitrateConfiguratorkeepsstart_bitrate_bps, andRtpTransportControllerSend::OnNetworkRouteChangedre-applies it fromGetConfig()(rtp_transport_controller_send.cc:390). A WiFi→cellular handover re-seeds the estimator from this hint automatically, with no renegotiation. A full reconnect builds a new peer connection and seeds the new estimator again.hasAppliedVideoStartBitratelatches only aftersetMungedSDPaccepts the offer, so a rejected munge retries on the next one.300 kbps target floor
Matches the Rust SDK. Below that, seeding above the real capacity costs more than the ramp it saves.
Refactor note
applyVideoStartBitratewas doing double duty as section-matcher, and the DD-extension munging depended on that match. Matching moved tofindTrackCodecPayload, so DD extension still runs on every offer while the bitrate write happens once.Tests
PCTransport.test.ts: 28 passing (6 new — the floor, the cap and its screen-share exemption, the connection-level max, stale-entry filtering, and section matching).All libwebrtc references are against
m144_release.Note
13 test files plus
lintandtscfail on this branch from a missingmachinapackage — reproduced on a clean checkout ofmain, unrelated to this change. All 4tscerrors are inSignalClient*.ts.