Skip to content

feat(rocJPEG): batch HIP color-conversion kernels in the batched decode path - #11876

Merged
AryanSalmanpour merged 14 commits into
developfrom
users/asalmanp/batched_hip_kernels
Oct 6, 2026
Merged

AryanSalmanpour merged 14 commits into
developfrom
users/asalmanp/batched_hip_kernels

Conversation

@AryanSalmanpour

Copy link
Copy Markdown
Member

Motivation

The batched decode path (FinalizeDecodeBatched) previously launched one
color-conversion kernel per image per sub-batch — for a batch of N images,
N launches per output format, so launch overhead dominated for small/medium
images.

This PR restructures the path to accumulate per-image parameters and issue a
single kernel launch per format per sub-batch
; hipBlockIdx_z selects the
image inside the kernel. For RGB / RGB_PLANAR, all supported YUV surface layouts
are collapsed into one unified kernel each, so a mixed-format batch is one
launch instead of up to five.

No public API change — behavior is identical, only the number of kernel launches
drops.

Technical Details

Kernels (rocjpeg_hip_kernels.{h,cpp})

  • ColorConvertYUVToRGBBatched / ColorConvertYUVToRGBPlanarBatched — unified
    kernels covering NV12, YUV444, YUV440 (422V), YUYV (422), and YUV400, selected
    per image by a YUVSurfaceLayout tag.
  • Per-format batched kernels for paths that don't fit the unified YUV model:
    ColorConvertRGBAToRGBBatched, ConvertPackedYUYVToPlanarYUVBatched,
    ConvertInterleavedUVToPlanarUVBatched, ExtractYFromPackedYUYVBatched.
  • All single-image kernels are unchanged.

Decoder (rocjpeg_decoder.{h,cpp})

  • BatchedKernelParams<T> — reusable scratch buffer (host param vector +
    lazily-grown device buffer + max grid extents), uploaded once per group.
    Copy/move are deleted since it owns a raw device allocation.
  • FinalizeDecodeBatched is now two-pass per sub-batch: (1) sync all VA surfaces
    and collect HIP interop memory; (2) accumulate per-image params, then one launch
    per kernel type, then a single hipStreamSynchronize.
  • Accumulate* helpers mirror the existing per-image conversion helpers'
    pointer/offset/ROI math exactly, but push params instead of launching.

Design notes

  • No warp divergence from the layout branch: hipBlockIdx_z fixes the image
    (hence layout) for a whole block, so the per-layout switch in the unified
    load is block-uniform.
  • Bit-identical conversion: the unified kernel's color-conversion body and
    planar-deinterleave write are copied verbatim from the YUV444 kernels; only the
    source load differs per layout. YUV400 folds in by feeding U=V=128, which the
    conversion collapses to R=G=B=Y exactly.
  • Grid sizing: one grid per launch, sized to the max width/height across the
    group — very uneven image sizes waste some threads on the padded axis.

Issue Tracking

JIRA ID: AICV-302

Test Plan

rocjpeg ctest.

Test Result

rocjpeg ctest should pass.

Submission Checklist

Copilot AI lite review requested due to automatic review settings September 18, 2026 19:31
@therock-pr-bot

therock-pr-bot Bot commented Sep 18, 2026 •

Copy link
Copy Markdown

✅ All Policy Checks Passed

Check Status Details
📝 PR Description ✅ Pass —
⛔ Forbidden Files ✅ Pass —
🧪 Unit Test ⚠️ Warning Error: Source/code files changed without an accompanying unit test.
Expected: add at least one test file named like test_<name>.py / test_<name>.cpp (or <name>_test.*).
Current: code file(s) changed: projects/rocjpeg/src/rocjpeg_decoder.cpp, projects/rocjpeg/src/rocjpeg_decoder.h, projects/rocjpeg/src/rocjpeg_hip_kernels.cpp, projects/rocjpeg/src/rocjpeg_hip_kernels.h, projects/rocjpeg/src/rocjpeg_vaapi_decoder.cpp; no test file found
🚫 Draft PR 🔜 To Be Enabled —
🚩 Feature Flag 🔜 To Be Enabled —
📊 Code Coverage 🔜 To Be Enabled —

🎉 All policy checks passed!

📖 Need help? See the Policy FAQ for details on every check and how to fix failures.

🙋 Wish to Override Policy?

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The batched parameter buffer can become unrecoverable after an allocation failure and needs state reset before approval.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Batches rocJPEG HIP color-conversion work across images to reduce kernel-launch overhead.

Changes:

  • Adds batched parameter structs and unified YUV-to-RGB kernels.
  • Refactors batched finalization to accumulate parameters and launch once per kernel type.
  • Bumps rocJPEG to 1.11.0 and updates the changelog.
