Skip to content

Not-normalized output from findHomography#25308

Merged
asmorkalov merged 8 commits intoopencv:4.xfrom
dkurt:not_normalized_findHomography
Apr 29, 2024
Merged

Not-normalized output from findHomography#25308
asmorkalov merged 8 commits intoopencv:4.xfrom
dkurt:not_normalized_findHomography

Conversation

@dkurt
Copy link
Copy Markdown
Member

@dkurt dkurt commented Apr 2, 2024 •
edited
Loading

Pull Request Readiness Checklist

resolves #25133
resolves #4834
resolves #22166
resolves #18592

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 dkurt marked this pull request as ready for review April 2, 2024 17:16
@asmorkalov asmorkalov requested a review from vpisarev April 3, 2024 06:58
@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented Apr 3, 2024

@ivashmak Could you take a look too?

@asmorkalov asmorkalov added this to the 4.10.0 milestone Apr 3, 2024
@ivashmak
Copy link
Copy Markdown
Contributor

ivashmak commented Apr 5, 2024

@ivashmak Could you take a look too?

The idea to not normalize h33 to 1 is valid. In my USAC implementation, h33 is not constrained to 1. However, I am not sure about these changes:

opencv/modules/calib3d/src/fundam.cpp

Line 213 in e76b1ca

float ww = scaleFor(Hf[6]*M[i].x + Hf[7]*M[i].y + Hf[8]);

opencv/modules/calib3d/src/fundam.cpp

Line 254 in e76b1ca

double ww = scaleFor(h[6]*Mx + h[7]*My + h[8]);

Before, ww was either 1/scale or 0. But new scaleFor function returns ww either 1/scale or 1. If scale = 0 then points are at infinity, and perhaps ww should be 1 in this case, otherwise newly transformed points become 0.

@dkurt
Copy link
Copy Markdown
Member Author

dkurt commented Apr 5, 2024 •
edited
Loading

@ivashmak, thank you for the prompt review! As I understand, both ww from computeError perform H*src projection to estimate reprojection error. Both assume normalized H with h33=1. That's why they do + 1.0.

Moreover I don't understand how ww may be zero in the following equation end then used in residual calculation:

opencv/modules/calib3d/src/fundam.cpp

Lines 253 to 258 in e76b1ca

double Mx = M[i].x, My = M[i].y;
double ww = scaleFor(h[6]*Mx + h[7]*My + h[8]);
double xi = (h[0]*Mx + h[1]*My + h[2])*ww;
double yi = (h[3]*Mx + h[4]*My + h[5])*ww;
errptr[i*2] = xi - m[i].x;
errptr[i*2+1] = yi - m[i].y;

For complete picture, these are homography matrices for 0, RANSAC and LMEDS methods with only the following patch:

- _H0.convertTo(_model, _H0.type(), 1./_H0.at<double>(2,2) );
+ _H0.convertTo(_model, _H0.type(), scaleFor(_H0.at<double>(2,2)));

  1. method 0 (max reproj error 0.00473197):

    H: [368.0887100147034, 0.0159247465476191, 368.0446295465918;
 92.15359859037321, 92.1888090966786, -0.09174469371200097;
 -183.5667621743224, 0.00123721357596798, 0]
    
reprojection: [-0.000326887168340034, -1.004731974700604;
 -1.002815316369015, -1.004482078393191;
 -0.0001533803367575149, -0.0003079740891392978;
 -1.002628296372845, -5.808115023404001e-05;
 -0.0002401331678432291, -0.5025165895582193]

  2. method RANSAC (max reproj error 0.00473197):

    H: [368.0887100147034, 0.0159247465476191, 368.0446295465918;
 92.15359859037321, 92.1888090966786, -0.09174469371200097;
 -183.5667621743224, 0.00123721357596798, 0]

reprojection: [-0.000326887168340034, -1.004731974700604;
 -1.002815316369015, -1.004482078393191;
 -0.0001533803367575149, -0.0003079740891392978;
 -1.002628296372845, -5.808115023404001e-05;
 -0.0002401331678432291, -0.5025165895582193]

  3. method LMEDS (max reproj error 2.81711):

    H: [-0.3549734087389866, -3.406047867269289e-17, -0.3549734087389865;
 0.1612566478152516, 0.161256647815252, -5.975639861076076e-16;
 0.6774867043694947, 1.281788930749374e-15, 1]

reprojection: [3.442410094922453e-16, -1;
 -0.9999999999999852, 1.8171124241458;
 0, -9.061724223317423e-16;
 -0.9999999999999998, -3.678776244851225e-17;
 1.721205047461219e-16, -0.4999999999999987]

