From c3ecc291170c3ef5cc7eb02cb786cd4fa495b219 Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Sun, 9 Aug 2026 00:01:24 +0200 Subject: [PATCH] feat(pf-capture): the zero-copy path never asked the compositor for buffer headroom MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Wave-2 PW5 stage 2, on the number stage 1 just made visible. `build_dmabuf_buffers` set `SPA_PARAM_BUFFERS_dataType` and stopped there — no `SPA_PARAM_BUFFERS_buffers` at all, so the pool depth the whole zero-copy safety argument rests on was entirely the producer's choice, and we never even expressed a preference. This asks for 8 (min 2, max 16). A **Choice Range**, deliberately, not a fixed count. SPA intersects the consumer's and producer's Buffers params, so a fixed 8 against a producer that can only afford 4 empties the intersection and the link stalls in "negotiating" with no error anywhere — the exact trap that cost this codebase the entire Linux cursor channel once, when a 256^2 cursor-meta max failed to intersect Mutter's fixed 384^2 offer. With a range the producer clamps into it and negotiation still succeeds; the min stays at 2 so nothing that works today stops working. The numbers, and what they are not: 8 buffers is ~133 ms of pool at 60 Hz and ~33 ms at 240 Hz, well past the ~3-4 ms capture-to-fence latency PW3/PW4 measured, with room for a second frame in flight. 16 is a ceiling rather than a request — a 4K 4:4:4 buffer is ~25 MB, so 16 of them is ~400 MB of compositor allocation. These are the values we ASK for; what a producer actually allocates is what stage 1's census line reports, and that line is the one to trust. Scoped to the dmabuf pod only. The mappable and SHM-only builders are untouched: their consumers copy out of the buffer inside `.process`, so pool depth is not part of their correctness argument. A test pins the pod SHAPE — Choice, Range, Int children, values default-first — so a later simplification cannot quietly turn the range back into a number and take the negotiation down with it. Gates green at CI parity; on-glass negotiation on each producer is stage 2's own gate and is reported with the stage-1 census numbers. --- crates/pf-capture/src/linux/pw_pods.rs | 96 ++++++++++++++++++++++++-- 1 file changed, 90 insertions(+), 6 deletions(-) diff --git a/crates/pf-capture/src/linux/pw_pods.rs b/crates/pf-capture/src/linux/pw_pods.rs index b1bd7b71..777913a2 100644 --- a/crates/pf-capture/src/linux/pw_pods.rs +++ b/crates/pf-capture/src/linux/pw_pods.rs @@ -288,16 +288,57 @@ pub(super) fn build_shm_only_buffers() -> Result> { }) } -/// Build a Buffers param requesting dmabuf-only buffers. +/// PW5 stage 2: the buffer-pool depth we ASK for on the zero-copy path, as a Choice range. +/// +/// The zero-copy path hands the SPA buffer back to the producer at `.process` return, while the +/// encode thread still holds a dup of its dmabuf fd and has not yet imported, let alone read, the +/// contents. Nothing bounds that window — see the `queue_raw_buffer` comment in `pipewire.rs` — so +/// the only thing that keeps capture untorn is the producer round-robining a pool deeper than our +/// import+encode latency. Until PW5 stage 1 nobody had ever counted what that pool was; we never +/// even asked for a size (`build_dmabuf_buffers` set `dataType` and nothing else). +/// +/// A **range**, deliberately, not a fixed count: SPA intersects the consumer's and producer's +/// Buffers params, so a fixed 8 against a producer that can only afford 4 empties the intersection +/// and the link silently stalls in "negotiating" — the exact failure mode the cursor-meta `size` +/// property already cost this codebase once (see `build_cursor_meta_param`). With a range the +/// producer clamps into it and negotiation still succeeds. +/// +/// The numbers: `min` stays at 2 so nothing that works today stops working; `default` 8 is ~133 ms +/// of buffer at 60 Hz and ~33 ms at 240 Hz, comfortably past the ~3-4 ms capture→fence latency +/// measured in PW3/PW4 even with a second frame in flight; `max` 16 is a ceiling, not a request +/// (a 4K 4:4:4 buffer is ~25 MB, so 16 is ~400 MB of compositor allocation and worth capping). +/// **What the producer actually picks is logged by the stage-1 census — trust that line, not +/// these constants.** +const POOL_MIN: i32 = 2; +const POOL_DEFAULT: i32 = 8; +const POOL_MAX: i32 = 16; + +/// Build a Buffers param requesting dmabuf-only buffers, with pool headroom (see [`POOL_DEFAULT`]). pub(super) fn build_dmabuf_buffers() -> Result> { serialize_pod(pw::spa::pod::Object { type_: pw::spa::utils::SpaTypes::ObjectParamBuffers.as_raw(), id: pw::spa::param::ParamType::Buffers.as_raw(), - properties: vec![pw::spa::pod::Property { - key: pw::spa::sys::SPA_PARAM_BUFFERS_dataType, - flags: pw::spa::pod::PropertyFlags::empty(), - value: pw::spa::pod::Value::Int(1i32 << pw::spa::sys::SPA_DATA_DmaBuf), - }], + properties: vec![ + pw::spa::pod::Property { + key: pw::spa::sys::SPA_PARAM_BUFFERS_dataType, + flags: pw::spa::pod::PropertyFlags::empty(), + value: pw::spa::pod::Value::Int(1i32 << pw::spa::sys::SPA_DATA_DmaBuf), + }, + pw::spa::pod::Property { + key: pw::spa::sys::SPA_PARAM_BUFFERS_buffers, + flags: pw::spa::pod::PropertyFlags::empty(), + value: pw::spa::pod::Value::Choice(pw::spa::pod::ChoiceValue::Int( + pw::spa::utils::Choice( + pw::spa::utils::ChoiceFlags::empty(), + pw::spa::utils::ChoiceEnum::Range { + default: POOL_DEFAULT, + min: POOL_MIN, + max: POOL_MAX, + }, + ), + )), + }, + ], }) } @@ -512,4 +553,47 @@ mod tests { "libspa renumbered spa_video_transfer_function — update the hardcoded PQ id" ); } + + /// PW5 stage 2: the pool request must be a **Choice Range**, never a fixed Int. + /// + /// This is the whole safety argument for asking at all: SPA intersects the two sides' Buffers + /// params, so a fixed count a producer cannot afford empties the intersection and the link + /// stalls in "negotiating" with no error anywhere — the same trap that cost this codebase the + /// entire Linux cursor channel once (see `build_cursor_meta_param`). Asserting the pod shape + /// is what keeps a later "simplify" from turning the range back into a number. + #[test] + fn the_dmabuf_pool_request_is_a_range_not_a_fixed_count() { + let pod = build_dmabuf_buffers().unwrap(); + let key = spa::sys::SPA_PARAM_BUFFERS_buffers.to_ne_bytes(); + let at = pod + .windows(4) + .position(|w| w == key) + .expect("the dmabuf Buffers pod must carry a buffers count"); + let word = |off: usize| u32::from_ne_bytes(pod[off..off + 4].try_into().unwrap()); + // Property = { key, flags, value_pod }; value_pod = { size, type, body }. A Choice body + // is { type: u32, flags: u32, child_size: u32, child_type: u32, values… }. + assert_eq!( + word(at + 12), + spa::sys::SPA_TYPE_Choice, + "the buffers count must be a Choice, not a bare Int — a fixed count can fail \ + negotiation outright" + ); + assert_eq!( + word(at + 16), + spa::sys::SPA_CHOICE_Range, + "the Choice must be a Range (default, min, max)" + ); + assert_eq!(word(at + 24), 4, "Choice child pods are 4-byte Ints"); + assert_eq!(word(at + 28), spa::sys::SPA_TYPE_Int, "…of type Int"); + let vals: Vec = (0..3) + .map(|i| i32::from_ne_bytes(pod[at + 32 + i * 4..at + 36 + i * 4].try_into().unwrap())) + .collect(); + assert_eq!( + vals, + vec![POOL_DEFAULT, POOL_MIN, POOL_MAX], + "Range values are serialized default-first" + ); + // The minimum must not exceed what producers already serve, or the ask becomes a demand. + const { assert!(POOL_MIN <= 2) }; + } }