Modifying SkPath to store all verbs provided by the user, and to give

Modifying SkPath to store all verbs provided by the user, and to give
correct results for all stroke and fill modes even on the various types
of degenerate paths.

The goals of this patch include:
1. Have Skia store all of the verbs implied by path construction methods, even
if those define degenerate paths. The SVG implementation in WebKit, which is
backed by Skia, needs to know about all elements of the path, even degenerate
ones, for the correct drawing of markers and line caps. For example, in SVG you
should be able to draw a scatter plot by specifying a marker for vertices and
then giving a sequence of moveTo commands. Skia will not store the moveTos,
requiring a different storage mechanism.

2. Assuming 1, maintain the current Skia behavior. That is, make Skia robust to
degenerate paths.

3. Fix an existing bug in Skia where a degenerate moveTo-lineTo pair spits out
warnings from rasterization and produces incorrect results in inverse-fill
renderings.

4. Adds extensive testing for degenerate paths and path rendering in general.

To meet these goals, the patch I am proposing will result in minor additional
storage for degenerate paths (a few bytes per degenerate path, only if the user
defines such paths). There is also some additional overhead in the iteration
code, with the path now cleaned to remove degenerate segments as part of the
iteration process. I suspect this will also fix issues with computing normal
vectors to degenerate segments. Benchmarking suggests that this change may
result in slightly (< 1%) slower path drawing due to the checks for
degeneracy. This overhead could be removed (in fact, a significant speedup
could occur) if the results of iterating to clean up the path were cached.
This would cost memory, of course, and quite a bit of it.

BUG=398
TEST=tests/PathTest.cpp
     gm/cubicpaths.cpp
     gm/degeneratesegments.cpp
     gm/movepaths.cpp
     gm/linepaths.cpp
     gm/quadpaths.cpp
Committed: http://code.google.com/p/skia/source/detail?r=2901

[TomH, 2011-12-12 22:45:21]

Looks pretty good; leaving the review with reed@ for semantics. I expect we'll
take a fast exit almost always from computeDegeneracy(), but do we have a
realistic benchmark that would ever hit it? kothic.org/js/ is path-heavy, but I
don't think we'll run into (m)any degenerate paths there.

http://codereview.appspot.com/5482051/diff/1/src/core/SkPath.cpp
File src/core/SkPath.cpp (right):

