Bug 32121 – [Qt] implement ImageDecoderQt::clearFrameBufferCache()

• Component: New Bugs
• Status: RESOLVED INVALID

[Yongjun Zhang]

ImageDecoder::clearFrameBufferCache(int index) is called to purge decoded images for animated GIFs.  The current implementation in Qt is empty.

[Yongjun Zhang]

Created attachment 44245 [details]
first attempt to implement willLoadFromCache.

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/graphics/qt/ImageDecoderQt.cpp:154:  Missing spaces around =  [whitespace/operators] [4]
WebCore/platform/graphics/qt/ImageDecoderQt.cpp:154:  Missing spaces around <  [whitespace/operators] [3]
WebCore/platform/graphics/qt/ImageDecoderQt.cpp:163:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 3

[Yongjun Zhang]

Created attachment 44250 [details]
fix style violations.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44250 [details] without any errors.

[Eric Seidel (no email)]

Looks sane to me.

[Simon Hausmann]

Holger, you're best qualified to review this patch :)

[Holger Freyther]

Comment on attachment 44250 [details]
fix style violations.


> Index: WebCore/platform/graphics/qt/ImageDecoderQt.cpp
> ===================================================================
> --- WebCore/platform/graphics/qt/ImageDecoderQt.cpp	(revision 51636)
> +++ WebCore/platform/graphics/qt/ImageDecoderQt.cpp	(working cop
> -void ImageDecoderQt::clearFrameBufferCache(size_t /*index*/)
> +void ImageDecoderQt::clearFrameBufferCache(size_t index)
>  {
> +    if (index >= m_frameBufferCache.size())
> +        return;

I think this should be >...as of the semantic of clearFrameBufferCache..



> +    // create the image reader object again
> +    if (!m_reader)
> +        m_reader = new QImageReader(m_buffer, m_format);

m_buffer is deleted here...compare with ::FailRead and internalReadImage...



You should move the deletion and creation of m_reader, m_buffer into a common method.

[Yongjun Zhang]

Created attachment 44809 [details]
include zecke's comments.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44809 [details] without any errors.

[Kenneth Rohde Christiansen]

Comment on attachment 44809 [details]
include zecke's comments.

LGTM

[Holger Freyther]

Cool! You could also call RGBA32Buffer::clear to get the same effect as setting the status and setting an empty image.

[WebKit Commit Bot]

Comment on attachment 44809 [details]
include zecke's comments.

Clearing flags on attachment: 44809

Committed r52516: <http://trac.webkit.org/changeset/52516>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Holger Freyther]

On second look the patch is not correct.

There is no gurantee that the clearFromFrameBufferCache is only called once. It might be called more than once and then we will run into the assert on debug, and leak memory in release. And this is interesting. We try to do a patch to save memory and then we create a memory leak. Something must be wrong with our process.


I think we should go back to start and do it properly this time:

1.) create a manual test with a big enough GIF that will actually call clearFromFrameBufferCache as nothing in the layout test suite is executing this behaviour, and we also need to verify that we load all frames again.

2.) Do the change, run the test, look at the memory usage, upload the images of the memory usage (memusagestat).

3.) Land the patch if we actually save memory and we pass the manual test.

[Yongjun Zhang]

(In reply to comment #14)
> On second look the patch is not correct.
> 
> There is no gurantee that the clearFromFrameBufferCache is only called once. It
> might be called more than once and then we will run into the assert on debug,
> and leak memory in release. And this is interesting. We try to do a patch to
> save memory and then we create a memory leak. Something must be wrong with our
> process.

Good catch! The current implementation decodes all the frames in GIF image in ImageDecoderQt::frameCount() because m_reader->imageCount() always returns 0, which I believe it is a Qt bug. The problem you mentioned will show up when clearFromFrameBufferCache is called for the second time AND when there are still frames waiting for being decoded.

I think a NULL check on m_buffer and m_reader should be enough to fix this. Comments?
 
> 
> I think we should go back to start and do it properly this time:
> 
> 1.) create a manual test with a big enough GIF that will actually call
> clearFromFrameBufferCache as nothing in the layout test suite is executing this
> behaviour, and we also need to verify that we load all frames again.
> 
> 2.) Do the change, run the test, look at the memory usage, upload the images of
> the memory usage (memusagestat).
> 
> 3.) Land the patch if we actually save memory and we pass the manual test.

