fix x-google-start-bitrate for vp8 and other codex and make it adaptive to network#1226
Conversation
…ve based on network conditions
ChangesetThe following package versions will be affected by this PR:
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| // 3) For video codecs without fmtp lines (VP8/VP9), create new fmtp lines | ||
| // This is needed because VP8/VP9 don't have fmtp lines by default | ||
| let pts_with_fmtp: std::collections::HashSet<String> = out | ||
| .iter() | ||
| .filter_map(|line| { | ||
| let l = line.trim(); | ||
| if let Some(rest) = l.strip_prefix("a=fmtp:") { | ||
| rest.split_whitespace().next().map(|s| s.to_string()) | ||
| } else { | ||
| None | ||
| } | ||
| }) | ||
| .collect(); | ||
|
|
||
| // Find rtpmap lines and insert fmtp after them for codecs without fmtp | ||
| let mut final_out: Vec<String> = Vec::with_capacity(out.len() + target_pts.len()); | ||
| for line in out.iter() { | ||
| final_out.push(line.clone()); | ||
|
|
||
| // Check if this is an rtpmap line for a video codec without fmtp | ||
| let l = line.trim(); | ||
| if let Some(rest) = l.strip_prefix("a=rtpmap:") { | ||
| let mut it = rest.split_whitespace(); | ||
| let pt = it.next().unwrap_or(""); | ||
| let codec = it.next().unwrap_or(""); | ||
| if Self::is_video_codec(codec) | ||
| && !pt.is_empty() | ||
| && !pts_with_fmtp.contains(pt) | ||
| { | ||
| // Create fmtp line with x-google-start-bitrate | ||
| let fmtp_line = | ||
| format!("a=fmtp:{pt} x-google-start-bitrate={start_bitrate_kbps}"); | ||
| log::debug!( | ||
| "Creating fmtp line for {} (pt={}): {}", | ||
| codec, | ||
| pt, | ||
| fmtp_line | ||
| ); | ||
| final_out.push(fmtp_line); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🔴 Existing test now contradicts the new behavior that creates format lines for codecs missing them, causing test failure
A new fmtp line is now synthesized for video codecs without one (step 3 added at livekit/src/rtc_engine/peer_transport.rs:403-446), but the pre-existing test asserts the SDP is unchanged when no fmtp line exists (livekit/src/rtc_engine/peer_transport.rs:652-655), so the test suite will fail.
Impact: The test suite cannot pass, blocking CI and merging.
New code path creates fmtp lines that the old test explicitly forbids
The test vp9_without_fmtp_line_is_noop at line 642 calls munge_x_google_start_bitrate with an SDP containing a=rtpmap:98 VP9/90000 but no a=fmtp:98 ... line, and asserts out == sdp (no modification). However, the new code at lines 407-446 builds a pts_with_fmtp set, finds that pt 98 has no fmtp, and inserts a=fmtp:98 x-google-start-bitrate=3200 after the rtpmap line. This makes out != sdp, failing the assert_eq! at line 652.
The test comment says "function intentionally does not insert a new fmtp line" which was the old behavior. The test needs to be updated to match the new intentional behavior.
Was this helpful? React with 👍 or 👎 to provide feedback.
| Err(WsError::Io(ref io_err)) | ||
| if io_err.kind() == std::io::ErrorKind::UnexpectedEof => | ||
| { | ||
| // TLS connection closed without close_notify - treat as normal close | ||
| // This happens when the server closes the connection abruptly | ||
| log::debug!("websocket closed with unexpected EOF (TLS close without close_notify)"); | ||
| break; |
There was a problem hiding this comment.
🔍 Matching on Err(WsError::Io(ref io_err)) may not match wrapped TLS errors
The pattern Err(WsError::Io(ref io_err)) if io_err.kind() == UnexpectedEof at livekit-api/src/signal_client/signal_stream.rs:413-419 assumes the TLS close-without-close_notify surfaces as a direct io::Error with UnexpectedEof kind. Depending on the tungstenite version and TLS backend (native-tls vs rustls), TLS errors may be wrapped differently — e.g., as WsError::Tls(...) rather than WsError::Io(...). If the TLS error doesn't surface as WsError::Io, this handler won't match and the error will fall through to the catch-all _ => branch (which still breaks, just with an error log). Not a correctness bug, but the handler may be less effective than intended.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Devin Review found 5 new potential issues.
⚠️ 1 issue in files not directly in the diff
⚠️ Changeset omits the livekit crate despite behavioral changes to degradation defaults and SDP munging (.changeset/agent-deployment-docs.md:1-5)
The changeset file (.changeset/agent-deployment-docs.md) only lists livekit-api for bumping, but the PR also changes the livekit crate's behavior (degradation preference defaults in livekit/src/room/options.rs:194-198 and SDP munging in livekit/src/rtc_engine/peer_transport.rs), so the livekit crate version won't be bumped.
Impact: Downstream consumers won't know the livekit crate behavior changed, violating semantic versioning expectations.
| // Default based on track source | ||
| match options.source { | ||
| TrackSource::Camera => DegradationPreference::MaintainFramerate, | ||
| TrackSource::Screenshare => DegradationPreference::MaintainResolution, | ||
| _ => DegradationPreference::Balanced, | ||
| } |
There was a problem hiding this comment.
🔍 Degradation preference default change is a significant behavioral shift for camera tracks
The default degradation preference for camera tracks changed from MaintainResolution (old code at the base, which returned MaintainResolution for all sources) to MaintainFramerate (livekit/src/room/options.rs:195). This means existing users who relied on the previous default will now see their camera tracks prioritize framerate over resolution during bandwidth constraints — the opposite of the previous behavior. While the new default may be more appropriate for real-time communication, this is a silent behavioral change that could surprise users who were satisfied with the previous clarity-preserving behavior. The _height parameter is now fully unused (livekit/src/room/options.rs:186), confirming the resolution-based logic was removed.
Was this helpful? React with 👍 or 👎 to provide feedback.
| /// Maximum x-google-start-bitrate (kbps). | ||
| /// 1 Mbps is a reasonable ceiling that prevents BWE from starting too aggressively. | ||
| const MAX_START_BITRATE_KBPS: u32 = 1000; | ||
|
|
||
| /// Compute the x-google-start-bitrate value for SDP munging. | ||
| /// | ||
| /// Returns min(90% of target, 1 Mbps). Returns None if no target bitrate is set | ||
| /// (initial offer before track publish) or if the target is too low. | ||
| fn compute_start_bitrate_kbps(target_bps: Option<u64>) -> Option<u32> { | ||
| let target_bps = target_bps?; | ||
| let target_kbps = (target_bps / 1000) as u32; | ||
|
|
||
| if target_kbps == 0 || target_kbps < 300 { | ||
| return None; | ||
| } | ||
|
|
||
| Some(start_kbps.min(ultimate_kbps)) | ||
| // Use 90% of target bitrate as start bitrate, capped at 1 Mbps | ||
| let start_kbps = (target_kbps as f64 * 0.9).round() as u32; | ||
| Some(start_kbps.min(target_kbps).min(Self::MAX_START_BITRATE_KBPS)) |
There was a problem hiding this comment.
🔍 New 1 Mbps cap on start bitrate may cause slow ramp-up for high-resolution streams
The new MAX_START_BITRATE_KBPS = 1000 constant at livekit/src/rtc_engine/peer_transport.rs:216 caps the x-google-start-bitrate hint at 1 Mbps. Previously, the start bitrate was simply 90% of the target with no upper cap. For high-resolution streams (e.g., 4K at 8 Mbps target), the old code would set start bitrate to ~7200 kbps, while the new code caps it at 1000 kbps. This means WebRTC's bandwidth estimator will start much lower and need to ramp up, potentially causing initial quality degradation for high-bitrate use cases. The comment says this "prevents BWE from starting too aggressively" but the tradeoff is slower convergence to target quality for high-bitrate streams.
Was this helpful? React with 👍 or 👎 to provide feedback.
| // 3) For video codecs that don't already have fmtp lines, create new ones | ||
| // with x-google-start-bitrate. This handles cases where the browser/WebRTC | ||
| // didn't generate an fmtp line for a particular payload type (e.g., VP8 | ||
| // typically has no fmtp, while H.264/VP9-SVC/AV1 usually do). | ||
| let pts_with_fmtp: std::collections::HashSet<String> = out | ||
| .iter() | ||
| .filter_map(|line| { | ||
| let l = line.trim(); | ||
| if let Some(rest) = l.strip_prefix("a=fmtp:") { | ||
| rest.split_whitespace().next().map(|s| s.to_string()) | ||
| } else { | ||
| None | ||
| } | ||
| }) | ||
| .collect(); | ||
|
|
||
| // Find rtpmap lines and insert fmtp after them for video codecs without existing fmtp | ||
| let mut final_out: Vec<String> = Vec::with_capacity(out.len() + target_pts.len()); | ||
| for line in out.iter() { | ||
| final_out.push(line.clone()); | ||
|
|
||
| // Check if this is an rtpmap line for a video codec without an existing fmtp line | ||
| let l = line.trim(); | ||
| if let Some(rest) = l.strip_prefix("a=rtpmap:") { | ||
| let mut it = rest.split_whitespace(); | ||
| let pt = it.next().unwrap_or(""); | ||
| let codec = it.next().unwrap_or(""); | ||
| if Self::is_video_codec(codec) | ||
| && !pt.is_empty() | ||
| && !pts_with_fmtp.contains(pt) | ||
| { | ||
| // Create fmtp line with x-google-start-bitrate | ||
| let fmtp_line = | ||
| format!("a=fmtp:{pt} x-google-start-bitrate={start_bitrate_kbps}"); | ||
| log::debug!( | ||
| "Creating fmtp line for {} (pt={}): {}", | ||
| codec, | ||
| pt, | ||
| fmtp_line | ||
| ); | ||
| final_out.push(fmtp_line); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🔍 New fmtp line creation for codecs without existing fmtp may produce non-standard SDP
The new step 3 at livekit/src/rtc_engine/peer_transport.rs:403-446 creates a=fmtp:<pt> x-google-start-bitrate=<value> lines for video codecs that don't already have fmtp lines (e.g., VP8 which typically has none). While x-google-start-bitrate is a Chrome/libwebrtc-specific hint, creating an fmtp line with ONLY this non-standard parameter (no standard codec parameters) is unusual. Some SDP parsers or SFUs might not expect an fmtp line for VP8 at all. The inserted fmtp line is placed immediately after the rtpmap line, which is correct per SDP ordering conventions, but the overall approach should be validated against the target SFU.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Summary
Improve initial video quality on constrained networks by adaptively setting x-google-start-bitrate based on detected network conditions.
Problem
On constrained networks, initial video quality is poor and takes a long time (30+ seconds) to stabilize due to several issues:
Solution
- Total connection time > 5 seconds (slow ICE)
- Reconnection triggered (network instability indicator)
- Full restart initiated
Changes
How to Test
Manual testing on constrained network:
- Logs should show Applying x-google-start-bitrate=300 kbps after reconnection/restart
- Video should stabilize faster compared to before (seconds vs 30+ seconds)
- Resolution starts at 480p or lower and can ramp up if bandwidth improves
Expected log sequence on poor network:
Reconnection triggered: setting constrained network hint to 300 kbps
Full restart: creating new session with network hint 300 kbps
Setting network hint to 300 kbps on publisher_pc (covers v0 fallback path)
Applying x-google-start-bitrate=300 kbps (ultimate_bps=Some(5000000), network_hint=Some(300))
Good network (no throttling):
Breaking Changes
None. This is an additive change that improves behavior on constrained networks without affecting good network scenarios.
Compatibility