From 3fac1a6da11710d90958d3076fe3ddb3edaa448b Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Sat, 25 Jul 2026 13:00:39 +0200 Subject: [PATCH] fix(encode/linux): stop the capability probes racing each other's libav log level MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every capability probe drops libav's log level to AV_LOG_FATAL around an encoder open it expects to fail, then restores what was there before. That level is one process-global int and the probes genuinely overlap — they are reached both from `/serverinfo` and from session bring-up. Two overlapping save/restore pairs interleave as get(INFO) → get(FATAL) → set(INFO) → set(FATAL), and the process is then pinned at AV_LOG_FATAL permanently: every later libav diagnostic silently dropped, including the ones you want when a stream fails to open later on. Replaces the four hand-rolled save/restore pairs with one RAII guard over a single shared mutex. The audit filed two sites; there are four — the NVENC 4:4:4 and 10-bit probes in `linux/mod.rs` and both VAAPI probes, which is why the lock has to be shared across the two modules rather than kept local to either. Being RAII also closes a smaller hole the old shape had: the restore was a statement after the open, so any early return added to those functions later would have leaked the quiet level. Now it cannot. The guard is poison-tolerant — a probe that panicked mid-window has already restored the level through `Drop`, so refusing the lock forever afterwards would be strictly worse than proceeding. It is not re-entrant, which is safe here because no probe body reaches another probe (they only call encoder-open helpers); that invariant is written down at the type. Serialising the probes costs nothing measurable: they are process-once behind their caches and each already pays for a real encoder open. Verified with the canonical Linux gate (docker linux/amd64): fmt, clippy --all-targets at default and at nvenc,vulkan-encode,pyrowave, and the pf-encode test leg (37 passed). --- crates/pf-encode/src/enc/linux/mod.rs | 84 ++++++++++++++++++------- crates/pf-encode/src/enc/linux/vaapi.rs | 47 +++++++------- 2 files changed, 81 insertions(+), 50 deletions(-) diff --git a/crates/pf-encode/src/enc/linux/mod.rs b/crates/pf-encode/src/enc/linux/mod.rs index 21abd638..69014d0f 100644 --- a/crates/pf-encode/src/enc/linux/mod.rs +++ b/crates/pf-encode/src/enc/linux/mod.rs @@ -904,6 +904,58 @@ impl Drop for NvencEncoder { } } +/// Serialises the save → `AV_LOG_FATAL` → restore window that every capability probe opens around +/// an encoder open it *expects* to fail. +/// +/// libav's log level is one process-global `int`, and the probes race each other for real: the +/// NVENC and VAAPI 4:4:4/10-bit probes are reached from `/serverinfo` and from session bring-up. +/// Two overlapping save/restore pairs interleave as get(INFO) → get(FATAL) → set(INFO) → +/// set(FATAL), and the process is then pinned at `AV_LOG_FATAL` for good — every later libav +/// diagnostic silently dropped, which is precisely the logging you want when a stream later fails +/// to open. The probes run process-once and already cost a real encoder open, so serialising them +/// costs nothing measurable. +static LIBAV_LOG_LEVEL: std::sync::Mutex<()> = std::sync::Mutex::new(()); + +/// RAII quiet-window over libav's global log level: drops it to `AV_LOG_FATAL` on construction and +/// restores the previous level on drop, holding [`LIBAV_LOG_LEVEL`] for the whole window. +/// +/// Callers must have completed `ffmpeg::init()` first. Not re-entrant — no probe may construct a +/// second guard while holding one (none do; the probe bodies only reach encoder-open helpers). +/// `pub(crate)` so the VAAPI probes share the one lock: they race the NVENC probes on the same +/// global. +pub(crate) struct QuietLibavLog { + prev: c_int, + // Held for the lifetime of the guard. `Drop for QuietLibavLog` runs before the struct's fields + // are dropped, so the restore below still happens under the lock. + _lock: std::sync::MutexGuard<'static, ()>, +} + +impl QuietLibavLog { + pub(crate) fn new() -> Self { + // Poison-tolerant: a probe that panicked mid-window already restored the level via `Drop`, + // and refusing the lock forever afterwards would be a worse outcome than proceeding. + let lock = LIBAV_LOG_LEVEL + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + // SAFETY: libav is initialized by the caller; `av_log_{get,set}_level` only read/write the + // global int level (no pointer args) and are always sound post-init. + let prev = unsafe { + let p = ffi::av_log_get_level(); + ffi::av_log_set_level(ffi::AV_LOG_FATAL); + p + }; + Self { prev, _lock: lock } + } +} + +impl Drop for QuietLibavLog { + fn drop(&mut self) { + // SAFETY: restore the saved global level (scalar arg, no pointers); libav was initialized + // before this guard was constructed. + unsafe { ffi::av_log_set_level(self.prev) }; + } +} + /// Probe whether this NVIDIA GPU + driver + libavcodec can actually encode HEVC **4:4:4** (Range /// Extensions). Opens a tiny real `hevc_nvenc` 4:4:4 session — the exact path [`NvencEncoder::open`] /// takes for a live 4:4:4 stream — and reports whether it succeeded. HEVC-only; the result is cached @@ -917,14 +969,9 @@ pub fn probe_can_encode_444(codec: Codec) -> bool { return false; } // Quiet ffmpeg's open error on a GPU that lacks 4:4:4 — the probe failing is an expected outcome. - // SAFETY: libav initialized above; `av_log_{get,set}_level` only read/write the global int level - // (no pointer args) and are always sound post-init. - let prev = unsafe { - let p = ffi::av_log_get_level(); - ffi::av_log_set_level(ffi::AV_LOG_FATAL); - p - }; - let ok = NvencEncoder::open( + // Held until the function returns, so the level is restored after the open either way. + let _quiet = QuietLibavLog::new(); + NvencEncoder::open( codec, PixelFormat::Bgra, 640, @@ -935,10 +982,7 @@ pub fn probe_can_encode_444(codec: Codec) -> bool { 8, ChromaFormat::Yuv444, ) - .is_ok(); - // SAFETY: restore the saved global log level (scalar arg, no pointers). - unsafe { ffi::av_log_set_level(prev) }; - ok + .is_ok() } /// Probe whether this NVIDIA GPU + driver + libavcodec can actually encode 10-bit (HEVC Main10 / @@ -954,14 +998,9 @@ pub fn probe_can_encode_10bit(codec: Codec) -> bool { return false; } // Quiet ffmpeg's open error on a GPU that lacks 10-bit — the probe failing is an expected outcome. - // SAFETY: libav initialized above; `av_log_{get,set}_level` only read/write the global int level - // (no pointer args) and are always sound post-init. - let prev = unsafe { - let p = ffi::av_log_get_level(); - ffi::av_log_set_level(ffi::AV_LOG_FATAL); - p - }; - let ok = NvencEncoder::open( + // Held until the function returns, so the level is restored after the open either way. + let _quiet = QuietLibavLog::new(); + NvencEncoder::open( codec, PixelFormat::X2Rgb10, 640, @@ -972,10 +1011,7 @@ pub fn probe_can_encode_10bit(codec: Codec) -> bool { 10, ChromaFormat::Yuv420, ) - .is_ok(); - // SAFETY: restore the saved global log level (scalar arg, no pointers). - unsafe { ffi::av_log_set_level(prev) }; - ok + .is_ok() } #[cfg(test)] diff --git a/crates/pf-encode/src/enc/linux/vaapi.rs b/crates/pf-encode/src/enc/linux/vaapi.rs index 632bc05d..228ccfc3 100644 --- a/crates/pf-encode/src/enc/linux/vaapi.rs +++ b/crates/pf-encode/src/enc/linux/vaapi.rs @@ -273,20 +273,20 @@ pub fn probe_can_encode(codec: Codec) -> bool { if ffmpeg::init().is_err() { return false; } - // SAFETY: `ffmpeg::init()` returned Ok above, so libav is initialized. `av_log_get_level`/ - // `av_log_set_level` only read/write libav's global integer log level (no pointer args) and are - // always sound to call post-init. `VaapiHw::new` (an `unsafe fn`) builds a VAAPI device + NV12 - // frames pool from the literal NV12/640x480/pool=2 args and hands back a RAII handle that unrefs - // both `AVBufferRef`s on drop. `open_vaapi_encoder` (an `unsafe fn`) borrows `hw.device_ref`/ - // `hw.frames_ref` — the two non-null refs `VaapiHw::new` just created — and `av_buffer_ref`s them - // into the encoder; `hw` is a live local for the whole match arm, so the borrows outlive the - // synchronous call, and both `hw` and the probe encoder are dropped (RAII) when the arm ends. + // A missing VA device (non-VAAPI host, GPU-less CI) is an expected probe outcome — quiet + // ffmpeg's "No VA display found" error for the probe. Held until the function returns, so the + // level is restored after the open either way. Shares one lock with the NVENC probes, which + // race this one on the same libav global (see [`crate::linux::QuietLibavLog`]). + let _quiet = crate::linux::QuietLibavLog::new(); + // SAFETY: `ffmpeg::init()` returned Ok above, so libav is initialized. `VaapiHw::new` (an + // `unsafe fn`) builds a VAAPI device + NV12 frames pool from the literal NV12/640x480/pool=2 + // args and hands back a RAII handle that unrefs both `AVBufferRef`s on drop. + // `open_vaapi_encoder` (an `unsafe fn`) borrows `hw.device_ref`/`hw.frames_ref` — the two + // non-null refs `VaapiHw::new` just created — and `av_buffer_ref`s them into the encoder; `hw` + // is a live local for the whole match arm, so the borrows outlive the synchronous call, and + // both `hw` and the probe encoder are dropped (RAII) when the arm ends. unsafe { - // A missing VA device (non-VAAPI host, GPU-less CI) is an expected probe outcome — quiet - // ffmpeg's "No VA display found" error for the probe, then restore the level. - let prev = ffi::av_log_get_level(); - ffi::av_log_set_level(ffi::AV_LOG_FATAL); - let ok = match VaapiHw::new(ffi::AVPixelFormat::AV_PIX_FMT_NV12, 640, 480, 2) { + match VaapiHw::new(ffi::AVPixelFormat::AV_PIX_FMT_NV12, 640, 480, 2) { Ok(hw) => open_vaapi_encoder( codec, 640, @@ -299,9 +299,7 @@ pub fn probe_can_encode(codec: Codec) -> bool { ) .is_ok(), Err(_) => false, - }; - ffi::av_log_set_level(prev); - ok + } } } @@ -317,18 +315,17 @@ pub fn probe_can_encode_10bit(codec: Codec) -> bool { if ffmpeg::init().is_err() { return false; } - // SAFETY: `ffmpeg::init()` returned Ok above, so libav is initialized. `av_log_{get,set}_level` - // only read/write libav's global integer log level (no pointer args). `VaapiHw::new` (an + // A missing VA device / no Main10 entrypoint is an expected probe outcome — quiet ffmpeg's + // error for the probe. Held until the function returns, so the level is restored after the open + // either way, and shared with the other probes (see [`crate::linux::QuietLibavLog`]). + let _quiet = crate::linux::QuietLibavLog::new(); + // SAFETY: `ffmpeg::init()` returned Ok above, so libav is initialized. `VaapiHw::new` (an // `unsafe fn`) builds a VAAPI device + P010 frames pool from the literal args and hands back a // RAII handle; `open_vaapi_encoder` (an `unsafe fn`) borrows `hw.device_ref`/`hw.frames_ref` — // the two non-null refs `VaapiHw::new` just created, live locals for the whole match arm — and // `av_buffer_ref`s them into the probe encoder. Both `hw` and the encoder drop (RAII) at arm end. unsafe { - // A missing VA device / no Main10 entrypoint is an expected probe outcome — quiet ffmpeg's - // error for the probe, then restore the level. - let prev = ffi::av_log_get_level(); - ffi::av_log_set_level(ffi::AV_LOG_FATAL); - let ok = match VaapiHw::new(ffi::AVPixelFormat::AV_PIX_FMT_P010LE, 640, 480, 2) { + match VaapiHw::new(ffi::AVPixelFormat::AV_PIX_FMT_P010LE, 640, 480, 2) { Ok(hw) => open_vaapi_encoder( codec, 640, @@ -341,9 +338,7 @@ pub fn probe_can_encode_10bit(codec: Codec) -> bool { ) .is_ok(), Err(_) => false, - }; - ffi::av_log_set_level(prev); - ok + } } }