ImageManipV2 optimization and refactor #1290
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…gemanipv2_optimization
…gemanipv2_optimization
…gemanipv2_optimization
…gemanipv2_optimization
… incorrect striding in Camera gray output, refactor ImageManipV2
…gemanipv2_optimization
…gemanipv2_optimization
…gemanipv2_optimization
…i-core into imagemanipv2_optimization
There was a problem hiding this comment.
Copilot reviewed 8 out of 11 changed files in this pull request and generated 1 comment.
Files not reviewed (3)
- cmake/Depthai/DepthaiDeviceRVC4Config.cmake: Language not supported
- cmake/Depthai/DepthaiDeviceSideConfig.cmake: Language not supported
- tests/CMakeLists.txt: Language not supported
Comments suppressed due to low confidence (1)
tests/src/ondevice_tests/pipeline/node/image_manip_v2_optimization_test.cpp:169
- [nitpick] Consider using more descriptive log messages instead of abbreviations like 'FSD' to improve test readability and maintainability.
std::cout << "FSD" << std::endl;
| [&](const ImageManipConfigV2& config, const ImgFrame& srcFrame, ImgFrame& dstFrame) { | ||
| auto outType = config.outputFrameType == ImgFrame::Type::NONE ? srcFrame.getType() : config.outputFrameType; | ||
| [&](std::shared_ptr<Memory>& src, std::shared_ptr<impl::_ImageManipMemory> dst) { | ||
| auto srcMem = std::make_shared<impl::_ImageManipMemory>(src->getData()); |
There was a problem hiding this comment.
[nitpick] Reusing or caching the wrapper for the source memory instead of creating a new shared pointer on every call might help further optimize performance if this code path is frequently executed.
Copilot uses AI. Check for mistakes.
…gemanipv2_optimization [no ci]
…gemanipv2_optimization [no ci]
…gemanipv2_optimization
…i-core into imagemanipv2_optimization
There was a problem hiding this comment.
Pull Request Overview
This PR optimizes and refactors ImageManipV2 to improve performance for simpler transformations by reducing reliance on warp affine. Key changes include:
- Removal of setRunOnHost(true) from one test to align with updated node behavior.
- Addition of new backend configuration methods (setBackendCPU and setBackendHW) along with property serialization updates.
- Refactored core image manipulation operations with revised lambda signatures and an added warp backend template parameter.
Reviewed Changes
Copilot reviewed 8 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/src/ondevice_tests/pipeline/node/image_manip_v2_test.cpp | Removed redundant runOnHost setting to reflect optimized behavior. |
| tests/src/ondevice_tests/pipeline/node/image_manip_v2_optimization_test.cpp | Added comprehensive tests comparing host and device implementations across various transformation modes. |
| src/pipeline/node/ImageManipV2.cpp | Refactored manipulation logic to include a new warp backend and updated lambda signatures. |
| include/depthai/properties/ImageManipPropertiesV2.hpp | Introduced Backend enum and updated serialization to include the new backend property. |
| include/depthai/pipeline/node/ImageManipV2.hpp | Exposed new backend setter functions in the public API. |
| bindings/python/src/pipeline/node/ImageManipV2Bindings.cpp | Added Python bindings for the newly introduced backend setter methods. |
Files not reviewed (3)
- cmake/Depthai/DepthaiDeviceRVC4Config.cmake: Language not supported
- cmake/Depthai/DepthaiDeviceSideConfig.cmake: Language not supported
- tests/CMakeLists.txt: Language not supported
Comments suppressed due to low confidence (2)
src/pipeline/node/ImageManipV2.cpp:13
- Ensure that the addition of the 'impl::WarpH' template parameter does not introduce unintended behavior in transformations that do not require warp operations. Please verify that each transformation mode is correctly handled with this new backend.
impl::ImageManipOperations<impl::_ImageManipBuffer, impl::_ImageManipMemory, impl::WarpH> manip(properties, logger);
src/pipeline/node/ImageManipV2.cpp:25
- Review the performance implications of wrapping the source memory in a new shared pointer for every call. Consider reusing buffers or optimizing this memory handling for high-frequency processing scenarios.
auto srcMem = std::make_shared<impl::_ImageManipMemory>(src->getData());
…gemanipv2_optimization
…gemanipv2_optimization
…gemanipv2_optimization
…gemanipv2_optimization
moratom
reviewed
May 7, 2025
Comment on lines
+37
to
+38
| .def("setBackendCPU", &ImageManipV2::setBackendCPU, DOC(dai, node, ImageManipV2, setBackendCPU)) | ||
| .def("setBackendHW", &ImageManipV2::setBackendHW, DOC(dai, node, ImageManipV2, setBackendHW)) |
There was a problem hiding this comment.
I'd go with just "setBackend()" and have an enum here.
Comment on lines
+76
to
+85
| /** | ||
| * Set CPU as backend preference | ||
| */ | ||
| ImageManipV2& setBackendCPU(); | ||
|
|
||
| /** | ||
| * Set HW as backend preference | ||
| */ | ||
| ImageManipV2& setBackendHW(); | ||
|
|
There was a problem hiding this comment.
setBackend(...) for consistency
Comment on lines
13
to
15
| enum class Backend : uint8_t { CPU, HW }; | ||
| enum class PerformanceMode : uint8_t { AUTO, PERFORMANCE, BALANCED, LOW_POWER }; | ||
|
|
There was a problem hiding this comment.
Let's document how each of the mode behaves (what's used for different operations, etc).
Yot down the platform differences as well, so it's clear that this doesn't take any effect on RVC2.
Comment on lines
+21
to
+28
| #ifndef DEPTHAI_STRIDE_ALIGNMENT | ||
| #define DEPTHAI_STRIDE_ALIGNMENT 128 | ||
| #endif | ||
| #ifndef DEPTHAI_HEIGHT_ALIGNMENT | ||
| #define DEPTHAI_HEIGHT_ALIGNMENT 32 | ||
| #endif | ||
| #ifndef DEPTHAI_PLANE_ALIGNMENT | ||
| #define DEPTHAI_PLANE_ALIGNMENT 128 * 32 |
There was a problem hiding this comment.
Does this mean that even RGB888p/i are strided again?
| @@ -29,7 +38,10 @@ | |||
| #define DEPTHAI_IMAGEMANIPV2_OPENCV 1 | |||
| #include <opencv2/opencv.hpp> | |||
| #endif | |||
|
|
|||
| #ifdef DEPTHAI_HAVE_FASTCV_SUPPORT | |||
| // #define DEPTHAI_IMAGEMANIPV2_FASTCV 1 | |||
…gemanipv2_optimization [no ci]
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Purpose
Currently ImageManipV2 on RVC4 uses warp affine even for simpler transformations like cropping and scaling. This PR improves the performance by optimizing these use cases which required refactoring in the ImageManipV2 code.
Specification
None / not applicable
Dependencies & Potential Impact
None / not applicable
Deployment Plan
None / not applicable
Testing & Validation
None / not applicable