Bug 211156

Taking this bug.

While fixing this, have been referring to this bug fix in Chromium: http://crbug.com/828262

and the associated WebGL conformance test added in:
https://github.com/KhronosGroup/WebGL/pull/2628

which is available online at:
https://www.khronos.org/registry/webgl/sdk/tests/conformance2/rendering/clearbuffer-and-draw.html

(this isn't in WebKit's 2.0.0 conformance test snapshot - it was added in 2.0.1.)

This conformance test passes with the fix, as does another one that's already in WebKit's conformance test snapshot: webgl/2.0.0/conformance2/reading/read-pixels-from-fbo-test.html .

Created attachment 404833 [details]
Patch

Comment on attachment 404833 [details]
Patch

Note: the ios-wk2 and api-gtk test failures are unrelated to this patch.

Comment on attachment 404833 [details]
Patch

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

> Source/WebCore/html/canvas/WebGL2RenderingContext.h:321
> +    enum ClearBufferCaller {

enum class ClearBufferCaller : uint8_t

> Source/WebCore/html/canvas/WebGLFramebuffer.cpp:-649
> -#if ENABLE(WEBGL2)
> -    // FIXME: The logic here seems wrong. If we don't have WebGL 2 enabled at all, then
> -    // we skip the m_webglDrawBuffers check. But if we do have WebGL 2 enabled, then we
> -    // perform this check, for WebGL 1 contexts only.
> -    if (!context()->m_webglDrawBuffers && !context()->isWebGL2())
> -        return;
> -#endif
> -    bool reset = force;
> -    // This filtering works around graphics driver bugs on Mac OS X.
> -    for (size_t i = 0; i < m_drawBuffers.size(); ++i) {
> -        if (m_drawBuffers[i] != GraphicsContextGL::NONE && getAttachment(m_drawBuffers[i])) {
> -            if (m_filteredDrawBuffers[i] != m_drawBuffers[i]) {
> -                m_filteredDrawBuffers[i] = m_drawBuffers[i];
> -                reset = true;
> +    if (context()->isWebGL2() || context()->m_webglDrawBuffers) {
> +        bool reset = force;
> +        // This filtering works around graphics driver bugs on Mac OS X.
> +        for (size_t i = 0; i < m_drawBuffers.size(); ++i) {
> +            if (m_drawBuffers[i] != GraphicsContextGL::NONE && getAttachment(m_drawBuffers[i])) {
> +                if (m_filteredDrawBuffers[i] != m_drawBuffers[i]) {
> +                    m_filteredDrawBuffers[i] = m_drawBuffers[i];
> +                    reset = true;
> +                }
> +            } else {
> +                if (m_filteredDrawBuffers[i] != GraphicsContextGL::NONE) {
> +                    m_filteredDrawBuffers[i] = GraphicsContextGL::NONE;
> +                    reset = true;
> +                }
>              }
> -        } else {
> -            if (m_filteredDrawBuffers[i] != GraphicsContextGL::NONE) {
> -                m_filteredDrawBuffers[i] = GraphicsContextGL::NONE;
> -                reset = true;
> +        }
> +        if (reset) {
> +            if (context()->isWebGL2()) {
> +                context()->graphicsContextGL()->drawBuffers(
> +                    m_filteredDrawBuffers.size(), m_filteredDrawBuffers.data());
> +            } else {
> +                context()->graphicsContextGL()->getExtensions().drawBuffersEXT(
> +                    m_filteredDrawBuffers.size(), m_filteredDrawBuffers.data());
>              }
>          }
>      }
> -    if (reset) {
> -        context()->graphicsContextGL()->getExtensions().drawBuffersEXT(
> -            m_filteredDrawBuffers.size(), m_filteredDrawBuffers.data());
> -    }

Dean will have to review this for me.

Comment on attachment 404833 [details]
Patch

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

Thanks Myles for being willing to review this on short notice - further patches depend on these correctness fixes. Revising this patch now with your review feedback.

>> Source/WebCore/html/canvas/WebGL2RenderingContext.h:321
>> +    enum ClearBufferCaller {
> 
> enum class ClearBufferCaller : uint8_t

Done.

>> Source/WebCore/html/canvas/WebGLFramebuffer.cpp:-649
>> -    }
> 
> Dean will have to review this for me.

I'll make sure to ask Dean to review this entire patch tomorrow, especially this section.

Created attachment 404883 [details]
Patch

Comment on attachment 404883 [details]
Patch

Addressed Myles' review feedback. CQ'ing now to be able to build on this patch.

Found 1 new test failure: fast/mediastream/captureStream/canvas3d.html

Comment on attachment 404883 [details]
Patch

I'm pretty sure that test failure is an unrelated flake. Re-CQ'ing.

Committed r264691: <https://trac.webkit.org/changeset/264691>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 404883 [details].

<rdar://problem/65918197>

Unfortunately, it's pretty clear this patch regressed fast/mediastream/captureStream/canvas3d.html .

https://results.webkit.org/?suite=layout-tests&test=fast%2Fmediastream%2FcaptureStream%2Fcanvas3d.html

Reopening and attempting a revert.

Reverted in r264700, Bug 214642. Investigating the test failure.

The earlier patch for this bug regressed the fix for Bug 170325.

Created attachment 404955 [details]
Patch

Comment on attachment 404955 [details]
Patch

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

> Source/WebCore/platform/graphics/GraphicsContextGL.h:1122
> +    virtual void enablePreserveDrawingBuffer();

This is the first non-pure virtual in GraphicsContextGL, and it's necessary because m_attrs is private in this class. Is this OK from an architectural standpoint?

Comment on attachment 404955 [details]
Patch

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

> Source/WebCore/html/canvas/WebGLFramebuffer.cpp:626
> +        // This filtering works around graphics driver bugs on Mac OS X.

Nit: macOS

>> Source/WebCore/platform/graphics/GraphicsContextGL.h:1122
>> +    virtual void enablePreserveDrawingBuffer();
> 
> This is the first non-pure virtual in GraphicsContextGL, and it's necessary because m_attrs is private in this class. Is this OK from an architectural standpoint?

This is probably ok. The alternative would be to use GraphicsContextGL::setContextAttributes.

Comment on attachment 404955 [details]
Patch

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

>> Source/WebCore/html/canvas/WebGLFramebuffer.cpp:626
>> +        // This filtering works around graphics driver bugs on Mac OS X.
> 
> Nit: macOS

Done.

The win and ios-wk2 failures of https://bugs.webkit.org/attachment.cgi?id=404955&action=edit are unrelated to this patch.

Want to get this relanded in order to continue with other patches on top, so revising and CQ'ing the result now - mac-debug-wk1 is a bit slow.

Created attachment 404988 [details]
Patch

Created attachment 404989 [details]
Patch

Comment on attachment 404989 [details]
Patch

CQ'ing this version.

Committed r264733: <https://trac.webkit.org/changeset/264733>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 404989 [details].