goodFeaturesToTrack returns also corner value by amirtu · Pull Request #19392 · opencv/opencv

amirtu · 2021-01-25T09:31:35Z

PR to extra with sanity data update: opencv/opencv_extra#841

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 other license that is incompatible with OpenCV
The PR is proposed to proper branch
There is reference to 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

(cherry picked from commit 4a8f067)

(cherry picked from commit b88fb40)

… compatibility (cherry picked from commit 6137383)

…rs values (cherry picked from commit f3d0ef2)

(cherry picked from commit be29755)

(cherry picked from commit ea3e118)

amirtu · 2021-01-25T09:36:02Z

This PR is created to support #17836 goodFeaturesToTrack returns also corner value

amirtu · 2021-02-01T05:39:22Z

modules/imgproc/include/opencv2/imgproc.hpp

                                     InputArray mask = noArray(), int blockSize = 3,
-                                     bool useHarrisDetector = false, double k = 0.04 );
+                                     bool useHarrisDetector = false, double k = 0.04,
+                                     OutputArray corners_values = noArray());


UB if we write at the noArray() -- static _InputOutputArray _none -- while someone is reading?

Added check if writing is actuallly requested (seems the object does not allow write access by default anyway).

modules/imgproc/include/opencv2/imgproc.hpp

modules/core/src/arithm.cpp

asmorkalov · 2021-02-03T09:24:57Z

modules/imgproc/perf/opencl/perf_gftt.cpp

+                                             minDistance, noArray(), 3, 3, harrisDetector, 0.04, values);

    SANITY_CHECK(dst);
+    SANITY_CHECK(values, 2.f * FLT_EPSILON, ERROR_RELATIVE);


I propose to use absolute error 1e-6. It's small enough relatively to values and should cover floating computation issue on different OpenCL devices.

Switched the tests back to comparison by absolute error.

modules/imgproc/test/ocl/test_gftt.cpp

asmorkalov · 2021-02-03T09:31:29Z

@vpisarev Please review the PR. @amirtu Cherry-picked original PR to presume original author and fixed found issues. We decided to create new PR because patch to extra repository is required. The PR introduces function overload instead of extra parameter with default value, because of API changes in Python. Python wrapper for function with 2 OutputArray parameter returns tuple of results and it breaks existing code. Overloads presume old behavior and introduces Python function with alternative name that returns tuple.

amirtu · 2021-02-03T09:34:22Z

To be merged with opencv/opencv_extra#841

modules/imgproc/test/test_goodfeaturetotrack.cpp

asmorkalov

Well done! 👍

asmorkalov · 2021-02-04T07:28:39Z

@vpisarev Could you take a look. Please ignore CN CI builds, they does not handle paired builds with extra right now.

alalek

Modification of both implementation and tests is not a good idea in general.

You should keep already existed test cases to ensure that there are no regressions in already existed functions.
For new functionality add new tests (or add new test parameters with keeping old checks for old test cases).

alalek · 2021-02-04T16:30:43Z

modules/imgproc/include/opencv2/imgproc.hpp

+CV_EXPORTS CV_WRAP_AS(goodFeaturesToTrackWithResponse) void goodFeaturesToTrack(
+        InputArray image, OutputArray corners,
+        int maxCorners, double qualityLevel, double minDistance,
+        InputArray mask, int blockSize,
+        bool useHarrisDetector, double k,
+        OutputArray corners_values);


Do not violate OpenCV arguments order: https://github.com/opencv/opencv/wiki/Coding_Style_Guide
or make it optional.

input parameters, output parameters, flags and optional parameters.

required input arrays

required output arrays

required input flags / scalar / non-array parameters

optional input arrays (mask)

optional output arrays

optional input flags / scalar / non-array parameters

alalek · 2021-02-04T16:31:43Z

modules/imgproc/include/opencv2/imgproc.hpp

+        int maxCorners, double qualityLevel, double minDistance,
+        InputArray mask, int blockSize,
+        bool useHarrisDetector, double k,
+        OutputArray corners_values);


corners_values

"values" are too abstract. All parameters are "values".
corners_response

alalek · 2021-02-04T16:35:45Z

modules/imgproc/perf/perf_goodFeaturesToTrack.cpp


    double minDistance = 1;
-    TEST_CYCLE() goodFeaturesToTrack(image, corners, maxCorners, qualityLevel, minDistance, noArray(), blockSize, gradientSize, useHarrisDetector);
+    TEST_CYCLE() goodFeaturesToTrack(image, corners, maxCorners, qualityLevel, minDistance, noArray(), blockSize, gradientSize, useHarrisDetector, 0.04, corners_values);


