Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1537403003:
WebGL2: add types to fix bugs for vertexAttribPointer (Closed)
DescriptionWebGL2: add types to fix bugs for vertexAttribPointer in NegativeVertexArrayAPI.
BUG=565347
TEST=deqp/functional/gles3/negativevertexarrayapi.html
Committed: https://crrev.com/5241acd1f09ce2d7f91dac386d1d6a0013e86ee3
Cr-Commit-Position: refs/heads/master@{#366928}
Patch Set 1 #Patch Set 2 : a small fix #
Total comments: 2
Patch Set 3 : addressed zmo@'s feedback #Patch Set 4 : addressed zmo@'s feedback #
Messages
Total messages: 27 (9 generated)
Description was changed from ========== WebGL2: add types to fix bugs for vertexAttribPointer BUG= ========== to ========== WebGL2: add types to fix bugs for vertexAttribPointer. BUG=565347 TEST=deqp/functional/gles3/negativevertexarrayapi.html ==========
yunchao.he@intel.com changed reviewers: + bajones@chromium.org, kbr@chromium.org, qiankun.miao@intel.com, zmo@chromium.org
Patchset #1 (id:1) has been deleted
Description was changed from ========== WebGL2: add types to fix bugs for vertexAttribPointer. BUG=565347 TEST=deqp/functional/gles3/negativevertexarrayapi.html ========== to ========== WebGL2: add types to fix bugs for vertexAttribPointer in NegativeVertexArrayAPI. BUG=565347 TEST=deqp/functional/gles3/negativevertexarrayapi.html ==========
PTAL. Thanks a lot!
https://codereview.chromium.org/1537403003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h (right): https://codereview.chromium.org/1537403003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h:484: virtual unsigned sizeInBytes(GLenum type, bool isIntegerAPI) const; I don't think it's a good idea to add isIntegerAPI only for the purpose of an ASSERT. It's better we do not introduce ES3 concepts to the base class.
Thanks for your review, Zhenyao. Code has been updated to address your feedback. Could you have a look? https://codereview.chromium.org/1537403003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h (right): https://codereview.chromium.org/1537403003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h:484: virtual unsigned sizeInBytes(GLenum type, bool isIntegerAPI) const; On 2015/12/22 19:15:51, Zhenyao Mo wrote: > I don't think it's a good idea to add isIntegerAPI only for the purpose of an > ASSERT. It's better we do not introduce ES3 concepts to the base class. Got it. Then I would like to remove 'isIntegerAPI' parameter. HALF_FLOAT, INT_2_10_10_10_REV and UNSIGNED_INT_2_10_10_10_REV types should be checked to generate INVALID_ENUM in vertexAttribIPointer according to WebGL spec (We have done this already in Chromium).
On 2015/12/23 00:20:38, yunchao wrote: > Thanks for your review, Zhenyao. Code has been updated to address your feedback. > Could you have a look? > > https://codereview.chromium.org/1537403003/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h > (right): > > https://codereview.chromium.org/1537403003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h:484: virtual > unsigned sizeInBytes(GLenum type, bool isIntegerAPI) const; > On 2015/12/22 19:15:51, Zhenyao Mo wrote: > > I don't think it's a good idea to add isIntegerAPI only for the purpose of an > > ASSERT. It's better we do not introduce ES3 concepts to the base class. > > Got it. Then I would like to remove 'isIntegerAPI' parameter. HALF_FLOAT, > INT_2_10_10_10_REV and UNSIGNED_INT_2_10_10_10_REV types should be checked to > generate INVALID_ENUM in vertexAttribIPointer according to WebGL spec (We have > done this already in Chromium). I think you need to add it on the blink side, not relying on command buffer to catch this. Otherwise you will trigger an ASSERTION in sizeInBytes() with a wrong type. Can you do it in this CL?
On 2015/12/23 00:30:43, Zhenyao Mo wrote: > On 2015/12/23 00:20:38, yunchao wrote: > > Thanks for your review, Zhenyao. Code has been updated to address your > feedback. > > Could you have a look? > > > > > https://codereview.chromium.org/1537403003/diff/40001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h > > (right): > > > > > https://codereview.chromium.org/1537403003/diff/40001/third_party/WebKit/Sour... > > third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h:484: > virtual > > unsigned sizeInBytes(GLenum type, bool isIntegerAPI) const; > > On 2015/12/22 19:15:51, Zhenyao Mo wrote: > > > I don't think it's a good idea to add isIntegerAPI only for the purpose of > an > > > ASSERT. It's better we do not introduce ES3 concepts to the base class. > > > > Got it. Then I would like to remove 'isIntegerAPI' parameter. HALF_FLOAT, > > INT_2_10_10_10_REV and UNSIGNED_INT_2_10_10_10_REV types should be checked to > > generate INVALID_ENUM in vertexAttribIPointer according to WebGL spec (We have > > done this already in Chromium). > > I think you need to add it on the blink side, not relying on command buffer to > catch this. Otherwise you will trigger an ASSERTION in sizeInBytes() with a > wrong type. Can you do it in this CL? Just checked the vertexAttribIPointer() in blink. It did check for type before calling sizeInBytes, except it overlimits the accepted types.
On 2015/12/23 00:30:43, Zhenyao Mo wrote: > On 2015/12/23 00:20:38, yunchao wrote: > > Thanks for your review, Zhenyao. Code has been updated to address your > feedback. > > Could you have a look? > > > > > https://codereview.chromium.org/1537403003/diff/40001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h > > (right): > > > > > https://codereview.chromium.org/1537403003/diff/40001/third_party/WebKit/Sour... > > third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h:484: > virtual > > unsigned sizeInBytes(GLenum type, bool isIntegerAPI) const; > > On 2015/12/22 19:15:51, Zhenyao Mo wrote: > > > I don't think it's a good idea to add isIntegerAPI only for the purpose of > an > > > ASSERT. It's better we do not introduce ES3 concepts to the base class. > > > > Got it. Then I would like to remove 'isIntegerAPI' parameter. HALF_FLOAT, > > INT_2_10_10_10_REV and UNSIGNED_INT_2_10_10_10_REV types should be checked to > > generate INVALID_ENUM in vertexAttribIPointer according to WebGL spec (We have > > done this already in Chromium). > > I think you need to add it on the blink side, not relying on command buffer to > catch this. Otherwise you will trigger an ASSERTION in sizeInBytes() with a > wrong type. Can you do it in this CL? Sorry, I didn't state this clearly. I mean Chromium/Blink. In fact, Blink side have done this.
On 2015/12/23 00:33:52, yunchao wrote: > On 2015/12/23 00:30:43, Zhenyao Mo wrote: > > On 2015/12/23 00:20:38, yunchao wrote: > > > Thanks for your review, Zhenyao. Code has been updated to address your > > feedback. > > > Could you have a look? > > > > > > > > > https://codereview.chromium.org/1537403003/diff/40001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h > > > (right): > > > > > > > > > https://codereview.chromium.org/1537403003/diff/40001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h:484: > > virtual > > > unsigned sizeInBytes(GLenum type, bool isIntegerAPI) const; > > > On 2015/12/22 19:15:51, Zhenyao Mo wrote: > > > > I don't think it's a good idea to add isIntegerAPI only for the purpose of > > an > > > > ASSERT. It's better we do not introduce ES3 concepts to the base class. > > > > > > Got it. Then I would like to remove 'isIntegerAPI' parameter. HALF_FLOAT, > > > INT_2_10_10_10_REV and UNSIGNED_INT_2_10_10_10_REV types should be checked > to > > > generate INVALID_ENUM in vertexAttribIPointer according to WebGL spec (We > have > > > done this already in Chromium). > > > > I think you need to add it on the blink side, not relying on command buffer to > > catch this. Otherwise you will trigger an ASSERTION in sizeInBytes() with a > > wrong type. Can you do it in this CL? > > Sorry, I didn't state this clearly. I mean Chromium/Blink. In fact, Blink side > have done this. But it misses a bunch of types, right? Looking for at the code, it seems to me sizeInBytes should just a helper non-member function, not a virtual member function, and it should just handle WebGL 1 and 2 types. For example, GL_FLOAT is added to WebGL 1 through extension and added to WebGL 1 by default, so it's already a mixed situation.
On 2015/12/23 00:38:58, Zhenyao Mo wrote: > On 2015/12/23 00:33:52, yunchao wrote: > > On 2015/12/23 00:30:43, Zhenyao Mo wrote: > > > On 2015/12/23 00:20:38, yunchao wrote: > > > > Thanks for your review, Zhenyao. Code has been updated to address your > > > feedback. > > > > Could you have a look? > > > > > > > > > > > > > > https://codereview.chromium.org/1537403003/diff/40001/third_party/WebKit/Sour... > > > > File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1537403003/diff/40001/third_party/WebKit/Sour... > > > > third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h:484: > > > virtual > > > > unsigned sizeInBytes(GLenum type, bool isIntegerAPI) const; > > > > On 2015/12/22 19:15:51, Zhenyao Mo wrote: > > > > > I don't think it's a good idea to add isIntegerAPI only for the purpose > of > > > an > > > > > ASSERT. It's better we do not introduce ES3 concepts to the base class. > > > > > > > > Got it. Then I would like to remove 'isIntegerAPI' parameter. HALF_FLOAT, > > > > INT_2_10_10_10_REV and UNSIGNED_INT_2_10_10_10_REV types should be checked > > to > > > > generate INVALID_ENUM in vertexAttribIPointer according to WebGL spec (We > > have > > > > done this already in Chromium). > > > > > > I think you need to add it on the blink side, not relying on command buffer > to > > > catch this. Otherwise you will trigger an ASSERTION in sizeInBytes() with a > > > wrong type. Can you do it in this CL? > > > > Sorry, I didn't state this clearly. I mean Chromium/Blink. In fact, Blink side > > have done this. > > But it misses a bunch of types, right? I checked with the man page (and the GLES 3.0.4 spec): https://www.khronos.org/opengles/sdk/docs/man3/html/glVertexAttribPointer.xhtml. Looks like the validation code in vertexAttrib'I'Pointer doesn't miss any type. Could you have a double check? > > Looking for at the code, it seems to me sizeInBytes should just a helper > non-member function, not a virtual member function, and it should just handle > WebGL 1 and 2 types. For example, GL_FLOAT is added to WebGL 1 through > extension and added to WebGL 1 by default, so it's already a mixed situation. GL_FLOAT is added into WebGL1's sizeInBytes, but it is invalid for vertexAttrib'I'Pointer. So it will not call into sizeInBytes with GL_FLOAT. WDYT?
On 2015/12/23 00:48:57, yunchao wrote: > On 2015/12/23 00:38:58, Zhenyao Mo wrote: > > On 2015/12/23 00:33:52, yunchao wrote: > > > On 2015/12/23 00:30:43, Zhenyao Mo wrote: > > > > On 2015/12/23 00:20:38, yunchao wrote: > > > > > Thanks for your review, Zhenyao. Code has been updated to address your > > > > feedback. > > > > > Could you have a look? > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1537403003/diff/40001/third_party/WebKit/Sour... > > > > > File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h > > > > > (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1537403003/diff/40001/third_party/WebKit/Sour... > > > > > third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h:484: > > > > virtual > > > > > unsigned sizeInBytes(GLenum type, bool isIntegerAPI) const; > > > > > On 2015/12/22 19:15:51, Zhenyao Mo wrote: > > > > > > I don't think it's a good idea to add isIntegerAPI only for the > purpose > > of > > > > an > > > > > > ASSERT. It's better we do not introduce ES3 concepts to the base > class. > > > > > > > > > > Got it. Then I would like to remove 'isIntegerAPI' parameter. > HALF_FLOAT, > > > > > INT_2_10_10_10_REV and UNSIGNED_INT_2_10_10_10_REV types should be > checked > > > to > > > > > generate INVALID_ENUM in vertexAttribIPointer according to WebGL spec > (We > > > have > > > > > done this already in Chromium). > > > > > > > > I think you need to add it on the blink side, not relying on command > buffer > > to > > > > catch this. Otherwise you will trigger an ASSERTION in sizeInBytes() with > a > > > > wrong type. Can you do it in this CL? > > > > > > Sorry, I didn't state this clearly. I mean Chromium/Blink. In fact, Blink > side > > > have done this. > > > > But it misses a bunch of types, right? > I checked with the man page (and the GLES 3.0.4 spec): > https://www.khronos.org/opengles/sdk/docs/man3/html/glVertexAttribPointer.xhtml. > Looks like the validation code in vertexAttrib'I'Pointer doesn't miss any type. > Could you have a double check? You are correct. My apology, I got it reversed. > > > > > Looking for at the code, it seems to me sizeInBytes should just a helper > > non-member function, not a virtual member function, and it should just handle > > WebGL 1 and 2 types. For example, GL_FLOAT is added to WebGL 1 through > > extension and added to WebGL 1 by default, so it's already a mixed situation. > GL_FLOAT is added into WebGL1's sizeInBytes, but it is invalid for > vertexAttrib'I'Pointer. So it will not call into sizeInBytes with GL_FLOAT. > WDYT? My point is, if WebGL1/2 VertexAttribPointer and WebGL2 VertexAttribIPointer all validate type before calling sizeInBytes, then sizeInBytes should not care and should just be a simple helper function that accepts all types and return their sizes. WDYT?
On 2015/12/23 00:58:31, Zhenyao Mo wrote: > On 2015/12/23 00:48:57, yunchao wrote: > > On 2015/12/23 00:38:58, Zhenyao Mo wrote: > > > On 2015/12/23 00:33:52, yunchao wrote: > > > > On 2015/12/23 00:30:43, Zhenyao Mo wrote: > > > > > On 2015/12/23 00:20:38, yunchao wrote: > > > > > > Thanks for your review, Zhenyao. Code has been updated to address your > > > > > feedback. > > > > > > Could you have a look? > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1537403003/diff/40001/third_party/WebKit/Sour... > > > > > > File > third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h > > > > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1537403003/diff/40001/third_party/WebKit/Sour... > > > > > > > third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h:484: > > > > > virtual > > > > > > unsigned sizeInBytes(GLenum type, bool isIntegerAPI) const; > > > > > > On 2015/12/22 19:15:51, Zhenyao Mo wrote: > > > > > > > I don't think it's a good idea to add isIntegerAPI only for the > > purpose > > > of > > > > > an > > > > > > > ASSERT. It's better we do not introduce ES3 concepts to the base > > class. > > > > > > > > > > > > Got it. Then I would like to remove 'isIntegerAPI' parameter. > > HALF_FLOAT, > > > > > > INT_2_10_10_10_REV and UNSIGNED_INT_2_10_10_10_REV types should be > > checked > > > > to > > > > > > generate INVALID_ENUM in vertexAttribIPointer according to WebGL spec > > (We > > > > have > > > > > > done this already in Chromium). > > > > > > > > > > I think you need to add it on the blink side, not relying on command > > buffer > > > to > > > > > catch this. Otherwise you will trigger an ASSERTION in sizeInBytes() > with > > a > > > > > wrong type. Can you do it in this CL? > > > > > > > > Sorry, I didn't state this clearly. I mean Chromium/Blink. In fact, Blink > > side > > > > have done this. > > > > > > But it misses a bunch of types, right? > > I checked with the man page (and the GLES 3.0.4 spec): > > > https://www.khronos.org/opengles/sdk/docs/man3/html/glVertexAttribPointer.xhtml. > > Looks like the validation code in vertexAttrib'I'Pointer doesn't miss any > type. > > Could you have a double check? > > You are correct. My apology, I got it reversed. > > > > > > > > > Looking for at the code, it seems to me sizeInBytes should just a helper > > > non-member function, not a virtual member function, and it should just > handle > > > WebGL 1 and 2 types. For example, GL_FLOAT is added to WebGL 1 through > > > extension and added to WebGL 1 by default, so it's already a mixed > situation. > > GL_FLOAT is added into WebGL1's sizeInBytes, but it is invalid for > > vertexAttrib'I'Pointer. So it will not call into sizeInBytes with GL_FLOAT. > > WDYT? > > My point is, if WebGL1/2 VertexAttribPointer and WebGL2 VertexAttribIPointer all > validate type before calling sizeInBytes, then sizeInBytes should not care and > should just be a simple helper function that accepts all types and return their > sizes. WDYT? It really doesn't need to be a member function, not to say virtual member function.
On 2015/12/23 00:59:03, Zhenyao Mo wrote: > On 2015/12/23 00:58:31, Zhenyao Mo wrote: > > On 2015/12/23 00:48:57, yunchao wrote: > > > On 2015/12/23 00:38:58, Zhenyao Mo wrote: > > > > On 2015/12/23 00:33:52, yunchao wrote: > > > > > On 2015/12/23 00:30:43, Zhenyao Mo wrote: > > > > > > On 2015/12/23 00:20:38, yunchao wrote: > > > > > > > Thanks for your review, Zhenyao. Code has been updated to address > your > > > > > > feedback. > > > > > > > Could you have a look? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1537403003/diff/40001/third_party/WebKit/Sour... > > > > > > > File > > third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h > > > > > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1537403003/diff/40001/third_party/WebKit/Sour... > > > > > > > > > third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h:484: > > > > > > virtual > > > > > > > unsigned sizeInBytes(GLenum type, bool isIntegerAPI) const; > > > > > > > On 2015/12/22 19:15:51, Zhenyao Mo wrote: > > > > > > > > I don't think it's a good idea to add isIntegerAPI only for the > > > purpose > > > > of > > > > > > an > > > > > > > > ASSERT. It's better we do not introduce ES3 concepts to the base > > > class. > > > > > > > > > > > > > > Got it. Then I would like to remove 'isIntegerAPI' parameter. > > > HALF_FLOAT, > > > > > > > INT_2_10_10_10_REV and UNSIGNED_INT_2_10_10_10_REV types should be > > > checked > > > > > to > > > > > > > generate INVALID_ENUM in vertexAttribIPointer according to WebGL > spec > > > (We > > > > > have > > > > > > > done this already in Chromium). > > > > > > > > > > > > I think you need to add it on the blink side, not relying on command > > > buffer > > > > to > > > > > > catch this. Otherwise you will trigger an ASSERTION in sizeInBytes() > > with > > > a > > > > > > wrong type. Can you do it in this CL? > > > > > > > > > > Sorry, I didn't state this clearly. I mean Chromium/Blink. In fact, > Blink > > > side > > > > > have done this. > > > > > > > > But it misses a bunch of types, right? > > > I checked with the man page (and the GLES 3.0.4 spec): > > > > > > https://www.khronos.org/opengles/sdk/docs/man3/html/glVertexAttribPointer.xhtml. > > > Looks like the validation code in vertexAttrib'I'Pointer doesn't miss any > > type. > > > Could you have a double check? > > > > You are correct. My apology, I got it reversed. > > > > > > > > > > > > > Looking for at the code, it seems to me sizeInBytes should just a helper > > > > non-member function, not a virtual member function, and it should just > > handle > > > > WebGL 1 and 2 types. For example, GL_FLOAT is added to WebGL 1 through > > > > extension and added to WebGL 1 by default, so it's already a mixed > > situation. > > > GL_FLOAT is added into WebGL1's sizeInBytes, but it is invalid for > > > vertexAttrib'I'Pointer. So it will not call into sizeInBytes with GL_FLOAT. > > > WDYT? > > > > My point is, if WebGL1/2 VertexAttribPointer and WebGL2 VertexAttribIPointer > all > > validate type before calling sizeInBytes, then sizeInBytes should not care and > > should just be a simple helper function that accepts all types and return > their > > sizes. WDYT? > > It really doesn't need to be a member function, not to say virtual member > function. Got it. Totally agree with you. Will update the code soon. Zhenyao.
On 2015/12/23 01:00:56, yunchao wrote: > On 2015/12/23 00:59:03, Zhenyao Mo wrote: > > On 2015/12/23 00:58:31, Zhenyao Mo wrote: > > > On 2015/12/23 00:48:57, yunchao wrote: > > > > On 2015/12/23 00:38:58, Zhenyao Mo wrote: > > > > > On 2015/12/23 00:33:52, yunchao wrote: > > > > > > On 2015/12/23 00:30:43, Zhenyao Mo wrote: > > > > > > > On 2015/12/23 00:20:38, yunchao wrote: > > > > > > > > Thanks for your review, Zhenyao. Code has been updated to address > > your > > > > > > > feedback. > > > > > > > > Could you have a look? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1537403003/diff/40001/third_party/WebKit/Sour... > > > > > > > > File > > > third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h > > > > > > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1537403003/diff/40001/third_party/WebKit/Sour... > > > > > > > > > > > third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h:484: > > > > > > > virtual > > > > > > > > unsigned sizeInBytes(GLenum type, bool isIntegerAPI) const; > > > > > > > > On 2015/12/22 19:15:51, Zhenyao Mo wrote: > > > > > > > > > I don't think it's a good idea to add isIntegerAPI only for the > > > > purpose > > > > > of > > > > > > > an > > > > > > > > > ASSERT. It's better we do not introduce ES3 concepts to the > base > > > > class. > > > > > > > > > > > > > > > > Got it. Then I would like to remove 'isIntegerAPI' parameter. > > > > HALF_FLOAT, > > > > > > > > INT_2_10_10_10_REV and UNSIGNED_INT_2_10_10_10_REV types should be > > > > checked > > > > > > to > > > > > > > > generate INVALID_ENUM in vertexAttribIPointer according to WebGL > > spec > > > > (We > > > > > > have > > > > > > > > done this already in Chromium). > > > > > > > > > > > > > > I think you need to add it on the blink side, not relying on command > > > > buffer > > > > > to > > > > > > > catch this. Otherwise you will trigger an ASSERTION in sizeInBytes() > > > with > > > > a > > > > > > > wrong type. Can you do it in this CL? > > > > > > > > > > > > Sorry, I didn't state this clearly. I mean Chromium/Blink. In fact, > > Blink > > > > side > > > > > > have done this. > > > > > > > > > > But it misses a bunch of types, right? > > > > I checked with the man page (and the GLES 3.0.4 spec): > > > > > > > > > > https://www.khronos.org/opengles/sdk/docs/man3/html/glVertexAttribPointer.xhtml. > > > > Looks like the validation code in vertexAttrib'I'Pointer doesn't miss any > > > type. > > > > Could you have a double check? > > > > > > You are correct. My apology, I got it reversed. > > > > > > > > > > > > > > > > > Looking for at the code, it seems to me sizeInBytes should just a helper > > > > > non-member function, not a virtual member function, and it should just > > > handle > > > > > WebGL 1 and 2 types. For example, GL_FLOAT is added to WebGL 1 through > > > > > extension and added to WebGL 1 by default, so it's already a mixed > > > situation. > > > > GL_FLOAT is added into WebGL1's sizeInBytes, but it is invalid for > > > > vertexAttrib'I'Pointer. So it will not call into sizeInBytes with > GL_FLOAT. > > > > WDYT? > > > > > > My point is, if WebGL1/2 VertexAttribPointer and WebGL2 VertexAttribIPointer > > all > > > validate type before calling sizeInBytes, then sizeInBytes should not care > and > > > should just be a simple helper function that accepts all types and return > > their > > > sizes. WDYT? > > > > It really doesn't need to be a member function, not to say virtual member > > function. > > Got it. Totally agree with you. Will update the code soon. Zhenyao. Code has been updated now...
lgtm
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/1537403003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1537403003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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/1537403003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1537403003/80001
Message was sent while issue was closed.
Description was changed from ========== WebGL2: add types to fix bugs for vertexAttribPointer in NegativeVertexArrayAPI. BUG=565347 TEST=deqp/functional/gles3/negativevertexarrayapi.html ========== to ========== WebGL2: add types to fix bugs for vertexAttribPointer in NegativeVertexArrayAPI. BUG=565347 TEST=deqp/functional/gles3/negativevertexarrayapi.html ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== WebGL2: add types to fix bugs for vertexAttribPointer in NegativeVertexArrayAPI. BUG=565347 TEST=deqp/functional/gles3/negativevertexarrayapi.html ========== to ========== WebGL2: add types to fix bugs for vertexAttribPointer in NegativeVertexArrayAPI. BUG=565347 TEST=deqp/functional/gles3/negativevertexarrayapi.html Committed: https://crrev.com/5241acd1f09ce2d7f91dac386d1d6a0013e86ee3 Cr-Commit-Position: refs/heads/master@{#366928} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/5241acd1f09ce2d7f91dac386d1d6a0013e86ee3 Cr-Commit-Position: refs/heads/master@{#366928} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
