Adding commandline option to gm to make it write image results for tests that fails due to pixel mi…

Adding commandline option to gm to make it write image results for tests that fails due to pixel mismatches.
Committed: https://code.google.com/p/skia/source/detail?r=7143

[junov, 2013-01-11 15:37:13]

The bots don't need this, but we need this so that gm can continue to be used
for debugging.

[EricB, 2013-01-11 15:40:01]

cc epoger, whose recent work might conflict with this.

[epoger, 2013-01-11 15:59:33]

I'm in favor of the idea... in fact, we *will* need it for the bots soon, so I
was going to add this same feature myself.

I am definitely mucking about in gm lately, so thanks Eric for forwarding it to
me.

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

https://codereview.appspot.com/7100043/diff/1/gm/gmmain.cpp#newcode1
gm/gmmain.cpp:1: /*
Have you run the gm self-test at gm/tests/run.sh?  It needs to be run on
Linux... if you could add a self-test for this new feature, I think that would
be helpful.

https://codereview.appspot.com/7100043/diff/1/gm/gmmain.cpp#newcode515
gm/gmmain.cpp:515: const char readPath [], const char writePath [], const
SkString& name, SkBitmap &bitmap,
Instead of adding writePath parameters to several methods, how about adding a
fWritePath field to the GMMain object instead?  (alongside fWriteMismatches)

https://codereview.appspot.com/7100043/diff/1/gm/gmmain.cpp#newcode532
gm/gmmain.cpp:532: if (retval == ERROR_IMAGE_MISMATCH && fWriteMismatches) {
I would prefer:

if ((ERROR_IMAGE_MISMATCH == retval) && fWriteMismatches)

to be more robust in the face of typos.  Or perhaps...

if (fWriteMismatches && (retval & ERROR_IMAGE_MISMATCH))

OR, even better...

if (writePath && fWriteMismatches && (retval & ERROR_IMAGE_MISMATCH))

(like line 611 below)

[junov, 2013-01-11 16:33:01]

Patch.

[epoger, 2013-01-11 16:47:20]

Looks pretty good...

Have you tried the self-test?  You should definitely RUN it to make sure
--writePath still works as expected, and if you're up for it please add a test
for --writeMismatches.

https://codereview.appspot.com/7100043/diff/4001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.appspot.com/7100043/diff/4001/gm/gmmain.cpp#newcode879
gm/gmmain.cpp:879: "    [--writeMismatches]: write image results for tests that
fail due to a\n"
maybe something like...

[--writeMismatches]: modulate --writePath behavior: only write image results for
tests that fail due to an image mismatch.

[bungeman, 2013-01-11 16:57:08]

On 2013/01/11 15:37:13, junov wrote:
> The bots don't need this, but we need this so that gm can continue to be used
> for debugging.

I wrote up something like this yesterday as well, see
https://codereview.appspot.com/7104043/ . I think the only real major difference
is that it helped to also have a switch for writing out comparison .skp files
from the comparison pictures.

[junov, 2013-01-11 17:53:48]

I'm fine with bungeman's patch instead of mine if we think it is better to
output images for all tests instead of just the failing ones.

[junov, 2013-01-11 17:56:10]

On 2013/01/11 17:53:48, junov wrote:
> I'm fine with bungeman's patch instead of mine if we think it is better to
> output images for all tests instead of just the failing ones.

I also think bungeman's approach of having a separate path for the comparison
images instead of using the write path is a better idea.

[epoger, 2013-01-11 18:00:41]

On 2013/01/11 17:56:10, junov wrote:
> On 2013/01/11 17:53:48, junov wrote:
> > I'm fine with bungeman's patch instead of mine if we think it is better to
> > output images for all tests instead of just the failing ones.
> 
> I also think bungeman's approach of having a separate path for the comparison
> images instead of using the write path is a better idea.

I think the two CLs are *complementary*, and as I noted in Ben's
https://codereview.appspot.com/7104043/ , my opinion is that we should land
*this* one and then fold in Ben's after https://codereview.appspot.com/7088044/
('gm: use
new Expectations struct instead of comparison bitmaps') lands (or dies trying).

1. I know we need to add the ability to write out ONLY failing tests' images in
certain circumstances.

2. Ben says it's handy to write out images for other rendering modes, and skps,
in addition to the images we write out now.

If you want to change this CL to write the failing images into a different
directory than specified by --writePath, I'm OK with it either way... as far as
the comparison images go, I think that's a separate issue.

[junov1, 2013-01-11 18:58:53]

Adding commandline option to gm to make it write image results for tests that
fails due to pixel mismatches.

[junov1, 2013-01-11 19:01:37]

Patch.

-passes gm/tests/run.sh
-refactored to use a --mismatchPath commandline option
-Does not affect behavior od --writePath

[junov1, 2013-01-11 19:02:09]

On 2013/01/11 19:01:37, junov1 wrote:
> -Does not affect behavior od --writePath
typo "od" -> "of"

[epoger, 2013-01-11 20:31:03]

LGTM