Add load && save method to ImgFrame - #1852
Conversation
|
Warning Review limit reached
Next review available in: 12 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdds protobuf-gated ChangesProtobuf File Persistence
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@bindings/python/tests/imgframe_test.py`:
- Line 187: The assertion checking for an empty payload uses the `.size`
attribute which is not available on all possible Python container types returned
by getData(). Replace the emptiness check on recovered.getData().size == 0 with
len(recovered.getData()) == 0 to use a container-agnostic approach that works
with any standard Python container type.
In `@src/opencv/ImgFrame.cpp`:
- Around line 66-70: The function parseImgFrameBytes creates an unnecessary copy
of the input parameter serialized into packetBytes before assigning it to the
packet structure. Remove the intermediate packetBytes copy and instead directly
use serialized.data() and serialized.size() when assigning packet.data and
packet.length. Apply the same optimization to any other instances in the file
that perform similar redundant buffer copies (referenced at lines 106-108) to
eliminate the avoidable memory overhead and CPU cost on large frames.
- Around line 102-108: The methods ImgFrame::save and ImgFrame::load are defined
only when DEPTHAI_HAVE_OPENCV_SUPPORT is enabled, but their declarations in the
header file are unconditional, causing linker errors when OpenCV is disabled.
Either guard the declarations of save and load methods in the ImgFrame header
with `#ifdef` DEPTHAI_HAVE_OPENCV_SUPPORT preprocessing directives, or provide
stub implementations (that throw or return appropriate error values) for the
non-OpenCV build path to ensure the symbols are available in all configurations.
🪄 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: 773f4adb-6c44-424b-bcef-fc616bc3fe8d
📒 Files selected for processing (4)
bindings/python/src/pipeline/datatype/ImgFrameBindings.cppbindings/python/tests/imgframe_test.pyinclude/depthai/pipeline/datatype/ImgFrame.hppsrc/opencv/ImgFrame.cpp
📜 Review details
🔇 Additional comments (2)
include/depthai/pipeline/datatype/ImgFrame.hpp (1)
4-4: LGTM!Also applies to: 363-377
bindings/python/src/pipeline/datatype/ImgFrameBindings.cpp (1)
324-334: No action needed. Thepybind11/stl/filesystem.hheader is already available through the transitive include chain:ImgFrameBindings.cppincludesDatatypeBindings.hpp, which includespybind11_common.hpp, which includes<pybind11/stl/filesystem.h>at line 14. Thestd::filesystem::pathcaster is properly available for thesaveandloadbindings.
| recovered.load(path, metadataOnly=True) | ||
|
|
||
| assert_frame_metadata_equal(frame, recovered) | ||
| assert recovered.getData().size == 0 |
There was a problem hiding this comment.
Use a container-agnostic emptiness check for payload.
The assertion currently depends on a .size attribute, which may not exist for all getData() Python return shapes. Use len(...) == 0 to avoid false test failures.
✅ Proposed fix
- assert recovered.getData().size == 0
+ assert len(recovered.getData()) == 0📝 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.
| assert recovered.getData().size == 0 | |
| assert len(recovered.getData()) == 0 |
🤖 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 `@bindings/python/tests/imgframe_test.py` at line 187, The assertion checking
for an empty payload uses the `.size` attribute which is not available on all
possible Python container types returned by getData(). Replace the emptiness
check on recovered.getData().size == 0 with len(recovered.getData()) == 0 to use
a container-agnostic approach that works with any standard Python container
type.
| std::shared_ptr<ImgFrame> parseImgFrameBytes(const std::vector<uint8_t>& serialized, bool metadataOnly) { | ||
| auto packetBytes = serialized; | ||
| streamPacketDesc_t packet{}; | ||
| packet.data = packetBytes.data(); | ||
| packet.length = packetBytes.size(); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Avoid the extra full-buffer copy during load parse.
parseImgFrameBytes copies serialized into packetBytes before parsing. On large frames this doubles peak memory and adds avoidable CPU overhead.
♻️ Proposed change
-std::shared_ptr<ImgFrame> parseImgFrameBytes(const std::vector<uint8_t>& serialized, bool metadataOnly) {
- auto packetBytes = serialized;
+std::shared_ptr<ImgFrame> parseImgFrameBytes(std::vector<uint8_t> packetBytes, bool metadataOnly) {
streamPacketDesc_t packet{};
packet.data = packetBytes.data();
packet.length = packetBytes.size();
packet.fd = -1;
@@
void ImgFrame::load(const std::filesystem::path& path, bool metadataOnly) {
- *this = *parseImgFrameBytes(readBinaryFile(path), metadataOnly);
+ *this = *parseImgFrameBytes(readBinaryFile(path), metadataOnly);
}Also applies to: 106-108
🤖 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/ImgFrame.cpp` around lines 66 - 70, The function
parseImgFrameBytes creates an unnecessary copy of the input parameter serialized
into packetBytes before assigning it to the packet structure. Remove the
intermediate packetBytes copy and instead directly use serialized.data() and
serialized.size() when assigning packet.data and packet.length. Apply the same
optimization to any other instances in the file that perform similar redundant
buffer copies (referenced at lines 106-108) to eliminate the avoidable memory
overhead and CPU cost on large frames.
0e9d4ea to
b148bc3
Compare
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 `@include/depthai/utility/ProtoSerializable.hpp`:
- Around line 20-22: Add Doxygen documentation for ProtoSerializable::save and
ProtoSerializable::load to match the existing serializeProto/serializeSchema
comments, and explicitly document the resolveDataPath behavior that appends a
.dai extension when the provided path has no extension. Mention the metadataOnly
parameter and make it clear that extensionless inputs may be resolved to
<path>.dai so callers understand the final on-disk filename.
- Line 21: The metadata-only path in ProtoSerializable::load leaves existing
payload state attached when reusing objects like ImgFrame, EncodedFrame,
PointCloudData, or RGBDData. Update the load(const std::filesystem::path&, bool
metadataOnly) flow so that when metadataOnly is true it first clears or replaces
the current payload members before applying the new metadata, ensuring stale
buffers/frames are not preserved across reloads.
🪄 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: 9564a817-a4cf-4483-860d-2c14bbe23352
📒 Files selected for processing (21)
bindings/python/src/pipeline/datatype/EncodedFrameBindings.cppbindings/python/src/pipeline/datatype/IMUDataBindings.cppbindings/python/src/pipeline/datatype/ImgFrameBindings.cppbindings/python/src/pipeline/datatype/PointCloudDataBindings.cppbindings/python/src/pipeline/datatype/RGBDDataBindings.cppbindings/python/tests/imgframe_test.pyinclude/depthai/pipeline/datatype/EncodedFrame.hppinclude/depthai/pipeline/datatype/IMUData.hppinclude/depthai/pipeline/datatype/ImgFrame.hppinclude/depthai/pipeline/datatype/PointCloudData.hppinclude/depthai/pipeline/datatype/RGBDData.hppinclude/depthai/utility/ProtoSerializable.hppsrc/opencv/ImgFrame.cppsrc/pipeline/datatype/EncodedFrame.cppsrc/pipeline/datatype/IMUData.cppsrc/pipeline/datatype/ImgFrame.cppsrc/pipeline/datatype/PointCloudData.cppsrc/pipeline/datatype/RGBDData.cppsrc/utility/ProtoSerializable.cpptests/CMakeLists.txttests/src/onhost_tests/pipeline/datatype/proto_file_io_test.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/datatype/RGBDData.cppsrc/pipeline/datatype/PointCloudData.cppsrc/pipeline/datatype/IMUData.cppsrc/pipeline/datatype/ImgFrame.cppsrc/pipeline/datatype/EncodedFrame.cpp
🪛 Cppcheck (2.21.0)
tests/src/onhost_tests/pipeline/datatype/proto_file_io_test.cpp
[error] 393-393: There is an unknown macro here somewhere. Configuration is required. If DEPTHAI_NLOHMANN_DEFINE_TYPE_INTRUSIVE is a macro then please configure it.
(unknownMacro)
src/utility/ProtoSerializable.cpp
[style] 60-60: The function 'serializeSchema' is never used.
(unusedFunction)
[style] 62-62: The function 'save' is never used.
(unusedFunction)
[style] 66-66: The function 'load' is never used.
(unusedFunction)
🔇 Additional comments (25)
bindings/python/tests/imgframe_test.py (2)
178-188: Same.sizeemptiness-check concern as previously flagged.This was already raised in a prior review: use a container-agnostic
len(...) == 0check instead of.sizefor portability acrossgetData()return types.
6-6: LGTM!Also applies to: 48-79, 165-176, 190-214
include/depthai/utility/ProtoSerializable.hpp (1)
4-4: LGTM!src/utility/ProtoSerializable.cpp (1)
3-75: LGTM!src/pipeline/datatype/PointCloudData.cpp (1)
10-10: LGTM!Also applies to: 247-260
src/pipeline/datatype/RGBDData.cpp (1)
66-69: 🗄️ Data Integrity & IntegrationNo issue:
metadataOnlyalready cascades inRGBDData
RGBDDataforwardsmetadataOnlyinto both nested frame serialization and deserialization, so the composite message respects the flag.> Likely an incorrect or invalid review comment.src/pipeline/datatype/IMUData.cpp (1)
19-30: 🎯 Functional CorrectnessNo change needed for
IMUData::serializeProto(bool)IMUDatahas nometadataOnly-strippable payload, so ignoring the flag here is fine.> Likely an incorrect or invalid review comment.bindings/python/src/pipeline/datatype/RGBDDataBindings.cpp (1)
39-42: 🩺 Stability & AvailabilityNo issue
bindings/python/src/DatatypeBindings.hppalready pulls inbindings/python/src/pybind11_common.hpp, which includespybind11/stl/filesystem.hfor this binding.> Likely an incorrect or invalid review comment.include/depthai/pipeline/datatype/RGBDData.hpp (1)
66-73: 🗄️ Data Integrity & IntegrationSame schema-compatibility concern as
ImgFrame.hpp.The
deserializeProtoMessageoverride is correct. TheDEPTHAI_SERIALIZEchange here duplicates theBuffer::tsSystemaddition flagged ininclude/depthai/pipeline/datatype/ImgFrame.hpp(lines 772-780) — see that comment for the wire-compatibility verification request; it applies identically here.include/depthai/pipeline/datatype/EncodedFrame.hpp (1)
220-243: 🗄️ Data Integrity & IntegrationSame schema-compatibility concern as
ImgFrame.hpp.
deserializeProtoMessageoverride is correct; theDEPTHAI_SERIALIZEchange duplicates theBuffer::tsSystemaddition flagged ininclude/depthai/pipeline/datatype/ImgFrame.hpp(lines 772-780). See that comment for the verification request regarding host/device wire compatibility.bindings/python/src/pipeline/datatype/ImgFrameBindings.cpp (2)
329-343: 🗄️ Data Integrity & IntegrationSame
std::filesystem::pathpybind11 caster concern asEncodedFrameBindings.cpp.
save/loadbindImgFrame::save/ImgFrame::loadtakingstd::filesystem::path. As inEncodedFrameBindings.cpp, confirm<pybind11/stl/filesystem.h>(or equivalent) is included in this translation unit so Pythonstr/pathlib.Patharguments convert correctly; otherwise these bindings will raiseTypeErrorat call time despite compiling successfully.
232-241: LGTM!Also applies to: 309-309
include/depthai/pipeline/datatype/ImgFrame.hpp (2)
34-35: LGTM!Also applies to: 111-125
772-780: 🗄️ Data Integrity & IntegrationInspect the
NOP_STRUCTUREexpansion before merging. If it feeds the libnop/XLink serializer, addingBuffer::tsSystemchanges the wire layout and can break older firmware; if it only affects the protobuf path, the compatibility concern does not apply.src/pipeline/datatype/ImgFrame.cpp (1)
1-9: LGTM!Also applies to: 68-86, 365-368
bindings/python/src/pipeline/datatype/EncodedFrameBindings.cpp (2)
79-79: LGTM!
123-126: 🚀 Performance & ScalabilityDrop this note: the filesystem caster is already available here.
pipeline/CommonBindings.hpppulls inpybind11_common.hpp, which includes<pybind11/stl/filesystem.h>, soEncodedFrame::save/loadalready accept Python path-like objects.> Likely an incorrect or invalid review comment.src/pipeline/datatype/EncodedFrame.cpp (1)
150-168: 🗄️ Data Integrity & Integration
setBufferMetadataFromalready copiessequenceNum,ts,tsDevice, andtsSystem, so this refactor preserves the metadata path ingetImgFrameMeta().> Likely an incorrect or invalid review comment.src/opencv/ImgFrame.cpp (1)
10-12: 🎯 Functional CorrectnessConfirm this include is actually used / verify save/load relocation resolved the prior OpenCV-link concern.
No symbol from
utility/ProtoFileIO.hppappears used anywhere else in this file (the rest of the file, lines 18-457, is unmodified and contains onlysetFrame/getFrame/getCvFrame/setCvFrame). A previous review round flagged a critical issue thatImgFrame::save/loadwere declared unconditionally in the header but only defined in this OpenCV-gated file, causing link errors when OpenCV support is disabled — but neithersave/loadnor the previously-discussedparseImgFrameByteshelper appear in this file anymore. This suggests the implementation moved elsewhere (e.g.src/pipeline/datatype/ImgFrame.cpp), which would resolve the prior concern, but that file isn't part of this review batch, so it can't be confirmed here. Please confirm the include is needed here (or drop it) and that save/load are defined in an always-compiled translation unit consistent with the unconditional header declaration.#!/bin/bash set -euo pipefail echo "== Usage of ProtoFileIO in this file ==" rg -n 'ProtoFileIO|parseImgFrameBytes' src/opencv/ImgFrame.cpp || echo "no usage found" echo "== Where are ImgFrame::save/load defined? ==" rg -n 'ImgFrame::save|ImgFrame::load' -g '*.cpp' echo "== Header declaration guard for save/load ==" rg -n 'DEPTHAI_HAVE_OPENCV_SUPPORT|DEPTHAI_ENABLE_PROTOBUF|void save|void load' include/depthai/pipeline/datatype/ImgFrame.hpp | sed -n '1,80p'include/depthai/pipeline/datatype/IMUData.hpp (1)
37-73: LGTM!Also applies to: 262-269
include/depthai/pipeline/datatype/PointCloudData.hpp (1)
47-51: LGTM!Also applies to: 271-277, 307-308
bindings/python/src/pipeline/datatype/IMUDataBindings.cpp (1)
53-53: LGTM!Also applies to: 156-162
bindings/python/src/pipeline/datatype/PointCloudDataBindings.cpp (1)
128-128: LGTM!Also applies to: 186-189
tests/src/onhost_tests/pipeline/datatype/proto_file_io_test.cpp (1)
1-443: LGTM!tests/CMakeLists.txt (1)
519-522: LGTM!
|
|
||
| #ifdef DEPTHAI_ENABLE_PROTOBUF | ||
| void save(const std::filesystem::path& path, bool metadataOnly = false) const; | ||
| void load(const std::filesystem::path& path, bool metadataOnly = false); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the actual deserialization/message-loading helper referenced by deserializeProtoMessage overrides
fd -i 'ProtoFileIO'
rg -n -A15 'loadProtoMessageFromBytes' --type=cppRepository: luxonis/depthai-core
Length of output: 158
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant header and locate callers/overrides.
git ls-files 'include/depthai/utility/ProtoSerializable.hpp' 'src/**' 'include/**' | sed -n '1,200p'
echo
ast-grep outline include/depthai/utility/ProtoSerializable.hpp --view expanded || true
echo
rg -n --hidden --glob '!**/.git/**' 'deserializeProtoMessage|metadataOnly|load\(const std::filesystem::path& path, bool metadataOnly = false\)' include srcRepository: luxonis/depthai-core
Length of output: 21097
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,120p' src/utility/ProtoSerializable.cpp
echo
sed -n '360,420p' src/pipeline/datatype/ImgFrame.cpp
echo
sed -n '150,190p' src/pipeline/datatype/EncodedFrame.cpp
echo
sed -n '240,270p' src/pipeline/datatype/PointCloudData.cpp
echo
sed -n '1,90p' src/pipeline/datatype/RGBDData.cpp
echo
sed -n '1,60p' src/pipeline/datatype/IMUData.cppRepository: luxonis/depthai-core
Length of output: 8552
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1050,1365p' src/utility/ProtoSerialize.cppRepository: luxonis/depthai-core
Length of output: 13503
Clear payload state on metadata-only reloads
load(..., metadataOnly=true) updates metadata in place but skips resetting payload members. Reusing an existing ImgFrame, EncodedFrame, PointCloudData, or RGBDData can leave old buffers/frames attached to the newly loaded metadata. Clear or replace the payload state before applying a metadata-only load.
🤖 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 `@include/depthai/utility/ProtoSerializable.hpp` at line 21, The metadata-only
path in ProtoSerializable::load leaves existing payload state attached when
reusing objects like ImgFrame, EncodedFrame, PointCloudData, or RGBDData. Update
the load(const std::filesystem::path&, bool metadataOnly) flow so that when
metadataOnly is true it first clears or replaces the current payload members
before applying the new metadata, ensuring stale buffers/frames are not
preserved across reloads.
| ProtoSerializable::SchemaPair serializeSchema() const override; | ||
| #endif | ||
|
|
||
| #ifdef DEPTHAI_ENABLE_PROTOBUF |
There was a problem hiding this comment.
Why is there a separate ifdef? Can this not be included in the above block?
|
|
||
| #ifdef DEPTHAI_ENABLE_PROTOBUF | ||
| protected: | ||
| void deserializeProtoMessage(const std::vector<std::uint8_t>& bytes, bool metadataOnly) override; |
There was a problem hiding this comment.
This should be with the rest of the methods before properties
|
|
||
| public: | ||
| #else | ||
| public: |
There was a problem hiding this comment.
There's no need for else, public can be outside the ifdef block
asahtik
left a comment
asahtik
left a comment
There was a problem hiding this comment.
Thanks for working on this. I left some comments.
utility/ProtoFileIO.hpp is missing.
I think it could be useful if ImgFrames were stored in a form that could be viewed as an image. On the other hand saving all datatypes in the same way simplifies the feature. @aljazkonec1 thoughts on handling ImgFrames separately to get something viewable? I think PNGs support extra metadata.
| ProtoSerializable::SchemaPair serializeSchema() const override; | ||
| #endif | ||
|
|
||
| #ifdef DEPTHAI_ENABLE_PROTOBUF |
There was a problem hiding this comment.
Can be merged with above ifdef block
| ProtoSerializable::SchemaPair serializeSchema() const override; | ||
| #endif | ||
|
|
||
| #ifdef DEPTHAI_ENABLE_PROTOBUF |
There was a problem hiding this comment.
Can be merged with above ifdef block
| ProtoSerializable::SchemaPair serializeSchema() const override; | ||
| #endif | ||
|
|
||
| #ifdef DEPTHAI_ENABLE_PROTOBUF |
There was a problem hiding this comment.
Can be merged with above ifdef block
| #include <opencv2/imgproc.hpp> | ||
|
|
||
| #ifdef DEPTHAI_ENABLE_PROTOBUF | ||
| #include "utility/ProtoFileIO.hpp" |
There was a problem hiding this comment.
This include seems unnecessary
|
|
||
| #ifdef DEPTHAI_ENABLE_PROTOBUF | ||
| void EncodedFrame::deserializeProtoMessage(const std::vector<std::uint8_t>& bytes, bool metadataOnly) { | ||
| utility::loadProtoMessageFromBytes(*this, bytes, metadataOnly); |
There was a problem hiding this comment.
Could deserializeProtoMessage be skipped and loadProtoMessageFromBytes be called directly where needed? I'm not sure since the implementation is missing.
There was a problem hiding this comment.
deserializeProtoMessage is the virtual hook that ProtoSerializable::load() uses for type-specific deserialization, so the EncodedFrame override is part of the current design, not redundant. Removing it would require redesigning the base ProtoSerializable::load() flow, not a minimal cleanup.
| return path; | ||
| } | ||
| auto resolved = path; | ||
| resolved += ".dai"; |
There was a problem hiding this comment.
Why is the extension necessary? I wouldn't change the requested path, if anything I'd prefer to throw here.
There was a problem hiding this comment.
The idea was that load("image.dai") and load("image") would behave the same. This pattern seems quite common to me, but I'm not insisting on it.
b148bc3 to
f035e5e
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
include/depthai/pipeline/datatype/EncodedFrame.hpp (1)
204-226: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMerge the two adjacent
DEPTHAI_ENABLE_PROTOBUFblocks.Same redundant split as in
PointCloudData.hpp: this can be folded into the existing#ifdef DEPTHAI_ENABLE_PROTOBUFblock above with aprotected:/public:toggle inside, instead of opening a second identical#ifdef.🤖 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 `@include/depthai/pipeline/datatype/EncodedFrame.hpp` around lines 204 - 226, Merge the redundant DEPTHAI_ENABLE_PROTOBUF guards in EncodedFrame so the existing protobuf block around serializeProto and serializeSchema also contains deserializeProtoMessage, using protected:/public: visibility switches inside the same conditional instead of opening a second identical `#ifdef`. Keep the class layout consistent with PointCloudData and preserve the access specifiers for serializeProto, serializeSchema, and deserializeProtoMessage.include/depthai/pipeline/datatype/PointCloudData.hpp (1)
255-277: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMerge the two adjacent
DEPTHAI_ENABLE_PROTOBUFblocks.Lines 255-269 and 271-277 are both guarded by the same
#ifdef DEPTHAI_ENABLE_PROTOBUF; splitting them into two separate blocks just to toggle access specifiers is redundant.♻️ Proposed merge
`#ifdef` DEPTHAI_ENABLE_PROTOBUF /** * Serialize message to proto buffer * * `@returns` serialized message */ std::vector<std::uint8_t> serializeProto(bool metadataOnly = false) const override; /** * Serialize schema to proto buffer * * `@returns` serialized schema */ ProtoSerializable::SchemaPair serializeSchema() const override; -#endif -#ifdef DEPTHAI_ENABLE_PROTOBUF protected: void deserializeProtoMessage(const std::vector<std::uint8_t>& bytes, bool metadataOnly) override; public: `#endif`🤖 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 `@include/depthai/pipeline/datatype/PointCloudData.hpp` around lines 255 - 277, Merge the two adjacent DEPTHAI_ENABLE_PROTOBUF guard blocks in PointCloudData so the protobuf methods and deserializeProtoMessage are wrapped by a single conditional section; keep the access specifier changes inside that one block instead of reopening the same `#ifdef` twice. Update the PointCloudData class declaration to group serializeProto, serializeSchema, and deserializeProtoMessage under one DEPTHAI_ENABLE_PROTOBUF block while preserving the protected/public split.src/pipeline/datatype/IMUData.cpp (1)
28-30: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
IMUData::serializeProtostill ignoresmetadataOnly
src/pipeline/datatype/IMUData.cppandsrc/utility/ProtoSerialize.cppboth drop the flag, sosave(path, true)still writespackets. ForwardmetadataOnlythrough the IMUData serializer, or the save/load contract stays inconsistent.🤖 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/pipeline/datatype/IMUData.cpp` around lines 28 - 30, IMUData::serializeProto is still discarding the metadataOnly flag, so save(path, true) cannot produce metadata-only output. Update IMUData::serializeProto to accept and forward the metadataOnly argument into utility::getProtoMessage, and make sure the matching utility::serializeProto path in ProtoSerialize.cpp also preserves that flag instead of always serializing packets.
♻️ Duplicate comments (2)
include/depthai/utility/ProtoSerializable.hpp (1)
32-32: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMetadata-only
load()doesn't clear stale payload state.
load(..., metadataOnly=true)only forwards todeserializeProtoMessage, which (perPointCloudData/RGBDData/ImgFrameoverrides) callsutility::loadProtoMessageFromBytes(*this, bytes, metadataOnly). If a caller reuses an existing object (e.g. re-loading metadata into anImgFramethat already hasdata/transformationset from a previous full load), old payload/buffers may remain attached alongside the newly loaded metadata since nothing here resets payload fields first. This was already raised in a previous review round and appears unresolved.🤖 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 `@include/depthai/utility/ProtoSerializable.hpp` at line 32, The metadata-only load path in ProtoSerializable::load leaves stale payload state on reused objects, so clear any existing payload/buffer fields before calling deserializeProtoMessage when metadataOnly is true. Update the load path and any overriding deserializeProtoMessage usage in types like ImgFrame, PointCloudData, and RGBDData so re-loading metadata cannot retain old data alongside the newly deserialized metadata.bindings/python/tests/imgframe_test.py (1)
187-187: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse a container-agnostic emptiness check.
.sizemay not exist on every possible return type ofgetData();len(...)is safer and was already suggested previously.✅ Proposed fix
- assert recovered.getData().size == 0 + assert len(recovered.getData()) == 0🤖 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 `@bindings/python/tests/imgframe_test.py` at line 187, The emptiness assertion in the imgframe test relies on getData().size, which is not container-agnostic and may fail for other return types. Update the assertion in the imgframe test to use a length-based emptiness check on recovered.getData() instead, following the safer pattern already suggested, so the test works regardless of the конкрет return container type.
🤖 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 `@include/depthai/pipeline/datatype/ImgFrame.hpp`:
- Around line 772-779: Simplify the ImgFrame visibility/protobuf preprocessor
block by removing the redundant public section from the conditional and leaving
the access specifier outside the `#ifdef` DEPTHAI_ENABLE_PROTOBUF guard. In
ImgFrame.hpp, adjust the deserializeProtoMessage declaration block so the
protected/public layout is preserved without a conditional `#else` public: `#endif`
wrapper, keeping the class interface the same while reducing unnecessary
preprocessor branching.
In `@include/depthai/pipeline/datatype/IMUData.hpp`:
- Around line 262-268: The IMUData protobuf declarations are split across two
separate DEPTHAI_ENABLE_PROTOBUF blocks, which should be merged for consistency.
Update IMUData to combine this protected/public deserializeProtoMessage section
with the earlier DEPTHAI_ENABLE_PROTOBUF block already wrapping the other
protobuf methods, matching the pattern used in RGBDData and ImgFrame so the
class has a single contiguous conditional section.
In `@include/depthai/pipeline/datatype/RGBDData.hpp`:
- Around line 66-72: Merge the new DEPTHAI_ENABLE_PROTOBUF guard in RGBDData so
deserializeProtoMessage stays inside the existing protobuf ifdef block already
wrapping the protobuf-related declarations above, rather than opening a second
adjacent `#ifdef`. Keep the access specifier changes (protected/public) within
that same block and remove the redundant guard so the class layout remains a
single contiguous conditional section.
In `@src/opencv/ImgFrame.cpp`:
- Around line 10-17: Remove the unused protobuf include from the ImgFrame.cpp
translation unit, since this file does not use any protobuf symbols. Update the
include block in src/opencv/ImgFrame.cpp to drop utility/ProtoFileIO.hpp, and
keep the protobuf-related serialization hooks confined to ImgFrame.cpp in
src/pipeline/datatype where deserializeProtoMessage and serializeProto are
actually used.
In `@src/utility/ProtoSerializable.cpp`:
- Around line 15-22: resolveDataPath is still silently rewriting extensionless
inputs to add a .dai suffix, which affects both save and load behavior. Update
ProtoSerializable::resolveDataPath so it does not mutate the requested path
implicitly; instead, preserve the original path when no extension is present or
make the function fail explicitly with an error/exception if that is the
intended contract. Make sure the change is applied consistently anywhere
resolveDataPath is used so callers see the expected path handling.
---
Outside diff comments:
In `@include/depthai/pipeline/datatype/EncodedFrame.hpp`:
- Around line 204-226: Merge the redundant DEPTHAI_ENABLE_PROTOBUF guards in
EncodedFrame so the existing protobuf block around serializeProto and
serializeSchema also contains deserializeProtoMessage, using protected:/public:
visibility switches inside the same conditional instead of opening a second
identical `#ifdef`. Keep the class layout consistent with PointCloudData and
preserve the access specifiers for serializeProto, serializeSchema, and
deserializeProtoMessage.
In `@include/depthai/pipeline/datatype/PointCloudData.hpp`:
- Around line 255-277: Merge the two adjacent DEPTHAI_ENABLE_PROTOBUF guard
blocks in PointCloudData so the protobuf methods and deserializeProtoMessage are
wrapped by a single conditional section; keep the access specifier changes
inside that one block instead of reopening the same `#ifdef` twice. Update the
PointCloudData class declaration to group serializeProto, serializeSchema, and
deserializeProtoMessage under one DEPTHAI_ENABLE_PROTOBUF block while preserving
the protected/public split.
In `@src/pipeline/datatype/IMUData.cpp`:
- Around line 28-30: IMUData::serializeProto is still discarding the
metadataOnly flag, so save(path, true) cannot produce metadata-only output.
Update IMUData::serializeProto to accept and forward the metadataOnly argument
into utility::getProtoMessage, and make sure the matching
utility::serializeProto path in ProtoSerialize.cpp also preserves that flag
instead of always serializing packets.
---
Duplicate comments:
In `@bindings/python/tests/imgframe_test.py`:
- Line 187: The emptiness assertion in the imgframe test relies on
getData().size, which is not container-agnostic and may fail for other return
types. Update the assertion in the imgframe test to use a length-based emptiness
check on recovered.getData() instead, following the safer pattern already
suggested, so the test works regardless of the конкрет return container type.
In `@include/depthai/utility/ProtoSerializable.hpp`:
- Line 32: The metadata-only load path in ProtoSerializable::load leaves stale
payload state on reused objects, so clear any existing payload/buffer fields
before calling deserializeProtoMessage when metadataOnly is true. Update the
load path and any overriding deserializeProtoMessage usage in types like
ImgFrame, PointCloudData, and RGBDData so re-loading metadata cannot retain old
data alongside the newly deserialized metadata.
🪄 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: ff0c55dc-9494-4c13-85f9-7bac619076bf
📒 Files selected for processing (21)
bindings/python/src/pipeline/datatype/EncodedFrameBindings.cppbindings/python/src/pipeline/datatype/IMUDataBindings.cppbindings/python/src/pipeline/datatype/ImgFrameBindings.cppbindings/python/src/pipeline/datatype/PointCloudDataBindings.cppbindings/python/src/pipeline/datatype/RGBDDataBindings.cppbindings/python/tests/imgframe_test.pyinclude/depthai/pipeline/datatype/EncodedFrame.hppinclude/depthai/pipeline/datatype/IMUData.hppinclude/depthai/pipeline/datatype/ImgFrame.hppinclude/depthai/pipeline/datatype/PointCloudData.hppinclude/depthai/pipeline/datatype/RGBDData.hppinclude/depthai/utility/ProtoSerializable.hppsrc/opencv/ImgFrame.cppsrc/pipeline/datatype/EncodedFrame.cppsrc/pipeline/datatype/IMUData.cppsrc/pipeline/datatype/ImgFrame.cppsrc/pipeline/datatype/PointCloudData.cppsrc/pipeline/datatype/RGBDData.cppsrc/utility/ProtoSerializable.cpptests/CMakeLists.txttests/src/onhost_tests/pipeline/datatype/proto_file_io_test.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/datatype/IMUData.cppsrc/pipeline/datatype/PointCloudData.cppsrc/pipeline/datatype/RGBDData.cppsrc/pipeline/datatype/ImgFrame.cppsrc/pipeline/datatype/EncodedFrame.cpp
🪛 Cppcheck (2.21.0)
src/utility/ProtoSerializable.cpp
[style] 71-71: The function 'serializeSchema' is never used.
(unusedFunction)
[style] 62-62: The function 'save' is never used.
(unusedFunction)
[style] 66-66: The function 'load' is never used.
(unusedFunction)
tests/src/onhost_tests/pipeline/datatype/proto_file_io_test.cpp
[error] 393-393: 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 (16)
src/utility/ProtoSerializable.cpp (2)
62-72: Cppcheck "unused function" hints are false positives.These functions are invoked from derived-class overrides and Python bindings in other translation units (e.g.
ImgFrame.cpp,ImgFrameBindings.cpp), only compiled underDEPTHAI_ENABLE_PROTOBUF, which cppcheck's single-TU analysis misses.
24-45: LGTM!Also applies to: 47-58
include/depthai/utility/ProtoSerializable.hpp (1)
20-33: Doxygen docs added, resolving prior comment.The
.daiextension behavior andmetadataOnlysemantics are now documented. This addresses the earlier documentation gap.src/pipeline/datatype/PointCloudData.cpp (1)
10-10: LGTM!Also applies to: 248-251
src/pipeline/datatype/RGBDData.cpp (1)
5-5: LGTM!Also applies to: 66-69
bindings/python/src/pipeline/datatype/EncodedFrameBindings.cpp (1)
123-126: LGTM!bindings/python/src/pipeline/datatype/IMUDataBindings.cpp (1)
159-162: LGTM!bindings/python/src/pipeline/datatype/PointCloudDataBindings.cpp (1)
186-189: LGTM!bindings/python/src/pipeline/datatype/RGBDDataBindings.cpp (1)
39-42: 🎯 Functional CorrectnessSame
std::filesystem::pathpybind11 caster dependency asImgFrameBindings.cpp.tests/src/onhost_tests/pipeline/datatype/proto_file_io_test.cpp (1)
1-443: LGTM!tests/CMakeLists.txt (1)
519-522: LGTM!bindings/python/src/pipeline/datatype/ImgFrameBindings.cpp (1)
329-343: 🎯 Functional CorrectnessFilesystem caster is already available here.
bindings/python/src/pipeline/CommonBindings.hppincludespybind11_common.hpp, which already pulls inpybind11/stl/filesystem.h, sosave/loadcan accept Python path-like arguments as-is.> Likely an incorrect or invalid review comment.src/pipeline/datatype/ImgFrame.cpp (1)
9-9: LGTM!Also applies to: 365-368
src/pipeline/datatype/EncodedFrame.cpp (1)
4-4: LGTM!Also applies to: 165-168
src/pipeline/datatype/IMUData.cpp (1)
6-6: LGTM on the new include anddeserializeProtoMessageoverride — consistent with the other datatypes.Also applies to: 19-22
bindings/python/tests/imgframe_test.py (1)
6-6: LGTM!Also applies to: 48-79, 165-176, 190-214
| std::filesystem::path resolveDataPath(const std::filesystem::path& path) { | ||
| if(path.has_extension()) { | ||
| return path; | ||
| } | ||
| auto resolved = path; | ||
| resolved += ".dai"; | ||
| return resolved; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Implicit path mutation on extensionless input — previously questioned, still unresolved.
resolveDataPath silently rewrites an extensionless path to <path>.dai for both save and load. A prior reviewer (asahtik) questioned this exact design, suggesting the requested path should not be changed or that it should throw instead. The current implementation still performs the silent rewrite.
🤖 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/utility/ProtoSerializable.cpp` around lines 15 - 22, resolveDataPath is
still silently rewriting extensionless inputs to add a .dai suffix, which
affects both save and load behavior. Update ProtoSerializable::resolveDataPath
so it does not mutate the requested path implicitly; instead, preserve the
original path when no extension is present or make the function fail explicitly
with an error/exception if that is the intended contract. Make sure the change
is applied consistently anywhere resolveDataPath is used so callers see the
expected path handling.
c4afcb3 to
9a84a06
Compare
c474ca2 to
9760b47
Compare
2 participants
Purpose
ImgFrameNotes
Buffers exceptMessageGroup... for now I did it only forImgFrameexample of usage:
load_save.py
Summary by CodeRabbit
Release Notes
New Features
save(path, metadataOnly=False)andload(path, metadataOnly=False)for image frames, including metadata-only mode that omits payload data..daisuffix in protobuf-enabled builds.Tests
.daipath handling, and error handling for missing/invalid files.