Summary: | [Qt] WebGL support missing in QtWebKit | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jarkko Sakkinen <jarkko.sakkinen> | ||||||||||||||||||||||||
Component: | WebKit Qt | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||
Severity: | Enhancement | CC: | ariya.hidayat, benjamin, commit-queue, henry.haverinen, jarkko.sakkinen, kent.hansen, laszlo.gombos, noam, skyul, webkit.review.bot | ||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||
Hardware: | PC | ||||||||||||||||||||||||||
OS: | Linux | ||||||||||||||||||||||||||
URL: | http://learningwebgl.com/lessons/lesson15/index.html | ||||||||||||||||||||||||||
Attachments: |
|
Description
Jarkko Sakkinen
2010-02-19 06:09:14 PST
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.
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.
Created subtask for accelerated compositioning changes: http://bugreports.qt.nokia.com/browse/QTWEBKIT-91 Created attachment 49080 [details]
Qt WebGL support
Coding style fixes.
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? 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. 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 :) 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 :) > 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.
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). Created attachment 49277 [details]
Qt WebGL support
Now works tested also on N900 and fixed issues on that platform.
Attachment 49277 [details] did not build on qt: Build output: http://webkit-commit-queue.appspot.com/results/300462 Created attachment 49279 [details]
Qt WebGL support
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 Forgot to run prepage-ChangeLog script :( Sorry! Created attachment 49284 [details]
Qt WebGL support
Forgot to add ChangeLog entry to previous patches, sorry!
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 ############################################################################## Comment on attachment 49284 [details]
Qt WebGL support
going to fix coding style issues.
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
Created attachment 49302 [details]
Qt WebGL support
Fixed whitespace style issues in ChangeLog files.
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.
Created attachment 49372 [details]
Qt WebGL support
Style fixed
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.
Created attachment 49381 [details]
Qt WebGL support
Comment on attachment 49381 [details]
Qt WebGL support
Looks good! some final comments on IRC that you can fix if you would like.
Created attachment 49383 [details]
Qt WebGL support
Final polishing of comments etc style :)
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;
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 Comment on attachment 49475 [details] Qt WebGL support Clearing flags on attachment: 49475 Committed r55235: <http://trac.webkit.org/changeset/55235> All reviewed patches have been landed. Closing bug. (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! |