Bug 48891 – renderbufferStorage(DEPTH_STENCIL) shouldn't generate an error

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Zhenyao Mo]

We should check the DEPTH24_STENCIL8 extension.  If it doesn't exist or it's GLES backend, we shouldn't call renderbufferStorage; instead we should mark the renderbuffer as invalid and return.

[Zhenyao Mo]

Created attachment 72880 [details]
patch

Test synched with khronos.

The code path where packed_depth_stencil extension doesn't exist has also been tested by faking m_isDepthStencilSupported = false in WebGLRenderingContext.

This is half of the effort to fix the issue.  We also need to map the enum in command buffer so INVALID_ENUM won't be generated.

[Kenneth Russell]

Comment on attachment 72880 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=72880&action=review

Thanks for fixing this. The code and test look fine aside from one minor issue. It's too bad we can't force testing on a configuration where GL_EXT_packed_depth_stencil isn't actually supported. Are you going to take care of the Chromium side changes or should I?

> WebCore/html/canvas/WebGLRenderingContext.cpp:160
> +        m_isDepthStencilSupported = extensions.contains("GL_EXT_packed_depth_stencil");

At this point the way to phrase this extension query is m_context->getExtensions()->supports("GL_OES_packed_depth_stencil"). We're supposed to be getting rid of the EXTENSIONS enum at the public API level in https://bugs.webkit.org/show_bug.cgi?id=40316 and presumably from GraphicsContext3D as well. (It will probably need to remain as a private enum in Extensions3D.)

[Zhenyao Mo]

(In reply to comment #2)
> (From update of attachment 72880 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=72880&action=review
> 
> Thanks for fixing this. The code and test look fine aside from one minor issue. It's too bad we can't force testing on a configuration where GL_EXT_packed_depth_stencil isn't actually supported. Are you going to take care of the Chromium side changes or should I?

snowleopard bot is without depth_stencil extension, so we might rely on it for testing this path.

I'll fix the chromium side.

> 
> > WebCore/html/canvas/WebGLRenderingContext.cpp:160
> > +        m_isDepthStencilSupported = extensions.contains("GL_EXT_packed_depth_stencil");
> 
> At this point the way to phrase this extension query is m_context->getExtensions()->supports("GL_OES_packed_depth_stencil"). We're supposed to be getting rid of the EXTENSIONS enum at the public API level in https://bugs.webkit.org/show_bug.cgi?id=40316 and presumably from GraphicsContext3D as well. (It will probably need to remain as a private enum in Extensions3D.)

Oh yeah, I forgot about the new mechanism just landed.  I'll change it.

[Zhenyao Mo]

Created attachment 72952 [details]
revised patch: using the new extension mechanism

[Zhenyao Mo]

Created attachment 72953 [details]
revised patch: using the new extension mechanism

sorry, the previous patch is the wrong one (it's for another bug)

[Kenneth Russell]

Comment on attachment 72953 [details]
revised patch: using the new extension mechanism

Looks good.

[Zhenyao Mo]

Committed r71367: <http://trac.webkit.org/changeset/71367>

[Zhenyao Mo]

Per discussion with [e-mail], we won't fix the enum mapping in chrome command buffer side; instead, we do it in WebGLRenderingContext.  See https://bugs.webkit.org/show_bug.cgi?id=49020.