[PDF] Add support for SrcIn, SrcOut, DstIn, DstOut xfermodes.

[PDF] Add support for SrcIn, SrcOut, DstIn, DstOut xfermodes.

This change uses the soft mask (aka soft clip) functionality of PDF to implement the xfermodes.  It has to put existing content (dst) into a form xobject as well as putting the new (src) content into a different form xobject.  It then draws one of them with the other as the soft mask.
To accomplish this, we add a call to finishContentEntry after each call to setUpContentEntry - this is kind of a hack, but I don't see a better way to extract src.
Unfortunately, soft mask is specified in the Graphic State PDF object (and not in the form xobject), so when handling one of these modes, we add a one time GS object to set the soft mask and invoke a simple GS to reset the soft mask when done.

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

[Steve VanDeBogart, 2011-05-10 23:56:08]

ping

[reed1, 2011-05-11 00:29:27]

some questions about ownership. Deferring to chris for PDF-ness of the change.

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

http://codereview.appspot.com/4496041/diff/1/src/pdf/SkPDFGraphicState.cpp#ne...
src/pdf/SkPDFGraphicState.cpp:72: resourceList->push(fResources[i]);
Just curious: what is the ownership rules here? Does the call to push() alter
something? Why the call to ref() after the push?

http://codereview.appspot.com/4496041/diff/1/src/pdf/SkPDFGraphicState.cpp#ne...
src/pdf/SkPDFGraphicState.cpp:162: invertFunc->ref();
why this call to ref(), with the call to unref() 7 lines above?

http://codereview.appspot.com/4496041/diff/1/src/pdf/SkPDFGraphicState.cpp#ne...
src/pdf/SkPDFGraphicState.cpp:168: // static
1. is this thread safe to return a global object? Is it ever modifed after this
point?
2. what is the ownership rule for calling getNoSMaskGraphicsState? Normally in
skia calling a "get" function does not modify the owner count.

[Chris Guillory, 2011-05-11 22:15:44]

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

