From fed8c2dcfd0f763dbc4deeed9c0fd2f28f56c118 Mon Sep 17 00:00:00 2001 From: dailz Date: Thu, 9 Jul 2026 15:20:16 +0800 Subject: [PATCH] docs(avhw): fix misleading Send soundness reasoning MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Oracle audit of all 5 `unsafe impl Send` in src/avhw/ found soundness intact but reasoning wrong in 3 of 5: - AvHwDevCtx: claimed '&mut self ensures exclusive access' — false, ref_clone() hands raw pointers to other threads / FFmpeg-internal codec workers. Real basis is AVBufferRef atomic_uint refcount + libva VADisplay thread safety. - AvHwFrameCtx: claimed 'send/receive pattern is thread-safe' — misdirection. Real basis is AVBufferPool atomic get/put. - EncState: claimed 'raw pointers not shared across threads' — false when FFmpeg frame/slice threading is enabled. Real basis is the hw device/frames contexts being designed for such sharing. SwEncState and SwEncEncode comments were acceptable; improved for clarity (note that contained FFmpeg handles are non-thread-safe but Send-sound under exclusive access, and that crossbeam/Arc fields are already Send by design). Added module-level convention doc to src/avhw/mod.rs centralizing the C-API-level justification rule and explicitly calling out the '&mut self as Send basis' anti-pattern so future contributors don't repeat the category error. Fixed AGENTS.md: - Stale claim that Cargo.toml 'only warns' on undocumented_unsafe_blocks (it's been 'deny' for a while) - Stale path src/avhw.rs → src/avhw/ (split in d53e881) - Stale 'avoid moving wrappers across threads' guidance — Send is sound, the audit just confirmed why No code behavior change. Verified: cargo build --release, cargo clippy --release --all-targets (0 warnings), cargo test --release (79 unit + 3 integration, 1 ignored). --- AGENTS.md | 4 ++-- src/avhw/device.rs | 22 ++++++++++++++++------ src/avhw/encode.rs | 7 +++++-- src/avhw/hardware.rs | 17 +++++++++-------- src/avhw/mod.rs | 29 +++++++++++++++++++++++++++++ src/avhw/state.rs | 6 ++++-- 6 files changed, 65 insertions(+), 20 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index cfaf5d5..b408a3b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -9,7 +9,7 @@ - Native prerequisites are FFmpeg 6+ dev libs with VAAPI, Wayland protocols/libs, libdrm, PipeWire, and libclang. `shell.nix` provides FFmpeg/Wayland/libdrm/Mesa/libva/clang and `LIBCLANG_PATH`, but does not currently list PipeWire. - Normal build: `cargo build`. Release binary required by README and integration tests: `cargo build --release`. -- `Cargo.toml` only warns on `clippy::undocumented_unsafe_blocks`; do not assume a broader clippy policy exists unless you add one. +- `Cargo.toml` sets `clippy::undocumented_unsafe_blocks = "deny"`; every `unsafe` block and `unsafe impl` must carry a `// SAFETY:` comment or the build fails. For `unsafe impl Send` on FFmpeg wrappers, see the convention in `src/avhw/mod.rs` — justification must be at the C-API level (atomic refcounts, libva `VADisplay` thread safety), not Rust borrow level. ## Testing and verification @@ -30,7 +30,7 @@ ## Unsafe and FFI work - FFmpeg/VAAPI/PipeWire code relies on raw FFI and many `unsafe` blocks. Preserve nearby `// SAFETY:` explanations and add one for any new unsafe block. -- `src/avhw.rs` owns FFmpeg `AVBufferRef`/frame contexts and has explicit `unsafe impl Send`; avoid moving those wrappers across threads without rechecking the documented exclusivity assumptions. +- `src/avhw/` (split from the former `src/avhw.rs` in commit d53e881) owns FFmpeg `AVBufferRef` / frame / codec contexts. Five types (`AvHwDevCtx`, `AvHwFrameCtx`, `EncState`, `SwEncState`, `SwEncEncode`) carry `unsafe impl Send`; soundness was Oracle-audited on 2026-07-09 against FFmpeg/libva threading semantics. Moving them across threads is sound *because the C APIs use atomic refcounts*, not because of any Rust-side exclusivity — see `src/avhw/mod.rs` for the full convention. - `CapPortal` stores the portal restore token under the user cache directory (`wl-webrtc/portal-restore-token`); use `--no-persist` when manually testing fresh authorization behavior. ## Useful manual commands diff --git a/src/avhw/device.rs b/src/avhw/device.rs index c07eef7..04d5471 100644 --- a/src/avhw/device.rs +++ b/src/avhw/device.rs @@ -12,9 +12,15 @@ pub struct AvHwDevCtx { ptr: *mut ffi::AVBufferRef, } -// SAFETY: AvHwDevCtx wraps an FFmpeg AVBufferRef which is not Send by default, -// but we guarantee exclusive access through &mut self. The underlying VAAPI -// device context is thread-safe for the operations we perform. +// SAFETY: AVBufferRef's refcount is atomic (atomic_uint in libavutil/buffer.c); +// av_buffer_ref / av_buffer_unref are safe to call concurrently from different +// threads on the same buffer. The underlying AVHWDeviceContext (VAAPI VADisplay) +// is designed by FFmpeg/libva to be shared across codec and filter contexts, +// including across FFmpeg-internal codec threads. Raw refs returned by +// ref_clone() may outlive this wrapper and be consumed by other threads; this +// is the intended usage pattern and is sound because refcount management is +// atomic. The &mut self on Rust methods is an API convenience, not the basis +// for soundness. unsafe impl Send for AvHwDevCtx {} impl AvHwDevCtx { @@ -65,9 +71,13 @@ pub struct AvHwFrameCtx { ptr: *mut ffi::AVBufferRef, } -// SAFETY: AvHwFrameCtx wraps an FFmpeg AVBufferRef to an AVHWFramesContext. -// It is only accessed through &mut self, ensuring no concurrent mutation. -// The underlying hardware frames pool is thread-safe for the send/receive pattern. +// SAFETY: AVBufferRef's refcount is atomic (see AvHwDevCtx). The underlying +// AVHWFramesContext allocates from an AVBufferPool, whose get/put operations +// are atomic and thread-safe. av_hwframe_get_buffer and av_hwframe_transfer_data +// are safe to call concurrently on distinct AVFrames. Cloned refs are typically +// attached to AVCodecContext.hw_frames_ctx and accessed by FFmpeg-internal codec +// threads; this is the designed usage. The &mut self on Rust methods is not the +// basis for soundness. unsafe impl Send for AvHwFrameCtx {} impl AvHwFrameCtx { diff --git a/src/avhw/encode.rs b/src/avhw/encode.rs index d7b6adb..ff6698e 100644 --- a/src/avhw/encode.rs +++ b/src/avhw/encode.rs @@ -53,8 +53,11 @@ pub struct SwEncEncode { /// MP4 mode keeps 1/fps time_base for file output simplicity. pub const WEBRTC_RTP_CLOCK_HZ: i128 = 90_000; -// SAFETY: SwEncEncode owns sws_ctx/yuv_frame/enc_video exclusively after construction. -// It is moved to a single encode thread and only accessed through &mut self there. +// SAFETY: SwEncEncode is moved to a single encode thread and accessed only there +// via &mut self. SwsContext, AVFrame, and AVCodecContext are NOT thread-safe for +// concurrent access but are Send-sound under single-thread exclusive use, which +// the encode worker invariant provides. crossbeam Receiver and Arc +// are Send by design. unsafe impl Send for SwEncEncode {} impl SwEncEncode { diff --git a/src/avhw/hardware.rs b/src/avhw/hardware.rs index 90aef9f..7a0b1db 100644 --- a/src/avhw/hardware.rs +++ b/src/avhw/hardware.rs @@ -27,14 +27,15 @@ pub struct EncState { frames_written: bool, } -// SAFETY: EncState is moved to exactly one thread (the encode worker) and used -// exclusively there. All fields are either plain Copy types (Option, bool) -// or ffmpeg-next / AvHw* owned wrappers whose raw inner pointers are not actually -// shared across threads - they're touched only from the owning encode thread. -// This impl exists only to satisfy Rust's auto-Send inference (which can't see -// through the raw pointers hidden inside the wrappers). Do NOT add fields that -// introduce shared mutable state without re-auditing this assumption; see -// AGENTS.md "Unsafe and FFI work" for the documented exclusivity requirement. +// SAFETY: EncState is moved to exactly one encode worker thread and all Rust +// methods take &mut self, so there is no concurrent *Rust-side* access. +// FFmpeg-internal codec threads may touch hw_device_ctx / frames_rgb through +// the encoder context if frame/slice threading is enabled; this is sound +// because AVHWDeviceContext and AVHWFramesContext are designed for such +// sharing (atomic refcounts, thread-safe pool, libva VADisplay thread safety). +// This impl only lifts auto-Send inference through raw pointers inside the +// ffmpeg-next wrappers; it does not introduce new sharing. Do NOT add fields +// that create shared mutable state across threads without re-auditing. unsafe impl Send for EncState {} impl EncState { diff --git a/src/avhw/mod.rs b/src/avhw/mod.rs index f05a7a0..0a6e0bd 100644 --- a/src/avhw/mod.rs +++ b/src/avhw/mod.rs @@ -1,3 +1,32 @@ +//! FFmpeg / VAAPI encoder wrappers. +//! +//! ## `Send` justification convention +//! +//! Several types in this module (`AvHwDevCtx`, `AvHwFrameCtx`, `EncState`, +//! `SwEncState`, `SwEncEncode`) carry raw FFmpeg pointers and therefore need +//! an explicit `unsafe impl Send`. The justification is always at the C-API +//! level, never at the Rust-borrow level: +//! +//! - `AVBufferRef` refcounts are `atomic_uint` (`libavutil/buffer.c`), so +//! `av_buffer_ref` / `av_buffer_unref` are safe to call concurrently. +//! - `AVHWDeviceContext` (VAAPI `VADisplay`) is designed by FFmpeg/libva to +//! be shared across codec and filter contexts, including FFmpeg-internal +//! codec worker threads. +//! - `AVHWFramesContext` allocates from an `AVBufferPool` whose get/put are +//! atomic; `av_hwframe_get_buffer` is safe to call concurrently on +//! distinct frames. +//! - `SwsContext`, `AVFilterGraph`, `AVFrame`, `AVCodecContext` are NOT +//! thread-safe for concurrent use, but are `Send`-sound under the +//! single-thread exclusive access invariant that the encode worker +//! enforces. +//! +//! **Anti-pattern**: justifying `Send` with "`&mut self` ensures exclusive +//! access". `Send` is about *moving ownership between threads*, not about +//! borrowing. The `&mut self` on Rust methods is API convenience and is not +//! the basis for soundness — refs cloned via `ref_clone()` routinely escape +//! to other threads / FFmpeg-internal workers, and that is fine because the +//! underlying C APIs are designed for it. + use std::path::Path; use anyhow::Result; diff --git a/src/avhw/state.rs b/src/avhw/state.rs index bd4f0d3..859dda2 100644 --- a/src/avhw/state.rs +++ b/src/avhw/state.rs @@ -13,8 +13,10 @@ pub struct SwEncState { encode: SwEncEncode, } -// SAFETY: SwEncState owns import and encode state exclusively and existing sync callers move it -// between threads only with external serialization; all FFI handles are accessed through &mut self. +// SAFETY: SwEncState is moved to a single encode thread and accessed only there. +// All FFmpeg handles (SwsContext, AVFrame, AVCodecContext) inside SwEncImport / +// SwEncEncode are non-thread-safe but Send-sound under exclusive access. +// Existing sync callers move it across threads only with external serialization. unsafe impl Send for SwEncState {} impl SwEncState {