search

dlbeer / quirc

QR decoder library
Other
865 stars 285 forks source link

finder_scan: Improve capstone detection on small images #106

Closed yamt closed 3 years ago

yamt commented 3 years ago

When using small captured images, I somehow frequently see failures to recognize a capstone due to rounding errors. eg. when pb[] = {2 3 8 3 2}.

This commit tries to improve it by using fixed-point arithmetics. The scaling factor was chosen somehow arbitrary. A moderate scaling should be enough.

kaworu commented 3 years ago

Hi @yamt, while the PR looks good it brings back the regression fixed by https://github.com/dlbeer/quirc/pull/87. Out of curiosity, could you provide one of the image that this PR attempt to fix? I could add it/them to the regression test suite at https://github.com/kAworu/node-quirc.

yamt commented 3 years ago

Hi @yamt, while the PR looks good it brings back the regression fixed by #87.

do you mean this PR caused "stack corruption and bus errors"?

Out of curiosity, could you provide one of the image that this PR attempt to fix?

sorry, i don't have any public example right now.

I could add it/them to the regression test suite at https://github.com/kAworu/node-quirc.

btw, why is it maintained externally?

yamt commented 3 years ago

Hi @yamt, while the PR looks good it brings back the regression fixed by #87.

do you mean this PR caused "stack corruption and bus errors"?

i tried to feed images in #87. it didn't crash for me. can you explain a bit more?

spacetanuki% ./qrtest *.png
quirc test program
Copyright (C) 2010-2012 Daniel Beer <dlbeer@gmail.com>
Library version: 1.0

                                          Time (ms)       Count
  Filename                         Load    ID Total    ID   Dec
-------------------------------------------------------------------------------
  93222834-ab901b00-f76f-11ea-80d5-79b211e5d189.png:     9    18    27     0     0
  93222943-c793bc80-f76f-11ea-9255-0898f9b1f598.png:     6    25    31     0     0
-------------------------------------------------------------------------------
TOTAL: 2 files, 0 codes, 0 decoded (0 failures)
Total time [load: 15, identify: 43, total: 58]
Average time [load: 7, identify: 21, total: 29]
spacetanuki%
dlbeer commented 3 years ago

I'm happy with this -- I can't see why it should cause stack corruption, unless there's already some issue that this exposes. If anyone has an image that causes a crash after this change, please attach it to an issue somewhere or send it to me.

kaworu commented 3 years ago

do you mean this PR caused "stack corruption and bus errors"?

I meant a false positive.

i tried to feed images in #87. it didn't crash for me. can you explain a bit more?

Try with -DQUIRC_MAX_REGIONS=65534 and you'll get a different result:

% ./qrtest ../node-quirc/test/data/black.png
quirc test program
Copyright (C) 2010-2012 Daniel Beer <dlbeer@gmail.com>
Library version: 1.0

                                          Time (ms)       Count
  Filename                         Load    ID Total    ID   Dec
-------------------------------------------------------------------------------
  black.png                     :     9    97   107     1     0

I could add it/them to the regression test suite at https://github.com/kAworu/node-quirc.

btw, why is it maintained externally?

The test suite initially covered the NodeJS interface, and then adding regression tests was friction-less since the testing infrastructure is already there. I'm happy to help porting the tests in quirc proper but I don't have the time to do it myself at the moment.

yamt commented 3 years ago

do you mean this PR caused "stack corruption and bus errors"?

I meant a false positive.

ok. it's entirely possible this change increases false positives.

i tried to feed images in #87. it didn't crash for me. can you explain a bit more?

Try with -DQUIRC_MAX_REGIONS=65534 and you'll get a different result:

% ./qrtest ../node-quirc/test/data/black.png
quirc test program
Copyright (C) 2010-2012 Daniel Beer <dlbeer@gmail.com>
Library version: 1.0

                                          Time (ms)       Count
  Filename                         Load    ID Total    ID   Dec
-------------------------------------------------------------------------------
  black.png                     :     9    97   107     1     0

thank you. i tried it. the false positive here was with version 18. so it's a different false positive from what has been fixed by #87 i guess.

I could add it/them to the regression test suite at https://github.com/kAworu/node-quirc.

btw, why is it maintained externally?

The test suite initially covered the NodeJS interface, and then adding regression tests was friction-less since the testing infrastructure is already there. I'm happy to help porting the tests in quirc proper but I don't have the time to do it myself at the moment.

ok.