This CL introduces a new path renderer.

This CL introduces a new path renderer.

Here are the characteristics :
- It uses the original path, before stroking
- It supports traight lines only (no curves)
- It supports butt or square caps only
- It supports miter or bevel joins only
- No AA support
Support for these will be added step by step later on.

A first pass at the benchmarks on my linux machine gave me these approximate speed improvements (running all bench with the option '--forceAA 0') :
path_stroke_small_long_line 4X
path_stroke_small_sawtooth 4X
path_stroke_big_rect 4X
path_stroke_small_rect 6X
path_stroke_big_triangle 4X
path_stroke_small_triangle 10X
lines_1_BW 1.5X
dashline_2_square 1.5X
dashline_1_square 1.5X

Also note that I can't submit this code until GrDrawTarget::isOpaque() is implemented, unless I just disable my renderer completely for now.

BUG=chromium:135111
TEST=The following gms are affected and may require rebaselining : lineclosepath, linepath, strokes_poly
Committed: https://code.google.com/p/skia/source/detail?r=7047

[Stephen White, 2013-01-03 19:27:14]

> Here are the characteristics :
> - It uses the non stroked path

Not sure what this means, perhaps "unstroked path"?  "path before stroking"?

> - It straight lines only (no curves)

This sentence no verb.

> - It butt or square caps only
> - It miter or bevel joins only

Same here.

> The name "ProceduralPathRenderer" was not the most inspired one, I'm open to
suggestions.

GrStrokedPathRenderer?  GrStrokePathRenderer:?GrWideStrokedPathRenderer? 
GrOpaqueStrokedPathRenderer?

[Stephen White, 2013-01-03 19:56:10]

https://codereview.appspot.com/7026049/diff/1/src/gpu/GrProceduralPathRendere...
File src/gpu/GrProceduralPathRenderer.cpp (right):

https://codereview.appspot.com/7026049/diff/1/src/gpu/GrProceduralPathRendere...
src/gpu/GrProceduralPathRenderer.cpp:35: SkScalar d = (x1 - x2) * (y3 - y4) -
(y1 - y2) * (x3 - x4);
Technically, you should use SkScalarMul() instead of * when dealing with
SkScalars.  Although since we're no longer testing SK_SCALAR_IS_FIXED, maybe
that doesn't matter?  Mike, should we worry about this?

