Bug 223734 - Depth test affects WebGL rendering when context has depth == false, stencil == true
Summary: Depth test affects WebGL rendering when context has depth == false, stencil =...
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kimmo Kinnunen
URL:
Keywords: InRadar
Depends on: 223735
Blocks: webgl2conformance
  Show dependency treegraph
 
Reported: 2021-03-25 01:19 PDT by Kimmo Kinnunen
Modified: 2021-04-01 01:20 PDT (History)
6 users (show)

See Also:


Attachments
Patch (25.77 KB, patch)
2021-03-25 03:45 PDT, Kimmo Kinnunen
dino: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kimmo Kinnunen 2021-03-25 01:19:32 PDT
Depth test affects WebGL rendering when context has depth == false, stencil == true

Happens when stencil is enabled and stencil is stencil+depth.
Comment 1 Kimmo Kinnunen 2021-03-25 03:45:46 PDT
Created attachment 424231 [details]
Patch
Comment 2 Kyle Piddington 2021-03-25 10:59:57 PDT
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
Comment 3 Kenneth Russell 2021-03-29 15:43:58 PDT
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 .
Comment 4 Kimmo Kinnunen 2021-03-30 03:45:17 PDT
(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.
Comment 5 Kenneth Russell 2021-03-30 10:58:04 PDT
(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
Comment 6 Radar WebKit Bug Importer 2021-04-01 01:20:14 PDT
<rdar://problem/76093639>