Add GPU support for roundrects

Add GPU support for roundrects

This uses the OvalRenderer to render roundrects as "stretched ovals." It adds an
additional shader that handles the straight edges of ellipsoid roundrects better, 
and uses the circle shader for roundrects where the two radii are the same. Only
axis-aligned, simple roundrects are supported. Handles fill, stroke and hairline.

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

[jvanverth1, 2013-04-24 13:16:00]

Here are the times before (Mac OS):

running bench [640 480]            rrects_3_stroke_4   GPU: msecs =  20.03
gmsecs =  13.73
running bench [640 480]                     rrects_3   GPU: msecs =   2.98
gmsecs =   0.65
running bench [640 480]            rrects_1_stroke_4   GPU: msecs =  37.01
gmsecs =  36.21
running bench [640 480]                     rrects_1   GPU: msecs =   4.00
gmsecs =   1.14

and after:

running bench [640 480]            rrects_3_stroke_4   GPU: msecs =  13.77
gmsecs =   5.32
running bench [640 480]                     rrects_3   GPU: msecs =   2.96
gmsecs =   0.90
running bench [640 480]            rrects_1_stroke_4   GPU: msecs =  19.01
gmsecs =  15.76
running bench [640 480]                     rrects_1   GPU: msecs =   3.01
gmsecs =   1.16

On fills, it appears that the win is mainly that the setup cost on the CPU is
lower. Also, I don't think these benches have a lot of roundrects with circular
corners.

[bsalomon, 2013-04-24 14:36:38]

Adding Mike for the SkCanvas/SkDevice changes.

https://codereview.chromium.org/13852049/diff/18001/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

https://codereview.chromium.org/13852049/diff/18001/src/gpu/GrContext.cpp#new...
src/gpu/GrContext.cpp:978: GrDrawState::AutoStageDisable atr(fDrawState);
I don't think we need this. Other draws will call prepareToDraw which will reset
stages. Non-draw calls (e.g. writeRenderTargetPixels) will reset the draw state.

Maybe leave for now and we can try removing this and in other draw calls at
once.

https://codereview.chromium.org/13852049/diff/18001/src/gpu/GrOvalRenderer.cpp
File src/gpu/GrOvalRenderer.cpp (right):

https://codereview.chromium.org/13852049/diff/18001/src/gpu/GrOvalRenderer.cp...
src/gpu/GrOvalRenderer.cpp:796: memcpy(indices, kRRectIndices,
sizeof(kRRectIndices));
Should we use a long-lived index buffer? Could use drawIndexedInstances to get
batching if multiple drawn back to back.

[jvanverth1, 2013-04-24 14:52:49]

Replies inline.

On 2013/04/24 14:36:38, bsalomon wrote:
> Adding Mike for the SkCanvas/SkDevice changes.
> 
> https://codereview.chromium.org/13852049/diff/18001/src/gpu/GrContext.cpp
> File src/gpu/GrContext.cpp (right):
> 
>
https://codereview.chromium.org/13852049/diff/18001/src/gpu/GrContext.cpp#new...
> src/gpu/GrContext.cpp:978: GrDrawState::AutoStageDisable atr(fDrawState);
> I don't think we need this. Other draws will call prepareToDraw which will
reset
> stages. Non-draw calls (e.g. writeRenderTargetPixels) will reset the draw
state.
> 
> Maybe leave for now and we can try removing this and in other draw calls at
> once.

Ok, I'll make a note to look at that later.

> https://codereview.chromium.org/13852049/diff/18001/src/gpu/GrOvalRenderer.cpp
> File src/gpu/GrOvalRenderer.cpp (right):
> 
>
https://codereview.chromium.org/13852049/diff/18001/src/gpu/GrOvalRenderer.cp...
> src/gpu/GrOvalRenderer.cpp:796: memcpy(indices, kRRectIndices,
> sizeof(kRRectIndices));
> Should we use a long-lived index buffer? Could use drawIndexedInstances to get
> batching if multiple drawn back to back.

Sure, is there an example of a long-lived index buffer to look at?

[robertphillips, 2013-04-24 15:13:30]

lgtm + nits

https://codereview.chromium.org/13852049/diff/18001/include/gpu/GrOvalRenderer.h
File include/gpu/GrOvalRenderer.h (right):