This change drops testing of old function.

You should create new one instead.

alalek · 2021-02-04T16:38:09Z

modules/imgproc/src/featureselect.cpp

            const Corner & c = corner_ptr[i];

            corners.push_back(Point2f((float)c.x, (float)c.y));
+            corners_values.push_back(eig.getMat(ACCESS_READ).at<float>(c.y, c.x));


.getMat(ACCESS_READ)

.getMat() / .getUMat() in a loop is not a good idea

Did you run perf tests locally?

alalek · 2021-02-04T16:41:45Z

modules/imgproc/test/test_goodfeaturetotrack.cpp

+        denom *= 2.;
    if(type != ftype )
-        denom *= 255.;
+        denom *= 255. * 255.;


@amirtu Please comment changes in reference code.

@alalek @asmorkalov Sorry for delayed answer, it has taken me some time to understand the context. Here are approximations of partial derivatives being computed with Sobel operator, the scaling is needed to preserve the energy after this filter: power of two due to averaging and differentiation parts of the filter, block size is considered in advance due to windowing of the image for every point, CV_32FC1 is (probably) normalized, CV_8U is not. First derivatives in both horizontal and vertical direction then should be scaled, as can be seen in cornerEigenValsVecs() (/imgproc/.../corner.cpp). Previously this test reference function was relying only on the relative order of the corner metrics, so scaling was not an issue.

modules/imgproc/include/opencv2/imgproc.hpp

asmorkalov · 2021-02-11T07:34:57Z

modules/imgproc/test/test_goodfeaturetotrack.cpp

+        denom *= 2.;
    if(type != ftype )
-        denom *= 255.;
+        denom *= 255. * 255.;


@amirtu Please comment changes in reference code.

amirtu · 2021-02-12T06:56:34Z

~~new PR for merge in 3.4: #19511~~

asmorkalov

Looks good for me. 👍 Please squash your own commits and presume commits of original author.

mineigen calculation rolled back; gftt function overload added (with quality parameter); perf tests were added for the new api function; external bindings were added for the function (with different alias); fixed issues with composition of the output array of the new function (e.g. as requested in comments) ; added sanity checks in the perf tests; removed C API changes.

alalek

Thank you! Looks good to me 👍

alalek · 2021-02-12T23:15:41Z

modules/features2d/src/gftt.cpp

-            *keypoint_it = KeyPoint( *corner_it, (float)blockSize );
+        std::vector<float>::iterator quality_it = cornersQuality.begin();
+        for( ; corner_it != corners.end() && keypoint_it != keypoints.end() && quality_it != cornersQuality.end(); ++corner_it, ++keypoint_it, ++quality_it )
+            *keypoint_it = KeyPoint( *corner_it, (float)blockSize, -1, *quality_it );


This part of code looks strange (it was strange before this patch too).

What should we do with "left" uninitialized items when keypoint_it != keypoints.end() && quality_it != cornersQuality.end() is triggered instead of corner_it != corners.end()?

Perhaps it makes sense to put after this loop consistency check:

CV_Assert(corner_it == corners.end());

Noted with thanks 👌

alalek · 2021-02-12T23:25:13Z

modules/imgproc/test/test_goodfeaturetotrack.cpp

+        ts->printf(cvtest::TS::CONSOLE, "actual error: %g, expected: %g", e, eps);
+        ts->set_failed_test_info(cvtest::TS::FAIL_BAD_ACCURACY);


Prefer to use GTest's EXPECT_LE(e, eps); instead of legacy "ts->printf()".
It would should failed line at least.

Fixed, but unfortunately the test itself does not extend GTest so semantics of EXPECT_LE looks oddly.

alalek · 2021-02-15T15:28:06Z

modules/features2d/src/gftt.cpp

-            *keypoint_it = KeyPoint( *corner_it, (float)blockSize );
+        CV_Assert(corners.size() == cornersQuality.size());
+
+        keypoints.clear();


.reserve() is necessary to avoid multiple re-allocations.

…uresToTrack_returns_also_corner_value_PR * goodFeaturesToTrack returns also corner value (cherry picked from commit 4a8f067) * Added response to GFTT Detector keypoints (cherry picked from commit b88fb40) * Moved corner values to another optional variable to preserve backward compatibility (cherry picked from commit 6137383) * Removed corners valus from perf tests and better unit tests for corners values (cherry picked from commit f3d0ef2) * Fixed detector gftt call (cherry picked from commit be29755) * Restored test_cornerEigenValsVecs (cherry picked from commit ea3e118) * scaling fixed; mineigen calculation rolled back; gftt function overload added (with quality parameter); perf tests were added for the new api function; external bindings were added for the function (with different alias); fixed issues with composition of the output array of the new function (e.g. as requested in comments) ; added sanity checks in the perf tests; removed C API changes. * minor change to GFTTDetector::detect * substitute ts->printf with EXPECT_LE * avoid re-allocations Co-authored-by: Anas <anas.el.amraoui@live.com> Co-authored-by: amir.tulegenov <amir.tulegenov@xperience.ai>

