Skip to content

Improve TheRock build and optional dependency handling - #280

Open
essamROCm wants to merge 3 commits into
ROCm:developfrom
essamROCm:ea/rocpydecode-therock
Open

Improve TheRock build and optional dependency handling#280
essamROCm wants to merge 3 commits into
ROCm:developfrom
essamROCm:ea/rocpydecode-therock

Conversation

@essamROCm

@essamROCm essamROCm commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Motivation

Enable reliable rocPyDecode builds and testing with current TheRock packages.

Technical Details

  • Add a TheRock development optional Dockerfile.
  • Detect optional FFmpeg utilities before enabling dependent bindings and tests.
  • Fix raw H.265 parsing and combined rocPyDecode/rocPyJPEG imports.
  • Improve CTest target and test-media detection.

Test Plan

Build the image, configure and install rocPyDecode, import both modules, and run all applicable CTests.

Test Result

The image and source built and installed successfully. Raw H.264 and H.265 tests each decoded 601 frames, and the applicable tests passed.

The optional Dockerfile

The Docker image provides:

  • TheRock ROCm SDK development environment and GPU runtime
  • rocDecode development and test packages, including utilities and sample videos
  • HIP/Clang compiler toolchain, CMake, and Ninja
  • Python 3.12 with NumPy, pybind11, and pytest
  • FFmpeg development libraries, DLPack, and other required build dependencies

This enables rocPyDecode to configure, build, install, and run its applicable CTest suite.

Signed-off-by: Essam Aly <essam.aly@amd.com>

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.

Pull request overview

Improves TheRock builds, optional dependency handling, raw H.265 decoding, and combined module testing.

Changes:

  • Adds a TheRock development Dockerfile.
  • Detects optional FFmpeg and test media availability.
  • Fixes H.265 parsing and module coexistence.
  • Improves CTest discovery and build-tree media setup.

Reviewed changes

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

Show a summary per file
File Summary
tests/module_coexistence_test.py Tests combined module imports.
tests/CMakeLists.txt Adds conditional test and capability discovery.
src/common/roc_pybuffer.cpp Enables module-local buffer bindings.
samples/rocdecode/videodecoderaw.py Fixes H.265 Annex-B parsing.
docker/rocpydecode-with-therock.dockerfile Adds the TheRock environment; repository verification and rocJPEG dependencies require changes.
CMakeLists.txt Detects optional FFmpeg and configures test media.
Suppressed comments (5)

CMakeLists.txt:243

  • When FFmpeg is installed but the rocDecode FFmpeg utilities are absent, this branch marks the build as GPU-only. The install logic later uses ROCPYDECODE_HOST_LIBRARY_FOUND only to omit the two CPU-backend scripts, so demux-dependent samples such as videodecode.py are still installed even though PyVideoDemuxer is not compiled (src/rocdecode/roc_pydecode.cpp:134-141). Running those installed samples then fails with missing module attributes. Use ROCPYDECODE_USE_FFMPEG when filtering the installed samples and omit all demux/FFmpeg-dependent scripts for GPU-only builds.
    elseif(FFMPEG_FOUND)
        message(STATUS "FFmpeg found, but rocDecode FFmpeg utilities were not found under ${ROCDECODE_FFMPEG_UTILS_DIR}; building rocPyDecode without demux/CPU backend (GPU decode only).")
        set(ROCPYDECODE_HOST_LIBRARY_FOUND FALSE)
    else()
        message(STATUS "FFmpeg not found; building rocPyDecode without demux/CPU backend (GPU decode only).")

docker/rocpydecode-with-therock.dockerfile:54

  • The image does not clone or copy the repository, so this contributor-specific WORKDIR points at an empty directory unless every user reproduces the same host mount path. A normal container launch therefore does not start in the checkout described by the build/test plan. Use a generic documented mount point (or /workspace) rather than embedding a personal workspace path.
WORKDIR /workspace/EssamWork/ROCPYDECODE

tests/CMakeLists.txt:387

  • decoder_api_test.py only calls the host-specific TestAll* APIs when they exist; its required API checks (AVCodec*, GPU decoder, packet, and surface APIs) are provided by the FFmpeg/GPU binding and do not require rocdecode_HOST_LIBRARY. This new host-library condition therefore skips an otherwise applicable test on GPU-only installations. Gate this test on the FFmpeg binding (and media availability), not on RUN_ROCPYDECODE_HOST_TESTS.
  if(ROCPYDECODE_USE_FFMPEG AND RUN_ROCPYDECODE_HOST_TESTS)

tests/CMakeLists.txt:220

  • Both ROCPYDECODE_PYTHONPATH and ROCPYJPEG_PYTHONPATH already prepend the configure-time $ENV{PYTHONPATH} (see lines 148-160). Consequently, an older installed module from the caller's environment is searched before these build/install outputs, so this coexistence test can import stale extensions and pass without testing the current artifacts. Put the module paths first and append the existing environment once.
    "PYTHONPATH=${ROCPYDECODE_PYTHONPATH}:${ROCPYJPEG_PYTHONPATH}:$ENV{PYTHONPATH}"

tests/CMakeLists.txt:177

  • When the installed test package is configured separately, this recomputes the FFmpeg capability from the test machine rather than from the already-built binding. A GPU-only rocPyDecode installed from a build where the rocDecode FFmpeg utilities were absent will be treated as FFmpeg-enabled if those files happen to be present later, causing demux/CPU CTests to run against symbols that were never compiled into rocpydecode. Persist the build feature in install metadata or probe the installed module before enabling these tests.
if(NOT DEFINED ROCPYDECODE_USE_FFMPEG)
  find_package(FFmpeg QUIET)
  set(ROCDECODE_FFMPEG_UTILS_DIR "${ROCM_PATH}/share/rocdecode/utils/ffmpegvideodecode")
  file(GLOB ROCDECODE_FFMPEG_UTILS_SOURCES "${ROCDECODE_FFMPEG_UTILS_DIR}/*.cpp")
  set(ROCPYDECODE_USE_FFMPEG FALSE)

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

Comment thread docker/rocpydecode-with-therock.dockerfile Outdated
Comment thread docker/rocpydecode-with-therock.dockerfile
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants