From fe4af1761e9f2e618758a89c8504c0417eecf2e5 Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Sun, 19 Jul 2026 23:16:25 +0200 Subject: [PATCH] fix(capture): stop stranding a PipeWire buffer on a caught panic; invalidate the PyroWave CSC on ring recreate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two medium findings from the round-1 sweep. - The `.process` callback dequeued a buffer INSIDE its `catch_unwind`, and every requeue site was inside too. `newest` is a raw pointer with no Drop, so any caught panic (update_cursor_meta / consume_frame) unwound past all three requeues and permanently stranded that buffer. Once the stream's fixed pool drained, `dequeue_raw_buffer` returned null every call and capture silently wedged while still reporting negotiated/active — defeating the very catch_unwind that was meant to keep a panic survivable. The drain loop now runs OUTSIDE the catch (dequeue/queue are non-panicking C FFI pointer ops) and `newest` is requeued exactly once after it, on every path: normal, corrupted-skip, or caught panic. - `recreate_ring` invalidated `video_conv` and `hdr_p010_conv` but not `pyro_conv`. That converter is mode-baked — BgraToYuvPlanes selects entirely different shaders and output formats for SDR (8-bit BT.709 → R8/R8G8) vs HDR (scRGB→PQ BT.2020 → R16/R16G16) — and `ensure_pyro_conv` only builds when None, so a display_hdr flip reused the stale SDR converter against a freshly HDR-formatted pyro ring, corrupting every frame. Reachable at the documented "Downgrade point D": a PyroWave session with client_10bit=true that opens on a box where HDR can't enable, then flips once the display comes up. Linux .21: pf-capture 1/0. Windows .173: `cargo check -p pf-capture` clean. Co-Authored-By: Claude Opus 4.8 --- crates/pf-capture/src/linux/mod.rs | 74 +++++++++++------------ crates/pf-capture/src/windows/idd_push.rs | 6 ++ 2 files changed, 43 insertions(+), 37 deletions(-) diff --git a/crates/pf-capture/src/linux/mod.rs b/crates/pf-capture/src/linux/mod.rs index 8e57d8a9..e5e56880 100644 --- a/crates/pf-capture/src/linux/mod.rs +++ b/crates/pf-capture/src/linux/mod.rs @@ -2191,36 +2191,37 @@ mod pipewire { } }) .process(|stream, ud| { - // PipeWire dispatches this from a C trampoline with no catch_unwind; a - // panic crossing that FFI boundary would abort the whole host. Contain it. + // Latest-frame-only (OBS pattern): Mutter delivers buffers in bursts and recycles its + // pool; an older queued buffer carries a STALE frame. Drain all queued buffers, requeue + // the older ones, keep only the newest. This dequeue/requeue runs OUTSIDE the + // `catch_unwind` below — they are non-panicking C FFI pointer ops, and `newest` is + // requeued exactly once AFTER the panic-containing region. Previously the whole thing was + // inside the catch, so a caught panic (in `update_cursor_meta`/`consume_frame`) stranded + // `newest` forever, permanently shrinking the stream's fixed pool until capture wedged. + // SAFETY: `stream` is the live stream PipeWire passes into this `.process` callback on the + // loop thread; `dequeue_raw_buffer` returns a stream-owned `*mut pw_buffer` or null + // (null-checked), single-threaded so no concurrent access. + let mut newest = unsafe { stream.dequeue_raw_buffer() }; + if newest.is_null() { + return; + } + let mut drained = 1u32; + loop { + // SAFETY: same stream/loop-thread contract; returns the next stream-owned buffer or null. + let next = unsafe { stream.dequeue_raw_buffer() }; + if next.is_null() { + break; + } + // SAFETY: `newest` was dequeued from this stream and not yet requeued; we immediately + // overwrite it, so the requeued pointer is never touched again. + unsafe { stream.queue_raw_buffer(newest) }; + newest = next; + drained += 1; + } + // PipeWire dispatches from a C trampoline with no catch_unwind; a panic crossing that FFI + // boundary would abort the whole host. Contain the inspect/consume work — the only Rust + // code here that can panic — and requeue `newest` unconditionally after it. let outcome = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { - // Latest-frame-only (OBS pattern): Mutter delivers buffers in bursts and - // recycles its pool; an older queued buffer carries a STALE frame. Drain all - // queued buffers, requeue the older ones, keep only the newest. - // SAFETY: `stream` is the live stream PipeWire passes into this `.process` callback on - // the loop thread, where `pw_stream_dequeue_buffer` is the documented call. It returns - // a `*mut pw_buffer` owned by the stream (or null when the queue is drained), - // null-checked before any use. The loop is single-threaded, so no concurrent access. - let mut newest = unsafe { stream.dequeue_raw_buffer() }; - if newest.is_null() { - return; - } - let mut drained = 1u32; - loop { - // SAFETY: same stream/loop-thread contract as the dequeue above; each call returns - // the next stream-owned `*mut pw_buffer` or null (null-checked before use). - let next = unsafe { stream.dequeue_raw_buffer() }; - if next.is_null() { - break; - } - // SAFETY: `newest` is a non-null `*mut pw_buffer` previously dequeued from this same - // stream and not yet requeued; `pw_stream_queue_buffer` hands ownership back to the - // stream. We immediately overwrite `newest = next`, so the requeued pointer is never - // touched again (no use-after-requeue). Loop thread, single-threaded. - unsafe { stream.queue_raw_buffer(newest) }; - newest = next; - drained += 1; - } // SAFETY: `newest` is the non-null buffer we still own (dequeued, not requeued); // `.buffer` is a `*mut spa_buffer` field libpipewire populated. This is a single field // load through a valid pointer — no mutation or aliasing. @@ -2299,19 +2300,18 @@ mod pipewire { "capture: skipped a stale CORRUPTED/cursor buffer (GNOME)" ); } - // SAFETY: `newest` is the non-null buffer we own (dequeued, never requeued on this - // skip path); hand it back to the stream exactly once and return without touching it - // again. Loop thread inside `.process`. - unsafe { stream.queue_raw_buffer(newest) }; + // Skip this stale/cursor buffer — `newest` is requeued unconditionally below. return; } consume_frame(ud, spa_buf); - // SAFETY: `consume_frame` has finished reading `spa_buf` (and the `datas` borrows derived - // from `newest`), so requeuing the owned `newest` exactly once here is sound — no - // use-after-requeue. Loop thread inside `.process`. - unsafe { stream.queue_raw_buffer(newest) }; })); + // Hand `newest` back to the stream exactly once, on EVERY path — normal, corrupted-skip, + // or a caught panic in the closure above. This single requeue is what keeps the fixed + // buffer pool from draining. + // SAFETY: all reads of `spa_buf`/`newest` (update_cursor_meta, consume_frame) completed + // inside the closure above; `newest` was dequeued from this stream and not yet requeued. + unsafe { stream.queue_raw_buffer(newest) }; if outcome.is_err() { // In the per-frame `.process` callback: a deterministic panic (e.g. a bad // format) would fire this every frame, so power-of-two throttle it — enough to diff --git a/crates/pf-capture/src/windows/idd_push.rs b/crates/pf-capture/src/windows/idd_push.rs index 7a37beec..32dbb6b2 100644 --- a/crates/pf-capture/src/windows/idd_push.rs +++ b/crates/pf-capture/src/windows/idd_push.rs @@ -1341,6 +1341,12 @@ impl IddPushCapturer { self.out_ring.clear(); // the output format changed → rebuild lazily at the new format self.video_conv = None; // converters are sized + HDR-specific → rebuild at the new mode self.hdr_p010_conv = None; + // The PyroWave CSC is mode-baked too (BgraToYuvPlanes picks different SDR vs HDR shaders + // and R8/R8G8 vs R16/R16G16 outputs). Without this, a display_hdr flip (Downgrade point D: + // client_10bit=true but HDR couldn't enable at open) reused the stale SDR converter against + // the freshly HDR-formatted pyro ring — every frame corrupted. `ensure_pyro_conv` only + // builds when None, so it must be reset here like its siblings. + self.pyro_conv = None; self.pyro_ring.clear(); // PyroWave two-plane ring is sized → rebuild at the new mode self.pyro_last = None; self.out_idx = 0;