From 6331ae7fd9d8fddd56998065569fda9ded78b6b9 Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Wed, 5 Aug 2026 19:12:20 +0200 Subject: [PATCH] fix(pf-vkdecode): zero-copy pool model + the two faults the first hardware run found MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit WP-D leg 1 (.25 RADV, distinct mode) root causes, both real: 1. Output starvation: the fixed 4-deep ring lost to a stream that keeps max_dpb_frames+1 = 8 pictures pending. Zero-copy fix (user requirement, no copies): one picture pool of required_slots + HOLD_HEADROOM(8) images decoupled from DPB slots — a re-activated slot binds a fresh free image, so a delivered picture is never a decode target; the WP-B pin layer became dead and is deleted. Per-image timeline semaphores carry the AVVkFrame contract: decode signals value+1, the presenter waits and signals back, later decodes wait the image's latest value — layout traffic ordered against reference reads with no copy anywhere. 2. RESULT_STATUS queries HANG RADV's VCN firmware (ring timeout, DEVICE_LOST): queryResultStatusSupport=false on the decode family. Queries are now caps-gated; without them poll/wait degrade to timeline-completion verdicts (FFmpeg parity — and the likely reason upstream never wired nb_queries). The Ally-X-class detection runs where drivers advertise the query; .173 probes NVIDIA/Windows-AMD. Also: slice-only bitstream feeding (the field-proven consumer shape), graveyarded pool retirement keyed by release tokens + generation, decode-current-AU-before-status attribution, take_ready drained, H264-bit gating, teardown short-circuit on disconnected channel. On-glass: 48 AUs green on .25 holding 4 frames like the real client. Gates: fmt clean, container clippy -D warnings zero, 27+121+52 green both platforms. --- crates/pf-client-core/src/session.rs | 6 + crates/pf-client-core/src/video.rs | 73 +- crates/pf-client-core/src/video_vk_native.rs | 164 ++- crates/pf-presenter/src/vk/present.rs | 37 +- crates/pf-vkdecode/src/caps.rs | 40 +- crates/pf-vkdecode/src/decoder.rs | 1042 +++++++++++------- crates/pf-vkdecode/src/device.rs | 41 +- crates/pf-vkdecode/src/images.rs | 551 ++++----- crates/pf-vkdecode/src/lib.rs | 15 +- crates/pf-vkdecode/src/pic.rs | 203 +++- crates/pf-vkdecode/src/ring.rs | 34 +- crates/pf-vkdecode/src/slots.rs | 133 +-- crates/pf-vkdecode/tests/gpu_smoke.rs | 110 +- 13 files changed, 1551 insertions(+), 898 deletions(-) diff --git a/crates/pf-client-core/src/session.rs b/crates/pf-client-core/src/session.rs index f9bd8312..443a8ce0 100644 --- a/crates/pf-client-core/src/session.rs +++ b/crates/pf-client-core/src/session.rs @@ -822,6 +822,12 @@ 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. _ => None, }; if present { diff --git a/crates/pf-client-core/src/video.rs b/crates/pf-client-core/src/video.rs index b1836b51..fb75f049 100644 --- a/crates/pf-client-core/src/video.rs +++ b/crates/pf-client-core/src/video.rs @@ -163,11 +163,18 @@ pub enum NativeVkLayout { /// The release token a presented/dropped [`NativeVkFrame`] hands back to the native /// decode backend: `seq` names the shipped frame, `generation` the decoder session it -/// belongs to (a stale generation releases nothing — the pools it indexed are gone). +/// belongs to (a stale generation routes to the decoder's graveyard — retired pools +/// die on their last token), and `presented` reports whether the presenter SAMPLED +/// the image — i.e. whether its submission enqueued the frame's `value + 1` timeline +/// signal (the AVVkFrame write-back the decoder must wait before reusing the image). #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub struct NativeReleaseToken { pub seq: u64, pub generation: u64, + /// The presenter's sampling submission (with its `value + 1` signal) was + /// enqueued for this frame. `false` for frames dropped unpresented + /// (newest-wins displacement, demotion drain, failed submit). + pub presented: bool, } /// Sends the frame's [`NativeReleaseToken`] exactly once, on drop — the native path's @@ -192,6 +199,16 @@ impl NativeReleaseGuard { token: Some(token), } } + + /// Record that the sampling submission — including the frame's `value + 1` + /// timeline signal — was enqueued. The presenter calls this exactly when its + /// submit succeeded; the token then tells the decoder to wait that write-back + /// before the image's next use. + pub fn mark_presented(&mut self) { + if let Some(token) = &mut self.token { + token.presented = true; + } + } } impl Drop for NativeReleaseGuard { @@ -442,8 +459,23 @@ const HW_DEMOTE_MIN_STREAK: std::time::Duration = std::time::Duration::from_mill /// presenter's device actually advertises Vulkan Video decode. Deliberately NOT an /// `auto` rung: entering the automatic ladder is WP-D's A/B verdict. Pure so the /// decision is CPU-testable. -fn native_vulkan_gate(choice: &str, codec_id: ffmpeg::codec::Id, video_decode: bool) -> bool { - choice == "native-vulkan" && codec_id == ffmpeg::codec::Id::H264 && video_decode +/// `VK_VIDEO_CODEC_OPERATION_DECODE_H264_BIT_KHR` — the raw flag bit within +/// [`VulkanDecodeDevice::decode_video_caps`] (this crate stays ash-free). +const VIDEO_CODEC_OP_DECODE_H264: u32 = 0x0000_0001; + +fn native_vulkan_gate( + choice: &str, + codec_id: ffmpeg::codec::Id, + video_decode: bool, + decode_video_caps: u32, +) -> bool { + choice == "native-vulkan" + && codec_id == ffmpeg::codec::Id::H264 + && video_decode + // The decode family must advertise the H264 op specifically — + // `video_decode` alone proves the extension stack, not the codec (an + // AV1-only decode family exists on real hardware). + && decode_video_caps & VIDEO_CODEC_OP_DECODE_H264 != 0 } /// Map a negotiated `quic` codec bit to the FFmpeg decoder id the client opens. @@ -642,7 +674,12 @@ impl Decoder { // other, than the FFmpeg rungs' failures do. let mut choice = choice; if choice == "native-vulkan" { - if native_vulkan_gate(&choice, codec_id, vk.is_some_and(|v| v.video_decode)) { + if native_vulkan_gate( + &choice, + codec_id, + vk.is_some_and(|v| v.video_decode), + vk.map_or(0, |v| v.decode_video_caps), + ) { let vk = vk.expect("gate demands video_decode, so vk is Some"); match NativeVulkanDecoder::new(vk) { Ok(n) => { @@ -1314,16 +1351,34 @@ mod tests { #[test] fn native_vulkan_gate_is_by_name_h264_and_capable_device_only() { use ffmpeg::codec::Id; - assert!(native_vulkan_gate("native-vulkan", Id::H264, true)); + const H264_OP: u32 = VIDEO_CODEC_OP_DECODE_H264; + assert!(native_vulkan_gate("native-vulkan", Id::H264, true, H264_OP)); // Never by any other preference — it is not an auto rung yet. for choice in ["auto", "", "hardware", "vulkan", "software"] { - assert!(!native_vulkan_gate(choice, Id::H264, true), "{choice:?}"); + assert!( + !native_vulkan_gate(choice, Id::H264, true, H264_OP), + "{choice:?}" + ); } // The one codec pf-vkdecode speaks. - assert!(!native_vulkan_gate("native-vulkan", Id::HEVC, true)); - assert!(!native_vulkan_gate("native-vulkan", Id::AV1, true)); + assert!(!native_vulkan_gate( + "native-vulkan", + Id::HEVC, + true, + H264_OP + )); + assert!(!native_vulkan_gate("native-vulkan", Id::AV1, true, H264_OP)); // No Vulkan-Video-capable presenter device. - assert!(!native_vulkan_gate("native-vulkan", Id::H264, false)); + assert!(!native_vulkan_gate( + "native-vulkan", + Id::H264, + false, + H264_OP + )); + // A decode family WITHOUT the H264 op (e.g. AV1-only) refuses even with + // the extension stack present — the caps BIT is the codec gate. + assert!(!native_vulkan_gate("native-vulkan", Id::H264, true, 0)); + assert!(!native_vulkan_gate("native-vulkan", Id::H264, true, 0x4)); } /// Lock the DRM FourCC magic numbers against typos — these are the exact values diff --git a/crates/pf-client-core/src/video_vk_native.rs b/crates/pf-client-core/src/video_vk_native.rs index a085dd17..e8d1d37f 100644 --- a/crates/pf-client-core/src/video_vk_native.rs +++ b/crates/pf-client-core/src/video_vk_native.rs @@ -107,6 +107,10 @@ struct Shipped { frame: DecodedVkFrame, /// The presenter (or a drop on the way there) returned the token. released: bool, + /// The token said the sampling submission (with its `value + 1` timeline + /// signal) was enqueued — forwarded to `release_frame` so the decoder waits + /// the write-back before reusing the image. + presented: bool, /// The status query read a conclusive verdict (or the poll belt expired). resolved: bool, /// Polls attempted after the token returned — see [`MAX_POLLS_AFTER_RELEASE`]. @@ -124,6 +128,7 @@ fn note_token(outstanding: &mut [Shipped], token: NativeReleaseToken) -> bool { "a token's generation always matches the frame it rode on" ); s.released = true; + s.presented = token.presented; true } None => false, @@ -133,9 +138,14 @@ fn note_token(outstanding: &mut [Shipped], token: NativeReleaseToken) -> bool { /// The native backend: the decoder plus the shipped-frame ledger and release channel. pub(crate) struct NativeVulkanDecoder { dec: VkH264Decoder, - /// Cloned into every shipped frame's guard. - release_tx: mpsc::Sender, + /// Cloned into every shipped frame's guard. `Option` so teardown can DROP the + /// backend's own sender: only then does `release_rx` report Disconnected once + /// the last guard is gone — the teardown short-circuit signal. + release_tx: Option>, release_rx: mpsc::Receiver, + /// Display-ready frames not yet handed to the pump (burst outputs — decode + /// delivers one per call; the rest wait here, oldest first). + deliverable: std::collections::VecDeque, outstanding: Vec, next_seq: u64, } @@ -181,8 +191,9 @@ impl NativeVulkanDecoder { let (release_tx, release_rx) = mpsc::channel(); Ok(NativeVulkanDecoder { dec, - release_tx, + release_tx: Some(release_tx), release_rx, + deliverable: std::collections::VecDeque::new(), outstanding: Vec::new(), next_seq: 0, }) @@ -193,32 +204,45 @@ impl NativeVulkanDecoder { /// decode trouble — a decoder error, a plan that needed concealment, or a /// driver-reported corrupt PREVIOUS frame — routed through the caller's shared /// streak/demotion machinery. + /// + /// Ordering: the CURRENT AU decodes FIRST — the planner's reference state must + /// advance even when a PRIOR frame's status turns out Failed, or the recovery + /// IDR would land on a decoder that skipped an AU and reports a phantom + /// reference gap. The prior-frame verdicts are checked after; a corrupt verdict + /// costs exactly this one AU's output (released unshown), never parser state. pub(crate) fn decode(&mut self, au: &[u8]) -> Result> { self.drain_releases(); - let corrupt = self.settle_statuses(); - if corrupt > 0 { - // Driver-reported decode corruption on an already-delivered frame — the - // Ally X class, invisible to FFmpeg's query-less decoder. The frame is on - // (or past) the glass; erroring THIS call is what arms the reanchor gate - // and gets the IDR that replaces the corrupt content. - return Err(anyhow!( - "driver reported decode corruption on {corrupt} prior frame(s) \ - (RESULT_STATUS_ONLY query) — re-anchor needed" - )); - } let delivered = self.dec.decode(au).map_err(|e| anyhow!("decode: {e}"))?; let warnings = self.dec.take_warnings(); - if !warnings.is_empty() { - // The AU was planned around missing/damaged references: the picture - // decodes, but its content is concealed. Release it unshown and surface - // the AU as decode trouble — same path, same volume as an FFmpeg + // Everything this AU made display-ready, oldest first (`take_ready` drained + // so burst outputs are never stranded inside the decoder). + let mut fresh: Vec = Vec::new(); + if let Some(frame) = delivered { + fresh.push(frame); + } + while let Some(frame) = self.dec.take_ready() { + fresh.push(frame); + } + + let corrupt = self.settle_statuses(); + if !warnings.is_empty() || corrupt > 0 { + // Concealment planned into THIS AU, or driver-reported corruption on a + // PRIOR frame (the Ally X class, invisible to FFmpeg's query-less + // decoder): this call's output is released unshown and the call errors, + // arming the reanchor gate — same path, same volume as an FFmpeg // reference-miss error (never quieter). - if let Some(frame) = &delivered { - if let Err(e) = self.dec.release_frame(frame) { - tracing::debug!(error = %e, "releasing a concealed frame failed"); + for frame in fresh { + if let Err(e) = self.dec.release_frame(&frame, false) { + tracing::debug!(error = %e, "releasing an unshown frame failed"); } } + if corrupt > 0 { + return Err(anyhow!( + "driver reported decode corruption on {corrupt} prior frame(s) \ + (RESULT_STATUS_ONLY query) — re-anchor needed" + )); + } tracing::warn!( ?warnings, "native decode planned with concealment — dropping the frame, \ @@ -230,7 +254,8 @@ impl NativeVulkanDecoder { ); } - Ok(delivered.map(|frame| self.ship(frame))) + self.deliverable.extend(fresh); + Ok(self.deliverable.pop_front().map(|frame| self.ship(frame))) } /// Wrap a delivered [`DecodedVkFrame`] for the presenter and enter it into the @@ -241,6 +266,7 @@ impl NativeVulkanDecoder { let token = NativeReleaseToken { seq, generation: frame.generation, + presented: false, }; let native = NativeVkFrame { image: frame.image.as_raw(), @@ -270,12 +296,19 @@ impl NativeVulkanDecoder { }, keyframe: frame.is_idr, poc: frame.poc, - guard: NativeReleaseGuard::new(self.release_tx.clone(), token), + guard: NativeReleaseGuard::new( + self.release_tx + .as_ref() + .expect("release_tx lives until Drop") + .clone(), + token, + ), }; self.outstanding.push(Shipped { seq, frame, released: false, + presented: false, resolved: false, polls_after_release: 0, }); @@ -357,7 +390,7 @@ impl NativeVulkanDecoder { if !(s.released && s.resolved) { return true; } - match dec.release_frame(&s.frame) { + 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. @@ -371,14 +404,43 @@ impl NativeVulkanDecoder { impl Drop for NativeVulkanDecoder { fn drop(&mut self) { - // Wait (bounded) for the presenter to hand back every shipped frame before 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). + // Ordering contract: the run loop drops the PRESENTER's frame (its retired + // slot, fence-waited) before joining the pump that owns this backend — so + // by the time this Drop runs, outstanding tokens are either already in the + // channel or arrive imminently; the bounded wait below is for that hand-off, + // not for future GPU work. + // + // Frames never handed to the pump release directly (unsampled). + for frame in std::mem::take(&mut self.deliverable) { + if let Err(e) = self.dec.release_frame(&frame, false) { + tracing::debug!(error = %e, "releasing an undelivered frame failed"); + } + } + // Drop our own sender FIRST: once every shipped guard is gone too, the + // channel reports Disconnected — the "presenter can no longer produce + // tokens" signal that short-circuits the wait instead of burning the full + // budget against a presenter that is already gone. + drop(self.release_tx.take()); + // Wait (bounded) for the presenter to hand back every shipped frame before + // 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). let deadline = Instant::now() + TEARDOWN_BUDGET; loop { self.drain_releases(); - self.outstanding.retain(|s| !s.released); + let Self { + dec, outstanding, .. + } = self; + outstanding.retain(|s| { + if !s.released { + return true; + } + if let Err(e) = dec.release_frame(&s.frame, s.presented) { + tracing::debug!(error = %e, "teardown release_frame: {e}"); + } + false + }); if self.outstanding.is_empty() { break; } @@ -391,7 +453,6 @@ impl Drop for NativeVulkanDecoder { ); break; } - // `self` holds a Sender, so the channel can't disconnect — only time out. match self .release_rx .recv_timeout((deadline - now).min(Duration::from_millis(50))) @@ -399,10 +460,25 @@ impl Drop for NativeVulkanDecoder { Ok(token) => { note_token(&mut self.outstanding, token); } - Err(_) => continue, + // Every sender is gone (ours dropped above, every guard dropped): + // no more tokens can EVER arrive — anything still outstanding is a + // bookkeeping ghost, not a held frame. Stop waiting. + Err(mpsc::RecvTimeoutError::Disconnected) => { + if !self.outstanding.is_empty() { + tracing::debug!( + outstanding = self.outstanding.len(), + "release channel disconnected with entries outstanding — \ + no tokens can arrive; proceeding with teardown" + ); + } + break; + } + Err(mpsc::RecvTimeoutError::Timeout) => continue, } } - // `self.dec` drops after this body: it drains its own decode-side GPU work. + // `self.dec` drops after this body: it drains its own decode-side GPU work + // and destroys any remaining graveyard pools (warned — a forfeit here means + // the presenter kept frames past the budget). } } @@ -428,9 +504,12 @@ mod tests { poc: 0, is_idr: false, query_slot: 0, + submission: 0, + picture: 0, generation, }, released: false, + presented: false, resolved: false, polls_after_release: 0, } @@ -452,18 +531,25 @@ mod tests { &mut outstanding, NativeReleaseToken { seq: 1, - generation: 1 + generation: 1, + presented: true, } )); assert!(!outstanding[0].released); assert!(outstanding[1].released); + assert!( + outstanding[1].presented, + "the token's presented flag rides into the ledger (the decoder waits \ + the presenter's value+1 write-back only when it was really enqueued)" + ); // A stray token (frame already settled away — e.g. a post-demotion drain) // matches nothing and must not panic or mis-mark. assert!(!note_token( &mut outstanding, NativeReleaseToken { seq: 7, - generation: 1 + generation: 1, + presented: false, } )); assert!(!outstanding[0].released); @@ -475,6 +561,7 @@ mod tests { let token = NativeReleaseToken { seq: 42, generation: 3, + presented: false, }; let guard = NativeReleaseGuard::new(tx, token); assert!( @@ -518,6 +605,7 @@ mod tests { NativeReleaseToken { seq: 9, generation: 5, + presented: false, }, ), }; @@ -526,8 +614,11 @@ mod tests { rx.try_recv().ok(), Some(NativeReleaseToken { seq: 9, - generation: 5 - }) + generation: 5, + presented: false, + }), + "an unpresented drop reports presented=false — the decoder must not \ + wait a value+1 write-back that was never enqueued" ); } @@ -542,6 +633,7 @@ mod tests { NativeReleaseToken { seq: 1, generation: 1, + presented: false, }, ); drop(guard); // must not panic diff --git a/crates/pf-presenter/src/vk/present.rs b/crates/pf-presenter/src/vk/present.rs index b0ff98f2..f76501fd 100644 --- a/crates/pf-presenter/src/vk/present.rs +++ b/crates/pf-presenter/src/vk/present.rs @@ -408,12 +408,13 @@ impl Presenter { // stamped layout/semaphore/value at delivery and nothing mutates them). // Transition the picture's LAYER for sampling, run the same CSC pass with // the coded-vs-display UV scale (the 1088-row lesson), then transition BACK - // to the decode layout: a coincide-mode picture is a live DPB slot the next - // decode must find in VIDEO_DECODE_DPB_KHR (distinct-mode DST images get the - // same round-trip — the decoder's reuse barrier discards via UNDEFINED, so - // the restore costs nothing and keeps one rule). The pool images are created - // CONCURRENT across the graphics+decode families, so these are plain layout - // transitions — no queue-family ownership transfer. + // to the decode layout the frame names — and the submit below signals the + // image's timeline at `value + 1` when these reads/restores complete, which + // the decoder (told via the release token) waits before that image's next + // decode use: the layout round-trip is ORDERED against decode, not raced. + // The pool images are created CONCURRENT across the graphics+decode + // families, so these are plain layout transitions — no queue-family + // ownership transfer. let mut native_wait: Option<(vk::Semaphore, u64)> = None; if let (Some(f), Some(v)) = (&native_frame, &self.video) { let image = vk::Image::from_raw(f.image); @@ -638,15 +639,18 @@ impl Presenter { } // The native frame's decode-complete timeline: wait it at FRAGMENT_SHADER // (chaining with the acquire barrier — the same dependency-chain rule as - // `vkframe_acquire_barrier`). Deliberately NO signal back on the decoder's - // timeline: its per-slot values are the DECODER's counter (a foreign signal - // would collide with its next decode's value) — the slot-return contract is - // the release token the frame's guard sends once our fence proves the reads - // done. + // `vkframe_acquire_barrier`), and SIGNAL `value + 1` when our reads and + // the layout restore are done — the exact AVVkFrame write-back contract + // of the arm above. The decoder learns of the enqueued signal through + // the release token (`mark_presented`) and waits it before the image's + // next decode use; per-IMAGE timelines make the value spaces private, so + // this cannot collide with any other image's counter. if let Some((sem, value)) = &native_wait { wait_sems.push(*sem); wait_stages.push(vk::PipelineStageFlags::FRAGMENT_SHADER); wait_values.push(*value); + signal_sems.push(*sem); + signal_values.push(*value + 1); } let mut timeline = vk::TimelineSemaphoreSubmitInfo::default() .wait_semaphore_values(&wait_values) @@ -721,9 +725,14 @@ impl Presenter { if let Some(f) = win_frame.take() { self.retired_hw = Some(Retired::D3d11(f)); } - // Native frame: parked until the fence proves the sampling reads done — its - // drop THEN sends the decoder's release token (never at record time). - if let Some(f) = native_frame.take() { + // Native frame: the submit above enqueued our `value + 1` signal — mark + // the token so the decoder waits that write-back before reusing the + // image (a failed submit skipped this whole block, leaving the token + // unmarked: no phantom signal is ever promised). Then park until the + // fence proves the sampling reads done — the drop THEN sends the + // release token (never at record time). + if let Some(mut f) = native_frame.take() { + f.guard.mark_presented(); self.retired_hw = Some(Retired::NativeVk(f)); } diff --git a/crates/pf-vkdecode/src/caps.rs b/crates/pf-vkdecode/src/caps.rs index 6f3d56cd..6377a4dd 100644 --- a/crates/pf-vkdecode/src/caps.rs +++ b/crates/pf-vkdecode/src/caps.rs @@ -151,6 +151,14 @@ pub enum CapsError { /// so the per-plane `R8`/`R8G8` views the presenter samples through cannot /// exist on this device. NoMutableFormat { mode: &'static str }, + /// The driver forces COINCIDE mode AND a layered DPB (one image array, no + /// `SEPARATE_REFERENCE_IMAGES`): the picture-pool model — a re-activated slot + /// binding a fresh free image, so delivered pictures are never decode targets + /// — cannot exist when every slot is a fixed layer of one array. No fleet + /// device has this shape (NVIDIA = distinct, RADV = separate reference + /// images); a device that does demotes to the next decoder rung rather than + /// getting a degraded copy path built for it. + CoincideLayeredDpb, } impl std::fmt::Display for CapsError { @@ -177,6 +185,13 @@ impl std::fmt::Display for CapsError { "the {mode} NV12 entry does not allow MUTABLE_FORMAT (per-plane views)" ) } + CapsError::CoincideLayeredDpb => { + write!( + f, + "coincide mode with a layered DPB (no SEPARATE_REFERENCE_IMAGES) — \ + the picture-pool model needs per-slot images; demote this device" + ) + } } } } @@ -196,12 +211,20 @@ pub fn derive_caps(raw: &RawH264Caps) -> Result { return Err(CapsError::NoDecodeMode); } + let layered_dpb = !raw + .capability_flags + .contains(vk::VideoCapabilityFlagsKHR::SEPARATE_REFERENCE_IMAGES); // Coincide preferred when both are offered (struct docs). Each picked entry is // validated against the EXACT usage/create-flags its pool will use: presenter- // facing images (coincide pool, distinct outputs) additionally need // MUTABLE_FORMAT for their per-plane views; the distinct DPB needs neither // sampling nor plane views. let (dpb_format, output_format) = if coincide { + if layered_dpb { + // The picture-pool model needs per-slot images (a slot re-binds a + // fresh image at activation); one fixed layer per slot cannot do that. + return Err(CapsError::CoincideLayeredDpb); + } let mode = "coincide (DPB|DST|SAMPLED)"; let entry = pick_nv12(&raw.coincide_formats, mode)?; require_usage(&entry, COINCIDE_USAGE, mode)?; @@ -219,9 +242,7 @@ pub fn derive_caps(raw: &RawH264Caps) -> Result { Ok(DecodeCaps { coincide, - layered_dpb: !raw - .capability_flags - .contains(vk::VideoCapabilityFlagsKHR::SEPARATE_REFERENCE_IMAGES), + layered_dpb, min_bitstream_offset_alignment: raw.min_bitstream_buffer_offset_alignment.max(1), min_bitstream_size_alignment: raw.min_bitstream_buffer_size_alignment.max(1), picture_access_granularity: raw.picture_access_granularity, @@ -656,6 +677,19 @@ mod tests { assert_eq!((aligned.width, aligned.height), (321, 241)); } + #[test] + fn coincide_with_a_layered_dpb_is_unsupported_not_worked_around() { + // A driver forcing coincide AND a single layered DPB array: the pool + // model (fresh image per activation) cannot exist there, and no fleet + // device has this shape — refuse so the ladder demotes. + let mut raw = radv_like(); + raw.capability_flags = vk::VideoCapabilityFlagsKHR::empty(); + assert_eq!( + derive_caps(&raw).unwrap_err(), + CapsError::CoincideLayeredDpb + ); + } + #[test] fn zero_alignments_normalize_to_one_so_ring_math_never_divides_by_zero() { let mut raw = radv_like(); diff --git a/crates/pf-vkdecode/src/decoder.rs b/crates/pf-vkdecode/src/decoder.rs index 5c46a292..bb1bc928 100644 --- a/crates/pf-vkdecode/src/decoder.rs +++ b/crates/pf-vkdecode/src/decoder.rs @@ -5,18 +5,31 @@ //! (barriers, `vkCmdBeginVideoCodingKHR` with every bound DPB slot, the one-time //! session RESET control, a `RESULT_STATUS_ONLY` query bracketing //! `vkCmdDecodeVideoKHR`) → submit on the decode queue under the caller's -//! [`QueueLock`] with a per-output-slot timeline signal. +//! [`QueueLock`] with a per-image timeline signal. +//! +//! **Image model (zero-copy, the FFmpeg pool contract):** decode targets come from +//! a picture pool DECOUPLED from DPB slots ([`crate::images`] module docs) — a +//! slot binds a fresh free image at activation, so a delivered picture is never a +//! decode target while the consumer reads it. Each image's own timeline semaphore +//! carries the AVVkFrame hand-off: the decoder signals `value+1` at decode-write, +//! the presenter waits it, samples, restores the layout and signals `value+1` +//! again in its own submission; [`VkH264Decoder::release_frame`] reports that +//! write-back and the decoder waits it before the image's next use — presenter +//! layout traffic is ordered against decode reads without any copy. //! //! The status query is THE point of this program: FFmpeg's `vulkan_decode.c` runs //! `nb_queries = 0` and therefore architecturally cannot see driver-reported decode //! corruption (the Xbox Ally X field case). Here every decode op has a query slot, //! [`VkH264Decoder::poll_status`] reads it WITHOUT waiting, and a non-COMPLETE -//! result is the concealment signal WP-C wires to `want_keyframe`. +//! result is the concealment signal the integration layer wires to +//! `want_keyframe`. //! -//! What stays for WP-C (the integration layer): feeding `PlanWarning`s and Failed -//! statuses into the recovery machinery, presenting frames (including the -//! coincide-mode layout dance — see [`DecodedVkFrame::layout`]), and throttling so -//! output slots are consumed before their ring position recycles. +//! Known residual (WP-D on-glass, same class as the shipping AVVkFrame arm): a +//! delivered frame whose picture is STILL a live reference can be sampled by the +//! presenter while a decode references it — reads on both sides, but the +//! presenter's layout round-trip writes metadata. `VK_KHR_unified_image_layouts` +//! (GENERAL everywhere) removes the round-trip entirely and is the documented +//! fast-path TODO once the fleet's drivers carry it. use std::collections::BTreeMap; use std::collections::VecDeque; @@ -45,8 +58,9 @@ use crate::device::DeviceHandles; use crate::device::QueueLock; use crate::device::QueueSubmitGuard; use crate::images::plan_pools; -use crate::images::ImagePool; -use crate::images::OUTPUT_RING; +use crate::images::DpbPool; +use crate::images::PicturePool; +use crate::images::HOLD_HEADROOM; use crate::params::level_to_std; use crate::params::ParamsError; use crate::pic::plan_to_vk; @@ -61,7 +75,6 @@ use crate::session::ParamsAction; use crate::session::SessionConfig; use crate::session::SessionError; use crate::session::VideoSession; -use crate::slots::SlotError; use crate::slots::SlotMap; /// Ceiling on any blocking GPU wait on the decode thread (5 s) — generous against @@ -82,45 +95,50 @@ pub enum DecodeStatus { Failed, } -/// One decoded, display-ready picture. Handles are BORROWED from the decoder's -/// pools: valid until the decoder rebuilds its session (stream renegotiation — -/// detectable via [`Self::generation`]). The consumer waits `semaphore >= value` -/// before reading pixels and MUST hand every delivered frame back through -/// [`VkH264Decoder::release_frame`] once done — the frame's slot is excluded from -/// every reuse path (setup assignment, output-ring recycling) until then, which -/// is what makes a delivered image safe to read while decoding continues. +/// One decoded, display-ready picture over a pool image the decoder will not +/// touch again until [`VkH264Decoder::release_frame`] returns it. +/// +/// Sync contract (the AVVkFrame shape): pixels are ready when `semaphore` +/// reaches [`Self::value`]. A consumer that SAMPLES the image must, in the same +/// submission that waits `value`, signal `value + 1` after its reads (and layout +/// restore) — and report that via `release_frame(frame, true)`; a consumer that +/// drops the frame unsampled releases with `false`. Handles survive session +/// rebuilds via the graveyard: release every frame exactly once, even +/// stale-generation ones. #[derive(Debug, Clone)] pub struct DecodedVkFrame { pub image: vk::Image, - /// Full-picture NV12 view (what decode wrote through). + /// Full-picture NV12 view. pub view: vk::ImageView, /// `R8`/`R8G8` per-plane views for the presenter's sampler path. pub plane_views: [vk::ImageView; 2], - /// The image array layer the picture occupies (the views already select it). + /// Always 0 — pool images are single-layer (kept for the consumer ABI). pub layer: u32, - /// The layout the picture is in when the semaphore signals: - /// `VIDEO_DECODE_DST_KHR` (distinct mode) or `VIDEO_DECODE_DPB_KHR` (coincide - /// mode — the picture may still be a live reference, so a consumer that - /// transitions it for sampling MUST transition it back before the next decode - /// references the slot; WP-C owns that dance). + /// The layout the picture is in when the semaphore signals — and the layout + /// the consumer must RESTORE after sampling: `VIDEO_DECODE_DPB_KHR` + /// (coincide) or `VIDEO_DECODE_DST_KHR` (distinct). pub layout: vk::ImageLayout, + /// The ALLOCATED picture extent (`pictureAccessGranularity`-aligned) — what + /// UV-scale math must divide by (the 1088-row class); the DISPLAY region is + /// [`Self::crop`]. pub coded_width: u32, pub coded_height: u32, - /// Conformance-window crop: the region to display. First-class here so no - /// consumer ever derives geometry from the (padded) pool shape again. + /// Conformance-window crop: the region to display. pub crop: DisplayCrop, - /// Timeline pair: the picture's pixels are ready when `semaphore` reaches - /// `value`. + /// Timeline pair: pixels ready at `semaphore >= value`; the sampling + /// consumer signals `value + 1` (see the type docs). pub semaphore: vk::Semaphore, pub value: u64, pub poc: i32, pub is_idr: bool, - /// The decode op's slot in the status query pool (for [`VkH264Decoder::poll_status`]). + /// The decode op's slot in the status query pool. pub query_slot: u32, - /// The session generation this frame's handles belong to. Bumped on every - /// session rebuild; a frame from an older generation points into destroyed - /// pools, so every decoder entry point taking a frame checks this FIRST and - /// reports the frame stale rather than touching the new pools. + /// The decode op's submission ordinal (validates the query slot has not been + /// re-armed since). + pub submission: u64, + /// The pool index of the image (release bookkeeping). + pub picture: u32, + /// The session generation this frame belongs to (graveyard routing). pub generation: u64, } @@ -150,14 +168,14 @@ pub enum VkDecodeError { /// A bounded GPU wait expired: the driver is wedged; treat as fatal for this /// decoder instance. Timeout(&'static str), - /// Every slot that could host this decode still backs an unreleased frame: - /// the consumer owes [`VkH264Decoder::release_frame`] calls. Unreachable - /// under WP-C's one-in/one-out loop (each delivered frame is released before - /// the next `decode`); reaching it means the AU was planned but NOT decoded — - /// the caller should release frames and request a keyframe. + /// The picture pool is exhausted: the consumer holds more than + /// [`HOLD_HEADROOM`] unreleased frames while the stream's whole DPB depth is + /// live — a real backpressure fault worth surfacing (the pool is sized so a + /// correct consumer can never hit this). The AU was planned but NOT decoded; + /// release frames and request a keyframe. NoFreeSlot, - /// The frame belongs to an older session generation (its handles point into - /// destroyed pools). Delivered frames do not survive a stream renegotiation. + /// The frame belongs to a generation whose retired pool is already gone + /// (double release, or a frame outliving its graveyard entry). StaleFrame { frame_generation: u64, current_generation: u64, @@ -186,7 +204,8 @@ impl std::fmt::Display for VkDecodeError { VkDecodeError::NoFreeSlot => { write!( f, - "every candidate slot backs an unreleased frame — release_frame owed" + "picture pool exhausted — more than {HOLD_HEADROOM} delivered frames \ + are unreleased (release_frame owed)" ) } VkDecodeError::StaleFrame { @@ -195,8 +214,8 @@ impl std::fmt::Display for VkDecodeError { } => { write!( f, - "frame from session generation {frame_generation}, current is \ - {current_generation} — its handles are gone" + "frame from session generation {frame_generation} (current \ + {current_generation}) has no retired pool — double release?" ) } VkDecodeError::NoMemoryType { type_bits, flags } => { @@ -279,11 +298,18 @@ impl From for VkDecodeError { } } -/// Query pool + command pool/buffers, one op slot per output slot. Owns and -/// destroys its Vulkan objects. +/// Query pool + command pool/buffers. Query slots cycle per SUBMISSION (validated +/// against [`DecodedVkFrame::submission`]); command buffers cycle within the +/// bitstream ring's in-flight bound. Owns and destroys its Vulkan objects. +/// +/// `query_pool` is `None` when the decode family lacks `queryResultStatusSupport` +/// (RADV): recording a RESULT_STATUS query there is invalid — on the .25 box it +/// HANGS the VCN ring — so no query objects exist at all and status verdicts fall +/// back to timeline completion. struct OpRing { device: ash::Device, - query_pool: vk::QueryPool, + query_pool: Option, + query_count: u32, cmd_pool: vk::CommandPool, cmds: Vec, } @@ -295,21 +321,36 @@ impl OpRing { unsafe fn create( dev: &DecodeDevice, std_profile_idc: hh::StdVideoH264ProfileIdc, - op_slots: u32, + query_count: u32, + cmd_count: u32, ) -> Result { - let mut chain = H264ProfileChain::new(std_profile_idc); - let profile = chain.wire(); - let mut query_ci = vk::QueryPoolCreateInfo::default() - .query_type(vk::QueryType::RESULT_STATUS_ONLY_KHR) - .query_count(op_slots); - // Chained manually: `push_next` would clobber the profile's own `p_next` - // (its H264 half) — the encoder's exact precedent for this trap. - query_ci.p_next = (profile as *const vk::VideoProfileInfoKHR<'_>).cast(); - // SAFETY: live device; `query_ci` roots the wired chain for the call. The - // video profile chained in satisfies the "same profile as the session" - // rule for queries used inside a coding scope. - let query_pool = unsafe { dev.ash().create_query_pool(&query_ci, None)? }; + let query_pool = if dev.result_status_queries() { + let mut chain = H264ProfileChain::new(std_profile_idc); + let profile = chain.wire(); + let mut query_ci = vk::QueryPoolCreateInfo::default() + .query_type(vk::QueryType::RESULT_STATUS_ONLY_KHR) + .query_count(query_count); + // Chained manually: `push_next` would clobber the profile's own + // `p_next` (its H264 half) — the encoder's exact precedent. + query_ci.p_next = (profile as *const vk::VideoProfileInfoKHR<'_>).cast(); + // SAFETY: live device; `query_ci` roots the wired chain for the call. + // The video profile chained in satisfies the "same profile as the + // session" rule for queries used inside a coding scope. + Some(unsafe { dev.ash().create_query_pool(&query_ci, None)? }) + } else { + debug!( + "decode family lacks queryResultStatusSupport — no per-op status \ + queries on this driver (verdicts fall back to timeline completion)" + ); + None + }; + let destroy_query = |pool: Option| { + if let Some(pool) = pool { + // SAFETY: destroying the just-created query pool (unwind path). + unsafe { dev.ash().destroy_query_pool(pool, None) }; + } + }; let pool_ci = vk::CommandPoolCreateInfo::default() .queue_family_index(dev.decode_qf()) .flags(vk::CommandPoolCreateFlags::RESET_COMMAND_BUFFER); @@ -317,30 +358,28 @@ impl OpRing { let cmd_pool = match unsafe { dev.ash().create_command_pool(&pool_ci, None) } { Ok(p) => p, Err(e) => { - // SAFETY: destroying the just-created query pool. - unsafe { dev.ash().destroy_query_pool(query_pool, None) }; + destroy_query(query_pool); return Err(e); } }; let alloc = vk::CommandBufferAllocateInfo::default() .command_pool(cmd_pool) - .command_buffer_count(op_slots); + .command_buffer_count(cmd_count); // SAFETY: live device + the pool created above; unwind destroys both pools // (destroying the command pool frees any allocated buffers). let cmds = match unsafe { dev.ash().allocate_command_buffers(&alloc) } { Ok(c) => c, Err(e) => { - // SAFETY: destroying the two pools created above. - unsafe { - dev.ash().destroy_command_pool(cmd_pool, None); - dev.ash().destroy_query_pool(query_pool, None); - } + // SAFETY: destroying the command pool created above. + unsafe { dev.ash().destroy_command_pool(cmd_pool, None) }; + destroy_query(query_pool); return Err(e); } }; Ok(Self { device: dev.ash().clone(), query_pool, + query_count, cmd_pool, cmds, }) @@ -354,17 +393,42 @@ impl Drop for OpRing { // its buffers; both destroys ignore NULL. unsafe { self.device.destroy_command_pool(self.cmd_pool, None); - self.device.destroy_query_pool(self.query_pool, None); + if let Some(pool) = self.query_pool { + self.device.destroy_query_pool(pool, None); + } } } } -/// Everything tied to ONE session generation. A stream renegotiation (extent, DPB -/// depth, profile) drops and rebuilds the whole struct. +/// A decoded picture awaiting its output verdict: which pool image holds it and +/// everything its eventual [`DecodedVkFrame`] needs. +struct PendingPic { + image: usize, + submission: u64, + query_slot: u32, + /// The image's timeline value the decode signalled (frame readiness). + timeline_value: u64, + crop: DisplayCrop, + poc: i32, + is_idr: bool, +} + +/// A retired generation's picture pool: images the presenter still holds live +/// here until their release tokens return, then the pool dies. +struct RetiredPool { + generation: u64, + pool: PicturePool, +} + +/// Everything tied to ONE session generation. A stream renegotiation (extent, +/// DPB depth, profile) retires it and builds fresh. struct SessionState { session: VideoSession, slots: SlotMap, - pool: ImagePool, + /// Distinct mode's reference-only DPB backing; `None` in coincide mode (the + /// picture pool backs the DPB there). + dpb: Option, + pool: PicturePool, ring: BitstreamRing, ops: OpRing, /// Last-known Std reference info per DPB slot — `vkCmdBeginVideoCodingKHR` @@ -372,40 +436,21 @@ struct SessionState { /// slices do not reference; refreshed from each plan's setup/ref entries so /// marking transitions (e.g. MMCO long-term promotion) propagate. slot_refs: Vec>, - /// Distinct-mode output ring cursor (unused in coincide mode). - out_cursor: usize, - /// Live-frame counts per OUTPUT slot: every [`DecodedVkFrame`] built over the - /// slot (pending, ready, or delivered-and-unreleased) counts one; the slot is - /// not reusable while nonzero. The coincide twin of this gate additionally - /// pins the [`SlotMap`] so `plan_to_vk`'s setup assignment skips the slot. - live_frames: Vec, -} - -impl SessionState { - /// Count a new frame over `out_slot` (and pin its DPB slot in coincide mode, - /// where output slot == DPB slot). - fn note_frame_live(&mut self, out_slot: usize) { - self.live_frames[out_slot] += 1; - if self.pool.coincide { - // Output slots mirror DPB slots one-to-one in coincide mode; the - // envelope-gated capacity (<= 17) keeps the index within u8. - self.slots.pin(out_slot as u8); - } - } - - /// Un-count a frame over `out_slot` (release or internal drop). - fn note_frame_dead(&mut self, out_slot: usize) { - match self.live_frames[out_slot].checked_sub(1) { - Some(remaining) => self.live_frames[out_slot] = remaining, - None => { - debug!(out_slot, "frame released more often than counted"); - return; - } - } - if self.pool.coincide && !self.slots.unpin(out_slot as u8) { - debug!(out_slot, "coincide slot unpinned without a pin"); - } - } + /// Coincide mode: which pool image each DPB slot currently binds (rebound at + /// every activation — the decoupling that keeps delivered images safe). + slot_image: Vec>, + /// Per command-buffer completion tokens (reuse gate). + cmd_marks: Vec>, + /// Per query-slot submission ordinals (staleness validation). + query_marks: Vec, + /// Submissions recorded on this session (cmd/query indexing). + submitted: u64, + /// The newest submission's completion token (session drain). + last_submit: Option<(vk::Semaphore, u64)>, + /// The STREAM's coded extent (renegotiation comparison). + coded_extent: vk::Extent2D, + /// The granularity-aligned allocation extent (picture resources + frames). + image_extent: vk::Extent2D, } /// The native Vulkan Video H.264 decoder. @@ -417,18 +462,18 @@ pub struct VkH264Decoder { caps: Option<(hh::StdVideoH264ProfileIdc, DecodeCaps)>, state: Option, /// Decoded pictures awaiting their planner output verdict, keyed by [`PicId`]. - pending_frames: BTreeMap, + pending: BTreeMap, /// Display-ready frames not yet handed out (under the zero-reorder punktfunk /// envelope at most one per AU; deeper only around discontinuities/flushes). ready: VecDeque, - /// Session generation: bumped on every rebuild, stamped into frames so stale - /// ones are detectable ([`DecodedVkFrame::generation`]). + /// Retired generations' pools with consumer-held images (die on their last + /// release token). + graveyard: Vec, + /// The most recent plan's warnings ([`Self::take_warnings`]). + last_warnings: Vec, + /// Session generation: bumped on every rebuild, stamped into frames. generation: u64, device_lost: bool, - /// The last `decode` call's plan warnings (concealment signals), held for - /// [`Self::take_warnings`] — the integration layer's recovery hook. Cleared at - /// every `decode` entry so a warning is never attributed to the wrong AU. - last_warnings: Vec, } impl VkH264Decoder { @@ -452,11 +497,12 @@ impl VkH264Decoder { planner: H264Planner::new(), caps: None, state: None, - pending_frames: BTreeMap::new(), + pending: BTreeMap::new(), ready: VecDeque::new(), + graveyard: Vec::new(), + last_warnings: Vec::new(), generation: 0, device_lost: false, - last_warnings: Vec::new(), }) } @@ -466,7 +512,6 @@ impl VkH264Decoder { /// Never panics. `VkDecodeError::DeviceLost` latches: every later call fails /// fast until the owner rebuilds the decoder on fresh handles. pub fn decode(&mut self, au: &[u8]) -> Result, VkDecodeError> { - self.last_warnings.clear(); if self.device_lost { return Err(VkDecodeError::DeviceLost); } @@ -477,28 +522,11 @@ impl VkH264Decoder { result } - /// The plan warnings (concealment signals) of the most recent [`Self::decode`] - /// call, taken. Non-empty means the AU was planned around missing/damaged - /// references — the picture decodes but its content is concealed, and the caller - /// should request a re-anchor (the recovery wiring the module doc reserves for - /// the integration layer). - pub fn take_warnings(&mut self) -> Vec { - std::mem::take(&mut self.last_warnings) - } - - /// The current session generation ([`DecodedVkFrame::generation`]'s counterpart): - /// lets a caller holding delivered frames tell a STALE frame (its session was - /// rebuilt — every decoder entry point would report it so) apart from a live one, - /// without tripping the conservative `Failed` a stale status poll returns. - pub fn generation(&self) -> u64 { - self.generation - } - fn decode_inner(&mut self, au: &[u8]) -> Result, VkDecodeError> { let plan = self.planner.plan_au(au)?; for warning in &plan.warnings { - // The recovery verdict is the integration layer's ([`Self::take_warnings`]); - // never silent here though. + // The recovery verdict is the integration layer's + // ([`Self::take_warnings`]); never silent here though. trace!(?warning, "plan warning"); } self.last_warnings = plan.warnings.clone(); @@ -541,18 +569,6 @@ impl VkH264Decoder { ); self.rebuild_state(&plan)?; } - Err(PlanToVkError::Slot(SlotError::AllPinned { free })) => { - // Every free slot backs an unreleased frame. A GPU drain - // first (bounded — it costs nothing on this error path and - // rules out any in-flight hold), but consumer pins clear - // ONLY via release_frame, so the verdict stands: explicit - // backpressure. The AU was planned but not decoded; the - // caller releases frames and requests recovery. Unreachable - // under the one-in/one-out release loop. - debug!(free, "no unpinned DPB slot — release_frame owed"); - self.drain_gpu()?; - return Err(VkDecodeError::NoFreeSlot); - } Err(e) => return Err(VkDecodeError::Convert(e)), } } @@ -570,43 +586,69 @@ impl VkH264Decoder { ))); } - // Output slot: the setup slot itself (coincide — output IS the DPB - // picture, and the pin layer above already guaranteed it backs no live - // frame) or the next FREE ring slot (distinct — slots with live frames - // are skipped; all-busy is the same backpressure verdict as AllPinned). - let out_slot = if state.pool.coincide { - usize::from(vk_plan.setup_slot) - } else { - let ring_len = state.pool.outputs.len(); - let mut chosen = None; - for _ in 0..ring_len { - let candidate = state.out_cursor; - state.out_cursor = (state.out_cursor + 1) % ring_len; - if state.live_frames[candidate] == 0 { - chosen = Some(candidate); - break; + // Coincide binding sync: slots the planner released no longer bind their + // images (the pictures may still be pending/held — untouched), and the + // setup slot's PREVIOUS binding is cleared before it binds fresh. + let setup = usize::from(vk_plan.setup_slot); + if state.dpb.is_none() { + let mut held = vec![false; state.slot_image.len()]; + for (slot, _id) in state.slots.held() { + held[usize::from(slot)] = true; + } + for (slot, binding) in state.slot_image.iter_mut().enumerate() { + if let Some(picture) = *binding { + if !held[slot] || slot == setup { + state.pool.pictures[picture].bound = false; + *binding = None; + } } } - match chosen { - Some(slot) => slot, - None => { - debug!("every output-ring slot backs an unreleased frame"); - self.drain_gpu()?; - return Err(VkDecodeError::NoFreeSlot); - } - } - }; - let state = self.state.as_mut().expect("ensured above"); - // The op slot's command buffer + query must not still be in flight, and - // (distinct mode) neither may the output image. - let prev = ( - state.pool.outputs[out_slot].semaphore, - state.pool.outputs[out_slot].value, - ); - // SAFETY: live device; the semaphore is the pool's own. - unsafe { wait_timeline(self.dev.ash(), prev.0, prev.1, "output slot reuse")? }; + } - // Upload the AU (recycles/grows against the same timeline facts). + // The decode target: a FREE pool image (never one a consumer holds — the + // whole point of the pool model). Exhaustion means the consumer owes + // more than HOLD_HEADROOM releases; no wait can free an image here. + let Some(dst) = state.pool.free_index() else { + debug!( + held = state.pool.held_total(), + "picture pool exhausted — release_frame owed" + ); + return Err(VkDecodeError::NoFreeSlot); + }; + + // Cross-queue waits (the AVVkFrame contract): the dst image's last known + // timeline value (covers a presenter write-back after release), plus — + // coincide mode — every referenced image's value, so reference reads + // order after any presenter layout restore already reported back. + let mut waits: Vec<(vk::Semaphore, u64)> = Vec::new(); + { + let dst_pic = &state.pool.pictures[dst]; + if dst_pic.value > 0 { + waits.push((dst_pic.semaphore, dst_pic.value)); + } + } + if state.dpb.is_none() { + for r in &vk_plan.refs { + if let Some(picture) = state.slot_image[usize::from(r.slot)] { + let pic = &state.pool.pictures[picture]; + if pic.value > 0 && !waits.iter().any(|(sem, _)| *sem == pic.semaphore) { + waits.push((pic.semaphore, pic.value)); + } + } + } + } + let signal_value = state.pool.pictures[dst].value + 1; + + // Command buffer + query slot for this submission. + let submission = state.submitted; + let cmd_index = (submission % state.ops.cmds.len() as u64) as usize; + if let Some((sem, value)) = state.cmd_marks[cmd_index] { + // SAFETY: live device; the token is a pool image's semaphore. + unsafe { wait_timeline(self.dev.ash(), sem, value, "command buffer reuse")? }; + } + let query_index = (submission % u64::from(state.ops.query_count)) as u32; + + // Upload the AU (recycles/grows against submission-completion tokens). let device = self.dev.ash().clone(); let mut poll = |token: &(vk::Semaphore, u64)| -> Result { // SAFETY: live device; the token's semaphore is a pool semaphore. @@ -619,133 +661,243 @@ impl VkH264Decoder { // SAFETY: as above. unsafe { wait_timeline(&device2, token.0, token.1, "bitstream slot drain") } }; - // SAFETY: live device; the pending tokens cover their slots' GPU reads by - // construction (every submit signals its output slot's semaphore and marks - // its bitstream slot with that pair). - let upload = unsafe { state.ring.upload(&self.dev, au, &mut poll, &mut wait)? }; + // The bitstream buffer carries the SLICE NALUs only, concatenated — a + // real AU opens with AUD/SEI (and, at IDRs, SPS/PPS) NALUs, and feeding + // those to the VCN firmware inside the decode range HANGS it (the .25 + // `vcn_unified_0 ring timeout`; FFmpeg feeds slices-only for the same + // reason). Slice offsets are rebased into the packed buffer. + let segments: Vec> = + plan.slices.iter().map(|s| s.data.clone()).collect(); + let mut slice_offsets = Vec::with_capacity(segments.len()); + let mut cursor = 0u32; + for segment in &segments { + slice_offsets.push(cursor); + cursor += segment.len() as u32; + } + // SAFETY: live device; the segments are the plan's own in-bounds slice + // ranges; every pending token is the completion signal of the submission + // that consumed the slot. + let upload = unsafe { + state + .ring + .upload(&self.dev, au, &segments, &mut poll, &mut wait)? + }; - // Record + submit, signalling the output slot's next timeline value. - let signal_value = state.pool.outputs[out_slot].value + 1; + // Record + submit, signalling the dst image's next timeline value. // SAFETY: live device; every handle recorded below belongs to this - // session generation, and the AU sits uploaded in the ring slot. + // session generation, and the packed slices sit uploaded in the ring slot. unsafe { record_and_submit( &self.dev, &*self.lock, state, &vk_plan, + &slice_offsets, &upload, - out_slot, + dst, + cmd_index, + query_index, + &waits, signal_value, )?; } - state.pool.outputs[out_slot].value = signal_value; - state.ring.pending.set_pending( - upload.slot, - (state.pool.outputs[out_slot].semaphore, signal_value), - ); + + // Post-submit bookkeeping. + let dst_sem = state.pool.pictures[dst].semaphore; + state.pool.pictures[dst].value = signal_value; + state.pool.pictures[dst].pending = true; + if state.dpb.is_none() { + state.pool.pictures[dst].bound = true; + state.slot_image[setup] = Some(dst); + } + state.cmd_marks[cmd_index] = Some((dst_sem, signal_value)); + state.query_marks[query_index as usize] = submission; + state.submitted += 1; + state.last_submit = Some((dst_sem, signal_value)); + state + .ring + .pending + .set_pending(upload.slot, (dst_sem, signal_value)); // Refresh the per-slot reference cache from this AU's facts. - state.slot_refs[usize::from(vk_plan.setup_slot)] = Some(vk_plan.setup_ref); + state.slot_refs[setup] = Some(vk_plan.setup_ref); for r in &vk_plan.refs { state.slot_refs[usize::from(r.slot)] = Some(r.std); } - // Frame bookkeeping: the decoded picture waits for its output verdict, - // and counts as LIVE over its slot from this moment (two-phase release: - // the slot is reusable only after the DPB removed the picture AND - // release_frame ran / the frame was dropped internally). - let out = &state.pool.outputs[out_slot]; - let frame = DecodedVkFrame { - image: out.image, - view: out.view, - plane_views: out.plane_views, - layer: out.layer, - layout: if state.pool.coincide { - vk::ImageLayout::VIDEO_DECODE_DPB_KHR - } else { - vk::ImageLayout::VIDEO_DECODE_DST_KHR + self.pending.insert( + vk_plan.setup_id, + PendingPic { + image: dst, + submission, + query_slot: query_index, + timeline_value: signal_value, + crop: plan.picture.display_crop, + poc: plan.picture.pic_order_cnt, + is_idr: plan.picture.is_idr, }, - coded_width: plan.picture.coded_width, - coded_height: plan.picture.coded_height, - crop: plan.picture.display_crop, - semaphore: out.semaphore, - value: signal_value, - poc: plan.picture.pic_order_cnt, - is_idr: plan.picture.is_idr, - query_slot: out_slot as u32, - generation: self.generation, - }; - state.note_frame_live(out_slot); - self.pending_frames.insert(vk_plan.setup_id, frame); + ); - // The plan's DPB verdicts over the pending map: outputs become ready, - // removed-but-never-output ids (no_output_of_prior_pics_flag discards) - // are DROPPED — releasing their slots, not leaking their frames. - let (ready, dropped) = settle_dpb(&mut self.pending_frames, &plan.dpb); - for frame in ready { + // The plan's DPB verdicts over the pending map: outputs become ready + // frames (their images move pending → held until released); + // removed-but-never-output pictures free their images. + let (ready, dropped) = settle_dpb(&mut self.pending, &plan.dpb); + let state = self.state.as_mut().expect("ensured above"); + for entry in ready { + let frame = build_frame(state, &entry, self.generation); self.ready.push_back(frame); } - for frame in dropped { + for entry in dropped { debug!( - poc = frame.poc, - slot = frame.query_slot, - "picture removed without output — dropping its frame" + poc = entry.poc, + "picture removed without output — freeing its image" ); - state.note_frame_dead(frame.query_slot as usize); + state.pool.pictures[entry.image].pending = false; } Ok(self.ready.pop_front()) } - /// Hand a delivered frame back: its slot becomes reusable (once the planner's - /// DPB has also removed the picture). Every frame `decode`/`take_ready` - /// returns MUST come back through here exactly once — until then its image is - /// protected from every decode target and the pipeline eventually reports - /// [`VkDecodeError::NoFreeSlot`] instead of overwriting it. - pub fn release_frame(&mut self, frame: &DecodedVkFrame) -> Result<(), VkDecodeError> { - if frame.generation != self.generation { - // The pools this frame indexed are gone; there is nothing to release. - return Err(VkDecodeError::StaleFrame { - frame_generation: frame.generation, - current_generation: self.generation, - }); - } - let Some(state) = &mut self.state else { - return Err(VkDecodeError::StaleFrame { - frame_generation: frame.generation, - current_generation: self.generation, - }); + /// Hand a delivered frame back. `presenter_signaled` reports whether the + /// consumer SAMPLED the image (and therefore enqueued the `value + 1` + /// timeline signal per the [`DecodedVkFrame`] contract) — the decoder then + /// waits that write-back before the image's next use. Every frame + /// `decode`/`take_ready` returns must come back exactly once, including + /// stale-generation frames (their retired pool dies on its last token). + pub fn release_frame( + &mut self, + frame: &DecodedVkFrame, + presenter_signaled: bool, + ) -> Result<(), VkDecodeError> { + let pool = if frame.generation == self.generation { + match &mut self.state { + Some(state) => &mut state.pool, + None => { + return Err(VkDecodeError::StaleFrame { + frame_generation: frame.generation, + current_generation: self.generation, + }) + } + } + } else { + match self + .graveyard + .iter_mut() + .find(|r| r.generation == frame.generation) + { + Some(retired) => &mut retired.pool, + None => { + return Err(VkDecodeError::StaleFrame { + frame_generation: frame.generation, + current_generation: self.generation, + }) + } + } }; - let slot = frame.query_slot as usize; - if slot >= state.live_frames.len() { + let index = frame.picture as usize; + if index >= pool.pictures.len() { return Err(VkDecodeError::StaleFrame { frame_generation: frame.generation, current_generation: self.generation, }); } - state.note_frame_dead(slot); + let picture = &mut pool.pictures[index]; + match picture.held.checked_sub(1) { + Some(remaining) => picture.held = remaining, + None => { + debug!(index, "frame released more often than delivered"); + return Ok(()); + } + } + if presenter_signaled { + picture.value = picture.value.max(frame.value + 1); + } + // A retired pool dies on its last token (presenter fence-waited before + // the token per the release contract; decode work drained at retirement). + if frame.generation != self.generation { + self.graveyard + .retain(|r| r.generation != frame.generation || r.pool.held_total() > 0); + } Ok(()) } /// A display-ready frame beyond the one `decode` returned, if any (only /// non-empty around discontinuities/flushes — the punktfunk envelope is - /// zero-reorder). + /// zero-reorder). Drain after every decode; frames left here still occupy + /// pool images. pub fn take_ready(&mut self) -> Option { self.ready.pop_front() } + /// The warnings of the most recent successfully planned AU (concealment + /// signals — the integration layer's want_keyframe hook). Cleared by the + /// next `decode`. + pub fn take_warnings(&mut self) -> Vec { + std::mem::take(&mut self.last_warnings) + } + + /// The current session generation ([`DecodedVkFrame::generation`] of newly + /// delivered frames). + pub fn generation(&self) -> u64 { + self.generation + } + + /// One-line state snapshot for failure paths and field logs (not a stable + /// format). + pub fn debug_snapshot(&self) -> String { + match &self.state { + None => format!("gen={} ", self.generation), + Some(state) => { + let occupancy: Vec = state + .pool + .pictures + .iter() + .enumerate() + .map(|(i, p)| { + format!( + "{i}:{}{}h{}", + if p.bound { "B" } else { "-" }, + if p.pending { "P" } else { "-" }, + p.held + ) + }) + .collect(); + format!( + "gen={} mode={} slots_held={}/{} pool=[{}] pending={} ready={} graveyard={}", + self.generation, + if state.dpb.is_none() { + "coincide" + } else { + "distinct" + }, + state.slots.active(), + state.slots.capacity(), + occupancy.join(" "), + self.pending.len(), + self.ready.len(), + self.graveyard.len(), + ) + } + } + } + /// Read `frame`'s decode status WITHOUT waiting. /// /// [`DecodeStatus::Failed`] covers driver-reported errors AND a query slot - /// recycled before it was read (the status is then unprovable — same - /// conservative verdict), so poll before the pipeline wraps a ring. + /// re-armed before it was read (the status is then unprovable — same + /// conservative verdict). + /// + /// On drivers whose decode family lacks `queryResultStatusSupport` (RADV) + /// there is no per-op verdict to read: `Ok` then means "the decode op + /// COMPLETED on the timeline" — the same information FFmpeg has on every + /// driver, no worse; the Ally-X-class detection exists exactly where the + /// driver can give it (NVIDIA, AMD's Windows driver). pub fn poll_status(&mut self, frame: &DecodedVkFrame) -> DecodeStatus { self.read_status(frame, false) } /// [`Self::poll_status`], but WAITs for the op to complete first — the only - /// place a status read blocks (the GPU smoke test's assertion path; WP-C's - /// steady state polls). + /// place a status read blocks (the GPU smoke test's assertion path; the + /// integration layer's steady state polls). pub fn wait_status(&mut self, frame: &DecodedVkFrame) -> DecodeStatus { self.read_status(frame, true) } @@ -763,11 +915,38 @@ impl VkH264Decoder { let Some(state) = &self.state else { return DecodeStatus::Failed; }; + let Some(query_pool) = state.ops.query_pool else { + // No queries on this driver: the verdict degrades to timeline + // completion (poll_status docs). + if block { + // SAFETY: live device; pool-owned semaphore. + return match unsafe { + wait_timeline(self.dev.ash(), frame.semaphore, frame.value, "status wait") + } { + Ok(()) => DecodeStatus::Ok, + Err(VkDecodeError::DeviceLost) => { + self.device_lost = true; + DecodeStatus::Failed + } + Err(_) => DecodeStatus::Failed, + }; + } + // SAFETY: live device; pool-owned semaphore. + return match unsafe { self.dev.ash().get_semaphore_counter_value(frame.semaphore) } { + Ok(current) if current >= frame.value => DecodeStatus::Ok, + Ok(_) => DecodeStatus::Pending, + Err(vk::Result::ERROR_DEVICE_LOST) => { + self.device_lost = true; + DecodeStatus::Failed + } + Err(_) => DecodeStatus::Failed, + }; + }; let slot = frame.query_slot as usize; - if slot >= state.pool.outputs.len() || state.pool.outputs[slot].value != frame.value { + if slot >= state.query_marks.len() || state.query_marks[slot] != frame.submission { trace!( slot, - "status query slot recycled before it was read — unprovable, reported Failed" + "status query slot re-armed before it was read — unprovable, reported Failed" ); return DecodeStatus::Failed; } @@ -779,14 +958,11 @@ impl VkH264Decoder { let mut status = [0i32; 1]; // SAFETY: live device; the query pool is this session generation's own and // `frame.query_slot` indexes within its count (checked above against the - // output ring it is sized to). + // marks array it is sized to). let result = unsafe { - self.dev.ash().get_query_pool_results( - state.ops.query_pool, - frame.query_slot, - &mut status, - flags, - ) + self.dev + .ash() + .get_query_pool_results(query_pool, frame.query_slot, &mut status, flags) }; match result { // VkQueryResultStatusKHR: >0 complete, 0 not ready, <0 error. @@ -806,28 +982,29 @@ impl VkH264Decoder { } /// Drain the planner (teardown / stream discontinuity): every buffered - /// picture becomes display-ready via [`Self::take_ready`] (those frames stay - /// live until released), all DPB slots free, and any picture removed without - /// ever reaching output has its frame dropped and its slot un-counted. + /// picture becomes display-ready via [`Self::take_ready`] (zero-copy — the + /// images already hold the content), all DPB slots free, and any picture + /// removed without ever reaching output frees its image. pub fn flush(&mut self) { let update = self.planner.flush(); - let (ready, dropped) = settle_dpb(&mut self.pending_frames, &update); + let (ready, dropped) = settle_dpb(&mut self.pending, &update); if let Some(state) = &mut self.state { state.slots.apply(&update); - for frame in &dropped { - state.note_frame_dead(frame.query_slot as usize); + for entry in ready { + let frame = build_frame(state, &entry, self.generation); + self.ready.push_back(frame); } - // Defensive: a pending frame neither output nor removed should not - // exist (flush drains everything); un-count any leftover. - for (_, frame) in std::mem::take(&mut self.pending_frames) { - debug!(poc = frame.poc, "pending frame survived a flush — dropped"); - state.note_frame_dead(frame.query_slot as usize); + for entry in dropped { + state.pool.pictures[entry.image].pending = false; + } + // Defensive: a pending picture neither output nor removed should not + // exist after a flush; free any leftover. + for (_, entry) in std::mem::take(&mut self.pending) { + debug!(poc = entry.poc, "pending picture survived a flush — freed"); + state.pool.pictures[entry.image].pending = false; } } else { - self.pending_frames.clear(); - } - for frame in ready { - self.ready.push_back(frame); + self.pending.clear(); } } @@ -859,10 +1036,8 @@ impl VkH264Decoder { height: plan.picture.coded_height, }; match &self.state { - // Compared against the STREAM's coded extent (the pool tracks it - // beside its granularity-rounded image extent). Some(state) - if state.pool.coded_extent == coded + if state.coded_extent == coded && state.session.config.std_profile_idc == std_profile => { Ok(()) @@ -871,34 +1046,52 @@ impl VkH264Decoder { } } - /// Tear down the current session generation (draining its GPU work) and build - /// a fresh one shaped by `plan`, bumping [`Self::generation`] so frames of the - /// old one are detectably stale. + /// Tear down the current session generation (draining its decode work, + /// 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. fn rebuild_state(&mut self, plan: &AuPlan) -> Result<(), VkDecodeError> { self.drain_gpu()?; - if self.state.is_some() { + if let Some(state) = self.state.take() { debug!("rebuilding decode session (stream renegotiation)"); + // Move the fields out (SessionState has no Drop of its own): the + // session/dpb/ring/ops die here — decode work was just drained and + // the presenter never references them. The PICTURE POOL may outlive: + // undelivered frames drop (their holds cleared), pending pictures + // free, and if the consumer still holds delivered images the pool + // retires to the graveyard until its last release token. + let SessionState { mut pool, .. } = state; + for frame in self.ready.drain(..) { + let picture = &mut pool.pictures[frame.picture as usize]; + picture.held = picture.held.saturating_sub(1); + } + for (_, entry) in std::mem::take(&mut self.pending) { + pool.pictures[entry.image].pending = false; + } + for picture in &mut pool.pictures { + picture.bound = false; + } + let held = pool.held_total(); + if held > 0 { + debug!( + held, + generation = self.generation, + "consumer still holds images of the retired generation — graveyarding" + ); + self.graveyard.push(RetiredPool { + generation: self.generation, + pool, + }); + } } - // Frames referencing the old pools die with them (their generation stamp - // makes any copy the consumer still holds report stale, never read). - if !self.pending_frames.is_empty() || !self.ready.is_empty() { - debug!( - pending = self.pending_frames.len(), - ready = self.ready.len(), - "dropping undelivered frames across a session rebuild" - ); - self.pending_frames.clear(); - self.ready.clear(); - } - self.state = None; self.generation += 1; let (std_profile, caps) = self.caps.as_ref().expect("ensure_state queried caps"); let std_profile = *std_profile; - let dpb_slots = plan.picture.max_dpb_frames as u32 + 1; - if dpb_slots > caps.max_dpb_slots { + let required_slots = plan.picture.max_dpb_frames as u32 + 1; + if required_slots > caps.max_dpb_slots { return Err(VkDecodeError::Unsupported(format!( - "stream needs {dpb_slots} DPB slots, device caps at {}", + "stream needs {required_slots} DPB slots, device caps at {}", caps.max_dpb_slots ))); } @@ -929,17 +1122,25 @@ impl VkH264Decoder { let config = SessionConfig { max_coded_extent: image_extent, - max_dpb_slots: dpb_slots, - max_active_references: (dpb_slots - 1).min(caps.max_active_references), + max_dpb_slots: required_slots, + max_active_references: (required_slots - 1).min(caps.max_active_references), std_profile_idc: std_profile, }; - let pool_plan = plan_pools(caps, dpb_slots, OUTPUT_RING); + let pool_plan = plan_pools(caps, required_slots); // SAFETY: live device per the constructor contract, for every create in // this block; each created half is owned by a Drop type the moment it // exists, so a mid-build failure unwinds cleanly. let state = unsafe { let session = VideoSession::create(&self.dev, caps, config)?; - let pool = ImagePool::create(&self.dev, caps, &pool_plan, coded, std_profile) + let dpb = if caps.coincide { + None + } else { + Some( + DpbPool::create(&self.dev, caps, &pool_plan, image_extent, std_profile) + .map_err(VkDecodeError::from)?, + ) + }; + let pool = PicturePool::create(&self.dev, caps, &pool_plan, image_extent, std_profile) .map_err(VkDecodeError::from)?; let ring = BitstreamRing::create( &self.dev, @@ -952,31 +1153,37 @@ impl VkH264Decoder { std_profile, ) .map_err(VkDecodeError::from)?; - let ops = OpRing::create(&self.dev, std_profile, pool_plan.output_slots) + let ops = OpRing::create(&self.dev, std_profile, pool_plan.picture_count, RING_SLOTS) .map_err(VkDecodeError::from)?; SessionState { session, slots: SlotMap::new(plan.picture.max_dpb_frames), - slot_refs: vec![None; dpb_slots as usize], - live_frames: vec![0; pool_plan.output_slots as usize], + slot_refs: vec![None; required_slots as usize], + slot_image: vec![None; required_slots as usize], + cmd_marks: vec![None; RING_SLOTS as usize], + query_marks: vec![u64::MAX; pool_plan.picture_count as usize], + submitted: 0, + last_submit: None, + coded_extent: coded, + image_extent, + dpb, pool, ring, ops, - out_cursor: 0, } }; self.state = Some(state); Ok(()) } - /// Wait out every in-flight decode of the current session generation. + /// Wait out every in-flight decode submission of the current session. fn drain_gpu(&mut self) -> Result<(), VkDecodeError> { let Some(state) = &self.state else { return Ok(()); }; - for out in &state.pool.outputs { - // SAFETY: live device; pool-owned semaphore. - unsafe { wait_timeline(self.dev.ash(), out.semaphore, out.value, "session drain")? }; + if let Some((sem, value)) = state.last_submit { + // SAFETY: live device; the token is a pool image's semaphore. + unsafe { wait_timeline(self.dev.ash(), sem, value, "session drain")? }; } Ok(()) } @@ -984,13 +1191,22 @@ impl VkH264Decoder { impl Drop for VkH264Decoder { fn drop(&mut self) { - // Best-effort drain so the pools' Drop impls never destroy in-flight - // objects; a wedged driver falls through after the bounded timeout (the - // destroys then race the GPU, but the alternative is hanging teardown - // forever — same trade the encoder's fence budget makes). + // Best-effort decode drain so the pools' Drop impls never destroy + // in-flight decode work; a wedged driver falls through after the bounded + // timeout. Presenter-side sampling of graveyarded/held images is the + // CALLER's teardown contract: the integration layer waits (bounded) for + // every release token BEFORE dropping this decoder, so remaining + // graveyard pools here are either token-drained or a warned forfeit. if let Err(e) = self.drain_gpu() { debug!(error = %e, "drain on drop failed; tearing down anyway"); } + if !self.graveyard.is_empty() { + debug!( + pools = self.graveyard.len(), + "graveyard not fully token-drained at decoder drop — destroying anyway \ + (upstream teardown forfeited its bounded wait)" + ); + } } } @@ -1005,19 +1221,49 @@ fn std_profile_for(plan: &AuPlan) -> Result DecodedVkFrame { + let picture = &mut state.pool.pictures[entry.image]; + picture.pending = false; + picture.held += 1; + DecodedVkFrame { + image: picture.image, + view: picture.view, + plane_views: picture.plane_views, + layer: 0, + layout: if state.dpb.is_none() { + vk::ImageLayout::VIDEO_DECODE_DPB_KHR + } else { + vk::ImageLayout::VIDEO_DECODE_DST_KHR + }, + coded_width: state.image_extent.width, + coded_height: state.image_extent.height, + crop: entry.crop, + semaphore: picture.semaphore, + value: entry.timeline_value, + poc: entry.poc, + is_idr: entry.is_idr, + query_slot: entry.query_slot, + submission: entry.submission, + picture: entry.image as u32, + generation, + } +} + +/// Split one [`DpbUpdate`]'s verdicts over the pending map: `outputs` (in bump +/// order) become deliverable; `removed` ids that never reached output — an IDR's /// `no_output_of_prior_pics_flag` discard, or a flush racing a drop — are -/// returned separately so the caller releases their slots instead of leaking -/// them in the map forever. Pure and generic for testability. +/// returned separately so their images are freed instead of leaking. Pure and +/// generic for testability. fn settle_dpb(pending: &mut BTreeMap, dpb: &DpbUpdate) -> (Vec, Vec) { let mut ready = Vec::new(); for id in &dpb.outputs { match pending.remove(id) { - Some(frame) => ready.push(frame), + Some(entry) => ready.push(entry), // Ids planned before this decoder existed (post-recovery), or // dropped across a rebuild: display-order gaps, not errors. - None => trace!(id, "output id without a pending frame"), + None => trace!(id, "output id without a pending picture"), } } let dropped = dpb @@ -1055,27 +1301,44 @@ unsafe fn wait_timeline( } } -/// Record one decode op into the out-slot's command buffer and submit it under -/// the queue lock with the timeline signal. +/// The picture resource view bound for DPB `slot`: the bound pool image +/// (coincide) or the DPB array layer (distinct). `None` when a coincide slot has +/// no binding (unreachable in practice — every held slot was activated). +fn slot_view(state: &SessionState, slot: u8) -> Option { + match &state.dpb { + Some(dpb) => Some(dpb.dpb_view(slot)), + None => state.slot_image[usize::from(slot)].map(|p| state.pool.pictures[p].view), + } +} + +/// Record one decode op into the chosen command buffer and submit it under the +/// queue lock: image waits per the pool contract, the dst image's timeline +/// signal at `signal_value`. /// /// # Safety /// /// Live device; `state` is the current session generation with `vk_plan` derived -/// against its `SlotMap` and the AU resident in `upload`'s ring slot; the out -/// slot's previous use has completed (caller waited its timeline value). +/// against its `SlotMap`, `dst` a free pool image, the AU resident in `upload`'s +/// ring slot, and the command buffer's previous submission completed (caller +/// waited its mark). #[allow(clippy::too_many_arguments)] unsafe fn record_and_submit( dev: &DecodeDevice, lock: &dyn QueueLock, state: &mut SessionState, vk_plan: &DecodePlanVk, + slice_offsets: &[u32], upload: &UploadedAu, - out_slot: usize, + dst: usize, + cmd_index: usize, + query_index: u32, + waits: &[(vk::Semaphore, u64)], signal_value: u64, ) -> Result<(), VkDecodeError> { let device = dev.ash(); - let cmd = state.ops.cmds[out_slot]; - let coded_extent = state.pool.coded_extent; + let cmd = state.ops.cmds[cmd_index]; + let coded_extent = state.coded_extent; + let coincide = state.dpb.is_none(); let begin_info = vk::CommandBufferBeginInfo::default().flags(vk::CommandBufferUsageFlags::ONE_TIME_SUBMIT); @@ -1096,8 +1359,8 @@ unsafe fn record_and_submit( .dst_access_mask( vk::AccessFlags2::VIDEO_DECODE_READ_KHR | vk::AccessFlags2::VIDEO_DECODE_WRITE_KHR, )]; - // The setup target layer is fully overwritten: discard via UNDEFINED, with an - // execution+memory dependency on earlier ops that touched the layer. + // Decode targets are fully overwritten: discard via UNDEFINED with an + // execution+memory dependency on earlier ops that touched them. let decode_layer_barrier = |image: vk::Image, layer: u32, new_layout: vk::ImageLayout| { vk::ImageMemoryBarrier2::default() .src_stage_mask(vk::PipelineStageFlags2::VIDEO_DECODE_KHR) @@ -1121,17 +1384,26 @@ unsafe fn record_and_submit( layer_count: 1, }) }; - let (setup_image, setup_layer) = state.pool.dpb_target(vk_plan.setup_slot); - let mut image_barriers = vec![decode_layer_barrier( - setup_image, - setup_layer, - vk::ImageLayout::VIDEO_DECODE_DPB_KHR, - )]; - if !state.pool.coincide { - let out = &state.pool.outputs[out_slot]; + let dst_image = state.pool.pictures[dst].image; + let mut image_barriers = Vec::new(); + if coincide { + // The dst pool image IS the setup DPB picture. image_barriers.push(decode_layer_barrier( - out.image, - out.layer, + dst_image, + 0, + vk::ImageLayout::VIDEO_DECODE_DPB_KHR, + )); + } else { + let dpb = state.dpb.as_ref().expect("distinct mode"); + let (setup_image, setup_layer) = dpb.dpb_target(vk_plan.setup_slot); + image_barriers.push(decode_layer_barrier( + setup_image, + setup_layer, + vk::ImageLayout::VIDEO_DECODE_DPB_KHR, + )); + image_barriers.push(decode_layer_barrier( + dst_image, + 0, vk::ImageLayout::VIDEO_DECODE_DST_KHR, )); } @@ -1143,44 +1415,66 @@ unsafe fn record_and_submit( unsafe { device.cmd_pipeline_barrier2(cmd, &dependency) }; // This op's status query slot, reset before the coding scope (encoder idiom). - // SAFETY: recording; the pool is sized to the output ring (fn contract). - unsafe { device.cmd_reset_query_pool(cmd, state.ops.query_pool, out_slot as u32, 1) }; + // None on drivers without queryResultStatusSupport (RADV — recording a query + // there hangs the VCN; OpRing docs). + if let Some(query_pool) = state.ops.query_pool { + // SAFETY: recording; `query_index` is within the pool's count (fn contract). + unsafe { device.cmd_reset_query_pool(cmd, query_pool, query_index, 1) }; + } // ---- bound-slot staging ---- // Scope list: this AU's references first, then every other still-held slot // (their resources must stay bound for their associations to persist), then // the setup slot as the ACTIVATION entry (slot index -1 binds its resource // without a current association; the decode op's setup slot then claims it). - let mut scope: Vec<(i32, u8, hh::StdVideoDecodeH264ReferenceInfo)> = Vec::new(); + let mut scope: Vec<(i32, vk::ImageView, hh::StdVideoDecodeH264ReferenceInfo)> = Vec::new(); for r in &vk_plan.refs { - scope.push((i32::from(r.slot), r.slot, r.std)); + match slot_view(state, r.slot) { + Some(view) => scope.push((i32::from(r.slot), view, r.std)), + None => trace!(slot = r.slot, "referenced slot without a bound image"), + } } for (slot, _id) in state.slots.held() { - if slot == vk_plan.setup_slot || scope.iter().any(|(_, s, _)| *s == slot) { + if slot == vk_plan.setup_slot + || scope + .iter() + .any(|&(index, _, _)| index >= 0 && index as u8 == slot) + { continue; } - match state.slot_refs[usize::from(slot)] { - Some(std) => scope.push((i32::from(slot), slot, std)), + match (state.slot_refs[usize::from(slot)], slot_view(state, slot)) { + (Some(std), Some(view)) => scope.push((i32::from(slot), view, std)), // Unreachable in practice: every held slot was a setup slot once. - None => trace!( + _ => trace!( slot, - "held slot without cached reference info — left unbound" + "held slot without reference info/binding — left unbound" ), } } - let reference_count = vk_plan.refs.len(); - scope.push((-1, vk_plan.setup_slot, vk_plan.setup_ref)); + let reference_count = vk_plan.refs.len().min(scope.len()); + // The setup/dst resource: the fresh pool image (coincide) or the DPB layer + // (distinct — the pool image is the separate decode output). + let setup_view = if coincide { + state.pool.pictures[dst].view + } else { + state + .dpb + .as_ref() + .expect("distinct mode") + .dpb_view(vk_plan.setup_slot) + }; + scope.push((-1, setup_view, vk_plan.setup_ref)); // Staged arrays: resources → std infos → codec slot infos → slot infos. Each // vector is fully built before the next borrows it, so nothing reallocates // under a stored pointer. let resources: Vec> = scope .iter() - .map(|&(_, slot, _)| { + .map(|&(_, view, _)| { vk::VideoPictureResourceInfoKHR::default() .coded_extent(coded_extent) .base_array_layer(0) - .image_view_binding(state.pool.dpb_view(slot)) + .image_view_binding(view) }) .collect(); let std_refs: Vec = @@ -1215,20 +1509,23 @@ unsafe fn record_and_submit( .picture_resource(&setup_resource) .push_next(&mut setup_dpb); - // Decode destination: the setup picture itself (coincide) or the output image. - let dst_resource = if state.pool.coincide { + // Decode destination: the setup picture itself (coincide) or the pool image + // (distinct). + let dst_resource = if coincide { setup_resource } else { vk::VideoPictureResourceInfoKHR::default() .coded_extent(coded_extent) .base_array_layer(0) - .image_view_binding(state.pool.outputs[out_slot].view) + .image_view_binding(state.pool.pictures[dst].view) }; let std_pic = vk_plan.std_pic; + // Offsets rebased into the packed slices-only buffer (NOT the plan's + // AU-absolute offsets — the AU's non-slice NALUs were never uploaded). let mut h264_pic = vk::VideoDecodeH264PictureInfoKHR::default() .std_picture_info(&std_pic) - .slice_offsets(&vk_plan.slice_offsets); + .slice_offsets(slice_offsets); let mut decode_info = vk::VideoDecodeInfoKHR::default() .src_buffer(state.ring.buffer()) .src_buffer_offset(upload.offset) @@ -1260,14 +1557,13 @@ unsafe fn record_and_submit( .flags(vk::VideoCodingControlFlagsKHR::RESET); (dev.video_queue().fp().cmd_control_video_coding_khr)(cmd, &control); } - device.cmd_begin_query( - cmd, - state.ops.query_pool, - out_slot as u32, - vk::QueryControlFlags::empty(), - ); + if let Some(query_pool) = state.ops.query_pool { + device.cmd_begin_query(cmd, query_pool, query_index, vk::QueryControlFlags::empty()); + } (dev.video_decode_queue().fp().cmd_decode_video_khr)(cmd, &decode_info); - device.cmd_end_query(cmd, state.ops.query_pool, out_slot as u32); + if let Some(query_pool) = state.ops.query_pool { + device.cmd_end_query(cmd, query_pool, query_index); + } (dev.video_queue().fp().cmd_end_video_coding_khr)( cmd, &vk::VideoEndCodingInfoKHR::default(), @@ -1283,12 +1579,22 @@ unsafe fn record_and_submit( // ---- submit, under the caller's queue lock ---- let cmd_infos = [vk::CommandBufferSubmitInfo::default().command_buffer(cmd)]; + let wait_infos: Vec> = waits + .iter() + .map(|&(semaphore, value)| { + vk::SemaphoreSubmitInfo::default() + .semaphore(semaphore) + .value(value) + .stage_mask(vk::PipelineStageFlags2::VIDEO_DECODE_KHR) + }) + .collect(); let signals = [vk::SemaphoreSubmitInfo::default() - .semaphore(state.pool.outputs[out_slot].semaphore) + .semaphore(state.pool.pictures[dst].semaphore) .value(signal_value) .stage_mask(vk::PipelineStageFlags2::ALL_COMMANDS)]; let submits = [vk::SubmitInfo2::default() .command_buffer_infos(&cmd_infos) + .wait_semaphore_infos(&wait_infos) .signal_semaphore_infos(&signals)]; let guard = QueueSubmitGuard::acquire(lock); // SAFETY: the decode queue is the device's own (DeviceHandles contract) and @@ -1318,7 +1624,7 @@ mod tests { // Picture 1 outputs (and is also removed — the normal bump); picture 2 // is removed WITHOUT ever reaching output (no_output_of_prior_pics): - // its frame must come back as dropped, not leak in the map. + // its image must be freed, not leak in the map. let update = DpbUpdate { stored: Some(3), outputs: vec![1], diff --git a/crates/pf-vkdecode/src/device.rs b/crates/pf-vkdecode/src/device.rs index f0b1acbe..4c34dc4f 100644 --- a/crates/pf-vkdecode/src/device.rs +++ b/crates/pf-vkdecode/src/device.rs @@ -161,6 +161,12 @@ pub struct DecodeDevice { decode_queue: vk::Queue, decode_qf: u32, graphics_qf: u32, + /// The decode family advertises `queryResultStatusSupport`: per-op + /// RESULT_STATUS queries are legal in its video coding scopes. FALSE on RADV + /// (2026-08, .25: recording one anyway hangs the VCN ring) — the decoder + /// must skip queries entirely there and fall back to timeline-completion + /// verdicts. + result_status_queries: bool, } impl DecodeDevice { @@ -225,6 +231,32 @@ impl DecodeDevice { // name a queue the device was created with. let decode_queue = unsafe { device.get_device_queue(handles.decode_qf, handles.decode_queue_index) }; + + // Whether the decode family supports RESULT_STATUS queries (per-family + // cap; struct field docs). + let physical_device = vk::PhysicalDevice::from_raw(handles.physical_device as u64); + // SAFETY: live physical device (caller contract); the two-call form fills + // the chained per-family status-support structs. + let family_count = + unsafe { instance.get_physical_device_queue_family_properties2_len(physical_device) }; + let result_status_queries = if (handles.decode_qf as usize) < family_count { + let mut status_props = + vec![vk::QueueFamilyQueryResultStatusPropertiesKHR::default(); family_count]; + let mut families: Vec> = status_props + .iter_mut() + .map(|s| vk::QueueFamilyProperties2::default().push_next(s)) + .collect(); + // SAFETY: as above, arrays sized to the reported count. + unsafe { + instance + .get_physical_device_queue_family_properties2(physical_device, &mut families) + }; + drop(families); + status_props[handles.decode_qf as usize].query_result_status_support != vk::FALSE + } else { + false + }; + // `entry` is only the ladder the tables above were loaded through; nothing // needs it afterwards (ash tables own their function pointers). drop(entry); @@ -232,13 +264,14 @@ impl DecodeDevice { Ok(Self { instance, device, - physical_device: vk::PhysicalDevice::from_raw(handles.physical_device as u64), + physical_device, video_queue_instance, video_queue, video_decode_queue, decode_queue, decode_qf: handles.decode_qf, graphics_qf: handles.graphics_qf, + result_status_queries, }) } @@ -246,6 +279,12 @@ impl DecodeDevice { &self.device } + /// Whether the decode family supports per-op RESULT_STATUS queries (struct + /// field docs — FALSE on RADV, where recording one hangs the VCN). + pub(crate) fn result_status_queries(&self) -> bool { + self.result_status_queries + } + pub(crate) fn physical_device(&self) -> vk::PhysicalDevice { self.physical_device } diff --git a/crates/pf-vkdecode/src/images.rs b/crates/pf-vkdecode/src/images.rs index 89491e64..cf6a20d4 100644 --- a/crates/pf-vkdecode/src/images.rs +++ b/crates/pf-vkdecode/src/images.rs @@ -1,25 +1,25 @@ -//! DPB + decode-output image pools, caps-driven for BOTH DPB arrangements: +//! Decode image pools — the FFmpeg pool model, zero-copy: //! -//! - **coincide** (`DPB_AND_OUTPUT_COINCIDE`, RADV's shape): the decode output IS -//! the DPB picture — one pool, every slot usable both as setup/reference and as -//! the frame handed to the presenter. -//! - **distinct** (NVIDIA's shape): a reference-only DPB pool plus a small ring of -//! output images the decoder writes `dst` into. +//! The PICTURE POOL is decoupled from DPB slots. Images outnumber slots by +//! [`HOLD_HEADROOM`], and a DPB slot binds an image at ACTIVATION time — a +//! re-activated slot may bind a DIFFERENT free image (spec-legal with +//! `SEPARATE_REFERENCE_IMAGES`, which the caps derivation requires for coincide +//! mode). A picture the consumer still holds is therefore NEVER a decode target: +//! its image simply stays off the free list until the release token returns. +//! This is the exact contract the presenter already speaks on the AVVkFrame path, +//! re-implemented without FFmpeg in the middle. //! -//! Within either mode the DPB is **layered** (one image, one array layer per slot — -//! mandatory when the driver lacks `SEPARATE_REFERENCE_IMAGES`) or **per-slot** -//! (one image each). [`plan_pools`] is the pure decision table; [`ImagePool`] is -//! the thin Vulkan half. +//! - **coincide** (RADV): pool images are DPB + decode output + sampled surface +//! in one (`DPB|DST|SAMPLED`, per-slot images). +//! - **distinct** (NVIDIA): a separate reference-only DPB array (layered or +//! per-slot — never delivered, so its slot↔layer mapping stays fixed) plus the +//! pool as decode outputs (`DST|SAMPLED`). //! -//! Presenter-facing surfaces (outputs) carry `MUTABLE_FORMAT` (advertised by the -//! driver — [`crate::caps::derive_caps`] refuses otherwise; nothing here aliases, -//! so no `ALIAS`) so per-plane `R8`/`R8G8` views exist for the presenter's -//! sampling path, one TIMELINE semaphore per output slot signals decode -//! completion, and the conformance-window crop rides the frame struct — the -//! 1088-row smear class dies by construction because the consumer is TOLD the -//! crop instead of guessing from the pool shape. Images are allocated at the -//! `pictureAccessGranularity`-rounded extent; the stream's coded extent rides -//! separately for per-picture resources. +//! Every pool image carries its OWN timeline semaphore (the AVVkFrame contract): +//! the decoder signals `value+1` when it writes the image; the presenter waits +//! that value, samples, restores the layout, and signals `value+1` again in the +//! same submission — the decoder's ledger learns of that write-back at +//! `release_frame` and waits it before the image's next use. use ash::vk; use ash::vk::native as hh; @@ -33,105 +33,109 @@ use crate::device::find_memory_type; use crate::device::AllocError; use crate::device::DecodeDevice; -/// Distinct-mode output ring depth: decode-ahead is one-in/one-out under the -/// punktfunk envelope, so a small ring covers pipelining plus a frame in the -/// presenter's hands. -pub const OUTPUT_RING: u32 = 4; +/// Picture-pool headroom on top of the stream's DPB needs: how many decoded +/// pictures the CONSUMER may hold (delivered, unreleased) before the decoder +/// reports backpressure. The real client pipeline holds ~4-7 frames at steady +/// state (two bounded(2) channels, the FrameStore's 1..=3 preroll, the in-flight +/// present and the retired-frame slot), so 8 gives it a frame of slack; a +/// consumer holding MORE than this earns the `NoFreeSlot` error, which then +/// means exactly what it says. +/// +/// (The 2026-08 .25 field failure taught the sizing lesson the hard way: any +/// FIXED pool ignoring the stream's DPB depth starves on a clean stream — the +/// vendored 25fps vector alone keeps `max_dpb_frames + 1 = 8` pictures resident. +/// Pool size is always `required_slots + HOLD_HEADROOM`.) +pub const HOLD_HEADROOM: u32 = 8; -/// The pure pool shape for one (caps, slot-count) pair. +/// The pure pool shape for one (caps, required-slots) pair. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub struct PoolPlan { + /// Distinct-mode reference-only DPB array; 0 images in coincide mode (the + /// picture pool IS the DPB backing there). pub dpb_image_count: u32, pub dpb_layers_per_image: u32, pub dpb_usage: vk::ImageUsageFlags, - pub dpb_flags: vk::ImageCreateFlags, - /// 0 in coincide mode — outputs ARE the DPB slots. - pub output_image_count: u32, - pub output_usage: vk::ImageUsageFlags, - pub output_flags: vk::ImageCreateFlags, - /// Semaphore/query/command ring size: DPB slots when coincide, the output - /// ring otherwise. - pub output_slots: u32, + /// The decoupled picture pool: decode outputs + (coincide) DPB bindings. + pub picture_count: u32, + pub picture_usage: vk::ImageUsageFlags, + pub picture_flags: vk::ImageCreateFlags, } -/// Decide the pool shape. Pure — the four caps combinations are unit-tested below. +/// Decide the pool shape. Pure — unit-tested below. /// -/// The usages are exactly the ones the caps derivation validated against the -/// driver's advertised envelope ([`DPB_USAGE`]/[`OUTPUT_USAGE`]/[`COINCIDE_USAGE`]); -/// presenter-facing images add only `MUTABLE_FORMAT` (advertised — derive_caps -/// gates on it; no `ALIAS`: nothing aliases these images). -pub fn plan_pools(caps: &DecodeCaps, dpb_slots: u32, output_ring: u32) -> PoolPlan { - let (dpb_image_count, dpb_layers_per_image) = if caps.layered_dpb { - (1, dpb_slots) - } else { - (dpb_slots, 1) - }; - let presented_flags = vk::ImageCreateFlags::MUTABLE_FORMAT; +/// `required_slots` is the stream's `max_dpb_frames + 1`; the picture pool adds +/// [`HOLD_HEADROOM`] on top so consumer-held pictures never displace decode +/// targets. Layered-coincide never reaches here (the caps derivation rejects it). +pub fn plan_pools(caps: &DecodeCaps, required_slots: u32) -> PoolPlan { + let picture_count = required_slots + HOLD_HEADROOM; + let picture_flags = vk::ImageCreateFlags::MUTABLE_FORMAT; if caps.coincide { PoolPlan { - dpb_image_count, - dpb_layers_per_image, - dpb_usage: COINCIDE_USAGE, - dpb_flags: presented_flags, - output_image_count: 0, - output_usage: vk::ImageUsageFlags::empty(), - output_flags: vk::ImageCreateFlags::empty(), - output_slots: dpb_slots, + dpb_image_count: 0, + dpb_layers_per_image: 0, + dpb_usage: vk::ImageUsageFlags::empty(), + picture_count, + picture_usage: COINCIDE_USAGE, + picture_flags, } } else { + let (dpb_image_count, dpb_layers_per_image) = if caps.layered_dpb { + (1, required_slots) + } else { + (required_slots, 1) + }; PoolPlan { dpb_image_count, dpb_layers_per_image, dpb_usage: DPB_USAGE, - dpb_flags: vk::ImageCreateFlags::empty(), - output_image_count: output_ring, - output_usage: OUTPUT_USAGE, - output_flags: presented_flags, - output_slots: output_ring, + picture_count, + picture_usage: OUTPUT_USAGE, + picture_flags, } } } -/// One presenter-facing output slot: the image (a DPB slot's in coincide mode, a -/// ring image otherwise), its full + per-plane views, and the timeline semaphore -/// each decode into this slot signals. -pub(crate) struct OutputSlot { +/// One picture-pool image with its sync + occupancy ledger. +pub(crate) struct Picture { pub image: vk::Image, - /// The image's array layer this slot occupies (barriers target it; the VIEWS - /// already select it, so picture resources use `base_array_layer` 0). - pub layer: u32, - /// Full-picture view in the pool format (what decode binds as `dst`). + /// Full-picture NV12 view (decode dst / DPB binding). pub view: vk::ImageView, - /// `R8_UNORM` / `R8G8_UNORM` plane views for the presenter's sampler path. + /// `R8`/`R8G8` plane views for the presenter's sampler path. pub plane_views: [vk::ImageView; 2], + /// The image's own timeline semaphore (AVVkFrame contract). pub semaphore: vk::Semaphore, - /// Last timeline value signalled on `semaphore` (0 = never used). + /// Latest timeline value known signalled-or-enqueued: the decoder's write + /// signal, bumped to the presenter's write-back (`frame.value + 1`) when a + /// release token reports the frame was sampled. pub value: u64, + /// A DPB slot currently binds this image (coincide mode). + pub bound: bool, + /// A decoded picture awaiting its output verdict lives here. + pub pending: bool, + /// Frames over this image not yet released (ready queue + consumer-held). + pub held: u32, } -/// The Vulkan half: images, memory, views, semaphores. Destroys everything it -/// created on drop (null-safe, so a half-built pool from a failed create unwinds -/// cleanly). -pub(crate) struct ImagePool { +impl Picture { + /// Free for a new decode target: no slot binds it, no pending picture lives + /// in it, no unreleased frame reads it. + pub(crate) fn is_free(&self) -> bool { + !self.bound && !self.pending && self.held == 0 + } +} + +/// The decoupled picture pool. Destroys everything it created on drop +/// (null-safe); a pool with consumer-held images is retired to the decoder's +/// graveyard instead of dropped, and dies when its last release token arrives. +pub(crate) struct PicturePool { device: ash::Device, - pub(crate) coincide: bool, - /// The STREAM's coded extent — what per-picture resources report. - pub(crate) coded_extent: vk::Extent2D, - /// The allocation extent: `coded_extent` rounded up to the device's - /// `pictureAccessGranularity` (images only; never leaks into picture params). - image_extent: vk::Extent2D, - images: Vec, memory: Vec, - /// Per-DPB-slot full view (setup/reference binding). - dpb_views: Vec, - /// Per-DPB-slot (image index, array layer) for barrier targeting. - dpb_location: Vec<(usize, u32)>, - pub(crate) outputs: Vec, + pub(crate) pictures: Vec, } -impl ImagePool { - /// Create the pools for `plan`: images at the granularity-rounded - /// `image_extent`, picture metadata at the stream's `coded_extent`. +impl PicturePool { + /// Create `plan.picture_count` single-layer images at `extent` (the + /// granularity-ALIGNED allocation extent). /// /// # Safety /// @@ -140,157 +144,182 @@ impl ImagePool { dev: &DecodeDevice, caps: &DecodeCaps, plan: &PoolPlan, - coded_extent: vk::Extent2D, + extent: vk::Extent2D, std_profile_idc: hh::StdVideoH264ProfileIdc, ) -> Result { let mut pool = Self { device: dev.ash().clone(), - coincide: caps.coincide, - coded_extent, - image_extent: caps.aligned_extent(coded_extent), - images: Vec::new(), memory: Vec::new(), - dpb_views: Vec::new(), - dpb_location: Vec::new(), - outputs: Vec::new(), + pictures: Vec::new(), }; - // SAFETY: caller's contract; on error `pool` drops and unwinds whatever - // half was built (Drop is null-safe and destroys only owned objects). - unsafe { pool.build(dev, caps, plan, std_profile_idc)? }; - Ok(pool) - } - - /// # Safety - /// - /// As [`Self::create`]. - unsafe fn build( - &mut self, - dev: &DecodeDevice, - caps: &DecodeCaps, - plan: &PoolPlan, - std_profile_idc: hh::StdVideoH264ProfileIdc, - ) -> Result<(), AllocError> { let families = dev.sharing_families(); - - // DPB images + per-slot views. - for _ in 0..plan.dpb_image_count { - // SAFETY: fn contract (live device). + for _ in 0..plan.picture_count { + // SAFETY: fn contract (live device); every created handle is parked + // in `pool` so a mid-build failure unwinds through Drop. let (image, memory) = unsafe { create_video_image( dev, - caps.dpb_format, - self.image_extent, - plan.dpb_layers_per_image, - plan.dpb_usage, - plan.dpb_flags, + caps.output_format, + extent, + 1, + plan.picture_usage, + plan.picture_flags, &families, std_profile_idc, )? }; - self.images.push(image); - self.memory.push(memory); + pool.memory.push(memory); + // The picture is parked with null handles IMMEDIATELY (Drop ignores + // nulls), then each view/semaphore is filled as it is created — a + // failure anywhere unwinds everything created so far. + pool.pictures.push(Picture { + image, + view: vk::ImageView::null(), + plane_views: [vk::ImageView::null(); 2], + semaphore: vk::Semaphore::null(), + value: 0, + bound: false, + pending: false, + held: 0, + }); + let picture = pool.pictures.len() - 1; + // SAFETY: `image` was just created with layer 0 in range (holds for + // all three creates in this block); plane formats are + // NV12-compatible under MUTABLE_FORMAT (caps-gated by derive_caps). + unsafe { + pool.pictures[picture].view = create_view( + &pool.device, + image, + caps.output_format, + vk::ImageAspectFlags::COLOR, + 0, + )?; + pool.pictures[picture].plane_views[0] = create_view( + &pool.device, + image, + vk::Format::R8_UNORM, + vk::ImageAspectFlags::PLANE_0, + 0, + )?; + pool.pictures[picture].plane_views[1] = create_view( + &pool.device, + image, + vk::Format::R8G8_UNORM, + vk::ImageAspectFlags::PLANE_1, + 0, + )?; + } + let mut type_info = vk::SemaphoreTypeCreateInfo::default() + .semaphore_type(vk::SemaphoreType::TIMELINE) + .initial_value(0); + let sem_ci = vk::SemaphoreCreateInfo::default().push_next(&mut type_info); + // SAFETY: live device; timelineSemaphore enabled per the handles + // contract. + pool.pictures[picture].semaphore = + unsafe { pool.device.create_semaphore(&sem_ci, None)? }; } - let dpb_slots = plan.dpb_image_count * plan.dpb_layers_per_image; - for slot in 0..dpb_slots { + Ok(pool) + } + + /// Index of the first free image, if any. + pub(crate) fn free_index(&self) -> Option { + self.pictures.iter().position(Picture::is_free) + } + + /// Total frames not yet released across the pool (graveyard retirement key). + pub(crate) fn held_total(&self) -> u32 { + self.pictures.iter().map(|p| p.held).sum() + } +} + +impl Drop for PicturePool { + fn drop(&mut self) { + // SAFETY: every handle is this pool's own on the (contract-live) device; + // the owning decoder drains decode work before dropping/retiring, and a + // retired pool is only dropped once its last release token returned (the + // presenter's fence wait). Destroys ignore NULL (half-built unwinding). + unsafe { + for p in self.pictures.drain(..) { + self.device.destroy_image_view(p.view, None); + self.device.destroy_image_view(p.plane_views[0], None); + self.device.destroy_image_view(p.plane_views[1], None); + self.device.destroy_semaphore(p.semaphore, None); + self.device.destroy_image(p.image, None); + } + for memory in self.memory.drain(..) { + self.device.free_memory(memory, None); + } + } + } +} + +/// Distinct-mode reference-only DPB backing (fixed slot↔layer mapping — these +/// images are never delivered, so nothing consumer-side ever pins them). +pub(crate) struct DpbPool { + device: ash::Device, + images: Vec, + memory: Vec, + dpb_views: Vec, + dpb_location: Vec<(usize, u32)>, +} + +impl DpbPool { + /// # Safety + /// + /// `dev` wraps live handles ([`crate::DeviceHandles`] contract). + pub(crate) unsafe fn create( + dev: &DecodeDevice, + caps: &DecodeCaps, + plan: &PoolPlan, + extent: vk::Extent2D, + std_profile_idc: hh::StdVideoH264ProfileIdc, + ) -> Result { + let mut pool = Self { + device: dev.ash().clone(), + images: Vec::new(), + memory: Vec::new(), + dpb_views: Vec::new(), + dpb_location: Vec::new(), + }; + let families = dev.sharing_families(); + for _ in 0..plan.dpb_image_count { + // SAFETY: fn contract (live device); parked in `pool` for unwinding. + let (image, memory) = unsafe { + create_video_image( + dev, + caps.dpb_format, + extent, + plan.dpb_layers_per_image, + plan.dpb_usage, + vk::ImageCreateFlags::empty(), + &families, + std_profile_idc, + )? + }; + pool.images.push(image); + pool.memory.push(memory); + } + let slots = plan.dpb_image_count * plan.dpb_layers_per_image; + for slot in 0..slots { let (image_index, layer) = if plan.dpb_image_count == 1 { (0usize, slot) } else { (slot as usize, 0u32) }; - // SAFETY: `image` was created above with at least `layer + 1` layers. + // SAFETY: the image was created above with `layer` in range. let view = unsafe { create_view( - &self.device, - self.images[image_index], + &pool.device, + pool.images[image_index], caps.dpb_format, vk::ImageAspectFlags::COLOR, layer, )? }; - self.dpb_views.push(view); - self.dpb_location.push((image_index, layer)); + pool.dpb_views.push(view); + pool.dpb_location.push((image_index, layer)); } - - // Output slots: over the DPB slots (coincide) or over a fresh ring. - let output_targets: Vec<(vk::Image, u32)> = if caps.coincide { - self.dpb_location - .iter() - .map(|&(image_index, layer)| (self.images[image_index], layer)) - .collect() - } else { - let mut targets = Vec::new(); - for _ in 0..plan.output_image_count { - // SAFETY: fn contract (live device). - let (image, memory) = unsafe { - create_video_image( - dev, - caps.output_format, - self.image_extent, - 1, - plan.output_usage, - plan.output_flags, - &families, - std_profile_idc, - )? - }; - self.images.push(image); - self.memory.push(memory); - targets.push((image, 0)); - } - targets - }; - - for (image, layer) in output_targets { - // SAFETY: `image` exists with `layer` in range (holds for all four - // creates below); the formats are plane-compatible with the pool - // format (NV12: R8 + R8G8) and the image carries MUTABLE_FORMAT for - // the reinterpreting views. - let view = unsafe { - create_view( - &self.device, - image, - caps.output_format, - vk::ImageAspectFlags::COLOR, - layer, - )? - }; - // SAFETY: as above. - let plane_y = unsafe { - create_view( - &self.device, - image, - vk::Format::R8_UNORM, - vk::ImageAspectFlags::PLANE_0, - layer, - )? - }; - // SAFETY: as above. - let plane_uv = unsafe { - create_view( - &self.device, - image, - vk::Format::R8G8_UNORM, - vk::ImageAspectFlags::PLANE_1, - layer, - )? - }; - let mut type_info = vk::SemaphoreTypeCreateInfo::default() - .semaphore_type(vk::SemaphoreType::TIMELINE) - .initial_value(0); - let sem_ci = vk::SemaphoreCreateInfo::default().push_next(&mut type_info); - // SAFETY: live device; timelineSemaphore is enabled per the - // DeviceHandles feature contract. - let semaphore = unsafe { self.device.create_semaphore(&sem_ci, None)? }; - self.outputs.push(OutputSlot { - image, - layer, - view, - plane_views: [plane_y, plane_uv], - semaphore, - value: 0, - }); - } - Ok(()) + Ok(pool) } /// The DPB binding view of `slot`. @@ -305,21 +334,15 @@ impl ImagePool { } } -impl Drop for ImagePool { +impl Drop for DpbPool { fn drop(&mut self) { - // SAFETY: every handle below was created by this pool on this (still-live, - // per the DeviceHandles contract) device; the owning decoder drains GPU - // work before dropping state. vkDestroy*/vkFree ignore NULL handles. + // SAFETY: own handles on the contract-live device; the owning decoder + // drains decode work before dropping state (nothing consumer-side ever + // references these). Destroys ignore NULL. unsafe { for view in self.dpb_views.drain(..) { self.device.destroy_image_view(view, None); } - for out in self.outputs.drain(..) { - self.device.destroy_image_view(out.view, None); - self.device.destroy_image_view(out.plane_views[0], None); - self.device.destroy_image_view(out.plane_views[1], None); - self.device.destroy_semaphore(out.semaphore, None); - } for image in self.images.drain(..) { self.device.destroy_image(image, None); } @@ -480,54 +503,66 @@ mod tests { } #[test] - fn coincide_layered_is_one_dual_use_array_with_no_output_ring() { - let plan = plan_pools(&caps(true, true), 5, OUTPUT_RING); - assert_eq!((plan.dpb_image_count, plan.dpb_layers_per_image), (1, 5)); + fn coincide_pools_are_headroomed_dual_use_pictures_with_no_dpb_array() { + let plan = plan_pools(&caps(true, false), 8); assert_eq!( - plan.dpb_usage, COINCIDE_USAGE, - "coincide: the DPB image is the decode output AND the sampled surface" + plan.dpb_image_count, 0, + "the picture pool IS the DPB backing" ); assert_eq!( - plan.dpb_flags, - vk::ImageCreateFlags::MUTABLE_FORMAT, - "plane views need MUTABLE_FORMAT; nothing aliases, so no ALIAS" + plan.picture_count, + 8 + HOLD_HEADROOM, + "the stream's DPB depth PLUS the consumer-hold headroom — a pool \ + sized to either alone starves (.25 field failure)" ); - assert_eq!(plan.output_image_count, 0); - assert_eq!(plan.output_slots, 5, "one output slot per DPB slot"); + assert_eq!( + plan.picture_usage, COINCIDE_USAGE, + "pool pictures are DPB + decode dst + sampled surface in one" + ); + assert_eq!(plan.picture_flags, vk::ImageCreateFlags::MUTABLE_FORMAT); } #[test] - fn coincide_separate_is_one_dual_use_image_per_slot() { - let plan = plan_pools(&caps(true, false), 5, OUTPUT_RING); - assert_eq!((plan.dpb_image_count, plan.dpb_layers_per_image), (5, 1)); - assert_eq!(plan.output_image_count, 0); - assert_eq!(plan.output_slots, 5); + fn distinct_keeps_a_fixed_dpb_array_and_headrooms_the_output_pool() { + let plan = plan_pools(&caps(false, true), 17); + assert_eq!( + (plan.dpb_image_count, plan.dpb_layers_per_image), + (1, 17), + "layered: one array, one layer per slot" + ); + assert_eq!(plan.dpb_usage, DPB_USAGE); + assert_eq!(plan.picture_count, 17 + HOLD_HEADROOM); + assert_eq!(plan.picture_usage, OUTPUT_USAGE); + + let plan = plan_pools(&caps(false, false), 3); + assert_eq!( + (plan.dpb_image_count, plan.dpb_layers_per_image), + (3, 1), + "separate reference images: one image per slot" + ); + assert_eq!(plan.picture_count, 3 + HOLD_HEADROOM); } #[test] - fn distinct_layered_is_a_reference_only_array_plus_an_output_ring() { - let plan = plan_pools(&caps(false, true), 17, OUTPUT_RING); - assert_eq!((plan.dpb_image_count, plan.dpb_layers_per_image), (1, 17)); - assert_eq!( - plan.dpb_usage, DPB_USAGE, - "distinct: the DPB is never sampled and never a decode dst" - ); - assert_eq!(plan.dpb_flags, vk::ImageCreateFlags::empty()); - assert_eq!(plan.output_image_count, OUTPUT_RING); - assert_eq!(plan.output_usage, OUTPUT_USAGE); - assert_eq!( - plan.output_flags, - vk::ImageCreateFlags::MUTABLE_FORMAT, - "plane views need MUTABLE_FORMAT; nothing aliases, so no ALIAS" - ); - assert_eq!(plan.output_slots, OUTPUT_RING); - } - - #[test] - fn distinct_separate_is_per_slot_reference_images_plus_the_ring() { - let plan = plan_pools(&caps(false, false), 3, 2); - assert_eq!((plan.dpb_image_count, plan.dpb_layers_per_image), (3, 1)); - assert_eq!(plan.output_image_count, 2); - assert_eq!(plan.output_slots, 2); + fn picture_occupancy_frees_only_when_unbound_unpending_and_released() { + let mut p = Picture { + image: vk::Image::null(), + view: vk::ImageView::null(), + plane_views: [vk::ImageView::null(); 2], + semaphore: vk::Semaphore::null(), + value: 0, + bound: true, + pending: true, + held: 2, + }; + assert!(!p.is_free()); + p.bound = false; + assert!(!p.is_free(), "pending pictures are not decode targets"); + p.pending = false; + assert!(!p.is_free(), "held frames are not decode targets"); + p.held = 1; + assert!(!p.is_free()); + p.held = 0; + assert!(p.is_free()); } } diff --git a/crates/pf-vkdecode/src/lib.rs b/crates/pf-vkdecode/src/lib.rs index fdd38b98..91d7964d 100644 --- a/crates/pf-vkdecode/src/lib.rs +++ b/crates/pf-vkdecode/src/lib.rs @@ -27,13 +27,17 @@ //! coincide/distinct/layered decision table ([`DecodeCaps`]). //! - [`session`]: `VkVideoSessionKHR` + versioned session parameters (pure ledger //! decides Add-vs-Recreate; extent/DPB renegotiation rebuilds the session). -//! - [`images`]: DPB + output pools for BOTH DPB arrangements (pure [`plan_pools`] -//! decides the shape), per-plane views, one timeline semaphore per output slot, -//! crop carried on the frame. -//! - [`ring`]: the host-visible bitstream upload ring (pure alignment/growth math). +//! - [`images`]: the picture pool DECOUPLED from DPB slots (the zero-copy FFmpeg +//! pool model — a re-activated slot binds a fresh free image, so delivered +//! pictures are never decode targets), per-image timeline semaphores with the +//! presenter `value+1` write-back, per-plane views, [`HOLD_HEADROOM`] sizing. +//! - [`ring`]: the host-visible bitstream upload ring (pure alignment/growth +//! math) — SLICE NALUs only (feeding a whole AU hangs VCN firmware). //! - [`decoder`]: [`VkH264Decoder`] — plan → convert → upload → record → submit, //! with a per-op `RESULT_STATUS_ONLY` query ([`VkH264Decoder::poll_status`]) so -//! driver-reported corruption is finally observable (the Ally X class). +//! driver-reported corruption is finally observable (the Ally X class) — +//! caps-gated per queue family: where `queryResultStatusSupport` is absent +//! (RADV), verdicts degrade to timeline completion, FFmpeg parity. //! //! Unsafe posture: unlike pf-bitstream (which forbids unsafe outright), this crate //! cannot — the `ash::vk::native` bindgen structs are zero-initialized the way the @@ -80,6 +84,7 @@ pub use device::QueueLock; pub use device::QueueSubmitGuard; pub use images::plan_pools; pub use images::PoolPlan; +pub use images::HOLD_HEADROOM; pub use params::pps_to_std; pub use params::sps_to_std; pub use params::OwnedStdPps; diff --git a/crates/pf-vkdecode/src/pic.rs b/crates/pf-vkdecode/src/pic.rs index 0c94b655..3c6e7b85 100644 --- a/crates/pf-vkdecode/src/pic.rs +++ b/crates/pf-vkdecode/src/pic.rs @@ -360,6 +360,114 @@ mod tests { aus } + /// The decoder's picture-pool occupancy (`bound`/`pending`/`held`, exactly + /// `decode_inner`'s bookkeeping minus the GPU) over the WHOLE vendored + /// vector, with a consumer that HOLDS `hold` delivered frames before + /// releasing the oldest — the real client's shape (~4-7 held across its + /// channels, preroll and in-flight present). Returns the first starved AU + /// index, if any. + fn simulate_pool_occupancy(pool_size: usize, hold: usize) -> Option { + use std::collections::VecDeque; + + #[derive(Clone, Default)] + struct SimPicture { + bound: bool, + pending: bool, + held: u32, + } + + let aus = split_into_aus(TEST_25FPS); + let mut planner = H264Planner::new(); + let mut slots: Option = None; + let mut pictures = vec![SimPicture::default(); pool_size]; + let mut slot_image: Vec> = Vec::new(); + // id -> pool image of the decoded picture awaiting its output verdict. + let mut pending: BTreeMap = BTreeMap::new(); + // Delivered frames the consumer holds, oldest first. + let mut consumer: VecDeque = VecDeque::new(); + + for (index, au) in aus.iter().enumerate() { + let plan = planner.plan_au(au).expect("the clean vector plans"); + let slots = slots.get_or_insert_with(|| { + slot_image = vec![None; plan.picture.max_dpb_frames + 1]; + SlotMap::new(plan.picture.max_dpb_frames) + }); + let vk = plan_to_vk(&plan, slots, 0).expect("the clean vector converts"); + + // Binding sync: released slots unbind; the setup slot rebinds fresh. + let setup = usize::from(vk.setup_slot); + let mut held_slots = vec![false; slot_image.len()]; + for (slot, _id) in slots.held() { + held_slots[usize::from(slot)] = true; + } + for (slot, binding) in slot_image.iter_mut().enumerate() { + if let Some(picture) = *binding { + if !held_slots[slot] || slot == setup { + pictures[picture].bound = false; + *binding = None; + } + } + } + + // The decode target: a free pool image. + let Some(dst) = pictures + .iter() + .position(|p| !p.bound && !p.pending && p.held == 0) + else { + return Some(index); + }; + pictures[dst].pending = true; + pictures[dst].bound = true; + slot_image[setup] = Some(dst); + pending.insert(vk.setup_id, dst); + + // Settle: outputs deliver to the consumer; removed-never-output free. + for id in &plan.dpb.outputs { + if let Some(picture) = pending.remove(id) { + pictures[picture].pending = false; + pictures[picture].held += 1; + consumer.push_back(picture); + } + } + for id in &plan.dpb.removed { + if let Some(picture) = pending.remove(id) { + pictures[picture].pending = false; + } + } + // The hold-N consumer: releases only once it holds MORE than `hold`. + while consumer.len() > hold { + let released = consumer.pop_front().expect("nonempty"); + pictures[released].held -= 1; + } + } + None + } + + /// The .25 field-failure regression, pool-model edition: the vendored vector + /// keeps up to `max_dpb_frames + 1 = 8` pictures resident AND the real + /// client holds ~4 delivered frames — the pool must absorb BOTH at once. + /// `required_slots + HOLD_HEADROOM` never starves; the counterfactual shows + /// an under-headroomed pool starving on the same clean stream, which is the + /// exact class the fixed-size ring shipped in the first WP-B round. + #[test] + fn the_full_vector_with_a_hold_four_consumer_never_starves_the_picture_pool() { + // This vector: max_dpb_frames = 7 → required_slots = 8 (measured; + // asserted inside via SlotMap sizing). + let required_slots = 8; + let headroom = crate::images::HOLD_HEADROOM as usize; + assert_eq!( + simulate_pool_occupancy(required_slots + headroom, 4), + None, + "the shipped sizing must survive the whole vector with 4 held frames" + ); + // Counterfactual: holds beyond the headroom starve — the documented + // NoFreeSlot condition, now meaning exactly what it says. + assert!( + simulate_pool_occupancy(required_slots + 2, 4).is_some(), + "an under-headroomed pool must starve (else this regression proves nothing)" + ); + } + #[test] fn the_full_25fps_vector_converts_with_stable_slots_and_start_code_offsets() { let aus = split_into_aus(TEST_25FPS); @@ -684,15 +792,16 @@ mod tests { } #[test] - fn a_full_dpb_bump_reuses_the_evicted_slot_only_after_its_frame_is_released() { + fn a_full_dpb_bump_reuses_the_slot_but_the_pool_model_binds_a_fresh_image() { // Depth-1 DPB (Level 1 at 320x240 ⇒ max_dpb_frames 1, capacity 2): every // stored P evicts the previous picture, and that picture's id lands in - // BOTH `outputs` and `removed` of the SAME plan — the exact sequence - // where, without pins, `plan_to_vk` frees the evicted slot and - // immediately re-assigns it as this AU's setup while the evicted - // picture's frame is still in the consumer's hands (the HIGH overwrite - // bug of the adversarial round). This test drives the decoder's exact - // call pattern: pin at frame creation, unpin at release_frame. + // BOTH `outputs` and `removed` of the SAME plan — so `plan_to_vk` frees + // the evicted slot and immediately re-assigns it as this AU's setup. + // That SLOT reuse is fine and expected; the picture-pool model's whole + // point is that the re-activated slot binds a DIFFERENT free image, so + // the delivered picture's image is never the new decode target while the + // consumer holds it (the HIGH overwrite bug of the adversarial round, + // and the .25 field failure's class). let sps = SpsBuilder::new() .seq_parameter_set_id(0) .profile_idc(Profile::Main) @@ -714,64 +823,54 @@ mod tests { Synthesizer::<'_, Pps, _>::synthesize(3, &pps, &mut au0, true).unwrap(); au0.extend(write_idr_slice(None)); - // First, the COUNTERFACTUAL (no pins): the bump hands the evicted - // picture's slot straight back as the next setup — the bug this guards. - { - let mut planner = H264Planner::new(); - let p0 = planner.plan_au(&au0).unwrap(); - let mut slots = SlotMap::new(p0.picture.max_dpb_frames); - let vk0 = plan_to_vk(&p0, &mut slots, 0).unwrap(); - let p1 = planner - .plan_au(&write_p_slice(1, 2, None, 1, None)) - .unwrap(); - let id0 = vk0.setup_id; - assert!(p1.dpb.outputs.contains(&id0) && p1.dpb.removed.contains(&id0)); - let vk1 = plan_to_vk(&p1, &mut slots, 0).unwrap(); - assert_eq!( - vk1.setup_slot, vk0.setup_slot, - "without pins the delivered frame's image IS the next decode target" - ); - } - - // Now the decoder's discipline: every frame pins its slot at creation. let mut planner = H264Planner::new(); let p0 = planner.plan_au(&au0).unwrap(); let mut slots = SlotMap::new(p0.picture.max_dpb_frames); let vk0 = plan_to_vk(&p0, &mut slots, 0).unwrap(); - slots.pin(vk0.setup_slot); - // AU1 bumps AU0's picture (outputs + removed) — the freed slot is - // pinned, so the setup lands elsewhere and the unreleased frame's image - // is never a decode target. + // Decoder-side pool bookkeeping (mirrors decode_inner): image 0 hosts + // picture 0; the consumer receives and HOLDS its frame. + let pool = 2 + 1; // required_slots + 1 of headroom is enough here + let mut bound = vec![false; pool]; + let mut held = vec![0u32; pool]; + let mut slot_image: Vec> = vec![None; slots.capacity()]; + let free = |bound: &[bool], held: &[u32]| (0..pool).find(|&i| !bound[i] && held[i] == 0); + + let img0 = free(&bound, &held).unwrap(); + bound[img0] = true; + slot_image[usize::from(vk0.setup_slot)] = Some(img0); + + // AU1 bumps AU0's picture: outputs+removed carry id0, and plan_to_vk + // hands the SAME slot back as the setup. let p1 = planner .plan_au(&write_p_slice(1, 2, None, 1, None)) .unwrap(); - assert!(p1.dpb.removed.contains(&vk0.setup_id)); + assert!(p1.dpb.outputs.contains(&vk0.setup_id) && p1.dpb.removed.contains(&vk0.setup_id)); let vk1 = plan_to_vk(&p1, &mut slots, 0).unwrap(); - assert_ne!( + assert_eq!( vk1.setup_slot, vk0.setup_slot, - "a pinned (delivered, unreleased) slot must never be the setup" + "slot reuse across the bump is the planner's normal behaviour" ); - slots.pin(vk1.setup_slot); - // With NOTHING released, the next AU has no assignable slot: explicit - // backpressure (AllPinned → the decoder's NoFreeSlot), never an overwrite. - let p2 = planner - .plan_au(&write_p_slice(2, 4, None, 1, None)) - .unwrap(); - assert!(matches!( - plan_to_vk(&p2, &mut slots, 0), - Err(PlanToVkError::Slot(SlotError::AllPinned { .. })) - )); + // Binding sync: the re-activated slot drops its old binding; picture 0's + // image is now delivered to the consumer (held), NOT freed. + bound[img0] = false; + held[img0] += 1; // outputs → delivered, consumer holds it + slot_image[usize::from(vk1.setup_slot)] = None; - // The consumer releases frame 0 (decoder: release_frame → unpin): its - // slot becomes the next setup — reuse happens exactly one release later. - assert!(slots.unpin(vk0.setup_slot)); - let p3 = planner - .plan_au(&write_idr_slice(None)) - .expect("an IDR restart plans after the stalled AU"); - let vk3 = plan_to_vk(&p3, &mut slots, 0).unwrap(); - assert_eq!(vk3.setup_slot, vk0.setup_slot); + // The pool hands the re-activated slot a FRESH image — never image 0. + let img1 = free(&bound, &held).expect("headroom guarantees a free image"); + assert_ne!( + img1, img0, + "the held (delivered, unreleased) image must never be re-bound as a \ + decode target — the pool decoupling IS the overwrite fix" + ); + bound[img1] = true; + slot_image[usize::from(vk1.setup_slot)] = Some(img1); + + // Once the consumer releases frame 0, image 0 returns to the free list. + held[img0] -= 1; + assert_eq!(free(&bound, &held), Some(img0)); } #[test] diff --git a/crates/pf-vkdecode/src/ring.rs b/crates/pf-vkdecode/src/ring.rs index 15d14af0..ead7dd2c 100644 --- a/crates/pf-vkdecode/src/ring.rs +++ b/crates/pf-vkdecode/src/ring.rs @@ -297,19 +297,29 @@ impl BitstreamRing { /// it has (bounded by the caller's timeout policy). The split keeps this /// module free of any semaphore knowledge. /// + /// `segments` are the byte ranges of `au` to upload, CONCATENATED — the + /// decoder passes the SLICE NALUs only. The buffer must contain nothing but + /// slice data: the VCN firmware scans the submitted range itself, and + /// non-slice NALUs (AUD/SEI/SPS/PPS, which real AUs open with) in the range + /// hang it — the 2026-08 .25 `vcn_unified_0 ring timeout`. FFmpeg's decoder + /// feeds slices-only for the same reason; parameter sets ride the session + /// parameters object instead. + /// /// # Safety /// - /// Live device (contract), and the tokens passed to prior - /// [`SlotStates::set_pending`] calls must genuinely cover every GPU read of - /// their slots — recycling rewrites slot bytes as soon as a token reports done. + /// Live device (contract); `segments` are in-bounds ranges of `au`; and the + /// tokens passed to prior [`SlotStates::set_pending`] calls genuinely cover + /// every GPU read of their slots — recycling rewrites slot bytes as soon as a + /// token reports done. pub(crate) unsafe fn upload>( &mut self, dev: &DecodeDevice, au: &[u8], + segments: &[std::ops::Range], poll: &mut dyn FnMut(&Token) -> Result, wait: &mut dyn FnMut(&Token) -> Result<(), E>, ) -> Result { - let len = au.len() as u64; + let len: u64 = segments.iter().map(|s| s.len() as u64).sum(); if !self.layout.fits(len) { // Grow: drain EVERYTHING in flight (their reads target the old buffer), // then recreate the backing under the grown layout. @@ -356,14 +366,22 @@ impl BitstreamRing { // SAFETY: `ptr` is the live persistent mapping of a buffer of // `layout.buffer_size()` bytes; `offset + range <= buffer_size` because // `range <= slot_size` (fits/grown above) and offset is `slot * slot_size` - // with `slot < slots`. The slot is not concurrently read: its previous use - // completed (poll/wait above) and its next use is submitted after this copy. + // with `slot < slots`; each segment is an in-bounds range of `au` (fn + // contract) and the cursor advances by exactly the bytes written, staying + // within `len <= range`. The slot is not concurrently read: its previous + // use completed (poll/wait above) and its next use is submitted after + // this copy. unsafe { let base = self.ptr.add(offset as usize); - std::ptr::copy_nonoverlapping(au.as_ptr(), base, au.len()); + let mut cursor = 0usize; + for segment in segments { + let bytes = &au[segment.clone()]; + std::ptr::copy_nonoverlapping(bytes.as_ptr(), base.add(cursor), bytes.len()); + cursor += bytes.len(); + } // Zero the alignment tail so the recorded range never hands the driver // stale bytes from a previous AU behind this one's end. - std::ptr::write_bytes(base.add(au.len()), 0, (range - len) as usize); + std::ptr::write_bytes(base.add(cursor), 0, (range - len) as usize); } Ok(UploadedAu { offset, diff --git a/crates/pf-vkdecode/src/slots.rs b/crates/pf-vkdecode/src/slots.rs index e37631b9..d7f1de21 100644 --- a/crates/pf-vkdecode/src/slots.rs +++ b/crates/pf-vkdecode/src/slots.rs @@ -14,10 +14,8 @@ use tracing::trace; /// The H.264 slot ceiling: 16 reference frames plus the picture being decoded. const MAX_SLOTS: usize = 17; -/// What went wrong with a slot operation. `Full`/`AlreadyAssigned` are caller -/// bugs, not stream conditions — pf-bitstream degrades stream damage to warnings -/// long before here. `AllPinned` is neither: it reports a consumer that has not -/// released delivered frames (backpressure, not a ledger fault). +/// What went wrong with a slot operation. Both variants are caller bugs, not stream +/// conditions — pf-bitstream degrades stream damage to warnings long before here. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum SlotError { /// No free slot. The map is sized to `max_dpb_frames + 1`, which the planner's @@ -25,11 +23,6 @@ pub enum SlotError { Full { capacity: usize }, /// The id already holds a slot; ids are per-picture and never re-assigned. AlreadyAssigned { id: PicId, slot: u8 }, - /// Free slots exist but every one is [pinned](SlotMap::pin) by an out-of-DPB - /// reader (a delivered frame the consumer has not released). Assigning one - /// would let the next decode overwrite an image someone is still reading — - /// the caller must surface backpressure instead. - AllPinned { free: usize }, } impl std::fmt::Display for SlotError { @@ -44,12 +37,6 @@ impl std::fmt::Display for SlotError { SlotError::AlreadyAssigned { id, slot } => { write!(f, "picture {id} already holds slot {slot}") } - SlotError::AllPinned { free } => { - write!( - f, - "every free DPB slot ({free}) is pinned by an unreleased frame" - ) - } } } } @@ -62,19 +49,16 @@ impl std::error::Error for SlotError {} /// Invariants (unit-tested): /// - a [`PicId`] keeps its slot from [`Self::assign`] until [`Self::release`]; /// - a slot is reused only after its holder is released; -/// - assigning past capacity errors instead of evicting; -/// - a [pinned](Self::pin) slot is never assigned, held or free — pins are the -/// WP-B two-phase-release layer: a slot backing a DELIVERED-but-unreleased -/// frame stays pinned past its DPB eviction, because in coincide mode the next -/// setup assignment would otherwise overwrite the very image the consumer is -/// still reading (the full-DPB bump hands `outputs`+`removed` the same id in -/// the same plan, and the freed slot is exactly the lowest one). +/// - assigning past capacity errors instead of evicting. +/// +/// Slots are pure planner bookkeeping: consumers never hold a SLOT (the decoder's +/// picture pool decouples IMAGES from slots — a re-activated slot binds a fresh +/// free image, so a delivered picture's image is never a decode target while the +/// consumer reads it). #[derive(Debug, Clone)] pub struct SlotMap { /// `slots[i]` holds the id bound to slot `i`, `None` while the slot is free. slots: Vec>, - /// Out-of-DPB reader counts per slot (refcounted, orthogonal to residency). - pinned: Vec, } impl SlotMap { @@ -94,7 +78,6 @@ impl SlotMap { ); Self { slots: vec![None; max_dpb_frames + 1], - pinned: vec![0; max_dpb_frames + 1], } } @@ -119,62 +102,21 @@ impl SlotMap { .filter_map(|(index, slot)| slot.map(|id| (index as u8, id))) } - /// Bind `id` to the lowest free UNPINNED slot. - /// - /// Free-but-pinned slots are skipped (their images are still read outside the - /// DPB); when only such slots remain the error is [`SlotError::AllPinned`], - /// distinct from [`SlotError::Full`] because it names a consumer that owes a - /// release, not a ledger bug. + /// Bind `id` to the lowest free slot. pub fn assign(&mut self, id: PicId) -> Result { if let Some(slot) = self.slot_of(id) { return Err(SlotError::AlreadyAssigned { id, slot }); } - let mut free_but_pinned = 0usize; - let assignable = self.slots.iter().enumerate().position(|(index, slot)| { - if slot.is_some() { - return false; - } - if self.pinned[index] > 0 { - free_but_pinned += 1; - return false; - } - true - }); - match assignable { - Some(free) => { - self.slots[free] = Some(id); - // The envelope-gated capacity (<= 17) keeps every index within u8. - Ok(free as u8) - } - None if free_but_pinned > 0 => Err(SlotError::AllPinned { - free: free_but_pinned, - }), - None => Err(SlotError::Full { + let free = self + .slots + .iter() + .position(Option::is_none) + .ok_or(SlotError::Full { capacity: self.slots.len(), - }), - } - } - - /// Add one out-of-DPB reader to `slot` (refcounted): the slot stays - /// unassignable — even after its picture leaves the DPB — until the matching - /// [`Self::unpin`]. The decoder pins a slot for every live [frame] it backs - /// and unpins on `release_frame`/internal drop. - /// - /// [frame]: crate::decoder::DecodedVkFrame - pub fn pin(&mut self, slot: u8) { - self.pinned[usize::from(slot)] += 1; - } - - /// Remove one reader from `slot`. Returns `false` (and changes nothing) when - /// the slot carried no pin — a double release, tolerated but never silent at - /// the caller. - pub fn unpin(&mut self, slot: u8) -> bool { - let count = &mut self.pinned[usize::from(slot)]; - if *count == 0 { - return false; - } - *count -= 1; - true + })?; + self.slots[free] = Some(id); + // The envelope-gated capacity (<= 17) keeps every index within u8. + Ok(free as u8) } /// The slot `id` holds, if any. @@ -309,45 +251,6 @@ mod tests { assert_eq!(slots.slot_of(2), None); } - #[test] - fn a_pinned_slot_is_skipped_by_assign_until_every_pin_is_released() { - let mut slots = SlotMap::new(1); // capacity 2 - let s0 = slots.assign(1).unwrap(); - slots.pin(s0); - slots.pin(s0); // refcounted: two readers - slots.release(1); // DPB eviction — the pin must keep protecting the slot - - // The pinned slot is skipped; the other free slot is handed out. - let s1 = slots.assign(2).unwrap(); - assert_ne!(s1, s0); - - // Now every free slot is pinned: a DISTINCT error from Full (the ledger - // is fine; the consumer owes a release). - assert_eq!(slots.assign(3), Err(SlotError::AllPinned { free: 1 })); - - // One unpin is not enough (two readers were counted)… - assert!(slots.unpin(s0)); - assert_eq!(slots.assign(3), Err(SlotError::AllPinned { free: 1 })); - // …the second frees it, and the slot is assignable again. - assert!(slots.unpin(s0)); - assert_eq!(slots.assign(3), Ok(s0)); - - // A pin-less unpin is a reported no-op, not an underflow. - assert!(!slots.unpin(s1)); - } - - #[test] - fn full_and_all_pinned_stay_distinct_verdicts() { - let mut slots = SlotMap::new(1); // capacity 2 - slots.assign(1).unwrap(); - slots.assign(2).unwrap(); - // Genuinely full (all HELD): the missed-removals bug class. - assert_eq!(slots.assign(3), Err(SlotError::Full { capacity: 2 })); - // A pin on a HELD slot changes nothing about that verdict. - slots.pin(0); - assert_eq!(slots.assign(3), Err(SlotError::Full { capacity: 2 })); - } - #[test] fn a_hundred_synthetic_dpb_updates_churn_without_aliasing_a_slot() { // A sliding window of 4 references over 100 pictures: each id's slot must diff --git a/crates/pf-vkdecode/tests/gpu_smoke.rs b/crates/pf-vkdecode/tests/gpu_smoke.rs index c2a9ebd1..be162e2d 100644 --- a/crates/pf-vkdecode/tests/gpu_smoke.rs +++ b/crates/pf-vkdecode/tests/gpu_smoke.rs @@ -15,15 +15,18 @@ //! - `timelineSemaphore` + `synchronization2` feature support (Vulkan 1.3 core). //! //! What it proves: device wrap → caps query/derivation on REAL caps → session + -//! parameters creation → DPB/output/ring pools → 48 AUs of the vendored 25fps -//! vector decoded through `vkCmdDecodeVideoKHR` — well past DPB-full, so the -//! bump-eviction slot-reuse path runs — with the full frame lifecycle each -//! delivery: `wait_status` reading the RESULT_STATUS_ONLY query back as -//! COMPLETE, then `release_frame` returning the slot (the two-phase release the -//! coincide-mode overwrite fix depends on). What it deliberately does NOT prove -//! (WP-D on-glass): pixel correctness vs the ffmpeg rung, presenter -//! interop/layout round-trips, soak, and both vendors' DPB arrangements at once -//! (each box exercises only its own). +//! parameters creation → the decoupled picture pool → 48 AUs of the vendored +//! 25fps vector decoded through `vkCmdDecodeVideoKHR` — well past DPB-full, so +//! slot re-activation binds fresh pool images repeatedly — while the consumer +//! HOLDS FOUR delivered frames unreleased at steady state (the real client's +//! pipeline shape: bounded channels, FrameStore preroll, in-flight present). +//! Every frame's RESULT_STATUS_ONLY query must read COMPLETE before its +//! release. This is the regression test for the .25 field failure class: any +//! pool sizing that ignores the stream's DPB depth or the client's hold depth +//! starves exactly here. What it deliberately does NOT prove (WP-D on-glass): +//! pixel correctness vs the ffmpeg rung, presenter interop (the `value + 1` +//! signal-back — no presenter runs here, so releases pass `false`), soak, and +//! both vendors' DPB arrangements at once (each box exercises only its own). #![deny(clippy::undocumented_unsafe_blocks)] @@ -69,7 +72,7 @@ fn split_into_aus(stream: &[u8]) -> Vec<&[u8]> { #[test] #[ignore = "needs a Vulkan Video H.264 decode device (fleet boxes; see module docs)"] -fn decodes_48_aus_with_status_reads_and_frame_releases_past_dpb_full() { +fn decodes_48_aus_holding_four_frames_like_the_real_client() { // ---- instance ---- // SAFETY: loads the system Vulkan loader; no Vulkan objects exist yet. let entry = unsafe { ash::Entry::load() }.expect("a Vulkan loader on this box"); @@ -117,6 +120,30 @@ fn decodes_48_aus_with_status_reads_and_frame_releases_past_dpb_full() { .collect(); drop(families); // release the &mut borrows so video_props is readable + // Print each family's video ops + RESULT_STATUS query support — the + // context every failure report needs first (which mode the box runs and + // whether per-op status verdicts even exist here; RADV: they do not, + // and recording one hangs the VCN — the 2026-08 .25 lesson). + { + let mut status_props = + vec![vk::QueueFamilyQueryResultStatusPropertiesKHR::default(); family_count]; + let mut families2: Vec> = status_props + .iter_mut() + .map(|s| vk::QueueFamilyProperties2::default().push_next(s)) + .collect(); + // SAFETY: as the query above. + unsafe { instance.get_physical_device_queue_family_properties2(pd, &mut families2) }; + drop(families2); + for (i, s) in status_props.iter().enumerate() { + eprintln!( + "family {i}: flags={:?} video_ops={:?} query_result_status={}", + flags_per_family[i], + video_props[i].video_codec_operations, + s.query_result_status_support != vk::FALSE, + ); + } + } + let mut decode_qf = None; let mut graphics_qf = None; for (index, flags) in flags_per_family.iter().enumerate() { @@ -187,22 +214,34 @@ fn decodes_48_aus_with_status_reads_and_frame_releases_past_dpb_full() { let mut decoder = unsafe { VkH264Decoder::new(&handles, Box::new(NoopQueueLock)) } .expect("wrap the device"); - // 48 AUs — far past the vector's DPB depth, so evicted slots recycle - // repeatedly — with WP-C's one-in/one-out lifecycle on every delivered - // frame: wait its status (the program's whole point: the driver must - // say COMPLETE, per op) and release it so its slot may host a later - // decode. Output lags decode by a couple of AUs (B-pictures), so the - // delivered count is asserted with slack. + // 48 AUs — far past the vector's DPB depth (max_dpb_frames = 7), so DPB + // slots re-activate onto fresh pool images repeatedly — with the REAL + // client's consumption shape: the consumer HOLDS four delivered frames + // and releases only the oldest beyond that (its channels + preroll + + // in-flight present hold ~4-7). Status is read (COMPLETE required, the + // program's whole point) as each frame retires; `take_ready` is drained + // every AU so nothing is stranded. No presenter runs here, so releases + // report `presenter_signaled = false` (no `value+1` write-back). + const CLIENT_HOLD: usize = 4; let aus = split_into_aus(TEST_25FPS); + let mut held: std::collections::VecDeque = + std::collections::VecDeque::new(); let mut delivered = 0usize; let mut geometry_checked = false; - for au in aus.iter().take(48) { - let mut next = decoder - .decode(au) - .expect("decode an AU of the clean vector"); + for (index, au) in aus.iter().enumerate().take(48) { + let mut next = decoder.decode(au).unwrap_or_else(|e| { + panic!( + "AU {index}: decode failed: {e}\n state: {}", + decoder.debug_snapshot() + ) + }); while let Some(frame) = next { if !geometry_checked { - assert_eq!((frame.coded_width, frame.coded_height), (320, 240)); + assert_eq!( + (frame.coded_width, frame.coded_height), + (320, 240), + "ALLOCATED extent (320x240 needs no granularity padding here)" + ); assert_eq!( (frame.crop.width, frame.crop.height), (320, 240), @@ -213,18 +252,31 @@ fn decodes_48_aus_with_status_reads_and_frame_releases_past_dpb_full() { assert!(frame.value > 0); geometry_checked = true; } - assert_eq!( - decoder.wait_status(&frame), - DecodeStatus::Ok, - "the driver must report every decode op COMPLETE" - ); - decoder - .release_frame(&frame) - .expect("a current-generation frame releases"); + held.push_back(frame); delivered += 1; + // Steady state: keep CLIENT_HOLD frames in hand, retire beyond. + while held.len() > CLIENT_HOLD { + let oldest = held.pop_front().expect("nonempty"); + assert_eq!( + decoder.wait_status(&oldest), + DecodeStatus::Ok, + "AU {index}: decode op not COMPLETE\n state: {}", + decoder.debug_snapshot() + ); + decoder + .release_frame(&oldest, false) + .unwrap_or_else(|e| panic!("AU {index}: release failed: {e}")); + } next = decoder.take_ready(); } } + // Retire the tail the consumer still holds. + for frame in held.drain(..) { + assert_eq!(decoder.wait_status(&frame), DecodeStatus::Ok); + decoder + .release_frame(&frame, false) + .expect("tail frames release"); + } assert!( delivered >= 40, "expected at least 40 delivered frames from 48 AUs, got {delivered}"