diff --git a/crates/pf-client-core/src/lib.rs b/crates/pf-client-core/src/lib.rs index 6dd13733..ec88584c 100644 --- a/crates/pf-client-core/src/lib.rs +++ b/crates/pf-client-core/src/lib.rs @@ -50,6 +50,9 @@ pub mod video; mod video_color; #[cfg(any(target_os = "linux", windows))] mod video_software; +// libav ownership helpers shared by the hardware decoders below (`AvBuffer`). +#[cfg(any(target_os = "linux", windows))] +mod video_libav; #[cfg(target_os = "linux")] mod video_vaapi; #[cfg(any(target_os = "linux", windows))] diff --git a/crates/pf-client-core/src/video_libav.rs b/crates/pf-client-core/src/video_libav.rs new file mode 100644 index 00000000..0426a774 --- /dev/null +++ b/crates/pf-client-core/src/video_libav.rs @@ -0,0 +1,59 @@ +//! Shared libav ownership helpers for the hardware decoders (`video_vaapi`, `video_vulkan`, +//! `video_d3d11`). +//! +//! The host has its own copy of this in `pf-encode`'s `enc/libav.rs`. The two crates do not depend +//! on each other — host encode and client decode share no code path — and the client's copy needs +//! something the host's does not ([`AvBuffer::into_raw`], for the decoder contexts that take +//! ownership of our ref), so a shared crate would have to carry an ffmpeg dependency and both sets +//! of semantics to save ~20 lines. It is not worth the edge. + +use ffmpeg_next::ffi; + +/// An owned `AVBufferRef`, unref'd exactly once when it drops. +/// +/// Each decoder constructor creates a hwdevice and then does several more fallible things with it +/// — find the codec, alloc a context, open it — and every one of those failure branches used to +/// unref the device by hand, alongside a `Drop` doing it once more. Miss a branch and a decoder +/// device leaks per failed negotiation; double it up and the process aborts. Ownership lives here +/// instead, so an early `bail!` releases whatever exists and the branches carry no cleanup. +pub(crate) struct AvBuffer(*mut ffi::AVBufferRef); + +impl AvBuffer { + /// Take ownership of a freshly-created `AVBufferRef`, rejecting the null an ffmpeg allocator + /// returns on failure. + /// + /// # Safety + /// `p` must be null, or a live `AVBufferRef` whose ownership passes to the returned value — + /// nothing else may unref it. + pub(crate) unsafe fn from_raw(p: *mut ffi::AVBufferRef) -> Option { + (!p.is_null()).then_some(AvBuffer(p)) + } + + /// The borrowed pointer, for calls that read the ref without taking it (e.g. `av_buffer_ref`, + /// which makes its own). + pub(crate) fn as_ptr(&self) -> *mut ffi::AVBufferRef { + self.0 + } + + /// Give up ownership: the caller becomes responsible for the unref. + /// + /// This exists for the `get_format` callbacks, which hand a frames context to the codec — + /// `(*ctx).hw_frames_ctx = fr` means *the codec owns our ref now*, and it unrefs it when the + /// context closes. Dropping an `AvBuffer` there as well would be the double-unref this type is + /// meant to prevent, so the transfer is made explicit rather than left implicit. + pub(crate) fn into_raw(self) -> *mut ffi::AVBufferRef { + let p = self.0; + std::mem::forget(self); + p + } +} + +impl Drop for AvBuffer { + fn drop(&mut self) { + // SAFETY: `self.0` is the non-null ref `from_raw` took ownership of, and this type is its + // sole owner (neither `Clone` nor `Copy`; `as_ptr` only lends, and `into_raw` forgets + // instead of dropping), so this runs exactly once for that reference. `av_buffer_unref` + // drops the one reference and nulls the pointer through the `&mut`. + unsafe { ffi::av_buffer_unref(&mut self.0) }; + } +} diff --git a/crates/pf-client-core/src/video_vaapi.rs b/crates/pf-client-core/src/video_vaapi.rs index 0c5b48b7..8e6004c1 100644 --- a/crates/pf-client-core/src/video_vaapi.rs +++ b/crates/pf-client-core/src/video_vaapi.rs @@ -5,7 +5,8 @@ use crate::video::{ AVERROR_EAGAIN, }; use crate::video_color::ColorDesc; -use anyhow::{anyhow, bail, Result}; +use crate::video_libav::AvBuffer; +use anyhow::{anyhow, bail, Context, Result}; use ffmpeg_next as ffmpeg; use std::ptr; @@ -32,7 +33,9 @@ unsafe extern "C" fn pick_vaapi( #[cfg(target_os = "linux")] pub(crate) struct VaapiDecoder { ctx: *mut ffmpeg::ffi::AVCodecContext, - hw_device: *mut ffmpeg::ffi::AVBufferRef, + // Owned: unrefs itself. Declared after `ctx` so it still releases AFTER the `Drop` below frees + // packet/frame/ctx — the same order the hand-written unref had. + hw_device: AvBuffer, packet: *mut ffmpeg::ffi::AVPacket, frame: *mut ffmpeg::ffi::AVFrame, } @@ -57,14 +60,16 @@ impl VaapiDecoder { if r < 0 { bail!("no VAAPI device ({})", ffmpeg::Error::from(r)); } + // Owned from here: every `bail!` below drops it, so none of them unref by hand. + let hw_device = AvBuffer::from_raw(hw_device) + .context("av_hwdevice_ctx_create(VAAPI) gave no device")?; // The negotiated codec's decoder id (av_codec_id maps 1:1 from ffmpeg::codec::Id). let codec = ffi::avcodec_find_decoder(codec_id.into()); if codec.is_null() { - ffi::av_buffer_unref(&mut hw_device); bail!("no {codec_id:?} decoder"); } let ctx = ffi::avcodec_alloc_context3(codec); - (*ctx).hw_device_ctx = ffi::av_buffer_ref(hw_device); + (*ctx).hw_device_ctx = ffi::av_buffer_ref(hw_device.as_ptr()); (*ctx).get_format = Some(pick_vaapi); (*ctx).flags |= ffi::AV_CODEC_FLAG_LOW_DELAY as i32; (*ctx).thread_count = 1; // hwaccel: threads only add latency @@ -80,8 +85,6 @@ impl VaapiDecoder { if r < 0 { let mut ctx = ctx; ffi::avcodec_free_context(&mut ctx); - let mut hw_device = hw_device; - ffi::av_buffer_unref(&mut hw_device); bail!("avcodec_open2: {}", ffmpeg::Error::from(r)); } Ok(VaapiDecoder { @@ -237,7 +240,7 @@ impl Drop for VaapiDecoder { ffi::av_packet_free(&mut self.packet); ffi::av_frame_free(&mut self.frame); ffi::avcodec_free_context(&mut self.ctx); - ffi::av_buffer_unref(&mut self.hw_device); + // `hw_device` is an `AvBuffer` and unrefs itself when the field drops, right after this. } } } diff --git a/crates/pf-client-core/src/video_vulkan.rs b/crates/pf-client-core/src/video_vulkan.rs index a89c8ec2..72a7e495 100644 --- a/crates/pf-client-core/src/video_vulkan.rs +++ b/crates/pf-client-core/src/video_vulkan.rs @@ -6,7 +6,8 @@ use crate::video::{ AVERROR_EAGAIN, }; use crate::video_color::ColorDesc; -use anyhow::{bail, Result}; +use crate::video_libav::AvBuffer; +use anyhow::{bail, Context, Result}; use ffmpeg_next as ffmpeg; use std::ptr; @@ -18,7 +19,9 @@ use std::ptr; /// frames are `AVVkFrame`s whose VkImage the presenter feeds straight to its CSC pass. pub(crate) struct VulkanDecoder { ctx: *mut ffmpeg::ffi::AVCodecContext, - hw_device: *mut ffmpeg::ffi::AVBufferRef, + // Owned: unrefs itself. Declared after `ctx` so it still releases AFTER the `Drop` below frees + // packet/frame/ctx — the same order the hand-written unref had. + hw_device: AvBuffer, packet: *mut ffmpeg::ffi::AVPacket, frame: *mut ffmpeg::ffi::AVFrame, /// `vkWaitSemaphores` on the shared device — the decode-complete measurement @@ -187,6 +190,9 @@ impl VulkanDecoder { ffi::av_buffer_unref(&mut hw_device); return Err(averr("av_hwdevice_ctx_init(VULKAN)", r)); } + // Owned from here: every failure path below drops it instead of unref'ing by hand. + let hw_device = AvBuffer::from_raw(hw_device) + .context("av_hwdevice_ctx_alloc(VULKAN) gave no device")?; // vkWaitSemaphores for the pump's decode-complete stat: loader → // vkGetDeviceProcAddr → device fn (core 1.2, guaranteed by our gate). @@ -201,18 +207,16 @@ impl VulkanDecoder { c"vkWaitSemaphores".as_ptr(), )); if wait_semaphores.is_none() { - ffi::av_buffer_unref(&mut hw_device); bail!("vkWaitSemaphores unresolvable on this device"); } let vk_device = (*hwctx).act_dev; let codec = ffi::avcodec_find_decoder(codec_id.into()); if codec.is_null() { - ffi::av_buffer_unref(&mut hw_device); bail!("no {codec_id:?} decoder"); } let ctx = ffi::avcodec_alloc_context3(codec); - (*ctx).hw_device_ctx = ffi::av_buffer_ref(hw_device); + (*ctx).hw_device_ctx = ffi::av_buffer_ref(hw_device.as_ptr()); (*ctx).get_format = Some(pick_vulkan); (*ctx).flags |= ffi::AV_CODEC_FLAG_LOW_DELAY as i32; (*ctx).thread_count = 1; // hwaccel: threads only add latency @@ -223,7 +227,6 @@ impl VulkanDecoder { if r < 0 { let mut ctx = ctx; ffi::avcodec_free_context(&mut ctx); - ffi::av_buffer_unref(&mut hw_device); return Err(averr("avcodec_open2 (vulkan)", r)); } Ok(VulkanDecoder { @@ -358,7 +361,7 @@ impl Drop for VulkanDecoder { ffi::av_packet_free(&mut self.packet); ffi::av_frame_free(&mut self.frame); ffi::avcodec_free_context(&mut self.ctx); - ffi::av_buffer_unref(&mut self.hw_device); + // `hw_device` is an `AvBuffer` and unrefs itself when the field drops, right after this. } } } @@ -396,24 +399,29 @@ unsafe extern "C" fn pick_vulkan( tracing::warn!(code = r, "avcodec_get_hw_frames_parameters(VULKAN) failed"); return ffi::AVPixelFormat::AV_PIX_FMT_NONE; } - let fc = (*fr).data as *mut ffi::AVHWFramesContext; + // Owned until the codec takes it at the bottom: the init-failure path below just returns + // and the drop releases it. + let Some(fr) = AvBuffer::from_raw(fr) else { + return ffi::AVPixelFormat::AV_PIX_FMT_NONE; + }; + let fc = (*fr.as_ptr()).data as *mut ffi::AVHWFramesContext; let vkfc = (*fc).hwctx as *mut pf_ffvk::AVVulkanFramesContext; // MUTABLE_FORMAT: per-plane views (spec requirement); ALIAS is FFmpeg's default. // (`as _`: the FlagBits constants are i32 under MSVC, the img_flags field u32.) (*vkfc).img_flags = (pf_ffvk::VkImageCreateFlagBits_VK_IMAGE_CREATE_MUTABLE_FORMAT_BIT | pf_ffvk::VkImageCreateFlagBits_VK_IMAGE_CREATE_ALIAS_BIT) as _; - let r = ffi::av_hwframe_ctx_init(fr); + let r = ffi::av_hwframe_ctx_init(fr.as_ptr()); if r < 0 { tracing::warn!(code = r, "av_hwframe_ctx_init(VULKAN) failed"); - let mut fr = fr; - ffi::av_buffer_unref(&mut fr); return ffi::AVPixelFormat::AV_PIX_FMT_NONE; } if !(*ctx).hw_frames_ctx.is_null() { ffi::av_buffer_unref(&mut (*ctx).hw_frames_ctx); } - (*ctx).hw_frames_ctx = fr; // the codec owns our ref now + // Ownership TRANSFERS to the codec here, so hand over the raw pointer and forget the + // wrapper — dropping it as well would be the double-unref `AvBuffer` exists to prevent. + (*ctx).hw_frames_ctx = fr.into_raw(); ffi::AVPixelFormat::AV_PIX_FMT_VULKAN } }