Bug 35957 - [Qt] Do not use QImageReader::imageCount in QtWebKit
Summary: [Qt] Do not use QImageReader::imageCount in QtWebKit
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Enhancement
Assignee: Holger Freyther
URL:
Keywords: Qt
Depends on:
Blocks:
 
Reported: 2010-03-10 00:32 PST by Holger Freyther
Modified: 2014-02-03 03:16 PST (History)
7 users (show)

See Also:


Attachments
First attempt at the removal (4.82 KB, patch)
2010-03-10 00:40 PST, Holger Freyther
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Holger Freyther 2010-03-10 00:32:59 PST
In theory decoding animations should work like the following. We determine the number of frames and then WebCore starts animating and asks us for frame 1, 2, 3, 4, ..., 20 and the throw away some images to save memory 1-8, and when we are at the end n, we have to go back to image 1.

In Qt terms this would work by calling QImageReader::imageCount and then using QImageReader::jumpToImage. Qt 4.7 has gained the abilitiy to index the gif and determine the number of images but jumpToImage is not implemented. This would mean we need to add error handling to the ImageDecoderQt.cpp in case we can not jumpToImage. On top of that the jumpToImage will not be implemented for gif.

Besides the error handling we would need to add code to detect if we access the image in order, and if we don't access it in order recreate the QImageReader. E.g. this would happen if we are at image n of a large animation and need to get back to 'n'.

All this will make the ImageDecoderQt.cpp a lot more complicated without any real gain.
Comment 1 Holger Freyther 2010-03-10 00:40:45 PST
Created attachment 50377 [details]
First attempt at the removal

With the latest changes to QGifHandler::imageCount this will likely introduce a slowdown but doing the thing is the best we can do in WebCore to assure things will work properly.
Comment 2 Tor Arne Vestbø 2010-03-10 06:44:25 PST
Please follow the QtWebKit bug reporting guidelines when reporting bugs.

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

Specifically:

  - The 'QtWebKit' component should only 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

  - Add the keyword 'Qt' to signal that it's a Qt-related bug

      http://trac.webkit.org/wiki/QtWebKitBugs#Keywords
Comment 3 Simon Hausmann 2010-03-10 13:37:05 PST
Comment on attachment 50377 [details]
First attempt at the removal

Sad, but the right thing to do. I thought that the changes actually went into Qt 4.6?

Is it worth considering to use the WebCore GIF image decoder instead?
Comment 4 Eric Seidel (no email) 2010-03-24 14:46:48 PDT
Attachment 50377 [details] was posted by a committer and has review+, assigning to Holger Freyther for commit.
Comment 5 Holger Freyther 2010-03-26 07:14:46 PDT
Comment on attachment 50377 [details]
First attempt at the removal

I have stopped working on QtWebKit for now, so someone else needs to take over here.
Comment 6 Zoltan Horvath 2011-02-22 02:06:02 PST
(In reply to comment #3)

> Is it worth considering to use the WebCore GIF image decoder instead?

I think it's not, because WebCore's GIFImageDecoder also reads the whole data to determine the frame count of the gif. (see WebCore/platform/image-decoders/gif/GIFImageDecoder.cpp:76)
Comment 7 Zoltan Horvath 2011-02-22 05:53:35 PST
This looks reasonable, I'm going to land it soon.
Comment 8 Holger Freyther 2011-02-22 05:55:57 PST
(In reply to comment #7)
> This looks reasonable, I'm going to land it soon.

Please make a performance test. :)
Comment 9 Zoltan Horvath 2011-02-24 04:37:22 PST
(In reply to comment #8)
>
> Please make a performance test. :)

I did the measurement.

r79444, Qt 4.7.1

clean:
76.5 ms +/- 1.07%

patched:
74.3 ms +/- 0.65%

I started the timer before the load method of QtTestBrowser's LauncherWindow and stopped at loadFinished slot. The QtTestBrowser was running in an xvfb. It's even faster with this change. 

Should I land it now?
Comment 10 Zoltan Horvath 2011-02-24 04:47:41 PST
I've uploaded the test case to this location (too big to upload here):
http://zoltan.sed.hu/webkit/gifloadtest.tar
Comment 11 Holger Freyther 2011-02-24 06:28:51 PST
(In reply to comment #9)
> (In reply to comment #8)
> >
> > Please make a performance test. :)
> 
> I did the measurement.

The other part of the test is memory usage. E.g. have one of this videos as a gif and let it play twice and see how memory usage is changing... sorry to be the pain here.
Comment 12 Jocelyn Turcotte 2014-02-03 03:16:15 PST
=== Bulk closing of Qt bugs ===

If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it and remove [Qt] from the summary.

If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines.