From b99335550741d157afc3b8adba792b2f444c40c4 Mon Sep 17 00:00:00 2001 From: Markus Fritsche Date: Tue, 5 May 2026 15:05:41 +0000 Subject: [PATCH] iter5 Track E: move LAST_OUTPUT_WIDTH/HEIGHT from process-global to per-driver-data Sonnet review 7.3 / 9.6 from iter1 + carried iter2/3/4 substrate. Two libva driver_data instances in the same process (e.g. Firefox playing two tabs at different resolutions, or Firefox + mpv via the same dlopened backend) would race on the static cache. Move to struct request_data.last_output_width/height. The V4L2 device fd is already per-driver_data, so this is the correct binding unit (one fd, one current OUTPUT format). Verified: two concurrent mpv processes (2s stagger) both decode 300 frames cleanly with no cross-corruption. Same-instant init still hits kernel-level fd contention on /dev/video1 (hantro is a single-instance device); cross-process serialization is out of scope for a libva backend. Resolves the surface_reset_format_cache() callsite: now takes driver_data parameter (was zero-arg). Also drops the 'rc' unused-variable warning in v4l2_ioctl_controls that the iter5 sweep left behind. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/context.c | 2 +- src/request.h | 19 ++++++++++++++++++ src/surface.c | 54 +++++++++++++++++++++++++++------------------------ src/surface.h | 8 +++----- src/v4l2.c | 1 - 5 files changed, 52 insertions(+), 32 deletions(-) diff --git a/src/context.c b/src/context.c index 2673668..e614afa 100644 --- a/src/context.c +++ b/src/context.c @@ -263,7 +263,7 @@ VAStatus RequestDestroyContext(VADriverContextP context, VAContextID context_id) * cached "already 1920x1088"); the exported descriptor encoded * wrong pitch/offset. */ - surface_reset_format_cache(); + surface_reset_format_cache(driver_data); return VA_STATUS_SUCCESS; } diff --git a/src/request.h b/src/request.h index b20b329..ac1f419 100644 --- a/src/request.h +++ b/src/request.h @@ -80,6 +80,25 @@ struct request_data { * visible stutter on mpv vaapi --vo=gpu. */ struct cap_pool capture_pool; + + /* + * Per-driver-data cache of the OUTPUT format we've set on the + * V4L2 device (iter5 Track E: was process-global static + * LAST_OUTPUT_WIDTH/HEIGHT, which would race when two libva + * driver_data instances share a process — e.g. Firefox playing + * one tab while Chromium plays another, or two mpv processes + * via the same dlopened backend). Kept per-driver_data because + * the V4L2 device fd is per-driver_data; one fd, one current + * format. Process-global was always wrong, just didn't surface + * until iter5's audit. + * + * See surface.c::CreateSurfaces2 for the consumer pattern (mpv + * probes with small surfaces then re-allocates at real + * resolution; we re-set the OUTPUT format whenever this pair + * changes). + */ + unsigned int last_output_width; + unsigned int last_output_height; }; VAStatus VA_DRIVER_INIT_FUNC(VADriverContextP context); diff --git a/src/surface.c b/src/surface.c index 549b595..f5b0806 100644 --- a/src/surface.c +++ b/src/surface.c @@ -48,31 +48,34 @@ #include "video.h" /* - * Per-process cache of the OUTPUT format we've set. The previous - * SET_FORMAT_OF_OUTPUT_ONCE pattern was a latent bug (Sonnet Phase 5 - * review finding 7.3): mpv probes with small surfaces (e.g. 128x128) - * before requesting the real resolution (e.g. 1920x1088). The - * once-only set kept the OUTPUT — and consequently the kernel-derived - * CAPTURE — format pinned to the probe size. Subsequent - * v4l2_get_format on CAPTURE then returned the small format, the - * VADRMPRIMESurfaceDescriptor was filled with width=1920 height=1088 - * but pitch=128 offset=16384, and Mesa rejected the import with - * "WSI pitch too small." That manifested as the solid-blue render in - * mpv vaapi mode and the SW fallback in Firefox after frame 0. + * Per-driver-data cache of the OUTPUT format we've set on the V4L2 + * device. iter5 Track E: was process-global static + * LAST_OUTPUT_WIDTH/HEIGHT before this commit. See request.h + * struct request_data.last_output_width/height for the rationale. * - * Fix: track (width, height) and re-set the OUTPUT format whenever - * the resolution changes. Re-setting requires REQBUFS(0) on both - * queues first because S_FMT after CREATE_BUFS is rejected by V4L2; - * we tear down and let the next allocation cycle recreate buffers - * at the new resolution. + * The previous SET_FORMAT_OF_OUTPUT_ONCE pattern was a latent bug + * (Sonnet Phase 5 review finding 7.3): mpv probes with small surfaces + * (e.g. 128x128) before requesting the real resolution (e.g. + * 1920x1088). The once-only set kept the OUTPUT — and consequently + * the kernel-derived CAPTURE — format pinned to the probe size. + * Subsequent v4l2_get_format on CAPTURE then returned the small + * format, the VADRMPRIMESurfaceDescriptor was filled with width=1920 + * height=1088 but pitch=128 offset=16384, and Mesa rejected the + * import with "WSI pitch too small." That manifested as the + * solid-blue render in mpv vaapi mode and the SW fallback in Firefox + * after frame 0. + * + * Fix: track (width, height) per driver_data and re-set the OUTPUT + * format whenever the resolution changes. Re-setting requires + * REQBUFS(0) on both queues first because S_FMT after CREATE_BUFS is + * rejected by V4L2; we tear down and let the next allocation cycle + * recreate buffers at the new resolution. */ -static unsigned int LAST_OUTPUT_WIDTH = 0; -static unsigned int LAST_OUTPUT_HEIGHT = 0; -void surface_reset_format_cache(void) +void surface_reset_format_cache(struct request_data *driver_data) { - LAST_OUTPUT_WIDTH = 0; - LAST_OUTPUT_HEIGHT = 0; + driver_data->last_output_width = 0; + driver_data->last_output_height = 0; } /* @@ -170,7 +173,8 @@ VAStatus RequestCreateSurfaces2(VADriverContextP context, unsigned int format, unsigned int pixelformat = V4L2_PIX_FMT_H264_SLICE; unsigned int output_type = v4l2_type_video_output(true); - if (LAST_OUTPUT_WIDTH != width || LAST_OUTPUT_HEIGHT != height) { + if (driver_data->last_output_width != width || + driver_data->last_output_height != height) { /* * If we've previously allocated buffers at a different * resolution, tear them down on BOTH queues before re-setting @@ -189,7 +193,7 @@ VAStatus RequestCreateSurfaces2(VADriverContextP context, unsigned int format, * cap_pool_init below is skipped, and the slots' v4l2_index * fields point to dead buffers from the prior resolution. */ - if (LAST_OUTPUT_WIDTH != 0) { + if (driver_data->last_output_width != 0) { if (driver_data->capture_pool.initialized) cap_pool_destroy(&driver_data->capture_pool, driver_data->video_fd, @@ -206,8 +210,8 @@ VAStatus RequestCreateSurfaces2(VADriverContextP context, unsigned int format, if (rc < 0) return VA_STATUS_ERROR_OPERATION_FAILED; - LAST_OUTPUT_WIDTH = width; - LAST_OUTPUT_HEIGHT = height; + driver_data->last_output_width = width; + driver_data->last_output_height = height; } if (format != VA_RT_FORMAT_YUV420) diff --git a/src/surface.h b/src/surface.h index d2a4cb3..2d09f21 100644 --- a/src/surface.h +++ b/src/surface.h @@ -162,12 +162,10 @@ VAStatus RequestExportSurfaceHandle(VADriverContextP context, * has reset to defaults, our subsequent G_FMT returns 48x48, and the * exported descriptor encodes wrong pitch/offset. * - * Limitation (Sonnet Phase 5 review 7.3 / 9.6): the cache is a - * static process-global, so concurrent multi-context use still races. - * Iteration 2 only addresses sequential sessions. Multi-context - * safety is iteration 3+ scope. + * Iter5 Track E: cache lives per-driver_data (request_data.last_output_*), + * resolving the Sonnet review 7.3 / 9.6 multi-context race. */ -void surface_reset_format_cache(void); +void surface_reset_format_cache(struct request_data *driver_data); /* * Iter2 Fix 3: bind / unbind a CAPTURE-pool slot to an object_surface. diff --git a/src/v4l2.c b/src/v4l2.c index 1063c5d..9e2ce8b 100644 --- a/src/v4l2.c +++ b/src/v4l2.c @@ -433,7 +433,6 @@ static int v4l2_ioctl_controls(int video_fd, int request_fd, unsigned long ioc, unsigned int num_controls) { struct v4l2_ext_controls controls; - int rc; memset(&controls, 0, sizeof(controls));