Repository navigation
refactor(rocJPEG): route single-image decode through the batched kernels - #12976
Conversation
Motivation Commit 90d4836 added a batched color-conversion kernel family, including two unified YUV->RGB kernels that cover NV12/YUV444/YUV440/YUYV/YUV400 via a per-image layout tag selected by hipBlockIdx_z. It deliberately left the single-image kernels in place, so the library carried two complete and independently maintained implementations of the same conversion math. That duplication is a maintenance hazard: the BT.709 -> BT.601 change in c0b583c had to be applied in ten separate kernel bodies, and any future coefficient or layout fix would have to be too. A single image is just a batch of one, so the batched path already subsumes the single-image path entirely. Technical details Kernels (rocjpeg_hip_kernels.{h,cpp}) - Removed all 14 single-image kernels and their host launchers: the five YUV->RGB variants and their planar counterparts, plus ColorConvertRGBAToRGB, ConvertInterleavedUVToPlanarUV, ExtractYFromPackedYUYV and ConvertPackedYUYVToPlanarYUV. - The batched kernels are now the only conversion kernels in the library. Comments that referred to the single-image bodies as the source of truth were restated accordingly. Decoder (rocjpeg_decoder.{h,cpp}) - Decode and DecodeSync now call FinalizeDecodeBatched(..., 1) directly. - Removed FinalizeDecode. It had become a pass-through whose null guard was unreachable from both call sites, and FunctionExitLog is a no-op macro, so no logging coverage is lost. - Removed the four per-image output producers that only FinalizeDecode used: ColorConvertToRGB, ColorConvertToRGBPlanar, GetPlanarYUVOutputFormat and GetYOutputFormat. The Accumulate* helpers are now the only implementation. Behavior is unchanged. Error semantics are preserved: FinalizeDecodeBatched releases surfaces only on the success path, and both single-image callers still call SetSurfaceAsIdle on failure. Version bumped to 1.12.0. No public API or ABI change; the soname major stays at 1. Test plan - Fresh configure and build, clean with no unused-function warnings. - ctest: 26/26 passing. - Bit-exactness against a build of develop: decoded 4:4:4, 4:4:0, 4:2:2, 4:2:0 and grayscale JPEGs across all five output formats, with and without a crop rectangle, through both libraries. 50 outputs per side, byte identical. Repeated for the async path (DecodeSync), 10 outputs, also byte identical. - ROI note: this test GPU reports 0 VCN JPEG cores, so can_roi_decode is false and the crop runs exercised the software ROI offset math. The hardware-ROI branch needs a >=8-core part and was not covered. Co-Authored-By: Claude Opus 5 <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.
Copilot review overview
🟡 Changes recommended
Single-image decoding can hang indefinitely when JPEG-core discovery leaves the batched group size at zero.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Routes single-image rocJPEG decoding through the batched conversion pipeline, removing duplicate kernels and helpers.
Changes:
- Uses
FinalizeDecodeBatched(..., 1)for single-image decoding. - Removes redundant single-image kernels and output helpers.
- Bumps rocJPEG to 1.12.0 and updates the changelog.
| File | Description |
|---|---|
projects/rocjpeg/CMakeLists.txt |
Bumps version to 1.12.0. |
projects/rocjpeg/CHANGELOG.md |
Documents the unified decode path. |
projects/rocjpeg/src/rocjpeg_decoder.cpp |
Routes single-image finalization through batching and removes duplicate helpers. |
projects/rocjpeg/src/rocjpeg_decoder.h |
Removes obsolete declarations and updates documentation. |
projects/rocjpeg/src/rocjpeg_hip_kernels.cpp |
Removes single-image kernel implementations. |
projects/rocjpeg/src/rocjpeg_hip_kernels.h |
Removes single-image prototypes and documents batched kernels. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Motivation Review feedback on the previous commit: routing the synchronous single-image path through FinalizeDecodeBatched introduces a hang when JPEG-core discovery fails. Confirmed by fault injection -- forcing num_jpeg_cores to 0 makes a single-image decode hang indefinitely, stalling right after decode submission. GetNumJpegCores() returns void and both of its failure paths only log, leaving the zero-initialized count in place while InitializeDecoder still reports success. The work-grouping loops advance by `i += num_jpeg_cores`, so a zero stride never terminates. Technical details Fixed at the single producer of the value rather than at the loops, because there are three such loops -- in FinalizeDecodeBatched, DecodeBatched and DecodeBatchedAsync. The latter two carried the same latent hang before the single-image path was ever routed through them. - Default the spec to a core count of 1, so the early-return path on amdgpu_device_initialize failure leaves a usable stride. - Clamp a successful query that reports 0 to 1 as well; amdgpu_query_hw_ip_count can return success with a zero count. - Document the ">= 1" invariant on the field itself so it is not reintroduced. A GPU genuinely without JPEG cores cannot reach this code: CreateDecoderConfig runs first and fails with ROCJPEG_STATUS_HW_JPEG_DECODER_NOT_SUPPORTED unless VA-API advertises VAEntrypointVLD for VAProfileJPEGBaseline. So a zero here means discovery disagreed with VA-API about hardware that demonstrably decodes JPEG -- a bad reading, not a real capability. The previous behavior did not preserve a meaningful zero either: it produced a 1-surface pool and then hung. The capability flags still key off the raw queried value, so can_roi_decode and can_convert_to_rgb remain false under the fallback. The library never claims hardware ROI or hardware CSC it does not have; it degrades to one image at a time with software ROI. Discovery failure is deliberately not made fatal. VA-API submission still works in that state, so failing rocJpegCreate would turn a degraded-but-working setup into a dead one. The fallback is logged. Test plan - Fault-injected both failure modes (device-init failure, and a successful query returning zero cores) across jpegdecode, jpegdecodeasync and jpegdecodebatched: all six combinations exit 0, where the single-image case previously hung. Injection hooks were removed before committing. - Output under the fallback is byte-identical to the normal path. - ctest: 26/26 passing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
rrawther
left a comment
rrawther
left a comment
There was a problem hiding this comment.
Issues found
[Minor] The changelog does not describe this PR, and retroactively marks 1.11.0 as released
CHANGELOG.md:6-16
At the base commit, ## (unreleased) rocJPEG 1.11.0 held two entries: the BT.601 change (from c0b583cf8f5) and "Added HIP color-conversion kernels in the batched decode path" (from 90d48366c71). This PR drops the (unreleased) marker from 1.11.0 — claiming a release that never happened — and moves the batched-kernels entry up into the new 1.12.0 section, re-attributing 90d48366c71's work to a version it wasn't in. Meanwhile this PR's own change (single-image decode now goes through the batched kernels; 14 kernels removed) gets no entry at all.
Since 1.11.0 never shipped, the straightforward fix is to leave 1.11.0 (unreleased) with both of its original entries, drop the version bump, and add a ### Changed line there for this refactor — or, if you want the bump, keep 1.11.0 intact and put this PR's actual change under 1.12.0.
[Nit] SetPoolSize changes on the discovery-failure path
src/rocjpeg_vaapi_decoder.cpp:547
SetPoolSize(5 * num_jpeg_cores + 1) previously evaluated to 1 when core discovery failed (count was 0); it now evaluates to 6. That's a strict improvement and almost certainly intended, but it is a second behavior change riding along with the zero-guard and isn't mentioned in the commit message.
Suggestions
Decode() now acquires finalize_mutex_ while holding mutex_
src/rocjpeg_decoder.cpp:133 + src/rocjpeg_decoder.cpp:246
I checked the lock order across all five entry points: Decode/DecodeBatched take mutex_ → finalize_mutex_; DecodeSync/DecodeBatchedSync release mutex_ before taking finalize_mutex_; nothing takes finalize_mutex_ then mutex_. No deadlock. The observable change is that a concurrent DecodeBatchedSync can now stall Decode() before it starts work, extending the window in which Decode() holds mutex_ and thus blocks DecodeAsync submissions. This is arguably a correctness improvement — previously Decode()'s kernel launches shared hip_stream_ with a concurrent batched finalize with no serialization — but it is a real scheduling change that the "Behavior is unchanged" claim glosses over. Worth one sentence in the commit message.
Per-decode param upload on the single-image hot path
src/rocjpeg_decoder.h:71-82
Single-image decode now pays one hipMemcpyAsync of the param struct plus a device-memory indirection in the kernel. The hipMalloc is amortized (capacity grows on demand and persists). I measured this and it's lost in the noise against VCN decode time (200 iterations, 3840x2160 4:2:0: RGB 11.36 ms PR vs 12.05 ms base; native 11.42 ms PR vs 11.08 ms base) — just noting it as understood-and-accepted rather than unexamined.
Claims I verified rather than took on faith
- "the null guard was unreachable from both call sites" — correct.
Decode()null-checks all three pointers atrocjpeg_decoder.cpp:134, andGetJpegStreamParameters()returns&jpeg_stream_parameters_(rocjpeg_parser.h:231), which can never be null.DecodeSync()passes the address of a value member. - Error semantics preserved —
FinalizeDecodeBatchedreleases surfaces only afterLaunchBatchedParamsandhipStreamSynchronizeboth succeed; both single-image callers still callSetSurfaceAsIdleon failure. Withbatch_size == 1there is exactly one sub-batch, so no double-release. - Format coverage is complete — every case the removed
ColorConvertToRGB/ColorConvertToRGBPlanar/GetPlanarYUVOutputFormat/GetYOutputFormathandled has anAccumulate*equivalent, includingVA_FOURCC_RGBA(→b_rgba_rgb_) andVA_FOURCC_RGBP(→ threeCopyChannels). All six batched buffers are launched inLaunchBatchedParams. - YUV400 is bit-exact, not approximately equal — the removed kernel wrote R=G=B=Y directly; the unified kernel feeds neutral chroma 128 through the conversion. With the full-range BT.601 coefficients in
rocjpeg_hip_kernels.h:49-54and no studio-swing Y offset,fmaf(c, 0.0f, Y) == Yexactly. Equivalent. - Block dims match — removed YUV→RGB launchers used 16x4, batched uses 16x4; removed RGBA→RGB used 16x16, batched uses 16x16. Grid extent formulas identical.
- No dangling references to any of the 14 removed symbols anywhere in the repo.
Test coverage gaps, for the record
I built bb18b69d6c8~1 in a separate worktree and ran an identical matrix against both builds — 5 output formats (native/yuv_planar/y/rgb/rgb_planar) x crop and no-crop x 4 decode APIs (single, batched, async, batched-async) — and compared all 120 output files: byte-identical, zero mismatches. Negative API + fuzz test also passes.
However, the repo's data/images/ only contains 4:2:0, 4:2:2 and 4:0:0 samples, so the YUV444 (444P) and YUV440 (422V) layout tags in the unified kernel were not exercised. The RGBA/RGBP surface paths also weren't — they require VCN hardware CSC (>=8 JPEG cores + ROCJPEG_ENABLE_VCN_HW_CSC=1), and my test machine is an RX 7900 XT. Both gaps are pre-existing in the ctest suite, not introduced here, but they do mean three of the six batched kernels now have no single-image regression coverage at all once the alternative implementation is gone.
@rrawther Thanks for the review. None of the comments above require any code changes; they only request updates to the commit message. Regarding the changelog, it was intentionally left blank because the changes in this PR are transparent to the user and do not introduce any user-visible behavior changes. I have also updated the commit message based on the suggestions provided. As for the lack of YUV444/YUV440 test images, this is a known pre-existing issue and is not specific to this PR. Support for those test images will be added in upcoming PRs. |
TheRock Submodule Bump ActivityNewest first
|
…els (#12976) ## Motivation Commit 90d4836 added a batched color-conversion kernel family, including two unified YUV->RGB kernels that cover NV12/YUV444/YUV440/YUYV/YUV400 via a per-image layout tag selected by hipBlockIdx_z. It deliberately left the single-image kernels in place, so the library carried two complete and independently maintained implementations of the same conversion math. That duplication is a maintenance hazard: the BT.709 -> BT.601 change in c0b583c had to be applied in ten separate kernel bodies, and any future coefficient or layout fix would have to be too. A single image is just a batch of one, so the batched path already subsumes the single-image path entirely. ## Technical Details Kernels (rocjpeg_hip_kernels.{h,cpp}) - Removed all 14 single-image kernels and their host launchers: the five YUV->RGB variants and their planar counterparts, plus ColorConvertRGBAToRGB, ConvertInterleavedUVToPlanarUV, ExtractYFromPackedYUYV and ConvertPackedYUYVToPlanarYUV. - The batched kernels are now the only conversion kernels in the library. Comments that referred to the single-image bodies as the source of truth were restated accordingly. Decoder (rocjpeg_decoder.{h,cpp}) - Decode and DecodeSync now call FinalizeDecodeBatched(..., 1) directly. - Removed FinalizeDecode. It had become a pass-through whose null guard was unreachable from both call sites, and FunctionExitLog is a no-op macro, so no logging coverage is lost. - Removed the four per-image output producers that only FinalizeDecode used: ColorConvertToRGB, ColorConvertToRGBPlanar, GetPlanarYUVOutputFormat and GetYOutputFormat. The Accumulate* helpers are now the only implementation. Behavior is unchanged. Error semantics are preserved: FinalizeDecodeBatched releases surfaces only on the success path, and both single-image callers still call SetSurfaceAsIdle on failure. Version bumped to 1.12.0. No public API or ABI change; the soname major stays at 1. Output is unchanged. Error semantics are preserved: FinalizeDecodeBatched releases surfaces only on the success path, and both single-image callers still call SetSurfaceAsIdle on failure. Locking changes observably, however. Decode() now acquires finalize_mutex_ while still holding mutex_, so a concurrent DecodeBatchedSync can stall Decode() before it starts work, extending the window in which Decode() holds mutex_ and therefore blocks DecodeAsync submissions. Lock order is consistent across all five entry points -- Decode and DecodeBatched take mutex_ then finalize_mutex_, DecodeSync and DecodeBatchedSync release mutex_ before taking finalize_mutex_, and nothing takes finalize_mutex_ then mutex_ -- so there is no deadlock. The added serialization is arguably a correctness improvement: previously Decode()'s kernel launches shared hip_stream_ with a concurrent batched finalize with no serialization at all. ## Issue Tracking JIRA ID: AICV-314 ## Test Plan Test plan rocjpeg ctest - Single-image decode now pays one hipMemcpyAsync of the param struct plus a device-memory indirection in the kernel; the hipMalloc is amortized, since BatchedKernelParams grows its device buffer on demand and keeps it. Measured at 200 iterations on a 3840x2160 4:2:0 image: RGB 11.36 ms vs 12.05 ms base, native 11.42 ms vs 11.08 ms base -- lost in the noise against VCN decode time. Noted as understood and accepted rather than unexamined. ## Test Result rocjpeg ctest shoudl pass. ## Submission Checklist - [ ] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Motivation
Commit 90d4836 added a batched color-conversion kernel family, including two unified YUV->RGB kernels that cover NV12/YUV444/YUV440/YUYV/YUV400 via a per-image layout tag selected by hipBlockIdx_z. It deliberately left the single-image kernels in place, so the library carried two complete and independently maintained implementations of the same conversion math.
That duplication is a maintenance hazard: the BT.709 -> BT.601 change in c0b583c had to be applied in ten separate kernel bodies, and any future coefficient or layout fix would have to be too.
A single image is just a batch of one, so the batched path already subsumes the single-image path entirely.
Technical Details
Kernels (rocjpeg_hip_kernels.{h,cpp})
Decoder (rocjpeg_decoder.{h,cpp})
Behavior is unchanged. Error semantics are preserved: FinalizeDecodeBatched releases surfaces only on the success path, and both single-image callers still call SetSurfaceAsIdle on failure.
Version bumped to 1.12.0. No public API or ABI change; the soname major stays at 1.
Output is unchanged. Error semantics are preserved: FinalizeDecodeBatched
releases surfaces only on the success path, and both single-image callers
still call SetSurfaceAsIdle on failure.
Locking changes observably, however. Decode() now acquires finalize_mutex_
while still holding mutex_, so a concurrent DecodeBatchedSync can stall
Decode() before it starts work, extending the window in which Decode() holds
mutex_ and therefore blocks DecodeAsync submissions. Lock order is consistent
across all five entry points -- Decode and DecodeBatched take mutex_ then
finalize_mutex_, DecodeSync and DecodeBatchedSync release mutex_ before taking
finalize_mutex_, and nothing takes finalize_mutex_ then mutex_ -- so there is
no deadlock. The added serialization is arguably a correctness improvement:
previously Decode()'s kernel launches shared hip_stream_ with a concurrent
batched finalize with no serialization at all.
Issue Tracking
JIRA ID: AICV-314
Test Plan
Test plan
rocjpeg ctest
device-memory indirection in the kernel; the hipMalloc is amortized, since
BatchedKernelParams grows its device buffer on demand and keeps it. Measured
at 200 iterations on a 3840x2160 4:2:0 image: RGB 11.36 ms vs 12.05 ms base,
native 11.42 ms vs 11.08 ms base -- lost in the noise against VCN decode
time. Noted as understood and accepted rather than unexamined.
Test Result
rocjpeg ctest shoudl pass.
Submission Checklist