[PDF] Improve complex xfer mode support.

[PDF] Improve complex xfer mode support.

Xfer mode applies only to the shape of the source drawing, not everything in the clip as in currently implemented.  It's just that the current gm examples draw a shape that fills the visible layer.

R=edisonn@google.com, reed@google.com

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

[Chris Guillory, 2011-07-01 00:28:13]

I'm going to need an in person explanation of this change.

http://codereview.appspot.com/4631078/diff/1/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

http://codereview.appspot.com/4631078/diff/1/src/pdf/SkPDFDevice.cpp#newcode466
src/pdf/SkPDFDevice.cpp:466: SkPath* shape = &fShape;
Can this logic be pushed down into finishContentEntry?

[Steve VanDeBogart, 2011-07-01 01:27:48]

http://codereview.appspot.com/4631078/diff/1/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

http://codereview.appspot.com/4631078/diff/1/src/pdf/SkPDFDevice.cpp#newcode500
src/pdf/SkPDFDevice.cpp:500: /* If the shape is different than the alpha
component of the content, then
When drawing an image or a device, we need to call setShape.  This is because
images and devies may have an alpha channel that doesn't match their inherit
shape (rectangular), so if we put them on a form x-object and use the alpha
channel as a mask, we won't get the right result.

http://codereview.appspot.com/4631078/diff/1/src/pdf/SkPDFDevice.cpp#newcode1278
src/pdf/SkPDFDevice.cpp:1278: populateGraphicStateEntryFromPaint(matrix,
*clipStack, clipRegion, paint,
If we already have a shape, we need to make a new content entry.

[Chris Guillory, 2011-07-01 01:30:39]

I'd like to take another look at this. Thanks for explaining.

http://codereview.appspot.com/4631078/diff/1/include/pdf/SkPDFDevice.h
File include/pdf/SkPDFDevice.h (right):

http://codereview.appspot.com/4631078/diff/1/include/pdf/SkPDFDevice.h#newcod...
include/pdf/SkPDFDevice.h:192: void drawFormXObjectWithMask(int xObjectIndex,
Nit: Comment that xObjectIndex is indexing into the xobject resource list for
the device.

http://codereview.appspot.com/4631078/diff/1/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

http://codereview.appspot.com/4631078/diff/1/src/pdf/SkPDFDevice.cpp#newcode477
src/pdf/SkPDFDevice.cpp:477: /* Returns true when we explicitly need the shape
of the drawing. */
Nit: Add in comment that this based on transfer mode.

