Extend ArUcoDetector to run multiple dictionaries in an efficient manner. by BenjaminKnecht · Pull Request #26934 · opencv/opencv

BenjaminKnecht · 2025-02-18T10:20:36Z

Add constructor for multiple dictionaries
Add get/set/remove/add functions for multiple dictionaries
Add unit tests

TESTED=unit tests

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

manner. * Add constructor for multiple dictionaries * Add get/set/remove/add functions for multiple dictionaries * Add unit tests TESTED=unit tests

asmorkalov

I reviewed API and tests. Works on details. Please add also Python a test to https://github.com/opencv/opencv/blob/4.x/modules/objdetect/misc/python/test/test_objdetect_aruco.py

modules/objdetect/test/test_arucodetection.cpp

modules/objdetect/include/opencv2/objdetect/aruco_detector.hpp

modules/objdetect/test/test_arucodetection.cpp

vrabaud · 2025-02-18T12:33:15Z

For Python compatibility, please add typedef std::vector<aruco::Dictionary> vector_Dictionary; after

opencv/modules/python/src2/cv2.cpp

Line 47 in 6092499

typedef std::vector<Scalar> vector_Scalar;

modules/objdetect/misc/python/test/test_objdetect_aruco.py

modules/objdetect/src/aruco/aruco_detector.cpp

asmorkalov · 2025-02-19T11:35:03Z

