[PDF] Refactor content stream creation in SkPDFDevice to support more xfermodes.

[PDF] Refactor content stream creation in SkPDFDevice to support more xfermodes.

Instead of writing all drawing and state updates into the final content stream immediately, this change creates a new ContentEntry each time the transform, clip, or paint changes.  Drawing is done into a stream in the ContentEntry.  When the consumer asks for the content, we combine all the ContentEntries with appropriate updates to the state (clip, transform, paint) in between.  This allows us to modify the clip even after a drawing has completed.  It also lets us remove ContentEntries with no drawing.  Further optimization can be done to better use the stack features of PDF, for now we follow the previous model of having a single clip followed by a single transform on the graphic state stack.

Push rectangle logic into SkPDFUtil::AppendRectangle.
Change private functions to adhere to coding standards.

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

[Steve VanDeBogart, 2011-05-02 19:04:17]

This change doesn't add the new xfermode support, just make it possible.

[reed1, 2011-05-02 20:34:43]

Can we submit the clipstack changes separately, and add a small unittest for the
compare and assignment? The code looks clean.

[Steve VanDeBogart, 2011-05-02 21:01:22]

On 2011/05/02 20:34:43, reed1 wrote:
> Can we submit the clipstack changes separately, and add a small unittest for
the
> compare and assignment? The code looks clean.

SG, I'll take care of that shortly.

[Steve VanDeBogart, 2011-05-03 03:39:25]

On 2011/05/02 20:34:43, reed1 wrote:
> Can we submit the clipstack changes separately, and add a small unittest for
the
> compare and assignment? The code looks clean.

Done.

[Chris Guillory, 2011-05-04 19:27:50]

Some comments. Change adds complexity. I'd like to look at it again.

http://codereview.appspot.com/4459041/diff/5001/include/pdf/SkPDFUtils.h
File include/pdf/SkPDFUtils.h (right):

http://codereview.appspot.com/4459041/diff/5001/include/pdf/SkPDFUtils.h#newc...
include/pdf/SkPDFUtils.h:51: static void AppendRectangle(const SkRect& r,
SkWStream* content);
Nit: r -> rect?

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

http://codereview.appspot.com/4459041/diff/5001/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:109: // It is important to not confuse this with
SkPDFGraphicState, the later being
Nit: Optional: Readability: this -> GraphicStateEntry

http://codereview.appspot.com/4459041/diff/5001/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:154: fClipStack == b.fClipStack && (
Nit: Does this ( belong in the next line?

http://codereview.appspot.com/4459041/diff/5001/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:179: GraphicStateEntry* currentEntry() { return fEntries
+ fStackDepth; }
Nit: Optional: Readability: return &fEntries[fSackDepth]

http://codereview.appspot.com/4459041/diff/5001/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:185: GraphicStateEntry fEntries[kMaxStackDepth];
The size of this array seems like it should be kMaxStackDepth+1.
GraphicsStackState::push(..) will set fStackDepth to kMaxStackDepth without an
assert.

http://codereview.appspot.com/4459041/diff/5001/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:251: // There are several optimizations that can be done
in this function.
Nit: Convert this comment into a TODO?