WisdomPill added 6 commits January 22, 2021 14:36

goodFeaturesToTrack returns also corner value

b5f8f5c

(cherry picked from commit 4a8f067)

Added response to GFTT Detector keypoints

077a0a1

(cherry picked from commit b88fb40)

Moved corner values to another optional variable to preserve backward…

5a7010f

… compatibility (cherry picked from commit 6137383)

Removed corners valus from perf tests and better unit tests for corne…

59dceb3

…rs values (cherry picked from commit f3d0ef2)

Fixed detector gftt call

c8e63fe

(cherry picked from commit be29755)

Restored test_cornerEigenValsVecs

30507aa

(cherry picked from commit ea3e118)

amirtu commented Feb 1, 2021

View reviewed changes

asmorkalov changed the title ~~Temporary PR: finalize PR 17836 goodFeaturesToTrack w/ corner values~~ goodFeaturesToTrack returns also corner value Feb 3, 2021

asmorkalov mentioned this pull request Feb 3, 2021

goodFeaturesToTrack returns also corner value #17836

Closed

6 tasks

asmorkalov reviewed Feb 3, 2021

View reviewed changes

modules/imgproc/include/opencv2/imgproc.hpp Outdated Show resolved Hide resolved

asmorkalov added category: features2d category: imgproc feature labels Feb 3, 2021

asmorkalov requested changes Feb 3, 2021

View reviewed changes

asmorkalov requested a review from vpisarev February 3, 2021 09:27

asmorkalov reviewed Feb 3, 2021

View reviewed changes

modules/imgproc/test/test_goodfeaturetotrack.cpp Outdated Show resolved Hide resolved

asmorkalov approved these changes Feb 4, 2021

View reviewed changes

alalek reviewed Feb 4, 2021

View reviewed changes

asmorkalov requested changes Feb 11, 2021

View reviewed changes

asmorkalov self-requested a review February 12, 2021 07:10

asmorkalov approved these changes Feb 12, 2021

View reviewed changes

amirtu force-pushed the OCV-165_finalize_goodFeaturesToTrack_returns_also_corner_value_PR branch from 979e7f1 to 5e6d441 Compare February 12, 2021 07:20

alalek approved these changes Feb 12, 2021

View reviewed changes

alalek assigned asmorkalov Feb 12, 2021

amir.tulegenov added 2 commits February 15, 2021 18:54

minor change to GFTTDetector::detect

e4552cd

substitute ts->printf with EXPECT_LE

bb1e01b

alalek reviewed Feb 15, 2021

View reviewed changes

avoid re-allocations

c22721c

alalek merged commit 47426a8 into opencv:master Feb 15, 2021

WisdomPill mentioned this pull request Feb 18, 2021

GFFTDetector and goodFeaturesToTrack do not return intensity of the corner #16462

Closed

alalek mentioned this pull request Apr 9, 2021

(5.x) Merge 4.x #19885

Merged

		ts->printf(cvtest::TS::CONSOLE, "actual error: %g, expected: %g", e, eps);
		ts->set_failed_test_info(cvtest::TS::FAIL_BAD_ACCURACY);

Uh oh!

Conversation

amirtu commented Jan 25, 2021 • edited by asmorkalov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Readiness Checklist

Uh oh!

amirtu commented Jan 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amirtu Feb 1, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

asmorkalov commented Feb 3, 2021

Uh oh!

amirtu commented Feb 3, 2021

Uh oh!

Uh oh!

asmorkalov left a comment

Choose a reason for hiding this comment

Uh oh!

asmorkalov commented Feb 4, 2021

Uh oh!

alalek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amirtu Feb 11, 2021 • 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amirtu commented Feb 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asmorkalov left a comment

Choose a reason for hiding this comment

Uh oh!

alalek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amirtu Feb 15, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

amirtu commented Jan 25, 2021 •

edited by asmorkalov

Loading

amirtu commented Jan 25, 2021 •

edited

Loading

amirtu Feb 1, 2021 •

edited

Loading

amirtu Feb 11, 2021 •

edited

Loading

amirtu commented Feb 12, 2021 •

edited

Loading

amirtu Feb 15, 2021 •

edited

Loading