Depth test affects WebGL rendering when context has depth == false, stencil == true

• Component: WebGL
• Status: RESOLVED CONFIGURATION CHANGED

[Kimmo Kinnunen]

Depth test affects WebGL rendering when context has depth == false, stencil == true

Happens when stencil is enabled and stencil is stencil+depth.

[Kimmo Kinnunen]

Created attachment 424231 [details]
Patch

[Kyle Piddington]

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

[Kenneth Russell]

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 .

[Kimmo Kinnunen]

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

[Kenneth Russell]

(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

[Radar WebKit Bug Importer]

<rdar://problem/76093639>

[Kimmo Kinnunen]

Created attachment 436182 [details]
Patch for landing

[Kimmo Kinnunen]

https://github.com/KhronosGroup/WebGL/issues/3277

[Kimmo Kinnunen]

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