Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1300573002:
WebGL 2: add readPixels API to read pixels into pixel pack buffer (Closed)
DescriptionWebGL 2: add readPixels API to read pixels into pixel pack buffer
BUG=295792
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201599
Patch Set 1 : #
Total comments: 4
Patch Set 2 : move the new readPixels to WebGL 2 #
Total comments: 8
Patch Set 3 : addressed zmo@'s feedback #Patch Set 4 : offset should not be less than 0 #
Total comments: 9
Patch Set 5 : addressed zmo@'s and kbr@'s feedback #
Total comments: 1
Patch Set 6 : code refactoring to fix bugs in conformance test #
Messages
Total messages: 33 (11 generated)
Patchset #1 (id:1) has been deleted
yunchao.he@intel.com changed reviewers: + bajones@chromium.org, kbr@chromium.org, zmo@chromium.org
This new readPixel API should be only valid in WebGL 2 rendering context. However, seems that cross-class overloading (the original WebGLRenderingContext::readPixel and the new added WebGL2RenderingContext::readPixel) is not possible. So I put the new readPixel API into WebGLRenderingContext... Not very sure whether this is correct. PTAL when you have time. Corresponding conformance test will be submitted soon.
https://codereview.chromium.org/1300573002/diff/20001/Source/modules/webgl/We... File Source/modules/webgl/WebGLRenderingContextBase.idl (right): https://codereview.chromium.org/1300573002/diff/20001/Source/modules/webgl/We... Source/modules/webgl/WebGLRenderingContextBase.idl:601: void readPixels(GLint x, GLint y, GLsizei width, GLsizei height, GLenum format, GLenum type, GLintptr offset); No, you can't expose this to WebGL 1. It is a WebGL 2 only function.
Thanks for your reviewing, zmo@. Please see the comments inline. Thanks a lot! https://codereview.chromium.org/1300573002/diff/20001/Source/modules/webgl/We... File Source/modules/webgl/WebGLRenderingContextBase.idl (right): https://codereview.chromium.org/1300573002/diff/20001/Source/modules/webgl/We... Source/modules/webgl/WebGLRenderingContextBase.idl:601: void readPixels(GLint x, GLint y, GLsizei width, GLsizei height, GLenum format, GLenum type, GLintptr offset); On 2015/08/17 17:02:47, Zhenyao Mo wrote: > No, you can't expose this to WebGL 1. It is a WebGL 2 only function. In the implementation, we can directly return and do nothing under WebGL 1 rendering context. So this API will not be exposed to the web developers. WDYT? Another solution is that we can use the [Custom] tag in the IDL file, and manually write the binding code under WebKit/Source/bindings/core/v8/custom/. Final solution I can imagine: 1. create a new attribute, say [CrossClassOverload], in IDL file. 2. write python code to generate appropriate js binding code to support cross-class overloading, although cross-class overloading is not syntactically correct in C++. Any comments? Maybe I am wrong...
https://codereview.chromium.org/1300573002/diff/20001/Source/modules/webgl/We... File Source/modules/webgl/WebGLRenderingContextBase.idl (right): https://codereview.chromium.org/1300573002/diff/20001/Source/modules/webgl/We... Source/modules/webgl/WebGLRenderingContextBase.idl:601: void readPixels(GLint x, GLint y, GLsizei width, GLsizei height, GLenum format, GLenum type, GLintptr offset); On 2015/08/18 02:30:00, yunchao wrote: > On 2015/08/17 17:02:47, Zhenyao Mo wrote: > > No, you can't expose this to WebGL 1. It is a WebGL 2 only function. > > In the implementation, we can directly return and do nothing under WebGL 1 > rendering context. So this API will not be exposed to the web developers. WDYT? I'm not sure that this would be completely invisible to JavaScript. > Another solution is that we can use the [Custom] tag in the IDL file, and > manually write the binding code under WebKit/Source/bindings/core/v8/custom/. Why is this necessary? > Final solution I can imagine: > 1. create a new attribute, say [CrossClassOverload], in IDL file. Why can this not just be added to WebGL2RenderingContextBase.idl? Is there a deficiency in the code generator? > 2. write python code to generate appropriate js binding code to support > cross-class overloading, although cross-class overloading is not syntactically > correct in C++. What is the issue with cross-class overloading? WebGLRenderingContextBase.h would define one overloading of readPixels and WebGL2RenderingContextBase.h would define the other overload.
Patchset #2 (id:40001) has been deleted
Thanks for your reviewing, kbr@ and zmo@. I have resolved the cross-class overloading problem. PTAL. Thanks! https://codereview.chromium.org/1300573002/diff/20001/Source/modules/webgl/We... File Source/modules/webgl/WebGLRenderingContextBase.idl (right): https://codereview.chromium.org/1300573002/diff/20001/Source/modules/webgl/We... Source/modules/webgl/WebGLRenderingContextBase.idl:601: void readPixels(GLint x, GLint y, GLsizei width, GLsizei height, GLenum format, GLenum type, GLintptr offset); On 2015/08/18 02:55:48, Ken Russell wrote: > On 2015/08/18 02:30:00, yunchao wrote: > > On 2015/08/17 17:02:47, Zhenyao Mo wrote: > > > No, you can't expose this to WebGL 1. It is a WebGL 2 only function. > > > > In the implementation, we can directly return and do nothing under WebGL 1 > > rendering context. So this API will not be exposed to the web developers. > WDYT? > > I'm not sure that this would be completely invisible to JavaScript. > > > Another solution is that we can use the [Custom] tag in the IDL file, and > > manually write the binding code under WebKit/Source/bindings/core/v8/custom/. > > Why is this necessary? > > > Final solution I can imagine: > > 1. create a new attribute, say [CrossClassOverload], in IDL file. > > Why can this not just be added to WebGL2RenderingContextBase.idl? Is there a > deficiency in the code generator? > > > 2. write python code to generate appropriate js binding code to support > > cross-class overloading, although cross-class overloading is not syntactically > > correct in C++. > > What is the issue with cross-class overloading? WebGLRenderingContextBase.h > would define one overloading of readPixels and WebGL2RenderingContextBase.h > would define the other overload. cross-class overloading is not allowed, it will always call the function in the sub-class, making the JS binding code failed (parameter mismatch: 'DOMArrayBufferView' to 'long long' in this case). Well, we can make the original WebGLRenderingContextBase::readPixels virtual, and override it in WebGL2RenderingContext. then WebGL2RenderingContext::readPixels(,,,, DOMArrayBufferView* pixels ) can correctly overloads with WebGL2RenderingContext(,,,,, long long offset), because these two readPixels are in the same class (WebGL2RenderingContext) now. Code has been updated, PTAL when you have time.
https://codereview.chromium.org/1300573002/diff/60001/Source/modules/webgl/We... File Source/modules/webgl/WebGLRenderingContextBase.h (right): https://codereview.chromium.org/1300573002/diff/60001/Source/modules/webgl/We... Source/modules/webgl/WebGLRenderingContextBase.h:127: class ScopedDrawingBufferBinder { I'd like to move ScopedDrawingBufferBinder from WebGLRenderingContext.cpp to WebGLRenderingContext.h, then all subclasses of WebGLRenderingContext can call this ScopedDrawingBufferBinder too.
Mostly look good, but some extra work is needed. Also, can you add a conformance test case for reading into PIXEL_PACK_BUFFER if the deqp tests do not cover it already? https://codereview.chromium.org/1300573002/diff/60001/Source/modules/webgl/We... File Source/modules/webgl/WebGL2RenderingContextBase.cpp (right): https://codereview.chromium.org/1300573002/diff/60001/Source/modules/webgl/We... Source/modules/webgl/WebGL2RenderingContextBase.cpp:267: There are some missing validations here. I think you need to expand the WebGLBuffer class to cache buffer size (from bufferData() call). We don't have to validate if the buffer is mapped, since we decided not to expose mapBuffferRange() function in WebGL 2 (probably add a comment in case that decision gets revisited in the future), but we do need to validate if the buffer size is large enough. You need to validate of offset is within buffer range, and then pass buffer size minus offset as the last arg to readPixelImpl(), it gets validated. https://codereview.chromium.org/1300573002/diff/60001/Source/modules/webgl/We... Source/modules/webgl/WebGL2RenderingContextBase.cpp:268: WebGLRenderingContextBase::readPixelsImpl(x, y, width, height, format, type, (void*)offset, 0); use static_cast instead of (void*). https://codereview.chromium.org/1300573002/diff/60001/Source/modules/webgl/We... File Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1300573002/diff/60001/Source/modules/webgl/We... Source/modules/webgl/WebGLRenderingContextBase.cpp:3705: bool WebGLRenderingContextBase::validateReadPixelsFuncParameters(GLsizei width, GLsizei height, GLenum format, GLenum type, unsigned domArrayBufferLength, WebGLFramebuffer*& readFramebufferBinding) I prefer not to return the readFramebufferBinding here because the function name doesn't indicate that. You can remove readFramebufferBinding from the argument in this function, and add another helper function getBoundReadFramebuffer(). It's a trivia operation, so you can call this whenever you need it. It's clearer this way. https://codereview.chromium.org/1300573002/diff/60001/Source/modules/webgl/We... File Source/modules/webgl/WebGLRenderingContextBase.h (right): https://codereview.chromium.org/1300573002/diff/60001/Source/modules/webgl/We... Source/modules/webgl/WebGLRenderingContextBase.h:856: void readPixelsImpl(GLint x, GLint y, GLsizei width, GLsizei height, GLenum format, GLenum type, void* data, unsigned domArrayBufferLength); Just name it bufferSize, so it can cover the case of PIXEL_PACK_BUFFER also.
Patchset #3 (id:80001) has been deleted
Patchset #3 (id:100001) has been deleted
Patchset #3 (id:120001) has been deleted
Thanks for your reviewing, zmo@. The code has been updated accordingly, PTAL. BTW, there is no corresponding test case in deqp in WebGL conformance test suite. I am writing them now. https://codereview.chromium.org/1300573002/diff/60001/Source/modules/webgl/We... File Source/modules/webgl/WebGL2RenderingContextBase.cpp (right): https://codereview.chromium.org/1300573002/diff/60001/Source/modules/webgl/We... Source/modules/webgl/WebGL2RenderingContextBase.cpp:267: On 2015/08/18 21:48:39, Zhenyao Mo wrote: > There are some missing validations here. I think you need to expand the > WebGLBuffer class to cache buffer size (from bufferData() call). We don't have > to validate if the buffer is mapped, since we decided not to expose > mapBuffferRange() function in WebGL 2 (probably add a comment in case that > decision gets revisited in the future), but we do need to validate if the buffer > size is large enough. You need to validate of offset is within buffer range, > and then pass buffer size minus offset as the last arg to readPixelImpl(), it > gets validated. Yeah, I have written the code to validate the buffer access is within the buffer range. The spec also have related statement. but when I looked into bufferSubData, getBufferSubData and so forth, I found that they doesn't check this. I should obey to the WebGL spec, not to the code in Blink. It is done now. I will update the other functions later. https://codereview.chromium.org/1300573002/diff/60001/Source/modules/webgl/We... Source/modules/webgl/WebGL2RenderingContextBase.cpp:268: WebGLRenderingContextBase::readPixelsImpl(x, y, width, height, format, type, (void*)offset, 0); On 2015/08/18 21:48:39, Zhenyao Mo wrote: > use static_cast instead of (void*). Thanks! I tried static_cast, but it failed to build. So I used reinterpret_cast instead. https://codereview.chromium.org/1300573002/diff/60001/Source/modules/webgl/We... File Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1300573002/diff/60001/Source/modules/webgl/We... Source/modules/webgl/WebGLRenderingContextBase.cpp:3705: bool WebGLRenderingContextBase::validateReadPixelsFuncParameters(GLsizei width, GLsizei height, GLenum format, GLenum type, unsigned domArrayBufferLength, WebGLFramebuffer*& readFramebufferBinding) On 2015/08/18 21:48:39, Zhenyao Mo wrote: > I prefer not to return the readFramebufferBinding here because the function name > doesn't indicate that. You can remove readFramebufferBinding from the argument > in this function, and add another helper function getBoundReadFramebuffer(). > It's a trivia operation, so you can call this whenever you need it. It's clearer > this way. Make sense. Done.
Patchset #4 (id:160001) has been deleted
LGTM https://codereview.chromium.org/1300573002/diff/180001/Source/modules/webgl/W... File Source/modules/webgl/WebGL2RenderingContextBase.cpp (right): https://codereview.chromium.org/1300573002/diff/180001/Source/modules/webgl/W... Source/modules/webgl/WebGL2RenderingContextBase.cpp:276: long long size = buffer->getSize() - offset; nit: add a note if offset is larger than buffer size, |size| is negative, and that case is handled by readPixelsImpl() to generate an INVALID_OPERATION. https://codereview.chromium.org/1300573002/diff/180001/Source/modules/webgl/W... File Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1300573002/diff/180001/Source/modules/webgl/W... Source/modules/webgl/WebGLRenderingContextBase.cpp:3745: WebGLFramebuffer* readFramebufferBinding = getFramebufferBinding(target); nit: ASSERT(readFramebufferBinding) here?
Thanks for this contribution and thanks Mo for doing the first round of reviews. LGTM too. https://codereview.chromium.org/1300573002/diff/180001/Source/modules/webgl/W... File Source/modules/webgl/WebGL2RenderingContextBase.cpp (right): https://codereview.chromium.org/1300573002/diff/180001/Source/modules/webgl/W... Source/modules/webgl/WebGL2RenderingContextBase.cpp:252: synthesizeGLError(GL_INVALID_OPERATION, "readPixels", "PIXEL_PACK buffer should not bound"); nit: "should not be bound" https://codereview.chromium.org/1300573002/diff/180001/Source/modules/webgl/W... File Source/modules/webgl/WebGL2RenderingContextBase.h (right): https://codereview.chromium.org/1300573002/diff/180001/Source/modules/webgl/W... Source/modules/webgl/WebGL2RenderingContextBase.h:171: void readPixels(GLint x, GLint y, GLsizei width, GLsizei height, GLenum format, GLenum type, DOMArrayBufferView* pixels) override; I'm a little surprised this is necessary but trust you that it is.
Thanks for your reviews, zmo@ and kbr@. zmo@, seems that there is a small question, please see my comments inline. https://codereview.chromium.org/1300573002/diff/180001/Source/modules/webgl/W... File Source/modules/webgl/WebGL2RenderingContextBase.cpp (right): https://codereview.chromium.org/1300573002/diff/180001/Source/modules/webgl/W... Source/modules/webgl/WebGL2RenderingContextBase.cpp:252: synthesizeGLError(GL_INVALID_OPERATION, "readPixels", "PIXEL_PACK buffer should not bound"); On 2015/08/19 23:42:52, Ken Russell wrote: > nit: "should not be bound" Done. https://codereview.chromium.org/1300573002/diff/180001/Source/modules/webgl/W... Source/modules/webgl/WebGL2RenderingContextBase.cpp:276: long long size = buffer->getSize() - offset; On 2015/08/19 17:20:23, Zhenyao Mo wrote: > nit: add a note if offset is larger than buffer size, |size| is negative, and > that case is handled by readPixelsImpl() to generate an INVALID_OPERATION. Done. https://codereview.chromium.org/1300573002/diff/180001/Source/modules/webgl/W... File Source/modules/webgl/WebGL2RenderingContextBase.h (right): https://codereview.chromium.org/1300573002/diff/180001/Source/modules/webgl/W... Source/modules/webgl/WebGL2RenderingContextBase.h:171: void readPixels(GLint x, GLint y, GLsizei width, GLsizei height, GLenum format, GLenum type, DOMArrayBufferView* pixels) override; On 2015/08/19 23:42:52, Ken Russell wrote: > I'm a little surprised this is necessary but trust you that it is. :) https://codereview.chromium.org/1300573002/diff/180001/Source/modules/webgl/W... File Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1300573002/diff/180001/Source/modules/webgl/W... Source/modules/webgl/WebGLRenderingContextBase.cpp:3745: WebGLFramebuffer* readFramebufferBinding = getFramebufferBinding(target); On 2015/08/19 17:20:23, Zhenyao Mo wrote: > nit: ASSERT(readFramebufferBinding) here? Seems that we can't add this assertion. In my opinion, readFramebufferBinding can be nullptr, when readPixels reads from WebGL default framebuffer (an internal fbo). WDYT?
LGTM again https://codereview.chromium.org/1300573002/diff/180001/Source/modules/webgl/W... File Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1300573002/diff/180001/Source/modules/webgl/W... Source/modules/webgl/WebGLRenderingContextBase.cpp:3745: WebGLFramebuffer* readFramebufferBinding = getFramebufferBinding(target); On 2015/08/20 08:16:39, yunchao wrote: > On 2015/08/19 17:20:23, Zhenyao Mo wrote: > > nit: ASSERT(readFramebufferBinding) here? > > Seems that we can't add this assertion. In my opinion, readFramebufferBinding > can be nullptr, when readPixels reads from WebGL default framebuffer (an > internal fbo). WDYT? You are right.
To be clear, you don't need to wait for my re-review. Mo, thanks for doing such a thorough job on this review. Yunchao, please do make sure that there's a WebGL conformance test in place for this before CQ'ing it.
On 2015/08/25 02:06:55, Ken Russell wrote: > To be clear, you don't need to wait for my re-review. Mo, thanks for doing such > a thorough job on this review. Yunchao, please do make sure that there's a WebGL > conformance test in place for this before CQ'ing it. Got it. Thank you Ken. I will write conformance test before merging patch into Blink. The conformance test for this change has been submitted: https://github.com/KhronosGroup/WebGL/pull/1166. Please have a look. Currently, the conformance test failed in Chromium, because readPixels API in command buffer need to be updated too. I am working on this.
Patchset #6 (id:220001) has been deleted
A simple code refactoring to fix bugs. PTAL. Thanks! https://codereview.chromium.org/1300573002/diff/200001/Source/modules/webgl/W... File Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1300573002/diff/200001/Source/modules/webgl/W... Source/modules/webgl/WebGLRenderingContextBase.cpp:3762: DOMArrayBufferView::ViewType expectedViewType = readPixelsExpectedArrayBufferViewType(type); readPixelsExpectedArrayBufferViewType(type) supposes that the parameter 'type' is valid. So putting it before readPixelsImpl (who call validateReadPixelsFuncParameters to validate format / type) is not appropriate. It leads to failures in conformance test. So I removed the function readPixelsImpl() and did a code refactoring in the latest patch set.
The CQ bit was checked by yunchao.he@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1300573002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1300573002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2015/09/01 06:22:52, commit-bot: I haz the power wrote: > Dry run: This issue passed the CQ dry run. LGTM
LGTM again too. Thanks for following through on this.
On 2015/09/01 20:58:14, Ken Russell wrote: > LGTM again too. Thanks for following through on this. The conformance test has been merged into WebGL conformance test suite to trace the status of reading pixels into pixel pack buffer, So I will merge this one to avoid code rebase again and again. Some cases in the conformance test failed, because command buffer in Chromium can only support reading pixels into client memory directly. It can not read pixels into pixel pack buffer. I will add the code later.
The CQ bit was checked by yunchao.he@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1300573002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1300573002/240001
Message was sent while issue was closed.
Committed patchset #6 (id:240001) as https://src.chromium.org/viewvc/blink?view=rev&revision=201599 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
