fix(webrtc): PLI rate limiting + bitrate cap + VBV units (closes #23)
Root causes (3, discovered via Oracle review + x264 source verification):
A. PLI storm: WebRtcInner.force_keyframe_to_encode is a single bool with
no rate limit. Client PLI storms (5 PLIs in 845ms observed) produced
5 back-to-back IDRs (~1.5MB burst), swamping the network.
B. Bitrate runaway: BWE feedback had no upper bound. Observed bitrate
escalating from 5 Mbps to 9.9 Mbps in 12 seconds, all applied to
encoder. Combined with A, made each IDR grow 4-5x.
C. VBV effectively disabled (NEW finding not in original issue):
x264's vbv-maxrate and vbv-bufsize x264opts expect kbit/s and kbit
(confirmed via x264 source ratecontrol.c:658-661 which multiplies by
1000 at use site). The code passed bps, making VBV interpret 5.5 Mbps
as 5.5 Gbps (clipped to 2 Gbps). Buffer was 173MB instead of 172KB,
so VBV never constrained anything. This is why IDRs could balloon to
336KB after runtime bitrate increases.
Fixes (3, all in this commit per Oracle review):
Fix 0 (avhw.rs): divide bitrate by 1000 when formatting x264opts string.
Updated existing vbv_x264opts_format and vbv_bufsize_is_quarter_of_maxrate
tests to assert correct kbit/s values. Old tests passed but asserted
wrong values - classic 'tests covered the wrong implementation'.
Fix 1 (webrtc.rs): split keyframe trigger into two paths.
- set_need_keyframe() (internal: connect, resolution change) remains
unthrottled but updates last_forced_keyframe_at timestamp.
- request_keyframe_from_viewer() (external PLI) rate-limited to
Duration::from_secs(1), checked against last_forced_keyframe_at.
- Key insight from Oracle: track ALL keyframe production time, not
just PLI time, to prevent 'connect -> immediate PLI -> duplicate IDR'.
Fix 2 (args.rs + state_portal.rs + avhw.rs):
- New --max-bitrate CLI flag, default 8 Mbps.
- Primary clamp in webrtc_thread_loop (policy layer): clamp BWE via
variable shadowing so all downstream code (bitrate_tx, resolution
adaptation) uses clamped value.
- Defensive guardrail in encode_cpu_frame UpdateBitrate handler:
50 Mbps hard ceiling in case future callers bypass policy layer.
- Per Oracle: flat 8 Mbps default, no auto-scaling
(max(8M, 2*initial) would have failed the observed case).
Verification (82.1s session, 34.7 fps avg vs previous 18.2):
PLI storm absorption:
- 15 PLIs received from viewer
- 10 PLIs throttled (67%)
- 6 IDRs produced total (1 connect + 5 honored)
- 3 distinct PLI storms (625ms, 640ms, 858ms duration) all absorbed
VBV constraint working:
- First IDR: 65KB (was 67KB)
- Largest IDR: 169KB (was 336KB)
- All IDRs under VBV buffer bound of 172KB
- No more runaway IDR growth
Bitrate cap working:
- 5 bitrate updates applied (was 8)
- Peak bitrate: 7.4 Mbps (was 9.9 Mbps)
- 52952 BWE readings clamped (>8 Mbps filtered out)
Overall quality:
- Frame rate: 34.7 fps (was 18.2, 1.9x improvement, exceeds 30 fps target)
- Average bitrate: 4451 kb/s (was 5616, lower and more stable)
Out of scope (deferred to future work):
- Runtime VBV reconfiguration on bitrate change (Fix 3): encoder
recreation is expensive, observe whether VBV mismatch becomes a
quality issue after cap is in place
- Asymmetric BWE filter (Fix 4): rise=15%/fall=5% thresholds; nice
tuning but cap is sufficient for now
- Compositor stalls (#15): still causes some stutter at session end
but no longer compounds into latency
Tests:
- cargo test: 91 passed + 3 passed + 0 failed
- vbv_x264opts_format and vbv_bufsize_is_quarter_of_maxrate updated
and passing with new kbit/s assertions
- SAFETY comments preserved verbatim
- cargo build --release: clean, no new warnings
This commit is contained in:
dailz
@@ -31,6 +31,13 @@ pub struct Args {
|
||||
#[arg(long)]
|
||||
pub bitrate: Option<u64>,
|
||||
|
||||
/// Maximum bitrate in bps for WebRTC mode. Caps BWE-driven escalation to
|
||||
/// prevent large IDR bursts from swamping the network. Default 8 Mbps covers
|
||||
/// 1080p30/1440p30 H.264 acceptably. Does NOT affect MP4 (--output) mode.
|
||||
/// See issue #23.
|
||||
#[arg(long, default_value = "8000000")]
|
||||
pub max_bitrate: u64,
|
||||
|
||||
/// Group of Pictures (GOP) size
|
||||
#[arg(long)]
|
||||
pub gop_size: Option<u32>,
|
||||
|
||||
+25
-11
@@ -1142,6 +1142,11 @@ impl SwEncEncode {
|
||||
while let Ok(cmd) = self.bitrate_rx.try_recv() {
|
||||
match cmd {
|
||||
BitrateCommand::UpdateBitrate { target_bps } => {
|
||||
// #23 defensive guardrail: clamp to reasonable max even if policy layer
|
||||
// is bypassed. 50 Mbps is a hard ceiling; primary cap is enforced in
|
||||
// state_portal.rs webrtc_thread_loop via --max-bitrate flag.
|
||||
const ENCODER_BITRATE_HARD_CAP: u64 = 50_000_000;
|
||||
let target_bps = target_bps.min(ENCODER_BITRATE_HARD_CAP);
|
||||
tracing::info!(target_bps, "updating encoder bitrate from BWE feedback");
|
||||
self.bitrate = target_bps;
|
||||
// SAFETY: enc_video is an opened AVCodecContext exclusively owned by &mut self.
|
||||
@@ -1839,10 +1844,15 @@ fn create_software_h264_encoder(
|
||||
let val = CString::new("1").unwrap();
|
||||
ffi::av_opt_set((*enc.as_mut_ptr()).priv_data, key.as_ptr(), val.as_ptr(), 0);
|
||||
let key = CString::new("x264opts").unwrap();
|
||||
let vbv_maxrate = bitrate;
|
||||
let vbv_bufsize = bitrate / 4;
|
||||
// x264's vbv-maxrate unit is kbit/s and vbv-bufsize is kbit (NOT bps).
|
||||
// Confirmed via x264 source encoder/ratecontrol.c:658-661 which multiplies
|
||||
// these values by 1000 to convert kbit → bit at use site. Passing bps makes
|
||||
// VBV effectively unbounded (5.5 Mbps becomes 5.5 Gbps, clipped to 2 Gbps).
|
||||
// See https://github.com/mirror/x264/blob/c24e06c2e184345ceb33eb20a15d1024d9fd3497/encoder/ratecontrol.c#L658-L661
|
||||
let vbv_maxrate_kbps = bitrate / 1000;
|
||||
let vbv_bufsize_kbps = (bitrate / 4) / 1000;
|
||||
let val = CString::new(format!(
|
||||
"repeat_headers=1:vbv-maxrate={vbv_maxrate}:vbv-bufsize={vbv_bufsize}"
|
||||
"repeat_headers=1:vbv-maxrate={vbv_maxrate_kbps}:vbv-bufsize={vbv_bufsize_kbps}"
|
||||
))
|
||||
.unwrap();
|
||||
ffi::av_opt_set((*enc.as_mut_ptr()).priv_data, key.as_ptr(), val.as_ptr(), 0);
|
||||
@@ -1971,19 +1981,23 @@ mod tests {
|
||||
#[test]
|
||||
fn vbv_x264opts_format() {
|
||||
let bitrate: u64 = 5_000_000;
|
||||
let vbv_maxrate = bitrate;
|
||||
let vbv_bufsize = bitrate / 4;
|
||||
let opts = format!("repeat_headers=1:vbv-maxrate={vbv_maxrate}:vbv-bufsize={vbv_bufsize}");
|
||||
assert!(opts.contains("vbv-maxrate=5000000"));
|
||||
assert!(opts.contains("vbv-bufsize=1250000"));
|
||||
// x264 expects kbit/s and kbit, not bps
|
||||
let vbv_maxrate_kbps = bitrate / 1000;
|
||||
let vbv_bufsize_kbps = (bitrate / 4) / 1000;
|
||||
let opts = format!("repeat_headers=1:vbv-maxrate={vbv_maxrate_kbps}:vbv-bufsize={vbv_bufsize_kbps}");
|
||||
assert_eq!(vbv_maxrate_kbps, 5000);
|
||||
assert_eq!(vbv_bufsize_kbps, 1250);
|
||||
assert!(opts.contains("vbv-maxrate=5000"));
|
||||
assert!(opts.contains("vbv-bufsize=1250"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn vbv_bufsize_is_quarter_of_maxrate() {
|
||||
for bitrate in [1_000_000, 5_000_000, 10_000_000] {
|
||||
let maxrate = bitrate;
|
||||
let bufsize = bitrate / 4;
|
||||
assert_eq!(bufsize * 4, maxrate, "bufsize should be maxrate/4");
|
||||
// x264 expects kbit/s and kbit; both scaled by /1000, ratio preserved
|
||||
let maxrate_kbps = bitrate / 1000;
|
||||
let bufsize_kbps = (bitrate / 4) / 1000;
|
||||
assert_eq!(bufsize_kbps * 4, maxrate_kbps, "bufsize should be maxrate/4");
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -185,6 +185,7 @@ mod tests {
|
||||
hw_accel: "vaapi".to_string(),
|
||||
drm_device: None,
|
||||
bitrate: None,
|
||||
max_bitrate: 8_000_000,
|
||||
gop_size: None,
|
||||
verbose: false,
|
||||
backend: backend.map(String::from),
|
||||
|
||||
@@ -110,6 +110,7 @@ fn main() -> Result<()> {
|
||||
hw_accel: "vaapi".to_string(),
|
||||
drm_device: None,
|
||||
bitrate: None,
|
||||
max_bitrate: 8_000_000,
|
||||
gop_size: None,
|
||||
verbose: false,
|
||||
backend: Some("portal".to_string()),
|
||||
|
||||
@@ -881,6 +881,7 @@ fn main() -> Result<()> {
|
||||
hw_accel: "vaapi".to_string(),
|
||||
drm_device: None,
|
||||
bitrate: None,
|
||||
max_bitrate: 8_000_000,
|
||||
gop_size: None,
|
||||
verbose: false,
|
||||
backend: Some("portal".to_string()),
|
||||
|
||||
@@ -281,6 +281,7 @@ impl StatePortal {
|
||||
.ok_or_else(|| anyhow::anyhow!("internal: webrtc_paused missing"))?
|
||||
.clone();
|
||||
let fps = self.args.fps;
|
||||
let max_bitrate = self.args.max_bitrate;
|
||||
let (sent_gap_tx, sent_gap_rx) = crossbeam_channel::bounded(64);
|
||||
let webrtc_handle = std::thread::Builder::new()
|
||||
.name("wl-webrtc-webrtc".into())
|
||||
@@ -291,6 +292,7 @@ impl StatePortal {
|
||||
fps,
|
||||
enc_width,
|
||||
enc_height,
|
||||
max_bitrate,
|
||||
paused,
|
||||
sent_gap_tx,
|
||||
bitrate_tx,
|
||||
@@ -715,6 +717,7 @@ fn webrtc_thread_loop(
|
||||
fps: u32,
|
||||
enc_width: u32,
|
||||
enc_height: u32,
|
||||
max_bitrate: u64,
|
||||
paused: Arc<AtomicBool>,
|
||||
sent_gap_tx: crossbeam_channel::Sender<f64>,
|
||||
bitrate_tx: crossbeam_channel::Sender<BitrateCommand>,
|
||||
@@ -754,6 +757,19 @@ fn webrtc_thread_loop(
|
||||
paused.store(now_paused, Ordering::Relaxed);
|
||||
|
||||
if let Some(bwe) = wrtc.get_bwe_estimate() {
|
||||
// #23: Cap BWE to prevent runaway bitrate escalation. Without this, BWE
|
||||
// estimates can rise to 10+ Mbps, causing IDR bursts and PLI storms.
|
||||
let effective_bwe = bwe.min(max_bitrate);
|
||||
if effective_bwe != bwe {
|
||||
tracing::debug!(
|
||||
bwe,
|
||||
effective_bwe,
|
||||
max_bitrate,
|
||||
"BWE exceeds --max-bitrate cap, clamping"
|
||||
);
|
||||
}
|
||||
let bwe = effective_bwe;
|
||||
|
||||
let should_send = match last_sent_bitrate {
|
||||
None => true,
|
||||
Some(last) => {
|
||||
@@ -987,6 +1003,7 @@ mod tests {
|
||||
hw_accel: "vaapi".to_string(),
|
||||
drm_device: Some("/dev/dri/renderD128".to_string()),
|
||||
bitrate: None,
|
||||
max_bitrate: 8_000_000,
|
||||
gop_size: None,
|
||||
verbose: false,
|
||||
backend: None,
|
||||
@@ -1011,6 +1028,7 @@ mod tests {
|
||||
hw_accel: "vaapi".to_string(),
|
||||
drm_device: None,
|
||||
bitrate: None,
|
||||
max_bitrate: 8_000_000,
|
||||
gop_size: None,
|
||||
verbose: false,
|
||||
backend: None,
|
||||
|
||||
+55
-7
@@ -1,7 +1,7 @@
|
||||
// WebRTC 传输模块 — 使用 str0m (Sans-IO) 将 H.264 编码帧推送到浏览器
|
||||
use std::io::{Read, Write};
|
||||
use std::net::{SocketAddr, TcpListener, UdpSocket};
|
||||
use std::time::Instant;
|
||||
use std::time::{Duration, Instant};
|
||||
|
||||
use anyhow::{bail, Result};
|
||||
use str0m::bwe::{Bitrate, BweKind};
|
||||
@@ -11,6 +11,12 @@ use str0m::media::{Frequency, MediaKind, MediaTime, Mid, Pt};
|
||||
use str0m::net::{Protocol, Receive};
|
||||
use str0m::{Candidate, Event, IceConnectionState, Input, Output, Rtc, RtcConfig};
|
||||
|
||||
/// Minimum interval between honored keyframe productions, regardless of source
|
||||
/// (PLI from viewer, connect event, resolution change). Prevents PLI storms
|
||||
/// from causing back-to-back IDRs that swamp the network with multi-hundred-KB
|
||||
/// bursts. See issue #23.
|
||||
const FORCED_KEYFRAME_MIN_INTERVAL: Duration = Duration::from_secs(1);
|
||||
|
||||
// ── 嵌入式 HTML 测试页面 ──────────────────────────────────────────────────
|
||||
|
||||
const HTML_PAGE: &str = r#"<!DOCTYPE html>
|
||||
@@ -201,6 +207,7 @@ struct WebRtcInner {
|
||||
connected: bool,
|
||||
need_keyframe: bool,
|
||||
force_keyframe_to_encode: bool,
|
||||
last_forced_keyframe_at: Option<Instant>,
|
||||
current_bwe_estimate: Option<Bitrate>,
|
||||
rtp_clock: u32,
|
||||
buf: Vec<u8>,
|
||||
@@ -351,10 +358,23 @@ impl WebRtcState {
|
||||
.and_then(|inner| inner.current_bwe_estimate.map(|b| b.as_u64()))
|
||||
}
|
||||
|
||||
/// Internal keyframe request (connect, resolution change). Always honored,
|
||||
/// but updates last_forced_keyframe_at so a subsequent viewer PLI in the next
|
||||
/// second is throttled.
|
||||
pub fn set_need_keyframe(&mut self) {
|
||||
if let Some(inner) = self.inner.as_mut() {
|
||||
inner.need_keyframe = true;
|
||||
inner.force_keyframe_to_encode = true;
|
||||
inner.set_need_keyframe();
|
||||
}
|
||||
}
|
||||
|
||||
/// External keyframe request from viewer (PLI/FIR via str0m
|
||||
/// `Event::KeyframeRequest`). Rate-limited to FORCED_KEYFRAME_MIN_INTERVAL
|
||||
/// to prevent PLI storms from swamping the network with IDR bursts.
|
||||
/// See issue #23.
|
||||
#[allow(dead_code)]
|
||||
pub fn request_keyframe_from_viewer(&mut self) {
|
||||
if let Some(inner) = self.inner.as_mut() {
|
||||
inner.request_keyframe_from_viewer();
|
||||
}
|
||||
}
|
||||
|
||||
@@ -442,6 +462,7 @@ impl WebRtcInner {
|
||||
connected: false,
|
||||
need_keyframe: false,
|
||||
force_keyframe_to_encode: false,
|
||||
last_forced_keyframe_at: None,
|
||||
current_bwe_estimate: None,
|
||||
rtp_clock: 0,
|
||||
buf: vec![0u8; 65535],
|
||||
@@ -494,6 +515,35 @@ impl WebRtcInner {
|
||||
}
|
||||
}
|
||||
|
||||
/// Unthrottled keyframe trigger. Always sets the keyframe flags and refreshes
|
||||
/// `last_forced_keyframe_at` so a follow-up viewer PLI within the next
|
||||
/// `FORCED_KEYFRAME_MIN_INTERVAL` is dropped.
|
||||
fn set_need_keyframe(&mut self) {
|
||||
self.need_keyframe = true;
|
||||
self.force_keyframe_to_encode = true;
|
||||
self.last_forced_keyframe_at = Some(Instant::now());
|
||||
}
|
||||
|
||||
/// Throttled keyframe trigger used for viewer-originated PLI/FIR requests.
|
||||
/// Honored only if enough time has elapsed since the last forced keyframe.
|
||||
fn request_keyframe_from_viewer(&mut self) {
|
||||
let now = Instant::now();
|
||||
let should_honor = self
|
||||
.last_forced_keyframe_at
|
||||
.map_or(true, |last| now.duration_since(last) >= FORCED_KEYFRAME_MIN_INTERVAL);
|
||||
if should_honor {
|
||||
self.last_forced_keyframe_at = Some(now);
|
||||
self.need_keyframe = true;
|
||||
self.force_keyframe_to_encode = true;
|
||||
} else {
|
||||
tracing::warn!(
|
||||
"PLI throttled (last forced keyframe {:?} ago, min interval {:?})",
|
||||
self.last_forced_keyframe_at.map(|t| now.duration_since(t)),
|
||||
FORCED_KEYFRAME_MIN_INTERVAL
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
fn poll_rtc(&mut self) -> Result<bool> {
|
||||
loop {
|
||||
match self.rtc.poll_output() {
|
||||
@@ -516,8 +566,7 @@ impl WebRtcInner {
|
||||
Event::Connected => {
|
||||
tracing::info!("WebRTC connected!");
|
||||
self.connected = true;
|
||||
self.need_keyframe = true;
|
||||
self.force_keyframe_to_encode = true;
|
||||
self.set_need_keyframe();
|
||||
self.discover_video_params();
|
||||
}
|
||||
Event::IceConnectionStateChange(IceConnectionState::Disconnected) => {
|
||||
@@ -539,8 +588,7 @@ impl WebRtcInner {
|
||||
}
|
||||
Event::KeyframeRequest(_) => {
|
||||
tracing::info!("received keyframe request from viewer");
|
||||
self.need_keyframe = true;
|
||||
self.force_keyframe_to_encode = true;
|
||||
self.request_keyframe_from_viewer();
|
||||
}
|
||||
Event::EgressBitrateEstimate(est) => {
|
||||
let bitrate = match est {
|
||||
|
||||
Reference in New Issue
Block a user