modules/objdetect/src/aruco/aruco_detector.cpp

                                          InputOutputArrayOfArrays _rejectedCorners, InputArray _cameraMatrix,
                                          InputArray _distCoeffs, OutputArray _recoveredIdxs) const {
    DetectorParameters& detectorParams = arucoDetectorImpl->detectorParams;
-    const Dictionary& dictionary = arucoDetectorImpl->dictionary;


I propose to add refineDetectedMarkersMultiDict overload with dictionaries id like in detect. May be useful for multi-scale markers.

Made sure refine is now looping over the markers and checked the output. Looks like this can be done safely without having to have two functions.

using multiple dictionaries for refinement (function split not necessary as it's backwards compatible)

asmorkalov · 2025-02-20T05:52:26Z

Warning in all Windows builds:

C:\GHA-OCV-3\_work\opencv\opencv\opencv\modules\objdetect\src\aruco\aruco_detector.cpp(840): warning C4305: 'initializing': truncation from 'double' to 'float'

asmorkalov · 2025-02-20T05:53:03Z

@dkurt @AleksandrPanov @mshabunin What do you think?

BenjaminKnecht · 2025-02-20T12:53:23Z

Warning in all Windows builds:

C:\GHA-OCV-3\_work\opencv\opencv\opencv\modules\objdetect\src\aruco\aruco_detector.cpp(840): warning C4305: 'initializing': truncation from 'double' to 'float'

Fixing it now.

I also will add the MultiDict version for the refineDetectedMarkers function. It might make sense after all if there are multiple boards in the image.

BenjaminKnecht · 2025-02-20T14:42:10Z

So this hopefully fixes the warning on Windows (sorry, no access to Windows to test this on).

Concerning the refineDetectedMarkers function I'm not sure a multi-dict version makes sense as it improves the markers on a board. These boards usually (and also in the code) only have a single dictionary. So instead I think it's better to use just one dictionary (the one the board is using) to refine these markers. I added this insight as a @note to function so people don't stumble over this.

Let me know what you think. It's a bit unfortunate the extension wasn't possible without the additional optional function parameter in detectMarkers only. But the multiDict version of the function seems to work fine for what I'm trying to do detecting multiple dictionaries in the same image.

mshabunin · 2025-02-23T20:47:39Z

modules/objdetect/include/opencv2/objdetect/aruco_detector.hpp

+    CV_WRAP const std::vector<Dictionary>& getDictionaries() const;
+    CV_WRAP void setDictionaries(const std::vector<Dictionary>& dictionaries);
+    CV_WRAP void addDictionary(const Dictionary& dictionary);
+    CV_WRAP void removeDictionary(int index);


In my opinion, methods get(set)Dictionary, add(remove)Dictionary are redundant. It is enough to have get(set)Dictionaries. Perhaps it would be better to preserve old get(set)Dictionary, so that they would return the first one.

Is it safe to return reference to internal vector in method getDictionaries? Maybe it would be better to return a copy.

What about corner cases: empty vector, duplicates?

Methods should be documented.

Hey, thanks for the feedback.

yeah, add/remove may be a bit redundant. I went for maximal convenience here so the dictionaries list can be manipulated in many ways. I want to point out though that the default behavior of get/setDictionary (so without the optional index) is preserved.

As long as it's const it's safe. You can always get a copy, rather than a view of the insides by doing vector<Dictionary> copy = detector.getDictionaries() rather than vector<Dictionary>& reference = detector.getDictionary(). So in order to not do a copy you have to specify that you're taking a reference. If you're asking for safety in terms of a malevolent attacker rather than a potential source of a bug, then yes, returning a copy would be better.

Empty vector isn't allowed. I have a check in the constructor and the removeDictionary() function for that, but I forgot to check in the setDictionaries(). Thanks for spotting this! On duplicates I purposefully did not add a check and leave some of the responsibilities to the user. I could change this to use an unsorted_set internally that automatically ignores duplicates, but the interface would remain a vector. This would also mean that getDictionaries() would return a copy for sure. Even then you could do nonsensical stuff like running DICT_4X4_50 and then DICT_4X4_100 and potentially find a lot of duplicated markers. This can of course be solved by just running the larger dictionary and ignoring the smaller one, but that's a lot of hand-holding that I am not used to with the OpenCV API.

Sure, makes sense. I added some non-trivial complexity after all.

So for now I will fix the bug that allows for setting an empty vector and I'll add documentation. On the rest I'd like some confirmation for my suggestions and then I'm happy to make the changes.

Old get/setDictionary API would work, yes. But ABI compatibility would be broken. Can we avoid it?

No I meant safety in terms of exposing internal structure. For example it allows user to create iterator which might become invalid suddenly. Also we won't be able to change internal data structure.

Each access function must be tested for possible errors then.

Alright, I've updated the PR to keep the behavior of the set/get functions and the ABI the same.

Yeah, good point with the iterators. I make it return a copy now.

I think I had tests already, but since you asked about it earlier, the most current version now has no add/remove functions and thus it's a lot easier to use and also easier to test.

I hope this addresses all the concerns. I left a bit of documentation for the set/getDictionary functions so it's clear what they do with the dictionaries list.

mshabunin · 2025-02-23T20:53:29Z

modules/objdetect/include/opencv2/objdetect/aruco_detector.hpp

+     * input image with corresponding camera model, if camera parameters are known
+     * @sa undistort, estimatePoseSingleMarkers,  estimatePoseBoard
+     */
+    CV_WRAP void detectMarkersMultiDict(InputArray image, OutputArrayOfArrays corners, OutputArray ids,


Wouldn't it be more convenient to modify Dictionary class, so that it would be able to combine several dictionaries underneath. It could also handle duplicate markers and provide convenient indexing. Detector interface would stay the same and users would require minimal changes in their code.

Not a bad idea, but the Dictionary class is also used for these calibration boards and the Charuco stuff as well. Its public members (errorCorrection, markerSize, bytesList) would all need to change because they would be ambiguous. Just from a quick check I think such a change would be more disruptive than the interface change suggested in this PR.

I think such a change would be more disruptive than the interface change suggested in this PR.

Agree and support

We already have function extendDictionary which adds randomized markers to the existing dictionary, so I believe creating similar function to combine several dictionaries with the same marker size would be quite easy. Or do we need to have dictionaries with different marker sizes?

At least for the use case we have in mind you would want to scan dictionaries of different marker sizes, yes.

mshabunin · 2025-02-24T14:19:16Z

modules/objdetect/src/aruco/aruco_detector.cpp

-void ArucoDetector::write(FileStorage &fs) const
-{
-    arucoDetectorImpl->dictionary.writeDictionary(fs);
+void ArucoDetector::write(FileStorage &fs) const {


I believe (de)serialization is not tested. Is old format supported? I.e. if we have just one dictionary, would older versions of OpenCV be able to read it?

True, I couldn't find a unit test for it, but I tested it locally. I can clean the code up and add it as a test.

I made sure it could read the old format, but it wouldn't produce the old format if there's only one dictionary. I'll add that, so both read and write support the old format if there is only on dict.

functions

asmorkalov · 2025-02-27T06:27:58Z

Debug test failed:

[ RUN      ] CV_ArucoMultiDict.serialization
unknown file: Failure
C++ exception with description "OpenCV(4.12.0-dev) /home/ci/opencv/modules/core/include/opencv2/core/mat.inl.hpp:1014: error: (-215:Assertion failed) elemSize() == sizeof(_Tp) in function 'begin'
" thrown in the test body.
[  FAILED  ] CV_ArucoMultiDict.serialization (2 ms)

BenjaminKnecht · 2025-02-27T09:54:55Z

Debug test failed:

[ RUN      ] CV_ArucoMultiDict.serialization
unknown file: Failure
C++ exception with description "OpenCV(4.12.0-dev) /home/ci/opencv/modules/core/include/opencv2/core/mat.inl.hpp:1014: error: (-215:Assertion failed) elemSize() == sizeof(_Tp) in function 'begin'
" thrown in the test body.
[  FAILED  ] CV_ArucoMultiDict.serialization (2 ms)

Took me a bit to understand the problem with the Mat type here and why it only failed in Debug mode. Should be fine now and I hope the serialization test is valuable.

asmorkalov · 2025-02-28T13:13:05Z

@mshabunin Could you take a look again. I believe the patch is ready for integration.

mshabunin · 2025-03-03T08:21:22Z

modules/objdetect/src/aruco/aruco_detector.cpp

+                });
+            }
+        } else if (DictionaryMode::Multi == dictMode) {
+            unordered_set<int> uniqueMarkerSizes;


Why not regular std::set?

made it a map now as marker size needs to map to candidate tree. I made this connection now more deliberately and direct so I also don't need to calculate indices for other containers which may cause bounds issues as you correctly pointed out.

mshabunin · 2025-03-03T08:32:31Z

modules/objdetect/test/test_arucodetection.cpp

+
+TEST(CV_ArucoMultiDict, serialization)
+{
+    const std::string fileName("test_aruco_serialization.json");


Please use cv::tempfile for file name and make sure file is removed using remove at the end (no ASSERT_ can be used).

Alternatively you can try to use in-memory storage:

opencv/modules/features2d/test/test_matchers_algorithmic.cpp

Lines 591 to 595 in dbd3ef9

FileStorage fs_in(ymlfile, FileStorage::READ + FileStorage::MEMORY);

matcher->read(fs_in.root());

FileStorage fs_out(".yml", FileStorage::WRITE + FileStorage::MEMORY);

matcher->write(fs_out);

std::string out = fs_out.releaseAndGetString();

Thanks, that's what I was looking for.

mshabunin · 2025-03-03T08:33:14Z

modules/objdetect/test/test_arucodetection.cpp

+    {
+        FileStorage fs(fileName, FileStorage::Mode::WRITE);
+        if (!fs.isOpened()) {
+            cout << "[  SKIPPED ] " << "CV_ArucoMultiDict.serialization"


Failure to open file should become test failure. Here and in other places.

mshabunin · 2025-03-03T08:45:34Z

modules/objdetect/include/opencv2/objdetect/aruco_detector.hpp

+     * @param dictIndices vector of dictionary indices for each detected marker. Use getDictionaries() to get the
+     * list of corresponding dictionaries.
+     *
+     * Performs marker detection in the input image. Only markers included in the specific dictionaries


Perhaps it would be useful to define and test behavior in case when several dictionaries contain same markers.

Good idea. Added a test that shows off how you can detect three markers for 2 similar dictionaries.

mshabunin · 2025-03-03T08:59:32Z

modules/objdetect/src/aruco/aruco_detector.cpp

+                // copy candidates
+                vector<vector<Point2f>> candidatesCopy = candidates;
+                vector<vector<Point> > contoursCopy = contours;
+                candidatesPerDictionarySize[dictionarySizeIndex] = filterTooCloseCandidates(candidatesCopy, contoursCopy, markerSize);


If I understand correctly, this function filters candidates using marker size as a hint to estimate distances or something. Could dictionaries with different marker sizes affect detections of each other?

That was also my worry and when reading into the code of the function it may affect candidates in some corner cases. This is why I'm making copies of the vectors before filtering, so the order of identifying with multiple dictionaries doesn't matter. The copies are an overhead though that I'd like to avoid.

mshabunin · 2025-03-03T09:01:47Z

modules/objdetect/src/aruco/aruco_detector.cpp

+            }
+
+            // create at max 4 marker candidate trees for each dictionary size
+            vector<vector<MarkerCandidateTree>> candidatesPerDictionarySize = {{}, {}, {}, {}};


Can dictionary have marker size other than 4, 5, 6, 7? I don't see any checks in Dictionary constructor or other places. If it can, then we have possible out-of-bounds access.

Yeah, I was thinking about the predefined dictionaries only, but custom dictionaries could potentially break this. I made it more flexible that now any size of marker is supported.

modules/objdetect/src/aruco/aruco_detector.cpp

Use map to manage unique marker size candidate trees. Avoid code duplication. Add a test to show double detection with overlapping dictionaries. Generalize to marker sizes of not only predefined dictionaries.

mshabunin · 2025-03-04T15:14:25Z

Looks good to me. Please fix Windows warning:

C:\build\precommit_windows64\4.x\opencv\modules\objdetect\src\aruco\aruco_detector.cpp(768): warning C4267: 'argument': conversion from 'size_t' to 'int', possible loss of data [C:\build\precommit_windows64\build\modules\objdetect\opencv_objdetect.vcxproj]

BenjaminKnecht · 2025-03-11T09:56:08Z

Thank you to everyone for the reviews and the productive discussions. This is my first attempted contribution to OpenCV and I did enjoy the process so far.

@asmorkalov what's the next step? Can we merge the PR as it is now?

JonasPerolini · 2025-03-18T14:12:13Z

Hi @BenjaminKnecht,

While working on #23190 I've tested the Multi-dictionary implementation and noticed that if we use two dictionaries that have markers in common e.g. DICT_4X4_50_DATA and DICT_4X4_100_DATA, those 50 markers will be detected twice.

More generally, every candidate is tested against every dictionary with the same markerSize, even if some candidates were identified.

Is this intended?

If not, when looping over the dictionaries for (const Dictionary& currentDictionary : dictionaries) we can remove the identified candidates from the map candidatesPerDictionarySize.at(currentDictionary.markerSize).

Extend ArUcoDetector to run multiple dictionaries in an efficient

c759a7c

manner. * Add constructor for multiple dictionaries * Add get/set/remove/add functions for multiple dictionaries * Add unit tests TESTED=unit tests

BenjaminKnecht mentioned this pull request Feb 18, 2025

Extend ArucoDetector to identify multiple dictionaries simultaneously #26912

Closed

asmorkalov added feature category: objdetect labels Feb 18, 2025

asmorkalov self-requested a review February 18, 2025 10:23

asmorkalov self-assigned this Feb 18, 2025

asmorkalov added this to the 4.12.0 milestone Feb 18, 2025

asmorkalov reviewed Feb 18, 2025

View reviewed changes

Benjamin Knecht added 3 commits February 18, 2025 14:08

Fix python bindings

379b5a2

Address comments, add Python test

bb07ce7

Fix index comparison warnings

9ae23a7

asmorkalov reviewed Feb 19, 2025

View reviewed changes

have two detectMarkers functions for python backwards compatibility

f212c16

using multiple dictionaries for refinement (function split not necessary as it's backwards compatible)

asmorkalov requested review from opencv-alalek and removed request for opencv-alalek February 20, 2025 05:50

Benjamin Knecht added 2 commits February 20, 2025 15:12

Fixed warning on Windows, clarified refineDetectedMarkers method

1f9d6aa

Undo multi dict functionality of refineDetectedMarkers method

364eedb

mshabunin reviewed Feb 23, 2025

View reviewed changes

asmorkalov mentioned this pull request Feb 24, 2025

Fix Aruco marker incorrect detection near image edge #26968

Merged

6 tasks

Add docs to Dictionary get/set/add/remove functions

3c88a00

mshabunin reviewed Feb 24, 2025

View reviewed changes

Benjamin Knecht added 3 commits February 24, 2025 17:33

Make sure serialization with single dict preserves old behavior

6c3b195

Remove add/removeDictionary and retain ABI of set/getDictionary

314f99f

functions

Fixing warnings in tests

d869b12

asmorkalov approved these changes Feb 26, 2025

View reviewed changes

Fix dictionary comparison in test

3084f95

mshabunin reviewed Mar 3, 2025

View reviewed changes

Address more comments

1aa658f

Use map to manage unique marker size candidate trees. Avoid code duplication. Add a test to show double detection with overlapping dictionaries. Generalize to marker sizes of not only predefined dictionaries.

mshabunin approved these changes Mar 4, 2025

View reviewed changes

Attempt to fix Windows int type warning

d80fd56

asmorkalov merged commit d9956fc into opencv:4.x Mar 11, 2025
28 checks passed

asmorkalov mentioned this pull request Mar 11, 2025

5.x merge 4.x #27045

Merged

BenjaminKnecht deleted the new_4.x branch March 12, 2025 09:16

	FileStorage fs_in(ymlfile, FileStorage::READ + FileStorage::MEMORY);
	matcher->read(fs_in.root());
	FileStorage fs_out(".yml", FileStorage::WRITE + FileStorage::MEMORY);
	matcher->write(fs_out);
	std::string out = fs_out.releaseAndGetString();

Uh oh!

Conversation

BenjaminKnecht commented Feb 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Readiness Checklist

Uh oh!

asmorkalov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vrabaud commented Feb 18, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asmorkalov commented Feb 20, 2025

Uh oh!

asmorkalov commented Feb 20, 2025

Uh oh!

BenjaminKnecht commented Feb 20, 2025

Uh oh!

BenjaminKnecht commented Feb 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BenjaminKnecht Feb 24, 2025 • 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!

mshabunin Feb 24, 2025 • 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!

asmorkalov Feb 24, 2025 • 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!

mshabunin Feb 24, 2025 • 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!

asmorkalov commented Feb 27, 2025

Uh oh!

BenjaminKnecht commented Feb 27, 2025

Uh oh!

asmorkalov commented Feb 28, 2025

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!

Choose a reason for hiding this comment

BenjaminKnecht commented Feb 18, 2025 •

edited

Loading

BenjaminKnecht Feb 24, 2025 •

edited

Loading

mshabunin Feb 24, 2025 •

edited

Loading

asmorkalov Feb 24, 2025 •

edited

Loading

mshabunin Feb 24, 2025 •

edited

Loading