Conversation
asmorkalov
left a comment
There was a problem hiding this comment.
Please add:
- The table with benchmark results for 4.7 and new version;
- Performance test (parameterize existing) and it's result.
bdbe5de to
339e8a1
Compare
8602d7e to
04785da
Compare
fa28b6f to
3e981d0
Compare
|
|
General notes:
|
cb7a30b to
0d6eddf
Compare
3d9c6e5 to
fca3934
Compare
|
@opencv-alalek Could you update ABI-complience-checker settings to cover API changes. |
|
@opencv-alalek Friendly reminder. |
| /** @brief Aruco detector parameters are used to search for the finder patterns. */ | ||
| CV_PROP_RW aruco::DetectorParameters arucoParams; |
| ASSERT_FALSE(src.empty()) << "Can't read image: " << image_path; | ||
| #ifdef HAVE_QUIRC | ||
| QRCodeDetector qrcode; | ||
| Ptr<QRCodeDetectorBase> qrcode; |
There was a problem hiding this comment.
PImpl doesn't need Ptr<>. They are already smart pointers.
samples/cpp/qrcode.cpp
Outdated
| static | ||
| void runQR( | ||
| QRCodeDetector& qrcode, const Mat& input, | ||
| Ptr<QRCodeDetectorBase> qrcode, const Mat& input, |
There was a problem hiding this comment.
- We don't need
Ptrfor PImpl which is already smart ptr. Ptrs must be passed as "const reference" be default.
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
All "set" methods should return *this reference to allow construction chains.
There was a problem hiding this comment.
these setters drop CI (Python bindings)
Maybe update the setters later?
There was a problem hiding this comment.
In DNN pyopencv_cv_dnn_dnn_ClassificationModel_setEnableSoftmaxPostProcessing has been properly generated.
There was a problem hiding this comment.
done.
The directive CV_EXPORTS_W_SIMPLE instead of CV_EXPORTS_W directive helped to add setter
There was a problem hiding this comment.
first was added QRCodeDetectorAruco& setDetectorParameters
now were added QRCodeDetector& setEpsX, QRCodeDetector& setEpsY, QRCodeDetector& setUseAlignmentMarkers
now it's done)
c162f4f to
e964404
Compare
4a81fc3 to
b9d1942
Compare
| class CV_EXPORTS_W QRCodeDetector | ||
| { | ||
| class CV_EXPORTS_W QRCodeDetectorBase { | ||
| public: |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
//CV_DEPRECATED_EXTERNAL
Why is it commented?
There was a problem hiding this comment.
removed the comments
modules/objdetect/src/qrcode.cpp
Outdated
| virtual bool detectAndDecodeMulti(InputArray img, std::vector<std::string>& decoded_info, OutputArray points, | ||
| OutputArrayOfArrays straight_qrcode) const = 0; | ||
|
|
||
| virtual ~Impl() {} |
There was a problem hiding this comment.
It is better to declare virtual destructor first.
| }; | ||
|
|
||
| bool QRCodeDetectorBase::detect(InputArray img, OutputArray points) const { | ||
| return p->detect(img, points); |
There was a problem hiding this comment.
->
All deference cases must be pre-checked for NULL values.
There was a problem hiding this comment.
added CV_Assert(p != nullptr);
There was a problem hiding this comment.
Just CV_Assert(p) as we don't really want to retrieve the pointer itself.
It is like container.size() == 0 vs container.empty().
There was a problem hiding this comment.
added just CV_Assert(p)
modules/objdetect/src/qrcode.cpp
Outdated
|
|
||
| QRCodeDetector::QRCodeDetector() : p(new Impl) {} | ||
| QRCodeDetector::QRCodeDetector() { | ||
| p = new ImplContour(); |
modules/objdetect/src/qrcode.cpp
Outdated
| 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); |
There was a problem hiding this comment.
PImpl pattern classes should not have large method implementations.
All logic must go to Impl.
There was a problem hiding this comment.
I propose to make this fix in another PR. This fix will erase the history and increase the diff.
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
In DNN pyopencv_cv_dnn_dnn_ClassificationModel_setEnableSoftmaxPostProcessing has been properly generated.
e633c4c to
f860430
Compare
f860430 to
866289e
Compare
| class CV_EXPORTS_W QRCodeDetector | ||
| { | ||
| class CV_EXPORTS_W QRCodeDetectorBase { | ||
| public: |
There was a problem hiding this comment.
//CV_DEPRECATED_EXTERNAL
Why is it commented?
| }; | ||
|
|
||
| bool QRCodeDetectorBase::detect(InputArray img, OutputArray points) const { | ||
| return p->detect(img, points); |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
suspicious code, need to be revised later.
modules/objdetect/src/qrcode.cpp
Outdated
|
|
||
| QRCodeDetector::~QRCodeDetector() {} | ||
| QRCodeDetector& QRCodeDetector::setEpsX(double epsX) { | ||
| (std::static_pointer_cast<ImplContour>)(p)->epsX = epsX; |
There was a problem hiding this comment.
no need braces here:
std::static_pointer_cast<ImplContour>(p)
checked dynamic_pointer_cast is more secure.
There was a problem hiding this comment.
added dynamic_pointer_cast instead static_pointer_cast
| scaleTimingPatternScore = 0.9f; | ||
| } | ||
|
|
||
| struct FinderPatternInfo { |
There was a problem hiding this comment.
BTW, it makes sense to hide local internal classes (especially with some generic names) into inner anonymous namespace.
There was a problem hiding this comment.
added FinderPatternInfo and QRCode into anonymous namespace
samples/cpp/qrcode.cpp
Outdated
| static | ||
| void runQR( | ||
| QRCodeDetector& qrcode, const Mat& input, | ||
| QRCodeDetectorBase& qrcode, const Mat& input, |
There was a problem hiding this comment.
Use const reference for inputs
There was a problem hiding this comment.
added const reference and added const to decode methods
…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)  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)  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
…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)  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)  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

Using Aruco to detect finder patterns to search QR codes.
TODO (in next PR):
detect()anddetectAndDecode())decodestepCurrent results:
+20% total detect, +8% total decode in OpenCV QR benchmark
78.4% detect, 58.7% decode vs 58.5 detect, 50.5% decode in default
main.py.txt
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
Patch to opencv_extra has the same branch name.