fix(vkdecode): the address the driver keeps is now the address we keep
The AV1 use-after-free fix (cdd1f3ef) stabilised the wrong half. NVIDIA was
measured retaining pColorConfig, so StoredParamsAv1 boxed the colour and timing
blocks — but OwnedStdAv1SequenceHeader kept the Std struct ITSELF inline, so the
pStdSequenceHeader we handed vkCreateVideoSessionParametersKHR was a stack
address inside ensure_parameters, dead the moment it returned. The fix worked
because of WHICH pointer that driver happened to hold. A driver retaining the
outer one instead — no more of a spec violation than retaining pColorConfig was —
reproduces the original bug exactly: plausible pictures, wrong content, no error
and no counter moved.
The same shape was in the shipping codecs, one step further from evidence: the
H.264 and H.265 create paths pointed pStdSPSs/pStdPPSs/pStdVPSs at function-local
Vecs, and both Add paths handed over the wrapper's inline std field and then moved
the wrapper. Those are spec-legal — the object stores copies — and have never
misbehaved on the fleet. They are fixed anyway, because that is precisely what was
true of H.264/H.265 before the same class of bug was found in them, and a
correctness argument that reduces to which vendor we tested is not one.
So: the Std struct is boxed inside each owning wrapper (one level out from what
_color_backing already did), and the contiguous create-time arrays are now fields
of the stored parameters, assembled at their final address. Identical bytes at
identical offsets — only where they live changed.
The line drawn deliberately, in prose at session.rs:29: Std DATA is pinned; the
VkVideoSessionParametersCreateInfoKHR chain itself is not. Retention there would
be a different and far more extreme class of driver bug, and pinning it needs a
self-referential struct over lifetime-parameterised builders.
⚠ NOT hardware-verified. No GPU has run this — the fleet is unreachable and the
250/250 parity that proved this code bit-exact cannot be re-run. That is why the
change is constrained to address stability alone, and why it ships five CPU-only
tests instead: three capture the pointer handed to Vulkan, perform the real move,
and assert it survives — each verified FAILING first, with genuinely differing
addresses, not a tautology. Two more pin the create-array ownership; those fail
before the fix as compile errors rather than assertions, because the pre-fix bug
there is a dangling pointer and asserting on it is UB.
Also: caps.rs claimed the borrow checker pins a profile chain between wire() and
its last use. False at exactly one site — decoder.rs took a raw *const, ending the
borrow, leaving nothing but inspection to stop a future editor moving the chain
before create_query_pool. Correct today, guarded by prose, which is how the first
bug shipped. It is now compiler-enforced: the pointer write and the create call
live inside one helper that takes the profile by reference, so the borrow is held
across both by the signature. An audit cleared the chains otherwise — no entry
point we pass one to retains it.
Gates: fmt clean; clippy -D warnings over pf-vkdecode AND pf-client-core in the
Linux container (its only real consumer, which cannot build on macOS at all —
wol.rs uses deps its manifest gates to linux/windows, so workspace clippy has
never passed there and does not now); 187 lib tests green on Linux, up from 182.
This commit is contained in:
@@ -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
|
/// [`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
|
/// 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 {
|
pub(crate) struct H264ProfileChain {
|
||||||
h264: vk::VideoDecodeH264ProfileInfoKHR<'static>,
|
h264: vk::VideoDecodeH264ProfileInfoKHR<'static>,
|
||||||
profile: vk::VideoProfileInfoKHR<'static>,
|
profile: vk::VideoProfileInfoKHR<'static>,
|
||||||
|
|||||||
@@ -496,17 +496,9 @@ impl OpRing {
|
|||||||
) -> Result<Self, vk::Result> {
|
) -> Result<Self, vk::Result> {
|
||||||
let query_pool = if dev.result_status_queries() {
|
let query_pool = if dev.result_status_queries() {
|
||||||
let mut chain = decode_profile.chain();
|
let mut chain = decode_profile.chain();
|
||||||
let profile = chain.wire();
|
// SAFETY: fn contract. `chain` outlives the call, and the helper's
|
||||||
let mut query_ci = vk::QueryPoolCreateInfo::default()
|
// SIGNATURE — not a comment — is what keeps it immobile across it.
|
||||||
.query_type(vk::QueryType::RESULT_STATUS_ONLY_KHR)
|
Some(unsafe { Self::create_status_query_pool(dev, chain.wire(), query_count)? })
|
||||||
.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 {
|
} else {
|
||||||
debug!(
|
debug!(
|
||||||
"decode family lacks queryResultStatusSupport — no per-op status \
|
"decode family lacks queryResultStatusSupport — no per-op status \
|
||||||
@@ -554,6 +546,39 @@ impl OpRing {
|
|||||||
cmds,
|
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<vk::QueryPool, vk::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 {
|
impl Drop for OpRing {
|
||||||
|
|||||||
@@ -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 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
|
/// 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
|
/// - [`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
|
/// 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
|
/// 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.
|
/// Re-convert from the `Sps` instead — conversion is cheap and pure.
|
||||||
#[derive(Debug)]
|
#[derive(Debug)]
|
||||||
pub struct OwnedStdSps {
|
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<hh::StdVideoH264SequenceParameterSet>,
|
||||||
/// `pOffsetForRefFrame`'s target (POC type 1 only, else `None`/null).
|
/// `pOffsetForRefFrame`'s target (POC type 1 only, else `None`/null).
|
||||||
_offset_backing: Option<Box<[i32]>>,
|
_offset_backing: Option<Box<[i32]>>,
|
||||||
/// `pScalingLists`' target (`seq_scaling_matrix_present_flag` only, else null).
|
/// `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.
|
/// Same ownership contract as [`OwnedStdSps`], with the one pointer.
|
||||||
#[derive(Debug)]
|
#[derive(Debug)]
|
||||||
pub struct OwnedStdPps {
|
pub struct OwnedStdPps {
|
||||||
std: hh::StdVideoH264PictureParameterSet,
|
/// Boxed for [`OwnedStdSps`]'s reason: `pStdPPSs` is this field's address.
|
||||||
|
std: Box<hh::StdVideoH264PictureParameterSet>,
|
||||||
_scaling_backing: Option<Box<hh::StdVideoH264ScalingLists>>,
|
_scaling_backing: Option<Box<hh::StdVideoH264ScalingLists>>,
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -290,7 +300,7 @@ pub fn sps_to_std(sps: &Sps) -> Result<OwnedStdSps, ParamsError> {
|
|||||||
}
|
}
|
||||||
|
|
||||||
Ok(OwnedStdSps {
|
Ok(OwnedStdSps {
|
||||||
std,
|
std: Box::new(std),
|
||||||
_offset_backing: offset_backing,
|
_offset_backing: offset_backing,
|
||||||
_scaling_backing: scaling_backing,
|
_scaling_backing: scaling_backing,
|
||||||
})
|
})
|
||||||
@@ -368,7 +378,7 @@ pub fn pps_to_std(pps: &Pps) -> Result<OwnedStdPps, ParamsError> {
|
|||||||
}
|
}
|
||||||
|
|
||||||
Ok(OwnedStdPps {
|
Ok(OwnedStdPps {
|
||||||
std,
|
std: Box::new(std),
|
||||||
_scaling_backing: scaling_backing,
|
_scaling_backing: scaling_backing,
|
||||||
})
|
})
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -69,9 +69,17 @@ impl std::error::Error for ParamsAv1Error {}
|
|||||||
/// where the measurement lives. Boxed backing (rather than inline arrays) is what
|
/// 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
|
/// makes storing the wrapper enough — moving it does not move the blocks, which
|
||||||
/// `moving_the_wrapper_leaves_the_driver_s_pointers_put` pins.
|
/// `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)]
|
#[derive(Debug)]
|
||||||
pub struct OwnedStdAv1SequenceHeader {
|
pub struct OwnedStdAv1SequenceHeader {
|
||||||
std: hh::StdVideoAV1SequenceHeader,
|
std: Box<hh::StdVideoAV1SequenceHeader>,
|
||||||
_color_backing: Box<hh::StdVideoAV1ColorConfig>,
|
_color_backing: Box<hh::StdVideoAV1ColorConfig>,
|
||||||
/// `pTimingInfo` is null unless the stream carries timing info: a decoder needs
|
/// `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
|
/// 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 _);
|
.map_or(std::ptr::null(), |t| &**t as *const _);
|
||||||
|
|
||||||
Ok(OwnedStdAv1SequenceHeader {
|
Ok(OwnedStdAv1SequenceHeader {
|
||||||
std,
|
std: Box::new(std),
|
||||||
_color_backing: color_backing,
|
_color_backing: color_backing,
|
||||||
_timing_backing: timing_backing,
|
_timing_backing: timing_backing,
|
||||||
})
|
})
|
||||||
|
|||||||
@@ -189,7 +189,9 @@ where
|
|||||||
/// no mutation, deliberately not `Clone` (re-convert instead).
|
/// no mutation, deliberately not `Clone` (re-convert instead).
|
||||||
#[derive(Debug)]
|
#[derive(Debug)]
|
||||||
pub struct OwnedStdH265Vps {
|
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<hh::StdVideoH265VideoParameterSet>,
|
||||||
_ptl_backing: Box<hh::StdVideoH265ProfileTierLevel>,
|
_ptl_backing: Box<hh::StdVideoH265ProfileTierLevel>,
|
||||||
_dpb_backing: Box<hh::StdVideoH265DecPicBufMgr>,
|
_dpb_backing: Box<hh::StdVideoH265DecPicBufMgr>,
|
||||||
}
|
}
|
||||||
@@ -212,7 +214,8 @@ impl OwnedStdH265Vps {
|
|||||||
/// rejected, not dropped).
|
/// rejected, not dropped).
|
||||||
#[derive(Debug)]
|
#[derive(Debug)]
|
||||||
pub struct OwnedStdH265Sps {
|
pub struct OwnedStdH265Sps {
|
||||||
std: hh::StdVideoH265SequenceParameterSet,
|
/// Boxed for [`OwnedStdH265Vps`]'s reason: `pStdSPSs` is this field's address.
|
||||||
|
std: Box<hh::StdVideoH265SequenceParameterSet>,
|
||||||
_ptl_backing: Box<hh::StdVideoH265ProfileTierLevel>,
|
_ptl_backing: Box<hh::StdVideoH265ProfileTierLevel>,
|
||||||
_dpb_backing: Box<hh::StdVideoH265DecPicBufMgr>,
|
_dpb_backing: Box<hh::StdVideoH265DecPicBufMgr>,
|
||||||
_scaling_backing: Option<Box<hh::StdVideoH265ScalingLists>>,
|
_scaling_backing: Option<Box<hh::StdVideoH265ScalingLists>>,
|
||||||
@@ -232,7 +235,8 @@ impl OwnedStdH265Sps {
|
|||||||
/// targets. Same ownership contract as [`crate::OwnedStdSps`].
|
/// targets. Same ownership contract as [`crate::OwnedStdSps`].
|
||||||
#[derive(Debug)]
|
#[derive(Debug)]
|
||||||
pub struct OwnedStdH265Pps {
|
pub struct OwnedStdH265Pps {
|
||||||
std: hh::StdVideoH265PictureParameterSet,
|
/// Boxed for [`OwnedStdH265Vps`]'s reason: `pStdPPSs` is this field's address.
|
||||||
|
std: Box<hh::StdVideoH265PictureParameterSet>,
|
||||||
_scaling_backing: Option<Box<hh::StdVideoH265ScalingLists>>,
|
_scaling_backing: Option<Box<hh::StdVideoH265ScalingLists>>,
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -474,7 +478,7 @@ pub fn vps_to_std_h265(vps: &Vps) -> Result<OwnedStdH265Vps, H265ParamsError> {
|
|||||||
std.pProfileTierLevel = &*ptl_backing;
|
std.pProfileTierLevel = &*ptl_backing;
|
||||||
|
|
||||||
Ok(OwnedStdH265Vps {
|
Ok(OwnedStdH265Vps {
|
||||||
std,
|
std: Box::new(std),
|
||||||
_ptl_backing: ptl_backing,
|
_ptl_backing: ptl_backing,
|
||||||
_dpb_backing: dpb_backing,
|
_dpb_backing: dpb_backing,
|
||||||
})
|
})
|
||||||
@@ -507,7 +511,7 @@ pub fn fallback_vps_from_sps(sps: &Sps) -> Result<OwnedStdH265Vps, H265ParamsErr
|
|||||||
std.pProfileTierLevel = &*ptl_backing;
|
std.pProfileTierLevel = &*ptl_backing;
|
||||||
|
|
||||||
Ok(OwnedStdH265Vps {
|
Ok(OwnedStdH265Vps {
|
||||||
std,
|
std: Box::new(std),
|
||||||
_ptl_backing: ptl_backing,
|
_ptl_backing: ptl_backing,
|
||||||
_dpb_backing: dpb_backing,
|
_dpb_backing: dpb_backing,
|
||||||
})
|
})
|
||||||
@@ -729,7 +733,7 @@ pub fn sps_to_std_h265(sps: &Sps) -> Result<OwnedStdH265Sps, H265ParamsError> {
|
|||||||
// docs / check_envelope).
|
// docs / check_envelope).
|
||||||
|
|
||||||
Ok(OwnedStdH265Sps {
|
Ok(OwnedStdH265Sps {
|
||||||
std,
|
std: Box::new(std),
|
||||||
_ptl_backing: ptl_backing,
|
_ptl_backing: ptl_backing,
|
||||||
_dpb_backing: dpb_backing,
|
_dpb_backing: dpb_backing,
|
||||||
_scaling_backing: scaling_backing,
|
_scaling_backing: scaling_backing,
|
||||||
@@ -903,7 +907,7 @@ pub fn pps_to_std_h265(pps: &Pps) -> Result<OwnedStdH265Pps, H265ParamsError> {
|
|||||||
// pPredictorPaletteEntries stays null (rejected above).
|
// pPredictorPaletteEntries stays null (rejected above).
|
||||||
|
|
||||||
Ok(OwnedStdH265Pps {
|
Ok(OwnedStdH265Pps {
|
||||||
std,
|
std: Box::new(std),
|
||||||
_scaling_backing: scaling_backing,
|
_scaling_backing: scaling_backing,
|
||||||
})
|
})
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -26,6 +26,18 @@
|
|||||||
//! lifetime, and both the recreate path and `Drop` destroy the object before that
|
//! lifetime, and both the recreate path and `Drop` destroy the object before that
|
||||||
//! value is released.
|
//! 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);
|
//! [`ParamsLedger`] is the pure half of that decision table (unit-tested);
|
||||||
//! [`VideoSession`] is the thin Vulkan half.
|
//! [`VideoSession`] is the thin Vulkan half.
|
||||||
|
|
||||||
@@ -366,18 +378,47 @@ struct StoredParams {
|
|||||||
/// blocks they own.
|
/// blocks they own.
|
||||||
sps: Vec<OwnedStdSps>,
|
sps: Vec<OwnedStdSps>,
|
||||||
pps: Vec<OwnedStdPps>,
|
pps: Vec<OwnedStdPps>,
|
||||||
|
/// 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<hh::StdVideoH264SequenceParameterSet>,
|
||||||
|
std_pps: Vec<hh::StdVideoH264PictureParameterSet>,
|
||||||
}
|
}
|
||||||
|
|
||||||
impl StoredParams {
|
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<OwnedStdSps>, pps: Vec<OwnedStdPps>) -> 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`
|
/// The placeholder a half-built session holds. `vkDestroyVideoSessionParametersKHR`
|
||||||
/// ignores a NULL handle, so a [`VideoSession::create`] that fails before the
|
/// ignores a NULL handle, so a [`VideoSession::create`] that fails before the
|
||||||
/// object exists still drops cleanly.
|
/// object exists still drops cleanly.
|
||||||
fn none() -> Self {
|
fn none() -> Self {
|
||||||
Self {
|
Self::assemble(Vec::new(), Vec::new())
|
||||||
object: vk::VideoSessionParametersKHR::null(),
|
|
||||||
sps: Vec::new(),
|
|
||||||
pps: Vec::new(),
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Take over sets an `Add` just handed to the live object — they belong to the
|
/// Take over sets an `Add` just handed to the live object — they belong to the
|
||||||
@@ -490,17 +531,13 @@ impl VideoSession {
|
|||||||
sps: Vec<OwnedStdSps>,
|
sps: Vec<OwnedStdSps>,
|
||||||
pps: Vec<OwnedStdPps>,
|
pps: Vec<OwnedStdPps>,
|
||||||
) -> Result<StoredParams, SessionError> {
|
) -> Result<StoredParams, SessionError> {
|
||||||
// The contiguous Std arrays the call wants. These are COPIES of each
|
// Assembled FIRST so the arrays `pStdSPSs`/`pStdPPSs` will point at are
|
||||||
// wrapper's Std struct (it is `Copy`, and the driver copies them again
|
// already where they will stay: `stored` is returned by value, and moving a
|
||||||
// before returning); the embedded pointers they carry still address the
|
// `Vec` moves its handle, not the block the driver was given.
|
||||||
// wrappers' own boxed blocks, which are what must outlive the OBJECT.
|
let mut stored = StoredParams::assemble(sps, pps);
|
||||||
let std_sps: Vec<hh::StdVideoH264SequenceParameterSet> =
|
|
||||||
sps.iter().map(|o| *o.std()).collect();
|
|
||||||
let std_pps: Vec<hh::StdVideoH264PictureParameterSet> =
|
|
||||||
pps.iter().map(|o| *o.std()).collect();
|
|
||||||
let add = vk::VideoDecodeH264SessionParametersAddInfoKHR::default()
|
let add = vk::VideoDecodeH264SessionParametersAddInfoKHR::default()
|
||||||
.std_sp_ss(&std_sps)
|
.std_sp_ss(&stored.std_sps)
|
||||||
.std_pp_ss(&std_pps);
|
.std_pp_ss(&stored.std_pps);
|
||||||
let mut h264 = vk::VideoDecodeH264SessionParametersCreateInfoKHR::default()
|
let mut h264 = vk::VideoDecodeH264SessionParametersCreateInfoKHR::default()
|
||||||
.max_std_sps_count(MAX_STD_SPS as u32)
|
.max_std_sps_count(MAX_STD_SPS as u32)
|
||||||
.max_std_pps_count(MAX_STD_PPS as u32)
|
.max_std_pps_count(MAX_STD_PPS as u32)
|
||||||
@@ -509,9 +546,10 @@ impl VideoSession {
|
|||||||
.video_session(self.session)
|
.video_session(self.session)
|
||||||
.push_next(&mut h264);
|
.push_next(&mut h264);
|
||||||
let mut object = vk::VideoSessionParametersKHR::null();
|
let mut object = vk::VideoSessionParametersKHR::null();
|
||||||
// SAFETY: fn contract; `ci` roots locals outliving the call, and the blocks
|
// SAFETY: fn contract; `ci` roots locals outliving the call, and everything
|
||||||
// the driver may retain past it are owned by `sps`/`pps`, which are moved
|
// the driver may retain past it — the Std arrays AND the blocks their
|
||||||
// into the returned value rather than dropped here.
|
// embedded pointers address — is owned by `stored`, which is returned
|
||||||
|
// rather than dropped here.
|
||||||
let r = unsafe {
|
let r = unsafe {
|
||||||
(self.video_queue.fp().create_video_session_parameters_khr)(
|
(self.video_queue.fp().create_video_session_parameters_khr)(
|
||||||
self.device.handle(),
|
self.device.handle(),
|
||||||
@@ -523,7 +561,8 @@ impl VideoSession {
|
|||||||
if r != vk::Result::SUCCESS {
|
if r != vk::Result::SUCCESS {
|
||||||
return Err(SessionError::Vk(r));
|
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
|
/// 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");
|
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]
|
#[test]
|
||||||
fn a_reactivated_identical_pair_is_current_even_across_reparses() {
|
fn a_reactivated_identical_pair_is_current_even_across_reparses() {
|
||||||
let (sps_a, pps_a) = authored(0, 0, 26);
|
let (sps_a, pps_a) = authored(0, 0, 26);
|
||||||
|
|||||||
@@ -37,6 +37,14 @@
|
|||||||
//! entirely. That is the whole of the AV1 rung's parity gap (250/250 frames
|
//! entirely. That is the whole of the AV1 rung's parity gap (250/250 frames
|
||||||
//! divergent; 0/250 with the backing held, [`StoredParamsAv1`]).
|
//! 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);
|
//! `ParamsLedgerAv1` is the pure half of the decision (unit-tested);
|
||||||
//! [`VideoSessionAv1`] is the thin Vulkan half.
|
//! [`VideoSessionAv1`] is the thin Vulkan half.
|
||||||
|
|
||||||
@@ -124,7 +132,9 @@ pub struct SessionConfigAv1 {
|
|||||||
struct StoredParamsAv1 {
|
struct StoredParamsAv1 {
|
||||||
object: vk::VideoSessionParametersKHR,
|
object: vk::VideoSessionParametersKHR,
|
||||||
/// Held for the OBJECT's whole life. Never read by this crate after the
|
/// 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,
|
_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]
|
#[test]
|
||||||
fn the_first_activation_recreates_because_there_is_no_empty_parameters_object() {
|
fn the_first_activation_recreates_because_there_is_no_empty_parameters_object() {
|
||||||
let seq = authored(1919, false);
|
let seq = authored(1919, false);
|
||||||
|
|||||||
@@ -38,7 +38,9 @@
|
|||||||
//! carries the measurement). H.265 embeds MORE such pointers than any other codec
|
//! 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
|
//! 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
|
//! 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);
|
//! `ParamsLedgerH265` is the pure half of that decision table (unit-tested);
|
||||||
//! `VideoSessionH265` is the thin Vulkan half.
|
//! `VideoSessionH265` is the thin Vulkan half.
|
||||||
@@ -289,19 +291,47 @@ struct StoredParamsH265 {
|
|||||||
vps: Vec<OwnedStdH265Vps>,
|
vps: Vec<OwnedStdH265Vps>,
|
||||||
sps: Vec<OwnedStdH265Sps>,
|
sps: Vec<OwnedStdH265Sps>,
|
||||||
pps: Vec<OwnedStdH265Pps>,
|
pps: Vec<OwnedStdH265Pps>,
|
||||||
|
/// 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<hh::StdVideoH265VideoParameterSet>,
|
||||||
|
std_sps: Vec<hh::StdVideoH265SequenceParameterSet>,
|
||||||
|
std_pps: Vec<hh::StdVideoH265PictureParameterSet>,
|
||||||
}
|
}
|
||||||
|
|
||||||
impl StoredParamsH265 {
|
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<OwnedStdH265Vps>,
|
||||||
|
sps: Vec<OwnedStdH265Sps>,
|
||||||
|
pps: Vec<OwnedStdH265Pps>,
|
||||||
|
) -> 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`
|
/// The placeholder a half-built session holds. `vkDestroyVideoSessionParametersKHR`
|
||||||
/// ignores a NULL handle, so a [`VideoSessionH265::create`] that fails before
|
/// ignores a NULL handle, so a [`VideoSessionH265::create`] that fails before
|
||||||
/// the object exists still drops cleanly.
|
/// the object exists still drops cleanly.
|
||||||
fn none() -> Self {
|
fn none() -> Self {
|
||||||
Self {
|
Self::assemble(Vec::new(), Vec::new(), Vec::new())
|
||||||
object: vk::VideoSessionParametersKHR::null(),
|
|
||||||
vps: Vec::new(),
|
|
||||||
sps: Vec::new(),
|
|
||||||
pps: Vec::new(),
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Take over sets an `Add` just handed to the live object — they belong to the
|
/// Take over sets an `Add` just handed to the live object — they belong to the
|
||||||
@@ -420,20 +450,14 @@ impl VideoSessionH265 {
|
|||||||
sps: Vec<OwnedStdH265Sps>,
|
sps: Vec<OwnedStdH265Sps>,
|
||||||
pps: Vec<OwnedStdH265Pps>,
|
pps: Vec<OwnedStdH265Pps>,
|
||||||
) -> Result<StoredParamsH265, SessionError> {
|
) -> Result<StoredParamsH265, SessionError> {
|
||||||
// The contiguous Std arrays the call wants. These are COPIES of each
|
// Assembled FIRST so the arrays `pStdVPSs`/`pStdSPSs`/`pStdPPSs` will point
|
||||||
// wrapper's Std struct (it is `Copy`, and the driver copies them again
|
// at are already where they will stay: `stored` is returned by value, and
|
||||||
// before returning); the embedded pointers they carry still address the
|
// moving a `Vec` moves its handle, not the block the driver was given.
|
||||||
// wrappers' own boxed blocks, which are what must outlive the OBJECT.
|
let mut stored = StoredParamsH265::assemble(vps, sps, pps);
|
||||||
let std_vps: Vec<hh::StdVideoH265VideoParameterSet> =
|
|
||||||
vps.iter().map(|o| *o.std()).collect();
|
|
||||||
let std_sps: Vec<hh::StdVideoH265SequenceParameterSet> =
|
|
||||||
sps.iter().map(|o| *o.std()).collect();
|
|
||||||
let std_pps: Vec<hh::StdVideoH265PictureParameterSet> =
|
|
||||||
pps.iter().map(|o| *o.std()).collect();
|
|
||||||
let add = vk::VideoDecodeH265SessionParametersAddInfoKHR::default()
|
let add = vk::VideoDecodeH265SessionParametersAddInfoKHR::default()
|
||||||
.std_vp_ss(&std_vps)
|
.std_vp_ss(&stored.std_vps)
|
||||||
.std_sp_ss(&std_sps)
|
.std_sp_ss(&stored.std_sps)
|
||||||
.std_pp_ss(&std_pps);
|
.std_pp_ss(&stored.std_pps);
|
||||||
let mut h265 = vk::VideoDecodeH265SessionParametersCreateInfoKHR::default()
|
let mut h265 = vk::VideoDecodeH265SessionParametersCreateInfoKHR::default()
|
||||||
.max_std_vps_count(MAX_STD_VPS as u32)
|
.max_std_vps_count(MAX_STD_VPS as u32)
|
||||||
.max_std_sps_count(MAX_STD_SPS as u32)
|
.max_std_sps_count(MAX_STD_SPS as u32)
|
||||||
@@ -443,9 +467,10 @@ impl VideoSessionH265 {
|
|||||||
.video_session(self.session)
|
.video_session(self.session)
|
||||||
.push_next(&mut h265);
|
.push_next(&mut h265);
|
||||||
let mut object = vk::VideoSessionParametersKHR::null();
|
let mut object = vk::VideoSessionParametersKHR::null();
|
||||||
// SAFETY: fn contract; `ci` roots locals outliving the call, and the blocks
|
// SAFETY: fn contract; `ci` roots locals outliving the call, and everything
|
||||||
// the driver may retain past it are owned by `vps`/`sps`/`pps`, which are
|
// the driver may retain past it — the Std arrays AND the blocks their
|
||||||
// moved into the returned value rather than dropped here.
|
// embedded pointers address — is owned by `stored`, which is returned
|
||||||
|
// rather than dropped here.
|
||||||
let r = unsafe {
|
let r = unsafe {
|
||||||
(self.video_queue.fp().create_video_session_parameters_khr)(
|
(self.video_queue.fp().create_video_session_parameters_khr)(
|
||||||
self.device.handle(),
|
self.device.handle(),
|
||||||
@@ -457,12 +482,8 @@ impl VideoSessionH265 {
|
|||||||
if r != vk::Result::SUCCESS {
|
if r != vk::Result::SUCCESS {
|
||||||
return Err(SessionError::Vk(r));
|
return Err(SessionError::Vk(r));
|
||||||
}
|
}
|
||||||
Ok(StoredParamsH265 {
|
stored.object = object;
|
||||||
object,
|
Ok(stored)
|
||||||
vps,
|
|
||||||
sps,
|
|
||||||
pps,
|
|
||||||
})
|
|
||||||
}
|
}
|
||||||
|
|
||||||
/// The ledger's verdict for activating (`vps`, `sps`, `pps`), without mutating
|
/// 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");
|
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]
|
#[test]
|
||||||
fn the_first_activation_adds_all_three_sets_in_one_update_call() {
|
fn the_first_activation_adds_all_three_sets_in_one_update_call() {
|
||||||
let sps = with_vps(&authored_sps(0, 0, 64), 0, 0);
|
let sps = with_vps(&authored_sps(0, 0, 64), 0, 0);
|
||||||
|
|||||||
Reference in New Issue
Block a user