Refactor Bitmap Storage for SkPicture using SkPipe's design.

Refactor Bitmap Storage for SkPicture using SkPipe's design.


Refactor Picture and Pipe bitmap storage into common data structure


Update SkFlattenable buffers to be more modular.

This CL is an effort to stage the conversion to named
parameters for all SkFlattenable commands. This particular
stage only does the following two things...

1. Move flattenable buffers from SkFlattenable.h into
   their own header.
2. Update and Add new read write methods for better clarity
   and convenience.

BUG=

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

[Leon, 2012-08-06 19:38:21]

I am still looking through the code, but I wanted to alert you to the beginning
of SkBitmapHeap::insert. It appears to me that we will never reuse bitmaps.

Also, where can I find the code that adds bitmaps used by shaders to the heap?

http://codereview.appspot.com/6445079/diff/1/src/core/SkBitmapHeap.cpp
File src/core/SkBitmapHeap.cpp (right):

http://codereview.appspot.com/6445079/diff/1/src/core/SkBitmapHeap.cpp#newcod...
src/core/SkBitmapHeap.cpp:131: // recently used one can not have used all it's
references yet either.
its*

http://codereview.appspot.com/6445079/diff/1/src/core/SkBitmapHeap.cpp#newcod...
src/core/SkBitmapHeap.cpp:182: SkDebugf("copy failed");
We should do more than state that the copy failed. In the case where we are
reusing an earlier bitmap (i.e. findEntryToReplace() returned non-NULL), this
will result in drawing the bitmap that we were intending to evict when
attempting to draw the new bitmap.

In the existing code, we see that it could not be copied, so we do not put the
drawBitmap call into the stream.

