Beginning to refactor nvpr code

Beginning to refactor nvpr code

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/92e496f96abbd664888f0c8a7d546ab02e703bf7

[joshualitt, 2014-10-30 19:55:33]

joshualitt@google.com changed reviewers:
+ joshualitt@google.com

[joshualitt, 2014-10-30 19:55:33]

Brian, this is a first step towards cleaning this up.  I am seeing 11 gm
miscompares, all having to do with inverted fill.  When I look at the images
though, the only difference is there appears to be more anti aliasing on the
edge of the inverted shape.  I can't fathom why this might be happening but its
somehow related to going through the inorder draw buffer

[joshualitt, 2014-10-30 19:55:46]

joshualitt@google.com changed reviewers:
+ bsalomon@google.com

[bsalomon, 2014-10-30 20:11:02]

I don't see any obvious reasons that the GMs would change. Do the new images
look less correct?

Also, I'm starting to think we should just ask the NV guys for inverse fill in a
future version of the extension. We'll have to support this for a while, but
it'd be nice to not need to go through extra hoops for inverse fills and I can't
imagine that it would be hard for them to implement.

https://codereview.chromium.org/687563008/diff/20001/src/gpu/GrStencilAndCove...
File src/gpu/GrStencilAndCoverPathRenderer.cpp (right):

https://codereview.chromium.org/687563008/diff/20001/src/gpu/GrStencilAndCove...
src/gpu/GrStencilAndCoverPathRenderer.cpp:86: target->stencilPath(p,
convert_skpath_filltype(path.getFillType()));
This function is used by GrCMM to draw paths into the stencil buffer. Do we know
that it never sends inverse fills here? If it can then this change will invert
them back to regular fills. If it doesn't then I think we need to document and
assert in the GrPR base class.

https://codereview.chromium.org/687563008/diff/20001/src/gpu/GrStencilAndCove...
src/gpu/GrStencilAndCoverPathRenderer.cpp:113:
drawState->setStencil(kInvertedStencilPass);
prolly want to remove this line

https://codereview.chromium.org/687563008/diff/20001/src/gpu/gl/GrGLPathRende...
File src/gpu/gl/GrGLPathRendering.cpp (right):

https://codereview.chromium.org/687563008/diff/20001/src/gpu/gl/GrGLPathRende...
src/gpu/gl/GrGLPathRendering.cpp:187: GrGLenum fillMode =
I realize you're not done and using StencilSettings temporarily, but here is
where some of the complexity lies that a could be alleviated with a
PathStencilSettings struct. It'd know the fill mode to use instead of inferring
it from the more general stencilsettings struct.

[joshualitt, 2014-10-30 21:24:34]

https://codereview.chromium.org/687563008/diff/20001/src/gpu/GrStencilAndCove...
File src/gpu/GrStencilAndCoverPathRenderer.cpp (right):

https://codereview.chromium.org/687563008/diff/20001/src/gpu/GrStencilAndCove...
src/gpu/GrStencilAndCoverPathRenderer.cpp:86: target->stencilPath(p,
convert_skpath_filltype(path.getFillType()));
On 2014/10/30 20:11:01, bsalomon wrote:
> This function is used by GrCMM to draw paths into the stencil buffer. Do we
know
> that it never sends inverse fills here? If it can then this change will invert
> them back to regular fills. If it doesn't then I think we need to document and
> assert in the GrPR base class.

Would the SkASSERT(!path.isInverseFillType()) catch that?  I don't think we ever
stencil inverse paths because GrGLInversePathRendering had an assert for that
case.

As things stand, GrPathRendering can't be given an inverse fill because it
doesn't take a fill type anymore, it takes stencil settings.

https://codereview.chromium.org/687563008/diff/20001/src/gpu/GrStencilAndCove...
src/gpu/GrStencilAndCoverPathRenderer.cpp:113:
drawState->setStencil(kInvertedStencilPass);
On 2014/10/30 20:11:01, bsalomon wrote:
> prolly want to remove this line

