Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1205573003:
WebGL 2: validate read buffer attachment when reading from FBO (Closed)
|
Created:
5 years, 6 months ago by yunchao Modified:
5 years, 5 months ago CC:
blink-reviews, dshwang, blink-reviews-html_chromium.org, dglazkov+blink, Rik, Justin Novosad Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionWebGL 2: validate read buffer attachment when reading from FBO
BUG=295792
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=198484
Patch Set 1 #
Total comments: 2
Patch Set 2 : addressed kbr@'s feedback: translate GL_BACK to GL_COLOR_ATTACHMENT0 on default fb of WebGL #
Total comments: 8
Patch Set 3 : addressed zmo@'s feedback: translate GL_COLOR_ATTACHMENT0 to GL_BACK in getter for default fb #
Total comments: 4
Patch Set 4 : addressed zmo@'s feedback: readbuffer is per framebuffer, not per rendering context #
Total comments: 8
Patch Set 5 : addressed zmo@'s and kbr@'s feedback, and rebased the code #
Total comments: 4
Patch Set 6 : addressed kbr's feedback: reading from framebuffer with GL_NONE is invalid #
Messages
Total messages: 31 (6 generated)
yunchao.he@intel.com changed reviewers: + bajones@chromium.org, kbr@chromium.org, zmo@chromium.org
Discussed in https://codereview.chromium.org/1120953002/. PTAL when you have time. Thanks a lot!
On 2015/06/23 08:57:02, yunchao wrote: > Discussed in https://codereview.chromium.org/1120953002/. PTAL when you have > time. Thanks a lot! Depend on https://codereview.chromium.org/1120953002/. The trybots will fail but I can not stop them...
Thanks for working on this. https://codereview.chromium.org/1205573003/diff/1/Source/core/html/canvas/Web... File Source/core/html/canvas/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1205573003/diff/1/Source/core/html/canvas/Web... Source/core/html/canvas/WebGLRenderingContextBase.cpp:1585: if (!validateReadBufferAttachment("copyTexImage2D", readFramebufferBinding)) Has this code path been tested? It looks to me like it will do the wrong thing in this situation: - The user calls readBuffer(gl.BACK) - The user attempts to copyTexImage2D from the "default" back buffer (which is actually the DrawingBuffer) to a texture The problem is that we need to translate the readBuffer from BACK to COLOR_ATTACHMENT0 when the DrawingBuffer is bound. Is this being done anywhere? If there aren't conformance tests for this then let's please add them.
Thanks for your reviewing, kbr@. Code has been updated to translate GL_BACK to GL_COLOR_ATTACHMENT0 for default fb of WebGL. PTAL. https://codereview.chromium.org/1205573003/diff/1/Source/core/html/canvas/Web... File Source/core/html/canvas/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1205573003/diff/1/Source/core/html/canvas/Web... Source/core/html/canvas/WebGLRenderingContextBase.cpp:1585: if (!validateReadBufferAttachment("copyTexImage2D", readFramebufferBinding)) On 2015/06/24 00:24:38, Ken Russell wrote: > Has this code path been tested? It looks to me like it will do the wrong thing > in this situation: > > - The user calls readBuffer(gl.BACK) > - The user attempts to copyTexImage2D from the "default" back buffer (which is > actually the DrawingBuffer) to a texture > > The problem is that we need to translate the readBuffer from BACK to > COLOR_ATTACHMENT0 when the DrawingBuffer is bound. Is this being done anywhere? > > If there aren't conformance tests for this then let's please add them. Good catch. I have updated the patch to translate GL_BACK to GL_COLOR_ATTACHMENT0 against default frame buffer of WebGL in readBuffer API. The conformance test of readbuffer and APIs related to reading from FBO is in preparation.
https://codereview.chromium.org/1205573003/diff/20001/Source/core/html/canvas... File Source/core/html/canvas/WebGL2RenderingContextBase.cpp (right): https://codereview.chromium.org/1205573003/diff/20001/Source/core/html/canvas... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:187: if (mode != GL_BACK || mode != GL_NONE) { &&, not || https://codereview.chromium.org/1205573003/diff/20001/Source/core/html/canvas... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:194: mode = GL_COLOR_ATTACHMENT0; You also need to emulate the reverse in getParameter(READ_BUFFER). I feel it should be done in the same CL. https://codereview.chromium.org/1205573003/diff/20001/Source/core/html/canvas... File Source/core/html/canvas/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1205573003/diff/20001/Source/core/html/canvas... Source/core/html/canvas/WebGLRenderingContextBase.cpp:1545: bool WebGLRenderingContextBase::validateReadBufferAttachment(WebGLFramebuffer* readFramebufferBinding) This should not even compile. You missed the functionName param here. https://codereview.chromium.org/1205573003/diff/20001/Source/core/html/canvas... Source/core/html/canvas/WebGLRenderingContextBase.cpp:1550: synthesizeGLError(GL_INVALID_OPERATION, "copyTexImage2D", "no image attached to read buffer"); Should be functionName instead of "copyTexImage2D"
Patchset #3 (id:40001) has been deleted
Thanks for your reviewing, zmo@. The code has been updated accordingly. PTAL when you have time. https://codereview.chromium.org/1205573003/diff/20001/Source/core/html/canvas... File Source/core/html/canvas/WebGL2RenderingContextBase.cpp (right): https://codereview.chromium.org/1205573003/diff/20001/Source/core/html/canvas... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:187: if (mode != GL_BACK || mode != GL_NONE) { On 2015/06/25 22:42:22, Zhenyao Mo wrote: > &&, not || Done. https://codereview.chromium.org/1205573003/diff/20001/Source/core/html/canvas... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:194: mode = GL_COLOR_ATTACHMENT0; On 2015/06/25 22:42:22, Zhenyao Mo wrote: > You also need to emulate the reverse in getParameter(READ_BUFFER). I feel it > should be done in the same CL. Yeah, totally agree with you. https://codereview.chromium.org/1205573003/diff/20001/Source/core/html/canvas... File Source/core/html/canvas/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1205573003/diff/20001/Source/core/html/canvas... Source/core/html/canvas/WebGLRenderingContextBase.cpp:1545: bool WebGLRenderingContextBase::validateReadBufferAttachment(WebGLFramebuffer* readFramebufferBinding) On 2015/06/25 22:42:22, Zhenyao Mo wrote: > This should not even compile. You missed the functionName param here. Sorry. My local Chromium src code has some issues last week. It can not fetch code and build because of proxy problem. It is OK now. I should run the trybots to check at that time. https://codereview.chromium.org/1205573003/diff/20001/Source/core/html/canvas... Source/core/html/canvas/WebGLRenderingContextBase.cpp:1550: synthesizeGLError(GL_INVALID_OPERATION, "copyTexImage2D", "no image attached to read buffer"); On 2015/06/25 22:42:22, Zhenyao Mo wrote: > Should be functionName instead of "copyTexImage2D" Done.
LGTM
I think there are remaining problems here: for example, what happens in the
following situation?
// No framebuffer object is initially bound
gl.readBuffer(BACK); // This translates to a call to
glReadBuffer(GL_COLOR_ATTACHMENT_0)
gl.bindFramebuffer(userDefinedFBO); // which has a color attachment
gl.readPixels(...); // or another operation using the read buffer
This should generate an INVALID_OPERATION error (though this isn't clearly
covered by the ES 3.0 spec). With this patch it'll probably succeed.
Can you please add a test for this case?
Can you please propose how we can fix this problem? One way would be to simply
cache the argument passed to readBuffer, and both validate the state and pass
the correct argument down to the WebGraphicsContext3D upon every call which
might use the bound read buffer (readPixels, blitFramebuffer,
copyTex{Sub}Image2D, others?)
On 2015/07/01 00:28:29, Ken Russell wrote:
> I think there are remaining problems here: for example, what happens in the
> following situation?
>
> // No framebuffer object is initially bound
> gl.readBuffer(BACK); // This translates to a call to
> glReadBuffer(GL_COLOR_ATTACHMENT_0)
> gl.bindFramebuffer(userDefinedFBO); // which has a color attachment
> gl.readPixels(...); // or another operation using the read buffer
>
> This should generate an INVALID_OPERATION error (though this isn't clearly
> covered by the ES 3.0 spec). With this patch it'll probably succeed.
Actually I think we don't need to generate an error here. ReadBuffer operation,
in my reading of the spec, is per framebuffer. So when you bind to a new
framebuffer here, its readbuffer isn't BACK, it is whatever was selected before,
or COLOR_ATTACHMENT0 by default.
I don't find explicit text for that, but there are some texts saying the default
readbuffer for backbuffer is BACK, and default readbuffer for a user defined
framebuffer is COLOR_ATTACHMENT0. That means readbuffer setting is per
framebuffer.
>
> Can you please add a test for this case?
>
> Can you please propose how we can fix this problem? One way would be to simply
> cache the argument passed to readBuffer, and both validate the state and pass
> the correct argument down to the WebGraphicsContext3D upon every call which
> might use the bound read buffer (readPixels, blitFramebuffer,
> copyTex{Sub}Image2D, others?)
Patchset #4 (id:80001) has been deleted
On 2015/07/01 02:33:27, Zhenyao Mo wrote:
> On 2015/07/01 00:28:29, Ken Russell wrote:
> > I think there are remaining problems here: for example, what happens in the
> > following situation?
> >
> > // No framebuffer object is initially bound
> > gl.readBuffer(BACK); // This translates to a call to
> > glReadBuffer(GL_COLOR_ATTACHMENT_0)
> > gl.bindFramebuffer(userDefinedFBO); // which has a color attachment
> > gl.readPixels(...); // or another operation using the read buffer
> >
> > This should generate an INVALID_OPERATION error (though this isn't clearly
> > covered by the ES 3.0 spec). With this patch it'll probably succeed.
>
> Actually I think we don't need to generate an error here. ReadBuffer
operation,
> in my reading of the spec, is per framebuffer. So when you bind to a new
> framebuffer here, its readbuffer isn't BACK, it is whatever was selected
before,
> or COLOR_ATTACHMENT0 by default.
>
> I don't find explicit text for that, but there are some texts saying the
default
> readbuffer for backbuffer is BACK, and default readbuffer for a user defined
> framebuffer is COLOR_ATTACHMENT0. That means readbuffer setting is per
> framebuffer.
>
Agree with zmo@. Web developer switch to user defined fbo from default fb, the
read buffer (color buffer of framebuffer) should be switched to
GL_COLOR_ATTACHMENT0 from GL_BACK automatically. No errors should be generated.
> >
> > Can you please add a test for this case?
> >
> > Can you please propose how we can fix this problem? One way would be to
simply
> > cache the argument passed to readBuffer, and both validate the state and
pass
> > the correct argument down to the WebGraphicsContext3D upon every call which
> > might use the bound read buffer (readPixels, blitFramebuffer,
> > copyTex{Sub}Image2D, others?)
https://codereview.chromium.org/1205573003/diff/60001/Source/core/html/canvas... File Source/core/html/canvas/WebGLRenderingContextBase.h (right): https://codereview.chromium.org/1205573003/diff/60001/Source/core/html/canvas... Source/core/html/canvas/WebGLRenderingContextBase.h:572: GLenum m_readbufferOfFBO; Now from our discussion, this should be part of the WebGLFramebuffer, as it is a per fbo state.
On 2015/07/01 02:33:27, Zhenyao Mo wrote:
> On 2015/07/01 00:28:29, Ken Russell wrote:
> > I think there are remaining problems here: for example, what happens in the
> > following situation?
> >
> > // No framebuffer object is initially bound
> > gl.readBuffer(BACK); // This translates to a call to
> > glReadBuffer(GL_COLOR_ATTACHMENT_0)
> > gl.bindFramebuffer(userDefinedFBO); // which has a color attachment
> > gl.readPixels(...); // or another operation using the read buffer
> >
> > This should generate an INVALID_OPERATION error (though this isn't clearly
> > covered by the ES 3.0 spec). With this patch it'll probably succeed.
>
> Actually I think we don't need to generate an error here. ReadBuffer
operation,
> in my reading of the spec, is per framebuffer. So when you bind to a new
> framebuffer here, its readbuffer isn't BACK, it is whatever was selected
before,
> or COLOR_ATTACHMENT0 by default.
>
> I don't find explicit text for that, but there are some texts saying the
default
> readbuffer for backbuffer is BACK, and default readbuffer for a user defined
> framebuffer is COLOR_ATTACHMENT0. That means readbuffer setting is per
> framebuffer.
Ah! Good point. I think you're right.
The state tables in the spec give the definitive answer, and they match your
understanding; see "Table 6.13: Framebuffer (state per framebuffer object)".
READ_BUFFER is a per-framebuffer state.
> > Can you please add a test for this case?
> >
> > Can you please propose how we can fix this problem? One way would be to
simply
> > cache the argument passed to readBuffer, and both validate the state and
pass
> > the correct argument down to the WebGraphicsContext3D upon every call which
> > might use the bound read buffer (readPixels, blitFramebuffer,
> > copyTex{Sub}Image2D, others?)
https://codereview.chromium.org/1205573003/diff/60001/Source/core/html/canvas... File Source/core/html/canvas/WebGLRenderingContextBase.h (right): https://codereview.chromium.org/1205573003/diff/60001/Source/core/html/canvas... Source/core/html/canvas/WebGLRenderingContextBase.h:572: GLenum m_readbufferOfFBO; On 2015/07/01 15:21:27, Zhenyao Mo wrote: > Now from our discussion, this should be part of the WebGLFramebuffer, as it is a > per fbo state. Right. Thanks Mo for finding this. Yunchao, could you make this change, and also cache the readBuffer for the default framebuffer here? Could you please also add some tests? For example, have two framebuffers, one with COLOR_ATTACHMENT0 and one with both COLOR_ATTACHMENT0 and COLOR_ATTACHMENT1. Set READ_BUFFER to COLOR_ATTACHMENT0 for the first and COLOR_ATTACHMENT1 for the second. Ensure that calling bindFramebuffer and then readPixels for each has the expected behavior.
Patchset #4 (id:100001) has been deleted
On 2015/07/01 17:24:07, Ken Russell wrote: > https://codereview.chromium.org/1205573003/diff/60001/Source/core/html/canvas... > File Source/core/html/canvas/WebGLRenderingContextBase.h (right): > > https://codereview.chromium.org/1205573003/diff/60001/Source/core/html/canvas... > Source/core/html/canvas/WebGLRenderingContextBase.h:572: GLenum > m_readbufferOfFBO; > On 2015/07/01 15:21:27, Zhenyao Mo wrote: > > Now from our discussion, this should be part of the WebGLFramebuffer, as it is > a > > per fbo state. > > Right. Thanks Mo for finding this. Yunchao, could you make this change, and also > cache the readBuffer for the default framebuffer here? > > Could you please also add some tests? For example, have two framebuffers, one > with COLOR_ATTACHMENT0 and one with both COLOR_ATTACHMENT0 and > COLOR_ATTACHMENT1. Set READ_BUFFER to COLOR_ATTACHMENT0 for the first and > COLOR_ATTACHMENT1 for the second. Ensure that calling bindFramebuffer and then > readPixels for each has the expected behavior. Please see the conformance test: https://github.com/KhronosGroup/WebGL/pull/1089
On 2015/07/02 14:58:31, yunchao wrote: > On 2015/07/01 17:24:07, Ken Russell wrote: > > > https://codereview.chromium.org/1205573003/diff/60001/Source/core/html/canvas... > > File Source/core/html/canvas/WebGLRenderingContextBase.h (right): > > > > > https://codereview.chromium.org/1205573003/diff/60001/Source/core/html/canvas... > > Source/core/html/canvas/WebGLRenderingContextBase.h:572: GLenum > > m_readbufferOfFBO; > > On 2015/07/01 15:21:27, Zhenyao Mo wrote: > > > Now from our discussion, this should be part of the WebGLFramebuffer, as it > is > > a > > > per fbo state. > > > > Right. Thanks Mo for finding this. Yunchao, could you make this change, and > also > > cache the readBuffer for the default framebuffer here? > > > > Could you please also add some tests? For example, have two framebuffers, one > > with COLOR_ATTACHMENT0 and one with both COLOR_ATTACHMENT0 and > > COLOR_ATTACHMENT1. Set READ_BUFFER to COLOR_ATTACHMENT0 for the first and > > COLOR_ATTACHMENT1 for the second. Ensure that calling bindFramebuffer and then > > readPixels for each has the expected behavior. > > Please see the conformance test: https://github.com/KhronosGroup/WebGL/pull/1089 Sorry, please review this one: https://github.com/KhronosGroup/WebGL/pull/1090. I logged my private github account at home and pulled request...
Please see the comments inline. Thanks a lot! https://codereview.chromium.org/1205573003/diff/60001/Source/core/html/canvas... File Source/core/html/canvas/WebGLRenderingContextBase.h (right): https://codereview.chromium.org/1205573003/diff/60001/Source/core/html/canvas... Source/core/html/canvas/WebGLRenderingContextBase.h:572: GLenum m_readbufferOfFBO; On 2015/07/01 17:24:06, Ken Russell wrote: > On 2015/07/01 15:21:27, Zhenyao Mo wrote: > > Now from our discussion, this should be part of the WebGLFramebuffer, as it is > a > > per fbo state. > > Right. Thanks Mo for finding this. Yunchao, could you make this change, and also > cache the readBuffer for the default framebuffer here? Ken, I updated the patch locally, found that caching readbuffer for the default fb is not necessary (but caching readbuffer for fbo will be used to check whether the attachment exists before reading from fbo by copyTexImage2D/readPixels/...), so I prefer to not cache readbuffer for default fb, WDYT? > > Could you please also add some tests? For example, have two framebuffers, one > with COLOR_ATTACHMENT0 and one with both COLOR_ATTACHMENT0 and > COLOR_ATTACHMENT1. Set READ_BUFFER to COLOR_ATTACHMENT0 for the first and > COLOR_ATTACHMENT1 for the second. Ensure that calling bindFramebuffer and then > readPixels for each has the expected behavior. https://codereview.chromium.org/1205573003/diff/60001/Source/core/html/canvas... Source/core/html/canvas/WebGLRenderingContextBase.h:572: GLenum m_readbufferOfFBO; On 2015/07/01 15:21:27, Zhenyao Mo wrote: > Now from our discussion, this should be part of the WebGLFramebuffer, as it is a > per fbo state. Acknowledged.
https://codereview.chromium.org/1205573003/diff/120001/Source/core/html/canva... File Source/core/html/canvas/WebGL2RenderingContextBase.cpp (right): https://codereview.chromium.org/1205573003/diff/120001/Source/core/html/canva... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:204: m_readBuffer = mode; I think it's better to cache the original value here instead of the mapped ones, but it's just my opinion. See from your code, it actually doesn't make a difference in logic. https://codereview.chromium.org/1205573003/diff/120001/Source/core/html/canva... File Source/core/html/canvas/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1205573003/diff/120001/Source/core/html/canva... Source/core/html/canvas/WebGLRenderingContextBase.cpp:1556: return true; Shouldn't you also check here that the back buffer's readbuffer is not NONE? in which case, you should define m_readbuffer in this base class, and its value will just stay GL_BACK in WebGL 1.
https://codereview.chromium.org/1205573003/diff/120001/Source/core/html/canva... File Source/core/html/canvas/WebGL2RenderingContextBase.cpp (right): https://codereview.chromium.org/1205573003/diff/120001/Source/core/html/canva... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:204: m_readBuffer = mode; On 2015/07/06 17:52:25, Zhenyao Mo wrote: > I think it's better to cache the original value here instead of the mapped ones, > but it's just my opinion. See from your code, it actually doesn't make a > difference in logic. I agree with Mo; it's clearer if the cache holds the un-translated value. Also, m_readBuffer is not currently ever fetched. See below. https://codereview.chromium.org/1205573003/diff/120001/Source/core/html/canva... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:1821: value = GL_BACK; The point of caching m_readBuffer is so that its value can be used here instead of calling getIntegerv and doing this somewhat error-prone translation. Could you please modify this code to return the readBuffer from the bound read framebuffer (if any), or m_readBuffer if there is none? https://codereview.chromium.org/1205573003/diff/120001/Source/core/html/canva... File Source/core/html/canvas/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1205573003/diff/120001/Source/core/html/canva... Source/core/html/canvas/WebGLRenderingContextBase.cpp:1556: return true; On 2015/07/06 17:52:25, Zhenyao Mo wrote: > Shouldn't you also check here that the back buffer's readbuffer is not NONE? in > which case, you should define m_readbuffer in this base class, and its value > will just stay GL_BACK in WebGL 1. Agree with Mo's observation, though if the underlying GL implementation will catch this (since the call to glReadBuffer(GL_NONE) will go through to the driver), it's OK to not explicitly check it here. I generally agree with Mo's observation to move m_readBuffer to the base class and initialize it to GL_BACK. Also, I think should be renamed to something like m_defaultFramebufferReadBuffer.
Thanks for your reviewing, zmo@ and kbr@. Code has been updated, PTAL. https://codereview.chromium.org/1205573003/diff/120001/Source/core/html/canva... File Source/core/html/canvas/WebGL2RenderingContextBase.cpp (right): https://codereview.chromium.org/1205573003/diff/120001/Source/core/html/canva... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:204: m_readBuffer = mode; On 2015/07/06 21:19:13, Ken Russell wrote: > On 2015/07/06 17:52:25, Zhenyao Mo wrote: > > I think it's better to cache the original value here instead of the mapped > ones, > > but it's just my opinion. See from your code, it actually doesn't make a > > difference in logic. > > I agree with Mo; it's clearer if the cache holds the un-translated value. Also, > m_readBuffer is not currently ever fetched. See below. Yeah. Agree with you two. It should cache the original value, and fetch the cached value in getter. https://codereview.chromium.org/1205573003/diff/120001/Source/core/html/canva... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:1821: value = GL_BACK; On 2015/07/06 21:19:13, Ken Russell wrote: > The point of caching m_readBuffer is so that its value can be used here instead > of calling getIntegerv and doing this somewhat error-prone translation. Could > you please modify this code to return the readBuffer from the bound read > framebuffer (if any), or m_readBuffer if there is none? Done. https://codereview.chromium.org/1205573003/diff/120001/Source/core/html/canva... File Source/core/html/canvas/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1205573003/diff/120001/Source/core/html/canva... Source/core/html/canvas/WebGLRenderingContextBase.cpp:1556: return true; On 2015/07/06 21:19:13, Ken Russell wrote: > On 2015/07/06 17:52:25, Zhenyao Mo wrote: > > Shouldn't you also check here that the back buffer's readbuffer is not NONE? > in > > which case, you should define m_readbuffer in this base class, and its value > > will just stay GL_BACK in WebGL 1. > > Agree with Mo's observation, though if the underlying GL implementation will > catch this (since the call to glReadBuffer(GL_NONE) will go through to the > driver), it's OK to not explicitly check it here. > > I generally agree with Mo's observation to move m_readBuffer to the base class > and initialize it to GL_BACK. Also, I think should be renamed to something like > m_defaultFramebufferReadBuffer. Done.
LGTM https://codereview.chromium.org/1205573003/diff/140001/Source/modules/webgl/W... File Source/modules/webgl/WebGL2RenderingContextBase.cpp (right): https://codereview.chromium.org/1205573003/diff/140001/Source/modules/webgl/W... Source/modules/webgl/WebGL2RenderingContextBase.cpp:1813: GLint value = 0; nit: this should be GLenum to match.
https://codereview.chromium.org/1205573003/diff/140001/Source/modules/webgl/W... File Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1205573003/diff/140001/Source/modules/webgl/W... Source/modules/webgl/WebGLRenderingContextBase.cpp:1560: return false; This must synthesize an INVALID_OPERATION error per the specification (Section 4.3.2 "Reading Pixels" in the ES 3.0.3 spec). Was this error not caught by your tests? If not, please add another test covering it.
PTAL. Ken, the test cases for GL_NONE is added too: https://github.com/KhronosGroup/WebGL/pull/1104 https://codereview.chromium.org/1205573003/diff/140001/Source/modules/webgl/W... File Source/modules/webgl/WebGL2RenderingContextBase.cpp (right): https://codereview.chromium.org/1205573003/diff/140001/Source/modules/webgl/W... Source/modules/webgl/WebGL2RenderingContextBase.cpp:1813: GLint value = 0; On 2015/07/07 17:20:31, Zhenyao Mo wrote: > nit: this should be GLenum to match. Done. https://codereview.chromium.org/1205573003/diff/140001/Source/modules/webgl/W... File Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1205573003/diff/140001/Source/modules/webgl/W... Source/modules/webgl/WebGLRenderingContextBase.cpp:1560: return false; On 2015/07/07 18:21:26, Ken Russell wrote: > This must synthesize an INVALID_OPERATION error per the specification (Section > 4.3.2 "Reading Pixels" in the ES 3.0.3 spec). Was this error not caught by your > tests? If not, please add another test covering it. Done.
Thanks for following through on this. LGTM
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 Link to the patchset: https://codereview.chromium.org/1205573003/#ps160001 (title: "addressed kbr's feedback: reading from framebuffer with GL_NONE is invalid")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1205573003/160001
Message was sent while issue was closed.
Committed patchset #6 (id:160001) as https://src.chromium.org/viewvc/blink?view=rev&revision=198484 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
