ccalib: fix misleading assertion when no calibration views survive - #4196
Open
SomSamantray wants to merge 2 commits into
Open
ccalib: fix misleading assertion when no calibration views survive#4196SomSamantray wants to merge 2 commits into
SomSamantray wants to merge 2 commits into
Conversation
…nidir calibration has no valid views cv::omnidir::calibrate() and cv::omnidir::stereoCalibrate() both filter input views by initial-pose reprojection error before optimization. When every view is rejected (e.g. mismatched/uncorrelated object-image point correspondences, as reported in opencv/opencv#28462), the code proceeded into computeJacobian()/computeJacobianStereo() with empty point vectors, tripping CV_Assert(!objectPoints.empty() && objectPoints.type() == CV_64FC3). Since only the emptiness half of that assertion was actually false, the error message misleadingly pointed at a type mismatch that did not exist. Add explicit CV_Error guards at the point where each function discovers zero surviving views, with messages that name the actual cause (no views survived initialization, or no views were valid for both cameras) instead of the confusing low-level assertion. For stereoCalibrate(), the guard lives in internal::initializeStereoCalibration() rather than stereoCalibrate() itself, since the intersection-empty case previously also crashed one level earlier via Mat::copyTo() on a fixed-size temporary from OutputArray::getMat() -- fixed here too by copying into the OutputArray directly instead of through getMat(). Also adds modules/ccalib's first test suite (it had none), covering the error path for both functions plus a synthetic-projection-based happy-path regression test for calibrate().
- Add TEST(CV_OmnidirStereoCalibrate, succeeds_with_valid_overlapping_views) to cover the normal working stereo case, per the testing reviewer's finding that only the error path was exercised for stereoCalibrate(). - Fix the last remaining instance of the old _idx.copyTo(idx.getMat()) pattern in cv::omnidir::calibrate()'s idx output block, matching the two other call sites already fixed, per the reliability reviewer's residual risk note. - Seed the two RNG-based error-path tests explicitly instead of drawing from the shared cv::theRNG() singleton, so they're reproducible independent of run order or other tests' RNG draws.
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
1 participant
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.
cv2.omnidir.calibratefailed with a confusing assertion that looked like a type bug even when the input arrays had the correct type, shape, and were non-empty:The actual cause has nothing to do with types.
omnidir::calibrate()filters out any calibration view whose initial-pose reprojection error exceeds an internal threshold; if every view fails that filter (e.g. the object/image point correspondences don't come from a real calibration target), the point vectors end up empty and the code still proceeds intocomputeJacobian(), tripping the assertion above on its emptiness half only — the type half was never actually false.This PR replaces that misleading assertion with two explicit, actionable errors:
omnidir::calibrate()now reports "no calibration views survived initial pose estimation" when every view is rejected.omnidir::stereoCalibrate()has an independent failure mode not covered by the first fix: two per-camera views can each individually survive filtering, yet their surviving-view sets can fail to intersect. That path used to crash even earlier, viaMat::copyTo()on a fixed-sizeOutputArray::getMat()temporary that refuses to "reallocate" an empty array even to the same size. Both issues are fixed together — the intersection-empty case is now a clear "no calibration views are valid for both cameras" error.modules/ccalibhad no test suite at all before this change; this PR adds its first one, covering the error path for both functions plus a synthetic-projection-based happy-path regression test proving valid calibration is unaffected.Fixes opencv/opencv#28462
Validation
Built and ran the new test suite locally (
opencv_test_ccalib, scoped build ofcore, imgproc, geometry, calib, objdetect, features, xfeatures2d, highgui, imgcodecs, ccalib, ts): all 4 tests pass, including the happy-path tests that confirm valid single-camera and stereo calibration are unaffected.Pull Request Readiness Checklist
See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request
Patch to opencv_extra has the same branch name.