http://codereview.appspot.com/4459041/diff/5001/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:287: (clipEntry->fPath &&
clipEntry->fPath->isInverseFillType())) {
Nit: Is this extra indentation correct?

http://codereview.appspot.com/4459041/diff/5001/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:933: if (fPageSize != fContentSize) {
Don't this return incorrect results if the pdf device was constructed with an
initialTranform that contained a scale factor? Seems like we'd want to set
fContentSize to this->width(), this->height() in the constructor. I'd actually
suggest removing fContentSize if it's only use is here.

http://codereview.appspot.com/4459041/diff/5001/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:941: entry != NULL;
Nit: New indentation rules?

[Steve VanDeBogart, 2011-05-04 21:22:49]

http://codereview.appspot.com/4459041/diff/5001/include/pdf/SkPDFUtils.h
File include/pdf/SkPDFUtils.h (right):

http://codereview.appspot.com/4459041/diff/5001/include/pdf/SkPDFUtils.h#newc...
include/pdf/SkPDFUtils.h:51: static void AppendRectangle(const SkRect& r,
SkWStream* content);
On 2011/05/04 19:27:50, Chris Guillory wrote:
> Nit: r -> rect?

Done.

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

http://codereview.appspot.com/4459041/diff/5001/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:109: // It is important to not confuse this with
SkPDFGraphicState, the later being
On 2011/05/04 19:27:50, Chris Guillory wrote:
> Nit: Optional: Readability: this -> GraphicStateEntry

Done.

http://codereview.appspot.com/4459041/diff/5001/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:154: fClipStack == b.fClipStack && (
On 2011/05/04 19:27:50, Chris Guillory wrote:
> Nit: Does this ( belong in the next line?

Unclear, done.

http://codereview.appspot.com/4459041/diff/5001/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:179: GraphicStateEntry* currentEntry() { return fEntries
+ fStackDepth; }
On 2011/05/04 19:27:50, Chris Guillory wrote:
> Nit: Optional: Readability: return &fEntries[fSackDepth]

Done.

http://codereview.appspot.com/4459041/diff/5001/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:185: GraphicStateEntry fEntries[kMaxStackDepth];
On 2011/05/04 19:27:50, Chris Guillory wrote:
> The size of this array seems like it should be kMaxStackDepth+1.
> GraphicsStackState::push(..) will set fStackDepth to kMaxStackDepth without an
> assert.

Done.

http://codereview.appspot.com/4459041/diff/5001/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:251: // There are several optimizations that can be done
in this function.
On 2011/05/04 19:27:50, Chris Guillory wrote:
> Nit: Convert this comment into a TODO?

Done.

http://codereview.appspot.com/4459041/diff/5001/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:287: (clipEntry->fPath &&
clipEntry->fPath->isInverseFillType())) {
On 2011/05/04 19:27:50, Chris Guillory wrote:
> Nit: Is this extra indentation correct?

Skia doesn't have strict line wrapping rules.  Lining up with the character
after 'if (' makes things hard to read, I think.

http://codereview.appspot.com/4459041/diff/5001/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:933: if (fPageSize != fContentSize) {
On 2011/05/04 19:27:50, Chris Guillory wrote:
> Don't this return incorrect results if the pdf device was constructed with an
> initialTranform that contained a scale factor? Seems like we'd want to set
> fContentSize to this->width(), this->height() in the constructor. I'd actually
> suggest removing fContentSize if it's only use is here.

I don't think so, but I may be misunderstanding you.  I had to think about this
hard, so I could have gotten it wrong. The point of this if-block is to insert a
clip to the content area. fPageSize and fContentSize are both in the same units
(pts).  If they are equal, there's no point in generating a clip since PDF clips
to the page by default.  If they aren't equal, then we need to clip to the
content area.

http://codereview.appspot.com/4459041/diff/5001/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:941: entry != NULL;
On 2011/05/04 19:27:50, Chris Guillory wrote:
> Nit: New indentation rules?

Just trying to do something that makes sense.  Do you have a better suggestion?

[Chris Guillory, 2011-05-06 18:52:57]

LGTM. Some comments.

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

http://codereview.appspot.com/4459041/diff/13001/include/pdf/SkPDFDevice.h#ne...
include/pdf/SkPDFDevice.h:154: ContentEntry* fCurContentEntry;
Optional: The Cur abbreviation seems somewhat out of place. Would a rename make
sense? fContentEntry with comment?

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

http://codereview.appspot.com/4459041/diff/13001/src/pdf/SkPDFDevice.cpp#newc...
src/pdf/SkPDFDevice.cpp:109: // It is important to not confuse GrphicStateEntry
with SkPDFGraphicState, the
Spelling: GraphicStateEntry

http://codereview.appspot.com/4459041/diff/13001/src/pdf/SkPDFDevice.cpp#newc...
src/pdf/SkPDFDevice.cpp:113: bool operator==(const GraphicStateEntry& b);
Nit: GraphicStateEntry is not a class. If there are no plans to make it a class
can we make this:
friend int operator==(const GraphicStateEntry& a, const GraphicStateEntry& b)

http://codereview.appspot.com/4459041/diff/13001/src/pdf/SkPDFDevice.cpp#newc...
src/pdf/SkPDFDevice.cpp:113: bool operator==(const GraphicStateEntry& b);
Optional: This function doesn't compare all members. Perhaps it should have a
most descriptive name than operatator==?

http://codereview.appspot.com/4459041/diff/13001/src/pdf/SkPDFDevice.cpp#newc...
src/pdf/SkPDFDevice.cpp:217: SkASSERT(iterEntry);
Nit: Optional: Create operator== for SkClipStack::B2FIter::Clip.

http://codereview.appspot.com/4459041/diff/13001/src/pdf/SkPDFDevice.cpp#newc...
src/pdf/SkPDFDevice.cpp:254: static void update_clip(GraphicStackState* gsState,
It might make sense to have GraphicStackState be a class and this be a member.
Maybe in a future change?

http://codereview.appspot.com/4459041/diff/13001/src/pdf/SkPDFDevice.cpp#newc...
src/pdf/SkPDFDevice.cpp:289: (clipEntry->fPath &&
clipEntry->fPath->isInverseFillType())) {
Nit: Optional: Why not use Chromium indentation style.

http://codereview.appspot.com/4459041/diff/13001/src/pdf/SkPDFDevice.cpp#newc...
src/pdf/SkPDFDevice.cpp:324: static void update_matrix(GraphicStackState*
gsState, const SkMatrix& matrix,
It might make sense to have GraphicStackState be a class and this be a member.
Maybe in a future change?

http://codereview.appspot.com/4459041/diff/13001/src/pdf/SkPDFDevice.cpp#newc...
src/pdf/SkPDFDevice.cpp:334: gsState->currentEntry()[-1].fClipStack);
Nit: Unconventional notation. gsState->previousEntry()?

http://codereview.appspot.com/4459041/diff/13001/src/pdf/SkPDFDevice.cpp#newc...
src/pdf/SkPDFDevice.cpp:936: SkRect r = SkRect::MakeWH(this->width(),
this->height());
Can you add comment to explain this clip logic.

http://codereview.appspot.com/4459041/diff/13001/src/pdf/SkPDFUtils.cpp
File src/pdf/SkPDFUtils.cpp (right):

http://codereview.appspot.com/4459041/diff/13001/src/pdf/SkPDFUtils.cpp#newco...
src/pdf/SkPDFUtils.cpp:90: SkScalar bottom = rect.fBottom < rect.fTop ?
rect.fBottom : rect.fTop;
Nit: SkMinScalar

[Steve VanDeBogart, 2011-05-06 21:37:15]

Chris, I pulled those functions into GraphicStackState, so you may want to give
the diff a once over.

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

http://codereview.appspot.com/4459041/diff/13001/include/pdf/SkPDFDevice.h#ne...
include/pdf/SkPDFDevice.h:154: ContentEntry* fCurContentEntry;
On 2011/05/06 18:52:57, Chris Guillory wrote:
> Optional: The Cur abbreviation seems somewhat out of place. Would a rename
make
> sense? fContentEntry with comment?

would fCurrentContentEntry be better?  Having both fContentEntries and
fContentEntry seems like it could lead to confusion.

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

http://codereview.appspot.com/4459041/diff/13001/src/pdf/SkPDFDevice.cpp#newc...
src/pdf/SkPDFDevice.cpp:109: // It is important to not confuse GrphicStateEntry
with SkPDFGraphicState, the
On 2011/05/06 18:52:57, Chris Guillory wrote:
> Spelling: GraphicStateEntry

Done.

http://codereview.appspot.com/4459041/diff/13001/src/pdf/SkPDFDevice.cpp#newc...
src/pdf/SkPDFDevice.cpp:113: bool operator==(const GraphicStateEntry& b);
On 2011/05/06 18:52:57, Chris Guillory wrote:
> Optional: This function doesn't compare all members. Perhaps it should have a
> most descriptive name than operatator==?

Done.

http://codereview.appspot.com/4459041/diff/13001/src/pdf/SkPDFDevice.cpp#newc...
src/pdf/SkPDFDevice.cpp:217: SkASSERT(iterEntry);
On 2011/05/06 18:52:57, Chris Guillory wrote:
> Nit: Optional: Create operator== for SkClipStack::B2FIter::Clip.

Done.

http://codereview.appspot.com/4459041/diff/13001/src/pdf/SkPDFDevice.cpp#newc...
src/pdf/SkPDFDevice.cpp:254: static void update_clip(GraphicStackState* gsState,
On 2011/05/06 18:52:57, Chris Guillory wrote:
> It might make sense to have GraphicStackState be a class and this be a member.
> Maybe in a future change?

Indeed.  These update functions are all update on a GraphicStateStack.  I also
made the GraphicStateStack take the SkWStream content stream as a constructor
argument.

http://codereview.appspot.com/4459041/diff/13001/src/pdf/SkPDFDevice.cpp#newc...
src/pdf/SkPDFDevice.cpp:289: (clipEntry->fPath &&
clipEntry->fPath->isInverseFillType())) {
On 2011/05/06 18:52:57, Chris Guillory wrote:
> Nit: Optional: Why not use Chromium indentation style.

Cr has a 2 space indent.  With a 4 space indent, the continued if conditional
and the body of the if line up.  This seems less than ideal.

http://codereview.appspot.com/4459041/diff/13001/src/pdf/SkPDFDevice.cpp#newc...
src/pdf/SkPDFDevice.cpp:324: static void update_matrix(GraphicStackState*
gsState, const SkMatrix& matrix,
On 2011/05/06 18:52:57, Chris Guillory wrote:
> It might make sense to have GraphicStackState be a class and this be a member.
> Maybe in a future change?

Done.

http://codereview.appspot.com/4459041/diff/13001/src/pdf/SkPDFDevice.cpp#newc...
src/pdf/SkPDFDevice.cpp:334: gsState->currentEntry()[-1].fClipStack);
On 2011/05/06 18:52:57, Chris Guillory wrote:
> Nit: Unconventional notation. gsState->previousEntry()?

Done.

http://codereview.appspot.com/4459041/diff/13001/src/pdf/SkPDFDevice.cpp#newc...
src/pdf/SkPDFDevice.cpp:936: SkRect r = SkRect::MakeWH(this->width(),
this->height());
On 2011/05/06 18:52:57, Chris Guillory wrote:
> Can you add comment to explain this clip logic.

Done.

http://codereview.appspot.com/4459041/diff/13001/src/pdf/SkPDFUtils.cpp
File src/pdf/SkPDFUtils.cpp (right):

http://codereview.appspot.com/4459041/diff/13001/src/pdf/SkPDFUtils.cpp#newco...
src/pdf/SkPDFUtils.cpp:90: SkScalar bottom = rect.fBottom < rect.fTop ?
rect.fBottom : rect.fTop;
On 2011/05/06 18:52:57, Chris Guillory wrote:
> Nit: SkMinScalar

Done.

[Chris Guillory, 2011-05-06 21:51:12]

LGTM. Cleaner. Couple of nits.

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

http://codereview.appspot.com/4459041/diff/13001/include/pdf/SkPDFDevice.h#ne...
include/pdf/SkPDFDevice.h:154: ContentEntry* fCurContentEntry;
On 2011/05/06 21:37:15, Steve VanDeBogart wrote:
> On 2011/05/06 18:52:57, Chris Guillory wrote:
> > Optional: The Cur abbreviation seems somewhat out of place. Would a rename
> make
> > sense? fContentEntry with comment?
> 
> would fCurrentContentEntry be better?  Having both fContentEntries and
> fContentEntry seems like it could lead to confusion.
Yes.

http://codereview.appspot.com/4459041/diff/19001/include/core/SkClipStack.h
File include/core/SkClipStack.h (right):

http://codereview.appspot.com/4459041/diff/19001/include/core/SkClipStack.h#n...
include/core/SkClipStack.h:45: friend int operator==(const Clip& a, const Clip&
b);
Nit: int -> bool

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

http://codereview.appspot.com/4459041/diff/19001/src/pdf/SkPDFDevice.cpp#newc...
src/pdf/SkPDFDevice.cpp:180: const SkIPoint& translation);
Nit: Indentation.

[Steve VanDeBogart, 2011-05-07 01:59:27]

Thanks.

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

http://codereview.appspot.com/4459041/diff/13001/include/pdf/SkPDFDevice.h#ne...
include/pdf/SkPDFDevice.h:154: ContentEntry* fCurContentEntry;
On 2011/05/06 21:51:13, Chris Guillory wrote:
> On 2011/05/06 21:37:15, Steve VanDeBogart wrote:
> > On 2011/05/06 18:52:57, Chris Guillory wrote:
> > > Optional: The Cur abbreviation seems somewhat out of place. Would a rename
> > make
> > > sense? fContentEntry with comment?
> > 
> > would fCurrentContentEntry be better?  Having both fContentEntries and
> > fContentEntry seems like it could lead to confusion.
> Yes.

Done.

http://codereview.appspot.com/4459041/diff/19001/include/core/SkClipStack.h
File include/core/SkClipStack.h (right):

http://codereview.appspot.com/4459041/diff/19001/include/core/SkClipStack.h#n...
include/core/SkClipStack.h:45: friend int operator==(const Clip& a, const Clip&
b);
On 2011/05/06 21:51:13, Chris Guillory wrote:
> Nit: int -> bool

Done.

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

http://codereview.appspot.com/4459041/diff/19001/src/pdf/SkPDFDevice.cpp#newc...
src/pdf/SkPDFDevice.cpp:180: const SkIPoint& translation);
On 2011/05/06 21:51:13, Chris Guillory wrote:
> Nit: Indentation.

Done.

[Steve VanDeBogart, 2011-05-09 07:55:13]

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

http://codereview.appspot.com/4459041/diff/13001/src/pdf/SkPDFDevice.cpp#newc...
src/pdf/SkPDFDevice.cpp:936: SkRect r = SkRect::MakeWH(this->width(),
this->height());
On 2011/05/06 21:37:15, Steve VanDeBogart wrote:
> On 2011/05/06 18:52:57, Chris Guillory wrote:
> > Can you add comment to explain this clip logic.
> 
> Done.

The comment and code didn't match, turns out that the comment was correct (r is
already in device space, no need to mapRect).  Corrected the code.