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.

Sign up for GitHub

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

Jump to bottom

Add user-configurable largeImageThreshold #14

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add user-configurable largeImageThreshold #14

wants to merge 1 commit into from

Conversation

nicole-ashley
Copy link

Copied directly from upstream

@lksv
Copy link
Owner

lksv commented Nov 13, 2015

Thanks you for pull request. Could you add test for this too?

@nicole-ashley
Copy link
Author

Yep, I definitely can. There aren't any existing tests to base it from though, so which framework would you like me to set up? I'm most familiar with jasmine tests via gulp.

@lksv
Copy link
Owner

lksv commented Nov 16, 2015

Jasmine via gulp is perfect.

@nicole-ashley nicole-ashley force-pushed the user-configurable-largeimagethreshold branch from a28df81 to 36e65af Compare November 16, 2015 20:47
@nicole-ashley
Copy link
Author

Done – let me know if this suits.

@nicole-ashley nicole-ashley force-pushed the user-configurable-largeimagethreshold branch from 36e65af to 39c31ed Compare November 16, 2015 20:53
@lksv
Copy link
Owner

lksv commented Nov 17, 2015

really great work.

Unfortunatelly one test is falling (see bellow). The strength thing is that if I put writing diff image to file system before expectPixelsToBeSkipped then test pass.

var fs = require('fs');                                             
data.getDiffImage().pack().pipe(fs.createWriteStream('test.image.diff.png'));
expectPixelsToBeSkipped(data.getDiffImage(), OPTIMISATION_SKIP_STEP);

Second thing is that the diff image doen't look correctly to me. Am I wrong?

....F.........

Failures: 
1) node-resemble.js largeImageThreshold when explicitly set when ignoreAntialiasing is enabled skips pixels on images with a dimension larger than the given threshold
1.1) Expected 255 to be 0.
    Error: Expected 255 to be 0.
        at stack (/home/lukas/projects/node-resemble.js/node_modules/gulp-jasmine/node_modules/jasmine/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:1482:17)
        at buildExpectationResult (/home/lukas/projects/node-resemble.js/node_modules/gulp-jasmine/node_modules/jasmine/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:1452:14)
        at Spec.Env.expectationResultFactory (/home/lukas/projects/node-resemble.js/node_modules/gulp-jasmine/node_modules/jasmine/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:583:18)
        at Spec.addExpectationResult (/home/lukas/projects/node-resemble.js/node_modules/gulp-jasmine/node_modules/jasmine/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:324:34)
        at Expectation.addExpectationResult (/home/lukas/projects/node-resemble.js/node_modules/gulp-jasmine/node_modules/jasmine/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:533:21)
        at Expectation.toBe (/home/lukas/projects/node-resemble.js/node_modules/gulp-jasmine/node_modules/jasmine/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:1406:12)
        at expectPixelsToBeSkipped (/home/lukas/projects/node-resemble.js/resemble.spec.js:154:61)
        at Array.0 (/home/lukas/projects/node-resemble.js/resemble.spec.js:57:13)
        at triggerDataUpdate (/home/lukas/projects/node-resemble.js/resemble.js:69:28)
        at onceWeHaveBoth (/home/lukas/projects/node-resemble.js/resemble.js:447:6)
1.2) Expected 255 to be 0.
    Error: Expected 255 to be 0.
        at stack (/home/lukas/projects/node-resemble.js/node_modules/gulp-jasmine/node_modules/jasmine/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:1482:17)
        at buildExpectationResult (/home/lukas/projects/node-resemble.js/node_modules/gulp-jasmine/node_modules/jasmine/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:1452:14)
        at Spec.Env.expectationResultFactory (/home/lukas/projects/node-resemble.js/node_modules/gulp-jasmine/node_modules/jasmine/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:583:18)
        at Spec.addExpectationResult (/home/lukas/projects/node-resemble.js/node_modules/gulp-jasmine/node_modules/jasmine/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:324:34)
        at Expectation.addExpectationResult (/home/lukas/projects/node-resemble.js/node_modules/gulp-jasmine/node_modules/jasmine/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:533:21)
        at Expectation.toBe (/home/lukas/projects/node-resemble.js/node_modules/gulp-jasmine/node_modules/jasmine/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:1406:12)
        at expectPixelsToBeSkipped (/home/lukas/projects/node-resemble.js/resemble.spec.js:158:61)
        at Array.0 (/home/lukas/projects/node-resemble.js/resemble.spec.js:57:13)
        at triggerDataUpdate (/home/lukas/projects/node-resemble.js/resemble.js:69:28)
        at onceWeHaveBoth (/home/lukas/projects/node-resemble.js/resemble.js:447:6)
