[Webkit-unassigned] [Bug 110778] Unlock partially decoded images after passing them to the ImageDecodingStore

[bugzilla-daemon at webkit.org]

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





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

>>>>> Source/WebCore/platform/graphics/chromium/ImageFrameGenerator.cpp:145
>>>>> +        cachedDecoder->lockFrameBuffers();
>>>> 
>>>> You want to call this in ImageDecodingStore::lockCache() such that if lockFrameBuffers fails then a new decoder is created.
>>> 
>>> The lockFrameBuffers() will not fail here. We reach here only if ImageDecodingStore::lockCache() succeeds. Since that call will lock the DiscardablePixelRef, so lockFrameBuffers() will not fail. It only increments the pixelref lock count by 1.
>>> Maybe I should add a comment to mention that the FrameBuffer is already locked when this gets called, so we are safe to use the same decoder.
>> 
>> One more thing: ImageDecodingStore doesn't know the status of the decoding, while ImageFrameGenerator keeps track of the decoding status.
>> If we lock the frameBuffer inside lockCache(), imageDecodingStore will have a problem to determine when to unlock the framebuffer. The next call can be either unlockCache, insertAndLockCache, or overwriteAndLockCache.
>> Since this lock is associated with decoder, i think it should belong to ImageFrameGenerator, not the ImageDecodingStore. ideas?
> 
> This is probably okay for now for single frame images but won't work well for multiframe images.
> 
> Please add a comment here to explain why this call is safe and that it only works for single frame images. I would even do this:
> 
> bool frameBuffersLocked = cacheDecoder->lockFrameBuffers();
> ASSERT_UNUSED(frameBuffersLocked, frameBuffersLocked);

Done.

>>>> Source/WebCore/platform/graphics/chromium/ImageFrameGenerator.cpp:150
>>>> +            cachedDecoder->unlockFrameBuffers();
>>> 
>>> Move this to ImageDecodingStore as well.
>> 
>> same reason
> 
> Also add an explanation here why unlock is needed. The reason here is because ImageDecodingStore deletes the ImageDecoder if image is complete.

Done.

>>>> Source/WebCore/platform/graphics/chromium/ImageFrameGenerator.cpp:178
>>>> +        decoder->unlockFrameBuffers();
>>> 
>>> Move this to ImageDecodingStore.
>> 
>> same reason, we are safe to call this here.
> 
> Same here, add explanation please.

Done.

>> Source/WebCore/platform/image-decoders/ImageDecoder.h:426
>> +        virtual void lockFrameBuffers()
> 
> Can you change this to bool? It returns true if all frames are locked, false otherwise.

Done.

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