Add new skpinfo tool

Generating the 1M skps frequently yields truncated skps. This tool is intended to help automate weeding these out.

Please see skbug:1057

rmistry for tools, gyp
mtklein for src\core & include\core

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

[reed1, 2014-03-03 15:18:23]

https://codereview.chromium.org/176863004/diff/10001/include/core/SkPicture.h
File include/core/SkPicture.h (right):

https://codereview.chromium.org/176863004/diff/10001/include/core/SkPicture.h...
include/core/SkPicture.h:222: // will be ready to be parsed to create an
SkPicturePlayback.
This part about "ready to be parsed..." is confusing. Is this meant to be a
private API, but exposed here for the tool?
I wonder if we can make that part clearer.

[robertphillips, 2014-03-03 15:29:28]

https://codereview.chromium.org/176863004/diff/10001/include/core/SkPicture.h
File include/core/SkPicture.h (right):

https://codereview.chromium.org/176863004/diff/10001/include/core/SkPicture.h...
include/core/SkPicture.h:222: // will be ready to be parsed to create an
SkPicturePlayback.
On 2014/03/03 15:18:24, reed1 wrote:
> This part about "ready to be parsed..." is confusing. Is this meant to be a
> private API, but exposed here for the tool?
> I wonder if we can make that part clearer.

Done. Hopefully clearer.

[mtklein, 2014-03-03 15:48:47]

https://codereview.chromium.org/176863004/diff/10001/tools/skpinfo.cpp
File tools/skpinfo.cpp (right):

https://codereview.chromium.org/176863004/diff/10001/tools/skpinfo.cpp#newcode21
tools/skpinfo.cpp:21: // is just to check if an SKP has been truncated during
the recording
Just curious: do you suspect they're truncated during recording, or truncated
while writing to disk?  Like, did we just forget an fflush somewhere, or do we
have a recording bug?

https://codereview.chromium.org/176863004/diff/10001/tools/skpinfo.cpp#newcode36
tools/skpinfo.cpp:36: return kMissingInput;
Seems like it'd be nice to SkDebugf some message on errors when !FLAGS_quiet?

https://codereview.chromium.org/176863004/diff/10001/tools/skpinfo.cpp#newcode41
tools/skpinfo.cpp:41: SkStream* stream = SkNEW_ARGS(SkFILEStream, (fileName));
SkFILEStream stream(FLAGS_input[0]); ?

Might also want to check stream isValid() after creating it.

