From 6d550530fe1c6ec0c8011fe98f3485794f7aafc3 Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Sun, 9 Aug 2026 00:01:08 +0200 Subject: [PATCH 1/6] =?UTF-8?q?feat(pf-capture):=20nothing=20had=20ever=20?= =?UTF-8?q?counted=20the=20compositor's=20buffer=20pool=20=E2=80=94=20the?= =?UTF-8?q?=20number=20every=20zero-copy=20safety=20argument=20rests=20on?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Wave-2 PW5 stage 1, and the one stage with no risk at all. The zero-copy capture path dups the dmabuf fd, publishes the frame, and hands the SPA buffer straight back to the producer at `.process` return — while the encode thread has not yet imported it, let alone read it. The code says so itself ("content stability across the brief import/encode window relies on the compositor's buffer-pool depth, like any zero-copy capture"). That depth is therefore load-bearing: it is the ONLY thing standing between us and the producer overwriting a buffer mid-read. And it had never been measured. Not logged, not asserted, not even requested — `build_dmabuf_buffers` set `SPA_PARAM_BUFFERS_dataType` and nothing else, so whatever the producer picked is what we got, silently. This adds the `add_buffer`/`remove_buffer` stream callbacks PipeWire has always offered and logs the count once per distinct depth: `pool_depth`, `high_water`, and the latest-frame-only `drained` count beside it. One line per session on a stable pool (`.process` runs at the capture rate — an unconditional log would be 240 lines a second of the same number), a second line if a renegotiation changes the depth. `high_water` is tracked separately from `live` because a renegotiation frees the pool before re-allocating it: any decision keyed on the live count would read that dip as "the pool shrank". `remove` saturates at zero rather than wrapping, so an unmatched remove cannot report `u32::MAX` buffers. Measurement only — no behaviour change, and no consumer of the number yet. PW5's later stages need it (a deeper encode pipeline widens the overwrite window by a full frame period), but the number is worth having regardless of whether those stages ever land: it is the answer to "is our zero-copy capture actually safe on this compositor", and until now the honest answer was "nobody knows". 3 tests pin the once-per-depth logging, the renegotiation dip, and the saturating remove. Gates green at CI parity. --- crates/pf-capture/src/linux/pipewire.rs | 121 +++++++++++++++++++++++- 1 file changed, 120 insertions(+), 1 deletion(-) diff --git a/crates/pf-capture/src/linux/pipewire.rs b/crates/pf-capture/src/linux/pipewire.rs index 317ae693..14f3091e 100644 --- a/crates/pf-capture/src/linux/pipewire.rs +++ b/crates/pf-capture/src/linux/pipewire.rs @@ -73,6 +73,10 @@ struct UserData { /// PW4 step 1: the producer-fence wait distribution, measured on this (the PipeWire loop) /// thread. Per-session, like the fall-through tally. fence_wait: FenceWaitStats, + /// PW5 step 1: the negotiated buffer-pool depth, counted from `add_buffer`/`remove_buffer`. + /// See [`PoolCensus`] — this is the number that decides whether a deeper encode pipeline is + /// safe, and until now nobody had it. + pool: PoolCensus, /// Raw-passthrough frames that silently fell through to the CPU de-pad path, by reason — see /// [`PassthroughFallbacks`]. Per-session: a fresh `UserData` is built per pipeline, so a /// compositor that starts serving dmabufs again after a rebuild gets a fresh log budget. @@ -503,6 +507,51 @@ impl FenceWaitStats { } } +/// PW5 stage 1: how many buffers the producer actually allocated for this stream. +/// +/// **Nothing in this codebase had ever counted them.** The zero-copy path dups the dmabuf fd and +/// publishes the frame while the SPA buffer is handed straight back to the producer at `.process` +/// return — so the only thing keeping capture untorn is that the producer round-robins a pool +/// deeper than our import+encode window. That depth was an unmeasured assumption; this makes it a +/// logged number, on every producer, before anything is built on it. +/// +/// `live` is maintained by the `add_buffer`/`remove_buffer` stream callbacks, which PipeWire fires +/// on the loop thread as the pool is allocated (and again, remove-then-add, on a renegotiation that +/// replaces it). There is no "pool complete" event, so the count is published from `.process`: by +/// the time the first buffer is dequeued the allocation has finished. +#[derive(Debug, Default, Clone, Copy)] +pub(super) struct PoolCensus { + /// Buffers currently in the pool (adds minus removes). + live: u32, + /// Deepest `live` seen this session — the number a depth decision must key on, since a + /// renegotiation can transiently shrink the pool to zero. + high_water: u32, + /// The `live` value already logged, so a stable pool logs exactly one line per distinct depth + /// (a renegotiation that changes the depth is worth a second line; 240 frames a second of the + /// same number is not). + logged: Option, +} + +impl PoolCensus { + fn add(&mut self) { + self.live += 1; + self.high_water = self.high_water.max(self.live); + } + + fn remove(&mut self) { + self.live = self.live.saturating_sub(1); + } + + /// Called from `.process`, once per buffer. Returns `Some(live)` the first time each distinct + /// depth is seen — the caller logs then and only then. + fn note_frame(&mut self) -> Option { + (self.logged != Some(self.live)).then(|| { + self.logged = Some(self.live); + self.live + }) + } +} + /// Per-session tally of raw-passthrough frames that fell through to the CPU path, with a one-line /// budget per distinct reason. /// @@ -1397,6 +1446,7 @@ pub fn pipewire_thread( linear_nv12_failed: false, dbg_log_n: 0, fence_wait: FenceWaitStats::default(), + pool: PoolCensus::default(), passthrough_fallbacks: PassthroughFallbacks::default(), cursor: CursorState::new(cursor_id0_hides), expect_dims: if expect_exact_dims { @@ -1495,6 +1545,11 @@ pub fn pipewire_thread( } } }) + // PW5 stage 1 — the pool census. PipeWire fires these on the loop thread as it allocates + // (and, on a renegotiation, frees then re-allocates) the stream's buffers. Counting only: + // the buffer pointer is not touched, so no lifetime question arises here. + .add_buffer(|_stream, ud, _buf| ud.pool.add()) + .remove_buffer(|_stream, ud, _buf| ud.pool.remove()) .process(|stream, ud| { // 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 @@ -1523,6 +1578,25 @@ pub fn pipewire_thread( newest = next; drained += 1; } + // PW5 stage 1: publish the depth the producer actually negotiated, once per distinct + // value. MEASURED, not requested: `build_dmabuf_buffers` asks for a range and the + // producer picks — this line is the only place the picked number is visible. + // + // Why it matters beyond curiosity: `stream.queue_raw_buffer(newest)` at the end of this + // callback hands the buffer back while the encode thread may still be importing and + // reading its dmabuf, so content stability rests entirely on the producer not cycling + // back to this buffer before we are done with it. That window is `pool_depth` buffer + // periods wide. A pool of 2 has essentially none. + if let Some(depth) = ud.pool.note_frame() { + tracing::info!( + pool_depth = depth, + high_water = ud.pool.high_water, + drained, + "pipewire buffer pool negotiated — this is the producer's ACTUAL count \ + (add_buffer/remove_buffer), the window in which a buffer we handed back may \ + be rewritten while the encoder still reads it" + ); + } // Sacrificial-mode gate (kwin.rs `create`): until the producer renegotiates to the // expected dims, every buffer — frame AND cursor meta, whose positions are in the // doomed mode's space — belongs to the birth mode; consuming one would build the @@ -2128,7 +2202,7 @@ mod tests { use super::{ consumer_kind, resolved_capture_arm, CaptureArm, ConsumerKind, FenceWaitStats, - PassthroughFallback, PassthroughFallbacks, FENCE_WAIT_BUCKETS_US, + PassthroughFallback, PassthroughFallbacks, PoolCensus, FENCE_WAIT_BUCKETS_US, }; /// A PyroWave session is PyroWave even though it also flips `backend_is_vaapi` on (the @@ -2342,4 +2416,49 @@ mod tests { ); } } + + /// PW5 stage 1: a stable pool logs ONE line, not one per frame. `.process` runs at the capture + /// rate — an unconditional log here would be 240 lines a second of the same number. + #[test] + fn a_stable_pool_is_logged_once() { + let mut p = PoolCensus::default(); + for _ in 0..8 { + p.add(); + } + assert_eq!(p.note_frame(), Some(8)); + for _ in 0..100 { + assert_eq!(p.note_frame(), None, "the same depth must not re-log"); + } + } + + /// A renegotiation frees the pool and re-allocates it. The LIVE count therefore dips (and the + /// new depth is worth a second line), but `high_water` — the number a pipeline-depth decision + /// keys on — must not follow the dip down. + #[test] + fn a_renegotiated_pool_relogs_but_the_high_water_holds() { + let mut p = PoolCensus::default(); + for _ in 0..8 { + p.add(); + } + assert_eq!(p.note_frame(), Some(8)); + for _ in 0..8 { + p.remove(); + } + for _ in 0..4 { + p.add(); + } + assert_eq!(p.note_frame(), Some(4), "a changed depth is worth a line"); + assert_eq!(p.high_water, 8, "the deepest pool seen this session"); + } + + /// `remove_buffer` without a matching `add_buffer` must not wrap the count to `u32::MAX` — + /// a depth gate reading that would happily pipeline against a pool of zero. + #[test] + fn unmatched_removes_saturate_at_zero() { + let mut p = PoolCensus::default(); + p.remove(); + p.remove(); + assert_eq!(p.note_frame(), Some(0)); + assert_eq!(p.high_water, 0); + } } From c3ecc291170c3ef5cc7eb02cb786cd4fa495b219 Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Sun, 9 Aug 2026 00:01:24 +0200 Subject: [PATCH 2/6] 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) }; + } } From 95962f55d03b9b62e1331b1791586d4138b4b219 Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Sun, 9 Aug 2026 00:11:22 +0200 Subject: [PATCH 3/6] =?UTF-8?q?refactor(pf-encode):=20PyroWave=20waited=20?= =?UTF-8?q?its=20fence=20inside=20submit=20=E2=80=94=20the=20one=20backend?= =?UTF-8?q?=20that=20did?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Wave-2 PW5 stage 3. Depth is STILL 1; this is the shape change alone. `encode_frame` recorded CSC+encode, queue-submitted, waited the fence and packetized, all inside `Encoder::submit`. Every other backend in this crate puts the wait on the POLL side. That difference is the whole reason the host loop's cadence folds around this encoder: with the wait inline, `submit` returns only after the GPU is done, so the arrival-anchored floor absorbs the encode only while it stays under 0.9x the frame interval. Split into `submit_frame` (ingest -> CSC -> pyrowave encode -> queue-submit -> return) and `wait_and_packetize` (fence wait -> packetize -> AU), with an `InFlight` deque between them capped by `max_inflight`, which is 1. **One is the only value the resources can support today** — `cmd`, `fence`, `csc_set` and the y/uv images are one each, so a second concurrent frame would record into a PENDING command buffer and storage-write images pyrowave is still sampling. `submit` therefore drains to `max_inflight - 1` before recording, which states that invariant in one place instead of leaving it implicit in "the encode is synchronous". The subtle part is the command-buffer state machine, and it is unchanged: the record-and-submit closure still resets `cmd` on every PRE-submit failure (RECORDING/INVALID/EXECUTABLE, never PENDING), and the fence wait still does NOT reset on failure, because a timeout leaves the buffer PENDING where a reset violates VUID-vkResetCommandBuffer-commandBuffer-00045. What changed is that a failed wait now also leaves the entry IN FLIGHT — which is precisely what tells `reset()` there is live GPU work to re-wait before the pyrowave encoder object may be destroyed. `gpu_pending` is gone; `!inflight.is_empty()` is the same fact, and cannot drift from it. The split opened two windows that did not exist when everything ran inline, both closed here: `reconfigure_bitrate` and `set_wire_chunking` can now land BETWEEN a submit and its poll, so the packetize boundary and the bitstream cap are snapshotted into `InFlight` at submit time. Reading the live fields would have let a mid-flight bitrate drop turn a perfectly good frame into "unexpected packet count", and a mid-flight chunking change into an AU with the wrong `chunk_aligned` flag. `flush()` is no longer a no-op — it drains the in-flight frame, so the trait's poll-until-None contract still returns every AU (the `spike` subcommand and the hardware smoke tests are the real users). The perf instrument still measures submit->AU, stamped at submit and taken when the AU becomes readable, so `92326312`'s numbers stay directly comparable; the log line now carries `depth` and says plainly that above depth 1 the number legitimately grows by about one loop period. VERIFIED ON GLASS (.21, RTX 5070 Ti, GPU idle at 180 MHz of 3090 — so these are slow-clock runs, which is the worst case on this card, not the best): all 6 `#[ignore]`d GPU tests pass — the 4:2:0, 4:4:4 and 24-bpp PSNR smokes, the mode-mismatch refusal, the fd-leak check and the golden dump. Byte-identity, honestly: the AU bytes are NOT reproducible, and were not before this commit either. Three runs of the SAME unmodified binary produced three different `au-dense.bin` hashes (ab7ecaf6 / 8735700e / 933b3d40) — the vendored 4:2:0 encoder emits run-varying bytes that the decoder ignores. So the meaningful gate is DECODE identity, and that holds exactly: every decoded plane (`ref-dense-{y,cb,cr}`, `ref-chunked-{y,cb,cr}`, `ref-dense444-{y,cb,cr}`) is bit-identical between the pre-split base and this commit, across four runs. 4:4:4 AUs are additionally bit-stable and match the checked-in Apple fixture exactly. Gates green at CI parity. --- crates/pf-encode/src/enc/linux/pyrowave.rs | 193 ++++++++++++++++----- 1 file changed, 149 insertions(+), 44 deletions(-) diff --git a/crates/pf-encode/src/enc/linux/pyrowave.rs b/crates/pf-encode/src/enc/linux/pyrowave.rs index dbd8a267..c66d355f 100644 --- a/crates/pf-encode/src/enc/linux/pyrowave.rs +++ b/crates/pf-encode/src/enc/linux/pyrowave.rs @@ -393,6 +393,32 @@ fn priority_refused(e: vk::Result) -> bool { ) } +/// One frame recorded and queue-submitted, whose fence has not been waited and whose bitstream has +/// therefore not been packetized yet. PW5 stage 3: this is what the `submit`/`poll` split created — +/// before it, no such state could exist because `submit` did the whole thing inline. +#[derive(Clone, Copy)] +struct InFlight { + /// The capture timestamp this frame's AU must carry. Held here rather than re-read from the + /// `CapturedFrame` because the frame is the CALLER's and is gone by the time we packetize. + pts_ns: u64, + /// The bitstream buffer size this frame was ENCODED against (`frame_budget + BS_SLACK`), and + /// the packetize boundary in dense mode. + /// + /// Snapshotted at submit rather than re-read at poll because the split opened a window that + /// did not exist before: `reconfigure_bitrate` can land between the two, and in dense mode the + /// boundary IS this number — a shrunk budget would make `compute_num_packets` return more than + /// one packet and the encode would bail with "unexpected packet count" on a frame that was + /// perfectly fine. + cap: usize, + /// The datagram alignment this frame was encoded for; `set_wire_chunking` can likewise land + /// mid-flight, and a frame packetized at a boundary it was not rate-controlled for would ship + /// with the wrong `chunk_aligned` flag. + wire_chunk: Option, + /// `PUNKTFUNK_PERF`: when `submit` started. The summary keeps measuring submit→AU (what + /// `92326312` measured, so stage 0's baseline stays comparable), not just the wait half. + t0: std::time::Instant, +} + pub struct PyroWaveEncoder { // --- vulkan core (owned; private to this encoder) --- _entry: ash::Entry, @@ -447,10 +473,17 @@ pub struct PyroWaveEncoder { cmd_pool: vk::CommandPool, cmd: vk::CommandBuffer, fence: vk::Fence, - /// True between a successful `queue_submit` and its successful fence wait — i.e. exactly when - /// GPU work may still be executing. `reset()` keys its bounded wait on this: a never-submitted - /// fence would otherwise read as "wedged" (fences start unsignaled). - gpu_pending: bool, + /// Frames recorded and queue-submitted whose fence has not been waited yet — i.e. exactly the + /// work that may still be executing on the GPU. `reset()` keys its bounded wait on this being + /// non-empty: a never-submitted fence would otherwise read as "wedged" (fences start + /// unsignaled). At today's depth of 1 this holds at most one entry. + inflight: VecDeque, + /// How many frames may be submitted-but-not-polled at once. **1 today, and the only value the + /// resources below can support** — `cmd`, `fence`, `csc_set` and the y/uv images are one each, + /// so a second concurrent frame would record into a PENDING command buffer and storage-write + /// the images pyrowave is still sampling. PW5 stages 4-6 raise this; until then the split + /// exists so the fence wait is on the POLL side, where every other backend already has it. + max_inflight: usize, // --- state --- width: u32, @@ -490,11 +523,17 @@ fn budget_for(bitrate_bps: u64, fps: u32) -> usize { } impl PyroWaveEncoder { - /// `PUNKTFUNK_PERF`: record one synchronous-encode duration and summarise on a slow cadence. + /// `PUNKTFUNK_PERF`: record one encode duration and summarise on a slow cadence. /// - /// Whole-`submit` timing on purpose: for this backend that IS the encode — `encode_frame` - /// records CSC+encode, submits, waits the fence and packetizes, all inline (which is also - /// why the loop's period folds to `interval + encode`, the thing PW5 exists to unfold). + /// **submit→AU on purpose**, and unchanged by PW5 stage 3's `submit`/`poll` split: the sample + /// is stamped when `submit` starts and taken when the AU becomes readable, so it still covers + /// CSC + encode + fence wait + packetize and stays directly comparable to the pre-split + /// baseline (`92326312`). What the split changed is WHERE the wait sits — the host loop's own + /// `submit_us` now excludes it, which is the shape every other backend already had. + /// + /// ⚠ At a depth greater than 1 this number legitimately grows by roughly one loop period, + /// because frame N's AU is not retrieved until after N+1 has been submitted. That is real + /// added latency, not an instrumentation artefact — see PW5's escalation gate. fn note_encode_us(&mut self, us: u32) { if !pf_host_config::config().perf { return; @@ -521,9 +560,10 @@ impl PyroWaveEncoder { p50_us = pct(&s, 0.50), p99_us = pct(&s, 0.99), max_us = *s.last().unwrap_or(&0), - "pyrowave encode (synchronous: CSC + encode + fence wait + packetize). Under a \ + depth = self.max_inflight, + "pyrowave encode, submit->AU (CSC + encode + fence wait + packetize). Under a \ GPU-bound game this is the number the global-priority queue exists to protect — \ - watch p99, not the mean" + watch p99, not the mean. At depth > 1 it includes one loop period of pipelining" ); } @@ -869,7 +909,9 @@ impl PyroWaveEncoder { cmd_pool: vk::CommandPool::null(), cmd: vk::CommandBuffer::null(), fence: vk::Fence::null(), - gpu_pending: false, + inflight: VecDeque::new(), + // PW5: depth 1. Stages 4-6 raise it; the per-slot resources above cannot support 2. + max_inflight: 1, width: w, height: h, fps, @@ -1356,9 +1398,14 @@ impl PyroWaveEncoder { } } - /// One frame, synchronously: ingest → CSC → pyrowave encode (recorded into our command - /// buffer) → submit + fence wait (sub-ms) → packetize into an `EncodedFrame`. - unsafe fn encode_frame(&mut self, frame: &CapturedFrame) -> Result<()> { + /// The SUBMIT half of one frame (PW5 stage 3): ingest → CSC → pyrowave encode, recorded into + /// our command buffer → queue-submit → **return**. The fence wait and packetize moved to + /// [`wait_and_packetize`](Self::wait_and_packetize), which is where every other backend in + /// this crate has always had them. + /// + /// On success exactly one [`InFlight`] is pushed. On failure nothing is pushed and the command + /// buffer has been reset (see the error-arm note inside) — the caller may submit again. + unsafe fn submit_frame(&mut self, frame: &CapturedFrame, t0: std::time::Instant) -> Result<()> { // A failed `reset()` leaves the encoder destroyed and null. Callers today turn that into // a session error and never resubmit, but a null here would be a use-after-free inside // pyrowave rather than a clean error — so fail loudly instead of relying on that. @@ -1666,10 +1713,34 @@ impl PyroWaveEncoder { let _ = dev.reset_command_buffer(self.cmd, vk::CommandBufferResetFlags::empty()); return Err(e); } - self.gpu_pending = true; + // Submitted: from here the GPU may be executing, and NOTHING may touch `cmd`, the y/uv + // images or `csc_set` until this entry is retired by `wait_and_packetize`. + self.inflight.push_back(InFlight { + pts_ns: frame.pts_ns, + cap: self.frame_budget + BS_SLACK, + wire_chunk: self.wire_chunk, + t0, + }); + Ok(()) + } + + /// The POLL half of one frame (PW5 stage 3): wait the oldest in-flight frame's fence, then + /// packetize its bitstream into an `EncodedFrame` on `pending`. + /// + /// ⚠ The fence wait's failure path deliberately does NOT reset the command buffer: a timeout + /// leaves it PENDING, where a reset violates VUID-vkResetCommandBuffer-commandBuffer-00045. + /// The in-flight entry is likewise NOT popped on that failure — it is what tells `reset()` + /// there is still live GPU work to re-wait before the encoder object may be destroyed. + unsafe fn wait_and_packetize(&mut self) -> Result<()> { + let Some(fr) = self.inflight.front().copied() else { + return Ok(()); + }; + let dev = self.device.clone(); dev.wait_for_fences(&[self.fence], true, 5_000_000_000) .context("pyrowave encode fence")?; - self.gpu_pending = false; + // Waited and signaled: the command buffer is INVALID (one-time submit), which the next + // `begin` may implicitly reset, and the GPU is done with this frame's resources. + self.inflight.pop_front(); // ---- packetize ---- // Dense (default): boundary = whole buffer → the AU is exactly one pyrowave packet. @@ -1678,16 +1749,19 @@ impl PyroWaveEncoder { // self-delimiting packets — the client windows its parse and a lost shard costs // only those blocks. Padding cost is small: the packetizer fills close to the // boundary by design. - let cap = self.frame_budget + BS_SLACK; + // `fr.cap`/`fr.wire_chunk`, NOT the live fields: `reconfigure_bitrate` and + // `set_wire_chunking` may have landed since this frame was submitted, and packetizing at a + // boundary the frame was not rate-controlled for is a spurious failure. + let cap = fr.cap; self.bitstream.resize(cap, 0); // Chunked mode reserves the 4-byte window prefix from the packetize boundary (shared helper). - let boundary = crate::pyrowave_wire::packet_boundary(self.wire_chunk, cap); + let boundary = crate::pyrowave_wire::packet_boundary(fr.wire_chunk, cap); let mut n: usize = 0; pw_check( pw::pyrowave_encoder_compute_num_packets(self.pw_enc, boundary, &mut n), "compute_num_packets", )?; - if n == 0 || (self.wire_chunk.is_none() && n != 1) { + if n == 0 || (fr.wire_chunk.is_none() && n != 1) { bail!("pyrowave: unexpected packet count {n} at boundary {boundary}"); } let mut packets = vec![pw::pyrowave_packet { offset: 0, size: 0 }; n]; @@ -1713,21 +1787,34 @@ impl PyroWaveEncoder { // Frame into the wire AU via the shared helper (byte-identical on Linux + Windows): the dense // single packet, or the datagram-aligned windowed AU (§4.4). let pkts: Vec<(usize, usize)> = packets.iter().map(|p| (p.offset, p.size)).collect(); - let au = crate::pyrowave_wire::build_au(&pkts, &self.bitstream, self.wire_chunk); - if self.wire_chunk.is_some() { + let au = crate::pyrowave_wire::build_au(&pkts, &self.bitstream, fr.wire_chunk); + if fr.wire_chunk.is_some() { let raw: usize = pkts.iter().map(|&(_, s)| s).sum(); self.wire_budget.observe(raw, au.len()); } self.frame_count += 1; self.pending.push_back(EncodedFrame { data: au, - pts_ns: frame.pts_ns, + pts_ns: fr.pts_ns, // Every frame is independently decodable — SOF/keyframe on each AU is the codec's // whole recovery story (plan §1.2). keyframe: true, recovery_anchor: false, - chunk_aligned: self.wire_chunk.is_some(), + chunk_aligned: fr.wire_chunk.is_some(), }); + // submit→AU, the same quantity `92326312` measured before the split, so stage 0's + // baseline stays comparable. Stamped here rather than in `submit` because that is where + // the AU now becomes readable. + self.note_encode_us(fr.t0.elapsed().as_micros() as u32); + Ok(()) + } + + /// Retire in-flight frames until at most `keep` remain. Used by `submit` (make room before + /// recording) and by `flush`/`poll` (drain). + unsafe fn drain_to(&mut self, keep: usize) -> Result<()> { + while self.inflight.len() > keep { + self.wait_and_packetize()?; + } Ok(()) } } @@ -1737,17 +1824,22 @@ impl Encoder for PyroWaveEncoder { // `PUNKTFUNK_PERF` encode split (kept above the SAFETY comment so that comment stays // attached to the block it proves — the crate denies undocumented unsafe blocks). let t0 = std::time::Instant::now(); - // SAFETY: single-threaded encoder; `encode_frame` records/submits on handles this - // struct owns and waits its own fence before touching results. Command-buffer state on - // failure is `encode_frame`'s own business now: its record-and-submit closure resets the - // buffer on every pre-submit failure, and its post-submit failures (fence timeout — - // buffer possibly PENDING) deliberately do NOT reset, because that violates + // SAFETY: single-threaded encoder; both halves work on handles this struct owns. + // Command-buffer state on failure is `submit_frame`'s own business: its record-and-submit + // closure resets the buffer on every pre-submit failure, and the fence wait's failure + // (buffer possibly PENDING) deliberately does NOT reset, because that violates // VUID-vkResetCommandBuffer-commandBuffer-00045; the blanket reset that used to live // here fired on exactly that path. Recovery (`reset()`/`Drop`) waits the device idle // before anything touches `cmd` again. - let r = unsafe { self.encode_frame(frame) }; - self.note_encode_us(t0.elapsed().as_micros() as u32); - r + unsafe { + // Make room before recording. At `max_inflight == 1` this retires the previous frame, + // which is what makes the single `cmd`/`fence`/`csc_set`/y/uv set safe to reuse — the + // whole depth-1 invariant, stated in one place. The host loop polls after every submit + // so this is normally a no-op; it is here for callers that do not (the `spike` + // subcommand, the hardware smoke tests). + self.drain_to(self.max_inflight.saturating_sub(1))?; + self.submit_frame(frame, t0) + } } fn caps(&self) -> EncoderCaps { @@ -1764,6 +1856,14 @@ impl Encoder for PyroWaveEncoder { } fn poll(&mut self) -> Result> { + // PW5 stage 3: THIS is where the fence wait now lives. `submit` returns as soon as the + // work is queued; the AU only exists once the fence signals and the bitstream is + // packetized, so poll completes the oldest in-flight frame before answering. + if self.pending.is_empty() && !self.inflight.is_empty() { + // SAFETY: single-threaded encoder, waiting its own fence and reading its own + // bitstream; the failure path leaves the entry in flight for `reset()` to re-wait. + unsafe { self.wait_and_packetize()? }; + } Ok(self.pending.pop_front()) } @@ -1771,15 +1871,15 @@ impl Encoder for PyroWaveEncoder { // Cheap in-place rebuild: recreate only the pyrowave encoder object — there is no // rate-control history or reference state worth preserving (plan §4.3). // - // Bounded wait first: the only work possibly still executing is the one submitted frame - // whose synchronous fence wait timed out (`gpu_pending`). Re-wait it under the same 5 s - // cap as `encode_frame` — an untimed `device_wait_idle` here would park the recovery - // thread on the exact device it suspects is wedged, until the kernel's GPU reset, if - // ever. If the fence still won't signal, destroying the pyrowave encoder under live GPU - // work would be a use-after-free, so report "no in-place rebuild" and let the session - // surface a real error (`Drop`'s unbounded idle covers teardown, where blocking on the - // kernel is acceptable). - if self.gpu_pending { + // Bounded wait first: the only work possibly still executing is a submitted frame whose + // fence wait has not succeeded yet (`inflight` non-empty — either never polled, or polled + // and timed out). Re-wait it under the same 5 s cap as `wait_and_packetize` — an untimed + // `device_wait_idle` here would park the recovery thread on the exact device it suspects + // is wedged, until the kernel's GPU reset, if ever. If the fence still won't signal, + // destroying the pyrowave encoder under live GPU work would be a use-after-free, so + // report "no in-place rebuild" and let the session surface a real error (`Drop`'s + // unbounded idle covers teardown, where blocking on the kernel is acceptable). + if !self.inflight.is_empty() { // SAFETY: waiting this encoder's own fence under `&mut self`. if unsafe { self.device @@ -1794,7 +1894,9 @@ impl Encoder for PyroWaveEncoder { self.pending.clear(); return false; } - self.gpu_pending = false; + // The submitted frames are forfeit (their bitstream lives in the encoder object about + // to be destroyed), but the GPU is provably done with them. + self.inflight.clear(); } // SAFETY: the device is idle for this encoder's work (the fence wait above, or no submit // outstanding) — this sweep-up is instant — and the pyrowave device outlives the encoder @@ -1822,7 +1924,7 @@ impl Encoder for PyroWaveEncoder { let r = pw::pyrowave_encoder_create(&einfo, &mut enc); if r != pw::pyrowave_result_PYROWAVE_SUCCESS { tracing::error!(result = ?r, "pyrowave: encoder rebuild failed"); - // `pw_enc` stays null — `Drop` and `encode_frame` both guard on it. The queued + // `pw_enc` stays null — `Drop` and `submit_frame` both guard on it. The queued // AUs are forfeit either way (the caller turns a false reset into a session // error), so drop them rather than shipping output from a dead encoder. self.pending.clear(); @@ -1860,8 +1962,11 @@ impl Encoder for PyroWaveEncoder { } fn flush(&mut self) -> Result<()> { - // Synchronous per-frame encode: nothing buffered beyond `pending`. - Ok(()) + // Since PW5 stage 3 there IS something buffered beyond `pending`: a submitted frame whose + // fence has not been waited. Retire it so the caller's `poll`-until-`None` drain + // (the trait's contract) actually returns every AU. Bounded by the same 5 s fence cap. + // SAFETY: single-threaded encoder, waiting its own fence. + unsafe { self.drain_to(0) } } } From 29248dcab957055aed4b4f6171d7a611f6d86d48 Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Sun, 9 Aug 2026 00:18:38 +0200 Subject: [PATCH 4/6] feat(pf-encode): PyroWave had six single-slot resources, not the two the plan named MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Wave-2 PW5 stage 4. Pure capacity — `max_inflight` is STILL 1, nothing overlaps yet. The plan named the y/uv images as the thing to double. Reading the backend found five more, and each is a correctness problem under overlap rather than a performance one: * `csc_set` — ONE descriptor set, rewritten every frame by `bind_rgb`. Updating a set still bound by a PENDING command buffer violates VUID-vkUpdateDescriptorSets-None-03047, and on most drivers that is a wrong picture rather than an error. * `y_img`/`uv_img` — the CSC of N+1 storage-writes exactly the images pyrowave is still sampling for N. The barrier comment ("the previous frame's encode already completed under our synchronous fence") was load-bearing and said so. * `cursor_img` + `cursor_stage` — the struct comment stated the assumption outright: *"Single (not ring) because PyroWave encodes one frame synchronously — no in-flight overlap to race."* * `cmd` + `fence` — you cannot record into a PENDING command buffer at all. * `cpu_img`/`cpu_stage` (software capture / tests) — the host writes staging while the previous frame's copy is still pending. All of it moves into a `Slot`, and the encoder now owns `SLOTS` of them. Two, because Granite caps the overlap at two for us: the pyrowave device defaults to `init_frame_contexts(2)` and `next_frame_context()` — called at the top of every `encode_gpu_synchronous` — waits the context it rotates into. A third slot would need a vendored `init_frame_contexts(3)` that is not exposed. `bitstream` and `import_cache` are deliberately NOT per-slot, and the `Slot` doc says why so a later sweep does not "fix" it: `bitstream` is only touched during packetize, i.e. only on the poll side one frame at a time, and `import_cache` retaining the VkImage/VkDeviceMemory per dmabuf inode is precisely what makes it safe for two slots to sample the same imported buffer. `cpu_expand` is shared for the same reason — it is copied into staging before `submit_frame` returns, so no GPU work ever reads it. Each frame carries its slot index in `InFlight` rather than recomputing it, so `wait_and_packetize` cannot wait the wrong fence — the failure that would look like corruption rather than an error. `reset()` now waits EVERY in-flight fence, not just one, which matters the moment depth rises. WHAT IT COSTS, measured from the driver's own memory requirements rather than estimated (.21, RTX 5070 Ti, and there is now an `#[ignore]`d test that prints it on any GPU): 1080p 4:2:0 3872 KiB per slot 7744 KiB for both 4K 4:2:0 12992 KiB per slot 25984 KiB for both 4K 4:4:4 24992 KiB per slot 49984 KiB for both So the extra slot costs ~3.8 MiB at 1080p and ~24 MiB at 4K 4:4:4 — an order of magnitude under the plan's ~25-35 MB / 100-150 MB estimate, because that estimate included pyrowave's internal wavelet and scratch buffers, which stage 5's second encoder handle will add and this stage does not. Affordable on an iGPU. The open line now logs `slots`, `slot_kib` and `slots_kib` so this is visible per session and not only in a test. VERIFIED ON GLASS (.21, GPU idle at 195 MHz of 3090 — slow-clock, the worst case on this card): all 6 `#[ignore]`d GPU tests pass, and all NINE decoded-plane hashes (`ref-dense-{y,cb,cr}`, `ref-chunked-*`, `ref-dense444-*`) are bit-identical to the pre-PW5 base. Decode identity is the meaningful gate here — the raw AU bytes are not reproducible run-to-run even from an unmodified binary, which stage 3's message documents. Gates green at CI parity. --- crates/pf-encode/src/enc/linux/pyrowave.rs | 646 +++++++++++++-------- 1 file changed, 413 insertions(+), 233 deletions(-) diff --git a/crates/pf-encode/src/enc/linux/pyrowave.rs b/crates/pf-encode/src/enc/linux/pyrowave.rs index c66d355f..8a17d05d 100644 --- a/crates/pf-encode/src/enc/linux/pyrowave.rs +++ b/crates/pf-encode/src/enc/linux/pyrowave.rs @@ -396,8 +396,102 @@ fn priority_refused(e: vk::Result) -> bool { /// One frame recorded and queue-submitted, whose fence has not been waited and whose bitstream has /// therefore not been packetized yet. PW5 stage 3: this is what the `submit`/`poll` split created — /// before it, no such state could exist because `submit` did the whole thing inline. +/// How many independent per-frame resource sets the encoder allocates. +/// +/// PW5 stage 4. Two, because Granite caps the overlap at two anyway: the pyrowave device defaults +/// to `init_frame_contexts(2)` and `next_frame_context()` — called at the top of every +/// `encode_gpu_synchronous` — waits the context it rotates into, so "frame N may not begin +/// recording until N-2 completed" is enforced below us. A third slot would buy nothing without a +/// vendored `init_frame_contexts(3)`, which is not exposed. +/// +/// **Allocated is not the same as used.** `max_inflight` decides how many are live at once and is +/// still 1 here; this stage is pure capacity so the depth change that follows is a one-line +/// behaviour change rather than a simultaneous re-plumbing. +const SLOTS: usize = 2; + +/// Everything ONE in-flight frame needs exclusively. +/// +/// PW5 stage 4 exists because the analysis found six single-slot resources beyond the y/uv images +/// the plan named, and every one of them is a correctness problem under overlap — not a +/// performance one: +/// +/// * `csc_set` was ONE descriptor set rewritten every frame by `bind_rgb`. Updating a descriptor +/// set still bound by a PENDING command buffer is a spec violation +/// (VUID-vkUpdateDescriptorSets-None-03047), and the kind that produces a wrong picture rather +/// than a validation error on most drivers. +/// * `y_img`/`uv_img` — the CSC of N+1 storage-writes exactly the images pyrowave is still +/// sampling for N. The old barrier comment ("the previous frame's encode already completed under +/// our synchronous fence") was load-bearing and said so. +/// * `cursor_img`/`cursor_stage` — the struct comment said it plainly: *"Single (not ring) because +/// PyroWave encodes one frame synchronously — no in-flight overlap to race."* A new cursor +/// bitmap's host write + copy races N's sampled read. +/// * `cmd` + `fence` — you cannot record into a PENDING command buffer at all. +/// * `cpu_img`/`cpu_stage` (software capture / tests) — the host writes staging while N's copy is +/// still pending. +/// +/// `bitstream` and `import_cache` are deliberately NOT here. `bitstream` is only touched during +/// packetize, i.e. only on the poll side, one frame at a time. `import_cache` retains the +/// `VkImage`/`VkDeviceMemory` per dmabuf inode, so dropping a `CapturedFrame` (and its dup'd fd) +/// while the GPU still reads it is not a use-after-free — do not "optimise" that retention away. +struct Slot { + cmd: vk::CommandBuffer, + fence: vk::Fence, + csc_set: vk::DescriptorSet, + y_img: vk::Image, + y_mem: vk::DeviceMemory, + y_view: vk::ImageView, + uv_img: vk::Image, + uv_mem: vk::DeviceMemory, + uv_view: vk::ImageView, + cursor_img: vk::Image, + cursor_mem: vk::DeviceMemory, + cursor_view: vk::ImageView, + cursor_stage: vk::Buffer, + cursor_stage_mem: vk::DeviceMemory, + /// Per-slot: each slot's cursor image is its own, so a bitmap change is uploaded once per slot + /// (`SLOTS` small uploads instead of one) rather than once globally, which would leave the + /// other slot showing the previous pointer. + cursor_serial: u64, + cursor_ready: bool, + /// CPU-input staging (software capture / smoke tests), lazily (re)created on format change. + cpu_img: Option<(vk::Image, vk::DeviceMemory, vk::ImageView, vk::Format)>, + cpu_stage: Option<(vk::Buffer, vk::DeviceMemory, u64)>, +} + +impl Slot { + /// All-null, matching `open_inner`'s "construct then fill" unwind discipline: every + /// `vkDestroy*`/`vkFree*` of `VK_NULL_HANDLE` is the spec-defined no-op, so `Drop` running on a + /// partially-built slot is sound. + fn null() -> Self { + Self { + cmd: vk::CommandBuffer::null(), + fence: vk::Fence::null(), + csc_set: vk::DescriptorSet::null(), + y_img: vk::Image::null(), + y_mem: vk::DeviceMemory::null(), + y_view: vk::ImageView::null(), + uv_img: vk::Image::null(), + uv_mem: vk::DeviceMemory::null(), + uv_view: vk::ImageView::null(), + cursor_img: vk::Image::null(), + cursor_mem: vk::DeviceMemory::null(), + cursor_view: vk::ImageView::null(), + cursor_stage: vk::Buffer::null(), + cursor_stage_mem: vk::DeviceMemory::null(), + cursor_serial: u64::MAX, + cursor_ready: false, + cpu_img: None, + cpu_stage: None, + } + } +} + #[derive(Clone, Copy)] struct InFlight { + /// Which [`Slot`] this frame's command buffer, fence, descriptor set and images belong to. + /// Carried per-frame rather than recomputed so `wait_and_packetize` cannot wait the wrong + /// fence — the failure that would look like corruption, not like an error. + slot: usize, /// The capture timestamp this frame's AU must carry. Held here rather than re-read from the /// `CapturedFrame` because the frame is the CALLER's and is gone by the time we packetize. pts_ns: u64, @@ -437,52 +531,39 @@ pub struct PyroWaveEncoder { pw_dev: pw::pyrowave_device, pw_enc: pw::pyrowave_encoder, - // --- CSC + planes (single slot: encode is synchronous per frame) --- + // --- CSC pipeline + sampler: SHARED by every slot (immutable once built, read-only in + // recording, so no overlap hazard). The per-frame resources live in `slots`. --- csc_pipe: vk::Pipeline, csc_layout: vk::PipelineLayout, csc_dsl: vk::DescriptorSetLayout, csc_pool: vk::DescriptorPool, - csc_set: vk::DescriptorSet, sampler: vk::Sampler, - y_img: vk::Image, - y_mem: vk::DeviceMemory, - y_view: vk::ImageView, - uv_img: vk::Image, - uv_mem: vk::DeviceMemory, - uv_view: vk::ImageView, - - // Cursor overlay (cursor-as-metadata): a fixed CURSOR_MAX² RGBA8 sampled image (bound at binding - // 3) + host staging, re-uploaded only when the bitmap changes (`cursor_serial`). Single (not - // ring) because PyroWave encodes one frame synchronously — no in-flight overlap to race. - cursor_img: vk::Image, - cursor_mem: vk::DeviceMemory, - cursor_view: vk::ImageView, - cursor_stage: vk::Buffer, - cursor_stage_mem: vk::DeviceMemory, - cursor_serial: u64, - cursor_ready: bool, // Per-buffer dmabuf-import cache keyed by (st_dev, st_ino) — mirrors `vulkan_video.rs`. + // NOT per-slot: it retains the VkImage/VkDeviceMemory per inode, which is exactly what makes + // it safe for two slots to sample the same imported buffer. import_cache: Vec<(u64, u64, vk::Image, vk::DeviceMemory, vk::ImageView)>, - // CPU-input staging (software capture / smoke tests), lazily (re)created on format change. - cpu_img: Option<(vk::Image, vk::DeviceMemory, vk::ImageView, vk::Format)>, - cpu_stage: Option<(vk::Buffer, vk::DeviceMemory, u64)>, /// Reused 3→4 expansion buffer for 24-bpp CPU payloads (`vk_util::normalize_cpu_rgb`). + /// Not per-slot: it is consumed synchronously inside `submit_frame` (copied into staging + /// before the call returns), so no GPU work ever reads it. cpu_expand: Vec, cmd_pool: vk::CommandPool, - cmd: vk::CommandBuffer, - fence: vk::Fence, + /// The `SLOTS` independent per-frame resource sets — see [`Slot`] for why each member is in + /// there and why `bitstream`/`import_cache` are not. + slots: Vec, + /// Which slot the NEXT submit records into; advances modulo `SLOTS` per submitted frame. + next_slot: usize, /// Frames recorded and queue-submitted whose fence has not been waited yet — i.e. exactly the /// work that may still be executing on the GPU. `reset()` keys its bounded wait on this being /// non-empty: a never-submitted fence would otherwise read as "wedged" (fences start /// unsignaled). At today's depth of 1 this holds at most one entry. inflight: VecDeque, - /// How many frames may be submitted-but-not-polled at once. **1 today, and the only value the - /// resources below can support** — `cmd`, `fence`, `csc_set` and the y/uv images are one each, - /// so a second concurrent frame would record into a PENDING command buffer and storage-write - /// the images pyrowave is still sampling. PW5 stages 4-6 raise this; until then the split - /// exists so the fence wait is on the POLL side, where every other backend already has it. + /// How many frames may be submitted-but-not-polled at once. **Still 1**, even though stage 4 + /// allocated `SLOTS` resource sets: raising it is stage 6's job and needs the second pyrowave + /// encoder handle (stage 5) first — pyrowave's own `Encoder` object structurally cannot hold + /// two frames (single wavelet/scratch buffers, and `Impl::encode` opens by discarding them + /// with an UNDEFINED old layout). Never exceeds `SLOTS`. max_inflight: usize, // --- state --- @@ -887,30 +968,14 @@ impl PyroWaveEncoder { csc_layout: vk::PipelineLayout::null(), csc_dsl: vk::DescriptorSetLayout::null(), csc_pool: vk::DescriptorPool::null(), - csc_set: vk::DescriptorSet::null(), sampler: vk::Sampler::null(), - y_img: vk::Image::null(), - y_mem: vk::DeviceMemory::null(), - y_view: vk::ImageView::null(), - uv_img: vk::Image::null(), - uv_mem: vk::DeviceMemory::null(), - uv_view: vk::ImageView::null(), - cursor_img: vk::Image::null(), - cursor_mem: vk::DeviceMemory::null(), - cursor_view: vk::ImageView::null(), - cursor_stage: vk::Buffer::null(), - cursor_stage_mem: vk::DeviceMemory::null(), - cursor_serial: u64::MAX, - cursor_ready: false, import_cache: Vec::new(), - cpu_img: None, - cpu_stage: None, cpu_expand: Vec::new(), cmd_pool: vk::CommandPool::null(), - cmd: vk::CommandBuffer::null(), - fence: vk::Fence::null(), + slots: (0..SLOTS).map(|_| Slot::null()).collect(), + next_slot: 0, inflight: VecDeque::new(), - // PW5: depth 1. Stages 4-6 raise it; the per-slot resources above cannot support 2. + // PW5: depth 1 still. Stage 4 allocates the capacity; stage 6 spends it. max_inflight: 1, width: w, height: h, @@ -987,28 +1052,6 @@ impl PyroWaveEncoder { // swizzles synthesize Cb/Cr) ---- let device = me.device.clone(); // cheap fn-table clone; lets `me.*` assignments interleave let (cw, ch) = if chroma444 { (w, h) } else { (w / 2, h / 2) }; - let (y_img, y_mem, y_view) = make_plain_image( - &device, - &me.mem_props, - vk::Format::R8_UNORM, - w, - h, - vk::ImageUsageFlags::STORAGE | vk::ImageUsageFlags::SAMPLED, - )?; - me.y_img = y_img; - me.y_mem = y_mem; - me.y_view = y_view; - let (uv_img, uv_mem, uv_view) = make_plain_image( - &device, - &me.mem_props, - vk::Format::R8G8_UNORM, - cw, - ch, - vk::ImageUsageFlags::STORAGE | vk::ImageUsageFlags::SAMPLED, - )?; - me.uv_img = uv_img; - me.uv_mem = uv_mem; - me.uv_view = uv_view; // ---- CSC compute pipeline (same shader + layout as vulkan_video.rs) ---- me.sampler = device.create_sampler( @@ -1073,91 +1116,140 @@ impl PyroWaveEncoder { device.destroy_shader_module(shader, None); me.csc_pipe = pipe_res.map_err(|(_, e)| e)?[0]; + // Pool sized for ALL slots: 2 combined-image-samplers (binding 0 RGB + binding 3 cursor) + // and 2 storage images (Y, UV) per set. let pool_sizes = [ vk::DescriptorPoolSize::default() .ty(vk::DescriptorType::COMBINED_IMAGE_SAMPLER) - // binding 0 (RGB) + binding 3 (cursor). - .descriptor_count(2), + .descriptor_count(2 * SLOTS as u32), vk::DescriptorPoolSize::default() .ty(vk::DescriptorType::STORAGE_IMAGE) - .descriptor_count(2), + .descriptor_count(2 * SLOTS as u32), ]; me.csc_pool = device.create_descriptor_pool( &vk::DescriptorPoolCreateInfo::default() - .max_sets(1) + .max_sets(SLOTS as u32) .pool_sizes(&pool_sizes), None, )?; - me.csc_set = device.allocate_descriptor_sets( - &vk::DescriptorSetAllocateInfo::default() - .descriptor_pool(me.csc_pool) - .set_layouts(&dsls), - )?[0]; - // Cursor overlay: fixed CURSOR_MAX² RGBA8 sampled image + host staging (bound at binding 3). - let (cursor_img, cursor_mem, cursor_view) = make_plain_image( - &device, - &me.mem_props, - vk::Format::R8G8B8A8_UNORM, - CURSOR_MAX, - CURSOR_MAX, - vk::ImageUsageFlags::SAMPLED | vk::ImageUsageFlags::TRANSFER_DST, - )?; - me.cursor_img = cursor_img; - me.cursor_mem = cursor_mem; - me.cursor_view = cursor_view; - let (cursor_stage, cursor_stage_mem) = make_host_buffer( - &device, - &me.mem_props, - (CURSOR_MAX * CURSOR_MAX * 4) as u64, - vk::BufferUsageFlags::TRANSFER_SRC, - )?; - me.cursor_stage = cursor_stage; - me.cursor_stage_mem = cursor_stage_mem; - // Bindings 1/2 (Y, UV storage targets) + 3 (cursor sampler) are fixed for the encoder's life. - let yi = [vk::DescriptorImageInfo::default() - .image_view(me.y_view) - .image_layout(vk::ImageLayout::GENERAL)]; - let uvi = [vk::DescriptorImageInfo::default() - .image_view(me.uv_view) - .image_layout(vk::ImageLayout::GENERAL)]; - let curi = [vk::DescriptorImageInfo::default() - .sampler(me.sampler) - .image_view(me.cursor_view) - .image_layout(vk::ImageLayout::SHADER_READ_ONLY_OPTIMAL)]; - device.update_descriptor_sets( - &[ - vk::WriteDescriptorSet::default() - .dst_set(me.csc_set) - .dst_binding(1) - .descriptor_type(vk::DescriptorType::STORAGE_IMAGE) - .image_info(&yi), - vk::WriteDescriptorSet::default() - .dst_set(me.csc_set) - .dst_binding(2) - .descriptor_type(vk::DescriptorType::STORAGE_IMAGE) - .image_info(&uvi), - vk::WriteDescriptorSet::default() - .dst_set(me.csc_set) - .dst_binding(3) - .descriptor_type(vk::DescriptorType::COMBINED_IMAGE_SAMPLER) - .image_info(&curi), - ], - &[], - ); - me.cmd_pool = device.create_command_pool( &vk::CommandPoolCreateInfo::default() .queue_family_index(family) .flags(vk::CommandPoolCreateFlags::RESET_COMMAND_BUFFER), None, )?; - me.cmd = device.allocate_command_buffers( - &vk::CommandBufferAllocateInfo::default() - .command_pool(me.cmd_pool) - .level(vk::CommandBufferLevel::PRIMARY) - .command_buffer_count(1), - )?[0]; - me.fence = device.create_fence(&vk::FenceCreateInfo::default(), None)?; + + // ---- the per-frame resource sets (PW5 stage 4) ---- + // Each iteration builds ONE complete `Slot` and assigns as it goes, so a failure part-way + // leaves the earlier slots fully formed and the rest null — which `Drop` handles, since + // every `vkDestroy*` of VK_NULL_HANDLE is the spec-defined no-op. + for i in 0..SLOTS { + let (y_img, y_mem, y_view) = make_plain_image( + &device, + &me.mem_props, + vk::Format::R8_UNORM, + w, + h, + vk::ImageUsageFlags::STORAGE | vk::ImageUsageFlags::SAMPLED, + )?; + me.slots[i].y_img = y_img; + me.slots[i].y_mem = y_mem; + me.slots[i].y_view = y_view; + let (uv_img, uv_mem, uv_view) = make_plain_image( + &device, + &me.mem_props, + vk::Format::R8G8_UNORM, + cw, + ch, + vk::ImageUsageFlags::STORAGE | vk::ImageUsageFlags::SAMPLED, + )?; + me.slots[i].uv_img = uv_img; + me.slots[i].uv_mem = uv_mem; + me.slots[i].uv_view = uv_view; + // Cursor overlay: fixed CURSOR_MAX² RGBA8 sampled image + host staging (binding 3). + let (cursor_img, cursor_mem, cursor_view) = make_plain_image( + &device, + &me.mem_props, + vk::Format::R8G8B8A8_UNORM, + CURSOR_MAX, + CURSOR_MAX, + vk::ImageUsageFlags::SAMPLED | vk::ImageUsageFlags::TRANSFER_DST, + )?; + me.slots[i].cursor_img = cursor_img; + me.slots[i].cursor_mem = cursor_mem; + me.slots[i].cursor_view = cursor_view; + let (cursor_stage, cursor_stage_mem) = make_host_buffer( + &device, + &me.mem_props, + (CURSOR_MAX * CURSOR_MAX * 4) as u64, + vk::BufferUsageFlags::TRANSFER_SRC, + )?; + me.slots[i].cursor_stage = cursor_stage; + me.slots[i].cursor_stage_mem = cursor_stage_mem; + let csc_set = device.allocate_descriptor_sets( + &vk::DescriptorSetAllocateInfo::default() + .descriptor_pool(me.csc_pool) + .set_layouts(&dsls), + )?[0]; + me.slots[i].csc_set = csc_set; + // Bindings 1/2 (Y, UV storage targets) + 3 (cursor sampler) are fixed for the slot's + // life; only binding 0 (the frame's RGB view) is rewritten per frame, by `bind_rgb`, + // and THAT is why each slot needs its own set — see `Slot`. + let yi = [vk::DescriptorImageInfo::default() + .image_view(y_view) + .image_layout(vk::ImageLayout::GENERAL)]; + let uvi = [vk::DescriptorImageInfo::default() + .image_view(uv_view) + .image_layout(vk::ImageLayout::GENERAL)]; + let curi = [vk::DescriptorImageInfo::default() + .sampler(me.sampler) + .image_view(cursor_view) + .image_layout(vk::ImageLayout::SHADER_READ_ONLY_OPTIMAL)]; + device.update_descriptor_sets( + &[ + vk::WriteDescriptorSet::default() + .dst_set(csc_set) + .dst_binding(1) + .descriptor_type(vk::DescriptorType::STORAGE_IMAGE) + .image_info(&yi), + vk::WriteDescriptorSet::default() + .dst_set(csc_set) + .dst_binding(2) + .descriptor_type(vk::DescriptorType::STORAGE_IMAGE) + .image_info(&uvi), + vk::WriteDescriptorSet::default() + .dst_set(csc_set) + .dst_binding(3) + .descriptor_type(vk::DescriptorType::COMBINED_IMAGE_SAMPLER) + .image_info(&curi), + ], + &[], + ); + me.slots[i].cmd = device.allocate_command_buffers( + &vk::CommandBufferAllocateInfo::default() + .command_pool(me.cmd_pool) + .level(vk::CommandBufferLevel::PRIMARY) + .command_buffer_count(1), + )?[0]; + me.slots[i].fence = device.create_fence(&vk::FenceCreateInfo::default(), None)?; + } + + // What the extra slot actually COST, measured from the driver's own requirements rather + // than estimated from the dimensions — the plan's estimate is not evidence, and on an iGPU + // at 4K/4:4:4 this is the number that decides whether the capacity is affordable. The + // per-frame CPU staging is excluded because it is allocated lazily and only on the + // software-capture path. + let slot_bytes: u64 = [ + me.slots[0].y_img, + me.slots[0].uv_img, + me.slots[0].cursor_img, + ] + .iter() + .map(|&i| device.get_image_memory_requirements(i).size) + .sum::() + + device + .get_buffer_memory_requirements(me.slots[0].cursor_stage) + .size; let props = me.instance.get_physical_device_properties(pd); tracing::info!( @@ -1165,21 +1257,29 @@ impl PyroWaveEncoder { mode = %format!("{w}x{h}@{fps}"), budget_kib = me.frame_budget / 1024, chroma = if chroma444 { "4:4:4" } else { "4:2:0" }, + slots = SLOTS, + slot_kib = slot_bytes / 1024, + slots_kib = slot_bytes * SLOTS as u64 / 1024, "PyroWave encoder open (intra-only wavelet, BT.709 limited)" ); Ok(me) } - /// Point CSC binding 0 at this frame's RGB view. - unsafe fn bind_rgb(&self, rgb_view: vk::ImageView) { + /// Point slot `slot`'s CSC binding 0 at this frame's RGB view. + /// + /// ⚠ This is the `vkUpdateDescriptorSets` the analysis flagged: writing a set that is still + /// bound by a PENDING command buffer violates VUID-vkUpdateDescriptorSets-None-03047. It is + /// safe because the set belongs to the slot we are about to record into, and that slot's + /// previous frame was retired before `submit` chose it. + unsafe fn bind_rgb(&self, slot: usize, rgb_view: vk::ImageView) { let ii = [vk::DescriptorImageInfo::default() .sampler(self.sampler) .image_view(rgb_view) .image_layout(vk::ImageLayout::SHADER_READ_ONLY_OPTIMAL)]; self.device.update_descriptor_sets( &[vk::WriteDescriptorSet::default() - .dst_set(self.csc_set) + .dst_set(self.slots[slot].csc_set) .dst_binding(0) .descriptor_type(vk::DescriptorType::COMBINED_IMAGE_SAMPLER) .image_info(&ii)], @@ -1190,13 +1290,22 @@ impl PyroWaveEncoder { /// Cursor-as-metadata: bring the cursor image up to date for this frame and return the shader /// push constant `[origin_x, origin_y, size_w, size_h]` (size 0 ⇒ the CSC skips the blend). /// Records the small upload (only when the bitmap `serial` changed) + layout transition into - /// `cmd`, ahead of the CSC dispatch that samples binding 3. Encode is synchronous, so the single - /// shared image never races a prior frame; the first use transitions it to SHADER_READ_ONLY. - unsafe fn prep_cursor(&mut self, cursor: Option<&pf_frame::CursorOverlay>) -> Result<[i32; 4]> { + /// slot `slot`'s command buffer, ahead of the CSC dispatch that samples binding 3. + /// + /// PER SLOT since PW5 stage 4 — image, staging buffer and `cursor_serial` all. The old comment + /// said it outright: a single shared image was only safe because there was no in-flight + /// overlap to race. The cost of per-slot is that a changed bitmap uploads once per slot. + unsafe fn prep_cursor( + &mut self, + slot: usize, + cursor: Option<&pf_frame::CursorOverlay>, + ) -> Result<[i32; 4]> { let dev = self.device.clone(); - let cmd = self.cmd; - let img = self.cursor_img; - let ready = self.cursor_ready; + let cmd = self.slots[slot].cmd; + let img = self.slots[slot].cursor_img; + let stage = self.slots[slot].cursor_stage; + let stage_mem = self.slots[slot].cursor_stage_mem; + let ready = self.slots[slot].cursor_ready; let barrier = |old: vk::ImageLayout, new: vk::ImageLayout, ss, sa, ds, da| { vk::ImageMemoryBarrier2::default() .src_stage_mask(ss) @@ -1214,20 +1323,16 @@ impl PyroWaveEncoder { Some(c) if !c.rgba.is_empty() => { let cw = c.w.min(CURSOR_MAX); let ch = c.h.min(CURSOR_MAX); - if self.cursor_serial != c.serial { + if self.slots[slot].cursor_serial != c.serial { let bytes = (cw as usize) * (ch as usize) * 4; - let ptr = dev.map_memory( - self.cursor_stage_mem, - 0, - bytes as u64, - vk::MemoryMapFlags::empty(), - )?; + let ptr = + dev.map_memory(stage_mem, 0, bytes as u64, vk::MemoryMapFlags::empty())?; std::ptr::copy_nonoverlapping( c.rgba.as_ptr(), ptr as *mut u8, bytes.min(c.rgba.len()), ); - dev.unmap_memory(self.cursor_stage_mem); + dev.unmap_memory(stage_mem); let old = if ready { vk::ImageLayout::SHADER_READ_ONLY_OPTIMAL } else { @@ -1246,7 +1351,7 @@ impl PyroWaveEncoder { ); dev.cmd_copy_buffer_to_image( cmd, - self.cursor_stage, + stage, img, vk::ImageLayout::TRANSFER_DST_OPTIMAL, &[vk::BufferImageCopy::default() @@ -1272,8 +1377,8 @@ impl PyroWaveEncoder { vk::AccessFlags2::SHADER_READ, )]), ); - self.cursor_serial = c.serial; - self.cursor_ready = true; + self.slots[slot].cursor_serial = c.serial; + self.slots[slot].cursor_ready = true; } Ok([c.x, c.y, cw as i32, ch as i32]) } @@ -1290,7 +1395,7 @@ impl PyroWaveEncoder { vk::AccessFlags2::SHADER_READ, )]), ); - self.cursor_ready = true; + self.slots[slot].cursor_ready = true; } Ok([0, 0, 0, 0]) } @@ -1347,12 +1452,20 @@ impl PyroWaveEncoder { } /// CPU RGB staging (software capture / smoke tests) — mirrors `vulkan_video.rs::ensure_cpu_rgb`. - unsafe fn ensure_cpu_rgb(&mut self, fmt: vk::Format, bytes: &[u8]) -> Result { + /// + /// PER SLOT since PW5 stage 4: the host writes this staging buffer, so writing it while a + /// previous frame's buffer-to-image copy is still pending would race that copy. + unsafe fn ensure_cpu_rgb( + &mut self, + slot: usize, + fmt: vk::Format, + bytes: &[u8], + ) -> Result { let dev = self.device.clone(); let (w, h) = (self.width, self.height); let need = (w * h * 4) as u64; - if self.cpu_img.map(|(_, _, _, f)| f) != Some(fmt) { - if let Some((i, m, v, _)) = self.cpu_img.take() { + if self.slots[slot].cpu_img.map(|(_, _, _, f)| f) != Some(fmt) { + if let Some((i, m, v, _)) = self.slots[slot].cpu_img.take() { dev.destroy_image_view(v, None); dev.destroy_image(i, None); dev.free_memory(m, None); @@ -1365,10 +1478,14 @@ impl PyroWaveEncoder { h, vk::ImageUsageFlags::SAMPLED | vk::ImageUsageFlags::TRANSFER_DST, )?; - self.cpu_img = Some((i, m, v, fmt)); + self.slots[slot].cpu_img = Some((i, m, v, fmt)); } - if self.cpu_stage.map(|(_, _, s)| s < need).unwrap_or(true) { - if let Some((b, m, _)) = self.cpu_stage.take() { + if self.slots[slot] + .cpu_stage + .map(|(_, _, s)| s < need) + .unwrap_or(true) + { + if let Some((b, m, _)) = self.slots[slot].cpu_stage.take() { dev.destroy_buffer(b, None); dev.free_memory(m, None); } @@ -1378,14 +1495,14 @@ impl PyroWaveEncoder { need, vk::BufferUsageFlags::TRANSFER_SRC, )?; - self.cpu_stage = Some((buf, mem, need)); + self.slots[slot].cpu_stage = Some((buf, mem, need)); } - let (_, m, _) = self.cpu_stage.unwrap(); + let (_, m, _) = self.slots[slot].cpu_stage.unwrap(); let p = dev.map_memory(m, 0, vk::WHOLE_SIZE, vk::MemoryMapFlags::empty())? as *mut u8; let n = bytes.len().min(need as usize); std::ptr::copy_nonoverlapping(bytes.as_ptr(), p, n); dev.unmap_memory(m); - Ok(self.cpu_img.unwrap().2) + Ok(self.slots[slot].cpu_img.unwrap().2) } /// The per-frame budget handed to pyrowave rate control: `frame_budget`, deflated by the @@ -1460,16 +1577,22 @@ impl PyroWaveEncoder { // which the next `begin` may implicitly reset. // Resolved before the closure (which borrows `self` mutably for the recording calls). let rate_budget = self.rate_budget(); + // THE slot this frame owns for its whole life — command buffer, fence, descriptor set, + // y/uv images, cursor image and CPU staging (PW5 stage 4). `submit` guaranteed it is free + // by draining to `max_inflight - 1` before calling here. + let slot = self.next_slot; + let cmd = self.slots[slot].cmd; + let fence = self.slots[slot].fence; let record_and_submit = (|| -> Result<()> { dev.begin_command_buffer( - self.cmd, + cmd, &vk::CommandBufferBeginInfo::default() .flags(vk::CommandBufferUsageFlags::ONE_TIME_SUBMIT), )?; // Cursor-as-metadata: refresh the cursor image (only when the bitmap changed) + get the // shader push constant. Recorded into `self.cmd` before the CSC dispatch samples binding 3. - let cursor_pc = self.prep_cursor(frame.cursor.as_ref())?; + let cursor_pc = self.prep_cursor(slot, frame.cursor.as_ref())?; // ---- ingest RGB (same barrier discipline as vulkan_video.rs) ---- let rgb_view = match &frame.payload { @@ -1496,7 +1619,7 @@ impl PyroWaveEncoder { .image(img) .subresource_range(color_range(0)); dev.cmd_pipeline_barrier2( - self.cmd, + cmd, &vk::DependencyInfo::default().image_memory_barriers(&[acq]), ); view @@ -1509,13 +1632,13 @@ impl PyroWaveEncoder { normalize_cpu_rgb(frame.format, bytes, &mut scratch, false); let fmt = pixel_to_vk(norm_fmt).context("unsupported CPU pixel format"); let view = match fmt { - Ok(f) => self.ensure_cpu_rgb(f, norm_bytes), + Ok(f) => self.ensure_cpu_rgb(slot, f, norm_bytes), Err(e) => Err(e), }; self.cpu_expand = scratch; let view = view?; - let (img, ..) = self.cpu_img.unwrap(); - let (stage, ..) = self.cpu_stage.unwrap(); + let (img, ..) = self.slots[slot].cpu_img.unwrap(); + let (stage, ..) = self.slots[slot].cpu_stage.unwrap(); let to_dst = vk::ImageMemoryBarrier2::default() .src_stage_mask(vk::PipelineStageFlags2::NONE) .src_access_mask(vk::AccessFlags2::NONE) @@ -1526,11 +1649,11 @@ impl PyroWaveEncoder { .image(img) .subresource_range(color_range(0)); dev.cmd_pipeline_barrier2( - self.cmd, + cmd, &vk::DependencyInfo::default().image_memory_barriers(&[to_dst]), ); dev.cmd_copy_buffer_to_image( - self.cmd, + cmd, stage, img, vk::ImageLayout::TRANSFER_DST_OPTIMAL, @@ -1556,18 +1679,19 @@ impl PyroWaveEncoder { .image(img) .subresource_range(color_range(0)); dev.cmd_pipeline_barrier2( - self.cmd, + cmd, &vk::DependencyInfo::default().image_memory_barriers(&[to_read]), ); view } _ => bail!("pyrowave: unsupported FramePayload (need Dmabuf or Cpu RGB)"), }; - self.bind_rgb(rgb_view); + self.bind_rgb(slot, rgb_view); - // y/uv -> GENERAL for the CSC's storage writes (discard prior contents — the previous - // frame's encode already completed under our synchronous fence, which is also the - // "execution barrier before writing to images" pyrowave's contract asks for). + // y/uv -> GENERAL for the CSC's storage writes (discard prior contents — this SLOT's + // previous frame was retired before `submit` chose it, which is also the "execution + // barrier before writing to images" pyrowave's contract asks for). + let (y_img, uv_img) = (self.slots[slot].y_img, self.slots[slot].uv_img); let to_general = |img| { vk::ImageMemoryBarrier2::default() .src_stage_mask(vk::PipelineStageFlags2::NONE) @@ -1580,17 +1704,17 @@ impl PyroWaveEncoder { .subresource_range(color_range(0)) }; dev.cmd_pipeline_barrier2( - self.cmd, + cmd, &vk::DependencyInfo::default() - .image_memory_barriers(&[to_general(self.y_img), to_general(self.uv_img)]), + .image_memory_barriers(&[to_general(y_img), to_general(uv_img)]), ); - dev.cmd_bind_pipeline(self.cmd, vk::PipelineBindPoint::COMPUTE, self.csc_pipe); + dev.cmd_bind_pipeline(cmd, vk::PipelineBindPoint::COMPUTE, self.csc_pipe); dev.cmd_bind_descriptor_sets( - self.cmd, + cmd, vk::PipelineBindPoint::COMPUTE, self.csc_layout, 0, - &[self.csc_set], + &[self.slots[slot].csc_set], &[], ); let mut pc_bytes = [0u8; 16]; @@ -1598,7 +1722,7 @@ impl PyroWaveEncoder { pc_bytes[i * 4..i * 4 + 4].copy_from_slice(&v.to_ne_bytes()); } dev.cmd_push_constants( - self.cmd, + cmd, self.csc_layout, vk::ShaderStageFlags::COMPUTE, 0, @@ -1606,9 +1730,9 @@ impl PyroWaveEncoder { ); // 4:2:0: one invocation per 2x2 luma block (per chroma sample); 4:4:4: per pixel. if self.chroma444 { - dev.cmd_dispatch(self.cmd, w.div_ceil(8), h.div_ceil(8), 1); + dev.cmd_dispatch(cmd, w.div_ceil(8), h.div_ceil(8), 1); } else { - dev.cmd_dispatch(self.cmd, (w / 2).div_ceil(8), (h / 2).div_ceil(8), 1); + dev.cmd_dispatch(cmd, (w / 2).div_ceil(8), (h / 2).div_ceil(8), 1); } // CSC storage writes -> pyrowave's sampled reads (images stay GENERAL — the layout @@ -1625,9 +1749,9 @@ impl PyroWaveEncoder { .subresource_range(color_range(0)) }; dev.cmd_pipeline_barrier2( - self.cmd, + cmd, &vk::DependencyInfo::default() - .image_memory_barriers(&[to_sampled(self.y_img), to_sampled(self.uv_img)]), + .image_memory_barriers(&[to_sampled(y_img), to_sampled(uv_img)]), ); // ---- pyrowave encode, recorded into OUR command buffer ---- @@ -1654,7 +1778,7 @@ impl PyroWaveEncoder { let buffers = pw::pyrowave_gpu_buffers { planes: [ plane( - self.y_img, + y_img, w, h, r8, @@ -1665,14 +1789,14 @@ impl PyroWaveEncoder { // The view extent is the chroma IMAGE's own mip0 extent (it's a separate // image, not a planar aspect): half-res for 4:2:0, full-res for 4:4:4. plane( - self.uv_img, + uv_img, if self.chroma444 { w } else { w / 2 }, if self.chroma444 { h } else { h / 2 }, rg8, pw::VkComponentSwizzle_VK_COMPONENT_SWIZZLE_R, ), plane( - self.uv_img, + uv_img, if self.chroma444 { w } else { w / 2 }, if self.chroma444 { h } else { h / 2 }, rg8, @@ -1685,7 +1809,7 @@ impl PyroWaveEncoder { }; pw::pyrowave_device_set_command_buffer( self.pw_dev, - self.cmd.as_raw() as usize as pw::VkCommandBuffer, + cmd.as_raw() as usize as pw::VkCommandBuffer, ); let enc_res = pw::pyrowave_encoder_encode_gpu_synchronous( self.pw_enc, @@ -1697,25 +1821,27 @@ impl PyroWaveEncoder { pw::pyrowave_device_set_command_buffer(self.pw_dev, std::ptr::null_mut()); pw_check(enc_res, "encode_gpu_synchronous")?; - dev.end_command_buffer(self.cmd)?; - dev.reset_fences(&[self.fence])?; - let cmds = [self.cmd]; + dev.end_command_buffer(cmd)?; + dev.reset_fences(&[fence])?; + let cmds = [cmd]; dev.queue_submit( self.queue, &[vk::SubmitInfo::default().command_buffers(&cmds)], - self.fence, + fence, )?; Ok(()) })(); if let Err(e) = record_and_submit { // SAFETY: on every closure error arm the buffer is RECORDING/INVALID/EXECUTABLE — // never PENDING (nothing was enqueued) — and the pool allows the reset. - let _ = dev.reset_command_buffer(self.cmd, vk::CommandBufferResetFlags::empty()); + let _ = dev.reset_command_buffer(cmd, vk::CommandBufferResetFlags::empty()); return Err(e); } // Submitted: from here the GPU may be executing, and NOTHING may touch `cmd`, the y/uv // images or `csc_set` until this entry is retired by `wait_and_packetize`. + self.next_slot = (slot + 1) % SLOTS; self.inflight.push_back(InFlight { + slot, pts_ns: frame.pts_ns, cap: self.frame_budget + BS_SLACK, wire_chunk: self.wire_chunk, @@ -1736,7 +1862,7 @@ impl PyroWaveEncoder { return Ok(()); }; let dev = self.device.clone(); - dev.wait_for_fences(&[self.fence], true, 5_000_000_000) + dev.wait_for_fences(&[self.slots[fr.slot].fence], true, 5_000_000_000) .context("pyrowave encode fence")?; // Waited and signaled: the command buffer is INVALID (one-time submit), which the next // `begin` may implicitly reset, and the GPU is done with this frame's resources. @@ -1832,11 +1958,11 @@ impl Encoder for PyroWaveEncoder { // here fired on exactly that path. Recovery (`reset()`/`Drop`) waits the device idle // before anything touches `cmd` again. unsafe { - // Make room before recording. At `max_inflight == 1` this retires the previous frame, - // which is what makes the single `cmd`/`fence`/`csc_set`/y/uv set safe to reuse — the - // whole depth-1 invariant, stated in one place. The host loop polls after every submit - // so this is normally a no-op; it is here for callers that do not (the `spike` - // subcommand, the hardware smoke tests). + // Make room before recording. This is THE invariant that makes the slot + // `submit_frame` is about to pick provably free: at most `max_inflight - 1` frames may + // still be in flight, and `max_inflight <= SLOTS`, so the slot `next_slot` points at + // was retired. The host loop polls after every submit so this is normally a no-op; it + // is here for callers that do not (the `spike` subcommand, the hardware smoke tests). self.drain_to(self.max_inflight.saturating_sub(1))?; self.submit_frame(frame, t0) } @@ -1880,13 +2006,16 @@ impl Encoder for PyroWaveEncoder { // report "no in-place rebuild" and let the session surface a real error (`Drop`'s // unbounded idle covers teardown, where blocking on the kernel is acceptable). if !self.inflight.is_empty() { - // SAFETY: waiting this encoder's own fence under `&mut self`. - if unsafe { - self.device - .wait_for_fences(&[self.fence], true, 5_000_000_000) - } - .is_err() - { + // Every in-flight frame's fence, not just the oldest — at depth > 1 there may be + // several, and destroying the pyrowave encoder while ANY of them still executes is a + // use-after-free. + let fences: Vec = self + .inflight + .iter() + .map(|f| self.slots[f.slot].fence) + .collect(); + // SAFETY: waiting this encoder's own fences under `&mut self`. + if unsafe { self.device.wait_for_fences(&fences, true, 5_000_000_000) }.is_err() { tracing::error!( "pyrowave: in-flight encode did not complete within the reset budget — GPU \ or driver wedged; in-place rebuild abandoned" @@ -1993,16 +2122,32 @@ impl Drop for PyroWaveEncoder { self.device.destroy_image(i, None); self.device.free_memory(m, None); } - if let Some((i, m, v, _)) = self.cpu_img.take() { - self.device.destroy_image_view(v, None); - self.device.destroy_image(i, None); - self.device.free_memory(m, None); + // Every slot, in the same all-null-tolerant way (a failed open leaves a partial + // prefix built and the rest null; `vkDestroy*(VK_NULL_HANDLE)` is a spec no-op). + for sl in std::mem::take(&mut self.slots) { + if let Some((i, m, v, _)) = sl.cpu_img { + self.device.destroy_image_view(v, None); + self.device.destroy_image(i, None); + self.device.free_memory(m, None); + } + if let Some((b, m, _)) = sl.cpu_stage { + self.device.destroy_buffer(b, None); + self.device.free_memory(m, None); + } + self.device.destroy_fence(sl.fence, None); + self.device.destroy_image_view(sl.y_view, None); + self.device.destroy_image(sl.y_img, None); + self.device.free_memory(sl.y_mem, None); + self.device.destroy_image_view(sl.uv_view, None); + self.device.destroy_image(sl.uv_img, None); + self.device.free_memory(sl.uv_mem, None); + self.device.destroy_image_view(sl.cursor_view, None); + self.device.destroy_image(sl.cursor_img, None); + self.device.free_memory(sl.cursor_mem, None); + self.device.destroy_buffer(sl.cursor_stage, None); + self.device.free_memory(sl.cursor_stage_mem, None); } - if let Some((b, m, _)) = self.cpu_stage.take() { - self.device.destroy_buffer(b, None); - self.device.free_memory(m, None); - } - self.device.destroy_fence(self.fence, None); + // Command buffers and descriptor sets are freed with their pools. self.device.destroy_command_pool(self.cmd_pool, None); self.device.destroy_descriptor_pool(self.csc_pool, None); self.device.destroy_pipeline(self.csc_pipe, None); @@ -2010,17 +2155,6 @@ impl Drop for PyroWaveEncoder { self.device .destroy_descriptor_set_layout(self.csc_dsl, None); self.device.destroy_sampler(self.sampler, None); - self.device.destroy_image_view(self.y_view, None); - self.device.destroy_image(self.y_img, None); - self.device.free_memory(self.y_mem, None); - self.device.destroy_image_view(self.uv_view, None); - self.device.destroy_image(self.uv_img, None); - self.device.free_memory(self.uv_mem, None); - self.device.destroy_image_view(self.cursor_view, None); - self.device.destroy_image(self.cursor_img, None); - self.device.free_memory(self.cursor_mem, None); - self.device.destroy_buffer(self.cursor_stage, None); - self.device.free_memory(self.cursor_stage_mem, None); self.device.destroy_device(None); self.instance.destroy_instance(None); } @@ -2313,6 +2447,52 @@ mod tests { } } + /// PW5 stage 4: what the extra per-frame resource set actually COSTS in VRAM, at the modes + /// that decide whether it is affordable. Reported from the driver's own memory requirements, + /// not estimated from the dimensions — the plan's ~25-35 MB estimate is a guess, and on an + /// iGPU at 4K/4:4:4 the real number is the one that matters. + /// + /// Prints rather than asserts a threshold: a hard limit here would be a guess about every + /// future GPU. What it DOES assert is that a slot is not free and not absurd, so a refactor + /// that accidentally allocated per-slot copies of something large fails visibly. + #[test] + #[ignore = "needs a real Vulkan 1.3 compute device (run on a GPU host, not the build box)"] + fn slot_vram_cost_is_reported() { + for (w, h, chroma, name) in [ + (1920u32, 1080u32, crate::ChromaFormat::Yuv420, "1080p 4:2:0"), + (3840, 2160, crate::ChromaFormat::Yuv420, "4K 4:2:0"), + (3840, 2160, crate::ChromaFormat::Yuv444, "4K 4:4:4"), + ] { + let enc = PyroWaveEncoder::open(w, h, 60, 40_000_000, chroma).expect("open"); + // SAFETY: plain memory-requirement queries on images this encoder owns. + let per_slot: u64 = unsafe { + [ + enc.slots[0].y_img, + enc.slots[0].uv_img, + enc.slots[0].cursor_img, + ] + .iter() + .map(|&i| enc.device.get_image_memory_requirements(i).size) + .sum::() + + enc + .device + .get_buffer_memory_requirements(enc.slots[0].cursor_stage) + .size + }; + eprintln!( + "{name}: {} KiB per slot, {SLOTS} slots = {} KiB total", + per_slot / 1024, + per_slot * SLOTS as u64 / 1024 + ); + assert!(per_slot > 0, "{name}: a slot must own real memory"); + assert!( + per_slot < 512 * 1024 * 1024, + "{name}: {per_slot} bytes per slot — something large became per-slot that should \ + not have (bitstream? import cache?)" + ); + } + } + /// WP4.5: a frame that is not the session's mode must be REFUSED, not encoded. PyroWave /// applies no alignment, so a mismatch can only be a stale frame from a renegotiated mode — /// and the failure is silent without this check (`rgb2yuv.comp` clamps its fetches and the CPU From 077db416ec2aefeabb6ad3220c1d42b5b4afd8f9 Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Sun, 9 Aug 2026 00:27:28 +0200 Subject: [PATCH 5/6] feat(pf-encode): two PyroWave encoder handles, and the 3-bit landmine that makes them work MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Wave-2 PW5 stage 5. Depth is STILL 1 — the handles alternate per frame, one in flight. PyroWave's `Encoder` cannot hold two frames. Not "probably not" — structurally not. `Encoder::Impl` owns ONE each of `wavelet_img_high_res`, `bucket_buffer`, `meta_buffer`, `block_stat_buffer`, `payload_data` and `quant_buffer`, and `Impl::encode` OPENS by discarding them: an image barrier with `VK_IMAGE_LAYOUT_UNDEFINED` as the old layout — a written promise that nothing else is reading it — plus three `fill_buffer` clears. Two encodes recorded into two command buffers and submitted to one queue have no execution dependency in Vulkan (submission order orders the START, not the completion), so N+1's DWT would overwrite the wavelet bands and zero the RDO buckets while N's block packing still reads them. Content-dependent, silent. So overlap means TWO handles on one device, alternated — one per slot. Every resource above is then private per handle, and within a handle the encodes stay strictly serialized (a slot's next frame is recorded only after that slot's previous one retired), which leaves patch 0004's scratch-pool invariant intact without touching it. THE LANDMINE, and it is the reason this stage is its own commit: `sequence_count` ALSO lives on `Impl`, and it is the 3-bit counter stamped into every block header. Two handles each count 1,2,3... alone, so the wire sees 1,1,2,2,3,3.... The decoder restarts a frame only when the value CHANGES (`diff = (hdr.sequence - last_seq) & 0x7; restart = diff != 0`), so a repeat reads as MORE BLOCKS OF THE SAME FRAME: `clear()` never runs, `decoded_frame_for_current_sequence` stays true, and the second frame of each pair is swallowed. Half frame rate, occasional mixed-frame blocks, no error anywhere — on every client, since pf-client-core and the Apple Metal hand-port parse the same field. `patches/0007-encoder-sequence-override.patch` (new, ~38 lines) exposes `Encoder::set_next_sequence` + a `pyrowave_encoder_set_next_sequence` C entry + a `PYROWAVE_SEQUENCE_MASK` define, so ONE monotonic counter on the Rust side is stamped regardless of which handle encodes. The setter stores `(seq - 1) & mask` because `Impl::encode` pre-increments — its contract is about the next ENCODE, not the next store. Inert when unused, so the whole Windows backend is untouched. No `.def` change: the C API is a static archive. PREDICTED, THEN OBSERVED. A negative control on .21 (the override call removed, nothing else) reads the wire out at exactly: [1, 1, 2, 2, 3, 3, 4, 4, 5, 5, 6, 6, 7, 7, 0, 0, 1, 1, 2, 2] which is the analysis's prediction character for character, and with the override: +1 mod 8, all 20 frames, through the 3-bit wrap. THE GATE, `wire_sequence_increments_across_alternating_handles`, checks three things over 20 frames because any one alone could pass while the stream is broken: the wire counter advances by 1 mod 8; ONE persistent decoder (its `last_seq` carried across every push, exactly like a client's) reports every AU decodable; and consecutive decoded pictures DIFFER. Content moves every frame — and the first run caught a trap in the harness itself rather than the encoder: `test_card` starts its LCG at `seed | 1`, so seeds 2 and 3 build a byte-identical card and the test faked the very repeat it hunts. Odd seeds only now, with the reason written down. A runtime self-check backs the test up where the test cannot reach: after packetize, the stamped sequence is compared against what we asked for, and a mismatch logs once per process naming patch 0007. A re-vendor that loses the patch would not fail to build — it would fail on glass, subtly, and this makes it loud instead. Two byte reads per frame. `reset()` rebuilds both handles and `Drop` destroys both, each with the same null-immediately discipline the single handle had (`pyrowave_encoder_destroy` is a bare `delete` with no null check, so a stale pointer left in the field is a double free). Vendored-patch discipline: patch 0007 re-applies clean to a pristine vendor checkout (verified by stashing the vendor tree and re-applying), and `git diff crates/pyrowave-sys/vendor/` touches exactly the four intended files. VERIFIED ON GLASS (.21, RTX 5070 Ti, GPU idle at 180 MHz of 3090): all 8 `#[ignore]`d GPU tests pass, including the new gate and the 4:2:0 / 4:4:4 / 24-bpp PSNR smokes. Gates green at CI parity. --- crates/pf-encode/src/enc/linux/pyrowave.rs | 269 ++++++++++++++++-- crates/pf-encode/src/enc/pyrowave_wire.rs | 18 ++ .../0007-encoder-sequence-override.patch | 123 ++++++++ .../pyrowave-sys/vendor/pyrowave/pyrowave.h | 13 + .../vendor/pyrowave/pyrowave_c.cpp | 11 + .../vendor/pyrowave/pyrowave_encoder.cpp | 8 + .../vendor/pyrowave/pyrowave_encoder.hpp | 6 + 7 files changed, 417 insertions(+), 31 deletions(-) create mode 100644 crates/pyrowave-sys/patches/0007-encoder-sequence-override.patch diff --git a/crates/pf-encode/src/enc/linux/pyrowave.rs b/crates/pf-encode/src/enc/linux/pyrowave.rs index 8a17d05d..9124cdc6 100644 --- a/crates/pf-encode/src/enc/linux/pyrowave.rs +++ b/crates/pf-encode/src/enc/linux/pyrowave.rs @@ -504,6 +504,9 @@ struct InFlight { /// one packet and the encode would bail with "unexpected packet count" on a frame that was /// perfectly fine. cap: usize, + /// The wire sequence value stamped into this frame's block headers, so the AU can be checked + /// against what we asked for — see the self-check in `wait_and_packetize`. + seq: u8, /// The datagram alignment this frame was encoded for; `set_wire_chunking` can likewise land /// mid-flight, and a frame packetized at a boundary it was not rate-controlled for would ship /// with the wrong `chunk_aligned` flag. @@ -529,7 +532,30 @@ pub struct PyroWaveEncoder { // --- pyrowave (borrows our device; destroyed before it) --- pw_dev: pw::pyrowave_device, - pw_enc: pw::pyrowave_encoder, + /// ONE `pyrowave_encoder` per [`Slot`] (PW5 stage 5), alternated. + /// + /// The object CANNOT hold two frames in flight — not "probably not", structurally not. + /// `Encoder::Impl` owns one each of `wavelet_img_high_res`, `bucket_buffer`, `meta_buffer`, + /// `block_stat_buffer`, `payload_data` and `quant_buffer`, and `Impl::encode` OPENS by + /// discarding them: an image barrier with `VK_IMAGE_LAYOUT_UNDEFINED` as the old layout (a + /// written promise that nothing else is reading it) plus three `fill_buffer` clears. Two + /// encodes recorded into two command buffers and submitted to the same queue have NO execution + /// dependency in Vulkan — submission order orders the start, not the completion — so N+1's DWT + /// would overwrite the bands and zero the RDO buckets while N's block packing still reads them. + /// + /// So overlap means two handles on one device, and within a handle the encodes stay strictly + /// serialized (a slot's next frame is only recorded after that slot's previous one was + /// retired), which keeps patch 0004's scratch-pool invariant intact without touching it. + pw_encs: Vec, + /// The wire sequence counter, kept HERE rather than in the encoder objects. + /// + /// ⚠ pyrowave's own `sequence_count` is PER-ENCODER, so two alternating handles each count + /// 1,2,3… independently and the wire sees 1,1,2,2,3,3…. The decoder restarts a frame only when + /// the value CHANGES (`diff = (hdr.sequence - last_seq) & 0x7; restart = diff != 0`), so a + /// repeat reads as MORE BLOCKS OF THE SAME FRAME: `clear()` never runs and every second frame + /// is silently swallowed, on every client. `patches/0007-encoder-sequence-override.patch` + /// exposes a setter so this single counter is stamped regardless of which handle encodes. + wire_seq: u32, // --- CSC pipeline + sampler: SHARED by every slot (immutable once built, read-only in // recording, so no overlap hazard). The per-frame resources live in `slots`. --- @@ -947,7 +973,7 @@ impl PyroWaveEncoder { // Construct `Self` NOW, every not-yet-created resource at its null value, and assign // into it as resources come up. Any `?` from here drops `me`, and the existing `Drop` // tears down exactly the prefix that exists: it `device_wait_idle()`s first, null-guards - // `pw_enc` (`pyrowave_encoder_destroy` dereferences before deleting), + // `pw_encs` (`pyrowave_encoder_destroy` dereferences before deleting), // `pyrowave_device_destroy(null)` is a plain `delete nullptr` (pyrowave_c.cpp) and // every `vkDestroy*`/`vkFree*` of a VK_NULL_HANDLE is the spec-defined no-op. One // teardown path serves both the error unwind and the normal drop, so an open-path leak @@ -963,7 +989,8 @@ impl PyroWaveEncoder { mem_props, _hold: hold, pw_dev: std::ptr::null_mut(), - pw_enc: std::ptr::null_mut(), + pw_encs: vec![std::ptr::null_mut(); SLOTS], + wire_seq: 0, csc_pipe: vk::Pipeline::null(), csc_layout: vk::PipelineLayout::null(), csc_dsl: vk::DescriptorSetLayout::null(), @@ -1042,10 +1069,12 @@ impl PyroWaveEncoder { pw::pyrowave_chroma_subsampling_PYROWAVE_CHROMA_SUBSAMPLING_420 }, }; - pw_check( - pw::pyrowave_encoder_create(&einfo, &mut me.pw_enc), - "encoder_create", - )?; + for i in 0..SLOTS { + pw_check( + pw::pyrowave_encoder_create(&einfo, &mut me.pw_encs[i]), + "encoder_create", + )?; + } // ---- CSC planes: full-res R8 luma + RG8 chroma (half-res for 4:2:0, full-res for // 4:4:4), storage-written by the CSC and sampled directly by pyrowave (R/G view @@ -1527,7 +1556,7 @@ impl PyroWaveEncoder { // a session error and never resubmit, but a null here would be a use-after-free inside // pyrowave rather than a clean error — so fail loudly instead of relying on that. anyhow::ensure!( - !self.pw_enc.is_null(), + self.pw_encs.iter().all(|e| !e.is_null()), "pyrowave: encode after a failed reset (encoder was destroyed and not rebuilt)" ); let dev = self.device.clone(); @@ -1581,6 +1610,7 @@ impl PyroWaveEncoder { // y/uv images, cursor image and CPU staging (PW5 stage 4). `submit` guaranteed it is free // by draining to `max_inflight - 1` before calling here. let slot = self.next_slot; + let seq = self.wire_seq; let cmd = self.slots[slot].cmd; let fence = self.slots[slot].fence; let record_and_submit = (|| -> Result<()> { @@ -1811,8 +1841,20 @@ impl PyroWaveEncoder { self.pw_dev, cmd.as_raw() as usize as pw::VkCommandBuffer, ); + // ⚠ THE LANDMINE (PW5 stage 5). Stamp OUR monotonic counter before the encode, or + // the two alternating handles emit 1,1,2,2,3,3… and the decoder reads each repeat as + // more blocks of the same frame — half the frames silently swallowed, on every client. + // Needs `patches/0007-encoder-sequence-override.patch`; the round-trip test + // `wire_sequence_increments_across_alternating_handles` is what keeps it honest. + pw_check( + pw::pyrowave_encoder_set_next_sequence( + self.pw_encs[slot], + seq & pw::PYROWAVE_SEQUENCE_MASK, + ), + "set_next_sequence", + )?; let enc_res = pw::pyrowave_encoder_encode_gpu_synchronous( - self.pw_enc, + self.pw_encs[slot], std::ptr::null(), std::ptr::null(), &buffers, @@ -1840,8 +1882,13 @@ impl PyroWaveEncoder { // Submitted: from here the GPU may be executing, and NOTHING may touch `cmd`, the y/uv // images or `csc_set` until this entry is retired by `wait_and_packetize`. self.next_slot = (slot + 1) % SLOTS; + // Advance only on SUCCESS: a frame that never reached the wire must not burn a sequence + // value (a gap reads as a restart, which is right for a DROPPED frame and wrong for one + // that was never emitted at all). + self.wire_seq = self.wire_seq.wrapping_add(1); self.inflight.push_back(InFlight { slot, + seq: (seq & pw::PYROWAVE_SEQUENCE_MASK) as u8, pts_ns: frame.pts_ns, cap: self.frame_budget + BS_SLACK, wire_chunk: self.wire_chunk, @@ -1884,7 +1931,7 @@ impl PyroWaveEncoder { let boundary = crate::pyrowave_wire::packet_boundary(fr.wire_chunk, cap); let mut n: usize = 0; pw_check( - pw::pyrowave_encoder_compute_num_packets(self.pw_enc, boundary, &mut n), + pw::pyrowave_encoder_compute_num_packets(self.pw_encs[fr.slot], boundary, &mut n), "compute_num_packets", )?; if n == 0 || (fr.wire_chunk.is_none() && n != 1) { @@ -1894,7 +1941,7 @@ impl PyroWaveEncoder { let mut out_n: usize = 0; pw_check( pw::pyrowave_encoder_packetize( - self.pw_enc, + self.pw_encs[fr.slot], packets.as_mut_ptr(), boundary, &mut out_n, @@ -1909,6 +1956,27 @@ impl PyroWaveEncoder { // blacks. (Linux capture has no HDR path, so this side never stamps BT.2020/PQ.) if let Some(p) = packets.first() { crate::pyrowave_wire::stamp_color_bits(&mut self.bitstream, p.offset, false); + // Self-check on the ONE thing a dropped vendored patch would break silently. Without + // `0007-encoder-sequence-override.patch` the two handles count independently, the wire + // reads 1,1,2,2,3,3..., and every client's decoder folds each repeated value into the + // previous frame — half the frames gone, no error anywhere. A re-vendor that loses the + // patch would not fail to build; it would fail on glass, subtly. Two byte reads per + // frame to make that loud instead. Once per process: if it is wrong it is wrong for + // every frame. + if crate::pyrowave_wire::wire_sequence(&self.bitstream, p.offset) != Some(fr.seq) { + static WARNED: std::sync::atomic::AtomicBool = + std::sync::atomic::AtomicBool::new(false); + if !WARNED.swap(true, std::sync::atomic::Ordering::Relaxed) { + tracing::error!( + expected = fr.seq, + got = ?crate::pyrowave_wire::wire_sequence(&self.bitstream, p.offset), + "pyrowave: the wire sequence counter is NOT what we stamped — \ + patches/0007-encoder-sequence-override.patch is missing or ineffective. \ + With two alternating encoder handles this silently halves the frame rate \ + on every client" + ); + } + } } // Frame into the wire AU via the shared helper (byte-identical on Linux + Windows): the dense // single packet, or the datagram-aligned windowed AU (§4.4). @@ -2032,13 +2100,6 @@ impl Encoder for PyroWaveEncoder { // object being swapped. unsafe { self.device.device_wait_idle().ok(); - pw::pyrowave_encoder_destroy(self.pw_enc); - // Publish the null IMMEDIATELY: the create below is fallible, and its failure path - // must not leave a freed pointer in the field. `pyrowave_encoder_destroy` is a plain - // `delete` (pyrowave_c.cpp) with no null check, so `Drop` running on a stale handle - // is a double free — the exact shape this reset hits when the rebuild fails because - // the device is already lost, which is the state that made the watchdog fire. - self.pw_enc = std::ptr::null_mut(); let einfo = pw::pyrowave_encoder_create_info { device: self.pw_dev, width: self.width as i32, @@ -2049,17 +2110,32 @@ impl Encoder for PyroWaveEncoder { pw::pyrowave_chroma_subsampling_PYROWAVE_CHROMA_SUBSAMPLING_420 }, }; - let mut enc: pw::pyrowave_encoder = std::ptr::null_mut(); - let r = pw::pyrowave_encoder_create(&einfo, &mut enc); - if r != pw::pyrowave_result_PYROWAVE_SUCCESS { - tracing::error!(result = ?r, "pyrowave: encoder rebuild failed"); - // `pw_enc` stays null — `Drop` and `submit_frame` both guard on it. The queued - // AUs are forfeit either way (the caller turns a false reset into a session - // error), so drop them rather than shipping output from a dead encoder. - self.pending.clear(); - return false; + for i in 0..SLOTS { + pw::pyrowave_encoder_destroy(self.pw_encs[i]); + // Publish the null IMMEDIATELY: the create below is fallible, and its failure path + // must not leave a freed pointer in the field. `pyrowave_encoder_destroy` is a + // plain `delete` (pyrowave_c.cpp) with no null check, so `Drop` running on a stale + // handle is a double free — the exact shape this reset hits when the rebuild fails + // because the device is already lost, which is the state that made the watchdog + // fire. + self.pw_encs[i] = std::ptr::null_mut(); + let mut enc: pw::pyrowave_encoder = std::ptr::null_mut(); + let r = pw::pyrowave_encoder_create(&einfo, &mut enc); + if r != pw::pyrowave_result_PYROWAVE_SUCCESS { + tracing::error!(result = ?r, slot = i, "pyrowave: encoder rebuild failed"); + // This handle stays null — `Drop` and `submit_frame` both guard on it. The + // queued AUs are forfeit either way (the caller turns a false reset into a + // session error), so drop them rather than shipping output from a dead + // encoder. + self.pending.clear(); + return false; + } + self.pw_encs[i] = enc; } - self.pw_enc = enc; + // Fresh handles start their own counters at 0, but the CLIENT's `last_seq` does not + // reset — so keep counting from where the stream was. A rebuild loses frames, and a + // gap is exactly what tells the decoder to restart. + self.next_slot = 0; } self.pending.clear(); true @@ -2108,13 +2184,15 @@ impl Drop for PyroWaveEncoder { // up, so on a failed open this runs against a partial prefix. That is sound because // `pyrowave_device_destroy(null)` is a bare `delete nullptr` (pyrowave_c.cpp — safe // no-op) and every `vkDestroy*`/`vkFree*` of VK_NULL_HANDLE is the spec-defined no-op; - // `pw_enc` is the one null-UNSAFE destroy and carries its own guard below. + // `pw_encs` are the null-UNSAFE destroys and carry their own guard below. unsafe { self.device.device_wait_idle().ok(); // Null when a failed `reset()` already destroyed it — `pyrowave_encoder_destroy` // is not null-safe. - if !self.pw_enc.is_null() { - pw::pyrowave_encoder_destroy(self.pw_enc); + for &e in &self.pw_encs { + if !e.is_null() { + pw::pyrowave_encoder_destroy(e); + } } pw::pyrowave_device_destroy(self.pw_dev); for (_, _, i, m, v) in self.import_cache.drain(..) { @@ -2844,4 +2922,133 @@ mod tests { assert!(!priority_refused(vk::Result::ERROR_EXTENSION_NOT_PRESENT)); assert!(!priority_refused(vk::Result::SUCCESS)); } + + // ---- PW5 stage 5: the alternating-handle sequence gate ------------------------------------ + + /// **THE gate for the second encoder handle.** Two `pyrowave_encoder` objects each keep their + /// OWN 3-bit `sequence_count`, so alternating them emits `1,1,2,2,3,3…` on the wire. The + /// decoder restarts a frame only when the value CHANGES + /// (`diff = (hdr.sequence - last_seq) & 0x7; restart = diff != 0`), so every repeat reads as + /// "more blocks of the same frame": `clear()` never runs and the second frame of each pair is + /// silently swallowed. Half frame rate, occasional mixed-frame blocks, no error anywhere — a + /// failure that passes a smoke test, on every client. + /// + /// `patches/0007-encoder-sequence-override.patch` exists solely to make that impossible, and + /// this test is what proves it, three ways over 20 frames (well past the 3-bit wrap at 8): + /// + /// 1. the wire counter advances by exactly +1 mod 8 per AU, read straight out of the block + /// header the decoder reads; + /// 2. ONE persistent decoder — `last_seq` carried across every push, exactly as a client's is — + /// reports ready for every single AU, so nothing is swallowed; + /// 3. consecutive decoded pictures DIFFER. Content moves every frame (`test_card` reseeded per + /// frame; flat fills are the documented false-green trap here), so a swallowed frame would + /// show up as a repeat, and this catches it even if 1 and 2 somehow both passed. + #[test] + #[ignore = "needs a real Vulkan 1.3 compute device (run on a GPU host, not the build box)"] + fn wire_sequence_increments_across_alternating_handles() { + const FRAMES: u32 = 20; + let (w, h) = (256u32, 256u32); + let mut enc = + PyroWaveEncoder::open(w, h, 60, 40_000_000, crate::ChromaFormat::Yuv420).expect("open"); + // This gate is meaningless with a single encoder handle. + const { assert!(SLOTS >= 2) }; + + let mut aus: Vec> = Vec::new(); + for i in 0..FRAMES { + // Content MOVES every frame — a repeated picture is the symptom being hunted, and a + // static card would hide it. ODD seeds only: `test_card` starts its LCG at `seed | 1`, + // so 2 and 3 produce a byte-identical card and consecutive even/odd seeds would fake + // the very repeat this test looks for (it did, on the first run). + enc.submit(&test_card(w, h, 2 * i + 1)).expect("submit"); + let au = enc.poll().expect("poll").expect("one AU per frame"); + aus.push(au.data); + } + + // (1) the wire counter, read from the header the decoder parses. + let seqs: Vec = aus + .iter() + .map(|au| { + crate::pyrowave_wire::wire_sequence(au, 0).expect("AU carries a block header") + }) + .collect(); + for (i, pair) in seqs.windows(2).enumerate() { + assert_eq!( + pair[1], + (pair[0] + 1) & 7, + "frame {} -> {}: wire sequence went {} -> {} (all: {seqs:?}). Two encoder handles \ + each counting alone produce repeats, which the decoder reads as more blocks of \ + the same frame — check that patch 0007 is applied and set_next_sequence is called", + i, + i + 1, + pair[0], + pair[1] + ); + } + + // (2) + (3) ONE decoder for the whole run — a fresh decoder per AU would reset `last_seq` + // and hide the exact bug this exists to catch. + // SAFETY: test-only FFI into the vendored decoder with locally-owned buffers. + unsafe { + let mut dev: pw::pyrowave_device = std::ptr::null_mut(); + assert_eq!( + pw::pyrowave_create_default_device(&mut dev), + pw::pyrowave_result_PYROWAVE_SUCCESS + ); + let dinfo = pw::pyrowave_decoder_create_info { + device: dev, + width: w as i32, + height: h as i32, + chroma: pw::pyrowave_chroma_subsampling_PYROWAVE_CHROMA_SUBSAMPLING_420, + fragment_path: false, + }; + let mut dec: pw::pyrowave_decoder = std::ptr::null_mut(); + assert_eq!( + pw::pyrowave_decoder_create(&dinfo, &mut dec), + pw::pyrowave_result_PYROWAVE_SUCCESS + ); + let mut last_y: Option> = None; + for (i, au) in aus.iter().enumerate() { + assert_eq!( + pw::pyrowave_decoder_push_packet(dec, au.as_ptr() as *const _, au.len()), + pw::pyrowave_result_PYROWAVE_SUCCESS, + "frame {i} was rejected by the decoder" + ); + assert!( + pw::pyrowave_decoder_decode_is_ready(dec, false), + "frame {i} never became decodable — the decoder is still accumulating it into \ + the PREVIOUS frame, which is exactly the repeated-sequence failure" + ); + let mut y = vec![0u8; (w * h) as usize]; + let mut cb = vec![0u8; (w * h / 4) as usize]; + let mut cr = vec![0u8; (w * h / 4) as usize]; + let mut buf: pw::pyrowave_cpu_buffer = std::mem::zeroed(); + buf.format = pw::pyrowave_cpu_buffer_format_PYROWAVE_CPU_BUFFER_FORMAT_YUV420P; + buf.width = w as i32; + buf.height = h as i32; + buf.data = [ + y.as_mut_ptr() as *mut _, + cb.as_mut_ptr() as *mut _, + cr.as_mut_ptr() as *mut _, + ]; + buf.row_stride_in_bytes = [w as usize, (w / 2) as usize, (w / 2) as usize]; + buf.plane_size_in_bytes = [y.len(), cb.len(), cr.len()]; + assert_eq!( + pw::pyrowave_decoder_decode_cpu_buffer_synchronous(dec, &buf), + pw::pyrowave_result_PYROWAVE_SUCCESS, + "frame {i} failed to decode" + ); + if let Some(prev) = &last_y { + assert_ne!( + prev, + &y, + "frame {i} decoded to the SAME picture as frame {} — a swallowed frame", + i - 1 + ); + } + last_y = Some(y); + } + pw::pyrowave_decoder_destroy(dec); + pw::pyrowave_device_destroy(dev); + } + } } diff --git a/crates/pf-encode/src/enc/pyrowave_wire.rs b/crates/pf-encode/src/enc/pyrowave_wire.rs index c7e02653..5f1363d6 100644 --- a/crates/pf-encode/src/enc/pyrowave_wire.rs +++ b/crates/pf-encode/src/enc/pyrowave_wire.rs @@ -53,6 +53,24 @@ pub(crate) fn stamp_color_bits(bitstream: &mut [u8], seq_offset: usize, bt2020_p } } +/// Read the 3-bit wire sequence counter out of a pyrowave block header. +/// +/// Every block header is `{ u16 ballot; u16 payload_words:12, sequence:3, extended:1; u32 ... }` +/// (`pyrowave_common.hpp`, `static_assert(sizeof == 8)`), so the counter is bits 12..14 of the +/// little-endian half-word at `packet_offset + 2` — the same word `stamp_color_bits` reaches into +/// from the other end. +/// +/// This field is the entire frame-boundary signal on the wire: the decoder restarts a frame only +/// when the value CHANGES (`diff = (hdr.sequence - last_seq) & 0x7; restart = diff != 0`), so a +/// repeated value is read as more blocks of the same frame. That is why PW5's alternating encoder +/// handles need `pyrowave_encoder_set_next_sequence`, and why a test asserts this reader sees +/// +1 mod 8 across the pair. +pub(crate) fn wire_sequence(bitstream: &[u8], packet_offset: usize) -> Option { + let lo = *bitstream.get(packet_offset + 2)?; + let hi = *bitstream.get(packet_offset + 3)?; + Some(((u16::from_le_bytes([lo, hi]) >> 12) & 0x7) as u8) +} + /// The wavelet block space's total 32x32-block count for a mode — the exact counting walk of /// upstream `WaveletBuffers::init_block_meta` (also ported to the Apple `WaveletLayout`, whose /// golden tests pin it against real host AUs). Needed because the vendored RDO pass packs the diff --git a/crates/pyrowave-sys/patches/0007-encoder-sequence-override.patch b/crates/pyrowave-sys/patches/0007-encoder-sequence-override.patch new file mode 100644 index 00000000..d69514b6 --- /dev/null +++ b/crates/pyrowave-sys/patches/0007-encoder-sequence-override.patch @@ -0,0 +1,123 @@ +Encoder wire-sequence override — PUNKTFUNK LOCAL PATCH. + +Not upstream. Exposes `Encoder::set_next_sequence(uint32_t)` (and a +`pyrowave_encoder_set_next_sequence` C entry) so the caller can stamp the 3-bit wire sequence +counter itself instead of relying on the encoder object's private one. + +WHY IT EXISTS. PyroWave's `Encoder` structurally cannot hold two frames in flight: `Encoder::Impl` +owns ONE each of `wavelet_img_high_res`, `bucket_buffer`, `meta_buffer`, `block_stat_buffer`, +`payload_data` and `quant_buffer`, and `Impl::encode` OPENS by discarding them — an image barrier +with `VK_IMAGE_LAYOUT_UNDEFINED` as the old layout (a written promise nothing else is reading it) +plus three `fill_buffer` clears. Two `encode()` calls recorded into two command buffers and +submitted to the same queue have no execution dependency in Vulkan, so encode N+1's DWT would +overwrite the bands and zero the RDO buckets while encode N's block packing still reads them. + +So overlapping frames means TWO encoder handles on one device, alternated — which is fine for +every resource above, because each handle gets its own. It is NOT fine for `sequence_count`, which +also lives on `Impl` and is stamped into every block header (pyrowave_encoder.cpp `packing_push`). +Two alternating handles each count 1,2,3... independently, so the wire sees 1,1,2,2,3,3... + +That is silently fatal on the decode side. `pyrowave_decoder.cpp` computes +`diff = (hdr.sequence - last_seq) & 0x7` and treats `restart = diff != 0`, so a REPEATED value +reads as "more blocks of the same frame": `clear()` never runs, `decoded_frame_for_current_sequence` +stays true, and every second frame is swallowed. The symptom is "it works, just at half rate, with +occasional mixed-frame blocks" — the kind of failure that passes a smoke test. It would hit every +client, since pf-client-core and the Apple Metal hand-port parse the same field. + +WHAT IT DOES. `set_next_sequence(seq)` stores `(seq - 1) & SequenceCountMask`, because +`Impl::encode` pre-increments before stamping — the setter's contract is about the next ENCODE, not +the next store. The Rust side keeps one monotonic counter across both handles and calls this before +each encode, so the wire sequence increments by exactly 1 mod 8 regardless of which handle produced +the frame. + +INERT WHEN UNUSED. Nothing calls it unless the caller does, so the single-handle paths — including +the whole Windows backend — behave exactly as before. No `.def` change is needed: the C API is +built as a static archive (crates/pyrowave-sys/CMakeLists.txt). + +Upstream status: not reported. It is a hook for a use case upstream explicitly designed against +("For low-latency use cases, overlapping frames in encode is meaningless due to latency and the +encoder is so fast anyway" — pyrowave.h). That reasoning holds at 1080p60 and stops holding at 4K +or under a GPU-bound game, which is what PW5 measured. + +diff --git a/crates/pyrowave-sys/vendor/pyrowave/pyrowave.h b/crates/pyrowave-sys/vendor/pyrowave/pyrowave.h +index fc0d5834..aeb22ffc 100644 +--- a/crates/pyrowave-sys/vendor/pyrowave/pyrowave.h ++++ b/crates/pyrowave-sys/vendor/pyrowave/pyrowave.h +@@ -476,6 +476,19 @@ PYROWAVE_PUBLIC_API pyrowave_result + pyrowave_encoder_packetize(pyrowave_encoder encoder, pyrowave_packet *packets, size_t packet_boundary, + size_t *out_packets, void *bitstream, size_t size); + ++// PUNKTFUNK LOCAL EXTENSION (patches/0007-encoder-sequence-override.patch), not upstream. ++// The wire sequence counter is 3 bits (PyroWave::SequenceCountMask, pyrowave_common.hpp); ++// exported here so callers mask with the codec's own value instead of a copied literal. ++#define PYROWAVE_SEQUENCE_MASK 0x7u ++ ++// Overrides the 3-bit wire sequence counter the NEXT encode will stamp into every block header. ++// The counter lives on the encoder object, so a caller that alternates TWO encoders to overlap ++// frames emits 1,1,2,2,3,3... and the decoder — which restarts a frame only when the value ++// CHANGES — reads the repeat as more blocks of the same frame and silently swallows every second ++// frame. Stamp a single monotonic counter across the handles with this. Value is masked to 3 bits. ++PYROWAVE_PUBLIC_API pyrowave_result ++pyrowave_encoder_set_next_sequence(pyrowave_encoder encoder, uint32_t sequence); ++ + // Implementation ensures GPU is idle before destroying objects. + PYROWAVE_PUBLIC_API void + pyrowave_encoder_destroy(pyrowave_encoder encoder); +diff --git a/crates/pyrowave-sys/vendor/pyrowave/pyrowave_c.cpp b/crates/pyrowave-sys/vendor/pyrowave/pyrowave_c.cpp +index 985cd0a9..fcd7d6f8 100644 +--- a/crates/pyrowave-sys/vendor/pyrowave/pyrowave_c.cpp ++++ b/crates/pyrowave-sys/vendor/pyrowave/pyrowave_c.cpp +@@ -1196,6 +1196,17 @@ pyrowave_encoder_packetize(pyrowave_encoder encoder, pyrowave_packet *packets, s + return PYROWAVE_SUCCESS; + } + ++// PUNKTFUNK LOCAL EXTENSION (patches/0007-encoder-sequence-override.patch), not upstream. ++pyrowave_result ++pyrowave_encoder_set_next_sequence(pyrowave_encoder encoder, uint32_t sequence) ++{ ++ Util::set_thread_logging_interface(&null_logger); ++ if (!encoder) ++ return PYROWAVE_ERROR_GENERIC; ++ encoder->encoder.set_next_sequence(sequence); ++ return PYROWAVE_SUCCESS; ++} ++ + void pyrowave_encoder_destroy(pyrowave_encoder encoder) + { + auto *device = encoder->device; +diff --git a/crates/pyrowave-sys/vendor/pyrowave/pyrowave_encoder.cpp b/crates/pyrowave-sys/vendor/pyrowave/pyrowave_encoder.cpp +index ad4e9746..f23717f3 100644 +--- a/crates/pyrowave-sys/vendor/pyrowave/pyrowave_encoder.cpp ++++ b/crates/pyrowave-sys/vendor/pyrowave/pyrowave_encoder.cpp +@@ -1230,6 +1230,14 @@ bool Encoder::encode(CommandBuffer &cmd, const ViewBuffers &views, const Bitstre + return impl->encode(cmd, views, buffers); + } + ++// PUNKTFUNK: see the declaration in pyrowave_encoder.hpp. Impl::encode PRE-increments ++// (sequence_count = (sequence_count + 1) & mask before stamping), so store one less than the value ++// the caller wants stamped — the setter's contract is about the next ENCODE, not the next store. ++void Encoder::set_next_sequence(uint32_t sequence) ++{ ++ impl->sequence_count = (sequence - 1) & SequenceCountMask; ++} ++ + const Vulkan::ImageView &Encoder::get_wavelet_band(int component, int level) + { + return *impl->component_layer_views[component][level]; +diff --git a/crates/pyrowave-sys/vendor/pyrowave/pyrowave_encoder.hpp b/crates/pyrowave-sys/vendor/pyrowave/pyrowave_encoder.hpp +index a65447d5..8c0ef0d0 100644 +--- a/crates/pyrowave-sys/vendor/pyrowave/pyrowave_encoder.hpp ++++ b/crates/pyrowave-sys/vendor/pyrowave/pyrowave_encoder.hpp +@@ -37,6 +37,12 @@ public: + bool init(Vulkan::Device *device, int width, int height, ChromaSubsampling chroma); + bool encode(Vulkan::CommandBuffer &cmd, const ViewBuffers &views, const BitstreamBuffers &buffers); + ++ // PUNKTFUNK: override the 3-bit wire sequence counter the NEXT encode will stamp. ++ // The counter is per-Encoder, so alternating two encoder objects to overlap frames emits ++ // 1,1,2,2,3,3... and the decoder reads a repeated value as "more blocks of the same frame". ++ // See crates/pyrowave-sys/patches/0007-encoder-sequence-override.patch. ++ void set_next_sequence(uint32_t sequence); ++ + // Debug hackery + const Vulkan::ImageView &get_wavelet_band(int component, int level); + bool encode_pre_transformed(Vulkan::CommandBuffer &cmd, const BitstreamBuffers &buffers, float quant_scale); diff --git a/crates/pyrowave-sys/vendor/pyrowave/pyrowave.h b/crates/pyrowave-sys/vendor/pyrowave/pyrowave.h index fc0d5834..aeb22ffc 100644 --- a/crates/pyrowave-sys/vendor/pyrowave/pyrowave.h +++ b/crates/pyrowave-sys/vendor/pyrowave/pyrowave.h @@ -476,6 +476,19 @@ PYROWAVE_PUBLIC_API pyrowave_result pyrowave_encoder_packetize(pyrowave_encoder encoder, pyrowave_packet *packets, size_t packet_boundary, size_t *out_packets, void *bitstream, size_t size); +// PUNKTFUNK LOCAL EXTENSION (patches/0007-encoder-sequence-override.patch), not upstream. +// The wire sequence counter is 3 bits (PyroWave::SequenceCountMask, pyrowave_common.hpp); +// exported here so callers mask with the codec's own value instead of a copied literal. +#define PYROWAVE_SEQUENCE_MASK 0x7u + +// Overrides the 3-bit wire sequence counter the NEXT encode will stamp into every block header. +// The counter lives on the encoder object, so a caller that alternates TWO encoders to overlap +// frames emits 1,1,2,2,3,3... and the decoder — which restarts a frame only when the value +// CHANGES — reads the repeat as more blocks of the same frame and silently swallows every second +// frame. Stamp a single monotonic counter across the handles with this. Value is masked to 3 bits. +PYROWAVE_PUBLIC_API pyrowave_result +pyrowave_encoder_set_next_sequence(pyrowave_encoder encoder, uint32_t sequence); + // Implementation ensures GPU is idle before destroying objects. PYROWAVE_PUBLIC_API void pyrowave_encoder_destroy(pyrowave_encoder encoder); diff --git a/crates/pyrowave-sys/vendor/pyrowave/pyrowave_c.cpp b/crates/pyrowave-sys/vendor/pyrowave/pyrowave_c.cpp index 985cd0a9..fcd7d6f8 100644 --- a/crates/pyrowave-sys/vendor/pyrowave/pyrowave_c.cpp +++ b/crates/pyrowave-sys/vendor/pyrowave/pyrowave_c.cpp @@ -1196,6 +1196,17 @@ pyrowave_encoder_packetize(pyrowave_encoder encoder, pyrowave_packet *packets, s return PYROWAVE_SUCCESS; } +// PUNKTFUNK LOCAL EXTENSION (patches/0007-encoder-sequence-override.patch), not upstream. +pyrowave_result +pyrowave_encoder_set_next_sequence(pyrowave_encoder encoder, uint32_t sequence) +{ + Util::set_thread_logging_interface(&null_logger); + if (!encoder) + return PYROWAVE_ERROR_GENERIC; + encoder->encoder.set_next_sequence(sequence); + return PYROWAVE_SUCCESS; +} + void pyrowave_encoder_destroy(pyrowave_encoder encoder) { auto *device = encoder->device; diff --git a/crates/pyrowave-sys/vendor/pyrowave/pyrowave_encoder.cpp b/crates/pyrowave-sys/vendor/pyrowave/pyrowave_encoder.cpp index ad4e9746..f23717f3 100644 --- a/crates/pyrowave-sys/vendor/pyrowave/pyrowave_encoder.cpp +++ b/crates/pyrowave-sys/vendor/pyrowave/pyrowave_encoder.cpp @@ -1230,6 +1230,14 @@ bool Encoder::encode(CommandBuffer &cmd, const ViewBuffers &views, const Bitstre return impl->encode(cmd, views, buffers); } +// PUNKTFUNK: see the declaration in pyrowave_encoder.hpp. Impl::encode PRE-increments +// (sequence_count = (sequence_count + 1) & mask before stamping), so store one less than the value +// the caller wants stamped — the setter's contract is about the next ENCODE, not the next store. +void Encoder::set_next_sequence(uint32_t sequence) +{ + impl->sequence_count = (sequence - 1) & SequenceCountMask; +} + const Vulkan::ImageView &Encoder::get_wavelet_band(int component, int level) { return *impl->component_layer_views[component][level]; diff --git a/crates/pyrowave-sys/vendor/pyrowave/pyrowave_encoder.hpp b/crates/pyrowave-sys/vendor/pyrowave/pyrowave_encoder.hpp index a65447d5..8c0ef0d0 100644 --- a/crates/pyrowave-sys/vendor/pyrowave/pyrowave_encoder.hpp +++ b/crates/pyrowave-sys/vendor/pyrowave/pyrowave_encoder.hpp @@ -37,6 +37,12 @@ public: bool init(Vulkan::Device *device, int width, int height, ChromaSubsampling chroma); bool encode(Vulkan::CommandBuffer &cmd, const ViewBuffers &views, const BitstreamBuffers &buffers); + // PUNKTFUNK: override the 3-bit wire sequence counter the NEXT encode will stamp. + // The counter is per-Encoder, so alternating two encoder objects to overlap frames emits + // 1,1,2,2,3,3... and the decoder reads a repeated value as "more blocks of the same frame". + // See crates/pyrowave-sys/patches/0007-encoder-sequence-override.patch. + void set_next_sequence(uint32_t sequence); + // Debug hackery const Vulkan::ImageView &get_wavelet_band(int component, int level); bool encode_pre_transformed(Vulkan::CommandBuffer &cmd, const BitstreamBuffers &buffers, float quant_scale); From 7d37fe450df1a02466dbaecd1c6f37cb9da707d7 Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Sun, 9 Aug 2026 00:30:25 +0200 Subject: [PATCH 6/6] =?UTF-8?q?test(pf-encode):=20run=20PyroWave=20at=20de?= =?UTF-8?q?pth=202=20on=20real=20hardware=20=E2=80=94=20without=20shipping?= =?UTF-8?q?=20depth=202?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Wave-2 PW5, the stage-6 experiment. Shipped behaviour is UNCHANGED: `max_inflight` is still 1. Stage 6 is the frame-corruption stage, and its gate is an on-glass tear-hunt with a live compositor, a real client and ten minutes of moving content. That is not runnable from here. But the depth-2 risk has two halves, and one of them lives entirely in this crate — the per-slot resources (`cmd`/`fence`/`csc_set`/y/uv/cursor) and the alternating encoder handles — so that half can be answered now, on the GPU, and the answer is worth having before anyone attempts the other. The experiment drives the backend with two frames genuinely in flight (submit N+1, then poll N) and compares the result against the encoder's OWN synchronous output over the same 16 moving frames. Its own depth-1 decode is the honest reference: pyrowave's raw AU bytes are not reproducible run-to-run (see the stage-3 commit), but its decoded planes are. RESULT, .21 / RTX 5070 Ti (GPU idle at 180 MHz of 3090 — the slow-clock worst case on this card): depth-2 vs depth-1 over 16 frames: worst-case PSNR identical (inf) Bit-identical luma, every frame, in order. So stages 4 and 5 between them are sufficient for the encoder side: doubling the six single-slot resources and alternating two `pyrowave_encoder` handles under one monotonic wire sequence really does make overlap invisible to the decoder. The test is built to fail rather than to pass. Content MOVES every frame (flat fills are the documented false-green trap — a torn frame stitched from two halves of a static card is invisible), it asserts two frames were ACTUALLY in flight rather than silently proving nothing, it asserts the AU count is unchanged, and it carries an off-by-one discriminator that raw PSNR would miss: each overlapped frame must match its own reference BETTER than it matches the previous one, so a pipeline delivering frames one position late fails even though every individual PSNR looks fine. It reaches `max_inflight` directly instead of through a shipped knob, precisely so the shipped value stays 1. ⚠ WHAT THIS DOES NOT COVER, stated here so the next person does not read it as a green light for stage 6: the CAPTURE side. `.process` hands the SPA buffer back to the compositor at callback return while the encode thread holds only a dup of its dmabuf fd, so a second frame in flight widens the window in which the producer may overwrite a buffer we are still reading by a full frame period. Nothing in this crate can test that — it needs a live producer. Stages 1 and 2 are what make it answerable (the pool census says how deep the producer's ring is; the Choice range asks for headroom), and the on-glass hunt is what would settle it. Gates green at CI parity. --- crates/pf-encode/src/enc/linux/pyrowave.rs | 205 +++++++++++++++++++++ 1 file changed, 205 insertions(+) diff --git a/crates/pf-encode/src/enc/linux/pyrowave.rs b/crates/pf-encode/src/enc/linux/pyrowave.rs index 9124cdc6..f35df49e 100644 --- a/crates/pf-encode/src/enc/linux/pyrowave.rs +++ b/crates/pf-encode/src/enc/linux/pyrowave.rs @@ -3051,4 +3051,209 @@ mod tests { pw::pyrowave_device_destroy(dev); } } + + /// Decode a whole AU stream through ONE decoder (a client's `last_seq` is not reset per frame) + /// and return each frame's luma plane. + /// + /// # Safety + /// Test-only FFI into the vendored decoder with locally-owned buffers. + unsafe fn decode_stream_luma(w: u32, h: u32, aus: &[Vec]) -> Vec> { + let mut dev: pw::pyrowave_device = std::ptr::null_mut(); + assert_eq!( + pw::pyrowave_create_default_device(&mut dev), + pw::pyrowave_result_PYROWAVE_SUCCESS + ); + let dinfo = pw::pyrowave_decoder_create_info { + device: dev, + width: w as i32, + height: h as i32, + chroma: pw::pyrowave_chroma_subsampling_PYROWAVE_CHROMA_SUBSAMPLING_420, + fragment_path: false, + }; + let mut dec: pw::pyrowave_decoder = std::ptr::null_mut(); + assert_eq!( + pw::pyrowave_decoder_create(&dinfo, &mut dec), + pw::pyrowave_result_PYROWAVE_SUCCESS + ); + let mut out = Vec::with_capacity(aus.len()); + for (i, au) in aus.iter().enumerate() { + assert_eq!( + pw::pyrowave_decoder_push_packet(dec, au.as_ptr() as *const _, au.len()), + pw::pyrowave_result_PYROWAVE_SUCCESS, + "frame {i} rejected" + ); + assert!( + pw::pyrowave_decoder_decode_is_ready(dec, false), + "frame {i} never became decodable" + ); + let mut y = vec![0u8; (w * h) as usize]; + let mut cb = vec![0u8; (w * h / 4) as usize]; + let mut cr = vec![0u8; (w * h / 4) as usize]; + let mut buf: pw::pyrowave_cpu_buffer = std::mem::zeroed(); + buf.format = pw::pyrowave_cpu_buffer_format_PYROWAVE_CPU_BUFFER_FORMAT_YUV420P; + buf.width = w as i32; + buf.height = h as i32; + buf.data = [ + y.as_mut_ptr() as *mut _, + cb.as_mut_ptr() as *mut _, + cr.as_mut_ptr() as *mut _, + ]; + buf.row_stride_in_bytes = [w as usize, (w / 2) as usize, (w / 2) as usize]; + buf.plane_size_in_bytes = [y.len(), cb.len(), cr.len()]; + assert_eq!( + pw::pyrowave_decoder_decode_cpu_buffer_synchronous(dec, &buf), + pw::pyrowave_result_PYROWAVE_SUCCESS, + "frame {i} failed to decode" + ); + out.push(y); + } + pw::pyrowave_decoder_destroy(dec); + pw::pyrowave_device_destroy(dev); + out + } + + /// PSNR (dB) between two equal-sized 8-bit planes; `f64::INFINITY` when identical. + fn psnr(a: &[u8], b: &[u8]) -> f64 { + assert_eq!(a.len(), b.len()); + let mse = a + .iter() + .zip(b) + .map(|(&x, &y)| { + let d = x as f64 - y as f64; + d * d + }) + .sum::() + / a.len() as f64; + if mse == 0.0 { + f64::INFINITY + } else { + 10.0 * (255.0 * 255.0 / mse).log10() + } + } + + /// **PW5 stage 6's ENCODER-side gate.** Two frames genuinely in flight at once must produce the + /// same pictures, in the same order, as the synchronous depth-1 path. + /// + /// This is the half of the depth-2 risk that lives in THIS crate: the slot resources + /// (`cmd`/`fence`/`csc_set`/y/uv/cursor) and the alternating encoder handles. Ground truth is + /// the encoder's OWN depth-1 output over the same frames, which is the honest reference — + /// pyrowave's raw AU bytes are not reproducible run-to-run (see the stage-3 commit), but its + /// DECODED planes are. + /// + /// Content moves every frame. Flat fills are the documented false-green trap here: a torn frame + /// assembled from two halves of a static card is invisible, and a gray fill once green-lit a + /// broken import. + /// + /// ⚠ WHAT THIS DOES **NOT** COVER, and no in-tree test can: the CAPTURE side. `.process` + /// requeues the SPA buffer to the compositor at callback return while the encode thread still + /// holds only a dup of its fd, so a second frame in flight widens the window in which the + /// producer may overwrite a buffer we are still reading by a full frame period. That needs a + /// live compositor, a real client and a long moving-content session — the on-glass tear-hunt + /// PW5 stage 6 is gated on. This test passing is necessary, not sufficient. + /// + /// Drives the backend at depth 2 by setting `max_inflight` directly rather than through a + /// shipped knob: the shipped value is 1 and this must not change that. + #[test] + #[ignore = "needs a real Vulkan 1.3 compute device (run on a GPU host, not the build box)"] + fn overlapping_two_frames_reproduces_the_synchronous_picture() { + const FRAMES: u32 = 16; + let (w, h) = (256u32, 256u32); + // Odd seeds: `test_card` starts its LCG at `seed | 1`, so 2 and 3 build the same card. + let cards: Vec = (0..FRAMES).map(|i| test_card(w, h, 2 * i + 1)).collect(); + let open = || { + PyroWaveEncoder::open(w, h, 60, 40_000_000, crate::ChromaFormat::Yuv420).expect("open") + }; + + // --- reference: strictly synchronous, one frame at a time --- + let mut enc = open(); + let sync: Vec> = cards + .iter() + .map(|c| { + enc.submit(c).expect("sync submit"); + assert_eq!( + enc.inflight.len(), + 1, + "submit must leave exactly one in flight" + ); + enc.poll() + .expect("sync poll") + .expect("one AU per frame") + .data + }) + .collect(); + drop(enc); + + // --- overlapped: submit N+1 before polling N --- + let mut enc = open(); + enc.max_inflight = SLOTS; + let mut overlapped: Vec> = Vec::new(); + let mut saw_two_in_flight = false; + for c in &cards { + enc.submit(c).expect("overlapped submit"); + saw_two_in_flight |= enc.inflight.len() == 2; + if enc.inflight.len() >= SLOTS { + overlapped.push( + enc.poll() + .expect("overlapped poll") + .expect("an AU once the pipeline is full") + .data, + ); + } + } + enc.flush().expect("flush drains the tail"); + while let Some(au) = enc.poll().expect("tail poll") { + overlapped.push(au.data); + } + drop(enc); + assert!( + saw_two_in_flight, + "two frames were never actually in flight — this test proved nothing" + ); + assert_eq!( + overlapped.len(), + sync.len(), + "the overlapped run emitted a different number of AUs — a frame was lost" + ); + + // --- decode both streams and compare, frame by frame --- + // SAFETY: test-only FFI into the vendored decoder with locally-owned buffers. + let (sy, oy) = unsafe { + ( + decode_stream_luma(w, h, &sync), + decode_stream_luma(w, h, &overlapped), + ) + }; + let mut worst = f64::INFINITY; + for i in 0..sy.len() { + let p = psnr(&sy[i], &oy[i]); + worst = worst.min(p); + // 45 dB is far above "looks the same" — a torn frame stitched from two moving cards + // lands in the teens. Not an equality assert only because the wavelet RDO is not + // bit-reproducible; the printed worst-case is the number to read. + assert!( + p > 45.0, + "frame {i}: overlapped decode is {p:.1} dB from the synchronous one — the pipelined \ + path changed the picture" + ); + // The discriminator that PSNR alone can miss: a frame delivered ONE POSITION OFF still + // scores well against a similar neighbour. It must match its OWN reference best. + if i > 0 { + let prev = psnr(&sy[i - 1], &oy[i]); + assert!( + p > prev, + "frame {i} matches the PREVIOUS reference better ({prev:.1} dB) than its own \ + ({p:.1} dB) — the pipeline is off by one" + ); + } + } + eprintln!( + "depth-2 vs depth-1 over {} frames: worst-case PSNR {}", + sy.len(), + if worst.is_infinite() { + "identical (inf)".to_string() + } else { + format!("{worst:.1} dB") + } + ); + } }