Adding storage of SkPath::fIsOval

The fIsOval flag is lost when drawing is deferred. This prevents the gpu path from converting the path to a circle draw.

Without this change (bench w/ -mode deferred):

running bench [640 480] arc  8888: cmsecs =  30.99   GPU: cmsecs = 119.39 gmsecs = 106.94

With this change:

running bench [640 480] arc  8888: cmsecs =  33.70   GPU: cmsecs =  21.11 gmsecs =  12.65

[reed1, 2012-08-03 20:14:49]

If we can pack this into the existing int32 (segmasks, filltype), then we don't
break the picture format (and we save a whole byte)

[robertphillips, 2012-08-06 13:20:54]

Done. Also added storage of "fConvexity".

Here are the numbers on the existing bench marks:

With fix

bench -match ovals -repeat 150 -config 8888 -config GPU -mode deferred

ovals_3  8888: cmsecs =   4.16   GPU: cmsecs =   2.60 gmsecs =   1.41
ovals_1  8888: cmsecs =  11.44   GPU: cmsecs =   2.81 gmsecs =   3.53


With out fix

bench -match ovals -repeat 150 -config 8888 -config GPU -mode deferred

ovals_3  8888: cmsecs =   4.78   GPU: cmsecs =  10.19 gmsecs =   5.90
ovals_1  8888: cmsecs =  12.06   GPU: cmsecs =  23.19 gmsecs =  21.55

[reed1, 2012-08-06 13:34:25]

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

http://codereview.appspot.com/6453085/diff/1002/src/core/SkPath.cpp#newcode1712
src/core/SkPath.cpp:1712: int32_t packed = (fConvexity << 16) | (fFillType << 8)
| fSegmentMask;
why not (fIsOval << 24) ?

[robertphillips, 2012-08-06 13:51:15]

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

http://codereview.appspot.com/6453085/diff/1002/src/core/SkPath.cpp#newcode1712
src/core/SkPath.cpp:1712: int32_t packed = (fConvexity << 16) | (fFillType << 8)
| fSegmentMask;
On 2012/08/06 13:34:25, reed1 wrote:
> why not (fIsOval << 24) ?

Done.

[bsalomon, 2012-08-06 14:06:10]

LGTM

[reed1, 2012-08-06 14:07:05]

can we write a unittest that verifies that flattening works?

[robertphillips, 2012-08-06 17:19:10]

Added unit test

[reed1, 2012-08-06 20:20:25]

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

http://codereview.appspot.com/6453085/diff/7002/src/core/SkPath.cpp#newcode1734
src/core/SkPath.cpp:1734: // DIRTY_AFTER_EDIT resets fIsOval and fConvexity
Why are we calling SkToBool? fIsOval is SkBool8... a byte. Why not just

fIsOval = (packed >> 24) & 0xFF, mirroring what we say in the writer?

[reed1, 2012-08-06 20:21:09]

does the new test fail before your CL?

[robertphillips, 2012-08-07 11:31:22]

Yep. The bounds match but the oval & convexity information is lost.

[23/70] Path...
        FAILED: ..\..\tests\PathTest.cpp:770: readBack.getConvexityOrUnknown()
== p.getConvexityOrUnknown()
        FAILED: ..\..\tests\PathTest.cpp:772: readBack.isOval(NULL) ==
p.isOval(NULL)
---- FAILED

[robertphillips, 2012-08-07 11:43:15]

>>Why are we calling SkToBool? fIsOval is SkBool8... a byte. Why not just
>>fIsOval = (packed >> 24) & 0xFF, mirroring what we say in the writer?

It seemed wasteful to spend 8 bits on a Boolean. I'm also hoping that we could
cram future flags into the same space. For example, if we are going to
read/write the bounds we will also want to serialize the fIsFinite flag. Why
don't we just have an int32_t flags field instead of fBoundsIsDirty, fConvexity,
fIsFinite and fIsOval?

[robertphillips, 2012-08-07 11:49:02]

Ah ... so going back to your "why not (fIsOval << 24)" question. I was
attempting to store just 1 bit for the Boolean. So I should either revert the
change that moved to writing "fIsOval << 24" or update the reading code to use
"(packed >> 24) & 0xFF". Preferences?

[bsalomon, 2012-08-07 12:33:02]

On 2012/08/07 11:49:02, robertphillips wrote:
> Ah ... so going back to your "why not (fIsOval << 24)" question. I was
> attempting to store just 1 bit for the Boolean. So I should either revert the
> change that moved to writing "fIsOval << 24" or update the reading code to use
> "(packed >> 24) & 0xFF". Preferences?

Do we even need the "& 0xFF"?

[reed1, 2012-08-07 13:00:27]

packing tightly into an int is fine with me, either all the time (in SkPath) or
just when flattening/unflattening.

There is a tiny perf-hit for storing bitfields instead of bytes
(read/modify/write instead of write to set a value) but not a biggie to me.

[robertphillips, 2012-08-07 15:30:23]

I have made the read match the write (both 8 bits).

I will leave the bit packing for a future CL.

[bsalomon, 2012-08-07 15:33:13]

On 2012/08/07 15:30:23, robertphillips wrote:
> I have made the read match the write (both 8 bits).
> 
> I will leave the bit packing for a future CL.

LGTM

[reed1, 2012-08-07 15:44:06]

lgtm

[robertphillips, 2012-08-07 17:33:10]

committed as r4991