Fix: Early return when no detections in SpatialLocationCalculator#1824
Merged
Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughSegmentation passthrough handling was moved from SpatialUtils into SpatialLocationCalculator; SpatialUtils had logging, an empty-detections short-circuit, and passthrough code removed. A unit test was added to verify mask passthrough with empty detections, and a CMake version string was bumped. ChangesSegmentation passthrough refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
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 `@src/pipeline/node/SpatialLocationCalculator.cpp`:
- Around line 104-108: The passthrough branch should still validate that the
segmentation buffer size matches the reported
segmentationMaskWidth/segmentationMaskHeight to avoid downstream exceptions
(e.g., ImgDetectionsT::getCvSegmentationMask()). When
calculationConfig->segmentationPassthrough && !imgDetections->getData().empty(),
compute expectedSize = segmentationMaskWidth * segmentationMaskHeight and
compare to imgDetections->getData().size(); if they match, assign
outputSpatialImgDetections->data = imgDetections->data and copy
segmentationMaskWidth/Height as before, otherwise do not propagate the buffer
(clear outputSpatialImgDetections->data or set it to empty) and emit an
error/warning via the existing logger or return an error path so callers won't
hit ImgDetectionsT::getCvSegmentationMask() with inconsistent metadata. Note:
getData() is non-owning/span-based so this check avoids copies and only
validates consistency before copying the shared_ptr<Memory>.
🪄 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: fcbc44b6-692c-49f3-a4a9-bcf376491c0f
📒 Files selected for processing (3)
src/pipeline/node/SpatialLocationCalculator.cppsrc/pipeline/utilities/SpatialLocationCalculator/SpatialUtils.cpptests/src/ondevice_tests/pipeline/node/spatial_location_calculator_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/node/SpatialLocationCalculator.cppsrc/pipeline/utilities/SpatialLocationCalculator/SpatialUtils.cpp
🔇 Additional comments (2)
src/pipeline/utilities/SpatialLocationCalculator/SpatialUtils.cpp (1)
432-434: LGTM!tests/src/ondevice_tests/pipeline/node/spatial_location_calculator_test.cpp (1)
607-648: LGTM!
Comment on lines
+104
to
+108
| if(calculationConfig->segmentationPassthrough && !imgDetections->getData().empty()) { | ||
| outputSpatialImgDetections->data = imgDetections->data; | ||
| outputSpatialImgDetections->segmentationMaskWidth = imgDetections->getSegmentationMaskWidth(); | ||
| outputSpatialImgDetections->segmentationMaskHeight = imgDetections->getSegmentationMaskHeight(); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check return type of getData() and data field type in ImgDetections
# Find ImgDetections definition
ast-grep --pattern 'class ImgDetections {
$$$
}'
# Find getData method signature
rg -n -A2 'getData\s*\(' --type=cpp -g '*ImgDetections*'
# Check if data is a public field
rg -n 'public:' --type=cpp -g '*ImgDetections*' -A 20 | rg '\bdata\b'Repository: luxonis/depthai-core
Length of output: 1107
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect the target snippet in SpatialLocationCalculator.cpp
echo "---- SpatialLocationCalculator.cpp (around 90-130) ----"
nl -ba src/pipeline/node/SpatialLocationCalculator.cpp | sed -n '80,140p'
# 2) Locate definitions for ImgDetections / ImgDetectionsT
echo "---- Find ImgDetections* type definitions ----"
fd -a -g '*ImgDetections*' src/pipeline/datatype src/pipeline -t f || true
echo "---- Search for getData() declarations on ImgDetections/ImgDetectionsT ----"
rg -n --type=cpp --type=h "getData\s*\(" src/pipeline/datatype src/pipeline -S | rg -n "ImgDetections" || true
echo "---- Search for class/struct/typedef named ImgDetections ----"
rg -n --type=cpp --type=h "(class|struct|using|typedef)\s+ImgDetections" src/pipeline/datatype src/pipeline -S || true
rg -n --type=cpp --type=h "(ImgDetectionsT)" src/pipeline/datatype src/pipeline -S || true
# 3) If we find ImgDetectionsT.* files, print the portion that declares data and getData
echo "---- Inspect ImgDetectionsT definition region(s) ----"
# Try common filenames first; if not present, fall back to any matching *ImgDetectionsT* in repo
for f in $(fd -a -g '*ImgDetectionsT*' src/pipeline/datatype src/pipeline -t f 2>/dev/null | head -n 20); do
echo "===== $f (near getData / data) ====="
rg -n "getData\s*\(" "$f" -n --no-heading || true
# Print around first match (if any)
line=$(rg -n "getData\s*\(" "$f" | head -n 1 | cut -d: -f1 || true)
if [ -n "${line:-}" ]; then
start=$((line-15)); end=$((line+20))
nl -ba "$f" | sed -n "${start},${end}p"
fi
# Also print around "data" member if it exists
rg -n "\bdata\b" "$f" | head -n 5 | sed -n '1,5p' || true
doneRepository: luxonis/depthai-core
Length of output: 181
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- SpatialLocationCalculator.cpp (around 90-130) ----"
sed -n '90,130p' src/pipeline/node/SpatialLocationCalculator.cpp | cat -n
echo "---- Locate ImgDetections / ImgDetectionsT definitions ----"
# List likely datatype headers
fd -a -g '*ImgDetections*' src/pipeline/datatype -t f || true
fd -a -g '*ImgDetections*' src/pipeline -t f || true
echo "---- Search for struct/class/typedef named ImgDetections / ImgDetectionsT ----"
rg -n --type=cpp --type=h "(class|struct|using|typedef)\s+ImgDetections" src/pipeline/datatype src/pipeline -S || true
rg -n --type=cpp --type=h "ImgDetectionsT" src/pipeline/datatype src/pipeline -S || true
echo "---- Inspect getData() declaration(s) and data member declaration(s) ----"
# Focus on any files that mention getData() for ImgDetectionsT
targets=$(rg -l --type=cpp --type=h "ImgDetectionsT" src/pipeline/datatype src/pipeline | head -n 50)
echo "$targets" | sed 's/^/TARGET: /'
for f in $targets; do
if rg -q --type=cpp --type=h "getData\s*\(" "$f"; then
echo "===== getData occurrences in $f ====="
# show first few matches with context
rg -n -A3 -B2 "getData\s*\(" "$f" | head -n 60 || true
fi
done
# Specifically inspect ImgDetectionsT.cpp which we already saw referenced by the earlier run
if [ -f src/pipeline/datatype/ImgDetectionsT.cpp ]; then
echo "---- Inspect src/pipeline/datatype/ImgDetectionsT.cpp (around earlier hits) ----"
sed -n '1,220p' src/pipeline/datatype/ImgDetectionsT.cpp | cat -n
fi
# Also try to locate a header that declares the actual data member and getData() signature
headers=$(fd -a -g '*ImgDetections*' src/pipeline/datatype -t f --extension h --extension.hpp --extension hh 2>/dev/null || true)
for f in $headers; do
if rg -q --type=cpp "getData\s*\(" "$f"; then
echo "===== Inspect header $f near getData ====="
line=$(rg -n "getData\s*\(" "$f" | head -n1 | cut -d: -f1 || true)
if [ -n "${line:-}" ]; then
start=$((line-20)); end=$((line+30))
sed -n "${start},${end}p" "$f" | cat -n
fi
fi
if rg -q --type=cpp --type=h "\bdata\b" "$f"; then
echo "===== Inspect header $f near member 'data' ====="
rg -n "\bdata\b" "$f" | head -n 20 | sed 's/^/DATA_LINE: /'
fi
doneRepository: luxonis/depthai-core
Length of output: 16822
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- Locate relevant headers ----"
fd -a -g '*ImgDetectionsT*.hpp' src/pipeline/datatype src -t f || true
fd -a -g '*ImgDetections*.hpp' src/pipeline/datatype src -t f || true
fd -a -g '*SpatialImgDetections*.hpp' src/pipeline/datatype src -t f || true
echo "---- Inspect ImgDetectionsT.hpp for getData() and member data ----"
hdrs=$(fd -a -g 'ImgDetectionsT.hpp' src/pipeline/datatype src -t f 2>/dev/null || true)
for f in $hdrs; do
echo "===== $f ====="
# show where getData is declared (best-effort)
if rg -n --type=h "getData\s*\(" "$f"; then
line=$(rg -n --type=h "getData\s*\(" "$f" | head -n1 | cut -d: -f1)
start=$((line-30)); end=$((line+40))
sed -n "${start},${end}p" "$f" | cat -n
fi
# show around data member (best-effort)
if rg -n --type=h "\bdata\b" "$f" | head -n 20 >/dev/null; then
echo "-- Lines containing 'data' (first 30) --"
rg -n --type=h "\bdata\b" "$f" | head -n 30
fi
done
echo "---- Inspect SpatialImgDetections.hpp for public data member / segmentationMaskWidth/Height ----"
hdrs2=$(fd -a -g 'SpatialImgDetections*.hpp' src/pipeline/datatype src -t f 2>/dev/null || true)
for f in $hdrs2; do
echo "===== $f ====="
if rg -n --type=h "\bsegmentationMaskWidth\b|\bsegmentationMaskHeight\b" "$f"; then
rg -n --type=h "\bsegmentationMaskWidth\b|\bsegmentationMaskHeight\b" "$f" | head -n 20
fi
if rg -n --type=h "\bdata\b" "$f" | head -n 30 >/dev/null; then
rg -n --type=h "\bdata\b" "$f" | head -n 30
fi
done
echo "---- Inspect ImgDetections.hpp for getData if exists ----"
hdrs3=$(fd -a -g 'ImgDetections.hpp' src/pipeline/datatype src -t f 2>/dev/null || true)
for f in $hdrs3; do
echo "===== $f ====="
if rg -n --type=h "getData\s*\(" "$f" | head -n 20 >/dev/null; then
rg -n --type=h "getData\s*\(" "$f" | head -n 20
fi
rg -n --type=h "\bdata\b" "$f" | head -n 20 || true
doneRepository: luxonis/depthai-core
Length of output: 320
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- Find header files by exact filename ----"
fd -a -g 'ImgDetectionsT.hpp' . -t f || true
fd -a -g 'ImgDetections.hpp' . -t f || true
fd -a -g 'SpatialImgDetections.hpp' . -t f || true
echo "---- Find any file containing 'class ImgDetectionsT' (headers) ----"
rg -l --type=h "ImgDetectionsT<" . || true
rg -l --type=h "getData\s*\(" . | rg -n "ImgDetections" || trueRepository: luxonis/depthai-core
Length of output: 514
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in \
include/depthai/pipeline/datatype/ImgDetectionsT.hpp \
include/depthai/pipeline/datatype/ImgDetections.hpp \
include/depthai/pipeline/datatype/SpatialImgDetections.hpp
do
echo "===================== $f ====================="
# show getData declarations
if rg -n --type=h "getData\s*\(" "$f" >/dev/null; then
rg -n --type=h "getData\s*\(" "$f" | head -n 20
line=$(rg -n --type=h "getData\s*\(" "$f" | head -n 1 | cut -d: -f1)
start=$((line-30)); end=$((line+40))
sed -n "${start},${end}p" "$f" | cat -n
else
echo "(no getData found)"
fi
# show data member declarations/uses
if rg -n --type=h "\bdata\b" "$f" >/dev/null; then
echo "---- first 'data' mentions ----"
rg -n --type=h "\bdata\b" "$f" | head -n 40
else
echo "(no 'data' mentions)"
fi
# show segmentationMaskWidth/Height in SpatialImgDetections.hpp
if [[ "$f" == *SpatialImgDetections.hpp ]]; then
rg -n --type=h "segmentationMaskWidth|segmentationMaskHeight" "$f" | head -n 40
if rg -n --type=h "segmentationMaskWidth|segmentationMaskHeight" "$f" >/dev/null; then
line=$(rg -n --type=h "segmentationMaskWidth" "$f" | head -n 1 | cut -d: -f1 || true)
if [ -n "${line:-}" ]; then
start=$((line-20)); end=$((line+30))
sed -n "${start},${end}p" "$f" | cat -n
fi
fi
fi
doneRepository: luxonis/depthai-core
Length of output: 4208
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- Search for 'getData' in include/depthai/pipeline/datatype ----"
rg -n --type=h "\bgetData\b" include/depthai/pipeline/datatype || true
echo "---- Search for 'getData(' in entire include ----"
rg -n --type=h "\bgetData\s*\(" include || true
echo "---- Inspect ImgDetectionsT.hpp (top ~220 lines) ----"
sed -n '1,220p' include/depthai/pipeline/datatype/ImgDetectionsT.hpp | cat -n
echo "---- Inspect ImgDetectionsT.hpp around the first mention of 'data' comment ----"
# show around first occurrence of "data" word in comments/decls
line=$(rg -n "\bdata\b" include/depthai/pipeline/datatype/ImgDetectionsT.hpp | head -n 1 | cut -d: -f1 || true)
if [ -n "${line:-}" ]; then
start=$((line-60)); end=$((line+140))
sed -n "${start},${end}p" include/depthai/pipeline/datatype/ImgDetectionsT.hpp | cat -n
fiRepository: luxonis/depthai-core
Length of output: 9788
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- Inspect include/depthai/pipeline/datatype/Buffer.hpp ----"
sed -n '1,120p' include/depthai/pipeline/datatype/Buffer.hpp | cat -n
echo "---- Inspect Buffer.hpp for member 'data' declaration ----"
rg -n "\bdata\b" include/depthai/pipeline/datatype/Buffer.hpp
echo "---- Inspect include/depthai/pipeline/datatype/SpatialImgDetections.hpp around class header (public/protected) ----"
sed -n '1,120p' include/depthai/pipeline/datatype/SpatialImgDetections.hpp | cat -nRepository: luxonis/depthai-core
Length of output: 9535
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- Buffer.hpp full scan for member declaration of data ----"
rg -n --type=h "(\bdata\b\s*=|(\bdata\b\s*;)|\bstd::.*\bdata\b)" include/depthai/pipeline/datatype/Buffer.hpp || true
sed -n '96,220p' include/depthai/pipeline/datatype/Buffer.hpp | cat -n
echo "---- Inspect ADatatype.hpp for shared data storage ----"
fd -a -g 'ADatatype.hpp' include/depthai/pipeline/datatype -t f || true
if [ -f include/depthai/pipeline/datatype/ADatatype.hpp ]; then
sed -n '1,200p' include/depthai/pipeline/datatype/ADatatype.hpp | cat -n
rg -n --type=h "\bdata\b" include/depthai/pipeline/datatype/ADatatype.hpp || true
fi
echo "---- Search repo for declaration of member named 'data' in Buffer ----"
rg -n --type=cpp --type=h "class Buffer|Buffer::|struct Buffer" src include/depthai/pipeline/datatype -S || true
rg -n --type=cpp --type=h "(\bdata\b\s*;)" include/depthai/pipeline/datatype src/pipeline/datatype -S || true
echo "---- Locate Buffer.cpp / implementation of getData/setData ----"
fd -a -g 'Buffer.*' src/pipeline/datatype -t f || true
fd -a -g 'Buffer.*' . -t f | rg 'Buffer\.(cpp|cc|cxx)$' | head -n 20 || trueRepository: luxonis/depthai-core
Length of output: 10884
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- Inspect src/pipeline/datatype/Buffer.cpp around getData/setData ----"
sed -n '1,120p' src/pipeline/datatype/Buffer.cpp | cat -n
echo "---- Inspect src/pipeline/datatype/Buffer.cpp around serialize/getData further if needed ----"
sed -n '20,90p' src/pipeline/datatype/Buffer.cpp | cat -nRepository: luxonis/depthai-core
Length of output: 6884
Reduce concern about getData() cost; consider validating segmentation mask size/metadata consistency
imgDetections->getData()returnsspan<const uint8_t>(non-owning), so!imgDetections->getData().empty()shouldn’t incur copies;outputSpatialImgDetections->data = imgDetections->datajust copies thestd::shared_ptr<Memory>fromADatatype.- Consider adding a small sanity check that
segmentationMaskWidth * segmentationMaskHeightmatches the copied buffer size to prevent downstream failures (e.g.,ImgDetectionsT::getCvSegmentationMask()throws on size mismatches).
🤖 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/node/SpatialLocationCalculator.cpp` around lines 104 - 108, The
passthrough branch should still validate that the segmentation buffer size
matches the reported segmentationMaskWidth/segmentationMaskHeight to avoid
downstream exceptions (e.g., ImgDetectionsT::getCvSegmentationMask()). When
calculationConfig->segmentationPassthrough && !imgDetections->getData().empty(),
compute expectedSize = segmentationMaskWidth * segmentationMaskHeight and
compare to imgDetections->getData().size(); if they match, assign
outputSpatialImgDetections->data = imgDetections->data and copy
segmentationMaskWidth/Height as before, otherwise do not propagate the buffer
(clear outputSpatialImgDetections->data or set it to empty) and emit an
error/warning via the existing logger or return an error path so callers won't
hit ImgDetectionsT::getCvSegmentationMask() with inconsistent metadata. Note:
getData() is non-owning/span-based so this check avoids copies and only
validates consistency before copying the shared_ptr<Memory>.
moratom
approved these changes
Jun 1, 2026
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
2 participants
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
SLC would return before copying segmentation mask to the output message if there were no detections. Added a test case to cover this part. RVC2 implementation already has the proper implementation.
Specification
None / not applicable
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
Summary by CodeRabbit
Bug Fixes
Tests
Chores