Stateful PathRenderer implementation

Moving the oval and rectorii special cases out of the main rendering logic (and into a path renderer) seems to require a more stateful path renderer API. Here is one possible variant of that.

[robertphillips, 2013-09-17 15:09:31]

The only real excitement is in GrPathRenderer and GrClipMaskManager.

[robertphillips, 2013-09-17 18:05:16]

Much cleaner. Performance TBD.

Note that, if this proves acceptable, I will need to make another pass clearing
the path out of the PathRenderer(s) once it is no longer needed.

[bsalomon, 2013-09-17 18:16:48]

Do you think there's a way to make the unregistration of the path automatic?

https://codereview.chromium.org/23926019/diff/13001/src/gpu/GrPathRenderer.h
File src/gpu/GrPathRenderer.h (right):

https://codereview.chromium.org/23926019/diff/13001/src/gpu/GrPathRenderer.h#...
src/gpu/GrPathRenderer.h:144: SkPath fPath;
private w/ a const getter? I wonder if we ever want a subclass to be able to
modify this.

[robertphillips, 2013-09-17 18:42:17]

If you look at patch 6 vs. patch 8, GrPathRenderer.h
and GrPathRendererChain.cpp, you can see what I had in mind.

[bsalomon, 2013-09-17 18:44:48]

On 2013/09/17 18:42:17, robertphillips wrote:
> If you look at patch 6 vs. patch 8, GrPathRenderer.h
> and GrPathRendererChain.cpp, you can see what I had in mind.

So drawing the path resets it? Is there a way to use the stack?

[robertphillips, 2013-09-17 19:51:16]

Here is a version with a stack-based clearing system (and better protection of
the GrPathRenderer's fPath member.

https://codereview.chromium.org/23926019/diff/13001/src/gpu/GrPathRenderer.h
File src/gpu/GrPathRenderer.h (right):

https://codereview.chromium.org/23926019/diff/13001/src/gpu/GrPathRenderer.h#...
src/gpu/GrPathRenderer.h:144: SkPath fPath;
On 2013/09/17 18:16:49, bsalomon wrote:
> private w/ a const getter? I wonder if we ever want a subclass to be able to
> modify this.

Done.

[bsalomon, 2013-09-17 20:40:45]

On 2013/09/17 19:51:16, robertphillips wrote:
> Here is a version with a stack-based clearing system (and better protection of
> the GrPathRenderer's fPath member.
> 
> https://codereview.chromium.org/23926019/diff/13001/src/gpu/GrPathRenderer.h
> File src/gpu/GrPathRenderer.h (right):
> 
>
https://codereview.chromium.org/23926019/diff/13001/src/gpu/GrPathRenderer.h#...
> src/gpu/GrPathRenderer.h:144: SkPath fPath;
> On 2013/09/17 18:16:49, bsalomon wrote:
> > private w/ a const getter? I wonder if we ever want a subclass to be able to
> > modify this.
> 
> Done.

lgtm... it'd be nice if we really really trusted RVO and could just return an
ACP to force the caller to use it.

[robertphillips, 2013-09-18 20:05:14]

Looks like we do have some regressions (old on left; new on right):

        maskcolor GPU         3125.00    3359.38    -234.38    -7.5% 
    maskopaque GPU         3156.25    3375.00    -218.75    -6.9% 
        maskblack GPU         3156.25    3359.38    -203.13    -6.4% 
       maskshader GPU         4554.69    4640.62     -85.93    -1.9% 

mask* calls SkCanvas::drawPoints.

[robertphillips, 2014-01-29 18:40:32]

As of r13229 things are a bit better (old on left; new on right):

maskopaque GPU c       3008.97    3055.50     -46.53    -1.5% 
 maskcolor GPU c       2987.49    3016.75     -29.26    -1.0% 
 maskblack GPU c       3042.78    3044.12      -1.34    -0.0% 
maskshader GPU c       4193.61    4061.60    +132.01    +3.1%

[bsalomon, 2014-01-29 19:33:52]

Seems like those results code very be noise. lgtm w/ q about assert.

Oddly, the diff had some alignment issues and some of the changed lines showed
up in a condensed font. It must be some rietveld weirdness.

https://codereview.chromium.org/23926019/diff/103001/src/gpu/GrPathRenderer.h
File src/gpu/GrPathRenderer.h (right):

https://codereview.chromium.org/23926019/diff/103001/src/gpu/GrPathRenderer.h...
src/gpu/GrPathRenderer.h:93: fPath = path;
Should we assert here that there is not already a path?

[robertphillips, 2014-01-29 20:09:09]

The assert is a good idea.

I think the regressions are still real. Here are the numbers using the
unlandable (Move fLastMoveToIndex from SkPath to SkPathRef -
https://codereview.chromium.org/146913002/) patch:

maskshader GPU c       4193.61    4194.27      -0.66    -0.0% 
 maskcolor GPU c       2987.49    2877.46    +110.03    +3.7% 
maskopaque GPU c       3008.97    2894.53    +114.44    +3.8% 
 maskblack GPU c       3042.78    2897.78    +145.00    +4.8%

[robertphillips, 2014-02-10 15:22:32]

r13379 (Move fLastMoveToIndex from SkPath to SkPathRef -
https://codereview.chromium.org/146913002/) has landed unblocking this CL.
Please see that CL for performance numbers (comment #3).

https://codereview.chromium.org/23926019/diff/103001/src/gpu/GrPathRenderer.h
File src/gpu/GrPathRenderer.h (right):

https://codereview.chromium.org/23926019/diff/103001/src/gpu/GrPathRenderer.h...
src/gpu/GrPathRenderer.h:93: fPath = path;
On 2014/01/29 19:33:52, bsalomon wrote:
> Should we assert here that there is not already a path?

Done.

[robertphillips, 2014-02-10 16:46:55]

committed as r13384

[robertphillips, 2014-02-11 16:31:35]

This CL was reverted in r13409 (Revert of r13384 (Stateful PathRenderer
implementation) - https://codereview.chromium.org/142543007/) b.c. in some
circumstances the path renderers need to be reentrant.