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
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. |
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.
| // - 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.
| val startBitrate = if (trackBr.isScreenShare) { | ||
| calculatedStartBitrate | ||
| } else { | ||
| minOf(calculatedStartBitrate, maxStartBitrateKbps) |
There was a problem hiding this comment.
This cap regresses the configured start for high-bitrate SVC cameras: previously 0.7x with no ceiling, so a 3 Mbps VP9 camera publish started at 2100 kbps; now it starts at 1000. Above roughly 1.4 Mbps targets the new start is lower than the old one, which can lengthen the ramp on links that can carry the target (on constrained links the lower seed is arguably safer). Screen shares are exempt and go up (2100 to 2700), so it only affects camera SVC. JS ships the same values, so nothing may need to change here, but it is worth stating as a known tradeoff in the PR description, or reconsidering the cap for SVC.
There was a problem hiding this comment.
Yes, the 1,000 kbps cap is intentional for now. We found that setting a high startup bitrate can cause congestion under poor network conditions, which may lead to higher first-frame latency and choppy video during the first ~30 seconds.
Capping the startup bitrate at 1,000 kbps improves that behavior. Under good network conditions, 1,000 kbps is still enough to support 720p video, and the quality should ramp up quickly afterward. Under poor network conditions, for example, when the available bandwidth is below 300 kbps, the bandwidth estimator will converge more quickly and avoid overshooting the available capacity.
I'm planning to follow up by making the startup bitrate more adaptive to the network conditions, using signals such as previous bandwidth estimates, signaling latency, connection quality, or other indicators, instead of relying on a fixed 1,000 kbps cap.
| 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.
| // 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.
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 ? |
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