diff --git a/breadcast-caststream-sys/src/facade.cc b/breadcast-caststream-sys/src/facade.cc index 1a84e64..c4eed93 100644 --- a/breadcast-caststream-sys/src/facade.cc +++ b/breadcast-caststream-sys/src/facade.cc @@ -80,6 +80,27 @@ struct CastStreamSender { std::atomic needs_key_frame{true}; std::atomic estimated_bandwidth_bps{kDefaultBandwidthEstimateBps}; + // Set (never cleared except by the one consuming read, see + // breadcast_caststream_sender_needs_key_frame below) whenever a frame gets + // silently dropped after already being encoded -- either + // Sender::EnqueueFrame() rejecting it (e.g. MAX_DURATION_IN_FLIGHT, the + // in-flight budget openscreen enforces) or the non-monotonic-capture-time + // guard below. Either way, vah264enc already encoded the *next* frame as a + // P-slice depending on the one that just got dropped -- the encoder has no + // idea the drop happened, since it happens downstream of encoding, at this + // FFI boundary. Left alone, that reference is now dangling: the receiver + // decodes it against whatever picture it last successfully received, + // producing a stuck or corrupted frame that only resolves at the next + // regularly-scheduled key frame (key-int-max=60 -- up to ~2s, longer still + // if that key frame is itself dropped the same way). Forcing a key frame + // on the very next enqueue turns "up to several seconds of corruption" + // into "one dropped frame, then a clean resync" -- a separate flag rather + // than reusing `needs_key_frame` directly because SchedulePoll's 100ms + // timer unconditionally overwrites that one with the Sender's own + // (unrelated) NeedsKeyFrame() reading, which would silently clobber this + // signal before frame_pump_loop ever observed it. + std::atomic frame_chain_broken{false}; + // The caller's own user_data + callbacks, as passed to `_create`. Not // called directly -- session.h/message_port_bridge.h are instead given // trampolines below (with `this` as their user_data) so this struct can @@ -282,6 +303,7 @@ int32_t breadcast_caststream_sender_enqueue_frame(CastStreamSender* sender, // here rather than passed through. if (sender->have_last_capture_time && capture_time_us <= sender->last_capture_time_us) { + sender->frame_chain_broken.store(true, std::memory_order_relaxed); return; } @@ -311,12 +333,15 @@ int32_t breadcast_caststream_sender_enqueue_frame(CastStreamSender* sender, ByteView(owned_data->data(), owned_data->size())); // EnqueueFrame()'s result (e.g. MAX_DURATION_IN_FLIGHT under backpressure) - // isn't propagated to the caller: by the time this runs, enqueue_frame() - // has already returned 0 synchronously (this call is posted, not - // immediate -- see facade.h's threading contract). Backpressure here just - // means this one frame is dropped; the encoder finds out indirectly via - // needs_key_frame()/estimated_bandwidth_bps() polling. - (void)video_sender->EnqueueFrame(frame); + // can't be propagated to enqueue_frame()'s caller synchronously -- by the + // time this runs, that call has already returned 0 (this call is posted, + // not immediate -- see facade.h's threading contract). What it *can* do + // is flag the drop so the next frame comes in clean -- see + // `frame_chain_broken`'s doc comment on why that matters here, not just + // for the encoder's bitrate. + if (video_sender->EnqueueFrame(frame) != Sender::OK) { + sender->frame_chain_broken.store(true, std::memory_order_relaxed); + } }); return 0; diff --git a/breadcast-core/src/caststream.rs b/breadcast-core/src/caststream.rs index da8fa48..a6c6017 100644 --- a/breadcast-core/src/caststream.rs +++ b/breadcast-core/src/caststream.rs @@ -55,12 +55,14 @@ impl Default for VideoParams { // a resolution other than what's actually sent is a real // protocol mismatch that plausibly explains a receiver decoder // corrupting/freezing rather than just looking soft. - width: 1920, - height: 1080, + // Reverted from a brief 1920x1080 experiment -- see + // `build_video_pipeline_for_streaming`'s doc comment for why + // (the freeze wasn't a resolution/bandwidth problem at all). + width: 1280, + height: 720, // Kept equal to `breadcastd::cast_mirror::MAX_BITRATE_KBPS * // 1000` -- see that constant's doc comment for why 8 Mbps - // (this struct's previous value) isn't used here even though - // 1080p can look better with more headroom: real hardware + // (this struct's previous value) isn't used here: real hardware // testing showed the AIMD probe pinning to whatever this // ceiling is for the entire session once the estimator reports // (unreliably) that there's room, and 8 Mbps sustained was more diff --git a/breadcast-core/src/pipeline/mod.rs b/breadcast-core/src/pipeline/mod.rs index 110fa38..b5466ab 100644 --- a/breadcast-core/src/pipeline/mod.rs +++ b/breadcast-core/src/pipeline/mod.rs @@ -150,19 +150,20 @@ pub fn build_video_pipeline_for_streaming( ) -> Result<(gst::Pipeline, gst_app::AppSink, gst::Element)> { gst::init().context("failed to initialize GStreamer")?; - // 1920x1080@30 Main profile -- raised from the earlier 1280x720 - // baseline (kept in `build_video_pipeline`'s HLS path, which targets a - // different, less capable receiver -- see its doc comment) once real - // hardware testing showed the actual bottleneck on the *previous* - // network wasn't resolution but the encoder being driven well past - // what that link/receiver could sustain (see `MAX_BITRATE_KBPS` in - // `breadcastd::cast_mirror`). Must stay equal to `VideoParams::default` - // in `caststream.rs` -- the OFFER's advertised resolution and what's - // actually encoded disagreeing is a protocol-level mismatch, not just - // soft video (see that struct's doc comment for what that caused). + // 1280x720@30 Main profile. Briefly raised to 1080p, then reverted here: + // a near-instant freeze *on a faster network* turned out to have nothing + // to do with resolution or bandwidth at all -- see `frame_chain_broken` + // in `breadcast-caststream-sys/src/facade.cc` for the actual bug (the + // FFI silently drops frames under openscreen's in-flight budget and lets + // the encoder's reference chain corrupt as a result). 1080p roughly + // tripled the per-frame packet count, which made that bug's real trigger + // -- exceeding the in-flight window -- worse, not the resolution itself. + // Reverted alongside fixing that bug rather than keeping both variables + // in motion at once; revisit once `frame_chain_broken` on its own is + // confirmed to have fixed the freeze at 720p. let pipeline_str = "pipewiresrc path=%VIDEO_NODE_ID% do-timestamp=true ! \ videoconvert ! videoscale ! videorate ! \ - video/x-raw,format=NV12,width=1920,height=1080,framerate=30/1 ! \ + video/x-raw,format=NV12,width=1280,height=720,framerate=30/1 ! \ vah264enc name=venc bitrate=4000 key-int-max=60 rate-control=cbr ! \ video/x-h264,profile=main ! \ h264parse name=h264parse config-interval=-1 ! \ diff --git a/breadcastd/src/cast_mirror.rs b/breadcastd/src/cast_mirror.rs index f251067..22fcb15 100644 --- a/breadcastd/src/cast_mirror.rs +++ b/breadcastd/src/cast_mirror.rs @@ -29,7 +29,7 @@ use crate::daemon::DaemonCommand; /// `vah264enc` with, since [`bitrate_control_step`] treats it as the value /// already in effect at t=0. const INITIAL_BITRATE_KBPS: u32 = 4000; -/// Never encode below this. 1080p30 below roughly 1.5 Mbps is a wall of +/// Never encode below this. 720p30 below roughly 1.5 Mbps is a wall of /// blocking artifacts -- if the link genuinely can't carry that, dropping /// frames is a better failure mode than shipping unwatchable video. const MIN_BITRATE_KBPS: u32 = 1500; @@ -41,10 +41,11 @@ const MIN_BITRATE_KBPS: u32 = 1500; /// (the estimator it trusts read a suspiciously flat ~20 Mbps almost the /// whole time), and 8 Mbps sustained was more than the previous /// network+receiver could actually hold without repeated multi-second -/// freezes. 6 Mbps is a solid target for 1080p30 on its own merits, not -/// just a defensive number -- revisit upward only with real evidence this -/// specific link+receiver can sustain more, not just because the estimator -/// claims there's headroom. +/// freezes -- though the deeper cause of those freezes turned out to be +/// `frame_chain_broken` in `facade.cc`, not bitrate on its own. 6 Mbps is +/// still a very generous ceiling for 720p30; revisit only with real +/// evidence this specific link+receiver can sustain more, not just because +/// the estimator claims there's headroom. const MAX_BITRATE_KBPS: u32 = 6000; pub struct CastMirrorSession {