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