https://codereview.chromium.org/176863004/diff/10001/tools/skpinfo.cpp#newcode60
tools/skpinfo.cpp:60: if (!stream->readBool()) {
Got confused here.  Clearer as this?

if (stream->isAtEnd()) {
  return kSuccess;
}

https://codereview.chromium.org/176863004/diff/10001/tools/skpinfo.cpp#newcode70
tools/skpinfo.cpp:70: uint32_t size = stream->readU32();
tableSize?  I'm sometimes getting a bit tripped up by size/len.

https://codereview.chromium.org/176863004/diff/10001/tools/skpinfo.cpp#newcode91
tools/skpinfo.cpp:91: SkDebugf("Exiting early due to format limitations\n");
What's the deal with these early exits and TODOs?

https://codereview.chromium.org/176863004/diff/10001/tools/skpinfo.cpp#newcod...
tools/skpinfo.cpp:133: int main(int argc, char * const argv[]) {
I'm always puzzled why we don't just write int main(int argc, char** argv)

[robertphillips, 2014-03-03 16:13:18]

https://codereview.chromium.org/176863004/diff/10001/tools/skpinfo.cpp
File tools/skpinfo.cpp (right):

https://codereview.chromium.org/176863004/diff/10001/tools/skpinfo.cpp#newcode21
tools/skpinfo.cpp:21: // is just to check if an SKP has been truncated during
the recording
I suspect they are being truncated during writing. No idea why though.

https://codereview.chromium.org/176863004/diff/10001/tools/skpinfo.cpp#newcode36
tools/skpinfo.cpp:36: return kMissingInput;
On 2014/03/03 15:48:48, mtklein wrote:
> Seems like it'd be nice to SkDebugf some message on errors when !FLAGS_quiet?

Done.

https://codereview.chromium.org/176863004/diff/10001/tools/skpinfo.cpp#newcode41
tools/skpinfo.cpp:41: SkStream* stream = SkNEW_ARGS(SkFILEStream, (fileName));
Done. Somewhat surprised isValid isn't virtual.

https://codereview.chromium.org/176863004/diff/10001/tools/skpinfo.cpp#newcode60
tools/skpinfo.cpp:60: if (!stream->readBool()) {
Here we're getting into the oddities of the picture format. It stores a bool to
determine if there is a playback object in the file. If it doesn't then we're
done reading the file.

I've added a comment.

https://codereview.chromium.org/176863004/diff/10001/tools/skpinfo.cpp#newcode70
tools/skpinfo.cpp:70: uint32_t size = stream->readU32();
On 2014/03/03 15:48:48, mtklein wrote:
> tableSize?  I'm sometimes getting a bit tripped up by size/len.

Done. I renamed these to chunkSize & totStreamSize

https://codereview.chromium.org/176863004/diff/10001/tools/skpinfo.cpp#newcode91
tools/skpinfo.cpp:91: SkDebugf("Exiting early due to format limitations\n");
The size stored with each tag isn't always the size of the chunk in bytes. For
the three early exit cases it is something like "the number of fonts". I would
like to change the format so the size associated with each tag is the number of
bytes and then the tag-specific size is stored within that chunk. For the
current use case this early out seems to be sufficient (since the truncation has
already happened in the PICT_READER_TAG).

https://codereview.chromium.org/176863004/diff/10001/tools/skpinfo.cpp#newcod...
tools/skpinfo.cpp:133: int main(int argc, char * const argv[]) {
I think iOS will reject that.

[reed1, 2014-03-03 16:24:27]

Thanks, I do find the comment clearer. Requesting a more scary name, but not
insisting.

Perhaps we can use /** */ style for large block comments, as we do in other
places?

api lgtm

https://codereview.chromium.org/176863004/diff/50001/include/core/SkPicture.h
File include/core/SkPicture.h (right):

https://codereview.chromium.org/176863004/diff/50001/include/core/SkPicture.h...
include/core/SkPicture.h:226: // If false is returned, SkPictInfo is unmodified.
Do we expect to support these for chrome or other clients? If not (and I know of
no reason yet), perhaps we can make their names scary?
InternalOnly_StreamIsSKP(...) ? Just being scared.

[mtklein, 2014-03-03 16:27:36]

https://codereview.chromium.org/176863004/diff/10001/tools/skpinfo.cpp
File tools/skpinfo.cpp (right):

https://codereview.chromium.org/176863004/diff/10001/tools/skpinfo.cpp#newcode91
tools/skpinfo.cpp:91: SkDebugf("Exiting early due to format limitations\n");
On 2014/03/03 16:13:18, robertphillips wrote:
> The size stored with each tag isn't always the size of the chunk in bytes. For
> the three early exit cases it is something like "the number of fonts". I would
> like to change the format so the size associated with each tag is the number
of
> bytes and then the tag-specific size is stored within that chunk. For the
> current use case this early out seems to be sufficient (since the truncation
has
> already happened in the PICT_READER_TAG).

Ah!  Right.  I even knew this.  Thanks for the reminder.  Can you add this to
the code as a comment?

https://codereview.chromium.org/176863004/diff/10001/tools/skpinfo.cpp#newcod...
tools/skpinfo.cpp:133: int main(int argc, char * const argv[]) {
On 2014/03/03 16:13:18, robertphillips wrote:
> I think iOS will reject that.

This code is ifdef'd out on iOS?  Anyway, not important.

https://codereview.chromium.org/176863004/diff/50001/tools/skpinfo.cpp
File tools/skpinfo.cpp (right):

https://codereview.chromium.org/176863004/diff/50001/tools/skpinfo.cpp#newcode43
tools/skpinfo.cpp:43: SkFILEStream* stream = SkNEW_ARGS(SkFILEStream,
(FLAGS_input[0]));
Sorry if I wasn't clear, I think you really do want
  SkFILEStream stream(FLAGS_input[0]);
There's no reason to heap allocate this stream, and despite it not really
mattering it seems bad form to leak it, particularly when FILEs are involved.

https://codereview.chromium.org/176863004/diff/50001/tools/skpinfo.cpp#newcode69
tools/skpinfo.cpp:69: // If the read bool is false then we're done reading the
file.
I'm not really sure this comment says anything the code doesn't.  Maybe as you
explained it to me?

// If we read true there's a picture playback object flattened in the file; if
false, there isn't a playback, so we're done reading the file.

[robertphillips, 2014-03-03 18:44:23]

https://codereview.chromium.org/176863004/diff/10001/tools/skpinfo.cpp
File tools/skpinfo.cpp (right):

https://codereview.chromium.org/176863004/diff/10001/tools/skpinfo.cpp#newcode91
tools/skpinfo.cpp:91: SkDebugf("Exiting early due to format limitations\n");
On 2014/03/03 16:27:37, mtklein wrote:
> On 2014/03/03 16:13:18, robertphillips wrote:
> > The size stored with each tag isn't always the size of the chunk in bytes.
For
> > the three early exit cases it is something like "the number of fonts". I
would
> > like to change the format so the size associated with each tag is the number
> of
> > bytes and then the tag-specific size is stored within that chunk. For the
> > current use case this early out seems to be sufficient (since the truncation
> has
> > already happened in the PICT_READER_TAG).
> 
> Ah!  Right.  I even knew this.  Thanks for the reminder.  Can you add this to
> the code as a comment?

Done.

https://codereview.chromium.org/176863004/diff/50001/include/core/SkPicture.h
File include/core/SkPicture.h (right):

https://codereview.chromium.org/176863004/diff/50001/include/core/SkPicture.h...
include/core/SkPicture.h:226: // If false is returned, SkPictInfo is unmodified.
On 2014/03/03 16:24:28, reed1 wrote:
> Do we expect to support these for chrome or other clients? If not (and I know
of
> no reason yet), perhaps we can make their names scary?
> InternalOnly_StreamIsSKP(...) ? Just being scared.

Done.

https://codereview.chromium.org/176863004/diff/50001/tools/skpinfo.cpp
File tools/skpinfo.cpp (right):

https://codereview.chromium.org/176863004/diff/50001/tools/skpinfo.cpp#newcode43
tools/skpinfo.cpp:43: SkFILEStream* stream = SkNEW_ARGS(SkFILEStream,
(FLAGS_input[0]));
On 2014/03/03 16:27:37, mtklein wrote:
> Sorry if I wasn't clear, I think you really do want
>   SkFILEStream stream(FLAGS_input[0]);
> There's no reason to heap allocate this stream, and despite it not really
> mattering it seems bad form to leak it, particularly when FILEs are involved.

Done.

https://codereview.chromium.org/176863004/diff/50001/tools/skpinfo.cpp#newcode69
tools/skpinfo.cpp:69: // If the read bool is false then we're done reading the
file.
On 2014/03/03 16:27:37, mtklein wrote:
> I'm not really sure this comment says anything the code doesn't.  Maybe as you
> explained it to me?
> 
> // If we read true there's a picture playback object flattened in the file; if
> false, there isn't a playback, so we're done reading the file.

Done.

[mtklein, 2014-03-03 18:50:04]

On 2014/03/03 18:44:23, robertphillips wrote:
> https://codereview.chromium.org/176863004/diff/10001/tools/skpinfo.cpp
> File tools/skpinfo.cpp (right):
> 
>
https://codereview.chromium.org/176863004/diff/10001/tools/skpinfo.cpp#newcode91
> tools/skpinfo.cpp:91: SkDebugf("Exiting early due to format limitations\n");
> On 2014/03/03 16:27:37, mtklein wrote:
> > On 2014/03/03 16:13:18, robertphillips wrote:
> > > The size stored with each tag isn't always the size of the chunk in bytes.
> For
> > > the three early exit cases it is something like "the number of fonts". I
> would
> > > like to change the format so the size associated with each tag is the
number
> > of
> > > bytes and then the tag-specific size is stored within that chunk. For the
> > > current use case this early out seems to be sufficient (since the
truncation
> > has
> > > already happened in the PICT_READER_TAG).
> > 
> > Ah!  Right.  I even knew this.  Thanks for the reminder.  Can you add this
to
> > the code as a comment?
> 
> Done.
> 
> https://codereview.chromium.org/176863004/diff/50001/include/core/SkPicture.h
> File include/core/SkPicture.h (right):
> 
>
https://codereview.chromium.org/176863004/diff/50001/include/core/SkPicture.h...
> include/core/SkPicture.h:226: // If false is returned, SkPictInfo is
unmodified.
> On 2014/03/03 16:24:28, reed1 wrote:
> > Do we expect to support these for chrome or other clients? If not (and I
know
> of
> > no reason yet), perhaps we can make their names scary?
> > InternalOnly_StreamIsSKP(...) ? Just being scared.
> 
> Done.
> 
> https://codereview.chromium.org/176863004/diff/50001/tools/skpinfo.cpp
> File tools/skpinfo.cpp (right):
> 
>
https://codereview.chromium.org/176863004/diff/50001/tools/skpinfo.cpp#newcode43
> tools/skpinfo.cpp:43: SkFILEStream* stream = SkNEW_ARGS(SkFILEStream,
> (FLAGS_input[0]));
> On 2014/03/03 16:27:37, mtklein wrote:
> > Sorry if I wasn't clear, I think you really do want
> >   SkFILEStream stream(FLAGS_input[0]);
> > There's no reason to heap allocate this stream, and despite it not really
> > mattering it seems bad form to leak it, particularly when FILEs are
involved.
> 
> Done.
> 
>
https://codereview.chromium.org/176863004/diff/50001/tools/skpinfo.cpp#newcode69
> tools/skpinfo.cpp:69: // If the read bool is false then we're done reading the
> file.
> On 2014/03/03 16:27:37, mtklein wrote:
> > I'm not really sure this comment says anything the code doesn't.  Maybe as
you
> > explained it to me?
> > 
> > // If we read true there's a picture playback object flattened in the file;
if
> > false, there isn't a playback, so we're done reading the file.
> 
> Done.

lgtm

[robertphillips, 2014-03-03 18:56:21]

The CQ bit was checked by robertphillips@google.com

[commit-bot: I haz the power, 2014-03-03 18:56:28]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/robertphillips@google.com/176863004/7...

[commit-bot: I haz the power, 2014-03-03 19:18:44]

Change committed as 13643