Summary: | [WebGL][EFL][GTK][Qt]Add support for OES_vertex_array_object. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kalyan <kalyan.kondapally> | ||||||||
Component: | WebKit EFL | Assignee: | Kalyan <kalyan.kondapally> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | bajones, buildbot, dino, gman, gyuyoung.kim, hausmann, jturcotte, kadam, kbr, laszlo.gombos, lucas.de.marchi, mrobinson, noam, ossy, ostap73, rakuco, rgabor, rniwa, webkit.review.bot, zarvai, zeno | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | PC | ||||||||||
OS: | Linux | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 109797 | ||||||||||
Attachments: |
|
Description
Kalyan
2013-02-10 11:31:22 PST
Created attachment 187492 [details]
patch
I had to add the following to layout test: if (window.internals) window.internals.settings.setWebGLErrorsToConsoleEnabled(false); to disable WebGL from printing errors to console, as we do a test with extension disabled. Needed to add isSuccessfullyParsed(); at the end of file. Comment on attachment 187492 [details] patch Attachment 187492 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16488311 New failing tests: compositing/checkerboard.html accessibility/anchor-linked-anonymous-block-crash.html http/tests/cache/cancel-multiple-post-xhrs.html animations/3d/state-at-end-event-transform.html animations/animation-add-events-in-handler.html animations/3d/replace-filling-transform.html http/tests/cache/history-only-cached-subresource-loads.html compositing/bounds-in-flipped-writing-mode.html accessibility/accessibility-node-reparent.html animations/animation-border-overflow.html accessibility/accessibility-object-detached.html accessibility/anonymous-render-block-in-continuation-causes-crash.html animations/animation-controller-drt-api.html compositing/absolute-position-changed-with-composited-parent-layer.html compositing/absolute-position-changed-in-composited-layer.html http/tests/cache/iframe-304-crash.html animations/3d/transform-perspective.html http/tests/cache/cancel-during-failure-crash.html canvas/philip/tests/2d.clearRect+fillRect.alpha0.5.html canvas/philip/tests/2d.clearRect.basic.html animations/3d/transform-origin-vs-functions.html animations/animation-css-rule-types.html canvas/philip/tests/2d.clearRect+fillRect.basic.html canvas/philip/tests/2d.clearRect+fillRect.alpha0.html Created attachment 187621 [details]
patchv2
Gregg or Brandon, could one of you take a look at this patch? I recall Chromium's implementation of this extension being more complex. Is the layout test sufficient to catch any driver bugs that might have required workarounds? Comment on attachment 187621 [details] patchv2 View in context: https://bugs.webkit.org/attachment.cgi?id=187621&action=review > Source/WebCore/platform/graphics/OpenGLShims.cpp:132 > +#if defined(GL_ARB_vertex_array_object) > + ASSIGN_FUNCTION_TABLE_ENTRY(glBindVertexArray, success); > +#endif Wouldn't it make sense to include these entries even if GL_ARB_vertex_array_object isn't defined in the GL headers. We could still fill them out at runtime if the machine includes different symbols. > Source/WebCore/platform/graphics/opengl/Extensions3DOpenGL.cpp:165 > + if (name == "GL_OES_vertex_array_object") { > +#if (PLATFORM(GTK) || PLATFORM(QT) || PLATFORM(EFL)) > + return m_availableExtensions.contains("GL_ARB_vertex_array_object"); > +#else I'm guessing you want to check for both GL_OES_vertex_array_object and GL_ARB_vertex_array_object. > Source/WebCore/platform/graphics/opengl/Extensions3DOpenGL.cpp:194 > + static bool supportsVertexArrayObject = const_cast<Extensions3DOpenGL*>(this)->supports("GL_OES_vertex_array_object"); > + return supportsVertexArrayObject; > +} It's a bit funky to cast away the constness here. Why not make supports work with const pointers or just make this method work with non-const pointers. Created attachment 187808 [details]
patchv3
(In reply to comment #6) > (From update of attachment 187621 [details]) >We could still fill them out at runtime if the machine includes different symbols. done. > > > Source/WebCore/platform/graphics/opengl/Extensions3DOpenGL.cpp:165 > > + if (name == "GL_OES_vertex_array_object") { > > +#if (PLATFORM(GTK) || PLATFORM(QT) || PLATFORM(EFL)) > > + return m_availableExtensions.contains("GL_ARB_vertex_array_object"); > > +#else > > I'm guessing you want to check for both GL_OES_vertex_array_object and GL_ARB_vertex_array_object. It should be enough to check for GL_ARB_vertex_array_object here(Extensions3DOpenGL), as we know that GL_OES_vertex_array_object is an OpenGL-ES extension. > It's a bit funky to cast away the constness here. Why not make supports work with const pointers or just make this method work with non-const pointers. Done, made the method to work with non-const pointers. I did try earlier to make supports work with const pointers but ended up with a conclusion to do that change in another patch. I will probably do that after this one. (In reply to comment #5) > Gregg or Brandon, could one of you take a look at this patch? I recall Chromium's implementation of this extension being more complex. Is the layout test sufficient to catch any driver bugs that might have required workarounds? With the latest changes this patch looks good. All of the validation that made Chrome/Webkit's implementation complex are at a different layer, this patch is just exposing the underlying native function on new platforms, but the rest of the logic should still remain the same. The existing layout test should be sufficient to catch implementation issues. The changes to GraphicsContext3D et al. look okay to me, but someone else will likely need to review the test change. Comment on attachment 187808 [details]
patchv3
The test change looks fine.
Looks good to me. The things that made it hard on Chrome had to do with our OpenGL ES 2.0 emulation which exposes more features than WebGL. Comment on attachment 187808 [details]
patchv3
Since @mrobinson indicated the code changes look OK, marking r+ and cq+.
Comment on attachment 187808 [details] patchv3 Clearing flags on attachment: 187808 Committed r142786: <http://trac.webkit.org/changeset/142786> All reviewed patches have been landed. Closing bug. It broke the Qt-ARM build: /mnt/raptor2/slaves/qt5-linux-armv7-release/build/Source/WebCore/platform/graphics/OpenGLShims.cpp: In function 'bool WebCore::initializeOpenGLShims()': /mnt/raptor2/slaves/qt5-linux-armv7-release/build/Source/WebCore/platform/graphics/OpenGLShims.cpp:130:5: error: '::glBindVertexArray' has not been declared /mnt/raptor2/slaves/qt5-linux-armv7-release/build/Source/WebCore/platform/graphics/OpenGLShims.cpp:152:5: error: '::glDeleteVertexArrays' has not been declared /mnt/raptor2/slaves/qt5-linux-armv7-release/build/Source/WebCore/platform/graphics/OpenGLShims.cpp:162:5: error: '::glGenVertexArrays' has not been declared /mnt/raptor2/slaves/qt5-linux-armv7-release/build/Source/WebCore/platform/graphics/OpenGLShims.cpp:186:5: error: '::glIsVertexArray' has not been declared Gábor, could you check it? Same problem on Windows: OpenGLShims.cpp C:\WebKitBuildSlave\szeged-windows-1\qt-windows-32bit-release\build\Source\WebCore\platform\graphics\OpenGLShims.cpp(130) : error C2039: 'glBindVertexArray' : is not a member of '`global namespace'' C:\WebKitBuildSlave\szeged-windows-1\qt-windows-32bit-release\build\Source\WebCore\platform\graphics\OpenGLShims.cpp(130) : error C2065: 'glBindVertexArray' : undeclared identifier C:\WebKitBuildSlave\szeged-windows-1\qt-windows-32bit-release\build\Source\WebCore\platform\graphics\OpenGLShims.cpp(152) : error C2039: 'glDeleteVertexArrays' : is not a member of '`global namespace'' C:\WebKitBuildSlave\szeged-windows-1\qt-windows-32bit-release\build\Source\WebCore\platform\graphics\OpenGLShims.cpp(152) : error C2065: 'glDeleteVertexArrays' : undeclared identifier C:\WebKitBuildSlave\szeged-windows-1\qt-windows-32bit-release\build\Source\WebCore\platform\graphics\OpenGLShims.cpp(162) : error C2039: 'glGenVertexArrays' : is not a member of '`global namespace'' C:\WebKitBuildSlave\szeged-windows-1\qt-windows-32bit-release\build\Source\WebCore\platform\graphics\OpenGLShims.cpp(162) : error C2065: 'glGenVertexArrays' : undeclared identifier C:\WebKitBuildSlave\szeged-windows-1\qt-windows-32bit-release\build\Source\WebCore\platform\graphics\OpenGLShims.cpp(186) : error C2039: 'glIsVertexArray' : is not a member of '`global namespace'' C:\WebKitBuildSlave\szeged-windows-1\qt-windows-32bit-release\build\Source\WebCore\platform\graphics\OpenGLShims.cpp(186) : error C2065: 'glIsVertexArray' : undeclared identifier (In reply to comment #16) > It broke the Qt-ARM build: > Gábor, could you check it? I am also looking into it. Created a separate issue for this https://bugs.webkit.org/show_bug.cgi?id=109797 |