fix(dxvadec): the review round — a doc that had become false, a warn-storm on renegotiation, and HEVC's exemption made falsifiable

Four findings, all real.

**`SlotMap`'s own docs had become false.** "feed it every `DpbUpdate` in decode order
(via `Self::apply` or `plan_to_vk`, which applies internally)" — `plan_to_vk` no longer
applies internally, which is the entire point of the change, and `release`'s docs named
it as one of the two things that may free a slot. A reader following those docs would
build the next caller wrong in exactly the way this commit's parent fixed. Both now say
which conversions defer, which one does not, and why H.265 is the one that does not.

**The deferred release warned on a legitimate event.** `release_deferred` warned per id
when a deferred release found no slot — but 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 that same access unit's `removed`. Every one of those
ids then misses, and nothing is wrong. `debug!`, with the legitimate cause named so the
illegitimate one stays diagnosable.

**HEVC's exemption was asserted only in its consequence.** `the_current_picture_is_
named_by_curr_pic_and_never_aliases_a_reference` checked that no reference shares the
decode target's slot — which on the vendored vector holds whether or not the reasoning
behind it does. That is precisely how the H.264 leg passed for two milestones. The test
now also asserts the PLANNER property the exemption rests on (`removed ∩ dpb_refs = ∅`,
falsified by moving `dpb_snapshot()` above `decode_rps`), and records that the low-delay
measurement was 0 of 300 against H.264's 297 of 300 from the same host and the same run.
It also records what is still missing: no low-delay HEVC stream is vendored, so HEVC's
freedom is a re-derivable argument plus one measurement, not a standing hardware leg.

**Two stale cross-references.** Both AV1 conversions told the reader the H.264/H.265
zero was "measured on reordering vectors and not a proof" — the open question this
commit's parent closed. They now say what the answer was.
This commit is contained in:
2026-08-07 22:19:12 +02:00
parent 834b244301
commit fd6241a24f
5 changed files with 95 additions and 18 deletions
@@ -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");
}
}
}
+10 -4
View File
@@ -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
+51
View File
@@ -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]
+9 -6
View File
@@ -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();
+16 -3
View File
@@ -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