Skip to content

4.x: Two simple fixes to We Chat QR Code module#3698

Merged
opencv-pushbot merged 2 commits intoopencv:4.xfrom
elatkin:4.x_wechat_qrcode_fix_dims_and_size
Mar 20, 2024
Merged

4.x: Two simple fixes to We Chat QR Code module#3698
opencv-pushbot merged 2 commits intoopencv:4.xfrom
elatkin:4.x_wechat_qrcode_fix_dims_and_size

Conversation

@elatkin
Copy link
Copy Markdown

@elatkin elatkin commented Mar 15, 2024 •
edited by opencv-alalek
Loading

Two simple fixes to We Char QR Code module:
- decodemgr.cpp: fix messed: width <--> height
- unicomblock.cpp: fix wrong type at sizeof(...)

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
allow_multiple_commits=1

@ylatkin
Two simple fixes to We Char QR Code module:
d75a1c3 
- decodemgr.cpp: fix messed: width <--> height
- unicomblock.cpp: fix wrong type at sizeof(...)
@elatkin
Copy link
Copy Markdown
Author

elatkin commented Mar 18, 2024 •
edited
Loading

Dear reviewers!

CI failes at opencv_perf_wechat_qrcode test at 2_qrcodes.png image,
like this log (as I can reproduce this failure at my local machine):

However, I think this regression is not caused by this pull-request. 🤔
Algorithm appears too sensitive; this test case may pass or fail by occasion,
as such test image may be too difficult for decoding with this algorithm.

What if we fix/update this test: try another picture with 2 QR codes, easier
for decoding, which my fixed variant of algorithm would be able to process?
Specifically, I propose simpler picture with my pull-request to opencv_extra:

  • PR #1156 fix opencv_perf_wechat_qrcode failure by simplifying test image

@elatkin elatkin mentioned this pull request Mar 18, 2024
Closed
@mshabunin
Copy link
Copy Markdown
Contributor

mshabunin commented Mar 18, 2024

This image is excluded from accuracy test:

opencv_contrib/modules/wechat_qrcode/test/test_qrcode.cpp

Lines 44 to 46 in bf95e79

std::string qrcode_images_multiple[] = {/*"2_qrcodes.png",*/ "3_close_qrcodes.png", /*"3_qrcodes.png",
"4_qrcodes.png", "5_qrcodes.png", "6_qrcodes.png",*/
"7_qrcodes.png"/*, "8_close_qrcodes.png"*/};

I suggest excluding it from perf test too.

@ylatkin
Exclude "2_qrcodes.png" for opencv_perf_wechat_qrcode test,
3157cc1 
as this image appears too difficult, so that test fails on it
@elatkin
Copy link
Copy Markdown
Author

elatkin commented Mar 19, 2024

@mshabunin
Could you please review once again? I have added new commit to exclude 2_qrcodes.png, like you propose.

@elatkin elatkin changed the title 4.x: Two simple fixes to We Char QR Code module 4.x: Two simple fixes to We Chat QR Code module Mar 19, 2024
mshabunin
mshabunin approved these changes Mar 19, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@mshabunin mshabunin left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@opencv-pushbot opencv-pushbot merged commit b42682b into opencv:4.x Mar 20, 2024
@asmorkalov asmorkalov mentioned this pull request Apr 1, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mshabunin mshabunin mshabunin approved these changes

@SinM9 SinM9 Awaiting requested review from SinM9

Assignees

@mshabunin mshabunin

Labels

bug category: wechat_qrcode

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@elatkin @mshabunin @opencv-pushbot @opencv-alalek @ylatkin