fix: improve initial video quality by setting x-google-start-bitrate for all video codecs - #973
Conversation
🦋 Changeset detectedLatest commit: b1fc066 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 |
206a3e1 to
3dbe09b
Compare
adrian-niculescu
left a comment
adrian-niculescu
left a comment
There was a problem hiding this comment.
The start-bitrate consolidation is a reasonable idea, but a few things need addressing before this lands. Inline notes cover the simulcast bitrate regression, the global degradation-preference default, and the stray patch file.
One more that can't be anchored to a changed line: SdpMungingTest.ensureCodecBitratesTest (livekit-android-test/src/test/java/io/livekit/android/room/SdpMungingTest.kt) still asserts x-google-start-bitrate=700000 for a 1 Mbps target, but the multiplier is now 0.9, so the munge emits 900000. That test fails as-is and needs updating.
Thanks for pointing out, I am discussing with the team on the proper fix here.
|
Glad I can help. I'd go with option 2. Option 1 is what Android and JS already do today, so the default VP8 simulcast publish would still ramp up slowly and the original problem stays unfixed. All that would be left of this PR is the multiplier bump and the degradation default. Option 2 is also the correct one on the merits: Before the notes, a correction to my earlier inline comment. A few notes for the Android version:
On consistency: JS and Rust already disagree (SVC-only at 0.7 vs all codecs at 0.9), so that argument works for either option. Whatever you pick here is probably what the other SDKs should converge on, and Rust shipping the sum approach suggests that's the direction. On the degradation preference default, I replied in the review thread with the experiments. The submodule bump note still applies either way. |
![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.
Thanks for the technical insights.
it is addressed:
The startBitrate calculation (lines 484-488): For camera (720p simulcast):
For screen share (e.g., 3000 kbps target):
The 1 Mbps cap only affects the start hint for camera tracks, not the max bitrate constraint.
Done with addressing this corner case by skipping the hint if target bitrate is below 300, though I don't think it will affect the performance as it will start ramping up from 0.9xtarget maxBitrate to maxBitrate, which should be very quickly.
The min(1Mbps, 0.9 multiplier) is being applied consistently across all SDKs (Rust, JS, Android) as part of this alignment effort. We believe 0.9xtargetBitrate with the 1 Mbps cap provides reasonable bounds, and a reasonable UX. I'm planning to improve this further with smarter bitrate selection based on network conditions (e.g., using previous BWE estimates or connection quality signals, signaling latency, ..etc) as a follow up. The PR is WIP.
These should all be addressed. |
|
Hi @adrian-niculescu , could you please take another look ? thanks. |
There was a problem hiding this comment.
Devin Review found 1 new potential issue.
⚠️ 1 issue in files not directly in the diff
⚠️ Pull request is missing the required changeset file (CONTRIBUTING.md:8)
No changeset file was added under .changeset/ for this change, which the repository's contribution rules require for every PR.
Impact: The release/versioning tooling will not record these changes, so the fix may ship without a changelog entry or version bump.
CONTRIBUTING.md changeset requirement
CONTRIBUTING.md states: "Add a changeset file which explains the changes contained in the PR" via pnpm changeset. The PR only modifies PeerConnectionTransport.kt, LocalParticipant.kt, SdpMungingTest.kt, and the protocol submodule; the .changeset/ directory still contains only README.md and config.json, with no new changeset markdown file.
View 2 additional findings in Devin Review.
|
@xianshijing-lk two housekeeping items for CI: |
| // Screen share is not capped since text/UI clarity requires high bitrate from the start | ||
| // TODO: dynamically adjust start bitrate based on network conditions (e.g., use previous BWE estimate) | ||
| val calculatedStartBitrate = (trackBr.maxBitrate * startBitrateMultiplier).roundToLong() | ||
| val startBitrate = if (trackBr.isScreenShare) { |
There was a problem hiding this comment.
There is a structural problem with choosing the cap per track: libwebrtc consumes these fmtp params per connection, not per m-section. In the m144 line this SDK ships (144.7559.05), WebRtcVideoSendChannel::ApplyChangedParams reads x-google-start-bitrate/x-google-max-bitrate via GetBitrateConfigForCodec and pushes the result into the shared Call config, where RtpBitrateConfigurator::UpdateWithSdpParameters replaces the stored config unconditionally, last writer wins. ApplyChangedParams even carries a TODO noting codec max bitrate probably should not affect the global call max.
Two consequences:
-
The camera/screenshare split cannot work as written. With both published, the capped camera value and the uncapped screenshare value land in the same SDP, and whichever m-section applies its send parameters last seeds the single BWE. Depending on order, the uncapped screenshare start overrides the camera cap for the whole call, or the camera value clobbers the screenshare hint and the exemption silently does nothing.
-
The max-bitrate write is the sharper edge. A default camera publish now emits
x-google-max-bitrate=2310, and that value becomes the Call'smax_data_rate, a ceiling on total send bandwidth across all tracks. If the camera m-section applies last, a concurrent 3 Mbps screenshare is starved under a 2.3 Mbps connection ceiling. Before this PR the conflict required at least one SVC publication (a lone VP9 screenshare's max could already ceiling a concurrent camera); this change brings it to the default VP8 camera plus screenshare case.
Since the knob is per connection, the fix that actually holds is one connection-level value applied consistently to every video m-section, or dropping the global hints entirely; a uniform cap merely bounds the damage, since tracks with different targets still produce different values and the last-writer variance remains. For the max specifically, consider not writing x-google-max-bitrate for simulcast at all: the per-encoding maxBitrateBps already caps each layer, so the fmtp value only acts as the call-wide ceiling described above. The merged JS change (livekit/client-sdk-js#1987) shares the start-bitrate interaction, though not the max one (the JS munger never writes x-google-max-bitrate), so this is probably a cross-SDK decision rather than an Android-only fix.
There was a problem hiding this comment.
Thanks for the technical details, they are extremely helpful. I completely missed the webrtc implementation detail that x-google-start-bitrate and x-google-max-bitrate are effectively connection level knobs rather than per-track settings. Really appreciate you digging into the WebRTC internals and pointing this out.
After thinking about it some more, I wonder if the right direction is something like this:
x-google-max-bitrate:
SUM of all active tracks’ max bitrates
only if LiveKit wants a hard connection-wide ceiling
x-google-start-bitrate:
MAX of active tracks’ start hints
only when initializing a new PeerConnection
Track added later:
recompute max
leave start unchanged
Track removed:
recompute max
leave start unchanged
x-google-max-bitrate is effectively a ceiling for the whole PeerConnection, so an individual track's maximum is not sufficient once multiple video tracks are active. For example, if a camera is capped at 2.3 Mbps and a screenshare at 5 Mbps, setting either value alone would incorrectly cap the total connection. If we keep using this parameter, we should maintain the maximum bitrate requirements of all active video tracks, sum them, and apply the same aggregate value to every video m-section.
Note, we might need to apply this x-google-max-bitrate for both simulcast and non-simulcast, since we might have different config combinations of tracks, we will need to update the max-bitrate with new published track otherwise the old obsolete value might constraint the tracks in a wrong way.
x-google-start-bitrate has a different purpose. It seeds the shared bandwidth estimator when a new PeerConnection is initialized; it is not a per-track allocation. For the initial connection, we can use the maximum start hint among the tracks known at that time. I would avoid summing the start hints because that could initialize BWE too aggressively and cause congestion.
Once the PeerConnection is active and BWE has learned something about the network, publishing an additional track should not reset x-google-start-bitrate. The new track should share the existing estimated bandwidth, and WebRTC can probe upward if more capacity is available. We should still recompute the aggregate max bitrate so that the old connection ceiling does not prevent the new track from ramping up.
Similarly, when a track is removed, we should remove its entry from the active-track map and recompute the aggregate maximum. I would leave the start bitrate unchanged because the estimator already has live network information, and lowering or resetting the start hint would not be useful.
What do you think ?
There was a problem hiding this comment.
Yes, that is the right shape, and it is what landed: one value, the max across the video tracks present in the first video offer, written to every matching video m-section, once per publisher transport, and no max-bitrate write at all. I also checked the once-per-connection latch against reconnect: a full reconnect builds a new transport in RTCEngine.configure and the republish re-registers the tracks, so the new estimator is seeded again. Resolving.
| // - Screen share: MAINTAIN_RESOLUTION (clarity is critical for text/UI) | ||
| // - Other/unknown: BALANCED | ||
| rtpParameters.degradationPreference = finalOptions.degradationPreference | ||
| ?: getDefaultDegradationPreference(trackSource) |
There was a problem hiding this comment.
Revised, the first version of this comment overstated the default case. The backup transceiver reuses the same RTC track and source, and with no explicit preference libwebrtc's GetDegradationPreference resolves is_screencast sources to MAINTAIN_RESOLUTION and camera content to MAINTAIN_FRAMERATE, so for camera and screen share the backup sender lands on the same preference implicitly and the new defaults stay consistent. What does not carry over to the backup sender: an explicitly supplied degradationPreference (a gap that predates this PR), and for sources outside CAMERA/SCREEN_SHARE the new BALANCED fallback applies only to the primary, while the backup retains libwebrtc's source-derived default (MAINTAIN_FRAMERATE for camera content, MAINTAIN_RESOLUTION for screencast content). If publishAdditionalCodecForTrack gets touched anyway, setting the resolved preference on the backup sender would close both.
There was a problem hiding this comment.
Closed by #991, which applies the resolved preference on the backup transceiver as well. Resolving.
| data class TrackBitrateInfo( | ||
| val codec: String, | ||
| val maxBitrate: Long, | ||
| val isScreenShare: Boolean = false, |
There was a problem hiding this comment.
TrackBitrateInfo.maxBitrate has always carried kbps, but nothing in the name or type says so, and neighboring fields like RtpParameters' maxBitrateBps set a bps expectation. The old test demonstrated the ambiguity: it passed 1000000 into the kbps field and asserted a roughly 1 Gbps fmtp value. Renaming to maxBitrateKbps closes the trap. While here: isScreenShare could use a KDoc, and the camera/screenshare/other defaults mapping is now written out in five places (both data class comments, the base KDoc, the publish-site comment, and getDefaultDegradationPreference); one authoritative copy in the public KDoc would keep them from drifting.
One consideration for both the new isScreenShare parameter and the rename: TrackBitrateInfo is a public JVM type in the shipped 2.27.0 AAR (@suppress only hides it from docs), and changing the primary constructor drops the compiled (String, long) constructor and copy descriptors. Exposure is low since PeerConnectionTransport is internal and the only public entry point taking the type is the @VisibleForTesting ensureCodecBitrates, but if its ABI is meant to be stable that should be a deliberate call rather than an accidental side effect.
There was a problem hiding this comment.
Renamed — to targetBitrateKbps rather than maxBitrateKbps, since after dropping the x-google-max-bitrate write the field is only ever a target for the start hint, never a cap.
Confirmed the unit against libwebrtc while I was in there: GetBitrateConfigForCodec does config.start_bitrate_bps = bitrate_kbps * 1000. So the old test asserting x-google-start-bitrate=700000 was claiming 700 Mbps.
isScreenShare now has a KDoc covering why screen shares are exempt from the 1 Mbps cap. The five-places duplication resolved itself — those were all degradation-preference docs, which moved out to #991.
On ABI: agreed it shouldn't be accidental, so I made it deliberate in the other direction — TrackBitrateInfo, TrackBitrateInfoKey and the two-arg ensureCodecBitrates are now internal. They were public only so the separate test module could reach them, and friendPaths (#925) made that unnecessary. Worth noting the visibility change is itself ABI-neutral, javap shows byte-identical signatures, since Kotlin records internal in @metadata only and doesn't mangle top-level declarations. The constructor and copy descriptor change from the rename still stands; it's now just unambiguously a change to something that was never API. Called out in the changeset.
There was a problem hiding this comment.
Correct, and making the types internal is the cleaner call. Resolving.
| // Handle trackBitrates - apply start bitrate for all video codecs to prevent initial blurriness. | ||
| // - SVC codecs: use first encoding's bitrate (single stream with built-in layers) | ||
| // - Simulcast: sum all encoding bitrates (independent streams, BWE needs total) | ||
| if (encodings.isNotEmpty() && finalOptions is VideoTrackPublishOptions) { |
There was a problem hiding this comment.
A lifecycle gap this expansion widens: trackBitrates in PeerConnectionTransport is insert-only, and neither unpublishTrack nor RTCEngine.removeTrack removes the entry, so registrations outlive their publications for the transport's lifetime. Mostly that is just dead map entries scanned on every offer, but it becomes wrong munging on republish: cid is the RTC track id, so republishing the same track hits the same key, and if the new publish computes no encodings (videoEncoding == null with simulcast = false returns an empty list from computeVideoEncodings) this block is skipped and ensureCodecBitrates injects the previous publish's start/max values into the new offer. This existed for SVC registrations before, but the all-video path makes it reachable for every codec. An unregister call when the track is removed would close it.
There was a problem hiding this comment.
You're right that the map is insert-only, and it predates this PR. And I don't think an unregister is needed due to the following understanding.
Both halves of the concern are now gated on hasAppliedVideoStartBitrate. computeConnectionStartBitrate only runs while that latch is false, and ensureCodecBitrates returns before touching the map once it's set, so after the first video offer carries the hint, trackBitrates is never iterated again on that transport , the dead entries aren't scanned on every offer, they aren't scanned at all.
That also bounds the republish case. The latch is set by the first video publish itself, so reaching the stale entry additionally requires that first publish to have failed to set it , setMungedSdp falling back to the unmunged SDP, or a sub-300 kbps target (where the stale entry contributes null anyway). Only then do your conditions apply on top: same track object, and a republish that computes no encodings so no fresh entry overwrites the stale one.
The other half of the original concern is gone outright: the max-bitrate write was removed, so a stale entry can no longer cap the connection. What's left is one start-bitrate hint derived from the same track's previous target, bounded by the 300 kbps floor and the 1 Mbps cap, and self-correcting as the estimator converges.
There was a problem hiding this comment.
Agreed. Checked it against the current head: computeConnectionStartBitrate only runs while the latch is false, and ensureCodecBitrates returns before touching the map once connectionStartBitrate is null, so the stale entries are never read after the first video hint lands. What is left is a start hint bounded by the 300 kbps floor and the 1 Mbps cap. Resolving.
99ee7a7 to
9afea3b
Compare
|
Diffuse output: AARJAR |
…for all video codecs - Apply x-google-start-bitrate SDP hint to all video codecs (VP8, VP9, AV1, H264, H265), not just SVC codecs - Use 90% of target bitrate as start bitrate to prevent initial blurriness - Default degradationPreference to MAINTAIN_RESOLUTION for video tracks to prefer frame drops over resolution reduction when bandwidth is constrained This addresses the issue where video starts blurry for several seconds before improving, by telling WebRTC's bandwidth estimator to start at a higher bitrate instead of ramping up from ~300kbps. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Revert isVideoCodec back to isSVCCodec for bitrate registration - For simulcast, encodings are ordered smallest-to-largest, so encodings.first() returns the lowest layer's bitrate (e.g., 160kbps for H180), which would incorrectly cap all layers at that low value - SVC codecs (VP9, AV1) have a single encoding with the full bitrate, so this logic is safe for them - Remove unused isVideoCodec function - Remove accidentally committed munging.patch file Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…for the first track
TrackBitrateInfo and TrackBitrateInfoKey were public only so the separate test module could reach them. friendPaths (#925) made that unnecessary, so mark them internal along with the two-arg ensureCodecBitrates that exposes them. Both were already @Suppress'd and only reachable through a @VisibleForTesting helper, so this is not intended API. Compiled signatures are unchanged; internal is recorded in @metadata only. Document isScreenShare and the kbps unit on TrackBitrateInfo, and record why the start bitrate is written once to every video m-section: libwebrtc pushes these codec fmtp params into the shared Call, retains start_bitrate_bps in RtpBitrateConfigurator, and re-applies it on network route changes, so rewriting it later either no-ops or restarts a converged estimator. Same for why x-google-max-bitrate is never written. Add the changeset, including the max-bitrate removal as a behavior change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
655c9ed to
b1fc066
Compare
|
Another ping @adrian-niculescu @davidliu, could you please help review this PR ? |
There was a problem hiding this comment.
LGTM.
The latest revision addresses everything from my earlier rounds: a single connection-level start hint written once per publisher transport, no x-google-max-bitrate write, the protocol pointer restored, and the degradation defaults split out to #991.
2 participants
Problem
Published video is blurry for the first 5-15 seconds. WebRTC's bandwidth estimator starts near 300 kbps and ramps slowly, so the encoder spends the opening seconds far below the track's target bitrate.
Solution
Seed the estimator with
x-google-start-bitratefor all video codecs, not just SVC. The hint is 90% of the track's target bitrate (10% headroom for BWE), capped at 1 Mbps for camera and skipped below a 300 kbps target.One connection-level value, written once
These codec fmtp params are connection-scoped, not m-section-scoped. libwebrtc reads them per m-section (
WebRtcVideoSendChannel::ApplyChangedParams→GetBitrateConfigForCodec) but pushes the result into the sharedCallviaSetSdpBitrateParameters, whereRtpBitrateConfiguratorholds one config for the entire peer connection. Two m-sections carrying different values is last-writer-wins, decided by SDP order.So the SDK computes a single value — the max hint across video m-sections in the first offer containing local video — and writes that same value to every video m-section.
It is written once per publisher connection, because the value persists:
RtpBitrateConfiguratorretainsstart_bitrate_bpsin its stored config andRtpTransportControllerSend::OnNetworkRouteChangedre-applies it fromGetConfig()on every relevant route change. A WiFi-to-cellular handover therefore re-seeds the estimator from this hint with no renegotiation. Rewriting it on later offers is at best a no-op (libwebrtc ignores an unchanged value, and only re-reads it when the send codec changes) and at worst restarts a converged estimator. A full reconnect builds a new peer connection with a new estimator, and seeds it again.x-google-max-bitrateis no longer writtenThis is a behavior change. The same Call-level promotion turned a per-track cap into a ceiling on total send bandwidth for the whole connection — a default camera publish emitted
x-google-max-bitrate=2310, which could starve a concurrent 3 Mbps screen share depending on m-section order. libwebrtc carries a TODO conceding this is wrong: "codec max bitrate should probably not affect global call max bitrate."Per-track and per-layer limits are still enforced through
RtpParameters.Encoding.maxBitrateBps, which is genuinely scoped per encoding. client-sdk-js and the Rust SDK never write the SDP value either, so this is cross-SDK parity rather than an Android-only change.Also
TrackBitrateInfo/TrackBitrateInfoKeyare nowinternal. Both were@suppressed and only reachable via a@VisibleForTestinghelper; they were public solely to be visible from the test module, whichfriendPathshas made unnecessary since Tests for withDeadline #925.TrackBitrateInfo.maxBitrateis renamed totargetBitrateKbps— it always carried kbps, and nothing in the old name said so.Degradation-preference defaults are not part of this PR; they shipped separately in #991.
Test plan
SdpMungingTestcovers the connection-level value, the 300 kbps floor, and the cap