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

Bug in QRcode/Detector/ FinderPattern[] selectBestPatterns() #1144

Closed
Quatrus opened this issue Mar 5, 2019 · 5 comments
Closed

Bug in QRcode/Detector/ FinderPattern[] selectBestPatterns() #1144

Quatrus opened this issue Mar 5, 2019 · 5 comments

Comments

@Quatrus
Copy link

Quatrus commented Mar 5, 2019

Bug Reports

624 for (int i = 0; i < possibleCenters.size() && possibleCenters.size() > 3; i++) {
625 FinderPattern pattern = possibleCenters.get(i);
626 if (Math.abs(pattern.getEstimatedModuleSize() - average) > limit) {
627 possibleCenters.remove(i);
628 i--;
629 }
630 }
I am pretty sure that line 626 is wrong. As written it removes the GOOD patterns.
So it should be
if (Math.abs(pattern.getEstimatedModuleSize() - average) < limit) {

@srowen
Copy link
Contributor

srowen commented Mar 5, 2019

No, it's checking whether the module size in the candidate is too different from the average. The check is correct. You can verify by trying your change; lots of tests fail.

@srowen srowen closed this as completed Mar 5, 2019
@Quatrus
Copy link
Author

Quatrus commented Mar 5, 2019

Yes, sorry, I was too fast with the conclusions. The point is that I have a big set of test images and on some of them this test is wrong. I work with the C++ version and modified the code the following way

    // But we can only afford to do so if we have at least 4 possibilities to choose from
    int   total = 0;      //<-----new
    float totalModuleSize = 0.0f;
    float average,stdDev,square = 0.0f;
    for (std::size_t i = 0; i < startSize; i++) {
      float size = possibleCenters_[i]->getEstimatedModuleSize();
      if (size != 1.0f)
      {
        totalModuleSize += size;
        square += size * size;
        total++;   //<-----new
      }
    }
    if(total)     //<-----new
    {
       average = totalModuleSize / total;   //<-----new
       stdDev = (float)std::sqrt(square / total - average * average); //<-----new
    }
    else
    {
       average = 1; //<-----new
       stdDev  = 0; //<-----new
    }

    sort(possibleCenters_.begin(), possibleCenters_.end(), FurthestFromAverageComparator(average));
    
    float limit = max(0.2f * average, stdDev);

    for (std::size_t i = 0; i < possibleCenters_.size() && possibleCenters_.size() > 3; i++) {
      if (abs(possibleCenters_[i]->getEstimatedModuleSize() - average) > limit) {
        possibleCenters_.erase(possibleCenters_.begin()+i);
        i--;
      }
    }
  }

Now it works MUCH better (for me ;) )

What I did was to exclude the possible centers with suspicious module size of 1 from the average and from the limit

@Quatrus
Copy link
Author

Quatrus commented Mar 5, 2019

image
I am trying to upload a demo image.
In my setup this image failed to be decoded using the Hybrid Binarizer and then was decoded using the GlobalHistogram

@srowen
Copy link
Contributor

srowen commented Mar 5, 2019

You can open a pull request with a similar change here. The question is whether it makes more images fail than succeed.

@Quatrus
Copy link
Author

Quatrus commented Mar 5, 2019

I use about 1200 images in my regression test and for the time being I do not see images to fail becuase of the change. Unfortunately there are images where selectBestPattern still fails.
For example, on the image below using after the HybridBinarizer there are 72 candidates. Sorting them using FurthestFromAverageComparator set the good ones on the top but further filters discard them.
Then the image is successfully decoded using the GlobalHistogramBinarizer but I would like to improve the selection somehow instead of using the backup solution

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants