Repository navigation
fix(rocJPEG): keep a VA surface pool entry busy until all its surfaces are released - #14028
Conversation
…s are released A pool entry holds all surfaces of a batched sub-batch, but surfaces are released one at a time, and releasing any one of them marked the whole entry idle. A submit could then reuse the entry while its other surfaces were still pending, decoding over images not yet synchronized. This happens with rocJpegDecodeBatchedAsync on GPUs with several JPEG cores, for example a partial rocJpegDecodeSync, or a submit that runs while rocJpegDecodeBatchedSync releases a sub-batch. Track the status of each surface in the entry and mark the entry idle only when all of its surfaces are idle. Co-Authored-By: Claude <noreply@anthropic.com>
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
There was a problem hiding this comment.
🟢 Approval recommended
The focused changes consistently enforce the surface-lifetime rule while preserving locking and caller contracts, with no unresolved findings.
No open issues 🎉
What changed in this PR
Fixes rocJPEG surface-pool reuse during batched asynchronous decoding by keeping each pool entry busy until all its surfaces are released.
Changes:
- Adds per-surface status tracking and documents the entry-release rule.
- Initializes surface statuses on insertion, resets them on reuse, and marks an entry idle only after every surface is idle.
| File | Description |
|---|---|
| projects/rocjpeg/src/rocjpeg_vaapi_decoder.h | Adds per-surface status storage and clarifies release documentation. |
| projects/rocjpeg/src/rocjpeg_vaapi_decoder.cpp | Updates insertion, reuse, and release paths to track all surfaces. |
Review effort: Balanced
rrawther
left a comment
rrawther
left a comment
There was a problem hiding this comment.
Reviewed the change and built/tested it locally. The diagnosis is correct and the fix sits at the right layer: I confirmed that both acquisition sites (SubmitDecode and SubmitDecodeBatched) consume an entire pool entry and that GetEntry requires an exact va_surface_ids.size() == num_surfaces match, so surfaces are never split across submissions — which is exactly the invariant the per-surface tracking relies on. All mutations stay under pool_mutex_, and since RocJpegVaapiMemPoolEntry is internal to src/, no ABI bump is needed.
Build and test: clean build with 0 warnings; 27/27 ctest cases pass. jpeg-decode-perf-fmt-native reports 155.6 Images/Sec / 806.8 Mpixels/Sec, consistent with the "no throughput change" claim.
One caveat on my test run, since it affects how much weight it carries: I probed the hardware directly and this machine reports VCN_JPEG cores = 1 (RX 7900 XT / gfx1100). Pool entries are sized to the core count, so every entry here holds exactly one surface — surface_status is a one-element vector and the std::all_of is trivially equivalent to the old behavior. My run confirms no regression on single-core hardware; it does not independently confirm the fix. For that I'm relying on your forced-4-core repro, which is clearly described and a reasonable methodology.
Related to that: no automated test is added here. Covering this needs either multi-core hardware in CI or a seam to inject the core count, so it's understandable to defer — but it does mean the regression could silently return. Might be worth a tracking issue.
Comments inline. Nothing blocking; the only one I'd suggest landing with this PR rather than after is the unchecked index in SetSurfaceAsIdle.
One pre-existing gap, out of scope but adjacent. On a SubmitDecodeBatched failure, the cleanup loops in rocjpeg_decoder.cpp (lines 504-507 and 571-574) release surfaces only for sub-batches j < i. Surfaces that the failing sub-batch already acquired via GetEntry are never released, so that entry stays busy for the decoder's lifetime. Not a regression — the old code didn't release them either — but this PR makes entry lifetime stricter, which makes the gap more consequential. Worth a follow-up.
…-surface vectors Initialize surface_status in SubmitDecode and SubmitDecodeBatched next to va_surface_ids and hip_interops, instead of rewriting it in AddPoolEntry after the entry is already in the pool. AddPoolEntry no longer silently replaces a caller-provided surface_status. Review by rrawther: #14028 (comment) Co-Authored-By: Claude <noreply@anthropic.com>
…urfaceAsIdle Return an error instead of writing out of bounds if a pool entry's surface_status does not cover the released surface. Review by rrawther: #14028 (comment) Co-Authored-By: Claude <noreply@anthropic.com>
…PoolEntry Document that an entry stays busy until all its surfaces are released, so a partially synced batched decode can also grow the pool past its target. Review by rrawther: #14028 (comment) Co-Authored-By: Claude <noreply@anthropic.com>
…ry doc comment Review by rrawther: #14028 (comment) Co-Authored-By: Claude <noreply@anthropic.com>
|
@rrawther Thanks for testing. I'll open a tracking issue for an automated multi-core test (core-count injection seam) and fix the failing-sub-batch release in |
TheRock Submodule Bump ActivityNewest first
|
Motivation
On GPUs with multiple JPEG cores, a batched async decode can be overwritten by a later submit: releasing one surface of a pool entry marks the whole entry idle, so it can be reused while its other surfaces are still pending. Reported in review of #13997.
Technical Details
RocJpegVaapiMemPoolEntrynow tracks the status of each surface;SetSurfaceAsIdlemarks the entry idle only when all its surfaces are idle. No caller changes.Issue Tracking
JIRA ID: AICV-274
Test Plan
Ran the rocJPEG ctests. Reproduced the bug with a test build forcing 4 JPEG cores (so pool entries hold multiple surfaces):
rocJpegDecodeBatchedAsyncof 4 images,rocJpegDecodeSyncon one, then a secondrocJpegDecodeBatchedAsync, comparing the first batch with a reference decode. CheckedjpegDecodePerfthroughput.Test Result
27/27 tests passed. Repro:
developcorrupted 3 of 4 images in 3/3 runs; this PR passed 3/3. No throughput change.Submission Checklist
🤖 Generated with Claude Code