Fix crash in ap3p #23607

alexander-varjo · 2023-05-11T14:26:03Z

Pull Request Readiness Checklist

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

I agree to contribute to the project under Apache 2 License.
To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV
The PR is proposed to the proper branch
There is a reference to the original bug report and related work
There is accuracy test, performance test and test data in opencv_extra repository, if applicable
Patch to opencv_extra has the same branch name.
The feature is well documented and sample code can be built with the project CMake

dkurt · 2023-05-12T06:12:38Z

modules/calib3d/src/ap3p.cpp

@@ -256,6 +256,8 @@ int ap3p::computePoses(const double featureVectors[3][4],
    int nb_solutions = 0;
    for (int i = 0; i < 4; ++i) {
        double ctheta1p = s[i];
+        if (isnan(ctheta1p))
+            continue;


Hi! Can you provide a reproducer so this can be covered by some test?

Hi!
Sorry for the delay. Here is the reproducer:

{ const float fx = 1, fy = 1, cx = 0, cy = 0; cv::ap3p solver(fx, fy, cx, cy); const std::array<cv::Point2d, 3> cameraPts = { // cv::Point2d{0.042784865945577621, 0.59844839572906494}, // cv::Point2d{-0.028428621590137482, 0.60354739427566528}, // cv::Point2d{0.0046037044376134872, 0.70674681663513184} // }; const std::array<cv::Point3d, 3> modelPts = { // cv::Point3d{-0.043258000165224075, 0.020459245890378952, -0.0069921980611979961}, // cv::Point3d{-0.045648999512195587, 0.0029820732306689024, 0.0079000638797879219}, // cv::Point3d{-0.043276999145746231, -0.013622495345771313, 0.0080113131552934647} // }; double R[4][3][3]; double t[4][3]; const int nb_solutions = solver.solve(R, t, // cameraPts[0].x, cameraPts[0].y, modelPts[0].x, modelPts[0].y, modelPts[0].z, // cameraPts[1].x, cameraPts[1].y, modelPts[1].x, modelPts[1].y, modelPts[1].z, // cameraPts[2].x, cameraPts[2].y, modelPts[2].x, modelPts[2].y, modelPts[2].z, // 0, 0, 0, 0, 0, false); }

Or, if rely on a public API:

std::vector<Mat> R, t; solveP3P(modelPts, cameraPts, Mat::eye(3, 3, CV_64F), Mat(), R, t, SOLVEPNP_AP3P);

@alexander-varjo, can you please share your compiler version and target CPU architecture?

My setup:

Compiler: MSVC 14.29.30133

CPU: i9-10850K

Did you run directly cv::ap3p::solve() or you just used cv::solveP3P() when you tested the reproducer?

Both, for me problem is not reproduced so it might depend on a compiler

Can reproduce with the following code on MSVC 14.35.32215. The app not crashed but nan returned.

Let me check if there is no other solution to this problem.

--- a/modules/calib3d/src/ap3p.cpp +++ b/modules/calib3d/src/ap3p.cpp @@ -3,6 +3,7 @@ #include <cmath> #include <complex> +#include <iostream> #if defined (_MSC_VER) && (_MSC_VER <= 1700) static inline double cbrt(double x) { return (double)cv::cubeRoot((float)x); }; #endif @@ -256,6 +257,7 @@ int ap3p::computePoses(const double featureVectors[3][4], int nb_solutions = 0; for (int i = 0; i < 4; ++i) { double ctheta1p = s[i]; + std::cout << i << " " << ctheta1p << std::endl; if (abs(ctheta1p) > 1) continue; double stheta1p = sqrt(1 - ctheta1p * ctheta1p);

#include <opencv2/opencv.hpp> using namespace cv; int main(int argc, char** argv) { const std::array<cv::Point2d, 3> cameraPts = { // cv::Point2d{0.042784865945577621, 0.59844839572906494}, // cv::Point2d{-0.028428621590137482, 0.60354739427566528}, // cv::Point2d{0.0046037044376134872, 0.70674681663513184} // }; const std::array<cv::Point3d, 3> modelPts = { // cv::Point3d{-0.043258000165224075, 0.020459245890378952, -0.0069921980611979961}, // cv::Point3d{-0.045648999512195587, 0.0029820732306689024, 0.0079000638797879219}, // cv::Point3d{-0.043276999145746231, -0.013622495345771313, 0.0080113131552934647} // }; std::vector<Mat> R, t; solveP3P(modelPts, cameraPts, Mat::eye(3, 3, CV_64F), Mat(), R, t, SOLVEPNP_AP3P); for (int i = 0; i < R.size(); ++i) { std::cout << R[i] << std::endl; std::cout << t[i] << std::endl; std::cout << " " << std::endl; } return 0; }

0 nan 1 nan 2 -nan(ind) 3 -nan(ind)

The issue is reproducible with ENABLE_FAST_MATH option in Cmake. I do not get crash, but R and t contains NaNs.

asmorkalov · 2023-06-05T10:40:59Z

@alexander-varjo Friendly reminder.

asmorkalov · 2023-06-16T07:27:49Z

@alexander-varjo Friendly reminder.

asmorkalov · 2023-06-23T17:50:15Z

@dkurt Could you take a look on the reproducer?

asmorkalov · 2023-07-12T12:58:30Z

@dkurt Friendly reminder.

asmorkalov · 2023-07-25T11:27:27Z

I squashed commits, added test and replaced std::isnan with OpenCV version cvIsNaN. The issue is not reproducible even with -DENABLE_FAST_MATH=ON option, but requires this fix: #23881

dkurt · 2023-07-25T11:38:41Z

@asmorkalov, For the mentioned test data both R, t will be empty. Just wanted to verify if that's expected and we don't need to look for a root problem in the solver.

modules/calib3d/test/test_solvepnp_ransac.cpp

asmorkalov · 2023-07-27T11:29:46Z

@asmorkalov, For the mentioned test data both R, t will be empty. Just wanted to verify if that's expected and we don't need to look for a root problem in the solver.

@dkurt It's hard to say. What I see from implementation it's pure numerical method. There is no randomization or branches in the touched code. The coefficients in solveQuartic are ill-defined and the result is not stable. My understanding, that solveP3P should not return NaNs and all NaN solutions for solveQuartic should be ignored.

dkurt · 2023-08-28T12:28:13Z

I just checked that P3P finds valid two answers for the given data. Does it mean that it make sense to find a bug in AP3P implementation instead?

#include <opencv2/opencv.hpp>
using namespace cv;
int main(int argc, char** argv) {
    const std::array<cv::Point2d, 3> cameraPts = {                //
        cv::Point2d{0.042784865945577621, 0.59844839572906494},   //
        cv::Point2d{-0.028428621590137482, 0.60354739427566528},  //
        cv::Point2d{0.0046037044376134872, 0.70674681663513184}   //
    };
    const std::array<cv::Point3d, 3> modelPts = {                                          //
        cv::Point3d{-0.043258000165224075, 0.020459245890378952, -0.0069921980611979961},  //
        cv::Point3d{-0.045648999512195587, 0.0029820732306689024, 0.0079000638797879219},  //
        cv::Point3d{-0.043276999145746231, -0.013622495345771313, 0.0080113131552934647}   //
    };
    std::vector<Mat> R, t;
    solveP3P(modelPts, cameraPts, Mat::eye(3, 3, CV_64F), Mat(), R, t, SOLVEPNP_P3P);
    // Try apply rvec and tvec to get model points from camera points.
    for (int i = 0; i < R.size(); ++i) {
        std::cout << "Solution " << i << std::endl;
        std::cout << "rvec: " << R[i].reshape(1, 1) << std::endl;
        std::cout << "tvec: " << t[i].reshape(1, 1) << std::endl;
        for (int j = 0; j < 3; ++j) {
            Mat res(modelPts[j]);
            res = res.reshape(1, 3);
            Mat transform;
            cv::Rodrigues(R[i], transform);
            res = transform * res + t[i];
            std::cout << res.at<double>(0) / res.at<double>(2) << " " << res.at<double>(1) / res.at<double>(2) << std::endl;
        }
        std::cout << std::endl;
    }
    return 0;
}

Solution 0
rvec: [-1.171336208245342, -0.4659149293886356, 2.32178684497184]
tvec: [-0.01872864087862001, 0.08790966014410974, 0.06506316694719004]
0.0427849 0.598448
-0.0284286 0.603547
0.0046037 0.706747
Solution 1
rvec: [1.587639684352496, -0.4999068796382095, -0.3813392742525927]
tvec: [0.04003365897081281, 0.04628230938470984, 0.1056643523183554]
0.0427849 0.598448
-0.0284286 0.603547
0.0046037 0.706747

dkurt

I have found a bug in solveQuartic method. It computes incorrect roots for a given test example. Solution is in progress.
Current: [-0.0759858, -0.0759858, -0.0759858, -0.0759858]
Expected: [-0.07598547, -0.07598547, -0.70197669, 0.55000499] (according to numpy.roots).

dkurt

Check Clang issues:

dkurt · 2023-08-31T09:45:08Z

Can confirm that this PR passes on Raspberry Pi 3 and Clang 3.9.1 (https://downloads.raspberrypi.org/raspbian/images/raspbian-2018-11-15/).

[----------] Global test environment tear-down
[==========] 153 tests from 65 test cases ran. (867563 ms total)
[  PASSED  ] 153 tests.

Also reproduced failed tests with disabled fixes by @tomoaki0705 on the same setup:

[ RUN      ] Calib3d_SolveP3P.accuracy
mode: no distortion, method: SOLVEPNP_P3P -> 72.2% (maxErrT: 0, maxErrR: 0, meanErrT: 0, meanErrR: 0, medErrT: 0, medErrR: 0)
approximate translation threshold for 0.7: 0, approximate rotation threshold for 0.7: 0
mode: no distortion, method: SOLVEPNP_AP3P -> 4.8% (maxErrT: 0, maxErrR: 0, meanErrT: 0, meanErrR: 0, medErrT: 0, medErrR: 0)
approximate translation threshold for 0.7: 0, approximate rotation threshold for 0.7: 0
mode: distorsion, method: SOLVEPNP_P3P -> 75.5% (maxErrT: 0, maxErrR: 0, meanErrT: 0, meanErrR: 0, medErrT: 0, medErrR: 0)
approximate translation threshold for 0.7: 0, approximate rotation threshold for 0.7: 0
mode: distorsion, method: SOLVEPNP_AP3P -> 4.9% (maxErrT: 0, maxErrR: 0, meanErrT: 0, meanErrR: 0, medErrT: 0, medErrR: 0)
approximate translation threshold for 0.7: 0, approximate rotation threshold for 0.7: 0
/home/pi/opencv/modules/ts/src/ts.cpp:618: Failure
Failed
        failure reason: Bad accuracy
        test case #-1
        seed: 00000000000c5c4f
-----------------------------------
        LOG:
Invalid accuracy for SOLVEPNP_AP3P, failed 952 tests from 1000, no distortion, maxErrT: 0.000000, maxErrR: 0.000000, meanErrT: 0.000000, meanErrR: 0.000000, medErrT: 0.000000, medErrR: 0.000000
Invalid accuracy for SOLVEPNP_AP3P, failed 951 tests from 1000, distorsion, maxErrT: 0.000000, maxErrR: 0.000000, meanErrT: 0.000000, meanErrR: 0.000000, medErrT: 0.000000, medErrR: 0.000000
-----------------------------------
[  FAILED  ] Calib3d_SolveP3P.accuracy (20164 ms)

[ RUN      ] Calib3d_SolvePnP.accuracy
mode: no distortion, method: SOLVEPNP_ITERATIVE -> 98.2% (maxErrT: 7.31387e-05, maxErrR: 4.27558e-07, meanErrT: 2.35055e-07, meanErrR: 2.0052e-08, medErrT: 7.64453e-08, medErrR: 1.14873e-08)
approximate translation threshold for 0.7: 1.19251e-07, approximate rotation threshold for 0.7: 2.7426e-08
mode: no distortion, method: SOLVEPNP_EPNP -> 94.9% (maxErrT: 8.99499e-05, maxErrR: 5.65506e-06, meanErrT: 3.77629e-07, meanErrR: 4.69644e-08, medErrT: 1.20591e-07, medErrR: 1.75538e-08)
approximate translation threshold for 0.7: 2.11127e-07, approximate rotation threshold for 0.7: 5.0379e-08
mode: no distortion, method: SOLVEPNP_P3P -> 72.2% (maxErrT: 9.07199, maxErrR: 1.16209, meanErrT: 0.0266452, meanErrR: 0.00339638, medErrT: 5.8428e-05, medErrR: 7.3561e-06)
approximate translation threshold for 0.7: 0.000170339, approximate rotation threshold for 0.7: 2.01853e-05
mode: no distortion, method: SOLVEPNP_DLS (remaped to SOLVEPNP_EPNP) -> 94.9% (maxErrT: 8.99499e-05, maxErrR: 5.65506e-06, meanErrT: 3.77629e-07, meanErrR: 4.69644e-08, medErrT: 1.20591e-07, medErrR: 1.75538e-08)
approximate translation threshold for 0.7: 2.11127e-07, approximate rotation threshold for 0.7: 5.0379e-08
mode: no distortion, method: SOLVEPNP_UPNP (remaped to SOLVEPNP_EPNP) -> 94.9% (maxErrT: 8.99499e-05, maxErrR: 5.65506e-06, meanErrT: 3.77629e-07, meanErrR: 4.69644e-08, medErrT: 1.20591e-07, medErrR: 1.75538e-08)
approximate translation threshold for 0.7: 2.11127e-07, approximate rotation threshold for 0.7: 5.0379e-08
mode: no distortion, method: SOLVEPNP_AP3P -> 4.3% (maxErrT: 8.64444, maxErrR: 1.65803, meanErrT: 2.4403, meanErrR: 0.336879, medErrT: 0.795441, medErrR: 0.0991689)
approximate translation threshold for 0.7: 4.74074, approximate rotation threshold for 0.7: 0.622992
mode: no distortion, method: SOLVEPNP_IPPE -> 100% (maxErrT: -1, maxErrR: -1, meanErrT: -1, meanErrR: -1, medErrT: -1, medErrR: -1)
approximate translation threshold for 0.7: -1, approximate rotation threshold for 0.7: -1
mode: no distortion, method: SOLVEPNP_IPPE_SQUARE -> 100% (maxErrT: -1, maxErrR: -1, meanErrT: -1, meanErrR: -1, medErrT: -1, medErrR: -1)
approximate translation threshold for 0.7: -1, approximate rotation threshold for 0.7: -1
mode: no distortion, method: SOLVEPNP_SQPNP -> 82.5% (maxErrT: 0.000404908, maxErrR: 0.00014251, meanErrT: 1.63976e-06, meanErrR: 4.46639e-07, medErrT: 3.3522e-07, medErrR: 9.69447e-08)
approximate translation threshold for 0.7: 6.27461e-07, approximate rotation threshold for 0.7: 6.55592e-08
mode: distorsion, method: SOLVEPNP_ITERATIVE -> 98.2% (maxErrT: 7.05758e-05, maxErrR: 1.09816e-06, meanErrT: 3.41763e-07, meanErrR: 2.33893e-08, medErrT: 7.41102e-08, medErrR: 1.09326e-08)
approximate translation threshold for 0.7: 1.24488e-07, approximate rotation threshold for 0.7: 7.85199e-09
mode: distorsion, method: SOLVEPNP_EPNP -> 94.6% (maxErrT: 0.000602186, maxErrR: 1.89137e-05, meanErrT: 1.26335e-06, meanErrR: 8.09078e-08, medErrT: 1.19731e-07, medErrR: 1.70436e-08)
approximate translation threshold for 0.7: 2.3171e-07, approximate rotation threshold for 0.7: 3.63202e-09
mode: distorsion, method: SOLVEPNP_P3P -> 75.5% (maxErrT: 0.994515, maxErrR: 0.128852, meanErrT: 0.00417844, meanErrR: 0.000540457, medErrT: 5.61029e-05, medErrR: 7.18703e-06)
approximate translation threshold for 0.7: 0.000141204, approximate rotation threshold for 0.7: 1.80878e-05
mode: distorsion, method: SOLVEPNP_DLS (remaped to SOLVEPNP_EPNP) -> 94.6% (maxErrT: 0.000602186, maxErrR: 1.89137e-05, meanErrT: 1.26335e-06, meanErrR: 8.09078e-08, medErrT: 1.19731e-07, medErrR: 1.70436e-08)
approximate translation threshold for 0.7: 2.3171e-07, approximate rotation threshold for 0.7: 3.63202e-09
mode: distorsion, method: SOLVEPNP_UPNP (remaped to SOLVEPNP_EPNP) -> 94.6% (maxErrT: 0.000602186, maxErrR: 1.89137e-05, meanErrT: 1.26335e-06, meanErrR: 8.09078e-08, medErrT: 1.19731e-07, medErrR: 1.70436e-08)
approximate translation threshold for 0.7: 2.3171e-07, approximate rotation threshold for 0.7: 3.63202e-09
mode: distorsion, method: SOLVEPNP_AP3P -> 5% (maxErrT: 8.88675, maxErrR: 1.68755, meanErrT: 2.57238, meanErrR: 0.353149, medErrT: 0.894434, medErrR: 0.109513)
approximate translation threshold for 0.7: 4.9921, approximate rotation threshold for 0.7: 0.637086
mode: distorsion, method: SOLVEPNP_IPPE -> 100% (maxErrT: -1, maxErrR: -1, meanErrT: -1, meanErrR: -1, medErrT: -1, medErrR: -1)
approximate translation threshold for 0.7: -1, approximate rotation threshold for 0.7: -1
mode: distorsion, method: SOLVEPNP_IPPE_SQUARE -> 100% (maxErrT: -1, maxErrR: -1, meanErrT: -1, meanErrR: -1, medErrT: -1, medErrR: -1)
approximate translation threshold for 0.7: -1, approximate rotation threshold for 0.7: -1
mode: distorsion, method: SOLVEPNP_SQPNP -> 83.8% (maxErrT: 0.00054474, maxErrR: 9.80958e-05, meanErrT: 2.51335e-06, meanErrR: 4.92474e-07, medErrT: 3.49072e-07, medErrR: 9.49575e-08)
approximate translation threshold for 0.7: 5.93851e-07, approximate rotation threshold for 0.7: 2.09181e-07
/home/pi/opencv/modules/ts/src/ts.cpp:618: Failure
Failed
        failure reason: Bad accuracy
        test case #-1
        seed: 00000000000c5c4f
-----------------------------------
        LOG:
Invalid accuracy for SOLVEPNP_AP3P, failed 957 tests from 1000, no distortion, maxErrT: 8.644444, maxErrR: 1.658034, meanErrT: 2.440300, meanErrR: 0.336879, medErrT: 0.795441, medErrR: 0.099169
Invalid accuracy for SOLVEPNP_AP3P, failed 950 tests from 1000, distorsion, maxErrT: 8.886748, maxErrR: 1.687546, meanErrT: 2.572377, meanErrR: 0.353149, medErrT: 0.894434, medErrR: 0.109513
-----------------------------------
[  FAILED  ] Calib3d_SolvePnP.accuracy (90396 ms)

asmorkalov

👍

…patch-1 Fix crash in ap3p opencv#23607 ### Pull Request Readiness Checklist See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request - [ ] I agree to contribute to the project under Apache 2 License. - [ ] To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV - [x] The PR is proposed to the proper branch - [ ] There is a reference to the original bug report and related work - [x] There is accuracy test, performance test and test data in opencv_extra repository, if applicable Patch to opencv_extra has the same branch name. - [x] The feature is well documented and sample code can be built with the project CMake

dkurt reviewed May 12, 2023

View reviewed changes

asmorkalov added category: calib3d pr: needs test New functionality requires minimal tests set labels May 12, 2023

dkurt self-assigned this Jun 26, 2023

asmorkalov force-pushed the alexander-varjo-patch-1 branch from 234de5f to 5eb9556 Compare July 25, 2023 11:25

asmorkalov approved these changes Jul 25, 2023

View reviewed changes

asmorkalov added this to the 4.9.0 milestone Jul 25, 2023

asmorkalov assigned asmorkalov and unassigned dkurt Jul 25, 2023

asmorkalov removed the pr: needs test New functionality requires minimal tests set label Jul 25, 2023

Fix NaN handling in ap3p and added test

168f793

asmorkalov force-pushed the alexander-varjo-patch-1 branch from 5eb9556 to 168f793 Compare July 26, 2023 10:37

dkurt reviewed Jul 26, 2023

View reviewed changes

modules/calib3d/test/test_solvepnp_ransac.cpp Show resolved Hide resolved

dkurt requested changes Aug 30, 2023

View reviewed changes

Replace solveQuartic to solve_deg4

de8ab2e

dkurt requested changes Aug 30, 2023

View reviewed changes

Limit number of roots to process

eef584f

dkurt self-assigned this Aug 31, 2023

dkurt approved these changes Aug 31, 2023

View reviewed changes

dkurt requested review from asmorkalov and opencv-alalek August 31, 2023 09:45

asmorkalov approved these changes Sep 4, 2023

View reviewed changes

asmorkalov merged commit 114c23e into opencv:4.x Sep 4, 2023

asmorkalov mentioned this pull request Sep 11, 2023

(5.x) Merge 4.x #24254

Merged

Uh oh!

Fix crash in ap3p #23607

Fix crash in ap3p #23607

Uh oh!

Conversation

alexander-varjo commented May 11, 2023 • edited by dkurt Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Readiness Checklist

Uh oh!

dkurt May 12, 2023

Choose a reason for hiding this comment

Uh oh!

alexander-varjo Jun 21, 2023

Choose a reason for hiding this comment

Uh oh!

dkurt Jun 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexander-varjo Jul 5, 2023

Choose a reason for hiding this comment

Uh oh!

dkurt Jul 6, 2023

Choose a reason for hiding this comment

Uh oh!

dkurt Jul 21, 2023

Choose a reason for hiding this comment

Uh oh!

asmorkalov Jul 25, 2023

Choose a reason for hiding this comment

Uh oh!

asmorkalov commented Jun 5, 2023

Uh oh!

asmorkalov commented Jun 16, 2023

Uh oh!

asmorkalov commented Jun 23, 2023

Uh oh!

asmorkalov commented Jul 12, 2023

Uh oh!

asmorkalov commented Jul 25, 2023

Uh oh!

dkurt commented Jul 25, 2023

Uh oh!

Uh oh!

asmorkalov commented Jul 27, 2023

Uh oh!

dkurt commented Aug 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dkurt left a comment

Choose a reason for hiding this comment

Uh oh!

dkurt left a comment

Choose a reason for hiding this comment

Uh oh!

dkurt commented Aug 31, 2023

Uh oh!

asmorkalov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alexander-varjo commented May 11, 2023 •

edited by dkurt

Loading

dkurt Jun 29, 2023 •

edited

Loading

dkurt commented Aug 28, 2023 •

edited

Loading