From 022ede651f069211b43e9cededd80757ed435ae5 Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Tue, 11 Aug 2026 22:01:57 +0200 Subject: [PATCH] fix(pf-capture): the truncated first attempt no longer latches the sticky downgrades MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The pipeline retry loop deliberately shortens its first attempt's first-frame wait to 2.5s so a stream bound during a gamescope re-init fails over quickly. But the portal capturer's timeout diagnosis treated EVERY expiry as a verdict: it latched whichever offer it implicated — HDR capture off for the source, the raw-dmabuf offer off, the EGL→CUDA offer off — process-wide and permanently, when the attempt was truncated by design and a gamescope cold start routinely delivers nothing inside that window while accepting every offer a few seconds later (observed on .41: pid 1962 hit the expiry at connect and every later session in that process ran silently degraded). This is bug #6 from the pf-capture sweep, verified then and unfixed until now. The truncated attempt is now declared PROVISIONAL end to end: a new `Capturer::next_frame_within_provisional` (default: delegates) lets the retry loop say "this budget is the schedule, not a verdict", and the portal capturer's timeout classification — split out as the pure `classify_first_frame_timeout` + `timeout_convicts`, with tests — names the same suspect in the error text but latches nothing unless the expired budget was full-length. --- crates/pf-capture/src/lib.rs | 15 + crates/pf-capture/src/linux/mod.rs | 316 ++++++++++++++++----- crates/punktfunk-host/src/native/stream.rs | 13 +- 3 files changed, 278 insertions(+), 66 deletions(-) diff --git a/crates/pf-capture/src/lib.rs b/crates/pf-capture/src/lib.rs index afc9e7b6..becd589e 100644 --- a/crates/pf-capture/src/lib.rs +++ b/crates/pf-capture/src/lib.rs @@ -43,6 +43,21 @@ pub trait Capturer: Send { self.next_frame() } + /// [`next_frame_within`](Self::next_frame_within), but the caller declares the budget + /// PROVISIONAL: its expiry is the retry schedule firing (the deliberately truncated first + /// attempt), not a verdict on anything this capture offered. The portal backend must NOT + /// latch its sticky process-wide downgrades (HDR capture, either dmabuf-only offer) from a + /// provisional expiry — a gamescope cold start routinely outlives the short window while it + /// would have accepted every offer, and one latched race used to pin the whole host process + /// to SDR/CPU capture. The full-length attempt that follows delivers the honest verdict. + /// Backends that latch nothing from a timeout just delegate. + fn next_frame_within_provisional( + &mut self, + budget: std::time::Duration, + ) -> Result { + self.next_frame_within(budget) + } + /// Non-blocking: the freshest frame available since the last call, or `None` if none has /// arrived (the caller reuses its last frame to hold a steady output rate). The default /// just produces a frame each call — fine for instant synthetic sources; the portal diff --git a/crates/pf-capture/src/linux/mod.rs b/crates/pf-capture/src/linux/mod.rs index 60e43bd8..9458acee 100644 --- a/crates/pf-capture/src/linux/mod.rs +++ b/crates/pf-capture/src/linux/mod.rs @@ -533,7 +533,7 @@ fn spawn_pipewire( impl Capturer for PortalCapturer { fn next_frame(&mut self) -> Result { - self.frame_within(Duration::from_secs(10)) + self.frame_within(Duration::from_secs(10), TimeoutVerdict::Conclusive) } fn cursor(&mut self) -> Option { @@ -563,7 +563,13 @@ impl Capturer for PortalCapturer { } fn next_frame_within(&mut self, budget: Duration) -> Result { - self.frame_within(budget) + self.frame_within(budget, TimeoutVerdict::Conclusive) + } + + fn next_frame_within_provisional(&mut self, budget: Duration) -> Result { + // The retry loop's truncated first attempt: its expiry re-runs the schedule, it does not + // convict an offer — see `TimeoutVerdict` and the latch arms in `next_frame_timed_out`. + self.frame_within(budget, TimeoutVerdict::Provisional) } fn supports_arrival_wait(&self) -> bool { @@ -699,12 +705,73 @@ impl Capturer for PortalCapturer { } } +/// Whether an expired first-frame budget is allowed to CONVICT an offer. The retry loop's +/// deliberately truncated first attempt passes `Provisional`: its expiry means the schedule +/// moved on, not that the compositor refused anything — a gamescope cold start regularly needs +/// longer than that window to accept every offer it would have accepted. Latching from it pinned +/// the whole host process to SDR + CPU capture off a race the attempt lost by design; only a +/// full-length wait carries a verdict. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum TimeoutVerdict { + Conclusive, + Provisional, +} + +/// Which offer a first-frame timeout implicates — the diagnosis behind +/// [`PortalCapturer::next_frame_timed_out`], split out pure so the latch policy is testable. +/// Mirrors the negotiation state exactly: a negotiated format clears every offer (the compositor +/// accepted, it just produced nothing), and a forced `PUNKTFUNK_ZEROCOPY=1` keeps both dmabuf +/// arms erroring loudly instead of implicating them (the operator asked for exactly that path). +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum TimeoutOffer { + /// Format negotiated; no offer implicated — the compositor produced no buffers. + NoBuffers, + /// The 10-bit PQ/BT.2020 (HDR) dmabuf offer was never accepted. + Hdr, + /// The dmabuf-only raw-passthrough offer was never accepted. + RawDmabuf, + /// The dmabuf-only EGL→CUDA offer was never accepted. + GpuDmabuf, + /// Nothing negotiated and no offer implicated — format/modifier mismatch. + NoFormat, +} + +fn classify_first_frame_timeout( + negotiated: bool, + hdr_offer: bool, + vaapi_dmabuf: bool, + gpu_dmabuf_offer: bool, + zerocopy_forced: bool, +) -> TimeoutOffer { + if negotiated { + TimeoutOffer::NoBuffers + } else if hdr_offer { + TimeoutOffer::Hdr + } else if vaapi_dmabuf && !zerocopy_forced { + TimeoutOffer::RawDmabuf + } else if gpu_dmabuf_offer && !zerocopy_forced { + TimeoutOffer::GpuDmabuf + } else { + TimeoutOffer::NoFormat + } +} + +/// The latch policy: only a conclusive expiry of an offer-implicating timeout fires the offer's +/// sticky process-wide downgrade. +fn timeout_convicts(offer: TimeoutOffer, verdict: TimeoutVerdict) -> bool { + verdict == TimeoutVerdict::Conclusive + && matches!( + offer, + TimeoutOffer::Hdr | TimeoutOffer::RawDmabuf | TimeoutOffer::GpuDmabuf + ) +} + impl PortalCapturer { /// The blocking first-frame wait behind [`Capturer::next_frame`] / /// [`Capturer::next_frame_within`]. First frame can lag behind format negotiation; later /// frames arrive at ~fps. Wait in short slices so a GPU-import poison (worker death) fails /// the capture within ~0.5 s instead of sitting out the full first-frame budget. - fn frame_within(&mut self, budget: Duration) -> Result { + fn frame_within(&mut self, budget: Duration, verdict: TimeoutVerdict) -> Result { let deadline = std::time::Instant::now() + budget; loop { if self.signals.broken.load(Ordering::Relaxed) { @@ -730,7 +797,7 @@ impl PortalCapturer { if let Some(f) = self.take_frame() { return Ok(f); } - return self.next_frame_timed_out(e, budget); + return self.next_frame_timed_out(e, budget, verdict); } } } @@ -752,83 +819,118 @@ impl PortalCapturer { } /// The [`frame_within`](Self::frame_within) budget expired (or the thread ended) — turn it - /// into the diagnosis-bearing error. Split out of the slicing loop above; behavior unchanged. + /// into the diagnosis-bearing error, and fire the offer's sticky downgrade latch when — and + /// only when — the expiry convicts the offer (see [`timeout_convicts`]). fn next_frame_timed_out( &self, err: RecvTimeoutError, budget: Duration, + verdict: TimeoutVerdict, ) -> Result { let within = budget.as_secs_f32(); match err { RecvTimeoutError::Timeout => { - // Split the two black-screen root causes apart so the operator gets a cause, not - // just a symptom: did the format negotiate (compositor produced no buffers) or - // not (no acceptable format / node never emitted a param)? - if self.signals.negotiated.load(Ordering::Relaxed) { - Err(anyhow!( + let offer = classify_first_frame_timeout( + self.signals.negotiated.load(Ordering::Relaxed), + self.hdr_offer, + self.vaapi_dmabuf, + self.signals.gpu_dmabuf_offer.load(Ordering::Relaxed), + pf_zerocopy::zerocopy_forced(), + ); + let convicted = timeout_convicts(offer, verdict); + // A provisional expiry names the same suspect but hands down no sentence — the + // full-length retry that follows is the one whose timeout latches. + let sentence = if convicted { + "" // each arm below states its own downgrade + } else { + " (short first-attempt window — nothing is latched; the full-length retry \ + decides)" + }; + match offer { + TimeoutOffer::NoBuffers => Err(anyhow!( "no PipeWire frame within {within}s (node {}): format negotiated but no \ buffers arrived — the compositor produced no frames (virtual output \ idle/unmapped, capture never started, or a stream bound during a \ compositor (re)start that will never deliver — a reconnect fixes that)", self.node_id - )) - } else if self.hdr_offer { - // The HDR (10-bit PQ dmabuf) offer was never accepted — the monitor left HDR - // mode between the probe and the negotiation, the compositor pre-dates the - // GNOME 50 HDR formats, or its allocator can't do LINEAR for XR30/XB30. - // Latch the process-wide SDR downgrade so the next session (Moonlight - // auto-reconnects) negotiates SDR instead of re-running this same timeout. - super::note_hdr_capture_failed(self.hdr_source); - Err(anyhow!( - "no PipeWire frame within {within}s (node {}): the compositor never \ - accepted the HDR (10-bit PQ/BT.2020 dmabuf) offer — is the mirrored \ - monitor in HDR mode on GNOME 50+? Downgrading this host to SDR capture; \ - reconnect to stream SDR", - self.node_id - )) - } else if self.vaapi_dmabuf && !pf_zerocopy::zerocopy_forced() { - // The dmabuf-only raw-passthrough offer was never accepted. Latch the - // downgrade so the encode loop's pipeline rebuild retries on the CPU offer - // instead of failing this same negotiation forever. The latch is SCOPED to the - // raw-passthrough decision: it used to be `note_vaapi_dmabuf_failed`, which fed - // `pf_zerocopy::enabled()` and therefore dropped every later session on this - // host — NVENC's EGL→CUDA path included — to CPU capture. Since this offer is - // also the PyroWave one (any vendor), a single PyroWave negotiation timeout was - // enough to do that. - pf_zerocopy::note_raw_dmabuf_negotiation_failed(); - Err(anyhow!( - "no PipeWire frame within {within}s (node {}): the compositor never \ - accepted the dmabuf-only offer (raw-dmabuf passthrough) — downgrading \ - THIS path to CPU capture for the rest of the process; the pipeline \ - rebuild will renegotiate without dmabuf", - self.node_id - )) - } else if self.signals.gpu_dmabuf_offer.load(Ordering::Relaxed) - && !pf_zerocopy::zerocopy_forced() - { - // The EGL→CUDA dmabuf-only offer was never accepted — the twin of the raw- - // passthrough arm above (the offer the thread ACTUALLY made, per the signal - // it set — see `CaptureSignals::gpu_dmabuf_offer`). One timeout is conclusive: - // a compositor that allocates none of the importer's modifiers refuses them - // identically on every retry, so latch the offer off and let the pipeline - // rebuild renegotiate the CPU path instead of re-running this same 10 s - // timeout on every reconnect. A forced PUNKTFUNK_ZEROCOPY=1 keeps erroring - // loudly instead (same rule as the raw arm). - pf_zerocopy::note_gpu_dmabuf_negotiation_failed(); - Err(anyhow!( - "no PipeWire frame within {within}s (node {}): the compositor never \ - accepted the dmabuf-only offer (EGL→CUDA GPU import) — downgrading THIS \ - offer to the CPU path for the rest of the process; the pipeline rebuild \ - will renegotiate without dmabuf", - self.node_id - )) - } else { - Err(anyhow!( + )), + TimeoutOffer::Hdr => { + // The HDR (10-bit PQ dmabuf) offer was never accepted — the monitor left HDR + // mode between the probe and the negotiation, the compositor pre-dates the + // GNOME 50 HDR formats, or its allocator can't do LINEAR for XR30/XB30. + // Latch the SDR downgrade for THIS source (`HdrSource`, not process-wide — one + // shared flag let either Linux HDR source disable the other) so the next session + // (Moonlight auto-reconnects) negotiates SDR instead of re-running this timeout. + if convicted { + super::note_hdr_capture_failed(self.hdr_source); + } + Err(anyhow!( + "no PipeWire frame within {within}s (node {}): the compositor never \ + accepted the HDR (10-bit PQ/BT.2020 dmabuf) offer — is the mirrored \ + monitor in HDR mode on GNOME 50+?{}", + self.node_id, + if convicted { + " Downgrading this host to SDR capture; reconnect to stream SDR" + } else { + sentence + } + )) + } + TimeoutOffer::RawDmabuf => { + // The dmabuf-only raw-passthrough offer was never accepted. Latch the + // downgrade so the encode loop's pipeline rebuild retries on the CPU offer + // instead of failing this same negotiation forever. The latch is SCOPED to the + // raw-passthrough decision: it used to be `note_vaapi_dmabuf_failed`, which fed + // `pf_zerocopy::enabled()` and therefore dropped every later session on this + // host — NVENC's EGL→CUDA path included — to CPU capture. Since this offer is + // also the PyroWave one (any vendor), a single PyroWave negotiation timeout was + // enough to do that. + if convicted { + pf_zerocopy::note_raw_dmabuf_negotiation_failed(); + } + Err(anyhow!( + "no PipeWire frame within {within}s (node {}): the compositor never \ + accepted the dmabuf-only offer (raw-dmabuf passthrough){}", + self.node_id, + if convicted { + " — downgrading THIS path to CPU capture for the rest of the \ + process; the pipeline rebuild will renegotiate without dmabuf" + } else { + sentence + } + )) + } + TimeoutOffer::GpuDmabuf => { + // The EGL→CUDA dmabuf-only offer was never accepted — the twin of the raw- + // passthrough arm above (the offer the thread ACTUALLY made, per the signal + // it set — see `CaptureSignals::gpu_dmabuf_offer`). One FULL-LENGTH timeout + // is conclusive: a compositor that allocates none of the importer's + // modifiers refuses them identically on every retry, so latch the offer off + // and let the pipeline rebuild renegotiate the CPU path instead of + // re-running this same 10 s timeout on every reconnect. A forced + // PUNKTFUNK_ZEROCOPY=1 keeps erroring loudly instead (same rule as the raw + // arm). + if convicted { + pf_zerocopy::note_gpu_dmabuf_negotiation_failed(); + } + Err(anyhow!( + "no PipeWire frame within {within}s (node {}): the compositor never \ + accepted the dmabuf-only offer (EGL→CUDA GPU import){}", + self.node_id, + if convicted { + " — downgrading THIS offer to the CPU path for the rest of the \ + process; the pipeline rebuild will renegotiate without dmabuf" + } else { + sentence + } + )) + } + TimeoutOffer::NoFormat => Err(anyhow!( "no PipeWire frame within {within}s (node {}): format negotiation never \ completed — the compositor offered no format this consumer accepts \ (pixel-format/modifier mismatch) or the node never emitted a Format param", self.node_id - )) + )), } } RecvTimeoutError::Disconnected => Err(anyhow!( @@ -874,3 +976,89 @@ mod pipewire; // unit-test without a compositor, which is the point. mod pw_cursor; mod pw_pods; + +#[cfg(test)] +mod first_frame_timeout_tests { + use super::{classify_first_frame_timeout, timeout_convicts, TimeoutOffer, TimeoutVerdict}; + + #[test] + fn a_provisional_expiry_convicts_no_offer_whatever_was_on_the_table() { + // The bug this pins down: the retry loop's truncated 2.5 s first attempt latched all + // three sticky process-wide downgrades as if the compositor had refused the offers — a + // gamescope HDR cold start then streamed SDR (and CPU-copied) for the process lifetime. + for offer in [ + TimeoutOffer::NoBuffers, + TimeoutOffer::Hdr, + TimeoutOffer::RawDmabuf, + TimeoutOffer::GpuDmabuf, + TimeoutOffer::NoFormat, + ] { + assert!( + !timeout_convicts(offer, TimeoutVerdict::Provisional), + "provisional expiry must not latch {offer:?}" + ); + } + } + + #[test] + fn a_conclusive_expiry_convicts_exactly_the_offer_bearing_diagnoses() { + assert!(timeout_convicts( + TimeoutOffer::Hdr, + TimeoutVerdict::Conclusive + )); + assert!(timeout_convicts( + TimeoutOffer::RawDmabuf, + TimeoutVerdict::Conclusive + )); + assert!(timeout_convicts( + TimeoutOffer::GpuDmabuf, + TimeoutVerdict::Conclusive + )); + // A negotiated-but-idle stream and a plain format mismatch implicate no offer — nothing + // to latch even on a full-length wait. + assert!(!timeout_convicts( + TimeoutOffer::NoBuffers, + TimeoutVerdict::Conclusive + )); + assert!(!timeout_convicts( + TimeoutOffer::NoFormat, + TimeoutVerdict::Conclusive + )); + } + + #[test] + fn classification_mirrors_the_negotiation_state_precedence() { + // A negotiated format clears every offer, whatever else was on the table. + assert_eq!( + classify_first_frame_timeout(true, true, true, true, false), + TimeoutOffer::NoBuffers + ); + // The HDR offer outranks the dmabuf arms (it is the offer that failed to negotiate). + assert_eq!( + classify_first_frame_timeout(false, true, true, true, false), + TimeoutOffer::Hdr + ); + assert_eq!( + classify_first_frame_timeout(false, false, true, true, false), + TimeoutOffer::RawDmabuf + ); + assert_eq!( + classify_first_frame_timeout(false, false, false, true, false), + TimeoutOffer::GpuDmabuf + ); + assert_eq!( + classify_first_frame_timeout(false, false, false, false, false), + TimeoutOffer::NoFormat + ); + } + + #[test] + fn a_forced_zerocopy_keeps_both_dmabuf_arms_erroring_loudly_instead_of_implicated() { + // PUNKTFUNK_ZEROCOPY=1 is the operator insisting on the path — the timeout falls through + // to the generic diagnosis (and so never latches), exactly as the old else-if chain did. + assert_eq!( + classify_first_frame_timeout(false, false, true, true, true), + TimeoutOffer::NoFormat + ); + } +} diff --git a/crates/punktfunk-host/src/native/stream.rs b/crates/punktfunk-host/src/native/stream.rs index 31bd0c1e..149bc923 100644 --- a/crates/punktfunk-host/src/native/stream.rs +++ b/crates/punktfunk-host/src/native/stream.rs @@ -4129,7 +4129,10 @@ fn build_pipeline_with_retry( // SteamOS: every gamescope bring-up burned the full 10 s on attempt 1, then attempt 2 got // frames instantly → 17 s bring-ups). Healthy compositors deliver the first frame well inside // this window (KWin ~0.3 s), and the genuinely-slow cold start above still gets the patient - // 10 s window on every later attempt. + // 10 s window on every later attempt. The truncated attempt is PROVISIONAL end to end: its + // expiry must not latch the capturer's sticky downgrades (see + // `Capturer::next_frame_within_provisional`) — only the full-length attempts hand down + // negotiation verdicts. const FIRST_ATTEMPT_FRAME_BUDGET: std::time::Duration = std::time::Duration::from_millis(2500); let mut backoff = std::time::Duration::from_millis(500); for attempt in 1..=max_attempts { @@ -4484,7 +4487,13 @@ fn build_pipeline( } capturer.set_active(true); let first = match first_frame_budget { - Some(budget) => capturer.next_frame_within(budget), + // Provisional: this is the retry loop's deliberately truncated first attempt, and its + // expiry is the schedule firing, not a negotiation verdict — the capturer must not latch + // its sticky process-wide downgrades (HDR capture, the dmabuf-only offers) from it. A + // gamescope cold start regularly outlives this window and then accepts every offer on the + // full-length attempt that follows (observed on .41: one truncated expiry pinned the whole + // host process to SDR + CPU capture). + Some(budget) => capturer.next_frame_within_provisional(budget), None => capturer.next_frame(), }; let frame = match first.context("first frame") {