From 6be174dc9cc83772cae8e8993446b04b9d224fb0 Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Sat, 25 Jul 2026 00:05:45 +0200 Subject: [PATCH] fix(encode/windows): the encode bit depth must follow the pixels, not the negotiation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All three Windows backends derived it the same wrong way — `bit_depth >= 10 || matches!(format, P010 | Rgb10a2)` at ffmpeg_win.rs:154, amf.rs:1275 and qsv.rs:777 — so the NEGOTIATED depth could force a 10-bit encoder over an 8-bit capture. That combination is not hypothetical. A client advertises 10-bit, the handshake negotiates bit_depth=10, and then enabling advanced colour on the IDD virtual display fails — at which point the capturer says exactly that and delivers 8-bit NV12 anyway ("10-bit HDR was negotiated but enabling advanced color on the virtual display FAILED — encoding 8-bit SDR"). Every vendor then lost the session, each in its own way: - native AMF and native QSV derived `expected = P010`, saw Nv12, and bail!'d at open; - the libavcodec path accepted the open, built a P010 encoder, and then failed EVERY submit forever, because its per-frame check recomputes the depth from the frame and never matches. reset() could not help: the rebuild re-derived the same wrong depth from the same stored bit_depth. The host burned MAX_ENCODER_RESETS and ended the session. The last one is the worst of the three because it IS the fallback: native QSV correctly refuses this input at open, and lib.rs then falls back to the ffmpeg path "for robustness" — which accepted it and died per frame instead. Since the duplication was the bug, the fix is one shared `ten_bit_input()` in codec.rs that all three call, and it follows the delivered pixels. That also keeps the stream HONEST rather than merely alive: the depth selects the colour signalling (BT.2020 PQ vs BT.709) and the staging surface format, so an 8-bit capture now produces an 8-bit stream that says it is SDR — which is what the capturer already reported it was sending. It warns when the negotiated depth is discarded. The negotiated depth stays an upper bound. The session LABEL may still claim HDR; that mismatch lives in the negotiation, not in the encoder, and is not addressed here. `is_10bit_format` keeps its (now format-only) definition and is used by submit_d3d11's per-frame check, so the predicate the encoder was built from and the one it re-checks per frame cannot drift. That check can now only fire on a genuine mid-stream depth change. Verified `--all-targets -D warnings` on Windows (no features / pyrowave / qsv / nvenc,qsv; 31 tests) and Linux (default; shipped nvenc+vulkan-encode+pyrowave). Co-Authored-By: Claude Opus 5 (1M context) --- crates/pf-encode/src/enc/codec.rs | 39 +++++++++++++++++++ crates/pf-encode/src/enc/windows/amf.rs | 6 ++- .../pf-encode/src/enc/windows/ffmpeg_win.rs | 19 ++++++--- crates/pf-encode/src/enc/windows/qsv.rs | 7 +++- 4 files changed, 64 insertions(+), 7 deletions(-) diff --git a/crates/pf-encode/src/enc/codec.rs b/crates/pf-encode/src/enc/codec.rs index 520b9b2d..e6cbafc3 100644 --- a/crates/pf-encode/src/enc/codec.rs +++ b/crates/pf-encode/src/enc/codec.rs @@ -7,6 +7,45 @@ use anyhow::Result; use pf_frame::CapturedFrame; +/// Whether an encoder fed `format` must be built 10-bit — decided by **the pixels that actually +/// arrive**, never by the negotiated `bit_depth`. +/// +/// The three Windows backends each derived this as `bit_depth >= 10 || matches!(format, P010 | +/// Rgb10a2)`, i.e. the *negotiated* depth could force a 10-bit encoder over an 8-bit capture. That +/// combination is not hypothetical: a client advertises 10-bit, the handshake negotiates +/// `bit_depth = 10`, and then enabling advanced colour on the IDD virtual display fails — at which +/// point the capturer says so and delivers 8-bit NV12 anyway (`pf-capture`'s idd_push logs "10-bit +/// HDR was negotiated but enabling advanced color on the virtual display FAILED — encoding 8-bit +/// SDR"). Every backend then lost the session, each in its own way: native AMF and native QSV +/// `bail!` at open because the format does not match the P010 they derived, and the libavcodec +/// path accepted the open and then failed EVERY submit forever (its per-frame depth check +/// recomputes from the frame, which never matches), where `reset()` could not help because the +/// rebuild re-derived the same wrong depth. +/// +/// Following the pixels keeps the stream alive and, more importantly, keeps it HONEST: the depth +/// also selects the colour signalling (BT.2020 PQ vs BT.709) and the staging surface format, so an +/// 8-bit capture now yields an 8-bit stream that says it is SDR — which is what the capturer +/// already reported it is sending. The negotiated depth remains an upper bound; the session label +/// may still claim HDR, and that mismatch belongs to the negotiation, not to the encoder. +/// Windows-only: the three backends that derive an encoder depth from a capture live there +/// (native AMF, native QSV, libavcodec AMF/QSV). The Linux backends take the depth from the +/// negotiated `bit_depth` alone because their capture formats carry it unambiguously. +#[cfg(target_os = "windows")] +pub(crate) fn ten_bit_input(format: pf_frame::PixelFormat, negotiated_depth: u8) -> bool { + use pf_frame::PixelFormat; + let ten = matches!(format, PixelFormat::P010 | PixelFormat::Rgb10a2); + if negotiated_depth >= 10 && !ten { + tracing::warn!( + ?format, + negotiated_depth, + "session negotiated 10-bit but the capturer delivers an 8-bit format — encoding 8-bit \ + SDR (the stream's colour signalling follows the pixels; check whether advanced colour \ + failed to enable on the virtual display)" + ); + } + ten +} + /// An encoded access unit (one NAL/AU) to hand to `punktfunk_core` for FEC + packetization. /// `data` is in-band Annex-B (the encoder is opened without a global header), so each /// keyframe carries its own VPS/SPS/PPS — the bytes are both a playable elementary diff --git a/crates/pf-encode/src/enc/windows/amf.rs b/crates/pf-encode/src/enc/windows/amf.rs index c5dca47b..ec420dc3 100644 --- a/crates/pf-encode/src/enc/windows/amf.rs +++ b/crates/pf-encode/src/enc/windows/amf.rs @@ -1272,7 +1272,11 @@ impl AmfEncoder { if codec == Codec::Av1 && !probe_can_encode(Codec::Av1) { bail!("this GPU/driver declined AV1 encode (RDNA3+ required) — native AMF probe"); } - let ten_bit = bit_depth >= 10 || matches!(format, PixelFormat::P010 | PixelFormat::Rgb10a2); + // Depth follows the delivered pixels, not the negotiated depth ([`crate::ten_bit_input`]). + // With the old `bit_depth >= 10 || …` shape a 10-bit-negotiated session over an 8-bit + // capture derived `expected = P010`, tripped the format check below and ended the session + // at open — on exactly the configuration the capturer had already downgraded on purpose. + let ten_bit = crate::ten_bit_input(format, bit_depth); // Zero-copy by construction: the input ring is NV12/P010 fed by same-format // CopySubresourceRegion. Any other capture format (Bgra/Rgb10a2 video-processor fallback, // CPU frames) has no native input path — and since Phase 3 no ffmpeg readback to degrade diff --git a/crates/pf-encode/src/enc/windows/ffmpeg_win.rs b/crates/pf-encode/src/enc/windows/ffmpeg_win.rs index f5a8cc96..d82d600f 100644 --- a/crates/pf-encode/src/enc/windows/ffmpeg_win.rs +++ b/crates/pf-encode/src/enc/windows/ffmpeg_win.rs @@ -151,8 +151,14 @@ fn sws_src(format: PixelFormat) -> Result { } /// Does this captured format imply a 10-bit encode (P010 / Rgb10a2)? -fn is_10bit_format(format: PixelFormat, bit_depth: u8) -> bool { - bit_depth >= 10 || matches!(format, PixelFormat::P010 | PixelFormat::Rgb10a2) +/// +/// Depth follows the PIXELS, not the negotiated `bit_depth` — see +/// [`crate::ten_bit_input`] for why, and for the failure this shape used to produce here in +/// particular: a 10-bit-negotiated session over an 8-bit capture built a P010 encoder whose every +/// `submit_d3d11` then failed the depth check below, forever, with `reset()` unable to help +/// because the rebuild re-derived the same wrong answer. +fn is_10bit_format(format: PixelFormat) -> bool { + matches!(format, PixelFormat::P010 | PixelFormat::Rgb10a2) } /// Build the FFmpeg encoder context shared by both inner paths: name, mode, low-latency RC, @@ -350,7 +356,7 @@ impl SystemInner { bitrate_bps: u64, bit_depth: u8, ) -> Result { - let ten_bit = is_10bit_format(format, bit_depth); + let ten_bit = crate::ten_bit_input(format, bit_depth); let sw_av = if ten_bit { ffi::AVPixelFormat::AV_PIX_FMT_P010LE } else { @@ -472,7 +478,10 @@ impl SystemInner { pts: i64, idr: bool, ) -> Result<()> { - let fmt_10 = matches!(format, PixelFormat::P010 | PixelFormat::Rgb10a2); + // Same predicate the encoder was built from (`ten_bit_input`), so this can only fire on a + // genuine MID-STREAM depth change — never, as it used to, on every frame of a session that + // merely negotiated 10-bit over an 8-bit capture. + let fmt_10 = is_10bit_format(format); anyhow::ensure!( fmt_10 == self.ten_bit, "captured format {format:?} bit-depth changed under the encoder (built {}-bit)", @@ -851,7 +860,7 @@ impl ZeroCopyInner { bit_depth: u8, device: &ID3D11Device, ) -> Result { - let ten_bit = is_10bit_format(format, bit_depth); + let ten_bit = crate::ten_bit_input(format, bit_depth); let sw_av = if ten_bit { ffi::AVPixelFormat::AV_PIX_FMT_P010LE } else { diff --git a/crates/pf-encode/src/enc/windows/qsv.rs b/crates/pf-encode/src/enc/windows/qsv.rs index c90dadf6..f70a3925 100644 --- a/crates/pf-encode/src/enc/windows/qsv.rs +++ b/crates/pf-encode/src/enc/windows/qsv.rs @@ -774,7 +774,12 @@ impl QsvEncoder { if codec == Codec::Av1 && !probe_can_encode(Codec::Av1) { bail!("this GPU/driver declined AV1 encode (DG2/Arc or MTL+ required) — QSV probe"); } - let ten_bit = bit_depth >= 10 || matches!(format, PixelFormat::P010 | PixelFormat::Rgb10a2); + // Depth follows the delivered pixels, not the negotiated depth ([`crate::ten_bit_input`]). + // With the old `bit_depth >= 10 || …` shape a 10-bit-negotiated session over an 8-bit + // capture derived `expected = P010` and hit the bail below, ending the session at open — + // and taking the ffmpeg fallback with it, since that path had the same defect in a worse + // form (it accepted the open and then failed every frame). + let ten_bit = crate::ten_bit_input(format, bit_depth); if ten_bit && codec == Codec::H264 { bail!("native QSV: 10-bit is HEVC/AV1-only (H.264 High10 is not negotiated)"); }