diff --git a/crates/pf-client-core/src/video_vaapi_native.rs b/crates/pf-client-core/src/video_vaapi_native.rs index cd0a725a..5e0bde00 100644 --- a/crates/pf-client-core/src/video_vaapi_native.rs +++ b/crates/pf-client-core/src/video_vaapi_native.rs @@ -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)> { + 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> = 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 = 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. /// diff --git a/crates/pf-vaadec/src/pic.rs b/crates/pf-vaadec/src/pic.rs index a66d3825..5da7957e 100644 --- a/crates/pf-vaadec/src/pic.rs +++ b/crates/pf-vaadec/src/pic.rs @@ -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 = Vec::new(); let mut slots: Option = None; - let mut collisions = 0usize; - let mut first: Option = None; - let mut inherited = 0usize; + // Slot to surface — precisely `Session::surface_table()` on the Linux rung. + let mut table: Vec = 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 = 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 = 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 = None; + let mut table: Vec = 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" ); }