Add option to ignore small pixel diffs for --validate. By default, right now we will default to max…

Add option to ignore small pixel diffs for --validate. By default, right now we will default to max diff of 256, which means that for now we report all pixels that are not as expected and we do not error out. Ideally we will decrease the value of max diff to something that does not have visual impact, e.g. 10, then we will report small changes with the intensity under 10, but we will error out for anything larger.
Committed: https://code.google.com/p/skia/source/detail?r=7218

[EricB, 2013-01-16 14:30:35]

LGTM with parameter validation request.

https://codereview.appspot.com/7139043/diff/1002/tools/render_pictures_main.cpp
File tools/render_pictures_main.cpp (right):

https://codereview.appspot.com/7139043/diff/1002/tools/render_pictures_main.c...
tools/render_pictures_main.cpp:573: 
Please add a condition down here to verify that, if --maxComponentDiff is
supplied, then --validate is also supplied.

[edisonn, 2013-01-16 14:46:20]

https://codereview.appspot.com/7139043/diff/1002/tools/render_pictures_main.cpp
File tools/render_pictures_main.cpp (right):

https://codereview.appspot.com/7139043/diff/1002/tools/render_pictures_main.c...
tools/render_pictures_main.cpp:573: 
On 2013/01/16 14:30:35, EricB wrote:
> Please add a condition down here to verify that, if --maxComponentDiff is
> supplied, then --validate is also supplied.

Done.

[reed1, 2013-01-16 14:52:13]

command-line style question

Is this optional extra command+value the clearest, or should we always require a
tolerance for --validate?

e.g.

--validate 0 // this gives the same behavior we have today

https://codereview.appspot.com/7139043/diff/4001/tools/render_pictures_main.cpp
File tools/render_pictures_main.cpp (right):

https://codereview.appspot.com/7139043/diff/4001/tools/render_pictures_main.c...
tools/render_pictures_main.cpp:178: 
For clarity, we need to either
- Change these parameter types to SkPMColor, and use the macros in SkColorPriv.h
to access the components (e.g. SkGetPackedR32)

or

- add a big comment documenting that we are treating SkPMColors as SkColors, but
that its OK, because we treat all components the same

or

- use a generic set of getters that don't claim to be color-type specific. e.g.

getbyte(uint32_t value, int index) {
    return (value >> (index * 8)) & 0xFF;
}

https://codereview.appspot.com/7139043/diff/4001/tools/render_pictures_main.c...
tools/render_pictures_main.cpp:189: int diffs[256];
int diffs[256] = { 0 }; or {};  ?

or

sk_bzero(diffs, sizeof(diffs));

[edisonn, 2013-01-16 15:46:50]

1. I prefer to have an optional parameter for --validate, in this way we can be
clear about what is the expected maximum diff, rather than having to dig into
buildbot configuration files and scripts

new cl uploaded at https://codereview.appspot.com/7137046

https://codereview.appspot.com/7139043/diff/4001/tools/render_pictures_main.cpp
File tools/render_pictures_main.cpp (right):

https://codereview.appspot.com/7139043/diff/4001/tools/render_pictures_main.c...
tools/render_pictures_main.cpp:178: 
what about using SKColor with SkBitmap::getColor(x,y) ?


On 2013/01/16 14:52:13, reed1 wrote:
> For clarity, we need to either
> - Change these parameter types to SkPMColor, and use the macros in
SkColorPriv.h
> to access the components (e.g. SkGetPackedR32)
> 
> or
> 
> - add a big comment documenting that we are treating SkPMColors as SkColors,
but
> that its OK, because we treat all components the same
> 
> or
> 
> - use a generic set of getters that don't claim to be color-type specific.
e.g.
> 
> getbyte(uint32_t value, int index) {
>     return (value >> (index * 8)) & 0xFF;
> }

https://codereview.appspot.com/7139043/diff/4001/tools/render_pictures_main.c...
tools/render_pictures_main.cpp:189: int diffs[256];
On 2013/01/16 14:52:13, reed1 wrote:
> int diffs[256] = { 0 }; or {};  ?
> 
> or
> 
> sk_bzero(diffs, sizeof(diffs));

Done.