[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 #5 from Hin-Chung Lam <hclam at google.com>  2013-02-27 11:02:48 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);

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

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

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

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