add --readJsonSummaryPath to render_pictures

add --readJsonSummaryPath to render_pictures

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

[epoger, 2014-05-09 15:37:13]

OK, we're back to the level of functionality I had in
https://codereview.chromium.org/261313004/ ... but now that the refactoring part
has been split out, it should be a lot clearer to review.

PTAL at patchset 1; if you think it's on the right track, I'll work on the
things I know I need to resolve before committing (as noted below).

https://codereview.chromium.org/273783004/diff/1/tools/image_expectations.cpp
File tools/image_expectations.cpp (right):

https://codereview.chromium.org/273783004/diff/1/tools/image_expectations.cpp...
tools/image_expectations.cpp:110: // EPOGER: before committing, add a unittest
to exercise what happens if
something I will fix before committing the CL... (this and just below, too)

https://codereview.chromium.org/273783004/diff/1/tools/image_expectations.h
File tools/image_expectations.h (right):

https://codereview.chromium.org/273783004/diff/1/tools/image_expectations.h#n...
tools/image_expectations.h:70: // EPOGER: before committing, implement this
properly so that digest is computed lazily
something I will fix before committing the CL...

[borenet, 2014-05-09 15:54:01]

Seems right. A couple of concerns.

https://codereview.chromium.org/273783004/diff/1/tools/PictureRenderer.cpp
File tools/PictureRenderer.cpp (left):

https://codereview.chromium.org/273783004/diff/1/tools/PictureRenderer.cpp#ol...
tools/PictureRenderer.cpp:289: // Make sure we only compute the bitmap hash once
(at most).
Do we no longer care about this?

https://codereview.chromium.org/273783004/diff/1/tools/image_expectations.h
File tools/image_expectations.h (right):

https://codereview.chromium.org/273783004/diff/1/tools/image_expectations.h#n...
tools/image_expectations.h:77: class ImageResultsSummary {
Should we rename this class now that it contains both results AND expectations?

[epoger, 2014-05-09 16:06:24]

Thanks, Eric.  I made a couple of changes in response to your comments... I'll
let you know when the "things to resolve before commit" are resolved.

https://codereview.chromium.org/273783004/diff/1/tools/PictureRenderer.cpp
File tools/PictureRenderer.cpp (left):

https://codereview.chromium.org/273783004/diff/1/tools/PictureRenderer.cpp#ol...
tools/PictureRenderer.cpp:289: // Make sure we only compute the bitmap hash once
(at most).
On 2014/05/09 15:54:01, borenet wrote:
> Do we no longer care about this?

We do, but BitmapAndDigest takes care of it for us.  I added info about this to
the BitmapAndDigest declaration.

https://codereview.chromium.org/273783004/diff/1/tools/image_expectations.h
File tools/image_expectations.h (right):

https://codereview.chromium.org/273783004/diff/1/tools/image_expectations.h#n...
tools/image_expectations.h:77: class ImageResultsSummary {
On 2014/05/09 15:54:01, borenet wrote:
> Should we rename this class now that it contains both results AND
expectations?

Done.

[epoger, 2014-05-09 18:09:05]

Eric- PTAL as of patchset 4.  I have resolved all issues that I know of...

[epoger, 2014-05-12 15:16:23]

On 2014/05/09 18:09:05, epoger wrote:
> Eric- PTAL as of patchset 4.  I have resolved all issues that I know of...

Ping

[borenet, 2014-05-12 15:23:00]

Yikes! Sorry I missed this.  LGTM at patch set 4.

[epoger, 2014-05-12 15:28:48]

The CQ bit was checked by epoger@google.com

[commit-bot: I haz the power, 2014-05-12 15:28:57]

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

[commit-bot: I haz the power, 2014-05-12 15:37:35]

Change committed as 14695

[epoger, 2014-05-20 19:22:55]

Looks like this CL may have made bench_pictures take longer to run (looks like
the benches ran more slowly).  I'll check the contents of the CL to see if that
looks likely, based on the code change.

[epoger, 2014-05-20 19:41:57]

https://codereview.chromium.org/273783004/diff/60001/tools/PictureRenderer.cpp
File tools/PictureRenderer.cpp (right):

https://codereview.chromium.org/273783004/diff/60001/tools/PictureRenderer.cp...
tools/PictureRenderer.cpp:299: const ImageDigest *imageDigestPtr =
bitmapAndDigest.getImageDigestPtr();
I think this probably accounts for the measured performance change.

Before this CL, this code was manually making sure that the hash (a.k.a. "image
digest") was only computed if needed.

In this CL, I added the BitmapAndDigest class that handles the lazy hash
computation... but for some reason I called getImageDigestPtr() here, rather
than waiting until inside the if-check below!  So we are computing the hash
every time now, instead of only when needed.

I have created a CL that should return performance to what it was before this
one: https://codereview.chromium.org/299763002