From 26d06195f54d506c4804f56e219eb2415c253b52 Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Sun, 16 Aug 2026 01:14:30 +0200 Subject: [PATCH 1/3] fix(client): a host recovery anchor lifted the freeze onto a grey picture, unchecked Field report: the native-vulkan HEVC stream goes grey with moving artifacts after a host context change (starting a game), and recovers by itself in 0.5-2 s. Of the three signals that lift the post-loss freeze, two are self-evident to the client and one is pure hearsay. An IDR predicts from nothing. A recovery mark is half a re-anchor and the gate says so by requiring two. But USER_FLAG_RECOVERY_ANCHOR is the HOST asserting a fact about the CLIENT's decoder -- "the picture I coded this P-frame against is one you still hold, intact" -- and the gate took it on faith, on the first occurrence, with no scrutiny at all. The host derives that claim from bookkeeping that tracks what the client RECEIVED, not what it managed to DECODE. Those diverge exactly when the client had to conceal, and then the anchor lifts the freeze onto a picture predicted from damage AND LEAVES IT LIFTED -- so the grey plate reaches the screen and keeps reaching it until some later signal re-arms and the 500 ms backstop extracts a real IDR. That is the observed 0.5-2 s, and it is the worst-shaped failure in the module: a re-anchor claim the client can refute is worse than no claim, because no claim merely holds. So corroborate it. pf-bitstream's planners now carry a per-picture clean bit -- damage propagates down the prediction chain, because the descendants of a concealed picture raise no warning of their own -- surfaced as PicturePlan::references_clean and carried to the consumer on DecodedVkFrame. The gate gains AnchorEvidence and on_decoded_corroborated, and refuses an anchor whose references the client can prove were damaged. Refusing can only ever hold LONGER: the freeze stays up, the backstop fires on its ORIGINAL deadline, and the client escalates to the IDR the anchor failed to be. Lanes with no local parser pass Unavailable and are bit-for-bit unchanged -- all 29 pre-existing reanchor tests pass untouched. Also brings the H.264 decoder to parity with its two siblings, found on the way. It was the only one of the three that failed OPEN: a DPB slot with no bound image was traced and decoded anyway (H.265 and AV1 return UnboundReferenceSlot), reference_count was computed after the held-slot loop so a dropped reference silently took an unrelated slot's picture in its place, and there was no RecoveryLatch, so a failure left the planner DPB, the slot map and the image bindings disagreeing forever. None of it raised a warning, so the frame was shipped, presented, and cleared the demotion streak on its way past. Both latches landed with the HEVC and AV1 decoders in August; H.264 predates them and was never retro-fitted. The damage classification moves onto the warning enums so the planner's ledger and the client's concealment test cannot drift apart -- still exactly one list, now in the crate that owns the enum, with pf_vkdecode::is_integrity_warning* delegating. Every copy stays an exhaustive match with no wildcard: a future variant must stop the compiler, never default to clean. --- crates/pf-bitstream/src/av1.rs | 95 +++++- crates/pf-bitstream/src/clean.rs | 245 +++++++++++++++ crates/pf-bitstream/src/h264.rs | 192 +++++++++++- crates/pf-bitstream/src/h265.rs | 109 ++++++- crates/pf-bitstream/src/lib.rs | 1 + crates/pf-client-core/src/session.rs | 35 ++- crates/pf-client-core/src/video.rs | 45 +++ crates/pf-client-core/src/video_vk_native.rs | 19 ++ crates/pf-vkdecode/src/decoder.rs | 286 +++++++++++++++--- crates/pf-vkdecode/src/decoder_av1.rs | 1 + crates/pf-vkdecode/src/decoder_h265.rs | 132 +++++++-- crates/pf-vkdecode/src/integrity.rs | 36 +-- crates/pf-vkdecode/src/pic_h265.rs | 3 + crates/punktfunk-core/src/abi.rs | 9 + crates/punktfunk-core/src/reanchor.rs | 297 ++++++++++++++++++- include/punktfunk_core.h | 9 + 16 files changed, 1403 insertions(+), 111 deletions(-) create mode 100644 crates/pf-bitstream/src/clean.rs diff --git a/crates/pf-bitstream/src/av1.rs b/crates/pf-bitstream/src/av1.rs index 2a59d881..7dee0185 100644 --- a/crates/pf-bitstream/src/av1.rs +++ b/crates/pf-bitstream/src/av1.rs @@ -277,6 +277,19 @@ pub struct PicturePlan { /// Colour signalling, per picture and never latched — the same rule the other two /// planners follow, because a host can switch an HDR desktop to PQ/BT.2020 in band. pub colour: ColourDescription, + /// Every picture this frame predicts from was itself decoded from a fully-available + /// reference chain — so a host claim that this frame is a clean re-anchor + /// (`USER_FLAG_RECOVERY_ANCHOR`) can be corroborated rather than taken on trust. + /// `true` for a key or intra-only frame (nothing to predict from) and for any + /// frame whose whole reference chain is clean; `false` from the moment this frame + /// — or anything it descends from — needed concealment. + /// + /// On a `show_existing_frame` this describes the picture being DISPLAYED, which is + /// the only thing such a frame puts on the screen: it decodes nothing of its own. + /// + /// Purely additive observation. See [`crate::clean`] for why it propagates and why + /// every rule errs toward `false`. + pub references_clean: bool, } /// One planned access unit. @@ -329,6 +342,35 @@ pub enum PlanWarning { TruncatedAu { offset: usize }, } +impl PlanWarning { + /// Does this warning mean the PICTURE is damaged? The AV1 twin of + /// [`crate::h264::PlanWarning::is_integrity`], and + /// `pf_vkdecode::is_integrity_warning_av1` delegates here. + /// + /// Every variant AV1 has IS damage, and that is a fact about the codec rather than + /// an oversight: AV1 puts nothing in this channel resembling h265's + /// `NonZeroReorder` or h264's `Mmco5Rebase`. It has no reorder envelope to report + /// (no bumping process, no `max_num_reorder_pics`) and no MMCO to rebase — the + /// frame header states the whole reference update outright — so the only things + /// left to warn about are pictures that went missing and an OBU walk that stopped + /// early. + /// + /// `MissingShowExisting` is the one that could be argued, and it is damage: a + /// `show_existing_frame` naming an empty slot means the picture the STREAM chose + /// to display was lost upstream. Nothing is displayed for that frame, so the + /// screen keeps the previous one — exactly the "silently stale picture" state a + /// re-anchor exists to end. + /// + /// Exhaustive with no wildcard, for the reason the H.264 twin spells out. + pub fn is_integrity(&self) -> bool { + match self { + PlanWarning::MissingReference { .. } + | PlanWarning::MissingShowExisting { .. } + | PlanWarning::TruncatedAu { .. } => true, + } + } +} + /// Why an access unit cannot be planned at all. #[derive(Debug, Clone, PartialEq, Eq)] pub enum PlanError { @@ -366,6 +408,10 @@ pub struct Av1Planner { slots: [Option; NUM_REF_SLOTS], next_id: PicId, sequence: Option>, + /// Which resident pictures came off a BROKEN reference chain — the fact behind + /// [`PicturePlan::references_clean`]. Empty on a healthy stream; see + /// [`crate::clean::CleanLedger`] for the propagation rules. + clean: crate::clean::CleanLedger, } impl Default for Av1Planner { @@ -381,6 +427,7 @@ impl Av1Planner { slots: [None; NUM_REF_SLOTS], next_id: 1, sequence: None, + clean: Default::default(), } } @@ -549,7 +596,22 @@ impl Av1Planner { } else { Vec::new() }; - let picture = picture_plan(&header, &sequence); + // A `show_existing_frame` decodes nothing, so the only picture it puts on + // the screen is the one it displays: report THAT picture's cleanliness. A + // slot that held nothing already warned above and shows nothing at all, + // which is damage in its own right — reporting it unclean keeps the two + // statements consistent. + let references_clean = match shown { + Some(pic) => self.clean.references_clean([pic.id]), + None => false, + }; + let picture = picture_plan(&header, &sequence, references_clean); + // A key-frame `show_existing_frame` rewrote every slot with the shown + // picture (7.20), so the ledger has to follow that aliasing: the refreshed + // slots all hold `pic.id`, whose mark already stands. Nothing new is + // stored, so there is no verdict to fold — only residency to re-bound. + self.clean + .retain_live(self.slots.iter().flatten().map(|p| p.id)); return Ok(AuPlan { picture, tiles, @@ -597,12 +659,34 @@ impl Av1Planner { } let removed = self.refresh_slots(header.refresh_frame_flags, id, RefState::of(&header)); - let picture = picture_plan(&header, &sequence); + // Was every picture this frame predicts from decoded off an intact chain? + // Over the resolved names only: a `None` hole is a lost reference, which has + // already pushed `MissingReference` and therefore condemns this frame through + // `concealed` below. A key or intra-only frame names nothing, so this is + // vacuously true for it (`CleanLedger::references_clean`). + let references_clean = self + .clean + .references_clean(refs.iter().flatten().map(|r| r.id)); + + let picture = picture_plan(&header, &sequence, references_clean); let outputs = if header.show_frame { vec![id] } else { Vec::new() }; + // Fold this frame's verdict, then bound the ledger to slot residency. After + // `refresh_slots`, so the live set reflects the writes this frame performed. + // `concealed` mirrors what a consumer conceals on, via the ONE classification + // (`PlanWarning::is_integrity`), so the ledger and the consumer can never + // disagree about whether this frame was damaged. + self.clean.note_stored( + id, + references_clean, + warnings.iter().any(PlanWarning::is_integrity), + ); + self.clean + .retain_live(self.slots.iter().flatten().map(|p| p.id)); + Ok(AuPlan { picture, tiles, @@ -655,7 +739,11 @@ impl Av1Planner { } } -fn picture_plan(header: &FrameHeaderObu, sequence: &SequenceHeaderObu) -> PicturePlan { +fn picture_plan( + header: &FrameHeaderObu, + sequence: &SequenceHeaderObu, + references_clean: bool, +) -> PicturePlan { let color = &sequence.color_config; let bit_depth = if color.high_bitdepth { if color.twelve_bit { @@ -698,6 +786,7 @@ fn picture_plan(header: &FrameHeaderObu, sequence: &SequenceHeaderObu) -> Pictur matrix_coefficients: color.matrix_coefficients as u8, video_full_range: color.color_range, }, + references_clean, } } #[cfg(test)] diff --git a/crates/pf-bitstream/src/clean.rs b/crates/pf-bitstream/src/clean.rs new file mode 100644 index 00000000..f0ffb64b --- /dev/null +++ b/crates/pf-bitstream/src/clean.rs @@ -0,0 +1,245 @@ +//! Which decoded pictures came off a fully-available reference chain — the fact a +//! client needs to CORROBORATE a host's claim that a frame is a clean re-anchor. +//! +//! # Why this exists +//! +//! After a loss the client freezes on its last good picture and lifts only on a proven +//! re-anchor. One of those proofs — `USER_FLAG_RECOVERY_ANCHOR`, the host's LTR-RFI +//! recovery frame — lifts the freeze on its FIRST occurrence, exactly like a real IDR, +//! because the host says the frame was coded against a known-good reference. +//! +//! The host's "known-good" is an inference from what the client RECEIVED. The client's +//! own DPB is the only place that knows what it actually DECODED, and it did not +//! previously record it: a picture the planner concealed (a reference the DPB could not +//! resolve, an AU that stopped early) entered the DPB looking exactly like a clean one. +//! So an anchor naming that picture lifted the freeze onto a gray plate, and every +//! frame after it chained off the corruption — the freeze gone, nothing left to +//! re-arm it, and the picture stayed broken until an unrelated signal forced an IDR. +//! +//! This ledger is the missing fact, and it is deliberately the SMALLEST one that +//! answers the question: a set of picture ids that are NOT clean. Membership is +//! per-picture, so it costs one `u64` per damaged picture and nothing at all on a +//! healthy stream — the overwhelmingly common case, where the set stays empty for the +//! life of the session. +//! +//! # Damage propagates; that is the whole point +//! +//! A picture is unclean when the AU that produced it needed concealment, OR when +//! ANYTHING it predicted from was unclean. Without the second half the ledger would be +//! useless: the concealed picture itself is rarely the one an anchor names — it is the +//! chain of ordinary P-frames DESCENDING from it, each of which planned perfectly and +//! raised no warning of its own, that carries the corruption forward. +//! +//! # It errs toward "unclean", never toward "clean" +//! +//! Every rule here is one-way. An id the ledger has forgotten (evicted from the DPB, +//! dropped at a flush) reads as clean, which is correct — a picture no longer in the +//! DPB cannot be referenced. An id it holds stays unclean until the picture leaves the +//! DPB. There is no path that clears the mark on a picture that is still resident, so +//! the ledger can only ever make a consumer MORE conservative: hold the freeze longer +//! and take an IDR it might not have needed. The opposite mistake — reporting a damaged +//! chain as clean — is the failure this exists to end, so the asymmetry is deliberate. + +use std::collections::BTreeSet; + +/// Per-picture "this came off a broken chain" marks for one planner. +/// +/// Keyed by the planner's own `PicId` (a `u64` in all three codecs), so this type is +/// codec-agnostic and the H.264, H.265 and AV1 planners share ONE implementation rather +/// than three hand-copies that can drift apart. +#[derive(Debug, Clone, Default)] +pub struct CleanLedger { + /// Ids of resident pictures that are NOT clean. Empty on a healthy stream — the + /// set only ever gains an entry when a plan needed concealment. + unclean: BTreeSet, +} + +impl CleanLedger { + pub fn new() -> Self { + Self::default() + } + + /// Is every id in `references` clean? — i.e. may a picture predicted from exactly + /// these be trusted? + /// + /// Vacuously true for an empty list, which is what makes an IRAP/IDR clean by + /// construction: it predicts from nothing, so there is nothing to distrust. + pub fn references_clean(&self, references: I) -> bool + where + I: IntoIterator, + { + // Short-circuits on the first unclean reference, and — because the set is + // empty on a healthy stream — degenerates to one `is_empty`-cheap lookup per + // reference in the case that matters for throughput. + self.unclean.is_empty() || !references.into_iter().any(|id| self.unclean.contains(&id)) + } + + /// Record the verdict for the picture this AU stored. + /// + /// `references_clean` is what [`Self::references_clean`] answered for this AU's + /// reference lists; `concealed` is whether the AU's own plan carried an integrity + /// warning. Either one being bad makes the stored picture unclean, and its + /// descendants inherit that through their own `references_clean` call. + pub fn note_stored(&mut self, id: u64, references_clean: bool, concealed: bool) { + if references_clean && !concealed { + // The common path. Nothing is inserted, so a healthy stream never allocates + // — and `remove` still runs below because an id can be REUSED after the + // planner recycles it, and a stale mark would then condemn a fresh picture. + self.unclean.remove(&id); + } else { + self.unclean.insert(id); + } + } + + /// Drop the marks of pictures that have left the DPB. + /// + /// Called with the ids still live after each plan. Bounding the set to DPB + /// residency is what keeps it from growing without limit across a long lossy + /// session, and it is safe precisely because a picture outside the DPB can never + /// appear in a later reference list. + pub fn retain_live(&mut self, live: I) + where + I: IntoIterator, + { + if self.unclean.is_empty() { + return; + } + let live: BTreeSet = live.into_iter().collect(); + self.unclean.retain(|id| live.contains(id)); + } + + /// Forget everything — the DPB was drained (a flush, a stream discontinuity), so no + /// mark describes a resident picture any more. + pub fn clear(&mut self) { + self.unclean.clear(); + } + + /// Is this picture known to have come off a broken chain? (Diagnostics and tests; + /// the plan path uses [`Self::references_clean`].) + pub fn is_unclean(&self, id: u64) -> bool { + self.unclean.contains(&id) + } + + /// How many resident pictures are marked unclean (diagnostics and tests). + pub fn unclean_count(&self) -> usize { + self.unclean.len() + } +} + +#[cfg(test)] +mod tests { + use super::*; + + /// The headline: damage propagates down the prediction chain. The concealed + /// picture is rarely the one an anchor names — it is the ordinary P-frames + /// descending from it, each of which planned perfectly and warned about nothing. + #[test] + fn damage_propagates_to_every_descendant() { + let mut led = CleanLedger::new(); + // An IDR: no references, no concealment. + assert!(led.references_clean([])); + led.note_stored(0, true, false); + assert!(!led.is_unclean(0)); + + // A clean P off it. + assert!(led.references_clean([0])); + led.note_stored(1, true, false); + + // Picture 2's plan needed concealment. + let refs_clean = led.references_clean([1]); + assert!(refs_clean, "its reference was still fine"); + led.note_stored(2, refs_clean, true); + assert!(led.is_unclean(2)); + + // …and picture 3 predicts from it, raising NO warning of its own. + let refs_clean = led.references_clean([2]); + assert!(!refs_clean, "the chain is broken from here down"); + led.note_stored(3, refs_clean, false); + assert!(led.is_unclean(3), "3 inherited 2's damage"); + + // The rot keeps travelling, arbitrarily far from the original loss. + let refs_clean = led.references_clean([3]); + assert!(!refs_clean); + led.note_stored(4, refs_clean, false); + assert!(led.is_unclean(4)); + } + + /// A picture that references BOTH a clean and an unclean predecessor is unclean — + /// one broken reference is enough to make the reconstruction wrong. + #[test] + fn one_unclean_reference_is_enough() { + let mut led = CleanLedger::new(); + led.note_stored(0, true, false); + led.note_stored(1, true, true); // damaged + assert!(!led.references_clean([0, 1])); + assert!(!led.references_clean([1, 0]), "order does not matter"); + assert!(led.references_clean([0])); + } + + /// An IDR predicts from nothing, so it is clean however broken the stream was + /// before it. This is the property that lets a real keyframe end a damaged run. + #[test] + fn a_picture_with_no_references_is_clean_however_bad_the_stream_was() { + let mut led = CleanLedger::new(); + led.note_stored(0, true, true); + led.note_stored(1, false, false); + assert_eq!(led.unclean_count(), 2); + // The IDR: an empty reference list is vacuously clean. + assert!(led.references_clean([])); + led.note_stored(2, true, false); + assert!(!led.is_unclean(2)); + } + + /// Marks are bounded by DPB residency: a picture that left the DPB can never be + /// referenced again, so keeping its mark would only grow the set forever. + #[test] + fn marks_are_dropped_when_their_picture_leaves_the_dpb() { + let mut led = CleanLedger::new(); + led.note_stored(7, true, true); + led.note_stored(8, false, false); + assert_eq!(led.unclean_count(), 2); + led.retain_live([8, 9]); + assert!(!led.is_unclean(7), "7 was evicted"); + assert!(led.is_unclean(8), "8 is still resident and still damaged"); + assert_eq!(led.unclean_count(), 1); + } + + /// A flush drains the whole DPB, so no mark describes anything resident. + #[test] + fn a_flush_forgets_every_mark() { + let mut led = CleanLedger::new(); + led.note_stored(1, true, true); + led.note_stored(2, false, false); + led.clear(); + assert_eq!(led.unclean_count(), 0); + assert!(led.references_clean([1, 2])); + } + + /// Planners hand out ids from a counter the flush path can rewind, so an id CAN be + /// reused. A stale mark must not condemn the fresh picture that inherits the id. + #[test] + fn a_reused_id_is_not_condemned_by_its_predecessors_mark() { + let mut led = CleanLedger::new(); + led.note_stored(5, true, true); + assert!(led.is_unclean(5)); + // The same id, planned cleanly this time. + led.note_stored(5, true, false); + assert!(!led.is_unclean(5)); + assert!(led.references_clean([5])); + } + + /// A healthy stream never marks anything, forever — the property that makes this + /// free to carry on every session that is working correctly. + #[test] + fn a_stream_without_loss_never_marks_a_picture() { + let mut led = CleanLedger::new(); + for id in 0..512u64 { + let refs = if id == 0 { vec![] } else { vec![id - 1] }; + let clean = led.references_clean(refs.iter().copied()); + assert!(clean, "picture {id} must read clean"); + led.note_stored(id, clean, false); + led.retain_live(id.saturating_sub(3)..=id); + } + assert_eq!(led.unclean_count(), 0); + } +} diff --git a/crates/pf-bitstream/src/h264.rs b/crates/pf-bitstream/src/h264.rs index 146555c9..646865de 100644 --- a/crates/pf-bitstream/src/h264.rs +++ b/crates/pf-bitstream/src/h264.rs @@ -149,6 +149,18 @@ pub struct PicturePlan { /// DPB size in frames per A.3.1 — backends size their slot pool from this. pub max_dpb_frames: usize, pub recovery_point: Option, + /// Every picture this AU predicts from was itself decoded from a fully-available + /// reference chain — so a host claim that this AU is a clean re-anchor + /// (`USER_FLAG_RECOVERY_ANCHOR`) can be corroborated rather than taken on trust. + /// `true` for an IDR (nothing to predict from) and for any picture whose whole + /// reference chain is clean; `false` from the moment this AU — or anything it + /// descends from — needed concealment. + /// + /// Purely additive observation: nothing in the plan, the warnings or the DPB + /// changes because of it, and on a stream that never loses a reference it is + /// `true` on every picture forever. See [`crate::clean`] for why it propagates and + /// why every rule errs toward `false`. + pub references_clean: bool, } /// The region of the coded picture that is actually displayed. @@ -252,6 +264,41 @@ pub enum PlanWarning { }, } +impl PlanWarning { + /// Does this warning mean the PICTURE is damaged — the plan was completed with a + /// SUBSTITUTE in place of something that was lost — rather than reporting a + /// spec-legal fact about the stream's envelope? + /// + /// The distinction decides two things that must never disagree: whether a consumer + /// releases the AU's output unshown and asks for a re-anchor, and whether the + /// picture enters [`crate::clean::CleanLedger`] as unclean. It lives HERE, on the + /// enum, because those two consumers sit in different crates and a second copy of + /// the list would let one of them conceal damage the other reports — the exact + /// shape of the invisible-corruption failure the native-decode program exists to + /// end. `pf_vkdecode::is_integrity_warning` delegates to this. + /// + /// `Mmco5Rebase` is not damage: the AU carried an MMCO 5 and this planner planned + /// it in full (the plan holds the pre-rebase 8.2.1 values; later AUs reference the + /// rebased ones). `LevelDerivedDpb` is not either: the picture is intact and fully + /// planned — it reports that the SPS never declared its DPB depth, so the plan had + /// to size from A.3.1's level ceiling, a property of the STREAM's signalling which + /// a backend answers by failing to open a session, not by showing a damaged frame. + /// + /// Written as an EXHAUSTIVE match with no wildcard, deliberately. A `matches!` (or + /// a `_ => false`) makes "damage" the opt-in and silence the default, so a variant + /// added later — by definition one nobody here has classified — would be reported + /// as clean and its picture shown. The compiler is the only reviewer guaranteed to + /// be present when that variant is written, so it gets the decision. + pub fn is_integrity(&self) -> bool { + match self { + PlanWarning::FrameNumGap { .. } + | PlanWarning::MissingReference { .. } + | PlanWarning::TruncatedAu { .. } => true, + PlanWarning::Mmco5Rebase | PlanWarning::LevelDerivedDpb { .. } => false, + } + } +} + /// The AU cannot be planned at all. #[derive(Debug, Clone, PartialEq, Eq)] pub enum PlanError { @@ -482,6 +529,10 @@ pub struct H264Planner { reported_live: BTreeSet, /// Set by [`Self::flush`]: planning resumes only at an IDR (upstream: `Reset`). awaiting_idr: bool, + /// Which resident pictures came off a BROKEN reference chain — the fact behind + /// [`PicturePlan::references_clean`]. Empty on a healthy stream; see + /// [`crate::clean::CleanLedger`] for the propagation rules. + clean: crate::clean::CleanLedger, } impl H264Planner { @@ -600,9 +651,20 @@ impl H264Planner { let cur = current .ok_or_else(|| PlanError::Parse("access unit contains no coded picture".into()))?; + // Was every picture this AU predicts from decoded off an intact chain? Asked + // over the SLICE reference lists rather than the DPB snapshot, because those + // are what this picture actually predicts from — a resident-but-unreferenced + // damaged picture says nothing about this one. An IDR references nothing, so + // this is vacuously true for it (`CleanLedger::references_clean`). + let references_clean = self.clean.references_clean( + slices + .iter() + .flat_map(|s: &SlicePlan| s.ref_list0.iter().chain(&s.ref_list1)) + .map(|r| r.id), + ); // Captured before finish_picture: MMCO5 rewrites the stored POC afterwards, but // backends submit the picture with its 8.2.1 values. - let picture = Self::picture_plan(&cur, recovery_point); + let picture = Self::picture_plan(&cur, recovery_point, references_clean); // The activated parameter sets ride out with the plan (AuPlan field docs); // cloned before finish_picture consumes `cur`. let pps = Rc::clone(&cur.first_slice_pps); @@ -619,6 +681,20 @@ impl H264Planner { let removed = previously_live.difference(&live_after).copied().collect(); self.reported_live = live_after; + // Fold this picture's verdict, then bound the ledger to DPB residency. Both + // AFTER `finish_picture`, so `stored` is the id the picture really got and + // `live_after` reflects the marking this AU performed — a mark written against + // a pre-marking view could survive an eviction it should have died with. + // `concealed` mirrors what a consumer conceals on, via the ONE classification + // (`PlanWarning::is_integrity`), so the ledger and the consumer can never + // disagree about whether this AU was damaged. + self.clean.note_stored( + stored, + references_clean, + warnings.iter().any(PlanWarning::is_integrity), + ); + self.clean.retain_live(self.reported_live.iter().copied()); + Ok(AuPlan { picture, slices, @@ -650,6 +726,9 @@ impl H264Planner { self.max_long_term_frame_idx = Default::default(); self.negotiation_info = Default::default(); self.awaiting_idr = true; + // The DPB is drained, so no mark describes a resident picture any more — and + // planning resumes at an IDR, which is clean by construction. + self.clean.clear(); DpbUpdate { stored: None, @@ -1747,7 +1826,11 @@ impl H264Planner { Ok(id) } - fn picture_plan(cur: &CurrentPicState, recovery_point: Option) -> PicturePlan { + fn picture_plan( + cur: &CurrentPicState, + recovery_point: Option, + references_clean: bool, + ) -> PicturePlan { let pic = &cur.pic; // The first slice's PPS defines the picture's parameters (upstream's // start_picture semantics); `cur.pps` may have drifted to a later slice's. @@ -1791,6 +1874,7 @@ impl H264Planner { chroma_format_idc: sps.chroma_format_idc, max_dpb_frames: dpb_limit(sps), recovery_point, + references_clean, } } } @@ -2343,6 +2427,110 @@ mod tests { assert!(missing_seen); } + /// The clean bit, end to end through the real planner: a `frame_num` gap + /// concealed one picture, and EVERY picture descending from it reports + /// `references_clean == false` even though their own plans are spotless. That + /// propagation is the whole point — the concealed picture is rarely the one a host + /// recovery anchor names; the ordinary P-frames after it are. + #[test] + fn a_concealed_picture_makes_every_descendant_report_unclean_references() { + let (sps, pps) = authored_sps_pps(); + let mut au0 = param_set_au(&sps, &pps); + au0.extend(write_idr_slice()); + + let mut planner = H264Planner::new(); + let p0 = planner.plan_au(&au0).unwrap(); + assert!( + p0.picture.references_clean, + "an IDR references nothing, so it is clean by construction" + ); + + // A healthy P off the IDR: still clean. + let p1 = planner.plan_au(&write_p_slice(1, 2, 1, 1, None)).unwrap(); + assert!(picture_warnings(&p1).is_empty()); + assert!(p1.picture.references_clean); + + // frame_num 2 never arrives — 8.2.5.2 fabricates a placeholder and the plan + // conceals. THIS picture's references were still intact; the damage is its own. + let p3 = planner.plan_au(&write_p_slice(3, 6, 1, 3, None)).unwrap(); + assert!(p3 + .warnings + .iter() + .any(|w| matches!(w, PlanWarning::FrameNumGap { .. }))); + + // …and every picture after it inherits the damage with a clean plan of its own. + let p4 = planner.plan_au(&write_p_slice(4, 8, 1, 1, None)).unwrap(); + assert!( + picture_warnings(&p4).is_empty(), + "p4's own plan raises nothing — which is exactly why the bit is needed" + ); + assert!( + !p4.picture.references_clean, + "p4 predicts from the concealed chain, so it must not read as clean" + ); + + let p5 = planner.plan_au(&write_p_slice(5, 10, 1, 1, None)).unwrap(); + assert!(picture_warnings(&p5).is_empty()); + assert!(!p5.picture.references_clean, "the rot keeps travelling"); + } + + /// An IDR ends a damaged run: it predicts from nothing, so it reads clean however + /// broken the stream was before it. Without this a session could never recover a + /// trustworthy anchor. + #[test] + fn an_idr_reports_clean_references_however_damaged_the_run_before_it() { + let (sps, pps) = authored_sps_pps(); + let mut au0 = param_set_au(&sps, &pps); + au0.extend(write_idr_slice()); + + let mut planner = H264Planner::new(); + planner.plan_au(&au0).unwrap(); + planner.plan_au(&write_p_slice(1, 2, 1, 1, None)).unwrap(); + // Gap: frame_num 2 lost. + let p3 = planner.plan_au(&write_p_slice(3, 6, 1, 3, None)).unwrap(); + assert!(p3 + .warnings + .iter() + .any(|w| matches!(w, PlanWarning::FrameNumGap { .. }))); + let p4 = planner.plan_au(&write_p_slice(4, 8, 1, 1, None)).unwrap(); + assert!(!p4.picture.references_clean); + + // A fresh IDR re-anchors, and the pictures after it are clean again. + let mut idr = param_set_au(&sps, &pps); + idr.extend(write_idr_slice()); + let p5 = planner.plan_au(&idr).unwrap(); + assert!(p5.picture.references_clean, "an IDR is always clean"); + let p6 = planner.plan_au(&write_p_slice(1, 2, 1, 1, None)).unwrap(); + assert!( + p6.picture.references_clean, + "the damaged chain died with the IDR's DPB flush" + ); + } + + /// A stream that never loses a reference reports `references_clean` on every + /// picture, forever — the property that makes this free to carry in production. + #[test] + fn a_healthy_stream_reports_clean_references_on_every_picture() { + let (sps, pps) = authored_sps_pps(); + let mut au0 = param_set_au(&sps, &pps); + au0.extend(write_idr_slice()); + + let mut planner = H264Planner::new(); + assert!(planner.plan_au(&au0).unwrap().picture.references_clean); + // log2_max_frame_num_minus4 = 0 and pic_order_cnt_lsb is u(4): both wrap at 16. + for n in 1..16u32 { + let plan = planner + .plan_au(&write_p_slice(n, (n * 2) % 16, 1, 1, None)) + .unwrap(); + assert!( + picture_warnings(&plan).is_empty(), + "frame {n} should plan cleanly: {:?}", + picture_warnings(&plan) + ); + assert!(plan.picture.references_clean, "frame {n} must read clean"); + } + } + #[test] fn a_gap_placeholder_inside_a_ref_list_is_substituted_in_place_not_compacted() { let (sps, pps) = authored_sps_pps(); diff --git a/crates/pf-bitstream/src/h265.rs b/crates/pf-bitstream/src/h265.rs index 49774d13..68da10ca 100644 --- a/crates/pf-bitstream/src/h265.rs +++ b/crates/pf-bitstream/src/h265.rs @@ -166,6 +166,18 @@ pub struct PicturePlan { /// came from the SPS by index) — Vulkan's `NumBitsForSTRefPicSetInSlice`. pub short_term_ref_pic_set_size_bits: u32, pub recovery_point: Option, + /// Every picture this AU predicts from was itself decoded from a fully-available + /// reference chain — so a host claim that this AU is a clean re-anchor + /// (`USER_FLAG_RECOVERY_ANCHOR`) can be corroborated rather than taken on trust. + /// `true` for an IRAP (nothing to predict from) and for any picture whose whole + /// reference chain is clean; `false` from the moment this AU — or anything it + /// descends from — needed concealment. + /// + /// Purely additive observation: nothing in the plan, the warnings or the DPB + /// changes because of it, and on a stream that never loses a reference it is + /// `true` on every picture forever. See [`crate::clean`] for why it propagates and + /// why every rule errs toward `false`. + pub references_clean: bool, } /// A reference list / RPS entry: the minimum every backend picparams format needs. @@ -232,6 +244,28 @@ pub enum PlanWarning { NonZeroReorder { max_num_reorder_pics: u8 }, } +impl PlanWarning { + /// Does this warning mean the PICTURE is damaged? The H.265 twin of + /// [`crate::h264::PlanWarning::is_integrity`] — the same one-list argument applies, + /// and `pf_vkdecode::is_integrity_warning_h265` delegates here. + /// + /// `NonZeroReorder` is NOT damage, and excluding it matters more here than the + /// H.264 exclusions do: it fires on the AU that ACTIVATES an SPS — the opening + /// IRAP, and the fresh IRAP at every ABR resolution change — so treating it as + /// concealment would cost a released-unshown frame plus a keyframe round trip at + /// every renegotiation, on a stream the planner says it planned correctly. It + /// would also poison the [`crate::clean::CleanLedger`] at exactly those IRAPs, + /// marking the one picture that is clean by construction as broken. + /// + /// Exhaustive with no wildcard, for the reason the H.264 twin spells out. + pub fn is_integrity(&self) -> bool { + match self { + PlanWarning::MissingReference { .. } | PlanWarning::TruncatedAu { .. } => true, + PlanWarning::NonZeroReorder { .. } => false, + } + } +} + /// The AU cannot be planned at all. #[derive(Debug, Clone, PartialEq, Eq)] pub enum PlanError { @@ -452,6 +486,10 @@ pub struct H265Planner { reported_live: BTreeSet, /// Set by [`Self::flush`]: planning resumes only at an IRAP (upstream: `Reset`). awaiting_idr: bool, + /// Which resident pictures came off a BROKEN reference chain — the fact behind + /// [`PicturePlan::references_clean`]. Empty on a healthy stream; see + /// [`crate::clean::CleanLedger`] for the propagation rules. + clean: crate::clean::CleanLedger, } impl Default for H265Planner { @@ -471,6 +509,7 @@ impl Default for H265Planner { pending_outputs: Vec::new(), reported_live: BTreeSet::new(), awaiting_idr: false, + clean: Default::default(), } } } @@ -690,7 +729,20 @@ impl H265Planner { let cur = current .ok_or_else(|| PlanError::Parse("access unit contains no coded picture".into()))?; - let picture = Self::picture_plan(&cur, recovery_point); + // Was every picture this AU predicts from decoded off an intact chain? Asked + // over the SLICE reference lists rather than the RPS or the DPB snapshot, + // because those are what this picture actually predicts from: 8.3.2 RETAINS + // pictures in the RPS that the current picture does not use + // (`used_by_curr_pic` clear), and a damaged one among those says nothing about + // this picture. An IRAP's lists are empty, so this is vacuously true for it + // (`CleanLedger::references_clean`). + let references_clean = self.clean.references_clean( + slices + .iter() + .flat_map(|s: &SlicePlan| s.ref_list0.iter().chain(&s.ref_list1)) + .map(|r| r.id), + ); + let picture = Self::picture_plan(&cur, recovery_point, references_clean); let rps = cur.rps_plan.clone(); let dpb_refs = cur.dpb_refs.clone(); // The activated parameter sets ride out with the plan (AuPlan field docs); @@ -708,6 +760,20 @@ impl H265Planner { let removed = previously_live.difference(&live_after).copied().collect(); self.reported_live = live_after; + // Fold this picture's verdict, then bound the ledger to DPB residency. Both + // AFTER `finish_picture`, so `stored` is the id the picture really got and + // `live_after` reflects the C.3.4/8.3.2 marking this AU performed — a mark + // written against a pre-marking view could survive an eviction it should have + // died with. `concealed` mirrors what a consumer conceals on, via the ONE + // classification (`PlanWarning::is_integrity`), so the ledger and the consumer + // can never disagree about whether this AU was damaged. + self.clean.note_stored( + stored, + references_clean, + warnings.iter().any(PlanWarning::is_integrity), + ); + self.clean.retain_live(self.reported_live.iter().copied()); + Ok(AuPlan { picture, rps, @@ -745,6 +811,9 @@ impl H265Planner { // re-entry sound. self.first_picture_after_eos = true; self.awaiting_idr = true; + // The DPB is drained, so no mark describes a resident picture any more — and + // planning resumes at an IRAP, which is clean by construction. + self.clean.clear(); DpbUpdate { stored: None, @@ -1452,6 +1521,7 @@ impl H265Planner { fn picture_plan( cur: &CurrentPicState, recovery_point: Option, + references_clean: bool, ) -> PicturePlan { let pic = &cur.pic; // The first slice's PPS defines the picture's parameters; `cur.pps` may have @@ -1498,6 +1568,7 @@ impl H265Planner { max_dpb_frames: dpb_limit(sps), short_term_ref_pic_set_size_bits: pic.short_term_ref_pic_set_size_bits, recovery_point, + references_clean, } } } @@ -1550,11 +1621,15 @@ mod tests { /// `NonZeroReorder` is excluded: the vendored conformance clips are general /// (reordering) encodes, and the planner deliberately plans them while flagging /// the envelope fact. + /// + /// Delegates rather than restating the list. This harness exists to prove the + /// planner conceals exactly where production conceals, so a second copy here + /// could drift and quietly prove the wrong thing — and a `matches!` in + /// particular reads any FUTURE variant as clean, which is the one answer a + /// damage predicate must never default to. [`PlanWarning::is_integrity`] is an + /// exhaustive match, so a new variant stops the compiler there instead. fn is_integrity_warning(w: &PlanWarning) -> bool { - matches!( - w, - PlanWarning::MissingReference { .. } | PlanWarning::TruncatedAu { .. } - ) + w.is_integrity() } /// Plan a whole vendored clip and assert the global invariants: every AU plans, @@ -1658,6 +1733,30 @@ mod tests { assert!(!bbb.is_empty()); } + /// The false-positive guard for [`PicturePlan::references_clean`], on REAL + /// bitstreams rather than authored ones: two conformance clips that lose nothing + /// must report every single picture clean. A regression that let the ledger mark a + /// healthy stream would refuse every host recovery anchor and force an IDR on + /// every loss — the cheap re-anchor path gone, silently. + /// + /// These clips carry B-slices and real reordering, so they also exercise the + /// "reference lists, not the RPS" reading: 8.3.2 retains pictures the current + /// picture does not use, and folding those in would condemn pictures at random. + #[test] + fn a_lossless_conformance_clip_reports_clean_references_on_every_picture() { + for (name, clip) in [("bear", TEST_BEAR), ("bbb", TEST_BBB)] { + let (_, plans) = plan_whole_clip(clip); + assert!(!plans.is_empty(), "{name} produced no plans"); + for (i, plan) in plans.iter().enumerate() { + assert!( + plan.picture.references_clean, + "{name} picture {i} (poc {}) must read clean on a lossless clip", + plan.picture.pic_order_cnt + ); + } + } + } + #[test] fn b_slices_get_a_future_led_list1_distinct_from_list0() { let aus = split_into_aus(TEST_64X64_I_P_B_P); diff --git a/crates/pf-bitstream/src/lib.rs b/crates/pf-bitstream/src/lib.rs index ee6936c3..12f37b52 100644 --- a/crates/pf-bitstream/src/lib.rs +++ b/crates/pf-bitstream/src/lib.rs @@ -21,6 +21,7 @@ #![forbid(unsafe_code)] pub mod av1; +pub mod clean; pub mod h264; pub mod h265; pub mod sei; diff --git a/crates/pf-client-core/src/session.rs b/crates/pf-client-core/src/session.rs index 309b2226..d527c1e5 100644 --- a/crates/pf-client-core/src/session.rs +++ b/crates/pf-client-core/src/session.rs @@ -1093,9 +1093,38 @@ fn pump( // `image.is_keyframe()` as the decoder's own IDR belt, applies the two-mark // rule + the mark-patience backstop, clears the no-output streak, and returns // whether to present this frame or withhold it as a post-loss concealment. - let present = - gate.on_decoded(frame.flags, image.is_keyframe(), Instant::now()) - == GateVerdict::Present; + // + // CORROBORATED (the grey-frame fix): the wire's RECOVERY_ANCHOR is the host + // asserting something about THIS decoder — "the picture I coded this + // P-frame against is one you still hold, intact" — and it lifts the freeze + // on the FIRST occurrence, no two-mark wait. The host derives that from + // bookkeeping that tracks what the client RECEIVED, not what it managed to + // DECODE, and when those diverge the anchor lifts the freeze onto a + // concealed picture and LEAVES it lifted: grey with motion painted on it + // until some later signal re-arms and the 500 ms backstop extracts a real + // IDR. A rung that planned the AU itself knows better, so it says so here. + // + // What a refusal costs is exactly one thing: the freeze keeps holding the + // last good picture until the backstop fires on its ORIGINAL deadline and + // forces the IDR the anchor failed to be. That is strictly the better half + // of the trade — the alternative is presenting a picture this client can + // prove is damaged — and it is the same direction every rule in the gate + // errs in. Every non-native lane reports `Unavailable` and is untouched. + let evidence = image.anchor_evidence(); + if evidence == punktfunk_core::reanchor::AnchorEvidence::ReferencesDamaged + && frame.flags & punktfunk_core::packet::USER_FLAG_RECOVERY_ANCHOR != 0 + { + tracing::debug!( + "refused a host recovery anchor: this AU predicts from a picture \ + this decoder had to conceal — holding for a real IDR" + ); + } + let present = gate.on_decoded_corroborated( + frame.flags, + image.is_keyframe(), + evidence, + Instant::now(), + ) == GateVerdict::Present; total_frames += 1; // ⚠ The `stats:` decode-path tag is a machine interface — // additive only. M10 removed the rungs whose tags were `vaapi`, diff --git a/crates/pf-client-core/src/video.rs b/crates/pf-client-core/src/video.rs index b95dc96b..9a4016b2 100644 --- a/crates/pf-client-core/src/video.rs +++ b/crates/pf-client-core/src/video.rs @@ -493,6 +493,17 @@ pub struct NativeVkFrame { /// the host, and it cannot be lost separately from the picture. Fed to /// [`ReanchorGate::on_local_recovery`](punktfunk_core::reanchor::ReanchorGate::on_local_recovery). pub recovery: punktfunk_core::reanchor::LocalRecovery, + /// Every picture this AU predicts from was itself decoded from a fully-available + /// reference chain (pf-vkdecode's `DecodedVkFrame::references_clean`). + /// + /// The corroboration for the host's `USER_FLAG_RECOVERY_ANCHOR`, which is a claim + /// about THIS decoder that only this decoder can check. The host derives its + /// anchor from slot bookkeeping that tracks what the client RECEIVED; this tracks + /// what the client managed to DECODE. When they disagree the anchor lifts the + /// post-loss freeze onto a concealed picture and leaves it lifted, which is the + /// grey-with-motion field report. `true` on every ordinary frame of a healthy + /// stream, so the flag is only ever load-bearing on the AU that carries an anchor. + pub references_clean: bool, /// This picture's position in DECODE order (pf-vkdecode's strictly increasing /// per-session ordinal). Delivery order is not decode order: after a failed AU /// the H.265 decoder flushes its DPB, handing back every buffered picture at @@ -559,6 +570,40 @@ impl DecodedImage { } } + /// What this lane can say about the host's re-anchor claim on this frame — the + /// corroboration for `USER_FLAG_RECOVERY_ANCHOR`, fed to + /// [`ReanchorGate::on_decoded_corroborated`](punktfunk_core::reanchor::ReanchorGate::on_decoded_corroborated). + /// + /// An anchor is the host asserting a fact about THIS decoder — *the picture I + /// coded this P-frame against is one you still hold, intact* — and the gate lifts + /// its post-loss freeze on the first one, no two-mark wait. Only a rung that + /// planned the AU itself knows which pictures it predicts from and whether each of + /// those decoded cleanly, so only such a rung can catch the host being wrong. + /// + /// The native Vulkan rung answers; everyone else reports + /// [`AnchorEvidence::Unavailable`](punktfunk_core::reanchor::AnchorEvidence::Unavailable) + /// and the gate treats them exactly as it did before this existed — silence is not + /// refutation, so no lane becomes stricter by accident. + /// + /// ⚠ The CPU rung's H.264 leg plans every AU with the same `H264Planner` and so + /// COULD answer; it does not yet, because its frame type carries no equivalent of + /// [`NativeVkFrame::references_clean`]. Reporting `Unavailable` there is the + /// conservative reading (today's behaviour), not a claim that its references are + /// fine. + pub fn anchor_evidence(&self) -> punktfunk_core::reanchor::AnchorEvidence { + use punktfunk_core::reanchor::AnchorEvidence; + match self { + DecodedImage::NativeVk(f) => { + if f.references_clean { + AnchorEvidence::ReferencesClean + } else { + AnchorEvidence::ReferencesDamaged + } + } + _ => AnchorEvidence::Unavailable, + } + } + /// This frame's position in DECODE order, where the lane knows one — see /// [`NativeVkFrame::decode_order`]. `None` everywhere else, which is what the /// pump reads as "this lane reports no local recovery either, so there is diff --git a/crates/pf-client-core/src/video_vk_native.rs b/crates/pf-client-core/src/video_vk_native.rs index d6a5598d..5ab54bce 100644 --- a/crates/pf-client-core/src/video_vk_native.rs +++ b/crates/pf-client-core/src/video_vk_native.rs @@ -741,6 +741,12 @@ fn project_frame(frame: &DecodedVkFrame, guard: NativeReleaseGuard) -> NativeVkF sei_here: frame.recovery.sei_here, is_recovery_point: frame.recovery.is_recovery_point, }, + // Whether this picture's own references decoded cleanly — the corroboration + // the shared gate weighs a host `USER_FLAG_RECOVERY_ANCHOR` against. The + // planner already knows it (it is the one party that resolved this AU's + // reference lists), and it is the only thing that can catch the host + // asserting a re-anchor over a picture THIS decoder had to conceal. + references_clean: frame.references_clean, // Which side of a loss this picture was DECODED on. Carried beside the // recovery mark because the mark is worthless without it: a post-failure // DPB flush delivers pre-loss pictures after the loss, and their marks @@ -1690,6 +1696,11 @@ mod tests { sei_here: true, is_recovery_point: true, }, + // SET, per this fixture's no-boolean-is-false rule — and it earns the + // rule: a projection that dropped this would default it to `false`, + // which reads as "this picture's references were concealed" and would + // make the gate refuse EVERY host recovery anchor on a healthy stream. + references_clean: true, // Distinct from every other number here for the same reason: a // projection that dropped the decode ordinal would make every frame // look pre-loss (0) and silently disable the local-recovery path. @@ -1792,6 +1803,7 @@ mod tests { keyframe, poc, recovery, + references_clean, decode_order, guard: _, } = p; @@ -1838,6 +1850,12 @@ mod tests { "the recovery point SEI's verdict reaches the gate — it is the ONLY \ clean point an intra-refresh session has" ); + assert!( + references_clean, + "the reference-cleanliness verdict rides along — without it the gate \ + cannot refute a host recovery anchor that names a picture this decoder \ + had to conceal, which is the grey-with-motion field report" + ); assert_eq!( decode_order, 17, "the decode ordinal rides along — without it the pump cannot tell a \ @@ -2150,6 +2168,7 @@ mod tests { keyframe: true, poc: 0, recovery: punktfunk_core::reanchor::LocalRecovery::NONE, + references_clean: true, decode_order: 1, guard: NativeReleaseGuard::new( tx, diff --git a/crates/pf-vkdecode/src/decoder.rs b/crates/pf-vkdecode/src/decoder.rs index bf0854b8..462947ae 100644 --- a/crates/pf-vkdecode/src/decoder.rs +++ b/crates/pf-vkdecode/src/decoder.rs @@ -185,6 +185,24 @@ pub struct DecodedVkFrame { /// the loss, and lift a freeze on a wave that completed before it. Comparing /// this ordinal against the one current at the arm is what tells them apart. pub decode_order: u64, + /// Every picture this one was predicted from came off a fully-available reference + /// chain ([`pf_bitstream::h264::PicturePlan::references_clean`] and its two twins). + /// `true` for an IDR/IRAP/key frame and for any picture whose whole chain is clean; + /// `false` from the moment this AU — or anything it descends from — needed + /// concealment. + /// + /// It exists to let a consumer CORROBORATE a host's claim that a frame is a clean + /// re-anchor. `USER_FLAG_RECOVERY_ANCHOR` — the host's LTR-RFI recovery frame — + /// lifts a post-loss freeze on its FIRST occurrence, exactly like a real IDR, + /// because the host says the frame was coded against a known-good reference. The + /// host's "known-good" is an inference from what the client RECEIVED; this is what + /// the client actually DECODED. Where they disagree the freeze used to lift onto a + /// gray plate and every frame after it chained off the corruption, with nothing + /// left to re-arm the gate. + /// + /// A consumer with no such claim to check can ignore it: it is an observation + /// about the stream, and nothing in this crate's own behaviour reads it. + pub references_clean: bool, /// The decode op's slot in the status query pool. pub query_slot: u32, /// The decode op's submission ordinal (validates the query slot has not been @@ -281,12 +299,24 @@ pub enum VkDecodeError { /// correct consumer can never hit this). The AU was planned but NOT decoded; /// release frames and request a keyframe. NoFreeSlot, - /// A DPB slot this AU references holds no bound image. H.265 only, and fatal - /// rather than skippable: `StdVideoDecodeH265PictureInfo`'s RPS arrays are - /// INDICES into `pReferenceSlots`, so dropping one entry would silently - /// re-point every later index at the wrong picture — the exact class of - /// plausible-looking corruption this crate refuses to produce. (H.264 carries - /// no such index arrays and only traces the case.) + /// A DPB slot this AU references holds no bound image. Fatal on all three codecs + /// rather than skippable. + /// + /// H.265 and AV1 have a structural argument: their picture info names DPB slots by + /// INDEX (the H.265 RPS arrays, AV1's name-indexed `refs`), so dropping one entry + /// silently re-points a later index at the wrong picture. H.264 has no such index + /// arrays, and used to skip the case with a `trace!` on exactly that reasoning — + /// but the reasoning was about the STRUCTURE, not the output. The hardware still + /// decodes a P-picture against a reference that was never bound, and on the + /// DPB-and-output-COINCIDE path that is a gray plate with the new frame's motion + /// painted over it. Nothing warned: the planner's DPB genuinely holds the picture + /// (the breakage is this crate's slot→image ledger), so the frame was shipped, + /// presented, and cleared the consumer's demotion streak on the way past — + /// invisible damage, which is the one outcome this crate exists to make impossible. + /// + /// Failing closed is only safe because it is paired with recovery: the latch + /// ([`crate::decoder_h265::RecoveryLatch`]) flushes to the next IRAP/IDR rather + /// than leaving the stream wedged on a slot nothing can honour. UnboundReferenceSlot { slot: u8 }, /// The frame belongs to a generation whose retired pool is already gone /// (double release, or a frame outliving its graveyard entry). @@ -615,6 +645,11 @@ pub(crate) struct PendingPic { pub(crate) recovery: crate::recovery::RecoveryMark, /// See [`DecodedVkFrame::decode_order`]. pub(crate) decode_order: u64, + /// Read off the plan at DECODE time and carried here for the same reason + /// [`Self::recovery`] is: display order is not decode order, and this describes + /// the picture rather than the moment it is delivered. See + /// [`DecodedVkFrame::references_clean`]. + pub(crate) references_clean: bool, } /// A retired generation's picture pool: images the presenter still holds live @@ -678,7 +713,15 @@ pub struct VkH264Decoder { /// The outstanding recovery point SEI, if any — see [`crate::recovery`]. /// Survives session rebuilds on purpose: it is a fact about the STREAM's /// prediction structure, not about this decoder's Vulkan objects. + /// + /// Named apart from [`Self::recovery`], which is this decoder's DPB-recovery + /// latch: the two are unrelated (one is a fact about the stream's prediction + /// structure, the other about this decoder's own wedged state). recovery_watch: crate::recovery::RecoveryWatch, + /// Post-failure DPB recovery owed — see + /// [`crate::decoder_h265::RecoveryLatch`], whose docs carry the whole + /// fail-closed/recover argument for all three codecs. + recovery: crate::decoder_h265::RecoveryLatch, /// Pictures planned so far — stamped onto each one as /// [`DecodedVkFrame::decode_order`]. Survives session rebuilds for the same /// reason the watch does. @@ -724,6 +767,7 @@ impl VkH264Decoder { graveyard: Vec::new(), last_warnings: Vec::new(), recovery_watch: crate::recovery::RecoveryWatch::new(), + recovery: Default::default(), decoded: 0, generation: 0, device_lost: false, @@ -748,6 +792,12 @@ impl VkH264Decoder { } fn decode_inner(&mut self, au: &[u8]) -> Result, VkDecodeError> { + // A previous AU failed after its planning had advanced: clear the stale + // DPB residency BEFORE planning this one, or every AU referencing the + // stranded picture fails forever ([`RecoveryLatch`] docs). + if self.recovery.take() { + self.recover_dpb(); + } // `take_warnings` promises "cleared by the next decode", and this IS a // decode: clear BEFORE planning, so an AU that fails to plan at all cannot // leave the previous AU's warnings behind to be re-read as damage on the @@ -784,7 +834,38 @@ impl VkH264Decoder { ); } - self.ensure_state(&plan)?; + // From here the PLANNER has already advanced past this AU — its DPB holds + // the picture whatever happens next — so any failure below leaves the + // planner's DPB and this decoder's slot/image ledgers able to disagree. + // Latch the recovery for the next decode rather than returning into a + // permanently wedged state. (Deliberately wider than the paths that mutate + // the SlotMap: a failure BEFORE `plan_to_vk` mutates it — an `ensure_state` + // refusal, a `NoFreeSlot` — strands the picture the other way round, + // planner-resident with no slot at all, and wedges just as hard. One flush + // cures both.) The H.265 twin, for the same reason: `decoder_h265`'s + // `RecoveryLatch` docs carry the whole argument. + let result = self.decode_planned(&plan, au, recovery, decode_order); + if result.is_err() { + self.recovery.latch(); + } + result + } + + /// The submission half of one decode, from the point the planner has already + /// advanced. Split out so [`Self::decode_inner`] can latch recovery on ANY + /// failure past that line without threading a flag through every exit. + /// `au` is the same buffer `plan`'s slice ranges index into; `recovery` is the + /// recovery-point verdict already folded for this AU and `decode_order` its + /// decode-order ordinal (both advance in decode order, so neither can be + /// derived here — this path is not reached for every planned AU). + fn decode_planned( + &mut self, + plan: &AuPlan, + au: &[u8], + recovery: crate::recovery::RecoveryMark, + decode_order: u64, + ) -> Result, VkDecodeError> { + self.ensure_state(plan)?; let sps_id = plan.sps.seq_parameter_set_id; // Convert, with ONE rebuild retry on CapacityMismatch — the designed @@ -810,7 +891,7 @@ impl VkH264Decoder { // satisfies ensure_parameters' Recreate contract, and Current/Add // touch nothing a submitted decode reads. unsafe { state.session.ensure_parameters(&plan.sps, &plan.pps)? }; - match plan_to_vk(&plan, &mut state.slots, sps_id) { + match plan_to_vk(plan, &mut state.slots, sps_id) { Ok(converted) => { vk_plan = Some(converted); break; @@ -820,7 +901,7 @@ impl VkH264Decoder { required, capacity, "DPB depth renegotiated — rebuilding session" ); - self.rebuild_state(&plan)?; + self.rebuild_state(plan)?; } Err(e) => return Err(VkDecodeError::Convert(e)), } @@ -1002,6 +1083,7 @@ impl VkH264Decoder { is_idr: plan.picture.is_idr, recovery, decode_order, + references_clean: plan.picture.references_clean, }, ); Ok(()) @@ -1358,6 +1440,40 @@ impl VkH264Decoder { } } + /// Clear the DPB state a failed AU left behind, so planning resumes at the + /// next IDR instead of erroring on residency nothing can honour. + /// + /// Three ledgers have to agree and, after a post-planning failure, do not: + /// the PLANNER's DPB, this decoder's [`SlotMap`], and the slot→image + /// bindings. [`Self::flush`] settles the first (and hands back any picture + /// that did reach output — those frames are real and are still delivered), + /// then [`crate::decoder_h265::reset_slot_bindings`] empties the other two. + /// Pool images the stale bindings pinned go back on the free list; images a + /// consumer still HOLDS stay pinned by their own `held` counts, exactly as + /// they would across a session rebuild. + /// + /// Deliberately not a session rebuild: the session, pools and ring are all + /// still valid — only the DPB bookkeeping is stale — and a rebuild would + /// churn every image allocation for a condition an IDR fixes anyway. + /// + /// The H.265 twin (`decoder_h265::recover_dpb`) is the same function one codec + /// over; the two share `reset_slot_bindings` rather than the whole body because + /// each has to call its OWN `flush`, which settles its own planner's DPB. + fn recover_dpb(&mut self) { + debug!("recovering from a failed AU — flushing the H.264 DPB to the next IDR"); + self.flush(); + if let Some(state) = &mut self.state { + let unbound = crate::decoder_h265::reset_slot_bindings( + &mut state.slots, + &mut state.slot_image, + &mut state.slot_refs, + ); + for picture in unbound { + state.pool.pictures[picture].bound = false; + } + } + } + /// Session/caps for THIS plan exist and match its extent + profile, and the /// stream sits inside the device's level ceiling. DPB-depth mismatches /// surface later as `plan_to_vk`'s `CapacityMismatch` (the designed trigger) @@ -1657,6 +1773,7 @@ pub(crate) fn build_frame( is_idr: entry.is_idr, recovery: entry.recovery, decode_order: entry.decode_order, + references_clean: entry.references_clean, query_slot: entry.query_slot, submission: entry.submission, picture: entry.image as u32, @@ -1848,37 +1965,9 @@ unsafe fn record_and_submit( } // ---- bound-slot staging ---- - // Scope list: this AU's references first, then every other still-held slot - // (their resources must stay bound for their associations to persist), then - // the setup slot as the ACTIVATION entry (slot index -1 binds its resource - // without a current association; the decode op's setup slot then claims it). - let mut scope: Vec<(i32, vk::ImageView, hh::StdVideoDecodeH264ReferenceInfo)> = Vec::new(); - for r in &vk_plan.refs { - match slot_view(state, r.slot) { - Some(view) => scope.push((i32::from(r.slot), view, r.std)), - None => trace!(slot = r.slot, "referenced slot without a bound image"), - } - } - for (slot, _id) in state.slots.held() { - if slot == vk_plan.setup_slot - || scope - .iter() - .any(|&(index, _, _)| index >= 0 && index as u8 == slot) - { - continue; - } - match (state.slot_refs[usize::from(slot)], slot_view(state, slot)) { - (Some(std), Some(view)) => scope.push((i32::from(slot), view, std)), - // Unreachable in practice: every held slot was a setup slot once. - _ => trace!( - slot, - "held slot without reference info/binding — left unbound" - ), - } - } - let reference_count = vk_plan.refs.len().min(scope.len()); // The setup/dst resource: the fresh pool image (coincide) or the DPB layer - // (distinct — the pool image is the separate decode output). + // (distinct — the pool image is the separate decode output). Resolved before the + // scope is built, because it is the scope's last entry. let setup_view = if coincide { state.pool.pictures[dst].view } else { @@ -1888,31 +1977,48 @@ unsafe fn record_and_submit( .expect("distinct mode") .dpb_view(vk_plan.setup_slot) }; - scope.push((-1, setup_view, vk_plan.setup_ref)); + // Scope list: this AU's references first, then every other still-held slot + // (their resources must stay bound for their associations to persist), then + // the setup slot as the ACTIVATION entry (slot index -1 binds its resource + // without a current association; the decode op's setup slot then claims it). + // + // Shared with H.265 (`decoder_h265::build_scope`): the two codecs' layout, + // fail-closed rule and reference-count derivation are the same algorithm over a + // different `StdVideo*` type, and this function's whole job is refusing to guess — + // the property least tolerant of two copies drifting apart. + let held: Vec = state.slots.held().map(|(slot, _id)| slot).collect(); + let (scope, reference_count) = crate::decoder_h265::build_scope( + &vk_plan.refs, + held.into_iter(), + vk_plan.setup_slot, + setup_view, + vk_plan.setup_ref, + &state.slot_refs, + |slot| slot_view(state, slot), + )?; // Staged arrays: resources → std infos → codec slot infos → slot infos. Each // vector is fully built before the next borrows it, so nothing reallocates // under a stored pointer. let resources: Vec> = scope .iter() - .map(|&(_, view, _)| { + .map(|e| { vk::VideoPictureResourceInfoKHR::default() .coded_extent(coded_extent) .base_array_layer(0) - .image_view_binding(view) + .image_view_binding(e.view) }) .collect(); - let std_refs: Vec = - scope.iter().map(|&(_, _, std)| std).collect(); + let std_refs: Vec = scope.iter().map(|e| e.std).collect(); let mut dpb_infos: Vec> = std_refs .iter() .map(|std| vk::VideoDecodeH264DpbSlotInfoKHR::default().std_reference_info(std)) .collect(); let mut begin_slots: Vec> = Vec::with_capacity(scope.len()); - for (index, &(slot_index, _, _)) in scope.iter().enumerate() { + for (index, entry) in scope.iter().enumerate() { begin_slots.push( vk::VideoReferenceSlotInfoKHR::default() - .slot_index(slot_index) + .slot_index(entry.slot_index) .picture_resource(&resources[index]), ); } @@ -2038,8 +2144,96 @@ unsafe fn record_and_submit( #[cfg(test)] mod tests { + use ash::vk::Handle as _; + use super::*; + /// A fake, never-dereferenced view handle keyed by slot, so a scope's bindings + /// can be checked without a device (the H.265 tests' idiom, one codec over). + fn fake_view(slot: u8) -> vk::ImageView { + vk::ImageView::from_raw(u64::from(slot) + 1) + } + + /// A reference-info value carrying just the field the assertions read. + fn h264_std_ref(frame_num: u16) -> hh::StdVideoDecodeH264ReferenceInfo { + // SAFETY: StdVideoDecodeH264ReferenceInfo is a plain-C bindgen struct of a + // bitfield word and integers; all-zero is valid for every field. + let mut std: hh::StdVideoDecodeH264ReferenceInfo = unsafe { std::mem::zeroed() }; + std.FrameNum = frame_num; + std + } + + fn h264_ref(slot: u8, frame_num: u16) -> crate::pic::VkRef { + crate::pic::VkRef { + slot, + std: h264_std_ref(frame_num), + id: u64::from(slot), + } + } + + /// The H.264 leg of the fail-closed rule. It used to trace-and-continue here, on + /// the grounds that H.264 carries no RPS index arrays — but the hardware still + /// decoded the picture against a reference that was never bound, which is a gray + /// plate with motion on it, shipped with no warning attached. Fail closed. + #[test] + fn an_h264_reference_slot_without_a_bound_image_fails_the_whole_op() { + let refs = vec![h264_ref(1, 10), h264_ref(3, 20)]; + let slot_refs = vec![Some(h264_std_ref(0)); 8]; + let err = crate::decoder_h265::build_scope( + &refs, + [1u8, 3].into_iter(), + 0, + fake_view(0), + h264_std_ref(30), + &slot_refs, + |slot| (slot != 3).then(|| fake_view(slot)), + ) + .unwrap_err(); + assert!( + matches!(err, VkDecodeError::UnboundReferenceSlot { slot: 3 }), + "{err}" + ); + } + + /// `reference_count` must be the number of THIS AU's references and nothing else. + /// The old H.264 form (`refs.len().min(scope.len())`) was taken AFTER the + /// held-slot pass appended to the same vector, so a short refs list let the decode + /// op's reference array run past the references into unrelated held slots — a + /// picture predicted from something the stream never named. + #[test] + fn the_h264_reference_count_covers_the_references_and_never_a_held_slot() { + // Two references (slots 1, 3); slots 5 and 6 are held but NOT referenced. + let refs = vec![h264_ref(1, 10), h264_ref(3, 20)]; + let slot_refs = vec![Some(h264_std_ref(77)); 8]; + let (scope, reference_count) = crate::decoder_h265::build_scope( + &refs, + [1u8, 3, 5, 6].into_iter(), + 0, + fake_view(0), + h264_std_ref(30), + &slot_refs, + |slot| Some(fake_view(slot)), + ) + .unwrap(); + + assert_eq!(reference_count, 2, "exactly this AU's references"); + assert_eq!( + scope[..reference_count] + .iter() + .map(|e| e.slot_index) + .collect::>(), + vec![1, 3], + "the decode op's reference prefix is the references, in order" + ); + // The rest of the scope keeps the other slots bound (so their associations + // survive) and ends on the setup activation entry — but none of that is a + // reference of this AU. + assert_eq!( + scope.iter().map(|e| e.slot_index).collect::>(), + vec![1, 3, 5, 6, -1] + ); + } + #[test] fn settle_dpb_readies_outputs_in_order_and_returns_never_output_removals() { let mut pending: BTreeMap = BTreeMap::new(); diff --git a/crates/pf-vkdecode/src/decoder_av1.rs b/crates/pf-vkdecode/src/decoder_av1.rs index b106917b..88add75a 100644 --- a/crates/pf-vkdecode/src/decoder_av1.rs +++ b/crates/pf-vkdecode/src/decoder_av1.rs @@ -1114,6 +1114,7 @@ impl VkAv1Decoder { is_idr: plan.picture.is_key, recovery: crate::recovery::RecoveryMark::NONE, decode_order, + references_clean: plan.picture.references_clean, }, ); diff --git a/crates/pf-vkdecode/src/decoder_h265.rs b/crates/pf-vkdecode/src/decoder_h265.rs index a3713085..b1f338f2 100644 --- a/crates/pf-vkdecode/src/decoder_h265.rs +++ b/crates/pf-vkdecode/src/decoder_h265.rs @@ -139,13 +139,20 @@ struct SessionStateH265 { /// /// This decoder FAILS CLOSED, and that stays: when an AU cannot be carried /// through to a submitted decode, it returns an error rather than substituting a -/// reference or decoding against a slot whose image is gone. H.264's -/// soft-degrade (trace the missing binding, drop that reference, decode anyway) -/// is not available here because `StdVideoDecodeH265PictureInfo`'s +/// reference or decoding against a slot whose image is gone. The structural argument +/// is that `StdVideoDecodeH265PictureInfo`'s /// `RefPicSetStCurrBefore`/`StCurrAfter`/`LtCurr` arrays hold INDICES into the /// decode op's reference array — dropping one entry re-points every later index /// at the wrong picture, which is the corruption-hiding class this crate refuses. /// +/// ⚠ H.264 used to soft-degrade here (trace the missing binding, drop that reference, +/// decode anyway) on the grounds that it carries no such index arrays. It now fails +/// closed and carries this same latch: the arrays were never the point, the OUTPUT +/// was. A P-picture decoded against a reference that was never bound is a gray plate +/// with motion painted over it, and because the planner raises no warning for it, that +/// frame reached the screen and cleared the consumer's demotion streak. Both codecs +/// now fail closed, and both recover through this latch rather than wedging. +/// /// But failing closed once must not wedge the stream FOREVER, and without this /// latch it did: by the time an AU reaches a failure exit, `plan_to_vk_h265` has /// already mutated the [`SlotMap`] (releases + the setup assignment) and the @@ -636,6 +643,7 @@ impl VkH265Decoder { is_idr: plan.picture.is_idr, recovery, decode_order, + references_clean: plan.picture.references_clean, }, ); @@ -1247,10 +1255,15 @@ fn profile_key_for(plan: &AuPlan) -> Result { /// let [`build_scope`] bind a slot the planner no longer knows about, which is the /// same "plausible-looking picture in the wrong place" the unbound-reference /// refusal exists to prevent. -fn reset_slot_bindings( +/// +/// Generic over the cached reference-info type so H.264's recovery uses this exact +/// code rather than a twin: the three ledgers and the "empty them together" rule are +/// codec-independent (`SlotMap` is already shared), and only the `StdVideo*` type in +/// `slot_refs` differs. +pub(crate) fn reset_slot_bindings( slots: &mut SlotMap, slot_image: &mut [Option], - slot_refs: &mut [Option], + slot_refs: &mut [Option], ) -> Vec { // `release` is the only way a slot is freed (SlotMap docs); the collect is // because `held` borrows the map the releases mutate. @@ -1279,10 +1292,55 @@ fn slot_view(state: &SessionStateH265, slot: u8) -> Option { /// (No derived equality: `StdVideoDecodeH265ReferenceInfo` is a plain-C bindgen /// struct without it. Assertions compare the fields that carry meaning.) #[derive(Debug, Clone, Copy)] -struct ScopeEntry { - slot_index: i32, - view: vk::ImageView, - std: hh::StdVideoDecodeH265ReferenceInfo, +pub(crate) struct ScopeEntry { + pub(crate) slot_index: i32, + pub(crate) view: vk::ImageView, + pub(crate) std: S, +} + +/// One of this AU's references, as [`build_scope`] needs to see it: a DPB slot and +/// the codec reference info to bind with it. +/// +/// It exists so H.264 and H.265 share ONE scope builder instead of two hand-copies of +/// a function whose whole job is refusing to guess — the property most in need of a +/// single implementation. Their `VkRef`/`VkRefH265` differ only in the `StdVideo*` +/// type they carry, so the shape generalises exactly. +/// +/// ⚠ AV1 deliberately keeps its own ([`crate::decoder_av1`]'s `build_scope_av1`): its +/// reference array is indexed by reference NAME and may hold HOLES, so its walk is a +/// different algorithm rather than the same one over a different Std type. Folding it +/// in here would mean a builder with a mode flag, which is how the two would drift. +pub(crate) trait ScopeRef { + /// The codec's `StdVideoDecode*ReferenceInfo`. + type Std: Copy; + /// The DPB slot this reference is bound in. + fn slot(&self) -> u8; + fn std(&self) -> Self::Std; +} + +/// [`build_scope`]'s answer: the bound-slot list, and how many of its LEADING entries +/// are this AU's own references (the prefix the decode op takes as its reference +/// array — see the ordering note in `build_scope`'s docs). +pub(crate) type Scope = (Vec::Std>>, usize); + +impl ScopeRef for crate::pic_h265::VkRefH265 { + type Std = hh::StdVideoDecodeH265ReferenceInfo; + fn slot(&self) -> u8 { + self.slot + } + fn std(&self) -> Self::Std { + self.std + } +} + +impl ScopeRef for crate::pic::VkRef { + type Std = ash::vk::native::StdVideoDecodeH264ReferenceInfo; + fn slot(&self) -> u8 { + self.slot + } + fn std(&self) -> Self::Std { + self.std + } } /// Build the coding scope's bound-slot list and say how many leading entries are @@ -1295,36 +1353,51 @@ struct ScopeEntry { /// resources must stay bound even when this AU does not reference them); /// 3. the setup slot as the activation entry, slot index `-1`. /// -/// A reference whose slot binds no image is a hard error, never a skip: -/// `StdVideoDecodeH265PictureInfo`'s `RefPicSetStCurrBefore`/`StCurrAfter`/ -/// `LtCurr` arrays name DPB slots, and every slot they name is one of `refs`' -/// ([`crate::pic_h265`]) — so dropping an entry leaves the hardware with a named -/// slot this op never bound, which it can only answer by guessing or failing. -/// Output that looks plausible and is wrong is the outcome this refusal exists to -/// prevent. -fn build_scope( - refs: &[crate::pic_h265::VkRefH265], +/// A reference whose slot binds no image is a hard error, never a skip. For H.265 the +/// argument is `StdVideoDecodeH265PictureInfo`'s `RefPicSetStCurrBefore`/`StCurrAfter`/ +/// `LtCurr` arrays: they name DPB slots, every slot they name is one of `refs`' +/// ([`crate::pic_h265`]), so dropping an entry leaves the hardware with a named slot +/// this op never bound — which it can only answer by guessing or failing. +/// +/// H.264 has no such index arrays, and it used to skip the case with a `trace!` on +/// exactly that reasoning. The reasoning was wrong about the OUTPUT: the hardware +/// still decodes a P-picture against a reference that was never bound, which on the +/// DPB-and-output-COINCIDE path is a gray plate with the new frame's motion painted +/// over it — and because the planner raised no warning (its DPB genuinely holds the +/// picture; the breakage is in this ledger), the frame was shipped, presented, and +/// cleared the consumer's demotion streak on its way past. Both codecs fail closed +/// here now; the recovery latch is what keeps failing closed from wedging the stream. +/// +/// `reference_count` is captured the instant the `refs` loop ends, BEFORE the +/// held-slot pass appends anything. That ordering is load-bearing: the decode op takes +/// `scope[..reference_count]` as its reference list, so a count computed after the +/// second pass could hand it a still-held slot that this AU does not reference, in +/// place of one that failed to resolve. (Fail-closed above makes that unreachable — +/// but the construction must be correct on its own, not by depending on a check +/// somewhere else.) +pub(crate) fn build_scope( + refs: &[R], held_slots: impl Iterator, setup_slot: u8, setup_view: vk::ImageView, - setup_ref: hh::StdVideoDecodeH265ReferenceInfo, - slot_refs: &[Option], + setup_ref: R::Std, + slot_refs: &[Option], view_of: impl Fn(u8) -> Option, -) -> Result<(Vec, usize), VkDecodeError> { - let mut scope: Vec = Vec::with_capacity(refs.len() + slot_refs.len() + 1); +) -> Result, VkDecodeError> { + let mut scope: Vec> = Vec::with_capacity(refs.len() + slot_refs.len() + 1); for r in refs { - match view_of(r.slot) { + match view_of(r.slot()) { Some(view) => scope.push(ScopeEntry { - slot_index: i32::from(r.slot), + slot_index: i32::from(r.slot()), view, - std: r.std, + std: r.std(), }), - None => return Err(VkDecodeError::UnboundReferenceSlot { slot: r.slot }), + None => return Err(VkDecodeError::UnboundReferenceSlot { slot: r.slot() }), } } let reference_count = scope.len(); for slot in held_slots { - if slot == setup_slot || refs.iter().any(|r| r.slot == slot) { + if slot == setup_slot || refs.iter().any(|r| r.slot() == slot) { continue; } match ( @@ -1862,8 +1935,11 @@ mod tests { let setup_slot = slots.assign(400).unwrap(); assert_eq!(setup_slot, 0, "the freed slots are assignable again"); slot_image[usize::from(setup_slot)] = Some(9); + // The empty slice needs its element type named now that `build_scope` is + // generic over the two codecs' reference types. + let no_refs: [VkRefH265; 0] = []; let (scope, reference_count) = build_scope( - &[], + &no_refs, slots.held().map(|(slot, _id)| slot), setup_slot, fake_view(setup_slot), diff --git a/crates/pf-vkdecode/src/integrity.rs b/crates/pf-vkdecode/src/integrity.rs index a5f493a3..979669c9 100644 --- a/crates/pf-vkdecode/src/integrity.rs +++ b/crates/pf-vkdecode/src/integrity.rs @@ -38,19 +38,20 @@ use crate::{Av1PlanWarning, H265PlanWarning, PlanWarning}; /// property of the STREAM's signalling, which the decoder answers by failing to open /// a session, not by showing a damaged frame. /// -/// Written as an EXHAUSTIVE match with no wildcard, deliberately. A `matches!` (or -/// a `_ => false`) makes "damage" the opt-in and silence the default, so a -/// `PlanWarning` added later — by definition one nobody here has classified — -/// would be reported as clean and its picture shown. Invisible damage is the bug -/// this whole program exists to end; the compiler is the only reviewer guaranteed -/// to be present when that variant is written, so it gets the decision. +/// ⚠ The classification itself now lives on the warning enum, in pf-bitstream +/// ([`PlanWarning::is_integrity`]), and this function delegates. It moved there when +/// the planners gained the per-picture clean bit +/// ([`pf_bitstream::h264::PicturePlan::references_clean`]): that ledger has to mark a +/// picture damaged on exactly the warnings a consumer conceals on, and it lives one +/// crate DOWN from here. A copy of the list in each crate would let the two disagree — +/// the planner recording a picture as clean while the client concealed it, or the +/// reverse — which is the same invisible-damage failure the single-list rule below was +/// written to prevent, one layer lower. One list, in the crate that owns the enum. +/// +/// This function stays as the crate's public spelling of the question (the fault +/// harness, the client and the tests all name it) and keeps its exact semantics. pub fn is_integrity_warning(w: &PlanWarning) -> bool { - match w { - PlanWarning::FrameNumGap { .. } - | PlanWarning::MissingReference { .. } - | PlanWarning::TruncatedAu { .. } => true, - PlanWarning::Mmco5Rebase | PlanWarning::LevelDerivedDpb { .. } => false, - } + w.is_integrity() } /// The H.265 twin — the same set pf-bitstream's own `h265` conformance harness @@ -59,10 +60,7 @@ pub fn is_integrity_warning(w: &PlanWarning) -> bool { /// Exhaustive for the same reason as [`is_integrity_warning`]: a new H.265 warning /// must not be able to mean "damaged" and read as clean. pub fn is_integrity_warning_h265(w: &H265PlanWarning) -> bool { - match w { - H265PlanWarning::MissingReference { .. } | H265PlanWarning::TruncatedAu { .. } => true, - H265PlanWarning::NonZeroReorder { .. } => false, - } + w.is_integrity() } /// The AV1 twin (M7). Every variant the AV1 planner has today IS damage, and that @@ -93,11 +91,7 @@ pub fn is_integrity_warning_h265(w: &H265PlanWarning) -> bool { /// Exhaustive for the same reason as [`is_integrity_warning`]: a new AV1 warning /// must not be able to mean "damaged" and read as clean. pub fn is_integrity_warning_av1(w: &Av1PlanWarning) -> bool { - match w { - Av1PlanWarning::MissingReference { .. } - | Av1PlanWarning::MissingShowExisting { .. } - | Av1PlanWarning::TruncatedAu { .. } => true, - } + w.is_integrity() } #[cfg(test)] diff --git a/crates/pf-vkdecode/src/pic_h265.rs b/crates/pf-vkdecode/src/pic_h265.rs index d6fe2c1e..1a4b485b 100644 --- a/crates/pf-vkdecode/src/pic_h265.rs +++ b/crates/pf-vkdecode/src/pic_h265.rs @@ -899,6 +899,9 @@ mod tests { max_dpb_frames, short_term_ref_pic_set_size_bits: 0, recovery_point: None, + // These fixtures model a healthy stream; the clean bit is the planner's + // observation and nothing in this conversion layer reads it. + references_clean: true, } } diff --git a/crates/punktfunk-core/src/abi.rs b/crates/punktfunk-core/src/abi.rs index 31f6d972..b57fc50b 100644 --- a/crates/punktfunk-core/src/abi.rs +++ b/crates/punktfunk-core/src/abi.rs @@ -5167,6 +5167,15 @@ pub unsafe extern "C" fn punktfunk_reanchor_gate_arm_expecting_drops( /// `USER_FLAG_RECOVERY_POINT`. Pass `decoder_keyframe = false` where the platform decoder doesn't flag /// IDRs (VideoToolbox/MediaCodec) — the wire `FLAG_SOF` covers it. /// +/// This is the uncorroborated entry point and deliberately stays that way. Rust embedders whose +/// decoder parses the bitstream call [`ReanchorGate::on_decoded_corroborated`] to let their own +/// parser refute a `USER_FLAG_RECOVERY_ANCHOR` that names a picture they had to conceal; every +/// client reachable through THIS surface (Apple VideoToolbox, Android MediaCodec) uses a platform +/// decoder that surfaces no such fact, so it would have nothing to pass but +/// `AnchorEvidence::Unavailable` — which is exactly what this wrapper already means. Growing a +/// second export for a corroboration no C caller can supply would spend an ABI version bump on +/// dead surface. +/// /// # Safety /// `g` is a valid gate handle; `out_present` is writable or NULL. #[unsafe(no_mangle)] diff --git a/crates/punktfunk-core/src/reanchor.rs b/crates/punktfunk-core/src/reanchor.rs index b5977d1f..f6b27f11 100644 --- a/crates/punktfunk-core/src/reanchor.rs +++ b/crates/punktfunk-core/src/reanchor.rs @@ -20,6 +20,30 @@ //! VideoToolbox, every FFmpeg rung, which exposes no SEI) simply never calls it and every wire //! behaviour above is bit-for-bit unchanged. //! +//! # The one claim a client can REFUTE +//! +//! Of the three lifts, two are self-evident to the client and one is pure hearsay. An IDR predicts +//! from nothing, so "this re-anchors decode" is a property of the picture itself. A recovery mark is +//! only *half* a re-anchor and the gate says so by requiring two. But +//! [`USER_FLAG_RECOVERY_ANCHOR`] is the HOST asserting a fact about the CLIENT's decoder — *the +//! picture I coded this P-frame against is one you still hold, intact* — and until +//! [`AnchorEvidence`] existed the client took it on faith, on the first occurrence, with no +//! scrutiny at all. +//! +//! When that assertion is wrong the failure is the worst-shaped one in this module: the anchor lifts +//! the freeze onto a picture predicted from a reference the client had to conceal, so the gray plate +//! reaches the screen AND the gate stops holding, which means it keeps reaching the screen until +//! some later signal re-arms. A re-anchor claim the client can refute is therefore worse than no +//! claim at all — no claim merely holds the last good frame until the backstop. +//! +//! So a client whose decoder parses the bitstream corroborates it: it already knows which pictures +//! this AU predicts from and whether each of those decoded from a complete reference chain, and +//! [`on_decoded_corroborated`](ReanchorGate::on_decoded_corroborated) refuses an anchor whose +//! references it can prove were damaged. Refusing can only ever make the gate hold LONGER — the +//! freeze stays up, the backstop fires on its ORIGINAL deadline, and the client escalates to a real +//! IDR — which is the direction every other rule here errs in, deliberately. Lanes that cannot +//! answer pass [`AnchorEvidence::Unavailable`] and behave exactly as they always have. +//! //! [`USER_FLAG_RECOVERY_POINT`]: crate::packet::USER_FLAG_RECOVERY_POINT //! [`USER_FLAG_RECOVERY_ANCHOR`]: crate::packet::USER_FLAG_RECOVERY_ANCHOR @@ -97,8 +121,9 @@ pub fn index_gap(expected: u32, got: u32) -> Option { /// Fold one decoded frame into the re-anchor state and decide whether it lifts the post-loss freeze. /// /// `is_keyframe` — a real IDR (always a clean re-anchor). `has_anchor` — this AU carried -/// [`USER_FLAG_RECOVERY_ANCHOR`](crate::packet::USER_FLAG_RECOVERY_ANCHOR), the host's definitive -/// single-frame re-anchor from an LTR-RFI recovery (a clean P-frame coded against a known-good +/// [`USER_FLAG_RECOVERY_ANCHOR`](crate::packet::USER_FLAG_RECOVERY_ANCHOR) **and the caller did not +/// refute it** ([`AnchorEvidence`]), the host's definitive single-frame re-anchor from an LTR-RFI +/// recovery (a clean P-frame coded against a known-good /// reference), so it lifts on the FIRST occurrence exactly like an IDR — no two-mark wait. `has_mark` — /// this AU carried [`USER_FLAG_RECOVERY_POINT`](crate::packet::USER_FLAG_RECOVERY_POINT), a /// host-signalled intra-refresh wave boundary (only *half* a re-anchor). `marks` — recovery marks seen @@ -158,6 +183,44 @@ impl LocalRecovery { }; } +/// What a client's OWN parser can say about the host's re-anchor claim on one decoded frame — the +/// corroboration for [`USER_FLAG_RECOVERY_ANCHOR`](crate::packet::USER_FLAG_RECOVERY_ANCHOR). +/// +/// An anchor is the host asserting something about the CLIENT's decoder: *this P-frame is coded +/// against a picture you still hold, intact, so decoding it re-anchors you*. The host derives that +/// from its own slot bookkeeping — which tracks whether the client RECEIVED a frame, not whether it +/// DECODED that frame from a complete reference chain. Those two differ exactly when the client had +/// to conceal, and the gap between them is what puts a gray plate on screen with the freeze lifted. +/// +/// Three states rather than a bool, for the same reason [`LocalRecovery`] is two facts: a lane that +/// *cannot* answer must be able to say so instead of being folded into "nothing wrong here". Only +/// [`Self::ReferencesDamaged`] changes any behaviour; the other two are indistinguishable to the +/// gate and differ only in what they claim at the call site. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] +pub enum AnchorEvidence { + /// This lane has no local bitstream parser, so it cannot corroborate or refute anything — the + /// host's claim stands, exactly as it always has. Android MediaCodec, Apple VideoToolbox and + /// every lane reached over the C ABI pass this, and their behaviour is bit-for-bit unchanged. + #[default] + Unavailable, + /// Corroborated: every picture this AU predicts from was itself decoded from a fully-available + /// reference chain, so the host's claim is consistent with what this decoder actually holds. + ReferencesClean, + /// Refuted: this AU predicts from a picture that needed concealment. Whatever the host believes, + /// decoding this frame cannot re-anchor a decoder whose reference for it is already damaged, so + /// the anchor does not lift the freeze. + ReferencesDamaged, +} + +impl AnchorEvidence { + /// May an [`USER_FLAG_RECOVERY_ANCHOR`](crate::packet::USER_FLAG_RECOVERY_ANCHOR) on this frame + /// be honoured? Only an outright refutation withholds it — silence is not refutation, so a lane + /// that cannot corroborate never becomes *stricter* than it was. + fn honours_anchor(self) -> bool { + !matches!(self, AnchorEvidence::ReferencesDamaged) + } +} + /// Whether a decoded frame should be shown or withheld while the gate is (or isn't) frozen. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum GateVerdict { @@ -333,6 +396,11 @@ impl ReanchorGate { /// A decoded frame always clears the no-output streak. When frozen, a live mark stream pushes the /// backstop out ([`RECOVERY_MARK_PATIENCE`]) so a healing wave isn't pre-empted by a mid-heal IDR. /// + /// This is the whole-hearsay entry point: it believes an anchor on sight. A client whose decoder + /// parses the bitstream should call + /// [`on_decoded_corroborated`](Self::on_decoded_corroborated) instead and let its own parser + /// check the host's claim. + /// /// [`USER_FLAG_RECOVERY_ANCHOR`]: crate::packet::USER_FLAG_RECOVERY_ANCHOR /// [`USER_FLAG_RECOVERY_POINT`]: crate::packet::USER_FLAG_RECOVERY_POINT pub fn on_decoded( @@ -340,10 +408,49 @@ impl ReanchorGate { wire_flags: u32, decoder_keyframe: bool, now: Instant, + ) -> GateVerdict { + self.on_decoded_corroborated( + wire_flags, + decoder_keyframe, + AnchorEvidence::Unavailable, + now, + ) + } + + /// [`on_decoded`](Self::on_decoded) for a client that can CHECK the host's re-anchor claim + /// against its own decoder — the native-decode lanes, which parse every AU themselves and so + /// know both which pictures this one predicts from and whether each of those decoded cleanly. + /// + /// `evidence` is consulted for exactly one thing: whether a + /// [`USER_FLAG_RECOVERY_ANCHOR`](crate::packet::USER_FLAG_RECOVERY_ANCHOR) on THIS frame may + /// lift the freeze. [`AnchorEvidence::ReferencesDamaged`] withholds that lift and nothing else, + /// and the two exclusions are as deliberate as the rule itself: + /// + /// * **A real IDR still lifts.** It predicts from nothing, so no evidence about its references + /// can bear on it — and the IDR is precisely the escalation a refused anchor is trying to + /// provoke. Refusing it would turn the fix into the permanent freeze it exists to avoid. + /// * **The two-mark [`USER_FLAG_RECOVERY_POINT`](crate::packet::USER_FLAG_RECOVERY_POINT) rule + /// is untouched**, including its [`RECOVERY_MARK_PATIENCE`] deadline push. An intra-refresh + /// wave heals by overwriting stripes rather than by predicting from one named picture, so + /// "this frame's references were damaged" says nothing about whether the wave completed. + /// + /// A refused anchor also leaves the backstop deadline exactly where the arm put it. That is the + /// point rather than an omission: the freeze becomes overdue on its ORIGINAL schedule, [`poll`](Self::poll) + /// re-asks, and the client escalates to a real IDR — the recovery the host's anchor failed to + /// deliver. Pushing the deadline out on a refusal would reward a host whose anchors do not work + /// with a longer wait. + pub fn on_decoded_corroborated( + &mut self, + wire_flags: u32, + decoder_keyframe: bool, + evidence: AnchorEvidence, + now: Instant, ) -> GateVerdict { self.no_output_streak = 0; let is_keyframe = decoder_keyframe || (wire_flags & FLAG_SOF as u32 != 0); - let has_anchor = wire_flags & USER_FLAG_RECOVERY_ANCHOR != 0; + // An anchor the client's own parser refutes is not an anchor. Folded in HERE rather than + // inside `reanchor_after_frame` so that function stays a pure statement of the wire rules. + let has_anchor = wire_flags & USER_FLAG_RECOVERY_ANCHOR != 0 && evidence.honours_anchor(); let has_mark = wire_flags & USER_FLAG_RECOVERY_POINT != 0; if has_mark && self.awaiting { self.deadline = Some(now + RECOVERY_MARK_PATIENCE); @@ -888,4 +995,188 @@ mod tests { assert!(!g.poll(0, t + Duration::from_millis(1))); assert!(g.is_holding()); } + + // ---- the corroborated-anchor path (AnchorEvidence) ---- + + use AnchorEvidence::{ReferencesClean, ReferencesDamaged, Unavailable}; + + /// The headline. The host says "this P-frame re-anchors you"; the client's own parser says the + /// picture it predicts from is one IT had to conceal. Both cannot be true, and the client's + /// statement is about its OWN decoder — so the anchor does not lift and the gray plate the + /// anchor would have presented never reaches the screen. + #[test] + fn an_anchor_whose_references_the_decoder_concealed_does_not_lift() { + let mut g = ReanchorGate::new(0); + let now = t0(); + g.arm(now); + assert_eq!( + g.on_decoded_corroborated(ANCHOR, false, ReferencesDamaged, now), + GateVerdict::Hold, + "a refuted anchor is not a re-anchor" + ); + assert!(g.is_holding(), "and the freeze stays up"); + // Repeating it changes nothing — a host that keeps sending anchors it cannot honour never + // talks its way past the gate. + assert_eq!( + g.on_decoded_corroborated(ANCHOR, false, ReferencesDamaged, now), + GateVerdict::Hold + ); + assert!(g.is_holding()); + } + + /// The escalation a refusal exists to provoke must still work. An IDR predicts from nothing, so + /// no evidence about damaged references can bear on it — refusing it too would convert this fix + /// into the permanent freeze it is meant to avoid. + #[test] + fn a_real_idr_lifts_even_while_the_evidence_refutes_anchors() { + // The decoder's own keyframe flag... + let mut g = ReanchorGate::new(0); + let now = t0(); + g.arm(now); + assert_eq!( + g.on_decoded_corroborated(ANCHOR, false, ReferencesDamaged, now), + GateVerdict::Hold + ); + assert_eq!( + g.on_decoded_corroborated(0, true, ReferencesDamaged, now), + GateVerdict::Present, + "the IDR re-anchors regardless of what the anchor evidence says" + ); + assert!(!g.is_holding()); + + // ...and the wire's FLAG_SOF, for the lanes whose decoder does not flag IDRs. + let mut g = ReanchorGate::new(0); + g.arm(now); + assert_eq!( + g.on_decoded_corroborated(SOF, false, ReferencesDamaged, now), + GateVerdict::Present + ); + assert!(!g.is_holding()); + } + + /// A corroborated anchor is still an anchor: the whole point is to refuse the ones the client + /// can disprove, not to stop honouring the mechanism. + #[test] + fn a_corroborated_anchor_lifts_on_the_first_occurrence() { + let mut g = ReanchorGate::new(0); + let now = t0(); + g.arm(now); + assert_eq!( + g.on_decoded_corroborated(0, false, ReferencesClean, now), + GateVerdict::Hold, + "an ordinary frame is still withheld" + ); + assert_eq!( + g.on_decoded_corroborated(ANCHOR, false, ReferencesClean, now), + GateVerdict::Present + ); + assert!(!g.is_holding()); + } + + /// `Unavailable` is the promise made to every lane without a local parser: silence is not + /// refutation. This walks the same sequences the wire-path tests above assert and requires the + /// identical verdicts through the corroborated entry point. + #[test] + fn an_uncorroborated_lane_behaves_exactly_as_it_always_has() { + // The anchor lift, byte for byte the `a_gap_lifts_on_the_first_rfi_anchor` contract. + let mut g = ReanchorGate::new(0); + let now = t0(); + g.arm(now); + assert_eq!( + g.on_decoded_corroborated(0, false, Unavailable, now), + GateVerdict::Hold + ); + assert_eq!( + g.on_decoded_corroborated(ANCHOR, false, Unavailable, now), + GateVerdict::Present + ); + assert!(!g.is_holding()); + + // And `on_decoded` — which every such lane actually calls — must agree with it exactly. + let mut wire = ReanchorGate::new(0); + let mut corroborated = ReanchorGate::new(0); + wire.arm(now); + corroborated.arm(now); + for flags in [0, POINT, 0, ANCHOR, SOF, 0] { + assert_eq!( + wire.on_decoded(flags, false, now), + corroborated.on_decoded_corroborated(flags, false, Unavailable, now), + "flags {flags:#x} diverged between the two entry points" + ); + assert_eq!(wire.is_holding(), corroborated.is_holding()); + } + } + + /// A refused anchor must not buy the host time. The freeze becomes overdue on the deadline the + /// ARM set — not one pushed out by the refusal — so the client escalates to the real IDR that + /// the failed anchor did not deliver. + #[test] + fn a_refused_anchor_leaves_the_backstop_on_its_original_deadline() { + let mut g = ReanchorGate::new(0); + let start = t0(); + g.arm(start); + // Anchors keep arriving and keep being refused, right up to the deadline. + for ms in [10, 100, 300, 490] { + assert_eq!( + g.on_decoded_corroborated( + ANCHOR, + false, + ReferencesDamaged, + start + Duration::from_millis(ms) + ), + GateVerdict::Hold + ); + assert!(!g.poll(0, start + Duration::from_millis(ms)), "not yet due"); + } + let overdue = start + REANCHOR_FREEZE_MAX + Duration::from_millis(1); + assert!( + g.poll(0, overdue), + "the backstop fires on the arm's own deadline — the refusals did not extend it" + ); + assert!( + g.is_holding(), + "and it keeps holding, never resuming to gray" + ); + } + + /// Refuting an anchor says nothing about an intra-refresh wave: a wave heals by overwriting + /// stripes rather than by predicting from one named picture, so the two-mark rule and its + /// patience deadline must be untouched by the evidence. + #[test] + fn refuted_anchors_do_not_disturb_the_two_mark_rule() { + let mut g = ReanchorGate::new(0); + let now = t0(); + g.arm(now); + assert_eq!( + g.on_decoded_corroborated(POINT, false, ReferencesDamaged, now), + GateVerdict::Hold, + "mark #1 is still only half a re-anchor" + ); + // An anchor in between is refused and must not consume or reset the mark count. + assert_eq!( + g.on_decoded_corroborated(ANCHOR, false, ReferencesDamaged, now), + GateVerdict::Hold + ); + assert_eq!( + g.on_decoded_corroborated(POINT, false, ReferencesDamaged, now), + GateVerdict::Present, + "mark #2 lifts exactly as it does on the wire path" + ); + assert!(!g.is_holding()); + } + + /// The evidence is consulted only while an anchor flag is actually present — a refutation on an + /// ordinary frame must not become a second, sticky reason to hold. + #[test] + fn damaged_evidence_alone_neither_holds_nor_arms_an_unfrozen_gate() { + let mut g = ReanchorGate::new(0); + let now = t0(); + assert_eq!( + g.on_decoded_corroborated(0, false, ReferencesDamaged, now), + GateVerdict::Present, + "an unfrozen gate presents; the evidence is about anchors, not about frames" + ); + assert!(!g.is_holding()); + assert!(!g.poll(0, now)); + } } diff --git a/include/punktfunk_core.h b/include/punktfunk_core.h index 3d5b4acc..47d76c81 100644 --- a/include/punktfunk_core.h +++ b/include/punktfunk_core.h @@ -3774,6 +3774,15 @@ void punktfunk_reanchor_gate_arm_expecting_drops(ReanchorGate *g, uint64_t expec // `USER_FLAG_RECOVERY_POINT`. Pass `decoder_keyframe = false` where the platform decoder doesn't flag // IDRs (VideoToolbox/MediaCodec) — the wire `FLAG_SOF` covers it. // +// This is the uncorroborated entry point and deliberately stays that way. Rust embedders whose +// decoder parses the bitstream call [`ReanchorGate::on_decoded_corroborated`] to let their own +// parser refute a `USER_FLAG_RECOVERY_ANCHOR` that names a picture they had to conceal; every +// client reachable through THIS surface (Apple VideoToolbox, Android MediaCodec) uses a platform +// decoder that surfaces no such fact, so it would have nothing to pass but +// `AnchorEvidence::Unavailable` — which is exactly what this wrapper already means. Growing a +// second export for a corroboration no C caller can supply would spend an ABI version bump on +// dead surface. +// // # Safety // `g` is a valid gate handle; `out_present` is writable or NULL. PunktfunkStatus punktfunk_reanchor_gate_on_decoded(ReanchorGate *g, -- 2.54.0 From 93c1ed072369cfac8d9c63aec2c5e3608defcdf1 Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Sun, 16 Aug 2026 01:14:47 +0200 Subject: [PATCH 2/3] fix(host): an RFI anchor could be picked over damage the client had already reported The slot-family RFI backends choose a recovery anchor over `slot_wire`, which answers "did the client RECEIVE this frame" when the question is "did the client DECODE it intact". The taint sweep exists precisely to bridge that gap -- rfi.rs says so -- but it only ever runs inside invalidate_ref_frames, reachable from exactly ONE of the client's five damage signals (the frame-index gap). The other four send a plain keyframe request, which sets force_kf and taints nothing. That is self-healing while the IDR is actually emitted. It is not when the request is coalesced away by the 750 ms IDR cooldown: the client's damage then goes unrepaired AND unrecorded, and those references stay anchor candidates for the next loss -- so the host serves an anchor over damage the client already told it about, tagged as the client's definitive clean re-anchor. The client-side half of this fix now refuses such an anchor; this is the other half, which stops it being offered. Adds Encoder::distrust_references (defaulted no-op, forwarded through TrackedEncoder -- unforwarded it would have been a silent no-op for every session), implemented by the three slot-family backends through their own persistence markers, which rfi.rs explicitly says not to harmonize: Vulkan Video blanks slot_wire ONLY and leaves slot_poc naming every resident, or build_h265_rps_s0 stops retaining them and a conforming decoder evicts pictures the encoder still references -- a separate grey bug that file already documents. AMF clears its mirror slot; QSV raises ltr_tainted rather than clearing its mirror, because the RejectedRefList only names Some slots and a cleared entry would skip the very reference being distrusted. Called on the IDR-cooldown branch, where the client is still reporting damage and nothing in the table is honestly known-good until the in-flight IDR lands. Deliberately NOT on the RFI-echo branch while its budget holds: that branch's premise is that the request echoes the loss the RFI just repaired, and distrusting on the first echo would poison the table after EVERY successful recovery, so RFI could never fire twice running and a sustained-loss session would fall back to the IDR path this block exists to keep it off. RFI_ECHO_MAX_SWALLOWED is already the hedge for that premise being wrong: when the client keeps asking past the budget, the anchor demonstrably did not heal it, and the escalation arm withdraws trust then -- on evidence rather than on suspicion. Distrust never touches prediction (that runs off slot indices, not the wire domain) and all three markers self-correct within a few frames, so the suppression is brief by construction and never spans a session. --- crates/pf-encode/src/enc/codec.rs | 26 ++++++++++ .../pf-encode/src/enc/linux/vulkan_video.rs | 29 +++++++++++ crates/pf-encode/src/enc/rfi.rs | 52 +++++++++++++++++++ crates/pf-encode/src/enc/windows/amf.rs | 24 +++++++++ crates/pf-encode/src/enc/windows/qsv.rs | 33 ++++++++++++ crates/pf-encode/src/lib.rs | 8 +++ crates/punktfunk-host/src/native/stream.rs | 35 ++++++++++++- 7 files changed, 205 insertions(+), 2 deletions(-) diff --git a/crates/pf-encode/src/enc/codec.rs b/crates/pf-encode/src/enc/codec.rs index 61188a3f..5ee3fbc2 100644 --- a/crates/pf-encode/src/enc/codec.rs +++ b/crates/pf-encode/src/enc/codec.rs @@ -368,6 +368,32 @@ pub trait Encoder: Send { fn invalidate_ref_frames(&mut self, _first_frame: i64, _last_frame: i64) -> bool { false } + /// Mark every resident reference UNTRUSTED FOR RFI ANCHORING — the answer to "the client told + /// us it has damage and we did NOT repair it". + /// + /// Why this exists at all. The slot-family RFI trust domain is the WIRE index each reference + /// holds, which answers *did the client receive this frame*; what an anchor pick actually needs + /// is *did the client DECODE it intact*. [`super::rfi`]'s taint sweep bridges that gap, but it + /// only runs inside [`invalidate_ref_frames`] — reachable from exactly ONE of the client's five + /// damage signals (the frame-index gap, which carries a loss RANGE). The other four report + /// through [`request_keyframe`](Self::request_keyframe), which carries no range and so cannot + /// sweep anything. That is self-healing while the IDR is actually emitted — an IDR flushes the + /// DPB and rebuilds trust from scratch — but the host coalesces those requests (a keyframe + /// storm is a 20-40× spike that deepens the very loss it recovers), and a coalesced request + /// leaves the client's damage unrepaired AND unrecorded. Those references stay anchor + /// candidates, and the next loss is answered with one of them tagged `recovery_anchor` — the + /// client's *definitive* clean re-anchor signal, which lifts its post-loss freeze on the first + /// occurrence. Grey frames, presented, with the freeze lifted. + /// + /// Distrust is deliberately NOT "unusable": ordinary prediction runs off the backend's own slot + /// INDEX, never the wire domain, so this costs nothing but the next anchor pick — which + /// declines and falls through to the (still coalesced, so still non-storming) keyframe path. + /// It is also self-correcting on all three backends: a slot re-marked with a fresh frame, or an + /// IDR flushing the DPB, restores trust within a few frames. So this can suppress RFI briefly, + /// never permanently. + /// + /// Default: no-op — the backends with no reference bookkeeping have no trust to withdraw. + fn distrust_references(&mut self) {} /// Escalate into a pipelined (two-thread) retrieve mode under sustained GPU contention — the /// encoder analog of the capturer depth escalation: AUs ride ~one loop tick behind (`poll` /// may return `None` while an encode is in flight) in exchange for capture/submit no longer diff --git a/crates/pf-encode/src/enc/linux/vulkan_video.rs b/crates/pf-encode/src/enc/linux/vulkan_video.rs index 36d48ad2..732eb5fe 100644 --- a/crates/pf-encode/src/enc/linux/vulkan_video.rs +++ b/crates/pf-encode/src/enc/linux/vulkan_video.rs @@ -3991,6 +3991,35 @@ impl Encoder for VulkanVideoEncoder { } } + /// Withdraw anchor trust from every resident reference (trait docs carry the why). + /// + /// The mechanism is this backend's half of the split `enc::rfi` documents: blank `slot_wire` + /// ONLY. `slot_poc` MUST keep naming every physically-resident DPB picture — it is what + /// [`build_h265_rps_s0`] retains the RPS from, and an RPS that stops naming a resident lets a + /// conforming decoder mark it "unused for reference" and reclaim it (8.3.2), so a later anchor + /// would reference a picture the client has already dropped. That is its own grey-screen bug, + /// documented on `build_h265_rps_s0`, and it is the exact failure this method exists to + /// prevent — so getting the two domains the wrong way round here would trade one for the other. + /// `slot_wire` is the RFI/loss domain; `slot_poc` is the reference-delta domain. + /// + /// `pending_loss` is deliberately left armed, matching this backend's decline arm: a stale arm + /// is re-resolved at frame-build, where the re-pick now finds nothing trusted and forces the + /// IDR that heals the stream. Clearing it here would ship an untagged plain P instead. + /// + /// Ordinary prediction is untouched — it runs off `prev_slot`, an index, not a wire. + fn distrust_references(&mut self) { + let trusted = self.slot_wire.iter().filter(|&&w| w >= 0).count(); + if trusted == 0 { + return; // already fully distrusted — nothing to log or clear + } + self.slot_wire.iter_mut().for_each(|w| *w = -1); + tracing::debug!( + trusted, + "vulkan-encode: client reported unrepaired damage — withdrawing RFI anchor trust from \ + every resident reference (prediction and the RPS are unaffected)" + ); + } + fn poll(&mut self) -> Result> { // Backpressure-drained frames (already read, oldest) come out first, then the oldest slot // still in flight — both in submission order. BLOCKING, per the depth-1 pump contract diff --git a/crates/pf-encode/src/enc/rfi.rs b/crates/pf-encode/src/enc/rfi.rs index 214a3543..048d24ee 100644 --- a/crates/pf-encode/src/enc/rfi.rs +++ b/crates/pf-encode/src/enc/rfi.rs @@ -175,4 +175,56 @@ mod tests { apply(&mut all, plan.tainted); assert_eq!(pick_anchor(&view(&all), 5), None); } + + /// `Encoder::distrust_references` — the OTHER way trust is withdrawn, and the one that needs no + /// loss range. The host calls it when the client reports damage the host did not repair (a + /// coalesced keyframe request, or an RFI anchor the client kept asking past): the sweep cannot + /// run there because a keyframe request carries no range, so every resident reference is + /// withdrawn wholesale instead. All three backends persist that through their own marker; what + /// the shared policy must guarantee is the consequence — the next pick finds nothing and + /// declines, so the caller keyframes instead of serving an anchor over unrepaired damage. + #[test] + fn distrusting_every_reference_makes_the_next_anchor_pick_decline() { + // A table with plenty of pre-loss candidates: without the withdrawal, wire 7 anchors. + let mut wires = [4i64, 5, 6, 7, -1, -1, -1, -1]; + assert_eq!( + pick_anchor(&view(&wires), 9), + Some((3, 7)), + "precondition: this table would happily anchor" + ); + + // The Vulkan mechanism (blank the wire) stands in for all three: AMF clears its mirror and + // QSV raises `ltr_tainted`, but each is filtered out of the trusted view identically — + // which is exactly what makes one pure policy serve three persistence schemes. + apply(&mut wires, u32::MAX); + assert_eq!( + pick_anchor(&view(&wires), 9), + None, + "every reference withdrawn → no anchor, caller falls through to its keyframe path" + ); + // And it holds for ANY later loss, not just this one — the point of persisting distrust. + assert_eq!(pick_anchor(&view(&wires), 100), None); + } + + /// The withdrawal must be temporary, or one coalesced keyframe request would cost a session its + /// RFI recovery for good and every later loss would ride the 20-40× IDR path. Each backend + /// restores trust the same way it always did — a slot re-marked with a fresh frame (and an IDR + /// flush, which empties the table first) — so a refilled slot anchors again. + #[test] + fn a_re_marked_slot_restores_anchor_trust_after_a_full_withdrawal() { + let mut wires = [4i64, 5, 6, 7, -1, -1, -1, -1]; + apply(&mut wires, u32::MAX); + assert_eq!(pick_anchor(&view(&wires), 20), None); + + // Encoding continues; the ring refills two slots with post-withdrawal frames. Those really + // are clean — the client's damage was repaired by the IDR the withdrawal forced — so they + // are legitimate anchors and the sweep must not keep rejecting them. + wires[0] = 14; + wires[1] = 15; + assert_eq!( + pick_anchor(&view(&wires), 20), + Some((1, 15)), + "a re-marked slot is trusted again — the suppression is a few frames, not the session" + ); + } } diff --git a/crates/pf-encode/src/enc/windows/amf.rs b/crates/pf-encode/src/enc/windows/amf.rs index a11e2c56..879451c8 100644 --- a/crates/pf-encode/src/enc/windows/amf.rs +++ b/crates/pf-encode/src/enc/windows/amf.rs @@ -2010,6 +2010,30 @@ impl Encoder for AmfEncoder { } } + /// Withdraw anchor trust from every live LTR (trait docs carry the why). + /// + /// This backend's mechanism, unchanged from the sweep's: distrust = clear the mirror slot. + /// Dropped slots stay dropped and the marking cadence re-marks a clean frame within ~1/4 s, so + /// the suppression is brief by construction. + /// + /// `pending_force` is cleared with them, matching the decline arm above: an un-consumed force + /// would otherwise point at a slot this call just distrusted, and the next submit would + /// force-reference it anyway — shipping the corruption tagged `recovery_anchor`, which is the + /// whole failure being closed. + fn distrust_references(&mut self) { + let live = self.ltr_slots.iter().filter(|m| m.is_some()).count(); + if live == 0 && self.pending_force.is_none() { + return; + } + self.ltr_slots = [None; NUM_LTR_SLOTS]; + self.pending_force = None; + tracing::debug!( + live, + "AMF LTR-RFI: client reported unrepaired damage — withdrawing anchor trust from every \ + live LTR (the marking cadence re-marks a clean frame within ~1/4 s)" + ); + } + fn caps(&self) -> EncoderCaps { EncoderCaps { // As Windows NVENC: the capturer composites; this backend never reads `frame.cursor`. diff --git a/crates/pf-encode/src/enc/windows/qsv.rs b/crates/pf-encode/src/enc/windows/qsv.rs index d2448dfe..19e9f719 100644 --- a/crates/pf-encode/src/enc/windows/qsv.rs +++ b/crates/pf-encode/src/enc/windows/qsv.rs @@ -1467,6 +1467,39 @@ impl Encoder for QsvEncoder { } } + /// Withdraw anchor trust from every live LTR (trait docs carry the why). + /// + /// This backend's mechanism, unchanged from the sweep's: distrust is the SEPARATE + /// `ltr_tainted` flag, never a cleared mirror slot. `ltr_slots` mirrors the HARDWARE DPB and + /// nulling an entry issues no VPL call, so the frame stays marked long-term in the encoder — + /// and the RejectedRefList built at submit only names `Some` slots, so a cleared mirror would + /// silently SKIP the very entry being distrusted and the recovery frame could still predict + /// from it. Taint keeps the mirror intact and the rejection reachable. + /// + /// The taint lifts itself: an IDR flush and a re-mark both clear it, so this suppresses RFI + /// for a few frames, never for the session. + /// + /// `pending_force` is cleared for the same reason as the decline arm above — an un-consumed + /// force would point at a slot this call just distrusted. + fn distrust_references(&mut self) { + let live = self + .ltr_slots + .iter() + .enumerate() + .filter(|&(slot, m)| m.is_some() && !self.ltr_tainted[slot]) + .count(); + if live == 0 && self.pending_force.is_none() { + return; + } + self.ltr_tainted = [true; NUM_LTR_SLOTS]; + self.pending_force = None; + tracing::debug!( + live, + "QSV LTR-RFI: client reported unrepaired damage — withdrawing anchor trust from every \ + live LTR (cleared by the next re-mark or IDR flush)" + ); + } + fn caps(&self) -> EncoderCaps { EncoderCaps { // As Windows NVENC: the capturer composites; this backend never reads `frame.cursor`. diff --git a/crates/pf-encode/src/lib.rs b/crates/pf-encode/src/lib.rs index 918aefb8..5e6d89eb 100644 --- a/crates/pf-encode/src/lib.rs +++ b/crates/pf-encode/src/lib.rs @@ -274,6 +274,14 @@ impl Encoder for TrackedEncoder { fn invalidate_ref_frames(&mut self, first_frame: i64, last_frame: i64) -> bool { self.inner.invalidate_ref_frames(first_frame, last_frame) } + // Same trap class as `set_wire_chunking`, and the one where it would hurt most: unforwarded, + // the default no-op would leave every session serving RFI anchors over damage the client + // reported and the host never repaired — the failure this method exists to close, silently + // reintroduced by the wrapper. (The `every_encoder_method_is_forwarded` guard below catches + // it, which is exactly why that guard is there.) + fn distrust_references(&mut self) { + self.inner.distrust_references() + } // Forwarded for the same reason as `set_wire_chunking` below — the unforwarded default // (`false` = "backend can't pipeline, stop asking") silently killed the §7 LN3 contention // escalation for every session, since the host loop only ever holds the wrapped box. diff --git a/crates/punktfunk-host/src/native/stream.rs b/crates/punktfunk-host/src/native/stream.rs index 2bade60a..98a1a62a 100644 --- a/crates/punktfunk-host/src/native/stream.rs +++ b/crates/punktfunk-host/src/native/stream.rs @@ -2893,15 +2893,46 @@ pub(super) fn virtual_stream(ctx: SessionContext, prepared: Option 0; + tracing::debug!(rfi_unhealed, "forcing keyframe (client decode recovery)"); + if rfi_unhealed { + enc.distrust_references(); + } enc.request_keyframe(); last_forced_idr = Some(now); rfi_echo_swallowed = 0; // the IDR resets the episode — echoes of IT coalesce via the cooldown -- 2.54.0 From 89fd4c6f87f49ef78c959a59054b8d5362ec7c36 Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Sun, 16 Aug 2026 01:30:53 +0200 Subject: [PATCH 3/3] fix(client): teach the VAAPI AV1 sizing fixture about the clean bit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adding a public field breaks LITERAL constructors, and the only one outside pf-bitstream lives in a `cfg(test)` fixture in a crate that consumes the plan types through pf-vaadec's re-exports — so nothing on the macOS side and no single-crate test run could see it. The Linux `--all-targets` gate did. Vacuously `true`: the fixture codes a key frame, which predicts from nothing. It exists to exercise the sizing path (sequence max vs coded vs render), so the clean bit is incidental here — but it still has to state the honest value, because `false` is the answer that withholds a re-anchor. --- crates/pf-client-core/src/video_vaapi_native.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/crates/pf-client-core/src/video_vaapi_native.rs b/crates/pf-client-core/src/video_vaapi_native.rs index 51130618..0d876cd9 100644 --- a/crates/pf-client-core/src/video_vaapi_native.rs +++ b/crates/pf-client-core/src/video_vaapi_native.rs @@ -3060,6 +3060,11 @@ mod tests { picture: pf_vaadec::PicturePlanAv1 { frame_type: pf_vaadec::FrameTypeAv1::KeyFrame, is_key: true, + // Vacuously true for a key frame: it predicts from nothing. This fixture + // exists to exercise the SIZING path (sequence max vs coded vs render), so + // the clean bit is incidental here — but it must state the honest value, + // because `false` is the answer that withholds a re-anchor. + references_clean: true, show_frame: true, showable_frame: false, order_hint: 0, -- 2.54.0