From a86edba4cabd79206b279a152b261bebf41f02c0 Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Fri, 28 Aug 2026 09:02:59 +0200 Subject: [PATCH] =?UTF-8?q?fix(client):=20don't=20advertise=20HDR=20a=20vi?= =?UTF-8?q?deo=20processor=20can't=20tone-map=20=E2=80=94=20the=20green-mo?= =?UTF-8?q?nitor=20Arc=20report?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit VIDEO_CAP_HDR invites a PQ stream, and on a Windows box with no HDR10 swapchain the D3D11VA hand-off shows it through the video processor's PQ->sRGB tonemap — a driver capability nothing ever validated. The Blt accepts the colorspaces either way and renders garbage where the conversion is missing (host-side sibling: NVIDIA RGB->P010 'renders green'). Field 2026-08-26: an Arc A370M client went green on every HDR session while AV1 8-bit SDR at the same 2880x1620@120 streamed clean. video_d3d11 gains pq_tonemap_supported() — one throwaway device + enumerator asked CheckVideoProcessorFormatConversion for the exact pair the SDR ring sets (P010 G2084 studio in, BGRA8 sRGB out). Only the driver's definitive 'no' answers false; API failure keeps today's behaviour. video::hdr_presentable() wraps it behind the short-circuits that make it free everywhere it doesn't matter (!d3d11_import, d3d11_hdr10 pass-through, non-Windows shader tonemap), and the session binary ANDs the HDR setting with it exactly like the 4:4:4 promise — loud warn when the switch is on and not honored. ten_bit_sdr stays unprobed on purpose: 10-bit SDR is no tonemap. Also: log_layout_once keys on decoder x layout instead of decoder alone, so a mid-stream Reconfigure or SDR<->PQ flip re-logs the frame-vs-pool dims — the fact a green-bar/smear report hinges on — for the shape that failed, not just the one the session started at. Verified: cargo fmt + dockerized Linux clippy -D warnings green for pf-client-core + punktfunk-client-session; the cfg(windows) half needs the Windows CI leg (not compilable from this box). --- clients/session/src/main.rs | 21 +++- crates/pf-client-core/src/video.rs | 62 +++++++++--- crates/pf-client-core/src/video_d3d11.rs | 120 +++++++++++++++++++---- 3 files changed, 170 insertions(+), 33 deletions(-) diff --git a/clients/session/src/main.rs b/clients/session/src/main.rs index fa17f221..6fc9603b 100644 --- a/clients/session/src/main.rs +++ b/clients/session/src/main.rs @@ -369,6 +369,22 @@ mod session_main { to (PyroWave carries 4:4:4 on any GPU, if the link can take it)." ); } + // …and the HDR promise, same discipline: `VIDEO_CAP_HDR` invites a PQ stream, and + // a Windows box with no HDR10 swapchain whose video processor cannot tone-map + // PQ→sRGB shows that stream as garbage — the D3D11VA Blt accepts the colorspaces + // and renders green where the conversion is missing (Arc A370M field report, + // 2026-08-26). `ten_bit_sdr` is deliberately NOT gated on this: a 10-bit SDR + // stream is no tonemap, and every hardware rung decodes P010. + let hdr_enabled = + settings.hdr_enabled && pf_client_core::video::hdr_presentable(vulkan.as_ref()); + if settings.hdr_enabled && !hdr_enabled { + tracing::warn!( + "HDR requested but this device cannot present a PQ stream (no HDR10 \ + swapchain, and the video processor reports no PQ→sRGB conversion) — \ + asking for SDR instead. Advertising it would paint the stream green: \ + the driver accepts the tonemap it cannot do and renders garbage." + ); + } SessionParams { host: addr, port, @@ -415,7 +431,7 @@ mod session_main { // resolved chroma ("4:4:4→4:2:0" when the host declined) and the decode path // frames actually took. video_caps: pf_client_core::video::video_caps_for( - settings.hdr_enabled, + hdr_enabled, settings.ten_bit_sdr, want_444, ), @@ -427,8 +443,7 @@ mod session_main { // defaults; `PUNKTFUNK_CLIENT_PEAK_NITS` (read in the session pump) pins one // manually on either OS and wins over both. #[cfg(windows)] - display_hdr: settings - .hdr_enabled + display_hdr: hdr_enabled .then(|| pf_client_core::video_d3d11::display_hdr_volume(window_pos())) .flatten(), #[cfg(not(windows))] diff --git a/crates/pf-client-core/src/video.rs b/crates/pf-client-core/src/video.rs index 927f3352..dd7d3893 100644 --- a/crates/pf-client-core/src/video.rs +++ b/crates/pf-client-core/src/video.rs @@ -1567,14 +1567,16 @@ pub fn av1_hardware_decodable(vk: Option<&VulkanDecodeDevice>) -> bool { /// and a device offering `YUV444_8` but not `YUV444_10` would land in exactly the hole this /// closes. Asking for both costs one extra capability query and removes the case entirely. /// -/// ⚠ Deliberately NOT extended to `VIDEO_CAP_10BIT`/`VIDEO_CAP_HDR`, which are advertised -/// unprobed for the same reason this one was. The asymmetry is real: all three hardware -/// rungs implement 10-bit 4:2:0 (`profile_for` maps `(H265, 1, 10)` and `(Av1, 1, 10)`; -/// pf-dxvadec carries P010), so a Vulkan-only probe there would answer `false` on boxes -/// whose VAAPI/DXVA rung decodes 10-bit perfectly and would silently withdraw HDR from -/// them — a visible regression bought against a case that has never been observed. Gating -/// 10-bit honestly needs a libva/D3D11 probe, which this path cannot afford (same reason -/// [`av1_hardware_decodable`] does not consult VAAPI). +/// ⚠ Deliberately NOT extended to `VIDEO_CAP_10BIT`, which is advertised unprobed for the +/// same reason this one was. The asymmetry is real: all three hardware rungs implement +/// 10-bit 4:2:0 (`profile_for` maps `(H265, 1, 10)` and `(Av1, 1, 10)`; pf-dxvadec carries +/// P010), so a Vulkan-only probe there would answer `false` on boxes whose VAAPI/DXVA rung +/// decodes 10-bit perfectly and would silently withdraw depth from them — a visible +/// regression bought against a case that has never been observed. Gating decode-10-bit +/// honestly needs a libva/D3D11 probe, which this path cannot afford (same reason +/// [`av1_hardware_decodable`] does not consult VAAPI). `VIDEO_CAP_HDR` IS gated since the +/// 2026-08-26 Arc field report — but on a different question, PRESENTATION, where the +/// decode reasoning above does not apply: see [`hdr_presentable`]. pub fn hevc_444_hardware_decodable(vk: Option<&VulkanDecodeDevice>) -> bool { #[cfg(any(target_os = "linux", windows))] { @@ -1596,13 +1598,51 @@ pub fn hevc_444_hardware_decodable(vk: Option<&VulkanDecodeDevice>) -> bool { /// above and any future caller cannot disagree about the magic number. const CHROMA_444: u8 = 3; +/// Can this client PRESENT a PQ (HDR) stream correctly — the promise `VIDEO_CAP_HDR` +/// makes, asked the way [`hevc_444_hardware_decodable`] asks its question: by the caller, +/// once, while the device bundle is still borrowable. +/// +/// Decode is not the question — every hardware rung carries 10-bit 4:2:0 (see the note on +/// [`hevc_444_hardware_decodable`]) — presentation is. On Windows the D3D11VA hand-off +/// shows a PQ stream either as HDR10 pass-through ([`VulkanDecodeDevice::d3d11_hdr10`]) +/// or through the video processor's PQ→sRGB tonemap, and that tonemap is a driver +/// capability nothing validated: the Blt succeeds and renders garbage where it is missing +/// (field report 2026-08-26 — Arc A370M client, every HDR session green with a +/// decode-recovery storm, AV1 8-bit SDR at the same 2880x1620@120 clean). A device that +/// can do neither must not invite a PQ stream it can only show as garbage — and the +/// promise has to hold for D3D11VA specifically, not just the rung `auto` picks first, +/// because D3D11VA is in every Windows ladder (first on Intel/unknown, the demotion +/// target on NVIDIA/AMD). +/// +/// Everywhere else the answer is `true`: every Vulkan-presenter lane (native Vulkan, +/// VAAPI, software) tonemaps PQ in our own CSC shader, no driver opinion involved. The +/// same holds on Windows when the D3D11 import path is absent — the D3D11VA rung is then +/// skipped entirely (`supports_d3d11() == false`) and PQ presents through the shader. +pub fn hdr_presentable(vk: Option<&VulkanDecodeDevice>) -> bool { + #[cfg(windows)] + { + vk.is_none_or(|v| { + !v.d3d11_import + || v.d3d11_hdr10 + || crate::video_d3d11::pq_tonemap_supported(v.adapter_luid) + }) + } + #[cfg(not(windows))] + { + let _ = vk; + true + } +} + /// The desktop session's `video_caps` bitfield, as a pure function of the two user /// switches that move it — so the rule can be tested without a GPU, a host or a Hello. /// /// `want_444` is the "Full chroma" setting **already ANDed with this device's ability to -/// decode it** ([`hevc_444_hardware_decodable`]). Split that way on purpose: the caller -/// owns the expensive driver question and can log its own refusal with the user's setting -/// in hand, while the bit arithmetic — the part that was wrong — stays testable. +/// decode it** ([`hevc_444_hardware_decodable`]), and `hdr_enabled` the HDR setting +/// **already ANDed with this device's ability to present PQ** ([`hdr_presentable`]). +/// Split that way on purpose: the caller owns the expensive driver questions and can log +/// its own refusal with the user's setting in hand, while the bit arithmetic — the part +/// that was wrong — stays testable. /// /// `MULTI_SLICE` is unconditional and is decoder truth for THIS embedder: every desktop /// decode stack (Vulkan Video, D3D11VA, VAAPI, openh264/rav1d) handles AUs carrying diff --git a/crates/pf-client-core/src/video_d3d11.rs b/crates/pf-client-core/src/video_d3d11.rs index d85be9ce..67fb8582 100644 --- a/crates/pf-client-core/src/video_d3d11.rs +++ b/crates/pf-client-core/src/video_d3d11.rs @@ -54,14 +54,14 @@ use windows::core::Interface; use windows::Win32::d3d11::{ D3D11CreateDevice, ID3D11Device, ID3D11DeviceContext, ID3D11Multithread, ID3D11Texture2D, ID3D11VideoContext1, ID3D11VideoDevice, ID3D11VideoProcessor, ID3D11VideoProcessorEnumerator, - ID3D11VideoProcessorOutputView, D3D11_BIND_RENDER_TARGET, D3D11_BIND_SHADER_RESOURCE, - D3D11_CREATE_DEVICE_BGRA_SUPPORT, D3D11_CREATE_DEVICE_VIDEO_SUPPORT, - D3D11_RESOURCE_MISC_SHARED_KEYEDMUTEX, D3D11_RESOURCE_MISC_SHARED_NTHANDLE, D3D11_SDK_VERSION, - D3D11_TEXTURE2D_DESC, D3D11_USAGE_DEFAULT, D3D11_VIDEO_FRAME_FORMAT_PROGRESSIVE, - D3D11_VIDEO_PROCESSOR_CONTENT_DESC, D3D11_VIDEO_PROCESSOR_INPUT_VIEW_DESC, - D3D11_VIDEO_PROCESSOR_OUTPUT_VIEW_DESC, D3D11_VIDEO_PROCESSOR_STREAM, - D3D11_VIDEO_USAGE_PLAYBACK_NORMAL, D3D11_VPIV_DIMENSION_TEXTURE2D, - D3D11_VPOV_DIMENSION_TEXTURE2D, + ID3D11VideoProcessorEnumerator1, ID3D11VideoProcessorOutputView, D3D11_BIND_RENDER_TARGET, + D3D11_BIND_SHADER_RESOURCE, D3D11_CREATE_DEVICE_BGRA_SUPPORT, + D3D11_CREATE_DEVICE_VIDEO_SUPPORT, D3D11_RESOURCE_MISC_SHARED_KEYEDMUTEX, + D3D11_RESOURCE_MISC_SHARED_NTHANDLE, D3D11_SDK_VERSION, D3D11_TEXTURE2D_DESC, + D3D11_USAGE_DEFAULT, D3D11_VIDEO_FRAME_FORMAT_PROGRESSIVE, D3D11_VIDEO_PROCESSOR_CONTENT_DESC, + D3D11_VIDEO_PROCESSOR_INPUT_VIEW_DESC, D3D11_VIDEO_PROCESSOR_OUTPUT_VIEW_DESC, + D3D11_VIDEO_PROCESSOR_STREAM, D3D11_VIDEO_USAGE_PLAYBACK_NORMAL, + D3D11_VPIV_DIMENSION_TEXTURE2D, D3D11_VPOV_DIMENSION_TEXTURE2D, }; use windows::Win32::d3dcommon::{D3D_FEATURE_LEVEL_11_0, D3D_FEATURE_LEVEL_11_1}; use windows::Win32::dxgi::{ @@ -71,8 +71,8 @@ use windows::Win32::dxgi::{ DXGI_COLOR_SPACE_YCBCR_FULL_G22_LEFT_P601, DXGI_COLOR_SPACE_YCBCR_FULL_G22_LEFT_P709, DXGI_COLOR_SPACE_YCBCR_STUDIO_G2084_LEFT_P2020, DXGI_COLOR_SPACE_YCBCR_STUDIO_G22_LEFT_P2020, DXGI_COLOR_SPACE_YCBCR_STUDIO_G22_LEFT_P601, DXGI_COLOR_SPACE_YCBCR_STUDIO_G22_LEFT_P709, - DXGI_FORMAT_B8G8R8A8_UNORM, DXGI_FORMAT_R10G10B10A2_UNORM, DXGI_RATIONAL, DXGI_SAMPLE_DESC, - DXGI_SHARED_RESOURCE_READ, DXGI_SHARED_RESOURCE_WRITE, + DXGI_FORMAT_B8G8R8A8_UNORM, DXGI_FORMAT_P010, DXGI_FORMAT_R10G10B10A2_UNORM, DXGI_RATIONAL, + DXGI_SAMPLE_DESC, DXGI_SHARED_RESOURCE_READ, DXGI_SHARED_RESOURCE_WRITE, }; use windows::Win32::windef::RECT; use windows::Win32::winnt::HANDLE; @@ -205,6 +205,86 @@ pub(crate) fn create_device(luid: Option<[u8; 8]>) -> Result<(ID3D11Device, ID3D Ok((device, context)) } +/// Can this adapter's video processor actually CONVERT a PQ decode surface to sRGB — the +/// tonemap [`HandoffRing::present`] relies on for a PQ stream whenever the presenter has no +/// HDR10 swapchain to pass it through ([`crate::video::VulkanDecodeDevice::d3d11_hdr10`] +/// false)? +/// +/// Asked because setting the colorspaces is not a negotiation: `VideoProcessorSetStream/ +/// OutputColorSpace1` accept anything, and `VideoProcessorBlt` succeeds either way — a +/// driver that cannot do the conversion renders garbage instead of failing. The host +/// records the sibling failure on NVIDIA in `pf-capture`'s `VideoConverter` docs (RGB→P010 +/// "renders green"); field report 2026-08-26 is this direction on the client: an Arc A370M +/// went green on every HDR session while AV1 8-bit SDR at the same 2880x1620@120 streamed +/// clean. +/// +/// One throwaway device + enumerator on the presenter's adapter, asked the exact pair the +/// SDR ring sets: P010 `YCBCR_STUDIO_G2084_LEFT_P2020` in, BGRA8 `RGB_FULL_G22_NONE_P709` +/// out. Only the driver's definitive "no" answers `false`; an API failure answers `true` +/// (today's behaviour) — a box whose D3D11 is broken enough to fail the probe fails +/// D3D11VA construction too, and PQ then presents through a rung whose tonemap is our own +/// shader. Cost is a few ms, paid once per connect and only on the path that needs the +/// answer (`crate::video::hdr_presentable` short-circuits it away everywhere else). +pub(crate) fn pq_tonemap_supported(luid: Option<[u8; 8]>) -> bool { + fn probe(luid: Option<[u8; 8]>) -> Result { + let (device, _context) = create_device(luid)?; + let video_device: ID3D11VideoDevice = device + .cast() + .context("device lacks ID3D11VideoDevice (created without VIDEO_SUPPORT)")?; + // The enumerator wants a content shape; conversion support is a format/colorspace + // fact, so any plausible size asks the same question. + let rate = DXGI_RATIONAL { + Numerator: 60, + Denominator: 1, + }; + let desc = D3D11_VIDEO_PROCESSOR_CONTENT_DESC { + InputFrameFormat: D3D11_VIDEO_FRAME_FORMAT_PROGRESSIVE, + InputFrameRate: rate, + InputWidth: 1920, + InputHeight: 1080, + OutputFrameRate: rate, + OutputWidth: 1920, + OutputHeight: 1080, + Usage: D3D11_VIDEO_USAGE_PLAYBACK_NORMAL, + }; + // SAFETY: COM calls on the live device/enumerator just created, over a borrowed + // fully-initialized stack descriptor; the conversion query fills a BOOL by value. + unsafe { + let enumerator = video_device + .CreateVideoProcessorEnumerator(&desc) + .context("CreateVideoProcessorEnumerator")?; + let enumerator1: ID3D11VideoProcessorEnumerator1 = enumerator + .cast() + .context("enumerator lacks ID3D11VideoProcessorEnumerator1 (pre-Win10?)")?; + let ok = enumerator1 + .CheckVideoProcessorFormatConversion( + DXGI_FORMAT_P010, + DXGI_COLOR_SPACE_YCBCR_STUDIO_G2084_LEFT_P2020, + DXGI_FORMAT_B8G8R8A8_UNORM, + DXGI_COLOR_SPACE_RGB_FULL_G22_NONE_P709, + ) + .context("CheckVideoProcessorFormatConversion")?; + Ok(ok.as_bool()) + } + } + match probe(luid) { + Ok(supported) => { + if !supported { + tracing::warn!( + "video processor reports NO P010 PQ→sRGB conversion — a PQ stream on the \ + D3D11VA rung would render garbage (green) instead of tone-mapping" + ); + } + supported + } + Err(e) => { + tracing::debug!(error = %format!("{e:#}"), + "PQ tonemap probe failed — assuming supported"); + true + } + } +} + /// One shareable ring slot: the NV12/P010 texture, its keyed mutex, and the NT handle the /// presenter imports. Handle closed on drop (the presenter never owns it). struct Slot { @@ -638,13 +718,14 @@ impl HandoffRing { /// `tex_*` is the DXVA-aligned decode surface (>= the frame); the gap is the padding the /// stream source rect excludes. /// -/// Keyed by DECODER rather than latched once per process. Two rungs shared this hand-off -/// until M10 (libavcodec's D3D11VA and the native one), and a single process-wide latch -/// meant a session that pinned the native rung and then demoted logged the native layout -/// and nothing else, leaving the rung that actually painted the session's frames -/// undocumented in exactly the report that needs it. One rung fills the ring today, so the -/// set holds one short entry — kept keyed because the property is about which decoder -/// wrote the surface, and that is the question a new-GPU forensics report asks. +/// Keyed by DECODER × LAYOUT rather than latched once per process. Decoder, because two +/// rungs shared this hand-off until M10 and a process-wide latch left whichever rung a +/// session demoted onto undocumented in exactly the report that needs it. Layout +/// (frame + pool dims + PQ), because a mid-stream `Reconfigure` or in-band SDR↔PQ flip +/// rebuilds the decode pool at a new shape, and a latch keyed on the decoder alone left +/// the frame-vs-padding relationship — the very thing a green-bar/smear report hinges +/// on — logged only for the shape the session STARTED at. `slice` stays out of the key: +/// it varies per frame and would turn one line per shape into one per DPB slot. fn log_layout_once( width: u32, height: u32, @@ -656,12 +737,13 @@ fn log_layout_once( ) { use std::collections::HashSet; use std::sync::{Mutex, OnceLock}; - static SEEN: OnceLock>> = OnceLock::new(); + type LayoutKey = (String, u32, u32, u32, u32, bool); + static SEEN: OnceLock>> = OnceLock::new(); let seen = SEEN.get_or_init(|| Mutex::new(HashSet::new())); // A poisoned lock costs a log line, never a frame: a panic while holding it can only have // happened inside the set, and the worst outcome of ignoring it is a repeated line. let first = match seen.lock() { - Ok(mut seen) => seen.insert(decoder.to_owned()), + Ok(mut seen) => seen.insert((decoder.to_owned(), width, height, tex_w, tex_h, pq)), Err(_) => false, }; if first { -- 2.54.0