Added Serialization of SkPath's bound

It appears that this is a wash for the existing benchs:

bench.exe -repeat 200 -config 8888

                                            old        new
       path_reverse_add_path 8888 c         15.60      31.20     -15.60  -100.0% 
             morph_10_dilate 8888 c        140.40     124.80     +15.60   +11.1% 
              morph_10_erode 8888 c        140.40     124.80     +15.60   +11.1% 
            ref_cnt_new_weak 8888 c        124.80     109.20     +15.60   +12.5% 
bitmap_8888_update_volatile_scale_rotate_filter 
                            8888 c         46.80      31.20     +15.60   +33.3% 
              morph_2_dilate 8888 c         46.80      31.20     +15.60   +33.3% 
          grmemorypool_queue 8888 c         31.20      15.60     +15.60   +50.0% 
         path_add_path_trans 8888 c         31.20      15.60     +15.60   +50.0% 
      path_stroke_big_circle 8888 c         31.20      15.60     +15.60   +50.0% 
picture_playback_drawPosText 8888 c         31.20      15.60     +15.60   +50.0% 
picture_playback_drawPosTextH 8888 c         31.20      15.60     +15.60   +50.0% 
   picture_playback_drawText 8888 c         31.20      15.60     +15.60   +50.0% 
                ref_cnt_heap 8888 c         31.20      15.60     +15.60   +50.0% 
        region_difference_16 8888 c         31.20      15.60     +15.60   +50.0% 
             region_union_16 8888 c         31.20      15.60     +15.60   +50.0% 
          isfinite_and_float 8888 c         15.60       0.00     +15.60  +100.0% 
              math_fastIsqrt 8888 c         15.60       0.00     +15.60  +100.0% 
   path_fill_small_long_line 8888 c         15.60       0.00     +15.60  +100.0% 
                path_path_to 8888 c         15.60       0.00     +15.60  +100.0% 

bench.exe -repeat 200 -config 8888 -mode deferred

                                            old        new
              repeatTile_565 8888 c         15.60      31.20     -15.60  -100.0% 
              morph_10_erode 8888 c        140.40     124.80     +15.60   +11.1% 
                 ref_cnt_new 8888 c        124.80     109.20     +15.60   +12.5% 
        region_difference_16 8888 c         31.20      15.60     +15.60   +50.0%

[robertphillips, 2012-08-16 15:30:09]

http://codereview.appspot.com/6458143/diff/1/src/pipe/SkGPipeWrite.cpp
File src/pipe/SkGPipeWrite.cpp (right):

http://codereview.appspot.com/6458143/diff/1/src/pipe/SkGPipeWrite.cpp#newcod...
src/pipe/SkGPipeWrite.cpp:398: 
Leon - without this GM:complexclip_aa_layer triggers an assert on line 122 in
SkWriter32.cpp in SkWriter32::reserve. Any ideas what is going on or what is the
correct fix?

[Leon, 2012-08-16 15:32:53]

http://codereview.appspot.com/6458143/diff/1/src/pipe/SkGPipeWrite.cpp
File src/pipe/SkGPipeWrite.cpp (right):

http://codereview.appspot.com/6458143/diff/1/src/pipe/SkGPipeWrite.cpp#newcod...
src/pipe/SkGPipeWrite.cpp:398: 
On 2012/08/16 15:30:09, robertphillips wrote:
> Leon - without this GM:complexclip_aa_layer triggers an assert on line 122 in
> SkWriter32.cpp in SkWriter32::reserve. Any ideas what is going on or what is
the
> correct fix?

Yes. I have a misplaced parenthesis in SkGPipeWrite. I have a fix under review
at http://codereview.appspot.com/6453126/

[Leon, 2012-08-16 15:37:43]

http://codereview.appspot.com/6458143/diff/1/src/core/SkPath.cpp
File src/core/SkPath.cpp (right):

http://codereview.appspot.com/6458143/diff/1/src/core/SkPath.cpp#newcode1725
src/core/SkPath.cpp:1725: + 4 * sizeof(SkScalar);
I think this means we'll need to bump PICTURE_VERSION in SkPicture.cpp