Acknowledged.

https://codereview.chromium.org/687563008/diff/20001/src/gpu/gl/GrGLPathRende...
File src/gpu/gl/GrGLPathRendering.cpp (right):

https://codereview.chromium.org/687563008/diff/20001/src/gpu/gl/GrGLPathRende...
src/gpu/gl/GrGLPathRendering.cpp:187: GrGLenum fillMode =
On 2014/10/30 20:11:01, bsalomon wrote:
> I realize you're not done and using StencilSettings temporarily, but here is
> where some of the complexity lies that a could be alleviated with a
> PathStencilSettings struct. It'd know the fill mode to use instead of
inferring
> it from the more general stencilsettings struct.

Yea, I promise I will come back and work on this stuff.  I want to finish up
early clip, and then some of the cleanups associated with that code(ie, pass in
drawstate).  Then I'll come back and pass over this code.

[joshualitt, 2014-10-31 19:58:43]

On 2014/10/30 21:24:34, joshualitt wrote:
>
https://codereview.chromium.org/687563008/diff/20001/src/gpu/GrStencilAndCove...
> File src/gpu/GrStencilAndCoverPathRenderer.cpp (right):
> 
>
https://codereview.chromium.org/687563008/diff/20001/src/gpu/GrStencilAndCove...
> src/gpu/GrStencilAndCoverPathRenderer.cpp:86: target->stencilPath(p,
> convert_skpath_filltype(path.getFillType()));
> On 2014/10/30 20:11:01, bsalomon wrote:
> > This function is used by GrCMM to draw paths into the stencil buffer. Do we
> know
> > that it never sends inverse fills here? If it can then this change will
invert
> > them back to regular fills. If it doesn't then I think we need to document
and
> > assert in the GrPR base class.
> 
> Would the SkASSERT(!path.isInverseFillType()) catch that?  I don't think we
ever
> stencil inverse paths because GrGLInversePathRendering had an assert for that
> case.
> 
> As things stand, GrPathRendering can't be given an inverse fill because it
> doesn't take a fill type anymore, it takes stencil settings.
> 
>
https://codereview.chromium.org/687563008/diff/20001/src/gpu/GrStencilAndCove...
> src/gpu/GrStencilAndCoverPathRenderer.cpp:113:
> drawState->setStencil(kInvertedStencilPass);
> On 2014/10/30 20:11:01, bsalomon wrote:
> > prolly want to remove this line
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/687563008/diff/20001/src/gpu/gl/GrGLPathRende...
> File src/gpu/gl/GrGLPathRendering.cpp (right):
> 
>
https://codereview.chromium.org/687563008/diff/20001/src/gpu/gl/GrGLPathRende...
> src/gpu/gl/GrGLPathRendering.cpp:187: GrGLenum fillMode =
> On 2014/10/30 20:11:01, bsalomon wrote:
> > I realize you're not done and using StencilSettings temporarily, but here is
> > where some of the complexity lies that a could be alleviated with a
> > PathStencilSettings struct. It'd know the fill mode to use instead of
> inferring
> > it from the more general stencilsettings struct.
> 
> Yea, I promise I will come back and work on this stuff.  I want to finish up
> early clip, and then some of the cleanups associated with that code(ie, pass
in
> drawstate).  Then I'll come back and pass over this code.

Any other feedback Brian?

[bsalomon, 2014-10-31 20:06:59]

lgtm

Can you add a comment to GrPathRenderer.h/onStencilPath that the passed to this
function will never have an inverse fill?

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

https://codereview.chromium.org/687563008/diff/60001/src/gpu/GrGpu.cpp#newcod...
src/gpu/GrGpu.cpp:281: //GrDrawState::AutoRestoreStencil ars;
del this line?

[joshualitt, 2014-10-31 20:48:00]

The CQ bit was checked by joshualitt@google.com

[commit-bot: I haz the power, 2014-10-31 20:48:31]

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

