docs(avhw): fix misleading Send soundness reasoning
CI / Build + Clippy + Test (pull_request) Failing after 30s
CI / Security audit (RUSTSEC) (pull_request) Failing after 30s
CI / Build + Clippy + Test (pull_request) Failing after 30s
CI / Security audit (RUSTSEC) (pull_request) Failing after 30s
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).
This commit is contained in:
@@ -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.
|
- 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`.
|
- 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
|
## Testing and verification
|
||||||
|
|
||||||
@@ -30,7 +30,7 @@
|
|||||||
## Unsafe and FFI work
|
## 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.
|
- 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.
|
- `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
|
## Useful manual commands
|
||||||
|
|||||||
+16
-6
@@ -12,9 +12,15 @@ pub struct AvHwDevCtx {
|
|||||||
ptr: *mut ffi::AVBufferRef,
|
ptr: *mut ffi::AVBufferRef,
|
||||||
}
|
}
|
||||||
|
|
||||||
// SAFETY: AvHwDevCtx wraps an FFmpeg AVBufferRef which is not Send by default,
|
// SAFETY: AVBufferRef's refcount is atomic (atomic_uint in libavutil/buffer.c);
|
||||||
// but we guarantee exclusive access through &mut self. The underlying VAAPI
|
// av_buffer_ref / av_buffer_unref are safe to call concurrently from different
|
||||||
// device context is thread-safe for the operations we perform.
|
// 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 {}
|
unsafe impl Send for AvHwDevCtx {}
|
||||||
|
|
||||||
impl AvHwDevCtx {
|
impl AvHwDevCtx {
|
||||||
@@ -65,9 +71,13 @@ pub struct AvHwFrameCtx {
|
|||||||
ptr: *mut ffi::AVBufferRef,
|
ptr: *mut ffi::AVBufferRef,
|
||||||
}
|
}
|
||||||
|
|
||||||
// SAFETY: AvHwFrameCtx wraps an FFmpeg AVBufferRef to an AVHWFramesContext.
|
// SAFETY: AVBufferRef's refcount is atomic (see AvHwDevCtx). The underlying
|
||||||
// It is only accessed through &mut self, ensuring no concurrent mutation.
|
// AVHWFramesContext allocates from an AVBufferPool, whose get/put operations
|
||||||
// The underlying hardware frames pool is thread-safe for the send/receive pattern.
|
// 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 {}
|
unsafe impl Send for AvHwFrameCtx {}
|
||||||
|
|
||||||
impl AvHwFrameCtx {
|
impl AvHwFrameCtx {
|
||||||
|
|||||||
+5
-2
@@ -53,8 +53,11 @@ pub struct SwEncEncode {
|
|||||||
/// MP4 mode keeps 1/fps time_base for file output simplicity.
|
/// MP4 mode keeps 1/fps time_base for file output simplicity.
|
||||||
pub const WEBRTC_RTP_CLOCK_HZ: i128 = 90_000;
|
pub const WEBRTC_RTP_CLOCK_HZ: i128 = 90_000;
|
||||||
|
|
||||||
// SAFETY: SwEncEncode owns sws_ctx/yuv_frame/enc_video exclusively after construction.
|
// SAFETY: SwEncEncode is moved to a single encode thread and accessed only there
|
||||||
// It is moved to a single encode thread and only accessed through &mut self 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<AtomicBool>
|
||||||
|
// are Send by design.
|
||||||
unsafe impl Send for SwEncEncode {}
|
unsafe impl Send for SwEncEncode {}
|
||||||
|
|
||||||
impl SwEncEncode {
|
impl SwEncEncode {
|
||||||
|
|||||||
@@ -27,14 +27,15 @@ pub struct EncState {
|
|||||||
frames_written: bool,
|
frames_written: bool,
|
||||||
}
|
}
|
||||||
|
|
||||||
// SAFETY: EncState is moved to exactly one thread (the encode worker) and used
|
// SAFETY: EncState is moved to exactly one encode worker thread and all Rust
|
||||||
// exclusively there. All fields are either plain Copy types (Option<i64>, bool)
|
// methods take &mut self, so there is no concurrent *Rust-side* access.
|
||||||
// or ffmpeg-next / AvHw* owned wrappers whose raw inner pointers are not actually
|
// FFmpeg-internal codec threads may touch hw_device_ctx / frames_rgb through
|
||||||
// shared across threads - they're touched only from the owning encode thread.
|
// the encoder context if frame/slice threading is enabled; this is sound
|
||||||
// This impl exists only to satisfy Rust's auto-Send inference (which can't see
|
// because AVHWDeviceContext and AVHWFramesContext are designed for such
|
||||||
// through the raw pointers hidden inside the wrappers). Do NOT add fields that
|
// sharing (atomic refcounts, thread-safe pool, libva VADisplay thread safety).
|
||||||
// introduce shared mutable state without re-auditing this assumption; see
|
// This impl only lifts auto-Send inference through raw pointers inside the
|
||||||
// AGENTS.md "Unsafe and FFI work" for the documented exclusivity requirement.
|
// 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 {}
|
unsafe impl Send for EncState {}
|
||||||
|
|
||||||
impl EncState {
|
impl EncState {
|
||||||
|
|||||||
@@ -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 std::path::Path;
|
||||||
|
|
||||||
use anyhow::Result;
|
use anyhow::Result;
|
||||||
|
|||||||
+4
-2
@@ -13,8 +13,10 @@ pub struct SwEncState {
|
|||||||
encode: SwEncEncode,
|
encode: SwEncEncode,
|
||||||
}
|
}
|
||||||
|
|
||||||
// SAFETY: SwEncState owns import and encode state exclusively and existing sync callers move it
|
// SAFETY: SwEncState is moved to a single encode thread and accessed only there.
|
||||||
// between threads only with external serialization; all FFI handles are accessed through &mut self.
|
// 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 {}
|
unsafe impl Send for SwEncState {}
|
||||||
|
|
||||||
impl SwEncState {
|
impl SwEncState {
|
||||||
|
|||||||
Reference in New Issue
Block a user