Inverse fill support in PDF

Inverse fill support in PDF

BUG= https://code.google.com/p/skia/issues/detail?id=241 (partial fix)

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

[ducky, 2013-07-25 03:32:20]

This CL emulates inverse fill output in PDF by using Path Ops. It's currently
disabled by a #define guard since Cary advises against shipping Path Ops code.

Inverse clip support will come in a separate, later CL...

https://codereview.chromium.org/19519017/diff/1/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (left):

https://codereview.chromium.org/19519017/diff/1/src/pdf/SkPDFDevice.cpp#oldco...
src/pdf/SkPDFDevice.cpp:829: pathIsMutable = true;
This code really doesn't do anything (I think, at least), so I cleaned it out.

[vandebo (ex-Chrome), 2013-07-26 17:48:44]

https://codereview.chromium.org/19519017/diff/20001/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (left):

https://codereview.chromium.org/19519017/diff/20001/src/pdf/SkPDFDevice.cpp#o...
src/pdf/SkPDFDevice.cpp:829: pathIsMutable = true;
Even though this isn't (currently) used after this point, please leave it in so
that if it does get used after this point, it will be accurate.

https://codereview.chromium.org/19519017/diff/20001/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

https://codereview.chromium.org/19519017/diff/20001/src/pdf/SkPDFDevice.cpp#n...
src/pdf/SkPDFDevice.cpp:775: void SkPDFDevice::drawRect(const SkDraw& d, const
SkRect& rect,
Shouldn't this handle inverse fill as well?

https://codereview.chromium.org/19519017/diff/20001/src/pdf/SkPDFDevice.cpp#n...
src/pdf/SkPDFDevice.cpp:846: if (origPath.isInverseFillType()) {
Lets put this all into a method: handleInverseFill()

https://codereview.chromium.org/19519017/diff/20001/src/pdf/SkPDFDevice.cpp#n...
src/pdf/SkPDFDevice.cpp:846: if (origPath.isInverseFillType()) {
If paint style is just stroke do we need to do anything special?  Or does
inverse fill with stroke mean to stroke everywhere but the path?

https://codereview.chromium.org/19519017/diff/20001/src/pdf/SkPDFDevice.cpp#n...
src/pdf/SkPDFDevice.cpp:850: if (!pathIsMutable) {
Pull this into the if on line 857 so that you can get rid of the else clause.

https://codereview.chromium.org/19519017/diff/20001/src/pdf/SkPDFDevice.cpp#n...
src/pdf/SkPDFDevice.cpp:851: pathPtr = &modifiedPath;
pathIsMutable = true;

https://codereview.chromium.org/19519017/diff/20001/src/pdf/SkPDFDevice.cpp#n...
src/pdf/SkPDFDevice.cpp:854: SkPaint finalPaint(paint);
nit: finalPaint -> noInversePaint

https://codereview.chromium.org/19519017/diff/20001/src/pdf/SkPDFDevice.cpp#n...
src/pdf/SkPDFDevice.cpp:858: || SkPaint::kStrokeAndFill_Style ==
paint.getStyle()) {
predominate style in this file (and Chrome style) is to put || at the end of the
line.

https://codereview.chromium.org/19519017/diff/20001/src/pdf/SkPDFDevice.cpp#n...
src/pdf/SkPDFDevice.cpp:863: // Hairline strokes rendered as non-inverted.
What does this comment mean?

[edisonn, 2013-07-26 20:23:52]

run it please on all gms and on all pdfs we generate with render_pdf

https://codereview.chromium.org/19519017/diff/20001/gm/inversestrokefill.cpp
File gm/inversestrokefill.cpp (right):

https://codereview.chromium.org/19519017/diff/20001/gm/inversestrokefill.cpp#...
gm/inversestrokefill.cpp:35: paint.setStrokeWidth(SkIntToScalar(5));
add option for stoke width 0

https://codereview.chromium.org/19519017/diff/20001/gm/inversestrokefill.cpp#...
gm/inversestrokefill.cpp:36: paint.setStyle(SkPaint::kStrokeAndFill_Style);
what about the rest of styles?

https://codereview.chromium.org/19519017/diff/20001/gm/inversestrokefill.cpp#...
gm/inversestrokefill.cpp:41: canvas->drawPath(path, paint);
please add tests for other addFoo operations. some of them take shurtcuts, that
might expose bugs (drawRect, draw empty rect, draw rect as line, draw... )

[reed1, 2013-07-29 13:09:09]

https://codereview.chromium.org/19519017/diff/20001/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

https://codereview.chromium.org/19519017/diff/20001/src/pdf/SkPDFDevice.cpp#n...
src/pdf/SkPDFDevice.cpp:775: void SkPDFDevice::drawRect(const SkDraw& d, const
SkRect& rect,
On 2013/07/26 17:48:44, vandebo wrote:
> Shouldn't this handle inverse fill as well?

Inverseness is on the SkPath, not the paint, so there is no way to ask for
inverse-rect.

[ducky, 2013-07-29 19:07:51]

Fixed stylistic issues and replaced the inverse stroke fill GM with a more
comprehensive inverse paths GM.

https://codereview.chromium.org/19519017/diff/20001/gm/inversestrokefill.cpp
File gm/inversestrokefill.cpp (right):

https://codereview.chromium.org/19519017/diff/20001/gm/inversestrokefill.cpp#...
gm/inversestrokefill.cpp:35: paint.setStrokeWidth(SkIntToScalar(5));
On 2013/07/26 20:23:52, edisonn wrote:
> add option for stoke width 0

Done. The original intent of this GM was to test only combination stroke and
fill operations (where the stroke is bigger than the shape) as many other cases
are implicitly tested in other GMs like cubicpaths. I've changed this to a
comprehensive inverse paths GM now, and added some corner case tests.

https://codereview.chromium.org/19519017/diff/20001/gm/inversestrokefill.cpp#...
gm/inversestrokefill.cpp:36: paint.setStyle(SkPaint::kStrokeAndFill_Style);
On 2013/07/26 20:23:52, edisonn wrote:
> what about the rest of styles?

Done.

https://codereview.chromium.org/19519017/diff/20001/gm/inversestrokefill.cpp#...
gm/inversestrokefill.cpp:41: canvas->drawPath(path, paint);
On 2013/07/26 20:23:52, edisonn wrote:
> please add tests for other addFoo operations. some of them take shurtcuts,
that
> might expose bugs (drawRect, draw empty rect, draw rect as line, draw... )

Done.

https://codereview.chromium.org/19519017/diff/20001/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (left):

https://codereview.chromium.org/19519017/diff/20001/src/pdf/SkPDFDevice.cpp#o...
src/pdf/SkPDFDevice.cpp:829: pathIsMutable = true;
On 2013/07/26 17:48:44, vandebo wrote:
> Even though this isn't (currently) used after this point, please leave it in
so
> that if it does get used after this point, it will be accurate.

Done.

https://codereview.chromium.org/19519017/diff/20001/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

https://codereview.chromium.org/19519017/diff/20001/src/pdf/SkPDFDevice.cpp#n...
src/pdf/SkPDFDevice.cpp:846: if (origPath.isInverseFillType()) {
On 2013/07/26 17:48:44, vandebo wrote:
> If paint style is just stroke do we need to do anything special?  Or does
> inverse fill with stroke mean to stroke everywhere but the path?

Inverse stroke is kind of funny:
For a hairline (or width 1) stroke, it means do the positive stroke (non
inverted).
For thicker strokes, it means inverse stroke (draw everywhere but the path).

https://codereview.chromium.org/19519017/diff/20001/src/pdf/SkPDFDevice.cpp#n...
src/pdf/SkPDFDevice.cpp:846: if (origPath.isInverseFillType()) {
On 2013/07/26 17:48:44, vandebo wrote:
> Lets put this all into a method: handleInverseFill()

Done.

https://codereview.chromium.org/19519017/diff/20001/src/pdf/SkPDFDevice.cpp#n...
src/pdf/SkPDFDevice.cpp:850: if (!pathIsMutable) {
On 2013/07/26 17:48:44, vandebo wrote:
> Pull this into the if on line 857 so that you can get rid of the else clause.

Done - though this whole PathIsMutable thing is a fair bit of complexity just to
save one extra copy operation in some cases. Is it worth it?

https://codereview.chromium.org/19519017/diff/20001/src/pdf/SkPDFDevice.cpp#n...
src/pdf/SkPDFDevice.cpp:851: pathPtr = &modifiedPath;
On 2013/07/26 17:48:44, vandebo wrote:
> pathIsMutable = true;

Done.

https://codereview.chromium.org/19519017/diff/20001/src/pdf/SkPDFDevice.cpp#n...
src/pdf/SkPDFDevice.cpp:854: SkPaint finalPaint(paint);
On 2013/07/26 17:48:44, vandebo wrote:
> nit: finalPaint -> noInversePaint

Done.

https://codereview.chromium.org/19519017/diff/20001/src/pdf/SkPDFDevice.cpp#n...
src/pdf/SkPDFDevice.cpp:858: || SkPaint::kStrokeAndFill_Style ==
paint.getStyle()) {
On 2013/07/26 17:48:44, vandebo wrote:
> predominate style in this file (and Chrome style) is to put || at the end of
the
> line.

Done.

https://codereview.chromium.org/19519017/diff/20001/src/pdf/SkPDFDevice.cpp#n...
src/pdf/SkPDFDevice.cpp:863: // Hairline strokes rendered as non-inverted.
On 2013/07/26 17:48:44, vandebo wrote:
> What does this comment mean?

See above explanation of inverse hairline strokes. I'll expand this comment to
be more clear, though.

[vandebo (ex-Chrome), 2013-07-30 16:50:42]

https://codereview.chromium.org/19519017/diff/20001/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

https://codereview.chromium.org/19519017/diff/20001/src/pdf/SkPDFDevice.cpp#n...
src/pdf/SkPDFDevice.cpp:850: if (!pathIsMutable) {
On 2013/07/29 19:07:51, ducky wrote:
> On 2013/07/26 17:48:44, vandebo wrote:
> > Pull this into the if on line 857 so that you can get rid of the else
clause.
> 
> Done - though this whole PathIsMutable thing is a fair bit of complexity just
to
> save one extra copy operation in some cases. Is it worth it?

It depends on how big the thing you are copying is.  A path can be very complex
though.

https://codereview.chromium.org/19519017/diff/69001/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

https://codereview.chromium.org/19519017/diff/69001/src/pdf/SkPDFDevice.cpp#n...
src/pdf/SkPDFDevice.cpp:847: if (origPath.isInverseFillType()) {
nit: Push this into the handle method...

if (handleInversePath(...)) {
    return;
}

https://codereview.chromium.org/19519017/diff/69001/src/pdf/SkPDFDevice.cpp#n...
src/pdf/SkPDFDevice.cpp:1246: bool calculate_inverse_path(const SkRect& bounds,
const SkPath& invPath,
You can inline this function unless you intend to reuse it for something else.

https://codereview.chromium.org/19519017/diff/69001/src/pdf/SkPDFDevice.cpp#n...
src/pdf/SkPDFDevice.cpp:1279: // To be consistent with the raster output,
hairline strokes
At this point pathPtr points to modifiedPath that is default constructed.

https://codereview.chromium.org/19519017/diff/69001/src/pdf/SkPDFDevice.cpp#n...
src/pdf/SkPDFDevice.cpp:1283: return;
After you've changed this to return a bool though, you can just return false and
have the rest of the path code handle it without needing to toggle the inverse
fill bit.

[vandebo (ex-Chrome), 2013-07-30 17:34:34]

https://codereview.chromium.org/19519017/diff/69001/gm/inversepaths.cpp
File gm/inversepaths.cpp (right):

https://codereview.chromium.org/19519017/diff/69001/gm/inversepaths.cpp#newco...
gm/inversepaths.cpp:95: outlinePaint.setColor(0x20000000);
contant?

https://codereview.chromium.org/19519017/diff/69001/gm/inversepaths.cpp#newco...
gm/inversepaths.cpp:111: paint.setColor(0xff007000);
constant?

[ducky, 2013-07-30 22:01:55]

https://codereview.chromium.org/19519017/diff/69001/gm/inversepaths.cpp
File gm/inversepaths.cpp (right):

https://codereview.chromium.org/19519017/diff/69001/gm/inversepaths.cpp#newco...
gm/inversepaths.cpp:95: outlinePaint.setColor(0x20000000);
On 2013/07/30 17:34:35, vandebo wrote:
> contant?

Doesn't seem to be any defined 1/4 opaque constant...

https://codereview.chromium.org/19519017/diff/69001/gm/inversepaths.cpp#newco...
gm/inversepaths.cpp:111: paint.setColor(0xff007000);
On 2013/07/30 17:34:35, vandebo wrote:
> constant?

This was taken from the cubic paths GM, and the stock colors available in
SkColor.h don't really work that well here...

https://codereview.chromium.org/19519017/diff/69001/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

https://codereview.chromium.org/19519017/diff/69001/src/pdf/SkPDFDevice.cpp#n...
src/pdf/SkPDFDevice.cpp:847: if (origPath.isInverseFillType()) {
On 2013/07/30 16:50:42, vandebo wrote:
> nit: Push this into the handle method...
> 
> if (handleInversePath(...)) {
>     return;
> }

Done.

https://codereview.chromium.org/19519017/diff/69001/src/pdf/SkPDFDevice.cpp#n...
src/pdf/SkPDFDevice.cpp:1246: bool calculate_inverse_path(const SkRect& bounds,
const SkPath& invPath,
On 2013/07/30 16:50:42, vandebo wrote:
> You can inline this function unless you intend to reuse it for something else.

I probably should have noted that this was to get re-used in the upcoming Path
Ops clips GM.

https://codereview.chromium.org/19519017/diff/69001/src/pdf/SkPDFDevice.cpp#n...
src/pdf/SkPDFDevice.cpp:1279: // To be consistent with the raster output,
hairline strokes
On 2013/07/30 16:50:42, vandebo wrote:
> At this point pathPtr points to modifiedPath that is default constructed.

I think paint.getFillPath still duplicates the paint into pathPtr. Though I can
see how the style could be confusing, so I'll clarify it.
In either case, the GMs seem to come out sane.

https://codereview.chromium.org/19519017/diff/69001/src/pdf/SkPDFDevice.cpp#n...
src/pdf/SkPDFDevice.cpp:1283: return;
On 2013/07/30 16:50:42, vandebo wrote:
> After you've changed this to return a bool though, you can just return false
and
> have the rest of the path code handle it without needing to toggle the inverse
> fill bit.
I can see how that would be faster, but it seems less clean in that an inverse
path would continue down drawPath. It also seems like the required overhead is
minimal - just one function call, since I think getFillPath does a copy
regardless.

[vandebo (ex-Chrome), 2013-07-30 22:27:14]

LGTM

https://codereview.chromium.org/19519017/diff/87001/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

https://codereview.chromium.org/19519017/diff/87001/src/pdf/SkPDFDevice.cpp#n...
src/pdf/SkPDFDevice.cpp:1288: drawPath(d, *pathPtr, paint, NULL, true);
nit: *pathPtr -> modifiedPath

[ducky, 2013-07-30 22:45:44]

https://codereview.chromium.org/19519017/diff/87001/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

https://codereview.chromium.org/19519017/diff/87001/src/pdf/SkPDFDevice.cpp#n...
src/pdf/SkPDFDevice.cpp:1288: drawPath(d, *pathPtr, paint, NULL, true);
On 2013/07/30 22:27:15, vandebo wrote:
> nit: *pathPtr -> modifiedPath

Ehh, I think this is slightly cleaner, as paint.getFillPath writes to pathPtr
(though the assignment for pathPtr to modifiedPath is right above it), so the
flow of values is a bit more apparent. What do you think?

[vandebo (ex-Chrome), 2013-07-30 22:52:02]

https://codereview.chromium.org/19519017/diff/87001/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

https://codereview.chromium.org/19519017/diff/87001/src/pdf/SkPDFDevice.cpp#n...
src/pdf/SkPDFDevice.cpp:1288: drawPath(d, *pathPtr, paint, NULL, true);
On 2013/07/30 22:45:44, ducky wrote:
> On 2013/07/30 22:27:15, vandebo wrote:
> > nit: *pathPtr -> modifiedPath
> 
> Ehh, I think this is slightly cleaner, as paint.getFillPath writes to pathPtr
> (though the assignment for pathPtr to modifiedPath is right above it), so the
> flow of values is a bit more apparent. What do you think?

getFillPath could write to modifiedPath if you like.  The reason I think
modifiedPath should be passed here is because of the last arg - pathIsMutable. 
It's easier to see that it is correct if you pass modifiedPath.

[ducky, 2013-07-30 23:18:26]

Code should be a bit cleaner now.

https://codereview.chromium.org/19519017/diff/87001/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

https://codereview.chromium.org/19519017/diff/87001/src/pdf/SkPDFDevice.cpp#n...
src/pdf/SkPDFDevice.cpp:1288: drawPath(d, *pathPtr, paint, NULL, true);
On 2013/07/30 22:52:02, vandebo wrote:
> On 2013/07/30 22:45:44, ducky wrote:
> > On 2013/07/30 22:27:15, vandebo wrote:
> > > nit: *pathPtr -> modifiedPath
> > 
> > Ehh, I think this is slightly cleaner, as paint.getFillPath writes to
pathPtr
> > (though the assignment for pathPtr to modifiedPath is right above it), so
the
> > flow of values is a bit more apparent. What do you think?
> 
> getFillPath could write to modifiedPath if you like.  The reason I think
> modifiedPath should be passed here is because of the last arg - pathIsMutable.

> It's easier to see that it is correct if you pass modifiedPath.

Done.

[vandebo (ex-Chrome), 2013-07-31 18:47:10]

LGTM with nit.

https://codereview.chromium.org/19519017/diff/101001/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

https://codereview.chromium.org/19519017/diff/101001/src/pdf/SkPDFDevice.cpp#...
src/pdf/SkPDFDevice.cpp:1246: SkPath* outPath) {
nit: indent.

[commit-bot: I haz the power, 2013-07-31 22:47:50]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/richardlin@chromium.org/19519017/106001

[commit-bot: I haz the power, 2013-07-31 22:54:43]

Change committed as 10476