Skip to content
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

QR-Code detector : multiple detection #15338

Open
wants to merge 48 commits into
base: 3.4
from

Conversation

@rayonnant14
Copy link

rayonnant14 commented Aug 19, 2019

Merge with extra: opencv/opencv_extra#684

@rayonnant14 rayonnant14 changed the title My detect and decode 3.4 QR-Code detector : multiple detection Aug 19, 2019
modules/objdetect/include/opencv2/objdetect.hpp Outdated Show resolved Hide resolved
modules/objdetect/perf/perf_qrcode_pipeline.cpp Outdated Show resolved Hide resolved
modules/objdetect/src/qrcode.cpp Show resolved Hide resolved
modules/objdetect/src/qrcode.cpp Outdated Show resolved Hide resolved
modules/objdetect/src/qrcode.cpp Outdated Show resolved Hide resolved
modules/objdetect/src/qrcode.cpp Outdated Show resolved Hide resolved
modules/objdetect/src/qrcode.cpp Outdated Show resolved Hide resolved
modules/objdetect/src/qrcode.cpp Outdated Show resolved Hide resolved
modules/objdetect/src/qrcode.cpp Outdated Show resolved Hide resolved
modules/objdetect/test/test_qrcode.cpp Outdated Show resolved Hide resolved
modules/objdetect/src/qrcode.cpp Outdated Show resolved Hide resolved
rayonnant14 added 4 commits Oct 18, 2019
@asmorkalov

This comment has been minimized.

Copy link
Contributor

asmorkalov commented Oct 24, 2019

@rayonnant14, @allnes What is the PR status? Is it still WIP?

@allnes

This comment has been minimized.

Copy link
Contributor

allnes commented Oct 24, 2019

@rayonnant14, @allnes What is the PR status? Is it still WIP?

Yes, this PR in progress.

rayonnant14 added 5 commits Oct 30, 2019
…and vector<vector<Point2f>> in MultipleDetectAndDecode
rayonnant14 added 3 commits Nov 15, 2019
@allnes

This comment has been minimized.

Copy link
Contributor

allnes commented Nov 17, 2019

it looks good 👍
@alalek @dkurt pls review again.

@allnes
allnes approved these changes Nov 17, 2019
Copy link
Contributor

alalek left a comment

Use inheritance for MultipleQRDetect. Reduce amount of duplicated / similar code.

@param img grayscale or color (BGR) image containing (or not) QR codes.
@param points Output vector of vector of vertices of the minimum-area quadrangle containing the codes.
*/
CV_WRAP bool multipleDetect(InputArray img, OutputArrayOfArrays points) const;

This comment has been minimized.

Copy link
@alalek

alalek Nov 18, 2019

Contributor

detectMulti (like detectMultiScale() in objdetect)

From API point of view - it make sense to add maxCount or minSize parameters. Or add them as a class object properties.

