NV path rendering

[bsalomon, 2012-06-27 21:28:26]

It's not perfect but I think it's a good time to have checkpoint. It's hidden
behind a compile flag that defaults to 0 (and there is a gyp var to toggle it).

The big remaining issues are:
1) We leak paths because there seems to be a driver bug with delete (documented
in comments). Once this is in I'll loop in NV so they can take a look.

2) All paths get drawn with AA even if they are supposed to be non-AA. Drawing
with MSAA disabled doesn't seem to work so I always enable it. I think this is a
driver bug but I have to do a bit more testing and check the spec again.

3) The filltypes_persp slide draws incorrectly. I'm haven't looked into this
yet.

4) On some slides with inverted path fills and the complex clip option enabled
in SampleApp I see incorrect drawing on top of the clip. I haven't investigated
this yet.

I can show you the skdiff of a full GM run with and without this change for the
"msaa16" config.

[robertphillips, 2012-06-27 22:45:03]

LGTM modulo some questions and nits

http://codereview.appspot.com/6349049/diff/5001/gyp/gpu.gyp
File gyp/gpu.gyp (right):

http://codereview.appspot.com/6349049/diff/5001/gyp/gpu.gyp#newcode212
gyp/gpu.gyp:212: '../src/gpu/GrAAConvexPathRenderer.h',
Possible name change? Native, Platform, NV?

http://codereview.appspot.com/6349049/diff/5001/src/gpu/GrBuiltInPathRenderer...
File src/gpu/GrBuiltInPathRenderer.cpp (right):

