fix: improve initial video quality by setting x-google-start-bitrate for all video codecs#973
Conversation
|
…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>
206a3e1 to
3dbe09b
Compare
There was a problem hiding this comment.
This appears to be unrelated to the bitrate/SDP change.
There was a problem hiding this comment.
Thanks, yes, it should removed, it got checked in by mistake
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.
| // Handle trackBitrates - apply start bitrate for all video codecs to prevent initial blurriness | ||
| if (encodings.isNotEmpty()) { | ||
| if (finalOptions is VideoTrackPublishOptions && isSVCCodec(finalOptions.videoCodec) && encodings.firstOrNull()?.maxBitrateBps != null) { | ||
| if (finalOptions is VideoTrackPublishOptions && isVideoCodec(finalOptions.videoCodec) && encodings.firstOrNull()?.maxBitrateBps != null) { |
There was a problem hiding this comment.
Switching this gate from isSVCCodec to isVideoCodec pulls simulcast codecs (VP8 by default, plus H264) into this path, but maxBitrate is still taken from encodings.first(). For simulcast, computeVideoEncodings adds encodings smallest-to-largest (the sortedByDescending { calculateScaleDown } ordering), so encodings.first() is the lowest layer. A default 16:9 publish starts at H180 = 160 kbps. registerTrackBitrateInfo feeds that into ensureCodecBitrates, which writes a codec-level x-google-start-bitrate/x-google-max-bitrate from the low-layer value, capping the full-resolution layer far below its target. SVC kept a single full-bitrate encoding, which is why this was SVC-only before. For simulcast you'd need the top layer's bitrate (or to skip the codec-level cap).
There was a problem hiding this comment.
I ran a series of experiments comparing MAINTAIN_FRAMERATE and MAINTAIN_RESOLUTION under constrained uplink conditions using custom Network Link Conditioner profiles.
Network Profiles:
Poor 3G Uplink
Uplink: 300 Kbps
Packet loss: 3%
Downlink: Unbounded
Poor LTE Uplink
Uplink: 3 Mbps
Packet loss: 1%
Downlink: Unbounded
Results
Poor 3G (540p capture)
Both degradation preferences were tested at a 540p capture resolution.
MAINTAIN_FRAMERATE
The first ~10s of video is blurry, and it improves its resolution after that.
MAINTAIN_RESOLUTION
Preserves image clarity by holding a higher resolution.
Don't see much difference on the smoothness of the videos.
Poor LTE (720p capture)
Both degradation preferences were tested at a 720p capture resolution.
The same observation above.
Poor 3G (1080p capture)
An additional experiment was run using a 1080p capture resolution over the 300 Kbps uplink profile.
I start to notice differences of framerate and resolution here. In MaintainFrame mode, the video stays at lower resolution and it does look smoother;
While in MaintainResolution mode, the video stays at high resolution, but less smooth.
I don't see serious choppiness from either modes though.
Poor 3G (both uplink and downlink)
In this experiment, both uplink and downlink were bandwidth constrained, which produced noticeable choppiness regardless of the degradation preference. With the downlink no longer being the bottleneck, the differences between the two modes became much clearer.
Overall, the severe choppiness observed previously appears to have been caused by the network being constrained in both directions rather than by the degradation preference itself.
Personal Impression
When bandwidth is insufficient, the trade off between image quality and motion smoothness is largely subjective. Personally, I slightly prefer maintaining resolution, as I find occasional choppiness less distracting than a combination of choppiness and significant blurriness.
We are still having some discussions internally to figure out the best default option here, and appreciate any input / opinions here.
There was a problem hiding this comment.
The experiments are useful, and MAINTAIN_FRAMERATE is the right baseline: with the preference unset, the WebRTC build this SDK ships (io.github.webrtc-sdk:android-prefixed 144.7559.05) resolves ordinary camera content to MAINTAIN_FRAMERATE (GetDegradationPreference() in webrtc_video_engine.cc on m144_release, BALANCED sits behind the WebRTC-Video-BalancedDegradation field trial, which this SDK doesn't set), and screenshare already resolves to MAINTAIN_RESOLUTION. Two things still give me pause about changing the default:
-
Network conditioning only exercises half of what this knob controls. Degradation preference also drives CPU overuse adaptation. Under MAINTAIN_FRAMERATE an overloaded encoder downscales; under MAINTAIN_RESOLUTION it drops frames instead. A low-end phone that can't encode 720p30 won't show up in conditioner runs on a dev device, and Android has a lot of those phones.
-
Your own results describe the bandwidth-constrained tradeoff as subjective and content dependent, and the start bitrate fix already handles the blurry first seconds on links that can carry the target. What's left is a steady state preference, which is a product-level default to pick deliberately and across the SDKs, not on Android alone inside a bitrate PR. The option is exposed per publish today, so apps that prefer resolution can already opt in.
I'd take the default change out of this PR and run the default discussion separately.
There was a problem hiding this comment.
I changed the default degradation preference to Maintain_Resolution for screen sharing, Maintain_Framerate for camera sources, and Balanced for other sources (which aren't supported today).
This matches the behavior we're standardizing across the other LiveKit SDKs.
I'm happy to move these changes into a separate PR if you think they will need more discussions.
| override val backupCodec: BackupVideoCodec? = null, | ||
| override val degradationPreference: RtpParameters.DegradationPreference? = null, | ||
| // Default to MAINTAIN_RESOLUTION to prevent initial video blurriness | ||
| override val degradationPreference: RtpParameters.DegradationPreference? = RtpParameters.DegradationPreference.MAINTAIN_RESOLUTION, |
There was a problem hiding this comment.
This flips the default for every video publisher, not just the slow-ramp case. MAINTAIN_RESOLUTION holds resolution by dropping framerate under congestion, which suits screen share but turns camera video into low/stuttering framerate on constrained networks; WebRTC's default for camera content is maintain-framerate. The same change is in VideoTrackPublishOptions below, and the KDoc on the abstract degradationPreference still reads "null value indicates default value (maintain framerate)", which now contradicts the default. Consider scoping MAINTAIN_RESOLUTION to screen share rather than making it the global default, and updating the KDoc.
There was a problem hiding this comment.
Agreed. I changed the degradationPreference to Maintain_Resolution for screenshare, and back to Maintain_Framerate for Camera.
| @@ -0,0 +1,591 @@ | |||
| diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml | |||
There was a problem hiding this comment.
This 591-line patch file looks accidentally committed. It's a separate SDP-parser refactor (JainSDP MediaDescription to a hand-rolled SdpMediaSection), unrelated to the bitrate change, and shouldn't be in the tree. It also trips git diff --check on trailing whitespace. Please drop it before merge.
There was a problem hiding this comment.
this was moved, it was checked in mistakenly by claude.
- 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>
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.
| @@ -1 +1 @@ | |||
| Subproject commit 8381f2180c45ab926b3ebf19df0608f1dadcac1e | |||
| Subproject commit 4c05a3325ec35760bee1c0bfe57b7011604a124f | |||
There was a problem hiding this comment.
This moves the protocol submodule pointer backwards, from 8381f21 (current main) to 4c05a33, an ancestor ~303 commits / 8 months older. Looks accidental, probably a stale local submodule checkout that got staged with the fix. On merge it would roll the protobuf definitions back for everyone.
It is also not just a silent revert: the branch does not compile against 4c05a33. A clean :livekit-android-sdk:compileReleaseKotlin fails with unresolved references to symbols added to the protocol after that commit (AGENT_ERROR in RoomEvent.kt, PUBLISH_DATA_TRACK_RESPONSE/UNPUBLISH_DATA_TRACK_RESPONSE/DATA_TRACK_SUBSCRIBER_HANDLES in SignalClient.kt, clientProtocol/CONNECTOR/BRIDGE in Participant.kt and ClientInfo.kt). CI currently stops earlier at the Spotless check, so this will surface as the next failure once that is fixed.
Restoring it against main:
git checkout main -- protocol && git commit -m "chore: restore protocol submodule pointer"|
|
||
| // Skip start bitrate hint for very low bitrate tracks - the hint hurts more than it helps | ||
| if (trackBr.maxBitrate < minTargetBitrateKbps) { | ||
| continue |
There was a problem hiding this comment.
This continue skips more than the start hint: the x-google-max-bitrate write below is bypassed too, so a track with a sub-300 kbps target loses the codec-level cap it got before this PR. An SVC publish registered at 250 kbps previously produced x-google-start-bitrate=175;x-google-max-bitrate=250; now it gets neither. In Rust the equivalent early return is harmless because that munger never writes x-google-max-bitrate, but here it does. I'd gate only the x-google-start-bitrate append on the threshold (in both the existing-fmtp branch and the !fmtpFound one) and keep the max-bitrate write unconditional.
|
@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.
| 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.
2 participants
Summary
x-google-start-bitrateSDP hint to all video codecs (VP8, VP9, AV1, H264, H265), not just SVC codecsdegradationPreferencetoMAINTAIN_RESOLUTIONfor video tracksProblem
Video starts blurry for 5-15 seconds before improving. This is caused by WebRTC's bandwidth estimator starting at ~300kbps and slowly ramping up to the target bitrate.
Solution
Test plan
🤖 Generated with Claude Code