From dc0766b2f36e54d5e4cc52ef5837a2f80c6d00ac Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Wed, 5 Aug 2026 22:22:03 +0200 Subject: [PATCH] fix(client): the native rung now follows the stream's colour and reports true decode latency MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The round-4 residuals, closed after the WP-D hardware verdict: - VUI colour plumbing (the one silent-wrong): the picture's ACTIVE SPS's colour signalling (H.273 code points + range, with E.2.1's 'unspecified' inference where the VUI is silent — the vendored parser's defaults ARE the inferred values, verified) rides PicturePlan -> DecodedVkFrame -> NativeVkFrame per frame, never latched: the Windows host switches an HDR desktop to PQ/BT.2020 IN-BAND while the Welcome still says SDR. Before this, the native path would have painted PQ washed out, silently. - Native decode-latency stat: the deliberately-deferred NativeVk arm of the pump's sampled once-per-stats-window decode measurement now feeds - the frame's (semaphore, semaphore_value) is the decode-done signal, resolved through the shipped ledger before a bounded, pure-measurement vkWaitSemaphores (VkH264Decoder::wait_decoded). - The renegotiation-teardown window is settled as NO HOLE: rebuild_state now documents the full safety argument (graveyarded pools stay intact under presenter holds, tokens route strictly by generation, session objects die only post-drain with the generation gate INSIDE read_status), and the two backend comments that wrongly claimed stale pools were 'gone' are fixed. - VK_KHR_unified_image_layouts stays deferred (fleet drivers lack it). Adversarial review round 6: 3 minor findings (2 doc fixes applied; the SPS-replaced-without-PPS-resend divergence stays a documented envelope assumption - hosts re-send both at every keyframe, and a hardening PlanWarning could cost real frames on a false positive). Gates: fmt clean; clippy -D warnings zero (mac + pf-lxcheck2 container, incl. pf-client-core/pf-presenter); tests 45+30+53 mac, 30+121+53 container. --- crates/pf-bitstream/src/h264.rs | 143 +++++++++++++++++++ crates/pf-client-core/src/session.rs | 14 +- crates/pf-client-core/src/video.rs | 13 +- crates/pf-client-core/src/video_vk_native.rs | 57 ++++++-- crates/pf-vkdecode/src/decoder.rs | 48 +++++++ crates/pf-vkdecode/src/lib.rs | 9 +- 6 files changed, 259 insertions(+), 25 deletions(-) diff --git a/crates/pf-bitstream/src/h264.rs b/crates/pf-bitstream/src/h264.rs index fcbe3369..7eb38f7b 100644 --- a/crates/pf-bitstream/src/h264.rs +++ b/crates/pf-bitstream/src/h264.rs @@ -101,6 +101,12 @@ pub struct PicturePlan { pub coded_height: u32, /// Conformance-window crop (7.4.2.1.1), in luma samples of the coded picture. pub display_crop: DisplayCrop, + /// Colour signalling from the ACTIVE SPS's VUI (E.2.1 inference where absent). + /// Per picture, like [`Self::display_crop`], never latched at session start: + /// the Windows host switches an HDR desktop to PQ/BT.2020 IN-BAND with a new + /// SPS mid-stream, so a backend that captured the first AU's colour would + /// paint HDR frames washed out. + pub colour: ColourDescription, pub profile_idc: u8, pub level_idc: Level, pub bit_depth_luma_minus8: u8, @@ -120,6 +126,22 @@ pub struct DisplayCrop { pub height: u32, } +/// One picture's colour signalling: raw H.273 code points off the active SPS's +/// VUI. When the VUI (or its `video_signal_type`/`colour_description` blocks) is +/// absent these hold E.2.1's INFERRED values — 2/2/2 ("unspecified") with limited +/// range — never a raw struct-zero (0 is a reserved code point no real stream +/// means). That matches the CICP libavcodec reports for such streams, so backends +/// forward these untouched and the consumer's CSC resolves "unspecified" to its +/// SDR default. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct ColourDescription { + pub colour_primaries: u8, + pub transfer_characteristics: u8, + pub matrix_coefficients: u8, + /// `video_full_range_flag` (E.2.1 infers limited range when absent). + pub video_full_range: bool, +} + /// One slice NALU of the picture, with its reference lists fully derived. #[derive(Debug, Clone)] pub struct SlicePlan { @@ -1587,6 +1609,17 @@ impl H264Planner { width: rect.max.x, height: rect.max.y, }, + // Read unconditionally: the vendored parser builds every SPS from + // `Default`, whose `VuiParams` already holds E.2.1's inferred values + // (2/2/2, limited range), and parsing only overwrites them under the + // present flags — so this IS the spec inference whether or not the + // stream carried a VUI. + colour: ColourDescription { + colour_primaries: sps.vui_parameters.colour_primaries, + transfer_characteristics: sps.vui_parameters.transfer_characteristics, + matrix_coefficients: sps.vui_parameters.matrix_coefficients, + video_full_range: sps.vui_parameters.video_full_range_flag, + }, profile_idc: sps.profile_idc, level_idc: sps.level_idc, bit_depth_luma_minus8: sps.bit_depth_luma_minus8, @@ -2274,6 +2307,116 @@ mod tests { ); } + /// A 64x64 SPS with the VUI colour fields set as given. SpsBuilder has no + /// colour setters, so the built Sps is unwrapped and mutated directly (the + /// separate_colour_plane test's idiom); the synthesizer writes the whole + /// `video_signal_type` block from the struct. + fn sps_with_vui_colour( + signal_type: bool, + full_range: bool, + description: Option<(u8, u8, u8)>, + ) -> Rc { + let mut sps = Rc::try_unwrap(base_sps().resolution(64, 64).build()).expect("freshly built"); + sps.vui_parameters_present_flag = true; + sps.vui_parameters.video_signal_type_present_flag = signal_type; + sps.vui_parameters.video_full_range_flag = full_range; + if let Some((primaries, transfer, matrix)) = description { + sps.vui_parameters.colour_description_present_flag = true; + sps.vui_parameters.colour_primaries = primaries; + sps.vui_parameters.transfer_characteristics = transfer; + sps.vui_parameters.matrix_coefficients = matrix; + } + Rc::new(sps) + } + + fn plan_one_idr(sps: &Rc) -> AuPlan { + let pps = PpsBuilder::new(Rc::clone(sps)) + .pic_parameter_set_id(0) + .pic_init_qp(26) + .build(); + let mut au = param_set_au(sps, &pps); + au.extend(write_idr_slice()); + H264Planner::new().plan_au(&au).unwrap() + } + + #[test] + fn an_sps_without_vui_plans_the_e211_unspecified_colour() { + let (sps, pps) = authored_sps_pps(); + assert!( + !sps.vui_parameters_present_flag, + "the base SPS carries no VUI" + ); + let mut au = param_set_au(&sps, &pps); + au.extend(write_idr_slice()); + let plan = H264Planner::new().plan_au(&au).unwrap(); + assert_eq!( + plan.picture.colour, + ColourDescription { + colour_primaries: 2, + transfer_characteristics: 2, + matrix_coefficients: 2, + video_full_range: false, + }, + "E.2.1 inference: 'unspecified' code points + limited range, never a raw 0" + ); + } + + #[test] + fn an_explicit_colour_description_rides_the_plan_and_follows_a_new_sps() { + // BT.2020/PQ HDR signalling — the in-band switch the Windows host emits. + let hdr = sps_with_vui_colour(true, false, Some((9, 16, 9))); + let plan = plan_one_idr(&hdr); + assert_eq!( + plan.picture.colour, + ColourDescription { + colour_primaries: 9, + transfer_characteristics: 16, + matrix_coefficients: 9, + video_full_range: false, + } + ); + + // The colour must track the SPS active for EACH picture, not the + // session's first: an SDR stream renegotiated to HDR mid-stream (same + // SPS id, new content, SPS+PPS in-band at the IDR — the parser's Pps + // snapshots its SPS at PPS-parse time, and hosts re-send both exactly + // so the new content activates) flips at the very next planned picture. + let (sdr_sps, sdr_pps) = authored_sps_pps(); + let mut au0 = param_set_au(&sdr_sps, &sdr_pps); + au0.extend(write_idr_slice()); + let mut planner = H264Planner::new(); + let plan0 = planner.plan_au(&au0).unwrap(); + assert_eq!(plan0.picture.colour.matrix_coefficients, 2); + + let hdr_pps = PpsBuilder::new(Rc::clone(&hdr)) + .pic_parameter_set_id(0) + .pic_init_qp(26) + .build(); + let mut au1 = param_set_au(&hdr, &hdr_pps); + au1.extend(write_idr_slice()); + let plan1 = planner.plan_au(&au1).unwrap(); + assert_eq!( + plan1.picture.colour.matrix_coefficients, 9, + "the replacing SPS's colour lands on its own picture, not latched" + ); + } + + #[test] + fn a_vui_without_colour_description_keeps_unspecified_but_honours_the_range_flag() { + // video_signal_type present, full-range set, but NO colour description: + // the code points stay E.2.1's "unspecified" while the range flag rides. + let plan = plan_one_idr(&sps_with_vui_colour(true, true, None)); + assert_eq!( + plan.picture.colour, + ColourDescription { + colour_primaries: 2, + transfer_characteristics: 2, + matrix_coefficients: 2, + video_full_range: true, + } + ); + } + #[test] fn a_malformed_nalu_mid_au_truncates_with_a_warning_keeping_prior_slices() { let (sps, pps) = authored_sps_pps(); diff --git a/crates/pf-client-core/src/session.rs b/crates/pf-client-core/src/session.rs index 443a8ce0..9748e926 100644 --- a/crates/pf-client-core/src/session.rs +++ b/crates/pf-client-core/src/session.rs @@ -822,12 +822,14 @@ fn pump( // every frame (its decode really is done by now). let hw_fence = match &image { DecodedImage::VkFrame(v) => Some((v.timeline_sem, v.decode_done_value)), - // DecodedImage::NativeVk carries the same (semaphore, - // value) pair and COULD feed this sampled decode-time - // stat identically — deliberately deferred to WP-D: - // the native rung's field A/B should measure the same - // stats surface the FFmpeg rung had at parity time, - // and grow new ones after the verdict, not during it. + // The native rung's frame carries the same pair: the + // decode signals `semaphore_value` when the pixels are + // ready (the presenter's write-back is the `+ 1`), so + // waiting it measures received→decode-complete exactly + // like the AVVkFrame arm. Fed since the WP-D hardware + // verdict landed (bit-exact parity, both DPB modes) — + // one stats surface across both Vulkan rungs. + DecodedImage::NativeVk(f) => Some((f.semaphore, f.semaphore_value)), _ => None, }; if present { diff --git a/crates/pf-client-core/src/video.rs b/crates/pf-client-core/src/video.rs index fb75f049..f5dc6918 100644 --- a/crates/pf-client-core/src/video.rs +++ b/crates/pf-client-core/src/video.rs @@ -256,9 +256,11 @@ pub struct NativeVkFrame { /// wrong window — carried so that assumption is checkable, not silent. pub crop_x: u32, pub crop_y: u32, - /// Colour signalling. The native H.264 path serves the SDR envelope; the backend - /// fills H.273 "unspecified" code points, which every consumer already resolves to - /// the BT.709-limited SDR default (`csc_rows`' documented fallback). + /// Colour signalling, read from the SPS active for THIS picture (H.264 VUI → + /// H.273 code points, with E.2.1's "unspecified" inference where the VUI is + /// silent) — per frame, like the FFmpeg rungs' AVFrame CICP, because the host + /// switches HDR in-band; "unspecified" resolves to the BT.709-limited SDR + /// default (`csc_rows`' documented fallback). pub color: ColorDesc, /// IDR — the stream's re-anchor point (the pump's post-loss resume signal). pub keyframe: bool, @@ -877,10 +879,13 @@ impl Decoder { } /// Wait for a Vulkan-Video frame's GPU decode to complete (timeline semaphore) — - /// the pump's decode-stat measurement. `false` = not the Vulkan backend, or timeout. + /// the pump's decode-stat measurement. `false` = not a Vulkan backend, timeout, or + /// (native rung) a pair no longer in the shipped ledger / a stale session + /// generation — every false just declines the sample. pub fn wait_hw_decoded(&self, timeline_sem: u64, value: u64, timeout_ns: u64) -> bool { match &self.backend { Backend::Vulkan(v) => v.wait_timeline(timeline_sem, value, timeout_ns), + Backend::NativeVulkan(d) => d.wait_timeline(timeline_sem, value, timeout_ns), _ => false, } } diff --git a/crates/pf-client-core/src/video_vk_native.rs b/crates/pf-client-core/src/video_vk_native.rs index e8d1d37f..a4bc0929 100644 --- a/crates/pf-client-core/src/video_vk_native.rs +++ b/crates/pf-client-core/src/video_vk_native.rs @@ -286,13 +286,17 @@ impl NativeVulkanDecoder { coded_height: frame.coded_height, crop_x: frame.crop.x, crop_y: frame.crop.y, - // H.273 "unspecified" — every consumer resolves it to the BT.709-limited - // SDR default, the native H.264 envelope's colour contract (`csc_rows`). + // H.273 code points straight off the picture's ACTIVE SPS — per frame, + // never latched, because the Windows host switches an HDR desktop to + // PQ/BT.2020 IN-BAND (the Welcome still says SDR). pf-bitstream applies + // E.2.1's "unspecified" inference (2/2/2, limited) where the VUI is + // silent, and `csc_rows` resolves "unspecified" to its BT.709-limited + // SDR default — same verdicts libavcodec's CICP passthrough produced. color: ColorDesc { - primaries: 2, - transfer: 2, - matrix: 2, - full_range: false, + primaries: frame.colour.colour_primaries, + transfer: frame.colour.transfer_characteristics, + matrix: frame.colour.matrix_coefficients, + full_range: frame.colour.video_full_range, }, keyframe: frame.is_idr, poc: frame.poc, @@ -315,6 +319,20 @@ impl NativeVulkanDecoder { native } + /// Bounded wait for a shipped frame's decode-complete signal — the pump's + /// sampled decode-latency stat (`Decoder::wait_hw_decoded`), one frame per + /// stats window. The raw pair names a frame still in the shipped ledger (the + /// pump waits on the same thread that just shipped it, before any settle + /// could retire it); the ledger lookup is the liveness proof — an unreleased + /// frame pins its pool, so a pair matching nothing (already settled, or a + /// stray) just declines the sample instead of touching unknown handles. + pub(crate) fn wait_timeline(&self, sem: u64, value: u64, timeout_ns: u64) -> bool { + self.outstanding + .iter() + .find(|s| s.frame.semaphore.as_raw() == sem && s.frame.value == value) + .is_some_and(|s| self.dec.wait_decoded(&s.frame, timeout_ns)) + } + /// Drain the release channel, marking returned frames (release itself waits for /// the status read — see [`Self::settle_statuses`]). fn drain_releases(&mut self) { @@ -346,9 +364,12 @@ impl NativeVulkanDecoder { continue; } // A session rebuild (stream renegotiation) already made this frame stale: - // its pools are gone and a status poll would read the conservative Failed - // — which is NOT driver corruption. Resolve it quietly; the rebuild rode - // an IDR, so the stream has its re-anchor already. + // its SESSION objects (query pool included) are gone — the picture pool + // lives on in the decoder's graveyard while we hold the image, but the + // query verdict is unknowable and poll_status would report the + // conservative Failed — which is NOT driver corruption. Resolve it + // quietly; the rebuild rode an IDR, so the stream has its re-anchor + // already. if s.frame.generation != dec.generation() { tracing::debug!( poc = s.frame.poc, @@ -392,8 +413,11 @@ impl NativeVulkanDecoder { } match dec.release_frame(&s.frame, s.presented) { Ok(()) => {} - // A session rebuild (stream renegotiation) already dropped the pools - // this frame indexed — nothing left to release. + // Not a best-effort no-op: stale-generation frames release into the + // decoder's graveyard (a rebuild retires a still-held pool INTACT, + // and this very call is what lets it die on its last token). An Err + // is therefore a bookkeeping ghost — a double release — never a + // held image left dangling. Err(e) => tracing::debug!(error = %e, "release_frame: {e}"), } false @@ -425,7 +449,10 @@ impl Drop for NativeVulkanDecoder { // the decoder's Drop destroys the pool images: a returned token proves the // sampling submission's fence was waited, i.e. no GPU work of the // presenter's still reads the pools (the decoder's own drain covers only - // decode work; graveyarded pools ride the same token contract). + // decode work). Graveyarded pools ride the same token contract — a + // mid-stream renegotiation retires a still-held pool INTACT, and the + // release calls below route stale-generation frames into the graveyard, + // so those pools too die only once their last presenter fence was waited. let deadline = Instant::now() + TEARDOWN_BUDGET; loop { self.drain_releases(); @@ -499,6 +526,12 @@ mod tests { coded_width: 1920, coded_height: 1088, crop: pf_bitstream_crop(1920, 1080), + colour: pf_vkdecode::ColourDescription { + colour_primaries: 2, + transfer_characteristics: 2, + matrix_coefficients: 2, + video_full_range: false, + }, semaphore: vk::Semaphore::null(), value: 0, poc: 0, diff --git a/crates/pf-vkdecode/src/decoder.rs b/crates/pf-vkdecode/src/decoder.rs index 9159f2da..817af8f7 100644 --- a/crates/pf-vkdecode/src/decoder.rs +++ b/crates/pf-vkdecode/src/decoder.rs @@ -37,6 +37,7 @@ use std::collections::VecDeque; use ash::vk; use ash::vk::native as hh; use pf_bitstream::h264::AuPlan; +use pf_bitstream::h264::ColourDescription; use pf_bitstream::h264::DisplayCrop; use pf_bitstream::h264::DpbUpdate; use pf_bitstream::h264::H264Planner; @@ -125,6 +126,10 @@ pub struct DecodedVkFrame { pub coded_height: u32, /// Conformance-window crop: the region to display. pub crop: DisplayCrop, + /// Colour signalling from the picture's ACTIVE SPS (pf-bitstream applies + /// E.2.1's "unspecified" inference where the VUI is silent). Per frame, like + /// [`Self::crop`]: the host switches HDR in-band with a new SPS mid-stream. + pub colour: ColourDescription, /// Timeline pair: pixels ready at `semaphore >= value`; the sampling /// consumer signals `value + 1` (see the type docs). pub semaphore: vk::Semaphore, @@ -409,6 +414,7 @@ struct PendingPic { /// The image's timeline value the decode signalled (frame readiness). timeline_value: u64, crop: DisplayCrop, + colour: ColourDescription, poc: i32, is_idr: bool, } @@ -733,6 +739,7 @@ impl VkH264Decoder { query_slot: query_index, timeline_value: signal_value, crop: plan.picture.display_crop, + colour: plan.picture.colour, poc: plan.picture.pic_order_cnt, is_idr: plan.picture.is_idr, }, @@ -981,6 +988,30 @@ impl VkH264Decoder { } } + /// Wait — bounded by `timeout_ns` — for a delivered frame's decode-complete + /// signal ([`DecodedVkFrame::semaphore`] reaching [`DecodedVkFrame::value`]). + /// Pure measurement (the integration layer's sampled decode-latency stat): + /// touches no decoder state, so a timeout or error only degrades the stat — + /// the consumer's own GPU wait is what gates sampling, never this. `frame` + /// must be unreleased (`release_frame` still owed), which pins its pool — and + /// with it the semaphore — alive, graveyarded generations included; a + /// stale-generation frame declines rather than block on a verdict the + /// rebuild's drain already implied. + pub fn wait_decoded(&self, frame: &DecodedVkFrame, timeout_ns: u64) -> bool { + if frame.generation != self.generation { + return false; + } + let semaphores = [frame.semaphore]; + let values = [frame.value]; + let info = vk::SemaphoreWaitInfo::default() + .semaphores(&semaphores) + .values(&values); + // SAFETY: live device (constructor contract); the semaphore is a pool + // semaphore the unreleased frame keeps alive (fn docs); the info arrays + // are locals outliving the call. + unsafe { self.dev.ash().wait_semaphores(&info, timeout_ns) }.is_ok() + } + /// Drain the planner (teardown / stream discontinuity): every buffered /// picture becomes display-ready via [`Self::take_ready`] (zero-copy — the /// images already hold the content), all DPB slots free, and any picture @@ -1050,6 +1081,22 @@ impl VkH264Decoder { /// retiring its picture pool to the graveyard when the consumer still holds /// images) and build a fresh one shaped by `plan`, bumping /// [`Self::generation`] so frames of the old one route to the graveyard. + /// + /// Why a mid-stream rebuild is safe against presenter-held frames (the + /// renegotiation-teardown question, settled): + /// - **Images**: a pool with consumer holds retires to the graveyard INTACT — + /// images, views and semaphores stay live until `release_frame` takes its + /// last token, and a token is sent only after the presenter's sampling + /// submission's fence was waited (its `value+1` write-back included), so no + /// pool image is ever destroyed under in-flight GPU reads. + /// - **Tokens**: every frame and its token carry the generation they were + /// born under, and `release_frame` routes strictly by it (current pool vs + /// graveyard entry), so releases cannot alias across generations. + /// - **Session objects**: the session/ring/ops (query pool included) DO die + /// right here — but only after [`Self::drain_gpu`], and no consumer-facing + /// handle points at them: [`DecodedVkFrame`] borrows pool resources only, + /// and `poll_status` generation-gates before it would touch the NEW + /// generation's query pool with an old frame's slot. fn rebuild_state(&mut self, plan: &AuPlan) -> Result<(), VkDecodeError> { self.drain_gpu()?; if let Some(state) = self.state.take() { @@ -1251,6 +1298,7 @@ fn build_frame(state: &mut SessionState, entry: &PendingPic, generation: u64) -> coded_width: state.image_extent.width, coded_height: state.image_extent.height, crop: entry.crop, + colour: entry.colour, semaphore: picture.semaphore, value: entry.timeline_value, poc: entry.poc, diff --git a/crates/pf-vkdecode/src/lib.rs b/crates/pf-vkdecode/src/lib.rs index 91d7964d..f901ff83 100644 --- a/crates/pf-vkdecode/src/lib.rs +++ b/crates/pf-vkdecode/src/lib.rs @@ -62,10 +62,13 @@ pub mod slots; /// flattens them to raw `u64`s through `ash::vk::Handle` — via THIS instance of ash, so /// the versions can never skew. pub use ash; -/// Re-exported so [`VkH264Decoder::take_warnings`] consumers name the warning type — -/// and [`DecodedVkFrame::crop`]'s type — without growing a pf-bitstream dependency of -/// their own. +// The pf-bitstream types a [`DecodedVkFrame`] consumer names, re-exported so it +// doesn't grow a pf-bitstream dependency of its own: +/// [`DecodedVkFrame::colour`]'s type. +pub use pf_bitstream::h264::ColourDescription; +/// [`DecodedVkFrame::crop`]'s type. pub use pf_bitstream::h264::DisplayCrop; +/// [`VkH264Decoder::take_warnings`]'s warning type. pub use pf_bitstream::h264::PlanWarning; pub use caps::derive_caps;