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

Improve FinderPatternFinder.selectBestPatterns #1158

Merged
merged 9 commits into from Apr 18, 2019

Conversation

@makiuchi-d
Copy link
Contributor

commented Apr 15, 2019

I rewrote the FinderPatternFinder.selectBestPatterns().
I got a better score on blackbox test 2, and same scores on other blackbox tests.

In addition, this improvement makes some QR-codes readable which reported in #1157 and #1144.

@codecov-io

This comment has been minimized.

Copy link

commented Apr 15, 2019

Codecov Report

Merging #1158 into master will decrease coverage by 0.07%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1158      +/-   ##
============================================
- Coverage     78.01%   77.94%   -0.08%     
+ Complexity     4292     4288       -4     
============================================
  Files           254      254              
  Lines         14080    14076       -4     
  Branches       2891     2890       -1     
============================================
- Hits          10985    10972      -13     
- Misses         2264     2269       +5     
- Partials        831      835       +4
Impacted Files Coverage Δ Complexity Δ
...gle/zxing/qrcode/detector/FinderPatternFinder.java 94.82% <100%> (-0.08%) 143 <2> (+1)
...main/java/com/google/zxing/common/GridSampler.java 34.04% <0%> (-8.52%) 8% <0%> (-2%)
.../zxing/qrcode/detector/AlignmentPatternFinder.java 92.23% <0%> (-2.92%) 39% <0%> (-3%)
.../java/com/google/zxing/qrcode/decoder/Version.java 93.42% <0%> (-2.64%) 27% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0e8c46e...9381ea4. Read the comment docs.

for (int k = j + 1; k < possibleCenters.size(); k++) {
float minModuleSize = possibleCenters.get(i).getEstimatedModuleSize();
float maxModuleSize = possibleCenters.get(k).getEstimatedModuleSize();
if (maxModuleSize > minModuleSize * 1.4f) {

This comment has been minimized.

Copy link
@makiuchi-d

makiuchi-d Apr 15, 2019

Author Contributor

The limit of the module size in original code was average module size +/- 20%.
So I make a new limit +40% of smallest module size.

@srowen
Copy link
Contributor

left a comment

OK, I'm open to it. I'm only worried about performance on large images where a lot of finder patterns might be found, but it's not so bad. Can you explain the logic a little more?

for (int i = 0; i < possibleCenters.size() - 2; i++) {
for (int j = i + 1; j < possibleCenters.size() - 1; j++) {
for (int k = j + 1; k < possibleCenters.size(); k++) {
float minModuleSize = possibleCenters.get(i).getEstimatedModuleSize();

This comment has been minimized.

Copy link
@srowen

srowen Apr 15, 2019

Contributor

Can you lift call to things like possibleCenters.get(i) etc out of the loop for performance?


Collections.sort(possibleCenters, new FurthestFromAverageComparator((float) average));
double distotion = Double.MAX_VALUE;

This comment has been minimized.

Copy link
@srowen

srowen Apr 15, 2019

Contributor

distotion -> distortion

/**
* Get square of distance between a and b.
*/
private static double square(FinderPattern a, FinderPattern b) {

This comment has been minimized.

Copy link
@srowen

srowen Apr 15, 2019

Contributor

Call it squaredDistance?

}
double average = totalModuleSize / startSize;
float stdDev = (float) Math.sqrt(square / startSize - average * average);
Collections.sort(possibleCenters, new EstimatedModuleComparator());

This comment has been minimized.

Copy link
@srowen

srowen Apr 15, 2019

Contributor

This comparator can have a static INSTANCE rather than create it each time

if (Math.abs(pattern.getEstimatedModuleSize() - average) > limit) {
possibleCenters.remove(i);
i--;
squares[0] = square(possibleCenters.get(i), possibleCenters.get(j));

This comment has been minimized.

Copy link
@srowen

srowen Apr 15, 2019

Contributor

It seems like this computation (not assignment) could be lifted out of the inner loop too.


float limit = Math.max(0.2f * (float) average, stdDev);
for (int i = 0; i < possibleCenters.size() - 2; i++) {
float minModuleSize = possibleCenters.get(i).getEstimatedModuleSize();

This comment has been minimized.

Copy link
@srowen

srowen Apr 16, 2019

Contributor

It may not matter much, but was also thinking you can save the reference to possibleCenters.get(...) outside the loop to avoid repeatedly accessing the field and indexing into the collection.

This comment has been minimized.

Copy link
@makiuchi-d

makiuchi-d Apr 17, 2019

Author Contributor

Do you mean to call toArray before the loop?

This comment has been minimized.

Copy link
@srowen

srowen Apr 17, 2019

Contributor

No, I mean things like:

FinderPatternFinder fpi = possibleCenters.get(i);

outside the j/k loops for example.
It's not a big deal but this part is performance sensitive.

This comment has been minimized.

Copy link
@makiuchi-d

makiuchi-d Apr 18, 2019

Author Contributor

I see. I fixed it.

Show resolved Hide resolved .../src/main/java/com/google/zxing/qrcode/detector/FinderPatternFinder.java
@srowen

srowen approved these changes Apr 18, 2019

@srowen srowen merged commit 653ac2a into zxing:master Apr 18, 2019

6 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
LGTM analysis: Java No new or fixed alerts
Details
LGTM analysis: JavaScript No code changes detected
Details
codecov/patch 100% of diff hit (target 78.01%)
Details
codecov/project Absolute coverage decreased by -0.07% but relative coverage increased by +21.98% compared to 0e8c46e
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@makiuchi-d makiuchi-d deleted the makiuchi-d:modify-qrcode-finder-pattern-finder branch Apr 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.