diff --git a/crates/pf-bitstream/src/av1.rs b/crates/pf-bitstream/src/av1.rs index b20787a0..798d4e93 100644 --- a/crates/pf-bitstream/src/av1.rs +++ b/crates/pf-bitstream/src/av1.rs @@ -158,6 +158,47 @@ impl RefState { } } +/// One CDEF secondary strength as every hardware API wants it: the **coded +/// two-bit syntax element**, `0..=3`. +/// +/// ⚠⚠ The parser does not hold that value. AV1 5.9.19 reads `cdef_y_sec_strength[i]` +/// as `f(2)` and then mutates the variable of the same name in place — +/// `if (cdef_y_sec_strength[i] == 3) cdef_y_sec_strength[i] += 1` — so the spec's +/// own `cdef_y_sec_strength` afterwards holds `0, 1, 2` or **`4`**, and cros-codecs +/// follows the spec literally (`parser.rs`, `parse_cdef_params`). Every decode API +/// wants the value BEFORE that fixup, and applies the expansion itself: +/// +/// * **Vulkan** — libavcodec's `vulkan_av1.c` sends `frame_header->cdef_y_sec_strength[i]` +/// straight out of CBS, and `cbs_av1_syntax_template.c` reads it as a bare +/// `fbs(2, …)` with no fixup. Vulkan's `StdVideoAV1CDEF` therefore carries the +/// coded value, because libavcodec is what every driver was validated against; +/// * **VA-API** — `vaapi_av1.c` packs `(pri << 2) + sec`, two bits for `sec`; +/// * **NVDEC** — `nvdec_av1.c` packs `(pri & 0x0F) | (sec << 4)`, two bits again; +/// * **DXVA** — `DXVA_PicParams_AV1`'s `cdef_y_strength[i].secondary` IS a two-bit +/// bitfield. +/// +/// So sending `4` is not "a bigger number": on three of those four it overflows a +/// two-bit field and the strength reads back as **0** — no secondary CDEF filtering +/// at all, on exactly the blocks that asked for the strongest. That is a small, +/// everywhere, in-loop pixel difference, which is the hardest kind to see and the +/// easiest kind to propagate: CDEF runs before the frame is stored as a reference. +/// +/// Frame 0 of the vendored 25fps vector codes it (`cdef_y_sec_strength[3]` and +/// `cdef_uv_sec_strength[0]` are both 4), as do 68 of its 274 frames — +/// [`crate::av1::tests::the_cdef_secondary_strength_is_the_coded_value`] pins both +/// numbers. +/// +/// Values `0..=2` are untouched by the fixup and pass through; a hand-built header +/// carrying the coded `3` passes through too. Anything wider is CLAMPED rather than +/// masked, because `& 3` is precisely the truncation this function exists to +/// prevent. +pub fn coded_cdef_sec_strength(parsed: u32) -> u8 { + match parsed { + 0..=2 => parsed as u8, + _ => 3, + } +} + /// What this access unit does to the decoded-picture store. #[derive(Debug, Clone, Default, PartialEq, Eq)] pub struct DpbUpdate { @@ -938,6 +979,86 @@ mod tests { ); } + /// [`coded_cdef_sec_strength`] inverts the spec's in-place fixup — and the + /// vendored vector really does code the value that needs it, on frame 0. + /// + /// Both halves matter. The mapping is three lines and could be asserted against + /// itself forever; what makes it load-bearing is that the parser DOES hand out + /// `4`, on the very first frame the parity leg compares, and on 68 of 274 + /// frames overall. If a re-synced vector ever stopped coding a secondary + /// strength of 3, this test would be comparing a correction against a stream + /// that never needs it, and the four hardware APIs' two-bit fields would be + /// untested again. + #[test] + fn the_cdef_secondary_strength_is_the_coded_value() { + // The fixup's inverse, and the identity everywhere else. + assert_eq!( + [0, 1, 2, 3, 4].map(coded_cdef_sec_strength), + [0, 1, 2, 3, 3], + "0..=2 pass through, the spec's 4 is the coded 3, and a hand-built 3 is \ + already coded" + ); + + let mut planner = Av1Planner::new(); + let (mut frames, mut needing_fixup, mut strengths) = (0u32, 0u32, 0u32); + let mut frame0_raw: Vec = Vec::new(); + for packet in IvfIterator::new(AV1_25FPS) { + for plan in planner.plan_au(packet).expect("the clean vector plans") { + if plan.dpb.stored.is_none() { + continue; + } + frames += 1; + let cdef = &plan.header.cdef_params; + let coded = 1usize << cdef.cdef_bits; + let mut any = false; + for i in 0..coded { + for raw in [cdef.cdef_y_sec_strength[i], cdef.cdef_uv_sec_strength[i]] { + assert!( + raw <= 2 || raw == 4, + "frame {frames}: the parser can only hold 0, 1, 2 or the \ + fixed-up 4 — {raw} means the vendored parse changed" + ); + assert!( + coded_cdef_sec_strength(raw) <= 3, + "the corrected value must fit the two bits every hardware \ + API gives it" + ); + if raw == 4 { + any = true; + strengths += 1; + } + } + } + if any { + needing_fixup += 1; + } + if frames == 1 { + frame0_raw = cdef.cdef_y_sec_strength[..coded] + .iter() + .chain(cdef.cdef_uv_sec_strength[..coded].iter()) + .copied() + .collect(); + } + } + } + assert_eq!(frames, 274); + assert_eq!( + frame0_raw, + vec![1, 2, 0, 4, 4, 0, 0, 0], + "frame 0's four luma then four chroma secondary strengths — the first \ + frame the parity leg hashes, and it needs the correction" + ); + assert_eq!( + needing_fixup, 68, + "68 of 274 frames of this vector carry a secondary strength the spec \ + fixed up; at zero the correction above is untested by any real stream" + ); + eprintln!( + "frames {frames} · frames needing the fixup {needing_fixup} · strengths \ + corrected {strengths}" + ); + } + #[test] fn an_access_unit_with_no_frame_is_refused() { let mut planner = Av1Planner::new(); diff --git a/crates/pf-dxvadec/src/pic_av1.rs b/crates/pf-dxvadec/src/pic_av1.rs index 2d596e34..0218b14e 100644 --- a/crates/pf-dxvadec/src/pic_av1.rs +++ b/crates/pf-dxvadec/src/pic_av1.rs @@ -34,6 +34,7 @@ use std::ops::Range; +use pf_bitstream::av1::coded_cdef_sec_strength; use pf_bitstream::av1::AuPlan; use pf_bitstream::av1::FrameType; use pf_bitstream::av1::PicId; @@ -66,6 +67,15 @@ use crate::SlotMap; /// `DXVA_PicParams_AV1::tiles` holds at most 64 column and 64 row sizes. pub const MAX_TILE_DIM: usize = 64; +/// `log2_restoration_unit_size` on a frame that restores nothing. +/// +/// Not a meaningful size — every plane's `frame_restoration_type` is NONE and a +/// driver reading the field at all has nothing to apply it to. It is 8 because +/// that is what libavcodec's `dxva2_av1.c` writes, and 8 is the top of the range +/// `dxva.h` documents (6..8); the parser's own array is still zero here, whose +/// `trailing_zeros` would be 16. +const LOG2_RESTORATION_UNIT_SIZE_UNUSED: u16 = 8; + /// `LAST_FRAME` (AV1 spec): the first reference NAME, and the offset between a /// position in `ref_frame_idx` and the index the spec's per-reference arrays /// (global motion, order hints, sign bias) use. `INTRA_FRAME` is 0. @@ -278,12 +288,31 @@ pub fn plan_to_dxva_av1( loop_filter.ref_deltas = lf.loop_filter_ref_deltas; loop_filter.mode_deltas = lf.loop_filter_mode_deltas; loop_filter.delta_lf_res = lf.delta_lf_res; + // Loop restoration. + // + // ⚠ DXVA wants the LOG2 of the unit size where the parser records the size + // itself — and the parser records NOTHING when restoration is off. AV1 5.9.20 + // only computes `LoopRestorationSize` inside `if ( UsesLr )`, so on a frame with + // every plane's restoration type NONE the vendored parser's array is still + // `[0, 0, 0]`, and `0u16.trailing_zeros()` is **16** — a restoration unit of + // 65536 samples, in a field `dxva.h` documents as 6, 7 or 8. That is 271 of the + // vendored vector's 274 frames. + // + // libavcodec's `dxva2_av1.c` sends `uses_lr ? 6 + lr_unit_shift : 8` for luma + // and `uses_lr ? 6 + lr_unit_shift - lr_uv_shift : 8` for the two chroma planes, + // and libavcodec is the implementation every driver was validated against, so + // the OFF value is 8 rather than 0 or 16. With restoration on, the parser's own + // `loop_restoration_size[i]` already carries the per-plane `>> lr_uv_shift`, so + // its `trailing_zeros` IS `6 + lr_unit_shift - lr_uv_shift` — the two agree + // wherever the field is read at all. let lr = &h.loop_restoration_params; for i in 0..3 { loop_filter.frame_restoration_type[i] = lr.frame_restoration_type[i] as u8; - // DXVA wants the LOG2 of the unit size; the parser records the size itself. - loop_filter.log2_restoration_unit_size[i] = - lr.loop_restoration_size[i].trailing_zeros() as u16; + loop_filter.log2_restoration_unit_size[i] = if lr.uses_lr { + lr.loop_restoration_size[i].trailing_zeros() as u16 + } else { + LOG2_RESTORATION_UNIT_SIZE_UNUSED + }; } let q = &h.quantization_params; @@ -312,15 +341,24 @@ pub fn plan_to_dxva_av1( .pack(); // Two fields to a byte (module docs) — not the parallel arrays AV1's syntax // and Vulkan's Std block use. + // + // ⚠ `secondary` gets TWO bits here, and the parser's value does not fit them: + // AV1 5.9.19 rewrites the syntax element in place (a coded 3 becomes 4) and + // cros-codecs follows the spec, while `DXVA_PicParams_AV1` — like VA-API, + // NVDEC and Vulkan — wants the coded two-bit read, which is what libavcodec's + // `dxva2_av1.c` sends. Passing the parser's 4 through `pack`'s `& 0x3` would + // turn the STRONGEST secondary filter into NO filter, silently, on every frame + // that codes one. `coded_cdef_sec_strength` is the inverse; its docs carry the + // evidence. for i in 0..8 { cdef.y_strengths[i] = CdefStrength { primary: c.cdef_y_pri_strength[i] as u8, - secondary: c.cdef_y_sec_strength[i] as u8, + secondary: coded_cdef_sec_strength(c.cdef_y_sec_strength[i]), } .pack(); cdef.uv_strengths[i] = CdefStrength { primary: c.cdef_uv_pri_strength[i] as u8, - secondary: c.cdef_uv_sec_strength[i] as u8, + secondary: coded_cdef_sec_strength(c.cdef_uv_sec_strength[i]), } .pack(); } @@ -666,6 +704,183 @@ mod tests { ); } + /// The CHROMA deblocking levels reach `filter_level_u` / `filter_level_v`, and + /// `log2_restoration_unit_size` is never the parser's silence. + /// + /// Two halves of the same block, both measured on hardware rather than argued. + /// + /// The levels first. ⚠ The AV1 Vulkan rung's frame-0 parity leg came back `luma + /// IDENTICAL, chroma 319/38400 bytes differ` and that signature was reproduced + /// EXACTLY — count, `|delta|` histogram and the first six differing bytes with + /// their values — by decoding the vector's frame 0 with `loop_filter_level[2]` + /// and `[3]` forced to zero in the bitstream. **That divergence turned out NOT + /// to be a levels bug** (it was a freed sequence header making the driver treat + /// the frame as monochrome — `pf_vkdecode::session_av1`), so do not cite it as + /// evidence that a rung got the pair wrong. What it does establish, and what + /// keeps this test, is the SIGNATURE: frame 0 codes `[1, 7, 8, 12]`, two luma + /// levels and two chroma ones, and dropping only the chroma pair is invisible + /// to luma and to every other plane statistic. A rung that lost the pair would + /// fail Windows parity in a way nothing else here would notice, and this rung's + /// `[2]` and `[3]` reads are four characters from `[0]` and `[1]`. + /// + /// Then the restoration unit size, which is a units defect the vendored parser + /// invites: `LoopRestorationSize` is only computed inside `if ( UsesLr )` + /// (5.9.20), so the array is `[0, 0, 0]` on a frame that restores nothing and + /// `trailing_zeros` turns that into **16** — 271 of these 274 frames, in a field + /// `dxva.h` documents as 6..8. libavcodec sends 8. + #[test] + fn the_chroma_loop_filter_levels_and_the_restoration_unit_size_reach_the_driver() { + let mut planner = Av1Planner::new(); + let mut slots = SlotMap::new(NUM_REF_SLOTS); + let (mut frames, mut with_chroma_lf, mut with_lr) = (0u32, 0u32, 0u32); + + for packet in IvfIterator::new(AV1_25FPS) { + for plan in planner.plan_au(packet).expect("the clean vector plans") { + if plan.dpb.stored.is_none() { + continue; + } + let dx = plan_to_dxva_av1(&plan, &mut slots, frames).expect("converts"); + frames += 1; + let lf = &plan.header.loop_filter_params; + // `#[repr(packed)]` — copy the block out before reading its fields. + let sent = dx.pic_params.loop_filter; + assert_eq!( + ( + sent.filter_level[0], + sent.filter_level[1], + sent.filter_level_u, + sent.filter_level_v + ), + ( + lf.loop_filter_level[0], + lf.loop_filter_level[1], + lf.loop_filter_level[2], + lf.loop_filter_level[3] + ), + "frame {frames}: DXVA splits AV1's four levels into a luma PAIR \ + plus two named chroma fields — U is index 2 and V is index 3" + ); + if lf.loop_filter_level[2] != 0 || lf.loop_filter_level[3] != 0 { + with_chroma_lf += 1; + } + if frames == 1 { + assert_eq!( + (sent.filter_level, sent.filter_level_u, sent.filter_level_v), + ([1, 7], 8, 12), + "frame 0's levels, and the pair whose loss the Vulkan rung's \ + frame-0 divergence was reproduced from" + ); + } + + let lr = &plan.header.loop_restoration_params; + let sizes = sent.log2_restoration_unit_size; + if lr.uses_lr { + with_lr += 1; + assert_eq!(lr.loop_restoration_size, [128, 128, 128]); + assert_eq!(sizes, [7, 7, 7], "6 + lr_unit_shift, per plane"); + } else { + assert_eq!( + sizes, [LOG2_RESTORATION_UNIT_SIZE_UNUSED; 3], + "frame {frames}: restores nothing, so the size is \ + libavcodec's 8 — never the parser's zero read as 16" + ); + } + assert!( + sizes.iter().all(|s| (6..=8).contains(s)), + "frame {frames}: log2_restoration_unit_size is 6, 7 or 8" + ); + } + } + + assert_eq!(frames, 274); + assert_eq!( + with_chroma_lf, 123, + "123 of 274 frames of this vector deblock chroma; at zero the levels \ + above are all zero anyway and this test could not tell a dropped pair \ + from a carried one" + ); + assert_eq!( + with_lr, 3, + "three frames use loop restoration, so both branches of the size are \ + exercised" + ); + } + + /// The packed CDEF strength bytes carry the CODED secondary strength. + /// + /// `CdefStrength::pack` gives `secondary` TWO bits, and the vendored parser + /// holds the AV1 spec's post-fixup value — 4 where the stream coded 3 (5.9.19 + /// rewrites the syntax element in place). `& 0x3` then turns the STRONGEST + /// secondary filter into no filter at all, silently, on 68 of this vector's 274 + /// frames including the first. libavcodec's `dxva2_av1.c` assigns CBS's + /// unmodified two-bit read into the same bitfield, which is the convention every + /// driver was validated against. + /// + /// Asserted against the packed BYTE rather than the intermediate struct, + /// because the truncation is what `pack` does and a test that stopped at the + /// struct would not have seen it. + #[test] + fn cdef_secondary_strengths_survive_the_two_bit_pack() { + let mut planner = Av1Planner::new(); + let mut slots = SlotMap::new(NUM_REF_SLOTS); + let (mut frames, mut corrected_frames) = (0u32, 0u32); + + for packet in IvfIterator::new(AV1_25FPS) { + for plan in planner.plan_au(packet).expect("the clean vector plans") { + if plan.dpb.stored.is_none() { + continue; + } + let dx = plan_to_dxva_av1(&plan, &mut slots, frames).expect("converts"); + frames += 1; + let raw = &plan.header.cdef_params; + // `#[repr(packed)]` — copy the arrays out before indexing them. + let cdef = dx.pic_params.cdef; + let (y, uv) = (cdef.y_strengths, cdef.uv_strengths); + let mut corrected = false; + for i in 0..8 { + let want_y = coded_cdef_sec_strength(raw.cdef_y_sec_strength[i]); + let want_uv = coded_cdef_sec_strength(raw.cdef_uv_sec_strength[i]); + assert_eq!( + (y[i] >> 6, uv[i] >> 6), + (want_y, want_uv), + "frame {frames}: the secondary strength must survive the \ + two-bit field — the parser's 4 packs to 0" + ); + assert_eq!( + (y[i] & 0x3f, uv[i] & 0x3f), + ( + raw.cdef_y_pri_strength[i] as u8, + raw.cdef_uv_pri_strength[i] as u8 + ), + "the PRIMARY strengths are not fixed up by the spec and must \ + reach the driver untouched" + ); + if raw.cdef_y_sec_strength[i] == 4 || raw.cdef_uv_sec_strength[i] == 4 { + corrected = true; + } + } + if corrected { + corrected_frames += 1; + } + if frames == 1 { + assert_eq!( + (y[3] >> 6, uv[0] >> 6), + (3, 3), + "frame 0 codes the strongest secondary strength twice, and \ + the uncorrected conversion packed both as 0" + ); + } + } + } + + assert_eq!(frames, 274); + assert_eq!( + corrected_frames, 68, + "68 of 274 frames of this vector need the correction; at zero this test \ + compares an untouched conversion against itself" + ); + } + /// A key frame names no reference, and must say so with the unused sentinel /// rather than with surface 0. #[test] diff --git a/crates/pf-vkdecode/src/decoder_av1.rs b/crates/pf-vkdecode/src/decoder_av1.rs index f6508af2..84f66931 100644 --- a/crates/pf-vkdecode/src/decoder_av1.rs +++ b/crates/pf-vkdecode/src/decoder_av1.rs @@ -29,6 +29,14 @@ //! `pReferenceSlots` out in [`DecodePlanVkAv1::refs`] order INDEPENDENTLY, and //! [`build_scope_av1`] fails closed when the two disagree about a slot the op //! binds. +//! - **A DPB slot this frame READS may not be recycled until its decode op is +//! recorded.** `refresh_frame_flags` applies AFTER the frame decodes (7.20), so +//! almost every inter frame of a low-delay stream overwrites a slot it is +//! reading — 268 of the vendored vector's 274 frames. Releasing that slot inside +//! the conversion, which is what H.264 and H.265 do with their whole `removed` +//! list, gives it to this frame's own decode target: the reference then names +//! the slot being written. [`DecodePlanVkAv1::release_after_decode`] carries +//! those ids and this module releases them after the submission. //! - **A lost reference is fatal, not degraded.** `AuPlan::refs` is indexed by //! reference NAME and a lost reference leaves a HOLE there, so nothing is //! renumbered and the conversion could in principle write @@ -902,17 +910,10 @@ impl VkAv1Decoder { // setup slot's PREVIOUS binding is cleared before it binds fresh. let setup = usize::from(vk_plan.setup_slot); if state.dpb.is_none() { - let mut held = vec![false; state.slot_image.len()]; - for (slot, _id) in state.slots.held() { - held[usize::from(slot)] = true; - } - for (slot, binding) in state.slot_image.iter_mut().enumerate() { - if let Some(picture) = *binding { - if !held[slot] || slot == setup { - state.pool.pictures[picture].bound = false; - *binding = None; - } - } + let unbound = + sync_slot_bindings(&state.slots, &mut state.slot_image, vk_plan.setup_slot); + for picture in unbound { + state.pool.pictures[picture].bound = false; } } @@ -1031,6 +1032,19 @@ impl VkAv1Decoder { state.slot_refs[usize::from(r.slot)] = Some(r.std); } + // The slots this frame's own refresh displaced while it was still READING + // them. Held through the conversion and the submission above so neither the + // setup assignment nor the binding sync could take them + // (`DecodePlanVkAv1::release_after_decode`); free now that the decode op is + // recorded, so the next frame may have them. Their pool images stay pinned + // by `bound` until that frame's sync, which is the same one-frame grace + // every other released slot's image gets. + for &id in &vk_plan.release_after_decode { + if !state.slots.release(id) { + trace!(id, "deferred release of an id the slot map no longer holds"); + } + } + self.pending.insert( vk_plan.setup_id, PendingPic { @@ -1758,6 +1772,40 @@ fn reset_slot_bindings( unbound } +/// Coincide-mode binding sync, run once per frame between the plan conversion and +/// the decode target's allocation: a slot the ledger no longer holds stops binding +/// its pool image, and the setup slot's PREVIOUS binding is cleared before it binds +/// fresh. Returns the pool images that lost a binding — the caller clears their +/// `bound` flag, which is what puts them back in reach of the free list. +/// +/// The pictures themselves are untouched: one still `pending` or held by a consumer +/// stays off the free list on those flags alone (the decoupled-pool contract in +/// [`crate::images`]). +/// +/// A free function rather than four lines inline, because it is half of the +/// invariant `build_scope_av1` refuses on: a slot this frame REFERENCES must still +/// bind an image once this has run. Driving the two together over the vendored +/// vector is what `slot_recycling_waits_for_the_decode_op` does, and what no +/// hardware-free test could do while this lived inside `decode_planned`. +fn sync_slot_bindings( + slots: &SlotMap, + slot_image: &mut [Option], + setup_slot: u8, +) -> Vec { + let mut held = vec![false; slot_image.len()]; + for (slot, _id) in slots.held() { + held[usize::from(slot)] = true; + } + let setup = usize::from(setup_slot); + let mut unbound = Vec::new(); + for (slot, binding) in slot_image.iter_mut().enumerate() { + if binding.is_some() && (!held[slot] || slot == setup) { + unbound.extend(binding.take()); + } + } + unbound +} + /// The picture resource view bound for DPB `slot`: the bound pool image (coincide) /// or the DPB array layer (distinct). fn slot_view(state: &SessionStateAv1, slot: u8) -> Option { @@ -3001,4 +3049,220 @@ mod tests { } assert_eq!(frames, 274); } + + /// The whole vendored vector through the DPB bookkeeping `decode_planned` + /// runs — conversion, [`sync_slot_bindings`], [`build_scope_av1`] — with no + /// GPU anywhere. + /// + /// This is the test that was missing, and the defect it closes reached an + /// RTX 5070 Ti before anything on this machine noticed: 172 unit tests, clippy + /// clean, and `AU 4: DPB slot 2 is referenced by this AU but binds no image` + /// on the first hardware contact. Everything needed to see it was on the CPU. + /// What was not on the CPU was a test that ran the three pieces TOGETHER: the + /// conversion was tested against a `SlotMap`, the scope builder against + /// hand-made reference lists, and the binding sync against nothing at all (it + /// was four lines inline in `decode_planned`). Each was right about its own + /// half and the seam between them was where the picture went missing. + /// + /// So this walks the real vector through the real functions and asserts what + /// the hardware asserts: + /// + /// * every slot this frame references still binds an image when the scope is + /// built (the refusal that fired on the driver); + /// * the image it binds is the one that reference was DECODED into — the + /// assertion that matters more, because a slot recycled into the setup + /// picture is *bound*, just to the wrong picture, and the hardware would + /// have predicted from the frame it was in the middle of writing without + /// ever reporting an error; + /// * no held slot is left without a binding, which `build_scope_av1` only + /// traces as "unreachable in practice". + #[test] + fn slot_recycling_waits_for_the_decode_op() { + #[derive(Clone, Default)] + struct SimPicture { + bound: bool, + pending: bool, + held: u32, + } + // A distinguishable view per POOL IMAGE (never dereferenced), so a scope + // entry can be traced back to the picture that image holds. + let image_view = |picture: usize| vk::ImageView::from_raw(picture as u64 + 1); + + let mut planner = Av1Planner::new(); + let mut slots = SlotMap::new(NUM_REF_SLOTS); + let mut slot_image: Vec> = vec![None; REQUIRED_SLOTS as usize]; + let mut slot_refs: Vec> = + vec![None; REQUIRED_SLOTS as usize]; + let mut pictures = + vec![SimPicture::default(); (REQUIRED_SLOTS + crate::images::HOLD_HEADROOM) as usize]; + // Decoded pictures awaiting an output verdict, and the pool image each + // picture was decoded into (for as long as anything can reference it). + let mut pending: BTreeMap = BTreeMap::new(); + let mut image_of: BTreeMap = BTreeMap::new(); + + let (mut frames, mut deferring, mut scope_refs) = (0u32, 0u32, 0u32); + + for packet in IvfIterator::new(AV1_25FPS) { + for plan in planner.plan_au(packet).expect("the clean vector plans") { + let Some(setup_id) = plan.dpb.stored else { + // show_existing_frame decodes nothing; `decode_planned` + // settles it and releases its removals directly. + let (ready, dropped) = + settle_dpb_ids(&mut pending, &plan.dpb.outputs, &plan.dpb.removed); + for image in ready.into_iter().chain(dropped) { + pictures[image].pending = false; + } + for &id in &plan.dpb.removed { + slots.release(id); + image_of.remove(&id); + } + continue; + }; + frames += 1; + + let vk = plan_to_vk_av1(&plan, &mut slots).expect("the clean vector converts"); + let setup = usize::from(vk.setup_slot); + if !vk.release_after_decode.is_empty() { + deferring += 1; + } + + for picture in sync_slot_bindings(&slots, &mut slot_image, vk.setup_slot) { + pictures[picture].bound = false; + } + let dst = pictures + .iter() + .position(|p| !p.bound && !p.pending && p.held == 0) + .unwrap_or_else(|| panic!("frame {frames}: picture pool exhausted")); + + // Every held slot but the setup one must bind an image, or + // `build_scope_av1` silently drops it from the coding scope. + for (slot, _id) in slots.held() { + if usize::from(slot) == setup { + continue; + } + assert!( + slot_image[usize::from(slot)].is_some(), + "frame {frames}: held slot {slot} binds no image" + ); + } + + let held_slots: Vec = slots.held().map(|(slot, _id)| slot).collect(); + let (scope, reference_count) = build_scope_av1( + &vk.refs, + &vk.reference_name_slot_indices, + held_slots.iter().copied(), + vk.setup_slot, + image_view(dst), + vk.setup_ref, + &slot_refs, + |slot| slot_image[usize::from(slot)].map(image_view), + ) + .unwrap_or_else(|e| { + panic!( + "frame {frames}: {e}\n setup_slot={setup} setup_id={setup_id}\n \ + refs={:?}\n names={:?}\n bindings={slot_image:?}", + vk.refs.iter().map(|r| (r.slot, r.id)).collect::>(), + vk.reference_name_slot_indices, + ) + }); + + // The scope binds every held slot but the setup one exactly once, + // plus the setup slot as the `-1` activation entry — nothing + // dropped, nothing duplicated. (The setup slot is always held by + // now: `plan_to_vk_av1` assigned it.) + assert_eq!(reference_count, vk.refs.len()); + let mut bound: Vec = scope.iter().map(|e| e.slot_index).collect(); + assert_eq!(bound.pop(), Some(-1), "frame {frames}: no activation entry"); + bound.sort_unstable(); + let mut expected: Vec = held_slots + .iter() + .filter(|slot| usize::from(**slot) != setup) + .map(|slot| i32::from(*slot)) + .collect(); + expected.sort_unstable(); + assert_eq!( + bound, expected, + "frame {frames}: the coding scope must bind exactly the held \ + slots, once each" + ); + + // THE assertion: each reference's scope entry must carry the image + // that reference was decoded into. A slot recycled into the setup + // picture binds an image too — the wrong one — and nothing but this + // would say so. + for (entry, r) in scope[..reference_count].iter().zip(&vk.refs) { + let decoded_into = image_of[&r.id]; + assert_eq!( + entry.view, + image_view(decoded_into), + "frame {frames}: reference picture {} (slot {}) binds pool \ + image {:?}, but it was decoded into image {decoded_into}", + r.id, + r.slot, + slot_image[usize::from(r.slot)] + ); + assert_ne!( + decoded_into, dst, + "frame {frames}: reference picture {} resolves to the image \ + this very frame is decoding into", + r.id + ); + scope_refs += 1; + } + + // Post-submit bookkeeping, in `decode_planned`'s order. + pictures[dst].pending = true; + pictures[dst].bound = true; + slot_image[setup] = Some(dst); + slot_refs[setup] = Some(vk.setup_ref); + for r in &vk.refs { + slot_refs[usize::from(r.slot)] = Some(r.std); + } + for &id in &vk.release_after_decode { + assert!(slots.release(id), "frame {frames}: deferred release missed"); + } + pending.insert(setup_id, dst); + image_of.insert(setup_id, dst); + + let (ready, dropped) = + settle_dpb_ids(&mut pending, &plan.dpb.outputs, &plan.dpb.removed); + for image in ready.into_iter().chain(dropped) { + // A consumer that displays and releases at once: the harshest + // case for the pool, because an image comes back free the + // instant nothing else pins it. + pictures[image].pending = false; + } + for &id in &plan.dpb.removed { + image_of.remove(&id); + } + if plan.header.refresh_frame_flags == 0 { + slots.release(setup_id); + if let Some(image) = pending.remove(&setup_id) { + pictures[image].pending = false; + } + image_of.remove(&setup_id); + } + } + } + + assert_eq!(frames, 274, "every frame of the vector must decode"); + // Anti-vacuity. Releasing a displaced reference eagerly — the shape every + // codec in this crate shipped with — gave its slot to the decode target on + // 268 of these 274 frames, measured, the first at frame 6 (AU 4 of the + // stream, which is the AU the driver refused). So if this count ever + // reaches 0 the vector stopped exercising the case and the assertions above + // are comparing empty lists. + assert_eq!( + deferring, 268, + "268 of 274 frames displace a picture they are reading; at zero, \ + `release_after_decode` could be deleted and nothing here would fail" + ); + assert_eq!( + scope_refs, 1616, + "the references actually bound into a coding scope across the vector" + ); + eprintln!( + "frames {frames} · scope references {scope_refs} · deferred releases {deferring}" + ); + } } diff --git a/crates/pf-vkdecode/src/params_av1.rs b/crates/pf-vkdecode/src/params_av1.rs index 970ba5bc..9c72fb4f 100644 --- a/crates/pf-vkdecode/src/params_av1.rs +++ b/crates/pf-vkdecode/src/params_av1.rs @@ -62,6 +62,13 @@ impl std::fmt::Display for ParamsAv1Error { impl std::error::Error for ParamsAv1Error {} /// The converted sequence header plus the heap allocations its pointers target. +/// +/// ⚠⚠ **This must outlive the `VkVideoSessionParametersKHR` it is handed to, not +/// merely the create call.** A driver in this fleet keeps `pColorConfig` and reads +/// it at every decode; `session_av1::StoredParamsAv1` is where that is enforced and +/// where the measurement lives. Boxed backing (rather than inline arrays) is what +/// makes storing the wrapper enough — moving it does not move the blocks, which +/// `moving_the_wrapper_leaves_the_driver_s_pointers_put` pins. #[derive(Debug)] pub struct OwnedStdAv1SequenceHeader { std: hh::StdVideoAV1SequenceHeader, @@ -211,3 +218,70 @@ pub fn sequence_to_std( _timing_backing: timing_backing, }) } + +#[cfg(test)] +mod tests { + use super::*; + + /// The wrapper may be MOVED — into the session's stored parameters, out of a + /// `Result`, into a struct literal — without disturbing the addresses a driver + /// has already been given. + /// + /// Not a Rust triviality worth skipping: it is the whole reason + /// [`crate::session_av1`] can fix its use-after-free by storing this value + /// rather than by boxing it or pinning it. It holds because the two blocks are + /// `Box`ed; an "optimisation" that inlined either one as a field would keep + /// every other test in this crate green, keep compiling, and hand the driver a + /// pointer into a moved-from stack slot. The AV1 parity leg would catch it on + /// hardware — this catches it in ordinary CI. + #[test] + fn moving_the_wrapper_leaves_the_driver_s_pointers_put() { + let seq = SequenceHeaderObu { + max_frame_width_minus_1: 319, + max_frame_height_minus_1: 239, + timing_info_present_flag: true, + ..Default::default() + }; + let owned = sequence_to_std(&seq).expect("a plain 8-bit header converts"); + let (colour, timing) = (owned.std().pColorConfig, owned.std().pTimingInfo); + assert!(!colour.is_null(), "pColorConfig is always attached"); + assert!(!timing.is_null(), "this header states timing info"); + + // Every move a session parameters object's creation puts it through. + let moved = owned; + let boxed = Box::new(moved); + let stored = (*boxed, 0u8); + let owned = stored.0; + + assert_eq!(owned.std().pColorConfig, colour); + assert_eq!(owned.std().pTimingInfo, timing); + // And they still address the wrapper's own live blocks, not stale copies. + // SAFETY: `owned` is alive here and owns both blocks. + let subsampling = unsafe { ((*colour).subsampling_x, (*colour).subsampling_y) }; + assert_eq!( + subsampling, + (0, 0), + "the fixture's colour config, read back" + ); + } + + /// A stream without timing info gets a NULL `pTimingInfo`, and that is a + /// deliberate statement rather than an omission. + /// + /// Measured on NVIDIA 610.57.04: with the backing held for the parameters + /// object's life, all 250 frames of the vendored vector are bit-identical to + /// libavcodec with this pointer NULL. libavcodec always attaches a zeroed block + /// instead; both work. Attaching one here would claim a frame rate the stream + /// never stated, so the null stays — but if a future driver refuses it, this is + /// the line to change and the sentence to delete. + #[test] + fn a_stream_without_timing_info_sends_no_timing_block() { + let seq = SequenceHeaderObu { + timing_info_present_flag: false, + ..Default::default() + }; + let owned = sequence_to_std(&seq).expect("converts"); + assert!(owned.std().pTimingInfo.is_null()); + assert_eq!(owned.std().flags.timing_info_present_flag(), 0); + } +} diff --git a/crates/pf-vkdecode/src/pic_av1.rs b/crates/pf-vkdecode/src/pic_av1.rs index dd7c585c..28da7f11 100644 --- a/crates/pf-vkdecode/src/pic_av1.rs +++ b/crates/pf-vkdecode/src/pic_av1.rs @@ -29,6 +29,7 @@ //! first loss. use ash::vk::native as hh; +use pf_bitstream::av1::coded_cdef_sec_strength; use pf_bitstream::av1::AuPlan; use pf_bitstream::av1::PicId; use pf_bitstream::av1::REFS_PER_FRAME; @@ -82,6 +83,26 @@ pub struct DecodePlanVkAv1 { /// The unique referenced pictures of this frame, first appearance first. The /// backend lays `pReferenceSlots` out in THIS order. pub refs: Vec, + /// Pictures this frame's own `refresh_frame_flags` displaces from the store + /// while THIS frame still reads them — their slots are released only once the + /// decode op has been recorded, and the caller owes exactly that. + /// + /// AV1 applies `refresh_frame_flags` AFTER the frame is decoded (7.20), so a + /// frame reading a slot and overwriting it is ordinary rather than exotic: + /// `ref_frame_idx` resolves against the pre-decode store and the refresh lands + /// behind it. The picture is therefore a LIVE reference for exactly this decode + /// op and its DPB slot may not be recycled until the op is submitted. Releasing + /// it inside this conversion — which is what the H.264 and H.265 siblings do + /// with their whole `removed` list — hands its slot straight back to + /// [`Self::setup_slot`], because the lowest free slot is the one just vacated: + /// [`Self::refs`] then names the very slot the decode target activates, and the + /// decoder's binding sync clears its image on the way past. Measured on the + /// vendored vector at frame 6 of 274 (`slot_recycling_waits_for_the_decode_op`). + /// + /// Empty for the overwhelming majority of frames; the ids are always a subset + /// of the plan's `dpb.removed`, so applying them completes that plan's + /// bookkeeping and never invents a removal. + pub release_after_decode: Vec, } /// The Std picture info plus the heap allocations its eight pointers target. @@ -238,8 +259,19 @@ pub fn plan_to_vk_av1( let setup_ref = reference_info(&pf_bitstream::av1::RefState::of(header))?; // --- mutations, after every fallible step ----------------------------- + // A picture this frame READS may be displaced by this same frame's refresh — + // see `DecodePlanVkAv1::release_after_decode` for why that is ordinary AV1 and + // what releasing it here would cost. Its slot survives the assignment below and + // is handed to the caller to release once the decode op is recorded. + let release_after_decode: Vec = plan + .dpb + .removed + .iter() + .copied() + .filter(|id| *id != setup_id && refs.iter().any(|r| r.id == *id)) + .collect(); for &id in &plan.dpb.removed { - if id == setup_id { + if id == setup_id || release_after_decode.contains(&id) { continue; } let _ = slots.release(id); @@ -259,6 +291,7 @@ pub fn plan_to_vk_av1( setup_ref, setup_id, refs, + release_after_decode, }) } @@ -397,6 +430,14 @@ fn picture_info( let loop_filter = Box::new(lf_std); // CDEF. + // + // ⚠ The SECONDARY strengths are the CODED two-bit values, and the parser does + // not hold them: AV1 5.9.19 mutates the syntax element in place (`== 3` becomes + // 4) and cros-codecs follows the spec, while libavcodec sends CBS's unmodified + // two-bit read and every driver was validated against that. Sending 4 overflows + // the two bits VA-API, NVDEC and DXVA all give the field, so the strongest + // secondary filter reads back as NO filter. `coded_cdef_sec_strength` is the + // inverse, and its docs carry the four-API evidence. let c = &p.cdef_params; // SAFETY: see above. let mut c_std: hh::StdVideoAV1CDEF = unsafe { std::mem::zeroed() }; @@ -404,9 +445,9 @@ fn picture_info( c_std.cdef_bits = narrow("cdef_bits", c.cdef_bits)?; for i in 0..8 { c_std.cdef_y_pri_strength[i] = c.cdef_y_pri_strength[i] as u8; - c_std.cdef_y_sec_strength[i] = c.cdef_y_sec_strength[i] as u8; + c_std.cdef_y_sec_strength[i] = coded_cdef_sec_strength(c.cdef_y_sec_strength[i]); c_std.cdef_uv_pri_strength[i] = c.cdef_uv_pri_strength[i] as u8; - c_std.cdef_uv_sec_strength[i] = c.cdef_uv_sec_strength[i] as u8; + c_std.cdef_uv_sec_strength[i] = coded_cdef_sec_strength(c.cdef_uv_sec_strength[i]); } let cdef = Box::new(c_std); @@ -684,6 +725,24 @@ mod tests { "../../pf-bitstream/vendor/cros-codecs/src/codec/av1/test_data/test-25fps.ivf.av1" ); + /// Convert one plan the way a BACKEND must: the conversion, then the releases + /// it defers past the decode op ([`DecodePlanVkAv1::release_after_decode`]). + /// + /// Not a convenience — it is the caller's half of the contract. A loop that + /// converts without it leaks a slot on nearly every frame of this vector (268 + /// of 274) and runs the nine-slot ledger dry inside ten frames, so any test + /// walking the vector has to speak it. + fn convert(plan: &AuPlan, slots: &mut SlotMap) -> DecodePlanVkAv1 { + let vk = plan_to_vk_av1(plan, slots).expect("the clean vector converts"); + for &id in &vk.release_after_decode { + assert!( + slots.release(id), + "a deferred release named picture {id}, which holds no slot" + ); + } + vk + } + /// Convert every frame of the vendored vector and check the parts a driver /// reads against each other. /// @@ -707,7 +766,7 @@ mod tests { if plan.dpb.stored.is_none() { continue; // show_existing_frame: no submission } - let vk = plan_to_vk_av1(&plan, &mut slots).expect("the clean vector converts"); + let vk = convert(&plan, &mut slots); frames += 1; // Every named slot must be one the decode op will bind. @@ -735,6 +794,20 @@ mod tests { } } } + // No reference may share the decode target's slot — the assertion + // the H.264 and H.265 conversion tests have carried since M2, and + // the one this file was missing. A frame whose own refresh + // displaces a picture it READS had its slot recycled straight into + // `setup_slot`, so `refs` named the slot the decode target + // activates: the hardware would predict from the picture it is in + // the middle of writing. Frame 6 of this vector does it. + for r in &vk.refs { + assert_ne!( + r.slot, vk.setup_slot, + "frame {frames}: reference (picture {}) aliases the setup slot", + r.id + ); + } // The setup picture must hold the slot the plan says it does. assert_eq!(slots.slot_of(vk.setup_id), Some(vk.setup_slot)); } @@ -754,6 +827,87 @@ mod tests { ); } + /// A frame that READS a slot its own refresh overwrites keeps that slot until + /// the decode op is recorded — and the incidence is pinned, because it is the + /// ordinary case rather than the exotic one. + /// + /// AV1 applies `refresh_frame_flags` after decoding (7.20), so `ref_frame_idx` + /// resolves against the store as it stood BEFORE the frame. Cycling eight slots + /// in a low-delay stream therefore means almost every frame displaces something + /// it is reading: **268 of this vector's 274 frames**, first at frame 6. The + /// H.264 and H.265 planners can produce the same shape — `plan_to_vk`'s own + /// docs name the sliding window evicting a picture the slices reference — but + /// neither vendored vector ever does it (measured: zero on the 250-AU H.264 + /// clip), which is why the hole survived two hardware-proven codecs and opened + /// on the first AV1 frame that was not a key frame's neighbour. + #[test] + fn a_reference_this_frame_displaces_keeps_its_slot_until_after_the_decode() { + let mut planner = Av1Planner::new(); + let mut slots = SlotMap::new(NUM_REF_SLOTS); + let (mut frames, mut deferring, mut deferred_ids) = (0u32, 0u32, 0u32); + let mut peak_active = 0usize; + + for packet in IvfIterator::new(AV1_25FPS) { + for plan in planner.plan_au(packet).expect("the clean vector plans") { + if plan.dpb.stored.is_none() { + continue; + } + let vk = plan_to_vk_av1(&plan, &mut slots).expect("converts"); + frames += 1; + peak_active = peak_active.max(slots.active()); + + // Every deferred id is one this plan really removed AND this frame + // really reads — never an invented removal, never a live picture. + for &id in &vk.release_after_decode { + assert!( + plan.dpb.removed.contains(&id), + "frame {frames}: deferred picture {id} is not in this plan's \ + removed list" + ); + assert!( + vk.refs.iter().any(|r| r.id == id), + "frame {frames}: picture {id} is deferred without being read — \ + only a reference of THIS frame earns the reprieve" + ); + assert!( + slots.slot_of(id).is_some(), + "frame {frames}: a deferred picture must still hold its slot" + ); + } + // And the whole point: the slot it still holds is not the one the + // decode target just took. + for r in &vk.refs { + assert_ne!(r.slot, vk.setup_slot); + } + if !vk.release_after_decode.is_empty() { + deferring += 1; + deferred_ids += vk.release_after_decode.len() as u32; + } + for &id in &vk.release_after_decode { + assert!(slots.release(id)); + } + } + } + + assert_eq!(frames, 274); + assert_eq!( + deferring, 268, + "268 of 274 frames of this vector displace a picture they are reading; \ + at zero this test compares an empty list against itself and the \ + deferral could be deleted without a single assertion noticing" + ); + assert_eq!(deferred_ids, 268, "one displaced reference per frame here"); + assert!( + peak_active <= slots.capacity(), + "deferring a release must not overrun the ledger" + ); + // The nine-slot ledger is `NUM_REF_SLOTS + 1` and holding a displaced + // reference one frame longer is exactly what that spare slot is for. If + // this ever reaches capacity the sizing argument needs re-reading, not a + // bigger number. + eprintln!("frames {frames} · deferring {deferring} · peak slots held {peak_active}"); + } + /// Every `StdVideoDecodeAV1PictureInfoFlags` bit this conversion is responsible /// for, checked against the parsed header on all 274 frames — with the /// INCIDENCE of each pinned, so a bit that silently stopped being written @@ -776,7 +930,7 @@ mod tests { if plan.dpb.stored.is_none() { continue; } - let vk = plan_to_vk_av1(&plan, &mut slots).expect("converts"); + let vk = convert(&plan, &mut slots); let f = &vk.pic.std().flags; let p = &*plan.header; frames += 1; @@ -877,6 +1031,171 @@ mod tests { ); } + /// `StdVideoAV1LoopFilter` carries FOUR levels, and the last two are chroma. + /// + /// ⚠ **Read the history before trusting an older comment about this field.** + /// The AV1 rung's frame-0 divergence was `luma IDENTICAL, chroma 319/38400 + /// bytes differ, max |delta| 4` on an RTX 5070 Ti (610.57.04), and re-decoding + /// frame 0 in software with `loop_filter_level[2]` and `[3]` forced to zero + /// reproduced it exactly — same 319 bytes, same `1:219 2:64 3-4:36` histogram, + /// same first six differing bytes. That reading was right about the SYMPTOM + /// and wrong about the cause: the conclusion drawn from it, that the driver + /// ignores these two levels, is **refuted**. It reads them. What it also read, + /// at every `vkCmdDecodeVideoKHR`, was a FREED sequence header whose recycled + /// bytes happened to say `mono_chrome = 1` — so it deblocked the frame as + /// monochrome, which skips exactly `loop_filter_level[2..3]` (7.14) and + /// nothing else. [`crate::session_av1`] carries that measurement and the fix; + /// with the backing held, all 250 frames are bit-identical to libavcodec. + /// + /// So this stays, as the guard it always was rather than as evidence for a + /// driver claim. The conversion is a whole-array assignment and the test + /// therefore looks tautological. It is not: `loop_filter_level` is the ONE Std + /// array whose entries mean different things at different indices — `[0]` and + /// `[1]` are the luma passes, `[2]` and `[3]` are U and V — and both of the + /// other rungs spell it as a two-entry array plus two named fields + /// (`filter_level_u` / `filter_level_v` in DXVA, the same in VA-API). A + /// conversion that copied "the levels" as a pair is the natural mistake, it is + /// what the DXVA layout invites, and nothing else in this file would notice. + /// The values below are additionally confirmed against what libavcodec's own + /// Vulkan hwaccel puts on the wire for this vector, captured at the API with a + /// layer: `03 00 00 00 01 07 08 0c 00 00 01 00 00 00 ff 00 ff ff 00 00 …`. + #[test] + fn the_chroma_deblocking_levels_are_the_last_two_of_four() { + let mut planner = Av1Planner::new(); + let mut slots = SlotMap::new(NUM_REF_SLOTS); + let (mut frames, mut with_chroma_lf) = (0u32, 0u32); + + for packet in IvfIterator::new(AV1_25FPS) { + for plan in planner.plan_au(packet).expect("plans") { + if plan.dpb.stored.is_none() { + continue; + } + let vk = convert(&plan, &mut slots); + frames += 1; + let lf = &plan.header.loop_filter_params; + // SAFETY: `pLoopFilter` points at the boxed block `vk.pic` owns, + // alive for as long as `vk` is. + let sent = unsafe { *vk.pic.std().pLoopFilter }; + assert_eq!( + sent.loop_filter_level, lf.loop_filter_level, + "frame {frames}: all four levels, in order — [0] and [1] the \ + luma passes, [2] U, [3] V" + ); + assert_eq!(sent.loop_filter_sharpness, lf.loop_filter_sharpness); + assert_eq!(sent.loop_filter_ref_deltas, lf.loop_filter_ref_deltas); + assert_eq!(sent.loop_filter_mode_deltas, lf.loop_filter_mode_deltas); + if lf.loop_filter_level[2] != 0 || lf.loop_filter_level[3] != 0 { + with_chroma_lf += 1; + } + if frames == 1 { + assert_eq!( + sent.loop_filter_level, + [1, 7, 8, 12], + "frame 0's levels, and the four bytes libavcodec's Vulkan \ + hwaccel was captured sending for this same frame" + ); + assert_eq!( + sent.loop_filter_ref_deltas, + [1, 0, 0, 0, -1, 0, -1, -1], + "a PRIMARY_REF_NONE frame gets the spec's defaults from \ + setup_past_independence, and they bump every level by one — \ + luma included, which is how the parity leg's bit-exact luma \ + proves the driver read the deltas and the first two levels" + ); + } + } + } + + assert_eq!(frames, 274); + assert_eq!( + with_chroma_lf, 123, + "123 of 274 frames of this vector deblock chroma; at zero every level \ + compared above would be zero on both sides and a conversion that sent \ + only the luma pair would pass" + ); + } + + /// `StdVideoAV1CDEF`'s secondary strengths carry the CODED two-bit value. + /// + /// The defect this pins is the twin of the `LoopRestorationSize` one below: + /// the vendored parser stores what the AV1 SPEC leaves in the variable after + /// its in-place fixup (`== 3` becomes 4), and every decode API — Vulkan + /// included, because libavcodec sends CBS's unmodified two-bit read — wants the + /// value BEFORE it. It is worse than the loop-restoration one in exactly one + /// way: 4 is not an absurd number a driver would reject, it is a number that + /// overflows a two-bit field into 0, so the strongest secondary CDEF filter + /// becomes NO secondary filter and the frame is merely slightly wrong. + /// + /// Frame 0 of the vector codes it, which is why this was the AV1 parity leg's + /// FIRST divergent frame, and CDEF is in-loop, which is why every frame after + /// it diverged too. + #[test] + fn cdef_secondary_strengths_are_the_coded_value_not_the_spec_fixup() { + let mut planner = Av1Planner::new(); + let mut slots = SlotMap::new(NUM_REF_SLOTS); + let (mut frames, mut corrected_frames) = (0u32, 0u32); + + for packet in IvfIterator::new(AV1_25FPS) { + for plan in planner.plan_au(packet).expect("plans") { + if plan.dpb.stored.is_none() { + continue; + } + let vk = convert(&plan, &mut slots); + frames += 1; + let raw = &plan.header.cdef_params; + // SAFETY: `pCDEF` points at the boxed block `vk.pic` owns, alive + // for as long as `vk` is. + let sent = unsafe { *vk.pic.std().pCDEF }; + let mut corrected = false; + for i in 0..8 { + // Two bits is all any hardware API gives this field. + assert!( + sent.cdef_y_sec_strength[i] <= 3 && sent.cdef_uv_sec_strength[i] <= 3, + "frame {frames}: a secondary strength above 3 overflows the \ + two bits VA-API, NVDEC and DXVA pack it into" + ); + // The primaries are NOT fixed up by the spec and must reach the + // driver untouched — a correction applied to the wrong one of + // the four arrays would be just as silent. + assert_eq!( + u32::from(sent.cdef_y_pri_strength[i]), + raw.cdef_y_pri_strength[i] + ); + assert_eq!( + u32::from(sent.cdef_uv_pri_strength[i]), + raw.cdef_uv_pri_strength[i] + ); + if raw.cdef_y_sec_strength[i] == 4 || raw.cdef_uv_sec_strength[i] == 4 { + corrected = true; + } + } + if corrected { + corrected_frames += 1; + } + if frames == 1 { + let coded = 1usize << raw.cdef_bits; + assert_eq!(coded, 4, "frame 0 codes cdef_bits = 2"); + assert_eq!( + ( + &sent.cdef_y_sec_strength[..coded], + &sent.cdef_uv_sec_strength[..coded] + ), + (&[1u8, 2, 0, 3][..], &[3u8, 0, 0, 0][..]), + "frame 0's secondary strengths as libavcodec sends them — \ + the parser holds 4 where these read 3" + ); + } + } + } + + assert_eq!(frames, 274); + assert_eq!( + corrected_frames, 68, + "68 of 274 frames of this vector need the correction; at zero this test \ + compares an untouched conversion against itself" + ); + } + /// `LoopRestorationSize` carries the CODED value, not the pixel size. /// /// Three frames of the vector switch loop restoration on, at @@ -895,7 +1214,7 @@ mod tests { if plan.dpb.stored.is_none() { continue; } - let vk = plan_to_vk_av1(&plan, &mut slots).expect("converts"); + let vk = convert(&plan, &mut slots); frames += 1; let lr = &plan.header.loop_restoration_params; // SAFETY: `pLoopRestoration` points at the boxed block `vk.pic` @@ -944,7 +1263,7 @@ mod tests { if plan.dpb.stored.is_none() { continue; } - let vk = plan_to_vk_av1(&plan, &mut slots).expect("converts"); + let vk = convert(&plan, &mut slots); frames += 1; let current_type = plan.header.frame_type as u8; diff --git a/crates/pf-vkdecode/src/session.rs b/crates/pf-vkdecode/src/session.rs index 36d4ee49..4b6a0426 100644 --- a/crates/pf-vkdecode/src/session.rs +++ b/crates/pf-vkdecode/src/session.rs @@ -416,8 +416,18 @@ impl VideoSession { /// # Safety /// /// Live device + live session; the Std slices' backing (the `OwnedStd*` - /// wrappers) outlives this call — Vulkan copies all parameter data before - /// returning. + /// wrappers) outlives this call. + /// + /// ⚠ "Vulkan copies all parameter data before returning" is what this used to + /// say, and it is **not universally true**: NVIDIA 610.57.04 was measured + /// retaining `StdVideoAV1SequenceHeader::pColorConfig` and dereferencing it at + /// every `vkCmdDecodeVideoKHR` ([`crate::session_av1`]). The H.26x rungs are + /// bit-exact on four drivers with the backings dropped here, so nothing is + /// known to be wrong — but the H.264 and H.265 Std sets carry embedded pointers + /// too (`pScalingLists`, `pSequenceParameterSetVui`, `pOffsetForRefFrame`, and + /// H.265's seven), and this contract rests on a driver behaviour rather than on + /// ownership. Holding them for the object's life, as AV1 now does, is the + /// version of this that cannot rot. unsafe fn create_parameters_object( &self, sps: &[hh::StdVideoH264SequenceParameterSet], @@ -502,8 +512,9 @@ impl VideoSession { .update_sequence_count(self.ledger.next_update_seq()) .push_next(&mut add); // SAFETY: live device + parameters object; `update` roots locals - // (incl. the OwnedStd backings) outliving the call, and Vulkan - // copies parameter data before returning. + // (incl. the OwnedStd backings) outliving the call. See + // `create_parameters_object` on why "and the driver copies them" + // is an observation about this fleet rather than a guarantee. let r = unsafe { (self.video_queue.fp().update_video_session_parameters_khr)( self.device.handle(), diff --git a/crates/pf-vkdecode/src/session_av1.rs b/crates/pf-vkdecode/src/session_av1.rs index c8aca73a..7459806b 100644 --- a/crates/pf-vkdecode/src/session_av1.rs +++ b/crates/pf-vkdecode/src/session_av1.rs @@ -24,6 +24,19 @@ //! `ensure_parameters` before every submission and nothing else may create the //! session's coding scope. //! +//! ⚠⚠⚠ **The Std sequence header's heap blocks must outlive the parameters +//! OBJECT, not just the create call.** Vulkan reads as though parameter data were +//! captured by `vkCreateVideoSessionParametersKHR`, and this module assumed it — +//! [`sequence_to_std`]'s wrapper was a local, dropped the moment the call +//! returned. NVIDIA 610.57.04 keeps the pointer instead and dereferences +//! `pColorConfig` when a decode is RECORDED, so every AV1 frame was decoded +//! against whatever the allocator had since put in those 24 bytes. Measured on an +//! RTX 5070 Ti: correct at create, `23 00 00 00 00 00 00 00 77 29 …` by the first +//! `vkCmdDecodeVideoKHR` — which reads as `mono_chrome = 1`, so the driver +//! deblocked the frame as monochrome and skipped `loop_filter_level[2..3]` +//! entirely. That is the whole of the AV1 rung's parity gap (250/250 frames +//! divergent; 0/250 with the backing held, [`StoredParamsAv1`]). +//! //! `ParamsLedgerAv1` is the pure half of the decision (unit-tested); //! [`VideoSessionAv1`] is the thin Vulkan half. @@ -38,6 +51,7 @@ use crate::caps_av1::Av1ProfileChain; use crate::caps_av1::Av1ProfileKey; use crate::device::DecodeDevice; use crate::params_av1::sequence_to_std; +use crate::params_av1::OwnedStdAv1SequenceHeader; use crate::session::bind_session_memory; use crate::session::ResetArm; use crate::session::SessionError; @@ -99,15 +113,30 @@ pub struct SessionConfigAv1 { pub profile: Av1ProfileKey, } +/// A live parameters object **and the Std sequence header it was created from**, +/// in one field — because the two may not drift apart. +/// +/// The wrapper is not decoration and not defensive: the driver dereferences the +/// header's `pColorConfig` long after the create call returned (module docs), so +/// dropping the backing early hands it freed memory. One field rather than two +/// makes "an object whose backing is gone" unrepresentable, which is the only +/// shape of this bug — and the shape a `let owned = …;` local silently had. +struct StoredParamsAv1 { + object: vk::VideoSessionParametersKHR, + /// Held for the OBJECT's whole life. Never read by this crate after the + /// create call; the DRIVER reads it. + _sequence: OwnedStdAv1SequenceHeader, +} + /// The Vulkan half: session + bound memory + parameters object. pub(crate) struct VideoSessionAv1 { device: ash::Device, video_queue: ash::khr::video_queue::Device, session: vk::VideoSessionKHR, memory: Vec, - /// NULL until the first [`Self::ensure_parameters`] — an AV1 parameters object - /// has no empty form (module docs). - parameters: vk::VideoSessionParametersKHR, + /// `None` until the first [`Self::ensure_parameters`] — an AV1 parameters + /// object has no empty form (module docs). + parameters: Option, ledger: ParamsLedgerAv1, pub(crate) config: SessionConfigAv1, /// The session has never run a coding scope: the first one records a @@ -159,7 +188,7 @@ impl VideoSessionAv1 { video_queue: dev.video_queue().clone(), session, memory: Vec::new(), - parameters: vk::VideoSessionParametersKHR::null(), + parameters: None, ledger: ParamsLedgerAv1::default(), config, needs_reset: ResetArm::armed(), @@ -194,7 +223,7 @@ impl VideoSessionAv1 { /// [`Self::parameters_action`]: the FIRST `Recreate` of a session's life /// destroys nothing and needs no drain, every later one does. pub(crate) fn has_parameters(&self) -> bool { - self.parameters != vk::VideoSessionParametersKHR::null() + self.parameters.is_some() } /// Make the parameters object hold this frame's active sequence header. @@ -219,9 +248,12 @@ impl VideoSessionAv1 { "creating AV1 session parameters (first activation or a \ sequence-header content change)" ); - // The owned wrapper stays alive until after the create call: the - // Std struct embeds pointers into its heap blocks (the colour - // config, and the timing info when present). + // ⚠ The owned wrapper is MOVED INTO the stored parameters below + // and lives as long as the object does — not merely across the + // create call. Module docs carry the measurement; the short of it + // is that a driver in this fleet dereferences `pColorConfig` at + // every `vkCmdDecodeVideoKHR`, so an early drop decodes the whole + // stream against recycled heap. let owned = sequence_to_std(sequence).map_err(SessionError::ParamsAv1)?; let mut av1 = vk::VideoDecodeAV1SessionParametersCreateInfoKHR::default() .std_sequence_header(owned.std()); @@ -230,8 +262,8 @@ impl VideoSessionAv1 { .push_next(&mut av1); let mut fresh = vk::VideoSessionParametersKHR::null(); // SAFETY: live device + live session; `ci` roots locals (incl. the - // OwnedStd backing) outliving the call, and Vulkan copies all - // parameter data before returning. + // OwnedStd backing, which outlives the call AND the object it + // creates — see the module docs on why the second half matters). let r = unsafe { (self.video_queue.fp().create_video_session_parameters_khr)( self.device.handle(), @@ -243,22 +275,29 @@ impl VideoSessionAv1 { if r != vk::Result::SUCCESS { return Err(SessionError::Vk(r)); } - // Destroying NULL is defined as a no-op, so the first activation - // falls through here without a special case. - // - // SAFETY: the fn-level contract — the caller drained every - // in-flight decode before a Recreate over an existing object - // reached here (checked via parameters_action + has_parameters), - // so no submitted work reads the old object; it is this session's - // own handle. - unsafe { - (self.video_queue.fp().destroy_video_session_parameters_khr)( - self.device.handle(), - self.parameters, - std::ptr::null(), - ); + // The old object goes FIRST and its backing with it — taking the + // whole `StoredParamsAv1` keeps the destroy ahead of the free, + // which is the order a driver holding the pointer needs. + if let Some(old) = self.parameters.take() { + // SAFETY: the fn-level contract — the caller drained every + // in-flight decode before a Recreate over an existing object + // reached here (checked via parameters_action + + // has_parameters), so no submitted work reads the old object; + // it is this session's own handle, on a live device. + unsafe { + (self.video_queue.fp().destroy_video_session_parameters_khr)( + self.device.handle(), + old.object, + std::ptr::null(), + ); + } + // `old` (and its sequence-header blocks) drops here, after the + // object that pointed at them is gone. } - self.parameters = fresh; + self.parameters = Some(StoredParamsAv1 { + object: fresh, + _sequence: owned, + }); self.ledger.commit(action, sequence); Ok(()) } @@ -271,6 +310,8 @@ impl VideoSessionAv1 { pub(crate) fn parameters(&self) -> vk::VideoSessionParametersKHR { self.parameters + .as_ref() + .map_or(vk::VideoSessionParametersKHR::null(), |p| p.object) } /// Whether the next coding scope must record the initialization RESET — @@ -298,11 +339,13 @@ impl Drop for VideoSessionAv1 { // not stylistic: memory bound into a session may not be freed while the // session lives, so the session is destroyed first — which is also why a // failed bind hands its allocations back here instead of freeing them - // itself (`crate::session::BindFailure`). + // itself (`crate::session::BindFailure`). The sequence-header backing is + // freed after both, by the field's own drop, for the same reason + // `ensure_parameters` destroys before it replaces. unsafe { (self.video_queue.fp().destroy_video_session_parameters_khr)( self.device.handle(), - self.parameters, + self.parameters(), std::ptr::null(), ); (self.video_queue.fp().destroy_video_session_khr)( diff --git a/crates/pf-vkdecode/src/session_h265.rs b/crates/pf-vkdecode/src/session_h265.rs index 1fee642b..df0afbb7 100644 --- a/crates/pf-vkdecode/src/session_h265.rs +++ b/crates/pf-vkdecode/src/session_h265.rs @@ -336,7 +336,13 @@ impl VideoSessionH265 { /// /// Live device + live session; the Std slices' backing (the `OwnedStd*` /// wrappers, INCLUDING the heap blocks their embedded pointers target) - /// outlives this call — Vulkan copies all parameter data before returning. + /// outlives this call. + /// + /// ⚠ This used to end "— Vulkan copies all parameter data before returning", + /// and that is **not universally true**: see [`crate::session::VideoSession`]'s + /// twin of this comment and [`crate::session_av1`], where a driver was measured + /// dereferencing a retained pointer long after the create call. H.265's Std SPS + /// carries SEVEN embedded pointers, more than any other set in this crate. unsafe fn create_parameters_object( &self, vps: &[hh::StdVideoH265VideoParameterSet], @@ -441,8 +447,9 @@ impl VideoSessionH265 { .update_sequence_count(self.ledger.next_update_seq()) .push_next(&mut add); // SAFETY: live device + parameters object; `update` roots locals - // (incl. the OwnedStd backings) outliving the call, and Vulkan - // copies parameter data before returning. + // (incl. the OwnedStd backings) outliving the call. See + // `create_parameters_object` on why "and the driver copies them" + // is an observation about this fleet rather than a guarantee. let r = unsafe { (self.video_queue.fp().update_video_session_parameters_khr)( self.device.handle(), diff --git a/crates/pf-vkdecode/tests/data/test-25fps-av1.frame0.nv12 b/crates/pf-vkdecode/tests/data/test-25fps-av1.frame0.nv12 new file mode 100644 index 00000000..1f526a2b Binary files /dev/null and b/crates/pf-vkdecode/tests/data/test-25fps-av1.frame0.nv12 differ diff --git a/crates/pf-vkdecode/tests/gpu_parity.rs b/crates/pf-vkdecode/tests/gpu_parity.rs index 8c02a8a6..3b1724a8 100644 --- a/crates/pf-vkdecode/tests/gpu_parity.rs +++ b/crates/pf-vkdecode/tests/gpu_parity.rs @@ -93,6 +93,22 @@ const GOLDENS_H265: &str = include_str!("data/test-25fps-h265.nv12.sha256"); /// file's header — it is the only golden here with a third-party corroboration). const GOLDENS_AV1: &str = include_str!("data/test-25fps-av1.nv12.sha256"); +/// The AV1 vector's FIRST frame, as libavcodec decodes it: the 320x240 render +/// region, tightly packed NV12, 115200 bytes — the same bytes +/// [`GOLDENS_AV1`]'s first line hashes. +/// +/// Hashes tell you a frame is wrong; only pixels tell you HOW. This exists for +/// [`av1_frame0_pixels_say_which_plane_and_how_badly`], whose whole job is to turn +/// "FIRST DIVERGENT FRAME = 0" into a class: luma or chroma, a shift or a +/// difference, a filter's worth of error or a structural one. Frame 0 earns the +/// 113 KiB because it is intra-only — nothing upstream of it can be blamed — and +/// because on this rung it is where a divergence appears first. +/// +/// It cannot drift from the golden set it was cut out of: +/// [`the_av1_frame0_reference_is_the_first_golden`] re-derives its SHA-256 and +/// compares, in ordinary CI, with no GPU. +const AV1_FRAME0: &[u8] = include_bytes!("data/test-25fps-av1.frame0.nv12"); + /// The ten-bit vector and its goldens. No hardware leg in this file consumes them /// yet — the D3D11VA rung is where the ten-bit parity leg currently runs — but the /// files live here, beside the other goldens, so the guard that keeps them honest @@ -985,6 +1001,464 @@ fn av1_every_frame_hashes_bit_identical_to_libavcodec() { av1_parity_run(&common::split_av1_aus(common::TEST_25FPS_AV1), "AV1"); } +/// Frame 0's pixels against libavcodec's, byte for byte — the diagnostic leg. +/// +/// [`av1_every_frame_hashes_bit_identical_to_libavcodec`] is the verdict; this is +/// the microscope, and it decodes only as far as the first delivered frame. A hash +/// mismatch names no cause, and the four causes the parity leg's own message ranks +/// for a frame-0 divergence produce *completely different* pixel signatures: +/// +/// | printed here | what it means | +/// |---|---| +/// | `luma IDENTICAL`, chroma differs | the chroma plane's layout — `PLANE_1`'s copy region, or a pool whose chroma plane starts somewhere other than where the readback reads it. NOT a decode problem | +/// | both differ, and a **shift** matches | readback geometry: the crop origin, or a copy extent taken from the pool rather than the render region. The printed `dy`/`dx` IS the error | +/// | both differ, deltas ≤ ~8 over most of the plane | an in-loop filter parameter — CDEF, loop restoration, the deblocking levels. Small and everywhere is what a filter does, and it is why the whole 250 frames go with it: CDEF runs before the frame is stored as a reference | +/// | both differ, deltas large and structured | quantisation, tile geometry, or the tile payloads themselves — the frame was reconstructed from the wrong data rather than filtered wrongly | +/// | ours is CONSTANT | nothing was decoded into the image the readback read | +/// +/// It asserts equality last, so a failure prints the whole report above the panic. +#[test] +#[ignore = "needs a Vulkan Video AV1 decode device (fleet boxes; see module docs)"] +fn av1_frame0_pixels_say_which_plane_and_how_badly() { + let _gpu = common::gpu_lock(); + std::env::set_var("PF_VKD_TEST_READBACK", "1"); + + let aus = common::split_av1_aus(common::TEST_25FPS_AV1); + assert_eq!( + aus.len(), + FRAME_COUNT, + "the vector must split into 250 units" + ); + let ours = av1_first_frame(&aus); + + report_nv12_divergence(&ours, AV1_FRAME0, DISPLAY_AV1); + assert_eq!( + sha256_hex(&ours), + golden_hashes(GOLDENS_AV1)[0], + "AV1 frame 0 is not libavcodec's — read the report above for the class" + ); + eprintln!("AV1 frame 0 is byte-identical to libavcodec"); +} + +/// `loop_filter_level[2]` and `[3]` — the U and V deblocking levels — as BIT +/// offsets from the start of the vector's FIRST access unit. +/// +/// Derived rather than found: the IVF packet holds a temporal-delimiter OBU (2 +/// bytes), a sequence-header OBU (2 + 11) and an `OBU_FRAME` header (1 + a 2-byte +/// leb128 size), so the uncompressed frame header starts at byte 18. Inside it +/// `loop_filter_level[0]` begins at bit 35 and the four levels are `f(6)` back to +/// back (5.9.11), which puts U at bit 47 and V at bit 53. +/// +/// [`av1_frame0_probes_whether_the_driver_reads_the_chroma_deblocking_levels`] +/// re-parses the mutated unit before it decodes anything, so a re-synced vector +/// makes this fail loudly instead of poking an unrelated field. +const AV1_FRAME0_FILTER_LEVEL_U_BIT: usize = 18 * 8 + 47; +const AV1_FRAME0_FILTER_LEVEL_V_BIT: usize = 18 * 8 + 53; + +/// The strongest deblocking level AV1 can code (`f(6)`), and the value the probe +/// rewrites both chroma levels to. +const MAX_LOOP_FILTER_LEVEL: u8 = 63; + +/// The driver DOES read the chroma deblocking levels — and this is the test that +/// says so, after a pass of this program's history said the opposite. +/// +/// ⚠⚠ **The claim "NVIDIA ignores `StdVideoAV1LoopFilter::loop_filter_level[2..3]`" +/// is refuted. Do not reintroduce it.** It was an honest reading of a real +/// measurement: the AV1 frame-0 parity leg came back `luma IDENTICAL, chroma +/// 319/38400 bytes differ, max |delta| 4`, software re-decode with both chroma +/// levels forced to zero reproduced that signature byte for byte, and this very +/// probe then came back IDENTICAL for `[8, 12]` and `[63, 63]`. Every step was +/// sound; the inference was not. The levels were reaching the driver intact — what +/// was NOT intact was the sequence header, whose `pColorConfig` block this crate +/// freed the instant `vkCreateVideoSessionParametersKHR` returned while the driver +/// went on dereferencing it at every decode. The recycled bytes read as +/// `mono_chrome = 1`, and a monochrome frame skips exactly `loop_filter_level[2..3]` +/// (AV1 7.14) — which is why rewriting them changed nothing, and why the +/// fingerprint was a perfect match for levels that were never applied. +/// `pf-vkdecode`'s `session_av1` module docs carry the capture and the fix. +/// +/// So the probe survives its own refutation, with its verdict inverted: it now +/// PASSES, and it is the cheapest guard there is against that whole class coming +/// back. It decodes frame 0 twice — once from the vector as it sits, once from the +/// same unit with both chroma levels rewritten to the strongest AV1 can code — and +/// requires the pixels to differ. In software that rewrite moves 793 chroma bytes +/// with `max |delta| 29`, which no readback or crop error could hide, and it leaves +/// luma bit-identical, which is the control: a mutation that changed luma would +/// have desynchronised the header rather than changed the field. +/// +/// If the two decodes are ever IDENTICAL again, the message below is the one to +/// act on — and the FIRST thing to check is not the driver but whether some block +/// the decode op points at is being freed before the op is recorded. That is what +/// it was last time, on a bug this test could not see. +#[test] +#[ignore = "needs a Vulkan Video AV1 decode device (fleet boxes; see module docs)"] +fn av1_frame0_probes_whether_the_driver_reads_the_chroma_deblocking_levels() { + let _gpu = common::gpu_lock(); + std::env::set_var("PF_VKD_TEST_READBACK", "1"); + + let aus = common::split_av1_aus(common::TEST_25FPS_AV1); + assert_eq!(aus.len(), FRAME_COUNT); + + let mutated_au = av1_frame0_with_max_chroma_deblocking(aus[0]); + let mut units: Vec<&[u8]> = aus.clone(); + units[0] = &mutated_au; + + let coded = av1_first_frame(&aus); + let maxed = av1_first_frame(&units); + + let luma = (DISPLAY_AV1.0 * DISPLAY_AV1.1) as usize; + eprintln!(" coded chroma levels [8, 12] {}", sha256_hex(&coded)); + eprintln!(" chroma levels [63, 63] {}", sha256_hex(&maxed)); + eprintln!(" libavcodec's frame 0 {}", sha256_hex(AV1_FRAME0)); + assert_eq!( + coded[..luma], + maxed[..luma], + "the chroma deblocking levels must not move a luma sample — if they did, \ + the mutation desynchronised the frame header and the chroma comparison \ + below means nothing" + ); + assert_ne!( + coded[luma..], + maxed[luma..], + "the driver produced the SAME chroma from loop_filter_level[2..3] = [8, 12] \ + and from [63, 63]. This happened once before and the driver was INNOCENT: \ + a monochrome-looking sequence header makes it skip both levels, and ours \ + looked monochrome because its `pColorConfig` block had been freed and \ + reused before the decode op was recorded (see this test's docs). So audit \ + the LIFETIME of everything the submission points at — the Std sequence \ + header behind the parameters object first — before blaming the vendor" + ); + eprintln!("the driver reads the chroma deblocking levels — the two decodes differ"); +} + +/// The vector's first access unit with both CHROMA deblocking levels rewritten to +/// [`MAX_LOOP_FILTER_LEVEL`] — and the proof, through the real parser, that this is +/// the only thing it changed. +/// +/// The proof is not decoration. The offsets are derived from the spec's syntax +/// order rather than searched for, and a rewrite landing one field over would +/// desynchronise nothing (both neighbours are fixed-width) while silently probing +/// the wrong parameter. So every block the conversion reads is compared before and +/// after, and [`the_av1_chroma_deblocking_mutation_changes_only_those_two_levels`] +/// runs this on CPU in ordinary CI — the hardware run cannot be spent discovering +/// that the mutation was wrong. +fn av1_frame0_with_max_chroma_deblocking(au: &[u8]) -> Vec { + let mut mutated = au.to_vec(); + for bit in [AV1_FRAME0_FILTER_LEVEL_U_BIT, AV1_FRAME0_FILTER_LEVEL_V_BIT] { + set_bits(&mut mutated, bit, 6, MAX_LOOP_FILTER_LEVEL); + } + + let before = av1_first_header(au); + let after = av1_first_header(&mutated); + assert_eq!( + before.loop_filter_params.loop_filter_level, + [1, 7, 8, 12], + "the vendored vector's frame 0 codes these levels, and the whole probe is \ + built around the last two of them" + ); + assert_eq!( + after.loop_filter_params.loop_filter_level, + [1, 7, MAX_LOOP_FILTER_LEVEL, MAX_LOOP_FILTER_LEVEL], + "the rewrite must land on the two CHROMA levels and leave the luma pair \ + alone — a luma change would make the probe's control meaningless" + ); + // Everything else the conversion reads, unchanged: a rewrite that shifted the + // header would show up in one of these long before it showed up in pixels. + assert_eq!( + after.cdef_params, before.cdef_params, + "the CDEF block follows the loop filter block and is what a shifted rewrite \ + would corrupt first" + ); + assert_eq!(after.quantization_params, before.quantization_params); + assert_eq!(after.tile_info, before.tile_info); + assert_eq!( + after.loop_restoration_params, + before.loop_restoration_params + ); + assert_eq!(after.segmentation_params, before.segmentation_params); + assert_eq!( + ( + after.loop_filter_params.loop_filter_sharpness, + after.loop_filter_params.loop_filter_ref_deltas, + after.loop_filter_params.loop_filter_mode_deltas, + ), + ( + before.loop_filter_params.loop_filter_sharpness, + before.loop_filter_params.loop_filter_ref_deltas, + before.loop_filter_params.loop_filter_mode_deltas, + ), + "the rest of the loop filter block rides after the levels and must survive" + ); + assert_ne!(mutated, au, "the rewrite must actually change bytes"); + mutated +} + +/// [`av1_frame0_with_max_chroma_deblocking`] on CPU, so the GPU probe's mutation is +/// known-good before any device time is spent on it. +#[test] +fn the_av1_chroma_deblocking_mutation_changes_only_those_two_levels() { + let aus = common::split_av1_aus(common::TEST_25FPS_AV1); + let mutated = av1_frame0_with_max_chroma_deblocking(aus[0]); + // One byte may carry bits of both fields (U ends mid-byte), so the rewrite + // touches two or three bytes and no more — a whole-unit difference would mean + // `set_bits` walked off its field. + let changed = aus[0].iter().zip(&mutated).filter(|(a, b)| a != b).count(); + assert!( + (1..=3).contains(&changed), + "twelve bits spanning at most three bytes, and {changed} bytes changed" + ); +} + +/// Overwrite the `bits`-wide big-endian bitfield at `bit` in `data`. +/// +/// AV1's `f(n)` is MSB-first from the start of the OBU payload, which is what the +/// probe above needs to rewrite a syntax element in place: same width, same +/// position, so nothing after it shifts. +fn set_bits(data: &mut [u8], bit: usize, bits: usize, value: u8) { + for i in 0..bits { + let at = bit + i; + let mask = 1u8 << (7 - (at % 8)); + let set = (value >> (bits - 1 - i)) & 1 == 1; + if set { + data[at / 8] |= mask; + } else { + data[at / 8] &= !mask; + } + } +} + +/// The parsed frame header of the FIRST frame in one access unit. +fn av1_first_header(au: &[u8]) -> pf_bitstream::av1::ParsedFrameHeader { + let mut planner = pf_bitstream::av1::Av1Planner::new(); + let plans = planner.plan_au(au).expect("the unit plans"); + let plan = plans.first().expect("the unit carries a frame"); + (*plan.header).clone() +} + +/// Decode `aus` only as far as the FIRST delivered frame, and read it back as +/// tightly packed NV12 — the device half of both frame-0 legs. +/// +/// Brings its own device up and tears it down, so one test may call it more than +/// once; the GPU lock and the readback hook are the caller's. +fn av1_first_frame(aus: &[&[u8]]) -> Vec { + let setup = common::bring_up(&common::Request { + codec: common::AV1, + graphics: common::Graphics::Required, + report_families: true, + }); + let handles = setup.handles(); + + let ours = { + // SAFETY: as the parity legs — `setup` outlives this block and was created + // with the AV1 decode extension + timeline/sync2 features. + let mut decoder = unsafe { VkAv1Decoder::new(&handles, Box::new(NoopQueueLock)) } + .expect("wrap the device"); + decoder + .probe_stream_support(1, 8, false) + .expect("the box must host AV1 Main 4:2:0 8-bit, no film grain"); + // SAFETY: as the parity legs — live instance/device, queue 0 of `graphics_qf`. + let readback = unsafe { + Readback::new( + &setup.instance, + setup.pd, + &setup.device, + setup.graphics_qf, + DISPLAY_AV1, + EXPECTED_FORMAT, + ) + }; + + // The FIRST delivered frame and no further: the first temporal unit is a + // key frame that shows, so this is one decode. + let mut first: Option> = None; + for (index, au) in aus.iter().enumerate() { + let frame = decoder + .decode(au) + .unwrap_or_else(|e| panic!("AU {index}: decode failed: {e}")); + if let Some(frame) = frame { + assert_eq!( + decoder.wait_status(&frame), + DecodeStatus::Ok, + "frame 0: decode op not COMPLETE\n state: {}", + decoder.debug_snapshot() + ); + assert_eq!(frame.format, EXPECTED_FORMAT, "frame 0: pool format"); + // SAFETY: the frame is delivered and unreleased on the readback's + // device, the pool carries TRANSFER_SRC, and the test is serialized. + first = Some(unsafe { readback.read_nv12(&frame) }); + decoder + .release_frame(&frame, true) + .expect("frame 0: release"); + // A temporal unit may carry more than one frame, and this leg stops + // at the first. Anything else the unit made ready is handed straight + // back — with `false`, because no presenter signalled its timeline + // (nothing read it) — rather than left held while the decoder drops. + while let Some(spare) = decoder.take_ready() { + decoder + .release_frame(&spare, false) + .expect("release an unread frame of the same temporal unit"); + } + break; + } + } + // SAFETY: every readback was fence-waited inside `read_nv12`. + unsafe { readback.destroy() }; + first.expect("the vector's first temporal unit shows a frame") + }; + + // SAFETY: as the parity legs — the decoder and readback are gone. + unsafe { setup.destroy() }; + ours +} + +/// Per-plane statistics of `ours` against `want`, printed rather than asserted. +/// +/// Everything here answers a question a hash cannot: WHICH plane, whether the +/// difference is a displacement or a value error, and how big. See +/// [`av1_frame0_pixels_say_which_plane_and_how_badly`] for how to read it. +fn report_nv12_divergence(ours: &[u8], want: &[u8], display: (u32, u32)) { + let (width, height) = (display.0 as usize, display.1 as usize); + let luma = width * height; + assert_eq!(ours.len(), want.len(), "both frames are the same layout"); + assert_eq!(ours.len(), luma * 3 / 2, "tightly packed NV12"); + + eprintln!( + "--- AV1 frame 0: {width}x{height} NV12, {} bytes ---", + ours.len() + ); + eprintln!(" ours {}", sha256_hex(ours)); + eprintln!(" golden {}", sha256_hex(want)); + + // A plane that never varies means nothing was decoded into the image at all, + // which is a different failure from decoding it wrongly. + let flat = |plane: &[u8]| plane.iter().all(|b| *b == plane[0]); + if flat(&ours[..luma]) { + eprintln!( + " ⚠ our LUMA is constant ({}) — nothing decoded here", + ours[0] + ); + } + if flat(&ours[luma..]) { + eprintln!( + " ⚠ our CHROMA is constant ({}) — nothing decoded here", + ours[luma] + ); + } + + for (name, ours, want) in [ + ("luma ", &ours[..luma], &want[..luma]), + ("chroma", &ours[luma..], &want[luma..]), + ] { + if ours == want { + eprintln!(" {name}: IDENTICAL ({} bytes)", ours.len()); + continue; + } + let mut differing = 0usize; + let mut max_delta = 0u32; + let mut total_delta = 0u64; + // |delta| buckets: 1, 2, 3-4, 5-8, 9-16, 17-64, 65+. + let mut buckets = [0usize; 7]; + let mut first: Vec<(usize, u8, u8)> = Vec::new(); + for (i, (a, b)) in ours.iter().zip(want.iter()).enumerate() { + if a == b { + continue; + } + let delta = u32::from(a.abs_diff(*b)); + differing += 1; + max_delta = max_delta.max(delta); + total_delta += u64::from(delta); + let bucket = match delta { + 1 => 0, + 2 => 1, + 3..=4 => 2, + 5..=8 => 3, + 9..=16 => 4, + 17..=64 => 5, + _ => 6, + }; + buckets[bucket] += 1; + if first.len() < 8 { + first.push((i, *a, *b)); + } + } + let percent = 100.0 * differing as f64 / ours.len() as f64; + eprintln!( + " {name}: {differing}/{} bytes differ ({percent:.2}%), max |delta| {max_delta}, \ + mean |delta| over the differing bytes {:.2}", + ours.len(), + total_delta as f64 / differing as f64 + ); + eprintln!( + " |delta| histogram 1:{} 2:{} 3-4:{} 5-8:{} 9-16:{} 17-64:{} 65+:{}", + buckets[0], buckets[1], buckets[2], buckets[3], buckets[4], buckets[5], buckets[6] + ); + // One ROW is `width` bytes in both planes — luma because it is `width` + // samples wide, interleaved chroma because it is `width / 2` samples wide + // and two bytes per sample. So one formula serves both, and the chroma + // coordinates it prints are in chroma units. + let positions: Vec = first + .iter() + .map(|(i, a, b)| format!("(x{},y{}) {a}≠{b}", i % width, i / width)) + .collect(); + eprintln!(" first differing: {}", positions.join(" ")); + } + + // A displacement, not a difference: does our luma equal the reference read a + // few rows or columns over? That is what a wrong crop origin or a copy extent + // taken from the pool rather than the render region looks like, and the shift + // that matches IS the error. + if ours[..luma] != want[..luma] { + let mut best: Option<(i32, i32, f64)> = None; + for dy in -4i32..=4 { + for dx in -8i32..=8 { + if (dy, dx) == (0, 0) { + continue; + } + let (mut hit, mut seen) = (0usize, 0usize); + for y in 8..height - 8 { + for x in 8..width - 8 { + let sy = (y as i32 + dy) as usize; + let sx = (x as i32 + dx) as usize; + seen += 1; + if ours[y * width + x] == want[sy * width + sx] { + hit += 1; + } + } + } + let score = hit as f64 / seen as f64; + if best.is_none_or(|(_, _, b)| score > b) { + best = Some((dy, dx, score)); + } + } + } + // The identity's own score, for scale: a decode that is merely slightly + // wrong still matches most bytes in place, so a shift only means something + // when it beats staying put. + let (mut hit, mut seen) = (0usize, 0usize); + for y in 8..height - 8 { + for x in 8..width - 8 { + seen += 1; + if ours[y * width + x] == want[y * width + x] { + hit += 1; + } + } + } + let identity = hit as f64 / seen as f64; + if let Some((dy, dx, score)) = best { + eprintln!( + " luma shift probe: in place {:.3} · best shift dy{dy:+} dx{dx:+} {score:.3}{}", + identity, + if score > identity + 0.05 { + " ⚠ A SHIFT FITS BETTER — this is readback geometry, not decode" + } else { + " (no shift fits better: the pixels are in the right place and \ + carry the wrong values)" + } + ); + } + } +} + // --------------------------------------------------------------------------- // CPU coherence guards — NOT `#[ignore]`d. // @@ -1315,6 +1789,48 @@ fn av1_goldens_and_the_ivf_split_agree_with_the_planner() { ); } +/// The vendored frame-0 pixels ARE the first golden — not a second opinion about it. +/// +/// [`AV1_FRAME0`] is the one place in this file where reference PIXELS live rather +/// than hashes, and pixels are exactly the kind of file that rots: regenerate the +/// goldens from a re-synced vector and this blob keeps describing the old one, while +/// the diagnostic leg that reads it goes on confidently naming the wrong cause. So +/// its digest is re-derived here and compared against `GOLDENS_AV1`'s first line — +/// the trusted, three-way cross-checked set — on every platform, with no GPU. +/// +/// It also pins the layout the diagnostic's arithmetic assumes: 320x240 tightly +/// packed NV12 is 115200 bytes, luma first. +#[test] +fn the_av1_frame0_reference_is_the_first_golden() { + let (width, height) = (DISPLAY_AV1.0 as usize, DISPLAY_AV1.1 as usize); + assert_eq!( + AV1_FRAME0.len(), + width * height * 3 / 2, + "data/test-25fps-av1.frame0.nv12 must be one tightly packed NV12 frame of \ + the vector's render region" + ); + let goldens = golden_hashes(GOLDENS_AV1); + assert_goldens_are_a_real_set(&goldens, FRAME_COUNT, "data/test-25fps-av1.nv12.sha256"); + assert_eq!( + sha256_hex(AV1_FRAME0), + goldens[0], + "the vendored frame-0 pixels must hash to the AV1 golden set's FIRST entry — \ + if they no longer do, the blob is from a different decode than the goldens \ + and `av1_frame0_pixels_say_which_plane_and_how_badly` would attribute a \ + divergence to the wrong cause. Regenerate it alongside the goldens: decode \ + the vector with `-f rawvideo -pix_fmt nv12 -fps_mode passthrough` and take \ + the first 115200 bytes (the golden file's header carries the full command)" + ); + // Not a flat blob: a frame of one repeated byte would satisfy a length check and + // make every per-plane statistic in the diagnostic meaningless. + let luma = &AV1_FRAME0[..width * height]; + let chroma = &AV1_FRAME0[width * height..]; + assert!( + luma.iter().any(|b| *b != luma[0]) && chroma.iter().any(|b| *b != chroma[0]), + "both planes must carry real picture content" + ); +} + /// Count Annex-B start codes in `stream` as `(total, three_byte)`. /// /// Emulation prevention guarantees `00 00 01` cannot occur inside a NAL payload,