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

>> Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:46
>> +    OwnPtr<DiscardablePixelRef> pixelRef = adoptPtr(new DiscardablePixelRef(ctable, adoptPtr(new SkMutex())));
> 
> sorry for misleading you earlier, but if DiscardablePixelRef is SkRefCnt'd  then it's not a good idea to put it in an OwnPtr after all. This could go in an SkTAutoUnref<>

ok, will change

>> Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:49
>> +        dst->setPixelRef(pixelRef.leakPtr())->unref();
> 
> i'm not sure what the ->unref here is supposed to be balancing. SkBitmap appears to take a reference on the SkPixelRef, but it's storing it in a member so it should hold a ref.  is someone/something else ref()ing the SkPixelRef that we need to compensate for?

ok, will remove

>> Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:51
>> +        // It is necessary to lock this SkBitmap to have a valid pointer to SkPixelRef.
> 
> i can't figure out what "... a valid pointer to SkPixelRef" could mean. Do you mean a valid pointer to pixels (aka SkBitmap::fPixels set) ? why is it necessary that happens in this call instead of later?

Yes, so in skbitmap::lockpixels（）， updatePixelsFromRef() is the call we really we want, so fPixels will be set. This is to match the bahavior of skMallocPixelRef.

>>>> Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:89
>>>> +        m_lockedMemory = m_discardable->data();
>>> 
>>> this seems confused. what's the purpose of m_firstOnLock if we do the exact same thing (except for ASSERTS) in both branches?
>> 
>> we cannot lock the memory twice, since the memory is initially locked after allocation, the first onLockPixels() should not lock the memory again. So m_firstOnLock just skips it, while in other cases we call lock().
> 
> why not just set m_lockedMemory in allocAndLockDiscardableMemory when the allocateAndLock... succeeds and skip all this? then the m_lockedMemory pointer will be valid from allocation (if it succeeds) and you don't have to worry about initial setup logic here

We can do that, but we still have to call skbitmap->lockPixels() and that will call into this function.

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