[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 #48 from Min Qin <qinmin at chromium.org>  2013-01-25 13:11:17 PST ---
(From update of attachment 184554)
View in context: https://bugs.webkit.org/attachment.cgi?id=184554&action=review

>> Source/WebCore/ChangeLog:9
>> +        Instead of storing images in the memory, decoded images will be stored in DicardableMemory(such as ashmem).
> 
> Please state clearly what is involved in this patch:
> 
> - Allow use of discardable memory in deferred image decoding path.
> - Fully decoded images are unpinned and stored in ImageDecodingStore.
> - Partially decoded images are pinned and stored in ImageDecodingStore.
> - Separate caching policy for discardable memory
>    - Separate cache size limit for discardable entries
>    - Separate cache count limit for discardable entries

Done

>> Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:51
>> +        dst->lockPixels();
> 
> Say this instead:
> 
> This method is only called when a DiscardablePixelRef is created to back a SkBitmap. It is necessary to lock this SkBitmap to have a valid pointer to SkPixelRef.

Done

>>> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:113
>>> +        if (!pixelRef->pixels()) {
>> 
>> 1. Can we move this check before line 103 such that we only increment use count if pixelRef->pixels() return true?
>> 
>> 2. Please also run removeCacheEntryInternal(cacheEntry) outside of the lock. You can have another success variable and return outside of the block.
> 
> Sorry what I mean for 2 is to delete the CacheEntry outside the lock. I would merge removeCacheEntryInternal here and put the Vector<...> above the mutex.

Done. Added a temp variable image, so we don't return it to the caller thru cachedImage when this if statement returns false.

>> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:252
>> +        while (cacheEntry && isPruneNeeded()) {
> 
> There should be 2 while loops, one goes through heap backed cache entry. One goes through discardable cache entry.

Done. Need another vector for this though.

>> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:269
>> +bool ImageDecodingStore::needToPruneCacheEntry(const CacheEntry* cacheEntry) const
> 
> See my comments for line 252. I don't think you need a separate function for this.

Done

>> Source/WebCore/platform/graphics/chromium/ImageFrameGenerator.cpp:201
>>      ImageFrame* frame = (*decoder)->frameBufferAtIndex(0);
> 
> Add a comment after this line something like:
> 
> If this call results in a new DiscardablePixelRef created, then ImageFrame::m_bitmap and DiscardablePixelRef contained is unlocked when ImageDecoder is destroyed. This is because ImageDecoder owns ImageFrame. We do not unlock ImaegFrame::m_bitmap, the consequence is that partially decoded SkBitmap will not unlocked when it is inserted into the ImageDecodingStore.

Done

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