Bug 211156 - [WebGL2] Implement multiple render target entry points
Summary: [WebGL2] Implement multiple render target entry points
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kenneth Russell
URL:
Keywords: InRadar
Depends on: 126994 170325 210994 214642
Blocks: 126404 214780
  Show dependency treegraph
 
Reported: 2020-04-28 17:07 PDT by Kenneth Russell
Modified: 2020-07-24 20:56 PDT (History)
20 users (show)

See Also:


Attachments
Patch (45.32 KB, patch)
2020-07-21 10:27 PDT, Kenneth Russell
no flags Details | Formatted Diff | Diff
Patch (45.58 KB, patch)
2020-07-21 17:16 PDT, Kenneth Russell
no flags Details | Formatted Diff | Diff
Patch (50.66 KB, patch)
2020-07-22 13:15 PDT, Kenneth Russell
no flags Details | Formatted Diff | Diff
Patch (50.65 KB, patch)
2020-07-22 15:53 PDT, Kenneth Russell
no flags Details | Formatted Diff | Diff
Patch (50.65 KB, patch)
2020-07-22 15:55 PDT, Kenneth Russell
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kenneth Russell 2020-04-28 17:07:33 PDT
In Bug 126994 support was supposedly added for multiple render targets, including the following entry points:

drawBuffers
clearBufferfv
clearBufferiv
clearBufferuiv
clearBufferfi

but the implementations were no-op'ed. This leads to at least one conformance test failure.

It's not clear to me how many layout tests' results will be changed as a result of fixing this, so splitting it off from other bugs.
Comment 1 Kenneth Russell 2020-07-17 15:35:24 PDT
Taking this bug.
Comment 2 Kenneth Russell 2020-07-20 17:50:55 PDT
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 .
Comment 3 Kenneth Russell 2020-07-21 10:27:55 PDT
Created attachment 404833 [details]
Patch
Comment 4 Kenneth Russell 2020-07-21 12:03:45 PDT
Comment on attachment 404833 [details]
Patch

Note: the ios-wk2 and api-gtk test failures are unrelated to this patch.
Comment 5 Myles C. Maxfield 2020-07-21 16:56:04 PDT
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 6 Kenneth Russell 2020-07-21 17:15:03 PDT
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.
Comment 7 Kenneth Russell 2020-07-21 17:16:08 PDT
Created attachment 404883 [details]
Patch
Comment 8 Kenneth Russell 2020-07-21 17:17:20 PDT
Comment on attachment 404883 [details]
Patch

Addressed Myles' review feedback. CQ'ing now to be able to build on this patch.
Comment 9 EWS 2020-07-21 19:13:22 PDT
Found 1 new test failure: fast/mediastream/captureStream/canvas3d.html
Comment 10 Kenneth Russell 2020-07-21 20:48:48 PDT
Comment on attachment 404883 [details]
Patch

I'm pretty sure that test failure is an unrelated flake. Re-CQ'ing.
Comment 11 EWS 2020-07-21 21:08:03 PDT
Committed r264691: <https://trac.webkit.org/changeset/264691>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 404883 [details].
Comment 12 Radar WebKit Bug Importer 2020-07-21 21:09:13 PDT
<rdar://problem/65918197>
Comment 13 Kenneth Russell 2020-07-22 09:17:58 PDT
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.
Comment 14 Kenneth Russell 2020-07-22 09:28:41 PDT
Reverted in r264700, Bug 214642. Investigating the test failure.
Comment 15 Kenneth Russell 2020-07-22 11:25:30 PDT
The earlier patch for this bug regressed the fix for Bug 170325.
Comment 16 Kenneth Russell 2020-07-22 13:15:14 PDT
Created attachment 404955 [details]
Patch
Comment 17 Kenneth Russell 2020-07-22 13:16:24 PDT
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 18 Dean Jackson 2020-07-22 14:59:15 PDT
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 19 Kenneth Russell 2020-07-22 15:51:45 PDT
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.
Comment 20 Kenneth Russell 2020-07-22 15:53:04 PDT
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.
Comment 21 Kenneth Russell 2020-07-22 15:53:50 PDT
Created attachment 404988 [details]
Patch
Comment 22 Kenneth Russell 2020-07-22 15:55:04 PDT
Created attachment 404989 [details]
Patch
Comment 23 Kenneth Russell 2020-07-22 15:55:27 PDT
Comment on attachment 404989 [details]
Patch

CQ'ing this version.
Comment 24 EWS 2020-07-22 16:39:03 PDT
Committed r264733: <https://trac.webkit.org/changeset/264733>

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