Implement path cover with nv_path_rendering

Implement filling a path with nv_path_rendering cover

Implement filling a path with nv_path_rendering cover functionality.

The nv_path_rendering cover can be used if the fill is non-inverted
and the draw operation does not require use of vertex shaders.

Moves code for the inverted fill from GrStencilAndCoverPathRenderer
down to GrGpuGL.

Committed: http://code.google.com/p/skia/source/detail?r=11667

[Kimmo Kinnunen, 2013-08-08 12:46:27]

Brian, would it be possible for you to give early feedback on how to implement
the path cover with nv_path_rendering?

Here's what I have now, but I'm not sure it's factored in the spirit of the
existing code..

[bsalomon, 2013-08-08 21:32:25]

On 2013/08/08 12:46:27, kkinnunen wrote:
> Brian, would it be possible for you to give early feedback on how to implement
> the path cover with nv_path_rendering?
> 
> Here's what I have now, but I'm not sure it's factored in the spirit of the
> existing code..

Hi Kimmo, I'm still groking and will have more feedback tomorrow (sorry been a
busy day with most of the day at an offsite event). One initial suggestion: Can
you put up the uniform handle refactor as a separate change? It seems like a
nice improvement in its own right and it will make it easier to review and
detect any regressions if we tease some of this apart.

[Kimmo Kinnunen, 2013-08-09 13:11:51]

The refactor patch is separate here:
https://codereview.chromium.org/22340010/

Somehow I can't upload a dependent patch to that patch, so maybe that needs to
be merged first..

[Kimmo Kinnunen, 2013-08-14 12:18:12]

Depends on https://codereview.chromium.org/22340010/

[Kimmo Kinnunen, 2013-09-13 12:42:00]

Not sure if this is at all the correct approach.
Depends on https://chromiumcodereview.appspot.com/23537028

[Mark Kilgard, 2013-09-13 15:42:23]

Nothing wrong with the code (looks good to me) but I've made some forwarded
looking comments...

I like seeing GrGpuGL::onGpuDrawPath doing both the stencil and cover commands
in the same function.  Nice!

General comment:  How can we get NVpr stroking working?  Today, I think (my
understand this incorrectly) strokes are being converted to fills and then being
filled.  That's quite a bit more inefficient than using the first-class
stroking.

https://chromiumcodereview.appspot.com/22686002/diff/32001/src/gpu/GrDrawTarg...
File src/gpu/GrDrawTarget.h (right):

