Fix build when DEPTHAI_MERGED_TARGET is OFF - #1896
Open
asahtik wants to merge 8 commits into
Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughOpenCV implementations for segmentation masks, replay/network builders, camera replay, matrix conversions, and image snapshot uploads are moved into dedicated sources. CMake wiring, merged-target guards, non-OpenCV boundaries, and related tests are updated. ChangesOpenCV integration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ReplayVideo
participant Camera
participant NeuralNetwork
participant DetectionNetwork
participant EventsManager
participant FileGroup
ReplayVideo->>Camera: provide replay metadata and output
Camera->>Camera: configure mock ISP
NeuralNetwork->>DetectionNetwork: provide NN archive
DetectionNetwork->>DetectionNetwork: configure detection parser
EventsManager->>FileGroup: build snapshot file group
FileGroup->>EventsManager: return JPEG payloads and checksums
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to fix compilation/link issues when DEPTHAI_MERGED_TARGET is OFF while DEPTHAI_HAVE_OPENCV_SUPPORT is ON, by moving OpenCV-dependent implementations out of core sources and into the OpenCV target, and by tightening build-time gating for RecordReplay examples/APIs.
Changes:
- Moved multiple OpenCV-dependent method implementations (EventsManager, NN build overloads, segmentation helpers) into new
src/opencv/*translation units and added them toTARGET_OPENCV_SOURCES. - Adjusted
RecordReplayvideo-size API exposure to be available only when OpenCV support is enabled, and updated example build gating/messages aroundDEPTHAI_MERGED_TARGET. - Added a Catch2 test to verify
FileData(ImgFrame)encodes a JPEG when OpenCV support is enabled.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/src/onhost_tests/utility/events_manager_test.cpp | Adds OpenCV-gated test validating JPEG encoding for FileData(ImgFrame). |
| src/utility/RecordReplayImpl.hpp | Wraps getVideoSize declaration behind DEPTHAI_HAVE_OPENCV_SUPPORT. |
| src/utility/RecordReplay.cpp | Removes non-OpenCV stub implementation for getVideoSize. |
| src/utility/EventsManager.cpp | Removes ImgFrame-based helpers from core TU; moves checksum helper into FileData class. |
| src/pipeline/node/NeuralNetwork.cpp | Removes OpenCV-only ReplayVideo build overload implementation from core TU. |
| src/pipeline/node/DetectionNetwork.cpp | Removes OpenCV-only ReplayVideo build overload implementation from core TU. |
| src/pipeline/datatype/SegmentationMask.cpp | Removes OpenCV-only cv::Mat-based methods from core TU. |
| src/pipeline/datatype/ImgDetectionsT.cpp | Removes OpenCV-only cv::Mat-based methods from core TU. |
| src/opencv/SegmentationMask.cpp | Adds OpenCV-specific SegmentationMask cv::Mat helpers. |
| src/opencv/NeuralNetwork.cpp | Adds OpenCV-specific NeuralNetwork::build(ReplayVideo, ...) implementation. |
| src/opencv/ImgDetectionsT.cpp | Adds OpenCV-specific ImgDetectionsT segmentation-mask cv::Mat helpers + explicit instantiations. |
| src/opencv/EventsManager.cpp | Adds OpenCV-specific ImgFrame encoding + ImgFrame-based FileGroup/sendSnap helpers. |
| src/opencv/DetectionNetwork.cpp | Adds OpenCV-specific DetectionNetwork::build(ReplayVideo, ...) implementation. |
| include/depthai/utility/EventsManager.hpp | Exposes checksum helper as a FileData private static method; adds <deque> include. |
| include/depthai/pipeline/datatype/ImgDetectionsT.hpp | Adds non-OpenCV “static_assert” stubs for OpenCV-only APIs. |
| examples/cpp/RecordReplay/holistic_replay.cpp | Updates preprocessor error message to mention merged target requirement. |
| examples/cpp/RecordReplay/holistic_record.cpp | Updates preprocessor error message to mention merged target requirement. |
| examples/cpp/RecordReplay/CMakeLists.txt | Builds holistic examples only when DEPTHAI_MERGED_TARGET is enabled. |
| CMakeLists.txt | Extends TARGET_OPENCV_SOURCES with new OpenCV TUs; adjusts opencv-target link/include settings. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+23
to
+35
| const auto outputDirectory = | ||
| std::filesystem::temp_directory_path() / ("depthai_events_manager_" + std::to_string(std::chrono::steady_clock::now().time_since_epoch().count())); | ||
| std::filesystem::create_directory(outputDirectory); | ||
|
|
||
| FileData fileData(frame, "frame"); | ||
| std::array<unsigned char, 2> signature{}; | ||
| const bool fileWritten = fileData.toFile(outputDirectory); | ||
| if(fileWritten) { | ||
| std::ifstream output(outputDirectory / "frame.jpg", std::ios::binary); | ||
| output.read(reinterpret_cast<char*>(signature.data()), signature.size()); | ||
| } | ||
|
|
||
| std::filesystem::remove_all(outputDirectory); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/src/onhost_tests/utility/events_manager_test.cpp (1)
46-106: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winGuard
ImgFramedependent test sections with OpenCV checks.The implementations for the
ImgFrameoverloads ofFileGroup::addFileandFileGroup::addImageDetectionsPairwere moved tosrc/opencv/EventsManager.cppand are now only compiled whenDEPTHAI_OPENCV_SUPPORTis enabled.Since this test file still includes sections invoking these exact methods outside the
#ifdef DEPTHAI_HAVE_OPENCV_SUPPORTguard, compiling the tests without OpenCV support will result in linker errors. Wrap these specific test sections to ensure non-OpenCV configurations can build successfully.🛠️ Proposed fix
- SECTION("addFile with null ImgFrame throws") { - std::shared_ptr<ImgFrame> nullFrame = nullptr; - REQUIRE_THROWS_AS(fileGroup.addFile("test.jpg", nullFrame), std::invalid_argument); - } - - SECTION("addFile with null EncodedFrame throws") { - std::shared_ptr<EncodedFrame> nullFrame = nullptr; - REQUIRE_THROWS_AS(fileGroup.addFile("test.jpg", nullFrame), std::invalid_argument); - } - - SECTION("addFile with null ImgDetections throws") { - std::shared_ptr<ImgDetections> nullDetections = nullptr; - REQUIRE_THROWS_AS(fileGroup.addFile("test.json", nullDetections), std::invalid_argument); - } - - SECTION("addImageDetectionsPair with null ImgFrame throws") { - std::shared_ptr<ImgFrame> nullFrame = nullptr; - auto detections = std::make_shared<ImgDetections>(); - REQUIRE_THROWS_AS(fileGroup.addImageDetectionsPair("test", nullFrame, detections), std::invalid_argument); - } - - SECTION("addImageDetectionsPair with null EncodedFrame throws") { - std::shared_ptr<EncodedFrame> nullFrame = nullptr; - auto detections = std::make_shared<ImgDetections>(); - REQUIRE_THROWS_AS(fileGroup.addImageDetectionsPair("test", nullFrame, detections), std::invalid_argument); - } - - SECTION("addImageDetectionsPair with null ImgDetections throws") { - auto frame = std::make_shared<ImgFrame>(); - frame->setType(ImgFrame::Type::BGR888i).setSize(4, 4); - std::vector<uint8_t> data(4 * 4 * 3, 128); - frame->setData(data); - std::shared_ptr<ImgDetections> nullDetections = nullptr; - REQUIRE_THROWS_AS(fileGroup.addImageDetectionsPair("test", frame, nullDetections), std::invalid_argument); - } -} - -TEST_CASE("FileGroup accepts valid inputs", "[FileGroup][EventsManager]") { - FileGroup fileGroup; - - SECTION("addFile with valid ImgFrame works") { - auto frame = std::make_shared<ImgFrame>(); - frame->setType(ImgFrame::Type::BGR888i).setSize(4, 4); - std::vector<uint8_t> data(4 * 4 * 3, 128); // Gray image - frame->setData(data); - REQUIRE_NOTHROW(fileGroup.addFile("test.jpg", frame)); - } - - SECTION("addFile with valid ImgDetections works") { - auto detections = std::make_shared<ImgDetections>(); - REQUIRE_NOTHROW(fileGroup.addFile("test.json", detections)); - } - - SECTION("addImageDetectionsPair with valid inputs works") { - auto frame = std::make_shared<ImgFrame>(); - frame->setType(ImgFrame::Type::BGR888i).setSize(4, 4); - std::vector<uint8_t> data(4 * 4 * 3, 128); - frame->setData(data); - auto detections = std::make_shared<ImgDetections>(); - REQUIRE_NOTHROW(fileGroup.addImageDetectionsPair("test", frame, detections)); - } - - SECTION("addFile with string data works") { - REQUIRE_NOTHROW(fileGroup.addFile("test.txt", "hello world", "text/plain")); - } +#ifdef DEPTHAI_HAVE_OPENCV_SUPPORT + SECTION("addFile with null ImgFrame throws") { + std::shared_ptr<ImgFrame> nullFrame = nullptr; + REQUIRE_THROWS_AS(fileGroup.addFile("test.jpg", nullFrame), std::invalid_argument); + } +#endif + + SECTION("addFile with null EncodedFrame throws") { + std::shared_ptr<EncodedFrame> nullFrame = nullptr; + REQUIRE_THROWS_AS(fileGroup.addFile("test.jpg", nullFrame), std::invalid_argument); + } + + SECTION("addFile with null ImgDetections throws") { + std::shared_ptr<ImgDetections> nullDetections = nullptr; + REQUIRE_THROWS_AS(fileGroup.addFile("test.json", nullDetections), std::invalid_argument); + } + +#ifdef DEPTHAI_HAVE_OPENCV_SUPPORT + SECTION("addImageDetectionsPair with null ImgFrame throws") { + std::shared_ptr<ImgFrame> nullFrame = nullptr; + auto detections = std::make_shared<ImgDetections>(); + REQUIRE_THROWS_AS(fileGroup.addImageDetectionsPair("test", nullFrame, detections), std::invalid_argument); + } +#endif + + SECTION("addImageDetectionsPair with null EncodedFrame throws") { + std::shared_ptr<EncodedFrame> nullFrame = nullptr; + auto detections = std::make_shared<ImgDetections>(); + REQUIRE_THROWS_AS(fileGroup.addImageDetectionsPair("test", nullFrame, detections), std::invalid_argument); + } + +#ifdef DEPTHAI_HAVE_OPENCV_SUPPORT + SECTION("addImageDetectionsPair with null ImgDetections throws") { + auto frame = std::make_shared<ImgFrame>(); + frame->setType(ImgFrame::Type::BGR888i).setSize(4, 4); + std::vector<uint8_t> data(4 * 4 * 3, 128); + frame->setData(data); + std::shared_ptr<ImgDetections> nullDetections = nullptr; + REQUIRE_THROWS_AS(fileGroup.addImageDetectionsPair("test", frame, nullDetections), std::invalid_argument); + } +#endif +} + +TEST_CASE("FileGroup accepts valid inputs", "[FileGroup][EventsManager]") { + FileGroup fileGroup; + +#ifdef DEPTHAI_HAVE_OPENCV_SUPPORT + SECTION("addFile with valid ImgFrame works") { + auto frame = std::make_shared<ImgFrame>(); + frame->setType(ImgFrame::Type::BGR888i).setSize(4, 4); + std::vector<uint8_t> data(4 * 4 * 3, 128); // Gray image + frame->setData(data); + REQUIRE_NOTHROW(fileGroup.addFile("test.jpg", frame)); + } +#endif + + SECTION("addFile with valid ImgDetections works") { + auto detections = std::make_shared<ImgDetections>(); + REQUIRE_NOTHROW(fileGroup.addFile("test.json", detections)); + } + +#ifdef DEPTHAI_HAVE_OPENCV_SUPPORT + SECTION("addImageDetectionsPair with valid inputs works") { + auto frame = std::make_shared<ImgFrame>(); + frame->setType(ImgFrame::Type::BGR888i).setSize(4, 4); + std::vector<uint8_t> data(4 * 4 * 3, 128); + frame->setData(data); + auto detections = std::make_shared<ImgDetections>(); + REQUIRE_NOTHROW(fileGroup.addImageDetectionsPair("test", frame, detections)); + } +#endif + + SECTION("addFile with string data works") { + REQUIRE_NOTHROW(fileGroup.addFile("test.txt", "hello world", "text/plain")); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/src/onhost_tests/utility/events_manager_test.cpp` around lines 46 - 106, Guard the ImgFrame-dependent sections in the FileGroup validation tests with the existing OpenCV availability macro, including both addFile and addImageDetectionsPair null/valid cases. Leave the EncodedFrame and ImgDetections-only sections unguarded so they continue to run without OpenCV support.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/opencv/NeuralNetwork.cpp`:
- Around line 13-15: Update the capability assignments in NeuralNetwork
construction to check that cap.type and cap.size contain values before calling
value() or extracting the size pair. Handle missing capabilities safely by
skipping or otherwise safely handling those assignments, while preserving the
existing optional fps guard.
In `@src/opencv/SegmentationMask.cpp`:
- Around line 56-58: The custom allocator is not applied to output masks before
allocation. In src/opencv/SegmentationMask.cpp lines 56-58, update
getCvMaskByIndex to assign allocator to indexedMask before cv::compare; in
src/opencv/ImgDetectionsT.cpp lines 58-60, assign allocator to classMask before
cv::compare; and in src/opencv/ImgDetectionsT.cpp line 68, replace
cv::Mat::zeros in getCvSegmentationMaskByClass with manual construction,
allocator assignment, create(), and setTo(255).
- Around line 16-24: Fix continuous cv::Mat copying in
src/opencv/SegmentationMask.cpp lines 16-24 by reserving mask.total() *
mask.elemSize() bytes and using mask.data through that exact byte count instead
of datastart/dataend; retain the row-wise copy for non-continuous masks. Apply
the identical correction to src/opencv/ImgDetectionsT.cpp lines 16-24.
---
Outside diff comments:
In `@tests/src/onhost_tests/utility/events_manager_test.cpp`:
- Around line 46-106: Guard the ImgFrame-dependent sections in the FileGroup
validation tests with the existing OpenCV availability macro, including both
addFile and addImageDetectionsPair null/valid cases. Leave the EncodedFrame and
ImgDetections-only sections unguarded so they continue to run without OpenCV
support.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 767831c5-0252-4c9b-b9e3-4f99da69c174
📒 Files selected for processing (19)
CMakeLists.txtexamples/cpp/RecordReplay/CMakeLists.txtexamples/cpp/RecordReplay/holistic_record.cppexamples/cpp/RecordReplay/holistic_replay.cppinclude/depthai/pipeline/datatype/ImgDetectionsT.hppinclude/depthai/utility/EventsManager.hppsrc/opencv/DetectionNetwork.cppsrc/opencv/EventsManager.cppsrc/opencv/ImgDetectionsT.cppsrc/opencv/NeuralNetwork.cppsrc/opencv/SegmentationMask.cppsrc/pipeline/datatype/ImgDetectionsT.cppsrc/pipeline/datatype/SegmentationMask.cppsrc/pipeline/node/DetectionNetwork.cppsrc/pipeline/node/NeuralNetwork.cppsrc/utility/EventsManager.cppsrc/utility/RecordReplay.cppsrc/utility/RecordReplayImpl.hpptests/src/onhost_tests/utility/events_manager_test.cpp
💤 Files with no reviewable changes (5)
- src/utility/RecordReplay.cpp
- src/pipeline/node/DetectionNetwork.cpp
- src/pipeline/node/NeuralNetwork.cpp
- src/pipeline/datatype/ImgDetectionsT.cpp
- src/pipeline/datatype/SegmentationMask.cpp
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: copilot-pull-request-reviewer
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-05-28T13:36:26.383Z
Learnt from: moratom
Repo: luxonis/depthai-core PR: 1812
File: examples/cpp/ImageManip/image_manip_remap.cpp:51-52
Timestamp: 2026-05-28T13:36:26.383Z
Learning: In depthai-core example code, do not set `ImageManip::Backend::GPU` unconditionally. The GPU backend is only available on RVC4 (not RVC2). Prefer leaving the backend as the default, or comment out the GPU backend selection and add a clear note explaining it is RVC4-only support (so the example won’t fail or mislead on RVC2).
Applied to files:
examples/cpp/RecordReplay/holistic_replay.cppexamples/cpp/RecordReplay/holistic_record.cpp
🔇 Additional comments (10)
include/depthai/pipeline/datatype/ImgDetectionsT.hpp (1)
103-124: LGTM!examples/cpp/RecordReplay/holistic_record.cpp (1)
11-11: LGTM!src/utility/RecordReplayImpl.hpp (1)
167-169: LGTM!CMakeLists.txt (1)
1026-1027: 📐 Maintainability & Code QualityDrop the
include/depthaiconcern.TARGET_OPENCV_NAMEalready inherits the publicincludepath fromTARGET_CORE_NAME, and the privateinclude/depthaientries are needed for internal includes likebuild/version.hppand the generatedinclude/depthai/build/version.hpplayout.> Likely an incorrect or invalid review comment.src/opencv/DetectionNetwork.cpp (1)
7-13: LGTM!examples/cpp/RecordReplay/CMakeLists.txt (1)
30-37: LGTM!examples/cpp/RecordReplay/holistic_replay.cpp (1)
6-6: LGTM!include/depthai/utility/EventsManager.hpp (1)
5-5: LGTM!Also applies to: 42-43
src/opencv/EventsManager.cpp (1)
20-35: LGTM!Also applies to: 37-56, 58-74
src/utility/EventsManager.cpp (1)
79-115: LGTM!Also applies to: 127-136, 146-186, 787-846
Comment on lines
+13
to
+15
| input->setOutFrameType(cap.type.value()); | ||
| if(fps.has_value()) input->setFps(*fps); | ||
| input->setSize(std::get<std::pair<unsigned int, unsigned int>>(cap.size.value.value())); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add safety guards before unwrapping optional capability values.
Calling .value() on an empty std::optional will throw a std::bad_optional_access exception. If getFrameCapability fails to populate type or size (e.g., due to a malformed nnArchive), this will crash the pipeline builder. Guard the assignments to handle missing capabilities safely.
🛡️ Proposed fix
- input->setOutFrameType(cap.type.value());
- if(fps.has_value()) input->setFps(*fps);
- input->setSize(std::get<std::pair<unsigned int, unsigned int>>(cap.size.value.value()));
+ if(cap.type) {
+ input->setOutFrameType(cap.type.value());
+ }
+ if(fps.has_value()) {
+ input->setFps(*fps);
+ }
+ if(cap.size && cap.size.value.has_value()) {
+ input->setSize(std::get<std::pair<unsigned int, unsigned int>>(cap.size.value.value()));
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| input->setOutFrameType(cap.type.value()); | |
| if(fps.has_value()) input->setFps(*fps); | |
| input->setSize(std::get<std::pair<unsigned int, unsigned int>>(cap.size.value.value())); | |
| if(cap.type) { | |
| input->setOutFrameType(cap.type.value()); | |
| } | |
| if(fps.has_value()) { | |
| input->setFps(*fps); | |
| } | |
| if(cap.size && cap.size.value.has_value()) { | |
| input->setSize(std::get<std::pair<unsigned int, unsigned int>>(cap.size.value.value())); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/opencv/NeuralNetwork.cpp` around lines 13 - 15, Update the capability
assignments in NeuralNetwork construction to check that cap.type and cap.size
contain values before calling value() or extracting the size pair. Handle
missing capabilities safely by skipping or otherwise safely handling those
assignments, while preserving the existing optional fps guard.
Comment on lines
+16
to
+24
| std::vector<std::uint8_t> dataVec; | ||
| if(!mask.isContinuous()) { | ||
| for(int i = 0; i < mask.rows; i++) { | ||
| dataVec.insert(dataVec.end(), mask.ptr(i), mask.ptr(i) + mask.cols * mask.elemSize()); | ||
| } | ||
| } else { | ||
| dataVec.insert(dataVec.begin(), mask.datastart, mask.dataend); | ||
| } | ||
| setData(std::move(dataVec)); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Data corruption and size-mismatch risk when copying continuous cv::Mat ROIs.
When a cv::Mat is a continuous Region of Interest (ROI) sliced from a larger matrix, datastart and dataend point to the limits of the parent allocation, not the ROI itself. Using them to copy data forces the entire parent buffer into the vector, resulting in bloated sizes and throwing runtime exceptions when the actual size is later validated against the expected mask dimensions.
Use mask.data along with the element count instead, and pre-allocate the vector with reserve() to avoid repeated allocations.
src/opencv/SegmentationMask.cpp#L16-L24: Replace withdataVec.reserve(mask.total() * mask.elemSize());followed by the if/else usingmask.dataandmask.data + mask.total() * mask.elemSize()for the continuous branch.src/opencv/ImgDetectionsT.cpp#L16-L24: Apply the exact same correction for copyingmask.data.
🐛 Proposed fix for both files
std::vector<std::uint8_t> dataVec;
+ dataVec.reserve(mask.total() * mask.elemSize());
if(!mask.isContinuous()) {
for(int i = 0; i < mask.rows; i++) {
dataVec.insert(dataVec.end(), mask.ptr(i), mask.ptr(i) + mask.cols * mask.elemSize());
}
} else {
- dataVec.insert(dataVec.begin(), mask.datastart, mask.dataend);
+ dataVec.insert(dataVec.begin(), mask.data, mask.data + mask.total() * mask.elemSize());
}
setData(std::move(dataVec));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| std::vector<std::uint8_t> dataVec; | |
| if(!mask.isContinuous()) { | |
| for(int i = 0; i < mask.rows; i++) { | |
| dataVec.insert(dataVec.end(), mask.ptr(i), mask.ptr(i) + mask.cols * mask.elemSize()); | |
| } | |
| } else { | |
| dataVec.insert(dataVec.begin(), mask.datastart, mask.dataend); | |
| } | |
| setData(std::move(dataVec)); | |
| std::vector<std::uint8_t> dataVec; | |
| dataVec.reserve(mask.total() * mask.elemSize()); | |
| if(!mask.isContinuous()) { | |
| for(int i = 0; i < mask.rows; i++) { | |
| dataVec.insert(dataVec.end(), mask.ptr(i), mask.ptr(i) + mask.cols * mask.elemSize()); | |
| } | |
| } else { | |
| dataVec.insert(dataVec.begin(), mask.data, mask.data + mask.total() * mask.elemSize()); | |
| } | |
| setData(std::move(dataVec)); |
📍 Affects 2 files
src/opencv/SegmentationMask.cpp#L16-L24(this comment)src/opencv/ImgDetectionsT.cpp#L16-L24
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/opencv/SegmentationMask.cpp` around lines 16 - 24, Fix continuous cv::Mat
copying in src/opencv/SegmentationMask.cpp lines 16-24 by reserving mask.total()
* mask.elemSize() bytes and using mask.data through that exact byte count
instead of datastart/dataend; retain the row-wise copy for non-continuous masks.
Apply the identical correction to src/opencv/ImgDetectionsT.cpp lines 16-24.
Comment on lines
+56
to
+58
| cv::Mat indexedMask; | ||
| cv::compare(mask, index, indexedMask, cv::CmpTypes::CMP_EQ); | ||
| return indexedMask; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Custom allocator is ignored for the returned matrices.
The allocator argument is being forwarded to temporary internal matrices, but the final output matrices returned to the user are allocated using the default heap allocator. This defeats the purpose of providing an allocator for custom memory management (such as ensuring zero-copy or real-time constraints). Set the allocator field on the output matrices before they are allocated.
src/opencv/SegmentationMask.cpp#L56-L58: Addif(allocator) indexedMask.allocator = allocator;before callingcv::compare.src/opencv/ImgDetectionsT.cpp#L58-L60: Addif(allocator) classMask.allocator = allocator;before callingcv::compare.src/opencv/ImgDetectionsT.cpp#L68-L68: Avoid usingcv::Mat::zeros, which allocates a new buffer immediately. Instead, manually instantiateclassMask, assign the allocator, callcreate(), andsetTo(255).
🛠️ Proposed fixes for the allocator omissions
For src/opencv/SegmentationMask.cpp (getCvMaskByIndex):
cv::Mat indexedMask;
+ if(allocator) indexedMask.allocator = allocator;
cv::compare(mask, index, indexedMask, cv::CmpTypes::CMP_EQ);
return indexedMask;For src/opencv/ImgDetectionsT.cpp (getCvSegmentationMaskByIndex):
cv::Mat classMask;
+ if(allocator) classMask.allocator = allocator;
cv::compare(*mask, index, classMask, cv::CmpTypes::CMP_EQ);
return classMask;For src/opencv/ImgDetectionsT.cpp (getCvSegmentationMaskByClass):
- cv::Mat classMask = cv::Mat::zeros(mask->size(), CV_8UC1) + 255;
+ cv::Mat classMask;
+ if(allocator) classMask.allocator = allocator;
+ classMask.create(mask->size(), CV_8UC1);
+ classMask.setTo(255);
for(uint8_t idx = 0; idx < detections.size(); idx++) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| cv::Mat indexedMask; | |
| cv::compare(mask, index, indexedMask, cv::CmpTypes::CMP_EQ); | |
| return indexedMask; | |
| cv::Mat indexedMask; | |
| if(allocator) indexedMask.allocator = allocator; | |
| cv::compare(mask, index, indexedMask, cv::CmpTypes::CMP_EQ); | |
| return indexedMask; |
📍 Affects 2 files
src/opencv/SegmentationMask.cpp#L56-L58(this comment)src/opencv/ImgDetectionsT.cpp#L58-L60src/opencv/ImgDetectionsT.cpp#L68-L68
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/opencv/SegmentationMask.cpp` around lines 56 - 58, The custom allocator
is not applied to output masks before allocation. In
src/opencv/SegmentationMask.cpp lines 56-58, update getCvMaskByIndex to assign
allocator to indexedMask before cv::compare; in src/opencv/ImgDetectionsT.cpp
lines 58-60, assign allocator to classMask before cv::compare; and in
src/opencv/ImgDetectionsT.cpp line 68, replace cv::Mat::zeros in
getCvSegmentationMaskByClass with manual construction, allocator assignment,
create(), and setTo(255).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/opencv/matrixOps.cpp`:
- Around line 7-18: Update cvMatToMatrix3x3 to validate cvMat.type() == CV_32F
before the element-access loops, alongside the existing 3x3 dimension check.
Reject incompatible matrix types with the function’s existing invalid-argument
error path before calling cvMat.at<float>.
- Around line 20-31: Update cvMatToMatrix4x4 to validate that cvMat.type()
equals CV_32F, alongside the existing 4x4 dimension checks, before accessing
elements with cvMat.at<float>. Reject mismatched types using the function’s
existing invalid-argument validation path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a6a2fd93-ccad-498d-bbf0-583626045c70
📒 Files selected for processing (9)
CMakeLists.txtexamples/cpp/AutoCalibration/CMakeLists.txtexamples/cpp/DynamicCalibration/CMakeLists.txtinclude/depthai/pipeline/node/ToF.hppsrc/opencv/Camera.cppsrc/opencv/matrixOps.cppsrc/pipeline/node/Camera.cppsrc/pipeline/node/ToF.cppsrc/utility/matrixOps.cpp
💤 Files with no reviewable changes (3)
- include/depthai/pipeline/node/ToF.hpp
- src/pipeline/node/Camera.cpp
- src/utility/matrixOps.cpp
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-03-24T22:39:04.364Z
Learnt from: MaticTonin
Repo: luxonis/depthai-core PR: 1732
File: src/pipeline/Pipeline.cpp:705-705
Timestamp: 2026-03-24T22:39:04.364Z
Learning: Do not flag the `!= ""` part of the auto-calibration condition as redundant when it appears in `PipelineImpl::build()` (or closely related pipeline build logic). If the code uses `utility::getEnvAs<std::string>(..., default)` with a default such as `"ON_START"`, the explicit empty-string guard may still be intentional to treat an explicitly empty env var as “OFF/disabled” (or to avoid special-casing elsewhere). Only consider removing `!= ""` if the codebase has an explicit, enforceable guarantee that `DEPTHAI_AUTOCALIBRATION` can never be set to an empty string (e.g., via validated parsing/CI checks); otherwise, keep the guard.
Applied to files:
src/pipeline/node/ToF.cpp
🪛 Cppcheck (2.21.0)
src/opencv/matrixOps.cpp
[error] 43-43: There is an unknown macro here somewhere. Configuration is required. If DEPTHAI_NLOHMANN_DEFINE_TYPE_NON_INTRUSIVE is a macro then please configure it.
(unknownMacro)
src/opencv/Camera.cpp
[error] 33-33: There is an unknown macro here somewhere. Configuration is required. If DEPTHAI_NLOHMANN_DEFINE_TYPE_INTRUSIVE is a macro then please configure it.
(unknownMacro)
🔇 Additional comments (5)
CMakeLists.txt (1)
456-464: LGTM!src/opencv/Camera.cpp (1)
10-43: LGTM!examples/cpp/AutoCalibration/CMakeLists.txt (1)
4-6: LGTM!examples/cpp/DynamicCalibration/CMakeLists.txt (1)
7-12: LGTM!src/pipeline/node/ToF.cpp (1)
18-41: LGTM!
Comment on lines
+7
to
+18
| std::array<std::array<float, 3>, 3> cvMatToMatrix3x3(const cv::Mat& cvMat) { | ||
| if(cvMat.rows != 3 || cvMat.cols != 3) { | ||
| throw std::invalid_argument("Expected a 3x3 cv::Mat to convert to 3x3 matrix."); | ||
| } | ||
| std::array<std::array<float, 3>, 3> matrix; | ||
| for(size_t i = 0; i < 3; ++i) { | ||
| for(size_t j = 0; j < 3; ++j) { | ||
| matrix[i][j] = cvMat.at<float>(i, j); | ||
| } | ||
| } | ||
| return matrix; | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate the cv::Mat type before accessing its elements.
cv::Mat::at<float> strictly requires the matrix type to be CV_32F. If a matrix of a different type (like CV_64F or CV_8U) is provided, OpenCV will trigger a fatal assertion in debug builds and silently corrupt the data in release builds by misinterpreting the memory layout.
Please validate cvMat.type() == CV_32F before converting.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/opencv/matrixOps.cpp` around lines 7 - 18, Update cvMatToMatrix3x3 to
validate cvMat.type() == CV_32F before the element-access loops, alongside the
existing 3x3 dimension check. Reject incompatible matrix types with the
function’s existing invalid-argument error path before calling cvMat.at<float>.
Comment on lines
+20
to
+31
| std::array<std::array<float, 4>, 4> cvMatToMatrix4x4(const cv::Mat& cvMat) { | ||
| if(cvMat.rows != 4 || cvMat.cols != 4) { | ||
| throw std::invalid_argument("Expected a 4x4 cv::Mat to convert to 4x4 matrix."); | ||
| } | ||
| std::array<std::array<float, 4>, 4> matrix; | ||
| for(size_t i = 0; i < 4; ++i) { | ||
| for(size_t j = 0; j < 4; ++j) { | ||
| matrix[i][j] = cvMat.at<float>(i, j); | ||
| } | ||
| } | ||
| return matrix; | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate the cv::Mat type before accessing its elements.
Similar to the 3x3 conversion, cvMat.at<float> requires the matrix type to be CV_32F. A type mismatch will cause assertions or silent memory corruption.
Please validate cvMat.type() == CV_32F before converting.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/opencv/matrixOps.cpp` around lines 20 - 31, Update cvMatToMatrix4x4 to
validate that cvMat.type() equals CV_32F, alongside the existing 4x4 dimension
checks, before accessing elements with cvMat.at<float>. Reject mismatched types
using the function’s existing invalid-argument validation path.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/CMakeLists.txt`:
- Around line 583-586: Guard all OpenCV-dependent test blocks with the resolved
dependency flag DEPTHAI_HAVE_OPENCV_SUPPORT instead of DEPTHAI_OPENCV_SUPPORT.
Update tests/CMakeLists.txt sites 583-586 and 686-687 accordingly, and change
site 820 to require both DEPTHAI_FETCH_ARTIFACTS and
DEPTHAI_HAVE_OPENCV_SUPPORT.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 341c5894-c1da-4f21-bb5a-f99b5ec54614
📒 Files selected for processing (36)
cmake/depthaiOptions.cmakeinclude/depthai/pipeline/MessageQueue.hppinclude/depthai/pipeline/Node.hppinclude/depthai/pipeline/datatype/AutoCalibrationResult.hppinclude/depthai/pipeline/datatype/GateControl.hppinclude/depthai/utility/LockingQueue.hppinclude/depthai/utility/MemoryWrappers.hppinclude/depthai/utility/NlohmannJsonCompat.hppsrc/device/DeviceBase.cppsrc/pipeline/Node.cppsrc/pipeline/node/DetectionNetwork.cppsrc/utility/MemoryWrappers.cppsrc/utility/ObjectTrackerImpl.cppsrc/utility/Platform.cpptests/CMakeLists.txttests/src/ondevice_tests/encoded_frame_test.cpptests/src/ondevice_tests/filesystem_test.cpptests/src/ondevice_tests/img_transformation_test.cpptests/src/ondevice_tests/neural_depth_node_test.cpptests/src/ondevice_tests/pipeline/node/detection_parser_test.cpptests/src/ondevice_tests/pipeline/node/gate_node_tests.cpptests/src/ondevice_tests/pipeline/node/neural_assisted_stereo_node_test.cpptests/src/ondevice_tests/pipeline/node/neural_network_node_test.cpptests/src/ondevice_tests/pipeline/node/object_tracker_test.cpptests/src/ondevice_tests/pipeline/node/spatial_location_calculator_test.cpptests/src/ondevice_tests/pipeline_debugging_rvc2_test.cpptests/src/ondevice_tests/pipeline_debugging_rvc4_test.cpptests/src/ondevice_tests/resolutions_test.cpptests/src/ondevice_tests/video_encoder_test.cpptests/src/ondevice_tests/xlink_test.cpptests/src/onhost_tests/image_transformations_test.cpptests/src/onhost_tests/multi_device_fsync_test.cpptests/src/onhost_tests/multi_device_ptp_test.cpptests/src/onhost_tests/pipeline/datatype/imgframe_test.cpptests/src/onhost_tests/pipeline/node/internal/XLinkInHostTest.cpptests/src/onhost_tests/utility/events_manager_test.cpp
💤 Files with no reviewable changes (1)
- tests/src/ondevice_tests/pipeline/node/gate_node_tests.cpp
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-03-24T22:39:04.364Z
Learnt from: MaticTonin
Repo: luxonis/depthai-core PR: 1732
File: src/pipeline/Pipeline.cpp:705-705
Timestamp: 2026-03-24T22:39:04.364Z
Learning: Do not flag the `!= ""` part of the auto-calibration condition as redundant when it appears in `PipelineImpl::build()` (or closely related pipeline build logic). If the code uses `utility::getEnvAs<std::string>(..., default)` with a default such as `"ON_START"`, the explicit empty-string guard may still be intentional to treat an explicitly empty env var as “OFF/disabled” (or to avoid special-casing elsewhere). Only consider removing `!= ""` if the codebase has an explicit, enforceable guarantee that `DEPTHAI_AUTOCALIBRATION` can never be set to an empty string (e.g., via validated parsing/CI checks); otherwise, keep the guard.
Applied to files:
src/pipeline/Node.cppsrc/pipeline/node/DetectionNetwork.cpp
🔇 Additional comments (38)
include/depthai/pipeline/MessageQueue.hpp (1)
65-72: LGTM!include/depthai/pipeline/datatype/GateControl.hpp (1)
29-29: LGTM!src/device/DeviceBase.cpp (1)
413-414: LGTM!src/pipeline/Node.cpp (1)
336-336: LGTM!Also applies to: 369-369
src/pipeline/node/DetectionNetwork.cpp (1)
31-32: LGTM!Also applies to: 70-71
src/utility/ObjectTrackerImpl.cpp (1)
212-212: LGTM!include/depthai/pipeline/Node.hpp (1)
101-103: LGTM!include/depthai/pipeline/datatype/AutoCalibrationResult.hpp (1)
25-25: LGTM!include/depthai/utility/LockingQueue.hpp (1)
31-32: LGTM!include/depthai/utility/MemoryWrappers.hpp (1)
5-5: LGTM!include/depthai/utility/NlohmannJsonCompat.hpp (1)
6-7: LGTM!src/utility/MemoryWrappers.cpp (1)
5-5: LGTM!src/utility/Platform.cpp (1)
85-85: LGTM!Also applies to: 417-417, 442-442
tests/src/ondevice_tests/resolutions_test.cpp (1)
11-11: LGTM!Also applies to: 23-29
tests/src/ondevice_tests/video_encoder_test.cpp (1)
5-10: LGTM!tests/src/onhost_tests/multi_device_fsync_test.cpp (1)
11-11: LGTM!tests/src/onhost_tests/multi_device_ptp_test.cpp (1)
11-11: LGTM!tests/src/onhost_tests/utility/events_manager_test.cpp (1)
1-116: LGTM!tests/src/ondevice_tests/img_transformation_test.cpp (1)
19-30: LGTM!Also applies to: 247-401, 716-717, 904-904
tests/src/ondevice_tests/neural_depth_node_test.cpp (1)
104-105: LGTM!Also applies to: 243-244
tests/src/ondevice_tests/pipeline/node/detection_parser_test.cpp (1)
227-228: LGTM!Also applies to: 358-359, 497-498, 547-548
tests/src/ondevice_tests/pipeline/node/neural_assisted_stereo_node_test.cpp (1)
10-11: LGTM!Also applies to: 72-73
tests/src/ondevice_tests/pipeline/node/neural_network_node_test.cpp (1)
4-7: LGTM!Also applies to: 55-56, 140-200, 261-262
cmake/depthaiOptions.cmake (1)
43-47: LGTM!tests/CMakeLists.txt (3)
321-323: LGTM!
854-854: LGTM!
876-876: LGTM!tests/src/ondevice_tests/encoded_frame_test.cpp (2)
5-8: LGTM!
33-34: LGTM!Also applies to: 92-93
tests/src/ondevice_tests/filesystem_test.cpp (1)
8-8: LGTM!tests/src/onhost_tests/pipeline/datatype/imgframe_test.cpp (1)
13-14: LGTM!tests/src/onhost_tests/pipeline/node/internal/XLinkInHostTest.cpp (1)
100-101: LGTM!Also applies to: 239-240
tests/src/ondevice_tests/pipeline/node/object_tracker_test.cpp (1)
21-22: LGTM!Also applies to: 111-112
tests/src/ondevice_tests/pipeline/node/spatial_location_calculator_test.cpp (1)
29-30: LGTM!Also applies to: 56-57, 213-214, 1007-1008
tests/src/ondevice_tests/pipeline_debugging_rvc2_test.cpp (1)
13-14: LGTM!Also applies to: 81-82
tests/src/ondevice_tests/pipeline_debugging_rvc4_test.cpp (1)
13-14: LGTM!Also applies to: 83-84
tests/src/ondevice_tests/xlink_test.cpp (1)
74-75: LGTM!Also applies to: 214-215
tests/src/onhost_tests/image_transformations_test.cpp (1)
490-491: LGTM!Also applies to: 525-526
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmake/depthaiOptions.cmake`:
- Around line 81-85: Move the DEPTHAI_DYNAMIC_CALIBRATION_SUPPORT auto-disable
block to execute after DEPTHAI_MERGED_TARGET normalization and before the 32-bit
validation. Preserve its existing condition, warning, and cache update so 32-bit
configurations with DEPTHAI_MERGED_TARGET disabled are adjusted before
validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 839518b3-d2e6-4275-a7b6-848f333b6f51
📒 Files selected for processing (2)
cmake/depthaiOptions.cmaketests/CMakeLists.txt
📜 Review details
🔇 Additional comments (2)
cmake/depthaiOptions.cmake (1)
43-46: LGTM!tests/CMakeLists.txt (1)
321-324: LGTM!Also applies to: 583-606, 686-690, 820-823, 854-854, 876-876, 910-910
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
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
Fixes compiler and linker errors when DEPTHAI_MERGED_TARGET option is off, but DEPTHAI_OPENCV_SUPPORT is ON.
Specification
Had to move a bunch of functions and methods from core implementations to their opencv/ counterparts, added ifdefs and various cmake fixes.
When building examples with opencv support disabled configuration now outputs a warning and disables building examples (would a fatal error be better?).
Dynamic calibration is now only supported with the merged target. While it would be possible to make it work for the unmerged opencv target, I believe the effort for this refactor is out of the scope of this PR. Dynamic calibration now gets disabled if merged target is off (would a fatal error be better?)
Tests should now build even with opencv support disabled (some tests are omitted).
Dependencies & Potential Impact
None / not applicable
Deployment Plan
None / not applicable
Testing & Validation
None / not applicable
AI Usage
Assisted-by: AGENT_NAME:MODEL_VERSION [TOOL1] [TOOL2]
Submitted code was reviewed by a human: YES/NO
The author is taking the responsibility for the contribution: YES/NO
vvv this CodeRabbit summary is wrong - it picked up changes that were simply copied to opencv/ implementations
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Tests