Bug 109382

Summary: [WebGL][EFL][GTK][Qt]Add support for OES_vertex_array_object.
Product: WebKit Reporter: Kalyan <kalyan.kondapally>
Component: WebKit EFLAssignee: 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 Flags
patch
buildbot: commit-queue-
patchv2
none
patchv3 none

Description Kalyan 2013-02-10 11:31:22 PST
Add support for OES_vertex_array_object.
Comment 1 Kalyan 2013-02-10 13:15:41 PST
Created attachment 187492 [details]
patch
Comment 2 Kalyan 2013-02-10 13:32:51 PST
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 3 Build Bot 2013-02-11 07:30:31 PST
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
Comment 4 Kalyan 2013-02-11 11:19:16 PST
Created attachment 187621 [details]
patchv2
Comment 5 Kenneth Russell 2013-02-11 14:11:14 PST
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 6 Martin Robinson 2013-02-11 14:20:21 PST
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.
Comment 7 Kalyan 2013-02-12 01:40:27 PST
Created attachment 187808 [details]
patchv3
Comment 8 Kalyan 2013-02-12 01:54:35 PST
(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.
Comment 9 Brandon Jones 2013-02-12 09:44:26 PST
(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.
Comment 10 Martin Robinson 2013-02-12 09:47:18 PST
The changes to GraphicsContext3D et al. look okay to me, but someone else will likely need to review the test change.
Comment 11 Kenneth Russell 2013-02-12 11:50:16 PST
Comment on attachment 187808 [details]
patchv3

The test change looks fine.
Comment 12 Gregg Tavares 2013-02-12 17:39:39 PST
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 13 Kenneth Russell 2013-02-13 13:08:57 PST
Comment on attachment 187808 [details]
patchv3

Since @mrobinson indicated the code changes look OK, marking r+ and cq+.
Comment 14 WebKit Review Bot 2013-02-13 13:32:04 PST
Comment on attachment 187808 [details]
patchv3

Clearing flags on attachment: 187808

Committed r142786: <http://trac.webkit.org/changeset/142786>
Comment 15 WebKit Review Bot 2013-02-13 13:32:10 PST
All reviewed patches have been landed.  Closing bug.
Comment 16 Csaba Osztrogonác 2013-02-13 14:40:03 PST
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?
Comment 17 Csaba Osztrogonác 2013-02-13 23:36:24 PST
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
Comment 18 Kalyan 2013-02-14 02:13:38 PST
(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