https://codereview.chromium.org/13852049/diff/18001/include/gpu/GrOvalRendere...
include/gpu/GrOvalRenderer.h:34: bool drawSimpleRRect(GrDrawTarget* target,
const GrContext* context, const GrPaint& paint,
indent?

https://codereview.chromium.org/13852049/diff/18001/include/gpu/GrRect.h
File include/gpu/GrRect.h (right):

https://codereview.chromium.org/13852049/diff/18001/include/gpu/GrRect.h#newc...
include/gpu/GrRect.h:19: typedef SkRect  GrRect;
Do we really need to add a GrRRect?

https://codereview.chromium.org/13852049/diff/18001/include/gpu/SkGpuDevice.h
File include/gpu/SkGpuDevice.h (right):

https://codereview.chromium.org/13852049/diff/18001/include/gpu/SkGpuDevice.h...
include/gpu/SkGpuDevice.h:76: virtual void drawRRect(const SkDraw&, const
SkRRect& r,
indent

https://codereview.chromium.org/13852049/diff/18001/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/13852049/diff/18001/src/core/SkCanvas.cpp#new...
src/core/SkCanvas.cpp:1586: 
Hooray!

https://codereview.chromium.org/13852049/diff/18001/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

https://codereview.chromium.org/13852049/diff/18001/src/gpu/GrContext.cpp#new...
src/gpu/GrContext.cpp:973: void GrContext::drawRRect(const GrPaint& paint,
spaces

https://codereview.chromium.org/13852049/diff/18001/src/gpu/GrOvalRenderer.cpp
File src/gpu/GrOvalRenderer.cpp (right):

https://codereview.chromium.org/13852049/diff/18001/src/gpu/GrOvalRenderer.cp...
src/gpu/GrOvalRenderer.cpp:286: * The output of this effect is a modulation of
the input color and coverage for an axis-aligned
ellipse -> RR?

https://codereview.chromium.org/13852049/diff/18001/src/gpu/GrOvalRenderer.cp...
src/gpu/GrOvalRenderer.cpp:291: * scaling an ellipse to be a circle, it attempts
to find the distance from the offset point to the
ellipse -> RR?

https://codereview.chromium.org/13852049/diff/18001/src/gpu/GrOvalRenderer.cp...
src/gpu/GrOvalRenderer.cpp:330: GLEffect(const GrBackendEffectFactory& factory,
const GrDrawEffect&)
tab before the ':'

https://codereview.chromium.org/13852049/diff/18001/src/gpu/GrOvalRenderer.cp...
src/gpu/GrOvalRenderer.cpp:353: 
rm extra space?

https://codereview.chromium.org/13852049/diff/18001/src/gpu/GrOvalRenderer.cp...
src/gpu/GrOvalRenderer.cpp:356: builder->fsCodeAppendf("\tfloat len =
length(%s.xy);\n", fsOffsetName);
Couldn't offset be a vec2?

https://codereview.chromium.org/13852049/diff/18001/src/gpu/GrOvalRenderer.cp...
src/gpu/GrOvalRenderer.cpp:358: 
ellipse -> RR?

https://codereview.chromium.org/13852049/diff/18001/src/gpu/GrOvalRenderer.cp...
src/gpu/GrOvalRenderer.cpp:734: SkStrokeRec::Style style = stroke.getStyle();
SK_ScalarHalf?

https://codereview.chromium.org/13852049/diff/18001/src/gpu/GrOvalRenderer.cp...
src/gpu/GrOvalRenderer.cpp:756: 
I agree with Brian that this should be long lived like the AARect's (i.e.,
aaFillRectIndexBuffer)

https://codereview.chromium.org/13852049/diff/18001/src/gpu/GrOvalRenderer.cp...
src/gpu/GrOvalRenderer.cpp:769: 
Maybe a comment here r.e. the magic positioning of the center?

[bsalomon, 2013-04-24 15:17:20]

https://codereview.chromium.org/13852049/diff/18001/src/gpu/GrOvalRenderer.cpp
File src/gpu/GrOvalRenderer.cpp (right):

https://codereview.chromium.org/13852049/diff/18001/src/gpu/GrOvalRenderer.cp...
src/gpu/GrOvalRenderer.cpp:756: 
On 2013/04/24 15:13:30, robertphillips wrote:
> I agree with Brian that this should be long lived like the AARect's (i.e.,
> aaFillRectIndexBuffer)

Also can see GrAAHairLinePathRenderer::Create().

[jvanverth1, 2013-04-24 19:02:27]

https://codereview.chromium.org/13852049/diff/18001/include/gpu/GrOvalRenderer.h
File include/gpu/GrOvalRenderer.h (right):

https://codereview.chromium.org/13852049/diff/18001/include/gpu/GrOvalRendere...
include/gpu/GrOvalRenderer.h:34: bool drawSimpleRRect(GrDrawTarget* target,
const GrContext* context, const GrPaint& paint,
On 2013/04/24 15:13:30, robertphillips wrote:
> indent?

Done.

https://codereview.chromium.org/13852049/diff/18001/include/gpu/GrRect.h
File include/gpu/GrRect.h (right):

https://codereview.chromium.org/13852049/diff/18001/include/gpu/GrRect.h#newc...
include/gpu/GrRect.h:19: typedef SkRect  GrRect;
On 2013/04/24 15:13:30, robertphillips wrote:
> Do we really need to add a GrRRect?

Done.

https://codereview.chromium.org/13852049/diff/18001/include/gpu/SkGpuDevice.h
File include/gpu/SkGpuDevice.h (right):

https://codereview.chromium.org/13852049/diff/18001/include/gpu/SkGpuDevice.h...
include/gpu/SkGpuDevice.h:76: virtual void drawRRect(const SkDraw&, const
SkRRect& r,
On 2013/04/24 15:13:30, robertphillips wrote:
> indent

Done.

https://codereview.chromium.org/13852049/diff/18001/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

https://codereview.chromium.org/13852049/diff/18001/src/gpu/GrContext.cpp#new...
src/gpu/GrContext.cpp:973: void GrContext::drawRRect(const GrPaint& paint,
On 2013/04/24 15:13:30, robertphillips wrote:
> spaces

Done.

https://codereview.chromium.org/13852049/diff/18001/src/gpu/GrOvalRenderer.cpp
File src/gpu/GrOvalRenderer.cpp (right):

https://codereview.chromium.org/13852049/diff/18001/src/gpu/GrOvalRenderer.cp...
src/gpu/GrOvalRenderer.cpp:286: * The output of this effect is a modulation of
the input color and coverage for an axis-aligned
On 2013/04/24 15:13:30, robertphillips wrote:
> ellipse -> RR?

Renamed this to AltEllipseEdgeEffect to reflect that it's just a different way
of rendering an ellipse, which the roundrect renderer uses.

https://codereview.chromium.org/13852049/diff/18001/src/gpu/GrOvalRenderer.cp...
src/gpu/GrOvalRenderer.cpp:330: GLEffect(const GrBackendEffectFactory& factory,
const GrDrawEffect&)
On 2013/04/24 15:13:30, robertphillips wrote:
> tab before the ':'

Done.

https://codereview.chromium.org/13852049/diff/18001/src/gpu/GrOvalRenderer.cp...
src/gpu/GrOvalRenderer.cpp:353: 
On 2013/04/24 15:13:30, robertphillips wrote:
> rm extra space?

Done.

https://codereview.chromium.org/13852049/diff/18001/src/gpu/GrOvalRenderer.cp...
src/gpu/GrOvalRenderer.cpp:356: builder->fsCodeAppendf("\tfloat len =
length(%s.xy);\n", fsOffsetName);
On 2013/04/24 15:13:30, robertphillips wrote:
> Couldn't offset be a vec2?

Done.

https://codereview.chromium.org/13852049/diff/18001/src/gpu/GrOvalRenderer.cp...
src/gpu/GrOvalRenderer.cpp:358: 
On 2013/04/24 15:13:30, robertphillips wrote:
> ellipse -> RR?

Done.

https://codereview.chromium.org/13852049/diff/18001/src/gpu/GrOvalRenderer.cp...
src/gpu/GrOvalRenderer.cpp:734: SkStrokeRec::Style style = stroke.getStyle();
On 2013/04/24 15:13:30, robertphillips wrote:
> SK_ScalarHalf?

Done.

https://codereview.chromium.org/13852049/diff/18001/src/gpu/GrOvalRenderer.cp...
src/gpu/GrOvalRenderer.cpp:769: 
On 2013/04/24 15:13:30, robertphillips wrote:
> Maybe a comment here r.e. the magic positioning of the center?

Done.

https://codereview.chromium.org/13852049/diff/18001/src/gpu/GrOvalRenderer.cp...
src/gpu/GrOvalRenderer.cpp:796: memcpy(indices, kRRectIndices,
sizeof(kRRectIndices));
On 2013/04/24 14:36:38, bsalomon wrote:
> Should we use a long-lived index buffer? Could use drawIndexedInstances to get
> batching if multiple drawn back to back.

Done.

[bsalomon, 2013-04-24 19:55:22]

lgtm

For real adding reed@ for the canvas/device changes this time.

[reed1, 2013-04-25 13:33:55]

lgtm

https://codereview.chromium.org/13852049/diff/33002/include/core/SkDevice.h
File include/core/SkDevice.h (right):

https://codereview.chromium.org/13852049/diff/33002/include/core/SkDevice.h#n...
include/core/SkDevice.h:262: virtual void drawRRect(const SkDraw&, const
SkRRect& rr,
Woot!

https://codereview.chromium.org/13852049/diff/33002/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/13852049/diff/33002/src/core/SkCanvas.cpp#new...
src/core/SkCanvas.cpp:1584: return;
Do you think we should also check for isOval at this level, and call drawOval
before calling the device?

[jvanverth1, 2013-04-25 13:37:47]

https://codereview.chromium.org/13852049/diff/33002/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/13852049/diff/33002/src/core/SkCanvas.cpp#new...
src/core/SkCanvas.cpp:1584: return;
On 2013/04/25 13:33:55, reed1 wrote:
> Do you think we should also check for isOval at this level, and call drawOval
> before calling the device?

Good idea, I'll do that before committing.

[commit-bot: I haz the power, 2013-04-25 15:18:45]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/jvanverth@google.com/13852049/39001

[commit-bot: I haz the power, 2013-04-25 15:27:04]

Change committed as 8859