Command buffer: Fix bugs for drawing when the type of vertex attrib is a packed type

gpu/command_buffer/common/gles2_cmd_utils.cc

Index: gpu/command_buffer/common/gles2_cmd_utils.cc
diff --git a/gpu/command_buffer/common/gles2_cmd_utils.cc b/gpu/command_buffer/common/gles2_cmd_utils.cc
index 0011053ad2d63e0892e809baf6c74e1feb57d83b..8d460ba06ad27e31859841c2752dfc8b864e48bb 100644
--- a/gpu/command_buffer/common/gles2_cmd_utils.cc
+++ b/gpu/command_buffer/common/gles2_cmd_utils.cc
@@ -816,9 +816,9 @@ size_t GLES2Util::GetGLTypeSizeForTexturesAndBuffers(uint32_t type) {
     case GL_HALF_FLOAT:
       return sizeof(GLushort);  // NOLINT
     case GL_INT_2_10_10_10_REV:
-      return sizeof(GLint);  // NOLINT
+      return sizeof(GLint) / 4;  // NOLINT
     case GL_UNSIGNED_INT_2_10_10_10_REV:
-      return sizeof(GLuint);  // NOLINT
+      return sizeof(GLuint) / 4;  // NOLINT

[yunchao, 2016/02/18 14:47:55]

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.

[qiankun, 2016/02/18 15:42:10]

You should change the component count to 1 other than size of the type.

[Zhenyao Mo, 2016/02/18 17:39:50]

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.

[yunchao, 2016/02/19 00:03:10]

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). 

[yunchao, 2016/02/19 00:34:31]

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....

[Ken Russell (switch to Gerrit), 2016/02/19 18:54:51]

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 Russell (switch to Gerrit), 2016/02/23 22:56:00]

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.

[yunchao, 2016/02/25 15:29:41]

Done.

     default:
       return 0;
   }