Bug 40797 - [Qt] Decode images directly to QPixmap
Summary: [Qt] Decode images directly to QPixmap
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Benjamin Poulain
URL:
Keywords: Performance, Qt
Depends on: 40620
Blocks:
  Show dependency treegraph
 
Reported: 2010-06-17 12:40 PDT by Benjamin Poulain
Modified: 2011-04-19 05:15 PDT (History)
7 users (show)

See Also:


Attachments
Patch (6.63 KB, patch)
2010-06-17 13:01 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (6.63 KB, patch)
2010-06-17 13:12 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (6.68 KB, patch)
2010-06-17 14:24 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Update to take advantage of new APIs in Qt (2.60 KB, patch)
2010-07-05 10:59 PDT, Benjamin Poulain
kenneth: review+
benjamin: commit-queue+
Details | Formatted Diff | Diff
Update to take advantage of new APIs in Qt + null pixmap (2.49 KB, patch)
2010-07-06 02:28 PDT, Benjamin Poulain
hausmann: review+
hausmann: commit-queue+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Poulain 2010-06-17 12:40:13 PDT
WebKit could decode images directly to QPixmap instead of converting from QImage to QPixmap. This has the advantage of reducing memory consumption, because the images do not have to be twice in memory during the conversion.

Concretely, if a decoded image takes 10 mbytes of memory, converting it will currently use 20 mbytes of memory. Using pixmap directy would leave us a 10 mbytes of memory consumption.
Comment 1 Kenneth Rohde Christiansen 2010-06-17 12:47:20 PDT
That would be a really nice change.
Comment 2 Benjamin Poulain 2010-06-17 13:01:23 PDT
Created attachment 59029 [details]
Patch

This patch require changes in Qt to reduce the memory usage.

The change also has a side effect if the graphic system is OpenGL. With the patch, a compressed texture can be loaded as an image in WebKit.
Comment 3 WebKit Review Bot 2010-06-17 13:07:48 PDT
Attachment 59029 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/platform/graphics/qt/ImageDecoderQt.cpp:192:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Early Warning System Bot 2010-06-17 13:08:07 PDT
Attachment 59029 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3321328
Comment 5 Benjamin Poulain 2010-06-17 13:12:44 PDT
Created attachment 59030 [details]
Patch

