Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1412613004:
Set attachment for bound framebuffer in FramebufferTextureLayer (Closed)
DescriptionSet attachment for bound framebuffer in FramebufferTextureLayer
BUG=546411
Committed: https://crrev.com/3071b5391379d2abe2f6890989b2c1dfebbc0d2b
Cr-Commit-Position: refs/heads/master@{#357068}
Patch Set 1 #
Total comments: 10
Patch Set 2 : more validations and fix gpu_unittests #
Total comments: 15
Patch Set 3 : fix comments#11 #Patch Set 4 : Add zero texure support and fix conformance test #
Total comments: 2
Patch Set 5 : fix WebGLTextureAttachment::attach #Messages
Total messages: 33 (3 generated)
qiankun.miao@intel.com changed reviewers: + bajones@chromium.org, kbr@chromium.org, zmo@chromium.org
PTAL. I uploaded this CL to fix the bug. I think more validations should be added for FramebufferTextureLayer. I will add them later.
On 2015/10/26 10:21:24, qiankun wrote: > PTAL. I uploaded this CL to fix the bug. I think more validations should be > added for FramebufferTextureLayer. I will add them later. I will fix gpu_unittests first, and a conformance test will be added.
On 2015/10/26 15:08:50, qiankun wrote: > I will fix gpu_unittests first, and a conformance test will be added. Code looks good so far, I'll do another review pass when the test is in place. Thanks!
Thanks for fixing this. Let's get this in ASAP. LGTM once all the tests are passing. I'm hoping that the existing dEQP tests like deqp/functional/gles3/fbocolorbuffer.html will cover this functionality. https://codereview.chromium.org/1412613004/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp (right): https://codereview.chromium.org/1412613004/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:267: preserveObjectWrapper(scriptState, framebufferBinding, "texture3d", attachment, texture); I made a mistake in the original patch adding these preserveObjectWrapper calls. framebufferRenderbuffer, framebufferTexture2D, and framebufferTextureLayer should all reference the same string. Could you please change "texture3d" to "attachment" to follow https://codereview.chromium.org/1415873007/ ? Thanks.
https://codereview.chromium.org/1412613004/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1412613004/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder.cc:6448: // TODO(zmo): Unsafe ES3 API, missing states update. Now this is no longer valid with the changes here, but please add a TODO for "adding full validation" https://codereview.chromium.org/1412613004/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder.cc:6469: GLenum error = LOCAL_PEEK_GL_ERROR("glFramebufferTextureLayer"); This is incorrect. You need to check error before the GL call to make sure no error from previous GL calls leak into here. See LOCAL_COPY_REAL_GL_ERRORS_TO_WRAPPER https://codereview.chromium.org/1412613004/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder.cc:6471: framebuffer->AttachTexture(attachment, texture_ref, You should add a AttachTextureLayer function and use it here. Use AttachTexture is just wrong. https://codereview.chromium.org/1412613004/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp (right): https://codereview.chromium.org/1412613004/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:265: framebufferBinding->setAttachmentForBoundFramebuffer(target, attachment, textarget, texture, level); You miss validation of target, attachment, texture target, level, layer... So just caching the state here is problematic if such params are invalid. Also, in setAttachmentForBoundFramebuffer needs to incorporate layer param in it for 3D textures.
I uploaded a new CL to fix gpu_unittests and some review comments. Conformance test isn't ready for review now. I will submit it for review when it's finished. https://codereview.chromium.org/1412613004/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1412613004/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder.cc:6448: // TODO(zmo): Unsafe ES3 API, missing states update. On 2015/10/26 21:50:33, Zhenyao Mo wrote: > Now this is no longer valid with the changes here, but please add a TODO for > "adding full validation" Done. https://codereview.chromium.org/1412613004/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder.cc:6469: GLenum error = LOCAL_PEEK_GL_ERROR("glFramebufferTextureLayer"); On 2015/10/26 21:50:33, Zhenyao Mo wrote: > This is incorrect. You need to check error before the GL call to make sure no > error from previous GL calls leak into here. See > LOCAL_COPY_REAL_GL_ERRORS_TO_WRAPPER Done. https://codereview.chromium.org/1412613004/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder.cc:6471: framebuffer->AttachTexture(attachment, texture_ref, On 2015/10/26 21:50:33, Zhenyao Mo wrote: > You should add a AttachTextureLayer function and use it here. Use AttachTexture > is just wrong. Framebuffer::AttachTextureLayer is added. What API should I implement for TextureAttachment? https://codereview.chromium.org/1412613004/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp (right): https://codereview.chromium.org/1412613004/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:265: framebufferBinding->setAttachmentForBoundFramebuffer(target, attachment, textarget, texture, level); On 2015/10/26 21:50:33, Zhenyao Mo wrote: > You miss validation of target, attachment, texture target, level, layer... > So just caching the state here is problematic if such params are invalid. > > Also, in setAttachmentForBoundFramebuffer needs to incorporate layer param in it > for 3D textures. More validations were added. Incorporated layer param in setAttachmentForBoundFramebuffer. https://codereview.chromium.org/1412613004/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:267: preserveObjectWrapper(scriptState, framebufferBinding, "texture3d", attachment, texture); On 2015/10/26 21:23:09, Ken Russell wrote: > I made a mistake in the original patch adding these preserveObjectWrapper calls. > framebufferRenderbuffer, framebufferTexture2D, and framebufferTextureLayer > should all reference the same string. Could you please change "texture3d" to > "attachment" to follow https://codereview.chromium.org/1415873007/ ? Thanks. Done.
Still looks good; thanks for improving the correctness, but zmo@ should review. One general concern about WebGLFramebuffer.cpp that doesn't need to be addressed in this CL. https://codereview.chromium.org/1412613004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLFramebuffer.cpp (right): https://codereview.chromium.org/1412613004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLFramebuffer.cpp:232: context->framebufferTextureLayer(target, attachment, object, m_level, m_layer); The logic here has become really confusing. I am highly dubious about the correctness of the two overloads of "removeAttachmentFromBoundFramebuffer". It looks to me like during calls like framebufferRenderbuffer and framebufferTexture2D that duplicate work, at least, is being done -- two calls to WebGraphicsContext3D::framebufferRenderbuffer, one from WebGLRenderingContextBase::framebufferRenderbuffer and one from WebGLRenderbufferAttachment::attach. zmo@, do you remember the motivation for adding this code?
https://codereview.chromium.org/1412613004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp (right): https://codereview.chromium.org/1412613004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:249: bool WebGL2RenderingContextBase::validateTextureLayer(const char* functionName, GLenum target, GLint layer) Function name maybe validateTexFuncLayer()? to be consistent with validateTexFuncLevel(). It's better to use texTarget instead of target, less confusing. https://codereview.chromium.org/1412613004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:288: synthesizeGLError(GL_INVALID_VALUE, "framebufferTextureLayer", "invalid texture target"); This should be INVALID_OPERATION. Message should be "invalid texture type". https://codereview.chromium.org/1412613004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:291: if (!validateTexFuncLevel("framebufferTextureLayer", textarget, level)) You need to update validateTexFuncLevel() (in turn WebGLRenderingContextBase::getMaxTextureLevelForTarget) to handle 3D textures. https://codereview.chromium.org/1412613004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLFramebuffer.cpp (right): https://codereview.chromium.org/1412613004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLFramebuffer.cpp:241: context->framebufferTextureLayer(target, attachment, 0, m_level, m_layer); Here I think you need to handle the same DEPTH_STENCIL_ATTACHMENT situation.
Thanks for review. https://codereview.chromium.org/1412613004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp (right): https://codereview.chromium.org/1412613004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:249: bool WebGL2RenderingContextBase::validateTextureLayer(const char* functionName, GLenum target, GLint layer) On 2015/10/27 21:21:31, Zhenyao Mo wrote: > Function name maybe validateTexFuncLayer()? to be consistent with > validateTexFuncLevel(). > It's better to use texTarget instead of target, less confusing. Ok, I will fix this. https://codereview.chromium.org/1412613004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:288: synthesizeGLError(GL_INVALID_VALUE, "framebufferTextureLayer", "invalid texture target"); On 2015/10/27 21:21:31, Zhenyao Mo wrote: > This should be INVALID_OPERATION. > Message should be "invalid texture type". I will fix this. https://codereview.chromium.org/1412613004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:291: if (!validateTexFuncLevel("framebufferTextureLayer", textarget, level)) On 2015/10/27 21:21:31, Zhenyao Mo wrote: > You need to update validateTexFuncLevel() (in turn > WebGLRenderingContextBase::getMaxTextureLevelForTarget) to handle 3D textures. WebGL2RenderingContextBase::getMaxTextureLevelForTarget is already existed to handle 3D textures. https://codereview.chromium.org/1412613004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:2880: GLint WebGL2RenderingContextBase::getMaxTextureLevelForTarget(GLenum target) It overrides the implementation in WebGLRenderingContextBase to handle 3D texture. https://codereview.chromium.org/1412613004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLFramebuffer.cpp (right): https://codereview.chromium.org/1412613004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLFramebuffer.cpp:241: context->framebufferTextureLayer(target, attachment, 0, m_level, m_layer); On 2015/10/27 21:21:31, Zhenyao Mo wrote: > Here I think you need to handle the same DEPTH_STENCIL_ATTACHMENT situation. DEPTH_STENCIL_ATTACHMENT is supported in ES 3. Do we still need to handle it with GL_DEPTH_ATTACHMENT and the GL_STENCIL_ATTACHMENT attachment? I intend to submit a update to fix this in another CL.
https://codereview.chromium.org/1412613004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp (right): https://codereview.chromium.org/1412613004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:291: if (!validateTexFuncLevel("framebufferTextureLayer", textarget, level)) On 2015/10/27 23:25:27, qiankun wrote: > On 2015/10/27 21:21:31, Zhenyao Mo wrote: > > You need to update validateTexFuncLevel() (in turn > > WebGLRenderingContextBase::getMaxTextureLevelForTarget) to handle 3D textures. > > WebGL2RenderingContextBase::getMaxTextureLevelForTarget is already existed to > handle 3D textures. You are right. Code search points me to the WebGL 1 version, and I didn't realize it's been overloaded in WebGL 2. https://codereview.chromium.org/1412613004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLFramebuffer.cpp (right): https://codereview.chromium.org/1412613004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLFramebuffer.cpp:241: context->framebufferTextureLayer(target, attachment, 0, m_level, m_layer); On 2015/10/27 23:25:27, qiankun wrote: > On 2015/10/27 21:21:31, Zhenyao Mo wrote: > > Here I think you need to handle the same DEPTH_STENCIL_ATTACHMENT situation. > > DEPTH_STENCIL_ATTACHMENT is supported in ES 3. Do we still need to handle it > with GL_DEPTH_ATTACHMENT and the GL_STENCIL_ATTACHMENT attachment? I intend to > submit a update to fix this in another CL. OK, then could you please add a comment that DEPTH_STENCIL_ATTACHMENT is valid in ES3?
https://codereview.chromium.org/1412613004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp (right): https://codereview.chromium.org/1412613004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:249: bool WebGL2RenderingContextBase::validateTextureLayer(const char* functionName, GLenum target, GLint layer) On 2015/10/27 23:25:27, qiankun wrote: > On 2015/10/27 21:21:31, Zhenyao Mo wrote: > > Function name maybe validateTexFuncLayer()? to be consistent with > > validateTexFuncLevel(). > > It's better to use texTarget instead of target, less confusing. > > Ok, I will fix this. Done. https://codereview.chromium.org/1412613004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:288: synthesizeGLError(GL_INVALID_VALUE, "framebufferTextureLayer", "invalid texture target"); On 2015/10/27 21:21:31, Zhenyao Mo wrote: > This should be INVALID_OPERATION. > Message should be "invalid texture type". Done. https://codereview.chromium.org/1412613004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLFramebuffer.cpp (right): https://codereview.chromium.org/1412613004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLFramebuffer.cpp:241: context->framebufferTextureLayer(target, attachment, 0, m_level, m_layer); On 2015/10/27 23:40:28, Zhenyao Mo wrote: > On 2015/10/27 23:25:27, qiankun wrote: > > On 2015/10/27 21:21:31, Zhenyao Mo wrote: > > > Here I think you need to handle the same DEPTH_STENCIL_ATTACHMENT situation. > > > > DEPTH_STENCIL_ATTACHMENT is supported in ES 3. Do we still need to handle it > > with GL_DEPTH_ATTACHMENT and the GL_STENCIL_ATTACHMENT attachment? I intend to > > submit a update to fix this in another CL. > > OK, then could you please add a comment that DEPTH_STENCIL_ATTACHMENT is valid > in ES3? Done.
LGTM
LGTM again too. Thanks Qiankun for this contribution and Mo for carefully reviewing this.
Thanks all for review. I updated the CL to support zero texture and fix conformance test at: https://github.com/KhronosGroup/WebGL/pull/1288 . Would you mind to review it again?
LGTM again Thanks for the test. Great that it caught a bug we failed to realize before.
Still LGTM if this makes more tests pass, but a comment / concern. https://codereview.chromium.org/1412613004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLFramebuffer.cpp (right): https://codereview.chromium.org/1412613004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLFramebuffer.cpp:241: if (m_target == GL_TEXTURE_3D || m_target == GL_TEXTURE_2D_ARRAY) { This changed to test "m_target" instead of "target", since "target" will be GL_FRAMEBUFFER (from e.g. WebGLRenderingContextBase::framebufferTexture2D -> WebGLFramebuffer::setAttachmentForBoundFramebuffer -> WebGLAttachment::unattach). WebGLTextureAttachment::attach, above, wasn't changed; why not? I have a feeling that the WebGLAttachment::attach and unattach methods are only used for depth/stencil emulation in WebGL 1.0. Note that the *only* caller of WebGLAttachment::attach is WebGLFramebuffer::removeAttachmentFromBoundFramebuffer(GLenum target, GLenum attachment) and it only passes the depth/stencil arguments. I still have doubts about the correctness of and need for these WebGLAttachment::attach and unattach methods, and think it may be worth a TODO to rethink them.
https://codereview.chromium.org/1412613004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLFramebuffer.cpp (right): https://codereview.chromium.org/1412613004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLFramebuffer.cpp:241: if (m_target == GL_TEXTURE_3D || m_target == GL_TEXTURE_2D_ARRAY) { On 2015/10/28 23:48:32, Ken Russell wrote: > This changed to test "m_target" instead of "target", since "target" will be > GL_FRAMEBUFFER (from e.g. WebGLRenderingContextBase::framebufferTexture2D -> > WebGLFramebuffer::setAttachmentForBoundFramebuffer -> > WebGLAttachment::unattach). WebGLTextureAttachment::attach, above, wasn't > changed; why not? > Done. > I have a feeling that the WebGLAttachment::attach and unattach methods are only > used for depth/stencil emulation in WebGL 1.0. Note that the *only* caller of > WebGLAttachment::attach is > WebGLFramebuffer::removeAttachmentFromBoundFramebuffer(GLenum target, GLenum > attachment) and it only passes the depth/stencil arguments. I still have doubts > about the correctness of and need for these WebGLAttachment::attach and unattach > methods, and think it may be worth a TODO to rethink them. I will check the attach path. But the unattach is used when deleting texture. #0 blink::(anonymous namespace)::WebGLTextureAttachment::unattach (this=0x2c93eae81ad0, context=0x2228adcb43a0, target=36160, attachment=36064) at ../../third_ party/WebKit/Source/modules/webgl/WebGLFramebuffer.cpp:241 #1 0x00007f7d15c7b779 in blink::WebGLFramebuffer::removeAttachmentFromBoundFramebuffer (this=0x17eaa7b62d98, target=36160, attachment=0x17eaa7b62d28) at ../../ third_party/WebKit/Source/modules/webgl/WebGLFramebuffer.cpp:496 #2 0x00007f7d15c9a7a2 in blink::WebGLRenderingContextBase::deleteTexture (this=0x17d59f83c010, texture=0x17eaa7b62d28) at ../../third_party/WebKit/Source/modul es/webgl/WebGLRenderingContextBase.cpp:2183
On 2015/10/28 23:48:32, Ken Russell wrote: > Still LGTM if this makes more tests pass, but a comment / concern. > > https://codereview.chromium.org/1412613004/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/modules/webgl/WebGLFramebuffer.cpp (right): > > https://codereview.chromium.org/1412613004/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/webgl/WebGLFramebuffer.cpp:241: if (m_target > == GL_TEXTURE_3D || m_target == GL_TEXTURE_2D_ARRAY) { > This changed to test "m_target" instead of "target", since "target" will be > GL_FRAMEBUFFER (from e.g. WebGLRenderingContextBase::framebufferTexture2D -> > WebGLFramebuffer::setAttachmentForBoundFramebuffer -> > WebGLAttachment::unattach). WebGLTextureAttachment::attach, above, wasn't > changed; why not? > > I have a feeling that the WebGLAttachment::attach and unattach methods are only > used for depth/stencil emulation in WebGL 1.0. Note that the *only* caller of > WebGLAttachment::attach is > WebGLFramebuffer::removeAttachmentFromBoundFramebuffer(GLenum target, GLenum > attachment) and it only passes the depth/stencil arguments. I still have doubts > about the correctness of and need for these WebGLAttachment::attach and unattach > methods, and think it may be worth a TODO to rethink them. I double checked that attach method is only used by WebGLFramebuffer::removeAttachmentFromBoundFramebuffer which passes depth/stencil attachment to attach. I am not sure if current implementation is correct. Maybe we only need to call these attaches only when after attaching zero renderbuffer with WebGLRenderingContextBase::framebufferRenderbuffer and zero texture with WebGLRenderingContextBase::framebufferTexture2D. I will write a test to check this. The original behavior is introduced in https://bugs.webkit.org/show_bug.cgi?id=88697 and change to current behavior in https://bugs.webkit.org/show_bug.cgi?id=89387 .
In ES2, we don't have DEPTH_STENCIL_ATTACHMENT, so we emulate using DEPTH_ATTACHMENT and STENCIL_ATTACHMENT. In ES3, this emulation is still the correct behavior, although unnecessary because now DEPTH_STENCIL_ATTACHMENT is valid.
On 2015/10/29 20:30:34, Zhenyao Mo wrote: > In ES2, we don't have DEPTH_STENCIL_ATTACHMENT, so we emulate using > DEPTH_ATTACHMENT and STENCIL_ATTACHMENT. In ES3, this emulation is still the > correct behavior, although unnecessary because now DEPTH_STENCIL_ATTACHMENT is > valid. WebGL 2.0 spec says "4.1.5 Framebuffer Object Attachments In WebGL 1, DEPTH_STENCIL_ATTACHMENT is an alternative attachment point other than DEPTH_ATTACHMENT and STENCIL_ATTACHMENT. In WebGL 2, however, DEPTH_STENCIL_ATTACHMENT is considered an alias for DEPTH_ATTACHMENT + STENCIL_ATTACHMENT, i.e., the same image is attached to both DEPTH_ATTACHMENT and STENCIL_ATTACHMENT, overwriting the original images attached to the two attachment points." Do you know what's the reason DEPTH_STENCIL_ATTACHMENT is considered an alias for DEPTH_ATTACHMENT + STENCIL_ATTACHMENT in WebGl 2.0 though ES3 supports DEPTH_STENCIL_ATTACHMENT?
On 2015/10/29 23:51:35, qiankun wrote: > On 2015/10/29 20:30:34, Zhenyao Mo wrote: > > In ES2, we don't have DEPTH_STENCIL_ATTACHMENT, so we emulate using > > DEPTH_ATTACHMENT and STENCIL_ATTACHMENT. In ES3, this emulation is still the > > correct behavior, although unnecessary because now DEPTH_STENCIL_ATTACHMENT is > > valid. > > WebGL 2.0 spec says "4.1.5 Framebuffer Object Attachments > > In WebGL 1, DEPTH_STENCIL_ATTACHMENT is an alternative attachment point other > than DEPTH_ATTACHMENT and STENCIL_ATTACHMENT. In WebGL 2, however, > DEPTH_STENCIL_ATTACHMENT is considered an alias for DEPTH_ATTACHMENT + > STENCIL_ATTACHMENT, i.e., the same image is attached to both DEPTH_ATTACHMENT > and STENCIL_ATTACHMENT, overwriting the original images attached to the two > attachment points." > > Do you know what's the reason DEPTH_STENCIL_ATTACHMENT is considered an alias > for DEPTH_ATTACHMENT + STENCIL_ATTACHMENT in WebGl 2.0 though ES3 supports > DEPTH_STENCIL_ATTACHMENT? I don't know why the ES3 working group decides this, but I do feel this is a much better solution than the WebGL1's treating DEPTH_STENCIL_ATTACHMENT as something independent from DEPTH_ATTACHMENT and STENCIL_ATTACHMENT.
On 2015/10/29 23:54:28, Zhenyao Mo wrote: > On 2015/10/29 23:51:35, qiankun wrote: > > On 2015/10/29 20:30:34, Zhenyao Mo wrote: > > > In ES2, we don't have DEPTH_STENCIL_ATTACHMENT, so we emulate using > > > DEPTH_ATTACHMENT and STENCIL_ATTACHMENT. In ES3, this emulation is still > the > > > correct behavior, although unnecessary because now DEPTH_STENCIL_ATTACHMENT > is > > > valid. > > > > WebGL 2.0 spec says "4.1.5 Framebuffer Object Attachments > > > > In WebGL 1, DEPTH_STENCIL_ATTACHMENT is an alternative attachment point other > > than DEPTH_ATTACHMENT and STENCIL_ATTACHMENT. In WebGL 2, however, > > DEPTH_STENCIL_ATTACHMENT is considered an alias for DEPTH_ATTACHMENT + > > STENCIL_ATTACHMENT, i.e., the same image is attached to both DEPTH_ATTACHMENT > > and STENCIL_ATTACHMENT, overwriting the original images attached to the two > > attachment points." > > > > Do you know what's the reason DEPTH_STENCIL_ATTACHMENT is considered an alias > > for DEPTH_ATTACHMENT + STENCIL_ATTACHMENT in WebGl 2.0 though ES3 supports > > DEPTH_STENCIL_ATTACHMENT? > > I don't know why the ES3 working group decides this, but I do feel this is a > much better solution than the WebGL1's treating DEPTH_STENCIL_ATTACHMENT as > something independent from DEPTH_ATTACHMENT and STENCIL_ATTACHMENT. So should we consider to use DEPTH_ATTACHMENT and STENCIL_ATTACHMENT to emulate DEPTH_STENCIL_ATTACHMENT WebGL2RenderingContextBase::framebufferTextureLayer as we do in WebGLRenderingContextBase::framebufferTexture2D?
On 2015/10/30 00:04:52, qiankun wrote: > On 2015/10/29 23:54:28, Zhenyao Mo wrote: > > On 2015/10/29 23:51:35, qiankun wrote: > > > On 2015/10/29 20:30:34, Zhenyao Mo wrote: > > > > In ES2, we don't have DEPTH_STENCIL_ATTACHMENT, so we emulate using > > > > DEPTH_ATTACHMENT and STENCIL_ATTACHMENT. In ES3, this emulation is still > > the > > > > correct behavior, although unnecessary because now > DEPTH_STENCIL_ATTACHMENT > > is > > > > valid. > > > > > > WebGL 2.0 spec says "4.1.5 Framebuffer Object Attachments > > > > > > In WebGL 1, DEPTH_STENCIL_ATTACHMENT is an alternative attachment point > other > > > than DEPTH_ATTACHMENT and STENCIL_ATTACHMENT. In WebGL 2, however, > > > DEPTH_STENCIL_ATTACHMENT is considered an alias for DEPTH_ATTACHMENT + > > > STENCIL_ATTACHMENT, i.e., the same image is attached to both > DEPTH_ATTACHMENT > > > and STENCIL_ATTACHMENT, overwriting the original images attached to the two > > > attachment points." > > > > > > Do you know what's the reason DEPTH_STENCIL_ATTACHMENT is considered an > alias > > > for DEPTH_ATTACHMENT + STENCIL_ATTACHMENT in WebGl 2.0 though ES3 supports > > > DEPTH_STENCIL_ATTACHMENT? > > > > I don't know why the ES3 working group decides this, but I do feel this is a > > much better solution than the WebGL1's treating DEPTH_STENCIL_ATTACHMENT as > > something independent from DEPTH_ATTACHMENT and STENCIL_ATTACHMENT. > > So should we consider to use DEPTH_ATTACHMENT and STENCIL_ATTACHMENT to emulate > DEPTH_STENCIL_ATTACHMENT WebGL2RenderingContextBase::framebufferTextureLayer as > we do in WebGLRenderingContextBase::framebufferTexture2D? No I don't think that's necessary. I think you can land the CL as is with a comment here with something like "Clean up DEPTH_STENCIL_ATTACHMENT situation"
On 2015/10/30 00:14:19, Zhenyao Mo wrote: > On 2015/10/30 00:04:52, qiankun wrote: > > On 2015/10/29 23:54:28, Zhenyao Mo wrote: > > > On 2015/10/29 23:51:35, qiankun wrote: > > > > On 2015/10/29 20:30:34, Zhenyao Mo wrote: > > > > > In ES2, we don't have DEPTH_STENCIL_ATTACHMENT, so we emulate using > > > > > DEPTH_ATTACHMENT and STENCIL_ATTACHMENT. In ES3, this emulation is > still > > > the > > > > > correct behavior, although unnecessary because now > > DEPTH_STENCIL_ATTACHMENT > > > is > > > > > valid. > > > > > > > > WebGL 2.0 spec says "4.1.5 Framebuffer Object Attachments > > > > > > > > In WebGL 1, DEPTH_STENCIL_ATTACHMENT is an alternative attachment point > > other > > > > than DEPTH_ATTACHMENT and STENCIL_ATTACHMENT. In WebGL 2, however, > > > > DEPTH_STENCIL_ATTACHMENT is considered an alias for DEPTH_ATTACHMENT + > > > > STENCIL_ATTACHMENT, i.e., the same image is attached to both > > DEPTH_ATTACHMENT > > > > and STENCIL_ATTACHMENT, overwriting the original images attached to the > two > > > > attachment points." > > > > > > > > Do you know what's the reason DEPTH_STENCIL_ATTACHMENT is considered an > > alias > > > > for DEPTH_ATTACHMENT + STENCIL_ATTACHMENT in WebGl 2.0 though ES3 supports > > > > DEPTH_STENCIL_ATTACHMENT? > > > > > > I don't know why the ES3 working group decides this, but I do feel this is a > > > much better solution than the WebGL1's treating DEPTH_STENCIL_ATTACHMENT as > > > something independent from DEPTH_ATTACHMENT and STENCIL_ATTACHMENT. > > > > So should we consider to use DEPTH_ATTACHMENT and STENCIL_ATTACHMENT to > emulate > > DEPTH_STENCIL_ATTACHMENT WebGL2RenderingContextBase::framebufferTextureLayer > as > > we do in WebGLRenderingContextBase::framebufferTexture2D? > > No I don't think that's necessary. We will not comply the WebGL 2.0 spec then? I am confused. > I think you can land the CL as is with a > comment here with something like "Clean up DEPTH_STENCIL_ATTACHMENT situation" Clean up DEPTH_STENCIL_ATTACHMENT such as in WebGLRenderingContextBase::framebufferTexture2D, right?
The CQ bit was checked by qiankun.miao@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org, zmo@chromium.org Link to the patchset: https://codereview.chromium.org/1412613004/#ps80001 (title: "fix WebGLTextureAttachment::attach")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1412613004/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1412613004/80001
On 2015/10/30 00:27:18, qiankun wrote: > On 2015/10/30 00:14:19, Zhenyao Mo wrote: > > On 2015/10/30 00:04:52, qiankun wrote: > > > On 2015/10/29 23:54:28, Zhenyao Mo wrote: > > > > On 2015/10/29 23:51:35, qiankun wrote: > > > > > On 2015/10/29 20:30:34, Zhenyao Mo wrote: > > > > > > In ES2, we don't have DEPTH_STENCIL_ATTACHMENT, so we emulate using > > > > > > DEPTH_ATTACHMENT and STENCIL_ATTACHMENT. In ES3, this emulation is > > still > > > > the > > > > > > correct behavior, although unnecessary because now > > > DEPTH_STENCIL_ATTACHMENT > > > > is > > > > > > valid. > > > > > > > > > > WebGL 2.0 spec says "4.1.5 Framebuffer Object Attachments > > > > > > > > > > In WebGL 1, DEPTH_STENCIL_ATTACHMENT is an alternative attachment point > > > other > > > > > than DEPTH_ATTACHMENT and STENCIL_ATTACHMENT. In WebGL 2, however, > > > > > DEPTH_STENCIL_ATTACHMENT is considered an alias for DEPTH_ATTACHMENT + > > > > > STENCIL_ATTACHMENT, i.e., the same image is attached to both > > > DEPTH_ATTACHMENT > > > > > and STENCIL_ATTACHMENT, overwriting the original images attached to the > > two > > > > > attachment points." > > > > > > > > > > Do you know what's the reason DEPTH_STENCIL_ATTACHMENT is considered an > > > alias > > > > > for DEPTH_ATTACHMENT + STENCIL_ATTACHMENT in WebGl 2.0 though ES3 > supports > > > > > DEPTH_STENCIL_ATTACHMENT? > > > > > > > > I don't know why the ES3 working group decides this, but I do feel this is > a > > > > much better solution than the WebGL1's treating DEPTH_STENCIL_ATTACHMENT > as > > > > something independent from DEPTH_ATTACHMENT and STENCIL_ATTACHMENT. > > > > > > So should we consider to use DEPTH_ATTACHMENT and STENCIL_ATTACHMENT to > > emulate > > > DEPTH_STENCIL_ATTACHMENT > WebGL2RenderingContextBase::framebufferTextureLayer > > as > > > we do in WebGLRenderingContextBase::framebufferTexture2D? > > > > No I don't think that's necessary. > We will not comply the WebGL 2.0 spec then? I am confused. > After re-reading the WebGL 2.0 spec and ES 3.0 spec, I think current implementation is OK. DEPTH_STENCIL_ATTACHMENT is an valid parameter for FramebufferTextureLayer(). So we can use it directly. Setting attachment to the value DEPTH_STENCIL_ATTACHMENT is a special case causing both the depth and stencil attachments of the framebuffer object to be set to renderbuffer in WebGL 2.0.
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/3071b5391379d2abe2f6890989b2c1dfebbc0d2b Cr-Commit-Position: refs/heads/master@{#357068}
Message was sent while issue was closed.
On 2015/10/30 09:31:30, commit-bot: I haz the power wrote: > Patchset 5 (id:??) landed as > https://crrev.com/3071b5391379d2abe2f6890989b2c1dfebbc0d2b > Cr-Commit-Position: refs/heads/master@{#357068} This CL seems to be causing the WebGL 2 CTS on AMD to fail: http://build.chromium.org/p/chromium.gpu.fyi/builders/Win7%20Release%20%28ATI... WebglConformance.deqp_functional_gles3_fbostencilbuffer Unsure if it'll fail on other configurations yet, so not sure if it's a bug with AMD or with this CL.
Message was sent while issue was closed.
On 2015/10/30 12:29:38, Jamie Madill wrote: > On 2015/10/30 09:31:30, commit-bot: I haz the power wrote: > > Patchset 5 (id:??) landed as > > https://crrev.com/3071b5391379d2abe2f6890989b2c1dfebbc0d2b > > Cr-Commit-Position: refs/heads/master@{#357068} > > This CL seems to be causing the WebGL 2 CTS on AMD to fail: > > http://build.chromium.org/p/chromium.gpu.fyi/builders/Win7%20Release%20%28ATI... > > WebglConformance.deqp_functional_gles3_fbostencilbuffer > > Unsure if it'll fail on other configurations yet, so not sure if it's a bug with > AMD or with this CL. The next build passed, probably the gpu sherrif should watch for flakiness but doesn't seem to be a trackable failure. |
