Added GrAAFlatteningConvexPathRenderer

Added GrAAFlatteningConvexPathRenderer.

This is an alternate version of GrAAConvexPathRenderer which handles
curves by first flattening them to straight lines.

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/1a1b3ac0d4feecb0fefa8a07c7abf3471c96f545

[ethannicholas, 2015-05-27 15:10:43]

ethannicholas@google.com changed reviewers:
+ bsalomon@google.com, robertphillips@google.com

[bsalomon, 2015-05-27 17:11:07]

mostly looking good to me. One naming thing: We often use the term "flattening"
WRT a path to mean that we remove all self overlap. I'm thinking that using
flattening to mean linearizing might cause confusion.

What about keeping the name GrAAConvexPathRenderer and calling the tessellator
GrAAConvexPathTessellator?

https://codereview.chromium.org/1158803002/diff/60001/src/gpu/GrAAFlatteningC...
File src/gpu/GrAAFlatteningConvexPathRenderer.cpp (right):

https://codereview.chromium.org/1158803002/diff/60001/src/gpu/GrAAFlatteningC...
src/gpu/GrAAFlatteningConvexPathRenderer.cpp:44: return
(target->caps()->shaderCaps()->shaderDerivativeSupport() && antiAlias &&
probably don't need shader derivative support anymore.

https://codereview.chromium.org/1158803002/diff/60001/src/gpu/GrAAFlatteningC...
src/gpu/GrAAFlatteningConvexPathRenderer.cpp:206: #define MAX(a, b) (a > b ? a :
b)
There is an SkTMax

https://codereview.chromium.org/1158803002/diff/60001/src/gpu/GrAAFlatteningC...
File src/gpu/GrAAFlatteningConvexTessellator.cpp (right):

https://codereview.chromium.org/1158803002/diff/60001/src/gpu/GrAAFlatteningC...
src/gpu/GrAAFlatteningConvexTessellator.cpp:264: lineTo(m, pts[1], false);
style nit, for (non-static) method calls we prefix them with "this->"

e.g. this->lineTo(m, pts[1], false);

https://codereview.chromium.org/1158803002/diff/60001/src/gpu/GrAAFlatteningC...
src/gpu/GrAAFlatteningConvexTessellator.cpp:388: // therefore the correction
factor is negligible and we do not bother with it.
Is it reasonable to have an assert that verifies the assumption?

https://codereview.chromium.org/1158803002/diff/60001/src/gpu/GrAAFlatteningC...
src/gpu/GrAAFlatteningConvexTessellator.cpp:827: // include down here to avoid
compilation errors caused by "-" overload in SkGeometry.h
What's the error?

