As part of preliminary groundwork for a chromium fix, this changelist is deprecating GrPathFill so …

As part of preliminary groundwork for a chromium fix, this changelist is deprecating GrPathFill so that SkPath::FillType is used everywhere in order to remove some code duplication between Skia and Ganesh.

BUG=chromium:135111
TEST=Try path rendering tests from the gm
Committed: https://code.google.com/p/skia/source/detail?r=6693

[sugoi, 2012-12-04 20:16:49]

Note : Ignore "gyp/effects.gypi", it shouldn't be here. I will remove it from
the changelist.

[bsalomon, 2012-12-04 20:30:53]

Looks like the right direction to me (more red than green!). My most significant
suggestion is to get rid of kHairLine entirely and instead pass a strokeWidth
(initially always <= 0 but later defer stroking to Gr code).

https://codereview.appspot.com/6875058/diff/1/include/core/SkPath.h
File include/core/SkPath.h (right):

https://codereview.appspot.com/6875058/diff/1/include/core/SkPath.h#newcode533
include/core/SkPath.h:533: static bool IsFillInverted(FillType fill) {
Is SkPath::isInverseFillType() not sufficient?

https://codereview.appspot.com/6875058/diff/1/include/gpu/GrContext.h
File include/gpu/GrContext.h (left):

https://codereview.appspot.com/6875058/diff/1/include/gpu/GrContext.h#oldcode420
include/gpu/GrContext.h:420: * @param fill          the path filling rule to
use.
Instead of adding SkPath::kHairLine_FillType can we add a strokeWidth param
here?

I'm reluctant to add kHairLine for a few reasons. SkPath is a public class and
with the change the Skia API would have two ways of communicating hairlines
(kHairLine and SkPaint's stroke width) and I'm not sure what we'd do if they
disagree. Hairline is really a special case of stroke. The existing code in the
sw backend won't expect a value of kHairLine.

Could we instead add a strokeWidth param to GrPathRenderer's interface (neg is
fill, 0 is hairline)? Currently it would never be positive since non-hairline
strokes are handled upstack. But we want to change that anyway.

https://codereview.appspot.com/6875058/diff/1/src/gpu/GrClipMaskManager.cpp
File src/gpu/GrClipMaskManager.cpp (right):

https://codereview.appspot.com/6875058/diff/1/src/gpu/GrClipMaskManager.cpp#n...
src/gpu/GrClipMaskManager.cpp:1398: SkPath::FillType
invert_fill(SkPath::FillType fill) {
Maybe we could make a static version of SkPath::toggleInverseFillType() and
eliminate this function?

https://codereview.appspot.com/6875058/diff/1/src/gpu/GrDefaultPathRenderer.cpp
File src/gpu/GrDefaultPathRenderer.cpp (right):

https://codereview.appspot.com/6875058/diff/1/src/gpu/GrDefaultPathRenderer.c...
src/gpu/GrDefaultPathRenderer.cpp:157: if (SkPath::kEvenOdd_FillType ==
path.getFillType() || SkPath::kWinding_FillType == path.getFillType()) {
If we get rid of kHairLine and pass strokeWidth this could be:

return strokeWidth < 0 && !path.isInverseFilled() && path.isConvex();

https://codereview.appspot.com/6875058/diff/1/src/gpu/GrTextContext.cpp
File src/gpu/GrTextContext.cpp (right):

https://codereview.appspot.com/6875058/diff/1/src/gpu/GrTextContext.cpp#newco...
src/gpu/GrTextContext.cpp:182: SkPath tmpPath(*glyph->fPath);
I actually think we should just pass the original. I think the old code was just
assuming that the font used winding fill for no good reason other than most
fonts do.

https://codereview.appspot.com/6875058/diff/1/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.appspot.com/6875058/diff/1/src/gpu/SkGpuDevice.cpp#newcode...
src/gpu/SkGpuDevice.cpp:1023: SkPath temp(*devPathPtr);
If we pass stroke width then we don't need the temp

https://codereview.appspot.com/6875058/diff/4001/include/core/SkPath.h
File include/core/SkPath.h (right):

https://codereview.appspot.com/6875058/diff/4001/include/core/SkPath.h#newcod...
include/core/SkPath.h:554: static FillType NonInvertedFill(FillType fill) {
If we get rid of kHairLine then this could be implemented as 
(fill & 0x1)

[reed1, 2012-12-04 22:14:13]

I don't think hairline/stroke -vs- fill should be in the path geometry, but
rather in the paint.

[reed1, 2012-12-04 22:16:15]

https://codereview.appspot.com/6875058/diff/4001/include/core/SkPath.h
File include/core/SkPath.h (right):

https://codereview.appspot.com/6875058/diff/4001/include/core/SkPath.h#newcode75
include/core/SkPath.h:75: */
In addition to not wanting hairline here, I think I vote against adding public
enum values that don't correspond to 'real' values. e.g. we could replace
kFillCount with kLast_FillType = kInverseEvenOdd_FillType, and then in the
private section we can have kFillCount = kLast_FillType + 1.

[bsalomon, 2012-12-04 22:17:55]

https://codereview.appspot.com/6875058/diff/4001/include/core/SkPath.h
File include/core/SkPath.h (right):

https://codereview.appspot.com/6875058/diff/4001/include/core/SkPath.h#newcode75
include/core/SkPath.h:75: */
On 2012/12/04 22:16:15, reed1 wrote:
> In addition to not wanting hairline here, I think I vote against adding public
> enum values that don't correspond to 'real' values. e.g. we could replace
> kFillCount with kLast_FillType = kInverseEvenOdd_FillType, and then in the
> private section we can have kFillCount = kLast_FillType + 1.

I suspect with the other changes I suggested around inverse fill helper funcs we
won't need a count at all.

[sugoi, 2012-12-05 21:03:46]

https://codereview.appspot.com/6875058/diff/1/include/core/SkPath.h
File include/core/SkPath.h (right):

https://codereview.appspot.com/6875058/diff/1/include/core/SkPath.h#newcode533
include/core/SkPath.h:533: static bool IsFillInverted(FillType fill) {
Not unless I make other changes since GrDrawTarget::stencilPath() uses this
function without the SkPath object (only with the FillType).

https://codereview.appspot.com/6875058/diff/1/src/gpu/GrClipMaskManager.cpp
File src/gpu/GrClipMaskManager.cpp (right):

https://codereview.appspot.com/6875058/diff/1/src/gpu/GrClipMaskManager.cpp#n...
src/gpu/GrClipMaskManager.cpp:1398: SkPath::FillType
invert_fill(SkPath::FillType fill) {
No need for a static function, this function is always used with a SkPath
object, I'll just delete it.

[bsalomon, 2012-12-05 21:19:21]

Using SkStroke feels right, but I think we need to distinguish between
non-hairline stroke and fill many of the places where we're testing if
stroke.getWidth() is 0. The next step is to remove the stroke from SkGpuDevice
and allow Gr to decide how to handle the stroking. At that point Gr code will
see SkStroke objects that represent any of filling, stroking, or hair-line
stroking. Maybe SkStroke::getWidthIfStroked() is not a bad idea (proposed
below)?

https://codereview.appspot.com/6875058/diff/1/include/core/SkPath.h
File include/core/SkPath.h (right):

https://codereview.appspot.com/6875058/diff/1/include/core/SkPath.h#newcode533
include/core/SkPath.h:533: static bool IsFillInverted(FillType fill) {
On 2012/12/05 21:03:46, sugoi wrote:
> Not unless I make other changes since GrDrawTarget::stencilPath() uses this
> function without the SkPath object (only with the FillType).

Makes sense

https://codereview.appspot.com/6875058/diff/1/src/gpu/GrClipMaskManager.cpp
File src/gpu/GrClipMaskManager.cpp (right):

https://codereview.appspot.com/6875058/diff/1/src/gpu/GrClipMaskManager.cpp#n...
src/gpu/GrClipMaskManager.cpp:1398: SkPath::FillType
invert_fill(SkPath::FillType fill) {
On 2012/12/05 21:03:46, sugoi wrote:
> No need for a static function, this function is always used with a SkPath
> object, I'll just delete it.

great

https://codereview.appspot.com/6875058/diff/9001/include/core/SkPath.h
File include/core/SkPath.h (right):

https://codereview.appspot.com/6875058/diff/9001/include/core/SkPath.h#newcod...
include/core/SkPath.h:527: static bool IsFillInverted(FillType fill) {
bike-shed: Can we call these IsInverseFill and NonInverseFill?

https://codereview.appspot.com/6875058/diff/9001/include/core/SkPath.h#newcod...
include/core/SkPath.h:528: SkASSERT(0 == kWinding_FillType);
Could use SK_COMPILE_ASSERT to catch this at build time instead of runtime.

https://codereview.appspot.com/6875058/diff/9001/include/gpu/GrContext.h
File include/gpu/GrContext.h (left):

https://codereview.appspot.com/6875058/diff/9001/include/gpu/GrContext.h#oldc...
include/gpu/GrContext.h:421: * @param translate     optional additional
translation applied to the
heh, thanks for the cleanup! :)

https://codereview.appspot.com/6875058/diff/9001/include/gpu/GrTypes.h
File include/gpu/GrTypes.h (left):

https://codereview.appspot.com/6875058/diff/9001/include/gpu/GrTypes.h#oldcod...
include/gpu/GrTypes.h:589: 
w00t!

https://codereview.appspot.com/6875058/diff/9001/src/gpu/GrAAConvexPathRender...
File src/gpu/GrAAConvexPathRenderer.cpp (right):

https://codereview.appspot.com/6875058/diff/9001/src/gpu/GrAAConvexPathRender...
src/gpu/GrAAConvexPathRenderer.cpp:436: (0 != stroke.getWidth()) &&
!path.isInverseFillType() && path.isConvex());
Hmmm... it seems like this will break when GrPR's can receive any of
fill,hairline-stroke,regular-stroke since it would allow a non-hairline stroked
here. It seems like we want a function on SkStroke that is something like

SkScalar getWidthIfStroked() const {
   return fDoFill ? -SK_Scalar1 : fWidth;
}

https://codereview.appspot.com/6875058/diff/9001/src/gpu/GrClipMaskManager.cpp
File src/gpu/GrClipMaskManager.cpp (right):

https://codereview.appspot.com/6875058/diff/9001/src/gpu/GrClipMaskManager.cp...
src/gpu/GrClipMaskManager.cpp:422: SkStroke stroke;
It looks like the default cons sets this guy to stroke not fill. We definitely
want to fill here. I think we need to call setDoFill(true) here.

Since filling is common we could also consider one of two possibilities:

1) Have a static object on SkStroke that is already initialized to fill with a
static getter

2) Make the SkStroke param be a pointer where NULL means default to fill.

Not saying either is required for this change... just thinking "out loud".

https://codereview.appspot.com/6875058/diff/9001/src/gpu/GrClipMaskManager.cp...
src/gpu/GrClipMaskManager.cpp:715: bool doAA,
This guy doesn't need the Stroke param. Clips can't be stroke paths. We can make
the decision to fill within this function.

https://codereview.appspot.com/6875058/diff/9001/src/gpu/GrClipMaskManager.cp...
src/gpu/GrClipMaskManager.cpp:741: const SkStroke& stroke,
Same thing, doesn't need a param.

https://codereview.appspot.com/6875058/diff/9001/src/gpu/GrClipMaskManager.cp...
src/gpu/GrClipMaskManager.cpp:1111: SkStroke stroke;
should be filled.

https://codereview.appspot.com/6875058/diff/9001/src/gpu/GrClipMaskManager.cp...
src/gpu/GrClipMaskManager.cpp:1431: SkStroke stroke;
should be filled

https://codereview.appspot.com/6875058/diff/9001/src/gpu/GrDefaultPathRendere...
File src/gpu/GrDefaultPathRenderer.cpp (right):

https://codereview.appspot.com/6875058/diff/9001/src/gpu/GrDefaultPathRendere...
src/gpu/GrDefaultPathRenderer.cpp:159: (SkPath::kEvenOdd_FillType ==
path.getFillType() || SkPath::kWinding_FillType == path.getFillType())) {
&& !path.isInverseFillType()?

https://codereview.appspot.com/6875058/diff/9001/src/gpu/GrStencilAndCoverPat...
File src/gpu/GrStencilAndCoverPathRenderer.cpp (right):

https://codereview.appspot.com/6875058/diff/9001/src/gpu/GrStencilAndCoverPat...
src/gpu/GrStencilAndCoverPathRenderer.cpp:54: GrAssert(SkPath::kEvenOdd_FillType
== path.getFillType() ||
GrAssert(!path.isInverseFillType());

https://codereview.appspot.com/6875058/diff/9001/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.appspot.com/6875058/diff/9001/src/gpu/SkGpuDevice.cpp#newc...
src/gpu/SkGpuDevice.cpp:968: bool             doFill = true;
It seems like this var really controls hairline or not. Maybe we should rename
it doHairline (and reverse the true/false sense of it).

It is related to the fill path computation below, but we want to move that out
of here any way and let Gr code decide whether to compute the stroke path or
draw directly from the original path.

https://codereview.appspot.com/6875058/diff/9001/src/gpu/SkGpuDevice.cpp#newc...
src/gpu/SkGpuDevice.cpp:1024: SkStroke stroke;
This should probably be:

SkStroke stroke;
if (doHairline) {
  stroke.setWidth(0);
} else {
  stroke.setDoFill(true);
}

[sugoi, 2012-12-05 22:04:04]

https://codereview.appspot.com/6875058/diff/9001/src/gpu/GrClipMaskManager.cpp
File src/gpu/GrClipMaskManager.cpp (right):

https://codereview.appspot.com/6875058/diff/9001/src/gpu/GrClipMaskManager.cp...
src/gpu/GrClipMaskManager.cpp:719:
GrSWMaskHelper::DrawPathMaskToTexture(context, path,
hmmm... I was passing that parameter because draw_path_in_software() calls
GrSWMaskHelper::DrawPathMaskToTexture(), which calls GrSWMaskHelper::draw(),
which contains a hairline check (before my changes). So I should remove the
argument and create a dummy SkStroke inside draw_path_in_software() ?

[bsalomon, 2012-12-05 22:10:33]

https://codereview.appspot.com/6875058/diff/9001/src/gpu/GrClipMaskManager.cpp
File src/gpu/GrClipMaskManager.cpp (right):

https://codereview.appspot.com/6875058/diff/9001/src/gpu/GrClipMaskManager.cp...
src/gpu/GrClipMaskManager.cpp:719:
GrSWMaskHelper::DrawPathMaskToTexture(context, path,
On 2012/12/05 22:04:04, sugoi wrote:
> hmmm... I was passing that parameter because draw_path_in_software() calls
> GrSWMaskHelper::DrawPathMaskToTexture(), which calls GrSWMaskHelper::draw(),
> which contains a hairline check (before my changes). So I should remove the
> argument and create a dummy SkStroke inside draw_path_in_software() ?

Yeah, all the paths in this file are implicitly filled not stroked.

[sugoi, 2012-12-06 16:19:31]

Ok, I think I made most of the changes that were requested. Please tell me if I
forgot anything. I know some of the logic of SkGpuDevice still has to be moved
to GrContext, but if that's ok with you, I thought I could do this in a separate
patch (or changelist, I never know which one to use) after the changes necessary
to remove GrPathFill are landed, in order to make it easier to review.

[bsalomon, 2012-12-06 16:42:55]

I definitely agree that moving the stroking decision to Gr should be a separate
CL.

I've got one last round of comments.

https://codereview.appspot.com/6875058/diff/23001/include/gpu/GrContext.h
File include/gpu/GrContext.h (right):

https://codereview.appspot.com/6875058/diff/23001/include/gpu/GrContext.h#new...
include/gpu/GrContext.h:423: void drawPath(const GrPaint& paint, const SkPath&
path, bool doHairLine);
SkStroke? That can wait until the follow up change.

https://codereview.appspot.com/6875058/diff/23001/src/gpu/GrAAConvexPathRende...
File src/gpu/GrAAConvexPathRenderer.cpp (right):

https://codereview.appspot.com/6875058/diff/23001/src/gpu/GrAAConvexPathRende...
src/gpu/GrAAConvexPathRenderer.cpp:436: (0 != stroke.getWidthIfStroked()) &&
!path.isInverseFillType() && path.isConvex());
stroke.getWidthIfStroked() < 0, right? This guy only does fills.

https://codereview.appspot.com/6875058/diff/23001/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

https://codereview.appspot.com/6875058/diff/23001/src/gpu/GrContext.cpp#newco...
src/gpu/GrContext.cpp:983: GrAssert(strokeWidth <= 0);
Soon we will lift this restriction. (just noting it.. nothing to do now).

https://codereview.appspot.com/6875058/diff/23001/src/gpu/GrContext.cpp#newco...
src/gpu/GrContext.cpp:993: }
stroke.setWidth(strokeWidth);
if (strokeWidth < 0) {
  stroke.setDoFill(true);
}

https://codereview.appspot.com/6875058/diff/23001/src/gpu/GrContext.cpp#newco...
src/gpu/GrContext.cpp:1622: GrPathRenderer* pr =
fPathRendererChain->getPathRenderer(path, stroke,
nit, inconsistent line wrapping in the params. We don't have a very consistent
style globally for this so I don't care how you fix it (groups of 2, each its
own line, all on one line...)

https://codereview.appspot.com/6875058/diff/23001/src/gpu/GrDefaultPathRender...
File src/gpu/GrDefaultPathRenderer.cpp (right):

https://codereview.appspot.com/6875058/diff/23001/src/gpu/GrDefaultPathRender...
src/gpu/GrDefaultPathRenderer.cpp:498: return !antiAlias;
This guy doesn't know how to do non-hairline strokes so let's make this check
that getWidthIfStroked() <= 0.

https://codereview.appspot.com/6875058/diff/23001/src/gpu/GrGpu.h
File src/gpu/GrGpu.h (right):

https://codereview.appspot.com/6875058/diff/23001/src/gpu/GrGpu.h#newcode573
src/gpu/GrGpu.h:573: virtual void onStencilPath(const GrPath* path, const
SkStroke& stroke, SkPath::FillType) SK_OVERRIDE;
wrap (skia has a 100 col soft limit).

https://codereview.appspot.com/6875058/diff/23001/src/gpu/GrInOrderDrawBuffer...
File src/gpu/GrInOrderDrawBuffer.cpp (right):

https://codereview.appspot.com/6875058/diff/23001/src/gpu/GrInOrderDrawBuffer...
src/gpu/GrInOrderDrawBuffer.cpp:490: void
GrInOrderDrawBuffer::onStencilPath(const GrPath* path, const SkStroke& stroke,
SkPath::FillType fill) {
wrap

https://codereview.appspot.com/6875058/diff/23001/src/gpu/GrSWMaskHelper.cpp
File src/gpu/GrSWMaskHelper.cpp (right):

https://codereview.appspot.com/6875058/diff/23001/src/gpu/GrSWMaskHelper.cpp#...
src/gpu/GrSWMaskHelper.cpp:68: paint.setStyle(SkPaint::kFill_Style);
if (stroke.getDoFill()) {
   paint.setStyle(SkPaint::kFillStyle);
} else {
   paint.setStyle(SkPaint::kStrokeStyle);
   paint.setJoin(...);
   paint.setCap(...);
   paint.setStrokeWidth(...);
}

https://codereview.appspot.com/6875058/diff/23001/src/gpu/GrSWMaskHelper.cpp#...
src/gpu/GrSWMaskHelper.cpp:69: if (path.getFillType() !=
pathToDraw->getFillType()) {
I think we can axe this entire if... it was guarding against the GrPathFill
param not matching the path's fill. We don't take a GrPathFill param anymore so
there is no possibility of mismatch.

https://codereview.appspot.com/6875058/diff/23001/src/gpu/GrStencilAndCoverPa...
File src/gpu/GrStencilAndCoverPathRenderer.cpp (right):

https://codereview.appspot.com/6875058/diff/23001/src/gpu/GrStencilAndCoverPa...
src/gpu/GrStencilAndCoverPathRenderer.cpp:40: return (0 !=
stroke.getWidthIfStroked()) &&
This guy doesn't currently handle stroking, just filling. This should be
stroke.getDoFill() && ...

I can update him to handle both hairline and stroking in another CL.

https://codereview.appspot.com/6875058/diff/23001/src/gpu/GrTextContext.cpp
File src/gpu/GrTextContext.cpp (right):

https://codereview.appspot.com/6875058/diff/23001/src/gpu/GrTextContext.cpp#n...
src/gpu/GrTextContext.cpp:20: #include "SkStroke.h"
needed?

https://codereview.appspot.com/6875058/diff/23001/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.appspot.com/6875058/diff/23001/src/gpu/SkGpuDevice.cpp#new...
src/gpu/SkGpuDevice.cpp:967: bool             doHairLine = false;
thanks... i find it much easier to follow the pretty complicated logic below.

[bsalomon, 2012-12-06 16:48:59]

https://codereview.appspot.com/6875058/diff/23001/src/gpu/GrAAConvexPathRende...
File src/gpu/GrAAConvexPathRenderer.cpp (right):

https://codereview.appspot.com/6875058/diff/23001/src/gpu/GrAAConvexPathRende...
src/gpu/GrAAConvexPathRenderer.cpp:436: (0 != stroke.getWidthIfStroked()) &&
!path.isInverseFillType() && path.isConvex());
On 2012/12/06 16:42:55, bsalomon wrote:
> stroke.getWidthIfStroked() < 0, right? This guy only does fills.

Or perhaps more simply stroke.getDoFill()

[bsalomon, 2012-12-06 19:51:42]

On 2012/12/06 19:44:11, sugoi wrote:

LGTM. Thanks for putting up with multiple rounds of nit picking! :)

[sugoi, 2012-12-06 19:54:04]

No problem. I would've been disappointed if everyone thought I got it right the
first time :)

[sugoi, 2012-12-06 20:04:31]

I'll just wait for Mike to say it's also ok with him before I submit the code.

[bsalomon, 2012-12-06 20:07:51]

On 2012/12/06 20:04:31, sugoi wrote:
> I'll just wait for Mike to say it's also ok with him before I submit the code.

I think Mike is on a plane and won't be able to check this before tomorrow. I'd
just commit. If he has any objections we can address them later. I'll take full
responsibility. :)

[sugoi, 2012-12-06 20:10:20]

All right :) I'll try to remember to come to work with a fully armored suit
tomorrow :)

[reed1, 2012-12-06 21:07:26]

flight delayed :)

https://codereview.appspot.com/6875058/diff/17006/include/core/SkPath.h
File include/core/SkPath.h (right):

https://codereview.appspot.com/6875058/diff/17006/include/core/SkPath.h#newco...
include/core/SkPath.h:526: */
The method is named isInverseFillType, but the static is named IsInverseFill.
Why did we drop 'Type' from the end of the static name?

https://codereview.appspot.com/6875058/diff/17006/include/core/SkPath.h#newco...
include/core/SkPath.h:537: */
Not sure about the name. ConvertToNonInverseFillType? The existing name is
neither a verb nor a question, so not sure what it does.

Also, perhaps the dox could illustrate a quick table of the performed
conversions. e.g.

kWinding -> kWinding
kInverseWinding -> kWinding
etc.

[sugoi, 2012-12-06 21:20:31]

All right, thanks. The patch is already committed, but I have more work to do in
this area, so I'll make sure to follow-up on these comments.