Created attachment 424231 [details] Patch

Comment on attachment 424231 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=424231&action=review Overall, this should certainly work to keep depth and stencil correctly set. > Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:7694 > + enableOrDisable(GraphicsContextGL::DEPTH_TEST, enable); The logic here is sound. I also believe that you could get a similar result in Metal_Angle by attaching a d24s8 attachment to just STENCIL_ATTACHMENT instead of always DEPTH_STENCIL_ATTACHMENT when available. See void GraphicsContextGLOpenGL::attachDepthAndStencilBufferIfNeeded(GLuint internalDepthStencilFormat, int width, int height) in GraphicsContextANGLE

Great work on this Kimmo - would be great if you could propose the conformance test changes as a pull request against https://github.com/KhronosGroup/WebGL .

(In reply to Kyle Piddington from comment #2) > The logic here is sound. I also believe that you could get a similar result > in Metal_Angle by attaching a d24s8 attachment to just STENCIL_ATTACHMENT > instead of always DEPTH_STENCIL_ATTACHMENT when available. See void > GraphicsContextGLOpenGL::attachDepthAndStencilBufferIfNeeded(GLuint > internalDepthStencilFormat, int width, int height) in GraphicsContextANGLE Kyle, thanks for the review. I'm hesitant to do that cleanup yet since there's this comment // WebGL 1.0's rules state that combined depth/stencil renderbuffers // have to be attached to the synthetic DEPTH_STENCIL_ATTACHMENT point. I don't know enough of the spec / implementation to say if storing the stencil depth buffer in stencil for the case stencil=true, depth=false would satisfy the getter behavior we currently have or should have.

(In reply to Kimmo Kinnunen from comment #4) > (In reply to Kyle Piddington from comment #2) > > The logic here is sound. I also believe that you could get a similar result > > in Metal_Angle by attaching a d24s8 attachment to just STENCIL_ATTACHMENT > > instead of always DEPTH_STENCIL_ATTACHMENT when available. See void > > GraphicsContextGLOpenGL::attachDepthAndStencilBufferIfNeeded(GLuint > > internalDepthStencilFormat, int width, int height) in GraphicsContextANGLE > > Kyle, thanks for the review. > I'm hesitant to do that cleanup yet since there's this comment > // WebGL 1.0's rules state that combined depth/stencil renderbuffers > // have to be attached to the synthetic DEPTH_STENCIL_ATTACHMENT > point. > > I don't know enough of the spec / implementation to say if storing the > stencil depth buffer in stencil for the case stencil=true, depth=false would > satisfy the getter behavior we currently have or should have. Hopefully the WebGL 1.0 and 2.0 conformance tests should cover this behavior well. The bookkeeping for WebGL 1.0's synthetic DEPTH_STENCIL_ATTACHMENT can / should be managed entirely at the WebGLRenderingContextBase / WebGL2RenderingContext level, so maybe GraphicsContextANGLE can offer the semantic that Kyle suggests (depth/stencil are combined, and the same object is attached separately to the DEPTH and STENCIL attachment points). See the associated code in Chromium: https://source.chromium.org/search?q=DEPTH_STENCIL_ATTACHMENT%20f:third_party%2Fblink%2Frenderer%2Fmodules%2Fwebgl

<rdar://problem/76093639>

Created attachment 436182 [details] Patch for landing

This is fixed in bug 242657 and the patch is not needed.