add --writeChecksumBasedFilenames flag to render_pictures

add --writeChecksumBasedFilenames flag to render_pictures

BUG=skia:1455, skia:2230
Committed: http://code.google.com/p/skia/source/detail?r=13859

[epoger, 2014-03-18 04:44:59]

Ready for review at patchset 1.

https://codereview.chromium.org/202983003/diff/1/tools/PictureBenchmark.cpp
File tools/PictureBenchmark.cpp (right):

https://codereview.chromium.org/202983003/diff/1/tools/PictureBenchmark.cpp#n...
tools/PictureBenchmark.cpp:1: /*
Rob- I'm not sure who the best person to review this CL is.  Looking at
https://github.com/google/skia/commits/master/tools/PictureRenderer.h , I see
that you've made some modifications in here lately.  So are you as good a
candidate as any?

https://codereview.chromium.org/202983003/diff/1/tools/PictureBenchmark.cpp#n...
tools/PictureBenchmark.cpp:3: *
Reasons for the CL, in a nutshell:
1. Needed to efficiently record the buildbots' SkPicture image results over
time, like we do with GM results.
2. Needed to improve the way we record SkPicture results on Cluster Telemetry
tries.

For more background, see the related bugs.

[epoger, 2014-03-18 05:22:49]

I see some compile errors on some of the trybots that I don't get locally.  But
they're narrow in scope, so we should be able to go ahead and do a round of
reviews in spite of them.

I will correct those errors tomorrow.

[epoger, 2014-03-18 20:15:50]

On 2014/03/18 05:22:49, epoger wrote:
> I see some compile errors on some of the trybots that I don't get locally. 
But
> they're narrow in scope, so we should be able to go ahead and do a round of
> reviews in spite of them.
> 
> I will correct those errors tomorrow.

The errors have been fixed in patchset 3, and that seems to be doing much better
on the trybots.

Ready for review at patchset 3... please take a look (or nominate a different
reviewer if appropriate)

[robertphillips, 2014-03-19 14:42:36]

lgtm modulo questions

https://codereview.chromium.org/202983003/diff/60001/tools/CopyTilesRenderer.cpp
File tools/CopyTilesRenderer.cpp (right):

https://codereview.chromium.org/202983003/diff/60001/tools/CopyTilesRenderer....
tools/CopyTilesRenderer.cpp:24: const SkString& inputFilename, bool
useChecksumBasedFilenames) {
I think this should call INHERITED::init

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

https://codereview.chromium.org/202983003/diff/60001/tools/PictureRenderer.cp...
tools/PictureRenderer.cpp:70:
SkAssertResult(SkBitmapHasher::ComputeDigest(bitmap, &hash));
this->

[robertphillips1, 2014-03-19 14:44:15]

lgtm -- modulo questions

https://codereview.chromium.org/202983003/diff/60001/debugger/QT/SkDebuggerGU...
File debugger/QT/SkDebuggerGUI.cpp (right):

https://codereview.chromium.org/202983003/diff/60001/debugger/QT/SkDebuggerGU...
debugger/QT/SkDebuggerGUI.cpp:321: 
This seems a bit odd. We usually signal optional behavior by having a pointer
parameter and passing NULL.

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

https://codereview.chromium.org/202983003/diff/60001/tools/PictureRenderer.cp...
tools/PictureRenderer.cpp:70:
SkAssertResult(SkBitmapHasher::ComputeDigest(bitmap, &hash));
this->

https://codereview.chromium.org/202983003/diff/60001/tools/PictureRenderer.cp...
tools/PictureRenderer.cpp:312: */
Write

https://codereview.chromium.org/202983003/diff/60001/tools/PictureRenderer.h
File tools/PictureRenderer.h (right):

https://codereview.chromium.org/202983003/diff/60001/tools/PictureRenderer.h#...
tools/PictureRenderer.h:119: *     or empty string if this renderer should not
write files at all.
inputFilename?

[epoger, 2014-03-19 15:20:28]

Thanks, Rob.  PTAL

https://codereview.chromium.org/202983003/diff/60001/debugger/QT/SkDebuggerGU...
File debugger/QT/SkDebuggerGUI.cpp (right):

https://codereview.chromium.org/202983003/diff/60001/debugger/QT/SkDebuggerGU...
debugger/QT/SkDebuggerGUI.cpp:321: 
On 2014/03/19 14:44:15, robertphillips1 wrote:
> This seems a bit odd. We usually signal optional behavior by having a pointer
> parameter and passing NULL.

Agreed.  I think this means 1 or 2 changes, what do you think?

1. Change init() to take const SkString* (or perhaps const char*), and copy the
strings passed in

2. Change fOutputDir and fInputFilename to be SkString* (or perhaps const
char*).  Or should they stay as SkString, so we don't need to worry about
allocating/deallocating memory to hold the SkString object during the
PictureRenderer object's lifetime?

