unom/punktfunk
6
0
Fork 1
Code Issues 2 Pull Requests Actions 8 Packages Projects Releases 41 Wiki Activity

refactor(encode): the VAAPI dmabuf path stops unwinding by hand

Browse Source 
`DmabufInner::open` builds four owned objects — a DRM device, a VAAPI device
derived from it, a DRM-PRIME frames context, and a filter graph — and every one of
its eight failure branches unwound them by hand. The same four-line block
(`avfilter_graph_free` + three `av_buffer_unref`s) appeared eight times, once
*inside a macro*, plus a ninth copy in `Drop`. Adding a step to that function meant
remembering to extend the unwind at exactly the right depth; getting it wrong leaks
a device per failed session (the persistent listener accumulates them) or frees one
twice.

All eight are gone. `AvBuffer` already owned the buffer refs; `AvFilterGraph` now
does the same for the graph, so each handle is owned the moment it exists and an
early `bail!` releases whatever was built so far. `open` lost ~40 lines of cleanup
and gained none.

Field order in `DmabufInner` is load-bearing and says so: graph, frames, VAAPI
device, DRM device, then `enc` LAST. Fields drop in declaration order, and that
sequence reproduces the old hand-written `Drop` exactly — including that it ran
ahead of every field, so all four were released before ffmpeg-next dropped the
encoder. Everything here holds its own reference, so refcounting makes any order
sound; the ordering is pinned so a future reorder cannot quietly change what ships.

One subtlety preserved deliberately: the buffersrc parameters take `drm_frames`
BORROWED, not ref'd (`av_buffersrc_parameters_set` takes its own ref). That is now
`drm_frames.as_ptr()` — same borrow, same single owned ref, no new leak.

