marfrit/libva-v4l2-request-fourier
1
0
Fork 1
Code Issues 2 Pull Requests Actions Packages Projects Releases Wiki Activity

iter7: A+B+C — slot-leak fix, cap_pool harness, msync verify harness

Browse Source 
Closes three internal carry items in one fork commit. iter6 deferred
these as TODOs; iter7 lands the implementations + supporting tests.

# Track B — slot-leak error recovery (src/)

iter6 documented the RequestSyncSurface error paths as a "bounded
leak we accept" — slots stayed busy=true after REINIT/DQBUF failures
until RequestTerminate ran. With pool=16 and rare errors this was
acceptable, but a sustained-error scenario could starve the pool.

Adds request_pool_force_release(pool, index) which:
1. Tries media_request_reinit on the slot's fd (cheap path)
2. Falls back to close + media_request_alloc (recovery)
3. Leaves the slot dead-busy if even alloc fails (other slots
   unaffected, pool capacity reduced by 1 until destroy)

Wires it into surface.c RequestSyncSurface error paths only for
errors before the OUTPUT-DQBUF attempt. After OUTPUT-DQBUF failure
the V4L2 buffer is in indeterminate kernel state, so a separate
error label (`error_buffer_indeterminate`) leaves the slot
dead-busy — reusing the slot would QBUF on a kernel-still-held
buffer and EINVAL.

Phase 5 sonnet review caught this discriminator subtlety pre-commit.

Files: request_pool.{h,c}, surface.c.

# Track C — cap_pool race synthetic harness (tests/)

iter5 sonnet C4 / iter6 candidate A: cap_pool resolution-change
race was organically exercised by YT's quality renegotiations
(iter6 close, 4 cap_pool_init events clean) but had no
deterministic regression test.

tests/cap_pool_probe_pattern.c — ~170-line C program: opens
libva display, vaCreateConfig, vaCreateSurfaces(small) +
vaCreateContext (triggers OUTPUT pool init at small resolution),
dispose, vaCreateSurfaces(big) + vaCreateContext (forces S_FMT
on the new resolution against an in-use OUTPUT pool — the actual
race-hitting path).

Phase 5 sonnet flagged that without vaCreateContext the test
would pass trivially (OUTPUT pool never init'd, REQBUFS(0) on
empty queue is a no-op). Fixed before commit.

tests/run_cap_pool_probe.sh — runner; greps driver stderr for
REQBUFS / EBUSY / "Unable to set format" race indicators.

# Track A — msync pixel-correctness verify harness (tests/)

iter5 sweep removed msync(MS_SYNC|MS_INVALIDATE) from CAPTURE
DQBUF path. iter5 sonnet C3 flagged: no formal pixel verification.

tests/run_msync_pixel_verify.sh — runs FFmpeg SW decode (libavcodec
reference) and FFmpeg HW decode (via our v4l2_request driver),
compares NV12 byte streams. Probes fixture dimensions via ffprobe
and uses crop=$W:$H after hwdownload to normalize MB-padding
artifacts (hantro pads height to 16-line align; SW returns
crop-aligned).

Phase 5 sonnet flagged the stride-mismatch false-failure risk
pre-commit. Fixed: explicit crop + diagnostic that distinguishes
genuine pixel divergence from MB-padding stride artifacts.

# Phase 5 sonnet code review

Verdict: APPROVE-WITH-CHANGES. Three actionable findings, all
addressed before this commit:
1. surface.c error path: separated OUTPUT-DQBUF-failure into
   error_buffer_indeterminate label, slot stays dead-busy
2. cap_pool_probe_pattern.c: added vaCreateContext to actually
   exercise the OUTPUT pool init at the small resolution
3. run_msync_pixel_verify.sh: explicit crop on HW path,
   stride-mismatch diagnostic distinguished from corruption

Empirical verification (Phase 6+7 deploy + run): pending operator
ohm-tools availability.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Claude (noether)
2026-05-06 06:49:48 +00:00
parent a09c03c154
commit 988b848908
6 changed files with 494 additions and 13 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/surface.c
+57 -13
View File
@@ -484,7 +484,14 @@ VAStatus RequestSyncSurface(VADriverContextP context, VASurfaceID surface_id)
surface_object->source_index, 1);
if (rc < 0) {
status = VA_STATUS_ERROR_OPERATION_FAILED;
goto error;
/*
* iter7: OUTPUT DQBUF failed. The V4L2 buffer is in an
* indeterminate kernel state — it may still be QUEUED. Do
* NOT return the slot to acquire-rotation: the next QBUF
* on it would EINVAL. Leave source_data set so the error
* handler skips force_release and the slot stays dead-busy.
*/
goto error_buffer_indeterminate;
}
/*
@@ -523,21 +530,58 @@ VAStatus RequestSyncSurface(VADriverContextP context, VASurfaceID surface_id)
error:
/*
* iter6: request_fd is owned by the OUTPUT pool slot. Do not
* close here on error. The slot's REINIT may not have run if
* we errored before it; the slot stays busy=true and is not
* returned to acquire-rotation until RequestTerminate runs
* request_pool_destroy. (The pool is driver-wide; it survives
* RequestDestroyContext.) That's a bounded leak we accept: with
* pool size 16 and rare errors, slot starvation only matters
* after many error cycles — at which point acquire returns -1
* cleanly.
* iter7: error recovery for the OUTPUT pool slot. If the surface
* acquired a slot in BeginPicture (source_data != NULL indicates
* an active borrow), reset the slot's request_fd via
* request_pool_force_release so the slot returns to the
* acquire-rotation. force_release tries REINIT first; falls back
* to close+alloc if REINIT fails; leaves the slot dead-busy if
* even alloc fails (other slots unaffected). Replaces iter6's
* accepted bounded leak.
*
* TODO: a future iteration could add a request_pool_force_release
* that REINITs the fd and frees the slot for error recovery.
* Reachable from: media_request_queue / wait_completion / REINIT
* failures. NOT reachable for OUTPUT-DQBUF failure (separate label
* `error_buffer_indeterminate` below) because in that case the
* V4L2 buffer is in an indeterminate kernel state and reusing the
* slot would EINVAL on the next QBUF.
*
* If the surface never acquired a slot (source_data == NULL),
* there is no slot to release; nothing to do.
*/
if (surface_object != NULL)
if (surface_object != NULL) {
if (surface_object->source_data != NULL) {
request_pool_force_release(&driver_data->output_pool,
surface_object->source_index);
surface_object->source_data = NULL;
surface_object->source_size = 0;
}
surface_object->request_fd = -1;
}
goto complete;
error_buffer_indeterminate:
/*
* iter7: OUTPUT DQBUF failed after a successful REINIT. The kernel
* V4L2 buffer is in an unknown state (possibly still QUEUED with
* pending decode result, possibly half-dequeued, possibly stuck
* in driver internals). The slot's request_fd has already been
* REINIT'd to a clean state, but reusing the slot for a new
* decode would QBUF on a buffer the kernel may still hold —
* triggering exactly the iter6 race we eliminated for the happy
* path.
*
* Leave the slot dead-busy: don't release, don't force_release.
* Other slots are unaffected. If this fires repeatedly, the pool
* leaks slots until starvation, at which point acquire returns -1
* and BeginPicture cleanly propagates ALLOCATION_FAILED. This is
* a strictly safer failure mode than reusing an indeterminate
* V4L2 buffer.
*/
if (surface_object != NULL) {
surface_object->source_data = NULL;
surface_object->source_size = 0;
surface_object->request_fd = -1;
}
complete:
return status;