http://codereview.appspot.com/6349049/diff/5001/src/gpu/GrBuiltInPathRenderer...
src/gpu/GrBuiltInPathRenderer.cpp:38: bool antiAlias) const {
A comment would be good here to explain why this is firing when AA is false.

http://codereview.appspot.com/6349049/diff/5001/src/gpu/GrBuiltInPathRenderer...
src/gpu/GrBuiltInPathRenderer.cpp:121: drawState->viewMatrix()->reset();
Why do we bloat here? Aren't we already covering the entire screen?

http://codereview.appspot.com/6349049/diff/5001/src/gpu/GrBuiltInPathRenderer.h
File src/gpu/GrBuiltInPathRenderer.h (right):

http://codereview.appspot.com/6349049/diff/5001/src/gpu/GrBuiltInPathRenderer...
src/gpu/GrBuiltInPathRenderer.h:15: class GrGpu;
Don't think we need GrPath

http://codereview.appspot.com/6349049/diff/5001/src/gpu/GrBuiltInPathRenderer...
src/gpu/GrBuiltInPathRenderer.h:26: 
virtual on destructor

http://codereview.appspot.com/6349049/diff/5001/src/gpu/GrBuiltInPathRenderer...
src/gpu/GrBuiltInPathRenderer.h:33: 
SK_OVERRIDE?

http://codereview.appspot.com/6349049/diff/5001/src/gpu/GrDrawTarget.h
File src/gpu/GrDrawTarget.h (right):

http://codereview.appspot.com/6349049/diff/5001/src/gpu/GrDrawTarget.h#newcod...
src/gpu/GrDrawTarget.h:447: */
Now you're just going out of your way to remove the parameter names

http://codereview.appspot.com/6349049/diff/5001/src/gpu/GrGpu.cpp
File src/gpu/GrGpu.cpp (right):

http://codereview.appspot.com/6349049/diff/5001/src/gpu/GrGpu.cpp#newcode444
src/gpu/GrGpu.cpp:444: 
It seems that on changing "path" from a reference to a ptr we should be checking
for NULL somewhere or "path" should be non-const

http://codereview.appspot.com/6349049/diff/5001/src/gpu/gl/GrGpuGL.cpp
File src/gpu/gl/GrGpuGL.cpp (right):

http://codereview.appspot.com/6349049/diff/5001/src/gpu/gl/GrGpuGL.cpp#newcod...
src/gpu/gl/GrGpuGL.cpp:1329: GrAssert(fCaps.fPathStencilingSupport);
collapse the following two lines?

http://codereview.appspot.com/6349049/diff/5001/src/gpu/gl/GrGpuGL.h
File src/gpu/gl/GrGpuGL.h (right):

http://codereview.appspot.com/6349049/diff/5001/src/gpu/gl/GrGpuGL.h#newcode371
src/gpu/gl/GrGpuGL.h:371: fViewMatrix = GrMatrix::InvalidMatrix();
Set fHeight to -1 too?

http://codereview.appspot.com/6349049/diff/5001/src/gpu/gl/GrGpuGL_program.cpp
File src/gpu/gl/GrGpuGL_program.cpp (right):

http://codereview.appspot.com/6349049/diff/5001/src/gpu/gl/GrGpuGL_program.cp...
src/gpu/gl/GrGpuGL_program.cpp:478: 
Indentation seems a bit weird here

[bsalomon, 2012-06-28 12:42:50]

http://codereview.appspot.com/6349049/diff/5001/gyp/gpu.gyp
File gyp/gpu.gyp (right):

http://codereview.appspot.com/6349049/diff/5001/gyp/gpu.gyp#newcode212
gyp/gpu.gyp:212: '../src/gpu/GrAAConvexPathRenderer.h',
On 2012/06/27 22:45:03, robertphillips wrote:
> Possible name change? Native, Platform, NV?
 
I suppose Native is better... BuiltIn is pretty bad.  The naming is somewhat
confused by the fact that we may  provide a default implementation for
stencilPath() so this PR could be more general than NV.

Maybe GrStencilPathRenderer? The default PR also uses the stencil buffer but it
would go away if we implemented the above idea. (It would also allow us to
remove the awkward stencil queries/functions on the GrPathRenderer base class).

http://codereview.appspot.com/6349049/diff/5001/src/gpu/GrBuiltInPathRenderer...
File src/gpu/GrBuiltInPathRenderer.cpp (right):

http://codereview.appspot.com/6349049/diff/5001/src/gpu/GrBuiltInPathRenderer...
src/gpu/GrBuiltInPathRenderer.cpp:38: bool antiAlias) const {
On 2012/06/27 22:45:03, robertphillips wrote:
> A comment would be good here to explain why this is firing when AA is false.

Will do.

http://codereview.appspot.com/6349049/diff/5001/src/gpu/GrBuiltInPathRenderer...
src/gpu/GrBuiltInPathRenderer.cpp:121: drawState->viewMatrix()->reset();
On 2012/06/27 22:45:03, robertphillips wrote:
> Why do we bloat here? Aren't we already covering the entire screen?

True, it isn't needed. It also could be set to 0 at line 109 but there could be
precision issues in that case. I'll update.

http://codereview.appspot.com/6349049/diff/5001/src/gpu/GrBuiltInPathRenderer.h
File src/gpu/GrBuiltInPathRenderer.h (right):

http://codereview.appspot.com/6349049/diff/5001/src/gpu/GrBuiltInPathRenderer...
src/gpu/GrBuiltInPathRenderer.h:15: class GrGpu;
On 2012/06/27 22:45:03, robertphillips wrote:
> Don't think we need GrPath

True.

http://codereview.appspot.com/6349049/diff/5001/src/gpu/GrBuiltInPathRenderer...
src/gpu/GrBuiltInPathRenderer.h:26: 
On 2012/06/27 22:45:03, robertphillips wrote:
> virtual on destructor

nice catch

http://codereview.appspot.com/6349049/diff/5001/src/gpu/GrBuiltInPathRenderer...
src/gpu/GrBuiltInPathRenderer.h:33: 
On 2012/06/27 22:45:03, robertphillips wrote:
> SK_OVERRIDE?

yup

http://codereview.appspot.com/6349049/diff/5001/src/gpu/GrDrawTarget.h
File src/gpu/GrDrawTarget.h (right):

http://codereview.appspot.com/6349049/diff/5001/src/gpu/GrDrawTarget.h#newcod...
src/gpu/GrDrawTarget.h:447: */
On 2012/06/27 22:45:03, robertphillips wrote:
> Now you're just going out of your way to remove the parameter names

Correct :) I did have to edit the line to change & to *, though.

http://codereview.appspot.com/6349049/diff/5001/src/gpu/GrGpu.cpp
File src/gpu/GrGpu.cpp (right):

