Bug 163924 – Teach WebGL code about new buffer targets in WebGL2

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

Teach WebGL code about new buffer targets in WebGL2

[Myles C. Maxfield]

Created attachment 292680 [details]
WIP

[Myles C. Maxfield]

Created attachment 292839 [details]
Patch

[Build Bot]

Comment on attachment 292839 [details]
Patch

Attachment 292839 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2370827

New failing tests:
fast/canvas/webgl/bufferData-offset-length.html
svg/wicd/test-rightsizing-b.xhtml

[Build Bot]

Created attachment 292845 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 292839 [details]
Patch

Attachment 292839 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2370895

New failing tests:
fast/canvas/webgl/bufferData-offset-length.html

[Build Bot]

Created attachment 292851 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 292839 [details]
Patch

Attachment 292839 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2371058

New failing tests:
fast/canvas/webgl/bufferData-offset-length.html

[Build Bot]

Created attachment 292854 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Myles C. Maxfield]

Created attachment 292879 [details]
Patch

[Myles C. Maxfield]

Created attachment 292880 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 292880 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=292880&action=review

> Source/WebCore/html/canvas/WebGLBuffer.cpp:158
> +            m_byteLength = byteLength;

This is wrong.

[Dean Jackson]

Comment on attachment 292880 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=292880&action=review

> Source/WebCore/ChangeLog:16
> +        In order to test this correctly, I had to upgrade the macOS's OpenGL 
> +        context to a 3.2-compatible context to make sure the new binding
> +        points don't cause errors. Eventually, this 3.2-compatible context
> +        will have to be reverted and replaced with an ANGLE context. The current
> +        3.2-compatible context is just for testing.

As I say below, this is great. But I think it should land in a separate patch. You can consider this an r+ for it.

> Source/WebCore/html/canvas/WebGLBuffer.cpp:239
> +    switch (target) {
> +    case GraphicsContext3D::ARRAY_BUFFER:
> +    case GraphicsContext3D::ELEMENT_ARRAY_BUFFER:
>          m_target = target;
> +        break;
> +    default:
> +#if ENABLE(WEBGL2)
> +        if (forWebGL2) {
> +            switch (target) {
> +            case GraphicsContext3D::COPY_READ_BUFFER:
> +            case GraphicsContext3D::COPY_WRITE_BUFFER:
> +            case GraphicsContext3D::PIXEL_PACK_BUFFER:
> +            case GraphicsContext3D::PIXEL_UNPACK_BUFFER:
> +            case GraphicsContext3D::TRANSFORM_FEEDBACK_BUFFER:
> +            case GraphicsContext3D::UNIFORM_BUFFER:
> +                m_target = target;
> +                break;
> +            }
> +        }
> +#else
> +        UNUSED_PARAM(forWebGL2);
> +#endif

I don't like this switch within a switch, with the same test.

It might have been better as it was originally, with the if statement for the two cases we know should set the target all the time. Then the test for WebGL2, with the switch child.

Also, it's a shame we have to do both #if WEBGL2 and if (forWebGL2). I guess the only way to avoid that would be to define all the GC3D things like UNIFORM_BUFFER in all cases. Actually, maybe that's ok. GC3D should be a GLES3-like API, with WebGLRenderingContext being the thing that stops that from exposure to the web. What do you think?

> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:928
> -    if (target == GraphicsContext3D::ARRAY_BUFFER)
> +    switch (target) {
> +    case GraphicsContext3D::ARRAY_BUFFER:
>          m_boundArrayBuffer = buffer;
> -    else if (target == GraphicsContext3D::ELEMENT_ARRAY_BUFFER)
> +        break;
> +    case GraphicsContext3D::ELEMENT_ARRAY_BUFFER:
>          m_boundVertexArrayObject->setElementArrayBuffer(buffer);
> -    else {
> +        break;
> +    default:
> +#if ENABLE(WEBGL2)
> +        bool success = true;
> +        if (isWebGL2()) {
> +            switch (target) {

Same comment for this one.

> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:2206
> +    bool valid = false;
> +    switch (target) {
> +    case GraphicsContext3D::ARRAY_BUFFER:
> +    case GraphicsContext3D::ELEMENT_ARRAY_BUFFER:
> +        valid = true;

And here.

> Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:129
> +    if (useGLES3) {
> +        // FIXME: Instead of backing a WebGL2 GraphicsContext3D with a OpenGL 3.2 context, we should instead back it with ANGLE.
> +        // Use an OpenGL 3.2 context for now until the ANGLE backend is ready.
> +        attribs.append(kCGLPFAOpenGLProfile);
> +        attribs.append(static_cast<CGLPixelFormatAttribute>(kCGLOGLPVersion_3_2_Core));
> +    }

This is good. But could you land it separately?

> Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:278
> -    ::glTexParameterf(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR);
> -    ::glTexParameterf(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR);
> -    ::glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, GL_CLAMP);
> -    ::glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_CLAMP);
> +    ::glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR);
> +    ::glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR);
> +    ::glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, GL_CLAMP_TO_EDGE);
> +    ::glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE);

Nice!

> Source/WebCore/platform/graphics/opengl/Extensions3DOpenGLCommon.cpp:228
> +#if PLATFORM(MAC)
> +    if (m_useIndexedGetString) {
> +        GLint numExtensions = 0;
> +        ::glGetIntegerv(GL_NUM_EXTENSIONS, &numExtensions);
> +        for (GLint i = 0; i < numExtensions; ++i)
> +            m_availableExtensions.add(glGetStringi(GL_EXTENSIONS, i));
> +    } else
> +#endif
> +    {
> +        String extensionsString = getExtensions();
> +        Vector<String> availableExtensions;
> +        extensionsString.split(' ', availableExtensions);
> +        for (size_t i = 0; i < availableExtensions.size(); ++i)
> +            m_availableExtensions.add(availableExtensions[i]);
> +    }

Why didn't you change getExtensions() instead of putting this here?

Also, you should explain this in the ChangeLog.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:301
> +        if (isGLES2Compliant()) {
> +            ASSERT(::glGetError() == GL_NO_ERROR);
> +            ::glGetIntegerv(GL_MAX_VARYING_VECTORS, value);
> +            if (::glGetError() == GL_INVALID_ENUM) {
> +                ::glGetIntegerv(GL_MAX_VARYING_COMPONENTS, value);
> +                *value /= 4;
> +            }
> +        } else {
> +            ::glGetIntegerv(GL_MAX_VARYING_FLOATS, value);
> +            *value /= 4;
> +        }

Explain this in the ChangeLog.

[Myles C. Maxfield]

Comment on attachment 292880 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=292880&action=review

>> Source/WebCore/platform/graphics/opengl/Extensions3DOpenGLCommon.cpp:228
>> +    }
> 
> Why didn't you change getExtensions() instead of putting this here?
> 
> Also, you should explain this in the ChangeLog.

getExtensions() returns a string. It seems silly to build up a string just to immediately split it up again.

[Myles C. Maxfield]

Comment on attachment 292880 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=292880&action=review

>> Source/WebCore/ChangeLog:16
>> +        3.2-compatible context is just for testing.
> 
> As I say below, this is great. But I think it should land in a separate patch. You can consider this an r+ for it.

https://bugs.webkit.org/show_bug.cgi?id=164091

[Myles C. Maxfield]

Created attachment 293101 [details]
Patch

[Myles C. Maxfield]

Created attachment 293102 [details]
Patch for committing

[WebKit Commit Bot]

Comment on attachment 293102 [details]
Patch for committing

Clearing flags on attachment: 293102

Committed r208032: <http://trac.webkit.org/changeset/208032>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.