Repository navigation
Conversation
changt
requested review from
hiroshihorie,
pblazej and
xianshijing-lk
as code owners
October 16, 2026 21:49
|
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
The camera start bitrate hint was capped at a fixed 1 Mbps. On slow or lossy links that seed overshoots the real capacity. Connection setup time (signaling join plus ICE/DTLS) is the only network signal available before the first video offer, and it grows with round-trip time and loss. The cap now ramps linearly from 1 Mbps at a setup of 1.5 s or less down to 300 kbps (libwebrtc's own default) at 3.5 s or more. Screen share stays uncapped. The anchors and ramp are the ones in client-sdk-js (#2146), rust-sdks (#1467) and client-sdk-android (#1031). - Room.fullConnectSequence starts timing before any peer connection or the signal socket exists, so each attempt (initial connect, full reconnect, region failover) is timed from scratch. When the JOIN builds new transports, it hands the publisher the start time, then records the setup time once the primary transport connects. This matches the Kotlin and JS SDKs; Rust waits for both peer connections in dual-PC mode. - Transport.connectionSetupTime returns the recorded setup time or, while still connecting, the time elapsed so far. That is a lower bound, so it can only err toward the 1 Mbps ceiling. Same as Kotlin and JS. - Transport.startBitrateKbps takes an optional connectionSetupTime and applies startBitrateCapKbps(connectionSetupTime:) to non-screen-share hints. Both LocalParticipant video publish paths pass the publisher's connectionSetupTime. addTransceiver and the offer munge are unchanged. Tests: the ramp, with rust-sdks' cases at setup times measured on shaped links; the formula with a setup time, for camera and screen share; and Transport.connectionSetupTime as recorded, elapsed, and unset.
changt
force-pushed
the
sen/start-bitrate-by-setup-time
branch
from
October 16, 2026 21:52
de278b6 to
9754d2e
Compare
Transport is @RTC-isolated, so reading publisher.connectionSetupTime from LocalParticipant's publish code hopped onto the RTC executor separately. Both publish paths already enter it a few lines later to add the transceiver, so the hint is now computed there, with a synchronous read. The negotiate closure is @sendable and cannot capture the non-Sendable encodings array, so that path reads the encodings from transInit.sendEncodings, which is set from them.
pblazej
reviewed
Oct 19, 2026
| /// Screen share is exempt, however slow the setup. | ||
| /// | ||
| /// Same ramp as client-sdk-js (`computeStartBitrateCap`), rust-sdks | ||
| /// (`compute_start_bitrate_kbps`) and client-sdk-android (`computeTrackStartBitrate`). |
Contributor
There was a problem hiding this comment.
nit: merged rust-sdks and client-sdk-android do cap screen share once the ramp drops below 1 Mbps; only client-sdk-js exempts it. Worth saying "screen share stays uncapped as in client-sdk-js" here and in the changeset so the parity claim doesn't overstate.
Author
There was a problem hiding this comment.
The PR description was inaccurate - all SDKs uncap screen share. I updated the documentations.
| nonisolated static func startBitrateCapKbps(connectionSetupTime: TimeInterval?) -> Int { | ||
| guard let connectionSetupTime else { return maxStartBitrateKbps } | ||
| let ramp = min(max((connectionSetupTime - setupTimeForMaxStartBitrate) / (setupTimeForMinStartBitrate - setupTimeForMaxStartBitrate), 0), 1) | ||
| return Int((Double(maxStartBitrateKbps) - ramp * Double(maxStartBitrateKbps - 300)).rounded()) |
Contributor
There was a problem hiding this comment.
nit: the 300 kbps floor is repeated at L518 (targetKbps >= 300). The other ports name it (minTargetBitrateKbps); a static let minStartBitrateKbps = 300 would tie the two together.
| /// Same formula as client-sdk-js (`computeTrackStartBitrate`), rust-sdks | ||
| /// (`compute_start_bitrate_kbps`) and client-sdk-android (`computeTrackStartBitrate`). | ||
| nonisolated static func startBitrateKbps(targetBps: Int, isScreenShare: Bool) -> Int? { | ||
| nonisolated static func startBitrateKbps(targetBps: Int, isScreenShare: Bool, connectionSetupTime: TimeInterval? = nil) -> Int? { |
Contributor
There was a problem hiding this comment.
nit: connectionSetupTime: TimeInterval? = nil (here and at L528) only exists so the older tests compile, but it means a future video publish path that forgets the argument silently skips the ramp. Dropping the default and passing nil at the test sites would make that a compile error.
|
|
||
| await transport.set(connectStartedAt: ProcessInfo.processInfo.systemUptime - 60) | ||
| let elapsed = try #require(await transport.connectionSetupTime) | ||
| #expect(elapsed >= 60 && elapsed < 70) |
Contributor
There was a problem hiding this comment.
nit: elapsed < 70 is wall-clock bound; 10 s of slack should be plenty, but under TSan/CI stalls a wider window (or bracketing the read with uptime samples) costs nothing.
- Name the 300 kbps floor minStartBitrateKbps, shared by the target threshold and the bottom of the setup-time ramp. - Drop the connectionSetupTime defaults on both startBitrateKbps overloads, so a video publish path that leaves the setup time out fails to compile instead of skipping the ramp. The older tests pass nil. - State the screen-share difference from the other SDKs in the docs and the changeset: it stays uncapped as in client-sdk-js, while merged rust-sdks and client-sdk-android cap it once the ramp drops below 1 Mbps. - Bound the elapsed-time test by uptime samples taken around the read, rather than a fixed window, so a slow CI or TSan run cannot fail it.
rust-sdks (#1505) and client-sdk-android (#1036) have merged "Keep screen share uncapped by the start bitrate ramp", so on main they exempt screen share from the setup-time cap just as client-sdk-js does. The docs and changeset said merged rust-sdks and client-sdk-android still capped it on a slow setup, which no longer holds.
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
3 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 #1121. Ports the setup-time ramp from livekit/client-sdk-js#2146, livekit/rust-sdks#1467 and livekit/client-sdk-android#1031.
Problem
#1121 caps the camera
x-google-start-bitratehint at a fixed 1 Mbps. On slow or lossy links, that seed overshoots what the link can carry.Change
Connection setup time (signaling join plus ICE/DTLS) is the only network signal available before the first video offer, since libwebrtc cannot probe the path until a video sender exists. Setup time grows with round-trip time and loss, so the camera cap now scales with it:
fullConnectSequenceuntil the primary transport connects. Every attempt is timed from scratch: initial connect, full reconnect and region failover. A quick reconnect keeps its peer connection and estimator, so it doesn't measure again.Transport.startBitrateKbps(…connectionSetupTime:), the formula bothLocalParticipantvideo publish paths already call.addTransceiverand the offer munge are unchanged.Differences from the other SDKs
navigator.connection.effectiveType. It is browser-only and has no Apple-platform equivalent.Testing
Transport.connectionSetupTime: recorded, time elapsed so far, and unset.