-
Notifications
You must be signed in to change notification settings - Fork 151
fix: improve initial video quality by setting x-google-start-bitrate for all video codecs #973
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
3dbe09b
2c6a0ca
6364aee
67d21f1
8d2e188
99ee7a7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -437,13 +437,24 @@ fun ensureVideoDDExtensionForSVC(mediaDesc: MediaDescription) { | |
| } | ||
| } | ||
|
|
||
| /* The svc codec (av1/vp9) would use a very low bitrate at the beginning and | ||
| increase slowly by the bandwidth estimator until it reach the target bitrate. The | ||
| process commonly cost more than 10 seconds cause subscriber will get blur video at | ||
| the first few seconds. So we use a 70% of target bitrate here as the start bitrate to | ||
| eliminate this issue. | ||
| */ | ||
| private const val startBitrateForSVC = 0.7 | ||
| /* | ||
| * 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 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 | ||
|
|
||
| /** | ||
| * @suppress | ||
|
|
@@ -469,14 +480,29 @@ fun ensureCodecBitrates( | |
| ?: continue | ||
| val codecPayload = rtp.payload | ||
|
|
||
| // Skip start bitrate hint for very low bitrate tracks - the hint hurts more than it helps | ||
| if (trackBr.maxBitrate < minTargetBitrateKbps) { | ||
| continue | ||
| } | ||
|
|
||
| val fmtps = media.getFmtps() | ||
| // Use 90% of target bitrate, capped at 1 Mbps for camera to prevent BWE from starting too aggressively | ||
| // 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) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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), Two consequences:
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 |
||
| calculatedStartBitrate | ||
| } else { | ||
| minOf(calculatedStartBitrate, maxStartBitrateKbps) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| } | ||
|
|
||
| var fmtpFound = false | ||
| for ((attribute, fmtp) in fmtps) { | ||
| if (fmtp.payload == codecPayload) { | ||
| fmtpFound = true | ||
| var newFmtpConfig = fmtp.config | ||
| if (!fmtp.config.contains("x-google-start-bitrate")) { | ||
| newFmtpConfig = "$newFmtpConfig;x-google-start-bitrate=${(trackBr.maxBitrate * startBitrateForSVC).roundToLong()}" | ||
| newFmtpConfig = "$newFmtpConfig;x-google-start-bitrate=$startBitrate" | ||
| } | ||
| if (!fmtp.config.contains("x-google-max-bitrate")) { | ||
| newFmtpConfig = "$newFmtpConfig;x-google-max-bitrate=${trackBr.maxBitrate}" | ||
|
|
@@ -492,7 +518,7 @@ fun ensureCodecBitrates( | |
| media.addAttribute( | ||
| SdpFmtp( | ||
| payload = codecPayload, | ||
| config = "x-google-start-bitrate=${trackBr.maxBitrate * startBitrateForSVC};" + | ||
| config = "x-google-start-bitrate=$startBitrate;" + | ||
| "x-google-max-bitrate=${trackBr.maxBitrate}", | ||
| ).toAttributeField(), | ||
| ) | ||
|
|
@@ -512,6 +538,7 @@ internal fun isSVCCodec(codec: String?): Boolean { | |
| data class TrackBitrateInfo( | ||
| val codec: String, | ||
| val maxBitrate: Long, | ||
| val isScreenShare: Boolean = false, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
One consideration for both the new |
||
| ) | ||
|
|
||
| /** | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -697,14 +697,22 @@ internal constructor( | |
| track.statsGetter = engine.createStatsGetter(transceiver.sender) | ||
|
|
||
| val finalOptions = options | ||
| // Handle trackBitrates | ||
| if (encodings.isNotEmpty()) { | ||
| if (finalOptions is VideoTrackPublishOptions && isSVCCodec(finalOptions.videoCodec) && encodings.firstOrNull()?.maxBitrateBps != null) { | ||
| // 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) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A lifecycle gap this expansion widens: |
||
| val targetBitrateBps: Long = if (isSVCCodec(finalOptions.videoCodec)) { | ||
| (encodings.firstOrNull()?.maxBitrateBps ?: 0).toLong() | ||
| } else { | ||
| encodings.sumOf { (it.maxBitrateBps ?: 0).toLong() } | ||
| } | ||
| if (targetBitrateBps > 0) { | ||
| engine.registerTrackBitrateInfo( | ||
| cid = cid, | ||
| TrackBitrateInfo( | ||
| codec = finalOptions.videoCodec, | ||
| maxBitrate = (encodings.first().maxBitrateBps?.div(1000) ?: 0).toLong(), | ||
| maxBitrate = targetBitrateBps / 1000, | ||
| isScreenShare = trackSource == Track.Source.SCREEN_SHARE, | ||
| ), | ||
| ) | ||
| } | ||
|
|
@@ -716,7 +724,12 @@ internal constructor( | |
| (track as LocalVideoTrack).codec = finalOptions.videoCodec | ||
|
|
||
| val rtpParameters = transceiver.sender.parameters | ||
| // Use provided degradation preference, or default based on track source: | ||
| // - Camera: MAINTAIN_FRAMERATE (smoother video for real-time communication) | ||
| // - Screen share: MAINTAIN_RESOLUTION (clarity is critical for text/UI) | ||
| // - Other/unknown: BALANCED | ||
| rtpParameters.degradationPreference = finalOptions.degradationPreference | ||
| ?: getDefaultDegradationPreference(trackSource) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| transceiver.sender.parameters = rtpParameters | ||
| } | ||
|
|
||
|
|
@@ -1456,10 +1469,17 @@ abstract class BaseVideoTrackPublishOptions { | |
| abstract val backupCodec: BackupVideoCodec? | ||
|
|
||
| /** | ||
| * When bandwidth is constrained, this preference indicates which is preferred | ||
| * between degrading resolution vs. framerate. | ||
| * Controls how the encoder trades off between resolution and framerate | ||
| * when bandwidth is constrained. | ||
| * | ||
| * - MAINTAIN_FRAMERATE: Prioritizes framerate, reduces resolution if needed | ||
| * - MAINTAIN_RESOLUTION: Prioritizes resolution, drops frames if needed | ||
| * - BALANCED: Balances between both | ||
| * | ||
| * null value indicates default value (maintain framerate). | ||
| * If not set (null), the SDK uses defaults based on track source: | ||
| * - Camera: MAINTAIN_FRAMERATE (smoother video for real-time communication) | ||
| * - Screen share: MAINTAIN_RESOLUTION (clarity is critical for text/UI) | ||
| * - Other/unknown: BALANCED | ||
| */ | ||
| abstract val degradationPreference: RtpParameters.DegradationPreference? | ||
|
|
||
|
|
@@ -1481,6 +1501,8 @@ data class VideoTrackPublishDefaults( | |
| override val videoCodec: String = VideoCodec.VP8.codecName, | ||
| override val scalabilityMode: String? = null, | ||
| override val backupCodec: BackupVideoCodec? = null, | ||
| // Default is null - SDK applies source-based defaults at runtime: | ||
| // Camera: MAINTAIN_FRAMERATE, Screen share: MAINTAIN_RESOLUTION, Other: BALANCED | ||
| override val degradationPreference: RtpParameters.DegradationPreference? = null, | ||
| override val simulcastLayers: List<VideoPreset>? = null, | ||
| ) : BaseVideoTrackPublishOptions() | ||
|
|
@@ -1494,6 +1516,8 @@ data class VideoTrackPublishOptions( | |
| override val backupCodec: BackupVideoCodec? = null, | ||
| override val source: Track.Source? = null, | ||
| override val stream: String? = null, | ||
| // Default is null - SDK applies source-based defaults at runtime: | ||
| // Camera: MAINTAIN_FRAMERATE, Screen share: MAINTAIN_RESOLUTION, Other: BALANCED | ||
| override val degradationPreference: RtpParameters.DegradationPreference? = null, | ||
| override val simulcastLayers: List<VideoPreset>? = null, | ||
| ) : BaseVideoTrackPublishOptions(), TrackPublishOptions { | ||
|
|
@@ -1661,6 +1685,21 @@ internal fun VideoTrackPublishOptions.hasBackupCodec(): Boolean { | |
| private val backupCodecs = listOf(VideoCodec.VP8.codecName, VideoCodec.H264.codecName) | ||
| private fun isBackupCodec(codecName: String) = backupCodecs.contains(codecName) | ||
|
|
||
| /** | ||
| * Returns the appropriate degradation preference for a video track based on its source. | ||
| * | ||
| * - Camera: MAINTAIN_FRAMERATE (smoother video for real-time communication) | ||
| * - Screen share: MAINTAIN_RESOLUTION (clarity is critical for reading text/UI) | ||
| * - Other/unknown: BALANCED | ||
| */ | ||
| private fun getDefaultDegradationPreference(source: Track.Source): RtpParameters.DegradationPreference { | ||
| return when (source) { | ||
| Track.Source.CAMERA -> RtpParameters.DegradationPreference.MAINTAIN_FRAMERATE | ||
| Track.Source.SCREEN_SHARE -> RtpParameters.DegradationPreference.MAINTAIN_RESOLUTION | ||
| else -> RtpParameters.DegradationPreference.BALANCED | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * A handler that processes an RPC request and returns a string | ||
| * that will be sent back to the requester. The payload must | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This appears to be unrelated to the bitrate/SDP change.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, yes, it should removed, it got checked in by mistake |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This
continueskips more than the start hint: thex-google-max-bitratewrite 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 producedx-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 writesx-google-max-bitrate, but here it does. I'd gate only thex-google-start-bitrateappend on the threshold (in both the existing-fmtp branch and the!fmtpFoundone) and keep the max-bitrate write unconditional.