Bug 35153 – [Qt] WebGL support missing in QtWebKit

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Jarkko Sakkinen]

WebGL support for QtWebKit is missing.

[Jarkko Sakkinen]

http://bugreports.qt.nokia.com/browse/QTWEBKIT-18

[Jarkko Sakkinen]

Created attachment 49078 [details]
WebGL support for QtWebKit

NOTES:

    * Works only when accelerated composition is not enabled in compilation
    * Added --webgl command line switch to QGVLauncher, added toggle button to QtLauncher

Why GraphicsLayer (accelerated composition layer) doesn't handle WebGL? Missing methods:

    * setContentsToGraphicsContext3D
    * setGraphicsContext3DNeedsDisplay

I thinks adding support for content caching is subtask for this. WebGL support can be tested by compiling with  WTF_USE_ACCELERATED_COMPOSITING=0.

[WebKit Review Bot]

Attachment 49078 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
Last 3072 characters of output:
 Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:237:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:238:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:239:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:240:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:241:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:242:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:243:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:244:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:245:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:246:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:247:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:248:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:249:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:251:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:252:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:253:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:254:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:255:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:256:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:257:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:258:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:259:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:460:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:606:  Extra space after ( in function call  [whitespace/parens] [4]
WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:1585:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Ignoring "WebKit/qt/Api/qwebsettings.h": this file is exempt from the style guide.
Total errors found: 82 in 8 files


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

[Jarkko Sakkinen]

Created subtask for accelerated compositioning changes: http://bugreports.qt.nokia.com/browse/QTWEBKIT-91

[Jarkko Sakkinen]

Created attachment 49080 [details]
Qt WebGL support

Coding style fixes.

[Noam Rosenthal]

I see that when converting from WebCore::Image* we use QPixmap::toImage(). If we're in the OpenGL graphicssystem, that would be an unnecessary bottleneck. Anyway we could use the standard QPixmap->texture conversions from QtOpenGL?

[Jarkko Sakkinen]

You mean texImage2D and texSubImage2D methods that take Image* parameter? I would actually go even more conservative for this initial patch on Qt WebGL support because those two functions are not usuall that frequently called in common OpenGL code. See this changeset in gitorious:

http://gitorious.org/webkit/jarkkos-webkit/commit/521a68ff041db6e72d52439ed9fd02481eafb672

What do you think? 

After this patch has been accepted with possible corrections I think it might be good idea to keep this implementation fairly conservative until possible issues on different targets have been fixed.

[Jarkko Sakkinen]

I'm working on Fremantle fixes (see http://gitorious.org/~sakkinen/webkit/jarkkos-webkit). Review of this can wait until I get that escher demo succesfully running on N900 :)

[Noam Rosenthal]

Sure - I'm fine with keeping it traditional. Maybe some comments along the line of "FIXME: This can be optimized when using the OpenGL paint-engine" etc.
Same goes for beginPaint, by the way - if we're on a QGLWidget / opengl graphicssystem, we don't want to go through a QImage - but a FIXME comment at this stage is enough I think.

btw I'm not a reviewer - just commenting :)

[Ariya Hidayat]

> I'm working on Fremantle fixes (see
> http://gitorious.org/~sakkinen/webkit/jarkkos-webkit). Review of this can wait
> until I get that escher demo succesfully running on N900 :)

In this case, it would be wise to clear the review flag until you want it to be reviewed again.

[Jarkko Sakkinen]

Git version now runs most of the WebGL tests on N900 but they need default precision to be appended (like precision mediump float;). Should there be default precision appended to the shader code if it isn't defined? On desktop, precision stuff must be of course parsed out before compiling the shader. This is also said in http://www.khronos.org/webgl/wiki/Testing/Conformance but it doesn't say anything what to do if precision isn't defined (like it isn't in most of the webgl examples).

[Jarkko Sakkinen]

Created attachment 49277 [details]
Qt WebGL support

Now works tested also on N900 and fixed issues on that platform.

[WebKit Review Bot]

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

[Jarkko Sakkinen]

Created attachment 49279 [details]
Qt WebGL support

[Kenneth Rohde Christiansen]

Comment on attachment 49279 [details]
Qt WebGL support


> +
> +typedef void (APIENTRY *glActiveTextureType) (GLenum);
> +typedef void (APIENTRY *glAttachShaderType) (GLuint, GLuint);
> +typedef void (APIENTRY *glBindAttribLocationType) (GLuint, GLuint, const char *);
> +typedef void (APIENTRY *glBindBufferType) (GLenum, GLuint);

Lost of these have wrong coding style, like the * should be left aligned, thus:

+typedef void (APIENTRY* glBindAttribLocationType) (GLuint, GLuint, const char*)

We are pretty strict about coding style in WebKit, thus r-. Please make use of the tool check-webkit-style which can be found in WebKitTools/Scripts

[Jarkko Sakkinen]

Forgot to run prepage-ChangeLog script :(  Sorry!

[Jarkko Sakkinen]

Created attachment 49284 [details]
Qt WebGL support

Forgot to add ChangeLog entry to previous patches, sorry!

[Jarkko Sakkinen]

Note: apply change below if you want to try it out. See discussion in http://bugreports.qt.nokia.com/browse/QTWEBKIT-91 on accelerated compositing

##############################################################################
diff --git a/WebKit.pri b/WebKit.pri
index 907a92c..837ce67 100644
--- a/WebKit.pri
+++ b/WebKit.pri
@@ -49,7 +49,7 @@ building-libs {
     }
     DEPENDPATH += $$PWD/WebKit/qt/Api
 }
