fixed bad shape of markers (1x4) in several cases and added tests #3105

AleksandrPanov · 2021-11-11T16:39:30Z

Merge with extra: opencv/opencv_extra#935

detectMarkers() and refineDetectedMarkers() returns OutputArrayOfArrays corners and rejectedCorners with different dimensions (for similar cases).
This problem is only relevant for vector input.
In this case detectMarkers() always returns corners and rejectedCorners as vector<Mat>, with Mat dimension 4x1.
In this case refineDetectedMarkers() works differently, if it finds a new marker. Then the dimension of Mats inside the vector<Mat> changes from 4x1 to 1x4.
The reason for the problem is that detectMarkers() and refineDetectedMarkers() create corners and rejectedCorners with different ways.
Current tests show no errors because they use vector<vector<Point2f> > for corners and rejectedCorners. Also, the current tests only check the number of refined corners (dimension and value are not checked).

In this PR refineDetectedMarkers() for create OutputArrayOfArrays corners and rejectedCorners uses _copyVector2Output(), just like detectMarkers().

opencv_extra=fix_refineDetectedMarkers_shape

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

alalek · 2021-11-18T04:17:15Z

modules/aruco/src/aruco.cpp

@@ -1563,7 +1563,7 @@ void refineDetectedMarkers(InputArray _image, const Ptr<Board> &_board,

        _detectedCorners.create((int)finalAcceptedCorners.size(), 1, CV_32FC2);
        for(unsigned int i = 0; i < finalAcceptedCorners.size(); i++) {
-            _detectedCorners.create(4, 1, CV_32FC2, i, true);
+            _detectedCorners.create(1, 4, CV_32FC2, i, true);


Why?

Below we have 4,1 again: _rejectedCorners.create(4, 1, ...)

BTW, all usages from aruco module:

$ grep -Rn '.create(4, 1,' ./modules/aruco/ ./modules/aruco/src/aruco.cpp:584: out.create(4, 1, CV_32FC2, i); ./modules/aruco/src/aruco.cpp:591: out.create(4, 1, CV_32FC2, i); ./modules/aruco/src/aruco.cpp:598: out.create(4, 1, CV_32FC2, i); ./modules/aruco/src/aruco.cpp:696: _objPoints.create(4, 1, CV_32FC3); ./modules/aruco/src/aruco.cpp:1402: _detectedCorners.create(4, 1, CV_32FC2, i, true); ./modules/aruco/src/aruco.cpp:1420: _rejectedCorners.create(4, 1, CV_32FC2, i, true); ./modules/aruco/src/charuco.cpp:822: _diamondCorners.create(4, 1, CV_32FC2, i, true);

It is surprising that the dimension of the matrices out[i], _objPoints[i], _detectedCorners[i], _rejectedCorners[i], _diamondCorners[i] is: (1 (rows) , 4 (cols)). But create with (4 (rows), 1 (cols)) was used.

Flag allowTransposed is ignored in this case (same values for true/false).
Looks like a bug in void _OutputArray::create(int _rows, int _cols, int mtype, int i, bool allowTransposed, int fixedDepthMask) const.

Such changes affecting API are considered as a "breaking change".

So proper and complete change should have:

clear justification

consistent solution (fix 1 place only and left 6 others is not a good way)

adapt coding rules to avoid such/similar problems in the future.

surprising

There is allowTransposed parameter on this line.

This strange code change is removed. Now refineDetectedMarkers() for create OutputArrayOfArrays corners and rejectedCorners uses _copyVector2Output(), just like detectMarkers().

It's strange that before detectMarkers() and refineDetectedMarkers() created OutputArrayOfArrays corners and rejectedCorners in different ways.

alalek · 2021-11-18T04:20:30Z

modules/aruco/test/test_charucodetection.cpp

+
+    aruco::detectMarkers(img, dico, corners, ids, detectorParams, rejectedPoints);
+    ASSERT_FALSE(corners.empty());
+    EXPECT_EQ(Size(4, 1), corners[0].size());


EXPECT_EQ(Size(4, 1), corners[0].size());

Why do you think that Size(1, 4) is wrong?

Size(1, 4) is not wrong (because it's just four 2d-points) , but corners[i] must be the same size in all cases to avoid undefined behavior.
I also think that InputOutputArrayOfArrays should not be used to pass array of pointers. You suggested replacing the InputOutputArrayOfArrays with a InputOutputArray with ndarray shape [npoints, 4, 2]. I'll try to do that.

Let me clarify:
In all tests and all examples in OpenCV detectMarkers and refineDetectedMarkers return corners with Size(4, 1) (for corners[i]). It is strange if this condition is violated for no reason.

AleksandrPanov · 2022-02-09T12:41:54Z

@alalek, could you recheck PR? I changed the description of the PR. Code changes are made more clear. PR description has been improved.

AleksandrPanov · 2022-02-09T12:48:23Z

modules/aruco/src/aruco.cpp

-        _detectedCorners.clear();
-        _detectedIds.clear();
-
        // parse output
        Mat(finalAcceptedIds).copyTo(_detectedIds);
-
-        _detectedCorners.create((int)finalAcceptedCorners.size(), 1, CV_32FC2);
-        for(unsigned int i = 0; i < finalAcceptedCorners.size(); i++) {
-            _detectedCorners.create(4, 1, CV_32FC2, i, true);
-            for(int j = 0; j < 4; j++) {
-                _detectedCorners.getMat(i).ptr< Point2f >()[j] =
-                    finalAcceptedCorners[i].ptr< Point2f >()[j];
-            }
-        }
+        _copyVector2Output(finalAcceptedCorners, _detectedCorners);

        // recalculate _rejectedCorners based on alreadyIdentified
-        vector< Mat > finalRejected;
+        vector<vector<Point2f> > finalRejected;
        for(unsigned int i = 0; i < alreadyIdentified.size(); i++) {
            if(!alreadyIdentified[i]) {
                finalRejected.push_back(_rejectedCorners.getMat(i).clone());
            }
        }
-
-        _rejectedCorners.clear();
-        _rejectedCorners.create((int)finalRejected.size(), 1, CV_32FC2);
-        for(unsigned int i = 0; i < finalRejected.size(); i++) {
-            _rejectedCorners.create(4, 1, CV_32FC2, i, true);
-            for(int j = 0; j < 4; j++) {
-                _rejectedCorners.getMat(i).ptr< Point2f >()[j] =
-                    finalRejected[i].ptr< Point2f >()[j];
-            }
-        }
+        _copyVector2Output(finalRejected, _rejectedCorners);


Now, refineDetectedMarkers() uses _copyVector2Output() to copy, just as detectMarkers() does.

alalek

Well done 👍

AleksandrPanov mentioned this pull request Nov 11, 2021

fixed bad shape of markers (1x4) in several cases and added tests opencv/opencv_extra#935

Merged

AleksandrPanov added bug test labels Nov 11, 2021

AleksandrPanov self-assigned this Nov 11, 2021

AleksandrPanov force-pushed the fix_refineDetectedMarkers_shape branch 2 times, most recently from 6df7b11 to 9b8394a Compare November 12, 2021 09:18

AleksandrPanov requested a review from alalek November 16, 2021 18:59

alalek reviewed Nov 18, 2021

View reviewed changes

AleksandrPanov marked this pull request as draft November 29, 2021 16:57

AleksandrPanov mentioned this pull request Jan 31, 2022

refineDetectedMarkers return different arrays depending on if new markers were found #2720

Closed

4 tasks

AleksandrPanov linked an issue Jan 31, 2022 that may be closed by this pull request

refineDetectedMarkers return different arrays depending on if new markers were found #2720

Closed

4 tasks

AleksandrPanov force-pushed the fix_refineDetectedMarkers_shape branch 7 times, most recently from 8f02cad to 211d636 Compare February 1, 2022 15:56

fixed bad shape of markers (1x4) in several cases and added tests

d72a91f

AleksandrPanov force-pushed the fix_refineDetectedMarkers_shape branch from 211d636 to d72a91f Compare February 1, 2022 16:16

AleksandrPanov marked this pull request as ready for review February 7, 2022 11:36

AleksandrPanov commented Feb 9, 2022

View reviewed changes

AleksandrPanov removed their assignment Feb 9, 2022

alalek approved these changes Feb 9, 2022

View reviewed changes

opencv-pushbot merged commit ed0094c into opencv:3.4 Feb 9, 2022

AleksandrPanov mentioned this pull request Feb 9, 2022

cv::aruco::refineDetectedMarkers change detectedCorners[i] shape in some cases opencv/opencv#14014

Closed

alalek mentioned this pull request Feb 10, 2022

(4.x) Merge 3.4 #3173

Merged

alalek mentioned this pull request Feb 22, 2022

(5.x) Merge 4.x #3179

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fixed bad shape of markers (1x4) in several cases and added tests #3105

fixed bad shape of markers (1x4) in several cases and added tests #3105

Uh oh!

AleksandrPanov commented Nov 11, 2021 •

edited

Loading

Uh oh!

alalek Nov 18, 2021 •

edited

Loading

Uh oh!

AleksandrPanov Nov 19, 2021

Uh oh!

alalek Nov 20, 2021

Uh oh!

AleksandrPanov Feb 2, 2022 •

edited

Loading

Uh oh!

alalek Nov 18, 2021

Uh oh!

AleksandrPanov Nov 19, 2021 •

edited

Loading

Uh oh!

AleksandrPanov Nov 19, 2021 •

edited

Loading

Uh oh!

AleksandrPanov commented Feb 9, 2022 •

edited

Loading

Uh oh!

AleksandrPanov Feb 9, 2022

Uh oh!

alalek left a comment

Uh oh!

Uh oh!

fixed bad shape of markers (1x4) in several cases and added tests #3105

fixed bad shape of markers (1x4) in several cases and added tests #3105

Uh oh!

Conversation

AleksandrPanov commented Nov 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Readiness Checklist

Uh oh!

alalek Nov 18, 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!

AleksandrPanov Nov 19, 2021

Choose a reason for hiding this comment

Uh oh!

alalek Nov 20, 2021

Choose a reason for hiding this comment

Uh oh!

AleksandrPanov Feb 2, 2022 • 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!

alalek Nov 18, 2021

Choose a reason for hiding this comment

Uh oh!

AleksandrPanov Nov 19, 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!

AleksandrPanov Nov 19, 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!

AleksandrPanov commented Feb 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AleksandrPanov Feb 9, 2022

Choose a reason for hiding this comment

Uh oh!

alalek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AleksandrPanov commented Nov 11, 2021 •

edited

Loading

alalek Nov 18, 2021 •

edited

Loading

AleksandrPanov Feb 2, 2022 •

edited

Loading

AleksandrPanov Nov 19, 2021 •

edited

Loading

AleksandrPanov Nov 19, 2021 •

edited

Loading

AleksandrPanov commented Feb 9, 2022 •

edited

Loading