fix(dxvadec): the review round — a vacuous predicate, an overstated claim, and the H.264 twin of this defect

Five findings from the adversarial pass, all real.

**The deferral predicate was vacuous.** `plan.dpb.removed` is ALWAYS a subset of
`plan.dpb_refs`: `Av1Planner::plan_frame` snapshots `dpb_refs` before any mutation
and `refresh_slots` can only report a picture that was in `self.slots` at that
moment. So `filter(|id| dpb_refs.contains(id))` was a condition that is never
false, the eager-release loop beside it could never release anything, and the test
assertion "only a picture the submission points at earns the reprieve" could never
fire. Now: defer every removal, say why in terms of the planner, and assert the
PLANNER's property (`removed ⊆ dpb_refs`) — which is falsifiable, and whose failure
would mean the conversion is releasing a surface `ref_frame_map` points at.

**The failure-path claim was overstated.** Holding the decode's `Result` closes
this frame's leak, not the unit's: `decode_av1` returns on the first failing frame
and abandons the rest of the temporal unit's plans, so their removals are never
released. 24 of 250 units carry a second frame. Named rather than fixed — what to
do with the frames after a failure is the pump's question.

**⚠⚠ The H.264 leg plausibly has the same defect, and the comment this change added
said it could not.** `pic.rs` builds `RefFrameList` from `plan.dpb_refs`, and
`H264Planner` snapshots that in `begin_picture` — BEFORE 8.2.5 marking and the DPB
bump. The vendored bump drops a picture the sliding window just unmarked once it
has been output, so a picture can land in both `RefFrameList` and `dpb.removed`:
the AV1 aliasing shape exactly. Measured zero on the vendored vector — but that
vector REORDERS, which is precisely what keeps an unmarked picture alive past the
AU that unmarked it. A punktfunk host emits LOW-DELAY H.264, where output happens
as each picture is decoded, which is the condition that makes eviction and
unmarking land in the same access unit. Traced end to end in source, not
reproduced (no low-delay vector). NOT fixed: changing a hardware-proven codec on an
unreproduced suspicion is the worse risk two commits before a release. Instead
`no_au_removes_a_picture_its_own_reference_list_names` makes the assumption
falsifiable, and its message says what to do when it fires. HEVC is structurally
safe and now says why: `H265Planner` snapshots `dpb_refs` AFTER `decode_rps`.

**Four more stale promotion sites**, past the four already fixed: `Backend::
NativeD3d11va`'s variant doc, `Decoder::new`'s Windows rung comment, `lib.rs`'s
module note and `clients/session/README.md`. Two sites that used the AV1 leg as
the live EXAMPLE of an unproven rung are marked as expired rather than deleted —
the reasoning is what the next bad-evidence leg will need.