http://codereview.appspot.com/6445079/diff/1/src/core/SkBitmapHeap.cpp#newcod...
src/core/SkBitmapHeap.cpp:193: if (entry) {
If I understand correctly, "entry" will always be NULL here.

http://codereview.appspot.com/6445079/diff/1/src/core/SkBitmapHeap.cpp#newcod...
src/core/SkBitmapHeap.cpp:220: if (!entry) {
I prefer this style, but I think the coding style guidelines recommend

if (entry != NULL)

You do it the non-preferred way elsewhere. Probably not necessary to change
every single one (I know your code change is rather large), but something to be
aware of when writing new code.

http://codereview.appspot.com/6445079/diff/1/src/core/SkBitmapHeap.h
File src/core/SkBitmapHeap.h (right):

http://codereview.appspot.com/6445079/diff/1/src/core/SkBitmapHeap.h#newcode38
src/core/SkBitmapHeap.h:38: // its placement in the heap until it's owner count
goes to zero
its*

http://codereview.appspot.com/6445079/diff/1/src/core/SkBitmapHeap.h#newcode51
src/core/SkBitmapHeap.h:51: // in the case of external storage only the index is
valid
Won't the ref count be valid as well?

http://codereview.appspot.com/6445079/diff/1/src/core/SkBitmapHeap.h#newcode52
src/core/SkBitmapHeap.h:52: SkBitmapHeapEntry* insert(const SkBitmap& bitmap);
For my cross process case, how do I know whether a new one was added?

http://codereview.appspot.com/6445079/diff/1/src/core/SkBitmapHeap.h#newcode55
src/core/SkBitmapHeap.h:55: // returns NULL if your using external storage
you're*

http://codereview.appspot.com/6445079/diff/1/src/core/SkBitmapHeap.h#newcode56
src/core/SkBitmapHeap.h:56: SkBitmapHeapEntry* getEntry(int32_t index) const {
Should we have a comment that "index" just means "index" rather than the result
of SkBitmapHeapEntry::getIndex()?

http://codereview.appspot.com/6445079/diff/1/src/core/SkBitmapHeap.h#newcode104
src/core/SkBitmapHeap.h:104: // returns the index into the sorted array and if
it already existed it populates it's entry
its*

[Leon, 2012-08-06 19:47:36]

http://codereview.appspot.com/6445079/diff/1/src/core/SkBitmapHeap.h
File src/core/SkBitmapHeap.h (right):

http://codereview.appspot.com/6445079/diff/1/src/core/SkBitmapHeap.h#newcode52
src/core/SkBitmapHeap.h:52: SkBitmapHeapEntry* insert(const SkBitmap& bitmap);
On 2012/08/06 19:38:21, scroggo-work wrote:
> For my cross process case, how do I know whether a new one was added?

Oh, silly Leon. HeapExternalStorage takes care of that! This is a solution I am
happy with :) I was thinking that HeapExternalStorage was for the cross
process-shared address space, and it may be the right way to do that also, but
it did not occur to me that this is how the normal cross process case works..

[Leon, 2012-08-06 20:01:02]

http://codereview.appspot.com/6445079/diff/1/src/core/SkBitmapHeap.cpp
File src/core/SkBitmapHeap.cpp (right):

http://codereview.appspot.com/6445079/diff/1/src/core/SkBitmapHeap.cpp#newcod...
src/core/SkBitmapHeap.cpp:214: fBitmapIndex.remove(index);
Since we're removing the item at index, the rest of the array will be
downshifted, meaning that if searchIndex is greater than index, it is now
pointing to the wrong item.

We should add this code to fix it:

if (index < searchIndex) {
    searchIndex--;
}

[DerekS, 2012-08-06 20:07:49]

the new patch set has some better comments and I'll keep adding them as I go

http://codereview.appspot.com/6445079/diff/1/src/core/SkBitmapHeap.cpp
File src/core/SkBitmapHeap.cpp (right):

http://codereview.appspot.com/6445079/diff/1/src/core/SkBitmapHeap.cpp#newcod...
src/core/SkBitmapHeap.cpp:182: SkDebugf("copy failed");
On 2012/08/06 19:38:21, scroggo-work wrote:
> We should do more than state that the copy failed. In the case where we are
> reusing an earlier bitmap (i.e. findEntryToReplace() returned non-NULL), this
> will result in drawing the bitmap that we were intending to evict when
> attempting to draw the new bitmap.
> 
> In the existing code, we see that it could not be copied, so we do not put the
> drawBitmap call into the stream.

good point. I'll make this return a boolean and we can always bubble this up to
the caller.

http://codereview.appspot.com/6445079/diff/1/src/core/SkBitmapHeap.cpp#newcod...
src/core/SkBitmapHeap.cpp:193: if (entry) {
On 2012/08/06 19:38:21, scroggo-work wrote:
> If I understand correctly, "entry" will always be NULL here.

Nope, only if it was a cache hit.

http://codereview.appspot.com/6445079/diff/1/src/core/SkBitmapHeap.cpp#newcod...
src/core/SkBitmapHeap.cpp:214: fBitmapIndex.remove(index);
On 2012/08/06 20:01:02, scroggo-work wrote:
> Since we're removing the item at index, the rest of the array will be
> downshifted, meaning that if searchIndex is greater than index, it is now
> pointing to the wrong item.
> 
> We should add this code to fix it:
> 
> if (index < searchIndex) {
>     searchIndex--;
> }

I don't think that will work either as we don't know if the index actually
shifted or not. I think I need to wait until I'm done working with the existing
search index before I do this delete.

http://codereview.appspot.com/6445079/diff/1/src/core/SkBitmapHeap.cpp#newcod...
src/core/SkBitmapHeap.cpp:220: if (!entry) {
On 2012/08/06 19:38:21, scroggo-work wrote:
> I prefer this style, but I think the coding style guidelines recommend
> 
> if (entry != NULL)
> 
> You do it the non-preferred way elsewhere. Probably not necessary to change
> every single one (I know your code change is rather large), but something to
be
> aware of when writing new code.

Noted.

[Leon, 2012-08-06 21:32:43]

http://codereview.appspot.com/6445079/diff/7001/src/core/SkBitmapHeap.h
File src/core/SkBitmapHeap.h (right):

http://codereview.appspot.com/6445079/diff/7001/src/core/SkBitmapHeap.h#newco...
src/core/SkBitmapHeap.h:40: * Constructs a heap that is responsible for
allocating and managing it's own storage.  In the
its*

http://codereview.appspot.com/6445079/diff/7001/src/core/SkBitmapHeap.h#newco...
src/core/SkBitmapHeap.h:41: * case were we choose to allow the heap to grow
indefinitely (i.e. UNLIMITED_SIZE) we guarantee
case where*

http://codereview.appspot.com/6445079/diff/7001/src/core/SkBitmapHeap.h#newco...
src/core/SkBitmapHeap.h:42: * that once allocated in the heap a bitmap's index
in the heap is immutable otherwise we
"otherwise we" should start a new sentence (or be after a semicolon).

http://codereview.appspot.com/6445079/diff/7001/src/core/SkBitmapHeap.h#newco...
src/core/SkBitmapHeap.h:43: * guarantee the bitmaps placement in the heap until
it's owner count goes to zero.
its*

http://codereview.appspot.com/6445079/diff/7001/src/core/SkBitmapHeap.h#newco...
src/core/SkBitmapHeap.h:76: *          (2) until all owners have called unref on
the appropriate SkBitmapHeapEntry*
unref -> releaseRef

http://codereview.appspot.com/6445079/diff/7001/src/core/SkBitmapHeap.h#newco...
src/core/SkBitmapHeap.h:129: // returns the index into the sorted array and if
it already existed it populates it's entry
its*

http://codereview.appspot.com/6445079/diff/7001/src/core/SkOrderedReadBuffer.cpp
File src/core/SkOrderedReadBuffer.cpp (right):

http://codereview.appspot.com/6445079/diff/7001/src/core/SkOrderedReadBuffer....
src/core/SkOrderedReadBuffer.cpp:174: *bitmap =
*fBitmapHeap->getEntry(index)->getBitmap();
As we discussed, it would be great if this called
AbstractBaseClass::getBitmap(index) so my cross process case can handle this
properly.

[DerekS, 2012-08-07 16:05:02]

http://codereview.appspot.com/6445079/diff/7001/src/core/SkBitmapHeap.h
File src/core/SkBitmapHeap.h (right):

http://codereview.appspot.com/6445079/diff/7001/src/core/SkBitmapHeap.h#newco...
src/core/SkBitmapHeap.h:40: * Constructs a heap that is responsible for
allocating and managing it's own storage.  In the
On 2012/08/06 21:32:43, scroggo-work wrote:
> its*

Done.

http://codereview.appspot.com/6445079/diff/7001/src/core/SkBitmapHeap.h#newco...
src/core/SkBitmapHeap.h:41: * case were we choose to allow the heap to grow
indefinitely (i.e. UNLIMITED_SIZE) we guarantee
On 2012/08/06 21:32:43, scroggo-work wrote:
> case where*

Done.

http://codereview.appspot.com/6445079/diff/7001/src/core/SkBitmapHeap.h#newco...
src/core/SkBitmapHeap.h:42: * that once allocated in the heap a bitmap's index
in the heap is immutable otherwise we
On 2012/08/06 21:32:43, scroggo-work wrote:
> "otherwise we" should start a new sentence (or be after a semicolon).

Done.

http://codereview.appspot.com/6445079/diff/7001/src/core/SkBitmapHeap.h#newco...
src/core/SkBitmapHeap.h:43: * guarantee the bitmaps placement in the heap until
it's owner count goes to zero.
On 2012/08/06 21:32:43, scroggo-work wrote:
> its*

Done.

http://codereview.appspot.com/6445079/diff/7001/src/core/SkBitmapHeap.h#newco...
src/core/SkBitmapHeap.h:76: *          (2) until all owners have called unref on
the appropriate SkBitmapHeapEntry*
On 2012/08/06 21:32:43, scroggo-work wrote:
> unref -> releaseRef

Done.

http://codereview.appspot.com/6445079/diff/7001/src/core/SkBitmapHeap.h#newco...
src/core/SkBitmapHeap.h:129: // returns the index into the sorted array and if
it already existed it populates it's entry
On 2012/08/06 21:32:43, scroggo-work wrote:
> its*

Done.

[DerekS, 2012-08-07 16:11:44]

http://codereview.appspot.com/6445079/diff/6002/src/core/SkOrderedReadBuffer.cpp
File src/core/SkOrderedReadBuffer.cpp (right):

http://codereview.appspot.com/6445079/diff/6002/src/core/SkOrderedReadBuffer....
src/core/SkOrderedReadBuffer.cpp:166: SkRefCnt*
SkOrderedReadBuffer::readRefCntPtr() {
I'll look into trying to remove this call completely.

http://codereview.appspot.com/6445079/diff/6002/src/core/SkPictureFlat.h
File src/core/SkPictureFlat.h (right):

http://codereview.appspot.com/6445079/diff/6002/src/core/SkPictureFlat.h#newc...
src/core/SkPictureFlat.h:206: * Set an SkRefCntSet to be used to store
SkPixelRefs during flattening. Ref
update comment

[Leon, 2012-08-07 17:31:58]

LGTM!

http://codereview.appspot.com/6445079/diff/6002/src/core/SkBitmapHeap.h
File src/core/SkBitmapHeap.h (right):

http://codereview.appspot.com/6445079/diff/6002/src/core/SkBitmapHeap.h#newco...
src/core/SkBitmapHeap.h:23: // provides a means for a owner/consumer of the
bitmap to both access the bitmap and indicate it is done using it.
over 100 chars

http://codereview.appspot.com/6445079/diff/6002/src/core/SkBitmapHeap.h#newco...
src/core/SkBitmapHeap.h:118: virtual SkBitmap* getBitmap(int32_t slot) {
SK_OVERRIDE

Could also be const.

http://codereview.appspot.com/6445079/diff/6002/src/core/SkPictureFlat.h
File src/core/SkPictureFlat.h (right):

http://codereview.appspot.com/6445079/diff/6002/src/core/SkPictureFlat.h#newc...
src/core/SkPictureFlat.h:180: * Thhis should be set by the protected
setBitmapHeap.
This*

[DerekS, 2012-08-07 18:20:37]

http://codereview.appspot.com/6445079/diff/6002/src/core/SkBitmapHeap.h
File src/core/SkBitmapHeap.h (right):

http://codereview.appspot.com/6445079/diff/6002/src/core/SkBitmapHeap.h#newco...
src/core/SkBitmapHeap.h:23: // provides a means for a owner/consumer of the
bitmap to both access the bitmap and indicate it is done using it.
On 2012/08/07 17:31:58, scroggo-work wrote:
> over 100 chars

Done.

http://codereview.appspot.com/6445079/diff/6002/src/core/SkBitmapHeap.h#newco...
src/core/SkBitmapHeap.h:118: virtual SkBitmap* getBitmap(int32_t slot) {
On 2012/08/07 17:31:58, scroggo-work wrote:
> SK_OVERRIDE
> 
> Could also be const.

Done.