https://codereview.appspot.com/7026049/diff/1/src/gpu/GrProceduralPathRendere...
src/gpu/GrProceduralPathRenderer.cpp:45: res.set((pre * (x3 - x4) - (x1 - x2) *
post) / d,
Same as above:  if we care, SkScalarMul() instead of *, and SkScalarInvert(d)
and multiply instead of / (and factor this out of the two lines in case the
compiler is dumb).

https://codereview.appspot.com/7026049/diff/1/src/gpu/GrProceduralPathRendere...
src/gpu/GrProceduralPathRenderer.cpp:82: uint8_t* verbs = SkNEW_ARRAY(uint8_t,
nVerbs);
Maybe you want an SkPath::RawIter here to save alloc/free?

https://codereview.appspot.com/7026049/diff/1/src/gpu/GrProceduralPathRendere...
src/gpu/GrProceduralPathRenderer.cpp:129: uint16_t* idxs =
reinterpret_cast<uint16_t*>(arg.indices());
If the path exceeds 64K verts, I think you'll need to split it up, which you
don't appear to be doing currently.  As a first approximation, perhaps you could
check for this canDrawPath().

https://codereview.appspot.com/7026049/diff/1/src/gpu/GrProceduralPathRendere...
src/gpu/GrProceduralPathRenderer.cpp:186: SkScalarSqrt(SkScalarHalf(SK_Scalar1 +
dotProd));
Could you avoid doing the sqrt on each point by squaring the miter angle outside
the loop instead?

https://codereview.appspot.com/7026049/diff/1/src/gpu/GrProceduralPathRendere...
src/gpu/GrProceduralPathRenderer.cpp:258: *idxs++  = vCount+0;
As discussed, you might be able to avoid using indexed geometry altogether by
using a strip, although that might require some finessing on the joins.

[bsalomon, 2013-01-03 20:23:09]

I think we should check this into experimental and hide enabling it in the PR
chain behind a #define/gypvar or require a local patch.

https://codereview.appspot.com/7026049/diff/1/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

https://codereview.appspot.com/7026049/diff/1/src/gpu/GrContext.cpp#newcode1072
src/gpu/GrContext.cpp:1072: if (!strokeRec.isHairlineStyle()) {
I *think* we can just pass the stroke width (or 0 or -1) along without doing
applyToPath and the oval code can handle applying the stroke.

https://codereview.appspot.com/7026049/diff/1/src/gpu/GrProceduralPathRendere...
File src/gpu/GrProceduralPathRenderer.cpp (right):

https://codereview.appspot.com/7026049/diff/1/src/gpu/GrProceduralPathRendere...
src/gpu/GrProceduralPathRenderer.cpp:16: static bool is_clockwise(const
SkVector& before, const SkVector& after)
do we need static here?

https://codereview.appspot.com/7026049/diff/1/src/gpu/GrProceduralPathRendere...
src/gpu/GrProceduralPathRenderer.cpp:22: NO_INTERSECT,
Skia style would be:

kNone_IntersectionType,
kIn_IntersectionType,
kOut_...

https://codereview.appspot.com/7026049/diff/1/src/gpu/GrProceduralPathRendere...
src/gpu/GrProceduralPathRenderer.cpp:49: return (res.x() < GR_CT_MIN(x1, x2) ||
res.x() > GR_CT_MAX(x1, x2) ||
GR_CT_MIN/MAX are meant for compile time (CT) min/max. There are templated
GrMax/GrMin funcs.

https://codereview.appspot.com/7026049/diff/1/src/gpu/GrProceduralPathRendere...
src/gpu/GrProceduralPathRenderer.cpp:82: uint8_t* verbs = SkNEW_ARRAY(uint8_t,
nVerbs);
On 2013/01/03 19:56:10, Stephen White wrote:
> Maybe you want an SkPath::RawIter here to save alloc/free?

Prolly just want to look at the path's segment mask (SkPath::getSegmentMasks()).

https://codereview.appspot.com/7026049/diff/1/src/gpu/GrProceduralPathRendere...
src/gpu/GrProceduralPathRenderer.cpp:296: GrAssert(vCount <= vertexCount);
maybe vertexCount should be called maxVertexCount,  vertexBound or something
more distinct from vCount.

https://codereview.appspot.com/7026049/diff/1/src/gpu/GrProceduralPathRenderer.h
File src/gpu/GrProceduralPathRenderer.h (right):

https://codereview.appspot.com/7026049/diff/1/src/gpu/GrProceduralPathRendere...
src/gpu/GrProceduralPathRenderer.h:11: class GrProceduralPathRenderer : public
GrPathRenderer {
Let's have a general comment about what this guy does here.

[sugoi, 2013-01-04 16:48:12]

I tried to make it easy to enable this code just by setting a flag in
common_variables.gypi. I hope it's clean enough, even though it uses
experimental code. Thanks.

https://codereview.appspot.com/7026049/diff/1/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

https://codereview.appspot.com/7026049/diff/1/src/gpu/GrContext.cpp#newcode1072
src/gpu/GrContext.cpp:1072: if (!strokeRec.isHairlineStyle()) {
Well, the 1st line of drawOval() is :
GrAssert(strokeWidth <= 0);
So I'm guessing this isn't supported yet.

https://codereview.appspot.com/7026049/diff/1/src/gpu/GrProceduralPathRendere...
File src/gpu/GrProceduralPathRenderer.cpp (right):

https://codereview.appspot.com/7026049/diff/1/src/gpu/GrProceduralPathRendere...
src/gpu/GrProceduralPathRenderer.cpp:16: static bool is_clockwise(const
SkVector& before, const SkVector& after)
On 2013/01/03 20:23:09, bsalomon wrote:
> do we need static here?

Done.

https://codereview.appspot.com/7026049/diff/1/src/gpu/GrProceduralPathRendere...
src/gpu/GrProceduralPathRenderer.cpp:22: NO_INTERSECT,
On 2013/01/03 20:23:09, bsalomon wrote:
> Skia style would be:
> 
> kNone_IntersectionType,
> kIn_IntersectionType,
> kOut_...
> 

Done.

https://codereview.appspot.com/7026049/diff/1/src/gpu/GrProceduralPathRendere...
src/gpu/GrProceduralPathRenderer.cpp:35: SkScalar d = (x1 - x2) * (y3 - y4) -
(y1 - y2) * (x3 - x4);
On 2013/01/03 19:56:10, Stephen White wrote:
> Technically, you should use SkScalarMul() instead of * when dealing with
> SkScalars.  Although since we're no longer testing SK_SCALAR_IS_FIXED, maybe
> that doesn't matter?  Mike, should we worry about this?

Done.

https://codereview.appspot.com/7026049/diff/1/src/gpu/GrProceduralPathRendere...
src/gpu/GrProceduralPathRenderer.cpp:45: res.set((pre * (x3 - x4) - (x1 - x2) *
post) / d,
On 2013/01/03 19:56:10, Stephen White wrote:
> Same as above:  if we care, SkScalarMul() instead of *, and SkScalarInvert(d)
> and multiply instead of / (and factor this out of the two lines in case the
> compiler is dumb).

Done.

https://codereview.appspot.com/7026049/diff/1/src/gpu/GrProceduralPathRendere...
src/gpu/GrProceduralPathRenderer.cpp:49: return (res.x() < GR_CT_MIN(x1, x2) ||
res.x() > GR_CT_MAX(x1, x2) ||
On 2013/01/03 20:23:09, bsalomon wrote:
> GR_CT_MIN/MAX are meant for compile time (CT) min/max. There are templated
> GrMax/GrMin funcs.

Done.

https://codereview.appspot.com/7026049/diff/1/src/gpu/GrProceduralPathRendere...
src/gpu/GrProceduralPathRenderer.cpp:82: uint8_t* verbs = SkNEW_ARRAY(uint8_t,
nVerbs);
On 2013/01/03 19:56:10, Stephen White wrote:
> Maybe you want an SkPath::RawIter here to save alloc/free?

Done.

https://codereview.appspot.com/7026049/diff/1/src/gpu/GrProceduralPathRendere...
src/gpu/GrProceduralPathRenderer.cpp:82: uint8_t* verbs = SkNEW_ARRAY(uint8_t,
nVerbs);
On 2013/01/03 20:23:09, bsalomon wrote:
> On 2013/01/03 19:56:10, Stephen White wrote:
> > Maybe you want an SkPath::RawIter here to save alloc/free?
> 
> Prolly just want to look at the path's segment mask
(SkPath::getSegmentMasks()).

Done.

https://codereview.appspot.com/7026049/diff/1/src/gpu/GrProceduralPathRendere...
src/gpu/GrProceduralPathRenderer.cpp:129: uint16_t* idxs =
reinterpret_cast<uint16_t*>(arg.indices());
On 2013/01/03 19:56:10, Stephen White wrote:
> If the path exceeds 64K verts, I think you'll need to split it up, which you
> don't appear to be doing currently.  As a first approximation, perhaps you
could
> check for this canDrawPath().

Done.

https://codereview.appspot.com/7026049/diff/1/src/gpu/GrProceduralPathRendere...
src/gpu/GrProceduralPathRenderer.cpp:186: SkScalarSqrt(SkScalarHalf(SK_Scalar1 +
dotProd));
On 2013/01/03 19:56:10, Stephen White wrote:
> Could you avoid doing the sqrt on each point by squaring the miter angle
outside
> the loop instead?

Done.

https://codereview.appspot.com/7026049/diff/1/src/gpu/GrProceduralPathRendere...
src/gpu/GrProceduralPathRenderer.cpp:258: *idxs++  = vCount+0;
I'll do this in a follow up if it still makes sense to optimize this once a
final solution for AA is found.

https://codereview.appspot.com/7026049/diff/1/src/gpu/GrProceduralPathRendere...
src/gpu/GrProceduralPathRenderer.cpp:296: GrAssert(vCount <= vertexCount);
On 2013/01/03 20:23:09, bsalomon wrote:
> maybe vertexCount should be called maxVertexCount,  vertexBound or something
> more distinct from vCount.

Done.

https://codereview.appspot.com/7026049/diff/1/src/gpu/GrProceduralPathRenderer.h
File src/gpu/GrProceduralPathRenderer.h (right):

https://codereview.appspot.com/7026049/diff/1/src/gpu/GrProceduralPathRendere...
src/gpu/GrProceduralPathRenderer.h:11: class GrProceduralPathRenderer : public
GrPathRenderer {
On 2013/01/03 20:23:09, bsalomon wrote:
> Let's have a general comment about what this guy does here.

Done.

[bsalomon, 2013-01-04 18:52:27]

Can you add

#if !defined(GR_STROKE_PATH_RENDERING)
    #define GR_STROKE_PATH_RENDERING                 0
#endif

to GrConfig.h? (right above where the "tail section" begins.)

https://codereview.appspot.com/7026049/diff/4003/experimental/StrokePathRende...
File experimental/StrokePathRenderer/GrStrokePathRenderer.cpp (right):

https://codereview.appspot.com/7026049/diff/4003/experimental/StrokePathRende...
experimental/StrokePathRenderer/GrStrokePathRenderer.cpp:17: {
style-nit: { on previous line

https://codereview.appspot.com/7026049/diff/4003/experimental/StrokePathRende...
File experimental/StrokePathRenderer/GrStrokePathRenderer.h (right):

https://codereview.appspot.com/7026049/diff/4003/experimental/StrokePathRende...
experimental/StrokePathRenderer/GrStrokePathRenderer.h:12: // This path renderer
is made to create geometry (i.e. primitives) from the original path (before
I was thinking above (outside the class) but not big deal.

https://codereview.appspot.com/7026049/diff/4003/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

https://codereview.appspot.com/7026049/diff/4003/src/gpu/GrContext.cpp#newcod...
src/gpu/GrContext.cpp:1070: SkRect ovalRect;
Can this be simplified a bit? Why do we need to applyToPath here? 

It seems like we can just say

if (!isInverseFill && !isStrokeType & isoval) {
  drawOval
} 

If applyToPath actually does any work then the path will no longer be an oval.

[sugoi, 2013-01-04 20:40:37]

https://codereview.appspot.com/7026049/diff/4003/experimental/StrokePathRende...
File experimental/StrokePathRenderer/GrStrokePathRenderer.cpp (right):

https://codereview.appspot.com/7026049/diff/4003/experimental/StrokePathRende...
experimental/StrokePathRenderer/GrStrokePathRenderer.cpp:17: {
On 2013/01/04 18:52:28, bsalomon wrote:
> style-nit: { on previous line

Done.

https://codereview.appspot.com/7026049/diff/4003/experimental/StrokePathRende...
File experimental/StrokePathRenderer/GrStrokePathRenderer.h (right):

https://codereview.appspot.com/7026049/diff/4003/experimental/StrokePathRende...
experimental/StrokePathRenderer/GrStrokePathRenderer.h:12: // This path renderer
is made to create geometry (i.e. primitives) from the original path (before
On 2013/01/04 18:52:28, bsalomon wrote:
> I was thinking above (outside the class) but not big deal.

Done.

https://codereview.appspot.com/7026049/diff/4003/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

https://codereview.appspot.com/7026049/diff/4003/src/gpu/GrContext.cpp#newcod...
src/gpu/GrContext.cpp:1070: SkRect ovalRect;
On 2013/01/04 18:52:28, bsalomon wrote:
> Can this be simplified a bit? Why do we need to applyToPath here? 
> 
> It seems like we can just say
> 
> if (!isInverseFill && !isStrokeType & isoval) {
>   drawOval
> } 
> 
> If applyToPath actually does any work then the path will no longer be an oval.

Done.

[bsalomon, 2013-01-04 23:00:55]

On 2013/01/04 20:40:37, sugoi wrote:
>
https://codereview.appspot.com/7026049/diff/4003/experimental/StrokePathRende...
> File experimental/StrokePathRenderer/GrStrokePathRenderer.cpp (right):
> 
>
https://codereview.appspot.com/7026049/diff/4003/experimental/StrokePathRende...
> experimental/StrokePathRenderer/GrStrokePathRenderer.cpp:17: {
> On 2013/01/04 18:52:28, bsalomon wrote:
> > style-nit: { on previous line
> 
> Done.
> 
>
https://codereview.appspot.com/7026049/diff/4003/experimental/StrokePathRende...
> File experimental/StrokePathRenderer/GrStrokePathRenderer.h (right):
> 
>
https://codereview.appspot.com/7026049/diff/4003/experimental/StrokePathRende...
> experimental/StrokePathRenderer/GrStrokePathRenderer.h:12: // This path
renderer
> is made to create geometry (i.e. primitives) from the original path (before
> On 2013/01/04 18:52:28, bsalomon wrote:
> > I was thinking above (outside the class) but not big deal.
> 
> Done.
> 
> https://codereview.appspot.com/7026049/diff/4003/src/gpu/GrContext.cpp
> File src/gpu/GrContext.cpp (right):
> 
>
https://codereview.appspot.com/7026049/diff/4003/src/gpu/GrContext.cpp#newcod...
> src/gpu/GrContext.cpp:1070: SkRect ovalRect;
> On 2013/01/04 18:52:28, bsalomon wrote:
> > Can this be simplified a bit? Why do we need to applyToPath here? 
> > 
> > It seems like we can just say
> > 
> > if (!isInverseFill && !isStrokeType & isoval) {
> >   drawOval
> > } 
> > 
> > If applyToPath actually does any work then the path will no longer be an
oval.
> 
> Done.

lgtm