Verified on .21 (CachyOS, RTX 5070 Ti, FFmpeg 62): `cargo check -p pf-encode
--all-targets` clean at exit 0, `cargo test -p pf-encode` 33 passed / 0 failed, and
`cuda_hw_alloc_drop_cycles` still passes against real CUDA. vaapi.rs now contains
zero `av_buffer_unref` and zero `avfilter_graph_free` calls, down from 40 and 8.
The dmabuf path itself still needs AMD/Intel silicon to exercise end to end.
This commit is contained in:
enricobuehler
2026-07-28 21:31:41 +02:00
parent 7adc1db672
commit eb9c5be20d
2 changed files with 83 additions and 85 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/pf-encode/src/enc/linux/vaapi.rs
+46 -85
View File
@@ -36,8 +36,8 @@ use std::ptr;
use std::sync::{Mutex, OnceLock};
use super::libav::{
apply_low_latency_rc, pixel_to_av, poll_encoder, AvBuffer, PollOutcome, SWS_CS_ITU709,
SWS_POINT,
apply_low_latency_rc, pixel_to_av, poll_encoder, AvBuffer, AvFilterGraph, PollOutcome,
SWS_CS_ITU709, SWS_POINT,
};
use ffmpeg::ffi; // = ffmpeg_sys_next
@@ -738,14 +738,19 @@ impl Drop for CpuInner {
// ---------------------------------------------------------------------------------------------
struct DmabufInner {
enc: encoder::video::Encoder,
/// DRM device the source dmabuf frames reference (the buffersrc's `hw_frames_ctx` device).
drm_device: *mut ffi::AVBufferRef,
/// VAAPI device driving `hwmap`/`scale_vaapi`/the encoder.
vaapi_device: *mut ffi::AVBufferRef,
// FIELD ORDER IS LOAD-BEARING. These drop in declaration order, and this order reproduces
// exactly what the hand-written `Drop` this replaced did: graph, then frames, then the derived
// VAAPI device, then DRM — and `enc` last of all, since the old `Drop` ran before any field and
// so freed all four ahead of ffmpeg-next dropping the encoder. Every one of these holds its own
// reference, so refcounting makes any order sound; the point is that a field reorder must not
// silently change what ships. Do not move `enc`, and do not reorder the four below it.
graph: AvFilterGraph,
/// DRM-PRIME frames context for the imported dmabufs (buffersrc input).
drm_frames: *mut ffi::AVBufferRef,
graph: *mut ffi::AVFilterGraph,
drm_frames: AvBuffer,
/// VAAPI device driving `hwmap`/`scale_vaapi`/the encoder.
vaapi_device: AvBuffer,
/// DRM device the source dmabuf frames reference (the buffersrc's `hw_frames_ctx` device).
drm_device: AvBuffer,
src: *mut ffi::AVFilterContext,
sink: *mut ffi::AVFilterContext,
width: u32,
@@ -755,6 +760,10 @@ struct DmabufInner {
/// submit (import+push vs CSC pull vs encoder send), the stage that dominates AMD/Intel
/// host latency (7.9 ms p50 at 1440p on the 780M).
frames: u64,
/// Declared LAST on purpose — see the field-order note at the top of this struct. The encoder
/// holds its own device ref and must drop after the graph and the three buffers, which is what
/// the hand-written `Drop` used to guarantee by running ahead of every field.
enc: encoder::video::Encoder,
}
impl DmabufInner {
@@ -821,70 +830,58 @@ impl DmabufInner {
ffmpeg::Error::from(r)
);
}
// Own each handle the moment it exists: from here every `bail!` drops whatever has been
// built so far, in reverse order, so not one failure branch below carries cleanup code.
let drm_device = AvBuffer::from_raw(drm_device)
.context("av_hwdevice_ctx_create(DRM) gave no device")?;
let mut vaapi_device: *mut ffi::AVBufferRef = ptr::null_mut();
let r = ffi::av_hwdevice_ctx_create_derived(
&mut vaapi_device,
ffi::AVHWDeviceType::AV_HWDEVICE_TYPE_VAAPI,
drm_device,
drm_device.as_ptr(),
0,
);
if r < 0 {
ffi::av_buffer_unref(&mut drm_device);
bail!("derive VAAPI from DRM: {}", ffmpeg::Error::from(r));
}
let vaapi_device = AvBuffer::from_raw(vaapi_device)
.context("av_hwdevice_ctx_create_derived(VAAPI) gave no device")?;
// DRM-PRIME frames context for the imported dmabufs.
let mut drm_frames = ffi::av_hwframe_ctx_alloc(drm_device);
if drm_frames.is_null() {
ffi::av_buffer_unref(&mut vaapi_device);
ffi::av_buffer_unref(&mut drm_device);
bail!("av_hwframe_ctx_alloc(DRM) failed");
}
let fc = (*drm_frames).data as *mut ffi::AVHWFramesContext;
let drm_frames = AvBuffer::from_raw(ffi::av_hwframe_ctx_alloc(drm_device.as_ptr()))
.context("av_hwframe_ctx_alloc(DRM) failed")?;
let fc = (*drm_frames.as_ptr()).data as *mut ffi::AVHWFramesContext;
(*fc).format = ffi::AVPixelFormat::AV_PIX_FMT_DRM_PRIME;
(*fc).sw_format = sw_format; // packed XR24 RGB plane, or XR30/XB30 for HDR
(*fc).width = width as c_int;
(*fc).height = height as c_int;
if ffi::av_hwframe_ctx_init(drm_frames) < 0 {
ffi::av_buffer_unref(&mut drm_frames);
ffi::av_buffer_unref(&mut vaapi_device);
ffi::av_buffer_unref(&mut drm_device);
if ffi::av_hwframe_ctx_init(drm_frames.as_ptr()) < 0 {
bail!("av_hwframe_ctx_init(DRM) failed");
}
// Filter graph: buffer(drm_prime) → hwmap=derive_device=vaapi:mode=read →
// scale_vaapi=format=nv12 → buffersink.
let mut graph = ffi::avfilter_graph_alloc();
if graph.is_null() {
ffi::av_buffer_unref(&mut drm_frames);
ffi::av_buffer_unref(&mut vaapi_device);
ffi::av_buffer_unref(&mut drm_device);
bail!("avfilter_graph_alloc failed");
}
let graph = AvFilterGraph::alloc().context("avfilter_graph_alloc failed")?;
let mk = |name: &CStr, inst: &CStr| -> *mut ffi::AVFilterContext {
let f = ffi::avfilter_get_by_name(name.as_ptr());
if f.is_null() {
return ptr::null_mut();
}
ffi::avfilter_graph_alloc_filter(graph, f, inst.as_ptr())
ffi::avfilter_graph_alloc_filter(graph.as_ptr(), f, inst.as_ptr())
};
let src = mk(c"buffer", c"in");
let hwmap = mk(c"hwmap", c"map");
let scale = mk(c"scale_vaapi", c"csc");
let sink = mk(c"buffersink", c"out");
if src.is_null() || hwmap.is_null() || scale.is_null() || sink.is_null() {
ffi::avfilter_graph_free(&mut graph);
ffi::av_buffer_unref(&mut drm_frames);
ffi::av_buffer_unref(&mut vaapi_device);
ffi::av_buffer_unref(&mut drm_device);
bail!("a VAAPI filter (buffer/hwmap/scale_vaapi/buffersink) is missing");
}
// hwmap maps the DRM-PRIME input onto THIS vaapi device; scale_vaapi runs the CSC on
// it. Giving both our device (rather than `hwmap=derive_device`) keeps every surface —
// and the sink's output frames ctx the encoder adopts — on one VADisplay.
(*hwmap).hw_device_ctx = ffi::av_buffer_ref(vaapi_device);
(*scale).hw_device_ctx = ffi::av_buffer_ref(vaapi_device);
(*hwmap).hw_device_ctx = ffi::av_buffer_ref(vaapi_device.as_ptr());
(*scale).hw_device_ctx = ffi::av_buffer_ref(vaapi_device.as_ptr());
// buffersrc params: DRM-PRIME frames, the drm_frames ctx.
let par = ffi::av_buffersrc_parameters_alloc();
@@ -905,24 +902,16 @@ impl DmabufInner {
// the struct, not the ref. Our single owned `drm_frames` ref is retained, lives in
// `DmabufInner`, and is unref'd in `Drop`. Wrapping it in `av_buffer_ref` here would leak
// that extra ref every session (the persistent listener would accumulate them).
(*par).hw_frames_ctx = drm_frames;
(*par).hw_frames_ctx = drm_frames.as_ptr();
let r = ffi::av_buffersrc_parameters_set(src, par);
ffi::av_free(par as *mut _);
if r < 0 {
ffi::avfilter_graph_free(&mut graph);
ffi::av_buffer_unref(&mut drm_frames);
ffi::av_buffer_unref(&mut vaapi_device);
ffi::av_buffer_unref(&mut drm_device);
bail!("av_buffersrc_parameters_set failed ({r})");
}
macro_rules! init {
($ctx:expr, $args:expr, $what:literal) => {{
let r = ffi::avfilter_init_str($ctx, $args);
if r < 0 {
ffi::avfilter_graph_free(&mut graph);
ffi::av_buffer_unref(&mut drm_frames);
ffi::av_buffer_unref(&mut vaapi_device);
ffi::av_buffer_unref(&mut drm_device);
bail!(concat!("init ", $what, " failed ({})"), r);
}
}};
@@ -936,28 +925,16 @@ impl DmabufInner {
ffi::avfilter_link(a, 0, b, 0)
};
if link(src, hwmap) < 0 || link(hwmap, scale) < 0 || link(scale, sink) < 0 {
ffi::avfilter_graph_free(&mut graph);
ffi::av_buffer_unref(&mut drm_frames);
ffi::av_buffer_unref(&mut vaapi_device);
ffi::av_buffer_unref(&mut drm_device);
bail!("avfilter_link failed");
}
let r = ffi::avfilter_graph_config(graph, ptr::null_mut());
let r = ffi::avfilter_graph_config(graph.as_ptr(), ptr::null_mut());
if r < 0 {
ffi::avfilter_graph_free(&mut graph);
ffi::av_buffer_unref(&mut drm_frames);
ffi::av_buffer_unref(&mut vaapi_device);
ffi::av_buffer_unref(&mut drm_device);
bail!("avfilter_graph_config failed ({r})");
}
// The encoder takes NV12 surfaces from the sink's output frames context.
let nv12_ctx = ffi::av_buffersink_get_hw_frames_ctx(sink);
if nv12_ctx.is_null() {
ffi::avfilter_graph_free(&mut graph);
ffi::av_buffer_unref(&mut drm_frames);
ffi::av_buffer_unref(&mut vaapi_device);
ffi::av_buffer_unref(&mut drm_device);
bail!("filter sink has no VAAPI frames context");
}
// On encoder-open failure, free the graph + our owned buffer refs before bailing (matching
@@ -971,16 +948,12 @@ impl DmabufInner {
height,
fps,
bitrate_bps,
vaapi_device,
vaapi_device.as_ptr(),
nv12_ctx,
ten_bit,
) {
Ok(enc) => enc,
Err(e) => {
ffi::avfilter_graph_free(&mut graph);
ffi::av_buffer_unref(&mut drm_frames);
ffi::av_buffer_unref(&mut vaapi_device);
ffi::av_buffer_unref(&mut drm_device);
return Err(e);
}
};
@@ -991,17 +964,17 @@ impl DmabufInner {
if ten_bit { "P010 (HDR10)" } else { "NV12" }
);
Ok(DmabufInner {
enc,
drm_device,
vaapi_device,
drm_frames,
graph,
drm_frames,
vaapi_device,
drm_device,
src,
sink,
width,
height,
fourcc: drm_fourcc,
frames: 0,
enc,
})
}
}
@@ -1076,7 +1049,7 @@ impl DmabufInner {
// Mesa's is BT.601, contradicting the BT.709-limited VUI the encoder signals).
(*drm).color_range = ffi::AVColorRange::AVCOL_RANGE_JPEG;
(*drm).colorspace = ffi::AVColorSpace::AVCOL_SPC_RGB;
(*drm).hw_frames_ctx = ffi::av_buffer_ref(self.drm_frames);
(*drm).hw_frames_ctx = ffi::av_buffer_ref(self.drm_frames.as_ptr());
(*drm).data[0] = Box::into_raw(desc) as *mut u8;
// Own the descriptor so it frees with the frame (the fd is owned by the DmabufFrame,
// which outlives this call — the graph reads the surface before submit returns).
@@ -1156,23 +1129,11 @@ impl DmabufInner {
}
}
impl Drop for DmabufInner {
fn drop(&mut self) {
// SAFETY: `graph`/`drm_frames`/`vaapi_device`/`drm_device` are the non-null objects
// `DmabufInner::open` built and moved into `self` (open bails before constructing `Self` if any
// alloc fails). `avfilter_graph_free` frees the graph (and the per-filter device refs it owns);
// each `av_buffer_unref` drops one ref and nulls the pointer via `&mut`. `DmabufInner` owns all
// four exclusively and `Drop` runs once → no double-free/use-after-free. The graph is freed
// first (it holds refs on the devices), then frames, then the derived VAAPI device, then DRM.
// (`self.enc` drops via ffmpeg-next afterward, holding its own refs.)
unsafe {
ffi::avfilter_graph_free(&mut self.graph);
ffi::av_buffer_unref(&mut self.drm_frames);
ffi::av_buffer_unref(&mut self.vaapi_device);
ffi::av_buffer_unref(&mut self.drm_device);
}
}
}
// No `Drop` for `DmabufInner`: `AvFilterGraph`/`AvBuffer` each free themselves, in field-declaration
// order — graph, frames, derived VAAPI device, DRM device, then `enc` — which is exactly the order
// the hand-written `Drop` this replaced used. That `Drop` had to be kept in step with eight separate
// failure branches in `open`, each repeating the same four-line unwind; there is now one release
// path per handle and no branch can skip it.
// ---------------------------------------------------------------------------------------------