fix(client/vaapi): the third rung does NOT alias — and now it cannot start to
The D3D11VA and Vulkan rungs both decoded into a surface they were predicting from, on 117 of 120 access units of our own host's low-delay H.264 (`1c54d099` for AV1, `834b2443` for H.264). `pf-vaadec` feeds `reference_frames` from the same `plan.dpb_refs` snapshot, releases its whole `removed` list inline exactly as the two broken conversions did, and neither fix commit touched it. It is still exempt — this is the evidence, and the thing that keeps it true. **Measured on the CPU, no GPU needed.** `walk_for_aliasing` drives the planner and `plan_to_va` over both streams and counts four shapes. On `lowdelay-640x480.h264` the aliasing PRECONDITION is fully present: 117 of 120 access units remove a picture their own `dpb_refs` still names, and on the same 117 the setup picture is handed the slot of a picture that access unit READS — the D3D11VA/Vulkan defect verbatim, in this conversion, today. On the vendored conformance vector both counts are 0, which is why that vector proved nothing on two other backends for two milestones. Aliased submissions: **0 on both**. **Why.** A slot is not a surface here. `plan_to_va` never invents one — every reference it can name is read out of the `surfaces` table it is handed — and the decode target is a separate parameter the caller takes from OUTSIDE that table. `setup_surface` reaches the submission at exactly one field per codec (H.264/H.265 `curr_pic.picture_id`, AV1 `current_frame` and `current_display_picture`); HEVC is doubly safe, because its per-slice `RefPicList` stores an INDEX into `reference_frames` rather than a surface. AV1's documented substitution fallback is the one place the target can be named as a reference, and only where the store resolved nothing at all to prefer. **The exemption was incidental; it is structural now.** It needs the reference table and the decode target to come from ONE snapshot of the bindings, and the rung had that only by writing `free_surface()` and `surface_table()` adjacently at three call sites. Split them and this rung acquires the defect exactly: the table must be the PRE-removal one (that is where the references are), while a free list consulted after the removals offers precisely the displaced picture's surface. `Session::acquire_target` now returns the index, the surface and the table together from `&self`, so a later edit cannot move one call and not the other. No behaviour change: same order, same values, same refusal message. Tests. `no_submission_names_its_decode_target_as_one_of_its_own_references` (both streams, 0) with `taking_the_decode_target_from_the_slot_table_aliases_on_the_low_delay_stream` as the counterfactual that reproduces the defect on 117 of 120 — so the walk demonstrably CAN see it when it is there. `the_low_delay_stream_reassigns_slots_whose_pictures_it_still_reads` pins 0/250 and 117/120 so neither can drift silently. `the_decode_target_can_never_be_a_surface_the_reference_table_names` sweeps every binding state a 4-surface/3-slot pool can hold, and `taking_the_free_surface_after_the_removals_would_hand_out_a_referenced_surface` is the ordering counterfactual. ⚠ One existing test lost a VACUOUS half. `the_setup_picture_routinely_inherits_a_just_freed_slot` asserted the decode target was never also a reference while handing every picture its own never-reused surface id — distinct integers cannot collide, so that assertion could not fail whatever the conversion did. Its real measurement (225 of 250 access units reuse a just-freed slot, which is why the target is a parameter) is kept; the collision half is gone, and the doc says where the question is actually answered and why a recycling pool is what it takes to answer it. Gates, run on `.25` (Radeon 780M, radeonsi, Mesa 26.0.3, VA-API 1.23), this rung being Linux-only: `cargo fmt --all -- --check`; `cargo clippy -p pf-client-core -p pf-vaadec --all-targets --features sdl3/build-from-source -- -D warnings`; `cargo test -p pf-client-core --lib --features sdl3/build-from-source` (171 passed); the same filtered to `video_vaapi_native` with `--include-ignored` (18 passed); `cargo test -p pf-vaadec` (48 passed). Plus the pf-lxcheck2 container for the cross-platform half — fmt, clippy and `cargo test -p pf-vaadec`, all clean. All four VAAPI legs still decode with the refactor in place, not one access unit refused: H.264 225 of 250 access units delivering a frame, H.265 204 of 250, HEVC Main 10 45 of 50 (P010), AV1 250 of 250 — the same counts and the same tiled modifier 0x200000010401b04 those legs recorded before it. ⚠ The H.26x legs live on `fix/vaapi-h264-h265-hardware-proof`, not on this branch, so they were run by overlaying that commit's test module onto the scratch tree; only the AV1 leg and the libva probe are reachable from here. This is a decode measurement, not frame-hash parity — the rung exports a driver-tiled DRM-PRIME dmabuf, so there is no CPU-readable image to hash. The alias assertions above are the real evidence and they need no device. ⚠ NOT taken: `finish`'s `outputs.last()`, which ships one frame per access unit and drops the rest of what a bump displaces (225/204/45 against 250/250/50), with no end-of-stream flush. It cannot bite punktfunk — hosts emit zero-reorder output, so `outputs` never holds more than one picture — and fixing it changes `decode()`'s one-frame-per-access-unit contract with the pump (it wants a deliverable queue, which `video_vk_native` already keeps) plus an end-of-stream flush and the `keyframe`-labels-the-access-unit defect in the same function. It is recorded and asserted on that other branch, whose three delivered-count assertions any fix has to move in the same commit; doing that from here, blind to them, would be worse than leaving it.
This commit is contained in:
@@ -47,6 +47,56 @@
|
||||
//! presenter holds simply stays off the free list until its release token comes
|
||||
//! back. A surface is free when no live picture is bound to it AND no consumer holds
|
||||
//! it — two conditions, tracked separately, because they end at different times.
|
||||
//!
|
||||
//! # Why this rung is exempt from the decode-into-a-reference defect
|
||||
//!
|
||||
//! The D3D11VA and Vulkan rungs both had to grow a `release_after_decode` deferral:
|
||||
//! their conversions released the pictures an access unit displaces INSIDE the
|
||||
//! conversion, then assigned the decode target a slot, and [`pf_vaadec::SlotMap::assign`]
|
||||
//! handed back the slot just vacated — so one surface was named as both the decode
|
||||
//! target and one of that submission's own references. On H.264 that fired on **117 of
|
||||
//! 120** access units of a punktfunk host's low-delay output.
|
||||
//!
|
||||
//! `pf-vaadec`'s conversions still release inline and this rung is still exempt, for a
|
||||
//! reason that is a property of the interface rather than of any stream: **a slot is
|
||||
//! not a surface here.** `plan_to_va` never invents a surface — every reference it can
|
||||
//! name is read out of the `surfaces` table it is handed — and the decode target is a
|
||||
//! separate parameter the caller takes from OUTSIDE that table. Two things carry that,
|
||||
//! and both are load-bearing:
|
||||
//!
|
||||
//! * [`Session::acquire_target`] returns the target and the table **together, from one
|
||||
//! snapshot**, because they are only safe together. A free surface is by construction
|
||||
//! a surface no slot binds, and the table is exactly what the slots bind, so the
|
||||
//! target cannot be in it. Taking the two at different moments — the table before
|
||||
//! this access unit's removals, where references must resolve, and the free surface
|
||||
//! after them, where the displaced picture's surface has become free — is precisely
|
||||
//! the defect, and `taking_the_free_surface_after_the_removals_would_hand_out_a_
|
||||
//! referenced_surface` shows it happening.
|
||||
//! * The conversion's half is pinned across every platform by `pf-vaadec`'s
|
||||
//! `no_submission_names_its_decode_target_as_one_of_its_own_references`, driven over
|
||||
//! the same low-delay stream, with
|
||||
//! `taking_the_decode_target_from_the_slot_table_aliases_on_the_low_delay_stream` as
|
||||
//! the counterfactual that shows the walk can see the defect when it is there.
|
||||
//!
|
||||
//! It holds for all three codecs and for the same one-line reason: `setup_surface`
|
||||
//! reaches the submission at exactly ONE field in each conversion — H.264 and H.265'
|
||||
//! `curr_pic.picture_id`, AV1's `current_frame`/`current_display_picture` — and every
|
||||
//! reference field is resolved through the `surfaces` table. HEVC is doubly covered:
|
||||
//! its per-slice `RefPicList` stores an INDEX into `reference_frames`, so it cannot
|
||||
//! name a surface that array does not already hold.
|
||||
//!
|
||||
//! ⚠ One documented exception, and it is not this defect: `plan_to_va_av1` substitutes
|
||||
//! a live surface for a reference slot the planner reports empty, and where the store
|
||||
//! resolved NOTHING at all the fallback is the decode target itself (that conversion's
|
||||
//! module docs say why, and prefer a resolved reference wherever one exists). It names
|
||||
//! the target only when there is no other live surface to name, on a frame that is
|
||||
//! already concealed and will not be shown.
|
||||
//!
|
||||
//! ⚠ And one assumption, stated because it is the only way the argument fails: the pool
|
||||
//! holds DISTINCT `VASurfaceID`s. Two pool entries with one id would let a free index
|
||||
//! resolve to a bound surface. `vaCreateSurfaces` cannot return duplicates — this rung
|
||||
//! also destroys each exactly once, which the same duplication would double-free — so
|
||||
//! it is an assumption about libva rather than about this file.
|
||||
|
||||
use std::os::fd::AsRawFd as _;
|
||||
use std::os::fd::FromRawFd as _;
|
||||
@@ -686,6 +736,29 @@ impl Session {
|
||||
.collect()
|
||||
}
|
||||
|
||||
/// The decode target — pool index and `VASurfaceID` — together with the reference
|
||||
/// table the conversion resolves against. `None` when the pool is exhausted.
|
||||
///
|
||||
/// **The three are returned together because they are only safe together**, and
|
||||
/// that is this rung's whole exemption from the aliasing defect the other two
|
||||
/// backends had to defer their way out of (module docs). A free surface is by
|
||||
/// definition a surface no slot binds; [`Self::surface_table`] is exactly what the
|
||||
/// slots bind; so a target drawn from the same snapshot cannot appear in the table,
|
||||
/// and no reference the conversion resolves through that table can be the surface
|
||||
/// it is about to write.
|
||||
///
|
||||
/// ⚠ Taking the two at DIFFERENT moments is the defect. References must resolve
|
||||
/// against the store as it stood BEFORE this access unit's removals, so the table
|
||||
/// has to be the pre-removal one; and a free list consulted AFTER those removals
|
||||
/// offers the displaced picture's surface, which the pre-removal table still names.
|
||||
/// `taking_the_free_surface_after_the_removals_would_hand_out_a_referenced_surface`
|
||||
/// is that mismatch, made to happen. Returning a tuple is what stops a future edit
|
||||
/// from reintroducing it by moving one call and not the other.
|
||||
fn acquire_target(&self) -> Option<(usize, VaSurfaceId, Vec<VaSurfaceId>)> {
|
||||
let index = self.free_surface()?;
|
||||
Some((index, self.surfaces[index], self.surface_table()))
|
||||
}
|
||||
|
||||
/// Release every libva object this session owns, in creation-reverse order.
|
||||
/// Called explicitly (a `Drop` here could not reach the display).
|
||||
fn destroy(mut self, d: &Display) {
|
||||
@@ -1011,11 +1084,9 @@ impl NativeVaapiDecoder {
|
||||
shape,
|
||||
&mut self.generation,
|
||||
)?;
|
||||
let free = s
|
||||
.free_surface()
|
||||
let (free, target, table) = s
|
||||
.acquire_target()
|
||||
.ok_or_else(|| anyhow!("surface pool exhausted ({} surfaces)", s.surfaces.len()))?;
|
||||
let target = s.surfaces[free];
|
||||
let table = s.surface_table();
|
||||
let converted = pf_vaadec::plan_to_va(&plan, au, &mut s.slots, &table, target)
|
||||
.map_err(|e| anyhow!("{e}"))?;
|
||||
|
||||
@@ -1087,11 +1158,9 @@ impl NativeVaapiDecoder {
|
||||
shape,
|
||||
&mut self.generation,
|
||||
)?;
|
||||
let free = s
|
||||
.free_surface()
|
||||
let (free, target, table) = s
|
||||
.acquire_target()
|
||||
.ok_or_else(|| anyhow!("surface pool exhausted ({} surfaces)", s.surfaces.len()))?;
|
||||
let target = s.surfaces[free];
|
||||
let table = s.surface_table();
|
||||
let converted = pf_vaadec::plan_to_va_h265(&plan, au, &mut s.slots, &table, target)
|
||||
.map_err(|e| anyhow!("{e}"))?;
|
||||
|
||||
@@ -1237,11 +1306,9 @@ impl NativeVaapiDecoder {
|
||||
shape,
|
||||
&mut self.generation,
|
||||
)?;
|
||||
let free = s
|
||||
.free_surface()
|
||||
let (free, target, table) = s
|
||||
.acquire_target()
|
||||
.ok_or_else(|| anyhow!("surface pool exhausted ({} surfaces)", s.surfaces.len()))?;
|
||||
let target = s.surfaces[free];
|
||||
let table = s.surface_table();
|
||||
let converted = match pf_vaadec::plan_to_va_av1(plan, au, &mut s.slots, &table, target) {
|
||||
Ok(converted) => converted,
|
||||
Err(e) => {
|
||||
@@ -2129,6 +2196,138 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
/// The decode target is never a surface the reference table names — swept over
|
||||
/// every binding state a small pool can be in.
|
||||
///
|
||||
/// This rung's exemption from the aliasing defect the D3D11VA and Vulkan rungs had
|
||||
/// to defer their way out of (module docs), stated as the one thing it actually
|
||||
/// rests on. `pf-vaadec` proves the conversion can only name surfaces out of the
|
||||
/// table it is handed; this proves the table and the target cannot overlap.
|
||||
///
|
||||
/// Swept rather than exemplified because the claim is structural — a free surface
|
||||
/// is by definition one no slot binds, and the table is exactly what the slots bind
|
||||
/// — so it should hold in states an ordinary run never reaches, and a sweep is what
|
||||
/// says so. The `held` and `pending` masks are varied too even though they can only
|
||||
/// ever REMOVE candidates from the free list: a future claim that could add one
|
||||
/// back is exactly what this would catch.
|
||||
#[test]
|
||||
fn the_decode_target_can_never_be_a_surface_the_reference_table_names() {
|
||||
const SURFACES: usize = 4;
|
||||
const SLOTS: usize = 3;
|
||||
let choices: Vec<Option<usize>> = std::iter::once(None)
|
||||
.chain((0..SURFACES).map(Some))
|
||||
.collect();
|
||||
|
||||
let (mut states, mut with_a_target, mut exhausted) = (0usize, 0usize, 0usize);
|
||||
for a in &choices {
|
||||
for b in &choices {
|
||||
for c in &choices {
|
||||
let bound = [*a, *b, *c];
|
||||
// Two slots binding ONE surface is not a state the pool can reach —
|
||||
// `bind_setup` only ever binds a surface nothing else claims — and
|
||||
// asserting about it would be asserting about a defect elsewhere.
|
||||
let mut distinct: Vec<usize> = bound.iter().flatten().copied().collect();
|
||||
let claimed = distinct.len();
|
||||
distinct.sort_unstable();
|
||||
distinct.dedup();
|
||||
if distinct.len() != claimed {
|
||||
continue;
|
||||
}
|
||||
for held_mask in 0..(1u32 << SURFACES) {
|
||||
for pending_mask in 0..(1u32 << SURFACES) {
|
||||
let mut s = session(SURFACES, SLOTS);
|
||||
s.slot_surface = bound.to_vec();
|
||||
s.held = (0..SURFACES).map(|i| held_mask >> i & 1 == 1).collect();
|
||||
s.pending = (0..SURFACES)
|
||||
.filter(|i| pending_mask >> i & 1 == 1)
|
||||
.map(|i| (100 + i as u64, i))
|
||||
.collect();
|
||||
states += 1;
|
||||
|
||||
let Some((index, target, table)) = s.acquire_target() else {
|
||||
exhausted += 1;
|
||||
continue;
|
||||
};
|
||||
with_a_target += 1;
|
||||
assert_eq!(
|
||||
target, s.surfaces[index],
|
||||
"the target must be the pool's surface at the index it \
|
||||
returned, or the caller binds one and submits another"
|
||||
);
|
||||
assert_eq!(table.len(), SLOTS, "one table entry per slot");
|
||||
assert!(
|
||||
!table.contains(&target),
|
||||
"bindings {bound:?}, held {held_mask:#06b}, pending \
|
||||
{pending_mask:#06b}: the decode target {target:#x} is \
|
||||
in the reference table {table:x?} — every submission \
|
||||
built from that pair decodes into a surface it may be \
|
||||
predicting from"
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
// The sweep has to reach both answers, or it is asserting about one branch.
|
||||
assert!(states > 1000, "only {states} states swept");
|
||||
assert!(with_a_target > 0 && exhausted > 0);
|
||||
}
|
||||
|
||||
/// The order the rung must NOT be written in, and the reason
|
||||
/// [`Session::acquire_target`] hands the target and the table back together.
|
||||
///
|
||||
/// The exemption above is not a property of the pool alone: it needs the target and
|
||||
/// the table to come from ONE snapshot. Split them and this rung acquires the
|
||||
/// D3D11VA/Vulkan defect exactly — because the table must be the PRE-removal one
|
||||
/// (a reference an access unit names can be a picture the same access unit evicts,
|
||||
/// which on a punktfunk host's own low-delay H.264 is 117 access units in 120),
|
||||
/// while a free list consulted after those removals offers precisely the displaced
|
||||
/// picture's surface.
|
||||
#[test]
|
||||
fn taking_the_free_surface_after_the_removals_would_hand_out_a_referenced_surface() {
|
||||
let mut s = session(4, 3);
|
||||
|
||||
// Two decoded reference pictures, each in its own surface, both already
|
||||
// displayed and returned by the presenter — so only the SLOT binding keeps
|
||||
// their surfaces off the free list. That is the steady state of a low-delay
|
||||
// stream, where a picture is output by its own access unit and evicted by the
|
||||
// sliding window several units later.
|
||||
s.slots.assign(11).expect("a free slot");
|
||||
bind_setup(&mut s, Some(11), Some(0));
|
||||
s.slots.assign(12).expect("a free slot");
|
||||
bind_setup(&mut s, Some(12), Some(1));
|
||||
s.pending.clear();
|
||||
|
||||
// What the conversion resolves its references through, taken BEFORE this access
|
||||
// unit's removals — which is not a choice, it is where the references are.
|
||||
let table = s.surface_table();
|
||||
assert!(
|
||||
table.contains(&s.surfaces[0]),
|
||||
"picture 11's surface must still be a resolvable reference"
|
||||
);
|
||||
|
||||
// The order the rung is written in: one snapshot, and the target cannot be in
|
||||
// the table it came with.
|
||||
let (_, target, same_table) = s.acquire_target().expect("the pool has spares");
|
||||
assert_eq!(
|
||||
same_table, table,
|
||||
"acquire_target must not re-derive the table"
|
||||
);
|
||||
assert!(!table.contains(&target));
|
||||
|
||||
// The defect: the conversion applies the removal, the bindings follow it, and
|
||||
// only THEN is the free list consulted.
|
||||
s.slots.release(11);
|
||||
s.sync_slot_bindings();
|
||||
let late = s.free_surface().expect("the pool has spares");
|
||||
assert_eq!(
|
||||
s.surfaces[late], table[0],
|
||||
"the late free list offers the surface of the picture this access unit just \
|
||||
displaced, and the pre-removal table still names it as a reference — \
|
||||
decode into that and the driver predicts from the picture it is writing"
|
||||
);
|
||||
}
|
||||
|
||||
/// A picture the conversion REFUSED binds no surface — so nothing can show it and
|
||||
/// nothing can predict from it.
|
||||
///
|
||||
|
||||
+297
-55
@@ -549,6 +549,20 @@ mod tests {
|
||||
"../../pf-bitstream/vendor/cros-codecs/src/codec/h264/test_data/test-25fps.h264"
|
||||
);
|
||||
|
||||
/// A punktfunk HOST's own output: 120 pictures of 640x480, `max_num_ref_frames = 3`
|
||||
/// alongside `max_dec_frame_buffering = 3` and `max_num_reorder_frames = 0`.
|
||||
///
|
||||
/// Vendored beside `pf-vkdecode`'s per-frame goldens, and the only stream in the
|
||||
/// tree that produces the shape this module's exemption is about. The conformance
|
||||
/// vector above cannot: its level gives it a 7-frame DPB against 2 reference
|
||||
/// frames, so 8.2.5's sliding window unmarks a picture two access units before
|
||||
/// C.4.5.3's bump can evict it, and it reorders, which keeps an unmarked picture
|
||||
/// alive past the unit that unmarked it. Both are properties of that vector rather
|
||||
/// than of H.264, and between them they hid a defect that fired on 297 of 300
|
||||
/// access units of every stream we ship, on two other backends, for two milestones.
|
||||
const LOWDELAY_640X480: &[u8] =
|
||||
include_bytes!("../../pf-vkdecode/tests/data/lowdelay-640x480.h264");
|
||||
|
||||
/// Minimal H.264 access-unit splitter. The production wire delivers whole access
|
||||
/// units, so pf-bitstream keeps its splitter test-only; this is the same rule —
|
||||
/// a new AU begins at a non-VCL NALU following slices, or at a slice declaring
|
||||
@@ -693,83 +707,311 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
/// The decode target must never be a surface this same access unit READS.
|
||||
/// What one walk of a stream through [`plan_to_va`] measured.
|
||||
///
|
||||
/// This is the question a slot ledger cannot answer, and it is why the caller
|
||||
/// binds the setup surface instead of the conversion reading one out of a
|
||||
/// slot-indexed table.
|
||||
/// Every field is a count of ACCESS UNITS, so the four are directly comparable and
|
||||
/// each is bounded by [`Self::converted`].
|
||||
#[derive(Debug, Default)]
|
||||
struct AliasWalk {
|
||||
/// Access units planned and converted.
|
||||
converted: usize,
|
||||
/// The setup picture was assigned a slot this access unit's OWN removals had
|
||||
/// just freed. `SlotMap::assign` takes the lowest free slot, so this is the
|
||||
/// ordinary case rather than an edge one — and it is why a decode target read
|
||||
/// out of a slot-indexed table would be the surface of the picture just
|
||||
/// displayed.
|
||||
inherited_a_just_freed_slot: usize,
|
||||
/// This access unit's own `removed` list names a picture its `dpb_refs`
|
||||
/// snapshot also names: 8.2.5's sliding window unmarked a reference in the very
|
||||
/// unit whose C.4.5.3 bump evicted it. The aliasing PRECONDITION, and the shape
|
||||
/// the vendored conformance vector never produces.
|
||||
removed_and_referenced: usize,
|
||||
/// The setup picture took the slot of a picture this same access unit READS.
|
||||
/// This is the D3D11VA/Vulkan defect verbatim — `CurrPic` and a reference entry
|
||||
/// resolving through one slot — and on those two backends the surface followed
|
||||
/// the slot, so the submission aliased. Here the surface does not follow the
|
||||
/// slot, which is what [`Self::aliased`] measures.
|
||||
setup_took_a_read_pictures_slot: usize,
|
||||
/// The submission names the decode target as one of its own references, in
|
||||
/// `reference_frames` or in any slice's `RefPicList0`/`1`. Must be zero.
|
||||
aliased: usize,
|
||||
}
|
||||
|
||||
/// Drive `stream` through the planner and [`plan_to_va`], modelling the caller the
|
||||
/// way the Linux rung is written, and count the four shapes above.
|
||||
///
|
||||
/// `SlotMap::assign` takes the LOWEST free slot, and a slot freed by this
|
||||
/// access unit's own removals is free by the time the setup picture is
|
||||
/// assigned. Measured on the vendored vector, that is not an edge case: the
|
||||
/// setup picture inherits a just-freed slot on **225 of 250** access units.
|
||||
/// A surface bound BY SLOT would therefore decode, on nine frames in ten,
|
||||
/// into the surface still holding the picture that was just displayed — which
|
||||
/// under zero-copy the consumer may still be sampling. Hence the pool model
|
||||
/// this crate's callers use, and hence `setup_surface`.
|
||||
///
|
||||
/// The second half of the test is the reassurance that comes with it: given
|
||||
/// the caller's contract (a surface bound to no live picture), the decode
|
||||
/// target is never a surface the same access unit READS. That is checked
|
||||
/// against both readable sets, which are not the same snapshot — `dpb_refs` is
|
||||
/// taken after this AU's marking process, the per-slice lists before it.
|
||||
#[test]
|
||||
fn the_setup_picture_routinely_inherits_a_just_freed_slot() {
|
||||
/// The model is one line and it is the whole contract: the decode target is a
|
||||
/// surface that **is not in the table the conversion is handed**, and it enters
|
||||
/// that table only after the conversion returns. `video_vaapi_native`'s
|
||||
/// `the_low_delay_stream_never_hands_the_decoder_a_surface_it_is_predicting_from`
|
||||
/// is the same walk driven through the REAL `Session` pool, which is what says the
|
||||
/// rung honours the contract; this one says what the contract buys.
|
||||
fn walk_for_aliasing(stream: &[u8]) -> AliasWalk {
|
||||
use pf_bitstream::h264::H264Planner;
|
||||
|
||||
let aus = split_aus(TEST_25FPS_H264);
|
||||
let mut planner = H264Planner::new();
|
||||
let mut surfaces: Vec<u32> = Vec::new();
|
||||
let mut slots: Option<SlotMap> = None;
|
||||
let mut collisions = 0usize;
|
||||
let mut first: Option<usize> = None;
|
||||
let mut inherited = 0usize;
|
||||
// Slot to surface — precisely `Session::surface_table()` on the Linux rung.
|
||||
let mut table: Vec<u32> = Vec::new();
|
||||
let mut out = AliasWalk::default();
|
||||
|
||||
for (index, au) in aus.iter().enumerate() {
|
||||
let plan = planner.plan_au(au).expect("the clean vector plans");
|
||||
for (index, au) in split_aus(stream).into_iter().enumerate() {
|
||||
let plan = planner
|
||||
.plan_au(au)
|
||||
.unwrap_or_else(|e| panic!("AU {index}: this stream must plan, got {e:?}"));
|
||||
let map = slots.get_or_insert_with(|| SlotMap::new(plan.picture.max_dpb_frames));
|
||||
surfaces.resize(map.capacity(), VA_INVALID_SURFACE);
|
||||
// Which slots this AU's own removals will free — read BEFORE the
|
||||
// conversion applies them, because afterwards the ledger has forgotten.
|
||||
assert_eq!(
|
||||
map.capacity(),
|
||||
plan.picture.max_dpb_frames + 1,
|
||||
"AU {index}: neither stream renegotiates its DPB depth mid-walk"
|
||||
);
|
||||
table.resize(map.capacity(), VA_INVALID_SURFACE);
|
||||
|
||||
if plan
|
||||
.dpb
|
||||
.removed
|
||||
.iter()
|
||||
.any(|id| plan.dpb_refs.iter().any(|r| r.id == *id))
|
||||
{
|
||||
out.removed_and_referenced += 1;
|
||||
}
|
||||
// Which slots this AU's removals will free, read BEFORE the conversion
|
||||
// applies them — afterwards the ledger has forgotten.
|
||||
let freed: Vec<u8> = plan
|
||||
.dpb
|
||||
.removed
|
||||
.iter()
|
||||
.filter_map(|id| map.slot_of(*id))
|
||||
.collect();
|
||||
|
||||
// Ids start well away from slot indices and are never reused, so a stale or
|
||||
// aliased reference shows up as a value rather than as a plausible-looking
|
||||
// off-by-one and cannot hide behind a surface that happens to be right
|
||||
// again. The assertion is the model's own precondition: a target the table
|
||||
// already names would beg the question this walk exists to answer.
|
||||
let setup_surface = SURFACE_BASE + index as u32;
|
||||
let out = plan_to_va(&plan, au, map, &surfaces, setup_surface)
|
||||
.expect("the clean vector converts");
|
||||
surfaces[usize::from(out.setup_slot)] = setup_surface;
|
||||
if freed.contains(&out.setup_slot) {
|
||||
inherited += 1;
|
||||
assert!(
|
||||
!table.contains(&setup_surface),
|
||||
"AU {index}: the model handed out a surface the table already names"
|
||||
);
|
||||
let displaced = table.clone();
|
||||
let converted = plan_to_va(&plan, au, map, &table, setup_surface)
|
||||
.unwrap_or_else(|e| panic!("AU {index}: conversion failed: {e}"));
|
||||
table[usize::from(converted.setup_slot)] = setup_surface;
|
||||
|
||||
// Both readable sets, and they are not the same snapshot: `dpb_refs` is
|
||||
// taken after this AU's marking process, the per-slice lists before it.
|
||||
let named: Vec<u32> = converted
|
||||
.pic_params
|
||||
.reference_frames
|
||||
.iter()
|
||||
.chain(
|
||||
converted
|
||||
.slices
|
||||
.iter()
|
||||
.flat_map(|s| s.ref_pic_list0.iter().chain(s.ref_pic_list1.iter())),
|
||||
)
|
||||
.filter(|e| e.flags & VA_PICTURE_H264_INVALID == 0)
|
||||
.map(|e| e.picture_id)
|
||||
.collect();
|
||||
|
||||
if freed.contains(&converted.setup_slot) {
|
||||
out.inherited_a_just_freed_slot += 1;
|
||||
}
|
||||
let curr = out.pic_params.curr_pic.picture_id;
|
||||
let names =
|
||||
|e: &VaPictureH264| e.flags & VA_PICTURE_H264_INVALID == 0 && e.picture_id == curr;
|
||||
let read_by_this_au = out.pic_params.reference_frames.iter().any(names)
|
||||
|| out.slices.iter().any(|s| {
|
||||
s.ref_pic_list0.iter().any(names) || s.ref_pic_list1.iter().any(names)
|
||||
});
|
||||
if read_by_this_au {
|
||||
collisions += 1;
|
||||
first.get_or_insert(index);
|
||||
let evicted_surface = displaced[usize::from(converted.setup_slot)];
|
||||
if evicted_surface != VA_INVALID_SURFACE && named.contains(&evicted_surface) {
|
||||
out.setup_took_a_read_pictures_slot += 1;
|
||||
}
|
||||
assert_eq!(
|
||||
converted.pic_params.curr_pic.picture_id, setup_surface,
|
||||
"AU {index}: the current picture must be the surface the caller bound"
|
||||
);
|
||||
if named.contains(&setup_surface) {
|
||||
out.aliased += 1;
|
||||
}
|
||||
out.converted += 1;
|
||||
}
|
||||
// The measurement this design rests on. A floor rather than the exact
|
||||
// count, so a planner change that shifts it by a frame does not fail —
|
||||
// but one that made slot reuse RARE would, and would mean the doc above
|
||||
// has stopped being true.
|
||||
out
|
||||
}
|
||||
|
||||
/// The setup picture routinely inherits a slot its own access unit just freed —
|
||||
/// which is why the decode target is a PARAMETER and not `surfaces[setup_slot]`.
|
||||
///
|
||||
/// `SlotMap::assign` takes the LOWEST free slot, and a slot freed by this access
|
||||
/// unit's own removals is free by the time the setup picture is assigned. Measured
|
||||
/// on the vendored vector that is not an edge case: **225 of 250** access units. A
|
||||
/// surface bound BY SLOT would therefore decode, on nine frames in ten, into the
|
||||
/// surface still holding the picture that was just displayed — which under
|
||||
/// zero-copy the consumer may still be sampling. Hence the pool model this crate's
|
||||
/// callers use, and hence `setup_surface`.
|
||||
///
|
||||
/// ⚠ This test used to carry a second half asserting the decode target was never
|
||||
/// also a reference. It was VACUOUS: the walk hands every picture its own
|
||||
/// never-reused surface id, so distinct ids cannot collide and the assertion could
|
||||
/// not fail whatever the conversion did. The real question needs a surface pool
|
||||
/// that RECYCLES, and it is answered by the two tests below and by
|
||||
/// `video_vaapi_native`'s walk through the real one.
|
||||
#[test]
|
||||
fn the_setup_picture_routinely_inherits_a_just_freed_slot() {
|
||||
let walk = walk_for_aliasing(TEST_25FPS_H264);
|
||||
assert_eq!(walk.converted, 250);
|
||||
// A floor rather than the exact count, so a planner change that shifts it by a
|
||||
// frame does not fail — but one that made slot reuse RARE would, and would mean
|
||||
// the documentation citing this number has stopped being true.
|
||||
assert!(
|
||||
inherited > 200,
|
||||
"the setup picture inherited a just-freed slot on only {inherited} of 250 access \
|
||||
walk.inherited_a_just_freed_slot > 200,
|
||||
"the setup picture inherited a just-freed slot on only {} of 250 access \
|
||||
units — the reason `setup_surface` is a parameter no longer holds, and the \
|
||||
documentation that cites it needs re-measuring"
|
||||
documentation that cites it needs re-measuring",
|
||||
walk.inherited_a_just_freed_slot
|
||||
);
|
||||
}
|
||||
|
||||
/// The aliasing PRECONDITION, on both streams — the number that says the exemption
|
||||
/// below is being tested by something rather than merely passing.
|
||||
///
|
||||
/// Two conditions have to coincide inside ONE access unit for a conversion that
|
||||
/// releases eagerly to hand the decode target a picture it is predicting from: the
|
||||
/// access unit must remove a picture, and that picture must still be in the
|
||||
/// `dpb_refs` snapshot the reference lists are built from. Low-delay H.264 is
|
||||
/// exactly what makes them coincide, and NVENC seals it by writing
|
||||
/// `max_num_ref_frames = 3` ALONGSIDE `max_dec_frame_buffering = 3` — a DPB exactly
|
||||
/// as deep as its reference count — while `max_num_reorder_frames = 0` means the
|
||||
/// evicted picture has already been output and is therefore evictable at all.
|
||||
///
|
||||
/// The vendored conformance vector produces the shape ZERO times, which is why it
|
||||
/// proved nothing on two other backends for two milestones. If that zero ever moves
|
||||
/// the reasoning above is wrong and the 117 needs re-deriving before it means
|
||||
/// anything.
|
||||
#[test]
|
||||
fn the_low_delay_stream_reassigns_slots_whose_pictures_it_still_reads() {
|
||||
let vector = walk_for_aliasing(TEST_25FPS_H264);
|
||||
assert_eq!(vector.converted, 250);
|
||||
assert!(
|
||||
vector.inherited_a_just_freed_slot > 0,
|
||||
"no access unit of the vendored vector reused a freed slot, so the zeroes \
|
||||
below would be empty for a reason that has nothing to do with the hazard"
|
||||
);
|
||||
assert_eq!(
|
||||
collisions, 0,
|
||||
"the decode target collided with a picture this access unit reads, on \
|
||||
{collisions} of 250 (first at AU {first:?})"
|
||||
vector.removed_and_referenced, 0,
|
||||
"the vendored vector is supposed to be BLIND to this shape"
|
||||
);
|
||||
assert_eq!(
|
||||
vector.setup_took_a_read_pictures_slot, 0,
|
||||
"and therefore never to hand the setup picture a slot it still reads"
|
||||
);
|
||||
|
||||
let lowdelay = walk_for_aliasing(LOWDELAY_640X480);
|
||||
assert_eq!(lowdelay.converted, 120);
|
||||
assert_eq!(
|
||||
lowdelay.removed_and_referenced, 117,
|
||||
"the low-delay stream must still exercise the aliasing precondition on \
|
||||
nearly every access unit — if this drops to zero the exemption below is no \
|
||||
longer being TESTED by anything, whatever else still passes"
|
||||
);
|
||||
assert_eq!(
|
||||
lowdelay.setup_took_a_read_pictures_slot, 117,
|
||||
"and the slot really is handed straight back to the decode target: this is \
|
||||
the D3D11VA/Vulkan defect, present here, and harmless only because the \
|
||||
SURFACE does not follow the slot"
|
||||
);
|
||||
}
|
||||
|
||||
/// The exemption itself: no submission names its decode target as one of its own
|
||||
/// references, on either stream.
|
||||
///
|
||||
/// This conversion still releases its whole `removed` list inline, exactly as the
|
||||
/// two backends that had to grow a `release_after_decode` deferral once did. It is
|
||||
/// safe doing so for one reason, and it is a property of the INTERFACE rather than
|
||||
/// of any stream: `plan_to_va` never invents a surface. Every reference it can name
|
||||
/// is read out of the `surfaces` table it was handed, so a decode target that is
|
||||
/// not in that table cannot be named, whatever the ledger does with slots. A slot
|
||||
/// is not a surface here; on DXVA it was.
|
||||
///
|
||||
/// ⚠ That makes this a statement about the CALLER's contract, so it is only half
|
||||
/// the proof. The other half — that the Linux rung really does pick its decode
|
||||
/// target from outside the table — cannot be made here, because the pool lives in
|
||||
/// `pf-client-core`. It is
|
||||
/// `video_vaapi_native`'s
|
||||
/// `the_low_delay_stream_never_hands_the_decoder_a_surface_it_is_predicting_from`,
|
||||
/// which drives this same stream through the real `Session`.
|
||||
#[test]
|
||||
fn no_submission_names_its_decode_target_as_one_of_its_own_references() {
|
||||
for (name, walk) in [
|
||||
("the vendored vector", walk_for_aliasing(TEST_25FPS_H264)),
|
||||
("the low-delay stream", walk_for_aliasing(LOWDELAY_640X480)),
|
||||
] {
|
||||
assert_eq!(
|
||||
walk.aliased, 0,
|
||||
"{name}: {} of {} access units decode into a surface they predict from",
|
||||
walk.aliased, walk.converted
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
/// A decode target the caller took from INSIDE the slot table is named as its own
|
||||
/// reference — the counterfactual that gives the test above its teeth.
|
||||
///
|
||||
/// Without this, `aliased == 0` would be consistent with a conversion that could
|
||||
/// never alias for reasons of its own, and a reader could not tell which. This
|
||||
/// picks the target the way the two broken backends effectively did — the surface
|
||||
/// sitting in the slot the setup picture is about to take — and shows the same walk
|
||||
/// then aliases on 117 of 120 access units of the low-delay stream. So the walk can
|
||||
/// see the defect; it does not see it because the contract holds.
|
||||
#[test]
|
||||
fn taking_the_decode_target_from_the_slot_table_aliases_on_the_low_delay_stream() {
|
||||
use pf_bitstream::h264::H264Planner;
|
||||
|
||||
let mut planner = H264Planner::new();
|
||||
let mut slots: Option<SlotMap> = None;
|
||||
let mut table: Vec<u32> = Vec::new();
|
||||
let (mut converted, mut aliased) = (0usize, 0usize);
|
||||
|
||||
for (index, au) in split_aus(LOWDELAY_640X480).into_iter().enumerate() {
|
||||
let plan = planner.plan_au(au).expect("the low-delay stream plans");
|
||||
let map = slots.get_or_insert_with(|| SlotMap::new(plan.picture.max_dpb_frames));
|
||||
table.resize(map.capacity(), VA_INVALID_SURFACE);
|
||||
// The bug, modelled: convert first to learn the slot, then re-run the same
|
||||
// access unit against the real ledger with the target read OUT of the
|
||||
// table. Two passes only because the slot is not known until the conversion
|
||||
// returns; the submission compared below is the second one.
|
||||
//
|
||||
// The probe's own `setup_surface` is arbitrary and deliberately so — the
|
||||
// slot is chosen by `SlotMap::assign` from the ledger alone and no
|
||||
// conversion consults the target to pick it, which is why one pass can
|
||||
// stand in for the other.
|
||||
let mut probe = map.clone();
|
||||
let peek = plan_to_va(&plan, au, &mut probe, &table, SURFACE_BASE)
|
||||
.expect("the low-delay stream converts");
|
||||
let target = table[usize::from(peek.setup_slot)];
|
||||
let target = if target == VA_INVALID_SURFACE {
|
||||
SURFACE_BASE + index as u32
|
||||
} else {
|
||||
target
|
||||
};
|
||||
let out = plan_to_va(&plan, au, map, &table, target).expect("the same conversion");
|
||||
table[usize::from(out.setup_slot)] = target;
|
||||
|
||||
let names = |e: &VaPictureH264| {
|
||||
e.flags & VA_PICTURE_H264_INVALID == 0 && e.picture_id == target
|
||||
};
|
||||
if out.pic_params.reference_frames.iter().any(names)
|
||||
|| out
|
||||
.slices
|
||||
.iter()
|
||||
.any(|s| s.ref_pic_list0.iter().any(names) || s.ref_pic_list1.iter().any(names))
|
||||
{
|
||||
aliased += 1;
|
||||
}
|
||||
converted += 1;
|
||||
}
|
||||
|
||||
assert_eq!(converted, 120);
|
||||
assert_eq!(
|
||||
aliased, 117,
|
||||
"binding the decode target BY SLOT is supposed to reproduce the defect on \
|
||||
this stream; if it no longer does, the exemption test above is passing for \
|
||||
a reason nobody has checked"
|
||||
);
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user