Sure, I will update the fix and run the manual test.

[Yongjun Zhang]

Created attachment 46303 [details]
big animated GIF for manual test (1097x899 with 12 frames).

[Yongjun Zhang]

I did manual test with a big animated GIF (see attached).  The result didn't show any memory savings ;(

What happens is after each loop, all frames are cleared and cachedFrameBuffer is also emptied.  When ImageDecoderQt::frameBufferAtIndex() is called again for the next frame, the decoder has to figure out the frame count by ImageDecoderQt::frameCount().  Due to an error in Qt image decoder, imageReader->Count() always returns 0, which triggers ImageDecoderQt to decode all the frames in forceLoadEverything().

As a result, ImageDecoderQt always decodes all frames.  I believe clearFrameBufferCache() is designed with the assumption that platform image decoders should decode animated frames on-demand, not decoding them as a whole bunch. 

I think the patch would work (after null-checking the m_buffer & m_reader) if Qt image decoder can reliably fetch image counts WITHOUT decoding all frames.  Otherwise this patch won't help reduce memory consumption as expected.

[Peter Kasting]

You can look at the open-source GIF decoder in WebCore/platform to see how it handles this stuff.  Basically, it assumes that it might get different frame count values throughout the image decoding process, so every time setData() is called, it dirties the frame count.  But once the end of the image has been reached, it knows that it has the correct frame count and keeps it forever after (even if clearFrameBufferCache() is called).

Meanwhile, clearFrameBufferCache() is called in such a way as to free frames we don't need once we've decoded them, assuming that we have only decoded up through the desired current frame.  If at some point you force all frames to decode at once, then even if you subsequently support clearFrameBufferCache(), your high-water memory usage is going to be just as bad as if you didn't support it.

So you want to avoid anything that forces all frames to decode.  This may mean that you have to deal with uncertain or changing frame counts during your first loop through the image.

[Yongjun Zhang]

Created attachment 46377 [details]
null-check m_buffer and m_reader to avoid crashing in debug mode.

As per previous comment, without fixing qt Imagedecoder's frameCount() bug, this patch won't help reduce the peak memory consumption.

However I still would like to submit this patch for comments.

[Simon Hausmann]

The Qt issue is now tracked at http://bugreports.qt.nokia.com/browse/QTBUG-7514

Yongjun, can you elaborate in JIRA what exactly the Qt bug is?

[Yongjun Zhang]

(In reply to comment #20)
> The Qt issue is now tracked at http://bugreports.qt.nokia.com/browse/QTBUG-7514
> 
> Yongjun, can you elaborate in JIRA what exactly the Qt bug is?

Hi Simon, I just added the following commment to the JIRA report.  Hopefully it can help.

"Some details about the QImageReader issue:

For GIF animated images, QImageReader::imageCount() always returns 0, regardless of the image data are complete or not. This forces ImageDecoderQt to decode ALL the frames in order to figure out the frame counts which causes unnecessary memory consumption & performance penalty.

It seems like QImageReader::jumpToImage(int) doesn't work either - it returns false always.
"

[Tor Arne Vestbø]

Please follow the QtWebKit bug reporting guidelines when reporting bugs.

See http://trac.webkit.org/wiki/QtWebKitBugs

Specifically:

  - The 'QtWebKit' component should be used for bugs/features in the public QtWebKit API layer, not to signify that the bug is specific to the Qt port of WebKit

    http://trac.webkit.org/wiki/QtWebKitBugs#Component