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
Add detect qr with aruco #23264
Add detect qr with aruco #23264
Conversation
There was a problem hiding this 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.
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
broken indentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
|
||
std::string image_path = findDataFile(root + name_current_image); | ||
Mat src = imread(image_path); | ||
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.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed Ptr
samples/cpp/qrcode.cpp
Outdated
@@ -157,7 +160,7 @@ void drawQRCodeResults(Mat& frame, const vector<Point>& corners, const vector<cv | |||
|
|||
static | |||
void runQR( | |||
QRCodeDetector& qrcode, const Mat& input, | |||
Ptr<QRCodeDetectorBase> qrcode, const Mat& input, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- We don't need
Ptr
for PImpl which is already smart ptr. Ptr
s must be passed as "const reference" be default.
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
c162f4f
to
e964404
Compare
4a81fc3
to
b9d1942
Compare
@@ -763,29 +764,8 @@ class CV_EXPORTS_W QRCodeEncoder { | |||
|
|||
}; | |||
|
|||
class CV_EXPORTS_W QRCodeDetector | |||
{ | |||
class CV_EXPORTS_W QRCodeDetectorBase { | |||
public: |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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);
There was a problem hiding this comment.
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()
.
There was a problem hiding this comment.
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)
modules/objdetect/src/qrcode.cpp
Outdated
}; | ||
|
||
QRCodeDetector::QRCodeDetector() : p(new Impl) {} | ||
QRCodeDetector::QRCodeDetector() { | ||
p = new ImplContour(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new
makePtr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
replaced
modules/objdetect/src/qrcode.cpp
Outdated
@@ -2832,7 +2881,7 @@ cv::String QRCodeDetector::decodeCurved(InputArray in, InputArray points, | |||
CV_Assert(src_points.size() == 4); | |||
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.
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
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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.
e633c4c
to
f860430
Compare
f860430
to
866289e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
@@ -763,29 +764,8 @@ class CV_EXPORTS_W QRCodeEncoder { | |||
|
|||
}; | |||
|
|||
class CV_EXPORTS_W QRCodeDetector | |||
{ | |||
class CV_EXPORTS_W QRCodeDetectorBase { | |||
public: |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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.
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.
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.
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
samples/cpp/qrcode.cpp
Outdated
@@ -157,7 +160,7 @@ void drawQRCodeResults(Mat& frame, const vector<Point>& corners, const vector<cv | |||
|
|||
static | |||
void runQR( | |||
QRCodeDetector& qrcode, const Mat& input, | |||
QRCodeDetectorBase& qrcode, const Mat& input, |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
…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
Using Aruco to detect finder patterns to search QR codes.
TODO (in next PR):
detect()
anddetectAndDecode()
)decode
stepCurrent 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.