render_pictures: add --mismatchPath flag

render_pictures: add --mismatchPath flag

When set, it will only write out images that don't match expectations.

BUG=skia:1942
Committed: http://code.google.com/p/skia/source/detail?r=14748

[epoger, 2014-05-14 22:29:29]

Ravi- PTAL at patchset 4.

[rmistry, 2014-05-15 14:26:19]

https://codereview.chromium.org/283123002/diff/80001/tools/PictureRenderer.h
File tools/PictureRenderer.h (right):

https://codereview.chromium.org/283123002/diff/80001/tools/PictureRenderer.h#...
tools/PictureRenderer.h:90: *     which do not match expectations, or NULL if
this renderer should not write mismatches.
I am a little confused about the difference here.
It will write all images (regardless of them matching or not matching
expectations) to writePath. It will only write images that do not match
expectations to mismatchPath.

So if both writePath and mismatchPath are specified, images that do not match
expectations will be written to both directories? To avoid this duplication what
do you think about a boolean variable called writeOnlyMismatches that will only
write images that do not match expectations to writePath. (There is probably a
usecase I am not seeing here).

[epoger, 2014-05-15 14:39:09]

https://codereview.chromium.org/283123002/diff/80001/tools/PictureRenderer.h
File tools/PictureRenderer.h (right):

https://codereview.chromium.org/283123002/diff/80001/tools/PictureRenderer.h#...
tools/PictureRenderer.h:90: *     which do not match expectations, or NULL if
this renderer should not write mismatches.
On 2014/05/15 14:26:19, rmistry wrote:
> I am a little confused about the difference here.
> It will write all images (regardless of them matching or not matching
> expectations) to writePath. It will only write images that do not match
> expectations to mismatchPath.

It sounds to me like you understand the difference perfectly. :-)

> 
> So if both writePath and mismatchPath are specified, images that do not match
> expectations will be written to both directories? To avoid this duplication
what
> do you think about a boolean variable called writeOnlyMismatches that will
only
> write images that do not match expectations to writePath. (There is probably a
> usecase I am not seeing here).

You're right about what the behavior would be if both writePath and mismatchPath
were specified.

I am not aware of a current use case that would require such behavior, but at
the same time I'm not sure what we would gain by PREVENTING such use.

These --writePath / --mismatchPath flags are exactly the same as the ones we
provide in https://skia.googlesource.com/skia/+/master/gm/gmmain.cpp .  It seems
to me that the tools should be consistent, in general...

If you think there is a compelling reason to change both this tool and gm,
though, let me know!

Or, maybe we should continue the behavior as it is written here, and there's
more documentation I could add that would help?

[rmistry, 2014-05-15 14:47:14]

LGTM

https://codereview.chromium.org/283123002/diff/80001/tools/PictureRenderer.h
File tools/PictureRenderer.h (right):

https://codereview.chromium.org/283123002/diff/80001/tools/PictureRenderer.h#...
tools/PictureRenderer.h:90: *     which do not match expectations, or NULL if
this renderer should not write mismatches.
On 2014/05/15 14:39:09, epoger wrote:
> On 2014/05/15 14:26:19, rmistry wrote:
> > I am a little confused about the difference here.
> > It will write all images (regardless of them matching or not matching
> > expectations) to writePath. It will only write images that do not match
> > expectations to mismatchPath.
> 
> It sounds to me like you understand the difference perfectly. :-)
> 
> > 
> > So if both writePath and mismatchPath are specified, images that do not
match
> > expectations will be written to both directories? To avoid this duplication
> what
> > do you think about a boolean variable called writeOnlyMismatches that will
> only
> > write images that do not match expectations to writePath. (There is probably
a
> > usecase I am not seeing here).
> 
> You're right about what the behavior would be if both writePath and
mismatchPath
> were specified.
> 
> I am not aware of a current use case that would require such behavior, but at
> the same time I'm not sure what we would gain by PREVENTING such use.
> 
> These --writePath / --mismatchPath flags are exactly the same as the ones we
> provide in https://skia.googlesource.com/skia/+/master/gm/gmmain.cpp .  It
seems
> to me that the tools should be consistent, in general...

Yes tools should be consistent.

> 
> If you think there is a compelling reason to change both this tool and gm,
> though, let me know!
> 

No compelling reason, it just seemed unnecessary duplication to write the same
images in two different places if a boolean would work instead. I do not think
we should investigate changing this behavior right away but instead to keep it
in mind for the future if there is ever a refactor done of the two tools.

> Or, maybe we should continue the behavior as it is written here, and there's
> more documentation I could add that would help?

Since I understood the difference the documentation is probably sufficient.

[epoger, 2014-05-15 14:48:36]

The CQ bit was checked by epoger@google.com

[commit-bot: I haz the power, 2014-05-15 14:48:52]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/epoger@google.com/283123002/80001

[reed1, 2014-05-15 14:50:24]

On 2014/05/15 14:48:52, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>  https://skia-tree-status.appspot.com/cq/epoger@google.com/283123002/80001

sweet!

[commit-bot: I haz the power, 2014-05-15 14:58:26]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  Build-Ubuntu13.10-GCC4.8-x86_64-Release-Trybot on tryserver.chromium
(http://108.170.220.76:10117/builders/Build-Ubuntu13.10-GCC4.8-x86_64-Release-...)

[epoger, 2014-05-15 15:02:27]

On 2014/05/15 14:58:26, I haz the power (commit-bot) wrote:
> FYI, CQ is re-trying this CL (attempt #1).
> Please consider checking whether the failures are real,
> and report flakes to mailto:chrome-troopers@google.com.
> The failing builders are:
>   Build-Ubuntu13.10-GCC4.8-x86_64-Release-Trybot on tryserver.chromium
>
(http://108.170.220.76:10117/builders/Build-Ubuntu13.10-GCC4.8-x86_64-Release-...)

Nice catch, commit-queue!  It's a real problem.

I'll create a fix and recommit.

[epoger, 2014-05-15 15:02:31]

The CQ bit was unchecked by epoger@google.com

[epoger, 2014-05-15 15:04:39]

The CQ bit was checked by epoger@google.com

[commit-bot: I haz the power, 2014-05-15 15:05:00]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/epoger@google.com/283123002/100001

[commit-bot: I haz the power, 2014-05-15 15:11:02]

Change committed as 14748