From fecbec2daff6343d88497a974fe54e071cd3719d Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Sun, 19 Jul 2026 17:53:48 +0200 Subject: [PATCH] fix(encode): make LTR-RFI loss recovery sound under sustained loss MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Field report (lid-closed Intel laptop, ~6-19% sustained loss): the stream never healed — permanent macroblock soup. Three stacked bugs: - QSV answered RFI with PreferredRefList only, a reorder HINT per the VPL spec — the recovery frame could keep predicting from tainted short-term refs. Now rejects every other DPB candidate (RejectedRefList) and caps L0 at one active entry (AVC/HEVC), matching AMF's hard ForceLTRReferenceBitfield / NVENC invalidation semantics. - Neither QSV nor AMF taint-swept LTR slots across losses: a slot marked inside the client's corrupt window became the "known-good" anchor of the NEXT loss, propagating corruption through every recovery. Both now drop slots at-or-after the loss start before picking an anchor, and guard a queued force whose slot the sweep emptied (no false recovery_anchor tag). - The native plane re-anchored the FULL IDR cooldown on every successful RFI, so under sustained loss the client's escalating keyframe requests were coalesced away indefinitely (field log: dozens swallowed, one IDR per ~8 s). RFI now anchors a 300 ms echo window with a 2-swallow budget per loss episode; a client still asking past that gets its IDR. Live-validated on Arc (qsv feature): 6/6 including the new qsv_live_ltr_rfi_taint_sweep_declines (a loss covering every live mark declines the RFI and falls back to IDR recovery). Co-Authored-By: Claude Opus 4.8 --- crates/pf-encode/src/enc/windows/amf.rs | 23 +++- crates/pf-encode/src/enc/windows/qsv.rs | 126 +++++++++++++++++---- crates/punktfunk-host/src/native/stream.rs | 59 ++++++++-- 3 files changed, 176 insertions(+), 32 deletions(-) diff --git a/crates/pf-encode/src/enc/windows/amf.rs b/crates/pf-encode/src/enc/windows/amf.rs index c4cc568f..979048f3 100644 --- a/crates/pf-encode/src/enc/windows/amf.rs +++ b/crates/pf-encode/src/enc/windows/amf.rs @@ -2077,9 +2077,15 @@ impl Encoder for AmfEncoder { } // Apply a queued force (from invalidate_ref_frames / the test hook) to THIS frame: it // becomes the clean re-anchor P-frame the client lifts its post-loss freeze on. + // Guard against a slot the taint sweep emptied since the force was queued: the + // HARDWARE slot still holds the tainted mark, so forcing it would re-reference the + // very corruption being recovered from — and the frame must not ship tagged + // `recovery_anchor` either (the client lifts its post-loss freeze on that tag). if let Some(slot) = self.pending_force.take() { - force_slot = Some(slot); - recovery_anchor = true; + if self.ltr_slots[slot].is_some() { + force_slot = Some(slot); + recovery_anchor = true; + } } // Mark cadence: refresh a long-term reference on every IDR and every `ltr_mark_interval` // frames — but never on the recovery frame itself (marking rotates `next_ltr_slot` and @@ -2363,6 +2369,16 @@ impl Encoder for AmfEncoder { if !self.ltr_active || first < 0 || first > last { return false; } + // Taint sweep BEFORE picking the anchor: an LTR marked at-or-after the loss start was + // encoded inside the client's corrupt window — the client either never received it or + // decoded it against a broken reference chain. Serving it as "known-good" on a LATER + // loss ships corruption as the recovery anchor (and every subsequent mark re-samples + // it). Dropped slots stay dropped; the cadence re-marks a clean frame within ~1/4 s. + for marked in self.ltr_slots.iter_mut() { + if marked.is_some_and(|idx| idx >= first) { + *marked = None; + } + } // Pick the newest LTR strictly OLDER than the loss: the most recent known-good reference the // client still holds, so re-referencing it costs the least (smallest recovery-frame residual). // `ltr_slots` store the WIRE frame index of the marked frame (`submit_indexed` pins @@ -2391,6 +2407,9 @@ impl Encoder for AmfEncoder { true } None => { + // The sweep may have emptied the slot an earlier (un-consumed) force pointed + // at — clear it so the next submit can't force-reference a tainted hardware slot. + self.pending_force = None; tracing::info!( first, last, diff --git a/crates/pf-encode/src/enc/windows/qsv.rs b/crates/pf-encode/src/enc/windows/qsv.rs index c0bf4fbf..e0f72f8a 100644 --- a/crates/pf-encode/src/enc/windows/qsv.rs +++ b/crates/pf-encode/src/enc/windows/qsv.rs @@ -1017,7 +1017,7 @@ impl Encoder for QsvEncoder { self.frame_idx += 1; // --- LTR-RFI per-frame decisions (the AMF policy verbatim; see that module's doc) --- let mut mark_slot: Option = None; - let mut force_slot: Option = None; + let mut force_ltr: Option<(usize, i64)> = None; let mut recovery_anchor = false; if self.ltr_active { if forced { @@ -1035,10 +1035,16 @@ impl Encoder for QsvEncoder { ); } if let Some(slot) = self.pending_force.take() { - force_slot = Some(slot); - recovery_anchor = true; + // Resolve the anchor NOW: a taint sweep in `invalidate_ref_frames` may have + // emptied the slot since the force was queued. An empty slot means there is + // nothing clean to re-reference — the frame must ship as a plain P WITHOUT the + // `recovery_anchor` tag (the client lifts its post-loss freeze on that tag). + if let Some(idx) = self.ltr_slots[slot] { + force_ltr = Some((slot, idx)); + recovery_anchor = true; + } } - if force_slot.is_none() && (forced || cur_idx % self.ltr_mark_interval == 0) { + if force_ltr.is_none() && (forced || cur_idx % self.ltr_mark_interval == 0) { let slot = self.next_ltr_slot; self.ltr_slots[slot] = Some(cur_idx); self.next_ltr_slot = (self.next_ltr_slot + 1) % NUM_LTR_SLOTS; @@ -1046,6 +1052,7 @@ impl Encoder for QsvEncoder { } } let ltr_slots = self.ltr_slots; + let reject_ok = self.codec != Codec::Av1; let inner = self.inner.as_mut().expect("ensure_inner succeeded"); // Bound the in-flight window BEFORE submitting: drain finished AUs (buffered for // `poll`) instead of letting the queue grow under overload. @@ -1130,7 +1137,7 @@ impl Encoder for QsvEncoder { (*surf).Data.TimeStamp = captured.pts_ns.wrapping_mul(9) / 100_000; // 90 kHz // Per-frame control: forced IDR and/or the LTR reflist. let mut ctrl: Option> = None; - if forced || mark_slot.is_some() || force_slot.is_some() { + if forced || mark_slot.is_some() || force_ltr.is_some() { let mut c = FrameCtrl::new(); if forced { c.ctrl.FrameType = (vpl::MFX_FRAMETYPE_IDR @@ -1148,24 +1155,55 @@ impl Encoder for QsvEncoder { c.reflist.ApplyLongTermIdx = 1; use_reflist = true; } - if let Some(slot) = force_slot { - if let Some(ltr_frame) = ltr_slots[slot] { - // Force THIS frame to predict only from the known-good LTR — the - // clean re-anchor. LongTermIdx stays 0 inside PreferredRefList - // (the AV1 runtime rejects nonzero there; AVC/HEVC key on - // FrameOrder). - c.reflist.PreferredRefList[0].FrameOrder = ltr_frame as u32; - c.reflist.PreferredRefList[0].PicStruct = - vpl::MFX_PICSTRUCT_PROGRESSIVE as u16; - use_reflist = true; - tracing::info!( - slot, - ltr_frame, - frame = cur_idx, - "QSV LTR-RFI: re-referencing known-good LTR (clean recovery, \ - no IDR)" - ); + if let Some((slot, ltr_frame)) = force_ltr { + // Force THIS frame to predict only from the known-good LTR — the + // clean re-anchor. LongTermIdx stays 0 inside PreferredRefList + // (the AV1 runtime rejects nonzero there; AVC/HEVC key on + // FrameOrder). + c.reflist.PreferredRefList[0].FrameOrder = ltr_frame as u32; + c.reflist.PreferredRefList[0].PicStruct = + vpl::MFX_PICSTRUCT_PROGRESSIVE as u16; + // A preference alone is a reorder HINT (VPL spec) — the encoder may + // still predict from the tainted short-term refs alongside it. AMF's + // ForceLTRReferenceBitfield and NVENC's invalidation are hard + // exclusions; emulate that here by rejecting every other DPB + // candidate — the short-term sliding window (the 2 most recent + // frames) and the other LTR slot — and capping L0 at one active + // entry. Without this the "clean recovery" frame can carry the + // corruption forward, which the client cannot detect (the field + // failure: permanent macroblock soup under sustained loss). + // AVC/HEVC only: the AV1 runtime's universal-reflist rejection path + // is unvalidated, and an unhonored hint there still converges via + // the host's IDR escalation. + if reject_ok { + let mut rej = 0; + let mut reject = |idx: i64| { + if idx >= 0 && idx != ltr_frame { + c.reflist.RejectedRefList[rej].FrameOrder = idx as u32; + c.reflist.RejectedRefList[rej].PicStruct = + vpl::MFX_PICSTRUCT_PROGRESSIVE as u16; + rej += 1; + } + }; + reject(cur_idx - 1); + reject(cur_idx - 2); + for (s, marked) in ltr_slots.iter().enumerate() { + if s != slot { + if let Some(idx) = *marked { + reject(idx); + } + } + } + c.reflist.NumRefIdxL0Active = 1; } + use_reflist = true; + tracing::info!( + slot, + ltr_frame, + frame = cur_idx, + "QSV LTR-RFI: re-referencing known-good LTR (clean recovery, \ + no IDR)" + ); } if use_reflist { c.attach_reflist(); @@ -1265,6 +1303,17 @@ impl Encoder for QsvEncoder { if !self.ltr_active || first < 0 || first > last { return false; } + // Taint sweep BEFORE picking the anchor: an LTR marked at-or-after the loss start was + // encoded inside the client's corrupt window — the client either never received it or + // decoded it against a broken reference chain. Serving it as "known-good" on a LATER + // loss ships corruption as the recovery anchor, and every subsequent mark re-samples + // the soup — the sustained-loss field failure where the picture never healed. Dropped + // slots stay dropped; the cadence re-marks a clean frame within ~1/4 s. + for marked in self.ltr_slots.iter_mut() { + if marked.is_some_and(|idx| idx >= first) { + *marked = None; + } + } let mut best: Option<(usize, i64)> = None; for (slot, marked) in self.ltr_slots.iter().enumerate() { if let Some(idx) = *marked { @@ -1287,6 +1336,9 @@ impl Encoder for QsvEncoder { true } None => { + // The sweep may have emptied the slot an earlier (un-consumed) force pointed + // at — clear it so the next submit can't half-apply a stale recovery. + self.pending_force = None; tracing::info!( first, last, @@ -1845,6 +1897,36 @@ mod tests { ); } + /// Taint sweep: a loss that predates every live LTR mark leaves NO clean anchor — every + /// slot was marked inside the client's corrupt window. The invalidate must decline (the + /// caller then serves the IDR) and no recovery_anchor AU may ship; before the sweep this + /// force-referenced a tainted mark and shipped corruption tagged as a clean recovery. + #[test] + fn qsv_live_ltr_rfi_taint_sweep_declines() { + let mut rfi_answered = None; + let Some(aus) = drive_live(Codec::H264, false, 60, |enc, i| { + if i == 30 && enc.caps().supports_rfi { + // Frame 0 lost: the IDR itself — every mark (0, 15, ...) is at-or-after it. + rfi_answered = Some(enc.invalidate_ref_frames(0, 2)); + } + }) else { + return; + }; + assert_stream_shape(&aus, 60, true); + let Some(answered) = rfi_answered else { + eprintln!("note: driver declined LTR (supports_rfi=false) — sweep not exercised"); + return; + }; + assert!( + !answered, + "a loss covering every live LTR mark must fall back to IDR recovery" + ); + assert!( + !aus.iter().any(|a| a.recovery_anchor), + "no recovery_anchor AU may ship when the sweep left no clean LTR" + ); + } + /// No-IDR bitrate retarget — Phase 3 on-glass: `reconfigure_bitrate` mid-stream must be /// accepted (HRD off + StartNewSequence=OFF) and must not emit a keyframe. #[test] diff --git a/crates/punktfunk-host/src/native/stream.rs b/crates/punktfunk-host/src/native/stream.rs index 5b12f9c2..2fdb1390 100644 --- a/crates/punktfunk-host/src/native/stream.rs +++ b/crates/punktfunk-host/src/native/stream.rs @@ -1166,6 +1166,21 @@ pub(super) fn virtual_stream(ctx: SessionContext, prepared: Option = Some(std::time::Instant::now()); + // A successful LTR-RFI recovery anchors THIS clock, not the IDR cooldown: it justifies + // swallowing the client's `frames_dropped`-driven echo of the SAME loss (arriving ~one + // loss-window later), but must never indefinitely defer the client's ESCALATION — a + // keyframe request that keeps coming because the RFI recovery did not actually heal its + // decoder. Re-anchoring the full IDR cooldown here (the old behavior) livelocked under + // sustained loss: each new loss → RFI → cooldown re-anchored → the wedged client's IDR + // pleas coalesced away forever, and the picture never recovered (the lid-closed Intel + // laptop field report: permanent macroblock soup, dozens of swallowed requests per IDR). + let mut last_rfi: Option = None; + // Keyframe requests swallowed on RFI-echo grounds since the last real IDR / quiet period. + // Capped: requests past the cap mean RFI is not healing this client — escalate to the IDR. + let mut rfi_echo_swallowed: u32 = 0; + // When the previous keyframe request arrived — a long quiet gap means the client healed + // and the next request opens a NEW loss episode (the echo-swallow budget resets). + let mut last_kf_request: Option = None; // Self-diagnosis for the periodic-stutter class: warns when the served recovery IDRs settle // into a stable multi-second rhythm (see [`pf_frame::metronome::Metronome`]). let mut recovery_cadence = pf_frame::metronome::Metronome::new(); @@ -1536,11 +1551,12 @@ pub(super) fn virtual_stream(ctx: SessionContext, prepared: Option KF_EPISODE_RESET) { + rfi_echo_swallowed = 0; + } + last_kf_request = Some(now); + let idr_recent = last_forced_idr.is_some_and(|t| t.elapsed() < window); + let rfi_echo = last_rfi.is_some_and(|t| t.elapsed() < RFI_ECHO_WINDOW) + && rfi_echo_swallowed < RFI_ECHO_MAX_SWALLOWED; + if idr_recent { tracing::debug!("keyframe request coalesced — within the IDR cooldown"); + } else if rfi_echo { + rfi_echo_swallowed += 1; + tracing::debug!( + swallowed = rfi_echo_swallowed, + "keyframe request coalesced — echo of an RFI-recovered loss" + ); } else { tracing::debug!("forcing keyframe (client decode recovery)"); enc.request_keyframe(); - let now = std::time::Instant::now(); last_forced_idr = Some(now); + rfi_echo_swallowed = 0; // the IDR resets the episode — echoes of IT coalesce via the cooldown if let Some(period) = recovery_cadence.note(now) { tracing::warn!( period_s = format!("{:.1}", period.as_secs_f64()),