From 5d0b269e583b481e1ec19dbd6171affcf9581804 Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Thu, 6 Aug 2026 12:02:03 +0200 Subject: [PATCH] test(vkdecode): parity over the start-code form the host actually emits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both vendored vectors carry three-byte Annex-B start codes throughout. The real host emits four-byte ones on 100% of access units in both codecs — 1514/1514 H.264 and 1133/1133 HEVC, measured off the M0 NVENC corpus through the capture hook's own .idx offsets. So every parity verdict this program has recorded was taken on a prefix form that never ships, and the one form that does ship was exercised by nothing. That gap is not hypothetical. Submitting four-byte start codes to vkCmdDecodeVideoKHR unchanged is exactly what made HEVC unplayable on every driver tested: drivers are validated on the three-byte form, and a fixed +3 + 2 skip into a four-byte-prefixed slice reads a nonsense pps_id — the 115 and 119 both NVIDIAs printed. H.264 was never safe here by structure, only by its vendored encoder's convention, which is why the cure lives in the shared ring layer and why this coverage is generic over both codecs. Each codec's parity body now takes its access units as a parameter and runs twice: once over the vector as it sits, once over the same vector rewritten to four-byte prefixes. Prefix width carries no information, so both runs must reproduce the same goldens — sharing one body is what makes that an equality rather than two assertions that can drift. The rewrite copies nalu.data[nalu.offset..], the same nal_size bytes the parser hands the planner, so trailing_zero_8bits are dropped exactly where the production parser drops them: the only difference between the two streams is the width of every prefix. Two CPU guards keep the new legs from passing vacuously, which is the failure mode they are most exposed to — a rewrite that quietly returned its input would make them trivially green and nothing on the fleet would notice. They assert the original really does carry three-byte prefixes, that the rewritten stream carries none, that the NAL count is preserved exactly, and that the planner still yields 250 pictures. Hardware: all four legs 250/250 bit-identical to libavcodec on two independent driver stacks — AMD VanGogh on RADV/Mesa 26.0-devel (the Steam Deck) and NVIDIA 610.43.03 on Linux. NVIDIA is the family that rejected the four-byte form outright, so it is the meaningful witness for this regression. --- crates/pf-vkdecode/tests/common/mod.rs | 58 +++++++ crates/pf-vkdecode/tests/gpu_parity.rs | 224 +++++++++++++++++++++++-- 2 files changed, 265 insertions(+), 17 deletions(-) diff --git a/crates/pf-vkdecode/tests/common/mod.rs b/crates/pf-vkdecode/tests/common/mod.rs index ca76c8bf..12e30f29 100644 --- a/crates/pf-vkdecode/tests/common/mod.rs +++ b/crates/pf-vkdecode/tests/common/mod.rs @@ -130,6 +130,64 @@ pub fn split_h265_aus(stream: &[u8]) -> Vec<&[u8]> { aus } +/// Rewrite every Annex-B start code in `stream` to the FOUR-byte form +/// (`00 00 00 01`), leaving each NAL's payload bytes untouched. +/// +/// # Why the suite needs this +/// +/// Both vendored vectors use THREE-byte start codes throughout, while the real +/// host emits FOUR-byte ones on **100% of access units, both codecs** — measured +/// off the M0 NVENC corpus through the capture hook's `.idx` offsets: 1514/1514 +/// H.264 AUs and 1133/1133 HEVC. So without this, every parity verdict this +/// program has recorded was taken on a prefix form that never ships. +/// +/// That gap was not theoretical. Submitting four-byte start codes to +/// `vkCmdDecodeVideoKHR` is exactly what made HEVC unplayable on every driver +/// tested: drivers are validated on the three-byte form, and a fixed `+3 + 2` +/// skip into a four-byte-prefixed slice lands a byte early and reads a nonsense +/// `pps_id`. The cure lives in `ring::pack_slices`, which trims the leading zero +/// byte and derives the slice offsets from the trimmed lengths in one call. +/// H.264 was safe here only by its vendored encoder's convention, never by +/// structure — which is why the normalisation is shared and so is this helper. +/// +/// # What it preserves +/// +/// The payload copied is `nalu.data[nalu.offset..]`: exactly the `nal_size` +/// bytes the parser itself hands the planner. `Nalu::next` already discards +/// `trailing_zero_8bits` before the following start code, so the NALs in the +/// output are the NALs the production parser sees in the input, and the ONLY +/// difference between the two streams is the width of every prefix. +/// +/// Generic over the NAL header because both codecs share one `Nalu` type. The +/// AU splitters above cannot be shared for the opposite reason: their AU +/// boundary rules genuinely differ. +fn four_byte_start_codes(stream: &[u8]) -> Vec +where + H: cros_codecs::codec::h264::nalu::Header + std::fmt::Debug, +{ + use cros_codecs::codec::h264::nalu::Nalu; + + // A lower bound, not the answer: the output gains a byte per three-byte + // prefix and loses any trailing zeroes. + let mut out = Vec::with_capacity(stream.len()); + let mut cursor = Cursor::new(stream); + while let Ok(nalu) = Nalu::::next(&mut cursor) { + out.extend_from_slice(&[0x00, 0x00, 0x00, 0x01]); + out.extend_from_slice(&nalu.data[nalu.offset..]); + } + out +} + +/// [`four_byte_start_codes`] bound to H.264's NAL header. +pub fn h264_four_byte_start_codes(stream: &[u8]) -> Vec { + four_byte_start_codes::(stream) +} + +/// [`four_byte_start_codes`] bound to H.265's NAL header. +pub fn h265_four_byte_start_codes(stream: &[u8]) -> Vec { + four_byte_start_codes::(stream) +} + /// The slice of a decoder's surface the GPU legs drive. /// /// `VkH264Decoder` and `VkH265Decoder` expose it method-for-method (the crate diff --git a/crates/pf-vkdecode/tests/gpu_parity.rs b/crates/pf-vkdecode/tests/gpu_parity.rs index 66c43406..0a2027e5 100644 --- a/crates/pf-vkdecode/tests/gpu_parity.rs +++ b/crates/pf-vkdecode/tests/gpu_parity.rs @@ -29,6 +29,15 @@ //! decodes only one codec runs that leg and reports the other as "no physical //! device with VK_KHR_video_decode_…", which is a fact about the box. //! +//! Each codec runs that body TWICE: once over the vendored vector as it sits, +//! and once over the same vector rewritten to FOUR-byte start codes, which is +//! what the real host emits on 100% of access units in both codecs (1514/1514 +//! H.264 and 1133/1133 HEVC, measured off the M0 NVENC corpus). Prefix width +//! carries no information, so both runs must reproduce the same goldens — +//! and submitting the four-byte form to the driver unchanged is precisely the +//! defect that made HEVC unplayable on every driver tested. Until these legs +//! existed no parity vector exercised the form that actually ships. +//! //! The readback follows the presenter's exact frame contract: wait the frame's //! timeline `value`, round-trip the layout, signal `value + 1` in the SAME //! submission, then `release_frame(frame, true)` — and every submission is @@ -515,9 +524,15 @@ fn assert_bit_identical(hashes: &[String], goldens: &[&str], codec: &str) { ); } -#[test] -#[ignore = "needs a Vulkan Video H.264 decode device (fleet boxes; see module docs)"] -fn h264_every_frame_hashes_bit_identical_to_libavcodec() { +/// One H.264 parity run over a caller-supplied AU list. +/// +/// The AUs are a parameter rather than a constant because two legs share this +/// body: the vendored vector as it sits (three-byte start codes) and the same +/// vector rewritten to the four-byte ones the real host actually emits. Both +/// must reproduce the SAME goldens, because prefix width carries no +/// information — and running one body twice is what makes that an equality +/// rather than two similar-looking assertions that could drift apart. +fn h264_parity_run(aus: &[&[u8]], label: &str) { // One codec at a time on the device, and the `set_var` below happens only // under this lock (see `common::gpu_lock`). let _gpu = common::gpu_lock(); @@ -560,11 +575,7 @@ fn h264_every_frame_hashes_bit_identical_to_libavcodec() { DISPLAY_H264, ) }; - let hashes = collect_hashes( - &mut decoder, - &readback, - &common::split_h264_aus(common::TEST_25FPS_H264), - ); + let hashes = collect_hashes(&mut decoder, &readback, aus); // SAFETY: every readback was fence-waited inside `read_nv12`; nothing // else references its handles. unsafe { readback.destroy() }; @@ -576,12 +587,34 @@ fn h264_every_frame_hashes_bit_identical_to_libavcodec() { // references the setup's handles. unsafe { setup.destroy() }; - assert_bit_identical(&hashes, &goldens, "H.264"); + assert_bit_identical(&hashes, &goldens, label); } #[test] -#[ignore = "needs a Vulkan Video H.265 decode device (fleet boxes; see module docs)"] -fn h265_every_frame_hashes_bit_identical_to_libavcodec() { +#[ignore = "needs a Vulkan Video H.264 decode device (fleet boxes; see module docs)"] +fn h264_every_frame_hashes_bit_identical_to_libavcodec() { + h264_parity_run(&common::split_h264_aus(common::TEST_25FPS_H264), "H.264"); +} + +/// The same 250 frames, submitted the way the real host submits them. +/// +/// A failure here where the leg above passes means the four-byte prefix is +/// reaching the driver — `ring::pack_slices` stopped trimming the leading zero +/// byte, or stopped deriving the slice offsets from the trimmed lengths — which +/// is the defect that made HEVC unplayable on every driver tested. +#[test] +#[ignore = "needs a Vulkan Video H.264 decode device (fleet boxes; see module docs)"] +fn h264_four_byte_start_codes_decode_bit_identically() { + let stream = common::h264_four_byte_start_codes(common::TEST_25FPS_H264); + h264_parity_run( + &common::split_h264_aus(&stream), + "H.264 (4-byte start codes)", + ); +} + +/// The H.265 twin of [`h264_parity_run`]; see its docs for why the AUs are a +/// parameter. +fn h265_parity_run(aus: &[&[u8]], label: &str) { // As the H.264 leg: one codec at a time, `set_var` under the lock. let _gpu = common::gpu_lock(); @@ -624,11 +657,7 @@ fn h265_every_frame_hashes_bit_identical_to_libavcodec() { DISPLAY_H265, ) }; - let hashes = collect_hashes( - &mut decoder, - &readback, - &common::split_h265_aus(common::TEST_25FPS_H265), - ); + let hashes = collect_hashes(&mut decoder, &readback, aus); // SAFETY: every readback was fence-waited inside `read_nv12`; nothing // else references its handles. unsafe { readback.destroy() }; @@ -638,7 +667,25 @@ fn h265_every_frame_hashes_bit_identical_to_libavcodec() { // SAFETY: as the H.264 leg — decoder and readback are gone. unsafe { setup.destroy() }; - assert_bit_identical(&hashes, &goldens, "H.265"); + assert_bit_identical(&hashes, &goldens, label); +} + +#[test] +#[ignore = "needs a Vulkan Video H.265 decode device (fleet boxes; see module docs)"] +fn h265_every_frame_hashes_bit_identical_to_libavcodec() { + h265_parity_run(&common::split_h265_aus(common::TEST_25FPS_H265), "H.265"); +} + +/// The HEVC leg of the production prefix form — the one that would have caught +/// the shipped defect. See [`h264_four_byte_start_codes_decode_bit_identically`]. +#[test] +#[ignore = "needs a Vulkan Video H.265 decode device (fleet boxes; see module docs)"] +fn h265_four_byte_start_codes_decode_bit_identically() { + let stream = common::h265_four_byte_start_codes(common::TEST_25FPS_H265); + h265_parity_run( + &common::split_h265_aus(&stream), + "H.265 (4-byte start codes)", + ); } // --------------------------------------------------------------------------- @@ -796,3 +843,146 @@ fn h264_goldens_and_au_split_agree_with_the_planner() { goldens.len() ); } + +/// Count Annex-B start codes in `stream` as `(total, three_byte)`. +/// +/// Emulation prevention guarantees `00 00 01` cannot occur inside a NAL payload, +/// so every hit is a real prefix; a hit not preceded by a zero byte is a +/// three-byte one. +fn annexb_prefixes(stream: &[u8]) -> (usize, usize) { + let mut total = 0; + let mut three_byte = 0; + for i in 0..stream.len().saturating_sub(2) { + if stream[i..i + 3] == [0x00, 0x00, 0x01] { + total += 1; + if i == 0 || stream[i - 1] != 0x00 { + three_byte += 1; + } + } + } + (total, three_byte) +} + +// The two guards below are what stop the four-byte hardware legs from passing +// vacuously. Those legs assert that a rewritten vector decodes to the SAME +// goldens as the original — which is trivially true if the rewrite quietly +// returned its input, or dropped NALs the planner never missed. Nothing on the +// fleet would notice; these notice in ordinary CI, with the reason. + +#[test] +fn the_h264_four_byte_rewrite_changes_prefixes_and_nothing_else() { + use pf_bitstream::h264::H264Planner; + + let original = common::TEST_25FPS_H264; + let rewritten = common::h264_four_byte_start_codes(original); + + let (original_total, original_three) = annexb_prefixes(original); + let (rewritten_total, rewritten_three) = annexb_prefixes(&rewritten); + + assert!( + original_three > 0, + "the vendored H.264 vector is supposed to carry THREE-byte start codes; \ + if it no longer does, `h264_four_byte_start_codes_decode_bit_identically` \ + is feeding the hardware the same bytes as the leg above it and proves \ + nothing" + ); + assert_eq!( + rewritten_three, 0, + "every start code in the rewritten stream must be four-byte — {rewritten_three} \ + of {rewritten_total} are not" + ); + assert_eq!( + rewritten_total, original_total, + "the rewrite must preserve the NAL count exactly ({original_total}), not \ + drop or invent units" + ); + assert!( + rewritten.len() > original.len(), + "widening every prefix cannot shrink the stream" + ); + + // Same access units, same planner verdict: the rewrite changed the framing + // and nothing the decoder acts on. + let aus = common::split_h264_aus(&rewritten); + assert_eq!( + aus.len(), + common::split_h264_aus(original).len(), + "the rewritten stream must split into the same access units" + ); + assert_eq!( + aus.len(), + FRAME_COUNT, + "…and there are {FRAME_COUNT} of them" + ); + + let mut planner = H264Planner::new(); + let mut outputs = 0usize; + for (index, au) in aus.iter().enumerate() { + let plan = planner.plan_au(au).unwrap_or_else(|e| { + panic!("AU {index}: the four-byte rewrite must plan as the original does, got {e:?}") + }); + outputs += plan.dpb.outputs.len(); + } + outputs += planner.flush().outputs.len(); + assert_eq!( + outputs, FRAME_COUNT, + "the rewritten vector must still output {FRAME_COUNT} pictures" + ); +} + +#[test] +fn the_h265_four_byte_rewrite_changes_prefixes_and_nothing_else() { + use pf_bitstream::h265::H265Planner; + + let original = common::TEST_25FPS_H265; + let rewritten = common::h265_four_byte_start_codes(original); + + let (original_total, original_three) = annexb_prefixes(original); + let (rewritten_total, rewritten_three) = annexb_prefixes(&rewritten); + + assert!( + original_three > 0, + "the vendored H.265 vector is supposed to carry THREE-byte start codes; \ + if it no longer does, `h265_four_byte_start_codes_decode_bit_identically` \ + proves nothing" + ); + assert_eq!( + rewritten_three, 0, + "every start code in the rewritten stream must be four-byte — {rewritten_three} \ + of {rewritten_total} are not" + ); + assert_eq!( + rewritten_total, original_total, + "the rewrite must preserve the NAL count exactly ({original_total})" + ); + assert!( + rewritten.len() > original.len(), + "widening every prefix cannot shrink the stream" + ); + + let aus = common::split_h265_aus(&rewritten); + assert_eq!( + aus.len(), + common::split_h265_aus(original).len(), + "the rewritten stream must split into the same access units" + ); + assert_eq!( + aus.len(), + FRAME_COUNT, + "…and there are {FRAME_COUNT} of them" + ); + + let mut planner = H265Planner::new(); + let mut outputs = 0usize; + for (index, au) in aus.iter().enumerate() { + let plan = planner.plan_au(au).unwrap_or_else(|e| { + panic!("AU {index}: the four-byte rewrite must plan as the original does, got {e:?}") + }); + outputs += plan.dpb.outputs.len(); + } + outputs += planner.flush().outputs.len(); + assert_eq!( + outputs, FRAME_COUNT, + "the rewritten vector must still output {FRAME_COUNT} pictures" + ); +}