From 4c6df69fcfab3b96ee2262ec6e7ab8020ddc4d03 Mon Sep 17 00:00:00 2001 From: Benjamin Freeman Date: Wed, 26 Aug 2026 20:40:42 +0200 Subject: [PATCH 1/9] feat(capture): negotiate tiled dmabuf and plumb the GPU descriptor (#507) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Whole-monitor recordings freeze on GNOME/Wayland+AMD because the helper only advertised LINEAR/INVALID dmabuf modifiers, so a tiled monitor buffer could not negotiate as dmabuf and fell back to shm — which mutter throttles hard for a full monitor (measured ~2-11 distinct fps vs OBS's ~24 over dmabuf). Foundation for the zero-copy dmabuf -> VAAPI path (import lands next): - Enumerate the GPU's importable DRM modifiers via EGL surfaceless (dmabuf_modifiers.c, dlopen'd libEGL — no new hard dep) and advertise them in the dmabuf EnumFormat. Validated: mutter now negotiates a tiled dmabuf. - Carry a tiled buffer up as a raw descriptor (fd + modifier + fourcc + plane offsets/strides) instead of force-mmap'ing it; osc_on_add_buffer latches import mode on mmap failure rather than erroring. shm and linear-dmabuf paths unchanged. - ffmpeg DRM bindings (hwcontext_drm.h) for the upcoming av_hwframe_map. - Rust RawFrame mirrors the new descriptor; on_frame skips dmabuf frames until the importer lands (safe no-op). See docs/dmabuf-vaapi-plan.md. Co-Authored-By: Claude Opus 4.8 (1M context) --- electron/native/pipewire-capture/build.rs | 3 +- .../pipewire-capture/csrc/dmabuf_modifiers.c | 94 +++++++++ .../pipewire-capture/csrc/dmabuf_modifiers.h | 20 ++ .../native/pipewire-capture/csrc/pw_shim.c | 183 +++++++++++++----- .../native/pipewire-capture/csrc/pw_shim.h | 20 ++ .../docs/dmabuf-vaapi-plan.md | 108 +++++++++++ electron/native/pipewire-capture/src/shim.rs | 11 ++ 7 files changed, 388 insertions(+), 51 deletions(-) create mode 100644 electron/native/pipewire-capture/csrc/dmabuf_modifiers.c create mode 100644 electron/native/pipewire-capture/csrc/dmabuf_modifiers.h create mode 100644 electron/native/pipewire-capture/docs/dmabuf-vaapi-plan.md diff --git a/electron/native/pipewire-capture/build.rs b/electron/native/pipewire-capture/build.rs index 43ff9d6d4..7c863210f 100644 --- a/electron/native/pipewire-capture/build.rs +++ b/electron/native/pipewire-capture/build.rs @@ -24,7 +24,7 @@ fn main() { fn build_pipewire_shim(root: &Path) { let vendor = root.join("vendor/pipewire-1.0.5/include"); - let sources = ["csrc/pw_shim.c", "csrc/pw_audio.c"]; + let sources = ["csrc/pw_shim.c", "csrc/pw_audio.c", "csrc/dmabuf_modifiers.c"]; assert!( vendor.join("pipewire/pipewire.h").is_file(), @@ -167,6 +167,7 @@ fn link_ffmpeg(root: &Path) { #include #include #include + #include #include #include #include diff --git a/electron/native/pipewire-capture/csrc/dmabuf_modifiers.c b/electron/native/pipewire-capture/csrc/dmabuf_modifiers.c new file mode 100644 index 000000000..96f1849c8 --- /dev/null +++ b/electron/native/pipewire-capture/csrc/dmabuf_modifiers.c @@ -0,0 +1,94 @@ +#include "dmabuf_modifiers.h" + +#include +#include + +/* + * Minimal EGL surface, spelled out rather than pulled from so the + * build needs no EGL dev package (same reasoning as the DRM modifier constants + * in pw_shim.c). libEGL itself is dlopen'd at runtime; if it is absent the + * caller degrades to the LINEAR/INVALID offer. + */ +typedef void *EGLDisplay; +typedef unsigned int EGLBoolean; +typedef int EGLint; +typedef intptr_t EGLAttrib; +typedef uint64_t EGLuint64KHR; + +#define OSC_EGL_TRUE 1 +#define OSC_EGL_NO_DISPLAY ((EGLDisplay)0) +#define OSC_EGL_DEFAULT_DISPLAY ((void *)0) +/* EGL_MESA_platform_surfaceless — a display with no window system, exactly what + * a one-shot capability query wants. */ +#define OSC_EGL_PLATFORM_SURFACELESS_MESA 0x31DD + +typedef void *(*osc_eglGetProcAddress)(const char *); +typedef EGLDisplay (*osc_eglGetPlatformDisplay)(EGLint platform, void *native, + const EGLAttrib *attrib_list); +typedef EGLBoolean (*osc_eglInitialize)(EGLDisplay, EGLint *major, EGLint *minor); +typedef EGLBoolean (*osc_eglTerminate)(EGLDisplay); +typedef EGLBoolean (*osc_eglQueryDmaBufModifiersEXT)(EGLDisplay, EGLint format, + EGLint max_modifiers, + EGLuint64KHR *modifiers, + EGLBoolean *external_only, + EGLint *num_modifiers); + +int osc_query_dmabuf_modifiers(uint32_t fourcc, uint64_t *out, int max_out) +{ + if (out == NULL || max_out <= 0) { + return 0; + } + + /* RTLD_NODELETE: EGL keeps process-global state, so never let dlclose run + * its destructors — we deliberately do not dlclose at all. */ + void *egl = dlopen("libEGL.so.1", RTLD_NOW | RTLD_LOCAL | RTLD_NODELETE); + if (egl == NULL) { + return 0; + } + + osc_eglGetProcAddress get_proc = + (osc_eglGetProcAddress)dlsym(egl, "eglGetProcAddress"); + osc_eglInitialize egl_init = (osc_eglInitialize)dlsym(egl, "eglInitialize"); + osc_eglTerminate egl_terminate = (osc_eglTerminate)dlsym(egl, "eglTerminate"); + if (get_proc == NULL || egl_init == NULL || egl_terminate == NULL) { + return 0; + } + + osc_eglGetPlatformDisplay get_display = + (osc_eglGetPlatformDisplay)get_proc("eglGetPlatformDisplayEXT"); + osc_eglQueryDmaBufModifiersEXT query_mods = + (osc_eglQueryDmaBufModifiersEXT)get_proc("eglQueryDmaBufModifiersEXT"); + if (get_display == NULL || query_mods == NULL) { + return 0; + } + + EGLDisplay dpy = get_display(OSC_EGL_PLATFORM_SURFACELESS_MESA, + OSC_EGL_DEFAULT_DISPLAY, NULL); + if (dpy == OSC_EGL_NO_DISPLAY) { + return 0; + } + if (egl_init(dpy, NULL, NULL) != OSC_EGL_TRUE) { + return 0; + } + + int written = 0; + EGLint count = 0; + if (query_mods(dpy, (EGLint)fourcc, 0, NULL, NULL, &count) == OSC_EGL_TRUE && + count > 0) { + EGLuint64KHR mods[128]; + EGLBoolean external[128]; + EGLint cap = (EGLint)(sizeof(mods) / sizeof(mods[0])); + if (count > cap) { + count = cap; + } + if (query_mods(dpy, (EGLint)fourcc, count, mods, external, &count) == + OSC_EGL_TRUE) { + for (EGLint i = 0; i < count && written < max_out; i++) { + out[written++] = (uint64_t)mods[i]; + } + } + } + + egl_terminate(dpy); + return written; +} diff --git a/electron/native/pipewire-capture/csrc/dmabuf_modifiers.h b/electron/native/pipewire-capture/csrc/dmabuf_modifiers.h new file mode 100644 index 000000000..be6ac1212 --- /dev/null +++ b/electron/native/pipewire-capture/csrc/dmabuf_modifiers.h @@ -0,0 +1,20 @@ +#ifndef OSC_DMABUF_MODIFIERS_H +#define OSC_DMABUF_MODIFIERS_H + +#include + +/* + * Query the DRM format modifiers the local GPU's EGL stack can import for a + * given DRM fourcc (e.g. XRGB8888). These are the modifiers we can legitimately + * advertise to the compositor in the dmabuf EnumFormat: a compositor buffer + * whose modifier is in this set is one we can hand to VAAPI. Fills `out` with up + * to `max_out` modifiers and returns the count, or 0 when enumeration is + * unavailable (no libEGL, no surfaceless platform, driver refuses) — in which + * case the caller falls back to LINEAR/INVALID only. + * + * libEGL is loaded with dlopen, matching how this crate treats libpipewire: the + * helper stays buildable and runnable on a box without EGL dev packages. + */ +int osc_query_dmabuf_modifiers(uint32_t fourcc, uint64_t *out, int max_out); + +#endif diff --git a/electron/native/pipewire-capture/csrc/pw_shim.c b/electron/native/pipewire-capture/csrc/pw_shim.c index 82a8a129e..ece1e67ad 100644 --- a/electron/native/pipewire-capture/csrc/pw_shim.c +++ b/electron/native/pipewire-capture/csrc/pw_shim.c @@ -45,6 +45,7 @@ #include #include "pw_shim.h" +#include "dmabuf_modifiers.h" /* Defined next to osc_map_dmabuf; used earlier, at format negotiation. */ static int osc_debug_enabled(void); @@ -61,16 +62,43 @@ static int osc_debug_enabled(void); * header would put libdrm-dev in the build path of every contributor and CI * runner for two integers. That is the same trade the dlopen above makes. * - * These two are the ONLY modifiers this helper advertises, and the reason is - * osc_map_dmabuf(): a linear or implicit buffer can be read through a plain - * mmap of the dmabuf fd, while a tiled or compression-enabled one cannot — its - * bytes are not in raster order, so handing them to the encoder would produce a - * scrambled recording rather than an error. Anything else needs a real GPU - * import (EGL/gbm), which this helper deliberately does not link. + * LINEAR and INVALID are the universal fallbacks: a linear or implicit buffer + * can be read through a plain mmap of the dmabuf fd (osc_map_dmabuf). Tiled or + * compression-enabled buffers cannot — their bytes are not in raster order — so + * they require a real GPU import, which is being added for the VAAPI path (see + * issue #507 and docs/dmabuf-vaapi-plan.md). The additional importable modifiers + * are enumerated at runtime via EGL (osc_query_dmabuf_modifiers). */ #define OSC_DRM_FORMAT_MOD_LINEAR 0ULL #define OSC_DRM_FORMAT_MOD_INVALID 0x00ffffffffffffffULL +/* DRM fourccs for the 32-bit RGB formats we offer. XRGB8888 = fourcc('X','R', + * '2','4'); the others follow the same little-endian spelling. Used to enumerate + * importable modifiers and to describe a dmabuf to the GPU importer. Spelled out + * for the same reason as the modifiers above. */ +#define OSC_DRM_FORMAT_XRGB8888 0x34325258u /* SPA BGRx */ +#define OSC_DRM_FORMAT_ARGB8888 0x34325241u /* SPA BGRA */ +#define OSC_DRM_FORMAT_XBGR8888 0x34324258u /* SPA RGBx */ +#define OSC_DRM_FORMAT_ABGR8888 0x34324241u /* SPA RGBA */ + +/* SPA video format (byte order B,G,R,x ...) → the matching DRM fourcc (a + * little-endian 32-bit word), for the GPU dmabuf import. 0 = unmapped. */ +static uint32_t osc_spa_format_to_drm_fourcc(uint32_t spa_format) +{ + switch (spa_format) { + case SPA_VIDEO_FORMAT_BGRx: + return OSC_DRM_FORMAT_XRGB8888; + case SPA_VIDEO_FORMAT_BGRA: + return OSC_DRM_FORMAT_ARGB8888; + case SPA_VIDEO_FORMAT_RGBx: + return OSC_DRM_FORMAT_XBGR8888; + case SPA_VIDEO_FORMAT_RGBA: + return OSC_DRM_FORMAT_ABGR8888; + default: + return 0; + } +} + /* * Mapped dmabuf fds, keyed by fd. * @@ -182,6 +210,10 @@ struct osc_pw_session { /* Set from the negotiated format's SPA_VIDEO_FLAG_MODIFIER, which is what * decides whether buffers arrive as dmabuf fds or shared memory. */ int uses_dmabuf; + /* Latched when a dmabuf buffer cannot be CPU-mmap'd (a tiled buffer on, e.g., + * AMD/mutter). Frames then travel as raw dmabuf descriptors for a GPU import + * (issue #507) instead of the shared-memory path. */ + int import_dmabuf; struct osc_dmabuf_map dmabuf_maps[OSC_MAX_DMABUF_MAPS]; /* fd whose DMA_BUF_SYNC_START has not been closed by its END yet, or -1. * The bracket has to span the on_frame callback, not just osc_read_frame, @@ -415,9 +447,22 @@ static const struct spa_pod *osc_build_enum_format_dmabuf(struct spa_pod_builder * tolerating the key. */ spa_pod_builder_prop(builder, SPA_FORMAT_VIDEO_modifier, SPA_POD_PROP_FLAG_MANDATORY); spa_pod_builder_push_choice(builder, &choice_frame, SPA_CHOICE_Enum, 0); - /* Default first, then every alternative — the default is repeated, same + /* Advertise the modifiers our GPU's EGL can import, so a tiled compositor + * buffer — the common case on AMD/mutter — negotiates as dmabuf instead of + * falling back to the throttled shm path (issue #507). LINEAR and INVALID + * stay as universal fallbacks. Modifiers match across the 32-bit RGB formats + * we offer, so enumerating XRGB8888 is representative. + * + * Default first, then every alternative — the default is repeated, same * idiom as SPA_POD_CHOICE_ENUM_Id above. */ - spa_pod_builder_long(builder, (int64_t)OSC_DRM_FORMAT_MOD_LINEAR); + uint64_t egl_mods[128]; + int egl_mod_count = osc_query_dmabuf_modifiers(OSC_DRM_FORMAT_XRGB8888, egl_mods, 128); + int64_t default_mod = + egl_mod_count > 0 ? (int64_t)egl_mods[0] : (int64_t)OSC_DRM_FORMAT_MOD_LINEAR; + spa_pod_builder_long(builder, default_mod); + for (int i = 0; i < egl_mod_count; i++) { + spa_pod_builder_long(builder, (int64_t)egl_mods[i]); + } spa_pod_builder_long(builder, (int64_t)OSC_DRM_FORMAT_MOD_LINEAR); spa_pod_builder_long(builder, (int64_t)OSC_DRM_FORMAT_MOD_INVALID); spa_pod_builder_pop(builder, &choice_frame); @@ -805,23 +850,17 @@ static void osc_on_add_buffer(void *userdata, struct pw_buffer *pw_buf) maplen = data->maxsize; session->dmabuf_maps[i].ptr = osc_map_dmabuf((int)data->fd, &maplen, &why); if (session->dmabuf_maps[i].ptr == NULL) { - /* Reported once, through the buffer-info channel that already exists - * for describing what the compositor handed us — a mapping failure - * here means no frames at all, and silence would read as a hang. - * - * The reason is carried up rather than assumed: this used to say the - * driver refused CPU mapping no matter what actually went wrong, and - * that message sent the one real investigation of this path looking - * at the GPU for a size the compositor had simply left at 0. */ - if (session->callbacks.on_buffer_info != NULL && - session->buffer_info_reports < OSC_BUFFER_INFO_REPORTS) { - char detail[256]; - - snprintf(detail, sizeof(detail), "dmabuf import failed: %s; capture cannot proceed", - why); - session->buffer_info_reports++; - session->callbacks.on_buffer_info(session->callbacks.user, data->type, - pw_buf->buffer->n_datas, 0, 0, detail); + /* + * A mmap failure on a dmabuf is the tiled-buffer case (e.g. a whole + * monitor on AMD/mutter): the bytes are not in raster order and the + * driver refuses CPU access. That is no longer fatal — the frame + * instead travels as a raw dmabuf descriptor for a GPU import (see + * osc_read_frame and issue #507). Latch the mode; osc_read_frame will + * populate the descriptor from the same fd. No mapping is stored. + */ + session->import_dmabuf = 1; + if (osc_debug_enabled()) { + fprintf(stderr, "[osc-dmabuf] mmap failed (%s) — using GPU import path\n", why); } return; } @@ -985,6 +1024,7 @@ static int osc_read_frame(struct osc_pw_session *session, const struct spa_buffe uint32_t size; int32_t stride; int32_t height; + int is_dmabuf_import = 0; const uint8_t *base; @@ -1008,7 +1048,12 @@ static int osc_read_frame(struct osc_pw_session *session, const struct spa_buffe */ base = osc_find_dmabuf_map(session, (int)data->fd); if (base == NULL) { - return 0; + if (!session->import_dmabuf) { + return 0; + } + /* Tiled dmabuf: no CPU mapping exists. It travels up as a raw + * descriptor for a GPU import instead of being read here. */ + is_dmabuf_import = 1; } } else if (data->data == NULL) { /* @@ -1025,36 +1070,74 @@ static int osc_read_frame(struct osc_pw_session *session, const struct spa_buffe return 0; } - offset = SPA_MIN(data->chunk->offset, data->maxsize); - size = SPA_MIN(data->chunk->size, data->maxsize - offset); - height = (int32_t)session->format.size.height; stride = data->chunk->stride; - if (stride <= 0 || height <= 0) { - return 0; - } - /* One short row is one row of garbage in the recording; refuse the whole - * frame instead, and let the caller count it as dropped. */ - if ((uint64_t)stride * (uint64_t)height > (uint64_t)size) { + if (height <= 0) { return 0; } - /* - * Open the CPU-access window on a dmabuf and leave it open: the pixels are - * read by the on_frame callback, not here, so the matching SYNC_END lives in - * osc_inspect_buffer once that callback has returned. - */ - if (data->type == SPA_DATA_DmaBuf) { - session->dmabuf_sync_fd = (int)data->fd; - osc_dmabuf_sync(session->dmabuf_sync_fd, 1); - } + if (is_dmabuf_import) { + /* + * GPU import path. The buffer is not CPU-readable, so the raster bounds + * checks below do not apply — the modifier is what makes the producer's + * strides/offsets meaningful, and the importer validates the rest. We + * hand up the fd(s), modifier and fourcc; no SYNC bracket is opened + * because nothing here touches the pixels. n_datas is the plane count for + * a dmabuf (one per plane); our RGB formats are single-plane. + */ + uint32_t fourcc = osc_spa_format_to_drm_fourcc(session->format.format); + int32_t import_stride = + stride > 0 ? stride : (int32_t)session->format.size.width * 4; + int32_t p; + if (fourcc == 0) { + return 0; + } + out->is_dmabuf = 1; + out->data = NULL; + out->size = 0; + out->stride = import_stride; + out->width = (int32_t)session->format.size.width; + out->height = height; + out->video_format = session->format.format; + out->modifier = session->format.modifier; + out->drm_fourcc = fourcc; + out->n_planes = (int32_t)buffer->n_datas > 4 ? 4 : (int32_t)buffer->n_datas; + for (p = 0; p < out->n_planes; p++) { + const struct spa_data *pd = &buffer->datas[p]; + out->plane_fd[p] = (int)pd->fd; + out->plane_offset[p] = pd->chunk != NULL ? (int32_t)pd->chunk->offset : 0; + out->plane_stride[p] = + (pd->chunk != NULL && pd->chunk->stride > 0) ? pd->chunk->stride : import_stride; + } + } else { + offset = SPA_MIN(data->chunk->offset, data->maxsize); + size = SPA_MIN(data->chunk->size, data->maxsize - offset); + if (stride <= 0) { + return 0; + } + /* One short row is one row of garbage in the recording; refuse the whole + * frame instead, and let the caller count it as dropped. */ + if ((uint64_t)stride * (uint64_t)height > (uint64_t)size) { + return 0; + } + + /* + * Open the CPU-access window on a dmabuf and leave it open: the pixels + * are read by the on_frame callback, not here, so the matching SYNC_END + * lives in osc_inspect_buffer once that callback has returned. + */ + if (data->type == SPA_DATA_DmaBuf) { + session->dmabuf_sync_fd = (int)data->fd; + osc_dmabuf_sync(session->dmabuf_sync_fd, 1); + } - out->data = SPA_PTROFF(base, offset, const uint8_t); - out->size = size; - out->stride = stride; - out->width = (int32_t)session->format.size.width; - out->height = height; - out->video_format = session->format.format; + out->data = SPA_PTROFF(base, offset, const uint8_t); + out->size = size; + out->stride = stride; + out->width = (int32_t)session->format.size.width; + out->height = height; + out->video_format = session->format.format; + } header = spa_buffer_find_meta_data(buffer, SPA_META_Header, sizeof(*header)); if (header != NULL) { diff --git a/electron/native/pipewire-capture/csrc/pw_shim.h b/electron/native/pipewire-capture/csrc/pw_shim.h index ab6f78305..878f2445c 100644 --- a/electron/native/pipewire-capture/csrc/pw_shim.h +++ b/electron/native/pipewire-capture/csrc/pw_shim.h @@ -94,6 +94,26 @@ struct osc_pw_frame { * both draw the line in exactly this place. */ int has_crop; + + /* + * Zero-copy dmabuf hand-off (issue #507). When `is_dmabuf` is 1, `data` is + * NULL and the frame is not CPU-readable — a tiled compositor buffer that + * lives on the GPU. The consumer imports it as a VAAPI surface from the + * descriptor below instead of reading `data`. When 0, the CPU path above + * applies unchanged (shm, or a linear/implicit dmabuf we could mmap). + * + * The fds are BORROWED for the callback's duration only, exactly like + * `data`: the buffer re-queues to the compositor when the callback returns, + * so the import (map + GPU copy into an owned surface) must complete before + * then. `modifier`/`drm_fourcc` describe the tiling and pixel layout. + */ + int is_dmabuf; + uint64_t modifier; /* DRM format modifier of the buffer */ + uint32_t drm_fourcc; /* DRM fourcc matching `video_format` */ + int32_t n_planes; /* number of populated plane_* entries (1..4) */ + int plane_fd[4]; + int32_t plane_offset[4]; + int32_t plane_stride[4]; }; /* The negotiated video format. Reported once, from param_changed. */ diff --git a/electron/native/pipewire-capture/docs/dmabuf-vaapi-plan.md b/electron/native/pipewire-capture/docs/dmabuf-vaapi-plan.md new file mode 100644 index 000000000..15ac518e8 --- /dev/null +++ b/electron/native/pipewire-capture/docs/dmabuf-vaapi-plan.md @@ -0,0 +1,108 @@ +# Zero-copy dmabuf → VAAPI capture (fix for #507) + +## Problem + +On GNOME/mutter + AMD, the helper only advertises `LINEAR`/`INVALID` dmabuf +modifiers (it reads frames via CPU `mmap`, which needs linear). AMD monitor +buffers are **tiled**, so dmabuf negotiation can't succeed and we fall back to +**shm/memfd**. mutter throttles whole-monitor shm delivery hard (GPU→CPU copy +per frame), starving the recorder to ~2–11 distinct fps while OBS gets ~24 over +dmabuf. Result: whole-screen recordings look frozen. Window capture is less +affected (smaller surface → shm copy keeps up). + +Goal: import the compositor's **tiled** dmabuf directly as a VAAPI surface and +encode with the existing `h264_vaapi` path — no CPU readback, no shm. + +## Constraint that shapes the design: the clock-driven encoder + +`capture.rs` writes constant-frame-rate output by *holding the last staged +picture* across gaps (a static screen delivers no frames). We therefore cannot +pin the PipeWire dmabuf across that gap — the pool is 4–16 buffers. So on each +arriving frame we must copy it into a surface **we own**, then requeue the +compositor buffer promptly. The copy stays on the GPU (VAAPI VPP), so it's cheap. + +## Pipeline (new dmabuf path, shm path kept as fallback) + +1. **Negotiation** — advertise the dmabuf modifiers our importer supports. + Enumerate them from the DRM render node (VAAPI/`vaQuerySurfaceAttributes` or + EGL `eglQueryDmaBufModifiersEXT`), per fourcc, like OBS. Offer that list in + `osc_build_enum_format_dmabuf` instead of just LINEAR/INVALID. +2. **C shim** — on `SPA_DATA_DmaBuf`, stop mmap'ing. Extract the raw descriptor: + fd(s), `format_modifier`, and per-plane `offset`/`stride`, plus fourcc. Pass + them to Rust via an extended `osc_pw_frame`/`RawFrame`. Keep the + `DMA_BUF_IOCTL_SYNC` bracket only for the (unused-on-dmabuf) CPU path. +3. **Frame lifecycle** — the mailbox must not `memcpy` for dmabuf. It holds the + descriptor + a handle that keeps the PipeWire buffer un-requeued until the + main loop imports it; newest-wins requeues the superseded buffer. Requeue + happens right after import (fast), never across the clock gap. +4. **Encoder** — build an `AVFrame` of `AV_PIX_FMT_DRM_PRIME` wrapping an + `AVDRMFrameDescriptor`, `av_hwframe_map()` it to a VAAPI frame (DRM→VAAPI + zero-copy), then VPP (`scale_vaapi`/`vpp_vaapi`) into our own NV12 VAAPI pool + surface — this also applies the **crop** (VideoCrop) on the GPU, replacing the + current CPU pointer-offset crop. That owned surface becomes the staged frame; + `encode_staged` sends it directly (no `av_hwframe_transfer_data` upload). +5. **Fallback** — the dmabuf path is NOT GNOME-specific: it applies to any + compositor that offers dmabuf (GNOME/mutter, KDE/kwin, most wlroots) whenever + the encoder is **VAAPI** (the default Linux backend with any GPU), so the large + majority of PipeWire desktop users benefit. shm stays as the fallback only for: + (a) compositors that offer *only* shm (some `xdg-desktop-portal-wlr` configs — + why shm is listed first today), and (b) non-VAAPI encoders (software + libopenh264 / Vulkan), whose dmabuf import isn't wired yet — they keep today's + shm + sws_scale + hwupload path. Also fall back if enumeration/import/VPP fails. + Never regress software-encode or shm-only-compositor users. + +## Decision: zero-copy (option B) + +Chosen over the pragmatic GPU-detile→CPU-readback path. The dmabuf stays on the +GPU end to end: `av_hwframe_map` (DRM_PRIME→VAAPI) → `scale_vaapi` VPP (format + +crop) into our own NV12 surface → encode. No CPU readback. + +Key architecture calls: +- **One shared VAAPI `AVHWDeviceContext`** created up front, used by BOTH the + importer (PipeWire thread) and the encoder (main loop). A single mutex guards + all VADisplay ops (import+VPP vs encode) since libva isn't thread-safe per + display. Contention is negligible (both are GPU-driven). +- **Import runs on the PipeWire thread inside `on_frame`**, while the PW buffer is + still held (before requeue), so the dmabuf content is stable during the map+VPP + copy. The result is our own NV12 VAAPI surface (ref-counted `AVFrame`) placed in + the mailbox; the PW buffer requeues immediately after. This preserves the + clock-driven hold (we own the surface; the compositor buffer is returned). +- **v1 targets full monitor (no crop)**: importer/VPP sized to the stream at + `stream-started`. Window crop via VPP is a follow-up. +- **Fallback** to the existing shm + sws_scale + hwupload path when: backend isn't + VAAPI, the compositor only offers shm, or any of map/VPP/import fails. + +## Status / steps + +- [x] **Foundation**: generate ffmpeg DRM bindings (`build.rs` + + `hwcontext_drm.h`). Verified `AVDRMFrameDescriptor`, `AV_PIX_FMT_DRM_PRIME`, + `AV_HWDEVICE_TYPE_DRM` present. +- [x] **Negotiation** (validated on AMD/mutter): enumerate importable modifiers + via EGL surfaceless (`csrc/dmabuf_modifiers.c`) and advertise them in + `osc_build_enum_format_dmabuf`. Confirmed mutter now negotiates a **tiled + dmabuf** (`stream-started` fires) where before it failed with "no more input + formats". Enumeration returns 10 AMD GFX9 modifiers for XRGB8888. The existing + `mmap` path then correctly reports "driver does not allow CPU mapping" — the + exact branch point for the GPU import below. +- [x] Extend `osc_pw_frame` (pw_shim.h) + `RawFrame` (shim.rs) with + is_dmabuf/modifier/fourcc/n_planes/plane_fd/offset/stride. Layouts mirror + exactly; builds green. +- [x] C: `osc_read_frame` populates the descriptor for a tiled dmabuf; + `osc_on_add_buffer` latches `import_dmabuf` on mmap failure instead of erroring; + fourcc mapping added. shm/linear-dmabuf paths unchanged. `on_frame` currently + skips dmabuf frames (data==null) — safe no-op until the importer lands. +- [ ] Mailbox + requeue-handle rework (no memcpy for dmabuf). +- [ ] Encoder: DRM_PRIME → `av_hwframe_map` → VAAPI VPP (crop) → NV12 pool + surface; `encode_staged` direct. +- [ ] Shm fallback guard + error paths. +- [ ] Test on AMD/GNOME: confirm `uses_dmabuf=1`, distinct-fps ≈ OBS (~24), + crop correct for window captures, cursor unaffected. Regression-check + software encode and a wlroots/niri compositor. + +## Risk notes + +- radeonsi VAAPI must import the specific tiled modifier mutter exports — highly + likely OK (GNOME/OBS do DRM→VAAPI on this GPU), but the concrete failure mode + is `av_hwframe_map` returning an error → must fall back cleanly. +- Concurrency: import/VPP needs the VAAPI context; keep it on the main loop + (as sws_scale is today), holding the PW buffer only until the next tick. diff --git a/electron/native/pipewire-capture/src/shim.rs b/electron/native/pipewire-capture/src/shim.rs index 9d3a03ec7..5d1975d54 100644 --- a/electron/native/pipewire-capture/src/shim.rs +++ b/electron/native/pipewire-capture/src/shim.rs @@ -46,6 +46,17 @@ pub struct RawFrame { pub crop_width: i32, pub crop_height: i32, pub has_crop: i32, + /// Zero-copy dmabuf hand-off (issue #507). When non-zero, `data` is null and + /// the frame is a tiled GPU buffer described by the fields below — imported + /// as a VAAPI surface rather than read from `data`. Layout mirrors + /// `struct osc_pw_frame` in pw_shim.h exactly. + pub is_dmabuf: i32, + pub modifier: u64, + pub drm_fourcc: u32, + pub n_planes: i32, + pub plane_fd: [i32; 4], + pub plane_offset: [i32; 4], + pub plane_stride: [i32; 4], } #[repr(C)] From 138b4900ba7ee767720c6624fc2d46c7d25c891a Mon Sep 17 00:00:00 2001 From: Benjamin Freeman Date: Wed, 26 Aug 2026 20:55:39 +0200 Subject: [PATCH 2/9] feat(capture): add the dmabuf -> VAAPI importer module (#507) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The zero-copy import that replaces the throttled shm path: wrap a tiled compositor dmabuf as a DRM_PRIME frame, av_hwframe_map it into a VAAPI surface, and run scale_vaapi (VPP) to produce the NV12 the H.264 VAAPI encoder wants — no CPU readback. - New src/dmabuf_import.rs (DmabufImporter): owns a standalone VAAPI device, a derived DRM device, the DRM_PRIME + VAAPI-BGRx frames contexts, and the buffer -> scale_vaapi=nv12 -> buffersink graph. Exposes output_frames_ctx() so the encoder can be opened against the same NV12 pool it emits. - Vendored libavfilter wired into the build: bindgen headers + allowlist, link, and staged into helper-ffmpeg/ by the build script. Compiles and links against the vendored ffmpeg 8.1. Not yet wired into the frame flow (shim mailbox / encoder stage / capture branch) — that lands next, then the on-device record/check/fix loop. See docs/dmabuf-vaapi-plan.md. Co-Authored-By: Claude Opus 4.8 (1M context) --- electron/native/pipewire-capture/build.rs | 10 +- .../docs/dmabuf-vaapi-plan.md | 39 +- .../pipewire-capture/src/dmabuf_import.rs | 363 ++++++++++++++++++ electron/native/pipewire-capture/src/main.rs | 1 + scripts/build-linux-pipewire-helper.mjs | 2 +- 5 files changed, 409 insertions(+), 6 deletions(-) create mode 100644 electron/native/pipewire-capture/src/dmabuf_import.rs diff --git a/electron/native/pipewire-capture/build.rs b/electron/native/pipewire-capture/build.rs index 7c863210f..d6aec8e88 100644 --- a/electron/native/pipewire-capture/build.rs +++ b/electron/native/pipewire-capture/build.rs @@ -127,7 +127,9 @@ fn link_ffmpeg(root: &Path) { ); println!("cargo:rustc-link-search=native={}", lib.display()); - for name in ["avcodec", "avformat", "avutil", "swscale", "swresample"] { + // avfilter is for the VAAPI VPP (scale_vaapi) that converts an imported + // dmabuf surface to NV12 for the encoder — see the dmabuf import path. + for name in ["avcodec", "avformat", "avutil", "avfilter", "swscale", "swresample"] { println!("cargo:rustc-link-lib={name}"); } // A SUBDIRECTORY, NOT `$ORIGIN`. The helper is staged into @@ -170,6 +172,9 @@ fn link_ffmpeg(root: &Path) { #include #include #include + #include + #include + #include #include #include "#, @@ -182,6 +187,9 @@ fn link_ffmpeg(root: &Path) { .allowlist_function("avcodec_.*") .allowlist_function("avformat_.*") .allowlist_function("avio_.*") + .allowlist_function("avfilter_.*") + .allowlist_function("av_buffersrc_.*") + .allowlist_function("av_buffersink_.*") .allowlist_function("sws_.*") .allowlist_function("swr_.*") .allowlist_type("AV.*") diff --git a/electron/native/pipewire-capture/docs/dmabuf-vaapi-plan.md b/electron/native/pipewire-capture/docs/dmabuf-vaapi-plan.md index 15ac518e8..050326110 100644 --- a/electron/native/pipewire-capture/docs/dmabuf-vaapi-plan.md +++ b/electron/native/pipewire-capture/docs/dmabuf-vaapi-plan.md @@ -91,10 +91,41 @@ Key architecture calls: `osc_on_add_buffer` latches `import_dmabuf` on mmap failure instead of erroring; fourcc mapping added. shm/linear-dmabuf paths unchanged. `on_frame` currently skips dmabuf frames (data==null) — safe no-op until the importer lands. -- [ ] Mailbox + requeue-handle rework (no memcpy for dmabuf). -- [ ] Encoder: DRM_PRIME → `av_hwframe_map` → VAAPI VPP (crop) → NV12 pool - surface; `encode_staged` direct. -- [ ] Shm fallback guard + error paths. +- [x] Build foundation for the importer: vendored **libavfilter** wired in + (bindgen headers `avfilter.h`/`buffersrc.h`/`buffersink.h` + allowlist, link, + and staged into `helper-ffmpeg/` by the build script). `av_hwframe_map`, + `av_hwdevice_ctx_create_derived`, `AVDRMFrameDescriptor`, `AV_PIX_FMT_DRM_PRIME`, + `avfilter_graph_*`, `av_buffersrc/sink_*` all generate and link. Builds green. + +### Importer design decisions (settled while scoping) + +- **v1 buffer lifetime**: `on_frame` (PW thread) `dup()`s the plane fds into + `OwnedFd`s (std, no libc), puts the descriptor in the mailbox, and requeues the + PW buffer normally. The map+VPP+encode runs on the **main loop** — all VAAPI on + one thread, no cross-thread device or mutex. The fds keep the dmabuf alive for + the import; content-tear risk (compositor reusing the requeued buffer before the + main loop imports, ~1 tick later) is low with a 4–16 buffer pool and is the one + thing to watch. Upgrade to buffer-holding only if tearing shows. +- **Device & pool ownership**: create ONE standalone VAAPI `AVHWDeviceContext`; + derive a DRM device from it for the DRM_PRIME source frames ctx. Build the + `scale_vaapi` filtergraph (buffersrc VAAPI-BGR0 → `format=nv12` → buffersink) + and take the buffersink's **output NV12 hw_frames_ctx** as the encoder's + `codec_ctx->hw_frames_ctx`. That means for the dmabuf path the **encoder is + opened AFTER the importer/filtergraph is built**, so their pools match and + `avcodec_send_frame` accepts the surface directly. +- **Per frame**: build `AVDRMFrameDescriptor` (1 object: fd/size/modifier; 1 + layer: fourcc; 1 plane: offset/pitch) → DRM_PRIME AVFrame → `av_hwframe_map` + DIRECT|READ → VAAPI BGR0 → buffersrc→scale_vaapi→buffersink → NV12 VAAPI → + `encoder.stage_hw()` (held as `hw_staged`; `encode_staged` sends it with pts, + no unref between the clock-driven re-encodes). + +### Remaining +- [ ] `dmabuf_import.rs`: the map + scale_vaapi VPP → NV12 module (above). +- [ ] `shim.rs`: `DmabufDesc` (OwnedFd planes) + `Frame.dmabuf` + `on_frame` dup + + mailbox variant (no memcpy). +- [ ] `encoder.rs`: shared device + deferred open + `stage_hw`/`hw_staged` path. +- [ ] `capture.rs`: dmabuf branch → importer → `stage_hw`. +- [ ] Shm fallback guard + error paths; then on-device record/check/fix. - [ ] Test on AMD/GNOME: confirm `uses_dmabuf=1`, distinct-fps ≈ OBS (~24), crop correct for window captures, cursor unaffected. Regression-check software encode and a wlroots/niri compositor. diff --git a/electron/native/pipewire-capture/src/dmabuf_import.rs b/electron/native/pipewire-capture/src/dmabuf_import.rs new file mode 100644 index 000000000..7697c84d1 --- /dev/null +++ b/electron/native/pipewire-capture/src/dmabuf_import.rs @@ -0,0 +1,363 @@ +//! Zero-copy import of a compositor dmabuf into a VAAPI NV12 surface (issue #507). +//! +//! On GNOME/Wayland + AMD a whole-monitor capture arrives as a *tiled* dmabuf +//! that cannot be CPU-mapped. Rather than fall back to the shm path (which mutter +//! throttles hard), we keep the frame on the GPU: wrap the dmabuf as a +//! `DRM_PRIME` frame, [`av_hwframe_map`] it into a VAAPI surface, and run it +//! through a `scale_vaapi` VPP that converts the BGRx layout to the NV12 the +//! H.264 VAAPI encoder wants. Nothing is read back to system memory. +//! +//! The importer owns the VAAPI device and the filtergraph. The encoder is opened +//! against [`Self::output_frames_ctx`] so the NV12 surface this produces is one +//! `avcodec_send_frame` accepts directly. See docs/dmabuf-vaapi-plan.md. + +use crate::ffmpeg as ff; +use std::ffi::CString; +use std::ptr; + +/// `av_frame_free` wants a `**AVFrame`; wrap the pointer in a local so the null +/// it writes back does not land in a temporary. +unsafe fn free_frame(frame: *mut ff::AVFrame) { + let mut p = frame; + ff::av_frame_free(&mut p); +} + +/// One dmabuf plane. `fd` is borrowed for the duration of [`DmabufImporter::import`] +/// only — VAAPI dups it during surface creation, so the caller may close it after. +pub struct DmabufPlane { + pub fd: i32, + pub offset: i32, + pub stride: i32, +} + +/// A dmabuf frame to import. Borrows the fds; see [`DmabufPlane`]. +pub struct DmabufFrame<'a> { + pub width: i32, + pub height: i32, + pub drm_fourcc: u32, + pub modifier: u64, + pub planes: &'a [DmabufPlane], +} + +/// The pixel format the VAAPI-mapped surface presents, derived from the dmabuf's +/// DRM fourcc. Only the 32-bit RGB layouts we negotiate are handled. +fn sw_format_for_fourcc(drm_fourcc: u32) -> Option { + // DRM fourccs (little-endian) → the matching packed ffmpeg format. + const XRGB8888: u32 = 0x34325258; // SPA BGRx + const ARGB8888: u32 = 0x34325241; // SPA BGRA + const XBGR8888: u32 = 0x34324258; // SPA RGBx + const ABGR8888: u32 = 0x34324241; // SPA RGBA + match drm_fourcc { + XRGB8888 => Some(ff::AV_PIX_FMT_BGR0), + ARGB8888 => Some(ff::AV_PIX_FMT_BGRA), + XBGR8888 => Some(ff::AV_PIX_FMT_0BGR), + ABGR8888 => Some(ff::AV_PIX_FMT_ABGR), + _ => None, + } +} + +pub struct DmabufImporter { + width: i32, + height: i32, + sw_format: ff::AVPixelFormat, + /// VAAPI device, shared with the encoder (whose `hw_frames_ctx` comes from + /// [`Self::output_frames_ctx`]). + va_device: *mut ff::AVBufferRef, + /// DRM device derived from `va_device`; backs the DRM_PRIME source frames. + drm_device: *mut ff::AVBufferRef, + /// Frames context for the incoming DRM_PRIME buffers. + drm_frames: *mut ff::AVBufferRef, + /// Frames context for the VAAPI surface the dmabuf maps into (still BGRx). + va_map_frames: *mut ff::AVBufferRef, + graph: *mut ff::AVFilterGraph, + buffersrc_ctx: *mut ff::AVFilterContext, + buffersink_ctx: *mut ff::AVFilterContext, +} + +impl DmabufImporter { + /// Builds the device, frames contexts and `scale_vaapi` graph for a stream of + /// `width`×`height` `drm_fourcc` buffers. + pub fn new(width: i32, height: i32, drm_fourcc: u32) -> Result { + let sw_format = + sw_format_for_fourcc(drm_fourcc).ok_or_else(|| format!("unsupported dmabuf fourcc {drm_fourcc:#x}"))?; + + // SAFETY: every pointer is checked before use and freed in Drop. + unsafe { + let mut me = DmabufImporter { + width, + height, + sw_format, + va_device: ptr::null_mut(), + drm_device: ptr::null_mut(), + drm_frames: ptr::null_mut(), + va_map_frames: ptr::null_mut(), + graph: ptr::null_mut(), + buffersrc_ctx: ptr::null_mut(), + buffersink_ctx: ptr::null_mut(), + }; + + // A standalone VAAPI device (the encoder will share it). Default + // device selection matches how the encoder opens VAAPI today. + let created = ff::av_hwdevice_ctx_create( + &mut me.va_device, + ff::AV_HWDEVICE_TYPE_VAAPI, + ptr::null(), + ptr::null_mut(), + 0, + ); + if created < 0 { + return Err(format!("av_hwdevice_ctx_create(VAAPI): {}", ff::err_to_string(created))); + } + + // DRM device derived from the same GPU, so the mapped surface and the + // VAAPI device share a backing. + let derived = ff::av_hwdevice_ctx_create_derived( + &mut me.drm_device, + ff::AV_HWDEVICE_TYPE_DRM, + me.va_device, + 0, + ); + if derived < 0 { + return Err(format!( + "av_hwdevice_ctx_create_derived(DRM): {}", + ff::err_to_string(derived) + )); + } + + me.drm_frames = me.alloc_frames(me.drm_device, ff::AV_PIX_FMT_DRM_PRIME)?; + me.va_map_frames = me.alloc_frames(me.va_device, ff::AV_PIX_FMT_VAAPI)?; + me.build_graph()?; + Ok(me) + } + } + + /// Allocates and initialises a frames context of `hw_format` (VAAPI or + /// DRM_PRIME) whose software format is the stream's RGB layout. + unsafe fn alloc_frames( + &self, + device: *mut ff::AVBufferRef, + hw_format: ff::AVPixelFormat, + ) -> Result<*mut ff::AVBufferRef, String> { + let frames = ff::av_hwframe_ctx_alloc(device); + if frames.is_null() { + return Err("av_hwframe_ctx_alloc failed".to_owned()); + } + let ctx = (*frames).data as *mut ff::AVHWFramesContext; + (*ctx).format = hw_format; + (*ctx).sw_format = self.sw_format; + (*ctx).width = self.width; + (*ctx).height = self.height; + // A small pool: the mapped surface is short-lived (consumed by the VPP in + // the same call). DRM_PRIME frames are imported, not pooled, so the count + // is nominal there. + (*ctx).initial_pool_size = 4; + let init = ff::av_hwframe_ctx_init(frames); + if init < 0 { + let mut f = frames; + ff::av_buffer_unref(&mut f); + return Err(format!("av_hwframe_ctx_init: {}", ff::err_to_string(init))); + } + Ok(frames) + } + + /// Builds `buffer (VAAPI/BGRx) -> scale_vaapi=format=nv12 -> buffersink`. + unsafe fn build_graph(&mut self) -> Result<(), String> { + self.graph = ff::avfilter_graph_alloc(); + if self.graph.is_null() { + return Err("avfilter_graph_alloc failed".to_owned()); + } + + let buffersrc = ff::avfilter_get_by_name(c"buffer".as_ptr()); + let buffersink = ff::avfilter_get_by_name(c"buffersink".as_ptr()); + let scale = ff::avfilter_get_by_name(c"scale_vaapi".as_ptr()); + if buffersrc.is_null() || buffersink.is_null() || scale.is_null() { + return Err("a required filter (buffer/buffersink/scale_vaapi) is missing".to_owned()); + } + + // buffersrc: the input is a VAAPI surface, so pix_fmt is VAAPI and the + // real (sw) format rides on the hw_frames_ctx set below. + let args = CString::new(format!( + "video_size={}x{}:pix_fmt={}:time_base=1/1000000:pixel_aspect=1/1", + self.width, + self.height, + ff::AV_PIX_FMT_VAAPI as i32 + )) + .map_err(|_| "buffersrc args contained a NUL".to_owned())?; + + let rc = ff::avfilter_graph_create_filter( + &mut self.buffersrc_ctx, + buffersrc, + c"in".as_ptr(), + args.as_ptr(), + ptr::null_mut(), + self.graph, + ); + if rc < 0 { + return Err(format!("create buffersrc: {}", ff::err_to_string(rc))); + } + + // Attach the VAAPI (BGRx) frames context the mapped surfaces come from. + let par = ff::av_buffersrc_parameters_alloc(); + if par.is_null() { + return Err("av_buffersrc_parameters_alloc failed".to_owned()); + } + (*par).hw_frames_ctx = ff::av_buffer_ref(self.va_map_frames); + let set = ff::av_buffersrc_parameters_set(self.buffersrc_ctx, par); + ff::av_free(par as *mut _); + if set < 0 { + return Err(format!("av_buffersrc_parameters_set: {}", ff::err_to_string(set))); + } + + let rc = ff::avfilter_graph_create_filter( + &mut self.buffersink_ctx, + buffersink, + c"out".as_ptr(), + ptr::null(), + ptr::null_mut(), + self.graph, + ); + if rc < 0 { + return Err(format!("create buffersink: {}", ff::err_to_string(rc))); + } + + let mut scale_ctx: *mut ff::AVFilterContext = ptr::null_mut(); + let rc = ff::avfilter_graph_create_filter( + &mut scale_ctx, + scale, + c"vpp".as_ptr(), + c"format=nv12".as_ptr(), + ptr::null_mut(), + self.graph, + ); + if rc < 0 { + return Err(format!("create scale_vaapi: {}", ff::err_to_string(rc))); + } + // scale_vaapi needs a device to allocate its NV12 output pool; take it + // from the shared VAAPI device rather than relying on propagation. + (*scale_ctx).hw_device_ctx = ff::av_buffer_ref(self.va_device); + + let rc = ff::avfilter_link(self.buffersrc_ctx, 0, scale_ctx, 0); + if rc >= 0 { + ff::avfilter_link(scale_ctx, 0, self.buffersink_ctx, 0); + } + if rc < 0 { + return Err(format!("avfilter_link: {}", ff::err_to_string(rc))); + } + + let rc = ff::avfilter_graph_config(self.graph, ptr::null_mut()); + if rc < 0 { + return Err(format!("avfilter_graph_config: {}", ff::err_to_string(rc))); + } + Ok(()) + } + + /// The NV12 VAAPI frames context the graph emits into — the encoder opens + /// against this so it accepts the surfaces [`Self::import`] returns. + pub fn output_frames_ctx(&self) -> *mut ff::AVBufferRef { + // SAFETY: valid after a successful `build_graph`; the sink has one input. + unsafe { ff::av_buffersink_get_hw_frames_ctx(self.buffersink_ctx) } + } + + /// The shared VAAPI device, for the encoder's `hwaccel` context. + pub fn device(&self) -> *mut ff::AVBufferRef { + self.va_device + } + + /// Maps one dmabuf and returns an NV12 VAAPI frame (caller unrefs it). The + /// plane fds are only touched during this call. + pub fn import(&mut self, frame: &DmabufFrame) -> Result<*mut ff::AVFrame, String> { + if frame.planes.is_empty() || frame.planes.len() > 4 { + return Err(format!("dmabuf has {} planes", frame.planes.len())); + } + // SAFETY: the descriptor outlives the map call it is passed to; every + // allocated frame is unref'd on the error paths and on success ownership + // of the NV12 frame passes to the caller. + unsafe { + // Build the DRM PRIME descriptor. One object per unique fd; our RGB + // formats are a single object with a single layer and plane. + let mut desc: ff::AVDRMFrameDescriptor = std::mem::zeroed(); + desc.nb_objects = 1; + desc.objects[0].fd = frame.planes[0].fd; + desc.objects[0].size = 0; // recovered by the driver from the fd + desc.objects[0].format_modifier = frame.modifier; + desc.nb_layers = 1; + desc.layers[0].format = frame.drm_fourcc; + desc.layers[0].nb_planes = frame.planes.len() as i32; + for (i, plane) in frame.planes.iter().enumerate() { + desc.layers[0].planes[i].object_index = 0; + desc.layers[0].planes[i].offset = plane.offset as isize; + desc.layers[0].planes[i].pitch = plane.stride as isize; + } + + let src = ff::av_frame_alloc(); + if src.is_null() { + return Err("av_frame_alloc(src) failed".to_owned()); + } + (*src).format = ff::AV_PIX_FMT_DRM_PRIME as i32; + (*src).width = self.width; + (*src).height = self.height; + (*src).data[0] = &mut desc as *mut _ as *mut u8; + (*src).hw_frames_ctx = ff::av_buffer_ref(self.drm_frames); + + // Map the dmabuf into a VAAPI (BGRx) surface, zero-copy. + let mapped = ff::av_frame_alloc(); + if mapped.is_null() { + free_frame(src); + return Err("av_frame_alloc(mapped) failed".to_owned()); + } + (*mapped).format = ff::AV_PIX_FMT_VAAPI as i32; + (*mapped).hw_frames_ctx = ff::av_buffer_ref(self.va_map_frames); + let mrc = ff::av_hwframe_map( + mapped, + src, + (ff::AV_HWFRAME_MAP_DIRECT | ff::AV_HWFRAME_MAP_READ) as i32, + ); + // `src` (and thus `desc`) is no longer needed once mapped. + free_frame(src); + if mrc < 0 { + free_frame(mapped); + return Err(format!("av_hwframe_map: {}", ff::err_to_string(mrc))); + } + + // Push through scale_vaapi → NV12. + let pushed = ff::av_buffersrc_add_frame(self.buffersrc_ctx, mapped); + free_frame(mapped); + if pushed < 0 { + return Err(format!("av_buffersrc_add_frame: {}", ff::err_to_string(pushed))); + } + + let nv12 = ff::av_frame_alloc(); + if nv12.is_null() { + return Err("av_frame_alloc(nv12) failed".to_owned()); + } + let got = ff::av_buffersink_get_frame(self.buffersink_ctx, nv12); + if got < 0 { + free_frame(nv12); + return Err(format!("av_buffersink_get_frame: {}", ff::err_to_string(got))); + } + Ok(nv12) + } + } +} + +impl Drop for DmabufImporter { + fn drop(&mut self) { + // SAFETY: each pointer is freed once; nulls are ignored by the ffmpeg + // frees, and the order is graph → frames → devices. + unsafe { + if !self.graph.is_null() { + ff::avfilter_graph_free(&mut self.graph); + } + for frames in [&mut self.drm_frames, &mut self.va_map_frames] { + if !frames.is_null() { + ff::av_buffer_unref(frames); + } + } + for device in [&mut self.drm_device, &mut self.va_device] { + if !device.is_null() { + ff::av_buffer_unref(device); + } + } + } + } +} diff --git a/electron/native/pipewire-capture/src/main.rs b/electron/native/pipewire-capture/src/main.rs index 8ec4078c2..344d84b4b 100644 --- a/electron/native/pipewire-capture/src/main.rs +++ b/electron/native/pipewire-capture/src/main.rs @@ -26,6 +26,7 @@ mod bitmap; mod capture; +mod dmabuf_import; mod encoder; mod events; mod ffmpeg; diff --git a/scripts/build-linux-pipewire-helper.mjs b/scripts/build-linux-pipewire-helper.mjs index ac898c4c2..b6928c3a8 100644 --- a/scripts/build-linux-pipewire-helper.mjs +++ b/scripts/build-linux-pipewire-helper.mjs @@ -143,7 +143,7 @@ function stageFfmpeg(dir) { // Only the sonames the helper actually links, and only the real files — // the tree also holds unversioned `.so` symlinks that the loader never // consults at runtime. - const wanted = /^lib(avcodec|avformat|avutil|swscale|swresample)\.so\.\d+$/; + const wanted = /^lib(avcodec|avformat|avutil|avfilter|swscale|swresample)\.so\.\d+$/; let copied = 0; for (const entry of fs.readdirSync(source)) { if (!wanted.test(entry)) { From 61007efd9d0ecf8e6b3152e09dc2e695e8bf88f9 Mon Sep 17 00:00:00 2001 From: Benjamin Freeman Date: Wed, 26 Aug 2026 21:10:26 +0200 Subject: [PATCH 3/9] feat(capture): wire the dmabuf importer through to the encoder (#507) Completes the zero-copy path end to end (compiles; on-device testing next): - shim.rs: a tiled dmabuf frame carries a DmabufDesc with OwnedFd planes; on_frame dup()s the plane fds (keeps content reachable past the buffer re-queue) and the mailbox stores the descriptor with no pixel copy. - encoder.rs: open_importing() opens VAAPI against the importer's shared device and NV12 pool; stage_hw()/hw_staged sends the imported surface directly (no swscale, no upload) and holds it across the clock-driven re-encodes. - capture.rs: on a dmabuf frame, build the importer once (sized to the full monitor), import the descriptor to an NV12 surface, and stage it. The encoder is opened against the importer's pool so the surface is accepted directly. - main.rs: pass the first frame's dmabuf descriptor to Capture::start. Still gated behind OPENSCREEN_PIPEWIRE_FORCE_DMABUF while it is validated on hardware; the shm/software paths are unchanged. See docs/dmabuf-vaapi-plan.md. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../docs/dmabuf-vaapi-plan.md | 20 +++- .../native/pipewire-capture/src/capture.rs | 112 +++++++++++++++--- .../native/pipewire-capture/src/encoder.rs | 93 +++++++++++++-- electron/native/pipewire-capture/src/main.rs | 1 + electron/native/pipewire-capture/src/shim.rs | 104 ++++++++++++++++ 5 files changed, 294 insertions(+), 36 deletions(-) diff --git a/electron/native/pipewire-capture/docs/dmabuf-vaapi-plan.md b/electron/native/pipewire-capture/docs/dmabuf-vaapi-plan.md index 050326110..db9e093b3 100644 --- a/electron/native/pipewire-capture/docs/dmabuf-vaapi-plan.md +++ b/electron/native/pipewire-capture/docs/dmabuf-vaapi-plan.md @@ -120,12 +120,20 @@ Key architecture calls: no unref between the clock-driven re-encodes). ### Remaining -- [ ] `dmabuf_import.rs`: the map + scale_vaapi VPP → NV12 module (above). -- [ ] `shim.rs`: `DmabufDesc` (OwnedFd planes) + `Frame.dmabuf` + `on_frame` dup + - mailbox variant (no memcpy). -- [ ] `encoder.rs`: shared device + deferred open + `stage_hw`/`hw_staged` path. -- [ ] `capture.rs`: dmabuf branch → importer → `stage_hw`. -- [ ] Shm fallback guard + error paths; then on-device record/check/fix. +- [x] `dmabuf_import.rs`: the map + scale_vaapi VPP → NV12 module. +- [x] `shim.rs`: `DmabufDesc` (OwnedFd planes) + `Frame.dmabuf` + `on_frame` dup + + mailbox `put_dmabuf` (no memcpy). +- [x] `encoder.rs`: `open_importing` (shared device + external NV12 pool) + + `stage_hw`/`hw_staged` path (held across re-encodes) + Drop. +- [x] `capture.rs`: dmabuf branch → importer → `stage_hw`; deferred encoder open. +- [x] Whole pipeline compiles and links; full helper builds, libavfilter staged. +- [ ] **On-device record/check/fix.** Test via `OPENSCREEN_PIPEWIRE_FORCE_DMABUF=1` + (dmabuf still opt-in until proven). Expect a NON-garbled full-monitor recording + at distinct-fps ≈ OBS (~24). Likely first-try failure points: `av_hwframe_map` + wanting `buf[0]` on the DRM_PRIME source; scale_vaapi output pool size; the + DRM/VAAPI derive on radeonsi. +- [ ] Once proven: make dmabuf preference automatic when the backend is VAAPI + (drop the FORCE env gate) + window/crop via VPP + non-VAAPI fallback guard. - [ ] Test on AMD/GNOME: confirm `uses_dmabuf=1`, distinct-fps ≈ OBS (~24), crop correct for window captures, cursor unaffected. Regression-check software encode and a wlroots/niri compositor. diff --git a/electron/native/pipewire-capture/src/capture.rs b/electron/native/pipewire-capture/src/capture.rs index 17c42f801..628eba3a4 100644 --- a/electron/native/pipewire-capture/src/capture.rs +++ b/electron/native/pipewire-capture/src/capture.rs @@ -192,6 +192,11 @@ pub struct Summary { pub struct Capture { encoder: VideoEncoder, + /// The dmabuf → VAAPI importer, present only on the zero-copy path (issue + /// #507). When set, [`Self::stage`] imports the frame's descriptor into an + /// NV12 surface instead of running swscale, and the encoder was opened + /// against this importer's pool. + importer: Option, video_track: TrackId, audio: Option, /// `None` only between [`Self::finish`] taking it and the struct dropping. @@ -233,14 +238,45 @@ impl Capture { bitrate: Option, forced: Option, audio_sources: Vec, + // Present when the first frame is a tiled dmabuf: the encoder is then + // opened to consume the importer's NV12 pool directly (issue #507). + dmabuf: Option<&shim::DmabufDesc>, ) -> Result<(Self, Selection), String> { let bitrate = bitrate.unwrap_or_else(|| default_bitrate(width, height, fps)); let mut rejected = Vec::new(); - let encoder = VideoEncoder::open( - VideoParams { width, height, fps, bitrate }, - forced, - |backend, error| rejected.push(format!("{}: {error}", backend.as_str())), - )?; + let (encoder, importer) = match dmabuf { + Some(desc) => { + // v1 targets the whole monitor: the importer and encoder are + // sized to the full stream (no crop). The encoder is FORCED to + // VAAPI because that is the only backend that can consume the + // mapped surface; a non-VAAPI machine should never have + // negotiated dmabuf in the first place. + let importer = crate::dmabuf_import::DmabufImporter::new( + desc.width, + desc.height, + desc.drm_fourcc, + )?; + // SAFETY: the importer's device and NV12 frames context are live + // for as long as the returned encoder, which the Capture owns + // alongside it below. + let encoder = unsafe { + VideoEncoder::open_importing( + VideoParams { width: desc.width, height: desc.height, fps, bitrate }, + importer.device(), + importer.output_frames_ctx(), + )? + }; + (encoder, Some(importer)) + } + None => { + let encoder = VideoEncoder::open( + VideoParams { width, height, fps, bitrate }, + forced, + |backend, error| rejected.push(format!("{}: {error}", backend.as_str())), + )?; + (encoder, None) + } + }; let selection = Selection { backend: encoder.backend(), rejected }; // Every track must exist before the header: MP4 fixes its track list @@ -274,6 +310,7 @@ impl Capture { Ok(( Self { encoder, + importer, video_track, audio, muxer: Some(muxer), @@ -323,6 +360,38 @@ impl Capture { /// Converts a captured frame into the encoder's staging buffer. Nothing is /// written until [`Self::advance`] runs. pub fn stage(&mut self, frame: &shim::Frame) -> Result<(), String> { + // Zero-copy dmabuf path: import the tiled GPU buffer into an NV12 VAAPI + // surface and hand it to the encoder as-is — no swscale, no crop math + // (v1 is whole-monitor only). See issue #507. + if let Some(desc) = &frame.dmabuf { + use std::os::fd::AsRawFd; + let importer = self + .importer + .as_mut() + .ok_or_else(|| "dmabuf frame arrived but no importer was built".to_owned())?; + let planes: Vec = desc + .planes + .iter() + .map(|plane| crate::dmabuf_import::DmabufPlane { + fd: plane.fd.as_raw_fd(), + offset: plane.offset, + stride: plane.stride, + }) + .collect(); + let nv12 = importer.import(&crate::dmabuf_import::DmabufFrame { + width: desc.width, + height: desc.height, + drm_fourcc: desc.drm_fourcc, + modifier: desc.modifier, + planes: &planes, + })?; + // SAFETY: `nv12` is a VAAPI NV12 frame from the pool the encoder was + // opened against; the encoder takes ownership. + unsafe { self.encoder.stage_hw(nv12) }; + self.mark_started(); + return Ok(()); + } + let format = pixel_format(frame.video_format)?; // Address the crop by moving the START of the slice, and hand swscale the @@ -341,21 +410,28 @@ impl Capture { .ok_or_else(|| format!("crop offset {offset} is past the end of the frame"))?; self.encoder.stage(pixels, frame.stride, format)?; - if self.epoch.is_none() { - self.epoch = Some(Instant::now()); - // Audio has been accumulating since the process started, while the - // portal picker was up and the format was being negotiated. None of - // it belongs to the recording: video frame 0 is now, so audio - // sample 0 is now too. Keeping the backlog would shift the whole - // track earlier by however long the user took to click. - if let Some(mix) = &mut self.audio { - for input in &mut mix.inputs { - input.ring.clear(); - input.pending.clear(); - } + self.mark_started(); + Ok(()) + } + + /// Starts the timeline on the first staged frame and drops the audio backlog. + /// + /// Audio has been accumulating since the process started, while the portal + /// picker was up and the format was being negotiated. None of it belongs to + /// the recording: video frame 0 is now, so audio sample 0 is now too. Keeping + /// the backlog would shift the whole track earlier by however long the user + /// took to click. + fn mark_started(&mut self) { + if self.epoch.is_some() { + return; + } + self.epoch = Some(Instant::now()); + if let Some(mix) = &mut self.audio { + for input in &mut mix.inputs { + input.ring.clear(); + input.pending.clear(); } } - Ok(()) } /// Whether a picture has been staged, which is also whether the timeline has diff --git a/electron/native/pipewire-capture/src/encoder.rs b/electron/native/pipewire-capture/src/encoder.rs index e574ae405..0d66f5c90 100644 --- a/electron/native/pipewire-capture/src/encoder.rs +++ b/electron/native/pipewire-capture/src/encoder.rs @@ -193,6 +193,11 @@ pub struct VideoEncoder { sw_frame: *mut ff::AVFrame, /// The GPU-side frame handed to a hardware encoder. Null for software. hw_frame: *mut ff::AVFrame, + /// A ready-to-encode VAAPI NV12 surface produced by the dmabuf importer + /// (issue #507). When non-null it is sent directly — no sws_scale, no upload — + /// and held across the clock-driven re-encodes until the next frame replaces + /// it. Null on the shm/software path. + hw_staged: *mut ff::AVFrame, sws: *mut ff::SwsContext, sws_src_format: ff::AVPixelFormat, packet: *mut ff::AVPacket, @@ -235,7 +240,7 @@ impl VideoEncoder { failures.push(format!("{}: {reason}", backend.as_str())); continue; } - match Self::open_backend(backend, ¶ms) { + match Self::open_backend(backend, ¶ms, None) { Ok(encoder) => return Ok(encoder), Err(error) => { on_attempt(backend, &error); @@ -252,7 +257,26 @@ impl VideoEncoder { )) } - fn open_backend(backend: Backend, params: &VideoParams) -> Result { + /// Opens the VAAPI encoder to consume surfaces from an EXISTING device and + /// NV12 frames pool — the ones the dmabuf importer built. Sharing the pool is + /// what lets `encode_staged` send an imported surface straight to + /// `avcodec_send_frame` without a copy (issue #507). + /// + /// SAFETY: `device` and `frames_ctx` must be a live VAAPI device and an NV12 + /// VAAPI frames context on it; the encoder takes its own references. + pub unsafe fn open_importing( + params: VideoParams, + device: *mut ff::AVBufferRef, + frames_ctx: *mut ff::AVBufferRef, + ) -> Result { + Self::open_backend(Backend::Vaapi, ¶ms, Some((device, frames_ctx))) + } + + fn open_backend( + backend: Backend, + params: &VideoParams, + external: Option<(*mut ff::AVBufferRef, *mut ff::AVBufferRef)>, + ) -> Result { // SAFETY: this whole function is a single ffmpeg setup sequence. Every // allocation is stored in `encoder` as soon as it succeeds, so the Drop // impl frees whatever was reached if a later step fails. @@ -277,6 +301,7 @@ impl VideoEncoder { hw_frames: ptr::null_mut(), sw_frame: ptr::null_mut(), hw_frame: ptr::null_mut(), + hw_staged: ptr::null_mut(), sws: ptr::null_mut(), sws_src_format: ff::AV_PIX_FMT_NONE, packet: ptr::null_mut(), @@ -320,7 +345,7 @@ impl VideoEncoder { (*codec_ctx).flags |= ff::AV_CODEC_FLAG_GLOBAL_HEADER as i32; if let Some(device_type) = backend.hw_device_type() { - encoder.attach_hardware(device_type, params)?; + encoder.attach_hardware(device_type, params, external)?; } let opened = ff::avcodec_open2(codec_ctx, codec, ptr::null_mut()); @@ -350,7 +375,24 @@ impl VideoEncoder { &mut self, device_type: ff::AVHWDeviceType, params: &VideoParams, + external: Option<(*mut ff::AVBufferRef, *mut ff::AVBufferRef)>, ) -> Result<(), String> { + // The dmabuf importer already built a VAAPI device and an NV12 pool; the + // encoder must consume from THAT pool, so take references to it instead of + // creating a second, incompatible one. See `open_importing`. + if let Some((device, frames_ctx)) = external { + self.hw_device = ff::av_buffer_ref(device); + self.hw_frames = ff::av_buffer_ref(frames_ctx); + if self.hw_device.is_null() || self.hw_frames.is_null() { + return Err("av_buffer_ref on the shared VAAPI context returned null".to_owned()); + } + (*self.codec_ctx).hw_frames_ctx = ff::av_buffer_ref(self.hw_frames); + if (*self.codec_ctx).hw_frames_ctx.is_null() { + return Err("av_buffer_ref on the shared frames context returned null".to_owned()); + } + return Ok(()); + } + let created = ff::av_hwdevice_ctx_create( &mut self.hw_device, device_type, @@ -498,9 +540,24 @@ impl VideoEncoder { Ok(()) } - /// True once [`Self::stage`] has put a picture in the staging buffer. Before - /// that there is nothing to encode and [`Self::encode_staged`] would emit a - /// frame of uninitialised memory. + /// Stages a ready VAAPI NV12 surface produced by the dmabuf importer. Takes + /// ownership of `frame`; the previous one is released. Unlike [`Self::stage`] + /// there is no conversion or upload — the surface is encoded as-is and held + /// across the clock-driven re-encodes until the next frame replaces it. + /// + /// SAFETY: `frame` must be a valid VAAPI NV12 `AVFrame` from the shared pool + /// the encoder was opened against (see `open_importing`). + pub unsafe fn stage_hw(&mut self, frame: *mut ff::AVFrame) { + if !self.hw_staged.is_null() { + let mut old = self.hw_staged; + ff::av_frame_free(&mut old); + } + self.hw_staged = frame; + self.staged = true; + } + + /// True once a picture has been staged — via [`Self::stage`] (shm/software) + /// or [`Self::stage_hw`] (dmabuf). Before that there is nothing to encode. pub fn has_staged_frame(&self) -> bool { self.staged } @@ -521,7 +578,13 @@ impl VideoEncoder { // `sw_frame`, and every pointer below is owned by `self`. unsafe { let upload_started = std::time::Instant::now(); - let frame = if self.hw_frames.is_null() { + let mut used_upload = false; + let frame = if !self.hw_staged.is_null() { + // Imported dmabuf surface: already NV12 on the GPU. No upload, no + // conversion — just timestamp it. Held for the next re-encode. + (*self.hw_staged).pts = pts; + self.hw_staged + } else if self.hw_frames.is_null() { (*self.sw_frame).pts = pts; self.sw_frame } else { @@ -540,6 +603,7 @@ impl VideoEncoder { )); } (*self.hw_frame).pts = pts; + used_upload = true; self.hw_frame }; self.stats.upload_ns += upload_started.elapsed().as_nanos(); @@ -549,11 +613,13 @@ impl VideoEncoder { self.stats.encode_ns += encode_started.elapsed().as_nanos(); self.stats.frames += 1; - if !self.hw_frame.is_null() { - // Release our reference to the GPU surface; the encoder keeps - // its own for as long as it needs one. Without this the pool - // drains after `initial_pool_size` frames and every subsequent - // av_hwframe_get_buffer blocks. + if used_upload { + // Release our reference to the per-encode upload surface; the + // encoder keeps its own for as long as it needs one. Without this + // the pool drains after `initial_pool_size` frames and every + // subsequent av_hwframe_get_buffer blocks. The imported + // `hw_staged` surface is NOT released here — it is held for the + // next clock-driven re-encode and freed in `stage_hw`/`Drop`. ff::av_frame_unref(self.hw_frame); } } @@ -697,6 +763,9 @@ impl Drop for VideoEncoder { if !self.hw_frame.is_null() { ff::av_frame_free(&mut self.hw_frame); } + if !self.hw_staged.is_null() { + ff::av_frame_free(&mut self.hw_staged); + } if !self.sw_frame.is_null() { ff::av_frame_free(&mut self.sw_frame); } diff --git a/electron/native/pipewire-capture/src/main.rs b/electron/native/pipewire-capture/src/main.rs index 344d84b4b..d12b225f3 100644 --- a/electron/native/pipewire-capture/src/main.rs +++ b/electron/native/pipewire-capture/src/main.rs @@ -693,6 +693,7 @@ fn run( config.bitrate, config.forced_encoder, std::mem::take(&mut audio_sources), + frame.dmabuf.as_ref(), ) { Ok((started, selection)) => { let _ = emitter.emit(&Event::EncoderSelection { diff --git a/electron/native/pipewire-capture/src/shim.rs b/electron/native/pipewire-capture/src/shim.rs index 5d1975d54..6be0424e3 100644 --- a/electron/native/pipewire-capture/src/shim.rs +++ b/electron/native/pipewire-capture/src/shim.rs @@ -191,6 +191,30 @@ pub struct Frame { /// "invalid meta" and "meta covering everything" alike — none of which is a /// reason to crop, and none of which may be guessed apart. pub has_crop: bool, + /// Set for a tiled dmabuf frame (issue #507): `pixels` is empty and the + /// content is on the GPU, described here for a VAAPI import instead. The + /// owned fds close when the frame is dropped or superseded. + pub dmabuf: Option, +} + +/// A tiled dmabuf handed up for GPU import. Owns duplicated plane fds so the +/// descriptor outlives the PipeWire buffer it came from. +#[derive(Debug)] +pub struct DmabufDesc { + pub width: i32, + pub height: i32, + pub drm_fourcc: u32, + pub modifier: u64, + pub planes: Vec, +} + +/// One dmabuf plane: an owned (dup'd) fd plus its layout. The fd is closed when +/// this drops. +#[derive(Debug)] +pub struct DmabufPlaneOwned { + pub fd: std::os::fd::OwnedFd, + pub offset: i32, + pub stride: i32, } /// A rectangle inside a captured frame, in stream pixels. @@ -268,6 +292,43 @@ impl FrameMailbox { height: meta.crop_height, }, has_crop: meta.has_crop != 0, + dmabuf: None, + }); + self.received.fetch_add(1, Ordering::Relaxed); + } + + /// Stores a tiled dmabuf frame — the descriptor only, no pixel copy. Same + /// newest-wins discipline as [`Self::put`]; a superseded frame's owned fds + /// close when its `Frame` drops here. + fn put_dmabuf(&self, desc: DmabufDesc, meta: &RawFrame) { + use std::sync::atomic::Ordering; + + let Ok(mut inner) = self.inner.lock() else { + self.dropped.fetch_add(1, Ordering::Relaxed); + return; + }; + let pixels = match inner.pending.take() { + Some(stale) => { + self.dropped.fetch_add(1, Ordering::Relaxed); + stale.pixels + } + None => inner.spare.take().unwrap_or_default(), + }; + inner.pending = Some(Frame { + pixels, + stride: meta.stride as usize, + width: meta.width, + height: meta.height, + video_format: meta.video_format, + pts_ns: meta.pts_ns, + crop: CropRect { + x: meta.crop_x, + y: meta.crop_y, + width: meta.crop_width, + height: meta.crop_height, + }, + has_crop: meta.has_crop != 0, + dmabuf: Some(desc), }); self.received.fetch_add(1, Ordering::Relaxed); } @@ -854,6 +915,49 @@ extern "C" fn on_frame(user: *mut c_void, frame: *const RawFrame) { } // SAFETY: non-NULL for the duration of the callback, by contract. let frame = unsafe { &*frame }; + + // Tiled dmabuf: no pixels to copy. Duplicate the plane fds (cheap, and it + // keeps the buffer's content reachable after the PW buffer re-queues) and + // hand the descriptor to the main loop for a GPU import. + if frame.is_dmabuf != 0 { + use std::os::fd::{AsRawFd, BorrowedFd}; + let n = frame.n_planes.clamp(0, 4) as usize; + if n == 0 { + return; + } + let mut planes = Vec::with_capacity(n); + for i in 0..n { + let raw = frame.plane_fd[i]; + if raw < 0 { + return; + } + // SAFETY: `raw` is valid for the callback's duration; try_clone + // dups it (F_DUPFD_CLOEXEC) into an fd we own. + let borrowed = unsafe { BorrowedFd::borrow_raw(raw) }; + let Ok(owned) = borrowed.try_clone_to_owned() else { + return; + }; + debug_assert!(owned.as_raw_fd() >= 0); + planes.push(DmabufPlaneOwned { + fd: owned, + offset: frame.plane_offset[i], + stride: frame.plane_stride[i], + }); + } + mailbox.put_dmabuf( + DmabufDesc { + width: frame.width, + height: frame.height, + drm_fourcc: frame.drm_fourcc, + modifier: frame.modifier, + planes, + }, + frame, + ); + (state.sink)(StreamEvent::FrameReady); + return; + } + if frame.data.is_null() || frame.stride <= 0 || frame.height <= 0 { return; } From f8fe0036a0c666a6aece519b03275b0741737783 Mon Sep 17 00:00:00 2001 From: Benjamin Freeman Date: Wed, 26 Aug 2026 21:56:58 +0200 Subject: [PATCH 4/9] fix(capture): make the dmabuf VAAPI import work on radeonsi (#507) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four runtime fixes found by testing the import on AMD/mutter, end to end: 1. Create the DRM device on the render node and derive VAAPI FROM it. The reverse (DRM derived from VAAPI) returns ENOSYS on radeonsi. 2. initial_pool_size = 0 on the DRM_PRIME and mapped-VAAPI frames contexts: they only wrap/map imported surfaces, and asking for a pool makes av_hwframe_ctx_init reject the RGB layout with EINVAL. 3. Allocate the buffersrc (avfilter_graph_alloc_filter), set its params including hw_frames_ctx, THEN init it — a hardware pix_fmt is rejected at init while hw_frames_ctx is still null. 4. Wrap the DRM descriptor in an AVBufferRef so the DRM_PRIME source frame is ref-counted, which av_hwframe_map requires. Validated on AMD/radeonsi + mutter: whole-monitor editor scroll now records 42.4 distinct fps (was ~2 over shm; OBS ~24), with convertMs 0.0 and uploadMs ~0.002 — the frame stays on the GPU from capture to encode. Still gated behind OPENSCREEN_PIPEWIRE_FORCE_DMABUF. Follow-ups: make dmabuf automatic when the backend is VAAPI, window/crop via VPP, non-VAAPI fallback. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../docs/dmabuf-vaapi-plan.md | 14 ++- .../pipewire-capture/src/dmabuf_import.rs | 95 +++++++++++-------- 2 files changed, 64 insertions(+), 45 deletions(-) diff --git a/electron/native/pipewire-capture/docs/dmabuf-vaapi-plan.md b/electron/native/pipewire-capture/docs/dmabuf-vaapi-plan.md index db9e093b3..304187697 100644 --- a/electron/native/pipewire-capture/docs/dmabuf-vaapi-plan.md +++ b/electron/native/pipewire-capture/docs/dmabuf-vaapi-plan.md @@ -127,11 +127,15 @@ Key architecture calls: `stage_hw`/`hw_staged` path (held across re-encodes) + Drop. - [x] `capture.rs`: dmabuf branch → importer → `stage_hw`; deferred encoder open. - [x] Whole pipeline compiles and links; full helper builds, libavfilter staged. -- [ ] **On-device record/check/fix.** Test via `OPENSCREEN_PIPEWIRE_FORCE_DMABUF=1` - (dmabuf still opt-in until proven). Expect a NON-garbled full-monitor recording - at distinct-fps ≈ OBS (~24). Likely first-try failure points: `av_hwframe_map` - wanting `buf[0]` on the DRM_PRIME source; scale_vaapi output pool size; the - DRM/VAAPI derive on radeonsi. +- [x] **On-device validated** (AMD/radeonsi + mutter, via `FORCE_DMABUF`). + Full-monitor editor scroll: **42.4 distinct fps** (was ~2 on shm; OBS ~24), + `convertMs 0.0`, `uploadMs ~0.002` — the frame never touches the CPU. Four + runtime fixes were needed and are in: (1) create the DRM device on the render + node and derive VAAPI from it — the reverse is ENOSYS on radeonsi; (2) + `initial_pool_size = 0` on the map-only frames contexts; (3) allocate the + buffersrc then set params (hw_frames_ctx) then init, since a HW pix_fmt is + rejected at init otherwise; (4) wrap the DRM descriptor in an AVBufferRef so the + source frame is ref-counted for `av_hwframe_map`. - [ ] Once proven: make dmabuf preference automatic when the backend is VAAPI (drop the FORCE env gate) + window/crop via VPP + non-VAAPI fallback guard. - [ ] Test on AMD/GNOME: confirm `uses_dmabuf=1`, distinct-fps ≈ OBS (~24), diff --git a/electron/native/pipewire-capture/src/dmabuf_import.rs b/electron/native/pipewire-capture/src/dmabuf_import.rs index 7697c84d1..260e57878 100644 --- a/electron/native/pipewire-capture/src/dmabuf_import.rs +++ b/electron/native/pipewire-capture/src/dmabuf_import.rs @@ -12,7 +12,6 @@ //! `avcodec_send_frame` accepts directly. See docs/dmabuf-vaapi-plan.md. use crate::ffmpeg as ff; -use std::ffi::CString; use std::ptr; /// `av_frame_free` wants a `**AVFrame`; wrap the pointer in a local so the null @@ -22,6 +21,10 @@ unsafe fn free_frame(frame: *mut ff::AVFrame) { ff::av_frame_free(&mut p); } +/// The AVBufferRef around the DRM descriptor owns nothing heap-allocated (the +/// descriptor is a stack local that outlives the map), so freeing it is a no-op. +unsafe extern "C" fn noop_buffer_free(_opaque: *mut std::ffi::c_void, _data: *mut u8) {} + /// One dmabuf plane. `fd` is borrowed for the duration of [`DmabufImporter::import`] /// only — VAAPI dups it during surface creation, so the caller may close it after. pub struct DmabufPlane { @@ -96,30 +99,32 @@ impl DmabufImporter { buffersink_ctx: ptr::null_mut(), }; - // A standalone VAAPI device (the encoder will share it). Default - // device selection matches how the encoder opens VAAPI today. + // Order matters: create the DRM device on the render node FIRST, then + // derive VAAPI from it. The reverse (DRM derived from VAAPI) returns + // ENOSYS on radeonsi — VAAPI knows how to open on a DRM fd, but not the + // other way round. The DRM device backs the DRM_PRIME source frames; + // the derived VAAPI device backs the mapped surface and the encoder. + let node = c"/dev/dri/renderD128"; let created = ff::av_hwdevice_ctx_create( - &mut me.va_device, - ff::AV_HWDEVICE_TYPE_VAAPI, - ptr::null(), + &mut me.drm_device, + ff::AV_HWDEVICE_TYPE_DRM, + node.as_ptr(), ptr::null_mut(), 0, ); if created < 0 { - return Err(format!("av_hwdevice_ctx_create(VAAPI): {}", ff::err_to_string(created))); + return Err(format!("av_hwdevice_ctx_create(DRM): {}", ff::err_to_string(created))); } - // DRM device derived from the same GPU, so the mapped surface and the - // VAAPI device share a backing. let derived = ff::av_hwdevice_ctx_create_derived( - &mut me.drm_device, - ff::AV_HWDEVICE_TYPE_DRM, - me.va_device, + &mut me.va_device, + ff::AV_HWDEVICE_TYPE_VAAPI, + me.drm_device, 0, ); if derived < 0 { return Err(format!( - "av_hwdevice_ctx_create_derived(DRM): {}", + "av_hwdevice_ctx_create_derived(VAAPI): {}", ff::err_to_string(derived) )); } @@ -147,10 +152,12 @@ impl DmabufImporter { (*ctx).sw_format = self.sw_format; (*ctx).width = self.width; (*ctx).height = self.height; - // A small pool: the mapped surface is short-lived (consumed by the VPP in - // the same call). DRM_PRIME frames are imported, not pooled, so the count - // is nominal there. - (*ctx).initial_pool_size = 4; + // Pool size 0: these contexts only WRAP/MAP externally-supplied surfaces + // (the DRM_PRIME source is our imported dmabuf; the VAAPI context is filled + // by av_hwframe_map DIRECT). Asking for a pre-allocated pool makes + // av_hwframe_ctx_init reject the format with EINVAL, since neither has an + // allocator for these RGB layouts. + (*ctx).initial_pool_size = 0; let init = ff::av_hwframe_ctx_init(frames); if init < 0 { let mut f = frames; @@ -174,39 +181,32 @@ impl DmabufImporter { return Err("a required filter (buffer/buffersink/scale_vaapi) is missing".to_owned()); } - // buffersrc: the input is a VAAPI surface, so pix_fmt is VAAPI and the - // real (sw) format rides on the hw_frames_ctx set below. - let args = CString::new(format!( - "video_size={}x{}:pix_fmt={}:time_base=1/1000000:pixel_aspect=1/1", - self.width, - self.height, - ff::AV_PIX_FMT_VAAPI as i32 - )) - .map_err(|_| "buffersrc args contained a NUL".to_owned())?; - - let rc = ff::avfilter_graph_create_filter( - &mut self.buffersrc_ctx, - buffersrc, - c"in".as_ptr(), - args.as_ptr(), - ptr::null_mut(), - self.graph, - ); - if rc < 0 { - return Err(format!("create buffersrc: {}", ff::err_to_string(rc))); + // buffersrc: the input is a VAAPI surface. Allocate WITHOUT initialising + // (avfilter_graph_alloc_filter, not ..._create_filter): a hardware pix_fmt + // is rejected at init unless hw_frames_ctx is already set, and only + // av_buffersrc_parameters_set can set it. So: alloc → set params → init. + self.buffersrc_ctx = ff::avfilter_graph_alloc_filter(self.graph, buffersrc, c"in".as_ptr()); + if self.buffersrc_ctx.is_null() { + return Err("avfilter_graph_alloc_filter(buffersrc) failed".to_owned()); } - - // Attach the VAAPI (BGRx) frames context the mapped surfaces come from. let par = ff::av_buffersrc_parameters_alloc(); if par.is_null() { return Err("av_buffersrc_parameters_alloc failed".to_owned()); } + (*par).format = ff::AV_PIX_FMT_VAAPI as i32; + (*par).width = self.width; + (*par).height = self.height; + (*par).time_base = ff::AVRational { num: 1, den: 1_000_000 }; (*par).hw_frames_ctx = ff::av_buffer_ref(self.va_map_frames); let set = ff::av_buffersrc_parameters_set(self.buffersrc_ctx, par); ff::av_free(par as *mut _); if set < 0 { return Err(format!("av_buffersrc_parameters_set: {}", ff::err_to_string(set))); } + let inited = ff::avfilter_init_str(self.buffersrc_ctx, ptr::null()); + if inited < 0 { + return Err(format!("avfilter_init_str(buffersrc): {}", ff::err_to_string(inited))); + } let rc = ff::avfilter_graph_create_filter( &mut self.buffersink_ctx, @@ -296,7 +296,22 @@ impl DmabufImporter { (*src).format = ff::AV_PIX_FMT_DRM_PRIME as i32; (*src).width = self.width; (*src).height = self.height; - (*src).data[0] = &mut desc as *mut _ as *mut u8; + // av_hwframe_map rejects a source frame that is not ref-counted, so + // wrap the descriptor in an AVBufferRef (freed as a no-op — `desc` + // lives on the stack until this function returns, past the map). + let buf = ff::av_buffer_create( + &mut desc as *mut _ as *mut u8, + std::mem::size_of::(), + Some(noop_buffer_free), + ptr::null_mut(), + 0, + ); + if buf.is_null() { + free_frame(src); + return Err("av_buffer_create(drm descriptor) failed".to_owned()); + } + (*src).buf[0] = buf; + (*src).data[0] = (*buf).data; (*src).hw_frames_ctx = ff::av_buffer_ref(self.drm_frames); // Map the dmabuf into a VAAPI (BGRx) surface, zero-copy. From 5bbe2424fd92f7edf20283f16c41d59d46b9c1db Mon Sep 17 00:00:00 2001 From: Benjamin Freeman Date: Wed, 26 Aug 2026 22:07:52 +0200 Subject: [PATCH 5/9] feat(capture): auto-enable dmabuf import with shm fallback (#507) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Turns the zero-copy path from opt-in into the default, safely: - dmabuf_import::available() probes once per session by building a nominal importer (VAAPI device + DRM derive + scale_vaapi graph). It is the exact pipeline a real frame uses, so success is a strong guarantee the import will work — and it fails cleanly on a non-VAAPI GPU or a driver that cannot map. - When the probe passes, osc_pw_start is told to prefer_dmabuf: the stream now offers dmabuf BEFORE shm. mutter hands us a tiled dmabuf we import on the GPU; a compositor that cannot produce dmabuf still negotiates on the shm object that stays in the offer. No stream renegotiation, no regression path. - A linear/mmappable dmabuf still takes the CPU path unchanged; only a tiled buffer routes through the importer. Validated on AMD/radeonsi + mutter with NO env var: whole-monitor editor scroll records 39.8 distinct fps, convertMs 0.0, 0 dropped. OPENSCREEN_PIPEWIRE_FORCE_DMABUF still forces the swap for testing on machines where the probe would decline. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../native/pipewire-capture/csrc/pw_shim.c | 28 +++++++++++++------ .../native/pipewire-capture/csrc/pw_shim.h | 5 ++++ .../docs/dmabuf-vaapi-plan.md | 13 +++++++-- .../pipewire-capture/src/dmabuf_import.rs | 11 ++++++++ electron/native/pipewire-capture/src/main.rs | 16 +++++++++++ electron/native/pipewire-capture/src/shim.rs | 6 ++++ 6 files changed, 68 insertions(+), 11 deletions(-) diff --git a/electron/native/pipewire-capture/csrc/pw_shim.c b/electron/native/pipewire-capture/csrc/pw_shim.c index ece1e67ad..d094e3007 100644 --- a/electron/native/pipewire-capture/csrc/pw_shim.c +++ b/electron/native/pipewire-capture/csrc/pw_shim.c @@ -207,6 +207,12 @@ struct osc_pw_session { struct spa_video_info_raw format; int buffer_info_reports; int want_video; + /* Set by the caller when the VAAPI dmabuf-import pipeline is available, which + * makes the stream offer dmabuf BEFORE shm so a tiled monitor buffer is + * imported on the GPU instead of copied through throttled shm (issue #507). + * shm stays in the offer as the fallback, so a compositor that cannot produce + * dmabuf still negotiates. */ + int prefer_dmabuf; /* Set from the negotiated format's SPA_VIDEO_FLAG_MODIFIER, which is what * decides whether buffers arrive as dmabuf fds or shared memory. */ int uses_dmabuf; @@ -1322,6 +1328,7 @@ static const struct pw_stream_events osc_stream_events = { }; struct osc_pw_session *osc_pw_start(int fd, uint32_t node_id, int want_video, + int prefer_dmabuf, const struct osc_pw_callbacks *callbacks, char *err, size_t err_len) { @@ -1347,6 +1354,7 @@ struct osc_pw_session *osc_pw_start(int fd, uint32_t node_id, int want_video, } session->callbacks = *callbacks; session->want_video = want_video; + session->prefer_dmabuf = prefer_dmabuf; /* calloc zeroes these, and 0 is a legitimate fd — so the "nothing pending" * sentinel has to be set explicitly. dmabuf_maps is keyed on ptr != NULL, * which calloc does get right. */ @@ -1405,18 +1413,20 @@ struct osc_pw_session *osc_pw_start(int fd, uint32_t node_id, int want_video, params[1] = osc_build_enum_format_dmabuf(&builder); /* - * Test affordance. Every compositor available for local testing — mutter, - * sway via xdg-desktop-portal-wlr — offers shm, so params[0] always wins and - * the DMA-BUF branch below (osc_map_dmabuf, the DMA_BUF_IOCTL_SYNC bracket, - * the dmabuf arm of osc_read_frame) never executes outside niri. Dropping - * the shm object leaves the producer no choice, which is the only way to - * exercise that code without the compositor from issue #287. + * When the GPU import path is available (prefer_dmabuf), offer dmabuf FIRST + * and shm SECOND: mutter then hands us a tiled dmabuf we import on the GPU + * (issue #507) instead of the shm buffer it throttles for a whole monitor. + * shm stays as the fallback, so a compositor that cannot produce dmabuf still + * negotiates on the shm object. The env var forces the same swap for testing + * on a machine where the probe would say no. * - * Never set in production: it would break exactly the compatibility the - * ordering above exists to preserve. + * Without either, the ordering is unchanged — shm first — so nothing moves on + * a build or driver without the VAAPI import. */ - if (getenv("OPENSCREEN_PIPEWIRE_FORCE_DMABUF") != NULL) { + if (session->prefer_dmabuf || getenv("OPENSCREEN_PIPEWIRE_FORCE_DMABUF") != NULL) { + const struct spa_pod *shm = params[0]; params[0] = params[1]; + params[1] = shm; } if (params[0] == NULL || params[1] == NULL) { diff --git a/electron/native/pipewire-capture/csrc/pw_shim.h b/electron/native/pipewire-capture/csrc/pw_shim.h index 878f2445c..f4d3a04c3 100644 --- a/electron/native/pipewire-capture/csrc/pw_shim.h +++ b/electron/native/pipewire-capture/csrc/pw_shim.h @@ -217,9 +217,14 @@ const char *osc_pw_library_version(void); * buffer types; without it neither happens, and a cursor-only session never pays * to map a full-screen framebuffer per frame. * + * `prefer_dmabuf` offers dmabuf before shm so a tiled monitor buffer is imported + * on the GPU rather than copied through throttled shm (issue #507); set it only + * when the VAAPI import pipeline is available. shm remains the fallback. + * * Returns NULL on failure, with a message in `err`. */ struct osc_pw_session *osc_pw_start(int fd, uint32_t node_id, int want_video, + int prefer_dmabuf, const struct osc_pw_callbacks *callbacks, char *err, size_t err_len); diff --git a/electron/native/pipewire-capture/docs/dmabuf-vaapi-plan.md b/electron/native/pipewire-capture/docs/dmabuf-vaapi-plan.md index 304187697..e08bb18f3 100644 --- a/electron/native/pipewire-capture/docs/dmabuf-vaapi-plan.md +++ b/electron/native/pipewire-capture/docs/dmabuf-vaapi-plan.md @@ -136,8 +136,17 @@ Key architecture calls: buffersrc then set params (hw_frames_ctx) then init, since a HW pix_fmt is rejected at init otherwise; (4) wrap the DRM descriptor in an AVBufferRef so the source frame is ref-counted for `av_hwframe_map`. -- [ ] Once proven: make dmabuf preference automatic when the backend is VAAPI - (drop the FORCE env gate) + window/crop via VPP + non-VAAPI fallback guard. +- [x] **Auto-enable + fallback** (validated, no env). `dmabuf_import::available()` + probes once at session start by building a nominal importer; when it succeeds + the stream offers dmabuf BEFORE shm (`osc_pw_start(prefer_dmabuf)`), else stays + on shm. shm remains in the offer as the negotiation fallback, so a compositor + that cannot produce dmabuf — or a GPU where the importer will not build — keeps + today's path with no regression. Confirmed: full-monitor scroll records 39.8 + distinct fps with `convertMs 0.0` and no force flag. `OPENSCREEN_PIPEWIRE_FORCE_DMABUF` + still forces the swap for testing. +- [ ] Follow-ups: window/crop via VPP (v1 is whole-monitor); per-frame import + failure after a successful probe still errors (rare) rather than renegotiating + to shm; test on Intel/NVIDIA-vaapi. - [ ] Test on AMD/GNOME: confirm `uses_dmabuf=1`, distinct-fps ≈ OBS (~24), crop correct for window captures, cursor unaffected. Regression-check software encode and a wlroots/niri compositor. diff --git a/electron/native/pipewire-capture/src/dmabuf_import.rs b/electron/native/pipewire-capture/src/dmabuf_import.rs index 260e57878..95febbd11 100644 --- a/electron/native/pipewire-capture/src/dmabuf_import.rs +++ b/electron/native/pipewire-capture/src/dmabuf_import.rs @@ -59,6 +59,17 @@ fn sw_format_for_fourcc(drm_fourcc: u32) -> Option { } } +/// Whether the zero-copy VAAPI dmabuf-import pipeline can be built on this +/// machine. Constructs a nominal importer, which exercises the DRM→VAAPI device +/// creation, the frames contexts and the `scale_vaapi` graph — everything that +/// fails on a non-VAAPI GPU or a driver that cannot map a dmabuf. Success does +/// not depend on the exact dimensions, so a fixed probe size is representative. +/// When this is true the stream prefers dmabuf; when false it stays on shm. +pub fn available() -> bool { + const XRGB8888: u32 = 0x34325258; + DmabufImporter::new(1920, 1080, XRGB8888).is_ok() +} + pub struct DmabufImporter { width: i32, height: i32, diff --git a/electron/native/pipewire-capture/src/main.rs b/electron/native/pipewire-capture/src/main.rs index d12b225f3..61aba8c8f 100644 --- a/electron/native/pipewire-capture/src/main.rs +++ b/electron/native/pipewire-capture/src/main.rs @@ -506,6 +506,7 @@ fn begin_stream( portal_stream: &mut Option, granted_kind: &mut Option, stream: portal::PortalStream, + prefer_dmabuf: bool, ) -> Result<(), ()> { // The fd is consumed by libpipewire; the rest is kept for the // `stream-started` event, emitted once the format is negotiated. @@ -518,6 +519,7 @@ fn begin_stream( let _ = forward.send(Message::Stream(event)); }), frames.clone(), + prefer_dmabuf, ) { Ok(started) => { *session = Some(started); @@ -577,6 +579,18 @@ fn run( .output_path .as_ref() .map(|_| Arc::new(FrameMailbox::default())); + // Offer dmabuf ahead of shm (issue #507) only for a video session AND only + // when the VAAPI import pipeline actually builds on this GPU. Probed once + // here — it constructs a VAAPI device and filtergraph — so a machine that + // cannot import (no VAAPI, or a driver that will not map) simply keeps the + // shm path with no per-recording cost. + let prefer_dmabuf = frames.is_some() && crate::dmabuf_import::available(); + if frames.is_some() { + let _ = emitter.emit(&Event::Debug { + code: "dmabuf-import".to_owned(), + data: json_map([("available", prefer_dmabuf.into())]), + }); + } let mut capture: Option = None; // Started before the portal picker so the streams are warm and the graph // has settled by the time the first video frame arrives. Everything they @@ -798,6 +812,7 @@ fn run( &mut portal_stream, &mut granted_kind, stream, + prefer_dmabuf, ) { exit_code = 1; break; @@ -856,6 +871,7 @@ fn run( &mut portal_stream, &mut granted_kind, stream, + prefer_dmabuf, ) { exit_code = 1; break; diff --git a/electron/native/pipewire-capture/src/shim.rs b/electron/native/pipewire-capture/src/shim.rs index 6be0424e3..a37693b81 100644 --- a/electron/native/pipewire-capture/src/shim.rs +++ b/electron/native/pipewire-capture/src/shim.rs @@ -117,6 +117,7 @@ extern "C" { fd: i32, node_id: u32, want_video: i32, + prefer_dmabuf: i32, callbacks: *const RawCallbacks, err: *mut c_char, err_len: usize, @@ -827,8 +828,12 @@ impl Session { node_id: u32, sink: Sink, frames: Option>, + // Offer dmabuf before shm for a whole-monitor GPU import (issue #507). + // Only honoured for a video session; ignored for cursor-only. + prefer_dmabuf: bool, ) -> Result { let want_video = i32::from(frames.is_some()); + let prefer_dmabuf = i32::from(want_video != 0 && prefer_dmabuf); let state = Box::new(CallbackState { sink, frames }); let user = &*state as *const CallbackState as *mut c_void; let callbacks = RawCallbacks { @@ -848,6 +853,7 @@ impl Session { fd.into_raw_fd(), node_id, want_video, + prefer_dmabuf, &callbacks, err.as_mut_ptr(), ERR_LEN, From 0b6d7b5c72e7ede64f7271aa967108ab015148d8 Mon Sep 17 00:00:00 2001 From: Benjamin Freeman Date: Wed, 26 Aug 2026 23:39:30 +0200 Subject: [PATCH 6/9] feat(capture): crop window captures on the GPU in the VPP (#507) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extends the zero-copy path to window sources, not just monitors. mutter sizes a window stream to the whole monitor and reports the window rectangle as a crop, so the importer now carries two sizes: the source (full stream) it maps, and the output (the committed crop) the scale_vaapi VPP emits. Per frame, import() sets the mapped surface's crop_left/top/right/bottom to the live window origin and the committed size, so VAAPI reads exactly the window region and scales it to the output — cropped on the GPU, no CPU touch. A monitor is the degenerate case (source == output, crop 0), unchanged. Validated on AMD/mutter: a 724x576 GNOME window records at 724x576, sharp, convertMs 0.0, 26.6 distinct fps. (A CSD window's shadow margin appears as a black border because it is inside mutter's crop rect — same as any capturer.) Co-Authored-By: Claude Opus 4.8 (1M context) --- .../docs/dmabuf-vaapi-plan.md | 13 +++- .../native/pipewire-capture/src/capture.rs | 43 +++++++---- .../pipewire-capture/src/dmabuf_import.rs | 74 ++++++++++++++----- 3 files changed, 94 insertions(+), 36 deletions(-) diff --git a/electron/native/pipewire-capture/docs/dmabuf-vaapi-plan.md b/electron/native/pipewire-capture/docs/dmabuf-vaapi-plan.md index e08bb18f3..aa25576e4 100644 --- a/electron/native/pipewire-capture/docs/dmabuf-vaapi-plan.md +++ b/electron/native/pipewire-capture/docs/dmabuf-vaapi-plan.md @@ -144,9 +144,16 @@ Key architecture calls: today's path with no regression. Confirmed: full-monitor scroll records 39.8 distinct fps with `convertMs 0.0` and no force flag. `OPENSCREEN_PIPEWIRE_FORCE_DMABUF` still forces the swap for testing. -- [ ] Follow-ups: window/crop via VPP (v1 is whole-monitor); per-frame import - failure after a successful probe still errors (rare) rather than renegotiating - to shm; test on Intel/NVIDIA-vaapi. +- [x] **Window crop via VPP** (validated). The importer now takes a source size + (the full stream) and an output size (the committed crop); `scale_vaapi` outputs + the crop size and `import` sets the mapped surface's crop_left/top/right/bottom + per frame so the VA source region is the window rect — cropped and format- + converted on the GPU, no scaling (region == output). Confirmed: a 724×576 GNOME + window records at 724×576, sharp, convertMs 0.0, 26.6 distinct fps. (The black + margin some CSD windows show is the shadow/decoration in mutter's crop rect — + same on any capture tool, not introduced here.) +- [ ] Follow-ups: per-frame import failure after a successful probe still errors + (rare) rather than renegotiating to shm; test on Intel/NVIDIA-vaapi. - [ ] Test on AMD/GNOME: confirm `uses_dmabuf=1`, distinct-fps ≈ OBS (~24), crop correct for window captures, cursor unaffected. Regression-check software encode and a wlroots/niri compositor. diff --git a/electron/native/pipewire-capture/src/capture.rs b/electron/native/pipewire-capture/src/capture.rs index 628eba3a4..4324f3b01 100644 --- a/electron/native/pipewire-capture/src/capture.rs +++ b/electron/native/pipewire-capture/src/capture.rs @@ -246,14 +246,16 @@ impl Capture { let mut rejected = Vec::new(); let (encoder, importer) = match dmabuf { Some(desc) => { - // v1 targets the whole monitor: the importer and encoder are - // sized to the full stream (no crop). The encoder is FORCED to - // VAAPI because that is the only backend that can consume the - // mapped surface; a non-VAAPI machine should never have - // negotiated dmabuf in the first place. + // The importer maps the full stream (`desc`) and its VPP crops to + // the committed record size (`width`/`height`): equal to the source + // for a monitor, or the window's crop rectangle for a window. The + // encoder is FORCED to VAAPI — the only backend that can consume the + // mapped surface; a non-VAAPI machine never negotiates dmabuf. let importer = crate::dmabuf_import::DmabufImporter::new( desc.width, desc.height, + width, + height, desc.drm_fourcc, )?; // SAFETY: the importer's device and NV12 frames context are live @@ -261,7 +263,7 @@ impl Capture { // alongside it below. let encoder = unsafe { VideoEncoder::open_importing( - VideoParams { width: desc.width, height: desc.height, fps, bitrate }, + VideoParams { width, height, fps, bitrate }, importer.device(), importer.output_frames_ctx(), )? @@ -361,10 +363,15 @@ impl Capture { /// written until [`Self::advance`] runs. pub fn stage(&mut self, frame: &shim::Frame) -> Result<(), String> { // Zero-copy dmabuf path: import the tiled GPU buffer into an NV12 VAAPI - // surface and hand it to the encoder as-is — no swscale, no crop math - // (v1 is whole-monitor only). See issue #507. - if let Some(desc) = &frame.dmabuf { + // surface (the VPP crops a window to its committed rectangle) and hand it + // to the encoder as-is — no swscale. See issue #507. + if frame.dmabuf.is_some() { use std::os::fd::AsRawFd; + // The crop origin, clamped to stay inside the buffer — same rule as the + // CPU path. Computed before the mutable importer borrow. For a monitor + // this is (0, 0). + let (crop_x, crop_y) = self.read_origin(frame); + let desc = frame.dmabuf.as_ref().expect("checked is_some above"); let importer = self .importer .as_mut() @@ -378,13 +385,17 @@ impl Capture { stride: plane.stride, }) .collect(); - let nv12 = importer.import(&crate::dmabuf_import::DmabufFrame { - width: desc.width, - height: desc.height, - drm_fourcc: desc.drm_fourcc, - modifier: desc.modifier, - planes: &planes, - })?; + let nv12 = importer.import( + &crate::dmabuf_import::DmabufFrame { + width: desc.width, + height: desc.height, + drm_fourcc: desc.drm_fourcc, + modifier: desc.modifier, + planes: &planes, + }, + crop_x, + crop_y, + )?; // SAFETY: `nv12` is a VAAPI NV12 frame from the pool the encoder was // opened against; the encoder takes ownership. unsafe { self.encoder.stage_hw(nv12) }; diff --git a/electron/native/pipewire-capture/src/dmabuf_import.rs b/electron/native/pipewire-capture/src/dmabuf_import.rs index 95febbd11..1cc7669ee 100644 --- a/electron/native/pipewire-capture/src/dmabuf_import.rs +++ b/electron/native/pipewire-capture/src/dmabuf_import.rs @@ -67,12 +67,18 @@ fn sw_format_for_fourcc(drm_fourcc: u32) -> Option { /// When this is true the stream prefers dmabuf; when false it stays on shm. pub fn available() -> bool { const XRGB8888: u32 = 0x34325258; - DmabufImporter::new(1920, 1080, XRGB8888).is_ok() + DmabufImporter::new(1920, 1080, 1920, 1080, XRGB8888).is_ok() } pub struct DmabufImporter { - width: i32, - height: i32, + /// Size of the incoming dmabuf (the whole stream). For a window this is the + /// monitor; for a monitor it equals the output size. + src_width: i32, + src_height: i32, + /// Size of the NV12 the graph emits — the recorded size. For a window this is + /// the committed crop rectangle; for a monitor it equals the source size. + out_width: i32, + out_height: i32, sw_format: ff::AVPixelFormat, /// VAAPI device, shared with the encoder (whose `hw_frames_ctx` comes from /// [`Self::output_frames_ctx`]). @@ -89,17 +95,27 @@ pub struct DmabufImporter { } impl DmabufImporter { - /// Builds the device, frames contexts and `scale_vaapi` graph for a stream of - /// `width`×`height` `drm_fourcc` buffers. - pub fn new(width: i32, height: i32, drm_fourcc: u32) -> Result { + /// Builds the device, frames contexts and `scale_vaapi` graph. `src` is the + /// incoming dmabuf size (the whole stream); `out` is the recorded size — equal + /// to `src` for a monitor, or the window's crop rectangle for a window (the + /// graph then crops the source region down to it, on the GPU). + pub fn new( + src_width: i32, + src_height: i32, + out_width: i32, + out_height: i32, + drm_fourcc: u32, + ) -> Result { let sw_format = sw_format_for_fourcc(drm_fourcc).ok_or_else(|| format!("unsupported dmabuf fourcc {drm_fourcc:#x}"))?; // SAFETY: every pointer is checked before use and freed in Drop. unsafe { let mut me = DmabufImporter { - width, - height, + src_width, + src_height, + out_width, + out_height, sw_format, va_device: ptr::null_mut(), drm_device: ptr::null_mut(), @@ -161,8 +177,8 @@ impl DmabufImporter { let ctx = (*frames).data as *mut ff::AVHWFramesContext; (*ctx).format = hw_format; (*ctx).sw_format = self.sw_format; - (*ctx).width = self.width; - (*ctx).height = self.height; + (*ctx).width = self.src_width; + (*ctx).height = self.src_height; // Pool size 0: these contexts only WRAP/MAP externally-supplied surfaces // (the DRM_PRIME source is our imported dmabuf; the VAAPI context is filled // by av_hwframe_map DIRECT). Asking for a pre-allocated pool makes @@ -205,8 +221,8 @@ impl DmabufImporter { return Err("av_buffersrc_parameters_alloc failed".to_owned()); } (*par).format = ff::AV_PIX_FMT_VAAPI as i32; - (*par).width = self.width; - (*par).height = self.height; + (*par).width = self.src_width; + (*par).height = self.src_height; (*par).time_base = ff::AVRational { num: 1, den: 1_000_000 }; (*par).hw_frames_ctx = ff::av_buffer_ref(self.va_map_frames); let set = ff::av_buffersrc_parameters_set(self.buffersrc_ctx, par); @@ -231,12 +247,20 @@ impl DmabufImporter { return Err(format!("create buffersink: {}", ff::err_to_string(rc))); } + // Output size = the recorded (out) size. For a monitor that equals the + // source; for a window it is the crop rectangle, and the per-frame crop + // fields set in `import` pick which region of the source is scaled into it. + let scale_args = std::ffi::CString::new(format!( + "w={}:h={}:format=nv12", + self.out_width, self.out_height + )) + .map_err(|_| "scale_vaapi args contained a NUL".to_owned())?; let mut scale_ctx: *mut ff::AVFilterContext = ptr::null_mut(); let rc = ff::avfilter_graph_create_filter( &mut scale_ctx, scale, c"vpp".as_ptr(), - c"format=nv12".as_ptr(), + scale_args.as_ptr(), ptr::null_mut(), self.graph, ); @@ -275,8 +299,15 @@ impl DmabufImporter { } /// Maps one dmabuf and returns an NV12 VAAPI frame (caller unrefs it). The - /// plane fds are only touched during this call. - pub fn import(&mut self, frame: &DmabufFrame) -> Result<*mut ff::AVFrame, String> { + /// plane fds are only touched during this call. `crop_x`/`crop_y` are the + /// origin of the recorded region within the source; the region size is the + /// importer's output size. For a monitor both are 0 and out == src (no crop). + pub fn import( + &mut self, + frame: &DmabufFrame, + crop_x: i32, + crop_y: i32, + ) -> Result<*mut ff::AVFrame, String> { if frame.planes.is_empty() || frame.planes.len() > 4 { return Err(format!("dmabuf has {} planes", frame.planes.len())); } @@ -305,8 +336,8 @@ impl DmabufImporter { return Err("av_frame_alloc(src) failed".to_owned()); } (*src).format = ff::AV_PIX_FMT_DRM_PRIME as i32; - (*src).width = self.width; - (*src).height = self.height; + (*src).width = self.src_width; + (*src).height = self.src_height; // av_hwframe_map rejects a source frame that is not ref-counted, so // wrap the descriptor in an AVBufferRef (freed as a no-op — `desc` // lives on the stack until this function returns, past the map). @@ -345,6 +376,15 @@ impl DmabufImporter { return Err(format!("av_hwframe_map: {}", ff::err_to_string(mrc))); } + // Crop the source down to the recorded region at the live origin. + // scale_vaapi reads these fields to set the VA source rectangle, so a + // window is cropped on the GPU before scaling. A monitor leaves them + // at 0 (crop_x/y are 0 and out == src), so nothing is cropped. + (*mapped).crop_left = crop_x.max(0) as usize; + (*mapped).crop_top = crop_y.max(0) as usize; + (*mapped).crop_right = (self.src_width - crop_x - self.out_width).max(0) as usize; + (*mapped).crop_bottom = (self.src_height - crop_y - self.out_height).max(0) as usize; + // Push through scale_vaapi → NV12. let pushed = ff::av_buffersrc_add_frame(self.buffersrc_ctx, mapped); free_frame(mapped); From 8d91be7757cf638b96c4cc01f8a943f712911b8a Mon Sep 17 00:00:00 2001 From: Benjamin Freeman Date: Thu, 27 Aug 2026 01:13:03 +0200 Subject: [PATCH 7/9] fix(capture): address CodeRabbit review on #508 All seven findings on the dmabuf-import PR: 1. (major) Advertise tiled modifiers only when prefer_dmabuf: a shm-less producer could otherwise hand us a tiled buffer we cannot read when the importer is unavailable. Gate the EGL enumeration; LINEAR/INVALID stay. 2. Honour a forced non-VAAPI backend on the dmabuf path (skip the import when forced != VAAPI), so the documented `forced` workaround still applies. 3. Check the second avfilter_link's return value, not just the first. 4. Reject dmabuf descriptors whose planes span multiple fds (we build one DRM object from planes[0].fd), rather than silently reading wrong memory. 5. (major) Heap-allocate the AVDRMFrameDescriptor: av_hwframe_map retains the source frame (ref-counted) until the mapping is released, so a stack desc was a latent use-after-free. Free it from the buffer's own callback. 6. Update stale test callers (Frame.dmabuf, Session::start, Capture::start) so the test targets compile again. 7. (major) Keep the PipeWire buffer owned until the GPU copy completes. Duping the plane fds preserved the dmabuf object but not a content snapshot, so a re-queued buffer the compositor overwrote could be encoded torn. on_frame now TAKES the buffer (returns held); the shim leaves it un-queued; the descriptor carries the handle and, on drop (after Capture::stage imports it, or on supersede), pushes it to a queue the main loop drains and re-queues through a thread-loop-locked osc_pw_requeue_buffer. No fd duplication. Validated: 63 lib tests pass; on AMD/mutter both monitor and window record at a full 60fps CFR for the whole take (no pool-drain stall), convertMs 0.0. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../native/pipewire-capture/csrc/pw_shim.c | 53 +++- .../native/pipewire-capture/csrc/pw_shim.h | 35 ++- .../native/pipewire-capture/src/capture.rs | 33 ++- .../pipewire-capture/src/dmabuf_import.rs | 72 +++--- electron/native/pipewire-capture/src/main.rs | 12 + electron/native/pipewire-capture/src/shim.rs | 228 ++++++++++++------ 6 files changed, 303 insertions(+), 130 deletions(-) diff --git a/electron/native/pipewire-capture/csrc/pw_shim.c b/electron/native/pipewire-capture/csrc/pw_shim.c index d094e3007..95915dc98 100644 --- a/electron/native/pipewire-capture/csrc/pw_shim.c +++ b/electron/native/pipewire-capture/csrc/pw_shim.c @@ -432,7 +432,8 @@ static const struct spa_pod *osc_build_enum_format(struct spa_pod_builder *build * which needs a GPU query, so letting the producer fixate is both simpler and * one fewer round trip that can go wrong. */ -static const struct spa_pod *osc_build_enum_format_dmabuf(struct spa_pod_builder *builder) +static const struct spa_pod *osc_build_enum_format_dmabuf(struct spa_pod_builder *builder, + int prefer_dmabuf) { struct spa_pod_frame object_frame; struct spa_pod_frame choice_frame; @@ -459,10 +460,17 @@ static const struct spa_pod *osc_build_enum_format_dmabuf(struct spa_pod_builder * stay as universal fallbacks. Modifiers match across the 32-bit RGB formats * we offer, so enumerating XRGB8888 is representative. * + * ONLY when prefer_dmabuf: the tiled modifiers are advertised solely when the + * VAAPI import pipeline is available. Otherwise a producer that offers no shm + * format (some wlroots/portal setups) could select a tiled buffer we cannot + * read, where before it would have fallen to a CPU-mappable LINEAR/INVALID + * dmabuf. Offering just those two keeps that path intact. + * * Default first, then every alternative — the default is repeated, same * idiom as SPA_POD_CHOICE_ENUM_Id above. */ uint64_t egl_mods[128]; - int egl_mod_count = osc_query_dmabuf_modifiers(OSC_DRM_FORMAT_XRGB8888, egl_mods, 128); + int egl_mod_count = + prefer_dmabuf ? osc_query_dmabuf_modifiers(OSC_DRM_FORMAT_XRGB8888, egl_mods, 128) : 0; int64_t default_mod = egl_mod_count > 0 ? (int64_t)egl_mods[0] : (int64_t)OSC_DRM_FORMAT_MOD_LINEAR; spa_pod_builder_long(builder, default_mod); @@ -577,7 +585,9 @@ int osc_pw_enum_format_accepts_dmabuf_producer(int with_modifier, int64_t produc const struct spa_pod *consumer; const struct spa_pod *producer; - consumer = with_modifier ? osc_build_enum_format_dmabuf(&ours) : osc_build_enum_format(&ours); + /* The unit test exercises the full tiled offer, so enumerate unconditionally. */ + consumer = + with_modifier ? osc_build_enum_format_dmabuf(&ours, 1) : osc_build_enum_format(&ours); if (consumer == NULL) { return -1; } @@ -1241,8 +1251,11 @@ static void osc_describe_metas(const struct spa_buffer *buffer, char *out, size_ } } -static void osc_inspect_buffer(struct osc_pw_session *session, const struct spa_buffer *buffer) +/* Returns 1 when the on_frame callback took ownership of `pw_buf` (a dmabuf frame + * held for GPU import); the caller must then NOT re-queue it. 0 otherwise. */ +static int osc_inspect_buffer(struct osc_pw_session *session, struct pw_buffer *pw_buf) { + const struct spa_buffer *buffer = pw_buf->buffer; struct osc_pw_cursor cursor; uint32_t meta_size = 0; @@ -1274,7 +1287,15 @@ static void osc_inspect_buffer(struct osc_pw_session *session, const struct spa_ session->dmabuf_sync_fd = -1; if (osc_read_frame(session, buffer, &frame)) { - session->callbacks.on_frame(session->callbacks.user, &frame); + /* The callback needs the pw_buffer to hand back to osc_pw_requeue_buffer + * if it takes ownership of a dmabuf frame. */ + frame.buffer_handle = pw_buf; + if (session->callbacks.on_frame(session->callbacks.user, &frame)) { + /* Taken: leave it un-queued; the consumer will re-queue it once the + * import has copied the pixels. No SYNC bracket is open on this + * path (the import path does not CPU-read), so nothing to close. */ + return 1; + } } /* Closes the DMA_BUF_SYNC_START osc_read_frame opened, if any. Placed * here rather than inside it because the callback above is what actually @@ -1284,6 +1305,7 @@ static void osc_inspect_buffer(struct osc_pw_session *session, const struct spa_ session->dmabuf_sync_fd = -1; } } + return 0; } static void osc_on_process(void *userdata) @@ -1313,11 +1335,26 @@ static void osc_on_process(void *userdata) * throw away the cursor metadata riding on the same buffers. */ while ((b = api.stream_dequeue_buffer(session->stream)) != NULL) { - osc_inspect_buffer(session, b->buffer); - api.stream_queue_buffer(session->stream, b); + /* A dmabuf frame the consumer takes is held out of the queue until it has + * imported the pixels — see osc_pw_requeue_buffer. Everything else (shm, + * cursor-only buffers, declined frames) re-queues immediately. */ + if (!osc_inspect_buffer(session, b)) { + api.stream_queue_buffer(session->stream, b); + } } } +/* See the header. Locks the thread loop so a foreign thread can queue safely. */ +void osc_pw_requeue_buffer(struct osc_pw_session *session, void *buffer_handle) +{ + if (session == NULL || buffer_handle == NULL || session->stream == NULL) { + return; + } + api.thread_loop_lock(session->loop); + api.stream_queue_buffer(session->stream, (struct pw_buffer *)buffer_handle); + api.thread_loop_unlock(session->loop); +} + static const struct pw_stream_events osc_stream_events = { PW_VERSION_STREAM_EVENTS, .state_changed = osc_on_state_changed, @@ -1410,7 +1447,7 @@ struct osc_pw_session *osc_pw_start(int fd, uint32_t node_id, int want_video, * previously failed the whole negotiation with "no more input formats". */ params[0] = osc_build_enum_format(&builder); - params[1] = osc_build_enum_format_dmabuf(&builder); + params[1] = osc_build_enum_format_dmabuf(&builder, session->prefer_dmabuf); /* * When the GPU import path is available (prefer_dmabuf), offer dmabuf FIRST diff --git a/electron/native/pipewire-capture/csrc/pw_shim.h b/electron/native/pipewire-capture/csrc/pw_shim.h index f4d3a04c3..1f013e426 100644 --- a/electron/native/pipewire-capture/csrc/pw_shim.h +++ b/electron/native/pipewire-capture/csrc/pw_shim.h @@ -102,10 +102,13 @@ struct osc_pw_frame { * descriptor below instead of reading `data`. When 0, the CPU path above * applies unchanged (shm, or a linear/implicit dmabuf we could mmap). * - * The fds are BORROWED for the callback's duration only, exactly like - * `data`: the buffer re-queues to the compositor when the callback returns, - * so the import (map + GPU copy into an owned surface) must complete before - * then. `modifier`/`drm_fourcc` describe the tiling and pixel layout. + * When the on_frame callback TAKES a dmabuf frame (returns non-zero), the + * PipeWire buffer is NOT re-queued here — `buffer_handle` is retained by the + * consumer, which keeps the fds and their CONTENT valid until it has imported + * and copied the surface, then calls osc_pw_requeue_buffer. Duplicating the + * fds alone would preserve the dmabuf object but not a snapshot of its pixels, + * so a re-queued buffer the compositor overwrote could be encoded torn. + * `modifier`/`drm_fourcc` describe the tiling and pixel layout. */ int is_dmabuf; uint64_t modifier; /* DRM format modifier of the buffer */ @@ -114,6 +117,10 @@ struct osc_pw_frame { int plane_fd[4]; int32_t plane_offset[4]; int32_t plane_stride[4]; + /* The `struct pw_buffer *` this frame came from, opaque to the consumer. + * Passed back to osc_pw_requeue_buffer once the import is done. Only set (and + * only meaningful) for a dmabuf frame the consumer intends to take. */ + void *buffer_handle; }; /* The negotiated video format. Reported once, from param_changed. */ @@ -133,8 +140,12 @@ struct osc_pw_callbacks { void *user; void (*on_format)(void *user, const struct osc_pw_format *format); void (*on_cursor)(void *user, const struct osc_pw_cursor *cursor); - /* Only ever called when osc_pw_start was given want_video != 0. */ - void (*on_frame)(void *user, const struct osc_pw_frame *frame); + /* Only ever called when osc_pw_start was given want_video != 0. Returns + * non-zero to TAKE OWNERSHIP of the PipeWire buffer (`frame->buffer_handle`): + * the shim then does NOT re-queue it, and the consumer must later call + * osc_pw_requeue_buffer. Zero (the shm/CPU path, and any dmabuf frame the + * consumer declines) re-queues immediately as before. */ + int (*on_frame)(void *user, const struct osc_pw_frame *frame); /* Emitted once per negotiated buffer set. `data_type` is the SPA_DATA_* of * datas[0]; `metas` is a borrowed "Header:12,Cursor:589872" listing of every * metadata block that survived negotiation, which is what distinguishes a @@ -228,6 +239,18 @@ struct osc_pw_session *osc_pw_start(int fd, uint32_t node_id, int want_video, const struct osc_pw_callbacks *callbacks, char *err, size_t err_len); +/* + * Re-queues a PipeWire buffer the on_frame callback took ownership of (returned + * non-zero for), identified by the `buffer_handle` it was given. Call it once the + * frame's pixels have been imported and copied. + * + * SAFE TO CALL FROM ANY THREAD: it takes the PipeWire thread-loop lock around the + * queue, so unlike the shim's own callbacks it must NOT be called from the + * PipeWire thread itself (that would deadlock). The consumer requeues from its + * own loop, which is a different thread. NULL session or handle is a no-op. + */ +void osc_pw_requeue_buffer(struct osc_pw_session *session, void *buffer_handle); + /* Stops the thread loop, joins it, and frees everything. Safe with NULL. */ void osc_pw_stop(struct osc_pw_session *session); diff --git a/electron/native/pipewire-capture/src/capture.rs b/electron/native/pipewire-capture/src/capture.rs index 4324f3b01..f6fe75dc3 100644 --- a/electron/native/pipewire-capture/src/capture.rs +++ b/electron/native/pipewire-capture/src/capture.rs @@ -244,7 +244,11 @@ impl Capture { ) -> Result<(Self, Selection), String> { let bitrate = bitrate.unwrap_or_else(|| default_bitrate(width, height, fps)); let mut rejected = Vec::new(); - let (encoder, importer) = match dmabuf { + // The dmabuf import can only feed VAAPI, so a user forcing `software` or + // `vulkan` must skip it — otherwise the documented `forced` workaround + // (VideoEncoder::open) would be silently ignored on the dmabuf path. + let use_dmabuf = matches!(forced, None | Some(Backend::Vaapi)); + let (encoder, importer) = match dmabuf.filter(|_| use_dmabuf) { Some(desc) => { // The importer maps the full stream (`desc`) and its VPP crops to // the committed record size (`width`/`height`): equal to the source @@ -366,7 +370,6 @@ impl Capture { // surface (the VPP crops a window to its committed rectangle) and hand it // to the encoder as-is — no swscale. See issue #507. if frame.dmabuf.is_some() { - use std::os::fd::AsRawFd; // The crop origin, clamped to stay inside the buffer — same rule as the // CPU path. Computed before the mutable importer borrow. For a monitor // this is (0, 0). @@ -380,7 +383,7 @@ impl Capture { .planes .iter() .map(|plane| crate::dmabuf_import::DmabufPlane { - fd: plane.fd.as_raw_fd(), + fd: plane.fd, offset: plane.offset, stride: plane.stride, }) @@ -633,6 +636,7 @@ mod tests { pts_ns: -1, crop: shim::CropRect { x: 0, y: 0, width, height }, has_crop: false, + dmabuf: None, } } @@ -677,7 +681,7 @@ mod tests { fn the_timeline_does_not_start_until_the_first_frame_is_staged() { let output = std::env::temp_dir().join("openscreen-capture-epoch.mp4"); let (mut capture, _) = - Capture::start(&output, 320, 240, 30, Some(1_000_000), Some(Backend::Software), Vec::new()) + Capture::start(&output, 320, 240, 30, Some(1_000_000), Some(Backend::Software), Vec::new(), None) .expect("start"); assert!(!capture.started()); // Nothing staged: advance must not write a frame of uninitialised memory. @@ -696,7 +700,7 @@ mod tests { // further arrivals, and the file must still fill with frames. let output = std::env::temp_dir().join("openscreen-capture-static.mp4"); let (mut capture, _) = - Capture::start(&output, 320, 240, 30, Some(1_000_000), Some(Backend::Software), Vec::new()) + Capture::start(&output, 320, 240, 30, Some(1_000_000), Some(Backend::Software), Vec::new(), None) .expect("start"); capture .stage(&frame(320, 240, shim::constants().video_format_bgrx)) @@ -721,7 +725,7 @@ mod tests { fn a_window_is_staged_from_its_crop_inside_a_larger_frame() { let output = std::env::temp_dir().join("openscreen-capture-crop.mp4"); let (mut capture, _) = - Capture::start(&output, 320, 240, 30, Some(1_000_000), Some(Backend::Software), Vec::new()) + Capture::start(&output, 320, 240, 30, Some(1_000_000), Some(Backend::Software), Vec::new(), None) .expect("start"); // A 1920x1080 stream carrying a 320x240 window at (100, 50). @@ -752,7 +756,7 @@ mod tests { fn a_crop_against_the_right_edge_is_not_rejected_as_truncated() { let output = std::env::temp_dir().join("openscreen-capture-edge.mp4"); let (mut capture, _) = - Capture::start(&output, 320, 240, 30, Some(1_000_000), Some(Backend::Software), Vec::new()) + Capture::start(&output, 320, 240, 30, Some(1_000_000), Some(Backend::Software), Vec::new(), None) .expect("start"); let staged = capture.stage(&cropped_frame( @@ -775,7 +779,7 @@ mod tests { fn a_shrunken_window_is_read_from_inside_the_frame() { let output = std::env::temp_dir().join("openscreen-capture-shrunk.mp4"); let (mut capture, _) = - Capture::start(&output, 320, 240, 30, Some(1_000_000), Some(Backend::Software), Vec::new()) + Capture::start(&output, 320, 240, 30, Some(1_000_000), Some(Backend::Software), Vec::new(), None) .expect("start"); // Origin so close to the edge that a 320x240 read from it would overrun. @@ -801,7 +805,7 @@ mod tests { let output = std::env::temp_dir().join("openscreen-capture-odd.mp4"); // 321x241 rounds to the 320x240 the encoder is opened at. let (mut capture, _) = - Capture::start(&output, 320, 240, 30, Some(1_000_000), Some(Backend::Software), Vec::new()) + Capture::start(&output, 320, 240, 30, Some(1_000_000), Some(Backend::Software), Vec::new(), None) .expect("start"); let frame = cropped_frame( @@ -820,7 +824,7 @@ mod tests { fn an_uncropped_frame_reports_no_divergence() { let output = std::env::temp_dir().join("openscreen-capture-nocrop.mp4"); let (mut capture, _) = - Capture::start(&output, 320, 240, 30, Some(1_000_000), Some(Backend::Software), Vec::new()) + Capture::start(&output, 320, 240, 30, Some(1_000_000), Some(Backend::Software), Vec::new(), None) .expect("start"); assert!(!capture.crop_diverged(&frame(320, 240, shim::constants().video_format_bgrx))); @@ -832,7 +836,7 @@ mod tests { fn paused_time_does_not_advance_the_timeline() { let output = std::env::temp_dir().join("openscreen-capture-pause.mp4"); let (mut capture, _) = - Capture::start(&output, 320, 240, 30, Some(1_000_000), Some(Backend::Software), Vec::new()) + Capture::start(&output, 320, 240, 30, Some(1_000_000), Some(Backend::Software), Vec::new(), None) .expect("start"); capture .stage(&frame(320, 240, shim::constants().video_format_bgrx)) @@ -872,6 +876,7 @@ mod tests { Some(1_000_000), Some(Backend::Software), vec![AudioSource { label: "system", ring: ring.clone(), gain: 1.0, bitrate: 128_000 }], + None, ) .expect("start"); @@ -908,6 +913,7 @@ mod tests { Some(1_000_000), Some(Backend::Software), vec![AudioSource { label: "system", ring: ring.clone(), gain: 1.0, bitrate: 128_000 }], + None, ) .expect("start"); capture @@ -950,6 +956,7 @@ mod tests { Some(1_000_000), Some(Backend::Software), vec![AudioSource { label: "microphone", ring, gain: 4.0, bitrate: 128_000 }], + None, ) .expect("start"); capture @@ -989,6 +996,7 @@ mod tests { AudioSource { label: "system", ring: system.clone(), gain: 1.0, bitrate: 128_000 }, AudioSource { label: "microphone", ring: mic.clone(), gain: 1.0, bitrate: 128_000 }, ], + None, ) .expect("start"); @@ -1038,6 +1046,7 @@ mod tests { AudioSource { label: "system", ring: system.clone(), gain: 1.0, bitrate: 128_000 }, AudioSource { label: "microphone", ring: dead, gain: 1.0, bitrate: 128_000 }, ], + None, ) .expect("start"); capture @@ -1061,7 +1070,7 @@ mod tests { fn catch_up_is_bounded_so_a_stall_cannot_block_stop() { let output = std::env::temp_dir().join("openscreen-capture-catchup.mp4"); let (mut capture, _) = - Capture::start(&output, 320, 240, 60, Some(1_000_000), Some(Backend::Software), Vec::new()) + Capture::start(&output, 320, 240, 60, Some(1_000_000), Some(Backend::Software), Vec::new(), None) .expect("start"); capture .stage(&frame(320, 240, shim::constants().video_format_bgrx)) diff --git a/electron/native/pipewire-capture/src/dmabuf_import.rs b/electron/native/pipewire-capture/src/dmabuf_import.rs index 1cc7669ee..71b8d9a6f 100644 --- a/electron/native/pipewire-capture/src/dmabuf_import.rs +++ b/electron/native/pipewire-capture/src/dmabuf_import.rs @@ -21,9 +21,12 @@ unsafe fn free_frame(frame: *mut ff::AVFrame) { ff::av_frame_free(&mut p); } -/// The AVBufferRef around the DRM descriptor owns nothing heap-allocated (the -/// descriptor is a stack local that outlives the map), so freeing it is a no-op. -unsafe extern "C" fn noop_buffer_free(_opaque: *mut std::ffi::c_void, _data: *mut u8) {} +/// Frees the heap-allocated `AVDRMFrameDescriptor` when its AVBufferRef drops — +/// which is after the mapped frame that retained the source (and thus this +/// buffer) is released. +unsafe extern "C" fn drm_descriptor_free(_opaque: *mut std::ffi::c_void, data: *mut u8) { + ff::av_free(data as *mut std::ffi::c_void); +} /// One dmabuf plane. `fd` is borrowed for the duration of [`DmabufImporter::import`] /// only — VAAPI dups it during surface creation, so the caller may close it after. @@ -272,11 +275,12 @@ impl DmabufImporter { (*scale_ctx).hw_device_ctx = ff::av_buffer_ref(self.va_device); let rc = ff::avfilter_link(self.buffersrc_ctx, 0, scale_ctx, 0); - if rc >= 0 { - ff::avfilter_link(scale_ctx, 0, self.buffersink_ctx, 0); + if rc < 0 { + return Err(format!("avfilter_link(in->vpp): {}", ff::err_to_string(rc))); } + let rc = ff::avfilter_link(scale_ctx, 0, self.buffersink_ctx, 0); if rc < 0 { - return Err(format!("avfilter_link: {}", ff::err_to_string(rc))); + return Err(format!("avfilter_link(vpp->out): {}", ff::err_to_string(rc))); } let rc = ff::avfilter_graph_config(self.graph, ptr::null_mut()); @@ -311,44 +315,58 @@ impl DmabufImporter { if frame.planes.is_empty() || frame.planes.len() > 4 { return Err(format!("dmabuf has {} planes", frame.planes.len())); } - // SAFETY: the descriptor outlives the map call it is passed to; every - // allocated frame is unref'd on the error paths and on success ownership - // of the NV12 frame passes to the caller. + // All planes must share one fd: we build a single DRM object from + // planes[0].fd and point every plane at it, so a buffer whose planes span + // multiple fds would make VAAPI read the wrong memory. Our RGB formats are + // single-plane; guard the assumption rather than rely on it. + if frame.planes.iter().any(|plane| plane.fd != frame.planes[0].fd) { + return Err("dmabuf planes span multiple fds, which this importer does not handle".to_owned()); + } + // SAFETY: every allocated frame/buffer is freed on the error paths and on + // success ownership of the NV12 frame passes to the caller. unsafe { - // Build the DRM PRIME descriptor. One object per unique fd; our RGB - // formats are a single object with a single layer and plane. - let mut desc: ff::AVDRMFrameDescriptor = std::mem::zeroed(); - desc.nb_objects = 1; - desc.objects[0].fd = frame.planes[0].fd; - desc.objects[0].size = 0; // recovered by the driver from the fd - desc.objects[0].format_modifier = frame.modifier; - desc.nb_layers = 1; - desc.layers[0].format = frame.drm_fourcc; - desc.layers[0].nb_planes = frame.planes.len() as i32; + // The DRM descriptor must outlive the mapped frame: av_hwframe_map + // retains `src` (ref-counted) until the mapping is released, so a + // stack descriptor would dangle once this function returns. Allocate + // it on the heap and free it from the AVBufferRef's own callback. + let desc = ff::av_mallocz(std::mem::size_of::()) + as *mut ff::AVDRMFrameDescriptor; + if desc.is_null() { + return Err("av_mallocz(drm descriptor) failed".to_owned()); + } + (*desc).nb_objects = 1; + (*desc).objects[0].fd = frame.planes[0].fd; + (*desc).objects[0].size = 0; // recovered by the driver from the fd + (*desc).objects[0].format_modifier = frame.modifier; + (*desc).nb_layers = 1; + (*desc).layers[0].format = frame.drm_fourcc; + (*desc).layers[0].nb_planes = frame.planes.len() as i32; for (i, plane) in frame.planes.iter().enumerate() { - desc.layers[0].planes[i].object_index = 0; - desc.layers[0].planes[i].offset = plane.offset as isize; - desc.layers[0].planes[i].pitch = plane.stride as isize; + (*desc).layers[0].planes[i].object_index = 0; + (*desc).layers[0].planes[i].offset = plane.offset as isize; + (*desc).layers[0].planes[i].pitch = plane.stride as isize; } let src = ff::av_frame_alloc(); if src.is_null() { + ff::av_free(desc as *mut std::ffi::c_void); return Err("av_frame_alloc(src) failed".to_owned()); } (*src).format = ff::AV_PIX_FMT_DRM_PRIME as i32; (*src).width = self.src_width; (*src).height = self.src_height; - // av_hwframe_map rejects a source frame that is not ref-counted, so - // wrap the descriptor in an AVBufferRef (freed as a no-op — `desc` - // lives on the stack until this function returns, past the map). + // av_hwframe_map needs a ref-counted source; wrap the heap descriptor + // in an AVBufferRef that frees it when the last reference drops (which + // is after the mapped frame that retains `src` is released). let buf = ff::av_buffer_create( - &mut desc as *mut _ as *mut u8, + desc as *mut u8, std::mem::size_of::(), - Some(noop_buffer_free), + Some(drm_descriptor_free), ptr::null_mut(), 0, ); if buf.is_null() { + ff::av_free(desc as *mut std::ffi::c_void); free_frame(src); return Err("av_buffer_create(drm descriptor) failed".to_owned()); } diff --git a/electron/native/pipewire-capture/src/main.rs b/electron/native/pipewire-capture/src/main.rs index 61aba8c8f..2d547ac7a 100644 --- a/electron/native/pipewire-capture/src/main.rs +++ b/electron/native/pipewire-capture/src/main.rs @@ -609,6 +609,18 @@ fn run( let mut exit_code = 0; loop { + // Return PipeWire buffers whose dmabuf imports completed last iteration + // (or that were superseded on the capture thread). This MUST run on this + // loop, not the PipeWire thread — `Session::requeue` takes the thread-loop + // lock, which would deadlock from inside the loop. The one-tick delay is + // harmless: the import has already copied the pixels, so the buffer is + // free, and the pool has other buffers in flight meanwhile. + if let (Some(session), Some(mailbox)) = (session.as_ref(), frames.as_ref()) { + for handle in mailbox.drain_requeue() { + session.requeue(handle); + } + } + match receiver.recv_timeout(config.tick) { Ok(Message::Stop) => break, diff --git a/electron/native/pipewire-capture/src/shim.rs b/electron/native/pipewire-capture/src/shim.rs index a37693b81..3a4024b23 100644 --- a/electron/native/pipewire-capture/src/shim.rs +++ b/electron/native/pipewire-capture/src/shim.rs @@ -57,8 +57,22 @@ pub struct RawFrame { pub plane_fd: [i32; 4], pub plane_offset: [i32; 4], pub plane_stride: [i32; 4], + /// The `struct pw_buffer *` this frame came from (opaque). Returned to + /// `osc_pw_requeue_buffer` once the dmabuf import has copied the pixels, if + /// `on_frame` took ownership of it. Null/unused on the CPU path. + pub buffer_handle: *mut c_void, } +/// A `struct pw_buffer *` we are holding out of PipeWire's queue until its dmabuf +/// content has been imported. Send so it can travel through the mailbox; the raw +/// pointer is only ever handed back to `osc_pw_requeue_buffer`, never dereferenced +/// on the Rust side. +#[derive(Debug, Clone, Copy)] +pub struct BufferHandle(pub *mut c_void); +// SAFETY: the pointer is an opaque token owned by libpipewire; Rust neither reads +// nor writes through it, only returns it to the shim's locked requeue. +unsafe impl Send for BufferHandle {} + #[repr(C)] #[derive(Debug, Clone, Copy)] pub struct RawFormat { @@ -74,7 +88,7 @@ struct RawCallbacks { user: *mut c_void, on_format: extern "C" fn(*mut c_void, *const RawFormat), on_cursor: extern "C" fn(*mut c_void, *const RawCursor), - on_frame: extern "C" fn(*mut c_void, *const RawFrame), + on_frame: extern "C" fn(*mut c_void, *const RawFrame) -> i32, on_buffer_info: extern "C" fn(*mut c_void, u32, u32, i32, u32, *const c_char), on_state: extern "C" fn(*mut c_void, *const c_char, *const c_char), } @@ -123,6 +137,7 @@ extern "C" { err_len: usize, ) -> *mut RawSession; fn osc_pw_stop(session: *mut RawSession); + fn osc_pw_requeue_buffer(session: *mut RawSession, buffer_handle: *mut c_void); } /// Where stream events go. Called on the PipeWire thread, so it must not block: @@ -198,22 +213,54 @@ pub struct Frame { pub dmabuf: Option, } -/// A tiled dmabuf handed up for GPU import. Owns duplicated plane fds so the -/// descriptor outlives the PipeWire buffer it came from. -#[derive(Debug)] +/// A tiled dmabuf handed up for GPU import. Holds the PipeWire buffer OUT of the +/// queue (via `buffer_handle`) so the plane fds AND their content stay valid until +/// the import copies the surface — dup'ing the fds alone would preserve the object +/// but not a content snapshot, letting the compositor overwrite a re-queued buffer +/// (CodeRabbit / issue #507). On drop the handle is pushed to `requeue`, which the +/// main loop drains and hands back to the shim's locked re-queue. pub struct DmabufDesc { pub width: i32, pub height: i32, pub drm_fourcc: u32, pub modifier: u64, - pub planes: Vec, + pub planes: Vec, + buffer_handle: BufferHandle, + requeue: std::sync::Arc>>, +} + +impl std::fmt::Debug for DmabufDesc { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("DmabufDesc") + .field("width", &self.width) + .field("height", &self.height) + .field("drm_fourcc", &self.drm_fourcc) + .field("modifier", &self.modifier) + .field("planes", &self.planes) + .finish_non_exhaustive() + } +} + +impl Drop for DmabufDesc { + fn drop(&mut self) { + // Return the held PipeWire buffer once the import that read it is done + // (which is why this runs at drop, after `Capture::stage`). Pushed to the + // queue rather than re-queued here because re-queue must run on the main + // loop, not the PipeWire thread that supersedes a frame — see + // FrameMailbox and osc_pw_requeue_buffer. + if !self.buffer_handle.0.is_null() { + if let Ok(mut queue) = self.requeue.lock() { + queue.push(self.buffer_handle); + } + } + } } -/// One dmabuf plane: an owned (dup'd) fd plus its layout. The fd is closed when -/// this drops. +/// One dmabuf plane: a BORROWED fd (owned by the still-held PipeWire buffer) plus +/// its layout. Valid until the buffer is re-queued. #[derive(Debug)] -pub struct DmabufPlaneOwned { - pub fd: std::os::fd::OwnedFd, +pub struct DmabufPlane { + pub fd: i32, pub offset: i32, pub stride: i32, } @@ -243,6 +290,11 @@ pub struct FrameMailbox { inner: std::sync::Mutex, received: std::sync::atomic::AtomicU64, dropped: std::sync::atomic::AtomicU64, + /// PipeWire buffers held for a dmabuf import, to be re-queued once their + /// `DmabufDesc` drops (import done, or frame superseded). Drained by the main + /// loop, which re-queues each through the shim's locked path. Shared into each + /// `DmabufDesc` so its Drop can push here from either thread. + requeue: std::sync::Arc>>, } #[derive(Debug, Default)] @@ -349,6 +401,21 @@ impl FrameMailbox { inner.spare = Some(pixels); } + /// A clone of the held-buffer re-queue queue, for a `DmabufDesc` to push its + /// PipeWire buffer to when it drops. + fn requeue_queue(&self) -> std::sync::Arc>> { + self.requeue.clone() + } + + /// Takes the PipeWire buffers whose dmabuf imports have completed (or were + /// superseded), for the main loop to re-queue through the shim. + pub fn drain_requeue(&self) -> Vec { + match self.requeue.lock() { + Ok(mut queue) => std::mem::take(&mut *queue), + Err(_) => Vec::new(), + } + } + /// Frames the compositor delivered. pub fn received(&self) -> u64 { self.received.load(std::sync::atomic::Ordering::Relaxed) @@ -865,6 +932,16 @@ impl Session { Ok(Self { raw, _state: state }) } + + /// Re-queues a PipeWire buffer a dmabuf frame took ownership of, once its + /// import has copied the pixels. Call from the main loop (NOT the PipeWire + /// thread) — the shim takes the thread-loop lock. Drain the mailbox's + /// `drain_requeue` for the handles. + pub fn requeue(&self, handle: BufferHandle) { + // SAFETY: `raw` is a live session for the lifetime of `self`; the handle + // is an opaque pw_buffer token the shim validates and only re-queues. + unsafe { osc_pw_requeue_buffer(self.raw, handle.0) }; + } } impl Drop for Session { @@ -911,78 +988,73 @@ extern "C" fn on_format(user: *mut c_void, format: *const RawFormat) { }); } -extern "C" fn on_frame(user: *mut c_void, frame: *const RawFrame) { - with_state(user, |state| { - let Some(mailbox) = state.frames.as_ref() else { - return; - }; - if frame.is_null() { - return; +/// Returns 1 when we TAKE OWNERSHIP of the PipeWire buffer — a tiled dmabuf held +/// out of the queue until the main loop imports it — so the shim must not re-queue +/// it. 0 otherwise (the CPU path, or any frame we decline), which re-queues as +/// before. +extern "C" fn on_frame(user: *mut c_void, frame: *const RawFrame) -> i32 { + if user.is_null() || frame.is_null() { + return 0; + } + // SAFETY: `user` is the CallbackState pointer given to osc_pw_start, valid for + // the session's lifetime; `frame` is valid for the callback's duration. + let state = unsafe { &*(user as *const CallbackState) }; + let Some(mailbox) = state.frames.as_ref() else { + return 0; + }; + let frame = unsafe { &*frame }; + + // Tiled dmabuf: no pixels to copy. Take the PipeWire buffer (hold it out of + // the queue) so the plane fds AND their content stay valid until the main-loop + // import copies the surface; the buffer is re-queued when the DmabufDesc drops. + if frame.is_dmabuf != 0 { + let n = frame.n_planes.clamp(0, 4) as usize; + if n == 0 { + return 0; } - // SAFETY: non-NULL for the duration of the callback, by contract. - let frame = unsafe { &*frame }; - - // Tiled dmabuf: no pixels to copy. Duplicate the plane fds (cheap, and it - // keeps the buffer's content reachable after the PW buffer re-queues) and - // hand the descriptor to the main loop for a GPU import. - if frame.is_dmabuf != 0 { - use std::os::fd::{AsRawFd, BorrowedFd}; - let n = frame.n_planes.clamp(0, 4) as usize; - if n == 0 { - return; + let mut planes = Vec::with_capacity(n); + for i in 0..n { + let fd = frame.plane_fd[i]; + if fd < 0 { + return 0; } - let mut planes = Vec::with_capacity(n); - for i in 0..n { - let raw = frame.plane_fd[i]; - if raw < 0 { - return; - } - // SAFETY: `raw` is valid for the callback's duration; try_clone - // dups it (F_DUPFD_CLOEXEC) into an fd we own. - let borrowed = unsafe { BorrowedFd::borrow_raw(raw) }; - let Ok(owned) = borrowed.try_clone_to_owned() else { - return; - }; - debug_assert!(owned.as_raw_fd() >= 0); - planes.push(DmabufPlaneOwned { - fd: owned, - offset: frame.plane_offset[i], - stride: frame.plane_stride[i], - }); - } - mailbox.put_dmabuf( - DmabufDesc { - width: frame.width, - height: frame.height, - drm_fourcc: frame.drm_fourcc, - modifier: frame.modifier, - planes, - }, - frame, - ); - (state.sink)(StreamEvent::FrameReady); - return; - } - - if frame.data.is_null() || frame.stride <= 0 || frame.height <= 0 { - return; - } - // Copy only the rows, not the whole mapping. `size` can include trailing - // slack the compositor allocated, and re-checking the product here means - // the slice below cannot outrun the region the C side validated. - let Some(rows) = (frame.stride as usize).checked_mul(frame.height as usize) else { - return; - }; - if rows > frame.size { - return; + planes.push(DmabufPlane { fd, offset: frame.plane_offset[i], stride: frame.plane_stride[i] }); } - // SAFETY: the shim clamped `size` against the mapping's `maxsize` before - // the callback, `rows <= size` was just checked, and the mapping stays - // live until this returns. - let pixels = unsafe { std::slice::from_raw_parts(frame.data, rows) }; - mailbox.put(pixels, frame); + mailbox.put_dmabuf( + DmabufDesc { + width: frame.width, + height: frame.height, + drm_fourcc: frame.drm_fourcc, + modifier: frame.modifier, + planes, + buffer_handle: BufferHandle(frame.buffer_handle), + requeue: mailbox.requeue_queue(), + }, + frame, + ); (state.sink)(StreamEvent::FrameReady); - }); + return 1; + } + + if frame.data.is_null() || frame.stride <= 0 || frame.height <= 0 { + return 0; + } + // Copy only the rows, not the whole mapping. `size` can include trailing + // slack the compositor allocated, and re-checking the product here means + // the slice below cannot outrun the region the C side validated. + let Some(rows) = (frame.stride as usize).checked_mul(frame.height as usize) else { + return 0; + }; + if rows > frame.size { + return 0; + } + // SAFETY: the shim clamped `size` against the mapping's `maxsize` before + // the callback, `rows <= size` was just checked, and the mapping stays + // live until this returns. + let pixels = unsafe { std::slice::from_raw_parts(frame.data, rows) }; + mailbox.put(pixels, frame); + (state.sink)(StreamEvent::FrameReady); + 0 } extern "C" fn on_buffer_info( @@ -1207,6 +1279,8 @@ mod tests { // Cursor-only: this test is about negotiation reaching `streaming` // and about which metadata survives, neither of which needs pixels. None, + // Cursor-only, so dmabuf preference is irrelevant. + false, ) .expect("stream must connect"); From 2fd7528e27f6bc932a8453e82bcbaa58b697b056 Mon Sep 17 00:00:00 2001 From: Benjamin Freeman Date: Thu, 27 Aug 2026 01:26:21 +0200 Subject: [PATCH 8/9] fix(capture): invalidate held dmabuf buffers on remove (CodeRabbit #508) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the buffer-hold in the previous commit. A dmabuf frame the consumer holds for a GPU import keeps a struct pw_buffer *, re-queued later. But a stream renegotiation destroys the buffer set (remove_buffer fires), so that pointer can dangle — osc_pw_requeue_buffer would then queue freed storage. Track every pw_buffer the stream owns (osc_on_add_buffer) and clear it on osc_on_remove_buffer; osc_pw_requeue_buffer now skips a handle that is no longer live. The table is only touched on the PipeWire thread and, in requeue, under the thread-loop lock that pauses it, so no extra locking is needed. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../native/pipewire-capture/csrc/pw_shim.c | 60 ++++++++++++++++++- 1 file changed, 59 insertions(+), 1 deletion(-) diff --git a/electron/native/pipewire-capture/csrc/pw_shim.c b/electron/native/pipewire-capture/csrc/pw_shim.c index 95915dc98..5740b532a 100644 --- a/electron/native/pipewire-capture/csrc/pw_shim.c +++ b/electron/native/pipewire-capture/csrc/pw_shim.c @@ -114,6 +114,11 @@ static uint32_t osc_spa_format_to_drm_fourcc(uint32_t spa_format) */ #define OSC_MAX_DMABUF_MAPS 32 +/* The negotiated buffer pool is at most 16 (SPA_PARAM_BUFFERS below), plus a + * transient overlap while a renegotiation swaps the set. 32 covers it with room + * to spare, and a full table only means a held buffer is treated as stale. */ +#define OSC_MAX_LIVE_BUFFERS 32 + struct osc_dmabuf_map { int fd; void *ptr; @@ -225,6 +230,12 @@ struct osc_pw_session { * The bracket has to span the on_frame callback, not just osc_read_frame, * because the callback is where the pixels are actually read. */ int dmabuf_sync_fd; + /* Every pw_buffer the stream currently owns, added in osc_on_add_buffer and + * cleared in osc_on_remove_buffer. A dmabuf frame the consumer holds for a + * GPU import (issue #507) keeps the pw_buffer pointer, but a renegotiation + * destroys the buffer set — so osc_pw_requeue_buffer must check the handle is + * still here before touching it, or it would queue freed storage. */ + struct pw_buffer *live_buffers[OSC_MAX_LIVE_BUFFERS]; }; struct osc_pw_audio_api osc_audio_api; @@ -836,6 +847,42 @@ static void osc_dmabuf_sync(int fd, int start) } } +/* The live-buffer table (session->live_buffers) is only touched on the PipeWire + * thread (add/remove_buffer) and, in osc_pw_requeue_buffer, under the thread-loop + * lock which pauses that thread — so these need no locking of their own. */ +static void osc_track_live_buffer(struct osc_pw_session *session, struct pw_buffer *pw_buf) +{ + size_t i; + for (i = 0; i < OSC_MAX_LIVE_BUFFERS; i++) { + if (session->live_buffers[i] == NULL) { + session->live_buffers[i] = pw_buf; + return; + } + } +} + +static void osc_forget_live_buffer(struct osc_pw_session *session, struct pw_buffer *pw_buf) +{ + size_t i; + for (i = 0; i < OSC_MAX_LIVE_BUFFERS; i++) { + if (session->live_buffers[i] == pw_buf) { + session->live_buffers[i] = NULL; + return; + } + } +} + +static int osc_buffer_is_live(struct osc_pw_session *session, struct pw_buffer *pw_buf) +{ + size_t i; + for (i = 0; i < OSC_MAX_LIVE_BUFFERS; i++) { + if (session->live_buffers[i] == pw_buf) { + return 1; + } + } + return 0; +} + static void osc_on_add_buffer(void *userdata, struct pw_buffer *pw_buf) { struct osc_pw_session *session = userdata; @@ -847,6 +894,9 @@ static void osc_on_add_buffer(void *userdata, struct pw_buffer *pw_buf) if (pw_buf == NULL || pw_buf->buffer == NULL || pw_buf->buffer->n_datas < 1) { return; } + /* Record the buffer as live before anything else, so a handle the consumer + * holds can be validated against destruction in osc_pw_requeue_buffer. */ + osc_track_live_buffer(session, pw_buf); data = &pw_buf->buffer->datas[0]; if (data->type != SPA_DATA_DmaBuf) { return; @@ -895,6 +945,9 @@ static void osc_on_remove_buffer(void *userdata, struct pw_buffer *pw_buf) if (pw_buf == NULL || pw_buf->buffer == NULL || pw_buf->buffer->n_datas < 1) { return; } + /* The buffer is being destroyed: a consumer still holding it for a GPU import + * must not re-queue it. Forgetting it here makes osc_pw_requeue_buffer skip it. */ + osc_forget_live_buffer(session, pw_buf); data = &pw_buf->buffer->datas[0]; for (i = 0; i < OSC_MAX_DMABUF_MAPS; i++) { if (session->dmabuf_maps[i].ptr == NULL || @@ -1351,7 +1404,12 @@ void osc_pw_requeue_buffer(struct osc_pw_session *session, void *buffer_handle) return; } api.thread_loop_lock(session->loop); - api.stream_queue_buffer(session->stream, (struct pw_buffer *)buffer_handle); + /* Under the lock the PipeWire thread is paused, so the live-buffer table is + * stable: only re-queue a handle a renegotiation has not destroyed. A stale + * handle is simply dropped — PipeWire already freed that buffer. */ + if (osc_buffer_is_live(session, (struct pw_buffer *)buffer_handle)) { + api.stream_queue_buffer(session->stream, (struct pw_buffer *)buffer_handle); + } api.thread_loop_unlock(session->loop); } From fef27cf4761283163026c3947b27a2da80446475 Mon Sep 17 00:00:00 2001 From: Benjamin Freeman Date: Thu, 27 Aug 2026 01:37:49 +0200 Subject: [PATCH 9/9] fix(capture): generation-tag held dmabuf buffers against slot reuse (CodeRabbit #508) The previous commit rejected re-queuing a destroyed buffer by pointer equality, but PipeWire reuses buffer-wrapper slots: a renegotiation can register a NEW buffer at the SAME address as one the consumer still holds, so the stale handle would pass the live check and re-queue the wrong buffer (ABA). Stamp each registration with a unique generation (session->next_generation, never repeated); the frame carries it alongside buffer_handle, and osc_pw_requeue_buffer re-queues only when BOTH the pointer and its generation still match. A handle from a prior registration is dropped. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../native/pipewire-capture/csrc/pw_shim.c | 51 ++++++++++++++----- .../native/pipewire-capture/csrc/pw_shim.h | 7 ++- electron/native/pipewire-capture/src/shim.rs | 30 ++++++++--- 3 files changed, 66 insertions(+), 22 deletions(-) diff --git a/electron/native/pipewire-capture/csrc/pw_shim.c b/electron/native/pipewire-capture/csrc/pw_shim.c index 5740b532a..d0ff4ce64 100644 --- a/electron/native/pipewire-capture/csrc/pw_shim.c +++ b/electron/native/pipewire-capture/csrc/pw_shim.c @@ -234,8 +234,16 @@ struct osc_pw_session { * cleared in osc_on_remove_buffer. A dmabuf frame the consumer holds for a * GPU import (issue #507) keeps the pw_buffer pointer, but a renegotiation * destroys the buffer set — so osc_pw_requeue_buffer must check the handle is - * still here before touching it, or it would queue freed storage. */ + * still here before touching it, or it would queue freed storage. + * + * Pointer equality alone is not enough: PipeWire reuses these wrapper slots, + * so a renegotiation can register a NEW buffer at the SAME address as one the + * consumer still holds. Each registration therefore carries a unique + * `generation`, and a retained handle is only re-queued when BOTH the pointer + * and its generation match — an ABA guard. `next_generation` never repeats. */ struct pw_buffer *live_buffers[OSC_MAX_LIVE_BUFFERS]; + uint64_t live_generations[OSC_MAX_LIVE_BUFFERS]; + uint64_t next_generation; }; struct osc_pw_audio_api osc_audio_api; @@ -847,18 +855,25 @@ static void osc_dmabuf_sync(int fd, int start) } } -/* The live-buffer table (session->live_buffers) is only touched on the PipeWire - * thread (add/remove_buffer) and, in osc_pw_requeue_buffer, under the thread-loop - * lock which pauses that thread — so these need no locking of their own. */ -static void osc_track_live_buffer(struct osc_pw_session *session, struct pw_buffer *pw_buf) +/* The live-buffer table is only touched on the PipeWire thread (add/remove_buffer) + * and, in osc_pw_requeue_buffer, under the thread-loop lock which pauses that + * thread — so these need no locking of their own. */ + +/* Registers `pw_buf` and returns the unique generation stamped on it, which the + * frame carries so a later re-queue can prove it means THIS registration and not + * a newer buffer reusing the same slot. 0 is never a valid generation. */ +static uint64_t osc_track_live_buffer(struct osc_pw_session *session, struct pw_buffer *pw_buf) { size_t i; + uint64_t generation = ++session->next_generation; for (i = 0; i < OSC_MAX_LIVE_BUFFERS; i++) { if (session->live_buffers[i] == NULL) { session->live_buffers[i] = pw_buf; - return; + session->live_generations[i] = generation; + return generation; } } + return generation; } static void osc_forget_live_buffer(struct osc_pw_session *session, struct pw_buffer *pw_buf) @@ -867,17 +882,20 @@ static void osc_forget_live_buffer(struct osc_pw_session *session, struct pw_buf for (i = 0; i < OSC_MAX_LIVE_BUFFERS; i++) { if (session->live_buffers[i] == pw_buf) { session->live_buffers[i] = NULL; + session->live_generations[i] = 0; return; } } } -static int osc_buffer_is_live(struct osc_pw_session *session, struct pw_buffer *pw_buf) +/* The generation currently registered for `pw_buf`, or 0 if it is not tracked. */ +static uint64_t osc_live_buffer_generation(struct osc_pw_session *session, + struct pw_buffer *pw_buf) { size_t i; for (i = 0; i < OSC_MAX_LIVE_BUFFERS; i++) { if (session->live_buffers[i] == pw_buf) { - return 1; + return session->live_generations[i]; } } return 0; @@ -1341,8 +1359,10 @@ static int osc_inspect_buffer(struct osc_pw_session *session, struct pw_buffer * session->dmabuf_sync_fd = -1; if (osc_read_frame(session, buffer, &frame)) { /* The callback needs the pw_buffer to hand back to osc_pw_requeue_buffer - * if it takes ownership of a dmabuf frame. */ + * if it takes ownership of a dmabuf frame, plus its generation so the + * re-queue can reject a stale handle after a renegotiation. */ frame.buffer_handle = pw_buf; + frame.buffer_generation = osc_live_buffer_generation(session, pw_buf); if (session->callbacks.on_frame(session->callbacks.user, &frame)) { /* Taken: leave it un-queued; the consumer will re-queue it once the * import has copied the pixels. No SYNC bracket is open on this @@ -1398,16 +1418,21 @@ static void osc_on_process(void *userdata) } /* See the header. Locks the thread loop so a foreign thread can queue safely. */ -void osc_pw_requeue_buffer(struct osc_pw_session *session, void *buffer_handle) +void osc_pw_requeue_buffer(struct osc_pw_session *session, void *buffer_handle, + uint64_t buffer_generation) { if (session == NULL || buffer_handle == NULL || session->stream == NULL) { return; } api.thread_loop_lock(session->loop); /* Under the lock the PipeWire thread is paused, so the live-buffer table is - * stable: only re-queue a handle a renegotiation has not destroyed. A stale - * handle is simply dropped — PipeWire already freed that buffer. */ - if (osc_buffer_is_live(session, (struct pw_buffer *)buffer_handle)) { + * stable. Re-queue only when the SAME registration is still live: matching + * the generation as well as the pointer rejects both a destroyed buffer and a + * newer one PipeWire placed in the same slot after a renegotiation. A stale + * handle is simply dropped — PipeWire already owns or freed that buffer. */ + if (buffer_generation != 0 && + osc_live_buffer_generation(session, (struct pw_buffer *)buffer_handle) == + buffer_generation) { api.stream_queue_buffer(session->stream, (struct pw_buffer *)buffer_handle); } api.thread_loop_unlock(session->loop); diff --git a/electron/native/pipewire-capture/csrc/pw_shim.h b/electron/native/pipewire-capture/csrc/pw_shim.h index 1f013e426..23df69417 100644 --- a/electron/native/pipewire-capture/csrc/pw_shim.h +++ b/electron/native/pipewire-capture/csrc/pw_shim.h @@ -121,6 +121,10 @@ struct osc_pw_frame { * Passed back to osc_pw_requeue_buffer once the import is done. Only set (and * only meaningful) for a dmabuf frame the consumer intends to take. */ void *buffer_handle; + /* The registration generation of `buffer_handle`. Passed back alongside it so + * a re-queue can tell this buffer from a later one PipeWire put in the same + * slot after a renegotiation. */ + uint64_t buffer_generation; }; /* The negotiated video format. Reported once, from param_changed. */ @@ -249,7 +253,8 @@ struct osc_pw_session *osc_pw_start(int fd, uint32_t node_id, int want_video, * PipeWire thread itself (that would deadlock). The consumer requeues from its * own loop, which is a different thread. NULL session or handle is a no-op. */ -void osc_pw_requeue_buffer(struct osc_pw_session *session, void *buffer_handle); +void osc_pw_requeue_buffer(struct osc_pw_session *session, void *buffer_handle, + uint64_t buffer_generation); /* Stops the thread loop, joins it, and frees everything. Safe with NULL. */ void osc_pw_stop(struct osc_pw_session *session); diff --git a/electron/native/pipewire-capture/src/shim.rs b/electron/native/pipewire-capture/src/shim.rs index 3a4024b23..08bc20cf7 100644 --- a/electron/native/pipewire-capture/src/shim.rs +++ b/electron/native/pipewire-capture/src/shim.rs @@ -61,14 +61,21 @@ pub struct RawFrame { /// `osc_pw_requeue_buffer` once the dmabuf import has copied the pixels, if /// `on_frame` took ownership of it. Null/unused on the CPU path. pub buffer_handle: *mut c_void, + /// Registration generation of `buffer_handle`, handed back with it so the + /// re-queue can reject a stale pointer a renegotiation reused (see the C side). + pub buffer_generation: u64, } /// A `struct pw_buffer *` we are holding out of PipeWire's queue until its dmabuf -/// content has been imported. Send so it can travel through the mailbox; the raw -/// pointer is only ever handed back to `osc_pw_requeue_buffer`, never dereferenced -/// on the Rust side. +/// content has been imported, tagged with its registration `generation` so the +/// re-queue can tell it from a newer buffer reusing the same slot. Send so it can +/// travel through the mailbox; the pointer is only ever handed back to +/// `osc_pw_requeue_buffer`, never dereferenced on the Rust side. #[derive(Debug, Clone, Copy)] -pub struct BufferHandle(pub *mut c_void); +pub struct BufferHandle { + pub ptr: *mut c_void, + pub generation: u64, +} // SAFETY: the pointer is an opaque token owned by libpipewire; Rust neither reads // nor writes through it, only returns it to the shim's locked requeue. unsafe impl Send for BufferHandle {} @@ -137,7 +144,11 @@ extern "C" { err_len: usize, ) -> *mut RawSession; fn osc_pw_stop(session: *mut RawSession); - fn osc_pw_requeue_buffer(session: *mut RawSession, buffer_handle: *mut c_void); + fn osc_pw_requeue_buffer( + session: *mut RawSession, + buffer_handle: *mut c_void, + buffer_generation: u64, + ); } /// Where stream events go. Called on the PipeWire thread, so it must not block: @@ -248,7 +259,7 @@ impl Drop for DmabufDesc { // queue rather than re-queued here because re-queue must run on the main // loop, not the PipeWire thread that supersedes a frame — see // FrameMailbox and osc_pw_requeue_buffer. - if !self.buffer_handle.0.is_null() { + if !self.buffer_handle.ptr.is_null() { if let Ok(mut queue) = self.requeue.lock() { queue.push(self.buffer_handle); } @@ -940,7 +951,7 @@ impl Session { pub fn requeue(&self, handle: BufferHandle) { // SAFETY: `raw` is a live session for the lifetime of `self`; the handle // is an opaque pw_buffer token the shim validates and only re-queues. - unsafe { osc_pw_requeue_buffer(self.raw, handle.0) }; + unsafe { osc_pw_requeue_buffer(self.raw, handle.ptr, handle.generation) }; } } @@ -1027,7 +1038,10 @@ extern "C" fn on_frame(user: *mut c_void, frame: *const RawFrame) -> i32 { drm_fourcc: frame.drm_fourcc, modifier: frame.modifier, planes, - buffer_handle: BufferHandle(frame.buffer_handle), + buffer_handle: BufferHandle { + ptr: frame.buffer_handle, + generation: frame.buffer_generation, + }, requeue: mailbox.requeue_queue(), }, frame,