Bug 109382 – [WebGL][EFL][GTK][Qt]Add support for OES_vertex_array_object.

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

[Kalyan]

Add support for OES_vertex_array_object.

[Kalyan]

Created attachment 187492 [details]
patch

[Kalyan]

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.

[Build Bot]

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

[Kalyan]

Created attachment 187621 [details]
patchv2

[Kenneth Russell]

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?

[Martin Robinson]

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.

[Kalyan]

Created attachment 187808 [details]
patchv3

[Kalyan]

(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.

[Brandon Jones]

(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.

[Martin Robinson]

The changes to GraphicsContext3D et al. look okay to me, but someone else will likely need to review the test change.

[Kenneth Russell]

Comment on attachment 187808 [details]
patchv3

The test change looks fine.

[Gregg Tavares]

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.

[Kenneth Russell]

Comment on attachment 187808 [details]
patchv3

Since @mrobinson indicated the code changes look OK, marking r+ and cq+.

[WebKit Review Bot]

Comment on attachment 187808 [details]
patchv3

Clearing flags on attachment: 187808

Committed r142786: <http://trac.webkit.org/changeset/142786>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

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?

[Csaba Osztrogonác]

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

[Kalyan]

(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