From 26d06195f54d506c4804f56e219eb2415c253b52 Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Sun, 16 Aug 2026 01:14:30 +0200 Subject: [PATCH 01/11] 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, From 93c1ed072369cfac8d9c63aec2c5e3608defcdf1 Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Sun, 16 Aug 2026 01:14:47 +0200 Subject: [PATCH 02/11] 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 From 89fd4c6f87f49ef78c959a59054b8d5362ec7c36 Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Sun, 16 Aug 2026 01:30:53 +0200 Subject: [PATCH 03/11] 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, From 9e71cda298d18bfe43e1d2a0d594c0aa3619e2b5 Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Sun, 16 Aug 2026 09:56:34 +0200 Subject: [PATCH 04/11] fix(client-linux): the library grid never got the FlowBox recursion guard, so every game click overflowed the stack MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Clicking any game in the host library aborted punktfunk-client outright: thread 'main' has overflowed its stack fatal runtime error: stack overflow, aborting A click on a FlowBoxChild emits `child-activated` on the *FlowBox*, never the child's own `activate`, so a grid whose per-card handler hangs off `child.connect_activate()` has to bridge the two. The naive bridge is a cycle: FlowBoxChild's default `activate` handler re-emits `child-activated` on its parent, which re-enters the bridge, which activates the child again. 53c8eefa fixed exactly this for the host cards — but only in ui_hosts.rs. The library page had carried the same unguarded bridge since 7eea9836, and 883c3178 then added a third one for the launcher shelf. So the guard existed in one of three places, and both library grids still aborted on the first click. Rather than patch the two sites and leave the footgun loaded, the guarded bridge moves into ui_flow.rs and all three call sites go through it. The regression test moves with it, so it now covers the code both pages actually run instead of a hand-copied replica of it — which is why it never caught this. Verified in a Linux container (fmt / clippy -D warnings / build / test all green) with the display test executed for real against Xvfb rather than skipped. Removing the guard again reproduces the reported abort exactly (SIGABRT, stack overflow), so the test is known to catch the bug rather than merely pass. --- clients/linux/src/main.rs | 3 ++ clients/linux/src/ui_flow.rs | 70 +++++++++++++++++++++++++++++++++ clients/linux/src/ui_hosts.rs | 61 ++-------------------------- clients/linux/src/ui_library.rs | 11 ++---- 4 files changed, 81 insertions(+), 64 deletions(-) create mode 100644 clients/linux/src/ui_flow.rs diff --git a/clients/linux/src/main.rs b/clients/linux/src/main.rs index 7a90ff0c..dd7b5496 100644 --- a/clients/linux/src/main.rs +++ b/clients/linux/src/main.rs @@ -22,6 +22,9 @@ mod cli; mod shortcuts; #[cfg(target_os = "linux")] mod spawn; +// The guarded FlowBox `child-activated → activate` bridge every card grid needs. +#[cfg(target_os = "linux")] +mod ui_flow; #[cfg(target_os = "linux")] mod ui_hosts; #[cfg(target_os = "linux")] diff --git a/clients/linux/src/ui_flow.rs b/clients/linux/src/ui_flow.rs new file mode 100644 index 00000000..a8626b34 --- /dev/null +++ b/clients/linux/src/ui_flow.rs @@ -0,0 +1,70 @@ +//! One shared fix for a GTK4 footgun every card grid in this shell walks into. +//! +//! A pointer click (and keyboard activate) on a [`gtk::FlowBoxChild`] emits +//! `child-activated` on the *FlowBox*, never the child's own `activate` signal — so a +//! grid whose per-card handler hangs off `child.connect_activate()` has to bridge the +//! one to the other. The naive bridge is a stack overflow: `FlowBoxChild`'s default +//! `activate` handler re-emits `child-activated` on its parent, which calls the bridge, +//! which activates the child again, forever. +//! +//! [`bridge_child_activation`] is that bridge with the re-entrancy guard that breaks the +//! cycle. Use it for every `child-activated → child.activate()` hop; do not hand-roll it, +//! since a bare `flow.connect_child_activated(|_, c| c.activate())` aborts the process on +//! the first click and looks perfectly reasonable in review. + +use gtk::prelude::*; + +/// Bridge a FlowBox's `child-activated` to the activated child's own `activate` signal, +/// exactly once per click. The re-entrant emission the child's default handler bounces +/// back is swallowed rather than recursed into. +pub(crate) fn bridge_child_activation(flow: >k::FlowBox) { + let activating = std::cell::Cell::new(false); + flow.connect_child_activated(move |_, child| { + if activating.replace(true) { + return; + } + child.activate(); + activating.set(false); + }); +} + +#[cfg(test)] +mod tests { + use super::bridge_child_activation; + use gtk::prelude::*; + use std::cell::Cell; + use std::rc::Rc; + + // Reproduces the exact FlowBox/FlowBoxChild wiring the card grids use: the bridge + // calls `child.activate()`, whose own default handler re-emits `child-activated` — + // that ping-pong recursed forever (a real stack overflow on every card click/Enter, + // reported on the hosts page and then again on the library page) until the + // re-entrancy guard landed here, where both pages share it. + #[test] + #[ignore = "needs a Wayland/X display"] + fn flow_box_activation_bridge_does_not_recurse() { + assert!(gtk::init().is_ok(), "no display"); + + let flow = gtk::FlowBox::builder() + .selection_mode(gtk::SelectionMode::None) + .activate_on_single_click(true) + .build(); + bridge_child_activation(&flow); + + let child = gtk::FlowBoxChild::new(); + flow.insert(&child, -1); + let fired = Rc::new(Cell::new(0u32)); + { + let fired = fired.clone(); + child.connect_activate(move |_| fired.set(fired.get() + 1)); + } + + flow.emit_by_name::<()>("child-activated", &[&child]); + + assert_eq!( + fired.get(), + 1, + "the per-card handler should fire exactly once" + ); + } +} diff --git a/clients/linux/src/ui_hosts.rs b/clients/linux/src/ui_hosts.rs index 76e12576..28484bc0 100644 --- a/clients/linux/src/ui_hosts.rs +++ b/clients/linux/src/ui_hosts.rs @@ -781,18 +781,11 @@ impl SimpleComponent for HostsPage { // A pointer click (and keyboard activate) emits `child-activated` on the // *FlowBox*, never the child's own `activate` signal — bridge it back to the - // child, where each card wires its connect handler. The re-entrancy flag breaks - // the child-activated ↔ activate ping-pong that otherwise recurses forever - // (a real stack overflow on every card click; see the ignored display test). + // child, where each card wires its connect handler. The guard inside the bridge + // breaks the child-activated ↔ activate ping-pong that otherwise recurses forever + // (a real stack overflow on every card click; see `ui_flow`'s display test). for flow in [saved.widget(), discovered.widget()] { - let activating = std::cell::Cell::new(false); - flow.connect_child_activated(move |_, child| { - if activating.replace(true) { - return; - } - child.activate(); - activating.set(false); - }); + crate::ui_flow::bridge_child_activation(flow); } // Shown under the discovered heading while no (unsaved) advert is live yet. @@ -1466,49 +1459,3 @@ impl HostsPage { dialog.present(Some(&self.widgets.stack)); } } - -#[cfg(test)] -mod tests { - use adw::prelude::*; - use std::cell::Cell; - use std::rc::Rc; - - // Reproduces the exact FlowBox/FlowBoxChild wiring from `init()`: `child-activated` - // bridges to `child.activate()`, whose own default handler re-emits - // `child-activated` — that ping-pong recursed forever (stack overflow on every - // host-card click/Enter) until the re-entrancy guard was added. - #[test] - #[ignore = "needs a Wayland/X display"] - fn flow_box_activation_bridge_does_not_recurse() { - assert!(gtk::init().is_ok(), "no display"); - - let flow = gtk::FlowBox::builder() - .selection_mode(gtk::SelectionMode::None) - .activate_on_single_click(true) - .build(); - let activating = Cell::new(false); - flow.connect_child_activated(move |_, child| { - if activating.replace(true) { - return; - } - child.activate(); - activating.set(false); - }); - - let child = gtk::FlowBoxChild::new(); - flow.insert(&child, -1); - let fired = Rc::new(Cell::new(0u32)); - { - let fired = fired.clone(); - child.connect_activate(move |_| fired.set(fired.get() + 1)); - } - - flow.emit_by_name::<()>("child-activated", &[&child]); - - assert_eq!( - fired.get(), - 1, - "the per-card handler should fire exactly once" - ); - } -} diff --git a/clients/linux/src/ui_library.rs b/clients/linux/src/ui_library.rs index 7ec5d6d3..6574ab0b 100644 --- a/clients/linux/src/ui_library.rs +++ b/clients/linux/src/ui_library.rs @@ -116,10 +116,9 @@ fn build( .valign(gtk::Align::Start) .build(); // Click/keyboard activation fires `child-activated` on the FlowBox, not the child's own - // `activate` — bridge it so each poster's connect handler (below) runs on click. - flow.connect_child_activated(|_, child| { - child.activate(); - }); + // `activate` — bridge it so each poster's connect handler (below) runs on click. The + // bridge must be the guarded one: bare, it recurses until the stack overflows. + crate::ui_flow::bridge_child_activation(&flow); // The launcher shelf: same tile geometry as the games grid, its own FlowBox so the two // groups never interleave and each wraps on its own. let launcher_flow = gtk::FlowBox::builder() @@ -132,9 +131,7 @@ fn build( .row_spacing(18) .valign(gtk::Align::Start) .build(); - launcher_flow.connect_child_activated(|_, child| { - child.activate(); - }); + crate::ui_flow::bridge_child_activation(&launcher_flow); let launchers_heading = gtk::Label::new(Some("Launchers")); launchers_heading.add_css_class("pf-group-heading"); launchers_heading.set_halign(gtk::Align::Start); From f5f11f84f0d0e3faaa4d23b42dd9b1f298bea95f Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Sun, 16 Aug 2026 10:00:53 +0200 Subject: [PATCH 05/11] fix(plugin-kit): the Steam art scan knew one filename and one layout, so most covers were never found MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Forza Horizon 6 shows a banner where its cover should be. The cover is on disk the whole time: `librarycache/2483190/711e39.../library_capsule.jpg`, 300x450, the exact asset Steam itself draws. The scan never looked for that name, fell back to the flat CDN URL for `library_600x900.jpg`, which 404s for this appid, and the client then walked its candidate list down to the header — a 460x215 banner in a 2:3 poster slot. Measured against a real 779-app `appcache/librarycache` (the .41 box), the scan was missing far more than one title. Three findings, each independent: * `library_capsule.jpg` is the newer name for the 2:3 cover. 46 appids carry only that name; none carry both it and `library_600x900.jpg`. * `header.jpg` is the newer name for the header. 594 appids carry it, 122 carry `library_header.jpg`, and again no appid carries both — so the one name we knew covered 16% of them. * The `/` layout, with no hash dir in between, is the MAJORITY: 623 of 779 appids. `findLocalArtFile` walked only `//` and the oldest `_` form, so it saw none of them. The renamed files are the same assets — every `library_capsule.jpg` in that cache measures 300x450, the same as every `library_600x900.jpg`, and both header spellings measure 460x215 — so this is purely about knowing to look. Simulating the resolver over that cache, per-appid art found locally: portrait 25 -> 328 hero 75 -> 323 header 86 -> 716 logo 66 -> 295 None of this was visible before because a title that resolves no local file still gets a CDN URL, and for anything Valve has not re-hashed that URL works. It is the newer titles — the ones whose flat CDN URL 404s — that lose their art outright, which is why this reads as "some games" rather than "the library". --- plugin-kit/src/library/parsers/art.ts | 40 +++++++++++++++--- plugin-kit/test/library-parsers.test.ts | 56 +++++++++++++++++++++++++ 2 files changed, 90 insertions(+), 6 deletions(-) diff --git a/plugin-kit/src/library/parsers/art.ts b/plugin-kit/src/library/parsers/art.ts index 90cbdc88..35cdba03 100644 --- a/plugin-kit/src/library/parsers/art.ts +++ b/plugin-kit/src/library/parsers/art.ts @@ -54,21 +54,44 @@ export const steamCdnUrl = ( return `https://cdn.cloudflare.steamstatic.com/steam/apps/${appid}/${file}`; }; -/** Filenames Steam's local `librarycache` uses per kind, in preference order (2x is sharper). */ +/** + * Filenames Steam's local `librarycache` uses per kind, in preference order (2x is sharper). + * + * Two spellings per kind, because Steam renamed these assets and one cache holds both eras side by + * side — on a 779-app cache, 594 appids carry `header.jpg` and 122 carry `library_header.jpg`, and + * NO appid carries both. Same story for the cover: 46 appids have only `library_capsule.jpg`. The + * renamed files are the same assets, byte-for-byte the same shapes (cover 300×450, header 460×215), + * so which name wins is cosmetic — but knowing only one name loses the art outright. + * + * Missing the cover is the one that shows: the fallback is then the flat CDN URL, which 404s for + * anything Valve has re-hashed, so the client walks on to the header and draws a BANNER in a 2:3 + * poster slot (Forza Horizon 6 / appid 2483190 is the reference case). + */ const localFilenames = (kind: ArtKind): string[] => kind === "portrait" - ? ["library_600x900_2x.jpg", "library_600x900.jpg"] + ? ["library_600x900_2x.jpg", "library_600x900.jpg", "library_capsule.jpg"] : kind === "hero" ? ["library_hero.jpg"] : kind === "logo" ? ["logo.png"] : // Steam's local cache names the header asset differently from the store CDN's - // `header.jpg` — this trips everyone once. - ["library_header.jpg"]; + // `header.jpg` — this trips everyone once. Newer entries use the CDN's name, so + // both belong here. + ["library_header.jpg", "header.jpg"]; /** - * This kind's file under one Steam root's `appcache/librarycache///`, or `undefined`. - * Steam reuses one hash dir per asset version, so there is normally exactly one candidate. + * This kind's file under one Steam root's `appcache/librarycache/`, or `undefined`. + * + * Three layouts, all of them live in the same cache at the same time — a title's art is in exactly + * one of them, so all three have to be checked or its cover is simply not found: + * + * 1. `//` — per-asset-version hash dir. Steam reuses one hash dir per version, + * so there is normally exactly one candidate. Checked first: where a title has been re-fetched + * into this layout, this is the copy Steam itself is displaying. + * 2. `/` — straight in the appid dir, and the MAJORITY case (623 of 779 appids on the + * reference cache). A hash-dir-only walk misses every one of them, which stayed invisible only + * because the flat CDN URL those titles fall back to still resolves for older appids. + * 3. `_` flat in `librarycache/` — the oldest layout. */ export const findLocalArtFile = ( root: string, @@ -82,6 +105,11 @@ export const findLocalArtFile = ( if (isFile(p)) return p; } } + // Layout 2: no hash dir, the asset sits directly in the appid dir. + for (const name of localFilenames(kind)) { + const p = path.join(base, name); + if (isFile(p)) return p; + } // Older Steam wrote the files directly under `librarycache/` with the appid in the name. for (const name of localFilenames(kind)) { const flat = path.join( diff --git a/plugin-kit/test/library-parsers.test.ts b/plugin-kit/test/library-parsers.test.ts index 55ead4bf..b4b3e983 100644 --- a/plugin-kit/test/library-parsers.test.ts +++ b/plugin-kit/test/library-parsers.test.ts @@ -280,6 +280,62 @@ describe("art locations", () => { fs.rmSync(dir, { recursive: true, force: true }); }); + test("finds a cover cached under Steam's newer `library_capsule` name", () => { + // The bug this pins: appid 2483190 (Forza Horizon 6) caches its 300×450 cover as + // `library_capsule.jpg`, the flat CDN URL for its `library_600x900.jpg` 404s, and the client + // therefore fell through to the header and drew a banner in a 2:3 poster slot. + const dir = tmp("art-capsule"); + const hashDir = path.join( + dir, + "appcache", + "librarycache", + "2483190", + "711e", + ); + fs.mkdirSync(hashDir, { recursive: true }); + fs.writeFileSync(path.join(hashDir, "library_capsule.jpg"), "x"); + expect(findLocalArtFile(dir, 2483190, "portrait")).toBe( + path.join(hashDir, "library_capsule.jpg"), + ); + fs.rmSync(dir, { recursive: true, force: true }); + }); + + test("finds art stored straight in the appid dir, with no hash dir", () => { + // The majority layout — 623 of 779 appids on the reference cache. A hash-dir-only walk finds + // none of it and silently falls back to a CDN URL that 404s for anything re-hashed. + const dir = tmp("art-flat"); + const appDir = path.join(dir, "appcache", "librarycache", "813230"); + fs.mkdirSync(appDir, { recursive: true }); + fs.writeFileSync(path.join(appDir, "library_600x900.jpg"), "x"); + fs.writeFileSync(path.join(appDir, "header.jpg"), "x"); + expect(findLocalArtFile(dir, 813230, "portrait")).toBe( + path.join(appDir, "library_600x900.jpg"), + ); + // `header.jpg` is the CDN's name, but the local cache uses it too for newer entries — the + // two spellings are the same 460×215 asset and never appear together for one appid. + expect(findLocalArtFile(dir, 813230, "header")).toBe( + path.join(appDir, "header.jpg"), + ); + expect(findLocalArtFile(dir, 813230, "hero")).toBeUndefined(); + fs.rmSync(dir, { recursive: true, force: true }); + }); + + test("a hash dir wins over a loose file of the same kind", () => { + // No appid on the reference cache carries both, so this is only about which copy is the + // current one if Steam ever leaves the old layout behind: the hash dir is what it re-fetches + // into, so that is the copy it is itself displaying. + const dir = tmp("art-both"); + const appDir = path.join(dir, "appcache", "librarycache", "570"); + const hashDir = path.join(appDir, "abc123"); + fs.mkdirSync(hashDir, { recursive: true }); + fs.writeFileSync(path.join(appDir, "library_600x900.jpg"), "x"); + fs.writeFileSync(path.join(hashDir, "library_600x900.jpg"), "x"); + expect(findLocalArtFile(dir, 570, "portrait")).toBe( + path.join(hashDir, "library_600x900.jpg"), + ); + fs.rmSync(dir, { recursive: true, force: true }); + }); + test("fileUrl produces the host's local-art contract shape", () => { const u = fileUrl(path.join(path.sep, "home", "u", "My Games", "c.jpg")); expect(u.startsWith("file:///")).toBe(true); From 59fc9294ea6042ee2fa9eafbd6fe757d34877820 Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Sun, 16 Aug 2026 10:00:59 +0200 Subject: [PATCH 06/11] fix(client/apple): a fallback banner sized the poster tile it was supposed to be cropped into MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reported as the library "breaking" on iOS whenever a title showed a banner instead of a cover: the tile expanded out of its bounds. `PosterImage` already meant to prevent exactly this — its own comment said a banner "would otherwise report a much wider intrinsic size than the card and overflow into neighboring cards" — and the guard it used, a flexible frame plus `.clipped()`, does not do it. `scaledToFill` answers a proposal with a size that COVERS it, i.e. larger; `.frame(maxWidth: .infinity, maxHeight: .infinity)` then clamps that answer to infinity, which is no clamp at all, so the image's size propagates straight up through `aspectRatio(2/3, .fit)` and decides the tile. Measured offscreen with ImageRenderer, one LazyVGrid column at 170pt: 460x215 header, before poster 1750.9 x 818.4 tile 545.6 x 255.0 460x215 header, after poster 170.0 x 255.0 tile 170.0 x 255.0 300x450 cover, either poster 170.0 x 255.0 tile 170.0 x 255.0 A 545pt tile in a 170pt column is what the report is describing, and the cover case coming out right either way is why this only ever showed on the titles whose cover was missing. The fix gives the sizing role to something that has no opinion about it: `Color.clear` takes the proposal, and the art rides as its overlay, where it can be drawn but never measured. `LibraryCoverflowView` reuses `PosterImage` directly, so it is fixed by the same change. --- .../PunktfunkClient/Home/LibraryWidgets.swift | 24 ++++++++++++++----- 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/clients/apple/Sources/PunktfunkClient/Home/LibraryWidgets.swift b/clients/apple/Sources/PunktfunkClient/Home/LibraryWidgets.swift index 475b240b..f5dae289 100644 --- a/clients/apple/Sources/PunktfunkClient/Home/LibraryWidgets.swift +++ b/clients/apple/Sources/PunktfunkClient/Home/LibraryWidgets.swift @@ -63,9 +63,10 @@ private extension Image { /// Sequentially tries cover-art URLs over `loader` (so a paired client can reach the host's own /// art proxy, not just public CDNs — see `LibraryArtLoader`), advancing past any that fail to /// load, then a placeholder. The loaded image is hard-clipped to fill the card's actual frame -/// regardless of its own aspect ratio: a portrait capsule fills it as intended, but a fallback -/// banner (wide hero/header art, used when a title has no portrait capsule) would otherwise report -/// a much wider intrinsic size than the card and overflow into neighboring cards. Not `private` — +/// regardless of its own aspect ratio: a portrait capsule fills it as intended, and a fallback +/// banner (wide hero/header art, used when a title has no portrait capsule) is cropped to the same +/// tile rather than allowed to size it — see the `Color.clear` in `body` for why that takes more +/// than a `.frame(maxWidth:)` and a `.clipped()`. Not `private` — /// the gamepad coverflow (`LibraryCoverflowView`) reuses it directly rather than re-fetching art. struct PosterImage: View { let candidates: [URL] @@ -84,9 +85,20 @@ struct PosterImage: View { var body: some View { Group { if let image { - Image(platformImage: image) - .resizable() - .scaledToFill() + // `Color.clear` is what takes the proposed size; the art rides along as its + // overlay, where it can be DRAWN but never MEASURED. Handing the image the sizing + // role instead is what let a fallback banner escape the tile: `scaledToFill` + // reports a size that covers the proposal, and the flexible frame below clamps it + // to `.infinity` — i.e. not at all. Measured offscreen, a 460×215 `header.jpg` in a + // 170pt grid column resolved the tile to 545×255 and overran its neighbours, while + // a 300×450 cover in the same chain came out correct — which is why this only ever + // showed on the titles whose cover was missing. + Color.clear + .overlay { + Image(platformImage: image) + .resizable() + .scaledToFill() + } .transition(.opacity) } else if index < candidates.count { ZStack { placeholder; ProgressView() } From 5c746fbea58cce04da394680ff5139560dfa7153 Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Sun, 16 Aug 2026 10:01:08 +0200 Subject: [PATCH 07/11] =?UTF-8?q?chore(plugin-kit):=200.4.2=20=E2=80=94=20?= =?UTF-8?q?publish=20the=20art=20scan=20fix,=20which=20no=20plugin=20can?= =?UTF-8?q?=20pick=20up=20otherwise?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Steam scanner is `@punktfunk/plugin-steam`, in its own repo, and it builds with `--external '@punktfunk/*'` — so `findLocalArtFile` is resolved from the registry at install time, not bundled. Until the kit cuts a version, every host keeps resolving 0.4.1 and keeps missing the covers. Same shape as the `icon` field at 0.4.1: kit change, out-of-tree producers, and nothing in this repo says so. Nothing but the version moves here. --- plugin-kit/package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugin-kit/package.json b/plugin-kit/package.json index eb8fa5d3..689e5a27 100644 --- a/plugin-kit/package.json +++ b/plugin-kit/package.json @@ -1,6 +1,6 @@ { "name": "@punktfunk/plugin-kit", - "version": "0.4.1", + "version": "0.4.2", "description": "Effect-based framework for punktfunk plugins: lifecycle runtime, config/state, sync engine, UI serving, CLI scaffold, and browser helpers.", "type": "module", "license": "MIT OR Apache-2.0", From b949bf20621ad079b34beca14c51f04d59019196 Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Sun, 16 Aug 2026 10:09:53 +0200 Subject: [PATCH 08/11] feat(clients): the game library was unreachable in Android's touch UI, and no library view could copy a title's link MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two gaps, both about the library: * On Android, the shelf was reachable only with Y from a console tile — a finger has no Y, so the touch home had no route to it at all. LibraryScreen grows a second presentation (the poster GRID the Apple, GTK and Windows shells already draw) and a card's overflow grows "Browse library…", behind the same experimental toggle as everywhere else. * No library view offered "Copy link", although design/client-deep-links.md §5 names the library game context menu as an attach point beside the host card and the pinned card. Every shell now hands out a title's own URL — the same self-emitted form a host card gives, plus that game's launch= id, so pasting it into Playnite or a Stream Deck macro boots straight into the title. The gesture follows each surface: a context menu on the touch/mouse grids (Apple, Android, GTK; Windows uses a tile overflow button, since the reactor hangs menu_flyout off buttons only), and the X face button plus a legend entry on the three gamepad coverflows, where there is no right-click. --- .../src/main/kotlin/io/unom/punktfunk/App.kt | 38 ++ .../kotlin/io/unom/punktfunk/ConnectGrid.kt | 15 + .../kotlin/io/unom/punktfunk/ConnectScreen.kt | 20 +- .../kotlin/io/unom/punktfunk/LibraryScreen.kt | 481 ++++++++++++++++-- .../main/kotlin/io/unom/punktfunk/Links.kt | 34 ++ .../Home/LibraryCoverflowView.swift | 33 ++ .../PunktfunkClient/Home/LibraryView.swift | 41 +- clients/linux/src/app.rs | 7 + clients/linux/src/ui_library.rs | 70 ++- clients/windows/src/app/library.rs | 59 +++ crates/pf-console-ui/src/screens.rs | 39 +- crates/pf-console-ui/src/screens/library.rs | 33 +- 12 files changed, 779 insertions(+), 91 deletions(-) create mode 100644 clients/android/app/src/main/kotlin/io/unom/punktfunk/Links.kt diff --git a/clients/android/app/src/main/kotlin/io/unom/punktfunk/App.kt b/clients/android/app/src/main/kotlin/io/unom/punktfunk/App.kt index 63253bac..0b1480e4 100644 --- a/clients/android/app/src/main/kotlin/io/unom/punktfunk/App.kt +++ b/clients/android/app/src/main/kotlin/io/unom/punktfunk/App.kt @@ -54,6 +54,7 @@ import io.unom.punktfunk.kit.link.DeepLinkResult import io.unom.punktfunk.kit.link.DeepLinks import io.unom.punktfunk.kit.link.HostResolution import io.unom.punktfunk.kit.SessionEndReason +import io.unom.punktfunk.kit.security.KnownHost import io.unom.punktfunk.kit.security.KnownHostStore import io.unom.punktfunk.models.ActiveSession import io.unom.punktfunk.models.LibraryReturn @@ -76,6 +77,11 @@ fun App(forceGamepadUi: Boolean = false) { // navigation state does not outlive the stream. Cleared once the shell has consumed it, so a // later manual Back out of the library is not undone by a stale value. var reopenLibrary by remember { mutableStateOf(null) } + // Which host's game library the TOUCH shell is showing, and which of its pinned cards opened it + // (null = the host's own card). Held here beside `tab` rather than inside the tab content: it is + // a PUSH over the whole shell, and a `remember` down in ConnectScreen would not survive the + // stream that a launch off the shelf starts — which is exactly what `reopenLibrary` restores. + var touchLibrary by remember { mutableStateOf?>(null) } // Console (gamepad) mode mirrors the Apple client: the setting AND (its mode says Always OR a // pad is attached OR this is a TV OR the dev force flag). Flips live as controllers @@ -116,6 +122,21 @@ fun App(forceGamepadUi: Boolean = false) { } } + // The touch shell's half of "come back to the library this game was launched from" — the console + // shell consumes the same intent on its way in (see GamepadShell). Gated on which shell is up so + // exactly one of the two ever claims it, and cleared here so a later manual Back stays backed + // out. A host forgotten while the game ran simply leaves us on the host grid. + LaunchedEffect(reopenLibrary, gamepadUi) { + if (gamepadUi) return@LaunchedEffect + val (id, pinId) = reopenLibrary ?: return@LaunchedEffect + KnownHostStore(context).all().firstOrNull { it.id == id }?.let { kh -> + // A pin unpinned while the game was running is no longer a shelf: fall back to the + // host's own, rather than a card that no longer exists. + touchLibrary = kh to pinId?.takeIf { it in kh.pinnedProfileIds } + } + reopenLibrary = null + } + // The console backdrop's colour family, published once from the live settings rather than // threaded through every screen that draws a backdrop. Because it is read from the SAME // `settings` state the gamepad settings screen writes, stepping the Background row recolours @@ -158,6 +179,20 @@ fun App(forceGamepadUi: Boolean = false) { reopenLibrary = reopenLibrary, onReopenLibraryHandled = { reopenLibrary = null }, ) + } else if (touchLibrary != null) { + // The touch shell's library is a PUSHED screen, not a tab: it belongs to one host, and a + // third permanent tab for something you reach from a card would be a nav item that is + // meaningless until you pick one. So it takes the whole window (bar included, like the + // console shell's does) and Back — the arrow or the system gesture — returns to the grid. + val (host, pinId) = touchLibrary!! + LibraryScreen( + host = host, + settings = settings, + onLaunched = { session = it }, + onBack = { touchLibrary = null }, + pinnedProfileId = pinId, + console = false, + ) } else { // Adaptive nav: a bottom bar on phones; on tablets / large windows a side NavigationRail // with its items centred vertically (the common Android tablet idiom, mirroring iPad's @@ -193,6 +228,9 @@ fun App(forceGamepadUi: Boolean = false) { onSettingsChange = { settings = it; settingsStore.save(it) }, deepLink = pendingLink, onDeepLinkHandled = { activity?.pendingDeepLink = null }, + // "Browse library…" in a card's overflow — the touch route to the shelf + // the console shell reaches with Y. + onOpenLibrary = { kh, pinId -> touchLibrary = kh to pinId }, ) Tab.Settings -> SettingsScreen( initial = settings, diff --git a/clients/android/app/src/main/kotlin/io/unom/punktfunk/ConnectGrid.kt b/clients/android/app/src/main/kotlin/io/unom/punktfunk/ConnectGrid.kt index ed8155df..bf628d85 100644 --- a/clients/android/app/src/main/kotlin/io/unom/punktfunk/ConnectGrid.kt +++ b/clients/android/app/src/main/kotlin/io/unom/punktfunk/ConnectGrid.kt @@ -82,6 +82,14 @@ internal fun ConnectGrid( onSpeedTest: (KnownHost) -> Unit, onCopyLink: (KnownHost, StreamProfile?) -> Unit, onTogglePin: (KnownHost, StreamProfile) -> Unit, + /** The experimental game-library toggle — off hides "Browse library…" everywhere. */ + libraryEnabled: Boolean, + /** + * Open this card's game library. The second argument is the shelf's pinned profile id, exactly + * as [onConnect] takes the card's one-off: browsing IS this card's connect with a title picked + * first, so a pinned card's shelf launches with that card's profile. + */ + onBrowseLibrary: (KnownHost, StreamProfile?) -> Unit, onRescan: () -> Unit, onAddHost: () -> Unit, ) { @@ -90,6 +98,13 @@ internal fun ConnectGrid( // "Connect with" is a ONE-OFF on every card: it never rebinds the host, which is why rebinding // lives in the Edit sheet instead. fun hostMenu(kh: KnownHost, pin: StreamProfile?): List = buildList { + // Browsing IS a connect-shaped action — this card's connect with a title picked first — so + // a PINNED card offers it too, and its shelf launches with that card's profile. Without it + // the touch home had no route to the library at all: the console shell reaches it with Y + // from a tile, and a finger has no Y. + if (libraryEnabled) { + add(HostMenuItem("Browse library…") { onBrowseLibrary(kh, pin) }) + } if (pin == null) { add(HostMenuItem("Network speed test") { onSpeedTest(kh) }) } diff --git a/clients/android/app/src/main/kotlin/io/unom/punktfunk/ConnectScreen.kt b/clients/android/app/src/main/kotlin/io/unom/punktfunk/ConnectScreen.kt index 56b6fe0c..5a388948 100644 --- a/clients/android/app/src/main/kotlin/io/unom/punktfunk/ConnectScreen.kt +++ b/clients/android/app/src/main/kotlin/io/unom/punktfunk/ConnectScreen.kt @@ -1,8 +1,6 @@ package io.unom.punktfunk import android.Manifest -import android.content.ClipData -import android.content.ClipboardManager import android.content.Context import android.content.Intent import android.content.pm.PackageManager @@ -96,12 +94,13 @@ fun ConnectScreen( // screen that can land in the defaults layer (design/client-settings-profiles.md §5.3). onSettingsChange: (Settings) -> Unit = {}, // Console (gamepad) mode: render the host carousel instead of the touch grid, sharing all of this - // screen's connect/trust/discovery logic. [onOpenSettings]/[onOpenLibrary] are the X/Y actions the - // gamepad shell owns (the touch UI reaches Settings via the bottom bar and has no library button). + // screen's connect/trust/discovery logic. [onOpenSettings] is the console's X action (the touch + // UI reaches Settings via the bottom bar). gamepadUi: Boolean = false, onOpenSettings: () -> Unit = {}, // (host, pinned profile id) — a pinned host+profile card opens ITS shelf, and the id is the // one-off every launch off that shelf runs with (design §5.2a). Null = the host's own tile. + // BOTH homes raise it: Y on a console tile, and "Browse library…" in a touch card's overflow. onOpenLibrary: (KnownHost, String?) -> Unit = { _, _ -> }, navGate: Boolean = true, // false while the console home is cross-fading out // A `punktfunk://` URL to route (design/client-deep-links.md §3). This screen owns it because @@ -635,15 +634,8 @@ fun ConnectScreen( // host's binding, exactly like a tap on it does. fun copyLink(kh: KnownHost, pin: StreamProfile?) { val url = DeepLinks.forHost(kh, profile = pin?.id).toUrl() - val clipboard = context.getSystemService(Context.CLIPBOARD_SERVICE) as? ClipboardManager - val copied = clipboard != null && runCatching { - clipboard.setPrimaryClip(ClipData.newPlainText("Punktfunk link", url)) - }.isSuccess - // Android 13 draws its own clipboard confirmation, and stacking a second one on top of it is - // the platform's own documented anti-pattern. Below it nothing visible happens at all unless - // we say so — a silent menu item reads as a broken one. - if (copied && Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) return - val message = if (copied) "Link copied." else "Couldn't copy the link to the clipboard." + val copied = putLinkOnClipboard(context, url) + val message = linkCopyMessage(copied) ?: return // The console home renders neither the notice nor the status banner, so there it has to be a // toast; the touch grid has both, and a success dressed as an error banner is a small lie. when { @@ -823,6 +815,8 @@ fun ConnectScreen( onSpeedTest = { kh -> startSpeedTest(HostCardEntry(kh, null)) }, onCopyLink = { kh, pin -> copyLink(kh, pin) }, onTogglePin = { kh, p -> togglePin(kh, p) }, + libraryEnabled = settings.libraryEnabled, + onBrowseLibrary = { kh, pin -> onOpenLibrary(kh, pin?.id) }, onRescan = { discovery.restart() }, onAddHost = { showManualSheet = true }, ) diff --git a/clients/android/app/src/main/kotlin/io/unom/punktfunk/LibraryScreen.kt b/clients/android/app/src/main/kotlin/io/unom/punktfunk/LibraryScreen.kt index eb2c4956..9b16df0a 100644 --- a/clients/android/app/src/main/kotlin/io/unom/punktfunk/LibraryScreen.kt +++ b/clients/android/app/src/main/kotlin/io/unom/punktfunk/LibraryScreen.kt @@ -2,25 +2,39 @@ package io.unom.punktfunk import android.widget.Toast import androidx.activity.compose.BackHandler +import androidx.compose.foundation.ExperimentalFoundationApi import androidx.compose.foundation.background import androidx.compose.foundation.border import androidx.compose.foundation.clickable +import androidx.compose.foundation.combinedClickable import androidx.compose.foundation.layout.Arrangement import androidx.compose.foundation.layout.Box import androidx.compose.foundation.layout.BoxWithConstraints import androidx.compose.foundation.layout.Column import androidx.compose.foundation.layout.PaddingValues +import androidx.compose.foundation.layout.Row +import androidx.compose.foundation.layout.aspectRatio import androidx.compose.foundation.layout.fillMaxSize import androidx.compose.foundation.layout.fillMaxWidth import androidx.compose.foundation.layout.height import androidx.compose.foundation.layout.padding import androidx.compose.foundation.layout.size import androidx.compose.foundation.layout.width +import androidx.compose.foundation.lazy.grid.GridCells +import androidx.compose.foundation.lazy.grid.GridItemSpan +import androidx.compose.foundation.lazy.grid.LazyVerticalGrid +import androidx.compose.foundation.lazy.grid.items import androidx.compose.foundation.pager.HorizontalPager import androidx.compose.foundation.pager.PageSize import androidx.compose.foundation.pager.rememberPagerState +import androidx.compose.material.icons.Icons +import androidx.compose.material.icons.automirrored.filled.ArrowBack +import androidx.compose.material.icons.filled.Refresh import androidx.compose.material3.CircularProgressIndicator +import androidx.compose.material3.DropdownMenu +import androidx.compose.material3.DropdownMenuItem import androidx.compose.material3.Icon +import androidx.compose.material3.IconButton import androidx.compose.material3.MaterialTheme import androidx.compose.material3.Text import androidx.compose.runtime.Composable @@ -59,6 +73,7 @@ import coil.ImageLoader import coil.compose.AsyncImage import coil.request.ImageRequest import io.unom.punktfunk.components.launcherIcon +import io.unom.punktfunk.kit.link.DeepLinks import io.unom.punktfunk.kit.library.GameEntry import io.unom.punktfunk.kit.library.LibraryClient import io.unom.punktfunk.kit.library.LibraryResult @@ -75,9 +90,13 @@ import kotlin.math.sign import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.withContext -// The host game-library browser — the Android mirror of the Apple client's LibraryCoverflowView: -// a gamepad-driven poster coverflow (centered cover flat + prominent, neighbours receding on a 3D -// Y-tilt) fetched from the host's management API over mTLS. Reached with Y from a saved host. +// The host game-library browser — the Android mirror of the Apple client's LibraryView: ONE screen +// with two presentations of the same shelf, chosen the same way every other screen here chooses. +// The console (gamepad) one is a poster coverflow (centered cover flat + prominent, neighbours +// receding on a 3D Y-tilt), reached with Y from a saved host; the touch one is the poster GRID the +// Apple, GTK and Windows shells draw, reached from a host card's "Browse library…". Both fetch from +// the host's management API over mTLS, and everything data-shaped — the fetch, the states, the +// launch — is shared: only the arrangement differs, because only the input device does. private sealed class LibState { object Loading : LibState() @@ -100,13 +119,20 @@ fun LibraryScreen( * [ProfileStore.resolveFor] applies to every other connect. */ pinnedProfileId: String? = null, + /** + * Which presentation to draw: the console coverflow (default — this screen's original and only + * form) or the touch poster grid. The CALLER decides rather than this screen reading the + * gamepad setting itself, because the two shells reach it by different routes and each already + * knows which one it is; a screen that guessed could disagree with the shell that pushed it. + */ + console: Boolean = true, ) { - val ink = LocalGamepadInk.current BackHandler(onBack = onBack) val context = LocalContext.current val scope = rememberCoroutineScope() - val hazeState = remember { HazeState() } - val landscape = LocalConfiguration.current.orientation == Configuration.ORIENTATION_LANDSCAPE + // Bumped by the touch header's Reload — re-keys the fetch effect below without the screen + // having to be popped and re-pushed to try a flaky host again. + var reloadKey by remember { mutableIntStateOf(0) } var state by remember { mutableStateOf(LibState.Loading) } // A launch (connect) in flight: shows an overlay + gates the pad so a second press can't dial twice. var launching by remember { mutableStateOf(false) } @@ -121,7 +147,7 @@ fun LibraryScreen( // Keyed on the mgmt port too: a discovery tick can learn it after this screen is composed, and // the fetch must redo itself against the real port rather than stay on a stale 47990 failure. - LaunchedEffect(host.address, host.port, host.fpHex, host.effectiveMgmtPort) { + LaunchedEffect(host.address, host.port, host.fpHex, host.effectiveMgmtPort, reloadKey) { state = LibState.Loading state = withContext(Dispatchers.IO) { val id = runCatching { obtainIdentity(IdentityStore(context)) }.getOrNull() @@ -145,58 +171,122 @@ fun LibraryScreen( } } + // A pinned card's shelf says so, in the card's own `host · profile` shape: what a launch here + // will use is a property of the shelf, not something to remember from the tile two screens back. + val title = if (pinnedProfileId != null && profile != null) { + "${host.name} · ${profile.name} — Library" + } else { + "${host.name} — Library" + } + + // Dial the host over the same pinned mTLS trust, booting straight into this title (the host + // resolves `launch` = its library id). Shared by both presentations: a tap on a grid tile and A + // on a centred cover are the same act, and a launch that behaved differently between them would + // be a bug nobody could see until they switched input device. + fun launch(identity: ClientIdentity, game: GameEntry) { + if (launching) return + launching = true + scope.launch { + val handle = connectToHost( + context, streamSettings, identity, + host.address, host.port, host.fpHex, launch = game.id, + ) + launching = false + if (handle != 0L) { + onLaunched( + ActiveSession( + handle, + streamSettings, + host.clipboardSync, + profileName = profile?.name, + hostId = host.id, + // Where to come back to when this game exits — this shelf, pin and all, + // not the host's default one. + launchedFromLibrary = true, + libraryProfileId = pinnedProfileId, + ), + ) + } else { + Toast.makeText( + context, + "Launch failed — check the host and try again.", + Toast.LENGTH_LONG, + ).show() + } + } + } + + // "Copy link" for one TITLE — the self-emitted form a host card already hands out (design/ + // client-deep-links.md §4/§5), plus this game's `launch=` id, so pasting the URL into Playnite + // or a Stream Deck macro boots straight into it. A shelf opened from a PINNED card copies that + // card's profile with it, because that combination is the thing being copied. + fun copyLink(game: GameEntry) { + val url = DeepLinks.forHost(host, launch = game.id, profile = pinnedProfileId).toUrl() + // A toast either way here: this screen renders neither the touch home's notice banner nor + // the console's status line, and both of its presentations are full-bleed over artwork. + linkCopyMessage(putLinkOnClipboard(context, url))?.let { + Toast.makeText(context, it, Toast.LENGTH_SHORT).show() + } + } + + if (console) { + ConsoleLibrary( + title = title, + state = state, + launching = launching, + navActive = navActive, + onBack = onBack, + onLaunch = ::launch, + onCopyLink = ::copyLink, + ) + } else { + TouchLibrary( + title = title, + state = state, + launching = launching, + onBack = onBack, + onReload = { reloadKey++ }, + onLaunch = ::launch, + onCopyLink = ::copyLink, + ) + } +} + +/** The console (gamepad) shelf: aurora, console header, coverflow, floating legend. */ +@Composable +private fun ConsoleLibrary( + title: String, + state: LibState, + launching: Boolean, + navActive: Boolean, + onBack: () -> Unit, + onLaunch: (ClientIdentity, GameEntry) -> Unit, + onCopyLink: (GameEntry) -> Unit, +) { + val ink = LocalGamepadInk.current + val hazeState = remember { HazeState() } + val landscape = LocalConfiguration.current.orientation == Configuration.ORIENTATION_LANDSCAPE + // The cover the legend's X acts on — the coverflow reports it as the cursor settles, so the + // hint and the press agree about which title they mean. + var focused by remember { mutableStateOf(null) } + Box(Modifier.fillMaxSize()) { Box(Modifier.fillMaxSize().hazeSource(hazeState)) { GamepadAuroraBackground(Modifier.fillMaxSize()) Column(Modifier.fillMaxSize().consoleSafeArea()) { - // A pinned card's shelf says so, in the card's own `host · profile` shape: what a - // launch here will use is a property of the shelf, not something to remember from - // the tile two screens back. - ConsoleHeader( - if (pinnedProfileId != null && profile != null) { - "${host.name} · ${profile.name} — Library" - } else { - "${host.name} — Library" - }, - ) + ConsoleHeader(title) Box(Modifier.weight(1f).fillMaxWidth(), contentAlignment = Alignment.Center) { - when (val s = state) { + when (state) { is LibState.Loading -> LoadingState() - is LibState.Message -> MessageState(s.text) - is LibState.Ready -> Coverflow(s.games, s.loader, navActive && !launching) { game -> - if (!launching) { - launching = true - scope.launch { - // Dial the host over the same pinned mTLS trust, booting straight - // into this title (the host resolves `launch` = its library id). - val handle = connectToHost( - context, streamSettings, s.identity, - host.address, host.port, host.fpHex, launch = game.id, - ) - launching = false - if (handle != 0L) { - onLaunched( - ActiveSession( - handle, - streamSettings, - host.clipboardSync, - profileName = profile?.name, - hostId = host.id, - // Where to come back to when this game exits — - // this shelf, pin and all, not the host's default one. - launchedFromLibrary = true, - libraryProfileId = pinnedProfileId, - ), - ) - } - else Toast.makeText( - context, - "Launch failed — check the host and try again.", - Toast.LENGTH_LONG, - ).show() - } - } - } + is LibState.Message -> MessageState(state.text) + is LibState.Ready -> Coverflow( + games = state.games, + loader = state.loader, + navActive = navActive && !launching, + onFocus = { focused = it }, + onCopyLink = onCopyLink, + onLaunch = { game -> onLaunch(state.identity, game) }, + ) } } } @@ -225,7 +315,16 @@ fun LibraryScreen( ) { GamepadHintBar( buildList { - if (state is LibState.Ready) add(PadGlyph.hint('A', "Launch")) + if (state is LibState.Ready) { + add(PadGlyph.hint('A', "Launch")) + // A controller has no right-click, so the grid's context menu becomes a + // face button and a legend entry — the one per-game action there is. + add( + PadGlyph.hint('X', "Copy link") { + focused?.let(onCopyLink) + }, + ) + } add(PadGlyph.hint('B', "Close", onClick = onBack)) }, hazeState = hazeState, @@ -234,6 +333,267 @@ fun LibraryScreen( } } +/** + * The touch shelf: a Material poster grid under a back/reload header — the same page the Apple, + * GTK and Windows shells draw, and the reason a finger-driven user can reach the library at all. + * + * Deliberately NOT the coverflow with the pad legend hidden: a coverflow is a one-at-a-time strip + * built for a D-pad, and on a phone it turns a 400-title library into 400 swipes. The grid is what + * every other touch surface in this app (and every other client's) already shows. + */ +@Composable +private fun TouchLibrary( + title: String, + state: LibState, + launching: Boolean, + onBack: () -> Unit, + onReload: () -> Unit, + onLaunch: (ClientIdentity, GameEntry) -> Unit, + onCopyLink: (GameEntry) -> Unit, +) { + Box(Modifier.fillMaxSize()) { + Column(Modifier.fillMaxSize().consoleSafeArea()) { + Row( + verticalAlignment = Alignment.CenterVertically, + modifier = Modifier.fillMaxWidth().padding(start = 4.dp, end = 4.dp, top = 8.dp), + ) { + IconButton(onClick = onBack) { + Icon(Icons.AutoMirrored.Filled.ArrowBack, contentDescription = "Back") + } + Text( + title, + style = MaterialTheme.typography.titleLarge, + maxLines = 2, + overflow = TextOverflow.Ellipsis, + modifier = Modifier.weight(1f), + ) + // A shelf that failed to load is otherwise a dead end you have to back out of and + // re-enter; every other client's library page has had this from the start. + IconButton(onClick = onReload, enabled = state !is LibState.Loading) { + Icon(Icons.Filled.Refresh, contentDescription = "Reload") + } + } + when (state) { + is LibState.Loading -> Box( + Modifier.weight(1f).fillMaxWidth(), + contentAlignment = Alignment.Center, + ) { + Column( + horizontalAlignment = Alignment.CenterHorizontally, + verticalArrangement = Arrangement.spacedBy(14.dp), + ) { + CircularProgressIndicator() + Text("Loading library…", style = MaterialTheme.typography.bodyLarge) + } + } + is LibState.Message -> Box( + Modifier.weight(1f).fillMaxWidth(), + contentAlignment = Alignment.Center, + ) { + Text( + state.text, + style = MaterialTheme.typography.bodyLarge, + color = MaterialTheme.colorScheme.onSurfaceVariant, + textAlign = TextAlign.Center, + modifier = Modifier.padding(horizontal = 24.dp), + ) + } + is LibState.Ready -> TouchGrid( + games = state.games, + loader = state.loader, + onLaunch = { game -> onLaunch(state.identity, game) }, + onCopyLink = onCopyLink, + modifier = Modifier.weight(1f), + ) + } + } + if (launching) { + Box( + Modifier.fillMaxSize().background(Color.Black.copy(alpha = 0.55f)), + contentAlignment = Alignment.Center, + ) { + Column( + horizontalAlignment = Alignment.CenterHorizontally, + verticalArrangement = Arrangement.spacedBy(14.dp), + ) { + CircularProgressIndicator(color = Color.White) + Text("Launching…", color = Color.White, style = MaterialTheme.typography.bodyLarge) + } + } + } + } +} + +/** + * The poster grid. Design D4: launcher entries get their own shelf above the titles, never + * interleaved, and the headings only appear when both groups exist — so a library without + * launchers looks exactly like a plain grid. + */ +@Composable +private fun TouchGrid( + games: List, + loader: ImageLoader, + onLaunch: (GameEntry) -> Unit, + onCopyLink: (GameEntry) -> Unit, + modifier: Modifier = Modifier, +) { + val launchers = games.filter { it.isLauncher } + val titles = games.filter { !it.isLauncher } + val both = launchers.isNotEmpty() && titles.isNotEmpty() + LazyVerticalGrid( + columns = GridCells.Adaptive(minSize = 130.dp), + modifier = modifier.fillMaxWidth(), + contentPadding = PaddingValues(16.dp), + horizontalArrangement = Arrangement.spacedBy(12.dp), + verticalArrangement = Arrangement.spacedBy(16.dp), + ) { + if (launchers.isNotEmpty()) { + if (both) item(span = { GridItemSpan(maxLineSpan) }) { TouchGroupHeading("Launchers") } + items(launchers, key = { "launcher-${it.id}" }) { + TouchPoster(it, loader, onLaunch, onCopyLink) + } + } + if (titles.isNotEmpty()) { + if (both) item(span = { GridItemSpan(maxLineSpan) }) { TouchGroupHeading("Games") } + items(titles, key = { "game-${it.id}" }) { + TouchPoster(it, loader, onLaunch, onCopyLink) + } + } + } +} + +@Composable +private fun TouchGroupHeading(text: String) { + Text( + text.uppercase(), + style = MaterialTheme.typography.labelSmall, + color = MaterialTheme.colorScheme.onSurfaceVariant, + letterSpacing = 1.4.sp, + ) +} + +/** + * One touch tile: 2:3 poster, store badge, title. Tap launches; a LONG PRESS opens this title's own + * actions — the finger's context menu, and the same gesture the host cards' overflow answers to. + */ +@OptIn(ExperimentalFoundationApi::class) +@Composable +private fun TouchPoster( + game: GameEntry, + loader: ImageLoader, + onLaunch: (GameEntry) -> Unit, + onCopyLink: (GameEntry) -> Unit, +) { + var menu by remember { mutableStateOf(false) } + val shape = MaterialTheme.shapes.medium + Box { + Column( + Modifier.combinedClickable( + onClickLabel = "Launch ${game.title}", + onLongClickLabel = "More options for ${game.title}", + onClick = { onLaunch(game) }, + onLongClick = { menu = true }, + ), + ) { + Box( + Modifier + .fillMaxWidth() + .aspectRatio(2f / 3f) + .clip(shape) + .background(MaterialTheme.colorScheme.surfaceVariant), + contentAlignment = Alignment.Center, + ) { + TouchPosterArt(game, loader) + Box(Modifier.fillMaxSize().padding(6.dp), contentAlignment = Alignment.TopStart) { + Text( + game.storeLabel, + style = MaterialTheme.typography.labelSmall, + // A launcher's badge is brand-filled (design D4); a game's sits on a dark + // wash over its own art, where the theme's own ink would be a coin toss. + color = if (game.isLauncher) { + MaterialTheme.colorScheme.onPrimary + } else { + Color.White + }, + modifier = Modifier + .semantics { + contentDescription = if (game.isLauncher) { + "Opens ${game.storeLabel}" + } else { + "From ${game.storeLabel}" + } + } + .clip(ConsoleShape.Pill) + .background( + if (game.isLauncher) { + MaterialTheme.colorScheme.primary + } else { + Color.Black.copy(alpha = 0.5f) + }, + ) + .padding(horizontal = 8.dp, vertical = 3.dp), + ) + } + } + Text( + game.title, + style = MaterialTheme.typography.bodySmall, + color = MaterialTheme.colorScheme.onSurfaceVariant, + maxLines = 2, + overflow = TextOverflow.Ellipsis, + modifier = Modifier.padding(top = 6.dp), + ) + } + DropdownMenu(expanded = menu, onDismissRequest = { menu = false }) { + DropdownMenuItem( + text = { Text("Copy link") }, + onClick = { + menu = false + onCopyLink(game) + }, + ) + } + } +} + +/** The tile's artwork: the candidates in order (portrait → header → hero), then a placeholder. */ +@Composable +private fun TouchPosterArt(game: GameEntry, loader: ImageLoader) { + val candidates = game.art.posterCandidates + var idx by remember(game.id) { mutableIntStateOf(0) } + if (idx < candidates.size) { + AsyncImage( + model = ImageRequest.Builder(LocalContext.current).data(candidates[idx]).build(), + imageLoader = loader, + contentDescription = game.title, + contentScale = ContentScale.Crop, + modifier = Modifier.fillMaxSize(), + onError = { idx++ }, // this candidate failed — try the next, or fall to the placeholder + ) + return + } + // A launcher ships no poster by design, so its brand mark IS the poster; falling back to the + // launcher's NAME says "opens Steam", where a title would read as "a cover that failed to load". + val mark = launcherIcon(game.iconToken) + if (mark != null) { + Icon( + imageVector = mark, + contentDescription = game.title, + tint = MaterialTheme.colorScheme.onSurfaceVariant, + modifier = Modifier.fillMaxSize(0.45f), + ) + } else { + Text( + if (game.isLauncher) game.storeLabel else game.title, + style = MaterialTheme.typography.titleSmall, + fontWeight = FontWeight.SemiBold, + color = MaterialTheme.colorScheme.onSurfaceVariant, + textAlign = TextAlign.Center, + modifier = Modifier.padding(12.dp), + ) + } +} + @Composable private fun LoadingState() { val ink = LocalGamepadInk.current @@ -262,6 +622,10 @@ internal fun Coverflow( games: List, loader: ImageLoader, navActive: Boolean, + /** Reports the CENTRED title as the cursor settles, so the screen's legend acts on it too. */ + onFocus: (GameEntry?) -> Unit = {}, + /** X — copy the centred title's link. Defaulted so the screenshot harness needs no wiring. */ + onCopyLink: (GameEntry) -> Unit = {}, onLaunch: (GameEntry) -> Unit, ) { val ink = LocalGamepadInk.current @@ -275,9 +639,13 @@ internal fun Coverflow( var navTarget by remember { mutableIntStateOf(0) } LaunchedEffect(pagerState.settledPage) { navTarget = pagerState.settledPage } val current = games.getOrNull(navTarget) + // Publish the centred title outward. Keyed on the ENTRY, not the index, so a library + // refresh that shortens the strip can't leave the legend pointing at a title that moved. + LaunchedEffect(current) { onFocus(current) } // Controller nav: the pad drives the coverflow. Left/right steps a coalesced target the pager - // chases; A launches the centred title; B closes via the screen's BackHandler. + // chases; A launches the centred title; X copies its link; B closes via the screen's + // BackHandler. GamepadNavEffect( active = navActive && games.isNotEmpty(), onMove = { dir -> @@ -285,6 +653,9 @@ internal fun Coverflow( if (t != navTarget) { navTarget = t; scope.launch { pagerState.animateScrollToPage(t) } } }, onActivate = { games.getOrNull(navTarget)?.let(onLaunch) }, + // Read at press time rather than closed over: `navTarget` moves under this callback, + // and the link must be the one the cover under the cursor now points at. + onTertiary = { games.getOrNull(navTarget)?.let(onCopyLink) }, ) // Design D4: the launcher entries lead the strip (the client groups them at parse time). diff --git a/clients/android/app/src/main/kotlin/io/unom/punktfunk/Links.kt b/clients/android/app/src/main/kotlin/io/unom/punktfunk/Links.kt new file mode 100644 index 00000000..c2671cc0 --- /dev/null +++ b/clients/android/app/src/main/kotlin/io/unom/punktfunk/Links.kt @@ -0,0 +1,34 @@ +package io.unom.punktfunk + +import android.content.ClipData +import android.content.ClipboardManager +import android.content.Context +import android.os.Build + +// The clipboard half of "Copy link" (design/client-deep-links.md §4/§5), shared by every surface +// that hands a `punktfunk://` URL out: a host card, a pinned card, and a library title. The URL +// each one builds is its own business; whether the platform TOOK it, and what to say about that, +// is the same answer three times over — and getting it wrong in one place is how a menu item ends +// up silently doing nothing on exactly one screen. + +/** Put a `punktfunk://` URL on the clipboard. False = no clipboard service, or it refused. */ +internal fun putLinkOnClipboard(context: Context, url: String): Boolean { + val clipboard = context.getSystemService(Context.CLIPBOARD_SERVICE) as? ClipboardManager + ?: return false + return runCatching { + clipboard.setPrimaryClip(ClipData.newPlainText("Punktfunk link", url)) + }.isSuccess +} + +/** + * What to tell the user about a copy, or null for "say nothing". + * + * Android 13 draws its own clipboard confirmation, and stacking a second one on top of it is the + * platform's own documented anti-pattern. Below it nothing visible happens at all unless we say + * so — a silent menu item reads as a broken one. + */ +internal fun linkCopyMessage(copied: Boolean): String? = when { + copied && Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU -> null + copied -> "Link copied." + else -> "Couldn't copy the link to the clipboard." +} diff --git a/clients/apple/Sources/PunktfunkClient/Home/LibraryCoverflowView.swift b/clients/apple/Sources/PunktfunkClient/Home/LibraryCoverflowView.swift index 7b9b362b..538e4a81 100644 --- a/clients/apple/Sources/PunktfunkClient/Home/LibraryCoverflowView.swift +++ b/clients/apple/Sources/PunktfunkClient/Home/LibraryCoverflowView.swift @@ -29,6 +29,11 @@ struct LibraryCoverflowView: View { /// Button B (back) — dismisses the library screen. No touch equivalent needed here (the toolbar /// Close button already covers that); this is what makes gamepad-only exit possible. var onDismiss: (() -> Void)? + /// Button X — copy the centered title's `punktfunk://` link. The coverflow's answer to the + /// touch grid's context menu: a controller has no right-click, so the one per-game action + /// there is gets a face button and a legend entry rather than a menu holding a single row. + /// nil where the platform has no clipboard (tvOS), which also drops the hint. + var onCopyLink: ((GameEntry) -> Void)? /// Whether the carousel owns the controller — the in-place shell gates it (mid-transition, /// and under the connect takeover after A launches a title, where this coverflow used to /// keep polling underneath). Cover/sheet presentations keep the default. @@ -44,6 +49,11 @@ struct LibraryCoverflowView: View { private let compact = false // no size classes on macOS #endif @State private var selection: String? + /// The copy hint's acknowledgement. There is no toast on this surface, so the legend entry + /// says it itself — the same answer `GamepadHostOptionsView` gives, in the place the user is + /// already looking. Transient here (the screen stays up, unlike that menu), and cleared the + /// moment the strip moves, since "Copied" was about the cover that WAS centred. + @State private var copied = false /// How many covers have settled (art loaded, or every candidate exhausted). @State private var artSettled = 0 /// The backstop below has fired: play the entrance regardless of what the art is doing. @@ -79,6 +89,14 @@ struct LibraryCoverflowView: View { try? await Task.sleep(for: .milliseconds(700)) artWaitOver = true } + // "Copied" is an acknowledgement, not a state — it goes away on its own, and at once if + // the strip moves off the cover it was about. + .task(id: copied) { + guard copied else { return } + try? await Task.sleep(for: .milliseconds(1600)) + withAnimation(.smooth(duration: 0.2)) { copied = false } + } + .onChange(of: selection) { _, _ in copied = false } } @ViewBuilder private func content(for size: CGSize) -> some View { @@ -109,6 +127,7 @@ struct LibraryCoverflowView: View { itemWidth: coverWidth, spacing: 34, onActivate: { onLaunch?($0.id) }, + onTertiary: onCopyLink.map { copy in { copyCentered(copy) } }, onBack: { onDismiss?() }, shoulderJump: 5, isActive: controllerActive, @@ -160,6 +179,14 @@ struct LibraryCoverflowView: View { } } + /// Hand the CENTERED title to the copy action. Read at press time, not when the legend or + /// the carousel was built (the same rule A's hint follows), and inert with nothing centred. + private func copyCentered(_ copy: (GameEntry) -> Void) { + guard let game = games.first(where: { $0.id == selection }) else { return } + copy(game) + withAnimation(.smooth(duration: 0.2)) { copied = true } + } + /// Does this library have both groups? Only then does the heading earn its row — a /// launcher-less library gets exactly the layout it had before design D4. private var showsGroupHeading: Bool { @@ -217,6 +244,12 @@ struct LibraryCoverflowView: View { // what A does. action: { if let id = selection { onLaunch(id) } })) } + if let onCopyLink { + hints.append(.init( + glyph: buttonGlyph(\.buttonX, fallback: "x.circle"), + text: copied ? "Copied" : "Copy link", + action: { copyCentered(onCopyLink) })) + } hints.append(.init( glyph: buttonGlyph(\.buttonB, fallback: "b.circle"), text: "Close", action: { onDismiss?() })) diff --git a/clients/apple/Sources/PunktfunkClient/Home/LibraryView.swift b/clients/apple/Sources/PunktfunkClient/Home/LibraryView.swift index 2cd5de6c..84114c4e 100644 --- a/clients/apple/Sources/PunktfunkClient/Home/LibraryView.swift +++ b/clients/apple/Sources/PunktfunkClient/Home/LibraryView.swift @@ -165,6 +165,9 @@ struct LibraryView: View { LibraryCoverflowView( games: games, artLoader: artLoader, onLaunch: onLaunch, onDismiss: { (onClose ?? { dismiss() })() }, + // Nil where there is nothing to copy into (tvOS), which is what drops the + // hint from the legend rather than leaving a button that does nothing. + onCopyLink: LinkClipboard.isAvailable ? { copyLink($0) } : nil, controllerActive: controllerActive) } else { grid @@ -270,20 +273,44 @@ struct LibraryView: View { private func tiles(_ entries: [GameEntry]) -> some View { LazyVGrid(columns: columns, spacing: 18) { ForEach(entries) { game in - if let onLaunch { - Button { onLaunch(game.id) } label: { + Group { + if let onLaunch { + Button { onLaunch(game.id) } label: { + GameCard(game: game, artLoader: artLoader, selected: isKeyCursor(game)) + } + .buttonStyle(.plain) + } else { GameCard(game: game, artLoader: artLoader, selected: isKeyCursor(game)) } - .buttonStyle(.plain) - .id(game.id) - } else { - GameCard(game: game, artLoader: artLoader, selected: isKeyCursor(game)) - .id(game.id) } + .id(game.id) + // Right-click / long-press a poster for that TITLE's own actions — the same + // gesture a host card answers to, one level down. `.contextMenu` doesn't exist + // on tvOS, which is also the one platform with no clipboard to copy into. + #if !os(tvOS) + .contextMenu { + if LinkClipboard.isAvailable { + Button("Copy Link") { copyLink(game) } + } + } + #endif } } } + /// Put this title's self-emitted `punktfunk://` link on the clipboard: the shelf's host, + /// the pinned card's profile when a pin opened it, and the game's own `launch=` id — so + /// the URL boots straight into the title, the way a host card's link opens the desktop + /// (design/client-deep-links.md §5). + /// + /// Addressed to the STORE's current record rather than the one the shelf was opened with, + /// so a host re-addressed while browsing hands out the address it actually has now. + private func copyLink(_ game: GameEntry) { + let current = store.hosts.first { $0.id == host.id } ?? host + LinkClipboard.copy( + DeepLink.forHost(current, launch: game.id, profile: target.pinnedProfileID).urlString) + } + /// Whether the keyboard cursor is on this tile (always false where there is no keyboard /// navigation to have moved it). private func isKeyCursor(_ game: GameEntry) -> Bool { diff --git a/clients/linux/src/app.rs b/clients/linux/src/app.rs index 78c8963f..a213e900 100644 --- a/clients/linux/src/app.rs +++ b/clients/linux/src/app.rs @@ -61,6 +61,13 @@ const CSS: &str = " .pf-poster { border-radius: 10px; background: alpha(currentColor, 0.08); } .pf-poster-monogram { font-size: 2.4em; font-weight: bold; color: alpha(currentColor, 0.45); } .pf-store-badge { color: white; background: rgba(0, 0, 0, 0.55); } +/* The poster's own overflow menu button. It sits ON artwork, so it needs the badge's dark + scrim to read at all — a flat button inherits the page's foreground and vanishes into a + pale cover. Small and round so it balances the store badge opposite it rather than + competing with the art. (No quotes in here — see the top of this string.) */ +.pf-poster-menu { color: white; background: rgba(0, 0, 0, 0.55); border-radius: 999px; + min-width: 24px; min-height: 24px; padding: 0; margin: 6px; } +.pf-poster-menu:hover { background: rgba(0, 0, 0, 0.75); } /* Launcher entries (design D4) open the launcher itself. They rarely have poster art, so an art-less one must not read as a game whose cover failed to load: accent face, the launcher named instead of a title monogram, and an accent badge. */ diff --git a/clients/linux/src/ui_library.rs b/clients/linux/src/ui_library.rs index 7ec5d6d3..2bed6610 100644 --- a/clients/linux/src/ui_library.rs +++ b/clients/linux/src/ui_library.rs @@ -10,7 +10,7 @@ use crate::library::{self, GameEntry}; use crate::trust; use crate::ui_hosts::ConnectRequest; use adw::prelude::*; -use gtk::{gdk, glib}; +use gtk::{gdk, gio, glib}; use relm4::prelude::*; use std::cell::{Cell, RefCell}; use std::collections::{HashMap, VecDeque}; @@ -64,6 +64,32 @@ fn page_host_label(req: &ConnectRequest) -> String { ) } +/// One title's self-emitted `punktfunk://` link (design/client-deep-links.md §5): this +/// page's host with the game's own `launch=` id attached, so the URL boots straight into +/// that title instead of the desktop. Built from the STORE, like every other "Copy link" +/// in this shell, because the stable id and the pin live there rather than on the request. +/// +/// A shelf opened from a PINNED card carries that card's one-off profile into the link: +/// what you copy off that shelf is what pressing the card and picking the title does. +/// `None` only when the host has left the store while the page was open. +fn game_link(req: &ConnectRequest, game_id: &str) -> Option { + let known = pf_client_core::trust::KnownHosts::load(); + let host = req + .fp_hex + .as_deref() + .filter(|fp| !fp.is_empty()) + .and_then(|fp| known.find_by_fp(fp)) + .or_else(|| known.find_by_addr(&req.addr, req.port))?; + Some( + pf_client_core::deeplink::DeepLink::for_host( + host, + Some(game_id), + req.profile.as_deref().filter(|p| !p.is_empty()), + ) + .to_url(), + ) +} + /// Open the library page for a saved host and start the fetch. `mgmt_port` comes from /// the live mDNS `mgmt` TXT when the host is advertising (the hosts page resolves it). pub fn open( @@ -379,10 +405,44 @@ fn game_card(state: &Rc, game: &GameEntry) -> gtk::FlowBoxChild { badge.set_margin_start(6); badge.set_margin_top(6); + // The tile's own actions. Today that is one — "Copy link", the per-GAME half of the + // pairing the host cards already offer (design/client-deep-links.md §5 names the + // library game context menu as an attach point) — hung off a menu rather than a bare + // button so the next one lands next to it instead of growing a second affordance. + let actions = gio::SimpleActionGroup::new(); + { + let (sender, req, id) = (state.sender.clone(), state.req.clone(), game.id.clone()); + let a = gio::SimpleAction::new("copy-link", None); + a.connect_activate(move |_, _| match game_link(&req, &id) { + Some(url) => { + if let Some(display) = gdk::Display::default() { + display.clipboard().set_text(&url); + } + sender.input(AppMsg::Toast("Link copied".into())); + } + // Only reachable if the host was forgotten while this page was open. + None => sender.input(AppMsg::Toast("This host isn't saved any more".into())), + }); + actions.add_action(&a); + } + let menu = gio::Menu::new(); + menu.append(Some("Copy link"), Some("game.copy-link")); + let menu_btn = gtk::MenuButton::builder() + .icon_name("view-more-symbolic") + .menu_model(&menu) + .halign(gtk::Align::End) + .valign(gtk::Align::Start) + .build(); + menu_btn.add_css_class("flat"); + menu_btn.add_css_class("pf-poster-menu"); + menu_btn.set_tooltip_text(Some("More options")); + let poster = gtk::Overlay::new(); poster.set_child(Some(&placeholder)); poster.add_overlay(&pic); poster.add_overlay(&badge); + poster.add_overlay(&menu_btn); + poster.insert_action_group("game", Some(&actions)); poster.add_css_class("pf-poster"); if launcher { poster.add_css_class("pf-launcher"); @@ -403,6 +463,14 @@ fn game_card(state: &Rc, game: &GameEntry) -> gtk::FlowBoxChild { let child = gtk::FlowBoxChild::new(); child.set_child(Some(&card)); + // Right-click anywhere on the tile is the same menu — the desktop gesture for "this + // item's actions", and what the host cards already answer to. + let right_click = gtk::GestureClick::builder().button(3).build(); + { + let menu_btn = menu_btn.clone(); + right_click.connect_pressed(move |_, _, _, _| menu_btn.popup()); + } + child.add_controller(right_click); let sender = state.sender.clone(); let mut req = state.req.clone(); req.launch = Some((game.id.clone(), game.title.clone())); diff --git a/clients/windows/src/app/library.rs b/clients/windows/src/app/library.rs index 367c5c74..3d1b8c54 100644 --- a/clients/windows/src/app/library.rs +++ b/clients/windows/src/app/library.rs @@ -228,6 +228,37 @@ fn initials(title: &str) -> String { .collect() } +/// The tile overflow's only entry today — the per-GAME half of the pairing the host tiles +/// already offer (design/client-deep-links.md §5 names the library game context menu as an +/// attach point for exactly this). +const MENU_COPY_LINK: &str = "Copy link"; + +/// One title's self-emitted `punktfunk://` link: this page's host with the game's own +/// `launch=` id attached, so the URL boots straight into that title rather than the desktop. +/// Built from the STORE — the stable id and the fingerprint live there, not on the target — +/// which is what makes a link taken here identical to one taken off the host tile. +/// +/// A library opened from a "Connect with" one-off carries that profile into the link, so a +/// copied URL streams the way the shelf it came from does. `None` only when the host has +/// left the store while the page was open. +fn game_link(target: &super::Target, game_id: &str) -> Option { + let known = crate::trust::KnownHosts::load(); + let host = target + .fp_hex + .as_deref() + .filter(|fp| !fp.is_empty()) + .and_then(|fp| known.find_by_fp(fp)) + .or_else(|| known.find_by_addr(&target.addr, target.port))?; + Some( + pf_client_core::deeplink::DeepLink::for_host( + host, + Some(game_id), + target.profile.as_deref().filter(|p| !p.is_empty()), + ) + .to_url(), + ) +} + /// A small group label above a tile grid ("Launchers" / "Games"). Only drawn when the page shows /// both groups — a single unlabelled grid is what every launcher-less library looked like before. fn group_heading(text: &str) -> Element { @@ -246,6 +277,7 @@ fn poster_tile( art_uri: Option<&str>, poster_h: f64, on_tap: Box, + on_copy_link: Box, ) -> Element { let poster: Element = match art_uri { Some(uri) => Image::new_with_uri(uri) @@ -302,6 +334,25 @@ fn poster_tile( .vertical_alignment(VerticalAlignment::Top) .margin(uniform(6.0)) .into(), + // This entry's own actions, opposite the store badge. A button rather than a + // right-click context flyout because the reactor hangs `menu_flyout` off buttons + // only — and a menu you cannot see is one a mouse user never finds. Its click is + // handled by the button, so it never reaches the tile's launch backstop. + button("") + .icon(Symbol::More) + .subtle() + .tooltip("More options") + .automation_name(format!("More options for {}", game.title)) + .menu_flyout(vec![menu_item(MENU_COPY_LINK)]) + .on_item_clicked(move |item: String| { + if item == MENU_COPY_LINK { + on_copy_link(); + } + }) + .horizontal_alignment(HorizontalAlignment::Right) + .vertical_alignment(VerticalAlignment::Top) + .margin(uniform(2.0)) + .into(), ])) .corner_radius(8.0) .border_brush(if game.launcher { @@ -394,11 +445,19 @@ pub(crate) fn library_page(props: &LibraryProps, cx: &mut RenderCx) -> Element { let tile = |g: &Game| -> Element { let (ctx2, ss, st) = (ctx.clone(), ss.clone(), st.clone()); let (target, id) = (target.clone(), g.id.clone()); + let (link_target, link_id) = (target.clone(), id.clone()); poster_tile( g, props.state.art.get(&g.id).map(String::as_str), poster_h, Box::new(move || initiate_launch(&ctx2, target.clone(), id.clone(), &ss, &st)), + // Silent on success, exactly like the host tile's "Copy link" on the + // hosts page — this shell has no toast, and the two must not disagree + // about what copying a link looks like. + Box::new(move || match game_link(&link_target, &link_id) { + Some(url) => pf_client_core::clipboard::set_text(&url), + None => tracing::warn!(id = %link_id, "no saved host to build a link from"), + }), ) }; // Design D4: launcher entries get their own shelf above the titles, never diff --git a/crates/pf-console-ui/src/screens.rs b/crates/pf-console-ui/src/screens.rs index ab744cfa..331a0be3 100644 --- a/crates/pf-console-ui/src/screens.rs +++ b/crates/pf-console-ui/src/screens.rs @@ -103,23 +103,34 @@ impl Outbox { } } -/// This row's `punktfunk://` link, built from the STORE so it carries the fingerprint and -/// stable id a row doesn't hold — the same builder the desktop shells' "Copy link" uses, -/// so a link is identical whichever surface hands it to you. `None` if the host has left +/// A saved host's `punktfunk://` link, built from the STORE so it carries the fingerprint +/// and stable id a screen doesn't hold — the same builder the desktop shells' "Copy link" +/// uses, so a link is identical whichever surface hands it to you. `launch` attaches a +/// library id, which is what makes a game's link a game's link. `None` if the host has left /// the store since the menu was opened. -pub(crate) fn host_link(row: &HostRow) -> Option { +pub(crate) fn saved_host_link( + fp_hex: &str, + addr: &str, + port: u16, + profile: Option<&str>, + launch: Option<&str>, +) -> Option { let known = trust::KnownHosts::load(); - let host = (!row.fp_hex.is_empty()) - .then(|| known.find_by_fp(&row.fp_hex)) + let host = (!fp_hex.is_empty()) + .then(|| known.find_by_fp(fp_hex)) .flatten() - .or_else(|| known.find_by_addr(&row.addr, row.port))?; - Some( - pf_client_core::deeplink::DeepLink::for_host( - host, - None, - row.pin.as_ref().map(|p| p.id.as_str()), - ) - .to_url(), + .or_else(|| known.find_by_addr(addr, port))?; + Some(pf_client_core::deeplink::DeepLink::for_host(host, launch, profile).to_url()) +} + +/// This row's link — the host itself, with a pinned card's profile when the row is one. +pub(crate) fn host_link(row: &HostRow) -> Option { + saved_host_link( + &row.fp_hex, + &row.addr, + row.port, + row.pin.as_ref().map(|p| p.id.as_str()), + None, ) } diff --git a/crates/pf-console-ui/src/screens/library.rs b/crates/pf-console-ui/src/screens/library.rs index 91f005b0..16dfc5dc 100644 --- a/crates/pf-console-ui/src/screens/library.rs +++ b/crates/pf-console-ui/src/screens/library.rs @@ -75,6 +75,18 @@ impl LibraryScreen { } } + /// One title's self-emitted link: this shelf's host, this shelf's pinned profile (so a + /// link taken off a pinned card's shelf streams the way that card does), and the game. + fn game_link(&self, id: &str) -> Option { + crate::screens::saved_host_link( + &self.fp_hex, + &self.addr, + self.port, + self.pin.as_ref().map(|p| p.id.as_str()), + Some(id), + ) + } + fn fetch_cmd(&self) -> ConsoleCmd { ConsoleCmd::FetchLibrary { addr: self.addr.clone(), @@ -149,11 +161,29 @@ impl LibraryScreen { }); Some(MenuPulse::Confirm) } + // X copies the focused title's own `punktfunk://` link — the same + // self-emitted URL a host tile's "Copy link" hands out, plus this game's + // `launch=` id, so pasting it into Playnite or a Stream Deck macro boots + // straight into the title. Direct rather than behind an options screen: + // it is the only per-game action there is, and a menu holding one row is + // a press the user pays for nothing. + MenuEvent::Tertiary => { + let g = self.games.get(self.cursor as usize)?; + match self.game_link(&g.id) { + Some(url) => { + fx.copy = Some(url); + fx.toast = Some("Link copied".into()); + } + // Only if the host left the store while the shelf was open. + None => fx.toast = Some("This host isn't saved any more".into()), + } + Some(MenuPulse::Confirm) + } MenuEvent::Back => { fx.pop(); None } - MenuEvent::Move(_) | MenuEvent::Secondary | MenuEvent::Tertiary => None, + MenuEvent::Move(_) | MenuEvent::Secondary => None, }, LibraryPhase::Error { can_retry, .. } => match ev { MenuEvent::Confirm if *can_retry => { @@ -254,6 +284,7 @@ impl LibraryScreen { "Play" }, ), + Hint::new(HintKey::Tertiary, "Copy link"), Hint::new(HintKey::Shoulders, "Jump"), Hint::new(HintKey::Back, "Back"), ], From ae6d872beda9d364547d1e2fa9e471110af3416b Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Sun, 16 Aug 2026 10:19:53 +0200 Subject: [PATCH 09/11] test(client-android): shoot the touch library, and pass the shelf's callbacks as lambdas MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two follow-ups on the library work: * LibraryScreen handed ConsoleLibrary/TouchLibrary `::launch` and `::copyLink`. Two callable references to the same local function compare EQUAL however different the frame they captured, so a skipped recomposition would have left the child calling a closure over stale settings — the trap SettingsScreen documents at scopeProfile(), having already been bitten by it. Lambdas instead, which is what every other call site here passes. * The touch grid was a brand-new screen with no visual coverage. TouchGrid goes internal (the reason Coverflow already is) and the harness grows a portrait scene over the same mock shelf the coverflow scene uses, so the column count, the group headings and the launcher brand-mark tile are all captured. --- .../src/main/kotlin/io/unom/punktfunk/App.kt | 1 + .../kotlin/io/unom/punktfunk/LibraryScreen.kt | 17 ++++++--- .../punktfunk/screenshots/ScreenshotTest.kt | 8 ++++ .../unom/punktfunk/screenshots/ShotScenes.kt | 37 +++++++++++++++++++ 4 files changed, 58 insertions(+), 5 deletions(-) diff --git a/clients/android/app/src/main/kotlin/io/unom/punktfunk/App.kt b/clients/android/app/src/main/kotlin/io/unom/punktfunk/App.kt index 0b1480e4..64006b7a 100644 --- a/clients/android/app/src/main/kotlin/io/unom/punktfunk/App.kt +++ b/clients/android/app/src/main/kotlin/io/unom/punktfunk/App.kt @@ -184,6 +184,7 @@ fun App(forceGamepadUi: Boolean = false) { // third permanent tab for something you reach from a card would be a nav item that is // meaningless until you pick one. So it takes the whole window (bar included, like the // console shell's does) and Back — the arrow or the system gesture — returns to the grid. + // Read once: `touchLibrary` is a `var`, so it does not smart-cast through the branch. val (host, pinId) = touchLibrary!! LibraryScreen( host = host, diff --git a/clients/android/app/src/main/kotlin/io/unom/punktfunk/LibraryScreen.kt b/clients/android/app/src/main/kotlin/io/unom/punktfunk/LibraryScreen.kt index 9b16df0a..672592e3 100644 --- a/clients/android/app/src/main/kotlin/io/unom/punktfunk/LibraryScreen.kt +++ b/clients/android/app/src/main/kotlin/io/unom/punktfunk/LibraryScreen.kt @@ -229,6 +229,10 @@ fun LibraryScreen( } } + // Lambdas, NOT `::launch` / `::copyLink`: two callable references to the same local function + // compare EQUAL however different the frame they captured, so a skipped recomposition would + // leave the child calling a closure over stale settings. SettingsScreen documents the same + // trap at `scopeProfile()`, having been bitten by it. if (console) { ConsoleLibrary( title = title, @@ -236,8 +240,8 @@ fun LibraryScreen( launching = launching, navActive = navActive, onBack = onBack, - onLaunch = ::launch, - onCopyLink = ::copyLink, + onLaunch = { identity, game -> launch(identity, game) }, + onCopyLink = { game -> copyLink(game) }, ) } else { TouchLibrary( @@ -246,8 +250,8 @@ fun LibraryScreen( launching = launching, onBack = onBack, onReload = { reloadKey++ }, - onLaunch = ::launch, - onCopyLink = ::copyLink, + onLaunch = { identity, game -> launch(identity, game) }, + onCopyLink = { game -> copyLink(game) }, ) } } @@ -428,9 +432,12 @@ private fun TouchLibrary( * The poster grid. Design D4: launcher entries get their own shelf above the titles, never * interleaved, and the headings only appear when both groups exist — so a library without * launchers looks exactly like a plain grid. + * + * Internal (not private) for the same reason as [Coverflow]: the screenshot harness composes the + * real grid with a mock shelf, because the screen around it takes its state off the network. */ @Composable -private fun TouchGrid( +internal fun TouchGrid( games: List, loader: ImageLoader, onLaunch: (GameEntry) -> Unit, diff --git a/clients/android/app/src/test/kotlin/io/unom/punktfunk/screenshots/ScreenshotTest.kt b/clients/android/app/src/test/kotlin/io/unom/punktfunk/screenshots/ScreenshotTest.kt index 7bc47ccf..17b1aa91 100644 --- a/clients/android/app/src/test/kotlin/io/unom/punktfunk/screenshots/ScreenshotTest.kt +++ b/clients/android/app/src/test/kotlin/io/unom/punktfunk/screenshots/ScreenshotTest.kt @@ -210,6 +210,14 @@ class ScreenshotTest { @Config(sdk = [36], qualifiers = "w800dp-h360dp-xxhdpi") fun library() = shootRoot("library", statusBar = false) { LibraryScene() } + /** + * The same shelf as the TOUCH grid — the presentation a finger gets from a host card's + * "Browse library…". Portrait (the default qualifiers), because that is the orientation a + * phone browses a poster wall in, and the one whose column count the layout has to get right. + */ + @Test + fun libraryTouch() = shootRoot("library-touch") { TouchLibraryScene() } + @Test fun consoleControllersLight() = shootRoot("console-controllers-light", statusBar = false) { ConsoleControllersScene(paletteId = "holo") } diff --git a/clients/android/app/src/test/kotlin/io/unom/punktfunk/screenshots/ShotScenes.kt b/clients/android/app/src/test/kotlin/io/unom/punktfunk/screenshots/ShotScenes.kt index dacd6090..8c6dc1c8 100644 --- a/clients/android/app/src/test/kotlin/io/unom/punktfunk/screenshots/ShotScenes.kt +++ b/clients/android/app/src/test/kotlin/io/unom/punktfunk/screenshots/ShotScenes.kt @@ -26,10 +26,13 @@ import androidx.compose.foundation.layout.height import androidx.compose.foundation.layout.padding import androidx.compose.foundation.layout.size import androidx.compose.material.icons.Icons +import androidx.compose.material.icons.automirrored.filled.ArrowBack import androidx.compose.material.icons.filled.BatteryFull +import androidx.compose.material.icons.filled.Refresh import androidx.compose.material.icons.filled.SignalCellular4Bar import androidx.compose.material.icons.filled.Wifi import androidx.compose.material3.Icon +import androidx.compose.material3.IconButton import androidx.compose.foundation.lazy.grid.GridCells import androidx.compose.foundation.lazy.grid.GridItemSpan import androidx.compose.foundation.lazy.grid.LazyVerticalGrid @@ -67,6 +70,7 @@ import io.unom.punktfunk.ConsoleLegendInset import io.unom.punktfunk.ConsoleLicensesScreen import io.unom.punktfunk.ControllersScreen import io.unom.punktfunk.Coverflow +import io.unom.punktfunk.TouchGrid import io.unom.punktfunk.GamepadAuroraBackground import io.unom.punktfunk.GamepadHintBar import io.unom.punktfunk.PadGlyph @@ -700,6 +704,39 @@ internal fun LibraryScene(paletteId: String = "violet") = ConsolePalette(palette } } +/** + * The TOUCH library — the poster grid a finger reaches through a host card's "Browse library…", + * with the same mock shelf the coverflow scene uses. Same construction as [LibraryScene]: the real + * [TouchGrid] under a rebuilt header, because the screen around it takes its state off the network. + */ +@Composable +internal fun TouchLibraryScene() { + val context = LocalContext.current + val loader = remember { shotLibraryLoader(context) } + val games = remember { shotGames() } + Surface(color = MaterialTheme.colorScheme.background) { + Column(Modifier.fillMaxSize()) { + Row( + verticalAlignment = Alignment.CenterVertically, + modifier = Modifier.fillMaxWidth().padding(start = 4.dp, end = 4.dp, top = 8.dp), + ) { + IconButton(onClick = {}) { + Icon(Icons.AutoMirrored.Filled.ArrowBack, contentDescription = "Back") + } + Text( + "Living Room PC — Library", + style = MaterialTheme.typography.titleLarge, + modifier = Modifier.weight(1f), + ) + IconButton(onClick = {}) { + Icon(Icons.Filled.Refresh, contentDescription = "Reload") + } + } + TouchGrid(games, loader, onLaunch = {}, onCopyLink = {}, modifier = Modifier.weight(1f)) + } + } +} + /** A believable shelf: four titles with art plus the Steam launcher entry (brand-mark tile). */ private fun shotGames() = listOf( GameEntry("custom:aurora", "custom", "Aurora Drift", Artwork("shot://art/aurora", null, null)), From 14bdfeaeeb8067d4c1893dd0fec5aa6c8017d1da Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Sun, 16 Aug 2026 10:20:57 +0200 Subject: [PATCH 10/11] fix(client-windows): the library tile's overflow sat inside its own launch backstop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The poster's new "…" button was a child of the border carrying on_tapped, so opening the menu was also a click on the tile. host_tile on the hosts page already splits these — the tapped region is the inner summary and the menu button is its SIBLING in the grid — and the library tile now does the same. Relying on Button marking Tapped handled is a guess about event routing where the shell already has an answer that does not need one. --- clients/windows/src/app/library.rs | 59 +++++++++++++++++------------- 1 file changed, 33 insertions(+), 26 deletions(-) diff --git a/clients/windows/src/app/library.rs b/clients/windows/src/app/library.rs index 3d1b8c54..52e12104 100644 --- a/clients/windows/src/app/library.rs +++ b/clients/windows/src/app/library.rs @@ -334,10 +334,37 @@ fn poster_tile( .vertical_alignment(VerticalAlignment::Top) .margin(uniform(6.0)) .into(), - // This entry's own actions, opposite the store badge. A button rather than a - // right-click context flyout because the reactor hangs `menu_flyout` off buttons - // only — and a menu you cannot see is one a mouse user never finds. Its click is - // handled by the button, so it never reaches the tile's launch backstop. + ])) + .corner_radius(8.0) + .border_brush(if game.launcher { + ThemeRef::Accent + } else { + ThemeRef::CardStroke + }) + .border_thickness(uniform(1.0)); + + let tappable = border( + vstack(( + framed, + text_block(&game.title) + .font_size(12.0) + .wrap() + .margin(edges(2.0, 6.0, 2.0, 0.0)), + )) + .spacing(0.0), + ) + .background(hit_test_backstop()) + .on_tapped(on_tap); + + // This entry's own actions, opposite the store badge. A button rather than a right-click + // context flyout because the reactor hangs `menu_flyout` off buttons only — and a menu a + // mouse user cannot see is one they never find. + // + // A SIBLING of the tappable area, not a child of it: `host_tile` on the hosts page splits + // the two exactly this way, and that split is what keeps a click on the overflow from also + // launching the title underneath it. + grid(vec![ + tappable.into(), button("") .icon(Symbol::More) .subtle() @@ -351,29 +378,9 @@ fn poster_tile( }) .horizontal_alignment(HorizontalAlignment::Right) .vertical_alignment(VerticalAlignment::Top) - .margin(uniform(2.0)) + .margin(edges(0.0, 4.0, 4.0, 0.0)) .into(), - ])) - .corner_radius(8.0) - .border_brush(if game.launcher { - ThemeRef::Accent - } else { - ThemeRef::CardStroke - }) - .border_thickness(uniform(1.0)); - - border( - vstack(( - framed, - text_block(&game.title) - .font_size(12.0) - .wrap() - .margin(edges(2.0, 6.0, 2.0, 0.0)), - )) - .spacing(0.0), - ) - .background(hit_test_backstop()) - .on_tapped(on_tap) + ]) .into() } From d5838f381aae22e3a76c8e9409d4a08e6988d9d5 Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Sun, 16 Aug 2026 10:50:06 +0200 Subject: [PATCH 11/11] fix(client-core): a pipewire type in a `cfg(any(linux, test))` signature broke the Windows client leg MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Main has been red on both Windows client jobs since the pad-audio work landed: error[E0433]: cannot find module or crate `pipewire` --> crates\pf-client-core\src\pad_audio.rs:368:39 error: could not compile `pf-client-core` (lib test) due to 1 previous error `pad_audio.rs` gates its pure-logic helpers `cfg(any(target_os = "linux", test))` so the sink-picking decisions stay unit-testable on every platform. `sink_from_props` was given the same gate, but it is not pure logic: it takes a `&pipewire::spa::utils::dict::DictRef`, and `pipewire` is declared under `[target.'cfg(target_os = "linux")'.dependencies]`. So on Windows the `test` arm pulls the item into the `lib test` target, where the crate does not exist. Nothing but `--all-targets` compiles that target, which is why no Linux gate could see it — including the container gate used to validate the original change, which lints pf-client-core for the host target only. The function's sole caller already lives inside a plain `cfg(target_os = "linux")` block and no test refers to it, so the `test` arm was only ever dead weight. Dropped it. Swept the rest of the crate for the same shape: of the 13 `cfg(any(target_os = "linux", test))` items in pad_audio.rs, this is the only one that names a Linux-only crate, and no other file in pf-client-core uses that gate. All 13 other pipewire references sit under a plain `cfg(target_os = "linux")`. Linux codegen is unchanged: `target_os = "linux"` was already true on that arm, so the item compiles exactly as before there. --- crates/pf-client-core/src/pad_audio.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/crates/pf-client-core/src/pad_audio.rs b/crates/pf-client-core/src/pad_audio.rs index 5ad32638..bb175797 100644 --- a/crates/pf-client-core/src/pad_audio.rs +++ b/crates/pf-client-core/src/pad_audio.rs @@ -364,7 +364,12 @@ pub(crate) fn pick_pad_sink(sinks: &[SinkNode], cards: &[CardDevice]) -> Option< /// Read a sink node's facts out of a proplist. Split out because it has to run against the /// node's INFO props, not the registry's — see [`walk_graph`]. -#[cfg(any(target_os = "linux", test))] +/// +/// Linux-only, unlike its pure-logic neighbours: `DictRef` comes from `pipewire`, which is a +/// `cfg(target_os = "linux")` dependency. Widening this to `any(…, test)` the way the testable +/// helpers around it do puts the item into the Windows `lib test` target, where the crate does +/// not exist — E0433, visible only under `--all-targets`, and so only on the Windows CI leg. +#[cfg(target_os = "linux")] pub(crate) fn sink_from_props(props: &pipewire::spa::utils::dict::DictRef) -> Option { // Both spellings: PipeWire's own objects use the `device.`-prefixed keys, the pulse-facing // proplist GE reads uses the bare ones. Cheap to accept both.