[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 #45 from Hin-Chung Lam <hclam at google.com>  2013-01-25 11:25:49 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

> 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.

> 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.

> 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.

> 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.

> 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.

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