From 9491cc8759a1732a1019a5d7bf67ed67277b2acb Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Sat, 25 Jul 2026 21:51:11 +0200 Subject: [PATCH] refactor(pf-encode): extract the range-family RFI recovery policy (WP7.2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The two direct-NVENC backends carried hand-copied twins of the same loss-recovery decision: range validity, covering-range dedup, DPB window, clamp — ~30 duplicated lines each. The decision now lives once as nvenc_core::plan_range_recovery (the range half of WP7.2; the slot half is enc/rfi.rs), pure and unit-tested; each backend keeps its session gate, its unsafe per-timestamp driver loop, and its state stores. The step order is load-bearing and now pinned by tests: the covering dedup runs with the UNCLAMPED last and BEFORE the DPB window (a covered re-ask never touches the driver even when the range has since aged out of the DPB), the boundary at next_ts - RFI_DPB is inclusive, and the Invalidate carries the CLAMPED last — which is also what the caller records in last_rfi_range, exactly as the inline code stored it. A driver failure mid-loop still returns false with NO range recorded and no anchor armed. Decline deliberately clears nothing (neither twin touched pending_anchor on decline — same shape as Vulkan's non-clear, opposite of AMF/QSV; do not harmonize). The exact-cover → Covered test records EXISTING behavior including that a covered range survives a forced IDR with zero driver calls — a recorded fact, not an endorsement. RFI_DPB's import leaves both twins: its only per-backend use was the arithmetic that moved. Co-Authored-By: Claude Opus 5 (1M context) --- crates/pf-encode/src/enc/linux/nvenc_cuda.rs | 74 +++++---- crates/pf-encode/src/enc/nvenc_core.rs | 152 ++++++++++++++++++- crates/pf-encode/src/enc/windows/nvenc.rs | 97 ++++++------ 3 files changed, 232 insertions(+), 91 deletions(-) diff --git a/crates/pf-encode/src/enc/linux/nvenc_cuda.rs b/crates/pf-encode/src/enc/linux/nvenc_cuda.rs index 960a5b49..e274799b 100644 --- a/crates/pf-encode/src/enc/linux/nvenc_cuda.rs +++ b/crates/pf-encode/src/enc/linux/nvenc_cuda.rs @@ -61,9 +61,9 @@ #![deny(clippy::undocumented_unsafe_blocks)] use super::nvenc_core::{ - apply_low_latency_config, build_init_params, cached_ceiling, codec_guid, resolve_slices, - resolve_split_mode, resolve_subframe, store_ceiling, CeilingKey, LowLatencyConfig, NvStatusExt, - RFI_DPB, + apply_low_latency_config, build_init_params, cached_ceiling, codec_guid, plan_range_recovery, + resolve_slices, resolve_split_mode, resolve_subframe, store_ceiling, CeilingKey, + LowLatencyConfig, NvStatusExt, RangePlan, }; use super::nvenc_status; use super::{AuChunk, ChromaFormat, Codec, EncodedFrame, Encoder, EncoderCaps}; @@ -1847,48 +1847,46 @@ impl Encoder for NvencCudaEncoder { } fn invalidate_ref_frames(&mut self, first: i64, last: i64) -> bool { - if self.encoder.is_null() || !self.rfi_supported || first < 0 || first > last { + // Range validity, covering-range dedup, DPB window and clamp all live in + // `nvenc_core::plan_range_recovery` — one policy for both direct-NVENC backends; only the + // session gate and the driver loop are this backend's. + if self.encoder.is_null() || !self.rfi_supported { return false; } - // Already invalidated a covering range for this loss event — re-arm the anchor (the previous - // anchor AU may itself have been lost) but skip the driver calls. - if let Some((pf, pl)) = self.last_rfi_range { - if first >= pf && last <= pl { + match plan_range_recovery(first, last, self.frame_idx, self.last_rfi_range) { + // Already invalidated a covering range for this loss event — re-arm the anchor (the + // previous anchor AU may itself have been lost) but skip the driver calls. + RangePlan::Covered => { self.pending_anchor = true; - return true; + true } - } - // The DPB holds `[frame_idx - RFI_DPB, frame_idx - 1]`; a lost frame older than that can't be - // invalidated, so the only correct recovery is an IDR. - let oldest_in_dpb = self.frame_idx - RFI_DPB as i64; - if first < oldest_in_dpb { - return false; - } - let last = last.min(self.frame_idx - 1); - if first > last { - return false; - } - // Each input's `inputTimeStamp` is the WIRE frame index (pinned by `submit_indexed`), so the - // client's lost-frame range maps 1:1 onto the timestamps NVENC invalidates here. - // SAFETY: `invalidate_ref_frames` is a function pointer from the runtime table; `self.encoder` - // was checked non-null and is the live session; this runs on the encode thread (no concurrent - // NVENC use). Each `ts` is clamped to `[oldest_in_dpb, frame_idx - 1]`, naming a frame still - // in the DPB; the call passes only that `u64` (no struct). - unsafe { - for ts in first..=last { - if (api().invalidate_ref_frames)(self.encoder, ts as u64) - .nv_ok() - .is_err() - { - return false; + RangePlan::Decline => false, + RangePlan::Invalidate { first, last } => { + // Each input's `inputTimeStamp` is the WIRE frame index (pinned by + // `submit_indexed`), so the client's lost-frame range maps 1:1 onto the timestamps + // NVENC invalidates here. + // SAFETY: `invalidate_ref_frames` is a function pointer from the runtime table; + // `self.encoder` was checked non-null and is the live session; this runs on the + // encode thread (no concurrent NVENC use). The plan clamped each `ts` to + // `[oldest_in_dpb, frame_idx - 1]`, naming a frame still in the DPB; the call + // passes only that `u64` (no struct). + unsafe { + for ts in first..=last { + if (api().invalidate_ref_frames)(self.encoder, ts as u64) + .nv_ok() + .is_err() + { + return false; + } + } } + self.last_rfi_range = Some((first, last)); + // The next submitted frame is the clean re-anchor — arm the tag so its AU ships + // with `recovery_anchor` and the client lifts its post-loss freeze on it. + self.pending_anchor = true; + true } } - self.last_rfi_range = Some((first, last)); - // The next submitted frame is the clean re-anchor — arm the tag so its AU ships with - // `recovery_anchor` and the client lifts its post-loss freeze on it. - self.pending_anchor = true; - true } fn poll(&mut self) -> Result> { diff --git a/crates/pf-encode/src/enc/nvenc_core.rs b/crates/pf-encode/src/enc/nvenc_core.rs index 7c91777c..ba835410 100644 --- a/crates/pf-encode/src/enc/nvenc_core.rs +++ b/crates/pf-encode/src/enc/nvenc_core.rs @@ -294,10 +294,158 @@ mod tests { /// Reference-frame DPB depth when RFI is supported (Apollo uses 5). A deeper DPB lets an invalidated /// reference fall back to an older still-valid frame instead of a full IDR; `numRefL0 = 1` keeps each -/// P-frame single-reference for low latency. Also the window the backends' `invalidate_ref_frames` -/// paths check against (`frame_idx - RFI_DPB` = the oldest frame still in the DPB). +/// P-frame single-reference for low latency. Also the window [`plan_range_recovery`] checks against +/// (`next_ts - RFI_DPB` = the oldest frame still in the DPB). pub(super) const RFI_DPB: u32 = 5; +/// One loss event's recovery decision for the timestamp-range RFI both direct-NVENC backends run +/// (the range half of WP7.2's policy extraction; the slot half — AMF/QSV/Vulkan — is +/// `crate::rfi`). The mechanism (the per-timestamp `nvEncInvalidateRefFrames` loop, the +/// `last_rfi_range`/`pending_anchor` stores, the null-handle/`rfi_supported` gate) stays in each +/// backend. +pub(super) enum RangePlan { + /// The last successful invalidation already covers this range — no new driver calls, no IDR. + /// The caller must still RE-ARM its recovery anchor: the client re-asking means the previous + /// anchor AU may itself have been lost, and the next frame is just as clean a re-anchor. + Covered, + /// Invalidate `first..=last` (the CLAMPED range — this is also what the caller must record in + /// `last_rfi_range` on success, exactly as the inline code stored the post-clamp values). + Invalidate { first: i64, last: i64 }, + /// Recovery without an IDR is impossible (nonsense range, loss older than the DPB, or a range + /// entirely in the future) — the caller returns `false` and its (coalesced) keyframe path + /// recovers. Deliberately NOT paired with any state clearing: neither twin touches + /// `pending_anchor` on decline (matching Vulkan's decline, opposite of AMF/QSV's + /// `pending_force` clear — see `crate::rfi`'s module doc before "harmonizing"). + Decline, +} + +/// The range-RFI policy, extracted verbatim from the two backends' `invalidate_ref_frames` (they +/// were hand-copied twins). Step order is load-bearing and pinned by tests: +/// +/// 1. nonsense range (`first < 0 || first > last`) → [`RangePlan::Decline`]; +/// 2. covering-range dedup — checked with the UNCLAMPED `last`, BEFORE the DPB window, so a +/// covered re-ask never touches the driver even when the range has since left the DPB; +/// 3. DPB window: `first < next_ts - RFI_DPB` → Decline (a lost frame older than the DPB cannot +/// be invalidated; the only correct recovery is an IDR); +/// 4. clamp `last` to `next_ts - 1` (never invalidate a timestamp never assigned); an inverted +/// range after the clamp (loss entirely in the future — a prediction desync) → Decline. +/// +/// `next_ts` is the backend's `frame_idx`: the NEXT timestamp to assign, which `submit_indexed` +/// pins to the wire frame index — so the client's lost-frame range maps 1:1 onto the timestamps +/// the driver invalidates, across every rebuild/reset. Note `teardown()` clears `last_rfi_range` +/// but NOT `frame_idx`, so a post-reset call legitimately sees a stale-high `next_ts` with a +/// `None` range — the same view the inline code had. +pub(super) fn plan_range_recovery( + first: i64, + last: i64, + next_ts: i64, + last_rfi_range: Option<(i64, i64)>, +) -> RangePlan { + if first < 0 || first > last { + return RangePlan::Decline; + } + if let Some((pf, pl)) = last_rfi_range { + if first >= pf && last <= pl { + return RangePlan::Covered; + } + } + let oldest_in_dpb = next_ts - RFI_DPB as i64; + if first < oldest_in_dpb { + return RangePlan::Decline; + } + let last = last.min(next_ts - 1); + if first > last { + return RangePlan::Decline; + } + RangePlan::Invalidate { first, last } +} + +#[cfg(test)] +mod range_policy_tests { + use super::{plan_range_recovery, RangePlan, RFI_DPB}; + + /// Convenience: the plan with no prior invalidation recorded. + fn plan(first: i64, last: i64, next_ts: i64) -> RangePlan { + plan_range_recovery(first, last, next_ts, None) + } + + #[test] + fn nonsense_ranges_decline() { + assert!(matches!(plan(-1, 5, 100), RangePlan::Decline)); + assert!(matches!(plan(7, 5, 100), RangePlan::Decline)); + } + + #[test] + fn covering_range_dedups_partial_overlap_does_not() { + let prior = Some((90i64, 95i64)); + // Exact cover and sub-range → Covered. This pins EXISTING behavior, including that a + // covered range survives a forced IDR with zero driver calls (nothing clears + // `last_rfi_range` on a keyframe) — a recorded fact, not an endorsement. + assert!(matches!( + plan_range_recovery(90, 95, 100, prior), + RangePlan::Covered + )); + assert!(matches!( + plan_range_recovery(92, 94, 100, prior), + RangePlan::Covered + )); + // Partial overlap re-invalidates the FULL new range (next_ts = 98 keeps the window open: + // oldest_in_dpb = 93; at next_ts = 100 the same range would age out and Decline instead). + assert!(matches!( + plan_range_recovery(93, 97, 98, prior), + RangePlan::Invalidate { + first: 93, + last: 97 + } + )); + } + + /// The covering check runs BEFORE the DPB window: a covered re-ask stays Covered (no driver + /// calls needed) even when the range has since aged out of the DPB. + #[test] + fn covered_is_checked_before_the_dpb_window() { + let prior = Some((10i64, 12i64)); + assert!(matches!( + plan_range_recovery(10, 12, 100, prior), + RangePlan::Covered + )); + // ...whereas the same range with no prior invalidation is outside the window → Decline. + assert!(matches!(plan(10, 12, 100), RangePlan::Decline)); + } + + #[test] + fn dpb_window_boundary() { + let next_ts = 100i64; + let oldest = next_ts - RFI_DPB as i64; // 95: the oldest timestamp still in the DPB + assert!(matches!( + plan(oldest, oldest, next_ts), + RangePlan::Invalidate { .. } + )); + assert!(matches!( + plan(oldest - 1, oldest, next_ts), + RangePlan::Decline + )); + } + + /// `last` clamps to `next_ts - 1` (the newest encoded frame); the Invalidate carries the + /// CLAMPED value — which is also what the caller records in `last_rfi_range`. + #[test] + fn clamps_to_newest_encoded() { + assert!(matches!( + plan(98, 150, 100), + RangePlan::Invalidate { + first: 98, + last: 99 + } + )); + // A range entirely in the future inverts under the clamp → Decline (prediction desync). + assert!(matches!(plan(100, 150, 100), RangePlan::Decline)); + // Fresh session (`frame_idx == 0`): window passes (oldest = -5) but the clamp gives + // last = -1 < first → Decline. The inline code behaved identically. + assert!(matches!(plan(0, 3, 0), RangePlan::Decline)); + } +} + /// The per-session knobs both direct-NVENC backends feed [`apply_low_latency_config`]. `Copy` so the /// backend fills it from `self` at the call. The two input-format fields bridge the only real /// divergence between the CUDA and D3D11 paths (which surface formats can carry full chroma / 10-bit diff --git a/crates/pf-encode/src/enc/windows/nvenc.rs b/crates/pf-encode/src/enc/windows/nvenc.rs index 7d93e0ec..955738f6 100644 --- a/crates/pf-encode/src/enc/windows/nvenc.rs +++ b/crates/pf-encode/src/enc/windows/nvenc.rs @@ -37,9 +37,9 @@ #![deny(clippy::undocumented_unsafe_blocks)] use super::nvenc_core::{ - apply_low_latency_config, build_init_params, cached_ceiling, codec_guid, resolve_slices, - resolve_split_mode, resolve_subframe, store_ceiling, CeilingKey, LowLatencyConfig, NvStatusExt, - RFI_DPB, + apply_low_latency_config, build_init_params, cached_ceiling, codec_guid, plan_range_recovery, + resolve_slices, resolve_split_mode, resolve_subframe, store_ceiling, CeilingKey, + LowLatencyConfig, NvStatusExt, RangePlan, }; use super::nvenc_status; use super::{ChromaFormat, Codec, EncodedFrame, Encoder, EncoderCaps}; @@ -1679,61 +1679,56 @@ impl Encoder for NvencD3d11Encoder { } fn invalidate_ref_frames(&mut self, first: i64, last: i64) -> bool { - // No live session, the GPU can't invalidate, or a nonsense range → caller forces a full IDR. - // (NVENC handles are single-threaded; this runs on the encode thread, like submit/poll.) - if self.encoder.is_null() || !self.rfi_supported || first < 0 || first > last { + // No live session or the GPU can't invalidate → caller forces a full IDR. (NVENC handles + // are single-threaded; this runs on the encode thread, like submit/poll.) Everything else + // — range validity, covering-range dedup, the DPB window, the clamp — is + // `nvenc_core::plan_range_recovery`, one policy for both direct-NVENC backends. + if self.encoder.is_null() || !self.rfi_supported { return false; } - // Already invalidated a covering range for this loss event — no new driver calls needed, - // no IDR. RE-ARM the anchor though: the client re-asking means the previous recovery - // anchor AU may itself have been lost, and the next frame is just as clean a re-anchor - // (it too references only valid frames). - if let Some((pf, pl)) = self.last_rfi_range { - if first >= pf && last <= pl { + match plan_range_recovery(first, last, self.frame_idx, self.last_rfi_range) { + // Already invalidated a covering range for this loss event — no new driver calls + // needed, no IDR. RE-ARM the anchor though: the client re-asking means the previous + // recovery anchor AU may itself have been lost, and the next frame is just as clean a + // re-anchor (it too references only valid frames). + RangePlan::Covered => { self.pending_anchor = true; - return true; + true } - } - // `frame_idx` is the NEXT timestamp to assign, so the last encoded frame is `frame_idx - 1` - // and the DPB holds `[frame_idx - RFI_DPB, frame_idx - 1]`. A lost frame older than that - // can't be invalidated, so the only correct recovery is an IDR. - let oldest_in_dpb = self.frame_idx - RFI_DPB as i64; - if first < oldest_in_dpb { - return false; - } - // Clamp to frames we've actually encoded (don't invalidate a timestamp we never assigned). - let last = last.min(self.frame_idx - 1); - if first > last { - return false; - } - // Each input's `inputTimeStamp` is `frame_idx`, which `submit_indexed` pins to the WIRE - // frame index the AU carries — so the client's lost-frame range maps 1:1 onto the - // timestamps NVENC invalidates here, and stays 1:1 across encoder rebuilds/resets (an - // internal counter would desync on the first adaptive-bitrate rebuild and RFI would then - // clamp every range into first > last, silently degrading to IDR-only forever). - // SAFETY: `invalidate_ref_frames` is a function pointer from the runtime-loaded `EncodeApi` table. - // `self.encoder` was checked non-null at the top of this fn and is the live session; this runs - // on the encode thread (like submit/poll), so there is no concurrent NVENC use. Each `ts` was - // clamped to `[oldest_in_dpb, frame_idx - 1]` above, so it names a frame still in the session's - // DPB; the call passes only that `u64` timestamp (no struct), so there is no struct-size or - // lifetime concern. - unsafe { - for ts in first..=last { - if (api().invalidate_ref_frames)(self.encoder, ts as u64) - .nv_ok() - .is_err() - { - return false; // any failure → fall back to IDR + RangePlan::Decline => false, + RangePlan::Invalidate { first, last } => { + // Each input's `inputTimeStamp` is `frame_idx`, which `submit_indexed` pins to the + // WIRE frame index the AU carries — so the client's lost-frame range maps 1:1 onto + // the timestamps NVENC invalidates here, and stays 1:1 across encoder + // rebuilds/resets (an internal counter would desync on the first adaptive-bitrate + // rebuild and RFI would then clamp every range into first > last, silently + // degrading to IDR-only forever). + // SAFETY: `invalidate_ref_frames` is a function pointer from the runtime-loaded + // `EncodeApi` table. `self.encoder` was checked non-null at the top of this fn and + // is the live session; this runs on the encode thread (like submit/poll), so there + // is no concurrent NVENC use. The plan clamped each `ts` to + // `[oldest_in_dpb, frame_idx - 1]`, so it names a frame still in the session's + // DPB; the call passes only that `u64` timestamp (no struct), so there is no + // struct-size or lifetime concern. + unsafe { + for ts in first..=last { + if (api().invalidate_ref_frames)(self.encoder, ts as u64) + .nv_ok() + .is_err() + { + return false; // any failure → fall back to IDR + } + } } + self.last_rfi_range = Some((first, last)); + // The next submitted frame is the first one encoded after the invalidation — the + // clean re-anchor P-frame. Arm the tag so its AU ships with `recovery_anchor` and + // the client lifts its post-loss freeze on it (instead of waiting ~1 s for the + // cooldown-suppressed IDR fallback). + self.pending_anchor = true; + true } } - self.last_rfi_range = Some((first, last)); - // The next submitted frame is the first one encoded after the invalidation — the clean - // re-anchor P-frame. Arm the tag so its AU ships with `recovery_anchor` and the client - // lifts its post-loss freeze on it (instead of waiting ~1 s for the cooldown-suppressed - // IDR fallback). - self.pending_anchor = true; - true } fn poll(&mut self) -> Result> {