https://chromiumcodereview.appspot.com/22686002/diff/32001/src/gpu/GrDrawTarg...
src/gpu/GrDrawTarget.h:330: /**
Kimmo, I notice other method explanation comments wrap to a narrower line width
which may be the Skia code style convention.

https://chromiumcodereview.appspot.com/22686002/diff/32001/src/gpu/GrDrawTarg...
src/gpu/GrDrawTarget.h:335: void drawPath(const GrPath*, const SkStrokeRec&
stroke, SkPath::FillType fill);
Really drawPath is filling the path so does fillPath make more sense here?

Perhaps it should be named that way?

For another commit...

I'd like to see a similar strokePath version so we can do
glStencilStrokePathNV/glCoverStrokePathNV similarly.

https://chromiumcodereview.appspot.com/22686002/diff/32001/src/gpu/gl/GrGpuGL...
File src/gpu/gl/GrGpuGL.cpp (right):

https://chromiumcodereview.appspot.com/22686002/diff/32001/src/gpu/gl/GrGpuGL...
src/gpu/gl/GrGpuGL.cpp:1732: if (NULL ==
drawState->getRenderTarget()->getStencilBuffer()) {
Do we really expect to get here without a stencil buffer?

Could we simply assert that getStencilBuffer() returns NULL here?

I think you'd want to notice with an assert or bad drawing that you got here
accidently.

If we did the actual glStencilFillPathNV/glCoverFillPathNV and there was no
stencil buffer, neither command would generate an OpenGL error.  But the cover
would render the conservative cover geometry (there would be no stencil buffer
so the stencil test would never fail).  This is honestly probably what you (as a
developer) want so you'll notice "hey, this isn't what I expect; I wonder if I
don't have a stencil buffer!"

https://chromiumcodereview.appspot.com/22686002/diff/32001/src/gpu/gl/GrGpuGL...
src/gpu/gl/GrGpuGL.cpp:1742: } else {
Is there a reason we would ever want to get into this else clause?

I'm not clear what it would be.

Is this really the "old" (pre-glCoverFillPathNV code) that could be deleted now?

[Chris Dalton, 2013-09-13 19:49:58]

Looks great! Apart from the comments I'd just suggest that we rename
GrDrawTargetCaps::pathStencilingSupport to pathRenderingSupport.

Also, maybe for the future we can consider renaming
GrStencilAndCoverPathRenderer to something that indicates it's really just an
nvpr path renderer. Brian, thoughts?

And as an FYI, we're not planning to keep the vertexless patch this is based on
anymore. But it shouldn't make any difference to you because either way your
patch will be the same. Brian/Kimmo, how do you feel about submitting the parent
patch as-is so Kimmo can continue working on path rendering while I finish
cleaning up the vertexless stuff?

https://chromiumcodereview.appspot.com/22686002/diff/32001/src/gpu/GrDrawTarg...
File src/gpu/GrDrawTarget.h (right):

https://chromiumcodereview.appspot.com/22686002/diff/32001/src/gpu/GrDrawTarg...
src/gpu/GrDrawTarget.h:330: /**
On 2013/09/13 15:42:24, Mark Kilgard wrote:
> Kimmo, I notice other method explanation comments wrap to a narrower line
width
> which may be the Skia code style convention.

About a year ago the convention changed from 80 lines to 100. However, if you're
in a file that still wraps at 80, they want you to either wrap your new lines at
80, or promote all the surrounding code to 100.

Here's the style guide:

https://sites.google.com/site/skiadocs/developer-documentation/contributing-c...

https://chromiumcodereview.appspot.com/22686002/diff/32001/src/gpu/GrStencilA...
File src/gpu/GrStencilAndCoverPathRenderer.cpp (right):

https://chromiumcodereview.appspot.com/22686002/diff/32001/src/gpu/GrStencilA...
src/gpu/GrStencilAndCoverPathRenderer.cpp:43:
!target->getDrawState().willEffectReadDstColor() && // not supported yet
I don't think we need this constraint anymore. With the vertexless shaders
everything should "just work".

https://chromiumcodereview.appspot.com/22686002/diff/32001/src/gpu/gl/GrGpuGL...
File src/gpu/gl/GrGpuGL.cpp (right):

https://chromiumcodereview.appspot.com/22686002/diff/32001/src/gpu/gl/GrGpuGL...
src/gpu/gl/GrGpuGL.cpp:1741: GL_CALL(CoverFillPath(id, GR_GL_CONVEX_HULL));
I'd suggest using BOUNDING_BOX here.

Mark, thoughts?

https://chromiumcodereview.appspot.com/22686002/diff/32001/src/gpu/gl/GrGpuGL...
src/gpu/gl/GrGpuGL.cpp:1742: } else {
On 2013/09/13 15:42:24, Mark Kilgard wrote:
> Is there a reason we would ever want to get into this else clause?
> 
> I'm not clear what it would be.
> 
> Is this really the "old" (pre-glCoverFillPathNV code) that could be deleted
now?

Yes, the contract is that every time we draw a path there is no vertex shader.
This block of code should be very simple:

  SkASSERT(!fStencilSettings.isTwoSided());
  SkAssert(!fCurrentProgram->hasVertexShader());
  GrGLenum fillMode = sk_path_fill_type_to_gl_fill_mode(fill);
  GrGLint writeMask =
fStencilPathSettings.writeMask(GrStencilSettings::kFront_Face);
  GL_CALL(StencilFillPath(id, fillMode, writeMask));
  GL_CALL(CoverFillPath(id, GR_GL_BOUNDING_BOX));

https://chromiumcodereview.appspot.com/22686002/diff/32001/src/gpu/gl/GrGpuGL...
File src/gpu/gl/GrGpuGL_program.cpp (right):

https://chromiumcodereview.appspot.com/22686002/diff/32001/src/gpu/gl/GrGpuGL...
src/gpu/gl/GrGpuGL_program.cpp:218: if (kStencilPath_DrawType != type) {
Kimmo, we should be able to leave this function as-is, except perhaps for an
assert:

  SkASSERT(kDrawPath_DrawType != type || !fCurrentProgram->hasVertexShader());

When there isn't a vertex shader, fCurrentProgram->setData updates the
projection matrix already.

[bsalomon, 2013-09-13 20:24:04]

On 2013/09/13 19:49:58, cdalton wrote:
> Looks great! Apart from the comments I'd just suggest that we rename
> GrDrawTargetCaps::pathStencilingSupport to pathRenderingSupport.

Agreed, also agree with Mark that if we're headed towards separate fill and
stroke path methods then we probably want fillPath/onFillPath etc now.

> 
> Also, maybe for the future we can consider renaming
> GrStencilAndCoverPathRenderer to something that indicates it's really just an
> nvpr path renderer. Brian, thoughts?

Let's hold off... we may wind up just removing it entirely and skipping
GrPathRenderer entirely at a higher level when NVPR is around.

> 
> And as an FYI, we're not planning to keep the vertexless patch this is based
on
> anymore. But it shouldn't make any difference to you because either way your
> patch will be the same. Brian/Kimmo, how do you feel about submitting the
parent
> patch as-is so Kimmo can continue working on path rendering while I finish
> cleaning up the vertexless stuff?

SGTM

[bsalomon, 2013-09-13 20:25:27]

https://chromiumcodereview.appspot.com/22686002/diff/32001/src/gpu/GrDrawState.h
File src/gpu/GrDrawState.h (right):

https://chromiumcodereview.appspot.com/22686002/diff/32001/src/gpu/GrDrawStat...
src/gpu/GrDrawState.h:940: GrStencilSettings     fPathStencilSettings;
Is this necessary? I'm wondering if we could have something lighter weight like
the a fill type.

https://chromiumcodereview.appspot.com/22686002/diff/32001/src/gpu/GrDrawTarg...
File src/gpu/GrDrawTarget.h (right):

https://chromiumcodereview.appspot.com/22686002/diff/32001/src/gpu/GrDrawTarg...
src/gpu/GrDrawTarget.h:330: /**
On 2013/09/13 19:49:58, cdalton wrote:
> On 2013/09/13 15:42:24, Mark Kilgard wrote:
> > Kimmo, I notice other method explanation comments wrap to a narrower line
> width
> > which may be the Skia code style convention.
> 
> About a year ago the convention changed from 80 lines to 100. However, if
you're
> in a file that still wraps at 80, they want you to either wrap your new lines
at
> 80, or promote all the surrounding code to 100.
> 
> Here's the style guide:
> 
>
https://sites.google.com/site/skiadocs/developer-documentation/contributing-c...

In practice it's not quite so strict as whole-file at 80 or 100 it would
probably look better if this matched the other method comments.

https://chromiumcodereview.appspot.com/22686002/diff/32001/src/gpu/GrInOrderD...
File src/gpu/GrInOrderDrawBuffer.h (right):

https://chromiumcodereview.appspot.com/22686002/diff/32001/src/gpu/GrInOrderD...
src/gpu/GrInOrderDrawBuffer.h:184: DrawPath*      recordDrawPath();
space

https://chromiumcodereview.appspot.com/22686002/diff/32001/src/gpu/GrInOrderD...
src/gpu/GrInOrderDrawBuffer.h:193: kDrawPathPreallocCnt    = 8,
space

https://chromiumcodereview.appspot.com/22686002/diff/32001/src/gpu/GrStencilA...
File src/gpu/GrStencilAndCoverPathRenderer.cpp (right):

https://chromiumcodereview.appspot.com/22686002/diff/32001/src/gpu/GrStencilA...
src/gpu/GrStencilAndCoverPathRenderer.cpp:43:
!target->getDrawState().willEffectReadDstColor() && // not supported yet
On 2013/09/13 19:49:58, cdalton wrote:
> I don't think we need this constraint anymore. With the vertexless shaders
> everything should "just work".

Really? I think onDrawPath will have to handle this.

Of course we should also add support for advanced blending.. though there may
still be effects that require dst access so it's not a replacement for adding
support for willEffectReadDstColor().

https://chromiumcodereview.appspot.com/22686002/diff/32001/src/gpu/gl/GrGpuGL...
File src/gpu/gl/GrGpuGL.cpp (right):

https://chromiumcodereview.appspot.com/22686002/diff/32001/src/gpu/gl/GrGpuGL...
src/gpu/gl/GrGpuGL.cpp:1732: if (NULL ==
drawState->getRenderTarget()->getStencilBuffer()) {
On 2013/09/13 15:42:24, Mark Kilgard wrote:
> Do we really expect to get here without a stencil buffer?
> 
> Could we simply assert that getStencilBuffer() returns NULL here?
> 
> I think you'd want to notice with an assert or bad drawing that you got here
> accidently.
> 
> If we did the actual glStencilFillPathNV/glCoverFillPathNV and there was no
> stencil buffer, neither command would generate an OpenGL error.  But the cover
> would render the conservative cover geometry (there would be no stencil buffer
> so the stencil test would never fail).  This is honestly probably what you (as
a
> developer) want so you'll notice "hey, this isn't what I expect; I wonder if I
> don't have a stencil buffer!"

It's not illegal for a caller to give us an FBO without a stencil to draw into.
But maybe this should be handled in GrStencilAndCoverPathRenderer::canDrawPath
and then an assertion here.

https://chromiumcodereview.appspot.com/22686002/diff/32001/src/gpu/gl/GrGpuGL...
src/gpu/gl/GrGpuGL.cpp:1742: } else {
On 2013/09/13 19:49:58, cdalton wrote:
> On 2013/09/13 15:42:24, Mark Kilgard wrote:
> > Is there a reason we would ever want to get into this else clause?
> > 
> > I'm not clear what it would be.
> > 
> > Is this really the "old" (pre-glCoverFillPathNV code) that could be deleted
> now?
> 
> Yes, the contract is that every time we draw a path there is no vertex shader.
> This block of code should be very simple:
> 
>   SkASSERT(!fStencilSettings.isTwoSided());
>   SkAssert(!fCurrentProgram->hasVertexShader());
>   GrGLenum fillMode = sk_path_fill_type_to_gl_fill_mode(fill);
>   GrGLint writeMask =
> fStencilPathSettings.writeMask(GrStencilSettings::kFront_Face);
>   GL_CALL(StencilFillPath(id, fillMode, writeMask));
>   GL_CALL(CoverFillPath(id, GR_GL_BOUNDING_BOX));

agreed

[Kimmo Kinnunen, 2013-09-18 07:52:38]

https://chromiumcodereview.appspot.com/22686002/diff/32001/src/gpu/GrDrawState.h
File src/gpu/GrDrawState.h (right):

https://chromiumcodereview.appspot.com/22686002/diff/32001/src/gpu/GrDrawStat...
src/gpu/GrDrawState.h:940: GrStencilSettings     fPathStencilSettings;
On 2013/09/13 20:25:28, bsalomon wrote:
> Is this necessary? I'm wondering if we could have something lighter weight
like
> the a fill type.

Done.

https://chromiumcodereview.appspot.com/22686002/diff/32001/src/gpu/GrDrawTarg...
File src/gpu/GrDrawTarget.h (right):

https://chromiumcodereview.appspot.com/22686002/diff/32001/src/gpu/GrDrawTarg...
src/gpu/GrDrawTarget.h:330: /**
On 2013/09/13 20:25:28, bsalomon wrote:
> On 2013/09/13 19:49:58, cdalton wrote:
> > On 2013/09/13 15:42:24, Mark Kilgard wrote:
> > > Kimmo, I notice other method explanation comments wrap to a narrower line
> > width
> > > which may be the Skia code style convention.
> > 
> > About a year ago the convention changed from 80 lines to 100. However, if
> you're
> > in a file that still wraps at 80, they want you to either wrap your new
lines
> at
> > 80, or promote all the surrounding code to 100.
> > 
> > Here's the style guide:
> > 
> >
>
https://sites.google.com/site/skiadocs/developer-documentation/contributing-c...
> 
> In practice it's not quite so strict as whole-file at 80 or 100 it would
> probably look better if this matched the other method comments.

Done.

https://chromiumcodereview.appspot.com/22686002/diff/32001/src/gpu/GrDrawTarg...
src/gpu/GrDrawTarget.h:335: void drawPath(const GrPath*, const SkStrokeRec&
stroke, SkPath::FillType fill);
On 2013/09/13 15:42:24, Mark Kilgard wrote:
> Really drawPath is filling the path so does fillPath make more sense here?

The fillPath does not seem to differ that significantly from strokePath to
justify the extra boilerplate that would be needed IMO.

I'll call this fillPath now and then rename the function to drawPath in the
later commit, just to pre-empt discussing this comment now.

https://chromiumcodereview.appspot.com/22686002/diff/32001/src/gpu/GrInOrderD...
File src/gpu/GrInOrderDrawBuffer.h (right):

https://chromiumcodereview.appspot.com/22686002/diff/32001/src/gpu/GrInOrderD...
src/gpu/GrInOrderDrawBuffer.h:184: DrawPath*      recordDrawPath();
On 2013/09/13 20:25:28, bsalomon wrote:
> space

Done.

https://chromiumcodereview.appspot.com/22686002/diff/32001/src/gpu/GrInOrderD...
src/gpu/GrInOrderDrawBuffer.h:193: kDrawPathPreallocCnt    = 8,
On 2013/09/13 20:25:28, bsalomon wrote:
> space

Done.

https://chromiumcodereview.appspot.com/22686002/diff/32001/src/gpu/GrStencilA...
File src/gpu/GrStencilAndCoverPathRenderer.cpp (right):

https://chromiumcodereview.appspot.com/22686002/diff/32001/src/gpu/GrStencilA...
src/gpu/GrStencilAndCoverPathRenderer.cpp:43:
!target->getDrawState().willEffectReadDstColor() && // not supported yet
On 2013/09/13 19:49:58, cdalton wrote:
> I don't think we need this constraint anymore. With the vertexless shaders
> everything should "just work".

I don't think there's code for this yet.. Left as is for now.

https://chromiumcodereview.appspot.com/22686002/diff/32001/src/gpu/gl/GrGpuGL...
File src/gpu/gl/GrGpuGL.cpp (right):

https://chromiumcodereview.appspot.com/22686002/diff/32001/src/gpu/gl/GrGpuGL...
src/gpu/gl/GrGpuGL.cpp:1732: if (NULL ==
drawState->getRenderTarget()->getStencilBuffer()) {
On 2013/09/13 20:25:28, bsalomon wrote:
> On 2013/09/13 15:42:24, Mark Kilgard wrote:
> > Do we really expect to get here without a stencil buffer?
> > 
> > Could we simply assert that getStencilBuffer() returns NULL here?
> > 
> > I think you'd want to notice with an assert or bad drawing that you got here
> > accidently.
> > 
> > If we did the actual glStencilFillPathNV/glCoverFillPathNV and there was no
> > stencil buffer, neither command would generate an OpenGL error.  But the
cover
> > would render the conservative cover geometry (there would be no stencil
buffer
> > so the stencil test would never fail).  This is honestly probably what you
(as
> a
> > developer) want so you'll notice "hey, this isn't what I expect; I wonder if
I
> > don't have a stencil buffer!"
> 
> It's not illegal for a caller to give us an FBO without a stencil to draw
into.
> But maybe this should be handled in GrStencilAndCoverPathRenderer::canDrawPath
> and then an assertion here.

Done.

https://chromiumcodereview.appspot.com/22686002/diff/32001/src/gpu/gl/GrGpuGL...
src/gpu/gl/GrGpuGL.cpp:1741: GL_CALL(CoverFillPath(id, GR_GL_CONVEX_HULL));
On 2013/09/13 19:49:58, cdalton wrote:
> I'd suggest using BOUNDING_BOX here.
> 
> Mark, thoughts?

Done.

https://chromiumcodereview.appspot.com/22686002/diff/32001/src/gpu/gl/GrGpuGL...
src/gpu/gl/GrGpuGL.cpp:1742: } else {
On 2013/09/13 19:49:58, cdalton wrote:
> On 2013/09/13 15:42:24, Mark Kilgard wrote:
> > Is there a reason we would ever want to get into this else clause?
> Yes, the contract is that every time we draw a path there is no vertex shader.


I'm not sure I follow. I thought an effect can force the use of vertex shaders.

If I understand correctly, currently resolving the need for vertex shader is
done at the program flush stage. At this point the algorithm to draw the path
has already been selected.

https://chromiumcodereview.appspot.com/22686002/diff/32001/src/gpu/gl/GrGpuGL...
File src/gpu/gl/GrGpuGL_program.cpp (right):

https://chromiumcodereview.appspot.com/22686002/diff/32001/src/gpu/gl/GrGpuGL...
src/gpu/gl/GrGpuGL_program.cpp:218: if (kStencilPath_DrawType != type) {
On 2013/09/13 19:49:58, cdalton wrote:
> Kimmo, we should be able to leave this function as-is, except perhaps for an
> assert:
> 
>   SkASSERT(kDrawPath_DrawType != type || !fCurrentProgram->hasVertexShader());
> 
> When there isn't a vertex shader, fCurrentProgram->setData updates the
> projection matrix already.

But sometimes gpu draw path is selected when the program has vertex shader. We
will still stencil the path with gpu path stencil func. This needs the proper
projection matrix.

[bsalomon, 2013-09-18 14:24:21]

https://chromiumcodereview.appspot.com/22686002/diff/32001/src/gpu/gl/GrGpuGL...
File src/gpu/gl/GrGpuGL.cpp (right):

https://chromiumcodereview.appspot.com/22686002/diff/32001/src/gpu/gl/GrGpuGL...
src/gpu/gl/GrGpuGL.cpp:1742: } else {
On 2013/09/18 07:52:38, kkinnunen wrote:
> On 2013/09/13 19:49:58, cdalton wrote:
> > On 2013/09/13 15:42:24, Mark Kilgard wrote:
> > > Is there a reason we would ever want to get into this else clause?
> > Yes, the contract is that every time we draw a path there is no vertex
shader.
> 
> 
> I'm not sure I follow. I thought an effect can force the use of vertex
shaders.
> 
> If I understand correctly, currently resolving the need for vertex shader is
> done at the program flush stage. At this point the algorithm to draw the path
> has already been selected.

Effects that require vertex shaders are only installed internally (that is below
GrContext). This will become actually enforced by the refactor Chris is doing
but it is true in practice today. So if we chose to use NVPR then we shouldn't
have an effect that requires a VS. The effects that require a VS implement
alternative ways of doing anti-aliasing in the absence of MSAA.

https://chromiumcodereview.appspot.com/22686002/diff/47001/src/gpu/GrDrawState.h
File src/gpu/GrDrawState.h (right):

https://chromiumcodereview.appspot.com/22686002/diff/47001/src/gpu/GrDrawStat...
src/gpu/GrDrawState.h:733: SkPath::FillType* pathStencilFillType() {
Is this really needed? Skia very rarely returns pointers to members (that are
not themselves pointers).

https://chromiumcodereview.appspot.com/22686002/diff/47001/src/gpu/GrGpu.cpp
File src/gpu/GrGpu.cpp (right):

https://chromiumcodereview.appspot.com/22686002/diff/47001/src/gpu/GrGpu.cpp#...
src/gpu/GrGpu.cpp:354: GrAutoTRestore<SkPath::FillType>
apsr(this->drawState()->pathStencilFillType());
Now I see why you added the ptr getter. :/

Maybe we don't need to add anything to the draw state at all? Could we just pass
the fill type to stencilPath/onStencilPath drawPath/onDrawPath and just document
that it is used to override the stencil settings on the GrDrawState?

https://chromiumcodereview.appspot.com/22686002/diff/47001/src/gpu/GrGpu.h
File src/gpu/GrGpu.h (right):

https://chromiumcodereview.appspot.com/22686002/diff/47001/src/gpu/GrGpu.h#ne...
src/gpu/GrGpu.h:343: virtual const GrStencilSettings&
getPathStencilSettingsForFillType(SkPath::FillType) = 0;
I wonder if this needs to be virtual. It seems like if D3D or some other API had
path stenciling the rules would probably be the same. Maybe we can make it a
static func on GrGpu for now and virtualize only if a need arises?

[Kimmo Kinnunen, 2013-09-20 12:20:58]

Thanks for the feedback, I'll see what I can come up with.

https://chromiumcodereview.appspot.com/22686002/diff/47001/src/gpu/GrGpu.cpp
File src/gpu/GrGpu.cpp (right):

https://chromiumcodereview.appspot.com/22686002/diff/47001/src/gpu/GrGpu.cpp#...
src/gpu/GrGpu.cpp:354: GrAutoTRestore<SkPath::FillType>
apsr(this->drawState()->pathStencilFillType());
On 2013/09/18 14:24:22, bsalomon wrote:
> Now I see why you added the ptr getter. :/
> 
> Maybe we don't need to add anything to the draw state at all? Could we just
pass
> the fill type to stencilPath/onStencilPath drawPath/onDrawPath and just
document
> that it is used to override the stencil settings on the GrDrawState?

I try to see if I can figure out a way. I was struggling to maintain the
structure where state modifications would happen in the "flush" methods. The
state modification for the path stencil happens now during stencil flush.

[Chris Dalton, 2013-09-23 02:05:59]

https://codereview.chromium.org/22686002/diff/32001/src/gpu/gl/GrGpuGL_progra...
File src/gpu/gl/GrGpuGL_program.cpp (right):

https://codereview.chromium.org/22686002/diff/32001/src/gpu/gl/GrGpuGL_progra...
src/gpu/gl/GrGpuGL_program.cpp:218: if (kStencilPath_DrawType != type) {
On 2013/09/18 07:52:38, kkinnunen wrote:
> On 2013/09/13 19:49:58, cdalton wrote:
> > Kimmo, we should be able to leave this function as-is, except perhaps for an
> > assert:
> > 
> >   SkASSERT(kDrawPath_DrawType != type ||
!fCurrentProgram->hasVertexShader());
> > 
> > When there isn't a vertex shader, fCurrentProgram->setData updates the
> > projection matrix already.
> 
> But sometimes gpu draw path is selected when the program has vertex shader. We
> will still stencil the path with gpu path stencil func. This needs the proper
> projection matrix.

When does it use kDrawPath_DrawType with a vertex shader? The intention is to
never do that, so if there are any cases where that happens we need to fix it.

[Chris Dalton, 2013-10-07 22:22:45]

A few minor comments. The rest looks good to me.

https://codereview.chromium.org/22686002/diff/72001/src/gpu/GrDrawState.h
File src/gpu/GrDrawState.h (right):

https://codereview.chromium.org/22686002/diff/72001/src/gpu/GrDrawState.h#new...
src/gpu/GrDrawState.h:25: #include "SkPath.h"
Is this needed in the .h file?

https://codereview.chromium.org/22686002/diff/72001/src/gpu/GrGpu.h
File src/gpu/GrGpu.h (right):

https://codereview.chromium.org/22686002/diff/72001/src/gpu/GrGpu.h#newcode496
src/gpu/GrGpu.h:496: SkPath::FillType) SK_OVERRIDE;
I think the skia preference is to put each individual argument on its own line,
and them to all be aligned on the same column.

https://codereview.chromium.org/22686002/diff/72001/src/gpu/GrStencilAndCover...
File src/gpu/GrStencilAndCoverPathRenderer.cpp (right):

https://codereview.chromium.org/22686002/diff/72001/src/gpu/GrStencilAndCover...
src/gpu/GrStencilAndCoverPathRenderer.cpp:43:
!target->getDrawState().willEffectReadDstColor() && // not supported yet
I'm trying to understand why reading the dst color wouldn't be supported.

Doesn't the shader builder just add some fragment code and uniforms to look the
color up? AFAICT, this works without a vertex shader.

https://codereview.chromium.org/22686002/diff/72001/src/gpu/gl/GrGpuGL.cpp
File src/gpu/gl/GrGpuGL.cpp (right):

https://codereview.chromium.org/22686002/diff/72001/src/gpu/gl/GrGpuGL.cpp#ne...
src/gpu/gl/GrGpuGL.cpp:360: fHWPathStencilSettings.invalidate();
It's a little confusing that kPathRendering_GrGLBackendState doesn't reset
PathTexGen.

Eventually we'll probably want to maintain PathTexGen and TexGen separately, and
maybe pass a DrawType to enableTexGen(). That way we can just reset PathTexGen
here instead of in the fixed function block.

https://codereview.chromium.org/22686002/diff/72001/src/gpu/gl/GrGpuGL.cpp#ne...
src/gpu/gl/GrGpuGL.cpp:372: if (resetBits & kFixedFunction_GrGLBackendState &&
this->glCaps().fixedFunctionSupport()) {
Maybe for now we can just mix path rendering and fixed function into the same
block?

(It's impossible to have path rendering without fixed function)

[Kimmo Kinnunen, 2013-10-08 12:14:09]

Thanks for the review, new version is up.

https://chromiumcodereview.appspot.com/22686002/diff/72001/src/gpu/GrDrawState.h
File src/gpu/GrDrawState.h (right):

https://chromiumcodereview.appspot.com/22686002/diff/72001/src/gpu/GrDrawStat...
src/gpu/GrDrawState.h:25: #include "SkPath.h"
On 2013/10/07 22:22:45, Chris Dalton wrote:
> Is this needed in the .h file?

Done.

https://chromiumcodereview.appspot.com/22686002/diff/72001/src/gpu/GrGpu.h
File src/gpu/GrGpu.h (right):

https://chromiumcodereview.appspot.com/22686002/diff/72001/src/gpu/GrGpu.h#ne...
src/gpu/GrGpu.h:496: SkPath::FillType) SK_OVERRIDE;
On 2013/10/07 22:22:45, Chris Dalton wrote:
> I think the skia preference is to put each individual argument on its own
line,
> and them to all be aligned on the same column.

Done.

https://chromiumcodereview.appspot.com/22686002/diff/72001/src/gpu/GrStencilA...
File src/gpu/GrStencilAndCoverPathRenderer.cpp (right):

https://chromiumcodereview.appspot.com/22686002/diff/72001/src/gpu/GrStencilA...
src/gpu/GrStencilAndCoverPathRenderer.cpp:43:
!target->getDrawState().willEffectReadDstColor() && // not supported yet
On 2013/10/07 22:22:45, Chris Dalton wrote:
> I'm trying to understand why reading the dst color wouldn't be supported.
> 
> Doesn't the shader builder just add some fragment code and uniforms to look
the
> color up? AFAICT, this works without a vertex shader.

True, should've mentioned in commit msg that it is broken in other places.

I don't know if the fix is right.

https://chromiumcodereview.appspot.com/22686002/diff/72001/src/gpu/gl/GrGpuGL...
File src/gpu/gl/GrGpuGL.cpp (right):

https://chromiumcodereview.appspot.com/22686002/diff/72001/src/gpu/gl/GrGpuGL...
src/gpu/gl/GrGpuGL.cpp:360: fHWPathStencilSettings.invalidate();
On 2013/10/07 22:22:45, Chris Dalton wrote:
> It's a little confusing that kPathRendering_GrGLBackendState doesn't reset
> PathTexGen.
> 
> Eventually we'll probably want to maintain PathTexGen and TexGen separately,
and
> maybe pass a DrawType to enableTexGen(). That way we can just reset PathTexGen
> here instead of in the fixed function block.

Done.

[bsalomon, 2013-10-08 14:05:08]

mostly looks good but I'd like to understand the dst copy problem.

https://chromiumcodereview.appspot.com/22686002/diff/32001/src/gpu/gl/GrGpuGL...
File src/gpu/gl/GrGpuGL_program.cpp (right):

https://chromiumcodereview.appspot.com/22686002/diff/32001/src/gpu/gl/GrGpuGL...
src/gpu/gl/GrGpuGL_program.cpp:218: if (kStencilPath_DrawType != type) {
On 2013/09/23 02:05:59, cdalton wrote:
> On 2013/09/18 07:52:38, kkinnunen wrote:
> > On 2013/09/13 19:49:58, cdalton wrote:
> > > Kimmo, we should be able to leave this function as-is, except perhaps for
an
> > > assert:
> > > 
> > >   SkASSERT(kDrawPath_DrawType != type ||
> !fCurrentProgram->hasVertexShader());
> > > 
> > > When there isn't a vertex shader, fCurrentProgram->setData updates the
> > > projection matrix already.
> > 
> > But sometimes gpu draw path is selected when the program has vertex shader.
We
> > will still stencil the path with gpu path stencil func. This needs the
proper
> > projection matrix.
> 
> When does it use kDrawPath_DrawType with a vertex shader? The intention is to
> never do that, so if there are any cases where that happens we need to fix it.
agreed.

https://chromiumcodereview.appspot.com/22686002/diff/72001/src/gpu/GrStencilA...
File src/gpu/GrStencilAndCoverPathRenderer.cpp (right):

https://chromiumcodereview.appspot.com/22686002/diff/72001/src/gpu/GrStencilA...
src/gpu/GrStencilAndCoverPathRenderer.cpp:43:
!target->getDrawState().willEffectReadDstColor() && // not supported yet
On 2013/10/08 12:14:09, kkinnunen wrote:
> On 2013/10/07 22:22:45, Chris Dalton wrote:
> > I'm trying to understand why reading the dst color wouldn't be supported.
> > 
> > Doesn't the shader builder just add some fragment code and uniforms to look
> the
> > color up? AFAICT, this works without a vertex shader.
> 
> True, should've mentioned in commit msg that it is broken in other places.
> 
> I don't know if the fix is right.

Do you mean there are cases where requiring the dst color in an effect causes us
to draw incorrectly?

https://chromiumcodereview.appspot.com/22686002/diff/81001/src/gpu/GrDrawTarg...
File src/gpu/GrDrawTarget.h (right):

https://chromiumcodereview.appspot.com/22686002/diff/81001/src/gpu/GrDrawTarg...
src/gpu/GrDrawTarget.h:855: return setupDstReadIfNecessary(&info->fDstCopy,
info->getDevBounds());
nit: this->setup...

https://chromiumcodereview.appspot.com/22686002/diff/81001/src/gpu/GrGpu.cpp
File src/gpu/GrGpu.cpp (right):

https://chromiumcodereview.appspot.com/22686002/diff/81001/src/gpu/GrGpu.cpp#...
src/gpu/GrGpu.cpp:250: namespace {
We were using anonymous namespaces in Gr code, but now we've adopted the style
of the rest of the Sk code and are using static instead (and slowly replacing
existing instances of anonymous namespace).

[Kimmo Kinnunen, 2013-10-09 07:07:01]

On 2013/10/08 14:05:08, bsalomon wrote:
> mostly looks good but I'd like to understand the dst copy problem.
> 

The problem: when using dst copy it would crash when accessing clip.

Eg. while replaying the recording, GrInOrderDrawBuffer would call
GrDrawTarget::fillPath(). This would setup the dstcopy, constraining it to the
clip. However, the dstcopy code assumes that the clip is always there, even
though the draw is not really in draw state. This works when not using
GrInOrderDrawBuffer and when recording to it, but not when replaying the
recording. GrInOrderDrawBuffer only records clip when draw is in clip state.

This would cause null ptr ref GrDrawTarget::setupDstReadIfNecessary while using
fClip.

The second-to last patch fixed this by copying the whole surface when the draw
state was not in clip state.

The last patch mimics the onDraw implementation by actually recording the copy
operation in the GrInOrderDrawBuffer (reffing the dstCopy), and calling
GrDrawTarget::onFillPath instead of fillPath.

https://codereview.chromium.org/22686002/diff/81001/src/gpu/GrDrawTarget.h
File src/gpu/GrDrawTarget.h (right):

https://codereview.chromium.org/22686002/diff/81001/src/gpu/GrDrawTarget.h#ne...
src/gpu/GrDrawTarget.h:855: return setupDstReadIfNecessary(&info->fDstCopy,
info->getDevBounds());
On 2013/10/08 14:05:08, bsalomon wrote:
> nit: this->setup...

Done.

https://codereview.chromium.org/22686002/diff/81001/src/gpu/GrGpu.cpp
File src/gpu/GrGpu.cpp (right):

https://codereview.chromium.org/22686002/diff/81001/src/gpu/GrGpu.cpp#newcode250
src/gpu/GrGpu.cpp:250: namespace {
On 2013/10/08 14:05:08, bsalomon wrote:
> We were using anonymous namespaces in Gr code, but now we've adopted the style
> of the rest of the Sk code and are using static instead (and slowly replacing
> existing instances of anonymous namespace).

Done.

[bsalomon, 2013-10-09 13:18:04]

On 2013/10/09 07:07:01, kkinnunen wrote:
> On 2013/10/08 14:05:08, bsalomon wrote:
> > mostly looks good but I'd like to understand the dst copy problem.
> > 
> 
> The problem: when using dst copy it would crash when accessing clip.
> 
> Eg. while replaying the recording, GrInOrderDrawBuffer would call
> GrDrawTarget::fillPath(). This would setup the dstcopy, constraining it to the
> clip. However, the dstcopy code assumes that the clip is always there, even
> though the draw is not really in draw state. This works when not using
> GrInOrderDrawBuffer and when recording to it, but not when replaying the
> recording. GrInOrderDrawBuffer only records clip when draw is in clip state.
> 
> This would cause null ptr ref GrDrawTarget::setupDstReadIfNecessary while
using
> fClip.
> 
> The second-to last patch fixed this by copying the whole surface when the draw
> state was not in clip state.
> 
> The last patch mimics the onDraw implementation by actually recording the copy
> operation in the GrInOrderDrawBuffer (reffing the dstCopy), and calling
> GrDrawTarget::onFillPath instead of fillPath.
> 
> https://codereview.chromium.org/22686002/diff/81001/src/gpu/GrDrawTarget.h
> File src/gpu/GrDrawTarget.h (right):
> 
>
https://codereview.chromium.org/22686002/diff/81001/src/gpu/GrDrawTarget.h#ne...
> src/gpu/GrDrawTarget.h:855: return setupDstReadIfNecessary(&info->fDstCopy,
> info->getDevBounds());
> On 2013/10/08 14:05:08, bsalomon wrote:
> > nit: this->setup...
> 
> Done.
> 
> https://codereview.chromium.org/22686002/diff/81001/src/gpu/GrGpu.cpp
> File src/gpu/GrGpu.cpp (right):
> 
>
https://codereview.chromium.org/22686002/diff/81001/src/gpu/GrGpu.cpp#newcode250
> src/gpu/GrGpu.cpp:250: namespace {
> On 2013/10/08 14:05:08, bsalomon wrote:
> > We were using anonymous namespaces in Gr code, but now we've adopted the
style
> > of the rest of the Sk code and are using static instead (and slowly
replacing
> > existing instances of anonymous namespace).
> 
> Done.

lgtm, will land.

[commit-bot: I haz the power, 2013-10-09 13:18:24]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/kkinnunen@nvidia.com/22686002/112001

[commit-bot: I haz the power, 2013-10-09 14:11:37]

Change committed as 11667