🤔 Or maybe this is something expected and we should not get exact matrix. I'll review it more carefully.

@ivashmak
Copy link
Copy Markdown
Contributor

ivashmak commented Apr 13, 2024

About ww I meant that before the update it was (line: https://github.com/opencv/opencv/blob/4.x/modules/calib3d/src/fundam.cpp#L247)

double ww = h[6]*Mx + h[7]*My + 1.;
ww = fabs(ww) > DBL_EPSILON ? 1./ww : 0;

So, ww is either 1/scale or 0. But then you changed it to

float ww = scaleFor(Hf[6]*M[i].x + Hf[7]*M[i].y + Hf[8]);

Where, ww is either 1/scale or 1, because of scaleFor function.

I saw the latest commit, and you removed the scaleFor function, and just put error as infinity if ww is close to 0. Basically, if ww is 0, then we need to compare vanishing points, the error does not have to be necessarily infinity. I think if ww is 0, perhaps we should compare just x and y coordinates?

@dkurt
Copy link
Copy Markdown
Member Author

dkurt commented Apr 18, 2024

@ivashmak, I see your concern. Reverted zero value condition back to the origin one.

@dkurt dkurt force-pushed the not_normalized_findHomography branch from 1940245 to aa41f91 Compare April 18, 2024 11:36
@dkurt dkurt requested a review from asmorkalov April 24, 2024 08:55
asmorkalov
asmorkalov approved these changes Apr 24, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@asmorkalov asmorkalov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 I do not see engineering issues, but need approve for Vadim or Maksim.

@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented Apr 25, 2024

@ivashmak @vpisarev could you take a look again?

@ivashmak
Copy link
Copy Markdown
Contributor

ivashmak commented Apr 27, 2024

@ivashmak @vpisarev could you take a look again?

I approve.

@asmorkalov asmorkalov assigned ivashmak and unassigned vpisarev Apr 29, 2024
@asmorkalov asmorkalov merged commit b1e0197 into opencv:4.x Apr 29, 2024
klatism pushed a commit to klatism/opencv that referenced this pull request May 17, 2024
@dkurt @klatism
Merge pull request opencv#25308 from dkurt:not_normalized_findHomography
1a1c5e7 
Not-normalized output from findHomography opencv#25308

### Pull Request Readiness Checklist

resolves opencv#25133
resolves opencv#4834
resolves opencv#22166
resolves opencv#18592

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

- [x] I agree to contribute to the project under Apache 2 License.
- [x] 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
- [x] 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
vrabaud added a commit to vrabaud/opencv that referenced this pull request May 29, 2024
@vrabaud
Fix Jacobian computation.
ef7fd0e 
The bug was introduced in opencv#25308
@vrabaud vrabaud mentioned this pull request May 29, 2024
Merged
6 tasks
@dkurt dkurt deleted the not_normalized_findHomography branch May 30, 2024 16:08
@dkurt dkurt restored the not_normalized_findHomography branch May 30, 2024 16:08
@dkurt
Copy link
Copy Markdown
Member Author

dkurt commented May 31, 2024

@asmorkalov, this PR has not been ported to 5.x: https://github.com/opencv/opencv/blob/5.x/modules/3d/src/fundam.cpp

@dkurt dkurt added the port to 5.x is needed Label for maintainers. Authors of PR can ignore this label May 31, 2024
@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented May 31, 2024

Thanks for the reminder. I have not merged 4.x->5.x for a month aprox. Will do right after release.

asmorkalov pushed a commit that referenced this pull request May 31, 2024
@vrabaud
Merge pull request #25665 from vrabaud:jacobian
1db6a8a 
Fix Homography computation. #25665

The bug was introduced in #25308

I am sorry I do not have a proper test.

### Pull Request Readiness Checklist

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

- [x] I agree to contribute to the project under Apache 2 License.
- [x] 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
- [x] 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
@mshabunin mshabunin mentioned this pull request Jun 12, 2024
Merged
@dkurt dkurt added port/backport done Label for maintainers. Authors of PR can ignore this and removed port to 5.x is needed Label for maintainers. Authors of PR can ignore this labels Dec 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@asmorkalov asmorkalov asmorkalov approved these changes

@vpisarev vpisarev Awaiting requested review from vpisarev

Assignees

@ivashmak ivashmak

Labels

category: calib3d port/backport done Label for maintainers. Authors of PR can ignore this

Projects

None yet

Milestone

4.10.0

4 participants

@dkurt @asmorkalov @ivashmak @vpisarev