Skip to content

Bugfix/qrcode version estimator#24364

Merged
asmorkalov merged 12 commits intoopencv:4.xfrom
bagelbytes61:bugfix/qrcode-version-estimator
Nov 16, 2023
Merged

Bugfix/qrcode version estimator#24364
asmorkalov merged 12 commits intoopencv:4.xfrom
bagelbytes61:bugfix/qrcode-version-estimator

Conversation

@bagelbytes61
Copy link
Copy Markdown
Contributor

@bagelbytes61 bagelbytes61 commented Oct 5, 2023 •
edited by asmorkalov
Loading

Fixes #24366

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

@bagelbytes61 bagelbytes61 changed the base branch from 4.x to 4.8.x October 5, 2023 17:57
@bagelbytes61 bagelbytes61 marked this pull request as draft October 5, 2023 18:11
@bagelbytes61 bagelbytes61 force-pushed the bugfix/qrcode-version-estimator branch 4 times, most recently from 05fa58a to fd815d8 Compare October 6, 2023 02:25
@bagelbytes61 bagelbytes61 marked this pull request as ready for review October 6, 2023 02:25
@bagelbytes61 bagelbytes61 mentioned this pull request Oct 6, 2023
Closed
4 tasks
@asmorkalov asmorkalov added this to the 4.9.0 milestone Oct 6, 2023
@asmorkalov asmorkalov changed the base branch from 4.8.x to 4.x October 6, 2023 10:25
@dkurt
Copy link
Copy Markdown
Member

dkurt commented Oct 6, 2023

Please rebase source branch

@bagelbytes61 bagelbytes61 force-pushed the bugfix/qrcode-version-estimator branch from fd815d8 to 62102d3 Compare October 6, 2023 14:10
@bagelbytes61
Copy link
Copy Markdown
Contributor Author

bagelbytes61 commented Oct 6, 2023

Please rebase source branch

Done!

@AleksandrPanov
Copy link
Copy Markdown
Contributor

AleksandrPanov commented Oct 7, 2023 •
edited
Loading

@bagelbytes61, several tests have fallen:

Objdetect_QRCode_Encode_Decode.regression :
unknown file: Failure
C++ exception with description "OpenCV(4.8.0-dev) /build/precommit_linux64/4.x/opencv/modules/objdetect/src/qrcode_encoder.cpp:371: error: (-5:Bad argument) The given version is not suitable for the given input string length  in function 'generateQR'
" thrown in the test body.

[  FAILED  ] Objdetect_QRCode_Encode.regression/3, where GetParam() = "version2_mode1.png"
[  FAILED  ] Objdetect_QRCode_Encode.regression/4, where GetParam() = "version2_mode2.png"
[  FAILED  ] Objdetect_QRCode_Encode.regression/6, where GetParam() = "version3_mode2.png"

https://pullrequest.opencv.org/buildbot/builders/precommit_linux64/builds/104823

https://pullrequest.opencv.org/buildbot/builders/precommit_linux64/builds/104823/steps/test_objdetect/logs/tests%20summary

There were also warnings:

/build/precommit_linux64/4.x/opencv/modules/objdetect/src/qrcode_encoder.cpp:681:12: warning: enumeration value 'MODE_AUTO' not handled in switch [-Wswitch]
/build/precommit_linux64/4.x/opencv/modules/objdetect/src/qrcode_encoder.cpp:681:12: warning: enumeration value 'MODE_ECI' not handled in switch [-Wswitch]
/build/precommit_linux64/4.x/opencv/modules/objdetect/src/qrcode_encoder.cpp:681:12: warning: enumeration value 'MODE_KANJI' not handled in switch [-Wswitch]
/build/precommit_linux64/4.x/opencv/modules/objdetect/src/qrcode_encoder.cpp:681:12: warning: enumeration value 'MODE_STRUCTURED_APPEND' not handled in switch [-Wswitch]

@AleksandrPanov
Copy link
Copy Markdown
Contributor

AleksandrPanov commented Oct 11, 2023 

[  FAILED  ] 4 tests, listed below:
[  FAILED  ] Objdetect_QRCode_Encode_Decode.regression
[  FAILED  ] Objdetect_QRCode_Encode.regression/3, where GetParam() = "version2_mode1.png"
[  FAILED  ] Objdetect_QRCode_Encode.regression/4, where GetParam() = "version2_mode2.png"
[  FAILED  ] Objdetect_QRCode_Encode.regression/6, where GetParam() = "version3_mode2.png"

@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented Oct 20, 2023

@bagelbytes61 Thanks for the patch. The fix changes version for several test cases, where version is set to auto (0). You need to create PR with the same branch name as this one to OpenCV extra and update reference images. Example:

[ RUN      ] Objdetect_QRCode_Encode.regression/3, where GetParam() = "version2_mode1.png"
[DEBUG:0@0.152] global qrcode_encoder.cpp:370 generateQR QR code detected_version: 4 version_level: 0
src resolution: 29x29
result resolution: 37x37
unknown file: Failure
C++ exception with description "OpenCV(4.8.0-dev) /home/alexander/Projects/OpenCV/opencv-master/modules/core/src/arithm.cpp:652: error: (-209:Sizes of input arguments do not match) The operation is neither 'array op array' (where arrays have the same size and the same number of channels), nor 'array op scalar', nor 'scalar op array' in function 'arithm_op'
" thrown in the test body.
[  FAILED  ] Objdetect_QRCode_Encode.regression/3, where GetParam() = "version2_mode1.png" (76 ms)

