fix: improve initial video quality by setting x-google-start-bitrate for all video codecs #973
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| --- | ||
| "client-sdk-android": patch | ||
| --- | ||
|
|
||
| Seed the bandwidth estimator with `x-google-start-bitrate` for all video codecs, not just SVC, so published video reaches its target quality in the first second or two instead of ramping from ~300 kbps over 5-15 seconds. The hint is 90% of the track's target bitrate, capped at 1 Mbps for camera tracks (screen shares are exempt, since they are published at high bitrates for text legibility) and skipped below a 300 kbps target, where seeding high costs more than it gains. | ||
|
|
||
| Because libwebrtc applies these codec fmtp parameters to the whole peer connection rather than the m-section carrying them, the SDK now writes a single connection-level value to every video m-section, once per publisher connection. Re-seeding a converged estimator is avoided: the value persists in libwebrtc's bitrate configurator and is automatically re-applied on network route changes, and a full reconnect builds a new peer connection and seeds it again. | ||
|
|
||
| **Behavior change:** the SDK no longer writes `x-google-max-bitrate` into SDP. That value was promoted to a ceiling on total send bandwidth for the entire connection, so a camera publication could throttle a concurrent screen share. Per-track and per-layer limits continue to be enforced through `RtpParameters.Encoding.maxBitrateBps`, which is correctly scoped per encoding. Applications that relied on the SDP value as a connection-wide cap should set encoding bitrates instead. This matches client-sdk-js and the Rust SDK, neither of which writes it. | ||
|
|
||
| `TrackBitrateInfo` and `TrackBitrateInfoKey` are now `internal`. They were never intended as public API (both were `@suppress`ed and only reachable through a `@VisibleForTesting` helper) and were public only to be visible from the test module, which is no longer necessary. |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -94,6 +94,11 @@ constructor( | |
| private var renegotiate = false | ||
|
|
||
| private val trackBitrates = mutableMapOf<TrackBitrateInfoKey, TrackBitrateInfo>() | ||
|
|
||
| // x-google-start-bitrate is a connection-level BWE hint in libwebrtc. Keep it | ||
| // available through data-channel/audio-only offers and consume it only after a | ||
| // local video m-section successfully gets the hint. | ||
| private var hasAppliedVideoStartBitrate = false | ||
| private var isClosed = AtomicBoolean(false) | ||
|
|
||
| private val latestOfferId = AtomicInteger(0) | ||
|
|
@@ -206,18 +211,37 @@ constructor( | |
| val sdpDescription = sdpFactory.createSessionDescription(sdpOffer.description) | ||
|
|
||
| val mediaDescs = sdpDescription.getMediaDescriptions(true) | ||
| .filterIsInstance<MediaDescription>() | ||
| // The publisher PeerConnection may negotiate before any video is published | ||
| // (for example, data channel only or audio first). Those offers should not | ||
| // consume the video start hint. When the first video offer is created, use | ||
| // one connection-level value across all video m-sections so libwebrtc's | ||
| // last-writer-wins handling cannot depend on SDP m-section order. | ||
| val connectionStartBitrate = if (!hasAppliedVideoStartBitrate) { | ||
| computeConnectionStartBitrate(mediaDescs, trackBitrates) | ||
| } else { | ||
| null | ||
| } | ||
| var appliedVideoStartBitrate = false | ||
| for (mediaDesc in mediaDescs) { | ||
| if (mediaDesc.media.mediaType == "audio") { | ||
| // TODO | ||
| } else if (mediaDesc.media.mediaType == "video") { | ||
| ensureVideoDDExtensionForSVC(mediaDesc) | ||
| appliedVideoStartBitrate = ensureCodecBitrates( | ||
| mediaDesc, | ||
| trackBitrates = trackBitrates, | ||
| connectionStartBitrate = connectionStartBitrate, | ||
| ) || appliedVideoStartBitrate | ||
| } | ||
| } | ||
| val mungedDescription = sdpDescription.toString() | ||
| finalSdp = setMungedSdp(sdpOffer, mungedDescription) | ||
| // setMungedSdp may fall back to the original SDP. Only mark the one-shot | ||
| // hint as used after the SDP with the hint is accepted locally. | ||
| if (appliedVideoStartBitrate && finalSdp?.description == mungedDescription) { | ||
| hasAppliedVideoStartBitrate = true | ||
| } | ||
| } | ||
|
|
||
| finalSdp?.let { sdp -> | ||
|
|
@@ -437,66 +461,157 @@ fun ensureVideoDDExtensionForSVC(mediaDesc: MediaDescription) { | |
| } | ||
| } | ||
|
|
||
| /* | ||
| * Video codecs use a very low bitrate at the beginning and increase slowly by | ||
| * the bandwidth estimator until they reach the target bitrate. The process commonly | ||
| * costs more than 10 seconds causing subscribers to get blurry video at the first | ||
| * few seconds. We use x-google-start-bitrate to hint the BWE to start higher. | ||
| * | ||
| * Why 90%: Gives ~10% headroom for bandwidth estimation while starting close to target. | ||
| * Why same for all codecs: Target bitrate already accounts for codec efficiency | ||
| * (e.g., users set lower targets for VP9/AV1 knowing they're more efficient). | ||
| * Why cap camera at 1 Mbps: Prevents BWE from starting too aggressively on high bitrate tracks. | ||
| */ | ||
| private const val startBitrateMultiplier = 0.9 | ||
|
|
||
| /** Maximum x-google-start-bitrate in kbps. 1 Mbps prevents BWE from starting too aggressively. */ | ||
| private const val maxStartBitrateKbps = 1000L | ||
|
|
||
| /** Minimum target bitrate in kbps to apply start bitrate hint. Below this, the hint hurts more than it helps. */ | ||
| private const val minTargetBitrateKbps = 300L | ||
|
|
||
| @VisibleForTesting | ||
| internal fun ensureCodecBitrates( | ||
| media: MediaDescription, | ||
| trackBitrates: Map<TrackBitrateInfoKey, TrackBitrateInfo>, | ||
| ) { | ||
| ensureCodecBitrates( | ||
| media = media, | ||
| trackBitrates = trackBitrates, | ||
| connectionStartBitrate = computeConnectionStartBitrate(trackBitrates.values), | ||
| ) | ||
| } | ||
|
|
||
| /* | ||
| * 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 shared Call via SetSdpBitrateParameters, where RtpBitrateConfigurator stores one | ||
| * config for the whole peer connection. Two m-sections carrying different values is last-writer-wins. | ||
| * | ||
| * Hence: one value, written to every video m-section, once. | ||
| * | ||
| * Write it once because the value persists. RtpBitrateConfigurator keeps start_bitrate_bps in its | ||
| * stored config and re-applies it on every network route change (RtpTransportControllerSend:: | ||
| * OnNetworkRouteChanged reads GetConfig()), so a WiFi-to-cellular handover re-seeds the estimator | ||
| * from this hint with no renegotiation. Rewriting it later 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 bandwidth estimator, so a full reconnect -- a new peer connection, a new estimator -- is | ||
| * the only thing that should seed it again. | ||
| * | ||
| * Never write x-google-max-bitrate. The same Call-level promotion turns a per-track cap into a | ||
| * ceiling on total send bandwidth, so a 2.3 Mbps camera would starve a concurrent 3 Mbps screen | ||
| * share. libwebrtc carries a TODO conceding this is wrong ("codec max bitrate should probably not | ||
| * affect global call max bitrate"). Per-track and per-layer caps belong in | ||
| * RtpParameters.Encoding.maxBitrateBps, which is genuinely scoped per encoding. client-sdk-js and | ||
| * the Rust SDK never write it either. | ||
| */ | ||
| @VisibleForTesting | ||
| internal fun ensureCodecBitrates( | ||
| media: MediaDescription, | ||
| trackBitrates: Map<TrackBitrateInfoKey, TrackBitrateInfo>, | ||
| connectionStartBitrate: Long?, | ||
| ): Boolean { | ||
| // Returns true when this media section maps to a local video track and has or | ||
| // receives the connection-level start hint. | ||
| val startBitrate = connectionStartBitrate ?: return false | ||
| val (_, codecPayload) = findTrackCodecBitrateInfo(media, trackBitrates) ?: return false | ||
|
|
||
| val fmtps = media.getFmtps() | ||
| var fmtpFound = false | ||
| for ((attribute, fmtp) in fmtps) { | ||
| if (fmtp.payload == codecPayload) { | ||
| fmtpFound = true | ||
| if (fmtp.config.contains("x-google-start-bitrate")) { | ||
| return true | ||
| } | ||
| attribute.value = "${fmtp.payload} ${fmtp.config};x-google-start-bitrate=$startBitrate" | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if (!fmtpFound) { | ||
| media.addAttribute( | ||
| SdpFmtp( | ||
| payload = codecPayload, | ||
| config = "x-google-start-bitrate=$startBitrate", | ||
| ).toAttributeField(), | ||
| ) | ||
| } | ||
| return true | ||
| } | ||
|
|
||
| private fun computeConnectionStartBitrate( | ||
| mediaDescriptions: Collection<MediaDescription>, | ||
| trackBitrates: Map<TrackBitrateInfoKey, TrackBitrateInfo>, | ||
| ): Long? { | ||
| // Use only video m-sections in the current SDP. trackBitrates can contain | ||
| // stale entries after unpublish, and those must not affect the connection hint. | ||
| return mediaDescriptions | ||
| .asSequence() | ||
| .filter { media -> media.media.mediaType == "video" } | ||
| .mapNotNull { media -> findTrackCodecBitrateInfo(media, trackBitrates)?.trackBitrateInfo } | ||
| .mapNotNull(::computeTrackStartBitrate) | ||
| .maxOrNull() | ||
| } | ||
|
|
||
| /** | ||
| * @suppress | ||
| */ | ||
| @VisibleForTesting | ||
| internal fun computeConnectionStartBitrate(trackBitrates: Collection<TrackBitrateInfo>): Long? { | ||
| return trackBitrates.mapNotNull(::computeTrackStartBitrate).maxOrNull() | ||
| } | ||
|
|
||
| private data class TrackCodecBitrateInfo( | ||
| val trackBitrateInfo: TrackBitrateInfo, | ||
| val codecPayload: Long, | ||
| ) | ||
|
|
||
| private fun findTrackCodecBitrateInfo( | ||
| media: MediaDescription, | ||
| trackBitrates: Map<TrackBitrateInfoKey, TrackBitrateInfo>, | ||
| ): TrackCodecBitrateInfo? { | ||
| val msid = media.getMsid()?.value ?: return null | ||
| for ((key, trackBitrateInfo) in trackBitrates) { | ||
| if (key !is TrackBitrateInfoKey.Cid) { | ||
| continue | ||
| } | ||
| if (!msid.contains(key.value)) { | ||
| continue | ||
| } | ||
| val (_, rtp) = media.getRtps() | ||
| .firstOrNull { (_, rtp) -> rtp.codec.equals(trackBitrateInfo.codec, ignoreCase = true) } | ||
| ?: continue | ||
| return TrackCodecBitrateInfo( | ||
| trackBitrateInfo = trackBitrateInfo, | ||
| codecPayload = rtp.payload, | ||
| ) | ||
| } | ||
| return null | ||
| } | ||
|
|
||
| private fun computeTrackStartBitrate(trackBr: TrackBitrateInfo): Long? { | ||
| if (trackBr.targetBitrateKbps < minTargetBitrateKbps) { | ||
| return null | ||
| } | ||
|
|
||
| // TODO: dynamically adjust start bitrate based on network conditions, such as | ||
| // using the previous BWE estimate. | ||
| val calculatedStartBitrate = (trackBr.targetBitrateKbps * startBitrateMultiplier).roundToLong() | ||
| return if (trackBr.isScreenShare) { | ||
| calculatedStartBitrate | ||
| } else { | ||
| minOf(calculatedStartBitrate, maxStartBitrateKbps) | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -507,17 +622,30 @@ internal fun isSVCCodec(codec: String?): Boolean { | |
| } | ||
|
|
||
| /** | ||
| * The bitrate a local video track was published at, used to derive the connection-level | ||
| * `x-google-start-bitrate` hint in [ensureCodecBitrates]. | ||
| * | ||
| * Carries no max bitrate: per-track and per-layer caps belong in | ||
| * [livekit.org.webrtc.RtpParameters.Encoding.maxBitrateBps], not in SDP. | ||
| * | ||
| * @param codec The codec the track is published with, matched against the m-section's rtpmap. | ||
| * @param targetBitrateKbps The track's target bitrate in **kbps** (not bps). For SVC this is the | ||
| * single encoding's bitrate; for simulcast it is the sum across layers, since the bandwidth | ||
| * estimator has to carry all of them. | ||
| * @param isScreenShare Whether the track is a screen share. Screen shares are exempt from the | ||
| * [maxStartBitrateKbps] cap: they are typically published at high bitrates for text legibility, | ||
| * and unlike camera content a conservative start is more costly than a brief overshoot. | ||
| */ | ||
| internal data class TrackBitrateInfo( | ||
| val codec: String, | ||
| val targetBitrateKbps: Long, | ||
| val isScreenShare: Boolean = false, | ||
|
Contributor
There was a problem hiding this comment.
One consideration for both the new
Contributor
Author
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.
Contributor
There was a problem hiding this comment. Correct, and making the types internal is the cleaner call. Resolving. |
||
| ) | ||
|
|
||
| /** | ||
| * Identifies the local track a [TrackBitrateInfo] belongs to. | ||
| */ | ||
| internal sealed class TrackBitrateInfoKey { | ||
| data class Cid(val value: String) : TrackBitrateInfoKey() | ||
| data class Transceiver(val value: RtpTransceiver) : TrackBitrateInfoKey() | ||
| } | ||
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
Oops, something went wrong.
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.