ImageAlign: Use imageTransformations - #1905
Conversation
|
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:
📝 WalkthroughWalkthroughImageAlign now derives calibration and rectification parameters from depth and align-to frame transformations, caches the latest align-to frame, refreshes state when transformations change, and removes calibration-handler refresh logic. ChangesImageAlign transformation flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
JakubFara
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pipeline/node/ImageAlign.cpp (1)
249-259: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPrevent Undefined Behavior when distortion coefficients are empty.
If
alignDistortionCoefficientsis empty,std::vector<float>(0, 0.0f)is passed tovecToCvMat, which creates an emptycv::Mat(1, 0). Because OpenCV does not allocate a data buffer for 0 total elements,cvMat.datawill benullptr. ThevecToCvMathelper then callsmemcpywith thisnullptr, which is Undefined Behavior in C/C++ and can cause the pipeline to crash.Since OpenCV accepts a 14-element vector of zeros to represent no distortion (and the code already does this to pad
depthDistortionCoefficients), you can safely hardcode the size to 14 to avoid the crash.🛡️ Proposed fix
const auto alignDistortionCoefficients = alignToTransformation.getDistortionCoefficients(); const auto depthToAlignRotation = depthSourceTransformation.getRotationMatrixTo(alignToTransformation); const auto depthToAlignTranslationArray = depthSourceTransformation.getTranslationVectorTo(alignToTransformation, false, LengthUnit::MILLIMETER); const std::vector<float> depthToAlignTranslation(depthToAlignTranslationArray.begin(), depthToAlignTranslationArray.end()); auto cv_M1 = arrayToCvMat(3, 3, CV_32FC1, depthSourceIntrinsics); auto cv_M2 = arrayToCvMat(3, 3, CV_32FC1, alignSourceIntrinsics); auto cv_d1 = vecToCvMat(1, depthDistortionCoefficients.size(), CV_32FC1, depthDistortionCoefficients); - auto cv_dNone = vecToCvMat( - 1, alignDistortionCoefficients.size(), CV_32FC1, std::vector<float>(alignDistortionCoefficients.size(), 0.0f)); // No distortion for aligned frame + auto cv_dNone = vecToCvMat(1, 14, CV_32FC1, std::vector<float>(14, 0.0f)); // No distortion for aligned frame🤖 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/ImageAlign.cpp` around lines 249 - 259, Update the cv_dNone construction near alignDistortionCoefficients to always create a 14-element zero vector instead of using alignDistortionCoefficients.size(). Preserve its role as the no-distortion matrix and avoid passing an empty vector to vecToCvMat.
🤖 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/ImageAlign.cpp`:
- Around line 357-360: Update the inputAlignTo handling in ImageAlign to
repeatedly call tryGet<ImgFrame>() until no pending messages remain, retaining
each retrieved frame so inputAlignToImg ends with the latest one. Preserve the
existing behavior when the queue is empty.
---
Outside diff comments:
In `@src/pipeline/node/ImageAlign.cpp`:
- Around line 249-259: Update the cv_dNone construction near
alignDistortionCoefficients to always create a 14-element zero vector instead of
using alignDistortionCoefficients.size(). Preserve its role as the no-distortion
matrix and avoid passing an empty vector to vecToCvMat.
🪄 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: 61a3f0db-d087-43c0-a25a-2195d24d3df7
📒 Files selected for processing (1)
src/pipeline/node/ImageAlign.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/ImageAlign.cpp
afcf501 to
0772265
Compare
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 `@src/pipeline/node/ImageAlign.cpp`:
- Around line 353-357: Update the transformation-change branch in ImageAlign so
it also resets allocated, frameSize, outFrameSize, and previousShiftFactor
alongside initialized and calibrationSet. Before reinitialization, re-derive any
auto-selected output dimensions from the new transformation rather than
retaining values from the previous transform.
- Around line 359-362: Update the frame-selection logic in ImageAlign so
inputAlignToImgFrame is refreshed from the currently selected inputAlignToImg on
every frame, moving that copy outside the !initialized block while keeping
alignToTransformation initialization there. This ensures
alignedImg->setMetadata(inputAlignToImgFrame) uses current timestamp and
sequence metadata.
- Around line 343-350: Guard the result of inputAlignTo.getAll<ImgFrame>() in
the align-to image selection logic before accessing back(). Only read and assign
the last message when the collection is non-empty, then preserve the existing
inputAlignTo.get<ImgFrame>() fallback when no valid image was selected.
🪄 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: 8ab485a0-2cb1-48b2-83f5-d28e10d3b120
📒 Files selected for processing (1)
src/pipeline/node/ImageAlign.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/ImageAlign.cpp
25d51e6 to
e1f8f10
Compare
MaticTonin
left a comment
MaticTonin
left a comment
There was a problem hiding this comment.
No issues from my side.
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/ImageAlign.cpp`:
- Around line 560-569: Update the stride and plane-offset initialization in the
aligned image setup to use the row stride defined by the ImgFrame contract, not
aggregate frame bytes-per-pixel for planar NV12 and YUV420p formats. Preserve
the existing chroma-plane layout, ensuring p2Offset starts at width × height and
YUV420p p3Offset follows the half-resolution chroma plane; use the local
frameTypeToBpp mapping or equivalent per-row value.
🪄 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 Plus
Run ID: d525a022-c136-489a-9774-7e306fb75b83
📒 Files selected for processing (1)
src/pipeline/node/ImageAlign.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/ImageAlign.cpp
🔇 Additional comments (5)
src/pipeline/node/ImageAlign.cpp (5)
69-73: LGTM!
187-188: LGTM!
235-247: LGTM!
331-392: LGTM!
439-439: LGTM!
@JakubFara please go over my changes here and on device (RVC2 and RVC4) as I might have made some mistakes. Thanks |
8bba2c3 to
f020966
Compare
Thanks, I was not aware of device side
Good point
What particularly was wrong?
Ok, I have found Implementation of I like to rebase branch ok develop not merge develop to branch since it keeps the branch clean. I prefere just 1 person to do the changes in a PR. |
offsets were set based on the input and not the alignTo frame offsets, Camera capabilities were copied from alignToFrame when in reality they should remain from the inputFrame, A lot of the metadata gets overwritten after that (size, type, timestamps, ImgTransformations ...) so I opted to just remove setMetadata and make it an explicit set
Yes but it was necessarry due to flaky-ness of test.
|
f020966 to
720fbb3
Compare
720fbb3 to
8ee2055
Compare
3 participants
Purpose
Summary by CodeRabbit