-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
[WeChat QR] Fixing issue that Hybrid Binarizer not working due to integral calculation. #3199
[WeChat QR] Fixing issue that Hybrid Binarizer not working due to integral calculation. #3199
Conversation
79516e4
to
97b7b35
Compare
Waiting for Maintainers to trigger CI manually. |
/cc @dddzg |
There are some test cases failed in Win64,Mac and Win32. |
97b7b35
to
d599619
Compare
Got it. Already fix those test cases in new commit. |
@@ -35,6 +35,8 @@ class HybridBinarizer : public GlobalHistogramBinarizer { | |||
|
|||
int subWidth_; | |||
int subHeight_; | |||
int blockIntegralWidth; | |||
int blockIntegralHeight; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see code changes related to reading of blockIntegral_
with new layout. I see changes in "generator" only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It took me a while to understand the logic in here due to unclear meaning of left
, top
, offset1
, offset2
opencv_contrib/modules/wechat_qrcode/src/zxing/common/binarizer/hybrid_binarizer.cpp
Lines 198 to 210 in 7a35d3d
int left = cap(x, THRES_BLOCKSIZE, subWidth - THRES_BLOCKSIZE - 1); | |
int top = cap(y, THRES_BLOCKSIZE, subHeight - THRES_BLOCKSIZE - 1); | |
int sum = 0; | |
// int sum2 = 0; | |
int offset1 = (top - THRES_BLOCKSIZE) * (subWidth + 1) + left - THRES_BLOCKSIZE; | |
int offset2 = (top + THRES_BLOCKSIZE + 1) * (subWidth + 1) + left - THRES_BLOCKSIZE; | |
int blocksize = THRES_BLOCKSIZE * 2 + 1; | |
sum = blockIntegral[offset1] - blockIntegral[offset1 + blocksize] - | |
blockIntegral[offset2] + blockIntegral[offset2 + blocksize]; |
As my understanding, the left
, top
correspond to centerX, centerY of 5 x 5 block square. And offset1
, offset2
are A, C in
A - - B
| |
C - - D
The blockIntegral
is already used with new layout so we don't need to modify anything in there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For more clear, we could change subWidth + 1
to blockIntegralWidth
in here
opencv_contrib/modules/wechat_qrcode/src/zxing/common/binarizer/hybrid_binarizer.cpp
Lines 204 to 205 in 7a35d3d
int offset1 = (top - THRES_BLOCKSIZE) * (subWidth + 1) + left - THRES_BLOCKSIZE; | |
int offset2 = (top + THRES_BLOCKSIZE + 1) * (subWidth + 1) + left - THRES_BLOCKSIZE; |
d599619
to
a2550a5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Thank you for contribution!
This should fixes #3156 #3008
The size of block array
blocks_
issubWidth * subHeight
but the block integral size iswidth * height
. I believe something wrong when calculating theblockIntegral
with that size.opencv_contrib/modules/wechat_qrcode/src/zxing/common/binarizer/hybrid_binarizer.cpp
Line 41 in 508c8db
opencv_contrib/modules/wechat_qrcode/src/zxing/common/binarizer/hybrid_binarizer.cpp
Line 58 in 508c8db
I suggest change it size to
(subWidth + 1) * (subHeight + 1)
(These extra row and column is just for padding 0 arroud). Also update the precalculation of theblockIntegral
, calculate sum base on thatblockIntegral
Pull Request Readiness Checklist
See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request
Patch to opencv_extra has the same branch name.
force_builders=linux,docs