Bug 177864 – [GTK][WPE] GIFImageDecoder never clears decoded frames even when told to do so

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Miguel Gomez]

While testing the fix for https://bugs.webkit.org/show_bug.cgi?id=176089, I realized that there was something weird going on with GIFImageDecoder::clearFrameBufferCache(). Despite I was being called, it wasn't really freeing any buffer. Due to this, all the decoded frames will stay in the cache and consume tons of memory instead of respecting the limit of 30MB.

The fix to properly delete frames from the cache is trivial, but doing so triggers an unexpected issue:
clearFrameBufferCache() receives a clearBeforeFrame parameter, searches backwards for the frame required to render clearBeforeFrame (clearing unneeded frames in between), and then clears all the buffers before the required one. This is because it expects that the next frame rendered will be clearBeforeFrame, and in order to accelerate that we keep the required frame decoded.
The problem comes when, after that, we request to decode a former frame that was cleared from the cache. We can't start decoding that frame directly because it may depend on a former frame. So we would need to find that former frame and decode it first. This causes that the fix for https://bugs.webkit.org/show_bug.cgi?id=176089 doesn't work once clearFrameBufferCache is working properly, because it would start decoding the required frame.

The proper fix for this would be, besides fixing cleatFrameBufferCache(), implementing a function to find the frame that is required to render the requested frame, and start decoding from there. We could just decode always from frame 0, and it would work, but it would be very inefficient.

[Miguel Gomez]

Created attachment 322809 [details]
Patch

[Build Bot]

Attachment 322809 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
Total errors found: 2 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Comment on attachment 322809 [details]
Patch

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

> Source/WebCore/platform/image-decoders/gif/GIFImageDecoder.cpp:110
> +    for (size_t i = frameIndex; i > 0; i--) {

we normally do --i in for loops.

> Source/WebCore/platform/image-decoders/gif/GIFImageDecoder.cpp:128
> +            const GIFFrameContext* frameContext = m_reader->frameContext(i - 1);

const auto*. This can return nullptr, if we know for sure we should never get nullptr in this particular case, then add an assert, otherwise, handle the null case here, I guess continuing the loop.

> Source/WebCore/platform/image-decoders/gif/GIFImageDecoder.cpp:133
> +            if (frameRect.contains(IntRect(IntPoint(), size())))

I think you can do something like

if (frameRect.contains({ { }, size() } })))

> Source/WebCore/platform/image-decoders/gif/GIFImageDecoder.cpp:149
> +        size_t startFrame = findFirstRequiredFrameToDecode(index);
> +        for (size_t i = startFrame; i <= index; i++)

This could probably be a single line

for (auto i = findFirstRequiredFrameToDecode(index); i <= index; ++i)

> Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:289
> +        return frame < m_frames.size() ? m_frames[frame].get() : 0;

0 -> nullptr

[Build Bot]

Comment on attachment 322809 [details]
Patch

Attachment 322809 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4765738

New failing tests:
workers/wasm-long-compile.html

[Build Bot]

Created attachment 322813 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 322809 [details]
Patch

Attachment 322809 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4765781

New failing tests:
accessibility/ios-simulator/video-elements-ios.html

[Build Bot]

Created attachment 322818 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Miguel Gomez]

Created attachment 322823 [details]
Patch

[Build Bot]

Attachment 322823 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
Total errors found: 2 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 322823 [details]
Patch

Clearing flags on attachment: 322823

Committed r222910: <http://trac.webkit.org/changeset/222910>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.