diff --git a/crates/pf-client-core/src/video.rs b/crates/pf-client-core/src/video.rs index 5b808a0c..0cfb4563 100644 --- a/crates/pf-client-core/src/video.rs +++ b/crates/pf-client-core/src/video.rs @@ -222,7 +222,11 @@ pub struct DmabufPlane { /// Owns the mapped DRM-PRIME `AVFrame` (which in turn references the VAAPI surface). /// Dropping it releases the surface back to the decoder pool and closes the fds. pub struct DrmFrameGuard(pub(crate) *mut ffmpeg::ffi::AVFrame); -// An AVFrame is plain refcounted data; freeing it from the GTK main thread is fine. +// SAFETY: the guard owns one `AVFrame` and frees it exactly once in `Drop`. libav's buffer +// refcounts are atomic and its hwframe pool is internally locked, so releasing the frame — and with +// it the VAAPI surface, back to the decoder's pool — from a different thread than the one that +// mapped it is sound. That is the whole point here: the guard is handed to GTK and dropped on the +// main thread while the pump thread keeps decoding. Moved, never shared; deliberately NOT `Sync`. unsafe impl Send for DrmFrameGuard {} impl Drop for DrmFrameGuard { diff --git a/crates/pf-client-core/src/video_d3d11.rs b/crates/pf-client-core/src/video_d3d11.rs index 3162ccf6..1fc42978 100644 --- a/crates/pf-client-core/src/video_d3d11.rs +++ b/crates/pf-client-core/src/video_d3d11.rs @@ -492,8 +492,12 @@ pub(crate) struct D3d11vaDecoder { hdr10_out: bool, } -// Single-owner pointers + COM interfaces, only touched from the session pump thread (the -// decode loop); the presenter reaches the shared textures exclusively via their NT handles. +// SAFETY: the libav pointers are this decoder's own allocations (freed once in `Drop`) and the COM +// interfaces it holds are reference-counted with interlocked counts, so moving the whole struct to +// another thread and releasing it there is sound. D3D11's immediate context is not thread-SAFE but +// it is thread-AGNOSTIC: it requires serialised use, which `&mut self` on every method gives, not +// use from one fixed thread. The presenter never touches these objects — it reaches the shared +// textures through their NT handles on its own device. Moved, never shared; deliberately NOT `Sync`. unsafe impl Send for D3d11vaDecoder {} impl D3d11vaDecoder { diff --git a/crates/pf-client-core/src/video_vaapi.rs b/crates/pf-client-core/src/video_vaapi.rs index 6eb1e311..000c5ffc 100644 --- a/crates/pf-client-core/src/video_vaapi.rs +++ b/crates/pf-client-core/src/video_vaapi.rs @@ -45,7 +45,13 @@ pub(crate) struct VaapiDecoder { frame: *mut ffmpeg::ffi::AVFrame, } -// Single-owner pointers, only touched from the session pump thread. +// SAFETY: the three raw pointers (`ctx`, `packet`, `frame`) are allocations this decoder makes in +// its constructor and frees exactly once in `Drop`; nothing else holds them, and `hw_device` is an +// owning `AvBuffer` whose refcount is atomic. `Send` only permits MOVING that ownership to another +// thread, which libav supports — a codec context may be used from a thread other than the one that +// created it, provided use is serialised, and `&mut self` on every method is that serialisation. +// Deliberately NOT `Sync`: two threads holding `&VaapiDecoder` could call into libav concurrently +// on one context, which libav does not allow. #[cfg(target_os = "linux")] unsafe impl Send for VaapiDecoder {} diff --git a/crates/pf-client-core/src/video_vulkan.rs b/crates/pf-client-core/src/video_vulkan.rs index 48c264c6..a0f96b80 100644 --- a/crates/pf-client-core/src/video_vulkan.rs +++ b/crates/pf-client-core/src/video_vulkan.rs @@ -39,7 +39,12 @@ pub(crate) struct VulkanDecoder { _ctx_storage: Box, } -// Single-owner pointers, only touched from the session pump thread. +// SAFETY: `ctx`/`packet`/`frame` are allocations this decoder owns from its constructor to `Drop`, +// `hw_device` is an owning `AvBuffer` (atomic refcount), and `_ctx_storage` is a `Box` that merely +// has to outlive them. `Send` only moves that ownership between threads, which libav permits for a +// codec context used serially — `&mut self` on every method provides that. The presenter reaches +// decoded images through the `AVFrame` guard's own references and the shared `QueueLock`, not +// through this struct. Deliberately NOT `Sync`. unsafe impl Send for VulkanDecoder {} struct VkCtxStorage { diff --git a/packaging/windows/drivers/pf-umdf-util/src/section.rs b/packaging/windows/drivers/pf-umdf-util/src/section.rs index da786530..ed0063e3 100644 --- a/packaging/windows/drivers/pf-umdf-util/src/section.rs +++ b/packaging/windows/drivers/pf-umdf-util/src/section.rs @@ -37,7 +37,22 @@ pub struct MappedView { // none of which require a single-thread owner, so sharing/sending the view across the driver's // callback threads is sound. unsafe impl Send for MappedView {} -// SAFETY: as above — `&MappedView` only exposes accessors that are safe under concurrent use. +// SAFETY: `&MappedView` IS shared across threads for real — `ChannelState::data()` hands out +// `&'static MappedView`, and every driver using it dispatches its queue +// `WdfIoQueueDispatchParallel` with `NumberOfPresentedRequests = u32::MAX`, so two callbacks can +// hold it at once. What makes that sound is NOT that the accessors are individually race-free: +// `read_u8`/`write_u8`/`read_u16`/… are plain unaligned accesses through `&self` and would race if +// two threads hit the same offset. It is that these bytes are a section mapped into ANOTHER PROCESS +// that writes them concurrently, so no Rust-level exclusivity over them is achievable in the first +// place — the peer defeats it regardless of what this type does. Consistency is therefore the +// channel protocol's job (seq-fenced publishes, per the struct doc), the cross-process +// synchronization fields go through the `load_*`/`store_*` ATOMIC accessors, and each side reads +// only fields the protocol says are stable. +// +// ⚠ The rule this proof implies, for anything added later: a new accessor may use the plain path +// ONLY for bytes the protocol already fences. Anything that must be coherent between threads or +// between processes belongs in the atomic set — a plain accessor over such a field is a race the +// `Sync` here does not cover. unsafe impl Sync for MappedView {} impl MappedView {