Fix silent frame-chain corruption on EnqueueFrame rejection; revert to 720p
Root cause (found by Opus 5 second-opinion review) of the freezing that survived every prior fix tonight: openscreen's Sender caps in-flight unacknowledged media at clamp(2*RTT, 66ms, 133ms) -- on a LAN that's pinned at the 66ms floor, about two frames at 30fps. When EnqueueFrame rejects a frame under that budget, facade.cc discarded the result (`(void)video_sender->EnqueueFrame(frame)`) and moved on. But vah264enc had already encoded the *next* frame as a P-slice depending on the one that just got silently dropped -- the encoder has no idea the drop happened, since it happens downstream of encoding, at this FFI boundary. The receiver sees an unbroken frame-ID sequence (nothing here told it otherwise) and decodes a P-slice against a reference picture it never received: a stuck/corrupted frame until the next regularly-scheduled key frame (up to ~2s, longer if that key frame is itself dropped the same way -- explains the 20+s outlier). Zero "receiver reported picture loss" lines across 195s of a visibly freezing session confirms the receiver genuinely never noticed anything was wrong, which a real decode-capability or packet-loss problem would have triggered. This also explains why moving to a faster network made it *worse*: the in-flight budget is RTT-derived, not bandwidth-derived, so more throughput doesn't raise the 66ms floor at all -- while 1080p tripled the per-frame packet count, increasing how often frames missed that window. Fixed the actual corruption: both drop paths in facade.cc (the EnqueueFrame rejection, and the pre-existing non-monotonic-capture-time guard) now set a `frame_chain_broken` flag, consumed once by breadcast_caststream_sender_needs_key_frame() so frame_pump_loop forces a key frame on the very next frame instead of chaining more P-slices onto a reference that no longer exists on the receiver. A separate flag from the existing `needs_key_frame` atomic because SchedulePoll's 100ms timer unconditionally overwrites that one with the Sender's own (unrelated) NeedsKeyFrame() reading, which would have silently clobbered this signal. Also reverted the Cast Streaming pipeline from tonight's 1080p experiment back to 1280x720 (build_video_pipeline_for_streaming + VideoParams::default(), which must agree -- a mismatch there is a separate protocol-level bug fixed earlier tonight). Not the root cause, but a real contributing factor per the packet-count reasoning above, and untangling it from the frame_chain_broken fix by changing both at once would make the next test ambiguous. Kept the 6000/1500 kbps bitrate range from earlier tonight, now actually paired with 720p for the first time. The deeper real fix -- raising openscreen's 66ms in-flight floor itself, which trades latency for headroom -- is out of scope for tonight; this targets the corruption mechanism (via the sanctioned, if awkward, needs_key_frame signal) without touching vendored openscreen constants.
This commit is contained in:
parent
a058482b39
commit
22a18eee1b
4 changed files with 55 additions and 26 deletions
|
|
@ -80,6 +80,27 @@ struct CastStreamSender {
|
||||||
std::atomic<bool> needs_key_frame{true};
|
std::atomic<bool> needs_key_frame{true};
|
||||||
std::atomic<int32_t> estimated_bandwidth_bps{kDefaultBandwidthEstimateBps};
|
std::atomic<int32_t> 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<bool> frame_chain_broken{false};
|
||||||
|
|
||||||
// The caller's own user_data + callbacks, as passed to `_create`. Not
|
// The caller's own user_data + callbacks, as passed to `_create`. Not
|
||||||
// called directly -- session.h/message_port_bridge.h are instead given
|
// called directly -- session.h/message_port_bridge.h are instead given
|
||||||
// trampolines below (with `this` as their user_data) so this struct can
|
// 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.
|
// here rather than passed through.
|
||||||
if (sender->have_last_capture_time &&
|
if (sender->have_last_capture_time &&
|
||||||
capture_time_us <= sender->last_capture_time_us) {
|
capture_time_us <= sender->last_capture_time_us) {
|
||||||
|
sender->frame_chain_broken.store(true, std::memory_order_relaxed);
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
@ -311,12 +333,15 @@ int32_t breadcast_caststream_sender_enqueue_frame(CastStreamSender* sender,
|
||||||
ByteView(owned_data->data(), owned_data->size()));
|
ByteView(owned_data->data(), owned_data->size()));
|
||||||
|
|
||||||
// EnqueueFrame()'s result (e.g. MAX_DURATION_IN_FLIGHT under backpressure)
|
// EnqueueFrame()'s result (e.g. MAX_DURATION_IN_FLIGHT under backpressure)
|
||||||
// isn't propagated to the caller: by the time this runs, enqueue_frame()
|
// can't be propagated to enqueue_frame()'s caller synchronously -- by the
|
||||||
// has already returned 0 synchronously (this call is posted, not
|
// time this runs, that call has already returned 0 (this call is posted,
|
||||||
// immediate -- see facade.h's threading contract). Backpressure here just
|
// not immediate -- see facade.h's threading contract). What it *can* do
|
||||||
// means this one frame is dropped; the encoder finds out indirectly via
|
// is flag the drop so the next frame comes in clean -- see
|
||||||
// needs_key_frame()/estimated_bandwidth_bps() polling.
|
// `frame_chain_broken`'s doc comment on why that matters here, not just
|
||||||
(void)video_sender->EnqueueFrame(frame);
|
// for the encoder's bitrate.
|
||||||
|
if (video_sender->EnqueueFrame(frame) != Sender::OK) {
|
||||||
|
sender->frame_chain_broken.store(true, std::memory_order_relaxed);
|
||||||
|
}
|
||||||
});
|
});
|
||||||
|
|
||||||
return 0;
|
return 0;
|
||||||
|
|
|
||||||
|
|
@ -55,12 +55,14 @@ impl Default for VideoParams {
|
||||||
// a resolution other than what's actually sent is a real
|
// a resolution other than what's actually sent is a real
|
||||||
// protocol mismatch that plausibly explains a receiver decoder
|
// protocol mismatch that plausibly explains a receiver decoder
|
||||||
// corrupting/freezing rather than just looking soft.
|
// corrupting/freezing rather than just looking soft.
|
||||||
width: 1920,
|
// Reverted from a brief 1920x1080 experiment -- see
|
||||||
height: 1080,
|
// `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 *
|
// Kept equal to `breadcastd::cast_mirror::MAX_BITRATE_KBPS *
|
||||||
// 1000` -- see that constant's doc comment for why 8 Mbps
|
// 1000` -- see that constant's doc comment for why 8 Mbps
|
||||||
// (this struct's previous value) isn't used here even though
|
// (this struct's previous value) isn't used here: real hardware
|
||||||
// 1080p can look better with more headroom: real hardware
|
|
||||||
// testing showed the AIMD probe pinning to whatever this
|
// testing showed the AIMD probe pinning to whatever this
|
||||||
// ceiling is for the entire session once the estimator reports
|
// ceiling is for the entire session once the estimator reports
|
||||||
// (unreliably) that there's room, and 8 Mbps sustained was more
|
// (unreliably) that there's room, and 8 Mbps sustained was more
|
||||||
|
|
|
||||||
|
|
@ -150,19 +150,20 @@ pub fn build_video_pipeline_for_streaming(
|
||||||
) -> Result<(gst::Pipeline, gst_app::AppSink, gst::Element)> {
|
) -> Result<(gst::Pipeline, gst_app::AppSink, gst::Element)> {
|
||||||
gst::init().context("failed to initialize GStreamer")?;
|
gst::init().context("failed to initialize GStreamer")?;
|
||||||
|
|
||||||
// 1920x1080@30 Main profile -- raised from the earlier 1280x720
|
// 1280x720@30 Main profile. Briefly raised to 1080p, then reverted here:
|
||||||
// baseline (kept in `build_video_pipeline`'s HLS path, which targets a
|
// a near-instant freeze *on a faster network* turned out to have nothing
|
||||||
// different, less capable receiver -- see its doc comment) once real
|
// to do with resolution or bandwidth at all -- see `frame_chain_broken`
|
||||||
// hardware testing showed the actual bottleneck on the *previous*
|
// in `breadcast-caststream-sys/src/facade.cc` for the actual bug (the
|
||||||
// network wasn't resolution but the encoder being driven well past
|
// FFI silently drops frames under openscreen's in-flight budget and lets
|
||||||
// what that link/receiver could sustain (see `MAX_BITRATE_KBPS` in
|
// the encoder's reference chain corrupt as a result). 1080p roughly
|
||||||
// `breadcastd::cast_mirror`). Must stay equal to `VideoParams::default`
|
// tripled the per-frame packet count, which made that bug's real trigger
|
||||||
// in `caststream.rs` -- the OFFER's advertised resolution and what's
|
// -- exceeding the in-flight window -- worse, not the resolution itself.
|
||||||
// actually encoded disagreeing is a protocol-level mismatch, not just
|
// Reverted alongside fixing that bug rather than keeping both variables
|
||||||
// soft video (see that struct's doc comment for what that caused).
|
// 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 ! \
|
let pipeline_str = "pipewiresrc path=%VIDEO_NODE_ID% do-timestamp=true ! \
|
||||||
videoconvert ! videoscale ! videorate ! \
|
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 ! \
|
vah264enc name=venc bitrate=4000 key-int-max=60 rate-control=cbr ! \
|
||||||
video/x-h264,profile=main ! \
|
video/x-h264,profile=main ! \
|
||||||
h264parse name=h264parse config-interval=-1 ! \
|
h264parse name=h264parse config-interval=-1 ! \
|
||||||
|
|
|
||||||
|
|
@ -29,7 +29,7 @@ use crate::daemon::DaemonCommand;
|
||||||
/// `vah264enc` with, since [`bitrate_control_step`] treats it as the value
|
/// `vah264enc` with, since [`bitrate_control_step`] treats it as the value
|
||||||
/// already in effect at t=0.
|
/// already in effect at t=0.
|
||||||
const INITIAL_BITRATE_KBPS: u32 = 4000;
|
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
|
/// blocking artifacts -- if the link genuinely can't carry that, dropping
|
||||||
/// frames is a better failure mode than shipping unwatchable video.
|
/// frames is a better failure mode than shipping unwatchable video.
|
||||||
const MIN_BITRATE_KBPS: u32 = 1500;
|
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
|
/// (the estimator it trusts read a suspiciously flat ~20 Mbps almost the
|
||||||
/// whole time), and 8 Mbps sustained was more than the previous
|
/// whole time), and 8 Mbps sustained was more than the previous
|
||||||
/// network+receiver could actually hold without repeated multi-second
|
/// network+receiver could actually hold without repeated multi-second
|
||||||
/// freezes. 6 Mbps is a solid target for 1080p30 on its own merits, not
|
/// freezes -- though the deeper cause of those freezes turned out to be
|
||||||
/// just a defensive number -- revisit upward only with real evidence this
|
/// `frame_chain_broken` in `facade.cc`, not bitrate on its own. 6 Mbps is
|
||||||
/// specific link+receiver can sustain more, not just because the estimator
|
/// still a very generous ceiling for 720p30; revisit only with real
|
||||||
/// claims there's headroom.
|
/// evidence this specific link+receiver can sustain more, not just because
|
||||||
|
/// the estimator claims there's headroom.
|
||||||
const MAX_BITRATE_KBPS: u32 = 6000;
|
const MAX_BITRATE_KBPS: u32 = 6000;
|
||||||
|
|
||||||
pub struct CastMirrorSession {
|
pub struct CastMirrorSession {
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue