Bug 40884 – [Qt] When we render WebGL offscreen, color conversion cost a lot of CPU cycles

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Benjamin Poulain]

When rendering WebGL offscreen (if we don't have a GL surface to draw on), we render copy the gl buffer onto QImage and render the QImage.

This operation is CPU intensive. The situation is made a bit worse because of the color conversions and the image transformations.

In particular: 
-we always render to ARGB32, forcing the costly conversion to ARGB32_PM to render
-we copy the image to convert the colors: m_pixels.rgbSwapped()
-we copy one more time to mirror the image: .transformed(QMatrix().rotate(180)

To render, we end up with 3 copies of the pixels, and we have a poor use of the CPU cache.

We could convert the colors and mirror the pixels in WebKit, doing it in a single buffer, and doing the two operations together to optimize the cache.

[Benjamin Poulain]

Created attachment 59197 [details]
Patch

Try to address the issues.

On the website http://www.plsw.net/gnomwgl.htm the rendering goes from 13 fps to 22 fps with the patch.

[Andreas Kling]

Comment on attachment 59197 [details]
Patch

WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:299
 +  
Unnecessary empty line.

WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:579
 +      // GL give us ABGR on 32 bits, and with the origin at the bottom left
s/give/gives/

WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:465
 +      free(m_pixels);
WebKit normally uses new/delete, is there a specific reason for malloc/free here?

WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:540
 +  static inline quint32 swapPixelsRGB32(quint32 pixel)
I don't like this function name - it's swapping color values, not pixels.

WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:586
 +          format = QImage::Format_ARGB32_Premultiplied;
 +          format = QImage::Format_RGB32;
You're setting format twice here. It ends up being RGB32 which is incorrect for this code path.

Looks good otherwise, r- for the wrong format in the alpha channel case.

[Jarkko Sakkinen]

I think this would be important addition to the GraphicsContext3D implementation in order to get reasonable performance when raw texture mapping cannot be used. Shouldn't be hard to get this integrated. With current implementation code just needs to be moved to GraphicsContext3DImpl::paint().

[Benjamin Poulain]

(In reply to comment #3)
> I think this would be important addition to the GraphicsContext3D implementation in order to get reasonable performance when raw texture mapping cannot be used. Shouldn't be hard to get this integrated. With current implementation code just needs to be moved to GraphicsContext3DImpl::paint().

Simon was of the opinion that we should not support software fallback at all since it is not possible to get descent performance with them.

I tend to agree, that is why I have not updated the patch.

I close this as won't fix. Please comment if you have use case where software fallback are desirable.

[Jarkko Sakkinen]

I don't have an use case for this. Just thinking that it gives more compatibility even if performance is not optimal. I think it would make the overall behaviour also more stable to a developer that uses QtWebKit. Documentation should document settings for optimal performance.

See for example this bug that I'm fixing:
https://bugs.webkit.org/show_bug.cgi?id=56555

Kind of rare use case in real-world, though.

Now, for example, fallback is needed for this use case:
http://developer.qt.nokia.com/forums/viewthread/4505/

Only when QGLWidget is used as the viewport GraphicsContext3D content can be texture mapped.

Should I make a patch for GraphicsContext3D that creation fails if accelerated compositing is not enabled from the settings or QGLWidget viewport does not exist? I can fix the bug 56555 by invalidating GraphicsContext3D if viewport changes after creation if this is the case.

I think this is anyway your (QtWebKit team) call and I don't really have too strong opinions on this :)

[Jarkko Sakkinen]

I actually think that in order to get the first release out with WebGL support the approach that support it with acccelerated compositing really is the right choice because it is always easier to expand support (adding fallback) than degrade it. So I propose that I do the check in constructor of GraphicsContext3D and invalidate context if accelerated compositing is not enabled. I think this fits well in the patch for 56555. What do you think?

[Benjamin Poulain]

(In reply to comment #6)
> I actually think that in order to get the first release out with WebGL support the approach that support it with acccelerated compositing really is the right choice because it is always easier to expand support (adding fallback) than degrade it. So I propose that I do the check in constructor of GraphicsContext3D and invalidate context if accelerated compositing is not enabled. I think this fits well in the patch for 56555. What do you think?

Yep, sounds reasonable to me.

And you have a good point, more support can be added later if necessary. Removing feature would be a lot harder.

[Benjamin Poulain]

Reopen by Jarkko's comments on 56555.

[Benjamin Poulain]

Created attachment 86788 [details]
Patch

[WebKit Review Bot]

Attachment 86788 [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/WebKit/qt/tests/benchmarks/webgl/tst_webgl.cpp:20:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/WebKit/qt/tests/benchmarks/webgl/tst_webgl.cpp:22:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit/qt/tests/benchmarks/webgl/tst_webgl.cpp:44:  The parameter name "format" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/qt/tests/benchmarks/webgl/tst_webgl.cpp:113:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/WebKit/qt/tests/qgraphicswebview/tst_qgraphicswebview.cpp:466:  Missing spaces around =  [whitespace/operators] [4]
Total errors found: 5 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Benjamin Poulain]

Created attachment 86797 [details]
Patch

Patches are better when I don't mess up the style :)
One error expected, the usual one for QtTest.

[WebKit Review Bot]

Attachment 86797 [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/WebKit/qt/tests/benchmarks/webgl/tst_webgl.cpp:21:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jarkko Sakkinen]

This looks good and those tests are very much needed!

[Benjamin Poulain]

Created attachment 86801 [details]
Patch

Ooops, one of the test was saving the ref image.

[WebKit Review Bot]

Attachment 86801 [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/WebKit/qt/tests/benchmarks/webgl/tst_webgl.cpp:21:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kenneth Rohde Christiansen]

Comment on attachment 86801 [details]
Patch

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

Pretty nice testing!

> Source/WebKit/qt/tests/benchmarks/webgl/tst_webgl.cpp:31
> +class tst_WebGlPerformance : public QObject {

We normally use GL as uppercase (ie QGLWidget)

> Source/WebKit/qt/tests/qgraphicswebview/tst_qgraphicswebview.cpp:475
> +        for (int column = 0; column < image1.size().height(); ++column)
> +            if (image1.pixel(row, column) != image2.pixel(row, column))
> +                ++diffPixelCount;

That for should really use {}

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 86801 [details]:

http/tests/appcache/fail-on-update.html bug 57043 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 86801 [details]
Patch

Clearing flags on attachment: 86801

Committed r81886: <http://trac.webkit.org/changeset/81886>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.