Skip to content

Fix infinite loop on ArUco apriltag refinement #3220

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

Merged
merged 4 commits into from
Apr 14, 2022

Conversation

buq2
Copy link
Contributor

@buq2 buq2 commented Apr 1, 2022

Software entered infinite loop when image height
was smaller than 10*cv::getNumThreads(). With high
core count machines this could happen with very
reasonable image sizes.

Fix is to ensure that chunksize is at least 1.

The bug can be triggered with following test program:

#include <opencv2/aruco.hpp>
#include <opencv2/core/utility.hpp>
#include <iostream>
#include <opencv2/imgcodecs.hpp>

int main(int argc, char *argv[]) {
    // Bug will happen even with 1 thread, but then the image
    // will be too small to detect corners.
    // cv::setNumThreads(1);
    const auto num_th = cv::getNumThreads();
    std::cout << "Number of threads: " << num_th << std::endl;

    // If 'freeze' is set to true, bug is triggered
    constexpr bool freeze = true;
    std::cout << "Should freeze: " << freeze << std::endl;

    // When triggering the bug, use image size that is
    // 1 pix smaller than 10*number of threads
    const auto height_img = freeze ? num_th*10-1 : num_th*10;
    std::cout << "Image height: " << height_img << std::endl;

    // Just to get nice white boarder
    const int shift = height_img > 10 ? 5 : 1;
    const auto height_marker = height_img-2*shift;

    // Create marker template
    cv::Mat img_marker;
    cv::Ptr<cv::aruco::Dictionary> dictionary = cv::aruco::getPredefinedDictionary(cv::aruco::DICT_4X4_50);
    cv::aruco::drawMarker(dictionary, 23, height_marker, img_marker, 1);

    // Copy to bigger image
    cv::Mat img(height_img, height_img, CV_8UC1);
    img.setTo(255);
    img_marker.copyTo(img(cv::Rect(shift, shift, height_marker, height_marker)));

    // Just for visualizing
    cv::imwrite("marker.png", img);

    // Bug only in apriltag refinement
    auto params = cv::aruco::DetectorParameters::create();
    params->cornerRefinementMethod = cv::aruco::CORNER_REFINE_APRILTAG;

    std::vector<std::vector<cv::Point2f>> corners;
    std::vector<int> ids;

    // If freeze=true, this will never return
    cv::aruco::detectMarkers(img, dictionary, corners, ids, params);

    // Ensure we get reasonable corners 
    // (not with this default test, white boarder needs to be bigger)
    for (const auto &marker : corners) {
        for (const auto &p : marker) {
            std::cout << p << std::endl;
        }
    }
}

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

Software entered infinite loop when image height
was smaller than 10*cv::getNumThreads(). With high
core count machines this could happen with very
reasonable image sizes.

Fix is to ensure that chunksize is at least 1.
@asmorkalov
Copy link
Contributor

@buq2 Thanks for the contribution. The patch looks reasonable for me. Could you add your reproducer with setNumThreads to the PR as test?

@asmorkalov asmorkalov added the test label Apr 5, 2022
@buq2
Copy link
Contributor Author

buq2 commented Apr 5, 2022

@asmorkalov Do you mean adding the main function code from the initial PR description to (for example) https://github.com/opencv/opencv_contrib/blob/3.4/modules/aruco/test/test_arucodetection.cpp ? Sure! The "test" is not a very good test case as if it fails, it hangs indefinitely, but maybe it is better than nothing. I'll add it soonish and make it such that it will test also the other refinement methods with different initial number of threads with small aruco images.

Test ensures that different aruco detection methods do not
produce different results based on number of threads.

Test was created after observing infinite loop caused
by small image and large number of threads when using
apriltag corner refinement.
@buq2 buq2 force-pushed the aruco-apriltag-infinite-loop-fix branch from 4e44309 to 43ff44b Compare April 9, 2022 13:46
@buq2 buq2 requested a review from asmorkalov April 9, 2022 13:57
@asmorkalov
Copy link
Contributor

@buq2 Thanks for the test. I refactored it a bit, to make it parameterized (obvious test report in case of failure) and extracted the iteration with num_threads=1 as ground truth to make it obvious for reader.

Copy link
Contributor

@AleksandrPanov AleksandrPanov left a comment

Choose a reason for hiding this comment

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

LGTM, thx for fix and tests

@buq2
Copy link
Contributor Author

buq2 commented Apr 11, 2022

Thanks for the reviews and refactoring! Next one will be less painful for everyone :)

@asmorkalov
Copy link
Contributor

@alalek I propose to merge the patch.

@opencv-pushbot opencv-pushbot merged commit 0596c05 into opencv:3.4 Apr 14, 2022
AleksandrPanov pushed a commit to AleksandrPanov/opencv_contrib that referenced this pull request Apr 14, 2022
…-fix

Fix infinite loop on ArUco apriltag refinement

* Fix infinite loop on ArUco apriltag refinement

Software entered infinite loop when image height
was smaller than 10*cv::getNumThreads(). With high
core count machines this could happen with very
reasonable image sizes.

Fix is to ensure that chunksize is at least 1.

* Test aruco detection with different number of threads

Test ensures that different aruco detection methods do not
produce different results based on number of threads.

Test was created after observing infinite loop caused
by small image and large number of threads when using
apriltag corner refinement.

* Test refactoring.

* Syntax fix for pre-C++11 compilers.

Co-authored-by: Alexander Smorkalov <[email protected]>
@AleksandrPanov AleksandrPanov mentioned this pull request Apr 14, 2022
Comment on lines +769 to +770
ASSERT_EQ(original_ids.size(), 1);
ASSERT_EQ(original_corners.size(), 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: comparison between signed and unsigned integer expressions

sorry, I overlooked

cv::aruco::detectMarkers(img, dictionary, corners, ids, params);

// If we don't find any markers, the test is broken
ASSERT_EQ(ids.size(), 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: comparison between signed and unsigned integer expressions

sorry, I overlooked

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.

5 participants