Change that intrinsics are taken from transformations in DCL#1795
Conversation
📝 WalkthroughWalkthroughDynamicCalibration now stores per-side ChangesImgTransformation Calibration Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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/DynamicCalibrationNode.cpp (1)
53-75:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFinish the intrinsics migration for pixel-space quality outputs.
This helper now feeds DCL with transformation-derived intrinsics, but
runQualityCheck()andrunCalibration()still convert Sampson error back to pixels withgetCameraIntrinsics(..., resolutionA). On cropped or scaled streams,sampsonErrorCurrent/Newwill be reported against a different camera matrix than the one used for calibration. ThreadImgTransformationinto that scale calculation as well, or derive the scale directly fromcameraMatrixA.🤖 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/DynamicCalibrationNode.cpp` around lines 53 - 75, convertDaiCalibrationToDcl now supplies transformation-derived intrinsics but runQualityCheck() and runCalibration() still compute pixel-scale using getCameraIntrinsics(..., resolutionA), causing Sampson errors to be reported against the wrong intrinsics for cropped/scaled streams; fix by threading the ImgTransformation (or its derived cameraMatrixA) into the scale calculation so the same intrinsics used to create DCL calib are used when converting Sampson error to pixels—specifically, update calls in runQualityCheck() and runCalibration() that call getCameraIntrinsics(..., resolutionA) to instead accept an ImgTransformation (or extract focal/px from cameraMatrixA returned by convertDaiCalibrationToDcl) and use those values to compute the pixel-scale for sampsonErrorCurrent/sampsonErrorNew consistently with convertDaiCalibrationToDcl.
🤖 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/DynamicCalibrationNode.cpp`:
- Around line 440-441: The node currently only sets imgTransformationA and
imgTransformationB at initialization, but runLoadImage() later replaces
leftFrame/rightFrame; update the cached transforms whenever new frames are
loaded by calling leftFrame->getTransformation() and
rightFrame->getTransformation() inside runLoadImage() (or explicitly
document/enforce that transformations are immutable for the node lifetime and
assert no change). Locate uses of leftFrame/rightFrame and update the
imgTransformationA/B assignments in the frame-loading path so downstream
calibration/metrics always use the fresh transforms.
---
Outside diff comments:
In `@src/pipeline/node/DynamicCalibrationNode.cpp`:
- Around line 53-75: convertDaiCalibrationToDcl now supplies
transformation-derived intrinsics but runQualityCheck() and runCalibration()
still compute pixel-scale using getCameraIntrinsics(..., resolutionA), causing
Sampson errors to be reported against the wrong intrinsics for cropped/scaled
streams; fix by threading the ImgTransformation (or its derived cameraMatrixA)
into the scale calculation so the same intrinsics used to create DCL calib are
used when converting Sampson error to pixels—specifically, update calls in
runQualityCheck() and runCalibration() that call getCameraIntrinsics(...,
resolutionA) to instead accept an ImgTransformation (or extract focal/px from
cameraMatrixA returned by convertDaiCalibrationToDcl) and use those values to
compute the pixel-scale for sampsonErrorCurrent/sampsonErrorNew consistently
with convertDaiCalibrationToDcl.
🪄 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: 3cc53e3c-c35d-4f90-ba9f-c63c7d91f20b
📒 Files selected for processing (3)
include/depthai/pipeline/node/DynamicCalibrationNode.hppsrc/pipeline/node/DynamicCalibrationNode.cppsrc/pipeline/node/DynamicCalibrationUtils.hpp
📜 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/DynamicCalibrationNode.cpp
🔇 Additional comments (1)
src/pipeline/node/DynamicCalibrationUtils.hpp (1)
4-30: LGTM!
| imgTransformationA = leftFrame->getTransformation(); | ||
| imgTransformationB = rightFrame->getTransformation(); |
There was a problem hiding this comment.
Refresh the cached transformation when loading new frames.
imgTransformationA/B are only assigned during initialization, but later calibration and metrics runs operate on frames loaded in runLoadImage(). If the upstream path changes crop/resize/warp after startup, the node will keep calibrating newer images with stale intrinsics. Update these members from each loaded pair, or enforce that the transformation is immutable for the node lifetime.
🤖 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/DynamicCalibrationNode.cpp` around lines 440 - 441, The
node currently only sets imgTransformationA and imgTransformationB at
initialization, but runLoadImage() later replaces leftFrame/rightFrame; update
the cached transforms whenever new frames are loaded by calling
leftFrame->getTransformation() and rightFrame->getTransformation() inside
runLoadImage() (or explicitly document/enforce that transformations are
immutable for the node lifetime and assert no change). Locate uses of
leftFrame/rightFrame and update the imgTransformationA/B assignments in the
frame-loading path so downstream calibration/metrics always use the fresh
transforms.
|
LFG! |
MaticTonin
changed the title
JakubFara
left a comment
JakubFara
left a comment
There was a problem hiding this comment.
LGTM, just consts; we can check the intrinsic in loadImages, but I do not insist on it.
| ImgTransformation& imgTransformationA, | ||
| ImgTransformation& imgTransformationB) { |
| ImgTransformation& imgTransformationA, | ||
| ImgTransformation& imgTransformationB); |
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 (2)
src/pipeline/node/DynamicCalibrationNode.cpp (1)
61-76:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winAdd bounds check before indexing distortion coefficients in
createDclCalibration.
createDclCalibration(lines 131–132 for Perspective, 137–138 for Fisheye) directly accessesdistortionCoefficients[i]up to index 13 or 3 without verifying the vector size.ImgTransformation::getDistortionCoefficients()returns an unpaddedstd::vector that can be any size, including empty (as shown in tests). If the vector has fewer elements than the loop expects, this causes out-of-bounds access and undefined behavior.Ensure the distortion vector size matches the expected model size before indexing, or fall back to
currentCalibration.getDistortionCoefficients(socketA/B)which is guaranteed to be padded to 14 entries.🤖 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/DynamicCalibrationNode.cpp` around lines 61 - 76, createDclCalibration currently indexes distortionCoefficients[i] up to model-specific sizes (Perspective up to 14, Fisheye up to 4) without checking vector length, causing OOB when ImgTransformation::getDistortionCoefficients() returns a shorter vector; update createDclCalibration to check the incoming distortionCoefficients.size() against the required size for the chosen distortion model and if too short, use currentCalibration.getDistortionCoefficients(socketA/B) (the padded 14-entry fallback) or otherwise pad/initialize remaining entries safely before indexing so loops in createDclCalibration never access beyond the vector bounds.src/pipeline/node/DynamicCalibrationUtils.hpp (1)
25-30: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd explicit include for
ImgTransformation.The function signature uses
const ImgTransformation¶meters, but this header does not explicitly include<depthai/common/ImgTransformations.hpp>. The type is not forward-declarable sinceImgTransformationis a struct with substantial member data and is used as a const reference. The header currently relies on transitive includes from<DynamicCalibration.hpp>, which is fragile; if the include graph changes, this becomes a compile error. Add an explicit include to make the header self-contained.🤖 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/DynamicCalibrationUtils.hpp` around lines 25 - 30, The header declares convertDaiCalibrationToDcl using const ImgTransformation& but never includes the ImgTransformation definition; add an explicit include for depthai/common/ImgTransformations.hpp at the top of src/pipeline/node/DynamicCalibrationUtils.hpp so the ImgTransformation type is available (do not rely on transitive includes from DynamicCalibration.hpp) and ensure the file becomes self-contained for the convertDaiCalibrationToDcl declaration.
♻️ Duplicate comments (1)
src/pipeline/node/DynamicCalibrationNode.cpp (1)
440-441:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStale
imgTransformationA/Bafter initialization (duplicate of prior review).These members are still only written once during
initializePipelineand not refreshed whenrunLoadImage()pulls a new pair of frames. Any change to the per-frameImgTransformationbetween init and a latersetCalibration/computeMetricswill cause those calls to use stale intrinsics. Either re-assignimgTransformationA = leftFrame->getTransformation();andimgTransformationB = rightFrame->getTransformation();insiderunLoadImage()(alongside the frame load at lines 374–375), or document and enforce immutability of the transformations for the node lifetime.🔧 Suggested fix to refresh inside runLoadImage()
auto leftFrame = inSyncGroup->get<dai::ImgFrame>(leftInputName); auto rightFrame = inSyncGroup->get<dai::ImgFrame>(rightInputName); if(!leftFrame || !rightFrame) { logger->trace("WARNING: Missing image(s) in MessageGroup (left={}, right={})", leftFrame ? "ok" : "missing", rightFrame ? "ok" : "missing"); return DynamicCalibration::ErrorCode::MISSING_IMAGE; } + + // Keep cached transformations in sync with the most recent frames so + // downstream setCalibration()/computeMetrics() see up-to-date intrinsics. + imgTransformationA = leftFrame->getTransformation(); + imgTransformationB = rightFrame->getTransformation();🤖 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/DynamicCalibrationNode.cpp` around lines 440 - 441, The imgTransformationA and imgTransformationB members are only set in initializePipeline and become stale when runLoadImage replaces frames; update runLoadImage to reassign imgTransformationA = leftFrame->getTransformation() and imgTransformationB = rightFrame->getTransformation() immediately after the new frames are loaded (where leftFrame/rightFrame are updated) so subsequent setCalibration and computeMetrics use current intrinsics, or alternatively document/enforce that transformations are immutable for the node lifetime in initializePipeline and refuse frame updates in runLoadImage.
🤖 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/DynamicCalibrationNode.cpp`:
- Line 268: setCalibration and computeMetrics are using stale member fields
imgTransformationA/imgTransformationB (they were only set once in
initializePipeline) so subsequent calls use init-time intrinsics; modify both
setCalibration and computeMetrics to not read the members but instead use the
forwarded transformations passed into the call site (the
imgTransformationA/imgTransformationB variables that are being forwarded to
DclUtils::convertDaiCalibrationToDcl) or retrieve the current transforms from
the pipeline/handler at the start of each call; update references inside
setCalibration/computeMetrics (and any use around convertDaiCalibrationToDcl) to
consume the fresh transformations rather than the cached members.
---
Outside diff comments:
In `@src/pipeline/node/DynamicCalibrationNode.cpp`:
- Around line 61-76: createDclCalibration currently indexes
distortionCoefficients[i] up to model-specific sizes (Perspective up to 14,
Fisheye up to 4) without checking vector length, causing OOB when
ImgTransformation::getDistortionCoefficients() returns a shorter vector; update
createDclCalibration to check the incoming distortionCoefficients.size() against
the required size for the chosen distortion model and if too short, use
currentCalibration.getDistortionCoefficients(socketA/B) (the padded 14-entry
fallback) or otherwise pad/initialize remaining entries safely before indexing
so loops in createDclCalibration never access beyond the vector bounds.
In `@src/pipeline/node/DynamicCalibrationUtils.hpp`:
- Around line 25-30: The header declares convertDaiCalibrationToDcl using const
ImgTransformation& but never includes the ImgTransformation definition; add an
explicit include for depthai/common/ImgTransformations.hpp at the top of
src/pipeline/node/DynamicCalibrationUtils.hpp so the ImgTransformation type is
available (do not rely on transitive includes from DynamicCalibration.hpp) and
ensure the file becomes self-contained for the convertDaiCalibrationToDcl
declaration.
---
Duplicate comments:
In `@src/pipeline/node/DynamicCalibrationNode.cpp`:
- Around line 440-441: The imgTransformationA and imgTransformationB members are
only set in initializePipeline and become stale when runLoadImage replaces
frames; update runLoadImage to reassign imgTransformationA =
leftFrame->getTransformation() and imgTransformationB =
rightFrame->getTransformation() immediately after the new frames are loaded
(where leftFrame/rightFrame are updated) so subsequent setCalibration and
computeMetrics use current intrinsics, or alternatively document/enforce that
transformations are immutable for the node lifetime in initializePipeline and
refuse frame updates in runLoadImage.
🪄 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: f0b92acd-e511-4d84-9975-485ea1b1142b
📒 Files selected for processing (2)
src/pipeline/node/DynamicCalibrationNode.cppsrc/pipeline/node/DynamicCalibrationUtils.hpp
📜 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/DynamicCalibrationNode.cpp
🔇 Additional comments (2)
src/pipeline/node/DynamicCalibrationNode.cpp (2)
100-105: LGTM!
459-459: LGTM!
| device->flashCalibration(handler); | ||
| } | ||
| auto [calibA, calibB] = DclUtils::convertDaiCalibrationToDcl(handler, daiSocketA, daiSocketB, resolutionA, resolutionB); | ||
| auto [calibA, calibB] = DclUtils::convertDaiCalibrationToDcl(handler, daiSocketA, daiSocketB, imgTransformationA, imgTransformationB); |
There was a problem hiding this comment.
Call sites correctly forward the cached transformations, but they ride on a stale-state bug.
Both setCalibration (line 268) and computeMetrics (line 274) read the member fields imgTransformationA/imgTransformationB. Those are only ever written once in initializePipeline (lines 440–441), so every subsequent invocation here uses init-time intrinsics. If the upstream pipeline ever changes crop/resize/warp after startup, both flows recalibrate / score with stale intrinsics. Root cause and recommended fix are noted at lines 440–441.
Also applies to: 274-274
🤖 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/DynamicCalibrationNode.cpp` at line 268, setCalibration and
computeMetrics are using stale member fields
imgTransformationA/imgTransformationB (they were only set once in
initializePipeline) so subsequent calls use init-time intrinsics; modify both
setCalibration and computeMetrics to not read the members but instead use the
forwarded transformations passed into the call site (the
imgTransformationA/imgTransformationB variables that are being forwarded to
DclUtils::convertDaiCalibrationToDcl) or retrieve the current transforms from
the pipeline/handler at the start of each call; update references inside
setCalibration/computeMetrics (and any use around convertDaiCalibrationToDcl) to
consume the fresh transformations rather than the cached members.
moratom
left a comment
moratom
left a comment
There was a problem hiding this comment.
This doens't enable actual dyanmic calibration < 800p yet thought right?
3 participants
Purpose
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