Fixing SkPicture serialization

Fixing SkPicture serialization

Fixed a few issues while attempting to use the new
serialization path for SkPicture inside a fuzzer:
- SkReadBuffer and SkValidatingReadBuffer both had a fReader 
member instead of sharing the same member, which leads to
problems if a base class function is used
- In SkPicture, a header is now written as a single chunk of 
data, so it also has to be read as a single chunk of data
- In the SkPicturePlayback destructor, a bad deserialization 
would lead to a crash if we don't safely unref fOpData
- Also in SkPicturePlayback, if we only use a ReadBuffer for 
the whole deserialization, additional tags must be added to
parseBufferTag()
- SkValidatingReadBuffer::readBitmap() was broken, but this
path wasn't usen't since the only use case for
SkValidatingReadBuffer is currently image filters and
bitmaps are unflattened as part of the deserialization of
SkBitmapSource
- SkPictureImageFilter was not deserializable. Added it to
SkGlobalInitialization*
- Added a test that exercises the SkPicture serialization /
deserialization code

BUG=skia:

Committed: http://code.google.com/p/skia/source/detail?r=13764

[sugoi1, 2014-03-11 17:12:05]

Multiple fixes around the new (still unused) path for SkPicture deserialization
through SkReadBuffer (as opposed to SkStream)

[Stephen White, 2014-03-11 18:00:57]

Nit: please keep CL description to 72 chars (git tools are dumb and wrap badly).

[sugoi, 2014-03-11 18:03:23]

On 2014/03/11 18:00:57, Stephen White wrote:
> Nit: please keep CL description to 72 chars (git tools are dumb and wrap
badly).

Ok. I'll just re-post my comments here and remove them from the description :

Fixed a few issues while attempting to use the new serialization path for
SkPicture inside a fuzzer:
- SkReadBuffer and SkValidatingReadBuffer both had a fReader member instead of
sharing the same member, which leads to problems if a base class function is
used
- In SkPicture, a header is now written as a single chunk of data, so it also
has to be read as a single chunk of data
- In the SkPicturePlayback destructor, a bad deserialization would lead to a
crash if we don't safely unref fOpData
- Also in SkPicturePlayback, if we only use a ReadBuffer for the whole
deserialization, additional tags must be added to parseBufferTag()
- SkValidatingReadBuffer::readBitmap() was broken, but this path wasn't usen't
since the only use case for SkValidatingReadBuffer is currently image filters
and bitmaps are unflattened as part of the deserialization of SkBitmapSource
- SkPictureImageFilter was not deserializable. Added it to
SkGlobalInitialization*
- Added a test that exercises the SkPicture serialization / deserialization code

[Stephen White, 2014-03-11 18:14:42]

https://codereview.chromium.org/195223003/diff/1/src/core/SkPicturePlayback.cpp
File src/core/SkPicturePlayback.cpp (right):

https://codereview.chromium.org/195223003/diff/1/src/core/SkPicturePlayback.c...
src/core/SkPicturePlayback.cpp:620: SkASSERT(NULL == fOpData);
It looks like if a picture has two SK_PICT_READER_TAGs, it'll leak one of them
in Release builds. Should we just early-out or abort with a validation failure
instead, in that case? (and maybe add a test for this case somewhere?)

https://codereview.chromium.org/195223003/diff/1/src/core/SkPicturePlayback.c...
src/core/SkPicturePlayback.cpp:624: fPictureCount = size;
As above, what happens if a picture has two SK_PICT_PICTURE_TAGs?

https://codereview.chromium.org/195223003/diff/1/src/core/SkValidatingReadBuf...
File src/core/SkValidatingReadBuffer.cpp (right):

