Write the video start bitrate hint as one connection-level value, once - #2102
xianshijing-lk wants to merge 1 commit into
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: 3d5615d 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 |
size-limit report 📦
|
| const codecPayload = findTrackCodecPayload(m, trackbr.cid, trackbr.codec); | ||
| if (codecPayload === undefined) { | ||
| continue; | ||
| } | ||
| const startBitrate = codecPayload > 0 ? computeTrackStartBitrate(trackbr) : undefined; | ||
| if ( | ||
| startBitrate !== undefined && | ||
| (connectionStartBitrate === undefined || startBitrate > connectionStartBitrate) | ||
| ) { | ||
| connectionStartBitrate = startBitrate; | ||
| } | ||
| break; |
There was a problem hiding this comment.
🟡 Reverted tracks inflate start bitrate
When an unpublished section retains its msid, computeConnectionStartBitrate counts its stale trackBitrates entry as published. A stale screen-share target can seed every active video section above its intended connection value.
Learn more
Unpublishing a track can leave its old a=msid and codec mapping in the SDP. The code already identifies these reverted sections through the transceiver's missing sender track in getPlaceholderMids. This computation instead treats any matching msid as proof that the track remains published. Since trackBitrates is append-only, a reverted section can therefore contribute an obsolete maximum before the one-shot latch is set.
Example: A 3,000 kbps screen share registers a 2,700 kbps hint, then is unpublished before the debounced first offer is created. Chrome retains its msid on the inactive section. A 1,000 kbps camera in that offer receives the stale 2,700 kbps connection hint instead of 900 kbps.
Recommended fix: Exclude mids returned by getPlaceholderMids() when computing the connection value, or remove/update trackBitrates entries during unpublish. Add a test whose stale SDP section retains its original msid but corresponds to a transceiver with no sender track.
Was this helpful? React with 👍 or 👎 to provide feedback.
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.