From ed4bbc6b0bd8ff8c244f9ce0c98aca85f98ffdca Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Tue, 28 Jul 2026 23:31:43 +0200 Subject: [PATCH] =?UTF-8?q?refactor(client-core):=20the=20first=20fence=20?= =?UTF-8?q?comes=20off=20=E2=80=94=20video=5Fvulkan.rs=20is=20at=20zero?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `video_vulkan.rs` was fenced with the other thirteen GPU/FFI backends, but it never belonged with them: its four sites are not ash calls, they are RAW POINTER DEREFERENCES inside the two FFmpeg `lock_queue`/`unlock_queue` callback trampolines — exactly the case where the lint pays, and where a proof carries an argument instead of restating a signature. Both bodies had none at all. They keep `unsafe extern "C"`: FFmpeg invokes them with a `*mut AVHWDeviceContext`, which is a real contract, now written down. What was missing is why the dereference chain is sound, and it is worth stating because it is not obvious — the trampoline casts between pf_ffvk's `AVHWDeviceContext` and ffmpeg-sys's (the same C struct declared twice), reads `user_opaque`, and dereferences it as a `QueueLock`. That pointer borrows `VkCtxStorage::_queue_lock`, an `Arc` whose field doc already says it exists to outlive every call the context can make. The proof now connects those two facts, so a future edit to the storage's lifetime has something to contradict. With that, the file needs no exemption and the workspace `deny` covers it: 14 fenced files -> 13. This is what removing a fence is supposed to look like — the sites that are worth narrowing get narrowed, and the exemption disappears rather than being renewed. Verified on .21: fmt + `clippy --workspace --all-targets -- -D warnings` + the feature-gated `-p pf-encode` step, all rc=0 with no allow in the file. --- crates/pf-client-core/src/video_vulkan.rs | 38 ++++++++++++++++------- 1 file changed, 26 insertions(+), 12 deletions(-) diff --git a/crates/pf-client-core/src/video_vulkan.rs b/crates/pf-client-core/src/video_vulkan.rs index c3aed5df..ea422116 100644 --- a/crates/pf-client-core/src/video_vulkan.rs +++ b/crates/pf-client-core/src/video_vulkan.rs @@ -1,10 +1,4 @@ //! FFmpeg Vulkan Video decode over the presenter's own VkDevice (zero-copy VkImage). -// UNSAFE-LINT EXEMPTION (rationale + exit criteria: `unsafe_op_in_unsafe_fn` in the workspace -// Cargo.toml). This body is raw libav + ash calls on the presenter's VkDevice almost line for line; -// narrowing it would add one `unsafe {}` plus one SAFETY comment per call that could only restate -// the signature. Clearing this file means DELETING the markers that carry no caller contract, not -// wrapping the calls — until then the lint is off HERE and enforced everywhere else. -#![allow(unsafe_op_in_unsafe_fn)] #![allow(clippy::unnecessary_cast)] use crate::video::{ @@ -67,25 +61,45 @@ struct VkCtxStorage { /// [`VkCtxStorage`], which outlives the context). Replaces FFmpeg's internal default, /// which only serializes FFmpeg against itself — the presenter submits to the same /// graphics queue from another thread and holds this same lock around its calls. +/// +/// # Safety +/// FFmpeg calls this with the `AVHWDeviceContext` it owns, whose `user_opaque` we set to a +/// `*const QueueLock` before handing the context over. unsafe extern "C" fn ffvk_lock_queue( ctx: *mut pf_ffvk::AVHWDeviceContext, _queue_family: u32, _index: u32, ) { - let dev = ctx as *mut ffmpeg::ffi::AVHWDeviceContext; - let lock = (*dev).user_opaque as *const QueueLock; - (*lock).lock(); + // SAFETY: `ctx` is the live context FFmpeg passes to its own callback, and the two + // `AVHWDeviceContext` declarations (pf_ffvk's and ffmpeg-sys's) describe the same C struct, so + // the cast reads the same `user_opaque` field. That field holds the pointer we stored, which + // borrows `VkCtxStorage::_queue_lock` — an `Arc` the storage keeps alive for as long + // as the hw device context can fire this trampoline (see its field doc), so the lock outlives + // every call FFmpeg can make. + unsafe { + let dev = ctx as *mut ffmpeg::ffi::AVHWDeviceContext; + let lock = (*dev).user_opaque as *const QueueLock; + (*lock).lock(); + } } /// The matching `unlock_queue` trampoline — see [`ffvk_lock_queue`]. +/// +/// # Safety +/// As [`ffvk_lock_queue`]; additionally, FFmpeg only calls this after a matching `lock_queue`, so +/// the lock it releases is one this pair took. unsafe extern "C" fn ffvk_unlock_queue( ctx: *mut pf_ffvk::AVHWDeviceContext, _queue_family: u32, _index: u32, ) { - let dev = ctx as *mut ffmpeg::ffi::AVHWDeviceContext; - let lock = (*dev).user_opaque as *const QueueLock; - (*lock).unlock(); + // SAFETY: as `ffvk_lock_queue` — same live context from FFmpeg, same `user_opaque` pointer into + // the `Arc` that `VkCtxStorage` keeps alive for the context's whole lifetime. + unsafe { + let dev = ctx as *mut ffmpeg::ffi::AVHWDeviceContext; + let lock = (*dev).user_opaque as *const QueueLock; + (*lock).unlock(); + } } impl VulkanDecoder {