https://codereview.chromium.org/195223003/diff/1/src/core/SkValidatingReadBuf...
src/core/SkValidatingReadBuffer.cpp:220: if (fError) {
Nit: could this just be 

if (!this->validate(...)) {
  return;

https://codereview.chromium.org/195223003/diff/1/src/ports/SkGlobalInitializa...
File src/ports/SkGlobalInitialization_chromium.cpp (right):

https://codereview.chromium.org/195223003/diff/1/src/ports/SkGlobalInitializa...
src/ports/SkGlobalInitialization_chromium.cpp:92:
SK_DEFINE_FLATTENABLE_REGISTRAR_ENTRY(SkPictureImageFilter)
This will still not deserialize the contained picture in Chrome, right? Just the
filter?

[Stephen White, 2014-03-11 18:14:51]

On 2014/03/11 18:03:23, sugoi wrote:

> - In SkPicture, a header is now written as a single chunk of data, so it also
> has to be read as a single chunk of data

Pardon my ignorance, but why is that? The new pointer math makes me slightly
nervous.

[sugoi1, 2014-03-11 18:29:48]

On 2014/03/11 18:14:51, Stephen White wrote:
> On 2014/03/11 18:03:23, sugoi wrote:
> 
> > - In SkPicture, a header is now written as a single chunk of data, so it
also
> > has to be read as a single chunk of data
> 
> Pardon my ignorance, but why is that? The new pointer math makes me slightly
> nervous.

Originally, SkPicture::createHeader() was created in an effort to reduce
duplicated code between the stream code and the readbuffer code (see
https://codereview.chromium.org/138063005/, comment #9). Unfortunately, I had
forgotten to adapt SkPicture::InternalOnly_BufferIsSKP() to it after doing the
change. All the pointer logic is done with constants, so that should be resolved
at compile time. I could use a header struct to serialize/deserialize the data
chunk instead if you want.

[sugoi1, 2014-03-11 18:54:03]

https://codereview.chromium.org/195223003/diff/1/src/core/SkPicturePlayback.cpp
File src/core/SkPicturePlayback.cpp (right):

https://codereview.chromium.org/195223003/diff/1/src/core/SkPicturePlayback.c...
src/core/SkPicturePlayback.cpp:620: SkASSERT(NULL == fOpData);
On 2014/03/11 18:14:43, Stephen White wrote:
> It looks like if a picture has two SK_PICT_READER_TAGs, it'll leak one of them
> in Release builds. Should we just early-out or abort with a validation failure
> instead, in that case? (and maybe add a test for this case somewhere?)

Done.

https://codereview.chromium.org/195223003/diff/1/src/core/SkPicturePlayback.c...
src/core/SkPicturePlayback.cpp:624: fPictureCount = size;
On 2014/03/11 18:14:43, Stephen White wrote:
> As above, what happens if a picture has two SK_PICT_PICTURE_TAGs?

Done.

https://codereview.chromium.org/195223003/diff/1/src/core/SkValidatingReadBuf...
File src/core/SkValidatingReadBuffer.cpp (right):

https://codereview.chromium.org/195223003/diff/1/src/core/SkValidatingReadBuf...
src/core/SkValidatingReadBuffer.cpp:220: if (fError) {
On 2014/03/11 18:14:43, Stephen White wrote:
> Nit: could this just be 
> 
> if (!this->validate(...)) {
>   return;
> 

Done.

https://codereview.chromium.org/195223003/diff/1/src/ports/SkGlobalInitializa...
File src/ports/SkGlobalInitialization_chromium.cpp (right):

https://codereview.chromium.org/195223003/diff/1/src/ports/SkGlobalInitializa...
src/ports/SkGlobalInitialization_chromium.cpp:92:
SK_DEFINE_FLATTENABLE_REGISTRAR_ENTRY(SkPictureImageFilter)
On 2014/03/11 18:14:43, Stephen White wrote:
> This will still not deserialize the contained picture in Chrome, right? Just
the
> filter?

Right. The contained picture's deserialization code is still #ifdefed out.

[Stephen White, 2014-03-11 19:05:54]

LGTM for non-API

https://codereview.chromium.org/195223003/diff/20001/src/core/SkPicturePlayba...
File src/core/SkPicturePlayback.cpp (right):

https://codereview.chromium.org/195223003/diff/20001/src/core/SkPicturePlayba...
src/core/SkPicturePlayback.cpp:621: SkASSERT(NULL == fOpData);
Nit: this assert is spurious now.

[sugoi1, 2014-03-11 19:20:02]

https://codereview.chromium.org/195223003/diff/20001/src/core/SkPicturePlayba...
File src/core/SkPicturePlayback.cpp (right):

https://codereview.chromium.org/195223003/diff/20001/src/core/SkPicturePlayba...
src/core/SkPicturePlayback.cpp:621: SkASSERT(NULL == fOpData);
On 2014/03/11 19:05:54, Stephen White wrote:
> Nit: this assert is spurious now.

Well, if you're using SkReadBuffer, it will still be reachable, since
SkReadBuffer::validate() always returns true. If you're using
SkValidatingReadBuffer, then this assert will never be reached.

[reed1, 2014-03-11 19:53:17]

Can we just move "magic" into SkPictInfo, to avoid having to have (yet) another
struct for the header?

https://codereview.chromium.org/195223003/diff/20001/src/core/SkValidatingRea...
File src/core/SkValidatingReadBuffer.cpp (right):

https://codereview.chromium.org/195223003/diff/20001/src/core/SkValidatingRea...
src/core/SkValidatingReadBuffer.cpp:216: const bool useBitmapHeap =
this->readBool();
Is the corresponding writeBool() also in this CL?

[reed1, 2014-03-11 19:53:41]

Can the CL description contain more information?

[sugoi1, 2014-03-11 19:57:30]

On 2014/03/11 19:53:41, reed1 wrote:
> Can the CL description contain more information?

Yeah, sorry, misunderstanding between Stephen and I. I thought he meant 72 chars
total, he actually meant 72 chars per line, I'll put my description back in
there.

[sugoi1, 2014-03-11 20:05:41]

On 2014/03/11 19:53:17, reed1 wrote:
> Can we just move "magic" into SkPictInfo, to avoid having to have (yet)
another struct for the header?

Will do.

https://codereview.chromium.org/195223003/diff/20001/src/core/SkValidatingRea...
File src/core/SkValidatingReadBuffer.cpp (right):

https://codereview.chromium.org/195223003/diff/20001/src/core/SkValidatingRea...
src/core/SkValidatingReadBuffer.cpp:216: const bool useBitmapHeap =
this->readBool();
On 2014/03/11 19:53:18, reed1 wrote:
> Is the corresponding writeBool() also in this CL?

It was already there (see SkWriteBuffer.cpp:172, approximately). This function
simply wasn't working properly. I thought it had been exercised when
deserializing SkImageFilters, but it turns out that if you have a bitmap in a
filter dag, that bitmap is deserialized within SkBitmapSource, so
SkValidatingReadBuffer::readBitmap() was never used.

[reed1, 2014-03-11 21:13:12]

lgtm w/ suggestions

https://codereview.chromium.org/195223003/diff/40001/src/core/SkPicture.cpp
File src/core/SkPicture.cpp (right):

https://codereview.chromium.org/195223003/diff/40001/src/core/SkPicture.cpp#n...
src/core/SkPicture.cpp:269: // Check magic bytes.
should we have a static helper 

static bool is_valid_pictinfo(const SkPictInfo&) { ... }

it seems like the stream and readbuffer could both call it.

https://codereview.chromium.org/195223003/diff/40001/src/core/SkPicture.cpp#n...
src/core/SkPicture.cpp:363: header->fVersion = CURRENT_PICTURE_VERSION;
nit: calling this info instead of header would be a smaller change, and just as
meaningful (since the parameter type is SkPictInfo). Just fyi.

[sugoi1, 2014-03-12 12:14:06]

On 2014/03/11 21:13:12, reed1 wrote:
> lgtm w/ suggestions
> 
> https://codereview.chromium.org/195223003/diff/40001/src/core/SkPicture.cpp
> File src/core/SkPicture.cpp (right):
> 
>
https://codereview.chromium.org/195223003/diff/40001/src/core/SkPicture.cpp#n...
> src/core/SkPicture.cpp:269: // Check magic bytes.
> should we have a static helper 
> 
> static bool is_valid_pictinfo(const SkPictInfo&) { ... }
> 
> it seems like the stream and readbuffer could both call it.
> 
>
https://codereview.chromium.org/195223003/diff/40001/src/core/SkPicture.cpp#n...
> src/core/SkPicture.cpp:363: header->fVersion = CURRENT_PICTURE_VERSION;
> nit: calling this info instead of header would be a smaller change, and just
as
> meaningful (since the parameter type is SkPictInfo). Just fyi.

Fixed the nits. One note: The static function has to be a member of SkPicture,
otherwise it can't access the MIN_PICTURE_VERSION / CURRENT_PICTURE_VERSION
values.

[reed1, 2014-03-12 13:23:42]

On 2014/03/12 12:14:06, sugoi1 wrote:
> On 2014/03/11 21:13:12, reed1 wrote:
> > lgtm w/ suggestions
> > 
> > https://codereview.chromium.org/195223003/diff/40001/src/core/SkPicture.cpp
> > File src/core/SkPicture.cpp (right):
> > 
> >
>
https://codereview.chromium.org/195223003/diff/40001/src/core/SkPicture.cpp#n...
> > src/core/SkPicture.cpp:269: // Check magic bytes.
> > should we have a static helper 
> > 
> > static bool is_valid_pictinfo(const SkPictInfo&) { ... }
> > 
> > it seems like the stream and readbuffer could both call it.
> > 
> >
>
https://codereview.chromium.org/195223003/diff/40001/src/core/SkPicture.cpp#n...
> > src/core/SkPicture.cpp:363: header->fVersion = CURRENT_PICTURE_VERSION;
> > nit: calling this info instead of header would be a smaller change, and just
> as
> > meaningful (since the parameter type is SkPictInfo). Just fyi.
> 
> Fixed the nits. One note: The static function has to be a member of SkPicture,
> otherwise it can't access the MIN_PICTURE_VERSION / CURRENT_PICTURE_VERSION
> values.

thanks

[sugoi1, 2014-03-12 13:52:07]

The CQ bit was checked by sugoi@chromium.org

[commit-bot: I haz the power, 2014-03-12 13:52:13]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/sugoi@chromium.org/195223003/60001

[commit-bot: I haz the power, 2014-03-12 14:46:46]

Change committed as 13764

[robertphillips, 2014-03-12 23:19:03]

I believe this CL has introduced a new memory leak in the SerializationTest:

12:35:20.745579] ==2938== 263,084 (40 direct, 263,044 indirect) bytes in 1
blocks are definitely lost in loss record 1,266 of 1,273
[12:35:20.745601] ==2938==    at 0x4C2B1C7: operator new(unsigned long) (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
[12:35:20.745622] ==2938==    by 0x54DA93:
SkPicture::CreateFromBuffer(SkReadBuffer&) (SkPicture.cpp:354)
[12:35:20.745642] ==2938==    by 0x4C5CAE: Serialization(skiatest::Reporter*)
(SerializationTest.cpp:386)
[12:35:20.745663] ==2938==    by 0x4C5E09:
skiatest::SerializationClass::onRun(skiatest::Reporter*)
(SerializationTest.cpp:291)
[12:35:20.745683] ==2938==    by 0x440DD2: skiatest::Test::run() (Test.cpp:108)
[12:35:20.745703] ==2938==    by 0x4070A6: SkTestRunnable::run()
(skia_test.cpp:108)
[12:35:20.745722] ==2938==    by 0x407065: SkTThreadPool<void>::Loop(void*)
(SkThreadPool.h:108)
[12:35:20.745743] ==2938==    by 0x65552A: thread_start(void*)
(SkThreadUtils_pthread.cpp:66)
[12:35:20.745762] ==2938==    by 0x5481E99: start_thread (pthread_create.c:308)
[12:35:20.745782] ==2938==    by 0x6A02CBC: clone (clone.S:112)

[sugoi, 2014-03-13 03:11:41]

On 2014/03/12 23:19:03, robertphillips wrote:
> I believe this CL has introduced a new memory leak in the SerializationTest:
> 
> 12:35:20.745579] ==2938== 263,084 (40 direct, 263,044 indirect) bytes in 1
> blocks are definitely lost in loss record 1,266 of 1,273
> [12:35:20.745601] ==2938==    at 0x4C2B1C7: operator new(unsigned long) (in
> /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> [12:35:20.745622] ==2938==    by 0x54DA93:
> SkPicture::CreateFromBuffer(SkReadBuffer&) (SkPicture.cpp:354)
> [12:35:20.745642] ==2938==    by 0x4C5CAE: Serialization(skiatest::Reporter*)
> (SerializationTest.cpp:386)
> [12:35:20.745663] ==2938==    by 0x4C5E09:
> skiatest::SerializationClass::onRun(skiatest::Reporter*)
> (SerializationTest.cpp:291)
> [12:35:20.745683] ==2938==    by 0x440DD2: skiatest::Test::run()
(Test.cpp:108)
> [12:35:20.745703] ==2938==    by 0x4070A6: SkTestRunnable::run()
> (skia_test.cpp:108)
> [12:35:20.745722] ==2938==    by 0x407065: SkTThreadPool<void>::Loop(void*)
> (SkThreadPool.h:108)
> [12:35:20.745743] ==2938==    by 0x65552A: thread_start(void*)
> (SkThreadUtils_pthread.cpp:66)
> [12:35:20.745762] ==2938==    by 0x5481E99: start_thread
(pthread_create.c:308)
> [12:35:20.745782] ==2938==    by 0x6A02CBC: clone (clone.S:112)

Hmmm... This is probably a problem that was already there, but was exposed by
using this previously unused code path in the tests. I'll look into it tomorrow.