http://codereview.appspot.com/5482051/diff/1/src/core/SkPath.cpp#newcode1611
src/core/SkPath.cpp:1611: bool SkPath::computeDegeneracy() const {
Comments in header read "// called, if dirty, by isDegenerate()". It looks like
you aren't caching values yet, but always calling?

[reed1, 2011-12-13 13:34:15]

Can we see an updated test before reviewing this.

http://codereview.appspot.com/5477054/

Also, can you document here what effect you're looking for? i.e. when stroking,
the following patterns should draw xxx: M, MC, Mdegnerateline, MdegeneratelineC,
etc.

http://codereview.appspot.com/5482051/diff/1/src/core/SkStroke.cpp
File src/core/SkStroke.cpp (left):

http://codereview.appspot.com/5482051/diff/1/src/core/SkStroke.cpp#oldcode620
src/core/SkStroke.cpp:620: // need a real shift methid on path. antialias paths
could use this too
what has changed here?

[TomH, 2011-12-13 13:39:07]

http://codereview.appspot.com/5482051/diff/1/src/core/SkStroke.cpp
File src/core/SkStroke.cpp (left):

http://codereview.appspot.com/5482051/diff/1/src/core/SkStroke.cpp#oldcode620
src/core/SkStroke.cpp:620: // need a real shift methid on path. antialias paths
could use this too
On 2011/12/13 13:34:15, reed1 wrote:
> what has changed here?
Spelling fix.

[caryclark1, 2011-12-13 13:56:19]

http://codereview.appspot.com/5482051/diff/1/src/core/SkPath.cpp
File src/core/SkPath.cpp (right):

http://codereview.appspot.com/5482051/diff/1/src/core/SkPath.cpp#newcode136
src/core/SkPath.cpp:136: fSegmentMask        = src.fSegmentMask;
more formatting changes, while important, not needed in this CL

http://codereview.appspot.com/5482051/diff/1/src/core/SkPath.cpp#newcode206
src/core/SkPath.cpp:206: return computeDegeneracy();
It looks like this is set up to allow computeDegeneracy() to be lazily
evaluated, and isDegenerate() to return the computed value -- but it's not coded
that way. Why have two functions that do exactly the same thing? Is this an
interim step towards caching the computeDegeneracy() result?

http://codereview.appspot.com/5482051/diff/1/src/core/SkPath.cpp#newcode1607
src/core/SkPath.cpp:1607: return SkScalarNearlyZero(a.fX - b.fX, tolerance) &&
why pass 'tolerance' to this function when the callers always use the default
parameter?

http://codereview.appspot.com/5482051/diff/1/src/core/SkPath.cpp#newcode1631
src/core/SkPath.cpp:1631: return false;
A control point close to or equal to an end point for quadratics or cubics does
not make the path degenerate. You'll need to look for all three or four points
to be nearly equal for the degenerate result to be true.

http://codereview.appspot.com/5482051/diff/1/src/core/SkStroke.cpp
File src/core/SkStroke.cpp (right):

http://codereview.appspot.com/5482051/diff/1/src/core/SkStroke.cpp#newcode620
src/core/SkStroke.cpp:620: // need a real shift method on path. antialias paths
could use this too
Fixing spelling and other formatting changes are important, but can distract
from the code changes central to this bug fix. Please separate out changes like
this one into a separate CL

[Stephen Chenney, 2011-12-13 14:57:39]

I'll get another patch up incorporating the changes necessary. I'll add some
testing in the unit tests and probably another gm test or two. Test coverage of
cubic and quadratic curve segments seems to be negligible.

http://codereview.appspot.com/5482051/diff/1/src/core/SkPath.cpp
File src/core/SkPath.cpp (right):

http://codereview.appspot.com/5482051/diff/1/src/core/SkPath.cpp#newcode136
src/core/SkPath.cpp:136: fSegmentMask        = src.fSegmentMask;
On 2011/12/13 13:56:19, caryclark1 wrote:
> more formatting changes, while important, not needed in this CL

This was an oversight - I changed it then reverted the change but not the
format. Sorry.

http://codereview.appspot.com/5482051/diff/1/src/core/SkPath.cpp#newcode206
src/core/SkPath.cpp:206: return computeDegeneracy();
On 2011/12/13 13:56:19, caryclark1 wrote:
> It looks like this is set up to allow computeDegeneracy() to be lazily
> evaluated, and isDegenerate() to return the computed value -- but it's not
coded
> that way. Why have two functions that do exactly the same thing? Is this an
> interim step towards caching the computeDegeneracy() result?

You surmised correctly. I originally coded it as caching the degeneracy result.
However, that required making this method non-const, which then required
changing several other functions and finally I hit a roadblock in
SkPicturePlayback where the path had to const. So I reverted.

While we could cache the result, we would have to store it indirectly, like the
bounds, so that we can update the flag while not breaking the const requirement.

http://codereview.appspot.com/5482051/diff/1/src/core/SkPath.cpp#newcode1607
src/core/SkPath.cpp:1607: return SkScalarNearlyZero(a.fX - b.fX, tolerance) &&
On 2011/12/13 13:56:19, caryclark1 wrote:
> why pass 'tolerance' to this function when the callers always use the default
> parameter?

Done.

http://codereview.appspot.com/5482051/diff/1/src/core/SkPath.cpp#newcode1611
src/core/SkPath.cpp:1611: bool SkPath::computeDegeneracy() const {
On 2011/12/12 22:45:21, TomH wrote:
> Comments in header read "// called, if dirty, by isDegenerate()". It looks
like
> you aren't caching values yet, but always calling?

Yeah. See the comment up in isDegenerate. I've actually removed this method and
just stuck the code in isDegenerate.

http://codereview.appspot.com/5482051/diff/1/src/core/SkPath.cpp#newcode1631
src/core/SkPath.cpp:1631: return false;
On 2011/12/13 13:56:19, caryclark1 wrote:
> A control point close to or equal to an end point for quadratics or cubics
does
> not make the path degenerate. You'll need to look for all three or four points
> to be nearly equal for the degenerate result to be true.

This is the same test used in SkStroke. To be considered degenerate (i.e. for
this if to be true) every one of the pairwise tests must be equal. This is not
quite the same as testing that all four points are within the _same_ square of
tolerance. However, we want the behavior here to match SkStroke so that lines
cannot make it through here then fall foul of SkStroke's path modifications.

http://codereview.appspot.com/5482051/diff/1/src/core/SkStroke.cpp
File src/core/SkStroke.cpp (right):

http://codereview.appspot.com/5482051/diff/1/src/core/SkStroke.cpp#newcode620
src/core/SkStroke.cpp:620: // need a real shift method on path. antialias paths
could use this too
On 2011/12/13 13:56:19, caryclark1 wrote:
> Fixing spelling and other formatting changes are important, but can distract
> from the code changes central to this bug fix. Please separate out changes
like
> this one into a separate CL

Done.

[Stephen Chenney, 2011-12-13 15:22:27]

To address Mike's question about goals of this patch:

1. Have Skia store all of the verbs implied by path construction methods, even
if those define degenerate paths. The SVG implementation in WebKit, which is
backed by Skia, needs to know about all elements of the path, even degenerate
ones, for the correct drawing of markers and line caps. For example, in SVG you
should be able to draw a scatter plot by specifying a marker for vertices and
then giving a sequence of moveTo commands. Skia will not store the moveTos,
requiring a different storage mechanism.

2. Assuming 1, maintain the current Skia behavior. That is, make Skia robust to
degenerate paths.

3. Fix an existing bug in Skia where a degenerate moveTo-lineTo pair spits out
warnings from rasterization and produces inclirrect results in inverse-fill
renderings.

To meet these goals, the patch I am proposing will result in minor additional
storage for degenerate paths (a few bytes per degenerate path, only if the user
defines such paths). Each time the SkCanvas::drawPath is called there will be
some additional overhead of testing for degeneracy, rather than the simpler
isEmpty test. For non-degenerate paths this will require iterating only until
the first non-degenerate segment, probably the second verb in the path.

Working around this issue in WebKit would require duplicating all of the path
storage code into the platform/graphics/skia code and maintaining duplicate
copies of the path information.

[reed1, 2011-12-13 15:53:52]

That explanation helps, thanks.

What is the drawing result of keeping all of the moveTos? i.e. when stroking,
what should be drawn (in the new world) with each of the following:

M M M
MC MC MC
Mdegenerate Mdegenerate Mdegenerate
MdegenerateC MdegenerateC MdegenerateC

(are there other patterns of interest?)

[Stephen Chenney, 2011-12-13 16:34:07]

On 2011/12/13 15:53:52, reed1 wrote:
> That explanation helps, thanks.
> 
> What is the drawing result of keeping all of the moveTos? i.e. when stroking,
> what should be drawn (in the new world) with each of the following:
> 
> M M M
> MC MC MC
> Mdegenerate Mdegenerate Mdegenerate
> MdegenerateC MdegenerateC MdegenerateC
> 
> (are there other patterns of interest?)

First, consider a path as a sequence of sub-paths, with a new sub-path starting
every time a move command is issued (a close implies a following move). Behavior
is defined per-sub-path.

In Skia, degenerate sub-paths draw nothing, always. So their inverse fill draws
everything. This is the existing behavior and I see no reason to change it.

Degenerate paths are those that go nowhere between their starting move and the
next move (a close implies a following move). Examples include
move; move-close; move-line_to_nowhere; move-cubic_to_nowhere;
move-quad_to_nowhere; and extensions with as many to_nowhere as you care to add,
with or without close on the end.

Other platforms, such as PDF and SVG, define specific behavior for degenerate
paths of various kinds, typically depending on the stroke width and line cap
properties. For example, PDF apparently draws nothing for a lone move, a dot for
move-close, a vertical bar stroke-width long for move-line_to_nowhere, a circle
of diameter stroke-width for round endcaps and zero length, etc. SVG draws
endcaps for zero-length paths, but not lone moves. The motivation in SVG seems
to be animated elements that shrink to zero size.

[reed1, 2011-12-13 18:32:30]

from an email thread, I think my answer is:

No change is planned for skia, other than to record all verbs as is (no
consolidation of consecutive moveTos). SVG will produces its particular results
for degenerate contours outside of skia.

[reed1, 2011-12-13 18:34:20]

I think we can easily cache the degenerate state (i.e. nothing to fill or
stroke) by repurposing one of the existing mutable bytes. For instance,
boundsIsDirty needs just a bit. We can change that to 8 bits of flags...

enum Flags {
    kDirtyBounds_Bit = 1 << 0,
    kDirtyIsDegenerate_Bit = 1 << 1,
    kIsDegenerate_Bit = 1 << 2,
};

[Stephen Chenney, 2011-12-14 20:26:16]

I think I'm done, although I have found another bug (415) in the process.

I also think that the behavior of inverse fill modes for paths with non-hairline
stroke width is incorrect - at least it makes no sense to me. This would be
another bug if it is indeed incorrect. See the result for the gm tests for
linepaths, cubicpaths and quadpaths.

Please also advise on how to handle updating gm base results.

http://codereview.appspot.com/5482051/diff/2003/include/core/SkPath.h
File include/core/SkPath.h (right):

http://codereview.appspot.com/5482051/diff/2003/include/core/SkPath.h#newcode112
include/core/SkPath.h:112: 
Changed the convexity get/set code to use flags and save us another byte.

http://codereview.appspot.com/5482051/diff/2003/include/core/SkPath.h#newcode231
include/core/SkPath.h:231: if (getFlag(kBoundsIsDirty_Bit)) {
I'm open to making this a private method: bool areBoundsDirty()

http://codereview.appspot.com/5482051/diff/2003/include/core/SkPath.h#newcode695
include/core/SkPath.h:695: 
We might want to move the flag get/set/clear code to some utility class, or even
its own file.

http://codereview.appspot.com/5482051/diff/2003/include/core/SkPoint.h
File include/core/SkPoint.h (right):

http://codereview.appspot.com/5482051/diff/2003/include/core/SkPoint.h#newcod...
include/core/SkPoint.h:322: SkScalarNearlyZero(fY - v.fY, SK_ScalarNearlyZero);
This code is needed by both SkPath and SkStroke, so I thought it could go here
in SkPoint, which is its natural home.

http://codereview.appspot.com/5482051/diff/2003/src/core/SkPath.cpp
File src/core/SkPath.cpp (right):

http://codereview.appspot.com/5482051/diff/2003/src/core/SkPath.cpp#newcode59
src/core/SkPath.cpp:59: fPath->clearFlags(SkPath::kBoundsIsDirty_Bit);
Could create separate methods to get/set each of the flags.

http://codereview.appspot.com/5482051/diff/2003/tests/PathTest.cpp
File tests/PathTest.cpp (right):

http://codereview.appspot.com/5482051/diff/2003/tests/PathTest.cpp#newcode754
tests/PathTest.cpp:754: }
Note the failure here due to poor addPath behavior, due in turn to a need for a
raw path iterator. See the bug 415.

[reed1, 2011-12-15 14:14:06]

I don't think all the changes around convexity are warranted. We actually
anticpate that we may add more entries to that enum, and the packed SkPath has a
spare byte, so I think convexity should stay as it was.

http://codereview.appspot.com/5482051/diff/2015/include/core/SkPath.h
File include/core/SkPath.h (right):

http://codereview.appspot.com/5482051/diff/2015/include/core/SkPath.h#newcode684
include/core/SkPath.h:684: bool getFlag(uint8_t flag) const {
Skia doesn't use this convention. The name seems to imply a single flag is being
tested, but you take a mask and not an enum value. Also this can generate
compiler warnings about an implicit conversion to bool.

Since the flags are all private, I think manipulating the fFlags field is OK
internally: e.g.

fFlags |= foo;

However, if you like a wrapper function (as we have for public flags in SkPaint)
you could write it in that style, and add some debug checks. e.g.

void setFlags(unsigned flags) {
    SkASSERT(0 == (flags & ~0x1F));
    fFlags |= flags;
}

[Stephen Chenney, 2011-12-16 17:06:46]

This patch passes all the tests I could throw at it, and addresses previous
comments. It's a larger patch than I would have liked, but I can't leave part of
it out and have everything work.

Actually, there is one more potential test, which I will get up this afternoon.
Then I hope we're done.

http://codereview.appspot.com/5482051/diff/2015/src/core/SkPath.cpp
File src/core/SkPath.cpp (right):

http://codereview.appspot.com/5482051/diff/2015/src/core/SkPath.cpp#newcode1285
src/core/SkPath.cpp:1285: fMoveTo = *srcPts;
Now set in consumeDegenerateSegments

http://codereview.appspot.com/5482051/diff/2015/src/core/SkPath.cpp#newcode1290
src/core/SkPath.cpp:1290: fNeedMoveTo = kAfterCons_NeedMoveToState;
Now set in consumeDegenerateSegments

http://codereview.appspot.com/5482051/diff/9002/src/core/SkPath.cpp
File src/core/SkPath.cpp (right):

http://codereview.appspot.com/5482051/diff/9002/src/core/SkPath.cpp#newcode1160
src/core/SkPath.cpp:1160: // closed the path. Also the initial state.
I renamed these to make it clear what they mean. In general we care about where
in a path we are, not just what the move to need is.

http://codereview.appspot.com/5482051/diff/9002/src/core/SkPath.cpp#newcode1240
src/core/SkPath.cpp:1240: bool SkPath::Iter::cons_moveTo(SkPoint pts[1]) {
Added commentary to this to make it clearer.

http://codereview.appspot.com/5482051/diff/9002/src/core/SkPath.cpp#newcode1269
src/core/SkPath.cpp:1269: void SkPath::Iter::consumeDegenerateSegments() {
Now that we can have degenerate segments, and the drawing code really dislikes
degenerate segments, we have this method to remove such unnecessary path data.
This will also fix some pre-existing bugs, I think.

[reed1, 2011-12-16 17:23:55]

Thanks for the cleanup around convexity and flags: much easier to read.

http://codereview.appspot.com/5482051/diff/9002/include/core/SkPath.h
File include/core/SkPath.h (right):

http://codereview.appspot.com/5482051/diff/9002/include/core/SkPath.h#newcode682
include/core/SkPath.h:682: void setFlags(uint8_t flags) const {
make the flags parameter be unsigned. no need to reveal to the caller how many
bits we actually store today. Same for clearFlags.

http://codereview.appspot.com/5482051/diff/9002/include/core/SkPath.h#newcode688
include/core/SkPath.h:688: SkASSERT(0 == (flags & ~0x7));
Slightly more general construct would be

fFlags &= ~flags;

since it hides any notion of how many bits are in flags.

http://codereview.appspot.com/5482051/diff/9002/include/core/SkPath.h#newcode698
include/core/SkPath.h:698: // Primitive degeneracy testing methods
static functions begin with Capitals in skia.

I like that these are private. Why are they in SkPath.h? Are they shared by
other .cpp files, or can these be just local static functions in SkPath.cpp?

http://codereview.appspot.com/5482051/diff/9002/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (left):

http://codereview.appspot.com/5482051/diff/9002/src/core/SkCanvas.cpp#oldcode...
src/core/SkCanvas.cpp:1309: if (path.isEmpty()) {
Is this change required? I only ask thinking that if the path hasn't already
cached the answer, then this check might be slower than just an empty check.
Perhaps this check is required to fix a bug?

http://codereview.appspot.com/5482051/diff/9002/src/core/SkPath.cpp
File src/core/SkPath.cpp (left):

http://codereview.appspot.com/5482051/diff/9002/src/core/SkPath.cpp#oldcode374
src/core/SkPath.cpp:374: fBoundsIsDirty = false;
skia adds this-> before calls to methods on self.

http://codereview.appspot.com/5482051/diff/9002/src/core/SkPath.cpp#oldcode1252
src/core/SkPath.cpp:1252: SkPath::Verb SkPath::Iter::next(SkPoint pts[4]) {
skia adds this-> before calls to methods on self.

http://codereview.appspot.com/5482051/diff/9002/src/core/SkPath.cpp
File src/core/SkPath.cpp (right):

http://codereview.appspot.com/5482051/diff/9002/src/core/SkPath.cpp#newcode206
src/core/SkPath.cpp:206: bool SkPath::isDegenerate() const {
skia adds this-> before calls to methods on self.

http://codereview.appspot.com/5482051/diff/9002/src/core/SkPath.cpp#newcode401
src/core/SkPath.cpp:401: do {                                                   
   \
skia adds this-> before calls to methods on self.

http://codereview.appspot.com/5482051/diff/9002/src/core/SkPath.cpp#newcode1571
src/core/SkPath.cpp:1571: if (!(fFlags & kDegeneracyIsDirty_Bit)) {
skia adds this-> before calls to methods on self.

[reed1, 2011-12-16 17:25:03]

BTW -- I know this is a long CL review cycle, but consider it your "readability
for skia". This is a good change, but touches core files and notions in skia,
hence the extra scrutiny you're having to undergo.

[Stephen White, 2011-12-16 19:24:36]

http://codereview.appspot.com/5482051/diff/9002/include/core/SkPoint.h
File include/core/SkPoint.h (right):

http://codereview.appspot.com/5482051/diff/9002/include/core/SkPoint.h#newcod...
include/core/SkPoint.h:320: bool equalsWithinTolerance(const SkPoint& v, const
SkScalar tol) const {
Drive-by comment:  Looks like parameter "tol" is ignored here.

[Stephen Chenney, 2011-12-16 19:32:28]

One more test to come, with any bug fixes that come out of it.

http://codereview.appspot.com/5482051/diff/9002/include/core/SkPath.h
File include/core/SkPath.h (right):

http://codereview.appspot.com/5482051/diff/9002/include/core/SkPath.h#newcode682
include/core/SkPath.h:682: void setFlags(uint8_t flags) const {
On 2011/12/16 17:23:55, reed1 wrote:
> make the flags parameter be unsigned. no need to reveal to the caller how many
> bits we actually store today. Same for clearFlags.

Done.

http://codereview.appspot.com/5482051/diff/9002/include/core/SkPath.h#newcode688
include/core/SkPath.h:688: SkASSERT(0 == (flags & ~0x7));
On 2011/12/16 17:23:55, reed1 wrote:
> Slightly more general construct would be
> 
> fFlags &= ~flags;
> 
> since it hides any notion of how many bits are in flags.

Done.

http://codereview.appspot.com/5482051/diff/9002/include/core/SkPath.h#newcode698
include/core/SkPath.h:698: // Primitive degeneracy testing methods
On 2011/12/16 17:23:55, reed1 wrote:
> static functions begin with Capitals in skia.
> 
> I like that these are private. Why are they in SkPath.h? Are they shared by
> other .cpp files, or can these be just local static functions in SkPath.cpp?

I'm going to make these public, so that SkStroke can use the same checks and we
have a single point to change, if needed, the concept of degenerate.

http://codereview.appspot.com/5482051/diff/9002/include/core/SkPoint.h
File include/core/SkPoint.h (right):

http://codereview.appspot.com/5482051/diff/9002/include/core/SkPoint.h#newcod...
include/core/SkPoint.h:320: bool equalsWithinTolerance(const SkPoint& v, const
SkScalar tol) const {
On 2011/12/16 19:24:37, Stephen White wrote:
> Drive-by comment:  Looks like parameter "tol" is ignored here.

It is indeed. Fixed.

http://codereview.appspot.com/5482051/diff/9002/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (left):

http://codereview.appspot.com/5482051/diff/9002/src/core/SkCanvas.cpp#oldcode...
src/core/SkCanvas.cpp:1309: if (path.isEmpty()) {
On 2011/12/16 17:23:55, reed1 wrote:
> Is this change required? I only ask thinking that if the path hasn't already
> cached the answer, then this check might be slower than just an empty check.
> Perhaps this check is required to fix a bug?

This change is needed to avoid warning messages when trying to draw paths. It is
comparable in speed to the old version once the result is cached, and only
slower the first time it is called.

http://codereview.appspot.com/5482051/diff/9002/src/core/SkPath.cpp
File src/core/SkPath.cpp (left):

http://codereview.appspot.com/5482051/diff/9002/src/core/SkPath.cpp#oldcode374
src/core/SkPath.cpp:374: fBoundsIsDirty = false;
On 2011/12/16 17:23:55, reed1 wrote:
> skia adds this-> before calls to methods on self.

Done.

http://codereview.appspot.com/5482051/diff/9002/src/core/SkPath.cpp#oldcode1252
src/core/SkPath.cpp:1252: SkPath::Verb SkPath::Iter::next(SkPoint pts[4]) {
On 2011/12/16 17:23:55, reed1 wrote:
> skia adds this-> before calls to methods on self.

Done.

http://codereview.appspot.com/5482051/diff/9002/src/core/SkPath.cpp
File src/core/SkPath.cpp (right):

http://codereview.appspot.com/5482051/diff/9002/src/core/SkPath.cpp#newcode206
src/core/SkPath.cpp:206: bool SkPath::isDegenerate() const {
On 2011/12/16 17:23:55, reed1 wrote:
> skia adds this-> before calls to methods on self.

Done.

http://codereview.appspot.com/5482051/diff/9002/src/core/SkPath.cpp#newcode401
src/core/SkPath.cpp:401: do {                                                   
   \
On 2011/12/16 17:23:55, reed1 wrote:
> skia adds this-> before calls to methods on self.

Done.

http://codereview.appspot.com/5482051/diff/9002/src/core/SkPath.cpp#newcode1571
src/core/SkPath.cpp:1571: if (!(fFlags & kDegeneracyIsDirty_Bit)) {
On 2011/12/16 17:23:55, reed1 wrote:
> skia adds this-> before calls to methods on self.

Done.

[reed1, 2011-12-16 19:37:02]

http://codereview.appspot.com/5482051/diff/9002/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (left):

http://codereview.appspot.com/5482051/diff/9002/src/core/SkCanvas.cpp#oldcode...
src/core/SkCanvas.cpp:1309: if (path.isEmpty()) {
What warning messages? Slower the first time might mean slower everytime, for
callers that don't cache the paths (which is common for ovals, roundrects).

On 2011/12/16 19:32:29, Stephen Chenney wrote:
> On 2011/12/16 17:23:55, reed1 wrote:
> > Is this change required? I only ask thinking that if the path hasn't already
> > cached the answer, then this check might be slower than just an empty check.
> > Perhaps this check is required to fix a bug?
> 
> This change is needed to avoid warning messages when trying to draw paths. It
is
> comparable in speed to the old version once the result is cached, and only
> slower the first time it is called.

[Stephen Chenney, 2011-12-16 19:57:53]

http://codereview.appspot.com/5482051/diff/9002/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (left):

http://codereview.appspot.com/5482051/diff/9002/src/core/SkCanvas.cpp#oldcode...
src/core/SkCanvas.cpp:1309: if (path.isEmpty()) {
On 2011/12/16 19:37:02, reed1 wrote:
> What warning messages? Slower the first time might mean slower everytime, for
> callers that don't cache the paths (which is common for ovals, roundrects).
> 
> On 2011/12/16 19:32:29, Stephen Chenney wrote:
> > On 2011/12/16 17:23:55, reed1 wrote:
> > > Is this change required? I only ask thinking that if the path hasn't
already
> > > cached the answer, then this check might be slower than just an empty
check.
> > > Perhaps this check is required to fix a bug?
> > 
> > This change is needed to avoid warning messages when trying to draw paths.
It
> is
> > comparable in speed to the old version once the result is cached, and only
> > slower the first time it is called.
> 

"Empty path should have been caught by canvas." comes out of the device when
running gm. And the inverse fill modes do not work correctly. Now inverse fill
doesn't work particularly correctly anyway, so I'm happy to go with isEmpty if
you prefer. The gm test expectations will be wrong, however.

[Stephen Chenney, 2011-12-16 20:00:55]

On 2011/12/16 19:57:53, Stephen Chenney wrote:
> http://codereview.appspot.com/5482051/diff/9002/src/core/SkCanvas.cpp
> File src/core/SkCanvas.cpp (left):
> 
>
http://codereview.appspot.com/5482051/diff/9002/src/core/SkCanvas.cpp#oldcode...
> src/core/SkCanvas.cpp:1309: if (path.isEmpty()) {
> On 2011/12/16 19:37:02, reed1 wrote:
> > What warning messages? Slower the first time might mean slower everytime,
for
> > callers that don't cache the paths (which is common for ovals, roundrects).
> > 
> > On 2011/12/16 19:32:29, Stephen Chenney wrote:
> > > On 2011/12/16 17:23:55, reed1 wrote:
> > > > Is this change required? I only ask thinking that if the path hasn't
> already
> > > > cached the answer, then this check might be slower than just an empty
> check.
> > > > Perhaps this check is required to fix a bug?
> > > 
> > > This change is needed to avoid warning messages when trying to draw paths.
> It
> > is
> > > comparable in speed to the old version once the result is cached, and only
> > > slower the first time it is called.
> > 
> 
> "Empty path should have been caught by canvas." comes out of the device when
> running gm. And the inverse fill modes do not work correctly. Now inverse fill
> doesn't work particularly correctly anyway, so I'm happy to go with isEmpty if
> you prefer. The gm test expectations will be wrong, however.

I should add that, for non-degenerate paths, the cost of the isDegenerate test
is small compared to the overall drawing stack. It creates an iterator and
iterates as long as it needs to to find a non-degenerate segment, which would
normally be the second verb.

[reed1, 2011-12-16 20:18:02]

Sounds like two different things.

1. warning from SkGpuDevice.
2. we don't draw inverse correctly with degenerate paths.

#1 seems odd. I don't know why that warning is there, but it doesn't seem
correct to me.

#2 I'd like to understand and fix this. Is checking for degeneracy the only
right fix, or is it just hiding something that should be fixed deeper?

I will grant you that checking for degeneracy may be very fast (for non
degenerate paths). I guess if we can run the existing path benchmarks w/ and w/o
that change and we don't see a marked change, then we're ok on the performance
side.

[Stephen Chenney, 2011-12-16 20:25:37]

On 2011/12/16 20:18:02, reed1 wrote:
> Sounds like two different things.
> 
> 1. warning from SkGpuDevice.
> 2. we don't draw inverse correctly with degenerate paths.
> 
> #1 seems odd. I don't know why that warning is there, but it doesn't seem
> correct to me.

It seems unnecessary to me. The code is catching and returning anyway. Note that
a path that is not empty can become empty once it is processed through the
SkPath::Iter (even before my change).

> #2 I'd like to understand and fix this. Is checking for degeneracy the only
> right fix, or is it just hiding something that should be fixed deeper?

I think this is a deeper issue that needs it's own bug. Someone with knowledge
of expected behavior should take a look at the gm results when they are
available.

> I will grant you that checking for degeneracy may be very fast (for non
> degenerate paths). I guess if we can run the existing path benchmarks w/ and
w/o
> that change and we don't see a marked change, then we're ok on the performance
> side.

I can look into the perf tests.

[reed1, 2011-12-16 20:35:32]

rev. 2891 removes the printf.

If the inverse-degenerate bug exists outside of this CL, I propose we don't
change the SkCanvas site, but rather submit a bug with a note that changing that
line to isDegenerate seems to fix/hide the bug.

[Stephen Chenney, 2011-12-19 18:20:48]

I have removed all the isDegenerate code, as it is not required now that the
SkPath::Iter code has been modified to clean-up degenerate segments during
iteration.

Everything tests out fine, including rigorous randomized testing for degenerate
segments within paths that previously crashed. Performance changes are
negligible - less than 1% slowdown in the path drawing benchmarks. That seems
worth it to fix crash bugs.

[TomH, 2011-12-19 18:40:22]

Luckily we don't have to perfectly follow the Chromium dictate that no
performance loss is acceptable...

[Stephen Chenney, 2011-12-19 18:48:27]

On 2011/12/19 18:40:22, TomH wrote:
> Luckily we don't have to perfectly follow the Chromium dictate that no
> performance loss is acceptable...

Fixing crashes is often not performance neutral. :-) Nor is fixing
functionality.

[reed1, 2011-12-19 21:49:13]

http://codereview.appspot.com/5482051/diff/9004/include/core/SkPoint.h
File include/core/SkPoint.h (right):

http://codereview.appspot.com/5482051/diff/9004/include/core/SkPoint.h#newcod...
include/core/SkPoint.h:319: */
Skia doesn't use 'const' for p.o.d. parameters, only on structs and classes.

http://codereview.appspot.com/5482051/diff/9004/src/core/SkPath.cpp
File src/core/SkPath.cpp (left):

http://codereview.appspot.com/5482051/diff/9004/src/core/SkPath.cpp#oldcode56
src/core/SkPath.cpp:56: fPath->fBoundsIsDirty = false;
This and the next two all have an extra ; at the end

[reed1, 2011-12-19 22:18:43]

lgtm w/ fixes mentioned in previous post.

[Stephen Chenney, 2011-12-19 23:11:04]

All done. I'll submit in the morning and follow up with instructions on how to
update gm results (if I need to) and how to check the build.

http://codereview.appspot.com/5482051/diff/9004/include/core/SkPoint.h
File include/core/SkPoint.h (right):

http://codereview.appspot.com/5482051/diff/9004/include/core/SkPoint.h#newcod...
include/core/SkPoint.h:319: */
On 2011/12/19 21:49:13, reed1 wrote:
> Skia doesn't use 'const' for p.o.d. parameters, only on structs and classes.

Done.

[TomH, 2011-12-28 18:47:46]

This change seems to have broken the SampleApp Fuzzer; see
http://code.google.com/p/skia/issues/detail?id=425. r2900 Fuzzer runs for
several minutes; r2901 Fuzzer fails an assert within a few frames.