diff --git a/crates/pf-client-core/src/video_d3d11_native.rs b/crates/pf-client-core/src/video_d3d11_native.rs index 78759d3f..6829aa93 100644 --- a/crates/pf-client-core/src/video_d3d11_native.rs +++ b/crates/pf-client-core/src/video_d3d11_native.rs @@ -20,6 +20,19 @@ //! * **H.264 and H.265** — frame-hash parity against libavcodec on an RTX 4090 and an AMD //! iGPU plus a 30-minute soak (M5), re-confirmed on an RTX 3500 Ada and an Intel Arc on //! 2026-08-07 (250/250 both codecs, plus 50/50 HEVC Main 10 on both). +//! +//! ⚠⚠ **All of that was against ONE vendored vector per codec, and for H.264 the vector +//! was blind to a defect present on 99% of the frames we actually stream.** It reorders +//! and carries a 7-frame DPB against 2 reference frames; a punktfunk host emits +//! low-delay IPPP whose DPB is exactly as deep as its 3 reference frames, so 8.2.5's +//! sliding window unmarks a picture in the very access unit whose C.4.5.3 bump evicts +//! it. `plan_to_dxva` released that surface before assigning the decode target one, and +//! `SlotMap::assign` handed it straight back — `CurrPic` and a `RefFrameList` entry +//! naming one surface, on 117 of 120 access units. Found 2026-08-07 by planning our own +//! host's output on the CPU, fixed with the same deferral the AV1 rung got +//! ([`pf_dxvadec::DecodePlanDxva::release_after_decode`]), and the stream is now +//! vendored so `low_delay_host_h264_every_frame_hashes_bit_identical_to_libavcodec` +//! holds the rung to what it streams rather than only to what it conforms to. //! * **AV1** — wired in M7, and frame-hash parity on the SAME two GPUs since 2026-08-07: //! 250/250 delivered frames bit-identical to libavcodec on the RTX 3500 Ada and on the //! Intel Arc. It streams 4K60 on both with a clean 5-minute soak, but that is throughput @@ -188,14 +201,19 @@ struct Submission { /// other two codecs do not (see [`NativeD3d11Decoder::frame_av1`]). All three /// conversions produce it; dropping it here made the AV1 leak invisible. setup_id: u64, - /// AV1 only: surfaces this frame's own refresh displaces while the submission - /// still NAMES them, released once the decode op has been issued. + /// Surfaces this picture's own end-of-picture bookkeeping retires while the + /// submission still NAMES them, released once the decode op has been issued + /// ([`NativeD3d11Decoder::release_deferred`]). /// - /// See [`pf_dxvadec::DecodePlanDxvaAv1::release_after_decode`] — this is the - /// caller's half of that contract, and dropping it decodes 268 of the vendored - /// vector's 274 frames into a surface they predict from. Always empty on - /// H.264 and H.265, whose conversions release their whole `removed` list - /// themselves (neither vendored vector ever produces the shape). + /// The caller's half of [`pf_dxvadec::DecodePlanDxvaAv1::release_after_decode`] + /// and [`pf_dxvadec::DecodePlanDxva::release_after_decode`]. Dropping it decodes + /// the picture into a surface it predicts from — 268 of the vendored AV1 vector's + /// 274 frames, and 297 of every 300 access units of low-delay H.264, which is what + /// every punktfunk host emits. + /// + /// Empty on H.265 alone, and that is structural rather than lucky: `H265Planner` + /// snapshots `dpb_refs` AFTER `decode_rps`, so a picture this AU's RPS dropped is + /// never in the set `RefPicList` is built from. release_after_decode: Vec, /// Which codec's slice-control record the packer's locations become. codec: Codec, @@ -458,11 +476,21 @@ impl NativeD3d11Decoder { // The plan needed a substitute for something lost. Fold it, ask for recovery, // and do NOT submit: a concealed picture is not fit to present, and submitting // it would put a wrong reference in the DPB for every AU after it. + // + // The deferred releases still run: they are the planner's verdict on + // pictures that left the DPB, and a converted-but-unsubmitted AU took its + // slot just the same. + self.release_deferred(&submission); self.health.note(true, false, 0); self.want_recovery = true; return Ok(None); } - let frame = match self.submit(au, &submission) { + let submitted = self.submit(au, &submission); + // The surfaces the conversion refused to release, freed now that the decode op + // has been issued (or has failed, where dropping them would leak just the + // same) — see [`Self::release_deferred`]. + self.release_deferred(&submission); + let frame = match submitted { Ok(frame) => frame, Err(e) => { self.health.note(false, true, 0); @@ -474,6 +502,32 @@ impl NativeD3d11Decoder { Ok(Some(frame)) } + /// Apply a submission's [`Submission::release_after_decode`] — the surfaces its + /// conversion held back because the submission still NAMED them. + /// + /// Safe here and nowhere earlier: the decode op has been issued (or will never be), + /// so nothing can be assigned these surfaces before the next access unit is + /// converted. Dropping the list instead holds a surface per AU and reaches + /// `SlotError::Full` within the ledger's depth — which is why every exit of + /// [`Self::decode`] runs it, the concealed and failed ones included. + /// + /// Empty on H.265, whose planner cannot produce the shape; populated on nearly + /// every H.264 and AV1 picture. + fn release_deferred(&mut self, sub: &Submission) { + let Some(session) = self.session.as_mut() else { + return; + }; + for &id in &sub.release_after_decode { + if !session.slots.release(id) { + // Never fatal, and never silent: a deferred id that holds no slot + // means the conversion and the ledger disagree about the DPB, which + // is a bug in one of them rather than a stream this AU can do + // anything about. + tracing::warn!(id, "a deferred release named a picture holding no surface"); + } + } + } + /// One AV1 **temporal unit**: decode every frame in it, present at most one. /// /// This is the whole of what AV1 adds to this rung's contract, and it is the @@ -627,20 +681,7 @@ impl NativeD3d11Decoder { // waits: the decode op has been issued, so nothing can be assigned them // until the next frame — and on the `damaged` and failed paths there is no // op at all, where dropping the release would leak a surface just the same. - if let Some(session) = self.session.as_mut() { - for &id in &sub.release_after_decode { - if !session.slots.release(id) { - // Never fatal, and never silent: a deferred id that holds no - // slot means the conversion and the ledger disagree about the - // store, which is a bug in one of them rather than a stream - // this frame can do anything about. - tracing::warn!( - id, - "AV1 deferred release named a picture holding no surface" - ); - } - } - } + self.release_deferred(&sub); // The slot nothing will ever ask for again (fn docs). Released AFTER the // blit above, so the surface is read before anything can be assigned it. @@ -836,21 +877,20 @@ impl NativeD3d11Decoder { slice_ranges: dxva.slice_ranges, setup_slot: dxva.setup_slot, setup_id: dxva.setup_id, - // ⚠ H.264's conversion still releases its whole `removed` list - // inside itself, and that is a MEASUREMENT rather than a proof. - // `pf_dxvadec::pic`'s `no_au_removes_a_picture_its_own_reference_ - // list_names` pins `removed ∩ dpb_refs` at zero over the vendored - // vector — but that vector REORDERS, and reordering is what keeps - // an unmarked picture in the DPB past the AU that unmarked it. On - // a low-delay stream, which is what a punktfunk host emits, the - // sliding window can unmark an already-output picture and - // `bump_as_needed` evict it in the same access unit, putting it in - // both `RefFrameList` and `removed` — the AV1 aliasing shape, - // exactly. Left alone deliberately: the AV1 defect is what this - // change fixes and proves, and giving two hardware-proven codecs a - // deferral no vector exercises would be an unmeasured change to - // working code. The tripwire is the test named above. - release_after_decode: Vec::new(), + // ⚠⚠ The suspicion of 2026-08-07 was RIGHT, and the shape is the + // ordinary case rather than a corner: on every stream a punktfunk + // host emits, 297 of 300 access units name one surface as both + // `CurrPic` and a `RefFrameList` entry. `H264Planner` snapshots + // `dpb_refs` before 8.2.5's marking, and low-delay H.264 — + // `max_num_reorder_frames = 0`, so a picture is output the moment + // it decodes — puts the unmarking and the eviction in one AU. + // NVENC seals it by writing `max_num_ref_frames = 3` AND + // `max_dec_frame_buffering = 3`: a DPB exactly as deep as the + // reference count. The vendored vector cannot reach the shape (a + // level-derived DPB of 7 against 2 reference frames, and it + // reorders), which is why it measured zero for two milestones. + // See `pf_dxvadec::DecodePlanDxva::release_after_decode`. + release_after_decode: dxva.release_after_decode, codec: Codec::H264, facts: PictureFacts { colour: colour_of(plan.picture.colour), @@ -910,13 +950,15 @@ impl NativeD3d11Decoder { slice_ranges: dxva.slice_ranges, setup_slot: dxva.setup_slot, setup_id: dxva.setup_id, - // HEVC is the one of the three where this IS structural rather - // than measured: `H265Planner` snapshots `dpb_refs` AFTER - // `decode_rps` has updated the DPB, so a picture this AU's RPS - // dropped is never in the snapshot `RefPicList` is built from, and - // the aliasing shape cannot arise. (The AV1 planner snapshots - // BEFORE, which is why that codec needed the deferral, and the - // H.264 one snapshots before marking too — see the note above.) + // HEVC is the one of the three that needs no deferral, and it is + // STRUCTURAL rather than measured: `H265Planner` snapshots + // `dpb_refs` AFTER `decode_rps` has updated the DPB, so a picture + // this AU's RPS dropped is never in the snapshot `RefPicList` is + // built from, and nothing later in the AU unmarks anything. Both + // other codecs snapshot BEFORE their marking, and both needed the + // deferral. Now measured as well as argued: a low-delay HEVC + // stream from the same host that aliases 297 of 300 H.264 access + // units aliases 0 of 300 here. release_after_decode: Vec::new(), codec: Codec::H265, facts: PictureFacts { @@ -1701,6 +1743,23 @@ mod parity { /// rungs measured against two copies of a golden set is two measurements, and the /// point of this file is that they are one. const GOLDENS_H264: &str = include_str!("../../pf-vkdecode/tests/data/test-25fps.nv12.sha256"); + + /// **Our own host's low-delay H.264** and its goldens — the stream the vendored + /// vector cannot be. 120 pictures of 640x480 IPPP with `max_num_reorder_frames = 0` + /// and a DPB exactly as deep as its 3 reference frames, so 8.2.5's sliding window + /// unmarks the oldest reference in the very access unit whose C.4.5.3 bump evicts + /// it: `dpb.removed` and `dpb_refs` intersect on 117 of the 120, and the conversion + /// used to release those surfaces before assigning the decode target one. + /// + /// The vendored vector passed 250/250 on four GPUs across two milestones while + /// that was true of every stream this program ships. Provenance, the + /// `punktfunk-host spike` command and the ffmpeg cross-check are in the golden + /// file's header. + const LOWDELAY_H264: &[u8] = + include_bytes!("../../pf-vkdecode/tests/data/lowdelay-640x480.h264"); + const GOLDENS_LOWDELAY: &str = + include_str!("../../pf-vkdecode/tests/data/lowdelay-640x480.nv12.sha256"); + const LOWDELAY_FRAME_COUNT: usize = 120; const GOLDENS_H265: &str = include_str!("../../pf-vkdecode/tests/data/test-25fps-h265.nv12.sha256"); @@ -2175,6 +2234,11 @@ mod parity { decoder .submit(au, &sub) .unwrap_or_else(|e| panic!("AU {index}: submit failed — {e:#}")); + // This harness drives `plan` + `submit` rather than `decode`, so it owes + // the deferred releases `decode` would have applied. Not optional + // bookkeeping: on a low-delay stream nearly every AU defers, and a loop + // that drops them exhausts the ledger within the DPB's depth. + decoder.release_deferred(&sub); let session = decoder.session.as_ref().expect("submit built a session"); let pool = session.pool.clone(); let bytes = readback.read(&decoder.device, &pool, slice, display); @@ -2545,6 +2609,32 @@ mod parity { ); } + /// The leg that would have caught this rung's H.264 defect, and the only one that + /// could: **our own host's output** rather than a conformance vector. + /// + /// `h264_every_frame_hashes_bit_identical_to_libavcodec` above passed 250/250 on an + /// RTX 4090, an AMD iGPU, an RTX 3500 Ada and an Intel Arc while this rung was + /// naming one surface as both `CurrPic` and a `RefFrameList` entry on 99% of the + /// access units of every stream punktfunk actually streams. The vector cannot reach + /// the shape — see [`LOWDELAY_H264`] — so no amount of running it harder would have + /// found this. That is the lesson worth keeping: a conformance vector proves + /// conformance to ITSELF, and the encoder we ship behind is a different stream. + #[test] + #[ignore = "needs a Windows D3D11 video device (see module docs)"] + fn low_delay_host_h264_every_frame_hashes_bit_identical_to_libavcodec() { + let aus = split_h264_aus(LOWDELAY_H264); + let order = order_h264(&aus); + parity_run( + Codec::H264, + StreamFormat::SDR_420_8, + &aus, + &order, + &golden_hashes(GOLDENS_LOWDELAY), + LOWDELAY_FRAME_COUNT, + "H.264 (low-delay host stream)", + ); + } + #[test] #[ignore = "needs a Windows D3D11 video device (see module docs)"] fn h265_every_frame_hashes_bit_identical_to_libavcodec() { diff --git a/crates/pf-dxvadec/src/descriptors.rs b/crates/pf-dxvadec/src/descriptors.rs index 7d5ed39e..41d2eab4 100644 --- a/crates/pf-dxvadec/src/descriptors.rs +++ b/crates/pf-dxvadec/src/descriptors.rs @@ -304,6 +304,7 @@ mod tests { setup_slot: 0, setup_id: 1, setup_is_reference: true, + release_after_decode: Vec::new(), refs: Vec::::new(), mb_count, } diff --git a/crates/pf-dxvadec/src/pic.rs b/crates/pf-dxvadec/src/pic.rs index e9a28730..06d650b9 100644 --- a/crates/pf-dxvadec/src/pic.rs +++ b/crates/pf-dxvadec/src/pic.rs @@ -133,6 +133,60 @@ pub struct DecodePlanDxva { /// surface exists for the decode itself plus any remaining DPB residency, /// and may already have been released by this very AU's `removed`. pub setup_is_reference: bool, + /// Surfaces this access unit's own end-of-picture bookkeeping retires while the + /// submission still NAMES them. Release them once the decode op is issued — + /// never inside the conversion, and never dropped. + /// + /// # Why the conversion cannot release them + /// + /// [`SlotMap::assign`] takes the LOWEST FREE slot. Release a picture here and the + /// setup assignment two lines later hands its surface straight back, so the + /// submission says `CurrPic = N` and `RefFrameList[k] = N` in one breath: the + /// picture decodes into a surface it predicts from. That is the AV1 defect of + /// 2026-08-07 ([`crate::pic_av1::DecodePlanDxvaAv1::release_after_decode`]) on this + /// codec, and on H.264 it is not exotic at all. + /// + /// [`AuPlan::dpb_refs`] — which `RefFrameList` is built from — is snapshotted in + /// `H264Planner::begin_picture`, BEFORE `finish_picture` runs 8.2.5's marking and + /// C.4.5.3's bump. So a picture the sliding window unmarks and the bump then evicts + /// lands in both `dpb_refs` and `dpb.removed` for the same AU. It needs the two to + /// coincide, which needs the evicted picture to be already OUTPUT — and that is + /// precisely low-delay H.264: `max_num_reorder_frames = 0`, a picture output the + /// moment it decodes. + /// + /// **Measured 2026-08-07, and it is the ordinary case, not a corner.** Every stream + /// a punktfunk host emits does it on 297 of 300 access units — 720p, 1080p and + /// 2160p alike, on both this rung and [`pf_vkdecode::pic::DecodePlanVk`]'s. NVENC + /// writes `max_num_ref_frames = 3` AND `max_dec_frame_buffering = 3`: the DPB is + /// exactly as deep as the reference count, so the window unmarks the oldest + /// reference in the very AU whose bump evicts it. The aliased picture is + /// `ref_idx 2` of a three-entry `num_ref_idx_l0_active` list — addressable by any + /// macroblock, not a spare the hardware could ignore. + /// + /// `test-25fps.h264` measures ZERO and that is why this survived to here: it is + /// level 1.3 with no VUI `bitstream_restriction`, so its DPB is the level-derived 7 + /// against 2 reference frames, and it REORDERS, which keeps an unmarked picture + /// alive past the AU that unmarked it. `data/lowdelay-640x480.h264` is vendored to + /// close exactly that gap. + /// + /// # Why the caller can release them safely + /// + /// The surfaces must outlive the CONVERSION, not the decode. One AU is planned, + /// converted and submitted before the next is planned, so once the decode op is + /// issued nothing can be assigned them before the next conversion — the same + /// argument the AV1 rung's deferral rests on. + /// + /// # Deferring the whole `removed` list rather than a filtered part + /// + /// Some removals are pictures no `RefFrameList` entry names (a non-reference + /// picture bumped long after it was unmarked), and those could be released here. + /// They are not, for three reasons: `refs` is built from the SLICE LISTS as well as + /// the snapshot, so a filter on `dpb_refs` would still miss a concealment + /// substitute the lists name; deferring costs nothing, because + /// [`SlotMap::new`]'s spare slot means `assign` always has a free slot while every + /// removal is still held (the DPB never exceeds `max_dpb_frames`, and the map holds + /// `max_dpb_frames + 1`); and one unconditional rule is a thing a reader can check. + pub release_after_decode: Vec, /// The marked DPB, resolved to surfaces — the AU's own references first, then /// every other marked picture (module docs). Laid out in exactly this order in /// `pic_params.RefFrameList`. @@ -268,7 +322,10 @@ impl From for PlanToDxvaError { /// 2. references resolve against the PRE-removal state (read-only) — this AU's /// own end-of-picture marking can evict a picture its slices legitimately /// reference; -/// 3. `removed` is applied, then the setup slot is assigned last. +/// 3. the setup slot is assigned last, and `removed` is NOT applied at all: it +/// leaves as [`DecodePlanDxva::release_after_decode`] for the caller to apply +/// once the decode op is issued. Applying it here would give the assignment +/// back a surface this submission still names — see that field's docs. pub fn plan_to_dxva( plan: &AuPlan, slots: &mut SlotMap, @@ -480,24 +537,29 @@ pub fn plan_to_dxva( let slice_ranges: Vec> = plan.slices.iter().map(|s| s.data.clone()).collect(); - // Mutations LAST, after every fallible step above (fn docs). Removals first — - // they were real regardless of this AU's fate — then the setup assignment. + // Mutations LAST, after every fallible step above (fn docs). + // + // The removals are NOT applied here — they are handed back as + // `release_after_decode` for the caller to apply once the decode op is issued, + // because releasing them now would return their surfaces to the setup + // assignment below and alias `CurrPic` with a `RefFrameList` entry. The field's + // docs carry the measurement; this is the ordinary case on every stream a + // punktfunk host emits. // // The AU's own picture can itself appear in `removed`: a non-reference // picture with no free frame buffer bypasses the DPB and is stored-and- // evicted within one plan. Its surface must still exist for the decode - // itself, so it is assigned here and released right after. + // itself, so it is assigned here and released right after — the one removal + // that cannot be deferred, since deferring it would hand the caller the + // surface being decoded into. let setup_evicted = plan.dpb.removed.contains(&setup_id); - for &id in &plan.dpb.removed { - if id == setup_id { - continue; - } - if !slots.release(id) { - // Tolerated but never silent: reachable only when the caller skipped - // feeding an AU's plan through this map. - trace!(id, "DpbUpdate removed an id this SlotMap never assigned"); - } - } + let release_after_decode: Vec = plan + .dpb + .removed + .iter() + .copied() + .filter(|id| *id != setup_id) + .collect(); let setup_slot = slots.assign(setup_id)?; if setup_evicted { slots.release(setup_id); @@ -523,6 +585,7 @@ pub fn plan_to_dxva( setup_slot, setup_id, setup_is_reference: pic.is_reference, + release_after_decode, refs, mb_count: width_mbs * height_mbs, }) @@ -570,6 +633,17 @@ mod tests { "../../pf-bitstream/vendor/cros-codecs/src/codec/h264/test_data/test-25fps.h264" ); + /// **Our own host's output**, and the only stream in this repository that reaches + /// the shape `release_after_decode` exists for: low-delay IPPP, 120 pictures, + /// `max_num_reorder_frames = 0`, and a DPB exactly as deep as its reference count. + /// + /// Vendored beside the goldens the GPU legs decode it against (that file's header + /// carries the `punktfunk-host spike` command and the ffmpeg cross-check), because + /// three crates need it: this one for the CPU proof, `pf-vkdecode`'s `gpu_parity` + /// and `pf-client-core`'s `video_d3d11_native::parity` for the hardware one. + const LOWDELAY_640X480: &[u8] = + include_bytes!("../../pf-vkdecode/tests/data/lowdelay-640x480.h264"); + /// Test-only AU splitter, the same shape pf-vkdecode's `pic` tests use /// (which in turn mirrors pf-bitstream's `#[cfg(test)]`-private helper): a /// new AU starts at a non-slice NALU following a slice, or at a slice whose @@ -602,10 +676,22 @@ mod tests { /// Plan the vendored stream and convert every AU, returning the plans paired /// with their conversions. fn convert_stream() -> Vec<(AuPlan, DecodePlanDxva)> { + convert(TEST_25FPS) + } + + /// The same over [`LOWDELAY_640X480`]. + fn convert_low_delay() -> Vec<(AuPlan, DecodePlanDxva)> { + convert(LOWDELAY_640X480) + } + + /// Plan and convert a whole stream the way a caller does — including applying + /// `release_after_decode` once the (notional) decode op is issued, which is what + /// keeps the ledger from filling up over 120 access units. + fn convert(stream: &[u8]) -> Vec<(AuPlan, DecodePlanDxva)> { let mut planner = H264Planner::new(); let mut slots: Option = None; let mut out = Vec::new(); - for (i, au) in split_into_aus(TEST_25FPS).into_iter().enumerate() { + for (i, au) in split_into_aus(stream).into_iter().enumerate() { let Ok(plan) = planner.plan_au(au) else { continue; }; @@ -614,6 +700,9 @@ mod tests { *map = SlotMap::new(plan.picture.max_dpb_frames); } let dxva = plan_to_dxva(&plan, map, i as u32 + 1).expect("conversion"); + for &id in &dxva.release_after_decode { + assert!(map.release(id), "AU {i}: deferred id {id} held no slot"); + } out.push((plan, dxva)); } out @@ -881,66 +970,153 @@ mod tests { assert_eq!(converted.len(), 250); } - /// ⚠⚠ **TRIPWIRE, not a proof — and the thing it watches for is UNRESOLVED.** + /// The hazard the vendored vector CANNOT see, on a stream that can. /// - /// This conversion releases the whole of `plan.dpb.removed` and then assigns the - /// decode target a slot. [`SlotMap::assign`] takes the LOWEST FREE slot, which is - /// the one just released — so if a picture is ever in BOTH `dpb_refs` (which is - /// what `RefFrameList` is built from, above) and `dpb.removed`, the submission - /// names one surface as `CurrPic` and as a `RefFrameList` entry at once, and the - /// frame decodes into a picture it predicts from. That is precisely the defect - /// measured on the AV1 leg on 2026-08-07: 245 of 250 delivered frames wrong on an - /// Intel Arc, invisible on NVIDIA for 63 frames, and invisible on glass entirely. + /// `removed ∩ dpb_refs` is the aliasing precondition: a picture this AU's own + /// end-of-picture bookkeeping retires while `RefFrameList` still names it. Release + /// it inside the conversion and [`SlotMap::assign`] hands its surface straight back + /// to `CurrPic`, so the picture decodes into one it predicts from. /// - /// For AV1 it was fixed by deferring the release past the decode op. For H.264 it - /// was NOT, because the intersection is empty on the vendored vector and changing - /// a hardware-proven codec on an unreproduced suspicion is the worse risk. What - /// this test does is make the assumption falsifiable instead of tacit. + /// On `test-25fps.h264` the intersection is **zero**, and for two independent + /// reasons that both happen to be properties of that vector rather than of H.264: + /// it is level 1.3 with no VUI `bitstream_restriction`, so `dpb_limit` falls back to + /// A.3.1's level ceiling and gives a 7-frame DPB against `max_num_ref_frames = 2` + /// (the sliding window unmarks two AUs before the bump can evict); and it REORDERS, + /// which keeps an unmarked picture alive for output past the AU that unmarked it. + /// That zero is what let the eager release survive two milestones. /// - /// **Why zero here is weak evidence.** `H264Planner` snapshots `dpb_refs` in - /// `begin_picture`, BEFORE `finish_picture` runs 8.2.5 marking and then bumps the - /// DPB — and the vendored bump drops a picture the sliding window just unmarked - /// only once it has been OUTPUT. This vector reorders (it has B-frames), so an - /// unmarked picture is still awaiting output and lingers past the AU that unmarked - /// it, which is exactly what keeps the intersection empty. **A punktfunk host emits - /// low-delay H.264 with no reordering**, where a picture is output the moment it is - /// decoded — the condition that puts eviction and unmarking in the same access - /// unit. So the shape is plausibly live in the field and merely unreachable here. + /// On `lowdelay-640x480.h264` — OUR host's output, vendored for exactly this — it is + /// **117 of 120 access units**, measured the same way at 720p, 1080p and 2160p. The + /// difference is the encoder, not the resolution: NVENC writes + /// `max_num_ref_frames = 3` AND `max_dec_frame_buffering = 3`, a DPB exactly as deep + /// as the reference count, so 8.2.5's window unmarks the oldest reference in the very + /// AU whose C.4.5.3 bump evicts it — and `max_num_reorder_frames = 0` means it has + /// already been output, which is what makes it evictable at all. /// - /// HEVC does not need this test and cannot be given one: `H265Planner` snapshots - /// `dpb_refs` AFTER `decode_rps` has updated the DPB, so an RPS-dropped picture is - /// structurally never in the snapshot `RefPicList` is built from. + /// So this is not a tripwire any more: it pins BOTH numbers, and the second one is + /// what makes `release_after_decode` a fixed defect rather than a precaution. /// - /// If this ever fires, the fix is `pic_av1.rs`'s: hand the removals back to the - /// caller as `release_after_decode` and let it release them once the decode op is - /// issued. Do not "fix" it by relaxing the count. + /// HEVC needs no such test: `H265Planner` snapshots `dpb_refs` AFTER `decode_rps` + /// has updated the DPB, so an RPS-dropped picture is structurally never in the + /// snapshot `RefPicList` is built from — and a low-delay HEVC stream from the same + /// host measures 0 of 300, which is the argument confirmed rather than assumed. #[test] - fn no_au_removes_a_picture_its_own_reference_list_names() { - let mut both = 0usize; - let mut aus_with_removals = 0usize; - for (plan, _) in convert_stream() { - if !plan.dpb.removed.is_empty() { - aus_with_removals += 1; - } - for id in &plan.dpb.removed { - if plan.dpb_refs.iter().any(|r| r.id == *id) { - both += 1; + fn the_low_delay_stream_removes_pictures_its_own_reference_list_names_and_the_vector_never_does( + ) { + fn intersections(stream: &[u8]) -> (usize, usize, usize) { + let mut planner = H264Planner::new(); + let (mut both, mut with_removals, mut planned) = (0usize, 0usize, 0usize); + for au in split_into_aus(stream) { + let Ok(plan) = planner.plan_au(au) else { + continue; + }; + planned += 1; + if !plan.dpb.removed.is_empty() { + with_removals += 1; } + both += plan + .dpb + .removed + .iter() + .filter(|id| plan.dpb_refs.iter().any(|r| r.id == **id)) + .count(); } + (planned, with_removals, both) } + + let (planned, with_removals, both) = intersections(TEST_25FPS); + assert_eq!(planned, 250); assert!( - aus_with_removals > 0, - "no access unit of this vector removed anything, so the intersection below \ - is empty for a reason that has nothing to do with the hazard" + with_removals > 0, + "no access unit of the vendored vector removed anything, so the zero below \ + would be empty for a reason that has nothing to do with the hazard" ); assert_eq!( both, 0, - "{both} picture(s) are in this AU's reference list AND removed by it — the \ - conversion releases them before assigning the decode target a slot, so \ - `CurrPic` and a `RefFrameList` entry now name one surface and the frame \ - predicts from the picture it is writing. This is the AV1 defect of \ - 2026-08-07 on the H.264 leg; fix it the same way (a deferred \ - `release_after_decode`), never by changing this number" + "the vendored vector is supposed to be BLIND to this shape — a non-zero \ + here means the reordering/DPB-depth reasoning above is wrong, and the \ + low-delay numbers below need re-deriving before they mean anything" + ); + + let (planned, with_removals, both) = intersections(LOWDELAY_640X480); + assert_eq!(planned, 120); + assert_eq!(with_removals, 117); + assert_eq!( + both, 117, + "the low-delay stream must still exercise the aliasing precondition on \ + nearly every access unit — if this ever drops to zero the deferral below \ + is no longer being TESTED by anything, whatever else still passes" + ); + } + + /// The fix itself: no submission names its decode surface as a reference. + /// + /// [`the_setup_surface_is_the_current_picture_entry_and_is_never_also_a_reference_entry`] + /// asserts this over the vendored vector, where it held even before + /// `release_after_decode` existed. This is the same invariant over the stream that + /// BREAKS it — 117 of 120 access units before the deferral, every one of them + /// decoding into a surface it predicts from. + #[test] + fn the_low_delay_stream_never_aliases_its_decode_surface_with_a_reference() { + let converted = convert_low_delay(); + assert_eq!(converted.len(), 120); + let mut deferred_total = 0usize; + for (i, (_, dxva)) in converted.iter().enumerate() { + assert_eq!(dxva.pic_params.CurrPic.index(), dxva.setup_slot); + deferred_total += dxva.release_after_decode.len(); + for r in &dxva.refs { + assert_ne!( + r.slot, dxva.setup_slot, + "AU {i}: reference picture {} shares surface {} with the decode \ + target — the deferral is not holding", + r.id, r.slot + ); + } + } + assert_eq!( + deferred_total, 117, + "every access unit that removes a picture must defer it; a zero here with \ + the assertions above still passing would mean the stream stopped \ + exercising the shape" + ); + } + + /// The deferral costs no slot the map does not have. + /// + /// Holding every removal through the setup assignment is only free because + /// [`SlotMap::new`] allocates `max_dpb_frames + 1` and the DPB never exceeds + /// `max_dpb_frames` — so a free slot always exists even with the whole `removed` + /// list still held. Measured rather than argued: the deepest the ledger ever gets + /// on the stream that defers on 117 of 120 access units. + #[test] + fn deferring_every_removal_still_fits_the_ledger() { + let mut planner = H264Planner::new(); + let mut slots: Option = None; + let mut peak = 0usize; + let mut capacity = 0usize; + for (i, au) in split_into_aus(LOWDELAY_640X480).into_iter().enumerate() { + let Ok(plan) = planner.plan_au(au) else { + continue; + }; + let map = slots.get_or_insert_with(|| SlotMap::new(plan.picture.max_dpb_frames)); + if map.capacity() != plan.picture.max_dpb_frames + 1 { + *map = SlotMap::new(plan.picture.max_dpb_frames); + } + let dxva = plan_to_dxva(&plan, map, i as u32 + 1).expect("conversion"); + // The peak is measured BEFORE the deferred releases are applied: that is + // the moment the map is fullest, and the moment `assign` had to find a + // free slot in. + peak = peak.max(map.held().count()); + capacity = map.capacity(); + for &id in &dxva.release_after_decode { + assert!(map.release(id), "AU {i}: deferred id {id} held no slot"); + } + } + assert_eq!(capacity, 4, "max_dec_frame_buffering 3 + the spare slot"); + assert_eq!( + peak, 4, + "the deferral is expected to USE the spare slot — a peak of 3 would mean \ + the removals are being released early after all" ); } @@ -1164,27 +1340,47 @@ mod tests { assert_eq!(&bytes[10..14], &40u32.to_le_bytes()); } + /// The whole-stream churn check: no two live pictures may share a surface index, + /// which for DXVA is the difference between a decode and a corrupted reference. + /// + /// Run over BOTH streams, because they stress opposite halves of the ledger: the + /// vendored vector has a DPB (7) far deeper than its reference count (2) and so + /// never has to reuse a surface promptly, while the low-delay stream's DPB is + /// exactly its reference count and cycles all four slots every four pictures. + /// + /// The loop applies `release_after_decode` because the CALLER does; a loop that + /// drops it holds a surface per access unit and dies of `SlotError::Full` — which + /// is what this test did the moment the deferral landed, and is the cheapest + /// possible demonstration that the deferral is real rather than decorative. #[test] fn a_slot_is_reused_only_after_its_picture_leaves_the_dpb() { - // The whole-stream churn check: no two live pictures may share a surface - // index, which for DXVA is the difference between a decode and a - // corrupted reference. - let mut planner = H264Planner::new(); - let mut slots: Option = None; - let mut live: Vec<(PicId, u8)> = Vec::new(); - for (i, au) in split_into_aus(TEST_25FPS).into_iter().enumerate() { - let Ok(plan) = planner.plan_au(au) else { - continue; - }; - let map = slots.get_or_insert_with(|| SlotMap::new(plan.picture.max_dpb_frames)); - let removed = plan.dpb.removed.clone(); - let dxva = plan_to_dxva(&plan, map, i as u32 + 1).expect("conversion"); - live.retain(|&(id, _)| !removed.contains(&id)); - assert!( - live.iter().all(|&(_, slot)| slot != dxva.setup_slot), - "AU {i} decodes into a surface a live picture still holds" - ); - live.push((dxva.setup_id, dxva.setup_slot)); + for (label, stream) in [("vendored", TEST_25FPS), ("low-delay", LOWDELAY_640X480)] { + let mut planner = H264Planner::new(); + let mut slots: Option = None; + let mut live: Vec<(PicId, u8)> = Vec::new(); + for (i, au) in split_into_aus(stream).into_iter().enumerate() { + let Ok(plan) = planner.plan_au(au) else { + continue; + }; + let map = slots.get_or_insert_with(|| SlotMap::new(plan.picture.max_dpb_frames)); + let removed = plan.dpb.removed.clone(); + let dxva = plan_to_dxva(&plan, map, i as u32 + 1).expect("conversion"); + // The check runs BEFORE the deferred releases: the aliasing this + // guards against is a property of the SUBMISSION, and at submission + // time every removed picture is still live by construction. + assert!( + live.iter().all(|&(_, slot)| slot != dxva.setup_slot), + "{label} AU {i} decodes into a surface a live picture still holds" + ); + for &id in &dxva.release_after_decode { + assert!( + map.release(id), + "{label} AU {i}: deferred id {id} held no slot" + ); + } + live.retain(|&(id, _)| !removed.contains(&id)); + live.push((dxva.setup_id, dxva.setup_slot)); + } } } } diff --git a/crates/pf-dxvadec/tests/libav_picparams_parity.rs b/crates/pf-dxvadec/tests/libav_picparams_parity.rs index 59e87429..ff56a630 100644 --- a/crates/pf-dxvadec/tests/libav_picparams_parity.rs +++ b/crates/pf-dxvadec/tests/libav_picparams_parity.rs @@ -540,6 +540,19 @@ fn our_h264_submissions() -> Vec { // libavcodec's `1 + report_id++` produces for a decoder that saw only this stream. let dxva = pf_dxvadec::plan_to_dxva(&plan, map, out.len() as u32 + 1) .unwrap_or_else(|e| panic!("AU {i} must convert: {e}")); + // The conversion's half of the deferral contract + // ([`pf_dxvadec::DecodePlanDxva::release_after_decode`]): a loop that converts + // AU after AU without applying it holds a surface per AU and runs the ledger + // dry. The vendored vector never puts a picture in both `RefFrameList` and + // `removed`, so this list is always empty HERE — applied anyway, because a + // harness that mirrors the caller only on the streams where it does not matter + // is a harness that would not notice the caller being wrong. + for &id in &dxva.release_after_decode { + assert!( + map.release(id), + "AU {i}: a deferred release named a picture holding no surface" + ); + } let packed = pf_dxvadec::pack(au, &dxva.slice_ranges, &mut mapping) .unwrap_or_else(|e| panic!("AU {i} must pack: {e}")); let unpadded = pf_dxvadec::packed_size(au, &dxva.slice_ranges).expect("packed size") as u32; diff --git a/crates/pf-vkdecode/src/decoder.rs b/crates/pf-vkdecode/src/decoder.rs index ac417aa5..d5fafc0b 100644 --- a/crates/pf-vkdecode/src/decoder.rs +++ b/crates/pf-vkdecode/src/decoder.rs @@ -822,175 +822,204 @@ impl VkH264Decoder { } let vk_plan = vk_plan.expect("the rebuilt session matches its own plan"); - let state = self.state.as_mut().expect("ensured above"); - // The per-AU active-reference gate: the session was created with - // maxActiveReferencePictures; binding more in one decode op would be a - // silent VUID violation on the drivers that matter most. - let max_active = state.session.config.max_active_references as usize; - if vk_plan.refs.len() > max_active { - return Err(VkDecodeError::Unsupported(format!( - "AU references {} pictures, session allows {max_active} active references", - vk_plan.refs.len() - ))); - } - - // Coincide binding sync: slots the planner released no longer bind their - // images (the pictures may still be pending/held — untouched), and the - // 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; + // Everything from here to the deferred release below is ONE unit of ledger + // work. `plan_to_vk` has already committed this AU's setup assignment and + // handed back the removals it deliberately did NOT apply + // (`DecodePlanVk::release_after_decode`); until those are applied the slot + // map holds one picture too many. A `?` anywhere in the region would skip + // them and leak a slot per failed AU — four `?`s and three early `return`s + // could — so the region's Result is HELD and the release runs either way. + let submitted = (|| -> Result<(), VkDecodeError> { + let state = self.state.as_mut().expect("ensured above"); + // The per-AU active-reference gate: the session was created with + // maxActiveReferencePictures; binding more in one decode op would be a + // silent VUID violation on the drivers that matter most. + let max_active = state.session.config.max_active_references as usize; + if vk_plan.refs.len() > max_active { + return Err(VkDecodeError::Unsupported(format!( + "AU references {} pictures, session allows {max_active} active references", + vk_plan.refs.len() + ))); } - 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; + + // Coincide binding sync: slots the planner released no longer bind their + // images (the pictures may still be pending/held — untouched), and the + // 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; + } } } } - } - // The decode target: a FREE pool image (never one a consumer holds — the - // whole point of the pool model). Exhaustion means the consumer owes - // more than HOLD_HEADROOM releases; no wait can free an image here. - let Some(dst) = state.pool.free_index() else { - debug!( - held = state.pool.held_total(), - "picture pool exhausted — release_frame owed" - ); - return Err(VkDecodeError::NoFreeSlot); - }; + // The decode target: a FREE pool image (never one a consumer holds — the + // whole point of the pool model). Exhaustion means the consumer owes + // more than HOLD_HEADROOM releases; no wait can free an image here. + let Some(dst) = state.pool.free_index() else { + debug!( + held = state.pool.held_total(), + "picture pool exhausted — release_frame owed" + ); + return Err(VkDecodeError::NoFreeSlot); + }; - // Cross-queue waits (the AVVkFrame contract): the dst image's last known - // timeline value (covers a presenter write-back after release), plus — - // coincide mode — every referenced image's value, so reference reads - // order after any presenter layout restore already reported back. - let mut waits: Vec<(vk::Semaphore, u64)> = Vec::new(); - { - let dst_pic = &state.pool.pictures[dst]; - if dst_pic.value > 0 { - waits.push((dst_pic.semaphore, dst_pic.value)); + // Cross-queue waits (the AVVkFrame contract): the dst image's last known + // timeline value (covers a presenter write-back after release), plus — + // coincide mode — every referenced image's value, so reference reads + // order after any presenter layout restore already reported back. + let mut waits: Vec<(vk::Semaphore, u64)> = Vec::new(); + { + let dst_pic = &state.pool.pictures[dst]; + if dst_pic.value > 0 { + waits.push((dst_pic.semaphore, dst_pic.value)); + } } - } - if state.dpb.is_none() { - for r in &vk_plan.refs { - if let Some(picture) = state.slot_image[usize::from(r.slot)] { - let pic = &state.pool.pictures[picture]; - if pic.value > 0 && !waits.iter().any(|(sem, _)| *sem == pic.semaphore) { - waits.push((pic.semaphore, pic.value)); + if state.dpb.is_none() { + for r in &vk_plan.refs { + if let Some(picture) = state.slot_image[usize::from(r.slot)] { + let pic = &state.pool.pictures[picture]; + if pic.value > 0 && !waits.iter().any(|(sem, _)| *sem == pic.semaphore) { + waits.push((pic.semaphore, pic.value)); + } } } } - } - let signal_value = state.pool.pictures[dst].value + 1; + let signal_value = state.pool.pictures[dst].value + 1; - // Command buffer + query slot for this submission. - let submission = state.submitted; - let cmd_index = (submission % state.ops.cmds.len() as u64) as usize; - if let Some((sem, value)) = state.cmd_marks[cmd_index] { - // SAFETY: live device; the token is a pool image's semaphore. - unsafe { wait_timeline(self.dev.ash(), sem, value, "command buffer reuse")? }; - } - let query_index = (submission % u64::from(state.ops.query_count)) as u32; + // Command buffer + query slot for this submission. + let submission = state.submitted; + let cmd_index = (submission % state.ops.cmds.len() as u64) as usize; + if let Some((sem, value)) = state.cmd_marks[cmd_index] { + // SAFETY: live device; the token is a pool image's semaphore. + unsafe { wait_timeline(self.dev.ash(), sem, value, "command buffer reuse")? }; + } + let query_index = (submission % u64::from(state.ops.query_count)) as u32; - // Upload the AU (recycles/grows against submission-completion tokens). - let device = self.dev.ash().clone(); - let mut poll = |token: &(vk::Semaphore, u64)| -> Result { - // SAFETY: live device; the token's semaphore is a pool semaphore. - let current = unsafe { device.get_semaphore_counter_value(token.0) } - .map_err(VkDecodeError::from)?; - Ok(current >= token.1) - }; - let device2 = self.dev.ash().clone(); - let mut wait = |token: &(vk::Semaphore, u64)| -> Result<(), VkDecodeError> { - // SAFETY: as above. - unsafe { wait_timeline(&device2, token.0, token.1, "bitstream slot drain") } - }; - // The bitstream buffer carries the SLICE NALUs only, concatenated — a - // real AU opens with AUD/SEI (and, at IDRs, SPS/PPS) NALUs, and feeding - // those to the VCN firmware inside the decode range HANGS it (the .25 - // `vcn_unified_0 ring timeout`; FFmpeg feeds slices-only for the same - // reason). `pack_slices` rebases the offsets into the packed buffer AND - // normalises each slice's Annex-B prefix to three bytes — the two go - // together by construction, see `crate::ring::three_byte_prefix`. - let plan_segments: Vec> = - plan.slices.iter().map(|s| s.data.clone()).collect(); - let Some(packed) = pack_slices(au, &plan_segments) else { - return Err(VkDecodeError::Unsupported( - "packed slice data exceeds the u32 offsets Vulkan submits".into(), - )); - }; - let slice_offsets = packed.offsets; - // SAFETY: live device; the segments are the plan's own in-bounds slice - // ranges (narrowed by the prefix normalisation, so still in bounds); every - // pending token is the completion signal of the submission that consumed - // the slot. - let upload = unsafe { + // Upload the AU (recycles/grows against submission-completion tokens). + let device = self.dev.ash().clone(); + let mut poll = |token: &(vk::Semaphore, u64)| -> Result { + // SAFETY: live device; the token's semaphore is a pool semaphore. + let current = unsafe { device.get_semaphore_counter_value(token.0) } + .map_err(VkDecodeError::from)?; + Ok(current >= token.1) + }; + let device2 = self.dev.ash().clone(); + let mut wait = |token: &(vk::Semaphore, u64)| -> Result<(), VkDecodeError> { + // SAFETY: as above. + unsafe { wait_timeline(&device2, token.0, token.1, "bitstream slot drain") } + }; + // The bitstream buffer carries the SLICE NALUs only, concatenated — a + // real AU opens with AUD/SEI (and, at IDRs, SPS/PPS) NALUs, and feeding + // those to the VCN firmware inside the decode range HANGS it (the .25 + // `vcn_unified_0 ring timeout`; FFmpeg feeds slices-only for the same + // reason). `pack_slices` rebases the offsets into the packed buffer AND + // normalises each slice's Annex-B prefix to three bytes — the two go + // together by construction, see `crate::ring::three_byte_prefix`. + let plan_segments: Vec> = + plan.slices.iter().map(|s| s.data.clone()).collect(); + let Some(packed) = pack_slices(au, &plan_segments) else { + return Err(VkDecodeError::Unsupported( + "packed slice data exceeds the u32 offsets Vulkan submits".into(), + )); + }; + let slice_offsets = packed.offsets; + // SAFETY: live device; the segments are the plan's own in-bounds slice + // ranges (narrowed by the prefix normalisation, so still in bounds); every + // pending token is the completion signal of the submission that consumed + // the slot. + let upload = unsafe { + state + .ring + .upload(&self.dev, au, &packed.segments, &mut poll, &mut wait)? + }; + + // Record + submit, signalling the dst image's next timeline value. + // SAFETY: live device; every handle recorded below belongs to this + // session generation, and the packed slices sit uploaded in the ring slot. + unsafe { + record_and_submit( + &self.dev, + &*self.lock, + state, + &vk_plan, + &slice_offsets, + &upload, + dst, + cmd_index, + query_index, + &waits, + signal_value, + )?; + } + + // Post-submit bookkeeping. + let dst_sem = state.pool.pictures[dst].semaphore; + state.pool.pictures[dst].value = signal_value; + state.pool.pictures[dst].pending = true; + if state.dpb.is_none() { + state.pool.pictures[dst].bound = true; + state.slot_image[setup] = Some(dst); + } + state.cmd_marks[cmd_index] = Some((dst_sem, signal_value)); + state.query_marks[query_index as usize] = submission; + state.submitted += 1; + state.last_submit = Some((dst_sem, signal_value)); state .ring - .upload(&self.dev, au, &packed.segments, &mut poll, &mut wait)? - }; + .pending + .set_pending(upload.slot, (dst_sem, signal_value)); - // Record + submit, signalling the dst image's next timeline value. - // SAFETY: live device; every handle recorded below belongs to this - // session generation, and the packed slices sit uploaded in the ring slot. - unsafe { - record_and_submit( - &self.dev, - &*self.lock, - state, - &vk_plan, - &slice_offsets, - &upload, - dst, - cmd_index, - query_index, - &waits, - signal_value, - )?; + // Refresh the per-slot reference cache from this AU's facts. + state.slot_refs[setup] = Some(vk_plan.setup_ref); + for r in &vk_plan.refs { + state.slot_refs[usize::from(r.slot)] = Some(r.std); + } + + self.pending.insert( + vk_plan.setup_id, + PendingPic { + image: dst, + submission, + query_slot: query_index, + timeline_value: signal_value, + crop: plan.picture.display_crop, + colour: plan.picture.colour, + poc: plan.picture.pic_order_cnt, + is_idr: plan.picture.is_idr, + recovery, + decode_order, + }, + ); + Ok(()) + })(); + + // The slots this AU's own 8.2.5 marking retired while the decode op still + // BOUND them (`DecodePlanVk::release_after_decode`). Held through the + // conversion, the coincide binding sync and the submission, so none of the + // three could take them; freed now that the op is recorded, so the next AU + // may have them. Their images stay pinned by `bound` until that AU's sync, + // the same one-frame grace every other released slot's image gets. + // + // This runs on the failure paths too, and must: the removals are the + // planner's verdict on pictures that left the DPB, which nothing this AU + // does can undo. + if let Some(state) = self.state.as_mut() { + 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"); + } + } } - - // Post-submit bookkeeping. - let dst_sem = state.pool.pictures[dst].semaphore; - state.pool.pictures[dst].value = signal_value; - state.pool.pictures[dst].pending = true; - if state.dpb.is_none() { - state.pool.pictures[dst].bound = true; - state.slot_image[setup] = Some(dst); - } - state.cmd_marks[cmd_index] = Some((dst_sem, signal_value)); - state.query_marks[query_index as usize] = submission; - state.submitted += 1; - state.last_submit = Some((dst_sem, signal_value)); - state - .ring - .pending - .set_pending(upload.slot, (dst_sem, signal_value)); - - // Refresh the per-slot reference cache from this AU's facts. - state.slot_refs[setup] = Some(vk_plan.setup_ref); - for r in &vk_plan.refs { - state.slot_refs[usize::from(r.slot)] = Some(r.std); - } - - self.pending.insert( - vk_plan.setup_id, - PendingPic { - image: dst, - submission, - query_slot: query_index, - timeline_value: signal_value, - crop: plan.picture.display_crop, - colour: plan.picture.colour, - poc: plan.picture.pic_order_cnt, - is_idr: plan.picture.is_idr, - recovery, - decode_order, - }, - ); + submitted?; // The plan's DPB verdicts over the pending map: outputs become ready // frames (their images move pending → held until released); diff --git a/crates/pf-vkdecode/src/pic.rs b/crates/pf-vkdecode/src/pic.rs index 2e7c0eda..df8aa772 100644 --- a/crates/pf-vkdecode/src/pic.rs +++ b/crates/pf-vkdecode/src/pic.rs @@ -12,7 +12,6 @@ use ash::vk::native as hh; use pf_bitstream::h264::AuPlan; use pf_bitstream::h264::PicId; use pf_bitstream::h264::RefPic; -use tracing::trace; use crate::slots::SlotError; use crate::slots::SlotMap; @@ -56,6 +55,35 @@ pub struct DecodePlanVk { pub setup_is_reference: bool, /// The unique referenced pictures across all slices, in first-appearance order. pub refs: Vec, + /// Slots this access unit's own end-of-picture bookkeeping retires while the + /// decode op still BINDS them. Release them once that op is recorded — never + /// inside the conversion, and never dropped. + /// + /// The H.264 twin of [`crate::pic_av1::DecodePlanVkAv1::release_after_decode`], + /// and it exists for exactly the same reason: [`SlotMap::assign`] takes the lowest + /// free slot, so a release here hands the setup assignment two lines later the + /// slot a reference of this very AU still occupies. `pf_bitstream`'s `H264Planner` + /// snapshots `dpb_refs` in `begin_picture`, BEFORE `finish_picture` runs 8.2.5's + /// marking and C.4.5.3's bump, so a picture the sliding window unmarks and the + /// bump evicts is in both `dpb_refs` and `dpb.removed` for one AU — which needs + /// the eviction to be of an already-OUTPUT picture, i.e. low-delay H.264. + /// + /// **Measured 2026-08-07 on this rung as well as the DXVA one:** 297 of 300 AUs of + /// every stream a punktfunk host emits, at 720p, 1080p and 2160p alike. See + /// [`pf_dxvadec::pic::DecodePlanDxva::release_after_decode`]'s docs for the full + /// measurement and for why `test-25fps.h264` measures zero. + /// + /// Both of this rung's DPB modes break on it, differently and neither loudly: + /// in DISTINCT mode `slot_view` hands the aliased reference the same DPB array + /// layer the setup writes, a read-write alias of one subresource; in COINCIDE mode + /// the binding sync clears `slot_image[setup]` (it is the setup slot now) and the + /// reference resolves to no bound image at all, dropping out of `pReferenceSlots` + /// with a `trace!` and nothing else. + /// + /// The caller must apply these on its FAILURE paths too. They are slot-ledger + /// bookkeeping the planner already committed, not something this AU's fate can + /// undo — dropping them leaks a slot per AU and reaches `SlotError::Full`. + pub release_after_decode: Vec, } /// Conversion failures. Stream damage never lands here — pf-bitstream degrades it to @@ -162,11 +190,14 @@ fn ref_info(rp: &RefPic) -> hh::StdVideoDecodeH264ReferenceInfo { /// reference, e.g. the sliding window dropping the oldest short-term reference, /// so `removed` must not be applied before the lists are mapped; /// 3. slice offsets are validated (read-only); -/// 4. `removed` is applied — removals were real regardless of this AU's fate — and -/// the setup slot is assigned last (its failures are caller bugs; nothing is ever -/// half-applied). Released slots become assignable to later pictures; keeping the -/// underlying images alive until in-flight decodes complete is WP-B's -/// synchronization, not this map's. +/// 4. the setup slot is assigned last (its failures are caller bugs; nothing is ever +/// half-applied). `removed` is NOT applied here — it leaves as +/// [`DecodePlanVk::release_after_decode`] for the caller to apply once the decode +/// op is recorded, because applying it now would give the assignment back a slot +/// this AU's own references occupy (that field's docs carry the measurement). +/// Released slots become assignable to later pictures; keeping the underlying +/// images alive until in-flight decodes complete is WP-B's synchronization, not +/// this map's. pub fn plan_to_vk( plan: &AuPlan, slots: &mut SlotMap, @@ -276,24 +307,26 @@ pub fn plan_to_vk( ); } - // Mutations LAST, after every fallible step above (fn docs). Removals first — - // they were real regardless of this AU's fate — then the setup assignment. + // Mutations LAST, after every fallible step above (fn docs). + // + // The removals are handed back rather than applied: releasing one here returns + // its slot to the setup assignment below, and this AU's own references sit in + // those slots (`DecodePlanVk::release_after_decode`). // // The AU's own picture can itself appear in `removed`: a non-reference picture // with no free frame buffer bypasses the DPB and is stored-and-evicted within // one plan. Its slot must still exist for the decode itself, so it is assigned - // here and released right after — see `DecodePlanVk::setup_is_reference`. + // here and released right after — see `DecodePlanVk::setup_is_reference`. That + // is the one removal that is NOT deferred: handing the caller the slot being + // decoded into is the very aliasing the deferral exists to prevent. let setup_evicted = plan.dpb.removed.contains(&setup_id); - for &id in &plan.dpb.removed { - if id == setup_id { - continue; - } - if !slots.release(id) { - // Tolerated but never silent: reachable only when the caller skipped - // feeding an AU's plan through this map. - trace!(id, "DpbUpdate removed an id this SlotMap never assigned"); - } - } + let release_after_decode: Vec = plan + .dpb + .removed + .iter() + .copied() + .filter(|id| *id != setup_id) + .collect(); let setup_slot = slots.assign(setup_id)?; if setup_evicted { slots.release(setup_id); @@ -307,6 +340,7 @@ pub fn plan_to_vk( setup_id, setup_is_reference: pic.is_reference, refs, + release_after_decode, }) } @@ -398,7 +432,10 @@ mod tests { }); let vk = plan_to_vk(&plan, slots, 0).expect("the clean vector converts"); - // Binding sync: released slots unbind; the setup slot rebinds fresh. + // Binding sync: released slots unbind; the setup slot rebinds fresh. The + // deferred releases have deliberately NOT run yet — that is the whole + // point of `DecodePlanVk::release_after_decode`, and doing it in the wrong + // order here would unbind the images this AU's own references read. let setup = usize::from(vk.setup_slot); let mut held_slots = vec![false; slot_image.len()]; for (slot, _id) in slots.held() { @@ -425,6 +462,12 @@ mod tests { slot_image[setup] = Some(dst); pending.insert(vk.setup_id, dst); + // Post-submit: the slots the conversion held back, exactly where + // `Decoder::decode` applies them. + for id in &vk.release_after_decode { + assert!(slots.release(*id), "a deferred id held no slot"); + } + // Settle: outputs deliver to the consumer; removed-never-output free. for id in &plan.dpb.outputs { if let Some(picture) = pending.remove(id) { @@ -522,10 +565,28 @@ mod tests { ); // Mirror the map's bookkeeping: record the new picture, drop the removed. + // + // The removals are dropped only after the deferred releases run, because + // that is when the MAP drops them — before that the conversion is still + // holding them so this AU's submission can name their slots + // (`DecodePlanVk::release_after_decode`). let stored = plan.dpb.stored.unwrap(); assert_eq!(vk.setup_id, stored); assert_eq!(vk.setup_is_reference, plan.picture.is_reference); held.insert(stored, vk.setup_slot); + assert_eq!( + vk.release_after_decode, + plan.dpb + .removed + .iter() + .copied() + .filter(|id| *id != stored) + .collect::>(), + "the deferral is the plan's whole `removed` list less the stored id" + ); + for id in &vk.release_after_decode { + assert!(slots.release(*id), "a deferred id held no slot"); + } for id in &plan.dpb.removed { held.remove(id); } @@ -799,13 +860,22 @@ mod tests { fn a_full_dpb_bump_reuses_the_slot_but_the_pool_model_binds_a_fresh_image() { // Depth-1 DPB (Level 1 at 320x240 ⇒ max_dpb_frames 1, capacity 2): every // stored P evicts the previous picture, and that picture's id lands in - // BOTH `outputs` and `removed` of the SAME plan — so `plan_to_vk` frees - // the evicted slot and immediately re-assigns it as this AU's setup. - // That SLOT reuse is fine and expected; the picture-pool model's whole - // point is that the re-activated slot binds a DIFFERENT free image, so - // the delivered picture's image is never the new decode target while the - // consumer holds it (the HIGH overwrite bug of the adversarial round, - // and the .25 field failure's class). + // BOTH `outputs` and `removed` of the SAME plan. + // + // ⚠ This test used to assert that `plan_to_vk` freed the evicted slot and + // re-assigned it as THIS AU's setup, calling that "the planner's normal + // behaviour". It was not: AU1 is a P picture that REFERENCES the picture it + // was evicting, so the submission named one slot as both `pSetupReferenceSlot` + // and a reference — a decode into the surface being predicted from. The same + // defect the AV1 rung was fixed for on 2026-08-07, authored here in miniature + // and asserted as correct. `DecodePlanVk::release_after_decode` is the fix, and + // this depth-1 stream is its tightest possible case: capacity 2, so the setup + // has exactly one slot to go to and it is the spare. + // + // What the test still proves, and what it was really written for, is the pool + // decoupling: the delivered picture's IMAGE is never the new decode target + // while the consumer holds it (the HIGH overwrite bug of the adversarial + // round, and the .25 field failure's class). let sps = SpsBuilder::new() .seq_parameter_set_id(0) .profile_idc(Profile::Main) @@ -851,16 +921,39 @@ mod tests { .unwrap(); assert!(p1.dpb.outputs.contains(&vk0.setup_id) && p1.dpb.removed.contains(&vk0.setup_id)); let vk1 = plan_to_vk(&p1, &mut slots, 0).unwrap(); - assert_eq!( + + // AU1 REFERENCES the very picture it evicts — so the eviction is deferred and + // the setup goes to the spare slot instead. Without the deferral these two + // assertions are what fails, and they are the whole defect in two lines. + assert!( + vk1.refs.iter().any(|r| r.id == vk0.setup_id), + "AU1 must reference the picture it evicts, or this proves nothing" + ); + assert_ne!( vk1.setup_slot, vk0.setup_slot, - "slot reuse across the bump is the planner's normal behaviour" + "the setup must not take the slot of a picture this AU still references" + ); + for r in &vk1.refs { + assert_ne!(r.slot, vk1.setup_slot, "a reference aliases the setup slot"); + } + assert_eq!( + vk1.release_after_decode, + vec![vk0.setup_id], + "the eviction is handed back, not applied" ); - // Binding sync: the re-activated slot drops its old binding; picture 0's - // image is now delivered to the consumer (held), NOT freed. + // Binding sync: the setup slot is fresh, so nothing is unbound for it; + // picture 0's image is delivered to the consumer (held), NOT freed. Its slot + // is still HELD at this point — which is exactly what keeps its image bound + // while the decode op reads it as a reference. + assert!( + slots + .held() + .any(|(slot, id)| slot == vk0.setup_slot && id == vk0.setup_id), + "the referenced picture must still hold its slot through the submission" + ); bound[img0] = false; held[img0] += 1; // outputs → delivered, consumer holds it - slot_image[usize::from(vk1.setup_slot)] = None; // The pool hands the re-activated slot a FRESH image — never image 0. let img1 = free(&bound, &held).expect("headroom guarantees a free image"); @@ -872,6 +965,17 @@ mod tests { bound[img1] = true; slot_image[usize::from(vk1.setup_slot)] = Some(img1); + // Post-submit: the deferred release lands, and NOW the evicted slot is free — + // a picture later than this submission may have it, which is the only thing + // the deferral ever postponed. + for id in &vk1.release_after_decode { + assert!(slots.release(*id)); + } + assert!( + !slots.held().any(|(slot, _)| slot == vk0.setup_slot), + "the deferred release must actually free the slot" + ); + // Once the consumer releases frame 0, image 0 returns to the free list. held[img0] -= 1; assert_eq!(free(&bound, &held), Some(img0)); diff --git a/crates/pf-vkdecode/tests/data/lowdelay-640x480.h264 b/crates/pf-vkdecode/tests/data/lowdelay-640x480.h264 new file mode 100644 index 00000000..5b56588c Binary files /dev/null and b/crates/pf-vkdecode/tests/data/lowdelay-640x480.h264 differ diff --git a/crates/pf-vkdecode/tests/data/lowdelay-640x480.nv12.sha256 b/crates/pf-vkdecode/tests/data/lowdelay-640x480.nv12.sha256 new file mode 100644 index 00000000..cea58232 --- /dev/null +++ b/crates/pf-vkdecode/tests/data/lowdelay-640x480.nv12.sha256 @@ -0,0 +1,149 @@ +# SHA-256 per decoded frame of lowdelay-640x480.h264, DISPLAY order — 120 frames. +# Each frame is the full 640x480 picture as tightly packed NV12: +# Y plane 640*480 bytes, then interleaved UV 640*240 bytes = 460800 bytes/frame. +# +# THE STREAM IS OURS, not a conformance vector, and that is the point of it. +# `punktfunk-host spike` on .21 (NVENC, RTX 5070 Ti, driver 610.57.04), 2026-08-07: +# +# punktfunk-host spike --source synthetic --codec h264 --width 640 --height 480 \ +# --fps 60 --seconds 2 --bitrate 1 --no-loopback --out lowdelay-640x480.h264 +# +# It is LOW-DELAY IPPP: one IDR, no B pictures, `max_num_reorder_frames = 0`, so a +# picture is output the moment it decodes. Its SPS says `max_num_ref_frames = 3` AND +# `max_dec_frame_buffering = 3` — the DPB is exactly as deep as the reference count, +# which is what makes 8.2.5's sliding window unmark a picture in the same access unit +# that the C.4.5.3 bump evicts it. `test-25fps.h264` cannot reach that shape (level +# 1.3, no VUI bitstream_restriction, so a level-derived DPB of 7 against 2 reference +# frames) and REORDERS besides, which is why it measured zero aliasing while every +# stream this program actually ships aliased on 99% of its frames. +# +# Goldens from libavcodec's SOFTWARE decoder, the same ground truth the vendored +# vectors' goldens use (H.264 decoding is exactly specified, so every conformant +# decoder is bit-identical): +# +# ffmpeg -i lowdelay-640x480.h264 -f rawvideo -pix_fmt nv12 \ +# -fps_mode passthrough ref.yuv +# # then split ref.yuv into 460800-byte frames and sha256 each +# +# ffmpeg version n8.1.2 (Arch/CachyOS, gcc 16, x86_64) — cross-checked BIT-IDENTICAL +# against ffmpeg 8.1.1 on macOS arm64, all 120 frames. 120 of 120 digests distinct. +be912b67b89d720b4e9403bde4c52f0e4414571e3323e4b5312ff5dc26309dd3 +87115b7a352e95baec35ad8bbbe45af5ac6c691264291c9c904e4d01c2e6f911 +03ad701f6c1c4421ad6f8396ed24b893c275d245cf93d24a4f7cf59f9c74675a +faee80c2c494b009b6bab7d9e6d6cb5ccf0921cfa1bdba0d83479b19d2a6f012 +a01b6234ff0bf5c222a0de7b574075f7dec5fc4ea3ccb6657941c791251c6242 +d0befc0eb1e018b671dea55d74009ef1d09cdcba392ecd8509ec000aae91fe1d +f15c273a6077f73296edf4e245069299c034e4a3f4dea10006d9bbf29f8f4220 +5e08336bba0a20ffd3efb8659424af9b76da0252c0cd5f1ccfcdbe92612bafc9 +a8bff0f9921ae301c17e83885760e3e085845732f8d48c4a0fd70f591e3043af +dd90da5b869d1026fba4e41cfba059b8e75fca383bd1ab76c493a82271e1141b +e9237c44040f78e453129daf71afe5e24a640a67524b1c8460c9efaac8eb4795 +fed123e00f51dab09a4b9964bd289c9db95b6a83564d35a5a5e3d5ac2ab8f371 +d7f2db0a52f5a0d835ca9d7d74ec30da29b36bba94b30c10058715c505e26cd9 +25ac9facc9d25c85d0d30864985a0fcad635bd7e65e4773a03e620cbd510ea97 +6d6558eb98c4fc4c8a3dfe9e9eefd744b3cf66f66c6f09c40afbaad272f3f16d +678cadfadfd980eac5aa8ca83eeff2112249262cc4747fac49ecba49a8fbbd42 +031d73796c6f2b9589bc1443937f77156d52aeb602fc7fbc7ec34f02e1ba33ba +59f26c57bade55876dc4ef17070a703cb5585f059f8db7a477a752e731830615 +783d10022eb2f8749f7130954b899c74ad950fb98305a8b0f83ef5a78f38207b +e8d8ce42e46c4121be8a270d2bb30c98a3d11bee7808dcacbdb19e9318e66989 +0b2fb3507e886953ab801e03a63715e6a6a26088de9fc809a56f454e1e2da069 +8bc350ca94eb356c1bd2f221d482eb6e6cdc6e132366236c8ec4b1a41289a01f +dd0f472b9c78ce5b2b4fa2a2354c46090f208fefb12af0440e9a18a2d61b3167 +08bceb5c011632733cc14edf01c301fa4ffe2ba5ee30424f518a23c908db7168 +142b77142fa169ebd231308b0f3162eedeac66bfecb60eb9493abe16bcccaffa +c1dec8e1876afb17d0881c6df6e32e5ee1d8d931f3bbccb079f3ba44380721a0 +e26bc6604826dd69d778ec77d0d3823a797faf32f838f3e13aceca3af15ddecb +bc7b93e2a39d2102539587d3aa96c4aaa58a13050a881bd4f9ced1411bca3a48 +f41bfc62cce822129d65952cbcee15345e5a5fbc529acffdb223851a6f24fdce +3d3b23a6f161580f2f82382c01901ebe0345194d33e07981c50d4cfe0f39bf02 +6e86c11618e37e770667245a679d359fe4a2f42c2a31ccbdc8eac2ee1a299fcd +9083b8c5455f513ed558954892e43b2b18c90928aa7a098ff49f4eff153ede34 +9037d398f4e7cf295ad3bb43069c6a2fa96769c9b20617c9bebd1f34ccb1be3d +a333ed7ba7c7403b9b3a548e1a5f1669218ad85e47f90d69308eb037b4729330 +aabbb4aa194537e260d5a11603fd1a9ef704d784b4d0ea592f7bea6d2a23af17 +a46f730e965d4e39a3120ea43cb02ac94342073b1cf1e473e8d43c49b0cffae4 +259d098d3f53248e889beff1517e4ca88aed4e3803b4ea4923d348c8109f5ac9 +3fc9bc21d3420726606a5534c6516b1ea0cc2140392e0bec2855a45f6f090b82 +2f3cebf32859649c29b1665c766fec4dbdbff2003e64be9e4720971d9b6ae2f1 +f756e4459576a5aeb4c260c9a6c8e115e43ec97a64bfe74fe128f03c3c61b9a1 +d6e101889565951515168958141a23950eb25637dd5202d55d24885eae4e1b42 +1402b95affc25769e00d8ef30eeb0a831bacc41649246e48a46bac1c362ef3a2 +7c838e5d2f191908e2d488185d209fe51c29a50e822ceb1a1f22b6eb6f28fcbf +ba39e5f0936af9706d9718390d03713a235b57102904e2a3972e67ff11b30800 +cfe7b79981886bf74c50957f30b1a98bc4a064e2b41f8fb3996b0a3e15d132ba +47a02c20be1d34caa0a0d406a12d4970dbf642085442663070a8ae942323eb1e +495c75b4d163828750d0e828243bcbff08354418dfd2d43c5d1fd93c3bf691a7 +5ed8d82a1aad0f27066028b4b1058d29f0b9ed5f424f378315ef18c883fe9fc2 +85a9bcb2df1feb0053fc707028df68e9fcf3aed326ec2438ad111dc264aa69bd +b8247ee3d57539fe80bee41669a8a4c6955fd1e374b33cdfe6a6ad9fbb7d9fcb +bcc0a47694f3074d1c7b812f9d00c3eb57f98ee981d8acd7cb504bbaa04275f2 +c5ca305e54f839a7e6f46d3280c17d81f5b3507310bec9a7fa2dae6a151fafd9 +f5807f97f81927b2226395fc27a5d91eef56732916d9e78f65eb485fc4b846ee +3b9d1302b30c11e91eab92eedfcc9dad71a0382488808acc851589740f01498f +619f4862e224fe000acb4edb645fbdaf9e2784e4917e4e3c994052df123c701c +dcabed23f10e192e6103c032f6049adc2e30379f5f71e62489008cfc905f4953 +85dac7f9dfe772670adb9087edc3825c201bd1dcfef412243e26317f5115c6d1 +3518fc0bd4ee33d08976a51f23215960a16bee54e4abd09612c2012c6c716431 +9e0761070df35ed19fc09b8795d66db6021142b99f69f549561390b61ac0a134 +52f92a9c0c2cd9803077ca9a5ee88fe455cd8dd17c8fdba99638200c0a63014f +afe7313b2073231a6c2db4ee749b49f5f0af3105631a77f2b8b27fac0d430430 +c09fde16b28b07291c8e35392a755e05875a4d2f83fbfc91c03ab4cfe7d2e39f +0bf84907d5bf24b62c17fb6f5747c410caecdeb39b5fb2449e1b2cd61b9ddddc +dcf0d031d689f362e04e29bfc2ebdd079fce9fab6624285b2da6e2acff61d1e0 +25bee66822c1dfbac341f6a8b4e91f6463cd44ef24403823040810d4cb9d35c9 +ec06e17bc3d2377b92c9c738964797a4fa6dc13645a1b0b563957a373f62e997 +81e49365823b69f1dadd84a0fcb07e0e21e573664f5dc3f4e3cb3ef5b70b1bb3 +cf7a344c4bbd78d334131dc0e9c4cadb7e4ba1540a16fae268652cc81cda3ef0 +03b9528d2ab09525e80257938bbed9fc4c8f50308e1aefc368c1702557882134 +05a20b533ab2a7345f2f7ca4c51974b5594779fc173dd405e5361f70bc28aff4 +f77cceadf8138be43e272695706573b3476215d788365b2e0f9deb881cade818 +35d39e923d98550ed36dec9836896c784bd7c8eb38bf78c4f1490579cab16d0f +95e721bd96198c40d37ef9c9c1ffb4473800367c81dab7170cf70e92ff8e0d70 +448d2db0f09244f85d7f851218d16e80b06bcba0ec2fc7e53e4bd1ab6a55b565 +9e1d141ad0231a0c67767758cbce6dbc9fb40aa4b68af9b533cff1adf5b205e9 +2af66d2f3158c7da06854f881a9212bf6d4ed09dcc4d32c9e12ef3d8a1f9d28e +20760574c8c569d998e606d72eae71f8f705be80c383ef91deb71f4b0a45024e +9e0df9a3771307e7ce09b9efe4a4ea4a648d744f32816d614909b82c0c150e3b +47e3350d8dbb2012a9f92870c100e8e97cb16f519746c4f7e6afc3e0731cb05c +54ff346f3517b007cea9cebcdfc11b77e488b6ee86b1593a6b275f018ed7024e +8f7fe4882777cc53ca1c3af01b4321a584119eb431114c441a9f275c16e0adf6 +ea08117dc667e86f3d99bec9fe7a76f5d6f675cb9d95bfa9d5e0e5ccaa498655 +73dfddf6c6722d6a6d7db0220bbc823aa121fc75ed6be74b05549bfa527748af +e9fadef92b639b39dea8303926a63ac6aceb8ca83daf49c7a7018d0fb9e195a8 +26eaeb1b0e2b5e9f2e88422c74bfaf162b2e3345cc6283096f96bbb23c88a59b +73dad6fffd397e25e122c01760b9b18f7cfb4d8bc53bd5f49168f6a91b9ac25f +3b28a8cbd94de49716240e43c3354c508222c888c7e86b645302924b630b7b82 +c860ad68336e11d9f41cea92bc62f8016e1edb36240fce059743800bd31140fb +2843fef676d2600c81228dc2c4578f9de17004f22d0bb17ba4afc78f9adb22f2 +9b01be7e851cbe311b14a5f904d94451cc467fe903aac8d4ed5b127967f02e47 +1e583c1c8018d869e73d88dadb65bd6b69b294e7b32b12158a77e43005d59717 +31f6da73506a565ec7bfb116086a4499992e357c96b2449c53e0b21624c579b9 +d2dce5507ffef26cefcfb4efb3c4abbba4f4d7b02288a75b7243e12889ca08f6 +b89bddfc2b94a40086a8169c4790352668e0365640cdd9cfc8f8256a8cd67a76 +5d1f8f7562f1c0e7bcef9ba025b79c68bb0651b5d2039802f3489ca194e5b1f0 +b7928b410e299e00bd69691f656b64de1598ac680effa3a6cc1456fc625544d0 +02334e28175693b207b50e7ced14110afa8a8335fd22d00b2c4dc4f6d8a3480b +a290bf65e6d5fc843ccdb4e33f4b53ffa7d17363c324ce0fe91fc5c59bbf7fc3 +191b54c810f0d694f457e4575d8c306f113fd6443dbbb0a403abb36e04381355 +a1be53163c757fefe4bacf3b4c295cacf6d54e467a3a41bd1718bb5552efb0d9 +cd151dbb675f5f25e34d08db74c9d6f203e9c7fc0fbe662bc8054c2d9c11a4d6 +3c3f77e544b8d21eed0f4f39c68ce5804e3affaaa7188f53c193b040a44afc68 +dba6ef4a4eb5cc89ee9dfe2161a0f727da6fc65aa646837a20601af9b57dfc70 +639c0d1145a5cad6f18a21205d83b2ba1c48fd027efce0fed5477e5d89b3e4b8 +353223c07d88e11081a886257460c8ff1b7ad8c957a567e43994cc9f2f08dc2b +0f0871d1f2c12f7b76a486da9d194b53756fddce56321aba2c2c16c8740bb377 +343ffd487e7c1da166d6fe1acebaa1d2ef89fe0fc16d6ad108a3813a0f8c3206 +625a4e45280b2028a0ca85e806ef4d716664a377fd6e4e0bfaf8cfe2fce2e6b3 +4c4617317e3cfc2369faccc1b6bc5826a7dd92f8b06552fd6fcd2aebedcfb9dd +738c656ffd4e8e0a902c4da9cd7cb19316f0d4aa9f0aaa68c1353f7965abaa6b +ede77993572a017e4dee8a8abc14c6446eed4369385265e18a810afd32e8b3d8 +3029c3cec839a7edfefd05a692cb25b50f029a10710aeef7791307fed8ca4203 +325b6248633ee801e4c0d695f54fd0dbd6dddb61848daf92f74d9895d15923fa +3688c307fe659d4a178ac08afd7cdaa787abe55498aa537e8bd670a63088b3d0 +c4372ea5bfb4003d1b2208a7eaa48259ed48ed41ba5cc930639b5ecd89d45fd8 +4f335634ca6bbfe265197ae86b6ac3fb66dc2aefe8451f135fc8dc211dfe4baa +f1126e7188550c0e5d1da828d942ec48f1e0fc5963303ee3f28fa1e3b3c64ab7 +27b8cb0963d22c67584ac2acfe0afa74328958617d6ca0287b772101cb7091ba +52c6865d58ec4b891ff3e6489475463f9807f2b4f25e60af439e52a04c0f2601 +193dd2b43319b131d821dc554da172b1d36858838c6a88f1f8b747026a7e814e diff --git a/crates/pf-vkdecode/tests/gpu_parity.rs b/crates/pf-vkdecode/tests/gpu_parity.rs index 3b1724a8..e40a69a7 100644 --- a/crates/pf-vkdecode/tests/gpu_parity.rs +++ b/crates/pf-vkdecode/tests/gpu_parity.rs @@ -116,6 +116,29 @@ const AV1_FRAME0: &[u8] = include_bytes!("data/test-25fps-av1.frame0.nv12"); const TEST_MAIN10_H265: &[u8] = include_bytes!("data/test-main10.h265"); const GOLDENS_MAIN10: &str = include_str!("data/test-main10.p010.sha256"); +/// **Our own host's H.264**, and the only stream here that is not a conformance +/// vector — vendored 2026-08-07 because the conformance vector is BLIND to the one +/// defect this rung had. +/// +/// `test-25fps.h264` reorders and carries a 7-frame DPB against 2 reference frames, +/// so a picture the sliding window unmarks is never evicted in the same access unit. +/// A punktfunk host emits low-delay IPPP with `max_num_reorder_frames = 0` and — this +/// is the part that matters — NVENC writes `max_num_ref_frames = 3` alongside +/// `max_dec_frame_buffering = 3`, a DPB exactly as deep as its reference count. 8.2.5's +/// window then unmarks the oldest reference in the very access unit whose C.4.5.3 bump +/// evicts it, and the conversion used to release that picture's slot before assigning +/// the setup one — so `pSetupReferenceSlot` and a reference named the same slot on +/// **117 of these 120 access units**. +/// +/// The vector passed 250/250 throughout. This is the stream that could not. +const LOWDELAY_H264: &[u8] = include_bytes!("data/lowdelay-640x480.h264"); +const GOLDENS_LOWDELAY: &str = include_str!("data/lowdelay-640x480.nv12.sha256"); + +/// The low-delay stream is 120 display frames at 640x480 (no conformance window — +/// both dimensions are macroblock-aligned, so the coded and display sizes agree). +const LOWDELAY_FRAME_COUNT: usize = 120; +const DISPLAY_LOWDELAY: (u32, u32) = (640, 480); + /// The Main 10 vector is 50 display frames. const MAIN10_FRAME_COUNT: usize = 50; @@ -690,6 +713,18 @@ fn assert_bit_identical(hashes: &[String], goldens: &[&str], codec: &str) { /// 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) { + h264_parity_run_against(aus, label, GOLDENS_H264, FRAME_COUNT, DISPLAY_H264); +} + +/// [`h264_parity_run`] with its stream's own goldens and geometry, for the legs that +/// do not decode the vendored vector. +fn h264_parity_run_against( + aus: &[&[u8]], + label: &str, + goldens: &'static str, + frame_count: usize, + display: (u32, u32), +) { // 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(); @@ -698,10 +733,10 @@ fn h264_parity_run(aus: &[&[u8]], label: &str) { // images grow TRANSFER_SRC so vkCmdCopyImageToBuffer is legal. std::env::set_var("PF_VKD_TEST_READBACK", "1"); - let goldens = golden_hashes(GOLDENS_H264); + let goldens = golden_hashes(goldens); assert_eq!( goldens.len(), - FRAME_COUNT, + frame_count, "the golden file carries one hash per libavcodec frame" ); @@ -729,7 +764,7 @@ fn h264_parity_run(aus: &[&[u8]], label: &str) { setup.pd, &setup.device, setup.graphics_qf, - DISPLAY_H264, + display, EXPECTED_FORMAT, ) }; @@ -770,6 +805,28 @@ fn h264_four_byte_start_codes_decode_bit_identically() { ); } +/// The leg the vendored vector cannot be: **our own host's low-delay H.264**. +/// +/// The vector above passed 250/250 on every driver in the fleet while this rung +/// named one DPB slot as both `pSetupReferenceSlot` and a reference on 117 of the +/// 120 access units below — the shape it simply never produces (see +/// [`LOWDELAY_H264`]). Both of this rung's DPB modes take it badly and neither +/// loudly: DISTINCT hands the aliased reference the same array layer the setup +/// writes; COINCIDE finds no bound image for it, drops it from `pReferenceSlots` and +/// `trace!`s. So a leg that decodes what we actually ship is not redundant with the +/// conformance leg, it is the only one that can see this class at all. +#[test] +#[ignore = "needs a Vulkan Video H.264 decode device (fleet boxes; see module docs)"] +fn low_delay_host_h264_every_frame_hashes_bit_identical_to_libavcodec() { + h264_parity_run_against( + &common::split_h264_aus(LOWDELAY_H264), + "H.264 (low-delay host stream)", + GOLDENS_LOWDELAY, + LOWDELAY_FRAME_COUNT, + DISPLAY_LOWDELAY, + ); +} + /// The H.265 twin of [`h264_parity_run`]; see its docs for why the AUs are a /// parameter. fn h265_parity_run( @@ -1598,6 +1655,76 @@ fn h264_goldens_and_au_split_agree_with_the_planner() { ); } +/// The low-delay stream's own CPU guard, plus the property that makes it worth +/// vendoring at all. +/// +/// The goldens/AU/output agreement is the same three-way check +/// [`h264_goldens_and_au_split_agree_with_the_planner`] does. What is extra here is +/// the last assertion: this stream must actually REACH the aliasing precondition — +/// a picture removed by the same access unit whose `dpb_refs` still names it — on +/// nearly every access unit. If a re-generation ever produced a stream that did not, +/// the GPU leg above would still pass 120/120 while proving nothing the vendored +/// vector does not already prove, and nothing else would say so. +#[test] +fn the_low_delay_stream_agrees_with_its_goldens_and_still_exercises_the_aliasing_shape() { + use pf_bitstream::h264::H264Planner; + + let goldens = golden_hashes(GOLDENS_LOWDELAY); + assert_goldens_are_a_real_set( + &goldens, + LOWDELAY_FRAME_COUNT, + "data/lowdelay-640x480.nv12.sha256", + ); + + let aus = common::split_h264_aus(LOWDELAY_H264); + assert_eq!(aus.len(), LOWDELAY_FRAME_COUNT); + + let mut planner = H264Planner::new(); + let mut outputs = 0usize; + let mut both = 0usize; + let mut first_sps = None; + for (index, au) in aus.iter().enumerate() { + let plan = planner + .plan_au(au) + .unwrap_or_else(|e| panic!("AU {index}: the low-delay stream must plan, got {e:?}")); + outputs += plan.dpb.outputs.len(); + both += plan + .dpb + .removed + .iter() + .filter(|id| plan.dpb_refs.iter().any(|r| r.id == **id)) + .count(); + first_sps.get_or_insert(( + plan.sps.max_num_ref_frames, + plan.picture.max_dpb_frames, + plan.sps.vui_parameters.max_num_reorder_frames, + )); + } + outputs += planner.flush().outputs.len(); + assert_eq!( + outputs, + goldens.len(), + "the planner outputs {outputs} pictures but the goldens carry {} hashes", + goldens.len() + ); + + // The three SPS facts that make the shape reachable, pinned so a regenerated + // stream from a different encoder cannot quietly stop being low-delay. + assert_eq!( + first_sps, + Some((3, 3, 0)), + "max_num_ref_frames, DPB depth and max_num_reorder_frames — a DPB exactly as \ + deep as the reference count, with no reordering, is what puts the unmarking \ + and the eviction in one access unit" + ); + assert_eq!( + both, 117, + "the stream must still remove pictures its own reference lists name — that is \ + the ONLY reason it is vendored, and without it the GPU leg is a duplicate of \ + the conformance one" + ); +} + #[test] fn the_main10_vector_is_ten_bit_and_agrees_with_its_goldens() { use pf_bitstream::h265::H265Planner;