gm: remove --diffPath option and all other pixel diff functionality.

gm: remove --diffPath option and all other pixel diff functionality.
If you want pixel diffs, you need to run skdiff instead; from now on,
gm will only tell you WHETHER the images differed (not HOW they differed).
Committed: https://code.google.com/p/skia/source/detail?r=7093

[epoger, 2013-01-06 14:57:06]

https://codereview.appspot.com/7064047/diff/1/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.appspot.com/7064047/diff/1/gm/gmmain.cpp#newcode1
gm/gmmain.cpp:1: /*
The reason for making this change:

We are in the process of changing the production buildbots to compare gm results
against checksums instead of image files.  (See full details at 
https://goto.google.com/ImprovingTheSkiaRebaseliningProcess )

When comparing against checksums, all you can tell is WHETHER the actual image
is pixel-identical to the expected image; you can't determine any of the details
of the difference (such as maximum pixel value difference).

If anyone wants to actually see the differences, they can still run the skdiff
tool... on balance, I think it's best to keep the gm tool simple (just tell you
if there ARE any diffs) and consistent for image vs checksum comparisons.

https://codereview.appspot.com/7064047/diff/1/gm/gmmain.cpp#newcode76
gm/gmmain.cpp:76: const static ErrorBitfield ERROR_DIMENSION_MISMATCH      =
0x04;
Soon I will remove ERROR_DIMENSION_MISMATCH, because when two image checksums
don't match we don't know whether it was the dimensions that mismatched, or the
pixels.

If we care about dimension mismatches, we *could* store image dimensions along
with checksums... but I don't think that buys us much in gm.  I think we're fine
with gm just telling us that the images differed SOMEHOW, and relying on skdiff
to tell us more when we go to investigate.

If anyone feels it would be particularly valuable to keep
ERROR_DIMENSION_MISMATCH, please let me know.

https://codereview.appspot.com/7064047/diff/1/gm/tests/outputs/compared-again...
File gm/tests/outputs/compared-against-different-pixels/output-expected/stdout
(left):

https://codereview.appspot.com/7064047/diff/1/gm/tests/outputs/compared-again...
gm/tests/outputs/compared-against-different-pixels/output-expected/stdout:4:
-----  max pixel mismatch for 8888/dashing2 is 64
This file illustrates one difference in the output: we no longer output "max
pixel mismatch" for each pair of differing images.

[epoger, 2013-01-06 21:52:15]

https://codereview.appspot.com/7064047/diff/1/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.appspot.com/7064047/diff/1/gm/gmmain.cpp#newcode76
gm/gmmain.cpp:76: const static ErrorBitfield ERROR_DIMENSION_MISMATCH      =
0x04;
On 2013/01/06 14:57:06, epoger wrote:
> Soon I will remove ERROR_DIMENSION_MISMATCH

In patchset 2, I have removed ERROR_DIMENSION_MISMATCH and renamed
ERROR_PIXEL_MISMATCH as ERROR_IMAGE_MISMATCH (it covers all cases of the images
not matching, as opposed to I/O problems and whatnot).

[EricB, 2013-01-07 14:15:05]

LGTM with a few questions.  Maybe we should wait a bit to see if there are any
responses to
https://groups.google.com/a/google.com/forum/?fromgroups=#!topic/skia-dev/a8j....

https://codereview.appspot.com/7064047/diff/3001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.appspot.com/7064047/diff/3001/gm/gmmain.cpp#newcode296
gm/gmmain.cpp:296: force_all_opaque(*bp);
Do we need to force opaque here?  I believe that we could avoid the above copies
if we skipped this, and it would be consistent with the way that skdiff now
works (https://codereview.appspot.com/7038048/).

https://codereview.appspot.com/7064047/diff/3001/gm/gmmain.cpp#newcode521
gm/gmmain.cpp:521: renderModeDescriptor);
Just to be clear: it's faster to directly compare the two bitmaps than to obtain
the checksum of each and compare that?

https://codereview.appspot.com/7064047/diff/3001/gm/gmmain.cpp#newcode576
gm/gmmain.cpp:576: renderModeDescriptor);
I'm now wondering whether we even want to keep the readPath parameter and do
reference image comparisons in GM.  It seems that GM's role is transitioning to
only generate images and assert that some configs are identical.  If all of our
comparison is taking place in skdiff, would it be simpler/cleaner to remove that
functionality from GM?  I guess that's probably a question for a future CL.

Note: I haven't polled to determine who, if anyone, is using  this.

[epoger, 2013-01-08 18:26:54]

https://codereview.appspot.com/7064047/diff/3001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.appspot.com/7064047/diff/3001/gm/gmmain.cpp#newcode296
gm/gmmain.cpp:296: force_all_opaque(*bp);
On 2013/01/07 14:15:05, EricB wrote:
> Do we need to force opaque here?  I believe that we could avoid the above
copies
> if we skipped this, and it would be consistent with the way that skdiff now
> works (https://codereview.appspot.com/7038048/).

I agree that it would be better to NOT force opaque here.  But I didn't want to
make that change yet, for two reasons:

1. I wanted this CL to change only certain behavior, and not clutter that with
possible side-effects of pixel comparison behavior

2. My next CL will remove this section of the code anyway, replacing it with
"get checksum" / "compare checksums".

https://codereview.appspot.com/7064047/diff/3001/gm/gmmain.cpp#newcode521
gm/gmmain.cpp:521: renderModeDescriptor);
On 2013/01/07 14:15:05, EricB wrote:
> Just to be clear: it's faster to directly compare the two bitmaps than to
obtain
> the checksum of each and compare that?

My next CL will indeed take the checksum of each and compare the checksums. 
That will definitely be faster than reading in the comparison image from disk
and then comparing the pixels...

https://codereview.appspot.com/7064047/diff/3001/gm/gmmain.cpp#newcode576
gm/gmmain.cpp:576: renderModeDescriptor);
On 2013/01/07 14:15:05, EricB wrote:
> I'm now wondering whether we even want to keep the readPath parameter and do
> reference image comparisons in GM.  It seems that GM's role is transitioning
to
> only generate images and assert that some configs are identical.  If all of
our
> comparison is taking place in skdiff, would it be simpler/cleaner to remove
that
> functionality from GM?  I guess that's probably a question for a future CL.
> 
> Note: I haven't polled to determine who, if anyone, is using  this.

I think we want to keep doing image comparisons (are the images equal or not,
not the pixel-level diffs) within gm.  That will allow us to run many tests on
our Android bots without incurring the overhead of writing out checksums, and
then running another tool to read in those checksums and tell us which ones
don't match.  (And then we can incur the overhead of pixel comparisons only when
there are failures.)

[EricB, 2013-01-08 18:34:54]

https://codereview.appspot.com/7064047/diff/3001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.appspot.com/7064047/diff/3001/gm/gmmain.cpp#newcode296
gm/gmmain.cpp:296: force_all_opaque(*bp);
On 2013/01/08 18:26:54, epoger wrote:
> On 2013/01/07 14:15:05, EricB wrote:
> > Do we need to force opaque here?  I believe that we could avoid the above
> copies
> > if we skipped this, and it would be consistent with the way that skdiff now
> > works (https://codereview.appspot.com/7038048/).
> 
> I agree that it would be better to NOT force opaque here.  But I didn't want
to
> make that change yet, for two reasons:
> 
> 1. I wanted this CL to change only certain behavior, and not clutter that with
> possible side-effects of pixel comparison behavior
> 
> 2. My next CL will remove this section of the code anyway, replacing it with
> "get checksum" / "compare checksums".

SGTM

https://codereview.appspot.com/7064047/diff/3001/gm/gmmain.cpp#newcode521
gm/gmmain.cpp:521: renderModeDescriptor);
On 2013/01/08 18:26:54, epoger wrote:
> On 2013/01/07 14:15:05, EricB wrote:
> > Just to be clear: it's faster to directly compare the two bitmaps than to
> obtain
> > the checksum of each and compare that?
> 
> My next CL will indeed take the checksum of each and compare the checksums. 
> That will definitely be faster than reading in the comparison image from disk
> and then comparing the pixels...

Great!

https://codereview.appspot.com/7064047/diff/3001/gm/gmmain.cpp#newcode576
gm/gmmain.cpp:576: renderModeDescriptor);
On 2013/01/08 18:26:54, epoger wrote:
> On 2013/01/07 14:15:05, EricB wrote:
> > I'm now wondering whether we even want to keep the readPath parameter and do
> > reference image comparisons in GM.  It seems that GM's role is transitioning
> to
> > only generate images and assert that some configs are identical.  If all of
> our
> > comparison is taking place in skdiff, would it be simpler/cleaner to remove
> that
> > functionality from GM?  I guess that's probably a question for a future CL.
> > 
> > Note: I haven't polled to determine who, if anyone, is using  this.
> 
> I think we want to keep doing image comparisons (are the images equal or not,
> not the pixel-level diffs) within gm.  That will allow us to run many tests on
> our Android bots without incurring the overhead of writing out checksums, and
> then running another tool to read in those checksums and tell us which ones
> don't match.  (And then we can incur the overhead of pixel comparisons only
when
> there are failures.)

Right - to avoid writing images which matched to disk.  I forgot about that
case.