diff --git a/crates/pf-vkdecode/src/caps.rs b/crates/pf-vkdecode/src/caps.rs index a857fa50..e42c89ac 100644 --- a/crates/pf-vkdecode/src/caps.rs +++ b/crates/pf-vkdecode/src/caps.rs @@ -494,7 +494,16 @@ fn require_mutable(entry: &VideoFormat, mode: &'static str) -> Result<(), CapsEr /// /// [`Self::wire`] links `profile.p_next` to this struct's OWN `h264` field; the /// value must not move between `wire()` and the last use of the returned reference -/// (the borrow checker pins it — `wire` borrows `self` for the reference's life). +/// — **or of any raw pointer taken from it**, which is the half that does not come +/// for free. `wire` borrows `self` for the reference's life, so wherever the +/// reference is passed on AS a reference the borrow checker does pin the chain: a +/// `profiles(std::slice::from_ref(profile))` builder carries the borrow in its own +/// lifetime parameter, and so does handing `profile` straight to an entry point. +/// Where a `*const` is taken instead, the borrow ENDS at that line and nothing but +/// inspection keeps the chain still — [`crate::decoder`]'s query pool must do +/// exactly that (`push_next` there would clobber the profile's own `p_next`), so it +/// holds the reference across the call in a helper's SIGNATURE +/// (`OpRing::create_status_query_pool`) rather than relying on this sentence. pub(crate) struct H264ProfileChain { h264: vk::VideoDecodeH264ProfileInfoKHR<'static>, profile: vk::VideoProfileInfoKHR<'static>, diff --git a/crates/pf-vkdecode/src/decoder.rs b/crates/pf-vkdecode/src/decoder.rs index 7dad434f..ac417aa5 100644 --- a/crates/pf-vkdecode/src/decoder.rs +++ b/crates/pf-vkdecode/src/decoder.rs @@ -496,17 +496,9 @@ impl OpRing { ) -> Result { let query_pool = if dev.result_status_queries() { let mut chain = decode_profile.chain(); - 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)? }) + // SAFETY: fn contract. `chain` outlives the call, and the helper's + // SIGNATURE — not a comment — is what keeps it immobile across it. + Some(unsafe { Self::create_status_query_pool(dev, chain.wire(), query_count)? }) } else { debug!( "decode family lacks queryResultStatusSupport — no per-op status \ @@ -554,6 +546,39 @@ impl OpRing { cmds, }) } + + /// The RESULT_STATUS query pool, created against `profile`. + /// + /// Split out for the BORROW rather than for tidiness. `VkQueryPoolCreateInfo` + /// has no codec-aware builder here — `push_next` would clobber the profile's + /// own `p_next` (its codec half), so the chain is written as a raw `*const`, and + /// a raw pointer ends the borrow the moment it is taken. Inline, only inspection + /// stopped a later edit from moving or dropping the chain between that write and + /// `vkCreateQueryPool`; taking `&vk::VideoProfileInfoKHR<'_>` as a PARAMETER + /// makes the compiler hold the borrow across the whole call instead + /// ([`crate::caps::H264ProfileChain`]'s contract, which used to claim the + /// borrow checker covered this site and did not). + /// + /// # Safety + /// + /// `dev` wraps live handles ([`DeviceHandles`] contract). + unsafe fn create_status_query_pool( + dev: &DecodeDevice, + profile: &vk::VideoProfileInfoKHR<'_>, + query_count: u32, + ) -> Result { + 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` + // — the encoder's exact precedent. + query_ci.p_next = std::ptr::from_ref(profile).cast(); + // SAFETY: fn contract; `query_ci` roots the wired chain for the call, and + // `profile` is borrowed for the whole of this body so the chain cannot move + // out from under that pointer. The video profile chained in satisfies the + // "same profile as the session" rule for queries used inside a coding scope. + unsafe { dev.ash().create_query_pool(&query_ci, None) } + } } impl Drop for OpRing { diff --git a/crates/pf-vkdecode/src/params.rs b/crates/pf-vkdecode/src/params.rs index 1f15efed..522ab0ab 100644 --- a/crates/pf-vkdecode/src/params.rs +++ b/crates/pf-vkdecode/src/params.rs @@ -77,7 +77,14 @@ impl std::error::Error for ParamsError {} /// /// - The backing is boxed, so the wrapper may be MOVED freely: moving it relocates /// the `Box` handles (pointer values), never the heap blocks the Std struct's -/// pointers hold the addresses of. +/// pointers hold the addresses of. ⚠ The Std struct ITSELF is boxed for the same +/// reason and it is not decoration: `pStdSPSs` — the OUTER pointer the create/add +/// info carries — is [`Self::std`]'s address, and the session hands it over +/// BEFORE moving the wrapper into its stored parameters. Inline, that address +/// would be a moved-from slot; boxed, it is the one the object keeps +/// (`crate::session`'s `an_added_set_keeps_the_address_the_update_call_was_given`). +/// A driver retaining the outer pointer rather than an inner one would otherwise +/// reproduce the AV1 use-after-free exactly, with the same silent signature. /// - [`Self::std`] hands the struct out by shared reference. The struct is `Copy`; a /// copy taken out of the wrapper still points INTO the wrapper's backing and must /// not outlive it. ⚠⚠ The obligation is NOT merely "keep the wrapper alive across @@ -93,7 +100,9 @@ impl std::error::Error for ParamsError {} /// Re-convert from the `Sps` instead — conversion is cheap and pure. #[derive(Debug)] pub struct OwnedStdSps { - std: hh::StdVideoH264SequenceParameterSet, + /// Boxed so [`Self::std`]'s ADDRESS — what `pStdSPSs` points at — survives every + /// move of the wrapper (type-level contract). + std: Box, /// `pOffsetForRefFrame`'s target (POC type 1 only, else `None`/null). _offset_backing: Option>, /// `pScalingLists`' target (`seq_scaling_matrix_present_flag` only, else null). @@ -112,7 +121,8 @@ impl OwnedStdSps { /// Same ownership contract as [`OwnedStdSps`], with the one pointer. #[derive(Debug)] pub struct OwnedStdPps { - std: hh::StdVideoH264PictureParameterSet, + /// Boxed for [`OwnedStdSps`]'s reason: `pStdPPSs` is this field's address. + std: Box, _scaling_backing: Option>, } @@ -290,7 +300,7 @@ pub fn sps_to_std(sps: &Sps) -> Result { } Ok(OwnedStdSps { - std, + std: Box::new(std), _offset_backing: offset_backing, _scaling_backing: scaling_backing, }) @@ -368,7 +378,7 @@ pub fn pps_to_std(pps: &Pps) -> Result { } Ok(OwnedStdPps { - std, + std: Box::new(std), _scaling_backing: scaling_backing, }) } diff --git a/crates/pf-vkdecode/src/params_av1.rs b/crates/pf-vkdecode/src/params_av1.rs index 9c72fb4f..1257d54c 100644 --- a/crates/pf-vkdecode/src/params_av1.rs +++ b/crates/pf-vkdecode/src/params_av1.rs @@ -69,9 +69,17 @@ impl std::error::Error for ParamsAv1Error {} /// where the measurement lives. Boxed backing (rather than inline arrays) is what /// makes storing the wrapper enough — moving it does not move the blocks, which /// `moving_the_wrapper_leaves_the_driver_s_pointers_put` pins. +/// +/// ⚠⚠ The Std struct ITSELF is boxed for the same reason, one level out: +/// `pStdSequenceHeader` is [`Self::std`]'s address, and `ensure_parameters` hands +/// it to the create call BEFORE moving the wrapper into the stored parameters. +/// Inline, that address would be a moved-from stack slot the instant the function +/// returned — the original bug's exact shape, differing only in WHICH pointer a +/// driver chose to retain (`session_av1`'s +/// `the_sequence_header_address_the_create_call_is_given_survives_being_stored`). #[derive(Debug)] pub struct OwnedStdAv1SequenceHeader { - std: hh::StdVideoAV1SequenceHeader, + std: Box, _color_backing: Box, /// `pTimingInfo` is null unless the stream carries timing info: a decoder needs /// none of it, and a zeroed block behind a non-null pointer would claim a frame @@ -213,7 +221,7 @@ pub fn sequence_to_std( .map_or(std::ptr::null(), |t| &**t as *const _); Ok(OwnedStdAv1SequenceHeader { - std, + std: Box::new(std), _color_backing: color_backing, _timing_backing: timing_backing, }) diff --git a/crates/pf-vkdecode/src/params_h265.rs b/crates/pf-vkdecode/src/params_h265.rs index 22635041..d96ecc24 100644 --- a/crates/pf-vkdecode/src/params_h265.rs +++ b/crates/pf-vkdecode/src/params_h265.rs @@ -189,7 +189,9 @@ where /// no mutation, deliberately not `Clone` (re-convert instead). #[derive(Debug)] pub struct OwnedStdH265Vps { - std: hh::StdVideoH265VideoParameterSet, + /// Boxed so [`Self::std`]'s ADDRESS — what `pStdVPSs` points at — survives every + /// move of the wrapper ([`crate::OwnedStdSps`]). + std: Box, _ptl_backing: Box, _dpb_backing: Box, } @@ -212,7 +214,8 @@ impl OwnedStdH265Vps { /// rejected, not dropped). #[derive(Debug)] pub struct OwnedStdH265Sps { - std: hh::StdVideoH265SequenceParameterSet, + /// Boxed for [`OwnedStdH265Vps`]'s reason: `pStdSPSs` is this field's address. + std: Box, _ptl_backing: Box, _dpb_backing: Box, _scaling_backing: Option>, @@ -232,7 +235,8 @@ impl OwnedStdH265Sps { /// targets. Same ownership contract as [`crate::OwnedStdSps`]. #[derive(Debug)] pub struct OwnedStdH265Pps { - std: hh::StdVideoH265PictureParameterSet, + /// Boxed for [`OwnedStdH265Vps`]'s reason: `pStdPPSs` is this field's address. + std: Box, _scaling_backing: Option>, } @@ -474,7 +478,7 @@ pub fn vps_to_std_h265(vps: &Vps) -> Result { std.pProfileTierLevel = &*ptl_backing; Ok(OwnedStdH265Vps { - std, + std: Box::new(std), _ptl_backing: ptl_backing, _dpb_backing: dpb_backing, }) @@ -507,7 +511,7 @@ pub fn fallback_vps_from_sps(sps: &Sps) -> Result Result { // docs / check_envelope). Ok(OwnedStdH265Sps { - std, + std: Box::new(std), _ptl_backing: ptl_backing, _dpb_backing: dpb_backing, _scaling_backing: scaling_backing, @@ -903,7 +907,7 @@ pub fn pps_to_std_h265(pps: &Pps) -> Result { // pPredictorPaletteEntries stays null (rejected above). Ok(OwnedStdH265Pps { - std, + std: Box::new(std), _scaling_backing: scaling_backing, }) } diff --git a/crates/pf-vkdecode/src/session.rs b/crates/pf-vkdecode/src/session.rs index c55717c3..773222ad 100644 --- a/crates/pf-vkdecode/src/session.rs +++ b/crates/pf-vkdecode/src/session.rs @@ -26,6 +26,18 @@ //! lifetime, and both the recreate path and `Drop` destroy the object before that //! value is released. //! +//! **Both LEVELS of pointer are covered, not just the one the measurement caught.** +//! Fixing the inner pointers left the OUTER ones — `pStdSPSs`/`pStdPPSs`, and AV1's +//! `pStdSequenceHeader` — still addressing function locals, and a driver retaining +//! those instead would reproduce the same bug with the same silent signature. So the +//! Std structs are boxed inside their wrappers ([`crate::OwnedStdSps`]) and the +//! contiguous arrays are FIELDS of `StoredParams`: no address the driver is given +//! is a temporary's. The line is drawn at Std DATA — `VkVideoSessionParametersCreateInfoKHR` +//! and its `pNext`/`pParametersAddInfo` plumbing stay function-local, because those +//! are ordinary create-info structures every `vkCreate*` in Vulkan reads during the +//! call; it is the `pStd*` members whose retention the spec's wording left ambiguous +//! and this fleet was measured exercising. +//! //! [`ParamsLedger`] is the pure half of that decision table (unit-tested); //! [`VideoSession`] is the thin Vulkan half. @@ -366,18 +378,47 @@ struct StoredParams { /// blocks they own. sps: Vec, pps: Vec, + /// The contiguous Std ARRAYS the create call was handed as `pStdSPSs`/`pStdPPSs` + /// — the OUTER pointers, held for the object's life for the reason the wrappers + /// are. They were function-local `Vec`s, dropped the moment + /// [`VideoSession::create_parameters_object`] returned; nothing but the spec's + /// wording said a driver may not keep them, and that wording is what the AV1 + /// measurement already disproved for the pointers one level in. Built by + /// [`Self::assemble`] at their final address, so the pointer the driver is given + /// never moves at all. + std_sps: Vec, + std_pps: Vec, } impl StoredParams { + /// The wrappers plus the contiguous Std arrays the create call reads its + /// `pStdSPSs`/`pStdPPSs` out of, with a NULL object the caller fills in once + /// `vkCreateVideoSessionParametersKHR` has succeeded. + /// + /// Assembling BEFORE the call is the point: the arrays are copies of the + /// wrappers' Std structs, and building them here puts them at the address they + /// will keep for the object's whole life rather than in a temporary the call + /// outlives. + fn assemble(sps: Vec, pps: Vec) -> Self { + // COPIES of each wrapper's Std struct (it is `Copy`); the embedded pointers + // they carry still address the wrappers' own boxed blocks, which is why + // both halves have to be kept. + let std_sps = sps.iter().map(|o| *o.std()).collect(); + let std_pps = pps.iter().map(|o| *o.std()).collect(); + Self { + object: vk::VideoSessionParametersKHR::null(), + sps, + pps, + std_sps, + std_pps, + } + } + /// The placeholder a half-built session holds. `vkDestroyVideoSessionParametersKHR` /// ignores a NULL handle, so a [`VideoSession::create`] that fails before the /// object exists still drops cleanly. fn none() -> Self { - Self { - object: vk::VideoSessionParametersKHR::null(), - sps: Vec::new(), - pps: Vec::new(), - } + Self::assemble(Vec::new(), Vec::new()) } /// Take over sets an `Add` just handed to the live object — they belong to the @@ -490,17 +531,13 @@ impl VideoSession { sps: Vec, pps: Vec, ) -> Result { - // The contiguous Std arrays the call wants. These are COPIES of each - // wrapper's Std struct (it is `Copy`, and the driver copies them again - // before returning); the embedded pointers they carry still address the - // wrappers' own boxed blocks, which are what must outlive the OBJECT. - let std_sps: Vec = - sps.iter().map(|o| *o.std()).collect(); - let std_pps: Vec = - pps.iter().map(|o| *o.std()).collect(); + // Assembled FIRST so the arrays `pStdSPSs`/`pStdPPSs` will point at are + // already where they will stay: `stored` is returned by value, and moving a + // `Vec` moves its handle, not the block the driver was given. + let mut stored = StoredParams::assemble(sps, pps); let add = vk::VideoDecodeH264SessionParametersAddInfoKHR::default() - .std_sp_ss(&std_sps) - .std_pp_ss(&std_pps); + .std_sp_ss(&stored.std_sps) + .std_pp_ss(&stored.std_pps); let mut h264 = vk::VideoDecodeH264SessionParametersCreateInfoKHR::default() .max_std_sps_count(MAX_STD_SPS as u32) .max_std_pps_count(MAX_STD_PPS as u32) @@ -509,9 +546,10 @@ impl VideoSession { .video_session(self.session) .push_next(&mut h264); let mut object = vk::VideoSessionParametersKHR::null(); - // SAFETY: fn contract; `ci` roots locals outliving the call, and the blocks - // the driver may retain past it are owned by `sps`/`pps`, which are moved - // into the returned value rather than dropped here. + // SAFETY: fn contract; `ci` roots locals outliving the call, and everything + // the driver may retain past it — the Std arrays AND the blocks their + // embedded pointers address — is owned by `stored`, which is returned + // rather than dropped here. let r = unsafe { (self.video_queue.fp().create_video_session_parameters_khr)( self.device.handle(), @@ -523,7 +561,8 @@ impl VideoSession { if r != vk::Result::SUCCESS { return Err(SessionError::Vk(r)); } - Ok(StoredParams { object, sps, pps }) + stored.object = object; + Ok(stored) } /// The ledger's verdict for activating (`sps`, `pps`), without mutating @@ -776,6 +815,107 @@ mod tests { assert_eq!(read_back, [0; 16], "the fixture's lists, read back live"); } + /// …and it keeps the ADDRESS too, not merely the blocks. + /// + /// The Add path hands `vkUpdateVideoSessionParametersKHR` a + /// `std::slice::from_ref(o.std())` — a one-element array that IS the wrapper's + /// own Std struct — and then moves the wrapper into [`StoredParams`]. The test + /// above covers a driver retaining `pScalingLists` (an INNER pointer); this + /// covers one retaining `pStdSPSs`/`pStdPPSs`, which the same wording in the + /// spec permits just as much. Boxing the Std struct inside the wrapper is what + /// makes the two addresses equal; un-boxing it would leave every other test in + /// this crate green and hand the driver a moved-from stack slot. + #[test] + fn an_added_set_keeps_the_address_the_update_call_was_given() { + let (sps, pps) = authored(0, 0, 26); + let owned_sps = sps_to_std(&sps).expect("converts"); + let owned_pps = pps_to_std(&pps).expect("converts"); + // Exactly what `ensure_parameters` puts in `pStdSPSs`/`pStdPPSs`. + let handed_sps = std::ptr::from_ref(owned_sps.std()); + let handed_pps = std::ptr::from_ref(owned_pps.std()); + + let mut stored = StoredParams::none(); + stored.adopt(Some(owned_sps), Some(owned_pps)); + assert_eq!( + std::ptr::from_ref(stored.sps[0].std()), + handed_sps, + "the SPS address handed to Vulkan must be the one the object keeps" + ); + assert_eq!( + std::ptr::from_ref(stored.pps[0].std()), + handed_pps, + "and likewise the PPS" + ); + // SAFETY: `stored` owns both structs — which is exactly the property here. + let ids = unsafe { + ( + (*handed_sps).seq_parameter_set_id, + (*handed_pps).pic_parameter_set_id, + ) + }; + assert_eq!( + ids, + (0, 0), + "read back through the pointers the driver holds" + ); + } + + /// The create path's OUTER pointers: `pStdSPSs`/`pStdPPSs` address contiguous + /// COPIES of the wrappers' Std structs, and those arrays must outlive the + /// create call the same way the wrappers do. + /// + /// [`StoredParams::assemble`] builds them at their final address — inside the + /// value the parameters object is returned in — so the pointer the driver is + /// given never moves at all. Before this, they were function-local `Vec`s that + /// were dropped the instant `create_parameters_object` returned: a driver + /// retaining the array (rather than the embedded pointer this fleet was + /// measured retaining) would have been reading freed heap from the first frame. + #[test] + fn the_std_arrays_the_create_call_is_given_are_the_ones_the_object_keeps() { + let sps = SpsBuilder::new() + .seq_parameter_set_id(0) + .profile_idc(Profile::Main) + .level_idc(Level::L4) + .frame_mbs_only_flag(true) + .direct_8x8_inference_flag(true) + .max_num_ref_frames(4) + .resolution(64, 64) + // So the copied Std struct carries a NON-null embedded pointer and the + // "still addresses the wrapper's live block" assertion below can bite. + .seq_scaling_matrix_present_flag(true) + .build(); + let pps = PpsBuilder::new(Rc::clone(&sps)) + .pic_parameter_set_id(0) + .pic_init_qp(26) + .build(); + let owned_sps = sps_to_std(&sps).expect("converts"); + let owned_pps = pps_to_std(&pps).expect("converts"); + + let stored = StoredParams::assemble(vec![owned_sps], vec![owned_pps]); + // Exactly what `create_parameters_object` puts in `pStdSPSs`/`pStdPPSs`. + let (handed_sps, handed_pps) = (stored.std_sps.as_ptr(), stored.std_pps.as_ptr()); + // The move `create_parameters_object` ends with: `Ok(stored)`. + let stored = std::hint::black_box(stored); + assert_eq!((stored.std_sps.len(), stored.std_pps.len()), (1, 1)); + assert_eq!( + (stored.std_sps.as_ptr(), stored.std_pps.as_ptr()), + (handed_sps, handed_pps), + "the arrays handed to Vulkan must be the ones the object keeps" + ); + // And their COPIES still address the wrappers' own live blocks. + let lists = stored.std_sps[0].pScalingLists; + assert!(!lists.is_null(), "the fixture attaches scaling lists"); + assert_eq!(lists, stored.sps[0].std().pScalingLists); + // SAFETY: `stored` owns the block the copy points at — the property here. + let read_back = unsafe { (*lists).ScalingList4x4[0] }; + assert_eq!(read_back, [0; 16], "the fixture's lists, read back live"); + assert_eq!( + stored.std_pps[0].pic_parameter_set_id, + stored.pps[0].std().pic_parameter_set_id, + "the PPS copy is the wrapper's, field for field" + ); + } + #[test] fn a_reactivated_identical_pair_is_current_even_across_reparses() { let (sps_a, pps_a) = authored(0, 0, 26); diff --git a/crates/pf-vkdecode/src/session_av1.rs b/crates/pf-vkdecode/src/session_av1.rs index 7459806b..b5144a7a 100644 --- a/crates/pf-vkdecode/src/session_av1.rs +++ b/crates/pf-vkdecode/src/session_av1.rs @@ -37,6 +37,14 @@ //! entirely. That is the whole of the AV1 rung's parity gap (250/250 frames //! divergent; 0/250 with the backing held, [`StoredParamsAv1`]). //! +//! ⚠⚠ That fix stabilised the INNER pointers only. `pStdSequenceHeader` — the +//! address `VkVideoDecodeAV1SessionParametersCreateInfoKHR` itself carries — was +//! still [`sequence_to_std`]'s stack local, dead the moment `ensure_parameters` +//! returned, and a driver retaining IT rather than `pColorConfig` would reproduce +//! the bug exactly. The Std struct is now boxed inside the wrapper, so the address +//! handed over is the one `StoredParamsAv1` keeps ([`crate::session`]'s module +//! docs carry the argument and the line it draws). +//! //! `ParamsLedgerAv1` is the pure half of the decision (unit-tested); //! [`VideoSessionAv1`] is the thin Vulkan half. @@ -124,7 +132,9 @@ pub struct SessionConfigAv1 { struct StoredParamsAv1 { object: vk::VideoSessionParametersKHR, /// Held for the OBJECT's whole life. Never read by this crate after the - /// create call; the DRIVER reads it. + /// create call; the DRIVER reads it — potentially through `pStdSequenceHeader` + /// itself, which is why the wrapper boxes its Std struct rather than holding it + /// inline (module docs). _sequence: OwnedStdAv1SequenceHeader, } @@ -376,6 +386,44 @@ mod tests { }) } + /// The OUTER pointer — `pStdSequenceHeader` itself — must still address the + /// stored wrapper's Std struct after the wrapper has been moved into + /// [`StoredParamsAv1`]. + /// + /// [`crate::params_av1`]'s `moving_the_wrapper_leaves_the_driver_s_pointers_put` + /// pins the INNER pointers (`pColorConfig`, `pTimingInfo`); this pins the one + /// the create info carries. `ensure_parameters` converts into a local, hands + /// `owned.std()` to `vkCreateVideoSessionParametersKHR`, and only THEN moves + /// the wrapper into the stored value — so a driver retaining + /// `pStdSequenceHeader` (rather than the `pColorConfig` this fleet was measured + /// retaining) would read a moved-from slot, with the same silent signature as + /// the original bug: plausible pictures, wrong content, no error and no + /// counter. Boxing the Std struct inside the wrapper is what makes the two + /// addresses equal, and this is the assertion that stops it being un-boxed. + #[test] + fn the_sequence_header_address_the_create_call_is_given_survives_being_stored() { + let seq = authored(1919, false); + let owned = sequence_to_std(&seq).expect("a plain 8-bit header converts"); + // Exactly what `ensure_parameters` puts in `pStdSequenceHeader`. + let handed = std::ptr::from_ref(owned.std()); + // `ensure_parameters`' own final move: `self.parameters = Some(…)`. + let parameters = Some(StoredParamsAv1 { + object: vk::VideoSessionParametersKHR::null(), + _sequence: owned, + }); + let Some(stored) = parameters else { + unreachable!("just installed") + }; + assert_eq!( + std::ptr::from_ref(stored._sequence.std()), + handed, + "the address handed to Vulkan must be the address the object keeps" + ); + // SAFETY: `stored` owns the header — which is exactly the property here. + let width = unsafe { (*handed).max_frame_width_minus_1 }; + assert_eq!(width, 1919, "the fixture's width, read back through it"); + } + #[test] fn the_first_activation_recreates_because_there_is_no_empty_parameters_object() { let seq = authored(1919, false); diff --git a/crates/pf-vkdecode/src/session_h265.rs b/crates/pf-vkdecode/src/session_h265.rs index f6c3a40a..345af934 100644 --- a/crates/pf-vkdecode/src/session_h265.rs +++ b/crates/pf-vkdecode/src/session_h265.rs @@ -38,7 +38,9 @@ //! carries the measurement). H.265 embeds MORE such pointers than any other codec //! here — the SPS alone carries seven — so [`StoredParamsH265`] holds the object and //! its backings in ONE value with one lifetime, and both the recreate path and -//! `Drop` destroy the object before that value is released. +//! `Drop` destroy the object before that value is released. The OUTER pointers +//! (`pStdVPSs`/`pStdSPSs`/`pStdPPSs`) are held the same way and for the same reason +//! — [`crate::session`]'s module docs carry the argument and the line it draws. //! //! `ParamsLedgerH265` is the pure half of that decision table (unit-tested); //! `VideoSessionH265` is the thin Vulkan half. @@ -289,19 +291,47 @@ struct StoredParamsH265 { vps: Vec, sps: Vec, pps: Vec, + /// The contiguous Std ARRAYS the create call was handed as + /// `pStdVPSs`/`pStdSPSs`/`pStdPPSs` — the OUTER pointers, held for the object's + /// life for the reason the wrappers are ([`crate::session`]'s `StoredParams` + /// carries the argument). Built by [`Self::assemble`] at their final address. + std_vps: Vec, + std_sps: Vec, + std_pps: Vec, } impl StoredParamsH265 { + /// The wrappers plus the contiguous Std arrays the create call reads its + /// `pStdVPSs`/`pStdSPSs`/`pStdPPSs` out of, with a NULL object the caller fills + /// in once `vkCreateVideoSessionParametersKHR` has succeeded + /// ([`crate::session`]'s `StoredParams::assemble` for why it happens here). + fn assemble( + vps: Vec, + sps: Vec, + pps: Vec, + ) -> Self { + // COPIES of each wrapper's Std struct (it is `Copy`); the embedded pointers + // they carry still address the wrappers' own boxed blocks, which is why + // both halves have to be kept. + let std_vps = vps.iter().map(|o| *o.std()).collect(); + let std_sps = sps.iter().map(|o| *o.std()).collect(); + let std_pps = pps.iter().map(|o| *o.std()).collect(); + Self { + object: vk::VideoSessionParametersKHR::null(), + vps, + sps, + pps, + std_vps, + std_sps, + std_pps, + } + } + /// The placeholder a half-built session holds. `vkDestroyVideoSessionParametersKHR` /// ignores a NULL handle, so a [`VideoSessionH265::create`] that fails before /// the object exists still drops cleanly. fn none() -> Self { - Self { - object: vk::VideoSessionParametersKHR::null(), - vps: Vec::new(), - sps: Vec::new(), - pps: Vec::new(), - } + Self::assemble(Vec::new(), Vec::new(), Vec::new()) } /// Take over sets an `Add` just handed to the live object — they belong to the @@ -420,20 +450,14 @@ impl VideoSessionH265 { sps: Vec, pps: Vec, ) -> Result { - // The contiguous Std arrays the call wants. These are COPIES of each - // wrapper's Std struct (it is `Copy`, and the driver copies them again - // before returning); the embedded pointers they carry still address the - // wrappers' own boxed blocks, which are what must outlive the OBJECT. - let std_vps: Vec = - vps.iter().map(|o| *o.std()).collect(); - let std_sps: Vec = - sps.iter().map(|o| *o.std()).collect(); - let std_pps: Vec = - pps.iter().map(|o| *o.std()).collect(); + // Assembled FIRST so the arrays `pStdVPSs`/`pStdSPSs`/`pStdPPSs` will point + // at are already where they will stay: `stored` is returned by value, and + // moving a `Vec` moves its handle, not the block the driver was given. + let mut stored = StoredParamsH265::assemble(vps, sps, pps); let add = vk::VideoDecodeH265SessionParametersAddInfoKHR::default() - .std_vp_ss(&std_vps) - .std_sp_ss(&std_sps) - .std_pp_ss(&std_pps); + .std_vp_ss(&stored.std_vps) + .std_sp_ss(&stored.std_sps) + .std_pp_ss(&stored.std_pps); let mut h265 = vk::VideoDecodeH265SessionParametersCreateInfoKHR::default() .max_std_vps_count(MAX_STD_VPS as u32) .max_std_sps_count(MAX_STD_SPS as u32) @@ -443,9 +467,10 @@ impl VideoSessionH265 { .video_session(self.session) .push_next(&mut h265); let mut object = vk::VideoSessionParametersKHR::null(); - // SAFETY: fn contract; `ci` roots locals outliving the call, and the blocks - // the driver may retain past it are owned by `vps`/`sps`/`pps`, which are - // moved into the returned value rather than dropped here. + // SAFETY: fn contract; `ci` roots locals outliving the call, and everything + // the driver may retain past it — the Std arrays AND the blocks their + // embedded pointers address — is owned by `stored`, which is returned + // rather than dropped here. let r = unsafe { (self.video_queue.fp().create_video_session_parameters_khr)( self.device.handle(), @@ -457,12 +482,8 @@ impl VideoSessionH265 { if r != vk::Result::SUCCESS { return Err(SessionError::Vk(r)); } - Ok(StoredParamsH265 { - object, - vps, - sps, - pps, - }) + stored.object = object; + Ok(stored) } /// The ledger's verdict for activating (`vps`, `sps`, `pps`), without mutating @@ -771,6 +792,112 @@ mod tests { assert_eq!(dpb, 5, "the fixture's DPB sizing, read back live"); } + /// …and it keeps the ADDRESS too, not merely the blocks. + /// + /// The Add path hands `vkUpdateVideoSessionParametersKHR` a + /// `std::slice::from_ref(o.std())` — a one-element array that IS the wrapper's + /// own Std struct — and then moves the wrapper into [`StoredParamsH265`]. The + /// test above covers a driver retaining `pDecPicBufMgr` (an INNER pointer); + /// this covers one retaining `pStdVPSs`/`pStdSPSs`/`pStdPPSs`, which the same + /// wording in the spec permits just as much. + #[test] + fn an_added_set_keeps_the_address_the_update_call_was_given() { + let mut base = (*authored_sps(0, 0, 64)).clone(); + base.profile_tier_level.general_profile_idc = 1; // Main + let sps = Rc::new(base); + let pps = authored_pps(&sps, 0, 0); + let owned_vps = VpsSource::for_sps(&sps).to_std().expect("converts"); + let owned_sps = sps_to_std_h265(&sps).expect("converts"); + let owned_pps = pps_to_std_h265(&pps).expect("converts"); + // Exactly what `ensure_parameters` puts in `pStdVPSs`/`pStdSPSs`/`pStdPPSs`. + let handed_vps = std::ptr::from_ref(owned_vps.std()); + let handed_sps = std::ptr::from_ref(owned_sps.std()); + let handed_pps = std::ptr::from_ref(owned_pps.std()); + + let mut stored = StoredParamsH265::none(); + stored.adopt(Some(owned_vps), Some(owned_sps), Some(owned_pps)); + assert_eq!( + ( + std::ptr::from_ref(stored.vps[0].std()), + std::ptr::from_ref(stored.sps[0].std()), + std::ptr::from_ref(stored.pps[0].std()), + ), + (handed_vps, handed_sps, handed_pps), + "the addresses handed to Vulkan must be the ones the object keeps" + ); + // SAFETY: `stored` owns all three — which is exactly the property here. + let ids = unsafe { + ( + (*handed_vps).vps_video_parameter_set_id, + (*handed_sps).sps_seq_parameter_set_id, + (*handed_pps).pps_pic_parameter_set_id, + ) + }; + assert_eq!(ids, (0, 0, 0), "read back through the driver's pointers"); + } + + /// The create path's OUTER pointers: `pStdVPSs`/`pStdSPSs`/`pStdPPSs` address + /// contiguous COPIES of the wrappers' Std structs, and those arrays must + /// outlive the create call the same way the wrappers do. + /// + /// [`StoredParamsH265::assemble`] builds them at their final address — inside + /// the value the parameters object is returned in — so the pointer the driver + /// is given never moves. Before this they were function-local `Vec`s, dropped + /// the instant `create_parameters_object` returned. + #[test] + fn the_std_arrays_the_create_call_is_given_are_the_ones_the_object_keeps() { + let mut base = (*authored_sps(0, 0, 64)).clone(); + base.profile_tier_level.general_profile_idc = 1; // Main + let sps = Rc::new(base); + let pps = authored_pps(&sps, 0, 0); + let owned_vps = VpsSource::for_sps(&sps).to_std().expect("converts"); + let owned_sps = sps_to_std_h265(&sps).expect("converts"); + let owned_pps = pps_to_std_h265(&pps).expect("converts"); + + let stored = StoredParamsH265::assemble(vec![owned_vps], vec![owned_sps], vec![owned_pps]); + // Exactly what `create_parameters_object` hands the create call. + let handed = ( + stored.std_vps.as_ptr(), + stored.std_sps.as_ptr(), + stored.std_pps.as_ptr(), + ); + // The move `create_parameters_object` ends with: `Ok(stored)`. + let stored = std::hint::black_box(stored); + assert_eq!( + ( + stored.std_vps.len(), + stored.std_sps.len(), + stored.std_pps.len() + ), + (1, 1, 1) + ); + assert_eq!( + ( + stored.std_vps.as_ptr(), + stored.std_sps.as_ptr(), + stored.std_pps.as_ptr(), + ), + handed, + "the arrays handed to Vulkan must be the ones the object keeps" + ); + // And their COPIES still address the wrappers' own live blocks. + let (ptl, dpb) = ( + stored.std_vps[0].pProfileTierLevel, + stored.std_sps[0].pDecPicBufMgr, + ); + assert!(!ptl.is_null() && !dpb.is_null(), "both are always attached"); + assert_eq!(ptl, stored.vps[0].std().pProfileTierLevel); + assert_eq!(dpb, stored.sps[0].std().pDecPicBufMgr); + // SAFETY: `stored` owns both blocks — which is exactly the property here. + let profile_idc = unsafe { (*ptl).general_profile_idc }; + assert_eq!(profile_idc, 1, "the fixture's Main profile, read back live"); + assert_eq!( + stored.std_pps[0].pps_pic_parameter_set_id, + stored.pps[0].std().pps_pic_parameter_set_id, + "the PPS copy is the wrapper's, field for field" + ); + } + #[test] fn the_first_activation_adds_all_three_sets_in_one_update_call() { let sps = with_vps(&authored_sps(0, 0, 64), 0, 0);