When cloning picture, use a bitmap heap to avoid flattening bitmaps.

When cloning picture, use a bitmap heap to avoid flattening bitmaps.

When cloning a picture, the paints are reflattened. Use a bitmap
heap so the bitmaps do not get unnecessarily get flattened as well.

For br.337, this speeds up bench_pictures timing the clone
operation (not yet checked in, but currently timing making five
clones) from around 180 ms to around 24ms.

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

[reed1, 2012-12-10 17:28:02]

https://codereview.appspot.com/6903063/diff/2001/src/core/SkPicturePlayback.cpp
File src/core/SkPicturePlayback.cpp (right):

https://codereview.appspot.com/6903063/diff/2001/src/core/SkPicturePlayback.c...
src/core/SkPicturePlayback.cpp:182: */
Can we store the bitmapheap on the stack?

SkBitmapHeap tempHeap;
if (fBitmapHeap == null) {
    heap = &tempHeap;
} else ...

[Leon, 2012-12-10 17:49:49]

Although I cannot put the heap on the stack, I did move the code around slightly
to keep the variable 'heap' in a small scope and avoid a second if statement (at
the cost of having two lines that are quite similar).

https://codereview.appspot.com/6903063/diff/2001/src/core/SkPicturePlayback.cpp
File src/core/SkPicturePlayback.cpp (right):

https://codereview.appspot.com/6903063/diff/2001/src/core/SkPicturePlayback.c...
src/core/SkPicturePlayback.cpp:182: */
On 2012/12/10 17:28:02, reed1 wrote:
> Can we store the bitmapheap on the stack?
> 
> SkBitmapHeap tempHeap;
> if (fBitmapHeap == null) {
>     heap = &tempHeap;
> } else ...

No. deepCopyInfo is passed in to this constructor, and will potentially be
passed to this same constructor for another SkPicturePlayback, which will need
access to the same heap (see line 214 below, outside of the if
(!deepCopyInfo->initialized) block).

[reed1, 2012-12-10 18:00:35]

On 2012/12/10 17:49:49, scroggo-work wrote:
> Although I cannot put the heap on the stack, I did move the code around
slightly
> to keep the variable 'heap' in a small scope and avoid a second if statement
(at
> the cost of having two lines that are quite similar).

Why can't the heap be on the stack?

> 
>
https://codereview.appspot.com/6903063/diff/2001/src/core/SkPicturePlayback.cpp
> File src/core/SkPicturePlayback.cpp (right):
> 
>
https://codereview.appspot.com/6903063/diff/2001/src/core/SkPicturePlayback.c...
> src/core/SkPicturePlayback.cpp:182: */
> On 2012/12/10 17:28:02, reed1 wrote:
> > Can we store the bitmapheap on the stack?
> > 
> > SkBitmapHeap tempHeap;
> > if (fBitmapHeap == null) {
> >     heap = &tempHeap;
> > } else ...
> 
> No. deepCopyInfo is passed in to this constructor, and will potentially be
> passed to this same constructor for another SkPicturePlayback, which will need
> access to the same heap (see line 214 below, outside of the if
> (!deepCopyInfo->initialized) block).

[reed1, 2012-12-10 18:02:17]

Should the caller be assigning the bitmapheap themselves, rather than us at this
level?

[reed1, 2012-12-10 18:12:45]

all thoughts for a later cl perhaps.... lgtm