[commit-bot: I haz the power, 2014-10-31 20:56:53]

Committed patchset #5 (id:80001) as 92e496f96abbd664888f0c8a7d546ab02e703bf7

[Kimmo Kinnunen, 2014-11-03 20:39:11]

kkinnunen@nvidia.com changed reviewers:
+ kkinnunen@nvidia.com

[Kimmo Kinnunen, 2014-11-03 20:39:12]

Some observations, should you need them..

https://codereview.chromium.org/687563008/diff/80001/src/gpu/GrStencilAndCove...
File src/gpu/GrStencilAndCoverPathRenderer.cpp (right):

https://codereview.chromium.org/687563008/diff/80001/src/gpu/GrStencilAndCove...
src/gpu/GrStencilAndCoverPathRenderer.cpp:113:
drawState->setStencil(kInvertedStencilPass);
So this hunk was intentionally moved from here down to path rendering
implementation.
The conceptual rationale was that since we have this abstract implementation of
general "stencil and cover path renderer", it would be conceivable that there
would be another implementation that would have the ability to fill inverted
paths. Thus the method here should be used only for NVPR, where we lack inverse
fills. Whether the lack of inverse fills is an inherent property of the STC
algorithm or not, that I'm not expert on.

The practical reason this was moved is that this causes different thing to be
recorded. E.g when recording and replaying InOrderDrawBuffer, this actually
erases the knowledge about path being drawn. Whether that is a bad thing or not,
is of course debatable. My initial thought was and is that I find it a bit
surprising, but I'm no expert and haven't really thought about it..

The actual rationale to address the above practical reason was, if  recall
correctly, to avoid a crash. The original drawSimpleRect had NULL parameter that
affected the DST copy. There was some convolution wrt the if conditions around
the subject that then triggered badness. 

The original commit is at:
https://codereview.chromium.org/22686002/

The move happened at patch #8. 
The DST copy problem is described in the late comments.

https://codereview.chromium.org/687563008/diff/80001/src/gpu/gl/GrGLPathRende...
File src/gpu/gl/GrGLPathRendering.cpp (right):

