Adding option to serialize mutable bitmaps in SkPicture

[junov1, 2012-05-23 15:03:24]

This patch addresses a major performance issue with deferred canvas rendering in
chrome.  It was underperforming when dealing with canvas to canvas copies. 
These operations were triggering a deferral flush because canvas to canvas
copies needed to execute immediately since there is no guarantee that the source
canvas will remain unchaged.

This patch solves the problem by flattening the source pixels, allowing the draw
operation to be deferred.  In the current implementation of SkPictureRecord,
bitmap parameter are flattened by in find() regardless of whether or not the
bitmap is a duplicate of a previously recorded bitmap. that approach is
nedlessly expensive when the pixels data is included in the flattened bitmap. 
To alleviate that problem a dictionary based on generationIDs is used to find
duplicates (only for bitmaps with flattened pixel data)

[reed1, 2012-05-23 18:59:22]

Not sure what different fixes/changes are in this CL.

1. How much does the CL reduce if we made the 2nd parameter have a default value
of 'false'?

2. How much does the CL reduce if we changed bitmap to always have a pixelref
(i.e. remove the raw-ptr option)?

[junov1, 2012-05-23 19:05:01]

On 2012/05/23 18:59:22, reed1 wrote:
> Not sure what different fixes/changes are in this CL.
> 
> 1. How much does the CL reduce if we made the 2nd parameter have a default
value
> of 'false'?

Second parameter of what?

> 2. How much does the CL reduce if we changed bitmap to always have a pixelref
> (i.e. remove the raw-ptr option)?

As far as the use case of HTML 2d canvases is concerned, the raw-ptr option does
not come into play.

[reed1, 2012-05-24 18:01:07]

On 2012/05/23 19:05:01, junov1 wrote:
> On 2012/05/23 18:59:22, reed1 wrote:
> > Not sure what different fixes/changes are in this CL.
> > 
> > 1. How much does the CL reduce if we made the 2nd parameter have a default
> value
> > of 'false'?
> 
> Second parameter of what?

Sorry for not being clearer. 2nd parameter to SkBitmap::flatten()

> 
> > 2. How much does the CL reduce if we changed bitmap to always have a
pixelref
> > (i.e. remove the raw-ptr option)?
> 
> As far as the use case of HTML 2d canvases is concerned, the raw-ptr option
does
> not come into play.

I was asking, thinking that much of the work in pictures was *because* we had
raw pixel pointers to deal with. Maybe I'm misreading the CL. Is the work solely
motivated by mutable bitmaps that *do* have a real pixelref?

[junov1, 2012-05-24 18:49:34]

On 2012/05/24 18:01:07, reed1 wrote:
> On 2012/05/23 19:05:01, junov1 wrote:
> > On 2012/05/23 18:59:22, reed1 wrote:
> > > Not sure what different fixes/changes are in this CL.
> > > 
> > > 1. How much does the CL reduce if we made the 2nd parameter have a default
> > value
> > > of 'false'?
> > 
> > Second parameter of what?
> 
> Sorry for not being clearer. 2nd parameter to SkBitmap::flatten()

Ah, ok, that makes sense, also that would avoid potential API compatibility
breakage (if anyone on the outside calls flatten directly?)

> 
> > 
> > > 2. How much does the CL reduce if we changed bitmap to always have a
> pixelref
> > > (i.e. remove the raw-ptr option)?
> > 
> > As far as the use case of HTML 2d canvases is concerned, the raw-ptr option
> does
> > not come into play.
> 
> I was asking, thinking that much of the work in pictures was *because* we had
> raw pixel pointers to deal with. Maybe I'm misreading the CL. Is the work
solely
> motivated by mutable bitmaps that *do* have a real pixelref?

Exactly, mutable bitmaps that have a pixelref.  The particular use case I am
optimizing is canvas to canvas copies where the source canvas has its pixels in
an SkMallocPixelRef.  This has nothing to do with the recent e-mail thread where
we were discussing SkPicture having problems with raw-ptr bitmaps.

[junov1, 2012-05-24 19:01:33]

-2 files in CL thanks to default param value.

