WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
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
Created
attachment 404833
[details]
Patch
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
Created
attachment 404883
[details]
Patch
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
<
rdar://problem/65918197
>
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
Created
attachment 404955
[details]
Patch
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
Created
attachment 404988
[details]
Patch
Kenneth Russell
Comment 22
2020-07-22 15:55:04 PDT
Created
attachment 404989
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug