From 87863c12a9134367731fa027b92ac9ae8f79ba93 Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Sat, 25 Jul 2026 14:35:35 +0200 Subject: [PATCH] =?UTF-8?q?fix(encode/windows):=20clear=20the=20Windows=20?= =?UTF-8?q?Phase=204=20backlog=20=E2=80=94=20NVENC=20sticky=20state,=20QSV?= =?UTF-8?q?=20HDR=20+=20ABR?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four items, gated on the RTX box (.173, all 7 Windows legs) and the Linux docker gate, with the QSV HDR one proven by an A/B on real Intel silicon (.42, UHD 750). WP4.1 — Windows NVENC sticky state. Both halves were the same mistake: a single field doing duty as both "what the session negotiated" and "what this session can actually do", so the first downgrade destroyed the negotiation. - `query_caps` clears `self.hdr` on a GPU without 10-bit encode, but `submit` re-derives the requested HDR state from every frame's pixel format and compared it against that cleared value. On such a GPU a P010 capturer therefore reported "HDR changed" on EVERY FRAME and tore down and rebuilt the whole encode session each time. The trigger now compares against `hdr_requested`, and the downgrade is latched in `hdr_unsupported` so it is remembered (and warned once) instead of rediscovered per init. - One subsampled-YUV frame cleared `chroma_444` permanently, so 4:4:4 never returned when the capturer went back to RGB. The negotiation now lives in `chroma_444_requested` and the effective value is recomputed at every init; the per-session downgrade still happens and is still reported honestly through `caps()`, it just is not destructive any more. WP4.3(a) — QSV HDR mastering luminance was divided by 10,000. The old comment justified it as "VPL wants whole cd/m²", which is true of the VIDEO-PROCESSING unit but not the encode path: the runtime passes the value straight into the ITU-T H.265 Annex D mastering-display SEI, whose unit is 0.0001 cd/m². Verified end to end on Intel UHD 750 by dumping the bitstream and reading SEI 137 with trace_headers, before and after: before: max_display_mastering_luminance = 1000 (0.1 nits) after: max_display_mastering_luminance = 10000000 (1000 nits) with `min` (500 = 0.05 nits) and both content-light-level fields unchanged. That asymmetry — a correct min beside a 10,000x-low max — was the original tell. WP4.3(b) — `reconfigure_bitrate` never re-read `BufferSizeInKB`, so an ABR step-up could hand the runtime an AU buffer sized for the old, lower bitrate. Fixed in both places it was broken: re-read the driver's answer via `GetVideoParam` after the `Reset` (mirroring `init_encode`), AND stop `take_bs` recycling pooled buffers without checking their capacity — the pool would otherwise keep serving short buffers even once `bs_bytes` was correct. WP7.8 — the `PUNKTFUNK_QSV_FFMPEG` half, finally landable now that a Windows gate exists (it sits behind `#[cfg(feature = "qsv")]`, so nothing in a Linux-only matrix compiles it — it was deliberately held back for exactly that reason). Trimmed like its siblings, and kept case-insensitive: the knob already accepted `TRUE`, and the bare house `matches!` would have silently dropped that spelling. Phase 4 of the audit is now complete, and WP7.8 with it. Verified: Windows gate on .173 — clippy -D warnings at nvenc,amf-qsv,qsv (host + pf-encode --all-targets), amf-qsv without qsv, qsv alone, no-features, `cargo test --features qsv` (34 passed), rustfmt. Linux docker gate L1-L4 green. On-glass: the QSV A/B above on .42. Owed: NVENC sticky-state on-glass on .173 (needs a live 10-bit-capable capture toggle). --- crates/pf-encode/src/enc/windows/nvenc.rs | 47 +++++++++++++++-- crates/pf-encode/src/enc/windows/qsv.rs | 61 ++++++++++++++++++++--- crates/pf-encode/src/lib.rs | 13 ++++- 3 files changed, 107 insertions(+), 14 deletions(-) diff --git a/crates/pf-encode/src/enc/windows/nvenc.rs b/crates/pf-encode/src/enc/windows/nvenc.rs index 82083af2..5b6be320 100644 --- a/crates/pf-encode/src/enc/windows/nvenc.rs +++ b/crates/pf-encode/src/enc/windows/nvenc.rs @@ -388,6 +388,12 @@ pub struct NvencD3d11Encoder { /// `NV_ENC_CAPS_SUPPORT_YUV444_ENCODE` (cleared in `query_caps` on a card that lacks it) and on an /// RGB input format (NV12/P010 capture can't reconstruct 4:4:4). HEVC-only. chroma_444: bool, + /// What the SESSION NEGOTIATED, before any per-init downgrade. `chroma_444` above is the + /// EFFECTIVE value for the session currently open and is cleared whenever the capturer hands us + /// subsampled YUV — which used to overwrite the negotiation itself, so a capturer that went + /// back to RGB (HDR toggle, capture-path switch) could never recover 4:4:4 for the rest of the + /// stream. Keeping the request separate lets every re-init recompute the effective value. + chroma_444_requested: bool, /// `NV_ENC_CAPS_SUPPORT_YUV444_ENCODE` from the caps probe — whether this GPU can 4:4:4 encode at /// all. `chroma_444` is forced off when this is false (graceful downgrade to 4:2:0). yuv444_supported: bool, @@ -395,6 +401,17 @@ pub struct NvencD3d11Encoder { /// `ABGR10` input format + the BT.2020/PQ colour VUI. Derived per-frame from the capture format /// (HDR can toggle mid-session); a change re-inits the session. hdr: bool, + /// The HDR state the CAPTURE FORMAT asks for, independent of whether this GPU can serve it. + /// + /// `hdr` above is the effective value and `query_caps` clears it on a card without 10-bit + /// encode. `submit` re-derives the requested value from every frame's pixel format, so + /// comparing that against the post-downgrade `hdr` made them disagree permanently: a P010 + /// capturer on such a GPU reported "HDR changed" on EVERY frame and tore down and rebuilt the + /// whole session each time. The re-init trigger compares against THIS instead. + hdr_requested: bool, + /// Latched when `query_caps` finds no 10-bit encode support, so the downgrade is remembered + /// across re-inits instead of being rediscovered (and re-warned) every session. + hdr_unsupported: bool, /// The source's static HDR mastering metadata (from the capturer's `GetDesc1`), emitted as /// in-band SEI (`mastering_display_colour_volume` + `content_light_level_info`) on each keyframe /// when `hdr`. `None` = unknown → no SEI (the VUI still signals BT.2020 PQ). Set per-frame via @@ -505,6 +522,9 @@ impl NvencD3d11Encoder { bit_depth, // 4:4:4 is HEVC-only; the GPU-support gate is applied in `query_caps`. chroma_444: chroma.is_444() && codec == Codec::H265, + chroma_444_requested: chroma.is_444() && codec == Codec::H265, + hdr_requested: false, + hdr_unsupported: false, yuv444_supported: false, hdr: false, hdr_meta: None, @@ -672,7 +692,12 @@ impl NvencD3d11Encoder { } // Degrade gracefully rather than fail: no 10-bit encode on this card → 8-bit SDR. if self.bit_depth >= 10 && ten_bit == 0 { - tracing::warn!("NVENC: this GPU can't 10-bit encode — falling back to 8-bit SDR"); + if !self.hdr_unsupported { + tracing::warn!("NVENC: this GPU can't 10-bit encode — falling back to 8-bit SDR"); + } + // Latched: `submit` compares the frame's REQUESTED hdr against `hdr_requested`, not + // against this cleared value, so a 10-bit capturer no longer re-inits every frame. + self.hdr_unsupported = true; self.bit_depth = 8; self.hdr = false; } @@ -1124,7 +1149,11 @@ impl Encoder for NvencD3d11Encoder { let dev_raw = frame.device.as_raw(); let size_changed = self.inited && (self.width != captured.width || self.height != captured.height); - let hdr_changed = self.inited && self.hdr != hdr; + // Compare against what was REQUESTED last init, not the effective `self.hdr`: on a GPU + // without 10-bit encode `query_caps` clears `self.hdr`, so comparing it against a P010 + // capturer's perpetual `hdr = true` reported a change on every single frame and tore the + // session down and rebuilt it each time. + let hdr_changed = self.inited && self.hdr_requested != hdr; if self.inited && (self.init_device != dev_raw || size_changed || hdr_changed) { tracing::info!( device_changed = self.init_device != dev_raw, @@ -1145,7 +1174,13 @@ impl Encoder for NvencD3d11Encoder { // Adopt the current frame size + colour so the encoder always matches the capturer output. self.width = captured.width; self.height = captured.height; + self.hdr_requested = hdr; + // Effective until `query_caps` says otherwise (it clears this on a card without 10-bit). self.hdr = hdr; + // Recompute the EFFECTIVE 4:4:4 from the negotiation on every init. This used to read + // (and overwrite) `chroma_444` itself, so the first subsampled-YUV frame permanently + // demoted the session — 4:4:4 never returned even when the capturer went back to RGB. + self.chroma_444 = self.chroma_444_requested; // Pick the NVENC input format from the captured pixel format. YUV (NV12/P010) is the // video-processor path — NVENC encodes it natively (no internal RGB→YUV, which is a hidden // 3D/compute step that would fight a GPU-saturating game). RGB (ARGB/ABGR10) is the legacy @@ -1171,9 +1206,11 @@ impl Encoder for NvencD3d11Encoder { }; // 4:4:4 honesty: the FREXT/chromaFormatIDC=3 config engages only on an RGB input (a // subsampled NV12/P010 source can't reconstruct full chroma). If the capturer handed - // native YUV despite a 4:4:4 negotiation, this session encodes 4:2:0 — clear the flag - // NOW so `caps().chroma_444` (and native's post-open cross-check) reports what - // the stream really carries instead of silently claiming full chroma. + // native YUV despite a 4:4:4 negotiation, THIS session encodes 4:2:0 — clear the + // effective flag now so `caps().chroma_444` (and native's post-open cross-check) + // reports what the stream really carries instead of silently claiming full chroma. + // Only the effective value is cleared: `chroma_444_requested` keeps the negotiation, so + // a later re-init on an RGB capture recovers 4:4:4 instead of being stuck at 4:2:0. if self.chroma_444 && !matches!( self.buffer_fmt, diff --git a/crates/pf-encode/src/enc/windows/qsv.rs b/crates/pf-encode/src/enc/windows/qsv.rs index c2315b90..a77f2512 100644 --- a/crates/pf-encode/src/enc/windows/qsv.rs +++ b/crates/pf-encode/src/enc/windows/qsv.rs @@ -519,9 +519,20 @@ fn build_params(cfg: &EncodeConfig) -> ParamSet { } b.WhitePointX = m.white_point[0]; b.WhitePointY = m.white_point[1]; - // Units diverge on the max: VPL wants whole cd/m² (HdrMeta carries 0.0001 cd/m²); the - // min is 0.0001 cd/m² on both sides. - b.MaxDisplayMasteringLuminance = m.max_display_mastering_luminance / 10_000; + // BOTH luminance fields are 0.0001 cd/m² here — do NOT scale the max. + // + // The `/10_000` this used to carry followed the VPL header's *video-processing* unit + // (whole cd/m², which is right for VPP), but on the ENCODE path the runtime passes these + // straight into the ITU-T H.265 Annex D mastering-display SEI, whose unit is 0.0001 cd/m². + // Dividing therefore under-reported peak brightness by 10,000x. Confirmed in a real + // bitstream on Intel UHD 750 (2026-07-25): SEI 137 carried + // max_display_mastering_luminance = 1000, i.e. **0.1 nits** for a 1000-nit display, while + // the undivided min = 500 (0.05 nits) was correct — the asymmetry was the tell. A client + // tone-mapping against 0.1 nits crushes the image. + // + // Unscaled also matches every sibling: `MinDisplayMasteringLuminance` right below, + // `pf_frame::hdr::hevc_mastering_display_sei`, and the AMF backend. + b.MaxDisplayMasteringLuminance = m.max_display_mastering_luminance; b.MinDisplayMasteringLuminance = m.min_display_mastering_luminance; b }); @@ -690,13 +701,17 @@ impl Inner { } fn take_bs(&mut self) -> Box { - match self.bs_pool.pop() { - Some(mut b) => { + // Pooled buffers were sized by whatever `bs_bytes` was when they were allocated. A bitrate + // retarget raises the driver's worst-case AU size, so a recycled buffer can be SMALLER than + // the current requirement — hand those back to the allocator instead of letting the runtime + // write an AU into a short buffer. + while let Some(mut b) = self.bs_pool.pop() { + if b.mfx.MaxLength as usize >= self.bs_bytes { b.recycle(); - b + return b; } - None => BsBuf::new(self.bs_bytes), } + BsBuf::new(self.bs_bytes) } } @@ -1583,6 +1598,38 @@ impl Encoder for QsvEncoder { self.bitrate_bps = old; return false; } + // Re-read the driver's worst-case AU size. `Reset` re-derives `BufferSizeInKB` from the new + // rate, and a step-up can raise it well above what `init_encode` computed — leaving + // `bs_bytes` (and every buffer already in the pool) sized for the OLD, lower bitrate. This + // mirrors `init_encode`, which asks the same question post-Init; `take_bs` drops any pooled + // buffer that is now too small. + // SAFETY: `session` is live on this thread and drained above; `got` and its (empty) ext + // chain outlive the synchronous call. + let refreshed = unsafe { + let mut got: vpl::mfxVideoParam = std::mem::zeroed(); + let sts = vpl::MFXVideoENCODE_GetVideoParam(session, &mut got); + (sts >= vpl::MFX_ERR_NONE).then(|| { + let m = &mut got.__bindgen_anon_1.mfx; + let mult = m.BRCParamMultiplier.max(1) as usize; + let kb = enc_of(m).BufferSizeInKB as usize; + (kb * mult * 1000).max(256 * 1024) + }) + }; + match (refreshed, self.inner.as_mut()) { + (Some(bytes), Some(inner)) if bytes > inner.bs_bytes => { + tracing::debug!( + old_bytes = inner.bs_bytes, + new_bytes = bytes, + mbps = bps / 1_000_000, + "QSV retarget raised the worst-case AU size — resizing the bitstream pool" + ); + inner.bs_bytes = bytes; + } + (None, _) => tracing::warn!( + "QSV retarget: GetVideoParam failed, keeping the previous AU buffer size" + ), + _ => {} + } true } diff --git a/crates/pf-encode/src/lib.rs b/crates/pf-encode/src/lib.rs index 84f125a6..59d1694d 100644 --- a/crates/pf-encode/src/lib.rs +++ b/crates/pf-encode/src/lib.rs @@ -657,8 +657,17 @@ fn open_video_backend( // Phase 4 after the field-silence gate. #[cfg(feature = "qsv")] { - let ffmpeg_forced = std::env::var("PUNKTFUNK_QSV_FFMPEG") - .is_ok_and(|v| v == "1" || v.eq_ignore_ascii_case("true")); + // Trimmed like every sibling knob, so `"1 "` from a shell script or a `.env` + // line takes effect instead of silently doing nothing. Case-insensitivity is + // kept deliberately — this knob already accepted `TRUE` via + // `eq_ignore_ascii_case`, and the bare house `matches!` would have dropped + // that spelling; widening to `yes`/`on` only adds accepted values. + let ffmpeg_forced = std::env::var("PUNKTFUNK_QSV_FFMPEG").is_ok_and(|v| { + matches!( + v.trim().to_ascii_lowercase().as_str(), + "1" | "true" | "yes" | "on" + ) + }); if !ffmpeg_forced { match qsv::QsvEncoder::open( codec,