File summaries
File Description
rocjpeg_hip_kernels.h Declares batched kernel parameters and APIs.
rocjpeg_hip_kernels.cpp Implements batched conversion kernels.
rocjpeg_decoder.h Adds reusable batched parameter buffers.
rocjpeg_decoder.cpp Accumulates and launches batched conversions.
CMakeLists.txt Updates the project version.
CHANGELOG.md Documents the feature.
Review details
  • Files reviewed: 6/6 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread projects/rocjpeg/src/rocjpeg_decoder.h Resolved
@AryanSalmanpour
AryanSalmanpour requested a lite review from Copilot September 18, 2026 20:05

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Shared batched scratch state is unsafe for concurrent finalization, and new launch failures can leave VA surfaces marked busy.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity

Open (1)
Resolved since last review (2)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Release sub-batch surfaces when parameter setup fails

projects/​rocjpeg/​src/​rocjpeg_decoder.cpp:394

If any newly added parameter upload/allocation fails here, CHECK_ROCJPEG returns before the cleanup loop below, so none of the sub-batch surfaces is passed to SetSurfaceAsIdle. The failed call can therefore leave pool entries permanently busy and make later decodes fail; error exits from this new launch path need to synchronize/ release the surfaces before returning.

Comment thread projects/rocjpeg/src/rocjpeg_decoder.h Resolved

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The substantial GPU-kernel and shared-buffer changes require final human validation across mixed layouts and ROI cases.

Review effort: Lite
Findings: None

Resolved since last review (1)

@rrawther rrawther left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggested by claude review
Bit-identity not machine-verified for 444P / 422V / RGBA. These formats aren't in the test image set, and the ctest doesn't MD5-compare (see §5). Given the whole PR rests on "verbatim/bit-identical," consider adding a 444/440 image + an output-hash check, or at least manually diffing single vs batched output for those formats before merge.
LaunchBatchedBuffer ignores the kernel-launch result (rocjpeg_decoder.h:496) — launch(...) returns void and no hipGetLastError() is checked; a bad-config launch only surfaces at the later hipStreamSynchronize. Acceptable, but a post-launch error check would localize failures.
finalize_mutex_ vs hip_stream_: the batched finalize is serialized, but the non-batched FinalizeDecode also uses hip_stream_ without taking finalize_mutex_. If both can run concurrently on one decoder, that's a shared-stream hazard. Worth confirming the intended threading model (async tests pass, so not observed).
Header comment // ---- Auto-generated batched param structs ---- (rocjpeg_hip_kernels.h:1042) — if these aren't actually generated by a tool, drop "Auto-generated" to avoid implying a generator that reviewers should look for.
Minor whitespace inconsistency mem.offset[2]+ roi_uv_offset (rocjpeg_decoder.cpp:300); pre-commit clang-format will likely fix.

@AryanSalmanpour

Copy link
Copy Markdown
Member Author

mem.offset[2]+ roi_uv_offset

Bit-identity validation / MD5 checks: Adding MD5-based verification is beyond the scope of this PR. However, I have manually verified that the batched output matches the single-image path for all currently supported chroma subsampling formats, including the formats covered by the new batched implementation. I agree that adding output hash comparisons would strengthen the test coverage, and we can address this as a testing enhancement in a follow-up PR by adding additional test images (e.g., 444P/422V) along with automated MD5-based validation.

Kernel launch error checking: Agreed. The current implementation relies on the subsequent hipStreamSynchronize() to surface launch failures. While this is consistent with the existing error handling flow, adding a post-launch hipGetLastError() check would improve error localization and can be considered as a follow-up improvement.

finalize_mutex_ vs. hip_stream_: The intended threading model does not allow concurrent execution of batched and non-batched finalize operations on the same decoder instance. This is also consistent with the current usage patterns and test coverage, which is why no issue has been observed. That said, the concern is valid, and we can add a clarifying comment or revisit synchronization if the threading model evolves.

@rrawther rrawther left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some minor comments for name change

@rrawther rrawther left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added some minor comments for name change

@rrawther rrawther left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added new comment

@AryanSalmanpour
AryanSalmanpour force-pushed the users/asalmanp/batched_hip_kernels branch from d51b1ca to 565b825 Compare October 6, 2026 00:15
@AryanSalmanpour
AryanSalmanpour merged commit 90d4836 into develop Oct 6, 2026
85 of 86 checks passed
@AryanSalmanpour
AryanSalmanpour deleted the users/asalmanp/batched_hip_kernels branch October 6, 2026 04:50
@systems-assistant

Copy link
Copy Markdown
Contributor

TheRock Submodule Bump Activity

Newest first

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants