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 6b62730a56213be5628b9f0961d636bbfaf3e67e..89ff619c4cc74ece47cdbf0cdbe73947f2c887a1 100644
--- a/gpu/command_buffer/service/gles2_cmd_decoder.cc
+++ b/gpu/command_buffer/service/gles2_cmd_decoder.cc
@@ -1795,6 +1795,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,
@@ -9079,6 +9083,24 @@ void GLES2DecoderImpl::RestoreStateForSimulatedFixedAttribs() {
                                       : 0);
 }
 
+bool GLES2DecoderImpl::AttribsTypeMatch() {

[Zhenyao Mo, 2016/07/21 13:10:46]

This function needs to be revised

1) you have to have a disable/enable mask from vertex_attrib_manager, see my
comments in another place.
2) The masks should be arrays instead of a single uint32_t, see my comments in
another place.

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

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();

[Zhenyao Mo, 2016/07/21 13:10:46]

Again, I really don't think you need this generic mask. Consider all of them
written (since they have default type as FLOAT), so this is always 0xFFFFFFFF.

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

Acknowledged.

+  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,
@@ -9125,6 +9147,12 @@ error::Error GLES2DecoderImpl::DoDrawArrays(
     return error::kNoError;
   }
 
+  if (feature_info_->IsWebGL2Context() && !AttribsTypeMatch()) {

[Zhenyao Mo, 2016/07/21 13:10:46]

This should be WebGL2 or ES3.

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

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()) {
@@ -9251,6 +9279,12 @@ error::Error GLES2DecoderImpl::DoDrawElements(const char* function_name,
     return error::kNoError;
   }
 
+  if (feature_info_->IsWebGL2Context() && !AttribsTypeMatch()) {

[Zhenyao Mo, 2016/07/21 13:10:46]

same here, webgl2 or es3.

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

Done.

+    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();
@@ -9850,6 +9884,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);
   }
 }
@@ -9857,6 +9893,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);
   }
 }
@@ -9865,6 +9903,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);
   }
 }
@@ -9873,6 +9913,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);
   }
 }
@@ -9880,6 +9922,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);
   }
 }
@@ -9887,6 +9931,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);
   }
 }
@@ -9894,12 +9940,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);
   }
 }
@@ -9908,12 +9958,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);
   }
 }
@@ -9922,12 +9976,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);
   }
 }
@@ -10006,6 +10064,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/21 13:10:46]

This is incorrect.  You can call VertexAttribPointer (at the call time the
vertex attrib array is disabled), but later enable the vertex attrib array. 
Then the state you are tracking will be wrong.  You have to store a type mask
and enable_disable mask separately.  (The enable / disable mask needs to be part
of the vertex_attrib_manager as it is per vertex array object, not per context)

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

Done.

+    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,
@@ -10100,6 +10164,11 @@ error::Error GLES2DecoderImpl::HandleVertexAttribPointer(
         "glVertexAttribPointer", "stride not valid for type");
     return error::kNoError;
   }
+
+  if (state_.vertex_attrib_manager->GetVertexAttrib(indx)->enabled()) {
+    state_.vertex_attrib_manager->UpdateAttribBaseTypeAndMask(
+        indx, SHADER_VARIABLE_FLOAT);
+  }
   GLsizei group_size = GLES2Util::GetGroupSizeForBufferType(size, type);
   state_.vertex_attrib_manager
       ->SetAttribInfo(indx,