Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

different interpolation by double image #23124

Merged
merged 7 commits into from
Feb 17, 2023

Conversation

vicsyl
Copy link
Contributor

@vicsyl vicsyl commented Jan 10, 2023

Fixing #23123
See also: https://github.com/vicsyl/dog_precision

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

Even though by detector scale invariance test for SIFT the constrains had to be relaxed for the test to pass, the change improved other test metrics in SIFT detector/descriptor/scale/rotation invariance tests. Even metrics for detector scale invariance test for SIFT are better after the change for different parameters. See my comment at test_detectors_invariance.cpp for more details.

Note that the scaling of keypoint locations was changed so that is corresponds to homography:

$$H_{gt} = \begin{bmatrix} s & 0 & (s-1) / 2 \\ 0 & s & (s-1) / 2 \\ 0 & 0 & 1\\ \end{bmatrix}$$

See my comment in test_invariance_utils.hpp.

The performance tests didn't show performance regression:

feature2d_detect.detect

WITH FIX:

 leuven/img1.png  samples=25   mean= 705.39   median= 704.76   min= 685.41   stddev=14.78 (2.1%)
stitching/a3.png  samples=10   mean= 631.09   median= 626.15   min= 615.54   stddev=16.39 (2.6%)
stitching/s2.jpg  samples=10   mean=1339.58   median=1339.54   min=1319.43   stddev=19.54 (1.5%)

WITHOUT FIX:

 leuven/img1.png  samples=10   mean= 707.30   median= 699.30   min= 693.54   stddev=20.09 (2.8%)
stitching/a3.png  samples=10   mean= 621.38   median= 616.74   min= 614.41   stddev= 8.80 (1.4%)
stitching/s2.jpg  samples=10   mean=1347.98   median=1351.27   min=1312.30   stddev=23.25 (1.7%)

feature2d_detectAndExtract.detectAndExtract

WITH FIX:

 leuven/img1.png   samples=10   mean=831.68  median= 834.14   min= 815.27   stddev=10.76 (1.3%)
stitching/a3.png   samples=10   mean=865.91  median= 858.49   min= 843.43   stddev=23.24 (2.7%)
stitching/s2.jpg   samples=10  mean=1913.42  median=1908.38   min=1861.93   stddev=46.72 (2.4%)

WITHOUT FIX:

 leuven/img1.png  samples=10   mean= 819.67   median= 816.28   min= 810.95   stddev=10.62 (1.3%)
stitching/a3.png  samples=10   mean= 841.28   median= 838.84   min= 826.58   stddev=13.54 (1.6%)
stitching/s2.jpg  samples=10   mean=1958.65   median=1952.93   min=1937.43   stddev=18.01 (0.9%)

@vpisarev
Copy link
Contributor

vpisarev commented Jan 23, 2023

@vicsyl, thank you for the contribution! The change seems reasonable. I wonder, whether this change in $H_{gt}$ will affect implementation of A-SIFT and/or any other non-trivial algorithms (SIFT=>findHomography/findFundamentalMat/...) that are based on SIFT?

Mat dbl;
#if DoG_TYPE_SHORT
resize(gray_fpt, dbl, Size(gray_fpt.cols*2, gray_fpt.rows*2), 0, 0, INTER_LINEAR_EXACT);
Copy link
Member

Choose a reason for hiding this comment

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

INTER_LINEAR_EXACT

Main problem here is that we already have declared the "bit-exact" implementation for SIFT.
"Silent" replacing of such implementation with new one is a regression in term of API interface stability.

I believe we should have an option for enabling of fixed version and a warning about the deprecated behavior.

/cc @vpisarev

@vicsyl
Copy link
Contributor Author

vicsyl commented Jan 25, 2023

@vicsyl, thank you for the contribution! The change seems reasonable. I wonder, whether this change in Hgt will affect implementation of A-SIFT and/or any other non-trivial algorithms (SIFT=>findHomography/findFundamentalMat/...) that are based on SIFT?

Somehow I cannot reply to this directly. $H_{gt}$ just reflects how the image is transformed by current resize in the test code:
resize(image0, image1, Size(), 1./scale, 1./scale, INTER_LINEAR_EXACT);

"Natural scaling"

$$H_{s} = \begin{bmatrix} s & 0 & 0 \\ 0 & s & 0 \\ 0 & 0 & 1\\ \end{bmatrix}$$

can be achieved via warpAffine - now present in SIFT but it is also used in ASIFT, so I don't see a problem there. findHomography performs better now as can be seen here:

https://github.com/vicsyl/dog_precision/blob/master/OpenCV%20homography%20estimation.ipynb

@vpisarev

@vpisarev
Copy link
Contributor

ok, since we now have a flag that switches between the modes and so the original behaviour can still be achieved, I suggest to merge it

modules/features2d/src/sift.dispatch.cpp Show resolved Hide resolved
@@ -37,7 +37,7 @@ INSTANTIATE_TEST_CASE_P(AKAZE_DESCRIPTOR_KAZE, DetectorRotationInvariance,
*/

INSTANTIATE_TEST_CASE_P(SIFT, DetectorScaleInvariance,
Value(IMAGE_BIKES, SIFT::create(0, 3, 0.09), 0.65f, 0.98f));
Value(IMAGE_BIKES, SIFT::create(0, 3, 0.09), 0.60f, 0.98f));
Copy link
Member

Choose a reason for hiding this comment

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

Why do we have this regression in accuracy?

@vpisarev
Copy link
Contributor

@vicsyl, the PR has been discussed at today's core meeting. It is almost ready for the merge. 2 things need to be done:

  1. please, add doxygen documentation for the added parameter enable_precise_upscale.
  2. please, reply to @alalek comment regarding the accuracy drop in one of the scale invariance tests: 0.65 => 0.6. If the behavior is expected, just confirm it

@vicsyl
Copy link
Contributor Author

vicsyl commented Feb 13, 2023

@vicsyl, the PR has been discussed at today's core meeting. It is almost ready for the merge. 2 things need to be done:

  1. please, add doxygen documentation for the added parameter enable_precise_upscale.
  2. please, reply to @alalek comment regarding the accuracy drop in one of the scale invariance tests: 0.65 => 0.6. If the behavior is expected, just confirm it

done both
@vpisarev

@vpisarev vpisarev merged commit 923dbcc into opencv:4.x Feb 17, 2023
a-sajjad72 pushed a commit to a-sajjad72/opencv that referenced this pull request Mar 30, 2023
* different interpolation by double image

* fixing scaling mapping

* fixing a test

* added an option to enable previous interpolation

* added doxygen entries for the new parameter

* ASSERT_TRUE -> ASSERT_EQ

* changed log message when using old upscale mode
@asmorkalov asmorkalov mentioned this pull request May 31, 2023
geversonsto pushed a commit to stodev-com-br/opencv that referenced this pull request Jun 3, 2023
* different interpolation by double image

* fixing scaling mapping

* fixing a test

* added an option to enable previous interpolation

* added doxygen entries for the new parameter

* ASSERT_TRUE -> ASSERT_EQ

* changed log message when using old upscale mode
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants