Use the SkBitmapHeap to handle SkBitmaps in SkGPipe cross process.

Use the SkBitmapHeap to handle SkBitmaps in SkGPipe cross process.

Required moving the LRU handles from SkBitmapHeapEntry to LookupEntry.

Allows simplification of drawBitmap* calls in SkGPipeCanvas.

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

[DerekS, 2012-08-13 13:13:14]

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

https://codereview.appspot.com/6460073/diff/2001/src/core/SkBitmapHeap.cpp#ne...
src/core/SkBitmapHeap.cpp:136: // With external storage, looking at the bitmap
is meaningless.
Can't we just compare the genID's of the replacement bitmaps pixelRef with the
one we have stored in our lookup entry?

https://codereview.appspot.com/6460073/diff/2001/src/core/SkBitmapHeap.cpp#ne...
src/core/SkBitmapHeap.cpp:155: SkBitmapHeapEntry* heapEntry;
does this need to be outside the scope of the while loop?

https://codereview.appspot.com/6460073/diff/2001/src/core/SkBitmapHeap.cpp#ne...
src/core/SkBitmapHeap.cpp:272: // later.
this comment can all be on one line

https://codereview.appspot.com/6460073/diff/2001/src/core/SkBitmapHeap.cpp#ne...
src/core/SkBitmapHeap.cpp:273: if (fMostRecentlyUsed == lookupEntry) {
why not just put the logic of this if statement into
this->removeFromLRU(lookupEntry) instead of having it here?

https://codereview.appspot.com/6460073/diff/2001/src/core/SkBitmapHeap.h
File src/core/SkBitmapHeap.h (right):

https://codereview.appspot.com/6460073/diff/2001/src/core/SkBitmapHeap.h#newc...
src/core/SkBitmapHeap.h:175: SkASSERT(fExternalStorage != NULL ||
fStorage.count() - fUnusedSlots.count() == fLookupTable.count());
exceeds 100 character limit

https://codereview.appspot.com/6460073/diff/2001/src/core/SkBitmapHeap.h#newc...
src/core/SkBitmapHeap.h:215: static int Compare(const LookupEntry* a, const
LookupEntry* b) {
let's move the contents of this function to the cpp.

https://codereview.appspot.com/6460073/diff/2001/src/core/SkBitmapHeap.h#newc...
src/core/SkBitmapHeap.h:240: * is no longer valid.
This last sentence could use some clarification. I'm not sure what it means?
Does it mean the pointer is no longer valid after the function returns?

[Leon, 2012-08-13 14:48:52]

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

https://codereview.appspot.com/6460073/diff/2001/src/core/SkBitmapHeap.cpp#ne...
src/core/SkBitmapHeap.cpp:136: // With external storage, looking at the bitmap
is meaningless.
On 2012/08/13 13:13:15, DerekS wrote:
> Can't we just compare the genID's of the replacement bitmaps pixelRef with the
> one we have stored in our lookup entry?

Done.

https://codereview.appspot.com/6460073/diff/2001/src/core/SkBitmapHeap.cpp#ne...
src/core/SkBitmapHeap.cpp:155: SkBitmapHeapEntry* heapEntry;
On 2012/08/13 13:13:15, DerekS wrote:
> does this need to be outside the scope of the while loop?

No. Done.

https://codereview.appspot.com/6460073/diff/2001/src/core/SkBitmapHeap.cpp#ne...
src/core/SkBitmapHeap.cpp:272: // later.
On 2012/08/13 13:13:15, DerekS wrote:
> this comment can all be on one line

Done.

https://codereview.appspot.com/6460073/diff/2001/src/core/SkBitmapHeap.cpp#ne...
src/core/SkBitmapHeap.cpp:273: if (fMostRecentlyUsed == lookupEntry) {
On 2012/08/13 13:13:15, DerekS wrote:
> why not just put the logic of this if statement into
> this->removeFromLRU(lookupEntry) instead of having it here?

Because the other time we call removeFromLRU, we do not want this behavior.
Granted, the other time we may call it, we skip it if fMostRecentlyUsed ==
lookupEntry, so it would not change the behavior, but it would mean an
unnecessary check for that other caller.

https://codereview.appspot.com/6460073/diff/2001/src/core/SkBitmapHeap.h
File src/core/SkBitmapHeap.h (right):

https://codereview.appspot.com/6460073/diff/2001/src/core/SkBitmapHeap.h#newc...
src/core/SkBitmapHeap.h:175: SkASSERT(fExternalStorage != NULL ||
fStorage.count() - fUnusedSlots.count() == fLookupTable.count());
On 2012/08/13 13:13:15, DerekS wrote:
> exceeds 100 character limit

Done.

https://codereview.appspot.com/6460073/diff/2001/src/core/SkBitmapHeap.h#newc...
src/core/SkBitmapHeap.h:215: static int Compare(const LookupEntry* a, const
LookupEntry* b) {
On 2012/08/13 13:13:15, DerekS wrote:
> let's move the contents of this function to the cpp.

Done.

https://codereview.appspot.com/6460073/diff/2001/src/core/SkBitmapHeap.h#newc...
src/core/SkBitmapHeap.h:240: * is no longer valid.
On 2012/08/13 13:13:15, DerekS wrote:
> This last sentence could use some clarification. I'm not sure what it means?
> Does it mean the pointer is no longer valid after the function returns?

Yes. I do not love that behavior, but both callers to this function need to
delete it. Since this is a private function, I figured I could keep the delete
in the function and comment it.

If, for some reason, some future caller put a LookupEntry on the stack and
called this on it, their entry will still exist, but the one in the table will
have been deleted.

I have updated the comment. Let me know if it is clearer. Also let me know if
you think we should do it differently.

[DerekS, 2012-08-13 15:01:55]

lgtm

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

https://codereview.appspot.com/6460073/diff/2001/src/core/SkBitmapHeap.cpp#ne...
src/core/SkBitmapHeap.cpp:273: if (fMostRecentlyUsed == lookupEntry) {
On 2012/08/13 14:48:52, scroggo-work wrote:
> On 2012/08/13 13:13:15, DerekS wrote:
> > why not just put the logic of this if statement into
> > this->removeFromLRU(lookupEntry) instead of having it here?
> 
> Because the other time we call removeFromLRU, we do not want this behavior.
> Granted, the other time we may call it, we skip it if fMostRecentlyUsed ==
> lookupEntry, so it would not change the behavior, but it would mean an
> unnecessary check for that other caller.

It seems cleaner to have all the logic in one place and I don't think the one if
statement is going to be costly, but I'm not too concerned about it.

[Leon, 2012-08-13 16:24:24]

On 2012/08/13 15:01:55, DerekS wrote:
> lgtm
> 
> https://codereview.appspot.com/6460073/diff/2001/src/core/SkBitmapHeap.cpp
> File src/core/SkBitmapHeap.cpp (right):
> 
>
https://codereview.appspot.com/6460073/diff/2001/src/core/SkBitmapHeap.cpp#ne...
> src/core/SkBitmapHeap.cpp:273: if (fMostRecentlyUsed == lookupEntry) {
> On 2012/08/13 14:48:52, scroggo-work wrote:
> > On 2012/08/13 13:13:15, DerekS wrote:
> > > why not just put the logic of this if statement into
> > > this->removeFromLRU(lookupEntry) instead of having it here?
> > 
> > Because the other time we call removeFromLRU, we do not want this behavior.
> > Granted, the other time we may call it, we skip it if fMostRecentlyUsed ==
> > lookupEntry, so it would not change the behavior, but it would mean an
> > unnecessary check for that other caller.
> 
> It seems cleaner to have all the logic in one place and I don't think the one
if
> statement is going to be costly, but I'm not too concerned about it.

Fair enough. Moved the logic into removeFromLRU

[Leon, 2012-08-13 16:24:24]

On 2012/08/13 15:01:55, DerekS wrote:
> lgtm
> 
> https://codereview.appspot.com/6460073/diff/2001/src/core/SkBitmapHeap.cpp
> File src/core/SkBitmapHeap.cpp (right):
> 
>
https://codereview.appspot.com/6460073/diff/2001/src/core/SkBitmapHeap.cpp#ne...
> src/core/SkBitmapHeap.cpp:273: if (fMostRecentlyUsed == lookupEntry) {
> On 2012/08/13 14:48:52, scroggo-work wrote:
> > On 2012/08/13 13:13:15, DerekS wrote:
> > > why not just put the logic of this if statement into
> > > this->removeFromLRU(lookupEntry) instead of having it here?
> > 
> > Because the other time we call removeFromLRU, we do not want this behavior.
> > Granted, the other time we may call it, we skip it if fMostRecentlyUsed ==
> > lookupEntry, so it would not change the behavior, but it would mean an
> > unnecessary check for that other caller.
> 
> It seems cleaner to have all the logic in one place and I don't think the one
if
> statement is going to be costly, but I'm not too concerned about it.

Fair enough. Moved the logic into removeFromLRU

[DerekS, 2012-08-13 18:12:17]

lgtm

[TomH, 2012-08-13 18:22:14]

Reverted in r5067 because we think this broke unit tests, I talked to Leon about
it when I noticed it, but neither of you seem to be around now...

[DerekS, 2012-08-13 18:26:40]

On 2012/08/13 18:22:14, TomH wrote:
> Reverted in r5067 because we think this broke unit tests, I talked to Leon
about
> it when I noticed it, but neither of you seem to be around now...

Ok.  I'll let Leon investigate this when he gets back thanks for keeping an eye
on the tree.

[Leon, 2012-08-13 18:56:00]

Sorry about that. I'll look into it now.


On Mon, Aug 13, 2012 at 2:26 PM, <djsollen@google.com> wrote:

> On 2012/08/13 18:22:14, TomH wrote:
>
>> Reverted in r5067 because we think this broke unit tests, I talked to
>>
> Leon about
>
>> it when I noticed it, but neither of you seem to be around now...
>>
>
> Ok.  I'll let Leon investigate this when he gets back thanks for keeping
> an eye on the tree.
>
>
https://codereview.appspot.**com/6460073/<https://codereview.appspot.com/6460...
>



-- 
Leon Scroggins III
scroggo@google.com