Buxfix/align runtime calibration change#1782
Conversation
📝 WalkthroughWalkthroughImageAlign::run() centralizes inputAlignTo handling into updateAlignToData, tracks EEPROM ID changes to trigger a refresh of align-to intrinsics/transforms by waiting for a new inputAlignTo frame, and test code adds helpers plus two runtime-calibration TEST_CASEs validating refresh behavior. ChangesRuntime Calibration Refresh for Image Alignment
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 5
🤖 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 464-471: After updateAlignToData runs on a stale ImgFrame you must
force-align the source intrinsics to the already-updated calibration to avoid
mixing old metadata with new calibHandler data; so immediately after calling
updateAlignToData(latestAlignToImg) in the refreshAlignToTransform branch,
overwrite alignSourceIntrinsics from calibHandler (and similarly ensure
inputAlignToTransform is synchronized from calibHandler if that transform can be
derived there) before clearing refreshAlignToTransform, so
extractCalibrationData sees consistent intrinsics/transforms even if the
buffered frame predates the EEPROM change.
In `@tests/src/ondevice_tests/image_align_node_test.cpp`:
- Around line 22-31: Remove the dead helper functions sameIntrinsics and
approxEqual from the test file: locate the definitions of sameIntrinsics (the
nested-loop matrix comparison) and approxEqual and delete them to clean up
unused test utilities; if you prefer to keep approximate comparison helpers for
future use, instead mark them static/anonymous or move them into a shared
test-utility file, but do not leave unused global definitions in this test file.
- Around line 121-187: The test only exercises the default ImageAlign run path
(device-side); add a host-side variant by extending
runImageAlignRuntimeCalibrationTest to accept a bool runOnHost, call
align->setRunOnHost(true) when that flag is true (symbol:
runImageAlignRuntimeCalibrationTest and class/method: ImageAlign::setRunOnHost /
align->setRunOnHost), and update the two TEST_CASEs that call
runImageAlignRuntimeCalibrationTest (and the other related tests around the
second block) to invoke it twice (once with runOnHost=false and once with
runOnHost=true) so the host-side EEPROM-change detection in ImageAlign::run() is
covered.
- Around line 180-184: The verification loop can see transient frames from
ImageAlign during the calibration revert; before the for-loop that reads from
alignedQueue and compares frames to revertedAlignTo->transformation, call
waitForFrameAlignedTo to wait until ImageAlign reaches the restored calibration
steady-state (use the same alignToQueue/alignedQueue and
revertedAlignTo->transformation) so you skip transition frames; update the test
to use waitForFrameAlignedTo(...) prior to the
kRuntimeCalibrationVerificationFrames loop so each
revertedAligned->transformation will reliably satisfy
isAlignedTo(revertedAlignTo->transformation).
- Around line 163-170: The fallback that calls waitForNextFrame when
waitForFrameWithChangedTransform returns nullptr should be removed so the test
fails fast if the calibration change isn't observed; specifically, delete the
fallback assignment that replaces shiftedAlignTo with
waitForNextFrame(alignToQueue) and keep the REQUIRE(shiftedAlignTo != nullptr)
to assert the change, and apply the same removal for the restore path (remove
replacing revertedAlignTo with waitForNextFrame and rely on
REQUIRE(revertedAlignTo != nullptr)); this ensures
waitForFrameAlignedTo(alignedQueue, shiftedAlignTo->transformation) and the
symmetric restore checks actually validate propagation rather than silently
using stale frames.
🪄 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: b304bc8c-f4cc-4a65-8bcb-8e18903745a6
📒 Files selected for processing (2)
src/pipeline/node/ImageAlign.cpptests/src/ondevice_tests/image_align_node_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/ImageAlign.cpp
🪛 Cppcheck (2.20.0)
tests/src/ondevice_tests/image_align_node_test.cpp
[error] 18-18: 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 (1)
src/pipeline/node/ImageAlign.cpp (1)
362-391: 💤 Low value
updateAlignToDatacentralisation looks correct.The lambda properly handles the scale adjustment and updates all dependent fields (
alignSourceIntrinsics,inputAlignToTransform,inputAlignToImgFrame).One note: the
if(alignWidth == 0 || alignHeight == 0)guard means that if the new calibration implies a differentinputAlignTocamera resolution, the storedalignWidth/alignHeight— and consequentlyoutFrameSize— are silently retained from the previous initialization. This is presumably intentional (resolution doesn't change during streaming), but worth a brief comment for future readers.
| if(refreshAlignToTransform) { | ||
| if(auto latestAlignToImg = inputAlignTo.tryGet<ImgFrame>()) { | ||
| updateAlignToData(latestAlignToImg); | ||
| refreshAlignToTransform = false; | ||
| } else { | ||
| logger->trace("Waiting for updated inputAlignTo frame after calibration change."); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Stale inputAlignTo frame may corrupt alignment after recalibration with no self-correction path.
The inputAlignTo queue is documented as non-blocking with size 1, which means overwrite semantics reduce the stale-frame window considerably. However, a narrow race still exists: if no new camera frame has arrived between the EEPROM change and the tryGet call, the single buffered frame may pre-date the calibration update. When updateAlignToData runs with that stale frame, alignSourceIntrinsics and inputAlignToTransform are set from the old calibration's metadata, while calibHandler already carries the new rotation/translation. The subsequent extractCalibrationData then mixes these inconsistently, producing incorrect rectification maps.
The critical problem is that currentEepromId is already advanced to latestEepromId at line 461, so no second recalibration cycle will fire to self-correct — the bad maps persist until the next external calibration change.
A targeted fix: after calling updateAlignToData, explicitly override alignSourceIntrinsics from the updated calibHandler for consistency, since the new intrinsics are already available there:
🛡️ Proposed hardening
if(refreshAlignToTransform) {
if(auto latestAlignToImg = inputAlignTo.tryGet<ImgFrame>()) {
updateAlignToData(latestAlignToImg);
+ // Guard against stale queued frames: re-read intrinsics from the freshly
+ // updated calibHandler so alignSourceIntrinsics is always consistent with
+ // the new calibration data used in extractCalibrationData.
+ auto freshIntrinsics = calibHandler.getCameraIntrinsics(alignTo, alignWidth, alignHeight);
+ for(size_t i = 0; i < 3; ++i)
+ for(size_t j = 0; j < 3; ++j)
+ alignSourceIntrinsics[i][j] = freshIntrinsics[i][j];
refreshAlignToTransform = false;
} else {🤖 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 464 - 471, After
updateAlignToData runs on a stale ImgFrame you must force-align the source
intrinsics to the already-updated calibration to avoid mixing old metadata with
new calibHandler data; so immediately after calling
updateAlignToData(latestAlignToImg) in the refreshAlignToTransform branch,
overwrite alignSourceIntrinsics from calibHandler (and similarly ensure
inputAlignToTransform is synchronized from calibHandler if that transform can be
derived there) before clearing refreshAlignToTransform, so
extractCalibrationData sees consistent intrinsics/transforms even if the
buffered frame predates the EEPROM change.
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 `@tests/src/ondevice_tests/image_align_node_test.cpp`:
- Around line 127-172: Add a short test-strategy comment at the top of the
TEST_CASE "ImageAlign runtime calibration update changes aligned depth output"
explaining that the test verifies ImageAlign detects runtime calibration changes
and recomputes warping while keeping the output coordinate frame stable (aligned
to CAM_A) but producing different pixel data due to changed stereo baseline;
place the comment immediately inside the TEST_CASE before creating the
dai::Pipeline so future readers quickly understand the validation intent.
- Around line 9-123: Add brief one-line comments explaining purpose and
behaviour for the helper functions: place a comment above
refreshAlignToTransformation describing that it scales the ImgTransformation
intrinsics and updates size when target align dimensions differ, and a comment
above makeCalibration stating it builds a CalibrationHandler with distortion
enabled and a configurable stereo baseline (translationXcm) and sets
intrinsics/distortion/extrinsics for CAM_A/CAM_B; keep comments short and to the
point.
🪄 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: a3ba876f-8417-4d3c-adbc-12bdca6d7738
📒 Files selected for processing (1)
tests/src/ondevice_tests/image_align_node_test.cpp
📜 Review details
🧰 Additional context used
🪛 Cppcheck (2.20.0)
tests/src/ondevice_tests/image_align_node_test.cpp
[error] 18-18: 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 (2)
tests/src/ondevice_tests/image_align_node_test.cpp (2)
174-227: LGTM - Thorough validation of align-to transformation refresh logic.The test provides comprehensive coverage of the intrinsic matrix update path:
- Lines 192-195 correctly compute expected transforms by manually applying the refresh logic that mirrors
ImageAlignbehavior- Lines 208-209 validate the baseline state before calibration change
- Lines 221-224 confirm that post-update intrinsics differ from pre-update and match the scaled expectation
The test design—manually computing expected transforms via
refreshAlignToTransformationand validating against actual output—provides strong confidence that the EEPROM-change detection correctly triggers align-to geometry refresh.
127-227: Test coverage appropriately addresses past review feedback.Both test cases correctly exercise the host-side EEPROM-change detection path via
setRunOnHost(true)(lines 130, 177), which was requested in prior reviews. Device-side variants are not needed since the PR description confirms the calibration-change detection logic is host-side only.The past concerns about dead code, fallback logic, and transition frame handling appear to have been resolved through the refactored implementation.
| TEST_CASE("ImageAlign runtime calibration update changes aligned depth output") { | ||
| dai::Pipeline pipeline; | ||
| auto align = pipeline.create<dai::node::ImageAlign>(); | ||
| align->setRunOnHost(true); | ||
|
|
||
| auto depthInQ = align->input.createInputQueue(); | ||
| auto alignToInQ = align->inputAlignTo.createInputQueue(); | ||
| auto alignedOutQ = align->outputAligned.createOutputQueue(); | ||
|
|
||
| auto initialCalibration = makeCalibration(2.0f); | ||
| auto updatedCalibration = makeCalibration(5.0f); | ||
|
|
||
| pipeline.getDefaultDevice()->setCalibration(initialCalibration); | ||
| pipeline.start(); | ||
|
|
||
| alignToInQ->send(makeAlignToFrame()); | ||
| depthInQ->send(makeDepthFrame()); | ||
|
|
||
| auto firstAligned = alignedOutQ->get<dai::ImgFrame>(); | ||
| REQUIRE(firstAligned != nullptr); | ||
| REQUIRE(firstAligned->getWidth() == kDepthWidth); | ||
| REQUIRE(firstAligned->getHeight() == kDepthHeight); | ||
| REQUIRE(firstAligned->getType() == dai::ImgFrame::Type::RAW16); | ||
| REQUIRE(firstAligned->getInstanceNum() == static_cast<unsigned>(dai::CameraBoardSocket::CAM_A)); | ||
|
|
||
| pipeline.getDefaultDevice()->setCalibration(updatedCalibration); | ||
|
|
||
| alignToInQ->send(makeAlignToFrame()); | ||
| depthInQ->send(makeDepthFrame()); | ||
|
|
||
| auto secondAligned = alignedOutQ->get<dai::ImgFrame>(); | ||
| REQUIRE(secondAligned != nullptr); | ||
| REQUIRE(secondAligned->getWidth() == kDepthWidth); | ||
| REQUIRE(secondAligned->getHeight() == kDepthHeight); | ||
| REQUIRE(secondAligned->getType() == dai::ImgFrame::Type::RAW16); | ||
| REQUIRE(secondAligned->getInstanceNum() == static_cast<unsigned>(dai::CameraBoardSocket::CAM_A)); | ||
|
|
||
| REQUIRE(firstAligned->transformation.isEqualTransformation(secondAligned->transformation)); | ||
|
|
||
| const auto firstData = firstAligned->getData(); | ||
| const auto secondData = secondAligned->getData(); | ||
| REQUIRE(firstData.size() == secondData.size()); | ||
| REQUIRE_FALSE(std::equal(firstData.begin(), firstData.end(), secondData.begin(), secondData.end())); | ||
|
|
||
| pipeline.stop(); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
LGTM - Correct validation of runtime calibration impact on aligned depth data.
The test correctly verifies that:
- The aligned output transformation remains consistent (aligned to CAM_A frame) at line 164
- The pixel data changes due to the updated stereo baseline warping at line 169
The logic properly validates that ImageAlign detects the EEPROM change and rebuilds alignment maps without changing the output coordinate frame.
📝 Optional: Add test strategy comment
Consider adding a brief comment at the test start explaining the validation strategy:
TEST_CASE("ImageAlign runtime calibration update changes aligned depth output") {
// Verify that ImageAlign detects runtime calibration changes and recomputes warping.
// The output transformation should remain stable (aligned to CAM_A), but pixel
// values should differ due to the changed stereo baseline.
dai::Pipeline pipeline;
// ...
}🤖 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/ondevice_tests/image_align_node_test.cpp` around lines 127 - 172,
Add a short test-strategy comment at the top of the TEST_CASE "ImageAlign
runtime calibration update changes aligned depth output" explaining that the
test verifies ImageAlign detects runtime calibration changes and recomputes
warping while keeping the output coordinate frame stable (aligned to CAM_A) but
producing different pixel data due to changed stereo baseline; place the comment
immediately inside the TEST_CASE before creating the dai::Pipeline so future
readers quickly understand the validation intent.
MaticTonin
left a comment
MaticTonin
left a comment
There was a problem hiding this comment.
Only one question otherwise seems fine.
|
Spoke to @aljazkonec1 |
JakubFara
force-pushed
the
buxfix/align_runtime_calibration_change
branch
from
b9d64ea to
f5667d6
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/pipeline/node/ImageAlign.cpp (1)
464-472:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftStale
inputAlignToframe may cause intrinsics mismatch after recalibration.When
refreshAlignToTransformis true,inputAlignTo.tryGet()may return a frame captured before the calibration update (the queue has size 1, so only the most recent frame is buffered). If no new camera frame has arrived since the EEPROM change,updateAlignToDatawill extractalignSourceIntrinsicsfrom the stale frame's transformation metadata. Meanwhile,calibHandlerat line 460 already holds the updated calibration, soextractCalibrationData(line 475) will mix old intrinsics with new rotation/translation, producing incorrect rectification maps. BecausecurrentEepromIdis advanced at line 461, no second recalibration cycle will self-correct.A targeted mitigation: after calling
updateAlignToData, explicitly overridealignSourceIntrinsicsfrom the updatedcalibHandlerto ensure consistency with the new rotation/translation used inextractCalibrationData.🛡️ Proposed hardening
if(refreshAlignToTransform) { if(auto latestAlignToImg = inputAlignTo.tryGet<ImgFrame>()) { updateAlignToData(latestAlignToImg); + // Guard against stale queued frames: re-read intrinsics from the freshly + // updated calibHandler so alignSourceIntrinsics is always consistent with + // the new calibration data used in extractCalibrationData. + auto freshIntrinsics = calibHandler.getCameraIntrinsics(alignTo, alignWidth, alignHeight); + for(size_t i = 0; i < 3; ++i) + for(size_t j = 0; j < 3; ++j) + alignSourceIntrinsics[i][j] = freshIntrinsics[i][j]; refreshAlignToTransform = false; } else {🤖 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 464 - 472, When refreshAlignToTransform is true and you successfully call updateAlignToData(latestAlignToImg), immediately override the possibly-stale alignSourceIntrinsics with the intrinsics from the new calibration (calibHandler) before calling extractCalibrationData; specifically, after updateAlignToData(...) set alignSourceIntrinsics to the current intrinsics provided by calibHandler (or via its accessor) so the intrinsics match the updated rotation/translation, then proceed to clear refreshAlignToTransform as before.
🤖 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.
Duplicate comments:
In `@src/pipeline/node/ImageAlign.cpp`:
- Around line 464-472: When refreshAlignToTransform is true and you successfully
call updateAlignToData(latestAlignToImg), immediately override the
possibly-stale alignSourceIntrinsics with the intrinsics from the new
calibration (calibHandler) before calling extractCalibrationData; specifically,
after updateAlignToData(...) set alignSourceIntrinsics to the current intrinsics
provided by calibHandler (or via its accessor) so the intrinsics match the
updated rotation/translation, then proceed to clear refreshAlignToTransform as
before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3d6379fd-a29b-4355-a2ba-a6eff0961e9a
📒 Files selected for processing (2)
src/pipeline/node/ImageAlign.cpptests/src/ondevice_tests/image_align_node_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/ImageAlign.cpp
🪛 Cppcheck (2.20.0)
tests/src/ondevice_tests/image_align_node_test.cpp
[error] 18-18: 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 (6)
src/pipeline/node/ImageAlign.cpp (3)
362-391: LGTM!
406-406: LGTM!
458-459: LGTM!tests/src/ondevice_tests/image_align_node_test.cpp (3)
9-123: LGTM!
127-172: LGTM!
174-227: LGTM!
aljazkonec1
left a comment
aljazkonec1
left a comment
There was a problem hiding this comment.
Thanks! Left some questions that need to be resolved.
| logger->debug("EEPROM data changed (ID: {} -> {}), reconfiguring ...", currentEepromId, latestEepromId); | ||
| calibrationSet = false; | ||
| previousShiftFactor = 0; | ||
| refreshAlignToTransform = true; |
There was a problem hiding this comment.
Do we need refreshAlignToTransform? To me it looks like the inverse of calibrationSet and they are inherently linked (when refreshAlignToTransform == True then setCalibration has to be false to properly update the maps). The updateAlignToData lambda has a capture by reference for setCalibration bool and could have the same logic as extractCalibrationData where if (calibrationSet) return;.
|
|
||
| if(refreshAlignToTransform) { | ||
| if(auto latestAlignToImg = inputAlignTo.tryGet<ImgFrame>()) { | ||
| updateAlignToData(latestAlignToImg); |
There was a problem hiding this comment.
This is going to be unpredictable behaviour. Lets say that the eeprom ID got updated but an ImgFrame with the new ImgTransformation hasn`t gotten to the inputAlignTo queue (eg. Camera hasnt generated a new ImgFrame yet), then latestAlignToImg will be a message with the old ImgTransformation. So the update wont actually update the ImgTransformation.
I'm guessing this happens more often if you dont have camera node connected directly to the ImageAling node but have some other nodes in between that increase the time between between new inputAlignTo messages. IMO best solution is to store the previous ImgTransformation and only update when that changes. But I have that made in another branch where its also fallsback to previous impl in case no ImgTransformations available.
| } | ||
|
|
||
| if(refreshAlignToTransform) { | ||
| if(auto latestAlignToImg = inputAlignTo.tryGet<ImgFrame>()) { |
There was a problem hiding this comment.
I would preffer get<ImgFrame>() instead of tryGet because this line will wait for new message to arrive instead of hoping that on the next iteration of this while loop there will be a message available.
3 participants
Summary
Fix runtime calibration handling in ImageAlign on branch bugfix/align_runtime_calibration_change.
What changed
rebuilding alignment maps.
Why
Before this fix, calling device.setCalibration(...) during streaming could leave ImageAlign using stale alignment
state. In practice this caused:
This change makes the align node switch coherently to the new calibration state without requiring a pipeline restart.
Testing
Summary by CodeRabbit
Bug Fixes
Tests