GM: create GmResultDigest that encapsulates digest type ("bitmap-64bitMD5") and value (12345)

GM: create GmResultDigest that encapsulates digest type ("bitmap-64bitMD5") and value (12345)

R=scroggo@google.com

Committed: https://code.google.com/p/skia/source/detail?r=9271

[epoger, 2013-05-23 15:29:57]

https://codereview.chromium.org/15883004/diff/1/src/utils/SkBitmapHasher.h
File src/utils/SkBitmapHasher.h (right):

https://codereview.chromium.org/15883004/diff/1/src/utils/SkBitmapHasher.h#ne...
src/utils/SkBitmapHasher.h:15: * Static class that generates a uint64 hash
digest from an SkBitmap.
Patchset 1 deletes the SkHashDigest typedef that we previously used to hide the
fact that SkBitmapHasher generates a particular data type (uint64_t).

By being clearer about the underlying data type, it will be easier to wrap it in
the new GmResultDigest object later in this CL.

[epoger, 2013-05-23 18:19:11]

https://codereview.chromium.org/15883004/diff/10001/gm/gm_expectations.cpp
File gm/gm_expectations.cpp (right):

https://codereview.chromium.org/15883004/diff/10001/gm/gm_expectations.cpp#ne...
gm/gm_expectations.cpp:169: // TODO(epoger): This implementation assumes that
all
This comment describes the remaining work for this CL... we'll need to change
the JSON format a bit, so that each allowedResultDigest can convert between its
JSON and in-memory representations.  That will allow us to support multiple
digest types (bitmap vs bytewise hashes, different hashing algorithms, etc.)

https://codereview.chromium.org/15883004/diff/10001/gm/gm_expectations.h
File gm/gm_expectations.h (right):

https://codereview.chromium.org/15883004/diff/10001/gm/gm_expectations.h#newc...
gm/gm_expectations.h:59: class GmResultDigest {
In patchset 2, we replace the Checksum typedef with a new GmResultDigest class
that specifies both digest type and value.

Currently, there is only one result digest type ("bitmap-64bitMD5"), which takes
a uint64_t value.  This is exactly the same as before, so there is no behavioral
change, and the GM self-tests still pass.

[epoger, 2013-05-23 19:18:31]

Ready for review at patchset 3.

https://codereview.chromium.org/15883004/diff/18001/gm/gm_expectations.cpp
File gm/gm_expectations.cpp (right):

https://codereview.chromium.org/15883004/diff/18001/gm/gm_expectations.cpp#ne...
gm/gm_expectations.cpp:23: const static char
kJsonKey_ExpectedResults_AllowedDigests[] = "allowed-digests";
As the GmResultDigest class learns about additional digest types, we will be
able to write them into the "allowed-digests" section of the JSON expectations
file.

https://codereview.chromium.org/15883004/diff/18001/gm/gm_expectations.h
File gm/gm_expectations.h (right):

https://codereview.chromium.org/15883004/diff/18001/gm/gm_expectations.h#newc...
gm/gm_expectations.h:1: /*
Toplevel comment:

The main reason for making this change is that I will soon be adding a flag to
GM allowing it to write out any mismatching images into a checksum-indexed
directory arrangement.

e.g. instead of writing out the actual 8888 selftest1 PNG to
  mismatchPath/8888/selftest1.png
or
  mismatchPath/selftest1_8888.png
I need to write it out to
  mismatchPath/bitmap-64bitMD5/selftest1.png/1209453360120438698.png

In order to do that, I need access to both digest type ("bitmap-64bitMD5") and
value (1209453360120438698) within GMMain::make_filename() (see
https://code.google.com/p/skia/source/browse/trunk/gm/gmmain.cpp?r=9215#217 )

Up until now, digest *type* has been stuck in the bowels of gm_expectations,
only usable when reading/writing JSON files.  Moving it into the new
GmResultDigest object makes it available for the new use described here.

https://codereview.chromium.org/15883004/diff/18001/gm/gm_expectations.h#newc...
gm/gm_expectations.h:71: GmResultDigest(const Json::Value &jsonTypeValuePair);
As of patchset 3, the digest type (e.g., "bitmap-64bitMD5") is now completely
encapsulated within the GmResultDigest object.  This means that the
GmResultDigest class can independently convert between in-memory and JSON
representations.

https://codereview.chromium.org/15883004/diff/18001/gm/tests/outputs/compared...
File
gm/tests/outputs/compared-against-different-pixels-images/output-expected/json-summary.txt
(right):

https://codereview.chromium.org/15883004/diff/18001/gm/tests/outputs/compared...
gm/tests/outputs/compared-against-different-pixels-images/output-expected/json-summary.txt:14:
[ "bitmap-64bitMD5", 8863920166200910451 ]
Note the newly symmetrical actual and expectation formats.  For each possible
result, a type/value pair.

[scroggo, 2013-05-23 20:42:58]

lgtm

https://codereview.chromium.org/15883004/diff/18001/gm/gm_expectations.cpp
File gm/gm_expectations.cpp (right):

https://codereview.chromium.org/15883004/diff/18001/gm/gm_expectations.cpp#ne...
gm/gm_expectations.cpp:111: return (this->fHashDigest == other.fHashDigest);
If we special cased 0, two failed GmResultDigests would not be equal. Would that
be useful so that we catch the error?

https://codereview.chromium.org/15883004/diff/18001/tools/skimage_main.cpp
File tools/skimage_main.cpp (right):

https://codereview.chromium.org/15883004/diff/18001/tools/skimage_main.cpp#ne...
tools/skimage_main.cpp:10: #include "SkBitmapHasher.h"
This file no longer needs to be included.

[epoger, 2013-05-23 21:06:18]

Thanks for the suggestions.  Please take a look at the changes in patchset 4...

https://codereview.chromium.org/15883004/diff/18001/gm/gm_expectations.cpp
File gm/gm_expectations.cpp (right):

https://codereview.chromium.org/15883004/diff/18001/gm/gm_expectations.cpp#ne...
gm/gm_expectations.cpp:111: return (this->fHashDigest == other.fHashDigest);
On 2013/05/23 20:42:58, scroggo wrote:
> If we special cased 0, two failed GmResultDigests would not be equal. Would
that
> be useful so that we catch the error?

Good idea!  I took this and ran with it, adding an inValid() method.  What do
you think?

https://codereview.chromium.org/15883004/diff/18001/tools/skimage_main.cpp
File tools/skimage_main.cpp (right):

https://codereview.chromium.org/15883004/diff/18001/tools/skimage_main.cpp#ne...
tools/skimage_main.cpp:10: #include "SkBitmapHasher.h"
On 2013/05/23 20:42:58, scroggo wrote:
> This file no longer needs to be included.

Done.

https://codereview.chromium.org/15883004/diff/18001/tools/skimage_main.cpp#ne...
tools/skimage_main.cpp:223: if (jsExpectation.match(resultDigest)) {
Added code to check the new isValid() method, regaining the error checking I had
previously removed from here.

[scroggo, 2013-05-23 21:19:28]

LGTM

[epoger, 2013-05-24 14:33:36]

Committed patchset #4 manually as r9271 (presubmit successful).

[epoger, 2013-05-24 16:07:29]

Good news: I have confirmed that this CL did not affect performance on
constrained platforms.  (At least on the one I examined)

BEFORE, GenerateGMs took 15:26 on
http://108.170.217.252:10117/builders/Test-Android-Nexus7-Tegra3-Arm7-Debug/b...

AFTER, GenerateGMs took 15:29 on
http://108.170.217.252:10117/builders/Test-Android-Nexus7-Tegra3-Arm7-Debug/b...