diff --git a/crates/pf-client-core/src/video_d3d11_native.rs b/crates/pf-client-core/src/video_d3d11_native.rs index 6829aa93..5323860c 100644 --- a/crates/pf-client-core/src/video_d3d11_native.rs +++ b/crates/pf-client-core/src/video_d3d11_native.rs @@ -519,11 +519,15 @@ impl NativeD3d11Decoder { }; 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"); + // Never fatal, and never silent — but `debug!` rather than `warn!`, + // because there is a LEGITIMATE way to get here: a renegotiation + // replaces the whole `Session` (and with it the slot map) inside + // `plan`, while the planner's own drain reports every drained picture + // in the same AU's `removed`. Those ids belong to the map that no + // longer exists, so every one of them misses and nothing is wrong. + // Outside a rebuild it means the conversion and the ledger disagree + // about the DPB, which the surrounding rebuild log makes separable. + tracing::debug!(id, "a deferred release named a picture holding no surface"); } } } diff --git a/crates/pf-dxvadec/src/pic_av1.rs b/crates/pf-dxvadec/src/pic_av1.rs index 6cfe6e0b..d8ce47c8 100644 --- a/crates/pf-dxvadec/src/pic_av1.rs +++ b/crates/pf-dxvadec/src/pic_av1.rs @@ -139,10 +139,16 @@ pub struct DecodePlanDxvaAv1 { /// breath: decode into the surface you are predicting from. /// /// Neither vendored H.264 nor H.265 vector ever produces that shape (measured: - /// zero on the 250-AU clips — ⚠ but see `plan_to_dxva`'s note, because that is a - /// measurement of two REORDERING vectors and not a proof about those codecs), - /// which is why the eager release survived two hardware-proven codecs and opened - /// on the first AV1 frame past the key frame's neighbourhood. The Vulkan rung + /// zero on the 250-AU clips), which is why the eager release survived two + /// codecs believed hardware-proven and opened on the first AV1 frame past the key + /// frame's neighbourhood. + /// + /// ⚠ That zero turned out to be a fact about the VECTORS. H.264 has the identical + /// defect on any low-delay stream — 117 of 120 access units of our own host's + /// output, wrong pixels on three GPUs — and now carries the identical deferral + /// ([`crate::pic::DecodePlanDxva::release_after_decode`], which records the + /// measurement). H.265 is the only one of the three that is genuinely safe, and + /// structurally: its planner snapshots `dpb_refs` after `decode_rps`. The Vulkan rung /// carries the same contract for the same reason /// (`pf_vkdecode::pic_av1::DecodePlanVkAv1::release_after_decode`), and this /// rung's constraint is the STRICTER of the two: Vulkan binds only the references diff --git a/crates/pf-dxvadec/src/pic_h265.rs b/crates/pf-dxvadec/src/pic_h265.rs index dbb32293..1c59d805 100644 --- a/crates/pf-dxvadec/src/pic_h265.rs +++ b/crates/pf-dxvadec/src/pic_h265.rs @@ -916,8 +916,36 @@ mod tests { } } + /// HEVC's freedom from the aliasing that cost AV1 and H.264 a deferral, and the + /// PLANNER property that grants it. + /// + /// Both other codecs release a removed picture's slot and then let + /// [`SlotMap::assign`] hand it straight back to the decode target, so `CurrPic` + /// and a `RefPicList` entry name one surface. This conversion still releases its + /// whole `removed` list inline, and is safe doing so for one reason: `H265Planner` + /// snapshots `dpb_refs` AFTER `decode_rps` has updated the DPB, so a picture this + /// AU's RPS dropped is never in the set `RefPicList` is built from, and nothing + /// later in the AU unmarks anything. `H264Planner` and `Av1Planner` both snapshot + /// BEFORE their marking, and both needed the deferral. + /// + /// The second assertion is that argument made falsifiable. The first is only the + /// consequence, and on this vector the consequence would hold even if the argument + /// stopped being true — the vendored H.264 vector taught that lesson expensively + /// (it measured zero aliasing for two milestones while every stream we ship + /// aliased on 99% of its frames). Moving `dpb_snapshot()` above `decode_rps` would + /// leave the first assertion passing and break the second on the first AU whose + /// RPS drops a picture, which on this vector is most of them. + /// + /// ⚠ No low-delay HEVC stream is vendored, so unlike H.264 this is not backed by a + /// hardware leg on our own encoder's output. It was MEASURED once, 2026-08-07, on + /// a 300-picture 1080p low-delay HEVC stream from a punktfunk host: 0 access units + /// with a `removed ∩ dpb_refs` intersection, against 297 of 300 for H.264 from the + /// same host and the same run. Vendoring that stream is the way to make this a + /// standing guarantee rather than a re-derivable argument. #[test] fn the_current_picture_is_named_by_curr_pic_and_never_aliases_a_reference() { + let mut aus_with_removals = 0usize; + let mut both = 0usize; for (plan, dxva) in convert_stream(TEST_25FPS) { assert_eq!(dxva.pic_params.CurrPic.index(), dxva.setup_slot); assert!(!dxva.pic_params.CurrPic.associated()); @@ -928,7 +956,30 @@ mod tests { for r in &dxva.refs { assert_ne!(r.slot, dxva.setup_slot, "a reference aliases the target"); } + if !plan.dpb.removed.is_empty() { + aus_with_removals += 1; + } + both += plan + .dpb + .removed + .iter() + .filter(|id| plan.dpb_refs.iter().any(|r| r.id == **id)) + .count(); } + assert!( + aus_with_removals > 0, + "no AU of this vector removed anything, so the zero below would be empty \ + for a reason that has nothing to do with the property being asserted" + ); + assert_eq!( + both, 0, + "{both} picture(s) are in an AU's own reference set AND removed by it. \ + That is the H.264/AV1 aliasing precondition, and HEVC is supposed to be \ + structurally incapable of it — so the snapshot in `H265Planner` has moved \ + ahead of `decode_rps`. Restore the ordering, or give this conversion the \ + `release_after_decode` deferral the other two carry; do NOT relax this \ + number" + ); } #[test] diff --git a/crates/pf-vkdecode/src/pic_av1.rs b/crates/pf-vkdecode/src/pic_av1.rs index 28da7f11..6a4bd9ab 100644 --- a/crates/pf-vkdecode/src/pic_av1.rs +++ b/crates/pf-vkdecode/src/pic_av1.rs @@ -834,12 +834,15 @@ mod tests { /// 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. + /// it is reading: **268 of this vector's 274 frames**, first at frame 6. + /// + /// The H.264 planner produces the same shape and the vendored vector never does + /// it (measured: zero on the 250-AU clip), which is why the hole survived a codec + /// believed hardware-proven and opened here first. That zero was a fact about the + /// vector: on a low-delay host stream H.264 aliases on 117 of 120 access units, + /// and `plan_to_vk` now carries the same deferral + /// ([`crate::pic::DecodePlanVk::release_after_decode`]). H.265 does not need one — + /// `H265Planner` snapshots `dpb_refs` after `decode_rps`. #[test] fn a_reference_this_frame_displaces_keeps_its_slot_until_after_the_decode() { let mut planner = Av1Planner::new(); diff --git a/crates/pf-vkdecode/src/slots.rs b/crates/pf-vkdecode/src/slots.rs index d7f1de21..34b730d4 100644 --- a/crates/pf-vkdecode/src/slots.rs +++ b/crates/pf-vkdecode/src/slots.rs @@ -44,7 +44,19 @@ impl std::fmt::Display for SlotError { impl std::error::Error for SlotError {} /// The slot ledger. One per decode session; feed it every [`DpbUpdate`] in decode -/// order (via [`Self::apply`] or `plan_to_vk`, which applies internally). +/// order. +/// +/// [`Self::apply`] does that whole-update. `plan_to_vk` and `plan_to_vk_av1` do it in +/// two halves instead: they ASSIGN the stored picture and hand the removals back as a +/// `release_after_decode` list for the caller to apply once the decode op is issued. +/// The split is not a convenience — releasing a removal before the assignment lets +/// [`Self::assign`] return the slot this AU's own submission still names, which is a +/// picture decoding into one it predicts from. A caller that drops the list leaks a +/// slot per AU. +/// +/// `plan_to_vk_h265` still applies its removals internally, and that is safe rather +/// than lucky: `H265Planner` snapshots `dpb_refs` AFTER `decode_rps`, so a picture +/// this AU's RPS dropped is never in the set its reference lists are built from. /// /// Invariants (unit-tested): /// - a [`PicId`] keeps its slot from [`Self::assign`] until [`Self::release`]; @@ -134,8 +146,9 @@ impl SlotMap { /// as the planner's DPB holds the picture — as a reference OR as a decoded /// picture awaiting output — and that residency ends only when a /// [`DpbUpdate::removed`] entry reports it. This method is that report's - /// primitive: `plan_to_vk` and [`Self::apply`] call it with the planner's - /// `removed` ids and nothing else may release a slot. + /// primitive: [`Self::apply`] calls it with the planner's `removed` ids, and so + /// do the conversions' callers via `release_after_decode` — one AU's removals, + /// deferred until its decode op is issued. Nothing else may release a slot. /// /// Releasing is CPU-side bookkeeping (the slot becomes assignable to a later /// picture); keeping the released slot's IMAGE out of reuse until in-flight