output max-pixel-err if we're comparing against known PNG files.

output max-pixel-err if we're comparing against known PNG files.

[bsalomon, 2013-01-28 20:14:43]

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

https://codereview.appspot.com/7238043/diff/1/gm/gmmain.cpp#newcode220
gm/gmmain.cpp:220: err = SkMax32(err, SkAbs32((int)SkGetPackedR32(ca) -
(int)SkGetPackedR32(cb)));
We've had images differ only in alpha in the past. skdiff was updated recently
to report alpha differences. Maybe we should track them here (either in err or
separately).

[epoger, 2013-01-30 16:25:23]

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

https://codereview.appspot.com/7238043/diff/1/gm/gmmain.cpp#newcode1
gm/gmmain.cpp:1: /*
High-level comments/questions:

This is intended to restore some of the functionality that was removed in
https://code.google.com/p/skia/source/detail?r=7093 ('gm: remove --diffPath
option and all other pixel diff functionality')

My intention had been to make pixel diffs PURELY the domain of skdiff.  If
anyone ran gm and got errors, if they wanted to know anything about pixel diffs,
they would have to run gm with flags set so that the actual images would be
written to disk, and then the user would have to run skdiff separately.  (Or
they could use a script that batched them together automatically.)

Am I correct in understanding that that would be painful?

In that case, do we need to also restore the --diffPath option to write out the
pixel diffs as a PNG?  Or is that functionality fine to keep in skdiff only?

[EricB, 2013-01-30 16:43:21]

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

https://codereview.appspot.com/7238043/diff/1/gm/gmmain.cpp#newcode1
gm/gmmain.cpp:1: /*
On 2013/01/30 16:25:23, epoger wrote:
> High-level comments/questions:
> 
> This is intended to restore some of the functionality that was removed in
> https://code.google.com/p/skia/source/detail?r=7093 ('gm: remove --diffPath
> option and all other pixel diff functionality')
> 
> My intention had been to make pixel diffs PURELY the domain of skdiff.  If
> anyone ran gm and got errors, if they wanted to know anything about pixel
diffs,
> they would have to run gm with flags set so that the actual images would be
> written to disk, and then the user would have to run skdiff separately.  (Or
> they could use a script that batched them together automatically.)
> 
> Am I correct in understanding that that would be painful?
> 
> In that case, do we need to also restore the --diffPath option to write out
the
> pixel diffs as a PNG?  Or is that functionality fine to keep in skdiff only?

Adding to these questions, would it be sufficient to create a wrapper script
that runs GM and then skdiff to achieve the same functionality as before?

[epoger, 2013-01-30 16:46:49]

Into the weeds...

https://codereview.appspot.com/7238043/diff/1/gm/gm_expectations.h
File gm/gm_expectations.h (right):

https://codereview.appspot.com/7238043/diff/1/gm/gm_expectations.h#newcode126
gm/gm_expectations.h:126: virtual Expectations get(const char *testName,
SkBitmap*) = 0;
I think it would be a lot cleaner to add an SkBitmap field to the Expectations
class, which may or may not be filled in depending on whether the source PNGs
are around.  Do you feel strongly one way or the other?

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

https://codereview.appspot.com/7238043/diff/1/gm/gmmain.cpp#newcode203
gm/gmmain.cpp:203: return -1;
This is OK, but it would be better to report ERROR_DIMENSION_MISMATCH like we
did before https://code.google.com/p/skia/source/detail?r=7093 .

Of course, it might be confusing for gm to know about expected image dimensions
only *sometimes* (if it is comparing against PNGs, as opposed to comparing
against a checksum file like
https://code.google.com/p/skia-autogen/source/browse/gm-actual/base-shuttle-w...
)

Here are some possibilities I can think of... which do you think is best?

1. Only report ERROR_DIMENSION_MISMATCH in the case when actual images are being
compared against local PNG files (not when comparing against checksums).  If
comparing against checksums, a dimension mismatch will be reported as
ERROR_IMAGE_MISMATCH (just like pixel mismatches would be)

2. Never report ERROR_DIMENSION_MISMATCH... even if we know the expected image
dimensions (because we are comparing against PNGs), pretend that we don't, and
report dimension mismatches as the generic ERROR_IMAGE_MISMATCH.

3. Report dimension mismatches even when comparing against checksums (JSON
expectations).  We could add new width/height fields to the JSON format, or we
could embed the dimensions in the same string as the checksum
("640_480_deadbeef" instead of "deadbeef").

https://codereview.appspot.com/7238043/diff/1/gm/gmmain.cpp#newcode207
gm/gmmain.cpp:207: return -2;
Do we not care at all about non-8888 images?

https://codereview.appspot.com/7238043/diff/1/gm/gmmain.cpp#newcode220
gm/gmmain.cpp:220: err = SkMax32(err, SkAbs32((int)SkGetPackedR32(ca) -
(int)SkGetPackedR32(cb)));
On 2013/01/28 20:14:43, bsalomon wrote:
> We've had images differ only in alpha in the past. skdiff was updated recently
> to report alpha differences. Maybe we should track them here (either in err or
> separately).

Right.  See http://code.google.com/p/skia/source/detail?r=7003 ('Extended skdiff
to report alpha channel differences.')

Maybe we should move
http://code.google.com/p/skia/source/browse/trunk/tools/skdiff.cpp 's
compute_diff() function somewhere that it could be shared between gm and skdiff?

[reed1, 2013-01-30 17:08:57]

To answer the macro question about pain.

gm -w foo (followed by skdiff) works, but
1. is several times slower
2. spreads out the key data into a large web-page (e.g. max error per test)

To encourage devs to *always* run gm locally, I think having this capability
directly in gm is preferable. (not advocating to removing anything from skdiff)

[reed1, 2013-01-30 17:13:37]

On 2013/01/30 16:46:49, epoger wrote:
> Into the weeds...
> 
> https://codereview.appspot.com/7238043/diff/1/gm/gm_expectations.h
> File gm/gm_expectations.h (right):
> 
> https://codereview.appspot.com/7238043/diff/1/gm/gm_expectations.h#newcode126
> gm/gm_expectations.h:126: virtual Expectations get(const char *testName,
> SkBitmap*) = 0;
> I think it would be a lot cleaner to add an SkBitmap field to the Expectations
> class, which may or may not be filled in depending on whether the source PNGs
> are around.  Do you feel strongly one way or the other?
> 
> https://codereview.appspot.com/7238043/diff/1/gm/gmmain.cpp
> File gm/gmmain.cpp (right):
> 
> https://codereview.appspot.com/7238043/diff/1/gm/gmmain.cpp#newcode203
> gm/gmmain.cpp:203: return -1;
> This is OK, but it would be better to report ERROR_DIMENSION_MISMATCH like we
> did before https://code.google.com/p/skia/source/detail?r=7093 .

Yes, that symbol is much clearer than a magic -1

> 
> Of course, it might be confusing for gm to know about expected image
dimensions
> only *sometimes* (if it is comparing against PNGs, as opposed to comparing
> against a checksum file like
>
https://code.google.com/p/skia-autogen/source/browse/gm-actual/base-shuttle-w...
> )
> 
> Here are some possibilities I can think of... which do you think is best?
> 
> 1. Only report ERROR_DIMENSION_MISMATCH in the case when actual images are
being
> compared against local PNG files (not when comparing against checksums).  If
> comparing against checksums, a dimension mismatch will be reported as
> ERROR_IMAGE_MISMATCH (just like pixel mismatches would be)
> 
> 2. Never report ERROR_DIMENSION_MISMATCH... even if we know the expected image
> dimensions (because we are comparing against PNGs), pretend that we don't, and
> report dimension mismatches as the generic ERROR_IMAGE_MISMATCH.
> 
> 3. Report dimension mismatches even when comparing against checksums (JSON
> expectations).  We could add new width/height fields to the JSON format, or we
> could embed the dimensions in the same string as the checksum
> ("640_480_deadbeef" instead of "deadbeef").
> 
> https://codereview.appspot.com/7238043/diff/1/gm/gmmain.cpp#newcode207
> gm/gmmain.cpp:207: return -2;
> Do we not care at all about non-8888 images?

Off-hand, no. In practice I have not seen any errors/difference in the 565 code
that didn't also reflect a change on the 8888 side. Certainly could be added
down the road if needed.

> 
> https://codereview.appspot.com/7238043/diff/1/gm/gmmain.cpp#newcode220
> gm/gmmain.cpp:220: err = SkMax32(err, SkAbs32((int)SkGetPackedR32(ca) -
> (int)SkGetPackedR32(cb)));
> On 2013/01/28 20:14:43, bsalomon wrote:
> > We've had images differ only in alpha in the past. skdiff was updated
recently
> > to report alpha differences. Maybe we should track them here (either in err
or
> > separately).
> 
> Right.  See http://code.google.com/p/skia/source/detail?r=7003 ('Extended
skdiff
> to report alpha channel differences.')
> 
> Maybe we should move
> http://code.google.com/p/skia/source/browse/trunk/tools/skdiff.cpp 's
> compute_diff() function somewhere that it could be shared between gm and
skdiff?

Agreed, I will investigate that.

[reed1, 2013-01-30 17:14:52]

If there is a different way to (re)gain this feature (quickly knowing the max
pixel-error per test), I'm fine with that instead of this CL. If not, I will
take this feedback and upload a new rev.

https://codereview.appspot.com/7238043/diff/1/gm/gm_expectations.h
File gm/gm_expectations.h (right):

https://codereview.appspot.com/7238043/diff/1/gm/gm_expectations.h#newcode126
gm/gm_expectations.h:126: virtual Expectations get(const char *testName,
SkBitmap*) = 0;
On 2013/01/30 16:46:50, epoger wrote:
> I think it would be a lot cleaner to add an SkBitmap field to the Expectations
> class, which may or may not be filled in depending on whether the source PNGs
> are around.  Do you feel strongly one way or the other?

No strong preference at the moment. I added it as an optional parameter partly
defensively, to avoid the cost of holding those pixels for callers that didn't
care... tho that may have been premature.

[epoger, 2013-01-30 17:39:59]

On 2013/01/30 17:14:52, reed1 wrote:
> If there is a different way to (re)gain this feature (quickly knowing the max
> pixel-error per test), I'm fine with that instead of this CL. If not, I will
> take this feedback and upload a new rev.

Actually, I'll be happy to whip up my own CL, using yours as a jumping-off
point... but first, I'm hoping to get some replies to my previous questions
about:

1. Whether we should report ERROR_DIMENSION_MISMATCH when comparing against
checksums

2. Should we restore --diffPath while we are at it, or is that significantly
less useful?
> 
> https://codereview.appspot.com/7238043/diff/1/gm/gm_expectations.h
> File gm/gm_expectations.h (right):
> 
> https://codereview.appspot.com/7238043/diff/1/gm/gm_expectations.h#newcode126
> gm/gm_expectations.h:126: virtual Expectations get(const char *testName,
> SkBitmap*) = 0;
> On 2013/01/30 16:46:50, epoger wrote:
> > I think it would be a lot cleaner to add an SkBitmap field to the
Expectations
> > class, which may or may not be filled in depending on whether the source
PNGs
> > are around.  Do you feel strongly one way or the other?
> 
> No strong preference at the moment. I added it as an optional parameter partly
> defensively, to avoid the cost of holding those pixels for callers that didn't
> care... tho that may have been premature.

[reed1, 2013-01-30 17:58:28]

On 2013/01/30 17:39:59, epoger wrote:
> On 2013/01/30 17:14:52, reed1 wrote:
> > If there is a different way to (re)gain this feature (quickly knowing the
max
> > pixel-error per test), I'm fine with that instead of this CL. If not, I will
> > take this feedback and upload a new rev.
> 
> Actually, I'll be happy to whip up my own CL, using yours as a jumping-off
> point... but first, I'm hoping to get some replies to my previous questions
> about:
> 
> 1. Whether we should report ERROR_DIMENSION_MISMATCH when comparing against
> checksums

No opinion. It does seem that changing dimensions *must* impact
pixels/checksums, so I don't see how/why we shouldn't report this.

> 
> 2. Should we restore --diffPath while we are at it, or is that significantly
> less useful?

I never used this option, so I'm not advocating to restore it.

> > 
> > https://codereview.appspot.com/7238043/diff/1/gm/gm_expectations.h
> > File gm/gm_expectations.h (right):
> > 
> >
https://codereview.appspot.com/7238043/diff/1/gm/gm_expectations.h#newcode126
> > gm/gm_expectations.h:126: virtual Expectations get(const char *testName,
> > SkBitmap*) = 0;
> > On 2013/01/30 16:46:50, epoger wrote:
> > > I think it would be a lot cleaner to add an SkBitmap field to the
> Expectations
> > > class, which may or may not be filled in depending on whether the source
> PNGs
> > > are around.  Do you feel strongly one way or the other?
> > 
> > No strong preference at the moment. I added it as an optional parameter
partly
> > defensively, to avoid the cost of holding those pixels for callers that
didn't
> > care... tho that may have been premature.

[epoger, 2013-01-30 18:23:02]

> > 1. Whether we should report ERROR_DIMENSION_MISMATCH when comparing against
> > checksums
> 
> No opinion. It does seem that changing dimensions *must* impact
> pixels/checksums, so I don't see how/why we shouldn't report this.

Yes, changing dimensions does affect the image checksum.  We even test for this
in https://code.google.com/p/skia/source/browse/trunk/tests/ChecksumTest.cpp#166
.

The problem is: when the checksums don't match, you can't tell if that was
because of a pixel mismatch or a dimension mismatch.

My inclination is to just report this as ERROR_IMAGE_MISMATCH when comparing
against checksums, and to provide the additional detail when comparing against a
PNG.  The downside is that it might be confusing for the gm tool to *sometimes*
but *not always* distinguish between pixel and dimension mismatches.

[reed1, 2013-01-30 18:29:50]

On 2013/01/30 18:23:02, epoger wrote:
> > > 1. Whether we should report ERROR_DIMENSION_MISMATCH when comparing
against
> > > checksums
> > 
> > No opinion. It does seem that changing dimensions *must* impact
> > pixels/checksums, so I don't see how/why we shouldn't report this.
> 
> Yes, changing dimensions does affect the image checksum.  We even test for
this
> in
https://code.google.com/p/skia/source/browse/trunk/tests/ChecksumTest.cpp#166
> .
> 
> The problem is: when the checksums don't match, you can't tell if that was
> because of a pixel mismatch or a dimension mismatch.
> 
> My inclination is to just report this as ERROR_IMAGE_MISMATCH when comparing
> against checksums, and to provide the additional detail when comparing against
a
> PNG.  The downside is that it might be confusing for the gm tool to
*sometimes*
> but *not always* distinguish between pixel and dimension mismatches.

Ah, gotcha. I understand now.

It would be nice to also know the dimensions as part of the checksum, but I'm
fine to defer that enhancement for when we actually need it (if ever).

I will freeze this CL pending your version.

[epoger, 2013-02-01 17:47:24]

> I will freeze this CL pending your version.

My version is out for review as https://codereview.appspot.com/7241064/ ('gm:
report max-pixel-error if comparing against PNG files (not checksums)')