Bug 60897 - [Qt] Implement toImageData() in QtWebKit Bridge
Summary: [Qt] Implement toImageData() in QtWebKit Bridge
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Enhancement
Assignee: Nobody
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks:
 
Reported: 2011-05-16 09:46 PDT by Andrew Wason
Modified: 2011-05-22 09:57 PDT (History)
4 users (show)

See Also:


Attachments
implement toImageData() in Qt bridge (9.55 KB, patch)
2011-05-16 09:52 PDT, Andrew Wason
no flags Details | Formatted Diff | Diff
implement toImageData() in Qt bridge (9.43 KB, patch)
2011-05-16 10:11 PDT, Andrew Wason
benjamin: review-
Details | Formatted Diff | Diff
QImage conversion performance testing (3.55 KB, text/plain)
2011-05-18 11:38 PDT, Andrew Wason
no flags Details
implement toImageData() in Qt bridge (9.18 KB, patch)
2011-05-19 11:02 PDT, Andrew Wason
benjamin: review-
Details | Formatted Diff | Diff
implement toImageData() in Qt bridge (11.23 KB, patch)
2011-05-21 17:29 PDT, Andrew Wason
benjamin: review+
benjamin: commit-queue-
Details | Formatted Diff | Diff
implement toImageData() in Qt bridge (11.27 KB, patch)
2011-05-22 07:57 PDT, Andrew Wason
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Wason 2011-05-16 09:46:26 PDT
I think it would be useful to have a toImageData() method on the Qt bridge image proxy object, in addition to the existing assignToHTMLImageElement(). This would also allow applications to work around bug #60770 and bug #59972 by using an ImageData with Canvas putImageData() or gl.texImage2D()

I will attach an implementation shortly.
Comment 1 Andrew Wason 2011-05-16 09:52:11 PDT
Created attachment 93655 [details]
implement toImageData() in Qt bridge
Comment 2 WebKit Review Bot 2011-05-16 09:53:18 PDT
Attachment 93655 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/bridge/qt/qt_pixmapruntime.cpp:103:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Andrew Wason 2011-05-16 10:11:38 PDT
Created attachment 93660 [details]
implement toImageData() in Qt bridge
Comment 4 Benjamin Poulain 2011-05-18 04:16:23 PDT
Comment on attachment 93660 [details]
implement toImageData() in Qt bridge 

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

> Source/WebCore/bridge/qt/qt_pixmapruntime.cpp:100
> +    void copyPixels(QImage image, int width, int height, unsigned char* destPixels)

This code looks very inefective.

What about using QImage::convertToFormat() to always convert to QImage::Format_ARGB32, and then do the byte shifting? Qt already has optimized color conversions fonctions for plenty of formats.
Comment 5 Benjamin Poulain 2011-05-18 04:20:50 PDT
I like the idea of this feature, I think ImageData is pretty useful.

No'am is rewriting the bridge at the moment (see the mailing list). Please sync with him before updating the patch.

I don't like color conversion done like that in WebKit when you can do so much better with SIMD inside Qt.

Please profile the conversion. Functions like QImage::pixel() are not an option when we have to act on every pixel of the image.
Comment 6 Andrew Wason 2011-05-18 11:38:36 PDT
Created attachment 93954 [details]
QImage conversion performance testing

