[Webkit-unassigned] [Bug 106842] adding support for DiscardablePixelRef for caching lazily decoded images
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Jan 25 11:23:55 PST 2013
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.
More information about the webkit-unassigned
mailing list