[Webkit-unassigned] [Bug 106842] adding support for DiscardablePixelRef for caching lazily decoded images

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=106842





--- Comment #27 from Min Qin <qinmin at chromium.org>  2013-01-18 12:46:37 PST ---
(From update of attachment 183223)
View in context: https://bugs.webkit.org/attachment.cgi?id=183223&action=review

>> Source/Platform/chromium/public/Platform.h:-160
>> -
> 
> nit: Don't remove this line.

Done

>> Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:45
>> +    dst->lockPixels();
> 
> I think this lockPixels() is not balanced by any unlockPixels().
> 
> I need to think about how to count all lock/unlock for ASSERT.

Removed the lockPixels() call here. We don't need this for DiscardablePixelRef, the memory should be pinned after allocDiscardableMemory(). 
I think there are some confusion here： lock the pixelref and pin the memory. The pixelref doesn't needs to be locked, but the memory needs to be pinned so imagede coders can write to it. Later on, the first lockPixels() call will always succeed and pin the memory, and the unlockPixels() call will unlock and unpin the memory.

>>> Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:65
>>> +    return m_discardable;
>> 
>> Is it possible that after this is allocated that it is immediately discard before lock? I think allocateAndLock has to be one operation.
> 
> +1 to that.  Could this be done in the onLockPixels() somehow?

This call should lock the pixelref and pin the memory. We don't want to move this into onLockPixels as then we have to deal with lock/unlock balances initially. And since we are not calling dst->lockPixels(), so we should not move this into onLockPixels().

>> Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:73
>> +    return m_lockedMemory;
> 
> This looks like it's the only place m_lockedMemory is assigned, and it'll return garbage if m_discardable is NULL.  Am I missing something?

good catch, I should set m_lockedMemory to NULL if m_discardable is NULL.

>> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:39
>> +
> 
> nit: No need to add empty lines.

Done

>>> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:114
>>> +    if (m_discardableMemoryEnabled && !(*cachedImage)->bitmap().pixelRef()) {
>> 
>> Is this correct? I think SkBitmap always has a pointer to SkPixelRef but SkBitmap::getPixels() can return 0 because lock on SkPixelRef failed.
> 
> SkBitmap can exist without a PixelRef (when the SkBitmap owns its own memory directly).  Not sure if that'll happen in this particular case, but it's definitely a supported semantic.

You are right, this seems a bug, should be getPixels().

>> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:115
>> +        removeCacheEntry(cacheEntry);
> 
> I believe you have to unlock the SkBitmap no matter what, before you can remove it.

Done.

>> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:-114
>> -
> 
> nit: Don't remove empty lines.

Done.

>> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.h:76
>> +    bool discardableMemoryEnabled() { return m_discardableMemoryEnabled; }
> 
> I would like to see some unit tests, even if it is just for Android.

Not sure unittest worth it here. I have added some unittest in base/.

>> Source/WebCore/platform/graphics/chromium/ImageFrameGenerator.cpp:118
>> +        scaledBitmap = skia::ImageOperations::Resize(fullSizeImage->bitmap(), resizeMethod(), scaledSize.width(), scaledSize.height(), &allocator);
> 
> Please add a comment of what this call entails.
> 
> This call allocates DiscardablePixelRef, pin and then unpin. Which means using scaledBitmap can be discarded immediately after this call so warn it here.

Done

>> Source/WebCore/platform/graphics/chromium/ImageFrameGenerator.cpp:-202
>> -
> 
> nit: Don't remove empty lines.

Done

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.