https://codereview.chromium.org/687563008/diff/80001/src/gpu/gl/GrGLPathRende...
src/gpu/gl/GrGLPathRendering.cpp:169: if (stroke.isFillStyle() ||
SkStrokeRec::kStrokeAndFill_Style == stroke.getStyle()) {
So I guess this is not part of "refactoring", rather intended functionality
change?

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

https://codereview.chromium.org/687563008/diff/80001/src/gpu/gl/GrGpuGL.cpp#n...
src/gpu/gl/GrGpuGL.cpp:1874: // TODO figure out why we need to flush stencil
settings on path draws at all
Can you expand on this, what is the unknown part or why do you think the stencil
settings are not needed?
When drawing the paths (e.g. covering the paths), the covered pixels are changed
based on the stencil values. The way they're changed is defined by the stencil
settings

[bsalomon, 2014-11-03 20:46:08]

https://codereview.chromium.org/687563008/diff/80001/src/gpu/GrStencilAndCove...
File src/gpu/GrStencilAndCoverPathRenderer.cpp (right):

https://codereview.chromium.org/687563008/diff/80001/src/gpu/GrStencilAndCove...
src/gpu/GrStencilAndCoverPathRenderer.cpp:113:
drawState->setStencil(kInvertedStencilPass);
On 2014/11/03 20:39:12, Kimmo Kinnunen wrote:
> So this hunk was intentionally moved from here down to path rendering
> implementation.
> The conceptual rationale was that since we have this abstract implementation
of
> general "stencil and cover path renderer", it would be conceivable that there
> would be another implementation that would have the ability to fill inverted
> paths. Thus the method here should be used only for NVPR, where we lack
inverse
> fills. Whether the lack of inverse fills is an inherent property of the STC
> algorithm or not, that I'm not expert on.
> 

This CL arose out a of a desire to make GrGpu no longer be a sublcass of
GrDrawTarget. GrGpu will no longer have a drawRect method. It will only be able
to execute draws that have been processed by a GrDT. The drawRect call had to
move somewhere upstack.

I agree that we lost a little bit of extensibility here. If NVPR or some similar
extension added support for inverse fills we'd have to revisit this.

[bsalomon, 2014-11-03 20:49:34]

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

https://codereview.chromium.org/687563008/diff/80001/src/gpu/gl/GrGpuGL.cpp#n...
src/gpu/gl/GrGpuGL.cpp:1874: // TODO figure out why we need to flush stencil
settings on path draws at all
On 2014/11/03 20:39:12, Kimmo Kinnunen wrote:
> Can you expand on this, what is the unknown part or why do you think the
stencil
> settings are not needed?
> When drawing the paths (e.g. covering the paths), the covered pixels are
changed
> based on the stencil values. The way they're changed is defined by the stencil
> settings

Our thought was that stencilSettings is more expressive than NVPR allows. The
fill type seems sufficient at the GrDT level. However, the clipping code needs
to be able to munge the stenciling in order to respect the current clip which
does require more expressiveness than just the fill type. We were thinking the
clipping code would read the fill type and then produce something for
GrGpu/GrPathRendering that more closely matches the semantics of NVPR than the
StencilSettings struct.

[Kimmo Kinnunen, 2014-11-03 20:50:24]

On 2014/11/03 20:46:08, bsalomon wrote:
> This CL arose out a of a desire to make GrGpu no longer be a sublcass of
> GrDrawTarget. GrGpu will no longer have a drawRect method. It will only be
able
> to execute draws that have been processed by a GrDT. The drawRect call had to
> move somewhere upstack.

Right.
FWIW: These kind of things are just impossible to know from commit msg of type
"Beginning to refactor nvpr code"
 
> I agree that we lost a little bit of extensibility here. If NVPR or some
similar
> extension added support for inverse fills we'd have to revisit this.

Sure. I'm actually not argumenting about extensibility. Just commenting about
back-and-forthness of the hunks and the noting a potential problem. I don't have
strong feelings where the hunk should be.

[joshualitt, 2014-11-03 20:51:47]

https://codereview.chromium.org/687563008/diff/80001/src/gpu/GrStencilAndCove...
File src/gpu/GrStencilAndCoverPathRenderer.cpp (right):

https://codereview.chromium.org/687563008/diff/80001/src/gpu/GrStencilAndCove...
src/gpu/GrStencilAndCoverPathRenderer.cpp:113:
drawState->setStencil(kInvertedStencilPass);
On 2014/11/03 20:39:12, Kimmo Kinnunen wrote:
> So this hunk was intentionally moved from here down to path rendering
> implementation.
> The conceptual rationale was that since we have this abstract implementation
of
> general "stencil and cover path renderer", it would be conceivable that there
> would be another implementation that would have the ability to fill inverted
> paths. Thus the method here should be used only for NVPR, where we lack
inverse
> fills. Whether the lack of inverse fills is an inherent property of the STC
> algorithm or not, that I'm not expert on.
> 
> The practical reason this was moved is that this causes different thing to be
> recorded. E.g when recording and replaying InOrderDrawBuffer, this actually
> erases the knowledge about path being drawn. Whether that is a bad thing or
not,
> is of course debatable. My initial thought was and is that I find it a bit
> surprising, but I'm no expert and haven't really thought about it..
> 
> The actual rationale to address the above practical reason was, if  recall
> correctly, to avoid a crash. The original drawSimpleRect had NULL parameter
that
> affected the DST copy. There was some convolution wrt the if conditions around
> the subject that then triggered badness. 
> 
> The original commit is at:
> https://codereview.chromium.org/22686002/
> 
> The move happened at patch #8. 
> The DST copy problem is described in the late comments.

Ahh, so I should explain.  It seems very possible to provide inverse fill at the
driver level.  We are kind of hoping one day to have a solution in the NVPR
driver itself.  Still, the reason I did this is because we are moving to a world
where GPU itself can no longer issue draw calls and ALL draw calls will go
through some kind of draw buffering.  As a result, we can no longer have a late
draw at the GrGLPathRendering level, as was the case with inverse fills.

If future hardware path rendering solutions do perform inverse fills, we'll
still have the decision about whether to do a 'true' inverse fill, or a stencil
+ draw simple rect made at the level of the StencilAndCoverPathRenderer itself.

Its good to see whatever may have effected the earlier patch, ie the crash,
seems to have been resolved.  Thanks for providing the link to the original
commit, it looks like the code I have here should be correct.

https://codereview.chromium.org/687563008/diff/80001/src/gpu/gl/GrGLPathRende...
File src/gpu/gl/GrGLPathRendering.cpp (right):

https://codereview.chromium.org/687563008/diff/80001/src/gpu/gl/GrGLPathRende...
src/gpu/gl/GrGLPathRendering.cpp:169: if (stroke.isFillStyle() ||
SkStrokeRec::kStrokeAndFill_Style == stroke.getStyle()) {
On 2014/11/03 20:39:12, Kimmo Kinnunen wrote:
> So I guess this is not part of "refactoring", rather intended functionality
> change?

Err, is there a problem with this chunk?  The original flow seemed to do this
test before stenciling inverse paths, so I figured we might have to do it.

[Kimmo Kinnunen, 2014-11-03 21:09:30]

On 2014/11/03 20:51:47, joshualitt wrote:
> Ahh, so I should explain.  It seems very possible to provide inverse fill at
the
> driver level.  We are kind of hoping one day to have a solution in the NVPR
> driver itself.

Yeah... It's an interesting question: can it?
Perhaps not the exact place to discuss the problem, but:
I think the problem is that it is not exactly defined what does the inverse fill
mean. (Of course it might be my fairly limited graphics and skia knowledge what
is the underlying the problem.)
If I understand  correctly the definition is along the lines of:
The inverse fill means that every pixel in the plane of the path is filled with
the fill color, unless it is covered by the path. This fill might be limited by
clip, or it might not.

So you can see from the definition that it's actually pretty hard to implement
correctly. Moreover, if you have transforms such as perspective projection, it
becomes quite hard to understand what is the correct rendering in many cases.

I haven't checked, but I don't think Skia gets this correct in any general case.
So if there's inverse filled path at 1,1 , and we are looking at skpicture at
10000,10000, will we get the correct background (inverse fill color)?
(not even talking about perspective here, as there's plenty of problems with it
in Skia)


> Its good to see whatever may have effected the earlier patch, ie the crash,
> seems to have been resolved.  Thanks for providing the link to the original
> commit, it looks like the code I have here should be correct.

Well, that's the kind of reason I actually wrote the comment. Can we know we are
not affected by the crash, if you didn't know it existed? :)

> 
>
https://codereview.chromium.org/687563008/diff/80001/src/gpu/gl/GrGLPathRende...
> File src/gpu/gl/GrGLPathRendering.cpp (right):
> 
>
https://codereview.chromium.org/687563008/diff/80001/src/gpu/gl/GrGLPathRende...
> src/gpu/gl/GrGLPathRendering.cpp:169: if (stroke.isFillStyle() ||
> SkStrokeRec::kStrokeAndFill_Style == stroke.getStyle()) {
> On 2014/11/03 20:39:12, Kimmo Kinnunen wrote:
> > So I guess this is not part of "refactoring", rather intended functionality
> > change?
> 
> Err, is there a problem with this chunk?  The original flow seemed to do this
> test before stenciling inverse paths, so I figured we might have to do it.

Now all the call sites gained the functionality added here.
In other words, the stencilPath, previously, only filled paths.
Not that I think it's a problem, I haven't checked. Just tried to ask that did
you intentionally change functionality in a commit that says refactor (which I
interpreted as not intending to change functionality)

[Kimmo Kinnunen, 2014-11-03 21:12:40]

On 2014/11/03 21:09:30, Kimmo Kinnunen wrote:
> On 2014/11/03 20:51:47, joshualitt wrote:
> > Err, is there a problem with this chunk?  The original flow seemed to do
this
> > test before stenciling inverse paths, so I figured we might have to do it.
> 
> Now all the call sites gained the functionality added here.
> In other words, the stencilPath, previously, only filled paths.
> Not that I think it's a problem, I haven't checked. Just tried to ask that did
> you intentionally change functionality in a commit that says refactor (which I
> interpreted as not intending to change functionality)

And to add: it's not my intention to bikeshed whether you did or not or how the
code is written, etc.

[joshualitt, 2014-11-03 21:24:25]

On 2014/11/03 21:09:30, Kimmo Kinnunen wrote:
> Yeah... It's an interesting question: can it?

Ahh, interesting point.  Well, if its not possible to handle the inverse fill of
a path at the driver level, then I think that makes an even clearer case for
performing the fake inverse fill as the stencilAndCoverPathRenderer.

> 
> 
> > Its good to see whatever may have effected the earlier patch, ie the crash,
> > seems to have been resolved.  Thanks for providing the link to the original
> > commit, it looks like the code I have here should be correct.
> 
> Well, that's the kind of reason I actually wrote the comment. Can we know we
are
> not affected by the crash, if you didn't know it existed? :)

True, I meant our bots have some gm miscompares, but I have yet to see any
crashes across all of our bots, or during any of my local GM runs.  I'll keep an
eye out for this and if it resurfaces we'll have to root cause and fix the
problem.

> Now all the call sites gained the functionality added here.
> In other words, the stencilPath, previously, only filled paths.
> Not that I think it's a problem, I haven't checked. Just tried to ask that did
> you intentionally change functionality in a commit that says refactor (which I
> interpreted as not intending to change functionality)

You are correct, and I did not intend to change the functionality anymore than
necessary.  I could make a separate stencil call for the inverse fill case, if
you think adding the stroke functionality to the general stencil case will be
problematic.  For the most part, th

[joshualitt, 2014-11-03 21:29:15]

On 2014/11/03 21:24:25, joshualitt wrote:
> On 2014/11/03 21:09:30, Kimmo Kinnunen wrote:
> > Yeah... It's an interesting question: can it?
> 
> Ahh, interesting point.  Well, if its not possible to handle the inverse fill
of
> a path at the driver level, then I think that makes an even clearer case for
> performing the fake inverse fill as the stencilAndCoverPathRenderer.
> 
> > 
> > 
> > > Its good to see whatever may have effected the earlier patch, ie the
crash,
> > > seems to have been resolved.  Thanks for providing the link to the
original
> > > commit, it looks like the code I have here should be correct.
> > 
> > Well, that's the kind of reason I actually wrote the comment. Can we know we
> are
> > not affected by the crash, if you didn't know it existed? :)
> 
> True, I meant our bots have some gm miscompares, but I have yet to see any
> crashes across all of our bots, or during any of my local GM runs.  I'll keep
an
> eye out for this and if it resurfaces we'll have to root cause and fix the
> problem.
> 
> > Now all the call sites gained the functionality added here.
> > In other words, the stencilPath, previously, only filled paths.
> > Not that I think it's a problem, I haven't checked. Just tried to ask that
did
> > you intentionally change functionality in a commit that says refactor (which
I
> > interpreted as not intending to change functionality)
> 
> You are correct, and I did not intend to change the functionality anymore than
> necessary.  I could make a separate stencil call for the inverse fill case, if
> you think adding the stroke functionality to the general stencil case will be
> problematic.  For the most part, th

Whoops got cut off, I meant to say "For the most part, this change only changed
a handful of gms in unnoticeable ways.  This does not mean its correct."