@asmorkalov asmorkalov requested a review from dkurt October 20, 2023 13:08
@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented Oct 20, 2023

@dkurt Could you take a look too? Loos like the PR overestimate minimal version of QR code for several test cases. The QR codes in our test data contains full (not truncated) sequence.

@bagelbytes61
Copy link
Copy Markdown
Contributor Author

bagelbytes61 commented Oct 21, 2023

@bagelbytes61 Thanks for the patch. The fix changes version for several test cases, where version is set to auto (0). You need to create PR with the same branch name as this one to OpenCV extra and update reference images. Example:

[ RUN      ] Objdetect_QRCode_Encode.regression/3, where GetParam() = "version2_mode1.png"
[DEBUG:0@0.152] global qrcode_encoder.cpp:370 generateQR QR code detected_version: 4 version_level: 0
src resolution: 29x29
result resolution: 37x37
unknown file: Failure
C++ exception with description "OpenCV(4.8.0-dev) /home/alexander/Projects/OpenCV/opencv-master/modules/core/src/arithm.cpp:652: error: (-209:Sizes of input arguments do not match) The operation is neither 'array op array' (where arrays have the same size and the same number of channels), nor 'array op scalar', nor 'scalar op array' in function 'arithm_op'
" thrown in the test body.
[  FAILED  ] Objdetect_QRCode_Encode.regression/3, where GetParam() = "version2_mode1.png" (76 ms)

Apologize for not getting back to you sooner. I addressed the test case failures except for the ECI case is still failing. Would you be able to provide any insight into the failure? I don't know much about QR code technology, so if there's a technical reason for the failures I am afraid that it may be a bit over my head...

dkurt
dkurt reviewed Oct 23, 2023
View reviewed changes
dkurt
dkurt reviewed Oct 23, 2023
View reviewed changes
dkurt
dkurt reviewed Oct 23, 2023
View reviewed changes
dkurt
dkurt reviewed Oct 23, 2023
View reviewed changes
dkurt
dkurt reviewed Oct 23, 2023
View reviewed changes
dkurt
dkurt reviewed Oct 23, 2023
View reviewed changes
@dkurt dkurt self-assigned this Nov 14, 2023
@dkurt dkurt force-pushed the bugfix/qrcode-version-estimator branch from 12e667a to 64a5ee8 Compare November 14, 2023 07:21
@dkurt dkurt force-pushed the bugfix/qrcode-version-estimator branch from 64a5ee8 to bdfa697 Compare November 15, 2023 08:12
asmorkalov
asmorkalov reviewed Nov 16, 2023
View reviewed changes
asmorkalov
asmorkalov approved these changes Nov 16, 2023
View reviewed changes
Copy link
Copy Markdown
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 asmorkalov merged commit fad0dbb into opencv:4.x Nov 16, 2023
AleksandrPanov
AleksandrPanov reviewed Nov 16, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@AleksandrPanov AleksandrPanov left a comment

Choose a reason for hiding this comment

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

👍

IskXCr pushed a commit to Haosonn/opencv that referenced this pull request Dec 20, 2023
@bagelbytes61 @IskXCr
Merge pull request opencv#24364 from bagelbytes61:bugfix/qrcode-versi…
f626314 
…on-estimator

Bugfix/qrcode version estimator opencv#24364

Fixes opencv#24366

### 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
- [x] There is accuracy test, performance test and test data in opencv_extra repository, if applicable
      Patch to opencv_extra has the same branch name.
- [x] The feature is well documented and sample code can be built with the project CMake
thewoz pushed a commit to thewoz/opencv that referenced this pull request Jan 4, 2024
@bagelbytes61 @thewoz
Merge pull request opencv#24364 from bagelbytes61:bugfix/qrcode-versi…
35686ae 
…on-estimator

Bugfix/qrcode version estimator opencv#24364

Fixes opencv#24366

### 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
- [x] There is accuracy test, performance test and test data in opencv_extra repository, if applicable
      Patch to opencv_extra has the same branch name.
- [x] The feature is well documented and sample code can be built with the project CMake
@asmorkalov asmorkalov mentioned this pull request Jan 19, 2024
Merged
thewoz pushed a commit to thewoz/opencv that referenced this pull request May 29, 2024
@bagelbytes61 @thewoz
Merge pull request opencv#24364 from bagelbytes61:bugfix/qrcode-versi…
18a24d2 
…on-estimator

Bugfix/qrcode version estimator opencv#24364

Fixes opencv#24366

### 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
- [x] There is accuracy test, performance test and test data in opencv_extra repository, if applicable
      Patch to opencv_extra has the same branch name.
- [x] The feature is well documented and sample code can be built with the project CMake
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AleksandrPanov AleksandrPanov AleksandrPanov left review comments

@dkurt dkurt dkurt left review comments

@asmorkalov asmorkalov asmorkalov approved these changes

Assignees

@dkurt dkurt

Labels

bug category: objdetect

Projects

None yet

Milestone

4.9.0

Development

Successfully merging this pull request may close these issues.

QRCodeEncoderImpl::estimateVersion does not properly estimate version for certain input length ranges

4 participants

@bagelbytes61 @dkurt @AleksandrPanov @asmorkalov