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

Feature barcode detector parameters #24903

Merged

Conversation

cristidbr-adapta
Copy link
Contributor

@cristidbr-adapta cristidbr-adapta commented Jan 22, 2024

Attempt to solve #24902 without changing the default detector behaviour.
Megre with extra: opencv/opencv_extra#1150

Introduces new parameters and methods to cv::barcode::BarcodeDetector.

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

@asmorkalov
Copy link
Contributor

@mshabunin could you take a look?

@cristidbr-adapta
Copy link
Contributor Author

cristidbr-adapta commented Feb 2, 2024

Just saw #23889 and I had verified that this PR is unable to solve the orientation delta issue. However, I believe users should be able to tune at least some subset of the parameters of bardetect.cpp.

Benefits and use-cases:

@mshabunin this can happen without breaking userland, but I don't like the idea of pushing additional tests wrt the algo state. You seem to have a better understanding of how it works.
@asmorkalov the ArUco params impl looks elegant. Is that the standard way users would expect to adjust such values for classes in opencv::objdetect?

@mshabunin
Copy link
Contributor

I believe extracting parameters to a separate structure is not necessary in this case, it can be done using get/set methods (see comments by @opencv-alalek).

Also I'm thinking whether passing window sizes array should be done using InputArray/OutputArray style, maybe it could be simple vector<double>?

We will also need new test case or even several. Perhaps existting test images can be resized or augmented to illustrate properties effect.

Are there any alternative names for the new properties? In my opinion they are too long and complex, for example word "detector" is not techically necessary. windowSizes are actually scales. Piece thresh usually should placed at the end - setDownSampleThresh instead of setThreshDownSample. setGradientThresh?

New parameter documentation should include estimated effect on quality, performance, detection count, etc. Maybe some of parameters better work when they correlate with other parameter or image dimensions? Allowed values range should be documented and verified in the set method.

@cristidbr-adapta
Copy link
Contributor Author

Thanks for the detailed feedback, it is clear how to proceed now. Will have some time to address this over the next couple of days.

@asmorkalov
Copy link
Contributor

@cristidbr-adapta Friendly reminder.

@cristidbr-adapta
Copy link
Contributor Author

@mshabunin added set/get methods for all parameters and vector<double> for scales. Removed unnecessary parameter for NMS box threshold as I was able to derive that based on the impl internals.

Regarding tests, I still need to tweak it for adding a regression to handle the _detector_sm.jpg file in extra. Allowed ranges are not asserted yet. Impact to performance should be expected when the default behaviour is changed to allow larger input sizes.

Docs TBD.

@cristidbr-adapta
Copy link
Contributor Author

For docs, should I update the existing tutorial with new methods and bump version to 4.10?

Copy link
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.

👍

@asmorkalov
Copy link
Contributor

@mshabunin @opencv-alalek could you take a look again.

@asmorkalov
Copy link
Contributor

@mshabunin @opencv-alalek Friendly reminder.

@asmorkalov
Copy link
Contributor

@dkurt Could you take a look?

modules/objdetect/include/opencv2/objdetect/barcode.hpp Outdated Show resolved Hide resolved
modules/objdetect/include/opencv2/objdetect/barcode.hpp Outdated Show resolved Hide resolved
@@ -82,6 +85,19 @@ TEST_P(BarcodeDetector_main, interface)
vector<string> types;
vector<string> lines;

// preset parameters based on filename
{
if(fname.find("_detector_lg.jpg") != string::npos || fname.find("_detector_sm.jpg") != string::npos)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we avoid creating extra test images?

I suggest the following test scenario:

  1. take one of existing images
  2. set detector parameters to some predefined values
  3. run detector and verify that result is as expected (e.g. it can be empty with some parameter set)

Test can be parametrised with (params set, expected result). Test can be combined with new regression test (check parameters after setting).

Input image can be modified in some way (scaled, rotated, filtered,...) or it can be built from several other images (vconcat).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we avoid creating extra test images?

Extra test data was designed to detect abnormal changes to the barcode_detector.cpp algorithm impl wrt to false positives. It needs to happen.

Copy link
Contributor

Choose a reason for hiding this comment

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

What is different in these images comparing to pre-existing ones? If we really need them, can we reduce their size, for example we could add only one of them (_sm) and get crops from it in the test to replace other sizes or maybe somehow extend _lg image instead.

Copy link
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.

👍

  • Renamed public methods according suggestion.
  • Use float for scales.
  • Minor cleanup.

modules/objdetect/src/barcode.cpp Outdated Show resolved Hide resolved
modules/objdetect/include/opencv2/objdetect/barcode.hpp Outdated Show resolved Hide resolved
modules/objdetect/src/barcode_detector/bardetect.cpp Outdated Show resolved Hide resolved
@asmorkalov asmorkalov force-pushed the feature-barcode-detector-parameters branch from 732c977 to de4700d Compare April 22, 2024 09:31
@asmorkalov asmorkalov merged commit e05ad56 into opencv:4.x May 20, 2024
29 of 30 checks passed
@mshabunin mshabunin mentioned this pull request Jun 14, 2024
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.

None yet

7 participants