Use prioritized list of path renderers

[bsalomon, 2011-08-17 18:05:38]

http://codereview.appspot.com/4867058/diff/2001/gpu/include/GrContext.h
File gpu/include/GrContext.h (left):

http://codereview.appspot.com/4867058/diff/2001/gpu/include/GrContext.h#oldco...
gpu/include/GrContext.h:593: void drawClipIntoStencil();
This is an ancient declaration that should have been removed many moons ago.

[TomH, 2011-08-17 18:35:51]

I assume the purpose of this change is to get rid of the hardwired
plain-and-maybe-tesselated assumption. Do we have more path renderers coming in
such that we want to genericize? (Something you said earlier suggests we do, but
I want it in writing.)

I need to leave for the airport, but what I've been able to look at so far LG.
If Steve can look at this, by all means don't wait until I'm back next week for
the TM.

http://codereview.appspot.com/4867058/diff/2001/gpu/src/GrContext.cpp
File gpu/src/GrContext.cpp (right):

http://codereview.appspot.com/4867058/diff/2001/gpu/src/GrContext.cpp#newcode...
gpu/src/GrContext.cpp:1422: GrPrintf("Unable to find pather renderer compatible
with path.\n");
nit: path, not pather

http://codereview.appspot.com/4867058/diff/2001/gpu/src/GrPathRenderer.cpp
File gpu/src/GrPathRenderer.cpp (right):

http://codereview.appspot.com/4867058/diff/2001/gpu/src/GrPathRenderer.cpp#ne...
gpu/src/GrPathRenderer.cpp:20: 
Is there a reason not to give this class its own header/cpp?

http://codereview.appspot.com/4867058/diff/2001/gpu/src/GrPathRenderer.h
File gpu/src/GrPathRenderer.h (right):

http://codereview.appspot.com/4867058/diff/2001/gpu/src/GrPathRenderer.h#newc...
gpu/src/GrPathRenderer.h:94: 
OK, maybe I see a reason not to put the PRChain into its own header - circular
dependency. Getting rid of that would require moving this onto something
PathRendererUtil-like.

http://codereview.appspot.com/4867058/diff/2001/gpu/src/GrTesselatedPathRende...
File gpu/src/GrTesselatedPathRenderer.cpp (right):

http://codereview.appspot.com/4867058/diff/2001/gpu/src/GrTesselatedPathRende...
gpu/src/GrTesselatedPathRenderer.cpp:611: bool
GrTesselatedPathRenderer::supportsAA(const GrDrawTarget* target,
+1

[bsalomon, 2011-08-17 18:40:41]

On Wed, Aug 17, 2011 at 2:35 PM, <tomhudson@google.com> wrote:

> I assume the purpose of this change is to get rid of the hardwired
> plain-and-maybe-tesselated assumption. Do we have more path renderers
> coming in such that we want to genericize? (Something you said earlier
> suggests we do, but I want it in writing.)
>
>
Yes,  look for GrAAHairlinePathRenderer coming to an inbox near you. :)

[bsalomon, 2011-08-17 19:02:17]

On 2011/08/17 18:35:51, TomH wrote:
> 
>
http://codereview.appspot.com/4867058/diff/2001/gpu/src/GrContext.cpp#newcode...
> gpu/src/GrContext.cpp:1422: GrPrintf("Unable to find pather renderer
compatible
> with path.\n");
> nit: path, not pather
Done.

>
http://codereview.appspot.com/4867058/diff/2001/gpu/src/GrPathRenderer.cpp#ne...
> gpu/src/GrPathRenderer.cpp:20: 
> Is there a reason not to give this class its own header/cpp?

Split GrPathRendererChain and GrDefaultPathRenderer into their own h/cpp files.

[Stephen White, 2011-08-17 21:32:11]

http://codereview.appspot.com/4867058/diff/11001/gpu/src/GrAddPathRenderers_t...
File gpu/src/GrAddPathRenderers_tesselated.cpp (right):

http://codereview.appspot.com/4867058/diff/11001/gpu/src/GrAddPathRenderers_t...
gpu/src/GrAddPathRenderers_tesselated.cpp:13: void
GrPathRenderer::AddPathRenderers(GrContext*,
Hmm.  Since this is static, I guess that means we can only have one optional
path renderer?  e.g., you couldn't combine GrTesselatedPathRenderer with your
new hairline renderer?  Or would you just have to write another version of
GrPathRenderer::AddPathRenderers() that adds the ones you want?

http://codereview.appspot.com/4867058/diff/11001/gpu/src/GrPathRenderer.cpp
File gpu/src/GrPathRenderer.cpp (left):

http://codereview.appspot.com/4867058/diff/11001/gpu/src/GrPathRenderer.cpp#o...
gpu/src/GrPathRenderer.cpp:56: bool stencilWrapOpsSupport)
Thanks for pulling this out -- nice to have it in its own file.

http://codereview.appspot.com/4867058/diff/11001/gpu/src/GrPathRendererChain.cpp
File gpu/src/GrPathRendererChain.cpp (right):

http://codereview.appspot.com/4867058/diff/11001/gpu/src/GrPathRendererChain....
gpu/src/GrPathRendererChain.cpp:19: , fOwner(context)
It looks like the context is only used at init() time (for checking gpu caps). 
Would it make sense to make it a parameter to getPathRenderer() instead?  Just a
thought.

[bsalomon, 2011-08-18 13:12:37]

http://codereview.appspot.com/4867058/diff/11001/gpu/src/GrAddPathRenderers_t...
File gpu/src/GrAddPathRenderers_tesselated.cpp (right):

http://codereview.appspot.com/4867058/diff/11001/gpu/src/GrAddPathRenderers_t...
gpu/src/GrAddPathRenderers_tesselated.cpp:13: void
GrPathRenderer::AddPathRenderers(GrContext*,
On 2011/08/17 21:32:11, Stephen White wrote:
> Hmm.  Since this is static, I guess that means we can only have one optional
> path renderer?  e.g., you couldn't combine GrTesselatedPathRenderer with your
> new hairline renderer?  Or would you just have to write another version of
> GrPathRenderer::AddPathRenderers() that adds the ones you want?

The latter. It's not an ideal solution. Another option I considered was using
static initialization to append path renderer factory functions to a global
list. The problem with that approach is that all the factories would have to be
appended from the same source file to control their order.

http://codereview.appspot.com/4867058/diff/11001/gpu/src/GrPathRendererChain.cpp
File gpu/src/GrPathRendererChain.cpp (right):

http://codereview.appspot.com/4867058/diff/11001/gpu/src/GrPathRendererChain....
gpu/src/GrPathRendererChain.cpp:19: , fOwner(context)
On 2011/08/17 21:32:11, Stephen White wrote:
> It looks like the context is only used at init() time (for checking gpu caps).

> Would it make sense to make it a parameter to getPathRenderer() instead?  Just
a
> thought.

I think its desirable for a chain of path renderers  to be specific to one
context over its lifetime. Otherwise, it would complicate a scenario where a
path renderer creates reusable resources (like say an index buffer).

[bsalomon, 2011-08-19 12:38:46]

Hey Stephen, are you OK with this as is?  I'm open to suggestions on how to
improve the method of adding PRs to the list either in this change or a future
enhancement. I think someone has to decide the preference order so they probably
have to be inserted in one place.

[Stephen White, 2011-08-19 14:49:25]

On 2011/08/19 12:38:46, bsalomon wrote:
> Hey Stephen, are you OK with this as is?  I'm open to suggestions on how to
> improve the method of adding PRs to the list either in this change or a future
> enhancement. I think someone has to decide the preference order so they
probably
> have to be inserted in one place.

LGTM.  (Sorry, meant to do this yesterday).

[bsalomon, 2011-08-19 15:42:58]

Closed with r2143.