[Webkit-unassigned] [Bug 110778] Unlock partially decoded images after passing them to the ImageDecodingStore
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Feb 27 12:21:03 PST 2013
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.
More information about the webkit-unassigned
mailing list