http://codereview.appspot.com/6349049/diff/5001/src/gpu/GrGpu.cpp#newcode444
src/gpu/GrGpu.cpp:444: 
On 2012/06/27 22:45:03, robertphillips wrote:
> It seems that on changing "path" from a reference to a ptr we should be
checking
> for NULL somewhere or "path" should be non-const

I'll add an assert in the GrDrawTarget top level function. I changed it from a &
to * because GrIODB needs to take a ref and hold on to the pointer until
playback. That implies that GrPath has to be heap allocated not stack allocated.
I believe we use ptrs param to convey that in Skia. This is at least true in Gr.
For example we pass const GrTexture* not const GrTexture&. We should consult
with Mike and update the style guide (and perhaps some code to reflect whatever
the actual style is).

http://codereview.appspot.com/6349049/diff/5001/src/gpu/gl/GrGpuGL.cpp
File src/gpu/gl/GrGpuGL.cpp (right):

http://codereview.appspot.com/6349049/diff/5001/src/gpu/gl/GrGpuGL.cpp#newcod...
src/gpu/gl/GrGpuGL.cpp:1329: GrAssert(fCaps.fPathStencilingSupport);
On 2012/06/27 22:45:03, robertphillips wrote:
> collapse the following two lines?

will do

http://codereview.appspot.com/6349049/diff/5001/src/gpu/gl/GrGpuGL_program.cpp
File src/gpu/gl/GrGpuGL_program.cpp (right):

http://codereview.appspot.com/6349049/diff/5001/src/gpu/gl/GrGpuGL_program.cp...
src/gpu/gl/GrGpuGL_program.cpp:478: 
On 2012/06/27 22:45:03, robertphillips wrote:
> Indentation seems a bit weird here

will fix.

[robertphillips, 2012-06-28 12:48:42]

http://codereview.appspot.com/6349049/diff/5001/src/gpu/GrGpu.cpp
File src/gpu/GrGpu.cpp (right):

http://codereview.appspot.com/6349049/diff/5001/src/gpu/GrGpu.cpp#newcode444
src/gpu/GrGpu.cpp:444: 
It makes sense to me now - I think it is fine. I still think the check in
GrDrawTarget would be a good idea though.

[bsalomon, 2012-06-28 16:48:20]

New patch addresses comments.

http://codereview.appspot.com/6349049/diff/5001/src/gpu/gl/GrGpuGL.h
File src/gpu/gl/GrGpuGL.h (right):

http://codereview.appspot.com/6349049/diff/5001/src/gpu/gl/GrGpuGL.h#newcode371
src/gpu/gl/GrGpuGL.h:371: fViewMatrix = GrMatrix::InvalidMatrix();
On 2012/06/27 22:45:03, robertphillips wrote:
> Set fHeight to -1 too?
Added a comment

[robertphillips, 2012-06-28 16:57:00]

LGTM - ignore the SkNew nit if you like.

http://codereview.appspot.com/6349049/diff/6004/src/gpu/gl/GrGpuGL.cpp
File src/gpu/gl/GrGpuGL.cpp (right):

http://codereview.appspot.com/6349049/diff/6004/src/gpu/gl/GrGpuGL.cpp#newcod...
src/gpu/gl/GrGpuGL.cpp:1329: GrAssert(fCaps.fPathStencilingSupport);
SKNEW?

[bsalomon, 2012-06-28 18:41:32]

On 2012/06/28 16:57:00, robertphillips wrote:
> LGTM - ignore the SkNew nit if you like.
> 
> http://codereview.appspot.com/6349049/diff/6004/src/gpu/gl/GrGpuGL.cpp
> File src/gpu/gl/GrGpuGL.cpp (right):
> 
>
http://codereview.appspot.com/6349049/diff/6004/src/gpu/gl/GrGpuGL.cpp#newcod...
> src/gpu/gl/GrGpuGL.cpp:1329: GrAssert(fCaps.fPathStencilingSupport);
> SKNEW?

Since we aren't really wrapping new at all in the gpu code I'll file a Skia
Issue for it.

[bsalomon, 2012-06-29 13:58:33]

Closed with r4390