From 9e598f85959eef68a41ec73cb1d3b34b75623705 Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Sun, 9 Aug 2026 00:08:12 +0200 Subject: [PATCH] fix(client): the 4:4:4 switch could cost a Deck its whole codec, and --probe-decode denied the queue it was decoding on MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two Steam Deck findings from a field report of "the decoder was not found, it fell back to H.264 — but sometimes HEVC worked". **The 4:4:4 advertisement was a promise nothing checked.** `VIDEO_CAP_444` rode the "Full chroma" setting alone. That was safe while a software HEVC decoder existed underneath it; M8 removed one (there is no permissively licensed HEVC CPU decoder, so `software_decodable_codecs()` is H.264|AV1). The host grants 4:4:4 on HEVC ONLY, and answers the resolved chroma in the Welcome before the client builds a decoder — so on a device with no 4:4:4 decode the toggle did not cost crispness, it cost the entire codec: the Vulkan rung refuses the shape at construction, VAAPI refuses it too, there is no CPU rung, and the session reconnects on H.264. AMD has no HEVC 4:4:4 decode on any silicon, so every Deck with that switch on lost HEVC. It is per-profile and default-off, which is exactly why it looked intermittent — a "Work" profile lost HEVC where "Game" kept it, same box, same host. Gated on `hevc_444_hardware_decodable`, which asks the driver through the SAME code the rung uses at construction (`VkH265Decoder::probe_stream_support`), so the advertisement and the rung that must honour it cannot disagree. Both depths are required, not either: with HDR on the host may resolve 4:4:4 10-bit, and a device offering YUV444_8 but not YUV444_10 would land in the same hole. Answering from the Vulkan rung alone is exact rather than approximate — it is the only rung in this build that implements 4:4:4 at all (`pf_vaadec::profile_for` errors on chroma_format_idc 3, pf-dxvadec refuses anything but 4:2:0, the CPU rung is 8-bit 4:2:0). Deliberately NOT extended to VIDEO_CAP_10BIT/HDR: all three rungs implement 10-bit 4:2:0, so a Vulkan-only probe there would withdraw HDR from boxes whose VAAPI/DXVA rung decodes it perfectly — a real regression against a case never observed. The bit arithmetic moves into `video::video_caps_for` so the part that was wrong is testable without a GPU, a host or a Hello; the test is verified non-vacuous against the planted original defect. **`--probe-decode` described a different device from the one that streams.** The RADV video-decode opt-in sat AFTER the --list-adapters/--probe-decode/--list-audio /--pair early exits, so the triage tool never had it. Measured on a Deck (canary e22af40f), same binary back to back: bare `--probe-decode` printed "vulkan video decode: no", "driver decode ops: none (0x0)", "no queue family advertises VIDEO_DECODE"; with RADV_PERFTEST=video_decode in the environment, "YES" and "H.264, H.265, AV1, VP9". Any Deck triage that consulted it reached the opposite of the truth. Hoisted to the top of `run`, ahead of every early exit — nothing touches Vulkan before it (`main` calls `run` directly). Gates, in the Linux container: fmt, plain `cargo build` (not only --all-targets), `clippy --all-targets -D warnings`, and 185 tests. --- clients/session/src/main.rs | 70 +++++++---- crates/pf-client-core/src/video.rs | 126 +++++++++++++++++++ crates/pf-client-core/src/video_vk_native.rs | 81 +++++++++--- 3 files changed, 238 insertions(+), 39 deletions(-) diff --git a/clients/session/src/main.rs b/clients/session/src/main.rs index 96272edc..0c3e804f 100644 --- a/clients/session/src/main.rs +++ b/clients/session/src/main.rs @@ -325,6 +325,22 @@ mod session_main { }; // Before the struct literal — `vulkan` moves into it below. let phase_lock = vulkan.as_ref().is_some_and(|v| v.present_timing); + // …and the 4:4:4 promise, for the same reason: asked while the device bundle is + // still borrowable. `&&` short-circuits, so a box that never enabled Full chroma + // pays no capability queries for a feature it does not want. + let want_444 = settings.enable_444 + && pf_client_core::video::hevc_444_hardware_decodable(vulkan.as_ref()); + if settings.enable_444 && !want_444 { + // Loud, because the user turned a switch on and is not getting it. The + // alternative is what this replaces: the host grants 4:4:4, the decode ladder + // has no rung that can take it, and the session drops HEVC entirely. + tracing::warn!( + "Full chroma (4:4:4) requested but this device has no 4:4:4 HEVC decode — \ + asking for 4:2:0 instead. Advertising it would cost the whole codec: 4:4:4 \ + is granted on HEVC only, and there is no software HEVC decoder to fall back \ + to (PyroWave carries 4:4:4 on any GPU, if the link can take it)." + ); + } SessionParams { host: addr, port, @@ -356,30 +372,16 @@ mod session_main { // slice NALs, so the host may keep its multi-slice low-latency default (§7 LN1). // The mobile/TV embedders must NOT copy this blindly — Amlogic MediaCodec wedges // on multi-slice AUs (see `VIDEO_CAP_MULTI_SLICE`), so they advertise per-decoder. - // 4:4:4 is opt-in and off by default (Settings "Full chroma"): the bit only says + // 4:4:4 is opt-in and off by default (Settings "Full chroma"): the bit says // "upgrade me if you can" — the host still gates on its own policy, its capturer, // HEVC, and a real GPU 4:4:4 encode probe, and answers the resolved chroma in the - // Welcome BEFORE we build a decoder. Advertised whenever the user asks because - // every path can DISPLAY it: the Vulkan presenter samples the 2-plane 4:4:4 pool - // formats (hardware RExt decode where the driver offers it — NVIDIA today), - // with the decoder ladder demoting on its own. No capability probe gates the - // bit — but note (M8) that the software rung below it is 4:2:0 8-bit ONLY and - // refuses anything else rather than mis-scaling it, so on a box whose hardware - // 4:4:4 decode fails the floor is a codec fallback, not a converted picture. + // Welcome BEFORE we build a decoder. It is now ALSO gated on this device being + // able to decode 4:4:4 (`want_444`, computed above); the rule and its reasoning + // live in `video::video_caps_for`, which is where they get tested. // The cost stays VISIBLE, not silent: the Detailed stats overlay prints the // resolved chroma ("4:4:4→4:2:0" when the host declined) and the decode path // frames actually took. - video_caps: punktfunk_core::quic::VIDEO_CAP_MULTI_SLICE - | if settings.hdr_enabled { - punktfunk_core::quic::VIDEO_CAP_10BIT | punktfunk_core::quic::VIDEO_CAP_HDR - } else { - 0 - } - | if settings.enable_444 { - punktfunk_core::quic::VIDEO_CAP_444 - } else { - 0 - }, + video_caps: pf_client_core::video::video_caps_for(settings.hdr_enabled, want_444), // This panel's HDR colour volume → the host's virtual-display EDID, so host // apps tone-map to the real glass. Windows reads it from DXGI (the // `--window-pos` monitor; advanced-color outputs only) — gated on the HDR @@ -496,6 +498,12 @@ mod session_main { /// decode is already the default just no-ops. Append rather than clobber so a user's own /// `RADV_PERFTEST` survives; `PUNKTFUNK_DECODER=native-vaapi` still overrides the decoder /// choice (the pre-M10 `vaapi` spelling reaches the same rung — it migrates, loudly). + /// + /// ⚠⚠ Called from the TOP of [`run`], ahead of the `--list-adapters` / `--probe-decode` + /// early exits — not merely "before `run_session` creates the instance". Those flags + /// create Vulkan instances of their own and RADV latches `RADV_PERFTEST` when its ICD + /// initialises, so a call placed after them leaves the triage tool describing a device + /// that cannot decode while the streaming path decodes on it. #[cfg(target_os = "linux")] fn enable_radv_video_decode() { const TOKEN: &str = "video_decode"; @@ -579,6 +587,23 @@ mod session_main { ) .init(); + // Before ANY Vulkan call — and that includes the two probe flags below, which is the + // whole reason this sits at the top of `run` instead of beside the session setup it + // was written for. Make RADV expose its video-decode queue + extensions so the + // decoder's `auto` path prefers Vulkan Video over VAAPI (Steam Deck, and any gated + // RADV). Windows drivers (NVIDIA/AMD Adrenalin) expose theirs unconditionally. + // + // ⚠⚠ It USED to sit after the `--list-adapters` / `--probe-decode` / `--list-audio` / + // `--pair` early exits, which meant the triage tool answered a DIFFERENT question from + // the one the streaming path asks. Measured on a Steam Deck (2026-08-08, canary + // `e22af40f`), same binary, back to back: bare `--probe-decode` printed `vulkan video + // decode: no`, `driver decode ops: none (0x0)`, `no queue family advertises + // VIDEO_DECODE`; the same call with `RADV_PERFTEST=video_decode` in the environment + // printed `YES` and `H.264, H.265, AV1, VP9`. The tool exists to be believed, so any + // Deck triage that consulted it reached the opposite of the truth. + #[cfg(target_os = "linux")] + enable_radv_video_decode(); + // `--list-adapters`: print the Vulkan physical devices' marketing names (one per // line, discrete first) for the desktop shells' GPU picker, then exit. if arg_flag("--list-adapters") { @@ -753,11 +778,8 @@ mod session_main { return headless_pair(&pin); } - // Before any Vulkan call: make RADV expose its video-decode queue + extensions so the - // decoder's `auto` path prefers Vulkan Video over VAAPI (Steam Deck, and any gated RADV). - // Windows drivers (NVIDIA/AMD Adrenalin) expose theirs unconditionally. - #[cfg(target_os = "linux")] - enable_radv_video_decode(); + // (The RADV video-decode opt-in that used to live here now runs at the very top of + // `run` — it has to precede the probe flags too, not just the session.) // The Settings device picks → env, unless the user already forced one by hand: // the GPU (the shells' pickers store the adapter's marketing name) for the diff --git a/crates/pf-client-core/src/video.rs b/crates/pf-client-core/src/video.rs index 8326fe18..b95dc96b 100644 --- a/crates/pf-client-core/src/video.rs +++ b/crates/pf-client-core/src/video.rs @@ -1531,6 +1531,85 @@ pub fn av1_hardware_decodable(vk: Option<&VulkanDecodeDevice>) -> bool { d3d11 } +/// Can this client actually DECODE 4:4:4 HEVC — the question `VIDEO_CAP_444` is a promise +/// about, and the one nothing asked until a Steam Deck lost HEVC over it. +/// +/// The bit used to ride the "Full chroma" toggle alone, with a comment saying the software +/// rung was the floor underneath it. M8 removed that floor: there is no CPU HEVC decoder at +/// all ([`software_decodable_codecs`]), and the host grants 4:4:4 only on HEVC. So on a +/// device with no 4:4:4 decode the toggle did not cost crispness — it cost the whole codec. +/// The Welcome resolves the chroma before a decoder exists, the native Vulkan constructor +/// then refuses the shape, VAAPI refuses it too, and the session reconnects on H.264 with +/// "HEVC decoding failed on this device" (field report 2026-08-08, Deck / VanGogh). +/// +/// ⭐ Answered from the VULKAN rung alone, and that is exact rather than approximate: it is +/// the only rung in this build that implements 4:4:4 at all. `pf_vaadec::profile_for` maps +/// only `chroma_format_idc == 1` and errors `UnsupportedShape` on 3; `pf_dxvadec`'s config +/// refuses "anything but 4:2:0" by construction; the CPU rung is 8-bit 4:2:0 only. So a +/// device whose Vulkan driver offers no 4:4:4 decode profile has no 4:4:4 path in this +/// client, whatever its silicon can do. (That is why an Intel box — whose hardware HAS done +/// HEVC 4:4:4 since Ice Lake — is still a `false` here: our DXVA/VAAPI rungs do not +/// implement it, so advertising it would be a lie about US, not about the GPU.) +/// +/// ⚠ Both depths are required, not either: with HDR on, the host may resolve 4:4:4 **10-bit**, +/// 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). +pub fn hevc_444_hardware_decodable(vk: Option<&VulkanDecodeDevice>) -> bool { + #[cfg(any(target_os = "linux", windows))] + { + vk.is_some_and(|v| { + crate::video_vk_native::hevc_shape_supported(v, CHROMA_444, 0) + && crate::video_vk_native::hevc_shape_supported(v, CHROMA_444, 2) + }) + } + // No native Vulkan rung is compiled in off the two desktop OSes, so nothing here can + // decode 4:4:4 and the honest answer is a constant. + #[cfg(not(any(target_os = "linux", windows)))] + { + let _ = vk; + false + } +} + +/// `chroma_format_idc` for 4:4:4 (H.265 7.4.3.2) — spelled once so the two depth probes +/// above and any future caller cannot disagree about the magic number. +const CHROMA_444: u8 = 3; + +/// 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. +/// +/// `MULTI_SLICE` is unconditional and is decoder truth for THIS embedder: every desktop +/// decode stack (Vulkan Video, D3D11VA, VAAPI, openh264/rav1d) handles AUs carrying +/// several slice NALs, so the host may keep its multi-slice low-latency default (§7 LN1). +/// ⚠ The mobile/TV embedders must NOT copy this blindly — Amlogic MediaCodec wedges on +/// multi-slice AUs (see `VIDEO_CAP_MULTI_SLICE`), so they advertise per-decoder. +/// +/// HDR off means 10-bit is not advertised either, so the host never upgrades depth. +pub fn video_caps_for(hdr_enabled: bool, want_444: bool) -> u8 { + let mut caps = punktfunk_core::quic::VIDEO_CAP_MULTI_SLICE; + if hdr_enabled { + caps |= punktfunk_core::quic::VIDEO_CAP_10BIT | punktfunk_core::quic::VIDEO_CAP_HDR; + } + if want_444 { + caps |= punktfunk_core::quic::VIDEO_CAP_444; + } + caps +} + /// [`decodable_codecs`] plus the PyroWave bit when the presenter's device passed the /// compute-feature probe, minus the codecs `decoder_pref` makes unreachable. /// Advertisement-only: `resolve_codec` never auto-picks PyroWave — the session must also @@ -2701,6 +2780,53 @@ mod tests { use super::*; use punktfunk_core::quic::{CODEC_AV1, CODEC_H264, CODEC_HEVC, CODEC_PYROWAVE}; + /// The 4:4:4 advertisement is a PROMISE, and M8 removed the floor that used to make a + /// broken one survivable: there is no CPU HEVC decoder, and the host grants 4:4:4 on + /// HEVC only, so advertising it on a device that cannot decode it costs the entire + /// codec (field 2026-08-08, Steam Deck / VanGogh — HEVC fell back to H.264). + /// + /// The device question needs a GPU; THIS is the half that does not, and it is the half + /// that was wrong — the bit used to ride `enable_444` alone. + #[test] + fn the_444_bit_needs_the_setting_and_a_device_that_can_decode_it() { + const V444: u8 = punktfunk_core::quic::VIDEO_CAP_444; + // The regression itself: setting on, device can't → the bit must NOT go out. + assert_eq!( + video_caps_for(true, false) & V444, + 0, + "a 4:4:4 promise this device cannot keep costs HEVC entirely" + ); + // ...and the feature still works where it can be honoured. + assert_ne!(video_caps_for(true, true) & V444, 0); + // Never advertised unasked, whatever the device can do. + assert_eq!(video_caps_for(true, false) & V444, 0); + assert_eq!(video_caps_for(false, false) & V444, 0); + + // The 4:4:4 gate must not disturb the other two bits (10-bit/HDR is deliberately + // NOT probe-gated — see `hevc_444_hardware_decodable`'s docs for why). + const HDR_BITS: u8 = + punktfunk_core::quic::VIDEO_CAP_10BIT | punktfunk_core::quic::VIDEO_CAP_HDR; + for want_444 in [false, true] { + assert_eq!(video_caps_for(true, want_444) & HDR_BITS, HDR_BITS); + assert_eq!(video_caps_for(false, want_444) & HDR_BITS, 0); + assert_ne!( + video_caps_for(false, want_444) & punktfunk_core::quic::VIDEO_CAP_MULTI_SLICE, + 0, + "MULTI_SLICE is unconditional for this embedder" + ); + } + } + + /// No presenter Vulkan device ⇒ no 4:4:4, and that is an ANSWER rather than a missing + /// one: the native Vulkan rung is the only one in this build that implements 4:4:4 at + /// all (`pf_vaadec::profile_for` errors on `chroma_format_idc == 3`, pf-dxvadec refuses + /// anything but 4:2:0, the CPU rung is 8-bit 4:2:0). The `Some` arm needs real hardware + /// and lives in the GPU suites. + #[test] + fn no_vulkan_device_means_no_444_promise() { + assert!(!hevc_444_hardware_decodable(None)); + } + /// The reconnect rule, as the invariant it is: an exhausted codec must come back as /// one this client can decode ALL THE WAY DOWN, and must never come back as itself. /// diff --git a/crates/pf-client-core/src/video_vk_native.rs b/crates/pf-client-core/src/video_vk_native.rs index 2d1cd6bb..d6a5598d 100644 --- a/crates/pf-client-core/src/video_vk_native.rs +++ b/crates/pf-client-core/src/video_vk_native.rs @@ -216,6 +216,70 @@ fn submit_queues_collide(graphics_qf: u32, decode_qf: u32) -> bool { graphics_qf == decode_qf } +/// The queue lock this device's decode lane submits under. One function so the +/// pre-session shape probe ([`hevc_shape_supported`]) and the real decoder cannot pick +/// different serialization for the same device. +fn queue_lock_for(vk: &VulkanDecodeDevice) -> Box { + if submit_queues_collide(vk.graphics_qf, vk.decode_qf) { + Box::new(NativeQueueLock::Shared(vk.queue_lock.clone())) + } else { + Box::new(NativeQueueLock::Uncontended) + } +} + +/// The presenter's handles in pf-vkdecode's shape. Same reason as [`queue_lock_for`]: +/// the probe must ask about the DEVICE THE SESSION WOULD USE, not a re-derived one. +fn device_handles(vk: &VulkanDecodeDevice) -> DeviceHandles { + DeviceHandles { + get_instance_proc_addr: vk.get_instance_proc_addr, + instance: vk.instance, + physical_device: vk.physical_device, + device: vk.device, + decode_qf: vk.decode_qf, + decode_queue_index: DECODE_QUEUE_INDEX, + graphics_qf: vk.graphics_qf, + } +} + +/// Can this device hardware-decode HEVC at the given picture shape? Asked BEFORE the +/// Hello, so the client never advertises a shape it would have to refuse a session over. +/// +/// This is the same question, through the same code, that +/// [`NativeVulkanDecoder::new`]'s H.265 arm asks at construction — `VkH265Decoder::new` +/// then `probe_stream_support` — deliberately, so an advertisement and the rung that has +/// to honour it cannot disagree. It creates and drops a decoder object; that costs a +/// handful of driver capability queries and no session, no images and no submits. +/// +/// `false` when the presenter has no Vulkan Video decode at all, which for 4:4:4 is the +/// right answer rather than a missing one — see +/// [`crate::video::hevc_444_hardware_decodable`] for why no other rung can be asked. +pub(crate) fn hevc_shape_supported( + vk: &VulkanDecodeDevice, + chroma_format_idc: u8, + bit_depth_luma_minus8: u8, +) -> bool { + if !vk.video_decode { + return false; + } + // The device-independent half first: a shape pf-vkdecode has no picture format for + // needs no driver to refuse it (and `probe_stream_support` would only re-derive it). + if pf_vkdecode::output_format_for(chroma_format_idc, bit_depth_luma_minus8).is_none() { + return false; + } + // SAFETY: the `DeviceHandles` contract exactly as `NativeVulkanDecoder::new` states + // it — these are the presenter's live instance/device, which outlive this call by + // construction (the presenter owns them for the whole process, and this runs on its + // thread while building the session's Hello). The decoder is dropped before return, + // so nothing outlives the borrow. + let dec = unsafe { pf_vkdecode::VkH265Decoder::new(&device_handles(vk), queue_lock_for(vk)) }; + match dec { + Ok(d) => d + .probe_stream_support(chroma_format_idc, bit_depth_luma_minus8) + .is_ok(), + Err(_) => false, + } +} + /// [`pf_vkdecode::QueueLock`] over the device's shared [`crate::video::QueueLock`] — /// or over nothing, when the decode queue provably has no other submitter (see the /// module doc's queue-lock section). @@ -934,21 +998,8 @@ impl NativeVulkanDecoder { if !vk.video_decode { bail!("presenter device lacks Vulkan Video decode"); } - let lock: Box = - if submit_queues_collide(vk.graphics_qf, vk.decode_qf) { - Box::new(NativeQueueLock::Shared(vk.queue_lock.clone())) - } else { - Box::new(NativeQueueLock::Uncontended) - }; - let handles = DeviceHandles { - get_instance_proc_addr: vk.get_instance_proc_addr, - instance: vk.instance, - physical_device: vk.physical_device, - device: vk.device, - decode_qf: vk.decode_qf, - decode_queue_index: DECODE_QUEUE_INDEX, - graphics_qf: vk.graphics_qf, - }; + let lock = queue_lock_for(vk); + let handles = device_handles(vk); // The `DeviceHandles` caller contract, held for the decoder's whole lifetime // and identical for both arms (it is the HANDLES' contract, not the codec's): // the handles are the presenter's live instance/device, which outlives every