Bug 215804

Created attachment 407182 [details] Patch

Note that there are important steps to take when updating ANGLE. See http://trac.webkit.org/wiki/UpdatingANGLE

Created attachment 407186 [details] Patch

Comment on attachment 407186 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407186&action=review Good work Kimmo tracking down what was going on. A couple of suggestions above. Would you consider trying that direction? I'm surprised this would have an effect on all GL_RGB textures. We have however seen problems with GL textures bound to IOSurfaces, where OpenGL on macOS zeros out the alpha channel of the destination IOSurface if a GL_RGB texture is bound to it. I wonder whether that issue may be the underlying cause here - but it doesn't look like it. Also - after you test your patch here but before it's committed, it would be great if you'd upload it to ANGLE and get reviews from the ANGLE team. They can review this better than me. > Source/ThirdParty/ANGLE/src/libANGLE/renderer/gl/BlitGL.cpp:-136 > - stateManager->setClearColor(gl::ColorF(0.0f, 0.0f, 0.0f, 0.0f)); Instead of removing this helper I suggest: 1) Pass gl::Context* to this helper 2) Pass another argument indicating whether there are alpha bits in the texture's format (see below) 3) Add a workaround to include/platform/FeaturesGL.h to be applied only on macOS (it looks like these changes regress a few WebGL conformance tests on iOS) 4) Initialize the feature in InitializeFeatures in src/libANGLE/renderer/gl/renderergl_utils.cpp 5) In this function, if there are alpha bits and the workaround is enabled, clear the alpha channel to 1.0 rather than 0.0 > Source/ThirdParty/ANGLE/src/libANGLE/renderer/gl/BlitGL.cpp:979 > + mStateManager->setClearColor(gl::ColorF(0.0f, 0.0f, 0.0f, 0.0f)); You didn't find the workaround necessary in this case? FramebufferGL::hasEmulatedAlphaChannelTextureAttachment() should, I think, return true if the back buffer of the WebGL canvas is alpha:false. What would you think about restoring the SetClearState helper, taking another argument (probably either the number of alpha bits or a bool whether to clear the alpha channel to 1.0 - seems hard to come up with an InternalFormatInfo here), and determining whether to set the alpha channel to 0.0 or 1.0 inside SetClearState depending on that argument?

CC'd ANGLE folks on this bug - maybe they can provide more feedback here.

BTW - I'm surprised that https://www.khronos.org/registry/webgl/sdk/tests/conformance/textures/misc/gl-teximage.html?webglVersion=2&quiet=0&quick=1 fails but the layout test webgl/2.0.0/conformance/textures/misc/gl-teximage.html doesn't. Kimmo, what hardware are you running on and which browser configuration are you testing? I typically run with the MiniBrowser on WebKit1 on a dual-GPU 2017 MacBook Pro 15" with Intel HD 630 / AMD Radeon Pro 560.

Agree with Ken's review. Happy to upstream with his suggestion to encapsulate this into a feature.

<rdar://problem/68132544>

Created attachment 408523 [details] Patch

Note that there are important steps to take when updating ANGLE. See https://trac.webkit.org/wiki/UpdatingANGLE

Comment on attachment 408523 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=408523&action=review Nice work Kimmo refining this. Could you address the build failures? Would be great if you could upload the ANGLE patch to ANGLE's Gerrit instance so that it can be automatically tested against the dEQP and more suites. > Source/ThirdParty/ANGLE/include/platform/FeaturesGL.h:475 > + "glClear writes alpha when clearing uninitialized RGB" End string in a space? > Source/ThirdParty/ANGLE/src/libANGLE/renderer/gl/BlitGL.cpp:546 > + blendState.setColorMaskIndexed(drawBuffer, true, true, true, true); Is it guaranteed that the needed extension will be available on all platforms where this code change might run? If not - should this be predicated on the availability of an extension? > Source/ThirdParty/ANGLE/src/libANGLE/renderer/gl/StateManagerGL.cpp:2124 > + ASSERT(!disableAlpha || disabledAlphaWrites.to_ulong() == 1); Are you sure that to_ulong() produces the number you expect in the situations you expect it?

Geoff's suggestions on the problematic part: Geoff Lang 1 month ago I think you want to check the DIRTY_BIT_COLOR_BUFFER_CONTENTS_0 bits in FramebufferGL::syncState Geoff Lang 1 month ago Our VK backend does it here Geoff Lang 1 month ago https://source.chromium.org/chromium/chromium/src/+/master:third_party/angle/src/libANGLE/renderer/vulkan/FramebufferVk.cpp;l=1710;drc=b79862be255715191be99c3ee924acdea58526fd;bpv=1;bpt=1?q=framebuffervk&ss=chromium%2Fchromium%2Fsrc source.chromium.orgsource.chromium.org Search and explore code Geoff Lang 1 month ago And then add a function to StateManagerGL to update the color mask state Geoff Lang 1 month ago And call that function both when syncing those bits in the framebuffer and when handling a new framebuffer binding in StateManagerGL Geoff Lang 1 month ago So: Geoff Lang 1 month ago Hereish: https://source.chromium.org/chromium/chromium/src/+/master:third_party/angle/src/libANGLE/renderer/gl/FramebufferGL.cpp;l=1307?q=framebuffergl.cpp&ss=chromium%2Fchromium%2Fsrc source.chromium.orgsource.chromium.org Search and explore code Geoff Lang 1 month ago And it looks like it's already handled in StatemanagerGL:syncState here: https://source.chromium.org/chromium/chromium/src/+/master:third_party/angle/src/libANGLE/renderer/gl/StateManagerGL.cpp;l=1919;drc=b79862be255715191be99c3ee924acdea58526fd;bpv=0;bpt=1