Make SkGPipe use SkBitmapHeap.

Make SkGPipe use SkBitmapHeap.

In the single process (or hypothetical cross process/shared address
space) mode, SkGPipe now uses SkBitmapHeap instead of SharedHeap.

Still need to use the shared heap for shaders as well as for cross
process.

TEST=PipeTest

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

[DerekS, 2012-08-08 13:16:51]

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

https://codereview.appspot.com/6461059/diff/1/src/core/SkBitmapHeap.cpp#newco...
src/core/SkBitmapHeap.cpp:283: entry = fStorage[slot];
need to set replacingAnOldEntry to true here.

https://codereview.appspot.com/6461059/diff/1/src/core/SkBitmapHeap.cpp#newco...
src/core/SkBitmapHeap.cpp:306: if (!replacingAnOldEntry) {
if we don't set replacingAnOldEntry to true when reusing a slot we will fall
into this code and delete an entry that may not be at the end of the list.

As another thought do we need replacingAnOldEntry at all? This if statement
could just check if the entry is the last in the list since that is the only
time we want to remove it.

if (fStorage.count() - 1 == entry->fSlot) { ... }

https://codereview.appspot.com/6461059/diff/1/src/pipe/SkGPipeWrite.cpp
File src/pipe/SkGPipeWrite.cpp (right):

https://codereview.appspot.com/6461059/diff/1/src/pipe/SkGPipeWrite.cpp#newco...
src/pipe/SkGPipeWrite.cpp:240: SkBitmapHeap*      fSharedHeap;
you could put this on the stack, but you would have to ensure your writer
outlives the readers.  I think that is already the case though as the reader's
don't refCnt the heap.  I would suggest you do one or the other.

[junov1, 2012-08-08 13:39:18]

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

https://codereview.appspot.com/6461059/diff/1/src/core/SkBitmapHeap.cpp#newco...
src/core/SkBitmapHeap.cpp:238: int index = findInLookupTable(key, NULL);
this->

https://codereview.appspot.com/6461059/diff/1/src/core/SkBitmapHeap.cpp#newco...
src/core/SkBitmapHeap.cpp:239: SkASSERT(count == fLookupTable.count());
If this is a way of verifying that findInLookupTable found an already existing
entry, I have to say that it's kind of non-obvious to the reader. An explanatory
comment would be nice.

https://codereview.appspot.com/6461059/diff/1/src/core/SkBitmapHeap.cpp#newco...
src/core/SkBitmapHeap.cpp:309: SkDELETE(entry);
I think you might need fBytesAllocated -= sizeof(SkBitmapHeapEntry);
That assert you added in the destructor should be able to tell you whether you
are doing it right.  Have you tried running debug builds of gm and tests with
this change?

Also, in what cases does the copy fail?  Do we have a unit test that visits this
code path?

[Leon, 2012-08-08 18:44:31]

New patch.

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

https://codereview.appspot.com/6461059/diff/1/src/core/SkBitmapHeap.cpp#newco...
src/core/SkBitmapHeap.cpp:238: int index = findInLookupTable(key, NULL);
On 2012/08/08 13:39:18, junov1 wrote:
> this->

Done.

https://codereview.appspot.com/6461059/diff/1/src/core/SkBitmapHeap.cpp#newco...
src/core/SkBitmapHeap.cpp:239: SkASSERT(count == fLookupTable.count());
On 2012/08/08 13:39:18, junov1 wrote:
> If this is a way of verifying that findInLookupTable found an already existing
> entry, I have to say that it's kind of non-obvious to the reader. An
explanatory
> comment would be nice.

Done.

https://codereview.appspot.com/6461059/diff/1/src/core/SkBitmapHeap.cpp#newco...
src/core/SkBitmapHeap.cpp:283: entry = fStorage[slot];
On 2012/08/08 13:16:51, DerekS wrote:
> need to set replacingAnOldEntry to true here.

Removed, per next comment.

https://codereview.appspot.com/6461059/diff/1/src/core/SkBitmapHeap.cpp#newco...
src/core/SkBitmapHeap.cpp:306: if (!replacingAnOldEntry) {
On 2012/08/08 13:16:51, DerekS wrote:
> if we don't set replacingAnOldEntry to true when reusing a slot we will fall
> into this code and delete an entry that may not be at the end of the list.
> 
> As another thought do we need replacingAnOldEntry at all? This if statement
> could just check if the entry is the last in the list since that is the only
> time we want to remove it.
> 
> if (fStorage.count() - 1 == entry->fSlot) { ... }

Good point. I'll do that instead.

https://codereview.appspot.com/6461059/diff/1/src/core/SkBitmapHeap.cpp#newco...
src/core/SkBitmapHeap.cpp:309: SkDELETE(entry);
On 2012/08/08 13:39:18, junov1 wrote:
> I think you might need fBytesAllocated -= sizeof(SkBitmapHeapEntry);

Agreed.

> That assert you added in the destructor should be able to tell you whether you
> are doing it right.  Have you tried running debug builds of gm and tests with
> this change?

Yes.

> Also, in what cases does the copy fail?  Do we have a unit test that visits
this
> code path?

In the old code, cmykjpeg failed, but now we special case an empty bitmap and
just reset the copy, which succeeds. 

Trying to copy a bitmap that has no config or is not readyToDraw will fail.

Trying to copy from/to a particular set of configs can fail, but we are always
using the same config. Copy can fail if allocPixels fails, which can happen if
malloc fails or the bitmap's size is greater than 32 bits.

Copying a bitmap with a GrPixelRef can also fail, although I do not understand
how that happens.

I can add a test that checks for this.

https://codereview.appspot.com/6461059/diff/1/src/pipe/SkGPipeWrite.cpp
File src/pipe/SkGPipeWrite.cpp (right):

https://codereview.appspot.com/6461059/diff/1/src/pipe/SkGPipeWrite.cpp#newco...
src/pipe/SkGPipeWrite.cpp:240: SkBitmapHeap*      fSharedHeap;
On 2012/08/08 13:16:51, DerekS wrote:
> you could put this on the stack, but you would have to ensure your writer
> outlives the readers.  I think that is already the case though as the reader's
> don't refCnt the heap.  I would suggest you do one or the other.

I do ensure that the Canvas (and thus the BitmapHeap) is alive as long as the
PipeController, but I do not think the client of the reader should have to know
about the BitmapHeap. I think a good solution will be to have the PipeState ref
the BitmapHeap.

[junov1, 2012-08-08 19:59:55]

I'm ok with this.  
lgtm

[DerekS, 2012-08-08 20:40:17]

lgtm