Currently the WebGL enabled build is broken against platforms that have OpenGL ES instead of desktop GL. On these platforms we should try to use the new GraphicsContext3DOpenGLES.cpp that was introduced with bug #75462. * WebCore/Target.pri needs to be changed to compile GraphicsContext3DQt.cpp, *OpenGLCommon and *OpenGLES.cpp when contains(QT_CONFIG, opengles2). * The functions declared in GraphicsContext3DQt.cpp that are surrounded by #if defined(QT_OPENGL_ES_2) should be removed (their replacement should be in *OpenGLCommon.cpp and *OpenGLES.cpp)
Created attachment 133473 [details] Patch From now, GraphicsContext3DOpenGLES.cpp will be used on OpenGL ES supported platform. Functions are removed from GraphicsContext3DQt that already implemented in GraphicsContext3DCommon. "EXT" has been deleted from function and macro names ending with "EXT". It was needed because OpenGLES does not support these (and functions and macros without "EXT" do the same things as the ones ending with "EXT"). Extensions3DQt is removed beceause it has never been used. The "m_depthStencilBuffer" member is added to Qt port because it uses "m_depthStencilBuffer" like the other platforms. Furthermore some syntax errors have been fixed. Unfortunately I could not test perfectly because the webgl does not work. I get Segmentation Fault when I try to open a webgl example within QtTestbrowser. (On desktop) I don't get segfault on my N9 but my device draws nothing, and says that this browser does not support webgl.
Comment on attachment 133473 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=133473&action=review Good work, see comments. > Source/WebCore/ChangeLog:9 > + No new tests, just a buildfix. > + I wouldn't call this a build fix. I'd say instead that it's tested by existing WebGL tests. > Source/WebCore/platform/graphics/GraphicsContext3D.h:947 > +#if defined(QT_OPENGL_ES_2) || USE(OPENGL_ES_2) I e should add #if defined(QT_OPENGL_ES_2) #define WTF_USE_OPENGL_ES_2 1 #endif to Platform.h instead. Then we don't need to use ||. > Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:173 > + ::glBindFramebuffer(GL_FRAMEBUFFER, m_fbo); Wouldn't this break some existing desktop stuff, where EXT is needed? I'd rather this be moved to a bindFramebuffer that's implemented differently in ES and desktop. > Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:212 > +#if defined(QT_OPENGL_ES_2) || USE(OPENGL_ES_2) I'd rather we move all the ES specific functionality into the ES/desktop files, rather than have #ifdefs inside common. This applies to a lot of the following changes :)
Comment on attachment 133473 [details] Patch I've been hoping to see this change for a long time! I agree with No'am comments.
Created attachment 134278 [details] patch The changes are implemented based on Noam advices.
was this tested (In reply to comment #4) > Created an attachment (id=134278) [details] > patch > > The changes are implemented based on Noam advices. Was this tested on actual ES hardware (e.g. N9)?
(In reply to comment #5) > was this tested (In reply to comment #4) > > Created an attachment (id=134278) [details] [details] > > patch > > > > The changes are implemented based on Noam advices. > > Was this tested on actual ES hardware (e.g. N9)? Of course, I sent the compiled webkit to my N9, but unfortunatelly it did not work. I wrote at my first comment why I could not to test. The working is similar to Deskto on my N9. My next task will fix this segfault error.
(In reply to comment #6) > (In reply to comment #5) > > was this tested (In reply to comment #4) > > > Created an attachment (id=134278) [details] [details] [details] > > > patch > > > > > > The changes are implemented based on Noam advices. > > > > Was this tested on actual ES hardware (e.g. N9)? > > Of course, I sent the compiled webkit to my N9, but unfortunatelly it did not work. I wrote at my first comment why I could not to test. > > The working is similar to Deskto on my N9. > > My next task will fix this segfault error. OK, let's wait with this until it works then.
> > My next task will fix this segfault error. > > OK, let's wait with this until it works then. btw - you're aware that this is only testable with WebKit-1 (QtTestBrowser) right now.
Comment on attachment 134278 [details] patch Clearing flags for now.
Any progress on this?
(In reply to comment #10) > Any progress on this? Sorry, I had a day-off then. The patch is ready, but it does not work on N9 (since --3d-canvas have been replaces with --webgl). When I run QtTestBrowser with these switches: -graphicsbased -gl-viewport -tiled-backing-store -viewport-update-mode BoundingRect -webgl http://jsbin.com/ulazel I get an error saying: "Your web browser does not support WebGL!" Is it possible that a switch is missing (it used to work when --3d-canvas was active)?
(In reply to comment #11) > (In reply to comment #10) > > Any progress on this? > > Sorry, I had a day-off then. > > The patch is ready, but it does not work on N9 (since --3d-canvas have been replaces with --webgl). > > When I run QtTestBrowser with these switches: > > -graphicsbased -gl-viewport -tiled-backing-store -viewport-update-mode BoundingRect -webgl http://jsbin.com/ulazel > > I get an error saying: "Your web browser does not support WebGL!" > > Is it possible that a switch is missing (it used to work when --3d-canvas was active)? I think the --3d-canvas switch for build-webkit has been replaced with --webgl. Are you sure the code is getting built?
Created attachment 145574 [details] patch WebGL works on N9 with this patch
Comment on attachment 145574 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=145574&action=review > Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:252 > + GraphicsContext3D::clearDepth(1); I don't think you need GraphicsContext3D:: > Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:274 > + GraphicsContext3D::clearDepth(clearDepth); ditto. > Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:93 > - ::glTexImage2D(GL_TEXTURE_2D, 0, GL_DEPTH_STENCIL_OES, width, height, 0, GL_DEPTH_STENCIL_OES, GL_UNSIGNED_INT_24_8_OES, 0); > + ::glTexImage2D(GL_TEXTURE_2D, 0, m_internalColorFormat, width, height, 0, colorFormat, pixelDataType, 0); This seems wrong. m_internalColorFormat is equivalent to GL_DEPTH_STENCIL_OES? I suggest we simply not allow this code-path if GL_DEPTH_STENCIL_OES is not defined, or something to that effect. > Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:236 > +#if !defined (QT_OPENGL_ES_2) Let's switch to USE(OPENGL_ES_2) in this file while we're at it.
(In reply to comment #14) > (From update of attachment 145574 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=145574&action=review > > > Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:252 > > + GraphicsContext3D::clearDepth(1); > > I don't think you need GraphicsContext3D:: > > > Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:274 > > + GraphicsContext3D::clearDepth(clearDepth); > > ditto. It is necessary to write 'GraphicsContext3D::' because there is a variable that name is 'clearDepth'. With 'GraphicsContext3D::' you can say to the compiler that it is a function and not the variable. > > Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:93 > > - ::glTexImage2D(GL_TEXTURE_2D, 0, GL_DEPTH_STENCIL_OES, width, height, 0, GL_DEPTH_STENCIL_OES, GL_UNSIGNED_INT_24_8_OES, 0); > > + ::glTexImage2D(GL_TEXTURE_2D, 0, m_internalColorFormat, width, height, 0, colorFormat, pixelDataType, 0); > > This seems wrong. m_internalColorFormat is equivalent to GL_DEPTH_STENCIL_OES? > I suggest we simply not allow this code-path if GL_DEPTH_STENCIL_OES is not defined, or something to that effect. You are right. I found the appropriate of GL_DEPTH_STENCIL_OES in GC3D.h that is GraphicsContext3D::DEPTH_STENCIL. But the GL_UNSIGNED_INT_24_8_OES has not an appropriate type in GC3D.h that's why I inserted a new type into GC3D's macros. It is UNSIGNED_INT_24_8. I hope it is a right way. > > Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:236 > > +#if !defined (QT_OPENGL_ES_2) > > Let's switch to USE(OPENGL_ES_2) in this file while we're at it. fixed
Created attachment 145737 [details] patch
Comment on attachment 145737 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=145737&action=review Great! One nitpick. > Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:240 > +#if !USE(OPENGL_ES_2) > glBindFramebuffer(GL_READ_FRAMEBUFFER_EXT, m_context->m_multisampleFBO); > glBindFramebuffer(GL_DRAW_FRAMEBUFFER_EXT, m_context->m_fbo); > +#endif > glBlitFramebuffer(0, 0, m_context->m_currentWidth, m_context->m_currentHeight, 0, 0, m_context->m_currentWidth, m_context->m_currentHeight, GL_COLOR_BUFFER_BIT, GL_LINEAR); The #endif should encompass the blit as well, we can't blit if we don't bind READ/DRAW fbos.
Created attachment 145796 [details] patch
Comment on attachment 145796 [details] patch Clearing flags on attachment: 145796 Committed r119552: <http://trac.webkit.org/changeset/119552>
All reviewed patches have been landed. Closing bug.