Skip to content

Add detect qr with aruco#23264

Merged
asmorkalov merged 16 commits intoopencv:4.xfrom
AleksandrPanov:add_detect_qr_with_aruco
Jun 2, 2023
Merged

Add detect qr with aruco#23264
asmorkalov merged 16 commits intoopencv:4.xfrom
AleksandrPanov:add_detect_qr_with_aruco

Conversation

@AleksandrPanov
Copy link
Copy Markdown
Contributor

@AleksandrPanov AleksandrPanov commented Feb 17, 2023

Using Aruco to detect finder patterns to search QR codes.

TODO (in next PR):

  • add single QR detect (update detect() and detectAndDecode())
  • need reduce full enumeration of finder patterns
  • need add finder pattern info to decode step
  • need to merge the pipeline of the old and new algorithm

Current results:
+20% total detect, +8% total decode in OpenCV QR benchmark

res1

78.4% detect, 58.7% decode vs 58.5 detect, 50.5% decode in default

main.py.txt

res2

add new info to google docs

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

Copy link
Copy Markdown
Contributor

@asmorkalov asmorkalov left a comment

Choose a reason for hiding this comment

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

Please add:

  • The table with benchmark results for 4.7 and new version;
  • Performance test (parameterize existing) and it's result.

@AleksandrPanov AleksandrPanov force-pushed the add_detect_qr_with_aruco branch 3 times, most recently from bdbe5de to 339e8a1 Compare March 23, 2023 22:40
@AleksandrPanov AleksandrPanov marked this pull request as ready for review March 29, 2023 13:12
@AleksandrPanov AleksandrPanov force-pushed the add_detect_qr_with_aruco branch 3 times, most recently from 8602d7e to 04785da Compare April 5, 2023 15:41
@AleksandrPanov AleksandrPanov force-pushed the add_detect_qr_with_aruco branch from fa28b6f to 3e981d0 Compare April 6, 2023 08:02
@asmorkalov asmorkalov added this to the 4.8.0 milestone Apr 7, 2023
@asmorkalov
Copy link
Copy Markdown
Contributor

/build/precommit_linux64/4.x/opencv/modules/objdetect/perf/perf_qrcode_pipeline.cpp:101:111: warning: passing 'cv::QRDetectMethod' chooses 'int' over 'unsigned int' [-Wsign-promo]
/build/precommit_linux64/4.x/opencv/modules/objdetect/perf/perf_qrcode_pipeline.cpp:101:111: warning:   in call to 'std::__cxx11::string std::__cxx11::to_string(int)' [-Wsign-promo]
/build/precommit_linux64/4.x/opencv/modules/objdetect/test/test_qrcode.cpp:371:111: warning: passing 'cv::QRDetectMethod' chooses 'int' over 'unsigned int' [-Wsign-promo]
/build/precommit_linux64/4.x/opencv/modules/objdetect/test/test_qrcode.cpp:371:111: warning:   in call to 'std::__cxx11::string std::__cxx11::to_string(int)' [-Wsign-promo]

@asmorkalov
Copy link
Copy Markdown
Contributor

General notes:

  • Only ::detectMulti is extended with the new algorithm. ::detect, detectAndDecode, detectAndDecodeCurved are not covered.
  • It makes sense to decouple the old algorithm and new one as 2 internal classes with own virtual detect, detectMulti, etc. The only check point for algo type is impl pointer initialization.

@AleksandrPanov AleksandrPanov force-pushed the add_detect_qr_with_aruco branch 4 times, most recently from cb7a30b to 0d6eddf Compare April 14, 2023 13:10
@asmorkalov asmorkalov force-pushed the add_detect_qr_with_aruco branch from 3d9c6e5 to fca3934 Compare May 17, 2023 14:51
@asmorkalov
Copy link
Copy Markdown
Contributor

@opencv-alalek Could you update ABI-complience-checker settings to cover API changes.

@asmorkalov
Copy link
Copy Markdown
Contributor

@opencv-alalek Friendly reminder.

Comment on lines +907 to +908
/** @brief Aruco detector parameters are used to search for the finder patterns. */
CV_PROP_RW aruco::DetectorParameters arucoParams;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

broken indentation

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed

ASSERT_FALSE(src.empty()) << "Can't read image: " << image_path;
#ifdef HAVE_QUIRC
QRCodeDetector qrcode;
Ptr<QRCodeDetectorBase> qrcode;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

PImpl doesn't need Ptr<>. They are already smart pointers.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

removed Ptr

