Bug 110778 – Unlock partially decoded images after passing them to the ImageDecodingStore

• Component: New Bugs
• Status: RESOLVED FIXED

[Min Qin]

Unlock partially decoded images after passing them to the ImageDecodingStore

[Min Qin]

Created attachment 190101 [details]
Patch

[Hin-Chung Lam]

Comment on attachment 190101 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190101&action=review

Please move locking logic of ImageDecoder into ImageDecodingStore. ImageFrameGenerator shouldn't know about the underlying, it should only know that when an entry is locked, all locked resources are ready to be used.

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

> Source/WebCore/platform/graphics/chromium/ImageFrameGenerator.cpp:150
> +            cachedDecoder->unlockFrameBuffers();

Move this to ImageDecodingStore as well.

> Source/WebCore/platform/graphics/chromium/ImageFrameGenerator.cpp:178
> +        decoder->unlockFrameBuffers();

Move this to ImageDecodingStore.

[Min Qin]

Comment on attachment 190101 [details]
Patch

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.

>> Source/WebCore/platform/graphics/chromium/ImageFrameGenerator.cpp:150
>> +            cachedDecoder->unlockFrameBuffers();
> 
> Move this to ImageDecodingStore as well.

same reason

>> Source/WebCore/platform/graphics/chromium/ImageFrameGenerator.cpp:178
>> +        decoder->unlockFrameBuffers();
> 
> Move this to ImageDecodingStore.

same reason, we are safe to call this here.

[Min Qin]

Comment on attachment 190101 [details]
Patch

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?

[Hin-Chung Lam]

Comment on attachment 190101 [details]
Patch

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.

[Hin-Chung Lam]

I'm okay to keep this change simple and leave the problems for multiframe images later.

A few more changes and this is good to go! :)

[Min Qin]

Created attachment 190576 [details]
Patch

[Min Qin]

Comment on attachment 190101 [details]
Patch

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.

[Hin-Chung Lam]

Comment on attachment 190576 [details]
Patch

LGTM.

[Min Qin]

Stephen, would you please take a look? Thanks

[Stephen White]

Comment on attachment 190576 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190576&action=review

Pretty much a rubber-stamp.  r=me

> Source/WebCore/platform/graphics/chromium/ImageFrameGenerator.cpp:157
> +        // ImageDecodingStore should have deleted the decoder here.

Nit:  Is this something worth asserting in code of instead of in a comment, if possible?

[Min Qin]

Comment on attachment 190576 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190576&action=review

>> Source/WebCore/platform/graphics/chromium/ImageFrameGenerator.cpp:157
>> +        // ImageDecodingStore should have deleted the decoder here.
> 
> Nit:  Is this something worth asserting in code of instead of in a comment, if possible?

hmm... probably this is not a good place to put the assertion. The cachedDecoder here is a dumb pointer. In order to check whether the decoder is deleted, we have to call lockCache() again. But that would break the lock/unlock situation.
An alternative is to put the assertion check into overwriteAndLockCache(). But the logic is already pretty straightforward inside that function.

[WebKit Review Bot]

Comment on attachment 190576 [details]
Patch

Clearing flags on attachment: 190576

Committed r144242: <http://trac.webkit.org/changeset/144242>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.