Bug 40797 – [Qt] Decode images directly to QPixmap

• Status: RESOLVED
• Resolution: FIXED
• Component: Images

[Benjamin Poulain]

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.

[Kenneth Rohde Christiansen]

That would be a really nice change.

[Benjamin Poulain]

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.

[WebKit Review Bot]

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.

[Early Warning System Bot]

Attachment 59029 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3321328

[Benjamin Poulain]

Created attachment 59030 [details]
Patch

Fix the style.

[Early Warning System Bot]

Attachment 59030 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3302306

[Benjamin Poulain]

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

[Kenneth Rohde Christiansen]

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?

[Benjamin Poulain]

Created attachment 59033 [details]
Patch

Add the case "imageCount == 0" thanks to Kenneth's comments on IRC.

[WebKit Review Bot]

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.

[Early Warning System Bot]

Attachment 59033 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3318304

[Kenneth Rohde Christiansen]

The Qt patch will be applied to Qt 4.6 as well? if not we need an ifdef for checking the Qt version.

[Kenneth Rohde Christiansen]

Comment on attachment 59033 [details]
Patch

You can re-set the cq when the patches are in Qt.

[Eric Seidel (no email)]

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.

[Benjamin Poulain]

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

[WebKit Commit Bot]

Comment on attachment 59033 [details]
Patch

Clearing flags on attachment: 59033

Committed r61534: <http://trac.webkit.org/changeset/61534>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Balazs Kelemen]

There is a useless conversion between QImage and QPixmap in RGBA32Buffer::zeroFill(). See bug 59242.

[Balazs Kelemen]

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.

[Simon Hausmann]

Revision r61534 cherry-picked into qtwebkit-2.0 with commit 7a3bddf10bee16ee9ff176d0e99d100afe8a0782

[Benjamin Poulain]

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.

[Kenneth Rohde Christiansen]

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.

[Benjamin Poulain]

Comment on attachment 60555 [details]
Update to take advantage of new APIs in Qt

Yup, it landed in 4.7 recently: c16162214eb8757a62e221c34d38cefc402e5b05

[Simon Hausmann]

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

[Benjamin Poulain]

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.

[Simon Hausmann]

Comment on attachment 60606 [details]
Update to take advantage of new APIs in Qt + null pixmap

r=me, this version is much nicer :)

[Simon Hausmann]

Committed r65227: <http://trac.webkit.org/changeset/65227>

[Simon Hausmann]

Revision r65227 cherry-picked into qtwebkit-2.1 with commit f1c7842276d4ca19ad19083b8182b2d1351ebc4f