Add user-configurable largeImageThreshold by nicole-ashley · Pull Request #14 · lksv/node-resemble.js

nicole-ashley · 2015-11-12T20:08:02Z

Copied directly from upstream

lksv · 2015-11-13T07:40:43Z

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

nicole-ashley · 2015-11-15T21:20:45Z

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 · 2015-11-16T08:23:01Z

Jasmine via gulp is perfect.

nicole-ashley · 2015-11-16T20:49:51Z

Done – let me know if this suits.

lksv · 2015-11-17T10:04:01Z

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 · 2015-11-17T17:17:03Z

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 · 2015-11-17T19:28:14Z

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 · 2015-11-17T20:37:11Z

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:

Honestly, I don't understand the random behaviour. The whole code is strictly deterministic, how can I get different results on multiple run?
What do you see when you write the diff image to file and view it?.
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.

(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 · 2015-11-17T22:37:53Z

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 · 2015-11-18T21:12:59Z

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.

In very rare cases (randomly) I see this:

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 · 2015-11-18T22:21:57Z

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 · 2015-11-18T23:56:56Z

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.

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

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

Add user-configurable largeImageThreshold

7fa328b

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add user-configurable largeImageThreshold#14

Add user-configurable largeImageThreshold#14
nicole-ashley wants to merge 1 commit intolksv:masterfrom
nicole-ashley:user-configurable-largeimagethreshold

nicole-ashley commented Nov 12, 2015

Uh oh!

lksv commented Nov 13, 2015

Uh oh!

nicole-ashley commented Nov 15, 2015

Uh oh!

lksv commented Nov 16, 2015

Uh oh!

nicole-ashley commented Nov 16, 2015

Uh oh!

lksv commented Nov 17, 2015

Uh oh!

nicole-ashley commented Nov 17, 2015

Uh oh!

nicole-ashley commented Nov 17, 2015

Uh oh!

lksv commented Nov 17, 2015

Uh oh!

nicole-ashley commented Nov 17, 2015

Uh oh!

lksv commented Nov 18, 2015

Uh oh!

lksv commented Nov 18, 2015

Uh oh!

nicole-ashley commented Nov 18, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

nicole-ashley commented Nov 12, 2015

Uh oh!

lksv commented Nov 13, 2015

Uh oh!

nicole-ashley commented Nov 15, 2015

Uh oh!

lksv commented Nov 16, 2015

Uh oh!

nicole-ashley commented Nov 16, 2015

Uh oh!

lksv commented Nov 17, 2015

Uh oh!

nicole-ashley commented Nov 17, 2015

Uh oh!

nicole-ashley commented Nov 17, 2015

Uh oh!

lksv commented Nov 17, 2015

Uh oh!

nicole-ashley commented Nov 17, 2015

Uh oh!

lksv commented Nov 18, 2015

Uh oh!

lksv commented Nov 18, 2015

Uh oh!

nicole-ashley commented Nov 18, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants