RESOLVED FIXED 211156
[WebGL2] Implement multiple render target entry points
https://bugs.webkit.org/show_bug.cgi?id=211156
Summary [WebGL2] Implement multiple render target entry points
Kenneth Russell
Reported 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.
Attachments
Patch (45.32 KB, patch)
2020-07-21 10:27 PDT, Kenneth Russell
no flags
Patch (45.58 KB, patch)
2020-07-21 17:16 PDT, Kenneth Russell
no flags
Patch (50.66 KB, patch)
2020-07-22 13:15 PDT, Kenneth Russell
no flags
Patch (50.65 KB, patch)
2020-07-22 15:53 PDT, Kenneth Russell
no flags
Patch (50.65 KB, patch)
2020-07-22 15:55 PDT, Kenneth Russell
no flags
Kenneth Russell
Comment 1 2020-07-17 15:35:24 PDT
Taking this bug.
Kenneth Russell
Comment 2 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 .
Kenneth Russell
Comment 3 2020-07-21 10:27:55 PDT
Kenneth Russell
Comment 4 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.
Myles C. Maxfield
Comment 5 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.
Kenneth Russell
Comment 6 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.
Kenneth Russell
Comment 7 2020-07-21 17:16:08 PDT
Kenneth Russell
Comment 8 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.
EWS
Comment 9 2020-07-21 19:13:22 PDT
Found 1 new test failure: fast/mediastream/captureStream/canvas3d.html
Kenneth Russell
Comment 10 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.
EWS
Comment 11 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].
Radar WebKit Bug Importer
Comment 12 2020-07-21 21:09:13 PDT
Kenneth Russell
Comment 13 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.
Kenneth Russell
Comment 14 2020-07-22 09:28:41 PDT
Reverted in r264700, Bug 214642. Investigating the test failure.
Kenneth Russell
Comment 15 2020-07-22 11:25:30 PDT
The earlier patch for this bug regressed the fix for Bug 170325.
Kenneth Russell
Comment 16 2020-07-22 13:15:14 PDT
Kenneth Russell
Comment 17 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?
Dean Jackson
Comment 18 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.
Kenneth Russell
Comment 19 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.
Kenneth Russell
Comment 20 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.
Kenneth Russell
Comment 21 2020-07-22 15:53:50 PDT
Kenneth Russell
Comment 22 2020-07-22 15:55:04 PDT
Kenneth Russell
Comment 23 2020-07-22 15:55:27 PDT
Comment on attachment 404989 [details] Patch CQ'ing this version.
EWS
Comment 24 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].
Note You need to log in before you can comment on or make changes to this bug.