From 9fe9cbbf079a9a299f880ac1312cf937aed96045 Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Sat, 25 Jul 2026 17:56:22 +0200 Subject: [PATCH] perf(encode/vulkan): build the padded RGB frame in the staging memory, not beside it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The RGB-direct CPU-upload path allocated and zero-filled a whole padded frame on every submit, filled it row by row, then memcpy'd the whole thing into the mapped staging buffer. The zero-fill was entirely dead: the row loop writes every byte of every row, and rows past the source re-copy the last source row. So the frame paid for an allocation, a page-fault storm over fresh pages, a full zero-fill, and two full-frame copies where one would do. Map first, write the padded rows straight into the mapping. Nothing reads back from the destination — the row source is always the caller's buffer, never the staging memory — so writing into (write-combined) host memory costs nothing extra, and `make_host_buffer` allocates HOST_COHERENT, so the writes need no flush before the transfer reads them. Measured on real RDNA3 780M silicon, release build, `submit` alone, 400 frames x 3 rounds: - **1920x1080** (rows padded to 1088): p50 2315 -> **1725 us** (-25%), p99 2819 -> **1972 us** (-30%), and the spread (p99-min) tightens 739 -> 483 us. - **1366x768 -> 1408x768** (BOTH axes padded, so the column tail loop runs): p50 1013 -> **926 us** (-8%), p99 1303 -> **1110 us** (-15%). This is the one case where the new code could have been slower — 4-byte stores straight into write-combined memory — and it is not. - **CONTROL, 1280x720** (64x16-aligned, so the branch is never entered): p50 692 vs 692 us. No delta, which is what makes the two above attributable to this change rather than to anything else on the branch. The branch is reached by any RGB-direct session at a mode that is not 64x16 aligned, whenever capture delivers CPU frames (the default Linux capture path is dmabuf and never enters it). 1080p qualifies, since 1080 aligns to 1088. ⚠ An earlier draft of this message claimed "33 MB at 4K" — that is wrong: 3840 and 2160 are both already aligned, so 4K UHD never enters this branch at all. The large case is an ultrawide like 3440x1440 -> 3456x1440, ~20 MB. Three things this deliberately does NOT do: - The extent guards stay scoped to the `pad` branch. The CSC path deliberately supports a source SMALLER than the encode extent — its shader clamps at sample time — so a guard hoisted above the branch would break it. - Every fallible step stays above the map. An error raised between `map_memory` and `unmap_memory` would strand the mapping for the life of the slot's staging buffer, and the next frame's `map_memory` on it then violates VUID-vkMapMemory-memory-00678. (The filed "two exits above the map leak Vulkan objects" hazard is separately already gone: `47a23bec` moved that unwind into `make_host_buffer`.) - It adds guards rather than removing them: a zero source axis made `sh - 1` underflow, and "cannot fail after the map" has to be true by construction. Three corrections to an earlier draft, all found by review of that draft: - The `dw*dh*4 == need` precondition was a `debug_assert!` placed BELOW the map. That is wrong twice: it made the only check on the slice length vanish from the builds that ship, and a fired assert would have unwound past `unmap_memory` — the exact failure the bullet above says was designed out. It is now a real checked `?` above the map. - `need` was `(iw * ih * 4) as u64`: a u32 multiply widened after the fact, which agreed with the usize slice length only up to ~32768x32768. The old code's `min(need)` was a hard backstop against exactly that and the rewrite dropped it. Now widened before the multiply, matching `read_slot`'s existing discipline. - The extent guard now runs BEFORE the payload-length guard, so the usize `sw * sh * 4` cannot overflow on a garbage frame header. WP6.2(a). Co-Authored-By: Claude Opus 5 (1M context) --- .../pf-encode/src/enc/linux/vulkan_video.rs | 117 +++++++++++++----- 1 file changed, 83 insertions(+), 34 deletions(-) diff --git a/crates/pf-encode/src/enc/linux/vulkan_video.rs b/crates/pf-encode/src/enc/linux/vulkan_video.rs index a3f247c1..b3c50154 100644 --- a/crates/pf-encode/src/enc/linux/vulkan_video.rs +++ b/crates/pf-encode/src/enc/linux/vulkan_video.rs @@ -1620,7 +1620,12 @@ impl VulkanVideoEncoder { } else { (src_w, src_h) }; - let need = (iw * ih * 4) as u64; + // Widened BEFORE the multiply, not after: `(iw * ih * 4) as u64` multiplied in u32 and + // widened the result, so it agreed with the usize slice length below only while + // `iw * ih <= 2^30`. The 8192 dimension cap leaves 16x of margin, but the padded-write arm + // now sizes a raw-pointer slice from these numbers, and `read_slot` already applies exactly + // this discipline to driver-reported sizes for exactly this reason. + let need = iw as u64 * ih as u64 * 4; if self.frames[slot] .cpu_img .map(|(_, _, _, f, iw, ih)| (f, iw, ih)) @@ -1699,26 +1704,28 @@ impl VulkanVideoEncoder { } let (_, m, _) = self.frames[slot].cpu_stage.unwrap(); // RGB-direct sessions upload the image the ENCODER reads directly, so an undersized - // source (unaligned mode) must be padded here — rows/columns duplicated from the edge, - // matching the CSC shader's clamped reads (and record_pad_blit's GPU-side equivalent). - // The CSC path keeps the raw copy: its shader clamps at sample time. - let padded_owned: Vec; - let upload: &[u8] = if self.rgb.is_some() && (src_w != w || src_h != h) { + // source (unaligned mode) must be padded — rows/columns duplicated from the edge, matching + // the CSC shader's clamped reads (and `record_pad_blit`'s GPU-side equivalent). The CSC + // path keeps the raw copy: its shader clamps at sample time, and it DELIBERATELY supports a + // source smaller than the encode extent — so none of the guards below may be hoisted out of + // this branch, they are only sound because `self.rgb.is_some()`. + // + // Every fallible step stays ABOVE the map: an error raised between `map_memory` and + // `unmap_memory` would strand the mapping for the life of this slot's staging buffer. + let pad = if self.rgb.is_some() && (src_w != w || src_h != h) { let (sw, sh) = (src_w as usize, src_h as usize); - let (dw, dh) = (w as usize, h as usize); - if bytes.len() < sw * sh * 4 { - bail!( - "vulkan-encode (rgb-direct): CPU frame {}x{} needs {} bytes, got {}", - src_w, - src_h, - sw * sh * 4, - bytes.len() - ); + // A zero axis makes `sh - 1` below underflow. It cannot arrive from a real capturer, + // but refusing it by name is what lets the padding write be infallible under the map. + if src_w == 0 || src_h == 0 { + bail!("vulkan-encode (rgb-direct): CPU frame has a zero axis ({src_w}x{src_h})"); } - // The padding loop below only ever GROWS the source into the aligned extent. A source + // Extent FIRST, deliberately: the payload-length check below multiplies `sw * sh * 4` + // in usize on caller-supplied dimensions, so bounding them against the encode extent + // first is what keeps that multiply from overflowing on a garbage frame header. + // The padding write below only ever GROWS the source into the aligned extent. A source // larger than the encode extent is a contract violation (the Dmabuf arms already bail - // on it) and would panic here on the row `copy_from_slice`, so refuse it by name - // instead of unwinding out of the encode thread with an index message. + // on it) and would panic on the row `copy_from_slice`, so refuse it by name instead of + // unwinding out of the encode thread with an index message. if src_w > w || src_h > h { bail!( "vulkan-encode (rgb-direct): CPU frame {}x{} exceeds the encode extent {}x{} \ @@ -1729,26 +1736,68 @@ impl VulkanVideoEncoder { h ); } - let mut out = vec![0u8; dw * dh * 4]; - for y in 0..dh { - let sy = y.min(sh - 1); - let srow = &bytes[sy * sw * 4..][..sw * 4]; - let drow = &mut out[y * dw * 4..][..dw * 4]; - drow[..sw * 4].copy_from_slice(srow); - let mut last = [0u8; 4]; - last.copy_from_slice(&srow[(sw - 1) * 4..]); - for x in sw..dw { - drow[x * 4..(x + 1) * 4].copy_from_slice(&last); - } + if bytes.len() < sw * sh * 4 { + bail!( + "vulkan-encode (rgb-direct): CPU frame {}x{} needs {} bytes, got {}", + src_w, + src_h, + sw * sh * 4, + bytes.len() + ); } - padded_owned = out; - &padded_owned + // The `unsafe` slice below is sized from `(w, h)` while the staging buffer is sized + // from `need`. That equality is the precondition of the whole write, so CHECK it — + // here, above the map, in release. It was a `debug_assert!` below the map, which was + // wrong twice over: it made the one guard on the slice length vanish in the builds that + // ship, and a fired assert would have unwound past `unmap_memory` and stranded the + // mapping for the life of the slot (the next frame's `map_memory` then violates + // VUID-vkMapMemory-memory-00678). Unreachable under the 8192 dimension cap either way — + // but "every fallible step stays above the map" has to be true, not nearly true. + let dst_len = (w as usize) + .checked_mul(h as usize) + .and_then(|n| n.checked_mul(4)) + .filter(|&n| n as u64 == need) + .context("vulkan-encode (rgb-direct): staging buffer is not the encode extent")?; + Some((sw, sh, dst_len)) } else { - bytes + None }; let p = dev.map_memory(m, 0, vk::WHOLE_SIZE, vk::MemoryMapFlags::empty())? as *mut u8; - let n = upload.len().min(need as usize); - std::ptr::copy_nonoverlapping(upload.as_ptr(), p, n); + match pad { + // WP6.2(a): build the padded frame STRAIGHT into the staging memory. This used to + // allocate and zero-fill a whole padded frame every submit (8 MB at 1080p — 4K UHD is + // 64x16-aligned and never reaches this branch; the big case is an ultrawide like + // 3440x1440 -> 3456x1440, ~20 MB) and then memcpy it in here — with the zero-fill + // entirely dead, because the loop writes every byte of every row. Now: no per-frame + // allocation, no zero-fill, no page-fault storm over a fresh mapping, and one + // full-frame copy instead of two. Nothing reads back from `dst`, so writing into + // (possibly write-combined) host memory costs nothing extra — the row source is always + // `bytes`, never the destination. + Some((sw, sh, dst_len)) => { + let (dw, dh) = (w as usize, h as usize); + // SAFETY: `dst_len == dw * dh * 4 == need`, checked above the map, and the staging + // buffer is only kept when its size is >= `need`, so the mapping covers this slice. + // `make_host_buffer` allocates HOST_COHERENT memory, so the writes need no explicit + // flush before the transfer submitted below reads them. The guards above make every + // index here in-bounds by construction. + let dst = std::slice::from_raw_parts_mut(p, dst_len); + for y in 0..dh { + let sy = y.min(sh - 1); + let srow = &bytes[sy * sw * 4..][..sw * 4]; + let drow = &mut dst[y * dw * 4..][..dw * 4]; + drow[..sw * 4].copy_from_slice(srow); + let mut last = [0u8; 4]; + last.copy_from_slice(&srow[(sw - 1) * 4..]); + for x in sw..dw { + drow[x * 4..(x + 1) * 4].copy_from_slice(&last); + } + } + } + None => { + let n = bytes.len().min(need as usize); + std::ptr::copy_nonoverlapping(bytes.as_ptr(), p, n); + } + } dev.unmap_memory(m); Ok(self.frames[slot].cpu_img.unwrap().2) }