WebGL 2: make sure VertexAttrib type match the corresponding attrib type in shader

gpu/command_buffer/service/gles2_cmd_decoder.cc

Index: gpu/command_buffer/service/gles2_cmd_decoder.cc
diff --git a/gpu/command_buffer/service/gles2_cmd_decoder.cc b/gpu/command_buffer/service/gles2_cmd_decoder.cc
index 670782ecb6243bb97252cfe4918b8eb3cc585274..84bb45bd3c3506d7821dfc375ac28286420a3714 100644
--- a/gpu/command_buffer/service/gles2_cmd_decoder.cc
+++ b/gpu/command_buffer/service/gles2_cmd_decoder.cc
@@ -1790,6 +1790,10 @@ class GLES2DecoderImpl : public GLES2Decoder, public ErrorStateClient {
   // false if pname is unknown.
   bool GetNumValuesReturnedForGLGet(GLenum pname, GLsizei* num_values);
 
+  // Checks if the type of an attribute set by vertexAttrib API match
+  // the type of corresponding attribute in vertex shader.
+  bool AttribsTypeMatch();
+
   // Checks if the current program and vertex attributes are valid for drawing.
   bool IsDrawValid(
       const char* function_name, GLuint max_vertex_accessed, bool instanced,
@@ -9035,6 +9039,24 @@ void GLES2DecoderImpl::RestoreStateForSimulatedFixedAttribs() {
                                       : 0);
 }
 
+bool GLES2DecoderImpl::AttribsTypeMatch() {

[Zhenyao Mo, 2016/07/20 17:54:56]

This function is incorrect.  Basically we should compare shader attrib types
with 
1) if a vertex attrib array is enabled, use the vertex_attrib_manager's types
(set by VertexAttribPoint)
2) if a vertex attrib array is disabled, use the generic vertex attrib type from
ContextState.

So what you need to do (to make this function efficient) is to keep the
disable_enable mask, and use enable_mask_ to filter 1) and use ~enable_mask_ to
filter 2.

Then compare.

[yunchao, 2016/07/21 06:15:27]

Zhenyao, thanks for your careful review, But I have different opinion here. My
implementation is almost the same with your suggestions. But here is a tiny
difference for case 2) above. If an attribute in shader with INT/UINT type, for
instance:
ivec2 a;
uint b;

If These two attributes have no vertexAttrib API to feed data. In 2), they will
think unmatched, because the default type is FLOAT by default for generic attrib
in ContextState. But they are actually OK, I think. 

My code is almost the same with your suggestions, but it will record the
written_mask from call sites of vertexAttrib{1|2|3|4}{i|ui}f{v}, and only
compare those written attributes by vertexAttrib API: 

mask = shader_mask & (vao_attrib_mask | generic_attrib_mask);

WDYT? 

[yunchao, 2016/07/22 13:43:53]

Acknowledged.

