Bug 40884

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.

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.

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

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

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

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?

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

Reopen by Jarkko's comments on 56555.

Created attachment 86788 [details] Patch

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.

Created attachment 86797 [details] Patch Patches are better when I don't mess up the style :) One error expected, the usual one for QtTest.

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.

This looks good and those tests are very much needed!

Created attachment 86801 [details] Patch Ooops, one of the test was saving the ref image.

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.

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 {}

The commit-queue encountered the following flaky tests while processing attachment 86801 [details]: http/tests/appcache/fail-on-update.html bug 57043 (author: ap@webkit.org) The commit-queue is continuing to process your patch.

Comment on attachment 86801 [details] Patch Clearing flags on attachment: 86801 Committed r81886: <http://trac.webkit.org/changeset/81886>

All reviewed patches have been landed. Closing bug.