perf(encode/vulkan): build the padded RGB frame in the staging memory, not beside it

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) <noreply@anthropic.com>
This commit is contained in:
2026-07-25 17:56:22 +02:00
co-authored by Claude Opus 5
parent e680096c6a
commit 9fe9cbbf07
+83 -34
View File
@@ -1620,7 +1620,12 @@ impl VulkanVideoEncoder {
} else { } else {
(src_w, src_h) (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] if self.frames[slot]
.cpu_img .cpu_img
.map(|(_, _, _, f, iw, ih)| (f, iw, ih)) .map(|(_, _, _, f, iw, ih)| (f, iw, ih))
@@ -1699,26 +1704,28 @@ impl VulkanVideoEncoder {
} }
let (_, m, _) = self.frames[slot].cpu_stage.unwrap(); let (_, m, _) = self.frames[slot].cpu_stage.unwrap();
// RGB-direct sessions upload the image the ENCODER reads directly, so an undersized // 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, // source (unaligned mode) must be padded — rows/columns duplicated from the edge, matching
// matching the CSC shader's clamped reads (and record_pad_blit's GPU-side equivalent). // the CSC shader's clamped reads (and `record_pad_blit`'s GPU-side equivalent). The CSC
// The CSC path keeps the raw copy: its shader clamps at sample time. // path keeps the raw copy: its shader clamps at sample time, and it DELIBERATELY supports a
let padded_owned: Vec<u8>; // source smaller than the encode extent — so none of the guards below may be hoisted out of
let upload: &[u8] = if self.rgb.is_some() && (src_w != w || src_h != h) { // 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 (sw, sh) = (src_w as usize, src_h as usize);
let (dw, dh) = (w as usize, h as usize); // A zero axis makes `sh - 1` below underflow. It cannot arrive from a real capturer,
if bytes.len() < sw * sh * 4 { // but refusing it by name is what lets the padding write be infallible under the map.
bail!( if src_w == 0 || src_h == 0 {
"vulkan-encode (rgb-direct): CPU frame {}x{} needs {} bytes, got {}", bail!("vulkan-encode (rgb-direct): CPU frame has a zero axis ({src_w}x{src_h})");
src_w,
src_h,
sw * sh * 4,
bytes.len()
);
} }
// 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 // 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 // on it) and would panic on the row `copy_from_slice`, so refuse it by name instead of
// instead of unwinding out of the encode thread with an index message. // unwinding out of the encode thread with an index message.
if src_w > w || src_h > h { if src_w > w || src_h > h {
bail!( bail!(
"vulkan-encode (rgb-direct): CPU frame {}x{} exceeds the encode extent {}x{} \ "vulkan-encode (rgb-direct): CPU frame {}x{} exceeds the encode extent {}x{} \
@@ -1729,26 +1736,68 @@ impl VulkanVideoEncoder {
h h
); );
} }
let mut out = vec![0u8; dw * dh * 4]; if bytes.len() < sw * sh * 4 {
for y in 0..dh { bail!(
let sy = y.min(sh - 1); "vulkan-encode (rgb-direct): CPU frame {}x{} needs {} bytes, got {}",
let srow = &bytes[sy * sw * 4..][..sw * 4]; src_w,
let drow = &mut out[y * dw * 4..][..dw * 4]; src_h,
drow[..sw * 4].copy_from_slice(srow); sw * sh * 4,
let mut last = [0u8; 4]; bytes.len()
last.copy_from_slice(&srow[(sw - 1) * 4..]); );
for x in sw..dw {
drow[x * 4..(x + 1) * 4].copy_from_slice(&last);
}
} }
padded_owned = out; // The `unsafe` slice below is sized from `(w, h)` while the staging buffer is sized
&padded_owned // 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 { } else {
bytes None
}; };
let p = dev.map_memory(m, 0, vk::WHOLE_SIZE, vk::MemoryMapFlags::empty())? as *mut u8; let p = dev.map_memory(m, 0, vk::WHOLE_SIZE, vk::MemoryMapFlags::empty())? as *mut u8;
let n = upload.len().min(need as usize); match pad {
std::ptr::copy_nonoverlapping(upload.as_ptr(), p, n); // 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); dev.unmap_memory(m);
Ok(self.frames[slot].cpu_img.unwrap().2) Ok(self.frames[slot].cpu_img.unwrap().2)
} }