Skip to content

Conversation

Certseeds
Copy link
Contributor

@Certseeds Certseeds commented Nov 19, 2020

Merge with extra: opencv/opencv_extra#860

opencv/opencv#18668

test_modules=barcode

opencv_extra=Barcode

@Certseeds
Copy link
Contributor Author

Certseeds commented Dec 1, 2020

The building bot is waiting for builds for a few days.Before it builds successfully, would anyone like to Review our class interface in barcode.hpp?

Certseeds and others added 5 commits December 9, 2020 22:26
…oint2f>>,

barcodeDirectly now just output one string,
this commit is still only contain interface, barcode module will not be compile.

Signed-off-by: Killer_Quinn <[email protected]>
@darkliang
Copy link
Contributor

darkliang commented Jan 4, 2021

Now, we pushed the implementation code that was tested in Raspberry Pi 4B and win64.

@Certseeds Certseeds marked this pull request as ready for review January 6, 2021 14:36
@Certseeds Certseeds changed the title [WIP] 1D Barcode support 1D Barcode support Jan 6, 2021
@JinhengZhang
Copy link
Contributor

@alalek @vpisarev Please, have a review. Thank you!

@darkliang
Copy link
Contributor

@alalek Hi, I thought our interface code was written in correct format, but there is no our class(BarcodeDetector) in the generated Python bindings. The Python interpreter shows:

PS C:\WINDOWS\System32> python
Python 3.7.3 (v3.7.3:ef4ec6ed12, Mar 25 2019, 22:22:05) [MSC v.1916 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import cv2.barcode as barcode
>>> barcode.BarcodeDetector()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: module 'cv2.barcode' has no attribute 'BarcodeDetector'
>>> dir(barcode)
['EAN_13', 'EAN_8', 'NONE', 'UPC_A', 'UPC_E', 'UPC_EAN_EXTENSION', '__doc__', '__loader__', '__name__', '__package__', '__spec__']

Can you help us for this?

@vpisarev
Copy link
Contributor

vpisarev commented Jan 22, 2021

thank you for the contribution! Could you please implement some regression tests and add example in C++ or Python that uses this functionality?

@darkliang darkliang force-pushed the Barcode-Support branch 2 times, most recently from c952114 to 69faf4d Compare January 26, 2021 16:00
@darkliang
Copy link
Contributor

@alalek Hi, we added the tests and sample, but tests failed because test data was not merged into opencv_extra. Can you fix it?

@alalek
Copy link
Member

alalek commented Feb 11, 2021

opencv_extra PR must be created from the same "user" GitHub account and use the same source branch name as "main" PR (to run validation).

@darkliang
Copy link
Contributor

@alalek Hi, we think our PR is ready to review. As mentioned above, we still have not resolved this issue. Can you look into this?

Copy link
Member

@alalek alalek left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution!

@darkliang
Copy link
Contributor

Thank you for the contribution!

Thanks for review, we have updated the code again. By the way, we figured out the usage of generated python code, it should be cv.barcode_BarcodeDetector().

@vpisarev vpisarev self-assigned this Apr 15, 2021
@vpisarev
Copy link
Contributor

👍

Copy link
Member

@alalek alalek left a comment

Choose a reason for hiding this comment

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

Great contribution!

Please take a look on the comments below.

// Author: darkliang

#ifndef BARCODE_CONVERTERS_HPP
#define BARCODE_CONVERTERS_HPP
Copy link
Member

Choose a reason for hiding this comment

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

please remove tabs from source code files

//void UPCEANDecoder::drawDebugLine(Mat &debug_img, const Point2i &begin, const Point2i &end) const
//{
// Result result;
// std::vector<uchar> middle;
Copy link
Member

Choose a reason for hiding this comment

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

//

Instead of using of multi-line comments, consider using of #if 0 to disable debug parts in the code.

// calculate magnitude of gradient, normalize and threshold
magnitude(scharr_x, scharr_y, gradient_magnitude);
threshold(gradient_magnitude, gradient_magnitude, 56, 1, THRESH_BINARY);
gradient_magnitude.convertTo(gradient_magnitude, CV_8U);
Copy link
Member

Choose a reason for hiding this comment

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

It is better to avoid in-placing of in/out parameters.
Especially with different types.

Copy link
Contributor

@darkliang darkliang Apr 16, 2021

Choose a reason for hiding this comment

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

It is better to avoid in-placing of in/out parameters.
Especially with different types.

I did not really understand what's your point? I rewrote the code like this:

    magnitude(scharr_x, scharr_y, temp);
    threshold(temp, temp, THRESHOLD_MAGNITUDE, 1, THRESH_BINARY);
    temp.convertTo(gradient_magnitude, CV_8U);

pt = Point(x, y);
cur_value = orientation.at<float_t>(pt);
sin_sum = sin(2 * cur_value);
cos_sum = cos(2 * cur_value);
Copy link
Member

Choose a reason for hiding this comment

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

BTW, in general sin/cos computation in nested loops is not very cheap.

Copy link
Contributor

@darkliang darkliang Apr 16, 2021

Choose a reason for hiding this comment

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

BTW, in general sin/cos computation in nested loops is not very cheap.

Do you have some suggestion for this? I analyzed the performance by Visual Studio, found that
image
It seems like preprocess() is much more time-consuming than this.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for checking this.
You are right, this is not critical part of computations for now.

Possible optimization are usually done through using of pre-computed sin/cos tables (with input value quantization).

@darkliang
Copy link
Contributor

Based on your comments, I refined the code. Please check again. @alalek @vpisarev

Copy link
Member

@alalek alalek left a comment

Choose a reason for hiding this comment

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

Well done! Thank you 👍

@darkliang
Copy link
Contributor

darkliang commented Apr 16, 2021

Well done! Thank you 👍

@alalek Thanks for your review. I fixed up a bug about java binding, please check. 😆

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.

6 participants