Chromium Code Reviews
|
|
|
Created:
5 years, 1 month ago by yunchao Modified:
5 years ago CC:
blink-reviews, dshwang, blink-reviews-html_chromium.org, Justin Novosad, dglazkov+blink, Rik, aandrey+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
Descriptionupdate getFramebufferAttachmentParameter for WebGL 2
BUG=295792
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=194785
Patch Set 1 : #
Total comments: 11
Patch Set 2 : addressed zmo@'s feedback #
Total comments: 11
Patch Set 3 : addressed zmo@'s feedback: wrong assertion and some small changes #
Total comments: 2
Patch Set 4 : code rebase #
Total comments: 6
Patch Set 5 : addressed kbr@'s feedback: get the 'correct' framebuffer bound to the given target #
Total comments: 2
Patch Set 6 : addressed kbr@'s feedback + rebased code #
Messages
Total messages: 27 (8 generated)
yunchao.he@intel.com changed reviewers: + bajones@chromium.org, kbr@chromium.org, zmo@chromium.org
PTAL. Thanks a lot!
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
I didn't finish reviewing this whole CL, but I feel we should not do all the stuff in WebGL 2 function. All we should do is call the underlying function, and cast to various types depending on the pname. We should rely on the underlying implementation to do error checking, etc. This will be our future direction. https://codereview.chromium.org/981913002/diff/60001/Source/core/html/canvas/... File Source/core/html/canvas/WebGL2RenderingContextBase.cpp (right): https://codereview.chromium.org/981913002/diff/60001/Source/core/html/canvas/... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:1199: bool WebGL2RenderingContextBase::validateGetFramebufferAttachmentParameters(const char* functionName, GLenum target, GLenum attachment) I think you should fold this function into getFramebufferAttachmentParameter() because you are not just checking for enums, but also other stuff, for example, of the depth_stencil attached objects are valid. https://codereview.chromium.org/981913002/diff/60001/Source/core/html/canvas/... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:1201: if (target != GL_FRAMEBUFFER && target != GL_READ_FRAMEBUFFER) { Add a comment GL_DRAW_FRAMEBUFFER == GL_FRAMEBUFFER> https://codereview.chromium.org/981913002/diff/60001/Source/core/html/canvas/... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:1205: ASSERT(!m_framebufferBinding && !drawingBuffer()); This ASSERTION is incorrect. It should be ||, not &&. https://codereview.chromium.org/981913002/diff/60001/Source/core/html/canvas/... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:1206: if (!m_framebufferBinding) { You also need to check || !m_framebufferBinding->object(). Maybe we don't need to do it, but it's better to be consistent with the existing code. https://codereview.chromium.org/981913002/diff/60001/Source/core/html/canvas/... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:1243: if (m_framebufferBinding && !m_framebufferBinding->object()) { This is incorrect, in ES3 we allow querying the back buffer. In the above function you actually handled that.
On 2015/03/06 18:35:37, Zhenyao Mo wrote: > I didn't finish reviewing this whole CL, but I feel we should not do all the > stuff in WebGL 2 function. All we should do is call the underlying function, > and cast to various types depending on the pname. We should rely on the > underlying implementation to do error checking, etc. This will be our future > direction. > > @zmo, Yes, I am aware of this. I think we can validate the parameters in the same way that WebGLRenderingContextBase does. In future, we can do the code refactoring in batches. And I am glad to help on this. WDYT? https://codereview.chromium.org/981913002/diff/60001/Source/core/html/canvas/... File Source/core/html/canvas/WebGL2RenderingContextBase.cpp (right): https://codereview.chromium.org/981913002/diff/60001/Source/core/html/canvas/... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:1199: bool WebGL2RenderingContextBase::validateGetFramebufferAttachmentParameters(const char* functionName, GLenum target, GLenum attachment) On 2015/03/06 18:35:37, Zhenyao Mo wrote: > I think you should fold this function into getFramebufferAttachmentParameter() > because you are not just checking for enums, but also other stuff, for example, > of the depth_stencil attached objects are valid. Agree. Considering that this validation is standalone, what about rename the function to validateGetFramebufferAttachmentParameterFunc? If you still think that it is better to fold this code snippet into getFramebufferAttachmentParameter, I will follow that. https://codereview.chromium.org/981913002/diff/60001/Source/core/html/canvas/... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:1201: if (target != GL_FRAMEBUFFER && target != GL_READ_FRAMEBUFFER) { On 2015/03/06 18:35:37, Zhenyao Mo wrote: > Add a comment GL_DRAW_FRAMEBUFFER == GL_FRAMEBUFFER> A little tricky here. GL_DRAW_FRAMEBUFFER is not equal to GL_FRAMEBUFFER in GLES 3.0 spec and WebGL Spec. This differs from openGL spec. I added GL_DRAW_FRAMEBUFFER in the latest patch set. https://codereview.chromium.org/981913002/diff/60001/Source/core/html/canvas/... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:1205: ASSERT(!m_framebufferBinding && !drawingBuffer()); On 2015/03/06 18:35:37, Zhenyao Mo wrote: > This ASSERTION is incorrect. It should be ||, not &&. If binding to default FB, m_framebufferBinding will be 0. Query the default FB is allowed in WebGL 2.0 (but it is not allowed in WebGL 1.0). In this case, Chromium doesn't really bind to window-system's default FB, but the drawingBuffer for WebGL. So, if binding to default FB, drawingBuffer should be existed. (seems that drawingBuffer should be always existed). https://codereview.chromium.org/981913002/diff/60001/Source/core/html/canvas/... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:1206: if (!m_framebufferBinding) { On 2015/03/06 18:35:37, Zhenyao Mo wrote: > You also need to check || !m_framebufferBinding->object(). > > Maybe we don't need to do it, but it's better to be consistent with the existing > code. Personally, I think it is not necessary to check m_framebufferBinding->object(), which refer to an FBO. https://codereview.chromium.org/981913002/diff/60001/Source/core/html/canvas/... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:1243: if (m_framebufferBinding && !m_framebufferBinding->object()) { On 2015/03/06 18:35:37, Zhenyao Mo wrote: > This is incorrect, in ES3 we allow querying the back buffer. In the above > function you actually handled that. Agree, but this is different from the original code in WebGLRenderingContext, whose condition for if-clause is !m_framebufferBinding || !m_framebufferBinding->object(). I add an assert here in the latest patch set: ASSERT(m_framebufferBinding || !m_framebufferBinding->object());
Many thanks for your feedback, @zmo. https://codereview.chromium.org/981913002/diff/80001/Source/core/html/canvas/... File Source/core/html/canvas/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/981913002/diff/80001/Source/core/html/canvas/... Source/core/html/canvas/WebGLRenderingContextBase.cpp:2265: WebGLSharedObject* attachmentObject = m_framebufferBinding->getAttachmentObject(attachment); To make it clearer, rename 'object' to 'attachmentObject', in order to distinguish m_framebufferBinding->getAttachmentObject(attachment) from m_framebufferBinding->object().
https://codereview.chromium.org/981913002/diff/60001/Source/core/html/canvas/... File Source/core/html/canvas/WebGL2RenderingContextBase.cpp (right): https://codereview.chromium.org/981913002/diff/60001/Source/core/html/canvas/... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:1201: if (target != GL_FRAMEBUFFER && target != GL_READ_FRAMEBUFFER) { On 2015/03/09 07:16:57, yunchao wrote: > On 2015/03/06 18:35:37, Zhenyao Mo wrote: > > Add a comment GL_DRAW_FRAMEBUFFER == GL_FRAMEBUFFER> > > A little tricky here. GL_DRAW_FRAMEBUFFER is not equal to GL_FRAMEBUFFER in GLES > 3.0 spec and WebGL Spec. This differs from openGL spec. I added > GL_DRAW_FRAMEBUFFER in the latest patch set. GLenum GL_DRAW_FRAMEBUFFER(0x8CA9) is not equal to GL_FRAMEBUFFER(0x8D40). In addtion, bindFramebuffer with GL_FRAMEBUFFER will bind to both read and draw buffers, although for framebufferRenderbuffer, framebufferTexture2D, checkFramebufferStatus as well as framebufferAttachmentParameter, target set to GL_FRAMEBUFFER has the same effect as target set to GL_DRAW_FRAMEBUFFER. This feature is the same in OpenGL and GLES. My statement above that the feature in GLES differs from that in OpenGL is wrong.
Mostly looks good https://codereview.chromium.org/981913002/diff/80001/Source/core/html/canvas/... File Source/core/html/canvas/WebGL2RenderingContextBase.cpp (right): https://codereview.chromium.org/981913002/diff/80001/Source/core/html/canvas/... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:1205: ASSERT(!m_framebufferBinding && !drawingBuffer()); OK, let me explain again. I understand your intention, but your assertion is incorrect. It's the same as !(m_framebufferBinding || drawingBuffer()), which is always false. You should assert the opposite. https://codereview.chromium.org/981913002/diff/80001/Source/core/html/canvas/... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:1214: synthesizeGLError(GL_INVALID_OPERATION, functionName, "invalid attachment"); Although the man page of this function says INVALID_OPERATION, but the es3 spec doesn't say anything about INVALID_OPERATION in this situation. Same as in the es2 spec. In ES2/WebGL1, we generate an INVALID_ENUM. I feel we should be consistent here. https://codereview.chromium.org/981913002/diff/80001/Source/core/html/canvas/... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:1231: synthesizeGLError(GL_INVALID_OPERATION, functionName, "invalid attachment"); Same here, maybe INVALID_ENUM. https://codereview.chromium.org/981913002/diff/80001/Source/core/html/canvas/... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:1257: if (!attachmentObject) nit: add ASSERT(!m_framebufferBinding) here to make code easier to read. https://codereview.chromium.org/981913002/diff/80001/Source/core/html/canvas/... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:1262: return WebGLAny(scriptState, GL_RENDERBUFFER); nit: ASSERT(attachmentObject->isTexture() || attachmentObject->isRenderbuffer()) here or above.
Thanks for your feedback, @zmo. The code has been updated accordingly. https://codereview.chromium.org/981913002/diff/80001/Source/core/html/canvas/... File Source/core/html/canvas/WebGL2RenderingContextBase.cpp (right): https://codereview.chromium.org/981913002/diff/80001/Source/core/html/canvas/... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:1205: ASSERT(!m_framebufferBinding && !drawingBuffer()); On 2015/03/09 18:39:19, Zhenyao Mo wrote: > OK, let me explain again. I understand your intention, but your assertion is > incorrect. It's the same as !(m_framebufferBinding || drawingBuffer()), which > is always false. You should assert the opposite. Yeah. I got it. Thank you, @zmo. https://codereview.chromium.org/981913002/diff/80001/Source/core/html/canvas/... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:1214: synthesizeGLError(GL_INVALID_OPERATION, functionName, "invalid attachment"); On 2015/03/09 18:39:19, Zhenyao Mo wrote: > Although the man page of this function says INVALID_OPERATION, but the es3 spec > doesn't say anything about INVALID_OPERATION in this situation. Same as in the > es2 spec. > > In ES2/WebGL1, we generate an INVALID_ENUM. I feel we should be consistent > here. Done. https://codereview.chromium.org/981913002/diff/80001/Source/core/html/canvas/... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:1231: synthesizeGLError(GL_INVALID_OPERATION, functionName, "invalid attachment"); On 2015/03/09 18:39:19, Zhenyao Mo wrote: > Same here, maybe INVALID_ENUM. Done. https://codereview.chromium.org/981913002/diff/80001/Source/core/html/canvas/... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:1257: if (!attachmentObject) On 2015/03/09 18:39:19, Zhenyao Mo wrote: > nit: add ASSERT(!m_framebufferBinding) here to make code easier to read. Done. https://codereview.chromium.org/981913002/diff/80001/Source/core/html/canvas/... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:1262: return WebGLAny(scriptState, GL_RENDERBUFFER); On 2015/03/09 18:39:19, Zhenyao Mo wrote: > nit: ASSERT(attachmentObject->isTexture() || attachmentObject->isRenderbuffer()) > here or above. Done.
https://codereview.chromium.org/981913002/diff/100001/Source/core/html/canvas... File Source/core/html/canvas/WebGL2RenderingContextBase.cpp (right): https://codereview.chromium.org/981913002/diff/100001/Source/core/html/canvas... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:1243: ASSERT(!m_framebufferBinding || m_framebufferBinding->object()); Now this ASSERTION is wrong. I think you can just remove this assertion here.
LGTM https://codereview.chromium.org/981913002/diff/100001/Source/core/html/canvas... File Source/core/html/canvas/WebGL2RenderingContextBase.cpp (right): https://codereview.chromium.org/981913002/diff/100001/Source/core/html/canvas... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:1243: ASSERT(!m_framebufferBinding || m_framebufferBinding->object()); On 2015/03/10 17:03:12, Zhenyao Mo wrote: > Now this ASSERTION is wrong. I think you can just remove this assertion here. Never mind. It is correct.
Yunchao, thanks for this contribution and Mo, thanks for reviewing it so carefully. Yunchao, I would like the tests for these new code paths to land in the WebGL conformance suite before these changes land. Please ping on this CL once the new tests are in place.
On 2015/03/10 19:28:18, Ken Russell wrote: > Yunchao, thanks for this contribution and Mo, thanks for reviewing it so > carefully. > > Yunchao, I would like the tests for these new code paths to land in the WebGL > conformance suite before these changes land. Please ping on this CL once the new > tests are in place. The conformance test is submitted: https://github.com/KhronosGroup/WebGL/pull/956. However, Chromium failed to bind/query default framebuffer, say bindFramebuffer(target, 0). I will submit patch to bind framebuffer to the default one if WebGL 2 should support this feature.
On 2015/04/27 05:32:42, yunchao wrote: > On 2015/03/10 19:28:18, Ken Russell wrote: > > Yunchao, thanks for this contribution and Mo, thanks for reviewing it so > > carefully. > > > > Yunchao, I would like the tests for these new code paths to land in the WebGL > > conformance suite before these changes land. Please ping on this CL once the > new > > tests are in place. > > The conformance test is submitted: > https://github.com/KhronosGroup/WebGL/pull/956. > > However, Chromium failed to bind/query default framebuffer, say > bindFramebuffer(target, 0). I will submit patch to bind framebuffer to the > default one if WebGL 2 should support this feature. I made a mistake, It should be bindFramebuffer(target, null) to bind to default framebuffer... It is OK now. Some cases failed because several GLenums are not supported in command buffer.
https://codereview.chromium.org/981913002/diff/120001/Source/core/html/canvas... File Source/core/html/canvas/WebGL2RenderingContextBase.cpp (right): https://codereview.chromium.org/981913002/diff/120001/Source/core/html/canvas... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:1466: if (target != GL_FRAMEBUFFER && target != GL_READ_FRAMEBUFFER && target != GL_DRAW_FRAMEBUFFER) { Please make sure that this code eventually uses the new validation function from https://codereview.chromium.org/1099853002/ . https://codereview.chromium.org/981913002/diff/120001/Source/core/html/canvas... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:1508: ASSERT(!m_framebufferBinding || m_framebufferBinding->object()); target can be either GL_READ_FRAMEBUFFER or GL_DRAW_FRAMEBUFFER. m_framebufferBinding is only the DRAW_FRAMEBUFFER. As in https://codereview.chromium.org/1099853002/ , this code needs to be updated to handle both binding points. https://codereview.chromium.org/981913002/diff/120001/Source/core/html/canvas... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:1549: webContext()->getFramebufferAttachmentParameteriv(target, attachment, pname, &value); This will return an incorrect answer if the DrawingBuffer's FBO (which is WebGL's default framebuffer) is bound. The code needs to be updated to handle that case.
Patchset #5 (id:140001) has been deleted
kbr@, Thanks for your reviewing. The code has been updated, PTAL when you have time. This change depends on this one: https://codereview.chromium.org/1099853002/. But I can pick functions defined in that one if this is OK. https://codereview.chromium.org/981913002/diff/120001/Source/core/html/canvas... File Source/core/html/canvas/WebGL2RenderingContextBase.cpp (right): https://codereview.chromium.org/981913002/diff/120001/Source/core/html/canvas... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:1466: if (target != GL_FRAMEBUFFER && target != GL_READ_FRAMEBUFFER && target != GL_DRAW_FRAMEBUFFER) { On 2015/04/28 05:43:23, Ken Russell wrote: > Please make sure that this code eventually uses the new validation function from > https://codereview.chromium.org/1099853002/ . Done. https://codereview.chromium.org/981913002/diff/120001/Source/core/html/canvas... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:1508: ASSERT(!m_framebufferBinding || m_framebufferBinding->object()); On 2015/04/28 05:43:23, Ken Russell wrote: > target can be either GL_READ_FRAMEBUFFER or GL_DRAW_FRAMEBUFFER. > m_framebufferBinding is only the DRAW_FRAMEBUFFER. As in > https://codereview.chromium.org/1099853002/ , this code needs to be updated to > handle both binding points. Done. https://codereview.chromium.org/981913002/diff/120001/Source/core/html/canvas... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:1549: webContext()->getFramebufferAttachmentParameteriv(target, attachment, pname, &value); On 2015/04/28 05:43:23, Ken Russell wrote: > This will return an incorrect answer if the DrawingBuffer's FBO (which is > WebGL's default framebuffer) is bound. The code needs to be updated to handle > that case. Agreed. The latest patch set has updated bindFramebuffer. If binding to the default Framebuffer, the latest patch set would bind to DrawingBuffer's FBO for the given target, so the query would be correct.
https://codereview.chromium.org/981913002/diff/160001/Source/core/html/canvas... File Source/core/html/canvas/WebGL2RenderingContextBase.cpp (right): https://codereview.chromium.org/981913002/diff/160001/Source/core/html/canvas... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:1558: break; The scenario where different objects are bound to the depth and stencil attachment points and the DEPTH_STENCIL_ATTACHMENT is queried is supposed to generate an INVALID_OPERATION error, not an INVALID_ENUM error. https://codereview.chromium.org/981913002/diff/160001/Source/core/html/canvas... File Source/core/html/canvas/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/981913002/diff/160001/Source/core/html/canvas... Source/core/html/canvas/WebGLRenderingContextBase.cpp:6075: drawingBuffer()->setFramebufferBinding(objectOrZero(m_framebufferBinding.get())); Looking more deeply at the DrawingBuffer code, it resets the GL_FRAMEBUFFER binding target internally, which will destroy the GL_READ_FRAMEBUFFER binding if it was set by the user. The more I look at this code the less correct it seems. The exposure of the separate DRAW_FRAMEBUFFER and READ_FRAMEBUFFER binding points in GLES3 changes all of the code in WebGLRenderingContextBase that deals with the default drawing buffer, and the DrawingBuffer class itself. Do you see the same problems here that I do? If so, what is your plan for making this code correct? User-level multisampled renderbuffers, and the exposure of blitFramebuffer, is a major feature in WebGL 2.0, and if we do a half-baked job on this code along the way then we are setting ourselves up for a huge debugging effort later.
On 2015/04/30 02:28:26, Ken Russell wrote: > https://codereview.chromium.org/981913002/diff/160001/Source/core/html/canvas... > File Source/core/html/canvas/WebGL2RenderingContextBase.cpp (right): > > https://codereview.chromium.org/981913002/diff/160001/Source/core/html/canvas... > Source/core/html/canvas/WebGL2RenderingContextBase.cpp:1558: break; > The scenario where different objects are bound to the depth and stencil > attachment points and the DEPTH_STENCIL_ATTACHMENT is queried is supposed to > generate an INVALID_OPERATION error, not an INVALID_ENUM error. > > https://codereview.chromium.org/981913002/diff/160001/Source/core/html/canvas... > File Source/core/html/canvas/WebGLRenderingContextBase.cpp (right): > > https://codereview.chromium.org/981913002/diff/160001/Source/core/html/canvas... > Source/core/html/canvas/WebGLRenderingContextBase.cpp:6075: > drawingBuffer()->setFramebufferBinding(objectOrZero(m_framebufferBinding.get())); > Looking more deeply at the DrawingBuffer code, it resets the GL_FRAMEBUFFER > binding target internally, which will destroy the GL_READ_FRAMEBUFFER binding if > it was set by the user. > > The more I look at this code the less correct it seems. The exposure of the > separate DRAW_FRAMEBUFFER and READ_FRAMEBUFFER binding points in GLES3 changes > all of the code in WebGLRenderingContextBase that deals with the default drawing > buffer, and the DrawingBuffer class itself. > > Do you see the same problems here that I do? If so, what is your plan for making > this code correct? User-level multisampled renderbuffers, and the exposure of > blitFramebuffer, is a major feature in WebGL 2.0, and if we do a half-baked job > on this code along the way then we are setting ourselves up for a huge debugging > effort later. Yes. I see this too, as I mentioned in https://codereview.chromium.org/1099853002/, almost all Framebuffer related APIs is impacted by these two Framebuffer binding points, I am working on the other APIs now.
On 2015/04/30 02:32:01, yunchao wrote: > On 2015/04/30 02:28:26, Ken Russell wrote: > > > https://codereview.chromium.org/981913002/diff/160001/Source/core/html/canvas... > > File Source/core/html/canvas/WebGL2RenderingContextBase.cpp (right): > > > > > https://codereview.chromium.org/981913002/diff/160001/Source/core/html/canvas... > > Source/core/html/canvas/WebGL2RenderingContextBase.cpp:1558: break; > > The scenario where different objects are bound to the depth and stencil > > attachment points and the DEPTH_STENCIL_ATTACHMENT is queried is supposed to > > generate an INVALID_OPERATION error, not an INVALID_ENUM error. > > > > > https://codereview.chromium.org/981913002/diff/160001/Source/core/html/canvas... > > File Source/core/html/canvas/WebGLRenderingContextBase.cpp (right): > > > > > https://codereview.chromium.org/981913002/diff/160001/Source/core/html/canvas... > > Source/core/html/canvas/WebGLRenderingContextBase.cpp:6075: > > > drawingBuffer()->setFramebufferBinding(objectOrZero(m_framebufferBinding.get())); > > Looking more deeply at the DrawingBuffer code, it resets the GL_FRAMEBUFFER > > binding target internally, which will destroy the GL_READ_FRAMEBUFFER binding > if > > it was set by the user. > > > > The more I look at this code the less correct it seems. The exposure of the > > separate DRAW_FRAMEBUFFER and READ_FRAMEBUFFER binding points in GLES3 changes > > all of the code in WebGLRenderingContextBase that deals with the default > drawing > > buffer, and the DrawingBuffer class itself. > > > > Do you see the same problems here that I do? If so, what is your plan for > making > > this code correct? User-level multisampled renderbuffers, and the exposure of > > blitFramebuffer, is a major feature in WebGL 2.0, and if we do a half-baked > job > > on this code along the way then we are setting ourselves up for a huge > debugging > > effort later. > > Yes. I see this too, as I mentioned in > https://codereview.chromium.org/1099853002/, almost all Framebuffer related APIs > is impacted by these two Framebuffer binding points, I am working on the other > APIs now. OK. I'll trust that you'll follow through on these changes and help write thorough conformance tests for these changes. The DrawingBuffer code is some of the trickiest in the WebGL implementation. Please fix the bug with the INVALID_OPERATION error that should be generated rather than INVALID_ENUM, then LGTM.
Patchset #6 (id:180001) has been deleted
The CQ bit was checked by yunchao.he@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from zmo@chromium.org, kbr@chromium.org Link to the patchset: https://codereview.chromium.org/981913002/#ps200001 (title: "addressed kbr@'s feedback + rebased code")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/981913002/200001
Message was sent while issue was closed.
Committed patchset #6 (id:200001) as https://src.chromium.org/viewvc/blink?view=rev&revision=194785 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