(In reply to comment #4)
> 
> What about using QImage::convertToFormat() to always convert to QImage::Format_ARGB32,
> and then do the byte shifting? Qt already has optimized color conversions fonctions
> for plenty of formats.

I had considered that (and also looked at QGLWidget::convertToGLFormat() which converts to RGBA), but was trying to avoid a full intermediate copy of the image bits by converting them directly into the ImageData ByteArray. My use case is thousands of relatively large images (1920x1080).

I tried to find a way to use the internal optimized image conversion routines, but they're all private.

You are right QImage::pixel() is a performance killer though. I'm attaching a simple app to compare the performance for each format of using convertToFormat() vs. directly copying/converting. For the formats I'm handling directly, convertToFormat() is about 1.5X slower. For the formats where I fall back to QImage::pixel(), convertToFormat() is about 2-3X faster. Here are the results when I run it:

$ ./imageconvert 
format 1 convertDirect=13432740 convertWithCopy=4496240 direct/copy=2.987550
format 2 convertDirect=13330259 convertWithCopy=4649569 direct/copy=2.866988
format 3 convertDirect=12529513 convertWithCopy=4600696 direct/copy=2.723395
format 4 convertDirect=2837000 convertWithCopy=4375153 direct/copy=0.648434
format 5 convertDirect=2891826 convertWithCopy=4293214 direct/copy=0.673581
format 6 convertDirect=12443809 convertWithCopy=12101966 direct/copy=1.028247
format 7 convertDirect=15110204 convertWithCopy=5870940 direct/copy=2.573728
format 8 convertDirect=19884922 convertWithCopy=9334570 direct/copy=2.130245
format 9 convertDirect=17433956 convertWithCopy=7134872 direct/copy=2.443485
format 10 convertDirect=18181269 convertWithCopy=8235076 direct/copy=2.207784
format 11 convertDirect=15399767 convertWithCopy=6015450 direct/copy=2.560036
format 12 convertDirect=16741198 convertWithCopy=7187635 direct/copy=2.329166
format 13 convertDirect=1690568 convertWithCopy=4665468 direct/copy=0.362358
format 14 convertDirect=14963419 convertWithCopy=5687373 direct/copy=2.630990
format 15 convertDirect=16249964 convertWithCopy=6523285 direct/copy=2.491071

So perhaps copyPixels() could continue to handle Format_RGB32, Format_ARGB32, Format_RGB888 directly, and for the fallback case use convertToFormat() instead of QImage::pixel(). i.e. avoid the intermediate copy for these 3 formats which are pretty simple to convert to RGBA instead of always converting to Format_ARGB32 first.
Comment 7 Benjamin Poulain 2011-05-18 13:02:39 PDT
Results on ARMv7: 
format 1 convertDirect=82860000 convertWithCopy=42430000 direct/copy=1.952864
format 2 convertDirect=82870000 convertWithCopy=42170000 direct/copy=1.965141
format 3 convertDirect=79000000 convertWithCopy=39900000 direct/copy=1.979950
format 4 convertDirect=12770000 convertWithCopy=34490000 direct/copy=0.370252
format 5 convertDirect=12770000 convertWithCopy=32280000 direct/copy=0.395601
format 6 convertDirect=75750000 convertWithCopy=39210000 direct/copy=1.931905
format 7 convertDirect=85550000 convertWithCopy=40820000 direct/copy=2.095786
format 8 convertDirect=95210000 convertWithCopy=47490000 direct/copy=2.004843
format 9 convertDirect=88930000 convertWithCopy=65640000 direct/copy=1.354814
format 10 convertDirect=89970000 convertWithCopy=44380000 direct/copy=2.027265
format 11 convertDirect=89660000 convertWithCopy=42900000 direct/copy=2.089977
format 12 convertDirect=89980000 convertWithCopy=43230000 direct/copy=2.081425
format 13 convertDirect=8290000 convertWithCopy=32760000 direct/copy=0.253053
format 14 convertDirect=88630000 convertWithCopy=41860000 direct/copy=2.117296
format 15 convertDirect=89670000 convertWithCopy=42890000 direct/copy=2.090697
Comment 8 Andrew Wason 2011-05-19 11:02:48 PDT
Created attachment 94090 [details]
implement toImageData() in Qt bridge

(In reply to comment #5)
> No'am is rewriting the bridge at the moment (see the mailing list).
> Please sync with him before updating the patch.

He said to go ahead with the patch and he will merge later.

> I don't like color conversion done like that in WebKit when you can do so much better
> with SIMD inside Qt.

Patch is updated to do a direct copy for RGB888, ARGB32 and RGB32 and for all other cases convertToFormat(ARGB32) and then copy from that.
Comment 9 Benjamin Poulain 2011-05-21 14:58:40 PDT
Comment on attachment 94090 [details]
implement toImageData() in Qt bridge

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

I was gonna r+, cq-, but I just noticed the doc is missing.
Please update the documentation since this is a public API.

> Source/WebCore/bridge/qt/qt_pixmapruntime.cpp:28
> +#include "JSImageData.h"

We need that one?

> Source/WebKit/qt/tests/hybridPixmap/test.html:23
> +                        myWidget.compare(data[i  ], 0xaa); // R

No space after the i, No need to align the columns in the source.

> Source/WebKit/qt/tests/hybridPixmap/widget.h:48
> +    Q_INVOKABLE QImage abcImage(int format);

Why not put this as a public slot like the others?
Comment 10 Andrew Wason 2011-05-21 17:29:31 PDT
Created attachment 94330 [details]
implement toImageData() in Qt bridge 

(In reply to comment #9)
> (From update of attachment 94090 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=94090&action=review
> 
> I was gonna r+, cq-, but I just noticed the doc is missing.
> Please update the documentation since this is a public API.

Added docs.

> > Source/WebCore/bridge/qt/qt_pixmapruntime.cpp:28
> > +#include "JSImageData.h"
> 
> We need that one?

It's needed for the declaration of WebCore::toJS(..., WebCore::ImageData*)

> 
> > Source/WebKit/qt/tests/hybridPixmap/test.html:23
> > +                        myWidget.compare(data[i  ], 0xaa); // R
> 
> No space after the i, No need to align the columns in the source.
> 
> > Source/WebKit/qt/tests/hybridPixmap/widget.h:48
> > +    Q_INVOKABLE QImage abcImage(int format);
> 
> Why not put this as a public slot like the others?

OK, fixed both those.
Comment 11 Benjamin Poulain 2011-05-22 07:29:11 PDT
Comment on attachment 94330 [details]
implement toImageData() in Qt bridge 

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

> Source/WebCore/bridge/qt/qt_pixmapruntime.cpp:100
> +    void copyPixels(QImage image, int width, int height, unsigned char* destPixels)

const QImage &image.
By convention, we pass non-basic types as const reference.
Comment 12 Andrew Wason 2011-05-22 07:57:57 PDT
Created attachment 94342 [details]
implement toImageData() in Qt bridge 

(In reply to comment #11)
> 
> const QImage &image.
> By convention, we pass non-basic types as const reference.

Changed this.
Comment 13 Benjamin Poulain 2011-05-22 08:34:00 PDT
Comment on attachment 94342 [details]
implement toImageData() in Qt bridge 

Let's go. Hopefully No'am will not kill me for breaking his refactoring :)
Comment 14 WebKit Commit Bot 2011-05-22 09:57:08 PDT
Comment on attachment 94342 [details]
implement toImageData() in Qt bridge 

Clearing flags on attachment: 94342

Committed r87032: <http://trac.webkit.org/changeset/87032>
Comment 15 WebKit Commit Bot 2011-05-22 09:57:14 PDT
All reviewed patches have been landed.  Closing bug.