Write the video start bitrate hint once per connection - #1430
Open
xianshijing-lk wants to merge 2 commits into
Open
xianshijing-lk wants to merge 2 commits into
xianshijing-lk wants to merge 2 commits 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. It retains start_bitrate_bps and re-applies it on network route changes, so a WiFi-to-cellular handover re-seeds the estimator from the original hint with no renegotiation. Rewriting it on later offers was therefore at best a no-op (libwebrtc ignores an unchanged value, and only re-reads it when the send codec changes) and at worst a restart of a converged bandwidth estimator. Write it only on the first offer that carries local video, and only latch it once that offer is accepted locally so a rejected munge retries. A full reconnect builds a new peer connection and seeds the new estimator again. Also exempt screen share from the 1 Mbps cap, matching client-sdk-js and client-sdk-android: unlike camera content, a screen share is published at a high bitrate so text stays legible, and a conservative start costs more than a brief overshoot. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Changeset ✓This PR includes a changeset covering all affected packages:
|
xianshijing-lk
marked this pull request as ready for review
September 14, 2026 17:35
Contributor
There was a problem hiding this comment.
🔍 Devin Review: 1 flag
Not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
livekit-ffi and livekit-capture depend on livekit, and the changeset check requires a package to be bumped whenever a package it depends on is bumped so downstream consumers get a matching release. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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 #1226, bringing the Rust start-bitrate munging in line with what came out of the review on client-sdk-android#973. The camera formula is unchanged: still 90% of the target, capped at 1 Mbps.
Rust was already ahead of the other SDKs here — a single connection-level
max_send_bitrate_bpsrather than per-track values, and a 300 kbps floor. Two gaps remain.1. Written on every offer
Both munge sites (
create_initial_offerandcreate_and_send_offer) rewrite the hint on every offer. libwebrtc guards this 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.
set_max_send_bitrate_bpsoverwrites on each video publish, so publishing a second video track mid-session changes the 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, with anRTC_DCHECK_GT(start_bitrate_bps, 0)right after). 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.start_bitrate_appliedlatches only after the offer carrying the hint is accepted locally — afterset_local_descriptionin the main path, and at the point the initial offer is stashed as pending in the single-PC path — so a rejected munge retries.Also recorded in a comment:
x-google-max-bitrateis deliberately never written, because the same Call-level promotion would turn a per-track cap into a ceiling on total send bandwidth. libwebrtc carries a TODO conceding this is wrong ("codec max bitrate should probably not affect global call max bitrate",webrtc_video_engine.cc:1327). Rust already avoided it; this just writes down why.2. Screen share was capped at 1 Mbps
Behavior change.
compute_start_bitrate_kbpscapped unconditionally. Screen share is now exempt, matching client-sdk-js, client-sdk-android and the pending Swift PR: unlike camera content, a screen share is published at a high bitrate so text stays legible, and a conservative start costs more than a brief overshoot.set_max_send_bitrate_bpstakesis_screen_share, threaded fromoptions.sourceincreate_sender.Tests
cargo test -p livekit --lib: 59 passing (3 new — the 90% formula, the camera cap and its screen-share exemption, and the 300 kbps floor including the no-target case).cargo checkandcargo fmt --checkclean.All libwebrtc references are against
m144_release, which is whatwebrtc-sysvendors.Note
cargo clippyreports pre-existing errors in the separatelibwebrtccrate (video_frame.rs), unrelated to this change. None in the files touched here.