https://codereview.chromium.org/1158803002/diff/60001/src/gpu/GrAAFlatteningC...
src/gpu/GrAAFlatteningConvexTessellator.cpp:890: void
GrAAFlatteningConvexTessellator::Ring::draw(SkCanvas* canvas, const
GrAAFlatteningConvexTessellator& tess) const {
nit, 100 col wrap

https://codereview.chromium.org/1158803002/diff/60001/src/gpu/GrAAFlatteningC...
File src/gpu/GrAAFlatteningConvexTessellator.h (right):

https://codereview.chromium.org/1158803002/diff/60001/src/gpu/GrAAFlatteningC...
src/gpu/GrAAFlatteningConvexTessellator.h:190: void quadTo(const SkMatrix& m,
SkPoint* pts);
just as documentation, it'd be nice to have pts[2] rather than * pts

[robertphillips, 2015-05-27 18:10:54]

https://codereview.chromium.org/1158803002/diff/60001/src/gpu/GrAAFlatteningC...
File src/gpu/GrAAFlatteningConvexPathRenderer.cpp (right):

https://codereview.chromium.org/1158803002/diff/60001/src/gpu/GrAAFlatteningC...
src/gpu/GrAAFlatteningConvexPathRenderer.cpp:2: /*
Update this ?

https://codereview.chromium.org/1158803002/diff/60001/src/gpu/GrAAFlatteningC...
src/gpu/GrAAFlatteningConvexPathRenderer.cpp:38: bool
GrAAFlatteningConvexPathRenderer::canDrawPath(const GrDrawTarget* target,
Align these guys ?

https://codereview.chromium.org/1158803002/diff/60001/src/gpu/GrAAFlatteningC...
File src/gpu/GrAAFlatteningConvexPathRenderer.h (right):

https://codereview.chromium.org/1158803002/diff/60001/src/gpu/GrAAFlatteningC...
src/gpu/GrAAFlatteningConvexPathRenderer.h:2: /*
Update this ?

https://codereview.chromium.org/1158803002/diff/60001/src/gpu/GrAAFlatteningC...
File src/gpu/GrAAFlatteningConvexTessellator.cpp (right):

https://codereview.chromium.org/1158803002/diff/60001/src/gpu/GrAAFlatteningC...
src/gpu/GrAAFlatteningConvexTessellator.cpp:53: int
GrAAFlatteningConvexTessellator::addPt(const SkPoint& pt,
line these guys up ?

https://codereview.chromium.org/1158803002/diff/60001/src/gpu/GrAAFlatteningC...
src/gpu/GrAAFlatteningConvexTessellator.cpp:89: void
GrAAFlatteningConvexTessellator::updatePt(int index,
line up ?

https://codereview.chromium.org/1158803002/diff/60001/src/gpu/GrAAFlatteningC...
src/gpu/GrAAFlatteningConvexTessellator.cpp:207: bool
GrAAFlatteningConvexTessellator::computePtAlongBisector(int startIdx,
line up ?

https://codereview.chromium.org/1158803002/diff/60001/src/gpu/GrAAFlatteningC...
src/gpu/GrAAFlatteningConvexTessellator.cpp:324: SkScalar cross =
SkPoint::CrossProduct(fNorms[0], fNorms.top());
Hmmm ...

https://codereview.chromium.org/1158803002/diff/60001/src/gpu/GrAAFlatteningC...
File src/gpu/GrAAFlatteningConvexTessellator.h (right):

https://codereview.chromium.org/1158803002/diff/60001/src/gpu/GrAAFlatteningC...
src/gpu/GrAAFlatteningConvexTessellator.h:23: 
Does this class really diverge enough that we need a second copy ?

https://codereview.chromium.org/1158803002/diff/60001/src/gpu/GrAddPathRender...
File src/gpu/GrAddPathRenderers_default.cpp (right):

https://codereview.chromium.org/1158803002/diff/60001/src/gpu/GrAddPathRender...
src/gpu/GrAddPathRenderers_default.cpp:49:
chain->addPathRenderer(SkNEW(GrAAFlatteningConvexPathRenderer))->unref();
What's up with this? Are we ready to switch?

[bsalomon, 2015-05-27 18:14:23]

I misread the diff as though it was replacing the old renderer/tessellator with
the new ones (because git recognized the similarity and used the old ones as
baseline files). Maybe the way to land this is to augment the existing
tessallator, add an additional path renderer, and then land another change that
enables the new one? That would make it easier to revert the switch over without
reverting the code (and any subsequent patches that land on top of it).

[ethannicholas, 2015-05-27 19:22:30]

https://codereview.chromium.org/1158803002/diff/60001/src/gpu/GrAAFlatteningC...
File src/gpu/GrAAFlatteningConvexPathRenderer.cpp (right):

https://codereview.chromium.org/1158803002/diff/60001/src/gpu/GrAAFlatteningC...
src/gpu/GrAAFlatteningConvexPathRenderer.cpp:2: /*
On 2015/05/27 18:10:53, robertphillips wrote:
> Update this ?

Done.

https://codereview.chromium.org/1158803002/diff/60001/src/gpu/GrAAFlatteningC...
src/gpu/GrAAFlatteningConvexPathRenderer.cpp:38: bool
GrAAFlatteningConvexPathRenderer::canDrawPath(const GrDrawTarget* target,
On 2015/05/27 18:10:54, robertphillips wrote:
> Align these guys ?

Done.

https://codereview.chromium.org/1158803002/diff/60001/src/gpu/GrAAFlatteningC...
src/gpu/GrAAFlatteningConvexPathRenderer.cpp:44: return
(target->caps()->shaderCaps()->shaderDerivativeSupport() && antiAlias &&
On 2015/05/27 17:11:07, bsalomon wrote:
> probably don't need shader derivative support anymore.

Done.

https://codereview.chromium.org/1158803002/diff/60001/src/gpu/GrAAFlatteningC...
src/gpu/GrAAFlatteningConvexPathRenderer.cpp:206: #define MAX(a, b) (a > b ? a :
b)
On 2015/05/27 17:11:07, bsalomon wrote:
> There is an SkTMax

Done.

https://codereview.chromium.org/1158803002/diff/60001/src/gpu/GrAAFlatteningC...
File src/gpu/GrAAFlatteningConvexPathRenderer.h (right):

https://codereview.chromium.org/1158803002/diff/60001/src/gpu/GrAAFlatteningC...
src/gpu/GrAAFlatteningConvexPathRenderer.h:2: /*
On 2015/05/27 18:10:54, robertphillips wrote:
> Update this ?

Done.

https://codereview.chromium.org/1158803002/diff/60001/src/gpu/GrAAFlatteningC...
File src/gpu/GrAAFlatteningConvexTessellator.cpp (right):

https://codereview.chromium.org/1158803002/diff/60001/src/gpu/GrAAFlatteningC...
src/gpu/GrAAFlatteningConvexTessellator.cpp:264: lineTo(m, pts[1], false);
On 2015/05/27 17:11:07, bsalomon wrote:
> style nit, for (non-static) method calls we prefix them with "this->"
> 
> e.g. this->lineTo(m, pts[1], false);

Done.

https://codereview.chromium.org/1158803002/diff/60001/src/gpu/GrAAFlatteningC...
src/gpu/GrAAFlatteningConvexTessellator.cpp:324: SkScalar cross =
SkPoint::CrossProduct(fNorms[0], fNorms.top());
On 2015/05/27 18:10:54, robertphillips wrote:
> Hmmm ...

The work to compute the min & max cross products showed up in profiling as
taking a significant amount of time, and as far as I could tell was solely used
in assertions that would have failed if the path were not convex. Since the path
has already been verified to be convex, I did not see this work as being worth
the effort.

https://codereview.chromium.org/1158803002/diff/60001/src/gpu/GrAAFlatteningC...
src/gpu/GrAAFlatteningConvexTessellator.cpp:388: // therefore the correction
factor is negligible and we do not bother with it.
On 2015/05/27 17:11:07, bsalomon wrote:
> Is it reasonable to have an assert that verifies the assumption?

Done.

https://codereview.chromium.org/1158803002/diff/60001/src/gpu/GrAAFlatteningC...
src/gpu/GrAAFlatteningConvexTessellator.cpp:827: // include down here to avoid
compilation errors caused by "-" overload in SkGeometry.h
On 2015/05/27 17:11:07, bsalomon wrote:
> What's the error?

In file included from ../../src/gpu/GrAAFlatteningConvexTessellator.cpp:14:
In file included from ../../src/core/SkGeometry.h:12:
../../src/core/SkNx.h:264:58: error: cannot initialize return object of type
'SkPoint::Side' with an rvalue of type 'int'
template <typename T> T operator - (const T& l) { return T(0) - l; }
                                                         ^~~~~~~~
../../src/gpu/GrAAFlatteningConvexTessellator.cpp:134:67: note: in instantiation
of function template specialization 'operator-<SkPoint::Side>' requested here
            fBisectors[cur].setOrthog(fNorms[cur], (SkPoint::Side)-fSide);

https://codereview.chromium.org/1158803002/diff/60001/src/gpu/GrAAFlatteningC...
src/gpu/GrAAFlatteningConvexTessellator.cpp:890: void
GrAAFlatteningConvexTessellator::Ring::draw(SkCanvas* canvas, const
GrAAFlatteningConvexTessellator& tess) const {
On 2015/05/27 17:11:07, bsalomon wrote:
> nit, 100 col wrap

Done.

https://codereview.chromium.org/1158803002/diff/60001/src/gpu/GrAAFlatteningC...
File src/gpu/GrAAFlatteningConvexTessellator.h (right):

https://codereview.chromium.org/1158803002/diff/60001/src/gpu/GrAAFlatteningC...
src/gpu/GrAAFlatteningConvexTessellator.h:190: void quadTo(const SkMatrix& m,
SkPoint* pts);
On 2015/05/27 17:11:07, bsalomon wrote:
> just as documentation, it'd be nice to have pts[2] rather than * pts

Done.

[bsalomon, 2015-05-29 15:07:56]

looking good to me... few small nits.

https://codereview.chromium.org/1158803002/diff/80001/src/gpu/GrAAConvexTesse...
File src/gpu/GrAAConvexTessellator.cpp (left):

https://codereview.chromium.org/1158803002/diff/80001/src/gpu/GrAAConvexTesse...
src/gpu/GrAAConvexTessellator.cpp:253: SkScalar minCross = SK_ScalarMax,
maxCross = -SK_ScalarMax;
I'm not sure how important these were for Rob to debug this thing and he's not
in today to tell us. We could just only compute them in debug builds by wrapping
relevant code in SkDEBUGCODE() or #ifdef SK_DEBUG.

https://codereview.chromium.org/1158803002/diff/80001/src/gpu/GrAAConvexTesse...
File src/gpu/GrAAConvexTessellator.h (right):

https://codereview.chromium.org/1158803002/diff/80001/src/gpu/GrAAConvexTesse...
src/gpu/GrAAConvexTessellator.h:51: #if GR_AA__CONVEX_TESSELLATOR_VIZ
why the extra underscore?

https://codereview.chromium.org/1158803002/diff/80001/src/gpu/GrAAConvexTesse...
src/gpu/GrAAConvexTessellator.h:143: #if GR_AA__CONVEX_TESSELLATOR_VIZ
and here?

https://codereview.chromium.org/1158803002/diff/80001/src/gpu/GrAAConvexTesse...
src/gpu/GrAAConvexTessellator.h:190: void quadTo(const SkMatrix& m, SkPoint*
pts);
pts[2] (and similar for below two funcs)?

https://codereview.chromium.org/1158803002/diff/80001/src/gpu/GrAAConvexTesse...
src/gpu/GrAAConvexTessellator.h:239: #if GR_AA__CONVEX_TESSELLATOR_VIZ
and here

[ethannicholas, 2015-06-02 20:20:42]

https://codereview.chromium.org/1158803002/diff/80001/src/gpu/GrAAConvexTesse...
File src/gpu/GrAAConvexTessellator.cpp (left):

https://codereview.chromium.org/1158803002/diff/80001/src/gpu/GrAAConvexTesse...
src/gpu/GrAAConvexTessellator.cpp:253: SkScalar minCross = SK_ScalarMax,
maxCross = -SK_ScalarMax;
On 2015/05/29 15:07:56, bsalomon wrote:
> I'm not sure how important these were for Rob to debug this thing and he's not
> in today to tell us. We could just only compute them in debug builds by
wrapping
> relevant code in SkDEBUGCODE() or #ifdef SK_DEBUG.

Fixed.

https://codereview.chromium.org/1158803002/diff/80001/src/gpu/GrAAConvexTesse...
File src/gpu/GrAAConvexTessellator.h (right):

https://codereview.chromium.org/1158803002/diff/80001/src/gpu/GrAAConvexTesse...
src/gpu/GrAAConvexTessellator.h:51: #if GR_AA__CONVEX_TESSELLATOR_VIZ
On 2015/05/29 15:07:56, bsalomon wrote:
> why the extra underscore?

Screwed up a search and repla... errr.... I mean.... quantum mechanics.

You wouldn't understand.

Fixed!

[bsalomon, 2015-06-02 20:52:08]

lgtm

[ethannicholas, 2015-06-10 19:05:39]

The CQ bit was checked by ethannicholas@google.com

[ethannicholas, 2015-06-10 19:05:40]

The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com
Link to the patchset: https://codereview.chromium.org/1158803002/#ps120001
(title: "rebased")

[commit-bot: I haz the power, 2015-06-10 19:05:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1158803002/120001

[commit-bot: I haz the power, 2015-06-10 19:11:22]

Committed patchset #7 (id:120001) as
https://skia.googlesource.com/skia/+/1a1b3ac0d4feecb0fefa8a07c7abf3471c96f545