[reed1, 2012-08-16 15:43:57]

http://codereview.appspot.com/6458143/diff/1/src/core/SkPath.cpp
File src/core/SkPath.cpp (right):

http://codereview.appspot.com/6458143/diff/1/src/core/SkPath.cpp#newcode1736
src/core/SkPath.cpp:1736: 
// We call getBounds() to get the side-effect of ensuring that fBounds has been
computed

http://codereview.appspot.com/6458143/diff/1/src/core/SkPath.cpp#newcode1744
src/core/SkPath.cpp:1744: 
or...

packed = ((fIsOval & 1) << 24) | (fIsFinite & 1) << 25) | ...

http://codereview.appspot.com/6458143/diff/1/src/core/SkPath.cpp#newcode1767
src/core/SkPath.cpp:1767: fSegmentMask = packed & 0xFF;
fIsOval = (packed >> 24) & 1;
fIsFinite = (packed >> 25) & 1;

Maybe we should have a private enum of shift-values for these flags... to use in
packing and unpacking...

http://codereview.appspot.com/6458143/diff/1/src/core/SkPath.cpp#newcode1778
src/core/SkPath.cpp:1778: 
buffer.readRect(&fBounds)? Same for writing?

http://codereview.appspot.com/6458143/diff/1/src/pipe/SkGPipeWrite.cpp
File src/pipe/SkGPipeWrite.cpp (right):

http://codereview.appspot.com/6458143/diff/1/src/pipe/SkGPipeWrite.cpp#newcod...
src/pipe/SkGPipeWrite.cpp:399: #define MIN_BLOCK_SIZE  (17 * 1024)
Should we revert this change, given Leon's pending CL?

[TomH, 2012-08-16 15:44:48]

Note how every single test changes by +/- 15.6 ms?

I think we've got another bit of quantization here.

This is 8888, not GPU. What could be the causes?

Could we be getting a low-rez timer from Windows? Didn't it used to be famous
for this kind of granularity in the bad old days?

[TomH, 2012-08-16 15:45:17]

+More guys for non-GPU quantization oddness

[Leon, 2012-08-16 16:13:32]

http://codereview.appspot.com/6458143/diff/1/src/core/SkPath.cpp
File src/core/SkPath.cpp (right):

http://codereview.appspot.com/6458143/diff/1/src/core/SkPath.cpp#newcode1778
src/core/SkPath.cpp:1778: 
On 2012/08/16 15:43:57, reed1 wrote:
> buffer.readRect(&fBounds)? Same for writing?

That was my first thought, too, but SkWBuffer and SkRBuffer don't have those.

[reed1, 2012-08-16 16:44:09]

buffer.read(&fBounds, sizeof(fBounds)) ?

Just seems like we should use a batchy entry-point for speed (and densenessity)

[robertphillips, 2012-08-16 17:45:38]

PTAL

http://codereview.appspot.com/6458143/diff/1/src/core/SkPath.cpp
File src/core/SkPath.cpp (right):

http://codereview.appspot.com/6458143/diff/1/src/core/SkPath.cpp#newcode1725
src/core/SkPath.cpp:1725: + 4 * sizeof(SkScalar);
On 2012/08/16 15:37:44, scroggo-work wrote:
> I think this means we'll need to bump PICTURE_VERSION in SkPicture.cpp

Done.

http://codereview.appspot.com/6458143/diff/1/src/core/SkPath.cpp#newcode1736
src/core/SkPath.cpp:1736: 
On 2012/08/16 15:43:57, reed1 wrote:
> // We call getBounds() to get the side-effect of ensuring that fBounds has
been
> computed

Done.

http://codereview.appspot.com/6458143/diff/1/src/core/SkPath.cpp#newcode1744
src/core/SkPath.cpp:1744: 
On 2012/08/16 15:43:57, reed1 wrote:
> or...
> 
> packed = ((fIsOval & 1) << 24) | (fIsFinite & 1) << 25) | ...

Done.