Fix the style.
Comment 6 Early Warning System Bot 2010-06-17 13:19:26 PDT
Attachment 59030 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3302306
Comment 7 Benjamin Poulain 2010-06-17 13:21:47 PDT
(In reply to comment #6)
> Attachment 59030 [details] did not build on qt:
> Build output: http://webkit-commit-queue.appspot.com/results/3302306

To the reviewer, it does not build because 40620 has not landed yet. If it is still not landed tomorrow, I'll do it manually.
Comment 8 Kenneth Rohde Christiansen 2010-06-17 13:57:43 PDT
Comment on attachment 59030 [details]
Patch

LGTM, cq- as the patch in Qt is not available on gitorious yet and waiting for final review.

A few questions:

 192     else {
 193         QImage img;
 194         const bool imageLoaded = m_reader->read(&img);
 195         if (imageLoaded) {
 196             pixmap = QPixmap::fromImage(img);
 197             pixmapLoaded = true;
 198         }
 199     }

So we do not support animations? gif etc? Can imageCount() be 0?
Comment 9 Benjamin Poulain 2010-06-17 14:24:40 PDT
Created attachment 59033 [details]
Patch

Add the case "imageCount == 0" thanks to Kenneth's comments on IRC.
Comment 10 WebKit Review Bot 2010-06-17 14:27:12 PDT
Attachment 59033 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/platform/graphics/qt/ImageDecoderQt.cpp:191:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Early Warning System Bot 2010-06-17 14:30:58 PDT
Attachment 59033 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3318304
Comment 12 Kenneth Rohde Christiansen 2010-06-17 15:26:54 PDT
The Qt patch will be applied to Qt 4.6 as well? if not we need an ifdef for checking the Qt version.
Comment 13 Kenneth Rohde Christiansen 2010-06-17 15:27:47 PDT
Comment on attachment 59033 [details]
Patch

You can re-set the cq when the patches are in Qt.
Comment 14 Eric Seidel 2010-06-17 15:29:25 PDT
Comment on attachment 59030 [details]
Patch

Cleared Kenneth Rohde Christiansen's review+ from obsolete attachment 59030 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 15 Benjamin Poulain 2010-06-17 17:31:59 PDT
(In reply to comment #12)
> The Qt patch will be applied to Qt 4.6 as well? if not we need an ifdef for checking the Qt version.

It will not be applied to 4.6, but that should not be a problem. The previous implementation of QPixmap::loadFromData() uses ImageReader, exactly what we do in ImageReaderQt.
Comment 16 WebKit Commit Bot 2010-06-21 01:30:52 PDT
Comment on attachment 59033 [details]
Patch

Clearing flags on attachment: 59033

Committed r61534: <http://trac.webkit.org/changeset/61534>
Comment 17 WebKit Commit Bot 2010-06-21 01:30:57 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Balazs Kelemen 2010-06-21 05:22:28 PDT
There is a useless conversion between QImage and QPixmap in RGBA32Buffer::zeroFill(). See bug 59242.
Comment 19 Balazs Kelemen 2010-06-21 05:25:19 PDT
Oh, no! It is the attachment, the bug is 40910 :S

(In reply to comment #18)
> There is a useless conversion between QImage and QPixmap in RGBA32Buffer::zeroFill(). See bug 59242.
Comment 20 Simon Hausmann 2010-06-21 06:01:47 PDT
Revision r61534 cherry-picked into qtwebkit-2.0 with commit 7a3bddf10bee16ee9ff176d0e99d100afe8a0782
Comment 21 Benjamin Poulain 2010-07-05 10:59:17 PDT
Created attachment 60555 [details]
Update to take advantage of new APIs in Qt

We added new API to Qt 4.7 in order to do in-place color conversion in the all cases. This patch update WebKit to take advantages of this new API.
Comment 22 Kenneth Rohde Christiansen 2010-07-05 11:04:34 PDT
Comment on attachment 60555 [details]
Update to take advantage of new APIs in Qt

r=me, you can set qc+ if this is already in public Qt repo.
Comment 23 Benjamin Poulain 2010-07-05 11:30:27 PDT
Comment on attachment 60555 [details]
Update to take advantage of new APIs in Qt

Yup, it landed in 4.7 recently: c16162214eb8757a62e221c34d38cefc402e5b05
Comment 24 Simon Hausmann 2010-07-06 00:39:43 PDT
Comment on attachment 60555 [details]
Update to take advantage of new APIs in Qt

WebCore/platform/graphics/qt/ImageDecoderQt.cpp:200
 +          pixmap = QPixmap::fromImage(img);
I believe this function can return a null pixmap if we run out of memory (I've seen this actually on Symbian).
Comment 25 Benjamin Poulain 2010-07-06 02:28:27 PDT
Created attachment 60606 [details]
Update to take advantage of new APIs in Qt + null pixmap

> (From update of attachment 60555 [details])
> WebCore/platform/graphics/qt/ImageDecoderQt.cpp:200
>  +          pixmap = QPixmap::fromImage(img);
> I believe this function can return a null pixmap if we run out of memory (I've seen this actually on Symbian).

Yep you are right, the pixmap could be null after this call.
I updated the patch to simply abort as soon as we get a null pixmap.
Comment 26 Simon Hausmann 2010-07-06 03:13:40 PDT
Comment on attachment 60606 [details]
Update to take advantage of new APIs in Qt + null pixmap

r=me, this version is much nicer :)
Comment 27 Simon Hausmann 2010-08-12 02:44:38 PDT
Committed r65227: <http://trac.webkit.org/changeset/65227>
Comment 28 Simon Hausmann 2010-08-12 05:48:02 PDT
Revision r65227 cherry-picked into qtwebkit-2.1 with commit f1c7842276d4ca19ad19083b8182b2d1351ebc4f