modules/objdetect/include/opencv2/objdetect.hpp Outdated Show resolved Hide resolved
@param points optional output vector of arrays of vertices of the found QR code quadrangle. Will be empty if not found.
@param straight_qrcode The optional output vector of images containing rectified and binarized QR codes
*/
CV_WRAP std::vector<cv::String> multipleDetectAndDecode(InputArray img, OutputArrayOfArrays points = noArray(),

This comment has been minimized.

Copy link
@alalek

alalek Nov 18, 2019

Contributor

detectAndDecodeMulti

with limitation parameters.

@param eps_y Epsilon neighborhood, which allows you to determine the vertical pattern of the scheme 1:1:3:1:1 according to QR code standard.
*/
CV_EXPORTS bool multipleDetectQRCode(InputArray in, std::vector< std::vector< Point > > &points,
double eps_x = 0.2, double eps_y = 0.1);

This comment has been minimized.

Copy link
@alalek

alalek Nov 18, 2019

Contributor

detectQRCodeMulti + parameters

This comment has been minimized.

Copy link
@dkurt

dkurt Nov 24, 2019

Member

Do we really need two different epsilons? I mean according to QR codes standard, both horizontal and vertical directions should be equal, aren't they?

@@ -1245,5 +1228,1568 @@ cv::String QRCodeDetector::detectAndDecode(InputArray in,
return decoded_info;
}

class MultipleQRDetect

This comment has been minimized.

Copy link
@alalek

alalek Nov 18, 2019

Contributor

Why is inheritance not used here?


switch(points.type())
{
case 10:

This comment has been minimized.

Copy link
@alalek

alalek Nov 18, 2019

Contributor

10?

How do you plan to refactor code about OpenCV types?
Magic and hard-coded values must be avoided.

Consider using .convertTo().

@rayonnant14

This comment has been minimized.

Copy link
Author

rayonnant14 commented Nov 19, 2019

@alalek fixed
Look again, please

@alalek alalek self-requested a review Nov 19, 2019
@param points vector of Quadrangle vertices found by detect() method (or some other algorithm).
@param straight_qrcode The optional output vector of images containing rectified and binarized QR codes
*/
CV_WRAP std::vector<cv::String> decodeMulti(InputArray img, InputArrayOfArrays points, OutputArrayOfArrays straight_qrcode = noArray());

This comment has been minimized.

Copy link
@alalek

alalek Nov 19, 2019

Contributor

We usually don't return std::vector from functions (it is very inefficient in C++98):

bool decodeMulti(InputArray img, CV_OUT std::vector<std::string>& decoded_info,
        InputArrayOfArrays points, OutputArrayOfArrays straight_qrcode = noArray());

cv::String => std::string

This comment has been minimized.

Copy link
@rayonnant14

rayonnant14 Nov 20, 2019

Author

@alalek std::string doesn't work fine in python and java
Maybe, it is better to use cv::String to avoid problems?

const int x = cvRound(list_lines[pnt][0] + list_lines[pnt][2] * 0.5);
const int y = cvRound(list_lines[pnt][1]);

// --------------- Search vertical up-lines --------------- //

This comment has been minimized.

Copy link
@alalek

alalek Nov 19, 2019

Contributor

There is the the similar code in QRDetect::separateVerticalLines().
What is the difference?
Why is the difference?

I try to describe problem which we should avoid by unifying and reusing the same code for features finding. We must have consistent behavior between QRDetect and QRDetectMulti:

  • QRDetectMulti - no detections ==> QRDetect - no detection too and vice versa
  • QRDetectMulti - has detections ==> QRDetect - one of these detections (largest)
  • QRDetect - has detection ==> QRDetectMulti - has detections including this one

This comment has been minimized.

Copy link
@rayonnant14

rayonnant14 Nov 20, 2019

Author

@alalek QRDetect::separateVerticalLines() finds the best coeff_epsilon by comparison of compactness, which kmeans returns (supposed that number of clusters is 3(1 qr-code).
If there is more than one qr-code on image this method doesn't work correctly.
By this reason there is some changes in code in QRDetectMulti::separateVerticalLines()
Maybe, it should be renamed to avoid the mess


bool QRDetectMulti::compareSquare::operator()(vector<int> a, vector<int> b) const
{
return 0.5 * abs((points[a[1]].x - points[a[0]].x) *

This comment has been minimized.

Copy link
@alalek

alalek Nov 19, 2019

Contributor

0.5 * is not needed (on both sides of the condition)

else return false;
}

bool QRDetectMulti::compareSquare::operator()(vector<int> a, vector<int> b) const

This comment has been minimized.

Copy link
@alalek

alalek Nov 19, 2019

Contributor

vector<int> a

Do we really need vector here? I see 3 values only.
vectors and other object must be passed by "const reference" by default.

{
CV_TRACE_FUNCTION();
vector<Point2f> tmp_localization_points;
int num_qrcodes;

This comment has been minimized.

Copy link
@alalek

alalek Nov 19, 2019

Contributor

Inconsistent indentation with other parts of the file.

bool QRDetectMulti::computeTransformationPoints(size_t cur_ind)
{
CV_TRACE_FUNCTION();
if (localization_points[cur_ind].size() != 3)

This comment has been minimized.

Copy link
@alalek

alalek Nov 19, 2019

Contributor

6 spaces => 4 spaces

@@ -0,0 +1,196 @@
#include "opencv2/objdetect.hpp"

This comment has been minimized.

Copy link
@alalek

alalek Nov 19, 2019

Contributor

We don't need new sample.

Please update existing one:

  • support both modes
  • switch between modes by "space" key
cap >> frame;
if (frame.empty())
{
cout << "End of video stream" << endl;
break;
}
cvtColor(frame, src, COLOR_BGR2GRAY);
total.start();
Comment on lines 107 to 114

This comment has been minimized.

Copy link
@alalek

alalek Nov 22, 2019

Contributor

This code should be wrote once. The same note for lines 128-135.

Switch processing/drawing code only.

rayonnant14 added 3 commits Nov 22, 2019
@rayonnant14

This comment has been minimized.

Copy link
Author

rayonnant14 commented Nov 26, 2019

@alalek @dkurt please, look again

QRCodeDetector detector = new QRCodeDetector();
List < String > output = new ArrayList< String >();
detector.detectAndDecodeMulti(img, output);
assertEquals(output.get(0), "SKIP");

This comment has been minimized.

Copy link
@alalek

alalek Dec 1, 2019

Contributor

Result must be checked first.
Don't try to access for elements without checking container size.

boolean result = detector.detectAndDecodeMulti(...);
assertTrue(result);
assertEquals(output.size(), 6);
...
@@ -26,4 +28,16 @@ public void testDetectAndDecode() {
assertEquals(output, "https://opencv.org/");
}

public void testDetectAndDecodeMulti() {
Mat img = Imgcodecs.imread(testDataPath + "/cv/qrcode/multiple/6_qrcodes.png");

This comment has been minimized.

Copy link
@alalek

alalek Dec 1, 2019

Contributor

imread

Result should be checked.
Tests reports must be relevant as much as possible (image load failure instead of detector malfunction).


std::string image_path = findDataFile(root + name_current_image);
Mat src = imread(image_path);
std::vector<Mat> straight_barcode;

This comment has been minimized.

Copy link
@alalek

alalek Dec 1, 2019

Contributor

straight_barcode

Not used.

Mat src = imread(image_path);
std::vector<Mat> straight_barcode;
ASSERT_FALSE(src.empty()) << "Can't read image: " << image_path;
Comment on lines 80 to 82

This comment has been minimized.

Copy link
@alalek

alalek Dec 1, 2019

Contributor

swap two last lines or move "straight_barcode" down (before TEST_CYCLE).

std::vector< std::vector< Point2f > > corners;
std::vector< cv::String > decoded_info;
QRCodeDetector qrcode;
ASSERT_TRUE(qrcode.detectMulti(src, corners));
Comment on lines 84 to 87

This comment has been minimized.

Copy link
@alalek

alalek Dec 1, 2019

Contributor

Please keep code readable.

  • Construct variables in order of their usage.
  • Reduce variables scope as much as possible (make them local).
qrcode.detectMulti(src, corners)
  ^1^                     ^2^

/*3*/ std::vector< cv::String > decoded_info;
/*4*/ std::vector<Mat> straight_barcode;
line( lineMask, p0, p1, 255, 1, 8, 0);
line( lineMask, p1, p2, 255, 1, 8, 0);
line( lineMask, p2, p3, 255, 1, 8, 0);
line( lineMask, p0, p3, 255, 1, 8, 0);
vector< vector< Point > > contours;
vector<Vec4i> hierarchy;
findContours(lineMask, contours, hierarchy, RETR_TREE, CHAIN_APPROX_SIMPLE);
Comment on lines 1586 to 1592

This comment has been minimized.

Copy link
@alalek

alalek Dec 1, 2019

Contributor

This part must be reworked - using of drawing functions in algorithms should be avoided.
/cc @allnes

findContours(lineMask, contours, hierarchy, RETR_TREE, CHAIN_APPROX_SIMPLE);
vector<Point> indices;
vector< vector< Point > > contours2;
if(contours.size() < 1) return false;

This comment has been minimized.

Copy link
@alalek

alalek Dec 1, 2019

Contributor

Avoid merging of if condition and if body into one line.
It is bad practice which blocks code coverage tools.

double points_distance = sqrt((list_lines_y[i].x - list_lines_y[j].x) * (list_lines_y[i].x - list_lines_y[j].x)
+ ((list_lines_y[i].y - list_lines_y[j].y) * (list_lines_y[i].y - list_lines_y[j].y)));
if((points_distance >= 0) && (points_distance <= 10))
Comment on lines 1742 to 1744

This comment has been minimized.

Copy link
@alalek

alalek Dec 1, 2019

Contributor

points_distance >= 0

Always true except NaN values.

norm_triangl[2] = norm(local_point[1] - local_point[0]);

cos_angles[0] = (norm_triangl[1] * norm_triangl[1] + norm_triangl[2] * norm_triangl[2]
- norm_triangl[0] * norm_triangl[0]) / (2 * norm_triangl[1] * norm_triangl[2]);

This comment has been minimized.

Copy link
@alalek

alalek Dec 1, 2019

Contributor

You might use dot product instead of law of cosines in case of input vectors:

num_points = 0;
for(size_t i = 0; i < list_lines_y.size() - 1; i++)
Comment on lines 1736 to 1737

This comment has been minimized.

Copy link
@alalek

alalek Dec 1, 2019

Contributor

Consider adding high-level non-obvious comments into your code.
What and why are these loops doing here? What are main stages of this algorithm?

This method is ~400 lines. It definitely should be divided into well named (and well tested) parts/stages.

rayonnant14 added 3 commits Dec 10, 2019
@rayonnant14 rayonnant14 requested a review from alalek Jan 14, 2020
@alalek

This comment has been minimized.

Copy link
Contributor

alalek commented Jan 15, 2020

Again, drawing functions should be avoided.

checkPointsInsideQuadrangle() and checkPointsInsideTriangle() should not "draw" anything.
Take a look on pointPolygonTest() function.

The similar note is about checkPoints(). Intensity check can be related for "bounding rectangle".

another one fillConvexPoly() call in findQRCodeContours().

need to revise code using of floodFill() / LineIterator() functionality.

@alalek alalek force-pushed the rayonnant14:my_detect_and_decode_3.4 branch from ab4c360 to 7e60719 Jan 20, 2020
@param img grayscale or color (BGR) image containing (or not) QR codes.
@param points Output vector of vector of vertices of the minimum-area quadrangle containing the codes.
*/
CV_WRAP bool detectMulti(InputArray img, OutputArrayOfArrays points) const;

This comment has been minimized.

Copy link
@alalek

alalek Jan 21, 2020

Contributor

OutputArrayOfArrays
InputArrayOfArrays
std::vector< std::vector< Point > > corners;

Please avoid using of *OfArrays types and/or nested std::vector.
This is unnecessary for fixed size elements (4 points per QR code).

Lets store points as one "array" (without nesting):

  • std::vector< Point > where each 4 points define QR code instance (<0, 1, 2, 3>, <4, 5, 6 ,7> ...).
  • Mat - CV_32FC2 with one or multiple rows.
    and pass them as OutputArray.

This is continued discussion from this comment: #15338 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants
You can’t perform that action at this time.