**The AV1 dump was missing.** `PF_DXVA_DUMP` wrote h264 and hevc only, for the one
codec whose libavcodec capture has never been taken and where the dump is
therefore the only tool.
This commit is contained in:
2026-08-07 21:34:59 +02:00
parent af4d265168
commit 3a4c94ad79
7 changed files with 198 additions and 57 deletions
+2 -2
View File
@@ -69,8 +69,8 @@ Video, then the platform's own (pf-dxvadec on Windows, pf-vaadec on Linux), then
rung (openh264/rav1d). The libavcodec rungs that used to sit under each of them are
deleted, along with `pf-ffvk` and the `ffmpeg-next` dependency.
Two of the native rungs have never decoded a frame on real hardware (native VAAPI at all;
native D3D11VA's AV1 leg). They run anyway — with the libavcodec twins gone, the only
One of the native rungs has never decoded a frame on real hardware (native VAAPI's H.264 and
H.265 legs; its AV1 leg has decoded but has never been parity-checked). It runs anyway — with the libavcodec twins gone, the only
thing below them is the CPU, so barring them would cost the session hardware decode
outright rather than move it one rung down. What replaces the safety net is the log: every
session names the rung it landed on with its evidence state,
+2 -2
View File
@@ -105,8 +105,8 @@ pub mod clipboard;
pub mod video_d3d11;
// Native D3D11VA (M5): `ID3D11VideoDecoder` driven from pf-bitstream plans, filling the
// hand-off ring `video_d3d11` owns. Since M10 it is the only DXVA rung there is. In `auto`
// for the codecs that have hardware evidence (H.264/H.265) and, with nothing proven left
// below it, for AV1 too — see `video`'s evidence table; `PUNKTFUNK_DECODER=native-d3d11va`
// for all three codecs, each of which now has hardware evidence H.264/H.265 since M5, AV1
// since 2026-08-07 — see `video`'s evidence table; `PUNKTFUNK_DECODER=native-d3d11va`
// reaches every leg by pin.
#[cfg(windows)]
pub mod video_d3d11_native;
+16 -10
View File
@@ -16,10 +16,11 @@
//! M9's evidence FILTER survives, narrowed to the one thing it can still protect
//! ([`native_rung_admitted`]). The filter kept a rung that had never decoded on real
//! hardware out of `auto` while its proven libavcodec twin was one step below. With the
//! twins deleted that is usually no longer the situation: below native-d3d11va's AV1 leg,
//! and below native-vaapi on NVIDIA/AMD, there is nothing proven left to fall onto, so
//! barring the rung would not move a session one rung DOWN — it would take hardware decode
//! away from it entirely, which is the worse answer.
//! twins deleted that is usually no longer the situation: below native-vaapi on NVIDIA/AMD
//! there is nothing proven left to fall onto, so barring the rung would not move a session
//! one rung DOWN — it would take hardware decode away from it entirely, which is the worse
//! answer. (native-d3d11va's AV1 leg was the other standing example until 2026-08-07, when
//! it earned parity on two vendors and stopped being an unproven rung at all.)
//!
//! **One column of that table is different, and it is the one the filter still guards.**
//! On Linux, Intel and every unknown vendor id run `native-vaapi → native-vk → sw`
@@ -788,8 +789,9 @@ enum Backend {
/// `ID3D11VideoDecoder` driven from pf-bitstream plans, filling the shareable-RGBA
/// hand-off ring in `crate::video_d3d11`.
/// Reachable by pin (`PUNKTFUNK_DECODER=native-d3d11va`) and by `auto` in the vendor
/// order: its H.264/H.265 legs have hardware parity + a soak (M5); its AV1 leg has
/// decoded nothing anywhere and runs with the warning [`log_rung`] emits. Errors
/// order: its H.264/H.265 legs have hardware parity + a soak (M5), and its AV1 leg has
/// frame-hash parity on two vendors since 2026-08-07 (M7) — 250/250 after a decode
/// target that aliased a reference surface was fixed. Errors
/// ride the SAME streak/demotion machinery as every other hardware rung.
/// Boxed: the decoder (two planners plus a session) dwarfs the other variants.
#[cfg(windows)]
@@ -1221,11 +1223,14 @@ pub fn native_vulkan_usable(wire: u8, video_decode: bool, decode_video_caps: u32
/// reaches it again if Vulkan can't be built) and by pin.
/// * **Everything else.** Below the unproven rung is the CPU. Trading hardware decode for
/// software decode to avoid an unproven decoder is the worse answer, so those rungs run,
/// with the warning [`log_rung`] emits. That includes Windows Intel/unknown, where the
/// with the warning [`log_rung`] emits. That included Windows Intel/unknown, where the
/// rung below native-d3d11va IS native Vulkan Video on paper: that vendor family is the
/// one thing in this program with a MEASURED wrong-pixel report against Vulkan decode
/// (the B580, see [`Decoder::new`]), and "has never run" is not a reason to move a
/// session onto "known to strobe here". Callers say so where they pass `None`.
/// session onto "known to strobe here". Callers say so where they pass `None`. ⚠ Since
/// 2026-08-07 no D3D11VA leg is unproven, so that arm no longer exercises this clause —
/// the reasoning is kept because the `None` those callers pass is still what decides the
/// answer if any leg's evidence ever goes bad again.
///
/// ⚠ This governs `auto` ONLY. An explicit `PUNKTFUNK_DECODER=` pin bypasses it exactly as
/// it bypasses the vendor order — a pin is how a lab run reaches a rung `auto` will not
@@ -1965,8 +1970,9 @@ impl Decoder {
//
// Windows' D3D11VA RUNG: native D3D11VA (pf-dxvadec). Its H.264/H.265 legs HAVE
// hardware evidence (parity on an RTX 4090 and an AMD iGPU plus a 30-minute soak,
// M5); its AV1 leg has none and runs with the warning `done` logs — until M10 that
// leg was skipped in `auto` in favour of libavcodec's DXVA rung, which no longer
// M5), and since 2026-08-07 so does its AV1 leg (250/250 on an RTX 3500 Ada and an
// Intel Arc, no soak) — until M10 that leg was skipped in `auto` in favour of
// libavcodec's DXVA rung, which no longer
// exists. The rung needs the presenter's win32 import path or its frames could
// never reach the screen — that check is first, once.
#[cfg(windows)]
+48 -11
View File
@@ -564,6 +564,21 @@ impl NativeD3d11Decoder {
/// rung closes it in `pf_vkdecode::decoder_av1`; this is the same close, and it
/// runs on the concealed path too, because a converted-but-unsubmitted frame
/// took a slot just the same.
///
/// # The surfaces the conversion refuses to release
///
/// The second of the two slot releases below, and the caller's half of
/// [`pf_dxvadec::DecodePlanDxvaAv1::release_after_decode`]. AV1 applies
/// `refresh_frame_flags` AFTER the frame is decoded (7.20), so a frame that reads
/// a slot its own refresh overwrites is ordinary — 268 of the vendored vector's
/// 274 frames — and `plan_to_dxva_av1` therefore hands those pictures back rather
/// than releasing them, because `SlotMap::assign` would return the surface just
/// vacated to `setup_slot` and the submission would name one surface as both
/// `CurrPicTextureIndex` and a `RefFrameMapTextureIndex` entry. Releasing them
/// HERE is safe for the same reason the `refresh_frame_flags == 0` release below
/// is: the decode op has been issued, so nothing can be assigned them before the
/// next frame. Dropping them instead holds a surface per frame and exhausts the
/// nine-slot ledger within ten.
fn frame_av1(
&mut self,
au: &[u8],
@@ -579,9 +594,18 @@ impl NativeD3d11Decoder {
// ⚠ The decode's `Result` is held rather than `?`-ed, so that the two slot
// releases below run on the FAILURE path too. `decode_av1` treats an error
// here as a health note and keeps the session — it does not rebuild the slot
// map — so an early return would leak a surface per failed frame and reach
// `SlotError::Full` after nine, which is a session that dies of an error it
// had already recovered from.
// map — so an early return leaked a surface per failed frame and would reach
// `SlotError::Full` after nine, a session dying of an error it had already
// recovered from.
//
// ⚠⚠ That closes THIS frame's leak and not the unit's: `decode_av1` returns on
// the first failing frame and abandons the rest of the temporal unit's plans,
// whose removals are then never released and whose stored ids are never
// assigned — the ledger and the planner's store desynchronise. 24 of the
// vendored vector's 250 units carry a second frame, so it is not hypothetical.
// Left as it is: recovering a partly-decoded unit means deciding what to do
// with the frames after the failure, which is the pump's question and not this
// function's, and the failure already ends in a keyframe request.
let shown = self.decode_and_present_av1(au, &sub, damaged);
// The surfaces this frame's own refresh displaced while its submission still
@@ -798,11 +822,20 @@ impl NativeD3d11Decoder {
slice_ranges: dxva.slice_ranges,
setup_slot: dxva.setup_slot,
setup_id: dxva.setup_id,
// H.264's conversion releases its whole `removed` list itself:
// neither vendored vector ever has a picture the slices read
// being displaced by the same access unit (measured: zero on
// 250 AUs), and the sliding window evicts only pictures no
// slice names.
// 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(),
codec: Codec::H264,
facts: PictureFacts {
@@ -863,9 +896,13 @@ impl NativeD3d11Decoder {
slice_ranges: dxva.slice_ranges,
setup_slot: dxva.setup_slot,
setup_id: dxva.setup_id,
// As H.264 above: an HEVC picture's RPS is resolved against the
// DPB before any removal, and a picture the RPS names is by
// construction still in it.
// 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.)
release_after_decode: Vec::new(),
codec: Codec::H265,
facts: PictureFacts {
+63
View File
@@ -881,6 +881,69 @@ mod tests {
assert_eq!(converted.len(), 250);
}
/// ⚠⚠ **TRIPWIRE, not a proof — and the thing it watches for is UNRESOLVED.**
///
/// 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.
///
/// 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.
///
/// **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.
///
/// 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.
///
/// 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.
#[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;
}
}
}
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"
);
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"
);
}
#[test]
fn the_setup_surface_is_the_current_picture_entry_and_is_never_also_a_reference_entry() {
for (_, dxva) in convert_stream() {
+63 -32
View File
@@ -139,19 +139,23 @@ 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), 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 carries the same contract for the same reason
/// 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
/// 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 the frame names, while `RefFrameMapTextureIndex` declares the
/// whole store, so every picture the store still names has to survive — not just
/// the seven the frame reads.
/// rung's constraint is the STRICTER of the two: Vulkan binds only the references
/// the frame names, while `RefFrameMapTextureIndex` declares the whole store, so
/// every picture the store still names has to survive — not just the seven the
/// frame reads.
///
/// The ids are always a subset of the plan's `dpb.removed`, so applying them
/// completes that plan's bookkeeping and never invents a removal. Empty on the
/// overwhelming minority of frames that displace nothing they name; a caller
/// that drops them leaks a surface per frame and runs the ledger dry within ten.
/// This is exactly `dpb.removed` less the picture being stored, and that is not a
/// coincidence to be tidied into a filter: the planner snapshots `dpb_refs` before
/// any mutation, so every removal is by construction a picture the store named
/// (see the conversion's own comment). Applying the list completes the plan's
/// bookkeeping and never invents a removal. A caller that drops it holds a surface
/// on nearly every frame and runs the ledger dry within ten.
pub release_after_decode: Vec<PicId>,
}
@@ -660,26 +664,32 @@ pub fn plan_to_dxva_av1(
}
// --- mutations, after every fallible step -----------------------------
// ⚠ A picture this submission NAMES may be displaced by this same frame's
// refresh. Its surface is still in `ref_frame_map` above, so releasing it here
// would hand that very surface to `setup_slot` below and the frame would decode
// into a picture it predicts from. Held back for the caller instead — see
// `DecodePlanDxvaAv1::release_after_decode` for the measurement and for why
// this rung's test is `dpb_refs` (the whole store `RefFrameMapTextureIndex`
// declares) rather than the Vulkan rung's narrower `refs`.
// ⚠ EVERY removed picture is held back, and nothing is released here at all.
//
// A picture this submission NAMES may be displaced by this same frame's refresh;
// its surface is still in `ref_frame_map` above, so releasing it would hand that
// very surface to `setup_slot` below and the frame would decode into a picture it
// predicts from. What makes the rule "every removal" rather than "the removals
// the store names" is a property of the PLANNER: `Av1Planner::plan_frame`
// snapshots `dpb_refs` before any mutation, and `refresh_slots` can only report a
// picture that was in `self.slots` at that moment, so `dpb.removed` is always a
// SUBSET of the store `ref_frame_map` was built from. Filtering on `dpb_refs`
// here would be a condition that is never false wearing the clothes of a
// decision; the subset relation is asserted in
// `the_decode_target_never_aliases_a_surface_the_submission_names` instead, where
// a planner change that broke it fails loudly.
//
// `setup_id` is excluded as a safety property rather than as a live case: a
// picture this frame stores cannot also be one its own refresh displaced, because
// `refresh_slots` retains out any displaced id still held anywhere. Releasing it
// would return the surface being decoded into.
let release_after_decode: Vec<PicId> = plan
.dpb
.removed
.iter()
.copied()
.filter(|id| *id != setup_id && plan.dpb_refs.iter().any(|r| r.id == *id))
.filter(|id| *id != setup_id)
.collect();
for &id in &plan.dpb.removed {
if id == setup_id || release_after_decode.contains(&id) {
continue;
}
let _ = slots.release(id);
}
let setup_slot = match slots.slot_of(setup_id) {
Some(existing) => existing,
None => slots.assign(setup_id)?,
@@ -906,19 +916,40 @@ mod tests {
RefFrameMapTextureIndex entry the frame decodes into a picture \
it predicts from"
);
// Every deferred id is one this plan really removed AND the store
// really names — never an invented removal, never a live picture.
// ⚠ THE PROPERTY THE DEFERRAL RESTS ON, asserted about the PLANNER
// rather than about the conversion's own output.
//
// The conversion holds back every removal, which is only safe-and-
// sufficient because `Av1Planner` snapshots `dpb_refs` before any
// mutation and `refresh_slots` can only report a picture that was in
// it — so `dpb.removed` is a subset of the store `ref_frame_map` was
// built from. Asserting instead that each DEFERRED id is in
// `dpb_refs` would be vacuous: the deferred list is filtered out of
// `removed`, so that check compares an expression with itself. This
// one can fail, and if a planner change ever makes it fail the
// conversion is releasing a surface the submission points at.
for &id in &plan.dpb.removed {
assert!(
plan.dpb_refs.iter().any(|r| r.id == id),
"frame {frames}: the planner removed picture {id}, which the \
pre-decode store never held `ref_frame_map` is built from \
that store, so a removal outside it is a picture this \
conversion could release without aliasing, and the blanket \
deferral above stops being justified"
);
}
// Every deferred id is one this plan really removed, and it still
// holds the surface the caller is being asked to give back.
for &id in &dx.release_after_decode {
assert!(
plan.dpb.removed.contains(&id),
"frame {frames}: deferred picture {id} is not in this plan's \
removed list"
);
assert!(
plan.dpb_refs.iter().any(|r| r.id == id),
"frame {frames}: picture {id} is deferred without being named \
by the store only a picture the submission points at earns \
the reprieve"
assert_ne!(
id, dx.setup_id,
"frame {frames}: the picture being decoded must never be \
deferred releasing it returns the surface being written"
);
assert!(
slots.slot_of(id).is_some(),
@@ -3690,6 +3690,10 @@ fn dump_our_submission_in_the_captures_own_format() {
let path = std::env::var("PF_DXVA_DUMP").expect("PF_DXVA_DUMP=<path> names the output file");
let mut text = dump("h264", &our_h264_submissions());
text.push_str(&dump("hevc", &our_hevc_submissions()));
// AV1 too, and it is the codec that needs this most: no libavcodec AV1 capture has
// ever been taken (module docs say why), so for that codec this dump is the only
// way to read what the driver is being handed at all.
text.push_str(&dump("av1", &our_av1_submissions()));
std::fs::write(&path, text).expect("write the dump");
println!("wrote {path}");
}