Check for invalid SkPictures

Check for invalid SkPictures

- Remove hasRecorded() since nobody uses it.
- Add "success" boolean to SkPicture stream constructor
- Track failures in render_pictures and bench_pictures
Committed: https://code.google.com/p/skia/source/detail?r=5573

[Leon, 2012-09-11 14:15:04]

https://codereview.appspot.com/6493105/diff/1/src/core/SkPicture.cpp
File src/core/SkPicture.cpp (right):

https://codereview.appspot.com/6493105/diff/1/src/core/SkPicture.cpp#newcode128
src/core/SkPicture.cpp:128: fValid = true;
fValid = src.fValid;

https://codereview.appspot.com/6493105/diff/1/src/core/SkPicture.cpp#newcode258
src/core/SkPicture.cpp:258: fPlayback = SkNEW_ARGS(SkPicturePlayback, (stream,
info, &isValid));
You can use &fValid here. Then you can eliminate isValid and the assignment at
the end.

[reed1, 2012-09-11 14:17:02]

can hasRecorded() serve the same purpose? It returns false if nothing has been
recorded. Not exactly the same as an invalide picture stream, but might be
sufficient.

[EricB, 2012-09-11 14:55:22]

Uploaded patch set 2.  I don't think hasRecorded() will work exactly the same
way (wouldn't that be more equivalent to an isEmpty()?), but if hasRecorded()
returns false for an SkPicture which failed to load from an SkStream, it would
serve the purpose just fine.

https://codereview.appspot.com/6493105/diff/1/src/core/SkPicture.cpp
File src/core/SkPicture.cpp (right):

https://codereview.appspot.com/6493105/diff/1/src/core/SkPicture.cpp#newcode128
src/core/SkPicture.cpp:128: fValid = true;
On 2012/09/11 14:15:04, scroggo-work wrote:
> fValid = src.fValid;

Done.

https://codereview.appspot.com/6493105/diff/1/src/core/SkPicture.cpp#newcode258
src/core/SkPicture.cpp:258: fPlayback = SkNEW_ARGS(SkPicturePlayback, (stream,
info, &isValid));
On 2012/09/11 14:15:04, scroggo-work wrote:
> You can use &fValid here. Then you can eliminate isValid and the assignment at
> the end.

Done.

[Leon, 2012-09-11 15:01:41]

Yeah, I noticed that on an SkPicture where endRecording has been called (so
fPlayback is not null) hasRecorded will return false. If we changed hasRecorded
to return true if fPlayback is non-NULL, we could reuse it.

I looked through our sources to see who calls hasRecorded, to see if would be
okay to change the meaning, and it looks like no one does (I am assuming it was
used by SkDeferredCanvas, which now calls a function called hasRecorded on its
PipeController, which has replaced its SkPicture).

[reed1, 2012-09-11 15:07:12]

On 2012/09/11 15:01:41, scroggo-work wrote:
> Yeah, I noticed that on an SkPicture where endRecording has been called (so
> fPlayback is not null) hasRecorded will return false. If we changed
hasRecorded
> to return true if fPlayback is non-NULL, we could reuse it.
> 
> I looked through our sources to see who calls hasRecorded, to see if would be
> okay to change the meaning, and it looks like no one does (I am assuming it
was
> used by SkDeferredCanvas, which now calls a function called hasRecorded on its
> PipeController, which has replaced its SkPicture).

Ah, if there are no callers, then I'd like to revise my suggestion.

1. remove hasRecorded !
2. add isEmpty() -- returns true if there are no drawing ops

[EricB, 2012-09-11 15:31:47]

Uploaded patch set 4.  Removed hasRecorded() in favor of isEmpty().  Use
isEmpty() to determine whether the input skp has any draw commands.

[Leon, 2012-09-11 15:43:58]

https://codereview.appspot.com/6493105/diff/8002/src/core/SkPicture.cpp
File src/core/SkPicture.cpp (right):

https://codereview.appspot.com/6493105/diff/8002/src/core/SkPicture.cpp#newco...
src/core/SkPicture.cpp:199: return NULL == fPlayback &&
There can actually be an fPlayback with no draw commands, if someone calls
beginRecording and then endRecording. That results in its own problem
(https://code.google.com/p/skia/issues/detail?id=855) so maybe we should not
worry about that here.

[reed1, 2012-09-11 15:47:05]

lets consider adding some unittests for this guy. Those could exercise
1. picture w/o any recording
2. picture w/ recording but no verbs yet
3. picture handed empty stream
...

[EricB, 2012-09-17 18:10:52]

Uploaded patch set 5.

[Leon, 2012-09-17 18:18:50]

On 2012/09/17 18:10:52, EricB wrote:
> Uploaded patch set 5.

lgtm

[reed1, 2012-09-17 18:20:58]

nit about lack of { }

https://codereview.appspot.com/6493105/diff/3003/src/core/SkPicture.cpp
File src/core/SkPicture.cpp (right):

https://codereview.appspot.com/6493105/diff/3003/src/core/SkPicture.cpp#newco...
src/core/SkPicture.cpp:246: if (success)
nit

if (success) {
...
}

[EricB, 2012-09-17 18:26:17]

Uploaded patch set 6: fix nits.  Committed as r5573.

https://codereview.appspot.com/6493105/diff/3003/src/core/SkPicture.cpp
File src/core/SkPicture.cpp (right):

https://codereview.appspot.com/6493105/diff/3003/src/core/SkPicture.cpp#newco...
src/core/SkPicture.cpp:246: if (success)
On 2012/09/17 18:20:59, reed1 wrote:
> nit
> 
> if (success) {
> ...
> }

Done.

https://codereview.appspot.com/6493105/diff/3003/src/core/SkPicture.cpp#newco...
src/core/SkPicture.cpp:275: *success = true;
And here.

[reed1, 2012-09-17 18:27:33]

lgtm