http://codereview.appspot.com/6458143/diff/1/src/core/SkPath.cpp#newcode1767
src/core/SkPath.cpp:1767: fSegmentMask = packed & 0xFF;
On 2012/08/16 15:43:57, reed1 wrote:
> fIsOval = (packed >> 24) & 1;
> fIsFinite = (packed >> 25) & 1;
> 
> Maybe we should have a private enum of shift-values for these flags... to use
in
> packing and unpacking...

Done.

http://codereview.appspot.com/6458143/diff/1/src/core/SkPath.cpp#newcode1778
src/core/SkPath.cpp:1778: 
On 2012/08/16 15:43:57, reed1 wrote:
> buffer.readRect(&fBounds)? Same for writing?

Done.

http://codereview.appspot.com/6458143/diff/1/src/pipe/SkGPipeWrite.cpp
File src/pipe/SkGPipeWrite.cpp (right):

http://codereview.appspot.com/6458143/diff/1/src/pipe/SkGPipeWrite.cpp#newcod...
src/pipe/SkGPipeWrite.cpp:399: #define MIN_BLOCK_SIZE  (17 * 1024)
On 2012/08/16 15:43:57, reed1 wrote:
> Should we revert this change, given Leon's pending CL?

Done.

[reed1, 2012-08-16 18:15:44]

http://codereview.appspot.com/6458143/diff/3002/include/core/SkPath.h
File include/core/SkPath.h (right):

http://codereview.appspot.com/6458143/diff/3002/include/core/SkPath.h#newcode827
include/core/SkPath.h:827: enum SerializationOffsets {
Offset or Shift? I think we use Shift to name these in other places in skia.
(grep *.h for Shift)

http://codereview.appspot.com/6458143/diff/3002/include/core/SkPath.h#newcode835
include/core/SkPath.h:835: int                 fLastMoveToIndex;
Future CL possibility: turn the following 6 bytes into an explicit uint32_t.

http://codereview.appspot.com/6458143/diff/3002/src/core/SkPath.cpp
File src/core/SkPath.cpp (right):

http://codereview.appspot.com/6458143/diff/3002/src/core/SkPath.cpp#newcode1721
src/core/SkPath.cpp:1721: if (NULL == storage) {
Why is this assert important for the rest of this file?

[robertphillips, 2012-08-16 18:56:15]

http://codereview.appspot.com/6458143/diff/3002/include/core/SkPath.h
File include/core/SkPath.h (right):

http://codereview.appspot.com/6458143/diff/3002/include/core/SkPath.h#newcode827
include/core/SkPath.h:827: enum SerializationOffsets {
On 2012/08/16 18:15:44, reed1 wrote:
> Offset or Shift? I think we use Shift to name these in other places in skia.
> (grep *.h for Shift)

Done.

http://codereview.appspot.com/6458143/diff/3002/src/core/SkPath.cpp
File src/core/SkPath.cpp (right):

http://codereview.appspot.com/6458143/diff/3002/src/core/SkPath.cpp#newcode1721
src/core/SkPath.cpp:1721: if (NULL == storage) {
On 2012/08/16 18:15:44, reed1 wrote:
> Why is this assert important for the rest of this file?

Done.

[reed1, 2012-08-16 20:18:24]

lgtm w/ suggestion for using the enum for *all* of our shift values for
serialization

http://codereview.appspot.com/6458143/diff/7002/include/core/SkPath.h
File include/core/SkPath.h (right):

http://codereview.appspot.com/6458143/diff/7002/include/core/SkPath.h#newcode827
include/core/SkPath.h:827: enum SerializationOffsets {
(late suggestions)

kConvexity_SerializationShift = 16,
kFillType_SerializationShift = 8,
kSegmentMask_SerializationShift = 0,

[robertphillips, 2012-08-17 10:59:26]

committed as r5143

http://codereview.appspot.com/6458143/diff/7002/include/core/SkPath.h
File include/core/SkPath.h (right):

http://codereview.appspot.com/6458143/diff/7002/include/core/SkPath.h#newcode827
include/core/SkPath.h:827: enum SerializationOffsets {
On 2012/08/16 20:18:24, reed1 wrote:
> (late suggestions)
> 
> kConvexity_SerializationShift = 16,
> kFillType_SerializationShift = 8,
> kSegmentMask_SerializationShift = 0,

Done.