+  uint32_t shader_mask = state_.current_program->vertex_input_written_mask();
+  uint32_t vao_attrib_mask =
+      state_.vertex_attrib_manager->attrib_written_mask();
+  uint32_t generic_attrib_mask = state_.GetGenericVertexAttribTypeMask();
+  uint32_t mask = shader_mask & (vao_attrib_mask | generic_attrib_mask);
+
+  uint32_t vertex_attrib_base_type =
+      (~vao_attrib_mask & state_.GetGenericVertexAttribBaseType()) |
+      state_.vertex_attrib_manager->attrib_base_type_mask();
+
+  if ((state_.current_program->vertex_input_type_mask() & mask) ==
+      (vertex_attrib_base_type & mask)) {
+    return true;
+  }
+  return false;
+}
+
 error::Error GLES2DecoderImpl::DoDrawArrays(
     const char* function_name,
     bool instanced,
@@ -9081,6 +9103,12 @@ error::Error GLES2DecoderImpl::DoDrawArrays(
     return error::kNoError;
   }
 
+  if (feature_info_->IsWebGL2Context() && !AttribsTypeMatch()) {

[Zhenyao Mo, 2016/07/20 17:54:56]

This should be WebGL2 and ES3.

[yunchao, 2016/07/22 13:43:53]

Done.

+    LOCAL_SET_GL_ERROR(GL_INVALID_OPERATION, function_name,
+        "vertexAttrib function must match shader attrib type");
+    return error::kNoError;
+  }
+
   GLuint max_vertex_accessed = first + count - 1;
   if (IsDrawValid(function_name, max_vertex_accessed, instanced, primcount)) {
     if (!ClearUnclearedTextures()) {
@@ -9204,6 +9232,12 @@ error::Error GLES2DecoderImpl::DoDrawElements(const char* function_name,
     return error::kNoError;
   }
 
+  if (feature_info_->IsWebGL2Context() && !AttribsTypeMatch()) {
+    LOCAL_SET_GL_ERROR(GL_INVALID_OPERATION, function_name,
+        "vertexAttrib function must match shader attrib type");
+    return error::kNoError;
+  }
+
   GLuint max_vertex_accessed;
   Buffer* element_array_buffer =
       state_.vertex_attrib_manager->element_array_buffer();
@@ -9797,6 +9831,8 @@ bool GLES2DecoderImpl::SetVertexAttribValue(
 void GLES2DecoderImpl::DoVertexAttrib1f(GLuint index, GLfloat v0) {
   GLfloat v[4] = { v0, 0.0f, 0.0f, 1.0f, };
   if (SetVertexAttribValue("glVertexAttrib1f", index, v)) {
+    state_.SetGenericVertexAttribBaseType(
+        index, SHADER_VARIABLE_FLOAT);
     glVertexAttrib1f(index, v0);
   }
 }
@@ -9804,6 +9840,8 @@ void GLES2DecoderImpl::DoVertexAttrib1f(GLuint index, GLfloat v0) {
 void GLES2DecoderImpl::DoVertexAttrib2f(GLuint index, GLfloat v0, GLfloat v1) {
   GLfloat v[4] = { v0, v1, 0.0f, 1.0f, };
   if (SetVertexAttribValue("glVertexAttrib2f", index, v)) {
+    state_.SetGenericVertexAttribBaseType(
+        index, SHADER_VARIABLE_FLOAT);
     glVertexAttrib2f(index, v0, v1);
   }
 }
@@ -9812,6 +9850,8 @@ void GLES2DecoderImpl::DoVertexAttrib3f(
     GLuint index, GLfloat v0, GLfloat v1, GLfloat v2) {
   GLfloat v[4] = { v0, v1, v2, 1.0f, };
   if (SetVertexAttribValue("glVertexAttrib3f", index, v)) {
+    state_.SetGenericVertexAttribBaseType(
+        index, SHADER_VARIABLE_FLOAT);
     glVertexAttrib3f(index, v0, v1, v2);
   }
 }
@@ -9820,6 +9860,8 @@ void GLES2DecoderImpl::DoVertexAttrib4f(
     GLuint index, GLfloat v0, GLfloat v1, GLfloat v2, GLfloat v3) {
   GLfloat v[4] = { v0, v1, v2, v3, };
   if (SetVertexAttribValue("glVertexAttrib4f", index, v)) {
+    state_.SetGenericVertexAttribBaseType(
+        index, SHADER_VARIABLE_FLOAT);
     glVertexAttrib4f(index, v0, v1, v2, v3);
   }
 }
@@ -9827,6 +9869,8 @@ void GLES2DecoderImpl::DoVertexAttrib4f(
 void GLES2DecoderImpl::DoVertexAttrib1fv(GLuint index, const GLfloat* v) {
   GLfloat t[4] = { v[0], 0.0f, 0.0f, 1.0f, };
   if (SetVertexAttribValue("glVertexAttrib1fv", index, t)) {
+    state_.SetGenericVertexAttribBaseType(
+        index, SHADER_VARIABLE_FLOAT);
     glVertexAttrib1fv(index, v);
   }
 }
@@ -9834,6 +9878,8 @@ void GLES2DecoderImpl::DoVertexAttrib1fv(GLuint index, const GLfloat* v) {
 void GLES2DecoderImpl::DoVertexAttrib2fv(GLuint index, const GLfloat* v) {
   GLfloat t[4] = { v[0], v[1], 0.0f, 1.0f, };
   if (SetVertexAttribValue("glVertexAttrib2fv", index, t)) {
+    state_.SetGenericVertexAttribBaseType(
+        index, SHADER_VARIABLE_FLOAT);
     glVertexAttrib2fv(index, v);
   }
 }
@@ -9841,12 +9887,16 @@ void GLES2DecoderImpl::DoVertexAttrib2fv(GLuint index, const GLfloat* v) {
 void GLES2DecoderImpl::DoVertexAttrib3fv(GLuint index, const GLfloat* v) {
   GLfloat t[4] = { v[0], v[1], v[2], 1.0f, };
   if (SetVertexAttribValue("glVertexAttrib3fv", index, t)) {
+    state_.SetGenericVertexAttribBaseType(
+        index, SHADER_VARIABLE_FLOAT);
     glVertexAttrib3fv(index, v);
   }
 }
 
 void GLES2DecoderImpl::DoVertexAttrib4fv(GLuint index, const GLfloat* v) {
   if (SetVertexAttribValue("glVertexAttrib4fv", index, v)) {
+    state_.SetGenericVertexAttribBaseType(
+        index, SHADER_VARIABLE_FLOAT);
     glVertexAttrib4fv(index, v);
   }
 }
@@ -9855,12 +9905,16 @@ void GLES2DecoderImpl::DoVertexAttribI4i(
     GLuint index, GLint v0, GLint v1, GLint v2, GLint v3) {
   GLint v[4] = { v0, v1, v2, v3 };
   if (SetVertexAttribValue("glVertexAttribI4i", index, v)) {
+    state_.SetGenericVertexAttribBaseType(
+        index, SHADER_VARIABLE_INT);
     glVertexAttribI4i(index, v0, v1, v2, v3);
   }
 }
 
 void GLES2DecoderImpl::DoVertexAttribI4iv(GLuint index, const GLint* v) {
   if (SetVertexAttribValue("glVertexAttribI4iv", index, v)) {
+    state_.SetGenericVertexAttribBaseType(
+        index, SHADER_VARIABLE_INT);
     glVertexAttribI4iv(index, v);
   }
 }
@@ -9869,12 +9923,16 @@ void GLES2DecoderImpl::DoVertexAttribI4ui(
     GLuint index, GLuint v0, GLuint v1, GLuint v2, GLuint v3) {
   GLuint v[4] = { v0, v1, v2, v3 };
   if (SetVertexAttribValue("glVertexAttribI4ui", index, v)) {
+    state_.SetGenericVertexAttribBaseType(
+        index, SHADER_VARIABLE_UINT);
     glVertexAttribI4ui(index, v0, v1, v2, v3);
   }
 }
 
 void GLES2DecoderImpl::DoVertexAttribI4uiv(GLuint index, const GLuint* v) {
   if (SetVertexAttribValue("glVertexAttribI4uiv", index, v)) {
+    state_.SetGenericVertexAttribBaseType(
+        index, SHADER_VARIABLE_UINT);
     glVertexAttribI4uiv(index, v);
   }
 }
@@ -9953,6 +10011,12 @@ error::Error GLES2DecoderImpl::HandleVertexAttribIPointer(
         "glVertexAttribIPointer", "stride not valid for type");
     return error::kNoError;
   }
+
+  if (state_.vertex_attrib_manager->GetVertexAttrib(indx)->enabled()) {

[Zhenyao Mo, 2016/07/20 17:54:56]

You should update it whether it's enabled or not, I'll explain in another
comment what should happen.

[yunchao, 2016/07/22 13:43:53]

Acknowledged.

+    GLenum base_type = (type == GL_BYTE || type == GL_SHORT || type == GL_INT) ?
+                        SHADER_VARIABLE_INT : SHADER_VARIABLE_UINT;
+    state_.vertex_attrib_manager->UpdateAttribBaseTypeAndMask(indx, base_type);
+  }
   GLsizei group_size = GLES2Util::GetGroupSizeForBufferType(size, type);
   state_.vertex_attrib_manager
       ->SetAttribInfo(indx,
@@ -10047,6 +10111,11 @@ error::Error GLES2DecoderImpl::HandleVertexAttribPointer(
         "glVertexAttribPointer", "stride not valid for type");
     return error::kNoError;
   }
+
+  if (state_.vertex_attrib_manager->GetVertexAttrib(indx)->enabled()) {

[Zhenyao Mo, 2016/07/20 17:54:56]

Same here.

[yunchao, 2016/07/22 13:43:53]

Acknowledged.

+    state_.vertex_attrib_manager->UpdateAttribBaseTypeAndMask(
+        indx, SHADER_VARIABLE_FLOAT);
+  }
   GLsizei group_size = GLES2Util::GetGroupSizeForBufferType(size, type);
   state_.vertex_attrib_manager
       ->SetAttribInfo(indx,