Bug 48891

Summary: renderbufferStorage(DEPTH_STENCIL) shouldn't generate an error
Product: WebKit Reporter: Zhenyao Mo <zmo>
Component: WebGLAssignee: Zhenyao Mo <zmo>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarrin, enne, kbr
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
patch
kbr: review-, zmo: commit-queue-
revised patch: using the new extension mechanism
zmo: commit-queue-
revised patch: using the new extension mechanism kbr: review+, zmo: commit-queue-

Zhenyao Mo
Reported 2010-11-02 17:55:52 PDT
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.
Attachments
patch (23.99 KB, patch)
2010-11-03 15:50 PDT, Zhenyao Mo
kbr: review-
zmo: commit-queue-
revised patch: using the new extension mechanism (3.70 KB, patch)
2010-11-04 09:57 PDT, Zhenyao Mo
zmo: commit-queue-
revised patch: using the new extension mechanism (24.23 KB, patch)
2010-11-04 09:59 PDT, Zhenyao Mo
kbr: review+
zmo: commit-queue-
Zhenyao Mo
Comment 1 2010-11-03 15:50:38 PDT
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 2 2010-11-03 16:59:36 PDT
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
Comment 3 2010-11-03 17:03:51 PDT
(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
Comment 4 2010-11-04 09:57:46 PDT
Created attachment 72952 [details] revised patch: using the new extension mechanism
Zhenyao Mo
Comment 5 2010-11-04 09:59:02 PDT
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 6 2010-11-04 16:51:42 PDT
Comment on attachment 72953 [details] revised patch: using the new extension mechanism Looks good.
Zhenyao Mo
Comment 7 2010-11-04 16:58:15 PDT
Zhenyao Mo
Comment 8 2010-11-04 18:09:22 PDT
Per discussion with gman@google.com, 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.
Note You need to log in before you can comment on or make changes to this bug.