[reed1, 2012-05-24 19:28:54]

Might it make sense to add kFlattenMutableBitmaps_Flag to
SkFlattenableWriteBuffer, rather than adding a parameter to SkBitmap::flatten?
Not sure, but we do have flags in the buffer already to inform our objects on
how to behave during flattening.

[junov1, 2012-05-24 19:54:30]

On Thu, May 24, 2012 at 3:28 PM, <reed@google.com> wrote:

> Might it make sense to add kFlattenMutableBitmaps_Flag to
> SkFlattenableWriteBuffer, rather than adding a parameter to
> SkBitmap::flatten? Not sure, but we do have flags in the buffer already
> to inform our objects on how to behave during flattening.
>
>
Hmmm... That does look like a good place to put it.
And then I could put the logic in
SkFlattenableWriteBuffer::persistBitmapPixels.  I think that makes for a
simpler CL.  I will try it out.


>
>
http://codereview.appspot.com/**6221066/<http://codereview.appspot.com/6221066/>
>

[junov1, 2012-05-25 15:01:31]

On 2012/05/24 19:28:54, reed1 wrote:
> Might it make sense to add kFlattenMutableBitmaps_Flag to
> SkFlattenableWriteBuffer, rather than adding a parameter to SkBitmap::flatten?

Done in patch 4.

[reed1, 2012-05-29 20:58:26]

http://codereview.appspot.com/6221066/diff/8001/src/core/SkPictureRecord.h
File src/core/SkPictureRecord.h (right):

http://codereview.appspot.com/6221066/diff/8001/src/core/SkPictureRecord.h#ne...
src/core/SkPictureRecord.h:99: struct BitmapDictionaryEntry {
Can we document that fKey == bitmap.generationID and fIndex points inside
fBitmaps[]

http://codereview.appspot.com/6221066/diff/8001/src/core/SkPictureRecord.h#ne...
src/core/SkPictureRecord.h:101: size_t fIndex;
Can you define operator<() instead and call the simpler version of SkTSearch?

[reed1, 2012-05-29 21:05:20]

much clearer than before, thanks.

http://codereview.appspot.com/6221066/diff/13002/include/core/SkFlattenable.h
File include/core/SkFlattenable.h (left):

http://codereview.appspot.com/6221066/diff/13002/include/core/SkFlattenable.h...
include/core/SkFlattenable.h:275: kInlineFactoryNames_Flag = 0x02,
Does this mean force-flatten-bitmap-pixels?

If so, perhaps the name should include pixels or force or both (not sure).
Either way, flattening of bitmaps is obsure as is, so we should probably add
come comments around this new flag.

http://codereview.appspot.com/6221066/diff/13002/include/core/SkPicture.h
File include/core/SkPicture.h (right):

http://codereview.appspot.com/6221066/diff/13002/include/core/SkPicture.h#new...
include/core/SkPicture.h:90: */
can this be const?

http://codereview.appspot.com/6221066/diff/13002/include/core/SkPicture.h#new...
include/core/SkPicture.h:95: */
can this be const?

[junov1, 2012-05-30 15:05:25]

Patch 5 addresses comments 10 and 11.

[junov1, 2012-06-01 17:35:02]

On 2012/05/30 15:05:25, junov1 wrote:
> Patch 5 addresses comments 10 and 11.

Ping!

[reed1, 2012-06-01 17:41:08]

http://codereview.appspot.com/6221066/diff/17001/src/core/SkPictureRecord.h
File src/core/SkPictureRecord.h (right):

http://codereview.appspot.com/6221066/diff/17001/src/core/SkPictureRecord.h#n...
src/core/SkPictureRecord.h:100: uint32_t fKey; // SkPixelRef GenerationID.
Didn't notice this before.

In general skia only uses size_t for number-of-bytes parameters, and int for
indices. In this case, using int (or explicitly int32_t or uint32_t) might be
nice to keep this struct explicitly packed to 64bits, since we have to allocate
an array of these. Also the pow-2 means (trivially) array indexing can be shifts
instead of multiplies.

other than that, lgtm

[junov1, 2012-07-16 18:58:30]

Fixed with r4130