sassycrown/punktfunk
1
0
Fork 0
forked from unom/punktfunk
Code Pull Requests Activity

refactor(client): the hardware decoders own their hwdevice ref

Browse Source 
`VaapiDecoder::new` and `VulkanDecoder::new` create a hwdevice and then do several
more fallible things with it — resolve `vkWaitSemaphores`, find the decoder, alloc
the context, open it — and every one of those branches unref'd the device by hand,
with a `Drop` doing it once more. Six hand-written unrefs between them, each one a
line somebody has to remember when adding a step.

`video_libav::AvBuffer` owns it instead, so an early `bail!` releases it and the
branches carry nothing. It is a deliberate second copy of `pf-encode`'s type: the
crates do not depend on each other (host encode and client decode share no code
path), and this one needs something the host's does not — see below — so hoisting
it to a shared crate would mean giving that crate an ffmpeg dependency and both
sets of semantics to save about twenty lines.

That extra piece is `into_raw`. `pick_vulkan` hands its frames context to the codec
(`(*ctx).hw_frames_ctx = fr` — the codec unrefs it when the context closes), so the
wrapper must give up ownership rather than drop. Making the transfer explicit is
the point: dropping an `AvBuffer` there too would be exactly the double-unref this
type exists to prevent.

Two `av_buffer_unref` calls survive in `video_vulkan.rs` on purpose. One runs before
ownership is taken (the `av_hwdevice_ctx_init` failure, ahead of `from_raw`); the
other releases the codec's OWN pre-existing frames ctx before we replace it, which
was never ours to model.

Field order preserved: `hw_device` stays declared after `ctx`, so it still releases
after each `Drop` frees packet/frame/context — the order the hand-written unref had.

Verified on .21 (CachyOS, FFmpeg 62): `cargo check --workspace --all-targets` clean
at exit 0 with zero errors — the workspace-wide check owed since the AvBuffer
commit — plus pf-client-core 34 passed / 0 failed and pf-encode 33 passed / 0
failed. The decoders themselves still need real VAAPI/Vulkan playback to exercise.
This commit is contained in:
enricobuehler
2026-07-28 21:31:41 +02:00
parent eb9c5be20d
commit a42ab075a8
4 changed files with 92 additions and 19 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/pf-client-core/src/lib.rs
+3
View File
@@ -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))]
crates/pf-client-core/src/video_libav.rs
+59
View File
@@ -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<Self> {
(!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) };
}
}
crates/pf-client-core/src/video_vaapi.rs
+10 -7
View File
@@ -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.
}
}
}
crates/pf-client-core/src/video_vulkan.rs
+20 -12
View File
@@ -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
}
}