-greaterThan(QT_MINOR_VERSION, 5):DEFINES += WTF_USE_ACCELERATED_COMPOSITING
+#greaterThan(QT_MINOR_VERSION, 5):DEFINES += WTF_USE_ACCELERATED_COMPOSITING
 
 !mac:!unix|symbian {
     DEFINES += USE_SYSTEM_MALLOC
##############################################################################

[Jarkko Sakkinen]

Comment on attachment 49284 [details]
Qt WebGL support

going to fix coding style issues.

[Jarkko Sakkinen]

Created attachment 49297 [details]
Qt WebGL support

- Fixed reported style issue.
- Went through code manually to find more style issues (by looking at the style guide same time)
- Run check-webkit-style script

[Jarkko Sakkinen]

Created attachment 49302 [details]
Qt WebGL support

Fixed whitespace style issues in ChangeLog files.

[Kenneth Rohde Christiansen]

Comment on attachment 49302 [details]
Qt WebGL support

I had a discussion with Jarkko on IRC and he is a.o updating the patch to make
use of a template for all calls similar to: 

#ifdef ...
enableVertexAttribArray =
reinterpret_cast<glEnableVertexAttribArrayType>(getProcAddress(ctx,
"glEnableVertexAttribArray"));
#else
enableVertexAttribArray = glEnableVertexAttribArray
#endif

that will simplify the code somewhat, as well as fixing coding styles issues
and the like.

[Jarkko Sakkinen]

Created attachment 49372 [details]
Qt WebGL support

Style fixed

[Kenneth Rohde Christiansen]

Comment on attachment 49372 [details]
Qt WebGL support

Please remove the changes to QGVLauncher as it is being removed. Also generally you should use webcore types inside webcore. 

I would also like you to use webcore logging instead of qWarning.

If you intend to land this through the commit queue, you need to remove the 'No new tests. (OOPS!)' from the ChangeLog.

[Jarkko Sakkinen]

Created attachment 49381 [details]
Qt WebGL support

[Kenneth Rohde Christiansen]

Comment on attachment 49381 [details]
Qt WebGL support

Looks good! some final comments on IRC that you can fix if you would like.

[Jarkko Sakkinen]

Created attachment 49383 [details]
Qt WebGL support

Final polishing of comments etc style :)

[Jarkko Sakkinen]

Created attachment 49475 [details]
Qt WebGL support

- Fixed compiler warning: forgot to remove now unused local variable ctx
- Fixed compiler error on desktop compilation: GraphicsContext3D::DEPTH_COMPONENT24 -> GraphicsContext3D::DEPTH_COMPONENT

NOTE: If WTF_USE_ACCELERATED_COMPOSITING is disabled the current qtwebkit master does not compile. Resolved this by following change (not included in this patch since this is not part of this):

diff --git a/WebCore/css/MediaQueryEvaluator.cpp b/WebCore/css/MediaQueryEvaluator.cpp
index 4963ed4..6864231 100644
--- a/WebCore/css/MediaQueryEvaluator.cpp
+++ b/WebCore/css/MediaQueryEvaluator.cpp
@@ -47,7 +47,7 @@
 #include "PlatformScreen.h"
 #include <wtf/HashMap.h>
 
-#if ENABLE(3D_RENDERING)
+#if ENABLE(3D_RENDERING) && USE(ACCELERATED_COMPOSITING)
 #include "RenderLayerCompositor.h"
 #endif
 
@@ -472,10 +472,12 @@ static bool transform_3dMediaFeatureEval(CSSValue* value, RenderStyle*, Frame* f
     bool returnValueIfNoParameter;
     int have3dRendering;
 
-#if ENABLE(3D_RENDERING)
+#if ENABLE(3D_RENDERING) 
     bool threeDEnabled = false;
+#if USE(ACCELERATED_COMPOSITING)
     if (RenderView* view = frame->contentRenderer())
         threeDEnabled = view->compositor()->hasAcceleratedCompositing();
+#endif
 
     returnValueIfNoParameter = threeDEnabled;
     have3dRendering = threeDEnabled ? 1 : 0;

[Jarkko Sakkinen]

Changed test page to http://learningwebgl.com/lessons/lesson15/index.html
instead of http://wakaba.c3.cx/w/escher_droste.html because khronos specification says that only GLSL 1.0 should be supported [1].

[1] https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/doc/spec/WebGL-spec.html#4.4

[WebKit Commit Bot]

Comment on attachment 49475 [details]
Qt WebGL support

Clearing flags on attachment: 49475

Committed r55235: <http://trac.webkit.org/changeset/55235>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Kent Hansen]

(In reply to comment #28)
> Created an attachment (id=49475) [details]
> Qt WebGL support

Hi,
This change adds public API, but lacks documentation for QWebSettings::WebGLEnabled; it should have an entry under "\enum QWebSettings::WebAttribute" in qwebsettings.cpp with a description of what it does and what the default value is. Could you add it?
Thanks!