Now store chunk size in skp file format

Now store chunk size in skp file format

[robertphillips, 2013-02-11 21:16:44]

In order to hop around in an skp file (for more complex optimizations involving
variable sized objects) this CL stores the chunk size inside the op-code's word
with an overflow fallback.

[robertphillips, 2013-02-13 13:07:53]

Any comments on this Mike?

[reed1, 2013-02-13 14:03:59]

Is this CL suggestive of ways we can eliminate some check-for-space overhead on
each call to writeFoo()?

e.g.

addDraw(SCALE, ...);
addScalar(sx);
addScalar(sy);

Now that we are passing the total size to addDraw(), perhaps it could allow us
to make the subsequent write calls faster...

SkScalar* ptr = (SkScalar*)addDraw(SCALE, ...);
*ptr++ = sx;
*ptr++ = sy;

https://codereview.appspot.com/7308087/diff/2001/src/core/SkPictureRecord.cpp
File src/core/SkPictureRecord.cpp (right):

https://codereview.appspot.com/7308087/diff/2001/src/core/SkPictureRecord.cpp...
src/core/SkPictureRecord.cpp:121: * Read the op code from 'offset' in 'writer'
and extract the size too.
Do we have a type/enum for the returned op-code, other than uint32_t?

https://codereview.appspot.com/7308087/diff/2001/src/core/SkPictureRecord.cpp...
src/core/SkPictureRecord.cpp:655: uint32_t initialOffset;
Can we neatly compute the opcode here, and only call addDraw() once with that as
its first parameter?

https://codereview.appspot.com/7308087/diff/2001/src/core/SkPictureRecord.cpp...
src/core/SkPictureRecord.cpp:811: 
Is this getting inlined at the call-sites? Could it be?

https://codereview.appspot.com/7308087/diff/2001/src/core/SkPictureRecord.h
File src/core/SkPictureRecord.h (right):

https://codereview.appspot.com/7308087/diff/2001/src/core/SkPictureRecord.h#n...
src/core/SkPictureRecord.h:110: * Returns the start offset of the chunk.
// Is the returned offset pointing to the op-code (+ size) , or that data after
it?

[junov1, 2013-02-13 14:47:26]

If I understand this correctly, we are adding the extra weight of 4 bytes per op
to all SkPictures, but these bytes are only useful in specific cases
(SK_DEVELOPER builds)?
Could we instead make the encoding of chunk sizes optional (off by default).
Tools that benefit from chunk sizes could draw the skp into an intermediate
SkPicture that encodes chunk sizes.

Also, what is the problem you are trying to solve? I think it might be possible
to solve it by leveraging stuff we already have.  Bounding box hierarchies, for
example, store the offsets of draw commands, without imposing any overhead to
vanilla SkPictures.

[reed1, 2013-02-13 15:09:29]

This adds 4bytes *only* if the data size for a given op exceeds 1 << 24 (which
none of ours do), so there should be zero impact on size.

[junov1, 2013-02-13 15:38:27]

Ah, so it kicks in for cases like skps with bitmaps serialized into the stream? 
Ok, then I retract my objection.

[robertphillips, 2013-02-14 15:54:48]

This CL certainly opens to the door to calling SkWriter32:reserve inside of
addDraw and returning a pointer. That could introduce some wasted space (e.g.,
if a block could only store half of the new chunk a new block would be
allocated) but it might reduce overhead and (depending on the minimum block
size) decrease memory fragmentation.

Justin - with this CL the size will always be stored at the start of each chunk.
It should almost never overflow and require an extra word. The goal of this is
so that we can skip around inside a picture in PictureRecord to evaluate and
perform more complex optimizations without having to actually read each chunk.

https://codereview.appspot.com/7308087/diff/2001/src/core/SkPictureRecord.cpp
File src/core/SkPictureRecord.cpp (right):

https://codereview.appspot.com/7308087/diff/2001/src/core/SkPictureRecord.cpp...
src/core/SkPictureRecord.cpp:121: * Read the op code from 'offset' in 'writer'
and extract the size too.
On 2013/02/13 14:03:59, reed1 wrote:
> Do we have a type/enum for the returned op-code, other than uint32_t?

Done.

https://codereview.appspot.com/7308087/diff/2001/src/core/SkPictureRecord.cpp...
src/core/SkPictureRecord.cpp:655: uint32_t initialOffset;
On 2013/02/13 14:03:59, reed1 wrote:
> Can we neatly compute the opcode here, and only call addDraw() once with that
as
> its first parameter?

Done.

https://codereview.appspot.com/7308087/diff/2001/src/core/SkPictureRecord.cpp...
src/core/SkPictureRecord.cpp:811: 
I have moved it back into the header to at least give it a chance.

https://codereview.appspot.com/7308087/diff/2001/src/core/SkPictureRecord.h
File src/core/SkPictureRecord.h (right):

https://codereview.appspot.com/7308087/diff/2001/src/core/SkPictureRecord.h#n...
src/core/SkPictureRecord.h:110: * Returns the start offset of the chunk.
On 2013/02/13 14:03:59, reed1 wrote:
> // Is the returned offset pointing to the op-code (+ size) , or that data
after
> it?

Done.

[robertphillips, 2013-02-15 13:29:02]

Mike - any additional thoughts on this?

[reed1, 2013-02-15 13:33:16]

lgtm

perhaps we can add a comment/todo somewhere local to addDraw() to remind us to
think about a variant that performs the reserve and returns a pointer (since we
may not choose to always operate that ways, since as you say, it forces a
contiguous block).

[robertphillips, 2013-02-15 13:48:03]

Added comment in SkPictureRecord.h

committed as r7747