1.3) Expected 255 to be 0.
    Error: Expected 255 to be 0.
        at stack (/home/lukas/projects/node-resemble.js/node_modules/gulp-jasmine/node_modules/jasmine/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:1482:17)
        at buildExpectationResult (/home/lukas/projects/node-resemble.js/node_modules/gulp-jasmine/node_modules/jasmine/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:1452:14)
        at Spec.Env.expectationResultFactory (/home/lukas/projects/node-resemble.js/node_modules/gulp-jasmine/node_modules/jasmine/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:583:18)
        at Spec.addExpectationResult (/home/lukas/projects/node-resemble.js/node_modules/gulp-jasmine/node_modules/jasmine/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:324:34)
        at Expectation.addExpectationResult (/home/lukas/projects/node-resemble.js/node_modules/gulp-jasmine/node_modules/jasmine/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:533:21)
        at Expectation.toBe (/home/lukas/projects/node-resemble.js/node_modules/gulp-jasmine/node_modules/jasmine/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:1406:12)
        at expectPixelsToBeSkipped (/home/lukas/projects/node-resemble.js/resemble.spec.js:161:60)
        at Array.0 (/home/lukas/projects/node-resemble.js/resemble.spec.js:57:13)
        at triggerDataUpdate (/home/lukas/projects/node-resemble.js/resemble.js:69:28)
        at onceWeHaveBoth (/home/lukas/projects/node-resemble.js/resemble.js:447:6)

14 specs, 1 failure
Finished in 3 seconds

@nicole-ashley
Copy link
Author

Hmm, odd, they were passing for me before I pushed them up. I'll take another look and see if I can get it to fail.

@nicole-ashley nicole-ashley force-pushed the user-configurable-largeimagethreshold branch from 39c31ed to 7fa328b Compare November 17, 2015 19:19
@nicole-ashley
Copy link
Author

Ok, can you check to see if this one is better?

I've noticed that with 500x500 images, some semi-transparent multi-coloured pixels appear in skipped pixels in the first few rows (usually near the top-left). This was causing false negatives in the tests, hence why I originally skipped the first set. Sounds like a bug that needs fixing, but as far as I can tell it's unrelated to this PR.

It doesn't seem to happen for larger images, so I've included a 1000x1000 image for testing the manual thresholds, and also blanked out the 1200x1200 image to save space (I didn't realise the original was 2.5MB; the new one is 8.6kB). This also means I can get rid of the workaround which I wasn't that happy with anyway.

Hopefully this issue is what is also causing it to fail on your end; let me know how it goes. If it keeps failing, could we set up something on Codeship/Travis/similar to ensure we are testing in a consistent environment? I'm on Windows and yours appears to be on Linux; I can spin up a Linux docker instance but it'd be good to know we have an authoritative test runner somewhere.

@lksv
Copy link
Owner

lksv commented Nov 17, 2015

It's really strength.

It randomly fails (in about 9 case of 10). I guess it might bug unrelated to your pull request. So one solution should be comment out the particular test - if we do not find the right reason.

My thoughts:

  1. Honestly, I don't understand the random behaviour. The whole code is strictly deterministic, how can I get different results on multiple run?
  2. What do you see when you write the diff image to file and view it?.
  3. I still see some noise in first line (on the left) in diff image. Randomly the result is totally wrong. I set the original picture because it's easy to see the diff. Here is my results, the first is "correct" case, second the wrong one.
    diffr
    diffr-wrong
    (you have to download the images to see the exact pixels). So I prefer to get back the "hack" to ignore the first line.

I plan to set up Travis after merge this pull request.

fail and pass execution:

lukas@lenprac:~/projects/node-resemble.js$ gulp test
[21:19:25] Using gulpfile ~/projects/node-resemble.js/gulpfile.js
[21:19:25] Starting 'test'...
....F.........

Failures: 
1) node-resemble.js largeImageThreshold when explicitly set when ignoreAntialiasing is enabled skips pixels on images with a dimension larger than the given threshold
1.1) Expected 255 to be 0.
    Error: Expected 255 to be 0.
        at stack (/home/lukas/projects/node-resemble.js/node_modules/gulp-jasmine/node_modules/jasmine/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:1482:17)
        at buildExpectationResult (/home/lukas/projects/node-resemble.js/node_modules/gulp-jasmine/node_modules/jasmine/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:1452:14)
        at Spec.Env.expectationResultFactory (/home/lukas/projects/node-resemble.js/node_modules/gulp-jasmine/node_modules/jasmine/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:583:18)
        at Spec.addExpectationResult (/home/lukas/projects/node-resemble.js/node_modules/gulp-jasmine/node_modules/jasmine/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:324:34)
        at Expectation.addExpectationResult (/home/lukas/projects/node-resemble.js/node_modules/gulp-jasmine/node_modules/jasmine/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:533:21)
        at Expectation.toBe (/home/lukas/projects/node-resemble.js/node_modules/gulp-jasmine/node_modules/jasmine/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:1406:12)
        at expectPixelsToBeSkipped (/home/lukas/projects/node-resemble.js/resemble.spec.js:148:57)
        at Array.0 (/home/lukas/projects/node-resemble.js/resemble.spec.js:55:13)
        at triggerDataUpdate (/home/lukas/projects/node-resemble.js/resemble.js:69:28)
        at onceWeHaveBoth (/home/lukas/projects/node-resemble.js/resemble.js:447:6)