https://codereview.chromium.org/202983003/diff/60001/tools/CopyTilesRenderer.cpp
File tools/CopyTilesRenderer.cpp (right):

https://codereview.chromium.org/202983003/diff/60001/tools/CopyTilesRenderer....
tools/CopyTilesRenderer.cpp:24: const SkString& inputFilename, bool
useChecksumBasedFilenames) {
On 2014/03/19 14:42:36, robertphillips wrote:
> I think this should call INHERITED::init

I disagree, but I'm not 100% certain about it.

INHERITED::init() would call fCanvas.reset(this->setupCanvas()), instead of (or
in addition to) fCanvas.reset(this->INHERITED::setupCanvas(fLargeTileWidth,
fLargeTileHeight)), and this init() intentionally avoids allocating a large
canvas.

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

https://codereview.chromium.org/202983003/diff/60001/tools/PictureRenderer.cp...
tools/PictureRenderer.cpp:70:
SkAssertResult(SkBitmapHasher::ComputeDigest(bitmap, &hash));
On 2014/03/19 14:44:15, robertphillips1 wrote:
> this->

Done.

https://codereview.chromium.org/202983003/diff/60001/tools/PictureRenderer.cp...
tools/PictureRenderer.cpp:312: */
On 2014/03/19 14:44:15, robertphillips1 wrote:
> Write

I think the function name is already consistent with
https://sites.google.com/site/skiadocs/developer-documentation/contributing-c...
, right?

Static non-class functions in implementation files are lower case with
underscores separating words:

static inline bool tastes_like_chicken(Food food) {
    return kIceCream_Food != food;
}

https://codereview.chromium.org/202983003/diff/60001/tools/PictureRenderer.h
File tools/PictureRenderer.h (right):

https://codereview.chromium.org/202983003/diff/60001/tools/PictureRenderer.h#...
tools/PictureRenderer.h:119: *     or empty string if this renderer should not
write files at all.
On 2014/03/19 14:44:15, robertphillips1 wrote:
> inputFilename?

Done.

[robertphillips, 2014-03-19 16:06:49]

https://codereview.chromium.org/202983003/diff/60001/debugger/QT/SkDebuggerGU...
File debugger/QT/SkDebuggerGUI.cpp (right):

https://codereview.chromium.org/202983003/diff/60001/debugger/QT/SkDebuggerGU...
debugger/QT/SkDebuggerGUI.cpp:321: 
I think changing the parameters to be pointers but keeping the internal class
members as SkStrings would be best. This would match our API style but minimize
the allocation/deallocation concerns.

https://codereview.chromium.org/202983003/diff/60001/tools/CopyTilesRenderer.cpp
File tools/CopyTilesRenderer.cpp (right):

https://codereview.chromium.org/202983003/diff/60001/tools/CopyTilesRenderer....
tools/CopyTilesRenderer.cpp:24: const SkString& inputFilename, bool
useChecksumBasedFilenames) {
You're right. We probably want a comment like the one in
TiledPictureRenderer::init though, i.e.:

// Do not call INHERITED::init(), which would create a (potentially large)
canvas which is not   
// used by bench_pictures.

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

https://codereview.chromium.org/202983003/diff/60001/tools/PictureRenderer.cp...
tools/PictureRenderer.cpp:312: */
Ah yes (thought it was class scoped for no good reason).

[epoger, 2014-03-19 16:48:26]

Thanks, Rob, modified as discussed.  Sending it to a few trybots, and unless you
or they complain, I'll commit.

https://codereview.chromium.org/202983003/diff/60001/debugger/QT/SkDebuggerGU...
File debugger/QT/SkDebuggerGUI.cpp (right):

https://codereview.chromium.org/202983003/diff/60001/debugger/QT/SkDebuggerGU...
debugger/QT/SkDebuggerGUI.cpp:321: 
On 2014/03/19 16:06:50, robertphillips wrote:
> I think changing the parameters to be pointers but keeping the internal class
> members as SkStrings would be best. This would match our API style but
minimize
> the allocation/deallocation concerns.

Done.

https://codereview.chromium.org/202983003/diff/60001/tools/CopyTilesRenderer.cpp
File tools/CopyTilesRenderer.cpp (right):

https://codereview.chromium.org/202983003/diff/60001/tools/CopyTilesRenderer....
tools/CopyTilesRenderer.cpp:24: const SkString& inputFilename, bool
useChecksumBasedFilenames) {
On 2014/03/19 16:06:50, robertphillips wrote:
> You're right. We probably want a comment like the one in
> TiledPictureRenderer::init though, i.e.:
> 
> // Do not call INHERITED::init(), which would create a (potentially large)
> canvas which is not   
> // used by bench_pictures.

Done.

[epoger, 2014-03-19 17:09:42]

The CQ bit was checked by epoger@google.com

[commit-bot: I haz the power, 2014-03-19 17:09:46]

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

[commit-bot: I haz the power, 2014-03-19 17:26:13]

Change committed as 13859