[PDF] Ensure that filling single line paths (with no area) does not draw anything.

[PDF] Ensure that filling single line paths (with no area) does not draw anything.

Add a test to a gm to confirm the new behavior.

Fixes http://crbug.com/123072

Committed: https://code.google.com/p/skia/source/detail?r=3884

[Steve VanDeBogart, 2012-04-30 23:40:10]

PTAL. I'm not particularly happy with this approach so I'm happy to consider
alternatives.

[reed1, 2012-05-01 12:33:05]

Is an equivalent fix the ability to detect contours with only 2 (non-degenerate)
line segments, and just ignore those?

[Steve VanDeBogart, 2012-05-01 17:07:40]

On 2012/05/01 12:33:05, reed1 wrote:
> Is an equivalent fix the ability to detect contours with only 2
(non-degenerate)
> line segments, and just ignore those?

That was my first approach but I then ran into the degenerate segment issue.  At
that point I realized that you could have multiple, non-degenerate line segments
that were all colinear.  Chrome probably doesn't do that, but just checking for
two point linear paths isn't extensible to solve the entire problem whereas this
approach is (with a bit more effort).

[reed1, 2012-05-01 17:41:34]

given that this is a fuzzy problem (fuzzy in that its not obvious how to state
which cases are problematic, and hard to write a test ensuring that we catch/fix
all of them), can we first implement this outside of SkPath.cpp, as a utility
that sites on top of SkPath::Iter? That way it could evolve as needed inside of
the PDF private code base.

[Steve VanDeBogart, 2012-05-02 20:50:20]

On 2012/05/01 17:41:34, reed1 wrote:
> given that this is a fuzzy problem (fuzzy in that its not obvious how to state
> which cases are problematic, and hard to write a test ensuring that we
catch/fix
> all of them), can we first implement this outside of SkPath.cpp, as a utility
> that sites on top of SkPath::Iter? That way it could evolve as needed inside
of
> the PDF private code base.

I think it's clear what the problematic cases are (filling a path that has no
area results in PDF output when it should not).  But I don't think we need to
fix all instance of the problem to make it disappear for Chrome (Chrome likely
only has linear paths with no area).

We could implement it completely outside core, but there would be an extra
performance cost (either we need to iterate the path twice, or store some
representation of the pending segment) or we could add a friend to SkPath/
SkPath::Iter and have a PDFPathIter implementation.

[reed1, 2012-05-03 12:14:34]

My preference would be to implement it outside initially, since it is only
needed by PDF. We can then time it and see if it impacts the overall PDF
generation time. If so, we can look into more invasive approaches.

[reed1, 2012-05-09 12:53:41]

lgtm (w/ suggestion)

https://codereview.appspot.com/6137060/diff/7001/src/pdf/SkPDFUtils.h
File src/pdf/SkPDFUtils.h (left):

https://codereview.appspot.com/6137060/diff/7001/src/pdf/SkPDFUtils.h#oldcode45
src/pdf/SkPDFUtils.h:45: static void EmitPath(const SkPath& path, SkWStream*
content);
It might be easier to interpret the call-sites, if this took SkPath::FillType as
its last parameter, instead of a bool. Just a thought.

[Steve VanDeBogart, 2012-05-09 17:07:51]

Use SkPaint::Style and fix argument order

[Steve VanDeBogart, 2012-05-09 17:09:09]

https://codereview.appspot.com/6137060/diff/7001/src/pdf/SkPDFUtils.h
File src/pdf/SkPDFUtils.h (left):

https://codereview.appspot.com/6137060/diff/7001/src/pdf/SkPDFUtils.h#oldcode45
src/pdf/SkPDFUtils.h:45: static void EmitPath(const SkPath& path, SkWStream*
content);
On 2012/05/09 12:53:41, reed1 wrote:
> It might be easier to interpret the call-sites, if this took SkPath::FillType
as
> its last parameter, instead of a bool. Just a thought.

We want SkPaint::Style, not SkPath::FileType, but done.  I also fixed up the arg
order (stream at the end).

[reed1, 2012-05-09 17:16:28]

doh, right. SkPaint::Style.

lgtm