http://codereview.appspot.com/4496041/diff/1/include/pdf/SkPDFGraphicState.h#...
include/pdf/SkPDFGraphicState.h:66: /** Get a graphic state that only unsets the
passed soft mask.  The
"the passed soft mask"? There are no arguments to getNoSMaskGraphicState().

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

http://codereview.appspot.com/4496041/diff/1/src/pdf/SkPDFDevice.cpp#newcode655
src/pdf/SkPDFDevice.cpp:655: 
Nit: Extra newline.

http://codereview.appspot.com/4496041/diff/1/src/pdf/SkPDFDevice.cpp#newcode1047
src/pdf/SkPDFDevice.cpp:1047: bool SkPDFDevice::setUpContentEntry(const
SkClipStack& clipStack,
Optional: Just a thought but maybe we can have a new class (AutoContentEntry?)
that we construct on the stack and it can perform the logic of this function and
save the return value. It's destructor it can call perform finishContentEntry
logic when the saved return value was true. Thoughts?

http://codereview.appspot.com/4496041/diff/1/src/pdf/SkPDFDevice.cpp#newcode1080
src/pdf/SkPDFDevice.cpp:1080: fTmpFormXObject = new SkPDFFormXObject(this);
fTmpFormXObject -> fSrcFormXObject?

http://codereview.appspot.com/4496041/diff/1/src/pdf/SkPDFDevice.cpp#newcode1080
src/pdf/SkPDFDevice.cpp:1080: fTmpFormXObject = new SkPDFFormXObject(this);
SkASSERT(fTmpFormXObject.get() == NULL);?

http://codereview.appspot.com/4496041/diff/1/src/pdf/SkPDFDevice.cpp#newcode1080
src/pdf/SkPDFDevice.cpp:1080: fTmpFormXObject = new SkPDFFormXObject(this);
Perhaps we can wrap these four lines into a another member function?
createFormXObjectFromDevice(SkRefPtr<SkPDFFormXObject>*)?

http://codereview.appspot.com/4496041/diff/1/src/pdf/SkPDFDevice.cpp#newcode1150
src/pdf/SkPDFDevice.cpp:1150: return;
SkAssert(fTmpFormXObject.get() == NULL);?

http://codereview.appspot.com/4496041/diff/1/src/pdf/SkPDFDevice.cpp#newcode1153
src/pdf/SkPDFDevice.cpp:1153: SkRefPtr<SkPDFFormXObject> srcFormXObject = new
SkPDFFormXObject(this);
srcFormXObject->dstFormXObject

[Steve VanDeBogart, 2011-05-12 17:51:39]

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

http://codereview.appspot.com/4496041/diff/1/include/pdf/SkPDFGraphicState.h#...
include/pdf/SkPDFGraphicState.h:66: /** Get a graphic state that only unsets the
passed soft mask.  The
On 2011/05/11 22:15:44, Chris Guillory wrote:
> "the passed soft mask"? There are no arguments to getNoSMaskGraphicState().

Done.

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

http://codereview.appspot.com/4496041/diff/1/src/pdf/SkPDFDevice.cpp#newcode655
src/pdf/SkPDFDevice.cpp:655: 
On 2011/05/11 22:15:44, Chris Guillory wrote:
> Nit: Extra newline.

Done.

http://codereview.appspot.com/4496041/diff/1/src/pdf/SkPDFDevice.cpp#newcode1047
src/pdf/SkPDFDevice.cpp:1047: bool SkPDFDevice::setUpContentEntry(const
SkClipStack& clipStack,
On 2011/05/11 22:15:44, Chris Guillory wrote:
> Optional: Just a thought but maybe we can have a new class (AutoContentEntry?)
> that we construct on the stack and it can perform the logic of this function
and
> save the return value. It's destructor it can call perform finishContentEntry
> logic when the saved return value was true. Thoughts?

Sounds like a good idea.  I did it (in patch set 2), but didn't like how it
turned out, so I removed it and uploaded patch set 3.  If you have comments
about how to improve the implementation I did, or why you like it, happy to
consider bringing it back.

http://codereview.appspot.com/4496041/diff/1/src/pdf/SkPDFDevice.cpp#newcode1080
src/pdf/SkPDFDevice.cpp:1080: fTmpFormXObject = new SkPDFFormXObject(this);
On 2011/05/11 22:15:44, Chris Guillory wrote:
> Perhaps we can wrap these four lines into a another member function?
> createFormXObjectFromDevice(SkRefPtr<SkPDFFormXObject>*)?

Done.

http://codereview.appspot.com/4496041/diff/1/src/pdf/SkPDFDevice.cpp#newcode1080
src/pdf/SkPDFDevice.cpp:1080: fTmpFormXObject = new SkPDFFormXObject(this);
On 2011/05/11 22:15:44, Chris Guillory wrote:
> SkASSERT(fTmpFormXObject.get() == NULL);?

Done.

http://codereview.appspot.com/4496041/diff/1/src/pdf/SkPDFDevice.cpp#newcode1080
src/pdf/SkPDFDevice.cpp:1080: fTmpFormXObject = new SkPDFFormXObject(this);
On 2011/05/11 22:15:44, Chris Guillory wrote:
> fTmpFormXObject -> fSrcFormXObject?

-> fDstFormXObject

http://codereview.appspot.com/4496041/diff/1/src/pdf/SkPDFDevice.cpp#newcode1150
src/pdf/SkPDFDevice.cpp:1150: return;
On 2011/05/11 22:15:44, Chris Guillory wrote:
> SkAssert(fTmpFormXObject.get() == NULL);?

Done.

http://codereview.appspot.com/4496041/diff/1/src/pdf/SkPDFDevice.cpp#newcode1153
src/pdf/SkPDFDevice.cpp:1153: SkRefPtr<SkPDFFormXObject> srcFormXObject = new
SkPDFFormXObject(this);
On 2011/05/11 22:15:44, Chris Guillory wrote:
> srcFormXObject->dstFormXObject

src is the new thing, dst is what was already there.

http://codereview.appspot.com/4496041/diff/1/src/pdf/SkPDFDevice.cpp#newcode1180
src/pdf/SkPDFDevice.cpp:1180: fCurContentEntry->fContent.writeText("/G");
Also pulled this set of lines into a function

http://codereview.appspot.com/4496041/diff/1/src/pdf/SkPDFDevice.cpp#newcode1185
src/pdf/SkPDFDevice.cpp:1185: fCurContentEntry->fContent.writeText("/X");
And this set.

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

http://codereview.appspot.com/4496041/diff/1/src/pdf/SkPDFGraphicState.cpp#ne...
src/pdf/SkPDFGraphicState.cpp:72: resourceList->push(fResources[i]);
On 2011/05/11 00:29:27, reed1 wrote:
> Just curious: what is the ownership rules here? Does the call to push() alter
> something? Why the call to ref() after the push?

fResources is an SkTDArray<SkPDFObject*>, so pushing fResources[i] doesn't ref
anything.  We need to ref it in case this class holds onto the resourceList
longer than this class exists.

http://codereview.appspot.com/4496041/diff/1/src/pdf/SkPDFGraphicState.cpp#ne...
src/pdf/SkPDFGraphicState.cpp:162: invertFunc->ref();
On 2011/05/11 00:29:27, reed1 wrote:
> why this call to ref(), with the call to unref() 7 lines above?

Habit... SkRefPtr<Foo> foo = new Foo; foo->unref.  Changed.

http://codereview.appspot.com/4496041/diff/1/src/pdf/SkPDFGraphicState.cpp#ne...
src/pdf/SkPDFGraphicState.cpp:168: // static
On 2011/05/11 00:29:27, reed1 wrote:
> 1. is this thread safe to return a global object? Is it ever modifed after
this
> point?

The object isn't modified after construction (other than ref count).  During
construction, it is protected by the canonicalPaintsMutex.  => yes, thread safe.

> 2. what is the ownership rule for calling getNoSMaskGraphicsState? Normally in
> skia calling a "get" function does not modify the owner count.

The ownership model for this method mirrors the ownership model of
getSMaskGraphicState and getGraphicStateForPaint, which return a reference.  It
needs to be this way because this class is implementing an interning pattern
with garbage collection:  From one of the get methods, we look up if we already
have that object, if so, we add a ref and return it.  Then if the object
reference ever reaches zero, we remove it from the lookup table.  Meaning the
first object must have a ref count of 1, implying reference before return. 
Happy to change to a different verb (from get), if there's something that would
make the code more consistent with the rest of Skia.

This is actually mentioned in the comments for this method in the header.

[Chris Guillory, 2011-05-12 19:03:09]

LGTM. Nits and general comment on AutoContentEntry.

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

http://codereview.appspot.com/4496041/diff/7001/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:451: class AutoContentEntry {
I think this class could be improved if it included more logic.
* Convert setUpContentEntry and finishContentEntry into member functions
* Make fDstFormXObject a member of this class.
* Somehow integrate this class with fCurrentContentEntry (I'm not seeing a way
immediately right now).

These changes are optional and may lead to suggestions orthogonal to this
change.

http://codereview.appspot.com/4496041/diff/12001/include/pdf/SkPDFGraphicState.h
File include/pdf/SkPDFGraphicState.h (right):

http://codereview.appspot.com/4496041/diff/12001/include/pdf/SkPDFGraphicStat...
include/pdf/SkPDFGraphicState.h:66: /** Get a graphic state that only unsets the
soft mask.  The reference
Nit: One space between sentences to be consistent.

http://codereview.appspot.com/4496041/diff/12001/include/pdf/SkPDFGraphicStat...
include/pdf/SkPDFGraphicState.h:68: *  to unreference it when done.  This is
needed to accommodate the weak
Nit: One space between sentences.

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

http://codereview.appspot.com/4496041/diff/12001/src/pdf/SkPDFDevice.cpp#newc...
src/pdf/SkPDFDevice.cpp:1049: cleanUp();
Nit: Comment here. Before you had "// Reset this device to have no content"

[Steve VanDeBogart, 2011-05-13 03:50:22]

Committing this now.  Reed: happy to address further comment in a follow up CL.

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

http://codereview.appspot.com/4496041/diff/7001/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:451: class AutoContentEntry {
On 2011/05/12 19:03:09, Chris Guillory wrote:
> I think this class could be improved if it included more logic.
> * Convert setUpContentEntry and finishContentEntry into member functions
> * Make fDstFormXObject a member of this class.
> * Somehow integrate this class with fCurrentContentEntry (I'm not seeing a way
> immediately right now).
> 
> These changes are optional and may lead to suggestions orthogonal to this
> change. 

Yea, I can see that to a certain extent, but there's a lot of state modified in
SkPDFDevice by those functions.  I think I'll defer trying a more complete
solution.

http://codereview.appspot.com/4496041/diff/12001/include/pdf/SkPDFGraphicState.h
File include/pdf/SkPDFGraphicState.h (right):

http://codereview.appspot.com/4496041/diff/12001/include/pdf/SkPDFGraphicStat...
include/pdf/SkPDFGraphicState.h:66: /** Get a graphic state that only unsets the
soft mask.  The reference
On 2011/05/12 19:03:09, Chris Guillory wrote:
> Nit: One space between sentences to be consistent.

Done.

http://codereview.appspot.com/4496041/diff/12001/include/pdf/SkPDFGraphicStat...
include/pdf/SkPDFGraphicState.h:68: *  to unreference it when done.  This is
needed to accommodate the weak
On 2011/05/12 19:03:09, Chris Guillory wrote:
> Nit: One space between sentences. 

Done.

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

http://codereview.appspot.com/4496041/diff/12001/src/pdf/SkPDFDevice.cpp#newc...
src/pdf/SkPDFDevice.cpp:1049: cleanUp();
On 2011/05/12 19:03:09, Chris Guillory wrote:
> Nit: Comment here. Before you had "// Reset this device to have no content"

Done.