http://codereview.appspot.com/4631078/diff/1/src/pdf/SkPDFDevice.cpp#newcode478
src/pdf/SkPDFDevice.cpp:478: bool needShape() {
Optional: Can this function name be more verbose.

http://codereview.appspot.com/4631078/diff/1/src/pdf/SkPDFDevice.cpp#newcode493
src/pdf/SkPDFDevice.cpp:493: bool needSource() {
Optional: Perhaps we can inline the logic of needSource or update the function
name so that is reflect more directly that it is really just checking whether or
not we need the clear transfer mode.

http://codereview.appspot.com/4631078/diff/1/src/pdf/SkPDFDevice.cpp#newcode1253
src/pdf/SkPDFDevice.cpp:1253: SkASSERT(isContentEmpty());
This seems unneeded. Maybe place this at the end of the
createFormXObjectFromDevice function if you think it would catch something.

http://codereview.appspot.com/4631078/diff/1/src/pdf/SkPDFDevice.cpp#newcode1321
src/pdf/SkPDFDevice.cpp:1321: SkMatrix matrix = fContentEntries->fState.fMatrix;
I'm not seeing a use of this local in this function.

[Steve VanDeBogart, 2012-10-25 21:14:57]

Here are my replies to Chris's comments.

https://codereview.appspot.com/4631078/diff/1/include/pdf/SkPDFDevice.h
File include/pdf/SkPDFDevice.h (right):

https://codereview.appspot.com/4631078/diff/1/include/pdf/SkPDFDevice.h#newco...
include/pdf/SkPDFDevice.h:192: void drawFormXObjectWithMask(int xObjectIndex,
On 2011/07/01 01:30:39, Chris Guillory wrote:
> Nit: Comment that xObjectIndex is indexing into the xobject resource list for
> the device.

Made argument name more descriptive.

https://codereview.appspot.com/4631078/diff/1/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

https://codereview.appspot.com/4631078/diff/1/src/pdf/SkPDFDevice.cpp#newcode466
src/pdf/SkPDFDevice.cpp:466: SkPath* shape = &fShape;
On 2011/07/01 00:28:13, Chris Guillory wrote:
> Can this logic be pushed down into finishContentEntry?

finishContentEntry doesn't have access to fShape.

https://codereview.appspot.com/4631078/diff/1/src/pdf/SkPDFDevice.cpp#newcode477
src/pdf/SkPDFDevice.cpp:477: /* Returns true when we explicitly need the shape
of the drawing. */
On 2011/07/01 01:30:39, Chris Guillory wrote:
> Nit: Add in comment that this based on transfer mode.

Done.

https://codereview.appspot.com/4631078/diff/1/src/pdf/SkPDFDevice.cpp#newcode478
src/pdf/SkPDFDevice.cpp:478: bool needShape() {
On 2011/07/01 01:30:39, Chris Guillory wrote:
> Optional: Can this function name be more verbose.

Done.

https://codereview.appspot.com/4631078/diff/1/src/pdf/SkPDFDevice.cpp#newcode493
src/pdf/SkPDFDevice.cpp:493: bool needSource() {
On 2011/07/01 01:30:39, Chris Guillory wrote:
> Optional: Perhaps we can inline the logic of needSource or update the function
> name so that is reflect more directly that it is really just checking whether
or
> not we need the clear transfer mode.

Made the name consistent with the new name for needShape.

https://codereview.appspot.com/4631078/diff/1/src/pdf/SkPDFDevice.cpp#newcode500
src/pdf/SkPDFDevice.cpp:500: /* If the shape is different than the alpha
component of the content, then
On 2011/07/01 01:27:48, Steve VanDeBogart wrote:
> When drawing an image or a device, we need to call setShape.  This is because
> images and devies may have an alpha channel that doesn't match their inherit
> shape (rectangular), so if we put them on a form x-object and use the alpha
> channel as a mask, we won't get the right result.

Updated.

https://codereview.appspot.com/4631078/diff/1/src/pdf/SkPDFDevice.cpp#newcode...
src/pdf/SkPDFDevice.cpp:1253: SkASSERT(isContentEmpty());
On 2011/07/01 01:30:39, Chris Guillory wrote:
> This seems unneeded. Maybe place this at the end of the
> createFormXObjectFromDevice function if you think it would catch something.

Done.

https://codereview.appspot.com/4631078/diff/1/src/pdf/SkPDFDevice.cpp#newcode...
src/pdf/SkPDFDevice.cpp:1278: populateGraphicStateEntryFromPaint(matrix,
*clipStack, clipRegion, paint,
On 2011/07/01 01:27:48, Steve VanDeBogart wrote:
> If we already have a shape, we need to make a new content entry.

I can;t figure out what I meant.

https://codereview.appspot.com/4631078/diff/1/src/pdf/SkPDFDevice.cpp#newcode...
src/pdf/SkPDFDevice.cpp:1321: SkMatrix matrix = fContentEntries->fState.fMatrix;
On 2011/07/01 01:30:39, Chris Guillory wrote:
> I'm not seeing a use of this local in this function.

Done.

[Steve VanDeBogart, 2013-10-29 21:09:26]

Mike, Edison, please review.

I cleaned up the xfer mode improvements that were languishing. This also adds
the GM that demonstrates some of the improvements, which was previously
committed and reverted.

[reed1, 2013-10-30 13:27:07]

On 2011/07/01 01:27:48, Steve VanDeBogart wrote:
> http://codereview.appspot.com/4631078/diff/1/src/pdf/SkPDFDevice.cpp
> File src/pdf/SkPDFDevice.cpp (right):
> 
>
http://codereview.appspot.com/4631078/diff/1/src/pdf/SkPDFDevice.cpp#newcode500
> src/pdf/SkPDFDevice.cpp:500: /* If the shape is different than the alpha
> component of the content, then
> When drawing an image or a device, we need to call setShape.  This is because
> images and devies may have an alpha channel that doesn't match their inherit
> shape (rectangular), so if we put them on a form x-object and use the alpha
> channel as a mask, we won't get the right result.
> 
>
http://codereview.appspot.com/4631078/diff/1/src/pdf/SkPDFDevice.cpp#newcode1278
> src/pdf/SkPDFDevice.cpp:1278: populateGraphicStateEntryFromPaint(matrix,
> *clipStack, clipRegion, paint,
> If we already have a shape, we need to make a new content entry.

Good explanations. Would be great if these were captured in the code for future
maintainers.

[reed1, 2013-10-30 13:31:28]

https://codereview.appspot.com/4631078/diff/90002/include/pdf/SkPDFDevice.h
File include/pdf/SkPDFDevice.h (right):

https://codereview.appspot.com/4631078/diff/90002/include/pdf/SkPDFDevice.h#n...
include/pdf/SkPDFDevice.h:284: SkPath* shape);
can this guy be const?

[edisonn, 2013-10-30 14:16:35]

LGTM - only nits, feel free to ignore them

https://codereview.appspot.com/4631078/diff/90002/gm/xfermodes.cpp
File gm/xfermodes.cpp (right):

https://codereview.appspot.com/4631078/diff/90002/gm/xfermodes.cpp#newcode56
gm/xfermodes.cpp:56: * 0x01: A WxH image with a rectangle in the lower right.
create an enum - it would make the code more readable.

https://codereview.appspot.com/4631078/diff/90002/gm/xfermodes.cpp#newcode153
gm/xfermodes.cpp:153: { SkXfermode::kClear_Mode,        "Clear",        0x3F  },
make 0x3f, 0x03 constants

https://codereview.appspot.com/4631078/diff/90002/gm/xfermodes.cpp#newcode202
gm/xfermodes.cpp:202: for (int sourceType = 1; sourceType <= kMaxSourceType;
sourceType *=2) {
maybe use << operator, to show that you move the bit

https://codereview.appspot.com/4631078/diff/90002/include/pdf/SkPDFDevice.h
File include/pdf/SkPDFDevice.h (right):

https://codereview.appspot.com/4631078/diff/90002/include/pdf/SkPDFDevice.h#n...
include/pdf/SkPDFDevice.h:294: int addXObjectResource(SkPDFObject* xObject);
maybe comment what the function does?

https://codereview.appspot.com/4631078/diff/90002/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

https://codereview.appspot.com/4631078/diff/90002/src/pdf/SkPDFDevice.cpp#new...
src/pdf/SkPDFDevice.cpp:1364: return;
comment? If the operation is not supported, add a NYI macro here.

https://codereview.appspot.com/4631078/diff/90002/src/pdf/SkPDFDevice.cpp#new...
src/pdf/SkPDFDevice.cpp:2109: // Assumes that xobject has been canonicalized (so
we can directly compare
I think we should move this comment in the header file

https://codereview.appspot.com/4631078/diff/90002/src/pdf/SkPDFGraphicState.cpp
File src/pdf/SkPDFGraphicState.cpp (right):

https://codereview.appspot.com/4631078/diff/90002/src/pdf/SkPDFGraphicState.c...
src/pdf/SkPDFGraphicState.cpp:50: return NULL;
Should we add here a NYI macro to report we do not support the feature?

[edisonn, 2013-10-30 14:50:03]

https://codereview.appspot.com/4631078/diff/90002/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

https://codereview.appspot.com/4631078/diff/90002/src/pdf/SkPDFDevice.cpp#new...
src/pdf/SkPDFDevice.cpp:2109: // Assumes that xobject has been canonicalized (so
we can directly compare
On 2013/10/30 14:16:36, edisonn wrote:
> I think we should move this comment in the header file

Also, an assumption should be accompanied by an assert, ideally

[Steve VanDeBogart, 2013-10-30 19:17:22]

On 2013/10/30 13:27:07, reed1 wrote:
> On 2011/07/01 01:27:48, Steve VanDeBogart wrote:
> > http://codereview.appspot.com/4631078/diff/1/src/pdf/SkPDFDevice.cpp
> > File src/pdf/SkPDFDevice.cpp (right):
> > 
> >
>
http://codereview.appspot.com/4631078/diff/1/src/pdf/SkPDFDevice.cpp#newcode500
> > src/pdf/SkPDFDevice.cpp:500: /* If the shape is different than the alpha
> > component of the content, then
> > When drawing an image or a device, we need to call setShape.  This is
because
> > images and devies may have an alpha channel that doesn't match their inherit
> > shape (rectangular), so if we put them on a form x-object and use the alpha
> > channel as a mask, we won't get the right result.
> > 
> >
>
http://codereview.appspot.com/4631078/diff/1/src/pdf/SkPDFDevice.cpp#newcode1278
> > src/pdf/SkPDFDevice.cpp:1278: populateGraphicStateEntryFromPaint(matrix,
> > *clipStack, clipRegion, paint,
> > If we already have a shape, we need to make a new content entry.
> 
> Good explanations. Would be great if these were captured in the code for
future
> maintainers.

This above comment is pretty much already included in the code, I touched it up
a bit.  Let me know if you would like something more somewhere.

https://codereview.appspot.com/4631078/diff/90002/gm/xfermodes.cpp
File gm/xfermodes.cpp (right):

https://codereview.appspot.com/4631078/diff/90002/gm/xfermodes.cpp#newcode56
gm/xfermodes.cpp:56: * 0x01: A WxH image with a rectangle in the lower right.
On 2013/10/30 14:16:36, edisonn wrote:
> create an enum - it would make the code more readable.

Done.

https://codereview.appspot.com/4631078/diff/90002/gm/xfermodes.cpp#newcode153
gm/xfermodes.cpp:153: { SkXfermode::kClear_Mode,        "Clear",        0x3F  },
On 2013/10/30 14:16:36, edisonn wrote:
> make 0x3f, 0x03 constants

Done.

https://codereview.appspot.com/4631078/diff/90002/gm/xfermodes.cpp#newcode202
gm/xfermodes.cpp:202: for (int sourceType = 1; sourceType <= kMaxSourceType;
sourceType *=2) {
On 2013/10/30 14:16:36, edisonn wrote:
> maybe use << operator, to show that you move the bit 

Done.

https://codereview.appspot.com/4631078/diff/90002/include/pdf/SkPDFDevice.h
File include/pdf/SkPDFDevice.h (right):

https://codereview.appspot.com/4631078/diff/90002/include/pdf/SkPDFDevice.h#n...
include/pdf/SkPDFDevice.h:284: SkPath* shape);
On 2013/10/30 13:31:28, reed1 wrote:
> can this guy be const?

shape? Done.

https://codereview.appspot.com/4631078/diff/90002/include/pdf/SkPDFDevice.h#n...
include/pdf/SkPDFDevice.h:294: int addXObjectResource(SkPDFObject* xObject);
On 2013/10/30 14:16:36, edisonn wrote:
> maybe comment what the function does?

Done.

https://codereview.appspot.com/4631078/diff/90002/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

https://codereview.appspot.com/4631078/diff/90002/src/pdf/SkPDFDevice.cpp#new...
src/pdf/SkPDFDevice.cpp:1364: return;
On 2013/10/30 14:16:36, edisonn wrote:
> comment? If the operation is not supported, add a NYI macro here.

Updated the comment for needSource().  This is WAI and not NYI.

https://codereview.appspot.com/4631078/diff/90002/src/pdf/SkPDFDevice.cpp#new...
src/pdf/SkPDFDevice.cpp:2109: // Assumes that xobject has been canonicalized (so
we can directly compare
On 2013/10/30 14:50:04, edisonn wrote:
> On 2013/10/30 14:16:36, edisonn wrote:
> > I think we should move this comment in the header file
> 
> Also, an assumption should be accompanied by an assert, ideally

Header comment has been updated.  There is no way to assert that this assumption
is true.  However, if the assumption is not true, there will be more copies of
the xobject than we would like, but the result will still function properly.

https://codereview.appspot.com/4631078/diff/90002/src/pdf/SkPDFGraphicState.cpp
File src/pdf/SkPDFGraphicState.cpp (right):

https://codereview.appspot.com/4631078/diff/90002/src/pdf/SkPDFGraphicState.c...
src/pdf/SkPDFGraphicState.cpp:50: return NULL;
On 2013/10/30 14:16:36, edisonn wrote:
> Should we add here a NYI macro to report we do not support the feature?

Done.

[edisonn, 2013-10-30 19:32:57]

LGTM

[reed1, 2013-10-30 20:08:09]

lgtm

is this going to land, since it is not using the chromium codereview site?

[Steve VanDeBogart, 2013-10-30 20:24:30]

On 2013/10/30 20:08:09, reed1 wrote:
> lgtm
> 
> is this going to land, since it is not using the chromium codereview site?

I can reupload to chromium codereview and use the CQ, or just dcommit.

[Steve VanDeBogart, 2013-10-30 20:48:12]

Committed patchset #8 manually as r12034 (presubmit successful).