Special-case stroking ovals

Special-case stroking ovals
- should extend for rects and rrects
- should write to be more sharable w/ other devices (and possibly public?)

[reed1, 2013-02-04 15:00:39]

old

running bench [640 480]       draw_stroke_oval_miter  8888: cmsecs =  20.04
running bench [640 480]       draw_stroke_oval_bevel  8888: cmsecs =  23.81
running bench [640 480]       draw_stroke_oval_round  8888: cmsecs =  19.98
running bench [640 480]      draw_stroke_rrect_miter  8888: cmsecs =  19.42
running bench [640 480]      draw_stroke_rrect_bevel  8888: cmsecs =  22.53
running bench [640 480]      draw_stroke_rrect_round  8888: cmsecs =  18.85
running bench [640 480]       draw_stroke_rect_miter  8888: cmsecs =   1.25
running bench [640 480]       draw_stroke_rect_bevel  8888: cmsecs =   9.47
running bench [640 480]       draw_stroke_rect_round  8888: cmsecs =  11.03


new

running bench [640 480]       draw_stroke_oval_miter  8888: cmsecs =  13.04
running bench [640 480]       draw_stroke_oval_bevel  8888: cmsecs =  13.01
running bench [640 480]       draw_stroke_oval_round  8888: cmsecs =  13.03
running bench [640 480]      draw_stroke_rrect_miter  8888: cmsecs =  19.77
running bench [640 480]      draw_stroke_rrect_bevel  8888: cmsecs =  23.01
running bench [640 480]      draw_stroke_rrect_round  8888: cmsecs =  19.64
running bench [640 480]       draw_stroke_rect_miter  8888: cmsecs =   1.28
running bench [640 480]       draw_stroke_rect_bevel  8888: cmsecs =   9.80
running bench [640 480]       draw_stroke_rect_round  8888: cmsecs =  11.37


Notice not just that we can speed-up ovals, but 
1. why is rrect slower for miter? (should have been no-op)
2. should we special-case rects?

[reed1, 2013-02-04 15:01:17]

FYI -- not ready to submit, until I try to (1) make it more sharable, and (2)
apply to rrects and possibly rects.

[bsalomon, 2013-02-04 15:10:43]

On 2013/02/04 15:01:17, reed1 wrote:
> FYI -- not ready to submit, until I try to (1) make it more sharable, and (2)
> apply to rrects and possibly rects.

Shared with whom? The GPU device would definitely be less happy to see a path
with two ovals instead of single oval with stroke style.

[caryclark1, 2013-02-04 16:35:32]

https://codereview.appspot.com/7294046/diff/1/trunk/src/core/SkDevice.cpp
File trunk/src/core/SkDevice.cpp (right):

https://codereview.appspot.com/7294046/diff/1/trunk/src/core/SkDevice.cpp#new...
trunk/src/core/SkDevice.cpp:380: break;
add:
default:
 SkASSERT(0);
?

https://codereview.appspot.com/7294046/diff/1/trunk/src/core/SkDevice.cpp#new...
trunk/src/core/SkDevice.cpp:390: } else {
This seems like it duplicates most of the flow of the switch above, i.e., if
this was
} else if (SkPaint::kStroke_Style == paint.getStyle() &&
!paint.getStrokeWidth()) {
...
} else {
  return false;
}
then the switch wouldn't be needed at all.

https://codereview.appspot.com/7294046/diff/1/trunk/src/core/SkDevice.cpp#new...
trunk/src/core/SkDevice.cpp:391: path->incReserve(34);    // enough for 2 ovals
Can '34' be replaced with a const expression that hints where the number came
from? Bonus points if it uses symbols or sizeof() for parts of the expression.

[reed1, 2013-02-11 18:43:34]

https://codereview.appspot.com/7294046/diff/1/trunk/src/core/SkDevice.cpp
File trunk/src/core/SkDevice.cpp (right):

https://codereview.appspot.com/7294046/diff/1/trunk/src/core/SkDevice.cpp#new...
trunk/src/core/SkDevice.cpp:380: break;
On 2013/02/04 16:35:32, caryclark1 wrote:
> add:
> default:
>  SkASSERT(0);
> ?

I guess, though we don't, in general, check for out-of-range enum values in
other switch statements. Are you thinking of a specific issue or bad-pattern?

[caryclark1, 2013-02-11 19:49:59]

https://codereview.appspot.com/7294046/diff/1/trunk/src/core/SkDevice.cpp
File trunk/src/core/SkDevice.cpp (right):

https://codereview.appspot.com/7294046/diff/1/trunk/src/core/SkDevice.cpp#new...
trunk/src/core/SkDevice.cpp:380: break;
I take it back. If one day a new style is added, leaving the default: off will
hopefully generate a compile error. On 2013/02/11 18:43:34, reed1 wrote:
> On 2013/02/04 16:35:32, caryclark1 wrote:
> > add:
> > default:
> >  SkASSERT(0);
> > ?
> 
> I guess, though we don't, in general, check for out-of-range enum values in
> other switch statements. Are you thinking of a specific issue or bad-pattern?