Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1120953002:
WebGL 2: add read/write framebuffer binding points to related APIs (Closed)
DescriptionWebGL 2: add read/write framebuffer binding points to related APIs.
BUG=295792
NOTRY=true
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197696
Patch Set 1 : #
Total comments: 17
Patch Set 2 : addressed kbr@'s and bajones@'s feedback #
Total comments: 2
Patch Set 3 : addressed kbr@'s feedback #
Total comments: 28
Patch Set 4 : addressed zmo@'s and kbr@'s feedback #
Total comments: 25
Patch Set 5 : addressed zmo@'s feedback: ScopedDrawingBufferBinder is not correct for WebGL2 #Patch Set 6 : in DrawingBuffer::commit, we should bind to the single-sampled FB at the end, instead of the multis… #
Total comments: 5
Patch Set 7 : addressed zmo@'s feedback #Patch Set 8 : addressed zmo@'s feedback #Patch Set 9 : addressed zmo@'s feedback: WebGLFramebuffer::isBound need to be updated #
Total comments: 4
Patch Set 10 : addressed kbr@'s feedback: remove an argument that is not used #Messages
Total messages: 47 (12 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
yunchao.he@intel.com changed reviewers: + bajones@chromium.org, kbr@chromium.org, zmo@chromium.org
PTAL when you have time. Thanks a lot!
Generally looking good, a couple of nits and one bigger request. https://codereview.chromium.org/1120953002/diff/60001/Source/core/html/canvas... File Source/core/html/canvas/WebGL2RenderingContextBase.cpp (right): https://codereview.chromium.org/1120953002/diff/60001/Source/core/html/canvas... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:1366: // Have to call bindFramebuffer here to bind back to internal fbo. Comment doesn't match associated line. Should be // Have to call drawingBuffer->bind()...? https://codereview.chromium.org/1120953002/diff/60001/Source/core/html/canvas... File Source/core/html/canvas/WebGLFramebuffer.h (right): https://codereview.chromium.org/1120953002/diff/60001/Source/core/html/canvas... Source/core/html/canvas/WebGLFramebuffer.h:76: void removeAttachmentFromBoundFramebuffer(WebGLSharedObject*, GLenum target = GL_FRAMEBUFFER); I realize why you did it this way, because it cuts down on changes elsewhere, but the fact that target is frequently the first argument but sometimes the last is confusing and not very GL-like. I'd prefer to see target consistently be the first argument for this function and others in this patch. That will prevent use of default arguments, which makes the patch a bit more noisy, but I think the uniformity will be worth it long term. https://codereview.chromium.org/1120953002/diff/60001/Source/core/html/canvas... File Source/core/html/canvas/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1120953002/diff/60001/Source/core/html/canvas... Source/core/html/canvas/WebGLRenderingContextBase.cpp:2099: // TODO: Check whether this is correct Let's figure this out before landing the patch.
not lgtm I'm sorry, but there are many flaws in this CL and I didn't go through all of it in detail. Please rework the patch and provide new conformance tests that ensure the new code's correctness. https://codereview.chromium.org/1120953002/diff/60001/Source/core/html/canvas... File Source/core/html/canvas/WebGL2RenderingContextBase.cpp (right): https://codereview.chromium.org/1120953002/diff/60001/Source/core/html/canvas... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:1757: return colorFormat & readColorFormat; I don't understand what this code is attempting to do -- it isn't legal to do a bitwise "or" of constants like GL_RGB and GL_RGBA. The only situation where boundFramebufferColorFormat is called is to check the format of the read buffer (or read framebuffer) against the internal format of a texture for CopyTexImage2D and CopyTexSubImage3D operations. This shouldn't be looking at the draw framebuffer in WebGL 2.0 / ES 3.0. https://codereview.chromium.org/1120953002/diff/60001/Source/core/html/canvas... File Source/core/html/canvas/WebGLFramebuffer.h (right): https://codereview.chromium.org/1120953002/diff/60001/Source/core/html/canvas... Source/core/html/canvas/WebGLFramebuffer.h:132: AttachmentMap m_readAttachments; This is not how framebuffer attachments work. An individual framebuffer object does not have separate "draw" and "read" attachments; there are separate binding points for draw and read framebuffers in the GL state. Please study the OpenGL ES 3.0 spec in more depth. https://codereview.chromium.org/1120953002/diff/60001/Source/core/html/canvas... File Source/core/html/canvas/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1120953002/diff/60001/Source/core/html/canvas... Source/core/html/canvas/WebGLRenderingContextBase.cpp:925: drawingBuffer()->clearFramebuffers(clearMask, GL_DRAW_FRAMEBUFFER); Please add a new virtual function that returns the enum GL_FRAMEBUFFER in WebGLRenderingContextBase or GL_DRAW_FRAMEBUFFER in WebGL2RenderingContextBase. This would eliminate the multiple if-tests here. https://codereview.chromium.org/1120953002/diff/60001/Source/core/html/canvas... Source/core/html/canvas/WebGLRenderingContextBase.cpp:1591: ScopedDrawingBufferBinder readBinder(drawingBuffer(), getFramebufferBinding(GL_READ_FRAMEBUFFER), GL_READ_FRAMEBUFFER); This won't work. The scoping of readBinder ends outside the if-clause. Please rethink this. https://codereview.chromium.org/1120953002/diff/60001/Source/core/html/canvas... Source/core/html/canvas/WebGLRenderingContextBase.cpp:1637: ScopedDrawingBufferBinder readBinder(drawingBuffer(), getFramebufferBinding(GL_READ_FRAMEBUFFER), GL_READ_FRAMEBUFFER); Same problem here. https://codereview.chromium.org/1120953002/diff/60001/Source/platform/graphic... File Source/platform/graphics/gpu/DrawingBuffer.cpp (right): https://codereview.chromium.org/1120953002/diff/60001/Source/platform/graphic... Source/platform/graphics/gpu/DrawingBuffer.cpp:713: m_context->bindFramebuffer(target, m_fbo); Let's add an ASSERT that target == GL_FRAMEBUFFER || target == GL_DRAW_FRAMEBUFFER. This code wouldn't make sense if the caller passed in GL_READ_FRAMEBUFFER.
Patchset #2 (id:80001) has been deleted
Thanks for your reviewing, kbr@ and bajones@. I am very sorry that I didn't respond to your feedback for a long time, for I have to prepare and attend BlinkOn conference. The code has been updated accordingly, PTAL. The corresponding conformance test is in preparation. https://codereview.chromium.org/1120953002/diff/60001/Source/core/html/canvas... File Source/core/html/canvas/WebGL2RenderingContextBase.cpp (right): https://codereview.chromium.org/1120953002/diff/60001/Source/core/html/canvas... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:1366: // Have to call bindFramebuffer here to bind back to internal fbo. On 2015/05/06 17:47:53, bajones wrote: > Comment doesn't match associated line. Should be // Have to call > drawingBuffer->bind()...? Acknowledged. https://codereview.chromium.org/1120953002/diff/60001/Source/core/html/canvas... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:1757: return colorFormat & readColorFormat; On 2015/05/06 21:17:33, Ken Russell wrote: > I don't understand what this code is attempting to do -- it isn't legal to do a > bitwise "or" of constants like GL_RGB and GL_RGBA. > > The only situation where boundFramebufferColorFormat is called is to check the > format of the read buffer (or read framebuffer) against the internal format of a > texture for CopyTexImage2D and CopyTexSubImage3D operations. This shouldn't be > looking at the draw framebuffer in WebGL 2.0 / ES 3.0. I thought that both the read buffer and the draw buffer should contain the internal format of a texture for copyTexImage2D. So I do a bitwise 'and' when both read FB and draw FB are existed. Code has been updated to handle read Framebuffer only. https://codereview.chromium.org/1120953002/diff/60001/Source/core/html/canvas... File Source/core/html/canvas/WebGLFramebuffer.h (right): https://codereview.chromium.org/1120953002/diff/60001/Source/core/html/canvas... Source/core/html/canvas/WebGLFramebuffer.h:76: void removeAttachmentFromBoundFramebuffer(WebGLSharedObject*, GLenum target = GL_FRAMEBUFFER); On 2015/05/06 17:47:53, bajones wrote: > I realize why you did it this way, because it cuts down on changes elsewhere, > but the fact that target is frequently the first argument but sometimes the last > is confusing and not very GL-like. I'd prefer to see target consistently be the > first argument for this function and others in this patch. That will prevent use > of default arguments, which makes the patch a bit more noisy, but I think the > uniformity will be worth it long term. Acknowledged. https://codereview.chromium.org/1120953002/diff/60001/Source/core/html/canvas... Source/core/html/canvas/WebGLFramebuffer.h:132: AttachmentMap m_readAttachments; On 2015/05/06 21:17:33, Ken Russell wrote: > This is not how framebuffer attachments work. An individual framebuffer object > does not have separate "draw" and "read" attachments; there are separate binding > points for draw and read framebuffers in the GL state. Please study the OpenGL > ES 3.0 spec in more depth. Yes. I know that FBO doesn't have "read" and "draw" attachments. The problem is that I give a bad name to this parameter. I have renamed it to m_attachmentsForReadFramebuffer. https://codereview.chromium.org/1120953002/diff/60001/Source/core/html/canvas... File Source/core/html/canvas/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1120953002/diff/60001/Source/core/html/canvas... Source/core/html/canvas/WebGLRenderingContextBase.cpp:925: drawingBuffer()->clearFramebuffers(clearMask, GL_DRAW_FRAMEBUFFER); On 2015/05/06 21:17:33, Ken Russell wrote: > Please add a new virtual function that returns the enum GL_FRAMEBUFFER in > WebGLRenderingContextBase or GL_DRAW_FRAMEBUFFER in WebGL2RenderingContextBase. > This would eliminate the multiple if-tests here. Done. https://codereview.chromium.org/1120953002/diff/60001/Source/core/html/canvas... Source/core/html/canvas/WebGLRenderingContextBase.cpp:1591: ScopedDrawingBufferBinder readBinder(drawingBuffer(), getFramebufferBinding(GL_READ_FRAMEBUFFER), GL_READ_FRAMEBUFFER); On 2015/05/06 21:17:33, Ken Russell wrote: > This won't work. The scoping of readBinder ends outside the if-clause. Please > rethink this. I am very sorry, Ken. I didn't got what you said. What is the problem that the readBinder ends outside the if-clause? when the function come to the end, the destructor of readBinder will be executed for WebGL2RenderingContext. It seems to be correct. Could you kindly explain the potential error? Many thanks! https://codereview.chromium.org/1120953002/diff/60001/Source/core/html/canvas... Source/core/html/canvas/WebGLRenderingContextBase.cpp:2099: // TODO: Check whether this is correct On 2015/05/06 17:47:54, bajones wrote: > Let's figure this out before landing the patch. I think this is correct. So remove this TODO. https://codereview.chromium.org/1120953002/diff/60001/Source/platform/graphic... File Source/platform/graphics/gpu/DrawingBuffer.cpp (right): https://codereview.chromium.org/1120953002/diff/60001/Source/platform/graphic... Source/platform/graphics/gpu/DrawingBuffer.cpp:713: m_context->bindFramebuffer(target, m_fbo); On 2015/05/06 21:17:33, Ken Russell wrote: > Let's add an ASSERT that target == GL_FRAMEBUFFER || target == > GL_DRAW_FRAMEBUFFER. This code wouldn't make sense if the caller passed in > GL_READ_FRAMEBUFFER. Done.
https://codereview.chromium.org/1120953002/diff/100001/Source/core/html/canva... File Source/core/html/canvas/WebGLFramebuffer.h (right): https://codereview.chromium.org/1120953002/diff/100001/Source/core/html/canva... Source/core/html/canvas/WebGLFramebuffer.h:132: AttachmentMap m_attachmentsForReadFramebuffer; This still doesn't model the OpenGL state correctly. Framebuffers don't have attachments specifically for the "read" framebuffer binindg point. The way to make this patch more correct is to split m_framebufferBinding in WebGLRenderingContextBase into m_drawFramebufferBinding and m_readFramebufferBinding. I don't see any way around the need to teach the WebGL 1.0 context's code about these two binding points. WebGL 1.0 apps can only set both at the same time, but WebGL 2.0 apps can set each individually. The WebGLFramebuffer class should only have one set of attachments. It can potentially call back into the WebGLRenderingContextBase if needed in order to do work with the context's currently bound draw and read framebuffers. If I'm not explaining this clearly, or if you'd like me to take over this patch, please let me know.
Thanks for your explanation, Ken. I have updated the code to resolve the error. PTAL. Thanks! https://codereview.chromium.org/1120953002/diff/100001/Source/core/html/canva... File Source/core/html/canvas/WebGLFramebuffer.h (right): https://codereview.chromium.org/1120953002/diff/100001/Source/core/html/canva... Source/core/html/canvas/WebGLFramebuffer.h:132: AttachmentMap m_attachmentsForReadFramebuffer; On 2015/05/22 21:45:46, Ken Russell wrote: > This still doesn't model the OpenGL state correctly. Framebuffers don't have > attachments specifically for the "read" framebuffer binindg point. > > The way to make this patch more correct is to split m_framebufferBinding in > WebGLRenderingContextBase into m_drawFramebufferBinding and > m_readFramebufferBinding. I don't see any way around the need to teach the WebGL > 1.0 context's code about these two binding points. WebGL 1.0 apps can only set > both at the same time, but WebGL 2.0 apps can set each individually. The > WebGLFramebuffer class should only have one set of attachments. It can > potentially call back into the WebGLRenderingContextBase if needed in order to > do work with the context's currently bound draw and read framebuffers. > > If I'm not explaining this clearly, or if you'd like me to take over this patch, > please let me know. I got it. WebGLFramebuffer don't have attachments specific for read/draw framebuffer binding points. They just have attachments, don't need to know its binding points. It is its user's duty to keep the info of binding points on top of WebGLFramebuffer. I mean the infomation of draw or read binding point should be kept in WebGLRenderingContext: WebGLRenderingContext should have two WebGLFramebuffer for draw and read binding points separately in WebGL 2 rendering context. And this has already been done. We have m_framebufferBinding for WebGL 1.0 rendering context. m_readFramebufferBinding is added for WebGL 2.0 (m_framebufferBinding is degenerated to draw binding points only in this situation, just like GL_FRAMEBUFFER is equivalent to GL_DRAW_FRAMEBUFFER for most of WebGL2 APIs except bindFramebuffer). I just removed the m_attachmentsForReadFramebuffer. I think it is OK now.
On 2015/05/25 08:01:19, yunchao wrote: > Thanks for your explanation, Ken. I have updated the code to resolve the error. > PTAL. Thanks! > > https://codereview.chromium.org/1120953002/diff/100001/Source/core/html/canva... > File Source/core/html/canvas/WebGLFramebuffer.h (right): > > https://codereview.chromium.org/1120953002/diff/100001/Source/core/html/canva... > Source/core/html/canvas/WebGLFramebuffer.h:132: AttachmentMap > m_attachmentsForReadFramebuffer; > On 2015/05/22 21:45:46, Ken Russell wrote: > > This still doesn't model the OpenGL state correctly. Framebuffers don't have > > attachments specifically for the "read" framebuffer binindg point. > > > > The way to make this patch more correct is to split m_framebufferBinding in > > WebGLRenderingContextBase into m_drawFramebufferBinding and > > m_readFramebufferBinding. I don't see any way around the need to teach the > WebGL > > 1.0 context's code about these two binding points. WebGL 1.0 apps can only set > > both at the same time, but WebGL 2.0 apps can set each individually. The > > WebGLFramebuffer class should only have one set of attachments. It can > > potentially call back into the WebGLRenderingContextBase if needed in order to > > do work with the context's currently bound draw and read framebuffers. > > > > If I'm not explaining this clearly, or if you'd like me to take over this > patch, > > please let me know. > > I got it. WebGLFramebuffer don't have attachments specific for read/draw > framebuffer binding points. They just have attachments, don't need to know its > binding points. It is its user's duty to keep the info of binding points on top > of WebGLFramebuffer. I mean the infomation of draw or read binding point should > be kept in WebGLRenderingContext: WebGLRenderingContext should have two > WebGLFramebuffer for draw and read binding points separately in WebGL 2 > rendering context. > > And this has already been done. We have m_framebufferBinding for WebGL 1.0 > rendering context. m_readFramebufferBinding is added for WebGL 2.0 > (m_framebufferBinding is degenerated to draw binding points only in this > situation, just like GL_FRAMEBUFFER is equivalent to GL_DRAW_FRAMEBUFFER for > most of WebGL2 APIs except bindFramebuffer). > > I just removed the m_attachmentsForReadFramebuffer. I think it is OK now. ping... @kbr, could you have a look?
There are a couple more issues in this CL, and I'm sorry, I'm debugging a P1 issue right now and don't have time to thoroughly re-review it. Mo, could you please carefully review this CL? https://codereview.chromium.org/1120953002/diff/120001/Source/core/html/canva... File Source/core/html/canvas/WebGLFramebuffer.cpp (right): https://codereview.chromium.org/1120953002/diff/120001/Source/core/html/canva... Source/core/html/canvas/WebGLFramebuffer.cpp:424: WebGLFramebuffer::WebGLAttachment* WebGLFramebuffer::getAttachment(GLenum target, GLenum attachment) const The "target" argument is not used here any more and should be removed. https://codereview.chromium.org/1120953002/diff/120001/Source/core/html/canva... Source/core/html/canvas/WebGLFramebuffer.cpp:490: GLenum WebGLFramebuffer::checkStatus(const char** reason, GLenum target) const The "target" argument is unused here and should be removed.
As kbr suggested, you really should incorporate the idea of separate DRAW/READ framebuffer in WebGLRenderingContextBase because it's all over the places. i.e., you should define m_drawFramebufferBinding and m_readFramebufferBinding instead of just m_framebufferBinding. In WebGL1, m_drawFramebufferBinding and m_readFramebufferBinding is always the same. It will be much better in design for this CL and future CLs. Please revise according to this idea. I am more than happy to review again after that. I also added a bunch of more detailed comments for DrawingBuffer based on the assumption of separate DRAW/READ framebuffer. https://codereview.chromium.org/1120953002/diff/120001/Source/core/html/canva... File Source/core/html/canvas/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1120953002/diff/120001/Source/core/html/canva... Source/core/html/canvas/WebGLRenderingContextBase.cpp:226: ScopedDrawingBufferBinder(DrawingBuffer* drawingBuffer, WebGLFramebuffer* framebufferBinding, GLenum target) No need for target. As I pointed out in DrawingBuffer, both commit and bind do not need to take a target arg. https://codereview.chromium.org/1120953002/diff/120001/Source/core/html/canva... Source/core/html/canvas/WebGLRenderingContextBase.cpp:878: GLenum WebGLRenderingContextBase::getDrawFramebufferTarget() I think this function is unnecessary. See comments where it's called. https://codereview.chromium.org/1120953002/diff/120001/Source/core/html/canva... Source/core/html/canvas/WebGLRenderingContextBase.cpp:928: drawingBuffer()->clearFramebuffers(target, clearMask); See DrawingBuffer, target isn't necessary. https://codereview.chromium.org/1120953002/diff/120001/Source/core/html/canva... Source/core/html/canvas/WebGLRenderingContextBase.cpp:932: webContext()->bindFramebuffer(target, objectOrZero(m_framebufferBinding.get())); I think you should just call DrawingBuffer's restoreFramebufferBindings() to do this, to avoid code duplication. https://codereview.chromium.org/1120953002/diff/120001/Source/core/html/canva... Source/core/html/canvas/WebGLRenderingContextBase.cpp:1038: if (isWebGL2OrHigher()) { Again, call DrawBuffer's restoreFramebufferBindings(). https://codereview.chromium.org/1120953002/diff/120001/Source/platform/graphi... File Source/platform/graphics/gpu/DrawingBuffer.cpp (right): https://codereview.chromium.org/1120953002/diff/120001/Source/platform/graphi... Source/platform/graphics/gpu/DrawingBuffer.cpp:709: void DrawingBuffer::clearFramebuffers(GLenum target, GLbitfield clearMask) No need for the target, the original code is correct. It's always correct to bind to GL_FRAMEBUFFER for clearing. We restore later both READ and DRAW binding points. https://codereview.chromium.org/1120953002/diff/120001/Source/platform/graphi... Source/platform/graphics/gpu/DrawingBuffer.cpp:787: void DrawingBuffer::commit(GLenum target) |target| is not needed. https://codereview.chromium.org/1120953002/diff/120001/Source/platform/graphi... Source/platform/graphics/gpu/DrawingBuffer.cpp:805: m_context->bindFramebuffer(target, m_fbo); Instead of calling this directly, you should call restoreFramebufferBindings(). https://codereview.chromium.org/1120953002/diff/120001/Source/platform/graphi... Source/platform/graphics/gpu/DrawingBuffer.cpp:809: void DrawingBuffer::restoreFramebufferBinding(GLenum target) It should not have a target, but restore both READ and DRAW bindings always. So maybe rename this to restoreFramebufferBindings(). If m_readFramebufferBinding and m_drawFramebufferBinding is the same, it could be just one call with GL_FRAMEBUFFER (always the case in ES2), or it needs to be restore separately sometimes in ES3 when the two bindings differ. https://codereview.chromium.org/1120953002/diff/120001/Source/platform/graphi... Source/platform/graphics/gpu/DrawingBuffer.cpp:826: m_context->bindFramebuffer(target, m_multisampleFBO ? m_multisampleFBO : m_fbo); I think we should get rid of the target in this function. If m_multisampleFBO isn't null, we should always bind m_multisampleFBO to DRAW_FRAMEBUFFER, and m_fbo to READ_FRAMEBUFFER; otherwise, we should always bind m_fbo to just FRAMEBUFFER. https://codereview.chromium.org/1120953002/diff/120001/Source/platform/graphi... File Source/platform/graphics/gpu/DrawingBuffer.h (right): https://codereview.chromium.org/1120953002/diff/120001/Source/platform/graphi... Source/platform/graphics/gpu/DrawingBuffer.h:126: void setFramebufferBinding(GLenum target, Platform3DObject fbo) This should be setFramebufferBinding(Platform3DObject draw_fbo, Platform3DObject read_fbo). https://codereview.chromium.org/1120953002/diff/120001/Source/platform/graphi... Source/platform/graphics/gpu/DrawingBuffer.h:235: Platform3DObject m_framebufferBinding; This should be m_drawFramebufferBinding, to be clearer. in ES2/WebGL1, read/draw should always be the same object.
Thanks for your reviewing, zmo@ and kbr@. I have updated the code accordingly. PTAL. Well, WebGL 1.0 just have GL_FRAMEBUFFER binding points, so I prefer not to separate DRAW/READ framebuffer in WebGLRenderingContext. m_framebufferBinding refers to the framebuffer object in WebGL 1.0, and degenerated to draw framebuffer only in WebGL 2.0. WDYT? In drawingbuffer, I renamed m_framebufferBinding to m_drawFramebufferBinding, because there are no WebGLDrawingBuffer and WebGL2DrawingBuffer respectively. If you still think that it is better to rename m_framebufferBinding to m_drawFramebufferBinding in WebGLRenderingContextBase, and move m_readFramebufferBinding to WebGLRenderingContextBase, I will do this tomorrow. https://codereview.chromium.org/1120953002/diff/120001/Source/core/html/canva... File Source/core/html/canvas/WebGLFramebuffer.cpp (right): https://codereview.chromium.org/1120953002/diff/120001/Source/core/html/canva... Source/core/html/canvas/WebGLFramebuffer.cpp:424: WebGLFramebuffer::WebGLAttachment* WebGLFramebuffer::getAttachment(GLenum target, GLenum attachment) const On 2015/06/04 02:45:33, Ken Russell wrote: > The "target" argument is not used here any more and should be removed. Acknowledged. https://codereview.chromium.org/1120953002/diff/120001/Source/core/html/canva... Source/core/html/canvas/WebGLFramebuffer.cpp:490: GLenum WebGLFramebuffer::checkStatus(const char** reason, GLenum target) const On 2015/06/04 02:45:33, Ken Russell wrote: > The "target" argument is unused here and should be removed. Acknowledged. https://codereview.chromium.org/1120953002/diff/120001/Source/core/html/canva... File Source/core/html/canvas/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1120953002/diff/120001/Source/core/html/canva... Source/core/html/canvas/WebGLRenderingContextBase.cpp:226: ScopedDrawingBufferBinder(DrawingBuffer* drawingBuffer, WebGLFramebuffer* framebufferBinding, GLenum target) On 2015/06/04 22:08:16, Zhenyao Mo wrote: > No need for target. As I pointed out in DrawingBuffer, both commit and bind do > not need to take a target arg. it is not appropriate to remove target from bind(). see the comments in DrawingBuffer https://codereview.chromium.org/1120953002/diff/120001/Source/core/html/canva... Source/core/html/canvas/WebGLRenderingContextBase.cpp:878: GLenum WebGLRenderingContextBase::getDrawFramebufferTarget() On 2015/06/04 22:08:16, Zhenyao Mo wrote: > I think this function is unnecessary. > > See comments where it's called. Acknowledged. https://codereview.chromium.org/1120953002/diff/120001/Source/core/html/canva... Source/core/html/canvas/WebGLRenderingContextBase.cpp:928: drawingBuffer()->clearFramebuffers(target, clearMask); On 2015/06/04 22:08:16, Zhenyao Mo wrote: > See DrawingBuffer, target isn't necessary. Acknowledged. https://codereview.chromium.org/1120953002/diff/120001/Source/core/html/canva... Source/core/html/canvas/WebGLRenderingContextBase.cpp:932: webContext()->bindFramebuffer(target, objectOrZero(m_framebufferBinding.get())); On 2015/06/04 22:08:16, Zhenyao Mo wrote: > I think you should just call DrawingBuffer's restoreFramebufferBindings() to do > this, to avoid code duplication. Acknowledged. https://codereview.chromium.org/1120953002/diff/120001/Source/core/html/canva... Source/core/html/canvas/WebGLRenderingContextBase.cpp:1038: if (isWebGL2OrHigher()) { On 2015/06/04 22:08:16, Zhenyao Mo wrote: > Again, call DrawBuffer's restoreFramebufferBindings(). Acknowledged. https://codereview.chromium.org/1120953002/diff/120001/Source/platform/graphi... File Source/platform/graphics/gpu/DrawingBuffer.cpp (right): https://codereview.chromium.org/1120953002/diff/120001/Source/platform/graphi... Source/platform/graphics/gpu/DrawingBuffer.cpp:709: void DrawingBuffer::clearFramebuffers(GLenum target, GLbitfield clearMask) On 2015/06/04 22:08:17, Zhenyao Mo wrote: > No need for the target, the original code is correct. It's always correct to > bind to GL_FRAMEBUFFER for clearing. We restore later both READ and DRAW > binding points. Acknowledged. https://codereview.chromium.org/1120953002/diff/120001/Source/platform/graphi... Source/platform/graphics/gpu/DrawingBuffer.cpp:787: void DrawingBuffer::commit(GLenum target) On 2015/06/04 22:08:17, Zhenyao Mo wrote: > |target| is not needed. Acknowledged. https://codereview.chromium.org/1120953002/diff/120001/Source/platform/graphi... Source/platform/graphics/gpu/DrawingBuffer.cpp:805: m_context->bindFramebuffer(target, m_fbo); On 2015/06/04 22:08:17, Zhenyao Mo wrote: > Instead of calling this directly, you should call restoreFramebufferBindings(). Acknowledged. https://codereview.chromium.org/1120953002/diff/120001/Source/platform/graphi... Source/platform/graphics/gpu/DrawingBuffer.cpp:809: void DrawingBuffer::restoreFramebufferBinding(GLenum target) On 2015/06/04 22:08:17, Zhenyao Mo wrote: > It should not have a target, but restore both READ and DRAW bindings always. So > maybe rename this to restoreFramebufferBindings(). > > If m_readFramebufferBinding and m_drawFramebufferBinding is the same, it could > be just one call with GL_FRAMEBUFFER (always the case in ES2), or it needs to be > restore separately sometimes in ES3 when the two bindings differ. Acknowledged. https://codereview.chromium.org/1120953002/diff/120001/Source/platform/graphi... Source/platform/graphics/gpu/DrawingBuffer.cpp:826: m_context->bindFramebuffer(target, m_multisampleFBO ? m_multisampleFBO : m_fbo); On 2015/06/04 22:08:17, Zhenyao Mo wrote: > I think we should get rid of the target in this function. If m_multisampleFBO > isn't null, we should always bind m_multisampleFBO to DRAW_FRAMEBUFFER, and > m_fbo to READ_FRAMEBUFFER; otherwise, we should always bind m_fbo to just > FRAMEBUFFER. In WebGL 1.0, there may have both m_multisampleFBO and m_fbo when render_to_texture is enabled for tiled gpu. But there is no DRAW or READ framebuffer binding point separately, framebuffer in WebGL 1.0 just have GL_FRAMEBUFFER binding point. So I prefer to keep target parameter. On the other hand, I think that it is a good idea to bind draw framebuffer to m_multisampleFBO if it exists, and bind read framebuffer to m_fbo against WebGL 2.0. https://codereview.chromium.org/1120953002/diff/120001/Source/platform/graphi... File Source/platform/graphics/gpu/DrawingBuffer.h (right): https://codereview.chromium.org/1120953002/diff/120001/Source/platform/graphi... Source/platform/graphics/gpu/DrawingBuffer.h:126: void setFramebufferBinding(GLenum target, Platform3DObject fbo) On 2015/06/04 22:08:17, Zhenyao Mo wrote: > This should be setFramebufferBinding(Platform3DObject draw_fbo, Platform3DObject > read_fbo). The call site may just set one binding point, it doesn't care about the status of the other binding point. So I think use target seems to be appropriate. https://codereview.chromium.org/1120953002/diff/120001/Source/platform/graphi... Source/platform/graphics/gpu/DrawingBuffer.h:235: Platform3DObject m_framebufferBinding; On 2015/06/04 22:08:17, Zhenyao Mo wrote: > This should be m_drawFramebufferBinding, to be clearer. in ES2/WebGL1, > read/draw should always be the same object. Done.
Much better, but still not 100% correct. https://codereview.chromium.org/1120953002/diff/140001/Source/core/html/canva... File Source/core/html/canvas/WebGL2RenderingContextBase.cpp (right): https://codereview.chromium.org/1120953002/diff/140001/Source/core/html/canva... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:1577: target = GL_READ_FRAMEBUFFER; As I mentioned in another place, no need for drawingBuffer::bind() if only REAd_FRAMEBUFFER is modified. https://codereview.chromium.org/1120953002/diff/140001/Source/core/html/canva... File Source/core/html/canvas/WebGLRenderingContextBase.cpp (left): https://codereview.chromium.org/1120953002/diff/140001/Source/core/html/canva... Source/core/html/canvas/WebGLRenderingContextBase.cpp:2063: case GL_DEPTH_ATTACHMENT: Why removing these two? https://codereview.chromium.org/1120953002/diff/140001/Source/core/html/canva... File Source/core/html/canvas/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1120953002/diff/140001/Source/core/html/canva... Source/core/html/canvas/WebGLRenderingContextBase.cpp:227: ScopedDrawingBufferBinder(DrawingBuffer* drawingBuffer, WebGLFramebuffer* framebufferBinding, GLenum target) That's why I suggest you have an idea of drawFramebufferBinding and readFramebufferBinding in the base class, even though they are just the same framebuffer in WebGL 1, at least you can always be explicit which one is intended. Otherwise it's very easy to get confused and make mistake. See below why. https://codereview.chromium.org/1120953002/diff/140001/Source/core/html/canva... Source/core/html/canvas/WebGLRenderingContextBase.cpp:233: if (!m_framebufferBinding && m_drawingBuffer) This should be readFramebufferBinding https://codereview.chromium.org/1120953002/diff/140001/Source/core/html/canva... Source/core/html/canvas/WebGLRenderingContextBase.cpp:240: if (!m_framebufferBinding && m_drawingBuffer) This actually should be drawFramebufferBinding. https://codereview.chromium.org/1120953002/diff/140001/Source/core/html/canva... Source/core/html/canvas/WebGLRenderingContextBase.cpp:1577: ScopedDrawingBufferBinder readBinder(drawingBuffer(), getFramebufferBinding(GL_READ_FRAMEBUFFER), GL_READ_FRAMEBUFFER); This is incorrect. readBinder is only alive with the if {} clause. Besides, as I mentioned in DrawingBuffer, there is no need to recover READ_FRAMEBUFFER for DrawingBuffer. https://codereview.chromium.org/1120953002/diff/140001/Source/core/html/canva... Source/core/html/canvas/WebGLRenderingContextBase.cpp:1623: ScopedDrawingBufferBinder readBinder(drawingBuffer(), getFramebufferBinding(GL_READ_FRAMEBUFFER), GL_READ_FRAMEBUFFER); Same here. https://codereview.chromium.org/1120953002/diff/140001/Source/core/html/canva... Source/core/html/canvas/WebGLRenderingContextBase.cpp:1744: // Have to call drawingBuffer()->bind() here to bind back to internal fbo. By the way, I just noticed, you should get rid of the default value for target in bind() and explicitly call with bind(GL_FRAMEBUFFER). There are only a few places. https://codereview.chromium.org/1120953002/diff/140001/Source/core/html/canva... Source/core/html/canvas/WebGLRenderingContextBase.cpp:3358: ScopedDrawingBufferBinder binder(drawingBuffer(), readFramebufferBinding, target); Again, I don't understand why we need the binder here. readPixels should not change any bindings. https://codereview.chromium.org/1120953002/diff/140001/Source/platform/graphi... File Source/platform/graphics/gpu/DrawingBuffer.cpp (right): https://codereview.chromium.org/1120953002/diff/140001/Source/platform/graphi... Source/platform/graphics/gpu/DrawingBuffer.cpp:840: bind(GL_READ_FRAMEBUFFER); As I explained below, this is unnecessary. https://codereview.chromium.org/1120953002/diff/140001/Source/platform/graphi... Source/platform/graphics/gpu/DrawingBuffer.cpp:852: if (target != GL_READ_FRAMEBUFFER) I think you misunderstood the situation here. I think you should ASSERT(target == GL_FRAMEBUFFER || target == GL_DRAW_FRAMEBUFFER) here because bind() is to get DrawingBuffer ready for draw. m_fbo should only be bound to READ_FRAMEBUFFER temporarily at blit time.
I don't think you necessarily have to define m_drawFramebufferBinding and m_readFramebufferBinding in the WebGLRenderingContextBase class. Personally I feel that will make things easier and clearer, but as far as you can get things correct, that's good enough. https://codereview.chromium.org/1120953002/diff/140001/Source/core/html/canva... File Source/core/html/canvas/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1120953002/diff/140001/Source/core/html/canva... Source/core/html/canvas/WebGLRenderingContextBase.cpp:3358: ScopedDrawingBufferBinder binder(drawingBuffer(), readFramebufferBinding, target); On 2015/06/09 20:29:51, Zhenyao Mo wrote: > Again, I don't understand why we need the binder here. readPixels should not > change any bindings. Never mind. Of course we need the binder for multisampled fbos. But I think the current setup for the binder is incorrect. See my comments in the binder class.
My apologies, I also got confused. So I spent some time rethinking this, please see my second round of comments. https://codereview.chromium.org/1120953002/diff/140001/Source/core/html/canva... File Source/core/html/canvas/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1120953002/diff/140001/Source/core/html/canva... Source/core/html/canvas/WebGLRenderingContextBase.cpp:240: if (!m_framebufferBinding && m_drawingBuffer) On 2015/06/09 20:29:50, Zhenyao Mo wrote: > This actually should be drawFramebufferBinding. This should also be readFramebufferBinding. Because ScopedDrawingBufferBinder is used for ReadPixels/CopyTexImage2D/CopySubImage2D in the situation of reading from a multisampled DrawingBuffer, so we need to blit to a single sampled buffer for reading, during which the bindings could be changed and need to be recovered. The below code where you have two ScopedDrawingBufferBinder doesn't seem correct. Instead, for WebGL 1, this Binder class is correct, but if you want it to work also for WebGL 2, you basically need to have readFramebufferBinding and drawFramebufferBinding as Binder class members, so you can recover them in the destructor. https://codereview.chromium.org/1120953002/diff/140001/Source/core/html/canva... Source/core/html/canvas/WebGLRenderingContextBase.cpp:1577: ScopedDrawingBufferBinder readBinder(drawingBuffer(), getFramebufferBinding(GL_READ_FRAMEBUFFER), GL_READ_FRAMEBUFFER); On 2015/06/09 20:29:50, Zhenyao Mo wrote: > This is incorrect. readBinder is only alive with the if {} clause. Besides, as > I mentioned in DrawingBuffer, there is no need to recover READ_FRAMEBUFFER for > DrawingBuffer. See my second comments in ScopedDrawingBufferBinder. If you want to handle WebGL2 case here, you should upgrade the ScopedDrawingBufferBinder to handle both READ and DRAW bindings all together in one Binder, not separately in two Binders. https://codereview.chromium.org/1120953002/diff/140001/Source/core/html/canva... Source/core/html/canvas/WebGLRenderingContextBase.cpp:1623: ScopedDrawingBufferBinder readBinder(drawingBuffer(), getFramebufferBinding(GL_READ_FRAMEBUFFER), GL_READ_FRAMEBUFFER); On 2015/06/09 20:29:50, Zhenyao Mo wrote: > Same here. See my second comments in ScopedDrawingBufferBinder. If you want to handle WebGL2 case here, you should upgrade the ScopedDrawingBufferBinder to handle both READ and DRAW bindings all together in one Binder, not separately in two Binders. https://codereview.chromium.org/1120953002/diff/140001/Source/platform/graphi... File Source/platform/graphics/gpu/DrawingBuffer.cpp (right): https://codereview.chromium.org/1120953002/diff/140001/Source/platform/graphi... Source/platform/graphics/gpu/DrawingBuffer.cpp:840: bind(GL_READ_FRAMEBUFFER); On 2015/06/09 20:29:51, Zhenyao Mo wrote: > As I explained below, this is unnecessary. I take it back. Your code is correct. https://codereview.chromium.org/1120953002/diff/140001/Source/platform/graphi... Source/platform/graphics/gpu/DrawingBuffer.cpp:852: if (target != GL_READ_FRAMEBUFFER) Never mind. Your code is correct.
Thank you for your careful reviewing, @zmo. The code has been updated accordingly. PTAL. https://codereview.chromium.org/1120953002/diff/140001/Source/core/html/canva... File Source/core/html/canvas/WebGL2RenderingContextBase.cpp (right): https://codereview.chromium.org/1120953002/diff/140001/Source/core/html/canva... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:1577: target = GL_READ_FRAMEBUFFER; On 2015/06/09 20:29:50, Zhenyao Mo wrote: > As I mentioned in another place, no need for drawingBuffer::bind() if only > REAd_FRAMEBUFFER is modified. seems that there is no problem. https://codereview.chromium.org/1120953002/diff/140001/Source/core/html/canva... File Source/core/html/canvas/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1120953002/diff/140001/Source/core/html/canva... Source/core/html/canvas/WebGLRenderingContextBase.cpp:227: ScopedDrawingBufferBinder(DrawingBuffer* drawingBuffer, WebGLFramebuffer* framebufferBinding, GLenum target) On 2015/06/09 20:29:51, Zhenyao Mo wrote: > That's why I suggest you have an idea of drawFramebufferBinding and > readFramebufferBinding in the base class, even though they are just the same > framebuffer in WebGL 1, at least you can always be explicit which one is > intended. Otherwise it's very easy to get confused and make mistake. > > See below why. I got it. But I think it is not a must. see the latest code. https://codereview.chromium.org/1120953002/diff/140001/Source/core/html/canva... Source/core/html/canvas/WebGLRenderingContextBase.cpp:233: if (!m_framebufferBinding && m_drawingBuffer) On 2015/06/09 20:29:51, Zhenyao Mo wrote: > This should be readFramebufferBinding Agree. I rename this variable to m_readFramebufferBinding to make it clear. https://codereview.chromium.org/1120953002/diff/140001/Source/core/html/canva... Source/core/html/canvas/WebGLRenderingContextBase.cpp:240: if (!m_framebufferBinding && m_drawingBuffer) On 2015/06/10 05:01:45, Zhenyao Mo wrote: > On 2015/06/09 20:29:50, Zhenyao Mo wrote: > > This actually should be drawFramebufferBinding. > > This should also be readFramebufferBinding. Because ScopedDrawingBufferBinder is > used for ReadPixels/CopyTexImage2D/CopySubImage2D in the situation of reading > from a multisampled DrawingBuffer, so we need to blit to a single sampled buffer > for reading, during which the bindings could be changed and need to be > recovered. The below code where you have two ScopedDrawingBufferBinder doesn't > seem correct. Instead, for WebGL 1, this Binder class is correct, but if you > want it to work also for WebGL 2, you basically need to have > readFramebufferBinding and drawFramebufferBinding as Binder class members, so > you can recover them in the destructor. To make it clear, I add a comment for the class ScopedDrawingBufferBinder. It is extracted from your comment above, :) https://codereview.chromium.org/1120953002/diff/140001/Source/core/html/canva... Source/core/html/canvas/WebGLRenderingContextBase.cpp:1577: ScopedDrawingBufferBinder readBinder(drawingBuffer(), getFramebufferBinding(GL_READ_FRAMEBUFFER), GL_READ_FRAMEBUFFER); On 2015/06/10 05:01:46, Zhenyao Mo wrote: > On 2015/06/09 20:29:50, Zhenyao Mo wrote: > > This is incorrect. readBinder is only alive with the if {} clause. Besides, > as > > I mentioned in DrawingBuffer, there is no need to recover READ_FRAMEBUFFER for > > DrawingBuffer. > > See my second comments in ScopedDrawingBufferBinder. If you want to handle > WebGL2 case here, you should upgrade the ScopedDrawingBufferBinder to handle > both READ and DRAW bindings all together in one Binder, not separately in two > Binders. I got it. The think the latest code set the correct binding point for Binder. https://codereview.chromium.org/1120953002/diff/140001/Source/core/html/canva... Source/core/html/canvas/WebGLRenderingContextBase.cpp:1623: ScopedDrawingBufferBinder readBinder(drawingBuffer(), getFramebufferBinding(GL_READ_FRAMEBUFFER), GL_READ_FRAMEBUFFER); On 2015/06/10 05:01:46, Zhenyao Mo wrote: > On 2015/06/09 20:29:50, Zhenyao Mo wrote: > > Same here. > > See my second comments in ScopedDrawingBufferBinder. If you want to handle > WebGL2 case here, you should upgrade the ScopedDrawingBufferBinder to handle > both READ and DRAW bindings all together in one Binder, not separately in two > Binders. I got it. The think the latest code set the correct binding point for Binder. https://codereview.chromium.org/1120953002/diff/140001/Source/core/html/canva... Source/core/html/canvas/WebGLRenderingContextBase.cpp:1744: // Have to call drawingBuffer()->bind() here to bind back to internal fbo. On 2015/06/09 20:29:50, Zhenyao Mo wrote: > By the way, I just noticed, you should get rid of the default value for target > in bind() and explicitly call with bind(GL_FRAMEBUFFER). There are only a few > places. Done. https://codereview.chromium.org/1120953002/diff/140001/Source/core/html/canva... Source/core/html/canvas/WebGLRenderingContextBase.cpp:3358: ScopedDrawingBufferBinder binder(drawingBuffer(), readFramebufferBinding, target); On 2015/06/09 20:33:40, Zhenyao Mo wrote: > On 2015/06/09 20:29:51, Zhenyao Mo wrote: > > Again, I don't understand why we need the binder here. readPixels should not > > change any bindings. > > Never mind. Of course we need the binder for multisampled fbos. But I think the > current setup for the binder is incorrect. See my comments in the binder class. I think there is no problem.
Hi, zmo@. I rethink of the patch to fix WebGL conformance test failures. Please take a look at the latest patch set. Thanks a lot! https://codereview.chromium.org/1120953002/diff/180001/Source/platform/graphi... File Source/platform/graphics/gpu/DrawingBuffer.cpp (right): https://codereview.chromium.org/1120953002/diff/180001/Source/platform/graphi... Source/platform/graphics/gpu/DrawingBuffer.cpp:817: m_context->bindFramebuffer(target, m_fbo); WebGL conformance tests show that this code snippet lead to some failures. I rethink of it, found that we should bound the framebuffer to m_fbo for reading(glReadPixels, glCopyTexImage2D, etc), instead of using restoreFramebufferBindings(), who bind to m_multisampleFBO. The data in multisampled FB has been blit-ed to single-sampled buffer, we should read from the single-sampled buffer afterwards. That is the reason why we need this function (DrawingBuffer::commit). And the failures in WebGL conformance tests go away now.
https://codereview.chromium.org/1120953002/diff/180001/Source/core/html/canva... File Source/core/html/canvas/WebGLRenderingContextBase.cpp (left): https://codereview.chromium.org/1120953002/diff/180001/Source/core/html/canva... Source/core/html/canvas/WebGLRenderingContextBase.cpp:2063: case GL_DEPTH_ATTACHMENT: You haven't answered my question: why removing these two? https://codereview.chromium.org/1120953002/diff/180001/Source/core/html/canva... File Source/core/html/canvas/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1120953002/diff/180001/Source/core/html/canva... Source/core/html/canvas/WebGLRenderingContextBase.cpp:244: m_drawingBuffer->bind(m_target); Instead of calling m_drawingBuffer->bind(m_target), you should call m_drawingBuffer->restoreFramebufferBindings(). This is because in commit, not just the read binding target, but the draw binding target could be disturbed in WebGL 2, and you didn't recover the draw binding target. So this way, you actually don't need the target parameter. (In theory you also don't need the readFramebufferBinding parameter because drawingBuffer should have it, but it's OK to be redundant here.)
Thanks for your reviewing, zmo@. I have updated the code accordingly. PTAL. https://codereview.chromium.org/1120953002/diff/180001/Source/core/html/canva... File Source/core/html/canvas/WebGLRenderingContextBase.cpp (left): https://codereview.chromium.org/1120953002/diff/180001/Source/core/html/canva... Source/core/html/canvas/WebGLRenderingContextBase.cpp:2063: case GL_DEPTH_ATTACHMENT: On 2015/06/10 19:35:41, Zhenyao Mo wrote: > You haven't answered my question: why removing these two? Sorry... seems that this code snippet is redundant: depth / stencil attachment don't need particular operation. They are exactly the same with the code in default branch, just like color attachment, who will go to the default branch. I have added these two again. https://codereview.chromium.org/1120953002/diff/180001/Source/core/html/canva... File Source/core/html/canvas/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1120953002/diff/180001/Source/core/html/canva... Source/core/html/canvas/WebGLRenderingContextBase.cpp:244: m_drawingBuffer->bind(m_target); On 2015/06/10 19:35:41, Zhenyao Mo wrote: > Instead of calling m_drawingBuffer->bind(m_target), you should call > m_drawingBuffer->restoreFramebufferBindings(). This is because in commit, not > just the read binding target, but the draw binding target could be disturbed in > WebGL 2, and you didn't recover the draw binding target. > > So this way, you actually don't need the target parameter. > > (In theory you also don't need the readFramebufferBinding parameter because > drawingBuffer should have it, but it's OK to be redundant here.) Yes, you are right. I removed the m_target variable. In DrawingBuffer::commit, just binding m_fbo to GL_FRAMEBUFFER binding point is OK. we will restore read/draw framebuffer after reading.
The code looks fine to me, but before I sign off, I applied your CL and tested locally. In WebGL 1 conformance test, no regressions. However, when I run the new conformance2/renderbuffers/framebuffer-test.html you just added, I got a crash ASSERTION FAILED: isBound() ../../third_party/WebKit/Source/core/html/canvas/WebGLFramebuffer.cpp(300) : void blink::WebGLFramebuffer::setAttachmentForBoundFramebuffer(GLenum, GLenum, GLenum, blink::WebGLTexture *, GLint) Is this expected?
On 2015/06/11 17:35:02, Zhenyao Mo wrote: > The code looks fine to me, but before I sign off, I applied your CL and tested > locally. In WebGL 1 conformance test, no regressions. However, when I run the > new conformance2/renderbuffers/framebuffer-test.html you just added, I got a > crash ASSERTION FAILED: isBound() > ../../third_party/WebKit/Source/core/html/canvas/WebGLFramebuffer.cpp(300) : > void blink::WebGLFramebuffer::setAttachmentForBoundFramebuffer(GLenum, GLenum, > GLenum, blink::WebGLTexture *, GLint) > > Is this expected? No. But I can not reproduce the crash introduced by the assertion you pointed out. Could you reproduce it 100%? What platform are you tested on, Android or Mac? What is your graphics card? I run the test on my Linux desktop which can support OGL 4.4, seems that there is no crash, although several failures exist in conformance2/renderbuffers/framebuffer-test.html w/ or w/o this patch (The failure decreased with this CL, but some still exist, I will try to fix these failures in Chromium/Blink later).
I can always reproduce on Mac/Linux, so I think it's everywhere. Maybe you build release build, and assertions on blink is debug only. You can either build debug build to reproduce, or add blink_asserts_always_on=1 to the GYP_DEFINES string, so you can still build and run in release, but the asserts will be triggered.
On 2015/06/12 17:52:08, Zhenyao Mo wrote: > I can always reproduce on Mac/Linux, so I think it's everywhere. > > Maybe you build release build, and assertions on blink is debug only. > > You can either build debug build to reproduce, or add blink_asserts_always_on=1 > to the GYP_DEFINES string, so you can still build and run in release, but the > asserts will be triggered. zmo@, you are correct. I didn't run the debug version. It is an error. The function WebGLFramebuffer::isBound need to be updated against WebGL 2. This function make sure that the current framebuffer equals to the fb got from current context bound to the specific target. The original code is written against WebGL 1, who just have one target: GL_FRAMEBUFFER. In WebGL2, however, we should add a parameter 'target' to get the designated fbo from current context. Code has been updated, PTAL.
LGTM (Thank you for your persistence) I verified locally on my Linux that no regression on WebGL 1 conformance tests, and on WebGL 2, the framebuffer.html is mostly passing.
On 2015/06/16 21:34:40, Zhenyao Mo wrote: > LGTM (Thank you for your persistence) > > I verified locally on my Linux that no regression on WebGL 1 conformance tests, > and on WebGL 2, the framebuffer.html is mostly passing. Yes. The failures of framebuffer-test.html on WebGL 2 are caused by some other APIs. For example, texture allocation with RG8/RG internal_format/format failed. This texture format should be supported in WebGL2, but the function texImage2D in Chromium has not been updated against WebGL 2 yet. Then request the blue_size of the texture attachment by getFramebufferAttachment will equal to zero and fail too. I will fix these failures later. @kbr, could you have a double check?
ping... Ken, could you have a look?
Apologies for being slow to re-review. This LGTM in its current form; not necessary to solve the copyTexImage2D and copyTexSubImage2D problem, if it exists, in this CL. Thanks zmo@ for your thorough review of this CL. Thanks yunchao@ for your contribution here. https://codereview.chromium.org/1120953002/diff/240001/Source/core/html/canva... File Source/core/html/canvas/WebGLFramebuffer.cpp (right): https://codereview.chromium.org/1120953002/diff/240001/Source/core/html/canva... Source/core/html/canvas/WebGLFramebuffer.cpp:550: bool WebGLFramebuffer::onAccess(WebGraphicsContext3D* context3d, GLenum target, const char** reason) The "target" argument isn't used, so it should be removed from here and the call sites. https://codereview.chromium.org/1120953002/diff/240001/Source/core/html/canva... File Source/core/html/canvas/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1120953002/diff/240001/Source/core/html/canva... Source/core/html/canvas/WebGLRenderingContextBase.cpp:1571: || (getFramebufferBinding(GL_READ_FRAMEBUFFER) && !getFramebufferBinding(GL_READ_FRAMEBUFFER)->onAccess(webContext(), GL_READ_FRAMEBUFFER, &reason))) { I think more checks may be needed regarding the current read buffer (glReadBuffer) and whether the currently bound framebuffer has such an attachment. For example, I think if: - there was a user-defined framebuffer - it has a COLOR_ATTACHMENT0 but not a COLOR_ATTACHMENT1 - glReadBuffer was called with COLOR_ATTACHMENT1 - glCopyTexImage2D is called that this needs to generate an INVALID_FRAMEBUFFER_OPERATION (though this behavior isn't in the manual page https://www.khronos.org/opengles/sdk/docs/man3/html/glCopyTexImage2D.xhtml , at least -- not sure about the ES 3.0 spec). Same for copyTexSubImage2D below.
Patchset #10 (id:260001) has been deleted
Thanks for your double-check, kbr@. I have updated the code to resolve one problem. I submitted a separate patch (https://codereview.chromium.org/1205573003/) to resolve the other one. If no objection, I will merge this one tomorrow, in order to avoid code rebase. :) https://codereview.chromium.org/1120953002/diff/240001/Source/core/html/canva... File Source/core/html/canvas/WebGLFramebuffer.cpp (right): https://codereview.chromium.org/1120953002/diff/240001/Source/core/html/canva... Source/core/html/canvas/WebGLFramebuffer.cpp:550: bool WebGLFramebuffer::onAccess(WebGraphicsContext3D* context3d, GLenum target, const char** reason) On 2015/06/20 00:08:42, Ken Russell wrote: > The "target" argument isn't used, so it should be removed from here and the call > sites. Done. https://codereview.chromium.org/1120953002/diff/240001/Source/core/html/canva... File Source/core/html/canvas/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1120953002/diff/240001/Source/core/html/canva... Source/core/html/canvas/WebGLRenderingContextBase.cpp:1571: || (getFramebufferBinding(GL_READ_FRAMEBUFFER) && !getFramebufferBinding(GL_READ_FRAMEBUFFER)->onAccess(webContext(), GL_READ_FRAMEBUFFER, &reason))) { On 2015/06/20 00:08:42, Ken Russell wrote: > I think more checks may be needed regarding the current read buffer > (glReadBuffer) and whether the currently bound framebuffer has such an > attachment. For example, I think if: > - there was a user-defined framebuffer > - it has a COLOR_ATTACHMENT0 but not a COLOR_ATTACHMENT1 > - glReadBuffer was called with COLOR_ATTACHMENT1 > - glCopyTexImage2D is called > > that this needs to generate an INVALID_FRAMEBUFFER_OPERATION (though this > behavior isn't in the manual page > https://www.khronos.org/opengles/sdk/docs/man3/html/glCopyTexImage2D.xhtml , at > least -- not sure about the ES 3.0 spec). Same for copyTexSubImage2D below. I checked this in ES 3.0 spec. This indeed should generate an INVALID_OPERATION error. However, I'd like to submit another patch to validate the read buffer attachment problem: https://codereview.chromium.org/1205573003/. PTAL.
Sounds fine. Let's land this.
The CQ bit was checked by kbr@chromium.org
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/1120953002/#ps280001 (title: "addressed kbr@'s feedback: remove an argument that is not used")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1120953002/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/60428)
The CQ bit was checked by kbr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1120953002/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/60456)
I don't think the failure is related. Landing with NOTRY=true
The CQ bit was checked by zmo@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1120953002/280001
Message was sent while issue was closed.
Committed patchset #10 (id:280001) as https://src.chromium.org/viewvc/blink?view=rev&revision=197696
Message was sent while issue was closed.
On 2015/06/24 03:13:40, Zhenyao Mo wrote: > I don't think the failure is related. Landing with NOTRY=true Thank you, zmo@. Yeah, I see several patches also failed on mac_blink_rel, and other bots are OK. |