static
void runQR(
QRCodeDetector& qrcode, const Mat& input,
Ptr<QRCodeDetectorBase> qrcode, const Mat& input,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

  1. We don't need Ptr for PImpl which is already smart ptr.
  2. Ptrs must be passed as "const reference" be default.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

removed Ptr, added ref

@param epsX Epsilon neighborhood, which allows you to determine the horizontal pattern
of the scheme 1:1:3:1:1 according to QR code standard.
*/
CV_WRAP void setEpsX(double epsX);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

All "set" methods should return *this reference to allow construction chains.

Copy link
Copy Markdown
Contributor Author

@AleksandrPanov AleksandrPanov May 26, 2023

Choose a reason for hiding this comment

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

these setters drop CI (Python bindings)
Maybe update the setters later?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In DNN pyopencv_cv_dnn_dnn_ClassificationModel_setEnableSoftmaxPostProcessing has been properly generated.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done.
The directive CV_EXPORTS_W_SIMPLE instead of CV_EXPORTS_W directive helped to add setter

Copy link
Copy Markdown
Contributor Author

@AleksandrPanov AleksandrPanov Jun 1, 2023

Choose a reason for hiding this comment

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

first was added QRCodeDetectorAruco& setDetectorParameters
now were added QRCodeDetector& setEpsX, QRCodeDetector& setEpsY, QRCodeDetector& setUseAlignmentMarkers

now it's done)

@AleksandrPanov AleksandrPanov force-pushed the add_detect_qr_with_aruco branch from c162f4f to e964404 Compare May 25, 2023 21:48
@AleksandrPanov AleksandrPanov force-pushed the add_detect_qr_with_aruco branch from 4a81fc3 to b9d1942 Compare May 26, 2023 08:37
class CV_EXPORTS_W QRCodeDetector
{
class CV_EXPORTS_W QRCodeDetectorBase {
public:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

As mentioned on team meetings, we have to follow dnn:Model approach.

Constructors / destructors / assignment / move operators should be defined explicitly.
Also we should not have public default constructor (but we have issue with bindings - deprecation and comment should be on-place).

Copy link
Copy Markdown
Contributor Author

@AleksandrPanov AleksandrPanov May 31, 2023

Choose a reason for hiding this comment

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

added with dnn::Model style:
image

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

//CV_DEPRECATED_EXTERNAL

Why is it commented?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

removed the comments

virtual bool detectAndDecodeMulti(InputArray img, std::vector<std::string>& decoded_info, OutputArray points,
OutputArrayOfArrays straight_qrcode) const = 0;

virtual ~Impl() {}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It is better to declare virtual destructor first.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

};

bool QRCodeDetectorBase::detect(InputArray img, OutputArray points) const {
return p->detect(img, points);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

->

All deference cases must be pre-checked for NULL values.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

added CV_Assert(p != nullptr);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just CV_Assert(p) as we don't really want to retrieve the pointer itself.

It is like container.size() == 0 vs container.empty().

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

added just CV_Assert(p)


QRCodeDetector::QRCodeDetector() : p(new Impl) {}
QRCodeDetector::QRCodeDetector() {
p = new ImplContour();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

new

makePtr

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

replaced

CV_CheckGT(contourArea(src_points), 0.0, "Invalid QR code source points");

QRDecode qrdec(p->useAlignmentMarkers);
QRDecode qrdec((std::static_pointer_cast<ImplContour>)(p)->useAlignmentMarkers);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

PImpl pattern classes should not have large method implementations.
All logic must go to Impl.

Copy link
Copy Markdown
Contributor Author

@AleksandrPanov AleksandrPanov May 31, 2023

Choose a reason for hiding this comment

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

I propose to make this fix in another PR. This fix will erase the history and increase the diff.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I moved decodeCurved and detectAndDecodeCurved to ImplContour. I think I've fixed everything you asked for. Is there anything else that needs to be fixed?

@param epsX Epsilon neighborhood, which allows you to determine the horizontal pattern
of the scheme 1:1:3:1:1 according to QR code standard.
*/
CV_WRAP void setEpsX(double epsX);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In DNN pyopencv_cv_dnn_dnn_ClassificationModel_setEnableSoftmaxPostProcessing has been properly generated.

@AleksandrPanov AleksandrPanov force-pushed the add_detect_qr_with_aruco branch 2 times, most recently from e633c4c to f860430 Compare May 30, 2023 21:29
@AleksandrPanov AleksandrPanov force-pushed the add_detect_qr_with_aruco branch from f860430 to 866289e Compare May 30, 2023 23:31
Copy link
Copy Markdown
Contributor

@opencv-alalek opencv-alalek left a comment

Choose a reason for hiding this comment

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

LGTM 👍

class CV_EXPORTS_W QRCodeDetector
{
class CV_EXPORTS_W QRCodeDetectorBase {
public:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

//CV_DEPRECATED_EXTERNAL

Why is it commented?

};

bool QRCodeDetectorBase::detect(InputArray img, OutputArray points) const {
return p->detect(img, points);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just CV_Assert(p) as we don't really want to retrieve the pointer itself.

It is like container.size() == 0 vs container.empty().

vector<vector<Point2f>> alignmentMarkers;
vector<Point2f> updateQrCorners;
mutable vector<vector<Point2f>> alignmentMarkers;
mutable vector<Point2f> updateQrCorners;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

suspicious code, need to be revised later.


QRCodeDetector::~QRCodeDetector() {}
QRCodeDetector& QRCodeDetector::setEpsX(double epsX) {
(std::static_pointer_cast<ImplContour>)(p)->epsX = epsX;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

no need braces here:

std::static_pointer_cast<ImplContour>(p)


checked dynamic_pointer_cast is more secure.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

added dynamic_pointer_cast instead static_pointer_cast

scaleTimingPatternScore = 0.9f;
}

struct FinderPatternInfo {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

BTW, it makes sense to hide local internal classes (especially with some generic names) into inner anonymous namespace.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

added FinderPatternInfo and QRCode into anonymous namespace

static
void runQR(
QRCodeDetector& qrcode, const Mat& input,
QRCodeDetectorBase& qrcode, const Mat& input,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Use const reference for inputs

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

added const reference and added const to decode methods

@asmorkalov asmorkalov merged commit 9fa014e into opencv:4.x Jun 2, 2023
@asmorkalov asmorkalov mentioned this pull request Jul 12, 2023
thewoz pushed a commit to thewoz/opencv that referenced this pull request Jan 4, 2024
…h_aruco

Add detect qr with aruco opencv#23264

Using Aruco to detect finder patterns to search QR codes.

TODO (in next PR):
- add single QR detect (update `detect()` and `detectAndDecode()`)
- need reduce full enumeration of finder patterns
- need add finder pattern info to `decode` step
- need to merge the pipeline of the old and new algorithm

[Current results:](https://docs.google.com/spreadsheets/d/1ufKyR-Zs-IGXwvqPgftssmTlceVjiQX364sbrjr2QU8/edit#gid=1192415584)
+20% total detect, +8% total decode in OpenCV [QR benchmark](https://github.com/opencv/opencv_benchmarks/tree/develop/python_benchmarks/qr_codes) 

![res1](https://user-images.githubusercontent.com/22337800/231228556-191d3eae-a318-44e1-af99-e7d420bf6248.png)


78.4% detect, 58.7% decode vs 58.5 detect, 50.5% decode in default

[main.py.txt](https://github.com/opencv/opencv/files/10762369/main.py.txt)

![res2](https://user-images.githubusercontent.com/22337800/231229123-ed7f1eda-159a-444b-a3ff-f107d8eb4a20.png)


add new info to [google docs](https://docs.google.com/spreadsheets/d/1ufKyR-Zs-IGXwvqPgftssmTlceVjiQX364sbrjr2QU8/edit?usp=sharing)


### Pull Request Readiness Checklist

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

- [x] I agree to contribute to the project under Apache 2 License.
- [x] 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
- [x] The PR is proposed to the proper branch
- [x] 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
thewoz pushed a commit to thewoz/opencv that referenced this pull request May 29, 2024
…h_aruco

Add detect qr with aruco opencv#23264

Using Aruco to detect finder patterns to search QR codes.

TODO (in next PR):
- add single QR detect (update `detect()` and `detectAndDecode()`)
- need reduce full enumeration of finder patterns
- need add finder pattern info to `decode` step
- need to merge the pipeline of the old and new algorithm

[Current results:](https://docs.google.com/spreadsheets/d/1ufKyR-Zs-IGXwvqPgftssmTlceVjiQX364sbrjr2QU8/edit#gid=1192415584)
+20% total detect, +8% total decode in OpenCV [QR benchmark](https://github.com/opencv/opencv_benchmarks/tree/develop/python_benchmarks/qr_codes) 

![res1](https://user-images.githubusercontent.com/22337800/231228556-191d3eae-a318-44e1-af99-e7d420bf6248.png)


78.4% detect, 58.7% decode vs 58.5 detect, 50.5% decode in default

[main.py.txt](https://github.com/opencv/opencv/files/10762369/main.py.txt)

![res2](https://user-images.githubusercontent.com/22337800/231229123-ed7f1eda-159a-444b-a3ff-f107d8eb4a20.png)


add new info to [google docs](https://docs.google.com/spreadsheets/d/1ufKyR-Zs-IGXwvqPgftssmTlceVjiQX364sbrjr2QU8/edit?usp=sharing)


### Pull Request Readiness Checklist

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

- [x] I agree to contribute to the project under Apache 2 License.
- [x] 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
- [x] The PR is proposed to the proper branch
- [x] 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
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