From 077db416ec2aefeabb6ad3220c1d42b5b4afd8f9 Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Sun, 9 Aug 2026 00:27:28 +0200 Subject: [PATCH] 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);