removed OpenCV decoder option for image#452
Conversation
|
@adeljo-amd Please take a look at this. We are removing OpenCV backend. Please update the examples from your PR: ROCm/rocm-examples#354 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #452 +/- ##
===========================================
- Coverage 80.31% 79.60% -0.71%
===========================================
Files 317 315 -2
Lines 25379 25305 -74
===========================================
- Hits 20382 20144 -238
- Misses 4997 5161 +164
🚀 New features to boost your workflow:
|
|
Minor changes required for merge
|
There was a problem hiding this comment.
Pull request overview
This PR removes the OpenCV-based image decoder option from rocAL, updating the decoder factory, public decoder enums, tests, and Python bindings accordingly.
Changes:
- Removed the OpenCV image decoder implementation and its registration in the decoder factory.
- Removed the OpenCV decoder option from C++ tests and Python/pybind exposed decoder enums.
- Updated API/internal decoder type enums to drop the OpenCV entry (currently by renumbering subsequent values).
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/cpp_api/image_augmentation/image_augmentation.cpp | Removes an OpenCV-decoder-specific message; still passes CLI decoder values through. |
| tests/cpp_api/basic_test/basic_test.cpp | Updates CLI decoder selection logic to drop OpenCV choice. |
| rocAL_pybind/rocal_pybind.cpp | Removes DECODER_OPENCV from the Python-exposed enum. |
| rocAL_pybind/amd/rocal/types.py | Removes DECODER_OPENCV import and mapping entry. |
| rocAL/source/decoders/image/open_cv_decoder.cpp | Deletes the OpenCV decoder implementation. |
| rocAL/source/decoders/image/decoder_factory.cpp | Removes OpenCV decoder include and factory case. |
| rocAL/source/api/rocal_api_data_loaders.cpp | Removes mapping from ROCAL_DECODER_OPENCV to DecoderType::OPENCV. |
| rocAL/include/decoders/image/open_cv_decoder.h | Deletes the OpenCV decoder header. |
| rocAL/include/decoders/image/decoder.h | Removes DecoderType::OPENCV and renumbers subsequent values. |
| rocAL/include/api/rocal_api_types.h | Removes ROCAL_DECODER_OPENCV and renumbers subsequent public API values. |
Comments suppressed due to low confidence (1)
tests/cpp_api/image_augmentation/image_augmentation.cpp:153
decoder_typeis parsed as an int and then used as aRocalDecoderType(dec_type), but after removing the OpenCV decoder (and renumberingRocalDecoderType) some numeric values now correspond to non-image decoders (video/audio). Passing those through torocalJpegFileSourcecan silently fall back to the default decoder, which makes the CLI option misleading. Consider explicitly mapping/validating the allowed image decoder values here (e.g., only TJPEG / ROCJPEG) and rejecting or warning on unsupported values.
} else {
// The jpeg file loader can automatically select the best size to decode all images to that size
// User can alternatively set the size or change the policy that is used to automatically find the size
if (decode_height <= 0 || decode_width <= 0)
input1 = rocalJpegFileSource(handle, folderPath1, color_format, shard_count, false, shuffle, false);
else
input1 = rocalJpegFileSource(handle, folderPath1, color_format, shard_count, false, shuffle, false, ROCAL_USE_USER_GIVEN_SIZE_RESTRICTED, decode_width, decode_height, dec_type);
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
HI @kiritigowda : Resolved all the comments. Please review again and merge |
|
@kiritigowda : all the comments are addressed in this PR and the codecov is dropping because of another commit. Can you please merge this? |
Motivation
removing OpenCV dependency and decoding option
Technical Details
OpenCV decoder files are removed and its use case
Test Plan
rocAL CTest
Test Result
rocAL CTest should pass
Submission Checklist