fix(pf-capture): portal teardown, observable death, one negotiation resolver (sweep 2.1–2.3)
**2.1 (L1) — the portal thread leaked a live screencast session.** It parked on
`future::pending()` and `Drop` stopped only the PipeWire thread, so every dropped portal capturer
permanently leaked a thread + a 2-worker tokio runtime + a zbus connection — and because dropping
that connection is what ends the compositor's cast (ashpd's `Session` has no `Drop`), the
COMPOSITOR-side session leaked too. GameStream drops the pooled capturer on every HDR↔SDR
reconnect flip and opens a fresh one, so the sessions accumulated: exactly the "second conflicting
screencast" the pooling exists to prevent. Both portal threads now park on a `oneshot::Receiver`
and `PortalSession::drop` fires it, then waits — BOUNDED (750 ms, then detach with a warning) —
for a completion signal the thread sends after its runtime has been dropped. Bounded because
teardown runs on the session path and the thread may be inside a D-Bus round-trip: an unbounded
`join()` there would hang the host rather than leak. Every early-return path in `open` drops the
session too, so a failed PipeWire spawn no longer strands a fresh cast.
**2.2 (L2) — a dead capturer was pooled forever.** All three of the portal backend's terminal
states are sticky and observable only by consuming a frame, and `Capturer` exposed no health
predicate, so GameStream re-pooled unconditionally — after `result` was already an `Err`. One
zerocopy-worker death or one compositor restart therefore wedged GameStream portal video at 10 s
per reconnect attempt, permanently (that path has no rebuild closure). Adds
`Capturer::is_alive()` (default `true`), implemented for `PortalCapturer` as
`!broken && streaming && !thread.is_finished()` — the third term catches a dead PipeWire thread,
which is otherwise indistinguishable from an idle desktop. A static desktop stays `Streaming`, so
an idle-but-healthy capture is never reported dead. The host re-pools only on
`result.is_ok() && capturer.is_alive()` and logs which of the two retired it. Both halves ship
together: either alone leaves the wedge reachable.
**2.3 (L3/F1) — a "VAAPI" downgrade latch was really a global zero-copy kill switch.**
`spawn_pipewire` hand-mirrored the thread's `vaapi_passthrough` decision and the copy omitted
`raw_dmabuf_import_disabled`, so once that latch fired the capturer still believed it had made the
passthrough offer while the thread had already fallen back — and its timeout branch then latched a
downgrade for an offer nobody made. That downgrade was `note_vaapi_dmabuf_failed`, which fed
`pf_zerocopy::enabled()`, so ONE failed negotiation dropped every later session on the host — the
NVENC EGL→CUDA path included — to CPU capture until restart. And since the raw-passthrough offer
is also the PyroWave one on ANY vendor, a single PyroWave negotiation timeout was enough to do it
on an NVIDIA box.
Two fixes, both structural:
- `negotiation_plan(NegotiationInputs) -> NegotiationPlan` is now the ONE resolver, consumed by
the thread AND by `spawn_pipewire` — the mirror is deleted, not re-synced (WP7.6's shape), so
the drift class is unrepresentable. It is pure (every env/latch read is an input), which is
what makes it testable; the runtime-dependent half stays a method (`want_dmabuf` needs the
modifier list the importer's construction actually yielded). The redundant `importer.is_none()`
term goes: `build_importer` already excludes the passthrough, and that term is what made the
decision look impure enough to "need" a mirror. `PUNKTFUNK_FORCE_SHM` was ALSO read in both
places; now once.
- the capture-side downgrade becomes `pf_zerocopy::note_raw_dmabuf_negotiation_failed`, which
latches `RAW_DMABUF_DISABLED` — gating only the raw-passthrough decision. `enabled()` is now
the env var alone, and `VAAPI_DMABUF_FAILED` is deleted.
Also (L13, needed by 2.3's plan inputs): `PUNKTFUNK_PIPEWIRE_NV12` honoured only the exact string
`"0"`, so `"0 "` or `"false"` read as force-ON — the bug class of ed525c4c. pf-host-config's
explicit-off grammar is now exported as `env_on()` and the four in-crate copies (`PUNKTFUNK_
ZEROCOPY`/`_10BIT`/`_444`/`_CHACHA20`) plus this new caller share it.
7 new tests pin the four invariants that were prose-only comments, plus the latch property that
made the mirror wrong. pf-capture 13/13; workspace clippy --all-targets clean on Linux and on
x86_64-pc-windows-msvc. On-glass validation of 2.1/2.2/2.3 is owed (sweep Phase 7.1).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Claude Opus 5
@@ -34,6 +34,28 @@
|
||||
|
||||
use std::sync::OnceLock;
|
||||
|
||||
/// Whether a `PUNKTFUNK_*` env var reads as ON, or `None` when it is unset — the host's
|
||||
/// **explicit-off** grammar: `0` / `false` / `off` / `no` (trimmed, case-insensitive) are off and ANY
|
||||
/// other value is on, so a presence-style `=1` keeps working. Every "default ON" knob below shares
|
||||
/// it.
|
||||
///
|
||||
/// Exported because callers in other crates need the SAME grammar. A hand-rolled
|
||||
/// `var(k).as_deref() != Ok("0")` accepts `"0 "` (trailing space, trivially produced by a systemd
|
||||
/// drop-in or a shell heredoc) and `"false"` as ON — the bug class of ed525c4c, and the reason
|
||||
/// `PUNKTFUNK_PIPEWIRE_NV12` in pf-capture now routes through here.
|
||||
///
|
||||
/// Note this is deliberately NOT the grammar `pf-zerocopy` uses for its own flags (truthy:
|
||||
/// `1|true|yes|on`, everything else off) — see the module docs: independent features that share a
|
||||
/// name prefix.
|
||||
pub fn env_on(name: &str) -> Option<bool> {
|
||||
std::env::var(name).ok().map(|s| {
|
||||
!matches!(
|
||||
s.trim().to_ascii_lowercase().as_str(),
|
||||
"0" | "false" | "off" | "no"
|
||||
)
|
||||
})
|
||||
}
|
||||
|
||||
/// Resolved host configuration. Holds the genuinely-constant operator/dispatch knobs (see module docs for
|
||||
/// what is deliberately excluded). Fields read on only one platform are kept alive cross-platform by the
|
||||
/// derived `Debug` impl, so the parser can stay a single platform-neutral function.
|
||||
@@ -128,42 +150,16 @@ impl HostConfig {
|
||||
idd_depth: val("PUNKTFUNK_IDD_DEPTH")
|
||||
.and_then(|s| s.parse::<usize>().ok())
|
||||
.unwrap_or(2),
|
||||
zerocopy: val("PUNKTFUNK_ZEROCOPY").map(|s| {
|
||||
!matches!(
|
||||
s.trim().to_ascii_lowercase().as_str(),
|
||||
"0" | "false" | "off" | "no"
|
||||
)
|
||||
}),
|
||||
zerocopy: env_on("PUNKTFUNK_ZEROCOPY"),
|
||||
// Default ON, explicit-off grammar (mirrors `four_four_four`: the client's HDR setting
|
||||
// is the real per-session switch; the encode probe keeps incapable GPUs honest at 8-bit).
|
||||
ten_bit: val("PUNKTFUNK_10BIT")
|
||||
.map(|s| {
|
||||
!matches!(
|
||||
s.trim().to_ascii_lowercase().as_str(),
|
||||
"0" | "false" | "off" | "no"
|
||||
)
|
||||
})
|
||||
.unwrap_or(true),
|
||||
ten_bit: env_on("PUNKTFUNK_10BIT").unwrap_or(true),
|
||||
// Default ON, explicit-off grammar (the client's own 4:4:4 setting — default OFF —
|
||||
// is the real switch; see the field doc).
|
||||
four_four_four: val("PUNKTFUNK_444")
|
||||
.map(|s| {
|
||||
!matches!(
|
||||
s.trim().to_ascii_lowercase().as_str(),
|
||||
"0" | "false" | "off" | "no"
|
||||
)
|
||||
})
|
||||
.unwrap_or(true),
|
||||
four_four_four: env_on("PUNKTFUNK_444").unwrap_or(true),
|
||||
// Default ON, explicit-off grammar (the client's VIDEO_CAP_CHACHA20 bit is the real
|
||||
// per-session switch; see the field doc).
|
||||
chacha20: val("PUNKTFUNK_CHACHA20")
|
||||
.map(|s| {
|
||||
!matches!(
|
||||
s.trim().to_ascii_lowercase().as_str(),
|
||||
"0" | "false" | "off" | "no"
|
||||
)
|
||||
})
|
||||
.unwrap_or(true),
|
||||
chacha20: env_on("PUNKTFUNK_CHACHA20").unwrap_or(true),
|
||||
perf: flag("PUNKTFUNK_PERF"),
|
||||
video_source: val("PUNKTFUNK_VIDEO_SOURCE"),
|
||||
compositor: val("PUNKTFUNK_COMPOSITOR"),
|
||||
|
||||
Reference in New Issue
Block a user