Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1708983002:
Command buffer: Fix bugs for drawing when the type of vertex attrib is a packed type (Closed)
DescriptionCommand buffer: Fix bugs for drawing when the type of vertex attrib is a packed type.
This small change fixed bugs in draw.html and vertexarrays.html in WebGL deqp
for GL_INT_2_10_10_10_REV and GL_UNSIGNED_INT_2_10_10_10_REV.
BUG=295792
TESTS=deqp/functional/gles3/draw.html, deqp/functional/gles3/vertexarrays.html
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel
Committed: https://crrev.com/6dc5f17704d3b1465ab48f25a11b7c9979892df3
Cr-Commit-Position: refs/heads/master@{#377836}
Patch Set 1 #Patch Set 2 : #
Total comments: 9
Patch Set 3 : addressed kbr@'s feedback: add some comments to explain the logic #
Total comments: 2
Patch Set 4 : addressed zmo@'s feedback #
Total comments: 2
Patch Set 5 : #
Messages
Total messages: 39 (19 generated)
Description was changed from ========== Fix bug for drawing when the type is packed type BUG= ========== to ========== Fix bug for drawing when the type is packed type BUG= CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Description was changed from ========== Fix bug for drawing when the type is packed type BUG= CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Fix bug for drawing when the type is packed type BUG=295792 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Description was changed from ========== Fix bug for drawing when the type is packed type BUG=295792 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Fix bug for drawing when the type is a packed type. This small change fixed some bugs in draw.html and vertexarrays.html in WebGL deqp tests for GL_INT_2_10_10_10_REV and GL_UNSIGNED_INT_2_10_10_10_REV BUG=295792 TESTS=deqp/functional/gles3/draw.html, deqp/functional/gles3/vertexarrays.html CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Description was changed from ========== Fix bug for drawing when the type is a packed type. This small change fixed some bugs in draw.html and vertexarrays.html in WebGL deqp tests for GL_INT_2_10_10_10_REV and GL_UNSIGNED_INT_2_10_10_10_REV BUG=295792 TESTS=deqp/functional/gles3/draw.html, deqp/functional/gles3/vertexarrays.html CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Fix bug for drawing when the type is a packed type. This small change fixed some bugs in draw.html and vertexarrays.html in WebGL deqp tests for GL_INT_2_10_10_10_REV and GL_UNSIGNED_INT_2_10_10_10_REV BUG=295792 TESTS=deqp/functional/gles3/draw.html, deqp/functional/gles3/vertexarrays.html CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Description was changed from ========== Fix bug for drawing when the type is a packed type. This small change fixed some bugs in draw.html and vertexarrays.html in WebGL deqp tests for GL_INT_2_10_10_10_REV and GL_UNSIGNED_INT_2_10_10_10_REV BUG=295792 TESTS=deqp/functional/gles3/draw.html, deqp/functional/gles3/vertexarrays.html CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Fix bug for drawing when the type is a packed type. This small change fixed bugs in draw.html and vertexarrays.html in WebGL deqp tests for GL_INT_2_10_10_10_REV and GL_UNSIGNED_INT_2_10_10_10_REV BUG=295792 TESTS=deqp/functional/gles3/draw.html, deqp/functional/gles3/vertexarrays.html CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Description was changed from ========== Fix bug for drawing when the type is a packed type. This small change fixed bugs in draw.html and vertexarrays.html in WebGL deqp tests for GL_INT_2_10_10_10_REV and GL_UNSIGNED_INT_2_10_10_10_REV BUG=295792 TESTS=deqp/functional/gles3/draw.html, deqp/functional/gles3/vertexarrays.html CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Fix bug for drawing when the type is a packed type. This small change fixed bugs in draw.html and vertexarrays.html in WebGL deqp tests for GL_INT_2_10_10_10_REV and GL_UNSIGNED_INT_2_10_10_10_REV BUG=295792 TESTS=deqp/functional/gles3/draw.html, deqp/functional/gles3/vertexarrays.html CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Description was changed from ========== Fix bug for drawing when the type is a packed type. This small change fixed bugs in draw.html and vertexarrays.html in WebGL deqp tests for GL_INT_2_10_10_10_REV and GL_UNSIGNED_INT_2_10_10_10_REV BUG=295792 TESTS=deqp/functional/gles3/draw.html, deqp/functional/gles3/vertexarrays.html CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== WebGL 2: Fix bugs for drawing when the type is a packed type. This small change fixed bugs in draw.html and vertexarrays.html in WebGL deqp tests for GL_INT_2_10_10_10_REV and GL_UNSIGNED_INT_2_10_10_10_REV BUG=295792 TESTS=deqp/functional/gles3/draw.html, deqp/functional/gles3/vertexarrays.html CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Description was changed from ========== WebGL 2: Fix bugs for drawing when the type is a packed type. This small change fixed bugs in draw.html and vertexarrays.html in WebGL deqp tests for GL_INT_2_10_10_10_REV and GL_UNSIGNED_INT_2_10_10_10_REV BUG=295792 TESTS=deqp/functional/gles3/draw.html, deqp/functional/gles3/vertexarrays.html CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== WebGL 2: Fix bugs for drawing when the type of vertex attrib is a packed type. This small change fixed bugs in draw.html and vertexarrays.html in WebGL deqp for GL_INT_2_10_10_10_REV and GL_UNSIGNED_INT_2_10_10_10_REV BUG=295792 TESTS=deqp/functional/gles3/draw.html, deqp/functional/gles3/vertexarrays.html CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Description was changed from ========== WebGL 2: Fix bugs for drawing when the type of vertex attrib is a packed type. This small change fixed bugs in draw.html and vertexarrays.html in WebGL deqp for GL_INT_2_10_10_10_REV and GL_UNSIGNED_INT_2_10_10_10_REV BUG=295792 TESTS=deqp/functional/gles3/draw.html, deqp/functional/gles3/vertexarrays.html CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== WebGL 2: Fix bugs for drawing when the type of vertex attrib is a packed type. This small change fixed bugs in draw.html and vertexarrays.html in WebGL deqp for GL_INT_2_10_10_10_REV and GL_UNSIGNED_INT_2_10_10_10_REV. BUG=295792 TESTS=deqp/functional/gles3/draw.html, deqp/functional/gles3/vertexarrays.html CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Patchset #2 (id:20001) has been deleted
yunchao.he@intel.com changed reviewers: + bajones@chromium.org, kbr@chromium.org, piman@chromium.org, qiankun.miao@intel.com, zmo@chromium.org
PTAL. Thanks a lot! https://codereview.chromium.org/1708983002/diff/40001/gpu/command_buffer/comm... File gpu/command_buffer/common/gles2_cmd_utils.cc (right): https://codereview.chromium.org/1708983002/diff/40001/gpu/command_buffer/comm... gpu/command_buffer/common/gles2_cmd_utils.cc:821: return sizeof(GLuint) / 4; // NOLINT the size of INT_2_10_10_10_REV and UNSIGNED_INT_2_10_10_10_REV is the same as the size of GLint and GLuint respectively. However, these two types are packed types, who has 4 components. This function gets the component size for attributes, instead of the element size. The return value of this function (component size) multiply with component count can get element size for attributes. See GLES2DecoderImpl::HandleVertexAttribPointer and VertexAttrib::canAccess (called by GLES2DecoderImpl::DoDrawArrays when check range for attributes). So the return value should be divided by 4.
https://codereview.chromium.org/1708983002/diff/40001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1708983002/diff/40001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder.cc:8934: if (stride & (data_size - 1)) { The offset and stride should be aligned with the element_size (component_size * component_count) for packed types(INT_2_10_10_10_REV and UNSIGNED_2_10_10_10_REV).
https://codereview.chromium.org/1708983002/diff/40001/gpu/command_buffer/comm... File gpu/command_buffer/common/gles2_cmd_utils.cc (right): https://codereview.chromium.org/1708983002/diff/40001/gpu/command_buffer/comm... gpu/command_buffer/common/gles2_cmd_utils.cc:821: return sizeof(GLuint) / 4; // NOLINT On 2016/02/18 14:47:55, yunchao wrote: > the size of INT_2_10_10_10_REV and UNSIGNED_INT_2_10_10_10_REV is the same as > the size of GLint and GLuint respectively. However, these two types are packed > types, who has 4 components. This function gets the component size for > attributes, instead of the element size. The return value of this function > (component size) multiply with component count can get element size for > attributes. See GLES2DecoderImpl::HandleVertexAttribPointer and > VertexAttrib::canAccess (called by GLES2DecoderImpl::DoDrawArrays when check > range for attributes). > > So the return value should be divided by 4. You should change the component count to 1 other than size of the type.
https://codereview.chromium.org/1708983002/diff/40001/gpu/command_buffer/comm... File gpu/command_buffer/common/gles2_cmd_utils.cc (right): https://codereview.chromium.org/1708983002/diff/40001/gpu/command_buffer/comm... gpu/command_buffer/common/gles2_cmd_utils.cc:821: return sizeof(GLuint) / 4; // NOLINT On 2016/02/18 15:42:10, qiankun wrote: > On 2016/02/18 14:47:55, yunchao wrote: > > the size of INT_2_10_10_10_REV and UNSIGNED_INT_2_10_10_10_REV is the same as > > the size of GLint and GLuint respectively. However, these two types are packed > > types, who has 4 components. This function gets the component size for > > attributes, instead of the element size. The return value of this function > > (component size) multiply with component count can get element size for > > attributes. See GLES2DecoderImpl::HandleVertexAttribPointer and > > VertexAttrib::canAccess (called by GLES2DecoderImpl::DoDrawArrays when check > > range for attributes). > > > > So the return value should be divided by 4. > > You should change the component count to 1 other than size of the type. I agree with Qiankun. You can't change the returned size here. Looking at ElementsPerGroup() above, we return 1 instead of 4 for these two types.
Thanks for your review, please see my comments inline. https://codereview.chromium.org/1708983002/diff/40001/gpu/command_buffer/comm... File gpu/command_buffer/common/gles2_cmd_utils.cc (right): https://codereview.chromium.org/1708983002/diff/40001/gpu/command_buffer/comm... gpu/command_buffer/common/gles2_cmd_utils.cc:821: return sizeof(GLuint) / 4; // NOLINT On 2016/02/18 15:42:10, qiankun wrote: > On 2016/02/18 14:47:55, yunchao wrote: > > the size of INT_2_10_10_10_REV and UNSIGNED_INT_2_10_10_10_REV is the same as > > the size of GLint and GLuint respectively. However, these two types are packed > > types, who has 4 components. This function gets the component size for > > attributes, instead of the element size. The return value of this function > > (component size) multiply with component count can get element size for > > attributes. See GLES2DecoderImpl::HandleVertexAttribPointer and > > VertexAttrib::canAccess (called by GLES2DecoderImpl::DoDrawArrays when check > > range for attributes). > > > > So the return value should be divided by 4. > > You should change the component count to 1 other than size of the type. No, the component_count should be 4, see the spec of vertexAttribPointer: https://www.khronos.org/opengles/sdk/docs/man3/html/glVertexAttribPointer.xhtml. The argument 'size' (component_count) must be 4. This argument is cached into AttribInfo in command buffer. Logically, we should not change the component _count too, this packed data indeed have 4 elements (RGBA or XYZW).
See more explanation inline. In fact, I have already tried the other solution as you suggested. But I think this solution is correct. PTAL. Thanks. https://codereview.chromium.org/1708983002/diff/40001/gpu/command_buffer/comm... File gpu/command_buffer/common/gles2_cmd_utils.cc (right): https://codereview.chromium.org/1708983002/diff/40001/gpu/command_buffer/comm... gpu/command_buffer/common/gles2_cmd_utils.cc:821: return sizeof(GLuint) / 4; // NOLINT On 2016/02/18 17:39:50, Zhenyao Mo wrote: > On 2016/02/18 15:42:10, qiankun wrote: > > On 2016/02/18 14:47:55, yunchao wrote: > > > the size of INT_2_10_10_10_REV and UNSIGNED_INT_2_10_10_10_REV is the same > as > > > the size of GLint and GLuint respectively. However, these two types are > packed > > > types, who has 4 components. This function gets the component size for > > > attributes, instead of the element size. The return value of this function > > > (component size) multiply with component count can get element size for > > > attributes. See GLES2DecoderImpl::HandleVertexAttribPointer and > > > VertexAttrib::canAccess (called by GLES2DecoderImpl::DoDrawArrays when check > > > range for attributes). > > > > > > So the return value should be divided by 4. > > > > You should change the component count to 1 other than size of the type. > > I agree with Qiankun. You can't change the returned size here. Looking at > ElementsPerGroup() above, we return 1 instead of 4 for these two types. Yes. these two types only have one element, but logically, they indeed have 4 components. This function returns component_size in a group (for example, R in RGBA, or X in XYZW). It is called by vertexAttribPointer and attribute range/index checking when call into drawArrrays. You can see the code in SetAttribInfo in GLES2DecoderImpl::HandleVertexAttribPointer and VertexAttrib::CanAccess in service/vertex_attrib_manager.cc|.h The callers need component_size, and the component_count is 4, according to the spec of vertexAttribPointer (https://www.khronos.org/opengles/sdk/docs/man3/html/glVertexAttribPointer.xhtml, parameter 'size' means the component_count in a group) Well, we can still keep this code snippet unchanged and return group_size for packed types, and make a workaround in the callers specific for these two types. You know, it is not difficult to think of that solution, I have already tried the solution. I have uploaded that code and run the tests through trybots. It still works. But the logic is not correct, and the code is more complicated. BTW, we can also refer to the code in native deqp: https://android.googlesource.com/platform/external/deqp/+/deqp-dev/modules/gl....
https://codereview.chromium.org/1708983002/diff/40001/gpu/command_buffer/comm... File gpu/command_buffer/common/gles2_cmd_utils.cc (right): https://codereview.chromium.org/1708983002/diff/40001/gpu/command_buffer/comm... gpu/command_buffer/common/gles2_cmd_utils.cc:821: return sizeof(GLuint) / 4; // NOLINT On 2016/02/19 00:34:31, yunchao wrote: > On 2016/02/18 17:39:50, Zhenyao Mo wrote: > > On 2016/02/18 15:42:10, qiankun wrote: > > > On 2016/02/18 14:47:55, yunchao wrote: > > > > the size of INT_2_10_10_10_REV and UNSIGNED_INT_2_10_10_10_REV is the same > > as > > > > the size of GLint and GLuint respectively. However, these two types are > > packed > > > > types, who has 4 components. This function gets the component size for > > > > attributes, instead of the element size. The return value of this function > > > > (component size) multiply with component count can get element size for > > > > attributes. See GLES2DecoderImpl::HandleVertexAttribPointer and > > > > VertexAttrib::canAccess (called by GLES2DecoderImpl::DoDrawArrays when > check > > > > range for attributes). > > > > > > > > So the return value should be divided by 4. > > > > > > You should change the component count to 1 other than size of the type. > > > > I agree with Qiankun. You can't change the returned size here. Looking at > > ElementsPerGroup() above, we return 1 instead of 4 for these two types. > > Yes. these two types only have one element, but logically, they indeed have 4 > components. This function returns component_size in a group (for example, R in > RGBA, or X in XYZW). It is called by vertexAttribPointer and attribute > range/index checking when call into drawArrrays. You can see the code in > SetAttribInfo in GLES2DecoderImpl::HandleVertexAttribPointer and > VertexAttrib::CanAccess in service/vertex_attrib_manager.cc|.h > > The callers need component_size, and the component_count is 4, according to the > spec of vertexAttribPointer > (https://www.khronos.org/opengles/sdk/docs/man3/html/glVertexAttribPointer.xhtml, > parameter 'size' means the component_count in a group) > > Well, we can still keep this code snippet unchanged and return group_size for > packed types, and make a workaround in the callers specific for these two types. > You know, it is not difficult to think of that solution, I have already tried > the solution. I have uploaded that code and run the tests through trybots. It > still works. But the logic is not correct, and the code is more complicated. > > BTW, we can also refer to the code in native deqp: > https://android.googlesource.com/platform/external/deqp/+/deqp-dev/modules/gl.... > The calling code will be incorrect in some cases if this function returns 1 for the sizes of these two types. Consider GLES2DecoderImpl::HandleVertexAttribPointer in src/gpu/command_buffer/service/gles2_cmd_decoder.cc . For these two types, valid strides are either 0, or multiples of 4; but this change will cause component_size to become 1, and any stride will become valid. More thought is needed. I think the callers need to be updated. A helper function which avoids direct calls to GLES2Util::GetGLTypeSizeForTexturesAndBuffers, and does the multiplication that most of the current callers do, would probably be useful.
On 2016/02/19 18:54:51, Ken Russell wrote: > https://codereview.chromium.org/1708983002/diff/40001/gpu/command_buffer/comm... > File gpu/command_buffer/common/gles2_cmd_utils.cc (right): > > https://codereview.chromium.org/1708983002/diff/40001/gpu/command_buffer/comm... > gpu/command_buffer/common/gles2_cmd_utils.cc:821: return sizeof(GLuint) / 4; // > NOLINT > On 2016/02/19 00:34:31, yunchao wrote: > > On 2016/02/18 17:39:50, Zhenyao Mo wrote: > > > On 2016/02/18 15:42:10, qiankun wrote: > > > > On 2016/02/18 14:47:55, yunchao wrote: > > > > > the size of INT_2_10_10_10_REV and UNSIGNED_INT_2_10_10_10_REV is the > same > > > as > > > > > the size of GLint and GLuint respectively. However, these two types are > > > packed > > > > > types, who has 4 components. This function gets the component size for > > > > > attributes, instead of the element size. The return value of this > function > > > > > (component size) multiply with component count can get element size for > > > > > attributes. See GLES2DecoderImpl::HandleVertexAttribPointer and > > > > > VertexAttrib::canAccess (called by GLES2DecoderImpl::DoDrawArrays when > > check > > > > > range for attributes). > > > > > > > > > > So the return value should be divided by 4. > > > > > > > > You should change the component count to 1 other than size of the type. > > > > > > I agree with Qiankun. You can't change the returned size here. Looking at > > > ElementsPerGroup() above, we return 1 instead of 4 for these two types. > > > > Yes. these two types only have one element, but logically, they indeed have 4 > > components. This function returns component_size in a group (for example, R in > > RGBA, or X in XYZW). It is called by vertexAttribPointer and attribute > > range/index checking when call into drawArrrays. You can see the code in > > SetAttribInfo in GLES2DecoderImpl::HandleVertexAttribPointer and > > VertexAttrib::CanAccess in service/vertex_attrib_manager.cc|.h > > > > The callers need component_size, and the component_count is 4, according to > the > > spec of vertexAttribPointer > > > (https://www.khronos.org/opengles/sdk/docs/man3/html/glVertexAttribPointer.xhtml, > > parameter 'size' means the component_count in a group) > > > > Well, we can still keep this code snippet unchanged and return group_size for > > packed types, and make a workaround in the callers specific for these two > types. > > You know, it is not difficult to think of that solution, I have already tried > > the solution. I have uploaded that code and run the tests through trybots. It > > still works. But the logic is not correct, and the code is more complicated. > > > > BTW, we can also refer to the code in native deqp: > > > https://android.googlesource.com/platform/external/deqp/+/deqp-dev/modules/gl.... > > > > The calling code will be incorrect in some cases if this function returns 1 for > the sizes of these two types. Consider > GLES2DecoderImpl::HandleVertexAttribPointer in > src/gpu/command_buffer/service/gles2_cmd_decoder.cc . For these two types, valid > strides are either 0, or multiples of 4; but this change will cause > component_size to become 1, and any stride will become valid. > > More thought is needed. I think the callers need to be updated. A helper > function which avoids direct calls to > GLES2Util::GetGLTypeSizeForTexturesAndBuffers, and does the multiplication that > most of the current callers do, would probably be useful. Ken, I have updated the code in GLES2DecoderImpl::HandleVertexAttribPointer. You know, the error is caused by this logic: data_size = component_size * component_count (or group_size = element_size * elements_per_group in some functions). Now both the component_size and component_count are 4 in command buffer for packed types (INT_2_10_10_10_REV and UNSIGNED_INT_2_10_10_10_REV), it is not correct. We should: 1) change the component_size to 1 for packed types. 2) either, change the component_count to 1 for packed types. Again, I think it is appropriate to keep the component_count to 4, and set the component_size to 1 for packed types (although they are 10bit, 10bit, 10bit and 2bit, not 8bits for every components). This conforms to the logic. You know, logically, we have 4 components in one packed data. If we change the component_count to 1, It is logically wrong. In addition, both client side and service side code in command buffer have cached the component_count in AttribInfo. if we change the component_count for validation, the cached data is not correct. Furthermore, if it is correct to set the component_count to 1, why not the gles spec set the parameter 'size' to 1 directly for packed types in vertexAttribPointer(GLuint index, GLint size, GLenum type, GLsizei stride, GLintptr offset)? But it doesn't do this. Instead, the native deqp(conformance tests for gles, you know) treat the component_size to 1, and keep the component_count to 4 to cover this feature: https://android.googlesource.com/platform/external/deqp/+/deqp-dev/modules/gl.... If you still think we should change component_count to 1, I will upload another patchset. it is not quite difficult.
On 2016/02/18 17:39:50, Zhenyao Mo wrote: > https://codereview.chromium.org/1708983002/diff/40001/gpu/command_buffer/comm... > File gpu/command_buffer/common/gles2_cmd_utils.cc (right): > > https://codereview.chromium.org/1708983002/diff/40001/gpu/command_buffer/comm... > gpu/command_buffer/common/gles2_cmd_utils.cc:821: return sizeof(GLuint) / 4; // > NOLINT > On 2016/02/18 15:42:10, qiankun wrote: > > On 2016/02/18 14:47:55, yunchao wrote: > > > the size of INT_2_10_10_10_REV and UNSIGNED_INT_2_10_10_10_REV is the same > as > > > the size of GLint and GLuint respectively. However, these two types are > packed > > > types, who has 4 components. This function gets the component size for > > > attributes, instead of the element size. The return value of this function > > > (component size) multiply with component count can get element size for > > > attributes. See GLES2DecoderImpl::HandleVertexAttribPointer and > > > VertexAttrib::canAccess (called by GLES2DecoderImpl::DoDrawArrays when check > > > range for attributes). > > > > > > So the return value should be divided by 4. > > > > You should change the component count to 1 other than size of the type. > > I agree with Qiankun. You can't change the returned size here. Looking at > ElementsPerGroup() above, we return 1 instead of 4 for these two types. Zhenyao, this is a little tricky. But they are different APIs. For image data transferred by tex{Compressed}Image{Sub}{2D|3D}, the data_size is defined by parameter "format" and "type". For attribute data in buffer transferred by vertexAttribPointer, the data_size is defined by parameter "size" (which is explicitly defined to be 4 for packed types in the gles spec) and "type".
On 2016/02/23 02:47:54, yunchao wrote: > On 2016/02/19 18:54:51, Ken Russell wrote: > > > https://codereview.chromium.org/1708983002/diff/40001/gpu/command_buffer/comm... > > File gpu/command_buffer/common/gles2_cmd_utils.cc (right): > > > > > https://codereview.chromium.org/1708983002/diff/40001/gpu/command_buffer/comm... > > gpu/command_buffer/common/gles2_cmd_utils.cc:821: return sizeof(GLuint) / 4; > // > > NOLINT > > On 2016/02/19 00:34:31, yunchao wrote: > > > On 2016/02/18 17:39:50, Zhenyao Mo wrote: > > > > On 2016/02/18 15:42:10, qiankun wrote: > > > > > On 2016/02/18 14:47:55, yunchao wrote: > > > > > > the size of INT_2_10_10_10_REV and UNSIGNED_INT_2_10_10_10_REV is the > > same > > > > as > > > > > > the size of GLint and GLuint respectively. However, these two types > are > > > > packed > > > > > > types, who has 4 components. This function gets the component size for > > > > > > attributes, instead of the element size. The return value of this > > function > > > > > > (component size) multiply with component count can get element size > for > > > > > > attributes. See GLES2DecoderImpl::HandleVertexAttribPointer and > > > > > > VertexAttrib::canAccess (called by GLES2DecoderImpl::DoDrawArrays when > > > check > > > > > > range for attributes). > > > > > > > > > > > > So the return value should be divided by 4. > > > > > > > > > > You should change the component count to 1 other than size of the type. > > > > > > > > I agree with Qiankun. You can't change the returned size here. Looking > at > > > > ElementsPerGroup() above, we return 1 instead of 4 for these two types. > > > > > > Yes. these two types only have one element, but logically, they indeed have > 4 > > > components. This function returns component_size in a group (for example, R > in > > > RGBA, or X in XYZW). It is called by vertexAttribPointer and attribute > > > range/index checking when call into drawArrrays. You can see the code in > > > SetAttribInfo in GLES2DecoderImpl::HandleVertexAttribPointer and > > > VertexAttrib::CanAccess in service/vertex_attrib_manager.cc|.h > > > > > > The callers need component_size, and the component_count is 4, according to > > the > > > spec of vertexAttribPointer > > > > > > (https://www.khronos.org/opengles/sdk/docs/man3/html/glVertexAttribPointer.xhtml, > > > parameter 'size' means the component_count in a group) > > > > > > Well, we can still keep this code snippet unchanged and return group_size > for > > > packed types, and make a workaround in the callers specific for these two > > types. > > > You know, it is not difficult to think of that solution, I have already > tried > > > the solution. I have uploaded that code and run the tests through trybots. > It > > > still works. But the logic is not correct, and the code is more complicated. > > > > > > > BTW, we can also refer to the code in native deqp: > > > > > > https://android.googlesource.com/platform/external/deqp/+/deqp-dev/modules/gl.... > > > > > > > The calling code will be incorrect in some cases if this function returns 1 > for > > the sizes of these two types. Consider > > GLES2DecoderImpl::HandleVertexAttribPointer in > > src/gpu/command_buffer/service/gles2_cmd_decoder.cc . For these two types, > valid > > strides are either 0, or multiples of 4; but this change will cause > > component_size to become 1, and any stride will become valid. > > > > More thought is needed. I think the callers need to be updated. A helper > > function which avoids direct calls to > > GLES2Util::GetGLTypeSizeForTexturesAndBuffers, and does the multiplication > that > > most of the current callers do, would probably be useful. > > Ken, I have updated the code in GLES2DecoderImpl::HandleVertexAttribPointer. > > You know, the error is caused by this logic: data_size = component_size * > component_count (or group_size = element_size * elements_per_group in some > functions). Now both the component_size and component_count are 4 in command > buffer for packed types (INT_2_10_10_10_REV and UNSIGNED_INT_2_10_10_10_REV), it > is not correct. We should: > 1) change the component_size to 1 for packed types. > 2) either, change the component_count to 1 for packed types. > > Again, I think it is appropriate to keep the component_count to 4, and set the > component_size to 1 for packed types (although they are 10bit, 10bit, 10bit and > 2bit, not 8bits for every components). This conforms to the logic. You know, > logically, we have 4 components in one packed data. If we change the > component_count to 1, It is logically wrong. In addition, both client side and > service side code in command buffer have cached the component_count in > AttribInfo. if we change the component_count for validation, the cached data is > not correct. Furthermore, if it is correct to set the component_count to 1, why > not the gles spec set the parameter 'size' to 1 directly for packed types in > vertexAttribPointer(GLuint index, GLint size, GLenum type, GLsizei stride, > GLintptr offset)? But it doesn't do this. Instead, the native deqp(conformance > tests for gles, you know) treat the component_size to 1, and keep the > component_count to 4 to cover this feature: > https://android.googlesource.com/platform/external/deqp/+/deqp-dev/modules/gl.... > > If you still think we should change component_count to 1, I will upload another > patchset. it is not quite difficult. After giving this more thought I agree with your assessment, Yunchao, that the component count should continue to be 4. I defer review to zmo@ and piman@ but I think this LGTM. One comment which I'll send separately.
https://codereview.chromium.org/1708983002/diff/40001/gpu/command_buffer/comm... File gpu/command_buffer/common/gles2_cmd_utils.cc (right): https://codereview.chromium.org/1708983002/diff/40001/gpu/command_buffer/comm... gpu/command_buffer/common/gles2_cmd_utils.cc:821: return sizeof(GLuint) / 4; // NOLINT On 2016/02/19 18:54:51, Ken Russell wrote: > On 2016/02/19 00:34:31, yunchao wrote: > > On 2016/02/18 17:39:50, Zhenyao Mo wrote: > > > On 2016/02/18 15:42:10, qiankun wrote: > > > > On 2016/02/18 14:47:55, yunchao wrote: > > > > > the size of INT_2_10_10_10_REV and UNSIGNED_INT_2_10_10_10_REV is the > same > > > as > > > > > the size of GLint and GLuint respectively. However, these two types are > > > packed > > > > > types, who has 4 components. This function gets the component size for > > > > > attributes, instead of the element size. The return value of this > function > > > > > (component size) multiply with component count can get element size for > > > > > attributes. See GLES2DecoderImpl::HandleVertexAttribPointer and > > > > > VertexAttrib::canAccess (called by GLES2DecoderImpl::DoDrawArrays when > > check > > > > > range for attributes). > > > > > > > > > > So the return value should be divided by 4. > > > > > > > > You should change the component count to 1 other than size of the type. > > > > > > I agree with Qiankun. You can't change the returned size here. Looking at > > > ElementsPerGroup() above, we return 1 instead of 4 for these two types. > > > > Yes. these two types only have one element, but logically, they indeed have 4 > > components. This function returns component_size in a group (for example, R in > > RGBA, or X in XYZW). It is called by vertexAttribPointer and attribute > > range/index checking when call into drawArrrays. You can see the code in > > SetAttribInfo in GLES2DecoderImpl::HandleVertexAttribPointer and > > VertexAttrib::CanAccess in service/vertex_attrib_manager.cc|.h > > > > The callers need component_size, and the component_count is 4, according to > the > > spec of vertexAttribPointer > > > (https://www.khronos.org/opengles/sdk/docs/man3/html/glVertexAttribPointer.xhtml, > > parameter 'size' means the component_count in a group) > > > > Well, we can still keep this code snippet unchanged and return group_size for > > packed types, and make a workaround in the callers specific for these two > types. > > You know, it is not difficult to think of that solution, I have already tried > > the solution. I have uploaded that code and run the tests through trybots. It > > still works. But the logic is not correct, and the code is more complicated. > > > > BTW, we can also refer to the code in native deqp: > > > https://android.googlesource.com/platform/external/deqp/+/deqp-dev/modules/gl.... > > > > The calling code will be incorrect in some cases if this function returns 1 for > the sizes of these two types. Consider > GLES2DecoderImpl::HandleVertexAttribPointer in > src/gpu/command_buffer/service/gles2_cmd_decoder.cc . For these two types, valid > strides are either 0, or multiples of 4; but this change will cause > component_size to become 1, and any stride will become valid. > > More thought is needed. I think the callers need to be updated. A helper > function which avoids direct calls to > GLES2Util::GetGLTypeSizeForTexturesAndBuffers, and does the multiplication that > most of the current callers do, would probably be useful. If we are going to go forward with this change then please add a comment here that the packed types need to be handled specially for size computations, and point to the other code (in GLES2DecoderImpl::HandleVertexAttribPointer) which needs to specially handle these types.
Patchset #3 (id:60001) has been deleted
Could you have a look, zmo@ and piman@. Thanks! https://codereview.chromium.org/1708983002/diff/40001/gpu/command_buffer/comm... File gpu/command_buffer/common/gles2_cmd_utils.cc (right): https://codereview.chromium.org/1708983002/diff/40001/gpu/command_buffer/comm... gpu/command_buffer/common/gles2_cmd_utils.cc:821: return sizeof(GLuint) / 4; // NOLINT On 2016/02/23 22:56:00, Ken Russell wrote: > On 2016/02/19 18:54:51, Ken Russell wrote: > > On 2016/02/19 00:34:31, yunchao wrote: > > > On 2016/02/18 17:39:50, Zhenyao Mo wrote: > > > > On 2016/02/18 15:42:10, qiankun wrote: > > > > > On 2016/02/18 14:47:55, yunchao wrote: > > > > > > the size of INT_2_10_10_10_REV and UNSIGNED_INT_2_10_10_10_REV is the > > same > > > > as > > > > > > the size of GLint and GLuint respectively. However, these two types > are > > > > packed > > > > > > types, who has 4 components. This function gets the component size for > > > > > > attributes, instead of the element size. The return value of this > > function > > > > > > (component size) multiply with component count can get element size > for > > > > > > attributes. See GLES2DecoderImpl::HandleVertexAttribPointer and > > > > > > VertexAttrib::canAccess (called by GLES2DecoderImpl::DoDrawArrays when > > > check > > > > > > range for attributes). > > > > > > > > > > > > So the return value should be divided by 4. > > > > > > > > > > You should change the component count to 1 other than size of the type. > > > > > > > > I agree with Qiankun. You can't change the returned size here. Looking > at > > > > ElementsPerGroup() above, we return 1 instead of 4 for these two types. > > > > > > Yes. these two types only have one element, but logically, they indeed have > 4 > > > components. This function returns component_size in a group (for example, R > in > > > RGBA, or X in XYZW). It is called by vertexAttribPointer and attribute > > > range/index checking when call into drawArrrays. You can see the code in > > > SetAttribInfo in GLES2DecoderImpl::HandleVertexAttribPointer and > > > VertexAttrib::CanAccess in service/vertex_attrib_manager.cc|.h > > > > > > The callers need component_size, and the component_count is 4, according to > > the > > > spec of vertexAttribPointer > > > > > > (https://www.khronos.org/opengles/sdk/docs/man3/html/glVertexAttribPointer.xhtml, > > > parameter 'size' means the component_count in a group) > > > > > > Well, we can still keep this code snippet unchanged and return group_size > for > > > packed types, and make a workaround in the callers specific for these two > > types. > > > You know, it is not difficult to think of that solution, I have already > tried > > > the solution. I have uploaded that code and run the tests through trybots. > It > > > still works. But the logic is not correct, and the code is more complicated. > > > > > > > BTW, we can also refer to the code in native deqp: > > > > > > https://android.googlesource.com/platform/external/deqp/+/deqp-dev/modules/gl.... > > > > > > > The calling code will be incorrect in some cases if this function returns 1 > for > > the sizes of these two types. Consider > > GLES2DecoderImpl::HandleVertexAttribPointer in > > src/gpu/command_buffer/service/gles2_cmd_decoder.cc . For these two types, > valid > > strides are either 0, or multiples of 4; but this change will cause > > component_size to become 1, and any stride will become valid. > > > > More thought is needed. I think the callers need to be updated. A helper > > function which avoids direct calls to > > GLES2Util::GetGLTypeSizeForTexturesAndBuffers, and does the multiplication > that > > most of the current callers do, would probably be useful. > > If we are going to go forward with this change then please add a comment here > that the packed types need to be handled specially for size computations, and > point to the other code (in GLES2DecoderImpl::HandleVertexAttribPointer) which > needs to specially handle these types. Done.
Sorry for the delay. https://codereview.chromium.org/1708983002/diff/80001/gpu/command_buffer/comm... File gpu/command_buffer/common/gles2_cmd_utils.cc (right): https://codereview.chromium.org/1708983002/diff/80001/gpu/command_buffer/comm... gpu/command_buffer/common/gles2_cmd_utils.cc:825: return sizeof(GLint) / 4; // NOLINT I still disagree with this. This function calls GetTypeSize, and we should return type size, as simple as that. Any hacking to fix a bug somewhere, you make the implementation different than what the function name indicates, then sooner or later a bug might happen because someone didn't realize your special case. I suggest the following: 1) Change this function name to GetGLTypeSizeForBuffers(). For textures we have BytesPerElement(). The function stays as is. 2) Add another helper function GetGroupSizeForBufferType(GLenum type, GLint size), inside you can deal with the packed type situation. Most callers of GetGLTypeSizeForBuffers() can be switched to GetGroupSizeForBufferType(). 3) In GLES2DecoderImpl::HandleVertexAttribPointer, you write this special case handling for packed type there.
lgtm if bots are happy https://codereview.chromium.org/1708983002/diff/100001/gpu/command_buffer/com... File gpu/command_buffer/common/gles2_cmd_utils.cc (right): https://codereview.chromium.org/1708983002/diff/100001/gpu/command_buffer/com... gpu/command_buffer/common/gles2_cmd_utils.cc:831: return type_size; nit: please DCHECK_EQ(4u, count) here.
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/1708983002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1708983002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for your careful review, Zhenyao and Ken. Merging now... https://codereview.chromium.org/1708983002/diff/80001/gpu/command_buffer/comm... File gpu/command_buffer/common/gles2_cmd_utils.cc (right): https://codereview.chromium.org/1708983002/diff/80001/gpu/command_buffer/comm... gpu/command_buffer/common/gles2_cmd_utils.cc:825: return sizeof(GLint) / 4; // NOLINT On 2016/02/25 21:40:43, Zhenyao Mo wrote: > I still disagree with this. > > This function calls GetTypeSize, and we should return type size, as simple as > that. Any hacking to fix a bug somewhere, you make the implementation different > than what the function name indicates, then sooner or later a bug might happen > because someone didn't realize your special case. > > I suggest the following: > > 1) Change this function name to GetGLTypeSizeForBuffers(). For textures we have > BytesPerElement(). The function stays as is. > > 2) Add another helper function GetGroupSizeForBufferType(GLenum type, GLint > size), inside you can deal with the packed type situation. Most callers of > GetGLTypeSizeForBuffers() can be switched to GetGroupSizeForBufferType(). > > 3) In GLES2DecoderImpl::HandleVertexAttribPointer, you write this special case > handling for packed type there. Thanks for your suggestion, Zhenyao. It is a pretty good solution.I should think of this carefully and get a good solution like this. https://codereview.chromium.org/1708983002/diff/100001/gpu/command_buffer/com... File gpu/command_buffer/common/gles2_cmd_utils.cc (right): https://codereview.chromium.org/1708983002/diff/100001/gpu/command_buffer/com... gpu/command_buffer/common/gles2_cmd_utils.cc:831: return type_size; On 2016/02/26 04:24:29, Zhenyao Mo wrote: > nit: please DCHECK_EQ(4u, count) here. Done.
The CQ bit was checked by yunchao.he@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org, zmo@chromium.org Link to the patchset: https://codereview.chromium.org/1708983002/#ps120001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1708983002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1708983002/120001
Message was sent while issue was closed.
Description was changed from ========== WebGL 2: Fix bugs for drawing when the type of vertex attrib is a packed type. This small change fixed bugs in draw.html and vertexarrays.html in WebGL deqp for GL_INT_2_10_10_10_REV and GL_UNSIGNED_INT_2_10_10_10_REV. BUG=295792 TESTS=deqp/functional/gles3/draw.html, deqp/functional/gles3/vertexarrays.html CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== WebGL 2: Fix bugs for drawing when the type of vertex attrib is a packed type. This small change fixed bugs in draw.html and vertexarrays.html in WebGL deqp for GL_INT_2_10_10_10_REV and GL_UNSIGNED_INT_2_10_10_10_REV. BUG=295792 TESTS=deqp/functional/gles3/draw.html, deqp/functional/gles3/vertexarrays.html CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Message was sent while issue was closed.
Committed patchset #5 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== WebGL 2: Fix bugs for drawing when the type of vertex attrib is a packed type. This small change fixed bugs in draw.html and vertexarrays.html in WebGL deqp for GL_INT_2_10_10_10_REV and GL_UNSIGNED_INT_2_10_10_10_REV. BUG=295792 TESTS=deqp/functional/gles3/draw.html, deqp/functional/gles3/vertexarrays.html CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== WebGL 2: Fix bugs for drawing when the type of vertex attrib is a packed type. This small change fixed bugs in draw.html and vertexarrays.html in WebGL deqp for GL_INT_2_10_10_10_REV and GL_UNSIGNED_INT_2_10_10_10_REV. BUG=295792 TESTS=deqp/functional/gles3/draw.html, deqp/functional/gles3/vertexarrays.html CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel Committed: https://crrev.com/6dc5f17704d3b1465ab48f25a11b7c9979892df3 Cr-Commit-Position: refs/heads/master@{#377836} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/6dc5f17704d3b1465ab48f25a11b7c9979892df3 Cr-Commit-Position: refs/heads/master@{#377836}
Message was sent while issue was closed.
Description was changed from ========== WebGL 2: Fix bugs for drawing when the type of vertex attrib is a packed type. This small change fixed bugs in draw.html and vertexarrays.html in WebGL deqp for GL_INT_2_10_10_10_REV and GL_UNSIGNED_INT_2_10_10_10_REV. BUG=295792 TESTS=deqp/functional/gles3/draw.html, deqp/functional/gles3/vertexarrays.html CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel Committed: https://crrev.com/6dc5f17704d3b1465ab48f25a11b7c9979892df3 Cr-Commit-Position: refs/heads/master@{#377836} ========== to ========== Command buffer: Fix bugs for drawing when the type of vertex attrib is a packed type. This small change fixed bugs in draw.html and vertexarrays.html in WebGL deqp for GL_INT_2_10_10_10_REV and GL_UNSIGNED_INT_2_10_10_10_REV. BUG=295792 TESTS=deqp/functional/gles3/draw.html, deqp/functional/gles3/vertexarrays.html CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel Committed: https://crrev.com/6dc5f17704d3b1465ab48f25a11b7c9979892df3 Cr-Commit-Position: refs/heads/master@{#377836} ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