1.2) Expected 255 to be 0.
    Error: Expected 255 to be 0.
        at stack (/home/lukas/projects/node-resemble.js/node_modules/gulp-jasmine/node_modules/jasmine/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:1482:17)
        at buildExpectationResult (/home/lukas/projects/node-resemble.js/node_modules/gulp-jasmine/node_modules/jasmine/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:1452:14)
        at Spec.Env.expectationResultFactory (/home/lukas/projects/node-resemble.js/node_modules/gulp-jasmine/node_modules/jasmine/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:583:18)
        at Spec.addExpectationResult (/home/lukas/projects/node-resemble.js/node_modules/gulp-jasmine/node_modules/jasmine/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:324:34)
        at Expectation.addExpectationResult (/home/lukas/projects/node-resemble.js/node_modules/gulp-jasmine/node_modules/jasmine/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:533:21)
        at Expectation.toBe (/home/lukas/projects/node-resemble.js/node_modules/gulp-jasmine/node_modules/jasmine/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:1406:12)
        at expectPixelsToBeSkipped (/home/lukas/projects/node-resemble.js/resemble.spec.js:152:57)
        at Array.0 (/home/lukas/projects/node-resemble.js/resemble.spec.js:55:13)
        at triggerDataUpdate (/home/lukas/projects/node-resemble.js/resemble.js:69:28)
        at onceWeHaveBoth (/home/lukas/projects/node-resemble.js/resemble.js:447:6)
1.3) Expected 255 to be 0.
    Error: Expected 255 to be 0.
        at stack (/home/lukas/projects/node-resemble.js/node_modules/gulp-jasmine/node_modules/jasmine/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:1482:17)
        at buildExpectationResult (/home/lukas/projects/node-resemble.js/node_modules/gulp-jasmine/node_modules/jasmine/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:1452:14)
        at Spec.Env.expectationResultFactory (/home/lukas/projects/node-resemble.js/node_modules/gulp-jasmine/node_modules/jasmine/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:583:18)
        at Spec.addExpectationResult (/home/lukas/projects/node-resemble.js/node_modules/gulp-jasmine/node_modules/jasmine/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:324:34)
        at Expectation.addExpectationResult (/home/lukas/projects/node-resemble.js/node_modules/gulp-jasmine/node_modules/jasmine/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:533:21)
        at Expectation.toBe (/home/lukas/projects/node-resemble.js/node_modules/gulp-jasmine/node_modules/jasmine/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:1406:12)
        at expectPixelsToBeSkipped (/home/lukas/projects/node-resemble.js/resemble.spec.js:155:60)
        at Array.0 (/home/lukas/projects/node-resemble.js/resemble.spec.js:55:13)
        at triggerDataUpdate (/home/lukas/projects/node-resemble.js/resemble.js:69:28)
        at onceWeHaveBoth (/home/lukas/projects/node-resemble.js/resemble.js:447:6)

14 specs, 1 failure
Finished in 4.1 seconds

events.js:72
        throw er; // Unhandled 'error' event
              ^
Error: Tests failed
lukas@lenprac:~/projects/node-resemble.js$ 
lukas@lenprac:~/projects/node-resemble.js$ gulp test
[21:19:50] Using gulpfile ~/projects/node-resemble.js/gulpfile.js
[21:19:50] Starting 'test'...
..............

14 specs, 0 failures
Finished in 4.1 seconds
[21:19:54] Finished 'test' after 4.13 s

@nicole-ashley
Copy link
Author

Yes, I was getting the same result with the 500x500 (People) image you posted above. However it disappeared completely (for me at least) when using my new 1000x1000 (SmallImage) image which I force-pushed to this branch – do you get the same result using the latest update?

Alternatively I can do the tests from the bottom-left of the image instead of the top-left. It's still a workaround, but I feel better about it being anchored somewhere rather than skipping an arbitrary number of pixels.

@lksv
Copy link
Owner

lksv commented Nov 18, 2015

EDIT: this comment contains two images which are mostly blank

For comparison of SmallImage.jpg I see almost all image blank expect the top left part.
diffr-wrong

In very rare cases (randomly) I see this:
diffr-wrong

I guess that non-deterministic behaviour is caused by lack of waiting to some of asynchronous call - but I didn't find it yet.

Test from bottom-left should be good "workaround".

@lksv
Copy link
Owner

lksv commented Nov 18, 2015

Maybe I found half of the problem: it's old library pngjs2, when I switched to pngjs: ~2.1.0 tests pass with SmallImage.png. Unfortunatelly it still fails with People.png.

Tomorrow I will continue the investigation.

@nicole-ashley
Copy link
Author

Interesting!

Just to clarify, a blank image is expected when comparing SmallImage.png to itself. It's a blank image to start with, and I figured this was fine because we're not testing the actual comparison here but simply the presence of transparent (skipped) pixels. However apart from the random glitched pixels, any image should work.

I'll do an update shortly using bottom-left instead of top-left, and if that works for you I'll leave it up to you whether you want to merge as-is and create a separate ticket for the pixel glitch, or I'd you want to continue investigating before merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nicole-ashley @lksv