Fix Uniform Name Parsing

BUG=144007


Review URL: https://chromiumcodereview.appspot.com/10855274

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@152707 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 174 insertions, 37 deletions

diff --git a/gpu/command_buffer/client/program_info_manager.cc b/gpu/command_buffer/client/program_info_manager.cc
index 86fbd13..eb5ee32 100644
--- a/gpu/command_buffer/client/program_info_manager.cc
+++ b/gpu/command_buffer/client/program_info_manager.cc
@@ -5,6 +5,7 @@
 #include "../client/program_info_manager.h"
 #include "../client/atomicops.h"
 #include "../client/gles2_implementation.h"
+#include "../common/gles2_cmd_utils.h"
 
 #include <map>
 
@@ -226,35 +227,27 @@ GLint CachedProgramInfoManager::ProgramInfo::GetAttribLocation(
   return -1;
 }
 
-// TODO(gman): Add a faster lookup.
 GLint CachedProgramInfoManager::ProgramInfo::GetUniformLocation(
     const std::string& name) const {
+  bool getting_array_location = false;
+  size_t open_pos = std::string::npos;
+  int index = 0;
+  if (!GLES2Util::ParseUniformName(
+      name, &open_pos, &index, &getting_array_location)) {
+    return -1;
+  }
   for (GLuint ii = 0; ii < uniform_infos_.size(); ++ii) {
     const UniformInfo& info = uniform_infos_[ii];
     if (info.name == name ||
         (info.is_array &&
          info.name.compare(0, info.name.size() - 3, name) == 0)) {
       return info.element_locations[0];
-    } else if (info.is_array &&
-               name.size() >= 3 && name[name.size() - 1] == ']') {
+    } else if (getting_array_location && info.is_array) {
       // Look for an array specification.
-      size_t open_pos = name.find_last_of('[');
-      if (open_pos != std::string::npos &&
-          open_pos < name.size() - 2 &&
-          info.name.size() > open_pos &&
+      size_t open_pos_2 = info.name.find_last_of('[');
+      if (open_pos_2 == open_pos &&
           name.compare(0, open_pos, info.name, 0, open_pos) == 0) {
-        GLint index = 0;
-        size_t last = name.size() - 1;
-        bool bad = false;
-        for (size_t pos = open_pos + 1; pos < last; ++pos) {
-          int8 digit = name[pos] - '0';
-          if (digit < 0 || digit > 9) {
-            bad = true;
-            break;
-          }
-          index = index * 10 + digit;
-        }
-        if (!bad && index >= 0 && index < info.size) {
+        if (index >= 0 && index < info.size) {
           return info.element_locations[index];
         }
       }
diff --git a/gpu/command_buffer/common/gles2_cmd_utils.cc b/gpu/command_buffer/common/gles2_cmd_utils.cc
index ea0bd73..3ceaad8 100644
--- a/gpu/command_buffer/common/gles2_cmd_utils.cc
+++ b/gpu/command_buffer/common/gles2_cmd_utils.cc
@@ -667,6 +667,39 @@ std::string GLES2Util::GetQualifiedEnumString(
   return GetStringEnum(value);
 }
 
+bool GLES2Util::ParseUniformName(
+    const std::string& name,
+    size_t* array_pos,
+    int* element_index,
+    bool* getting_array) {
+  bool getting_array_location = false;
+  size_t open_pos = std::string::npos;
+  int index = 0;
+  if (name[name.size() - 1] == ']') {
+    if (name.size() < 3) {
+      return false;
+    }
+    open_pos = name.find_last_of('[');
+    if (open_pos == std::string::npos ||
+        open_pos >= name.size() - 2) {
+      return false;
+    }
+    size_t last = name.size() - 1;
+    for (size_t pos = open_pos + 1; pos < last; ++pos) {
+      int8 digit = name[pos] - '0';
+      if (digit < 0 || digit > 9) {
+        return false;
+      }
+      index = index * 10 + digit;
+    }
+    getting_array_location = true;
+  }
+  *getting_array = getting_array_location;
+  *element_index = index;
+  *array_pos = open_pos;
+  return true;
+}
+
 ContextCreationAttribParser::ContextCreationAttribParser()
   : alpha_size_(-1),
     blue_size_(-1),
diff --git a/gpu/command_buffer/common/gles2_cmd_utils.h b/gpu/command_buffer/common/gles2_cmd_utils.h
index c4fe663..8b3d8a7 100644
--- a/gpu/command_buffer/common/gles2_cmd_utils.h
+++ b/gpu/command_buffer/common/gles2_cmd_utils.h
@@ -146,6 +146,19 @@ class GLES2_UTILS_EXPORT GLES2Util {
   static std::string GetStringBool(uint32 value);
   static std::string GetStringError(uint32 value);
 
+  // Parses a uniform name.
+  //   array_pos: the position of the last '[' character in name.
+  //   element_index: the index of the array element specifed in the name.
+  //   getting_array: True if name refers to array.
+  // returns true of parsing was successful. Returing true does NOT mean
+  // it's a valid uniform name. On the otherhand, returning false does mean
+  // it's an invalid uniform name.
+  static bool ParseUniformName(
+      const std::string& name,
+      size_t* array_pos,
+      int* element_index,
+      bool* getting_array);
+
   #include "../common/gles2_cmd_utils_autogen.h"
 
  private:
diff --git a/gpu/command_buffer/common/gles2_cmd_utils_unittest.cc b/gpu/command_buffer/common/gles2_cmd_utils_unittest.cc
index 97b5dba..0bf11f0 100644
--- a/gpu/command_buffer/common/gles2_cmd_utils_unittest.cc
+++ b/gpu/command_buffer/common/gles2_cmd_utils_unittest.cc
@@ -181,6 +181,48 @@ TEST_F(GLES2UtilTest, RenderbufferBytesPerPixel) {
    EXPECT_EQ(0u, GLES2Util::RenderbufferBytesPerPixel(-1));
 }
 
+namespace {
+
+void CheckParseUniformName(
+    const char* name,
+    bool expected_success,
+    size_t expected_array_pos,
+    int expected_index,
+    bool expected_getting_array) {
+  int index = 1234;
+  size_t array_pos = 1244;
+  bool getting_array = false;
+  bool success = GLES2Util::ParseUniformName(
+      name, &array_pos, &index, &getting_array);
+  EXPECT_EQ(expected_success, success);
+  if (success) {
+    EXPECT_EQ(expected_array_pos, array_pos);
+    EXPECT_EQ(expected_index, index);
+    EXPECT_EQ(expected_getting_array, getting_array);
+  }
+}
+
+}  // anonymous namespace
+
+TEST_F(GLES2UtilTest, ParseUniformName) {
+  CheckParseUniformName("u_name", true, std::string::npos, 0, false);
+  CheckParseUniformName("u_name[]", false, std::string::npos, 0, false);
+  CheckParseUniformName("u_name]", false, std::string::npos, 0, false);
+  CheckParseUniformName("u_name[0a]", false, std::string::npos, 0, false);
+  CheckParseUniformName("u_name[a0]", false, std::string::npos, 0, false);
+  CheckParseUniformName("u_name[0a0]", false, std::string::npos, 0, false);
+  CheckParseUniformName("u_name[0]", true, 6u, 0, true);
+  CheckParseUniformName("u_name[2]", true, 6u, 2, true);
+  CheckParseUniformName("u_name[02]", true, 6u, 2, true);
+  CheckParseUniformName("u_name[20]", true, 6u, 20, true);
+  CheckParseUniformName("u_name[020]", true, 6u, 20, true);
+  CheckParseUniformName("u_name[0][0]", true, 9u, 0, true);
+  CheckParseUniformName("u_name[3][2]", true, 9u, 2, true);
+  CheckParseUniformName("u_name[03][02]", true, 10u, 2, true);
+  CheckParseUniformName("u_name[30][20]", true, 10u, 20, true);
+  CheckParseUniformName("u_name[030][020]", true, 11u, 20, true);
+}
+
 }  // namespace gles2
 }  // namespace gpu
 
diff --git a/gpu/command_buffer/service/program_manager.cc b/gpu/command_buffer/service/program_manager.cc
index 55d31aa..aa729ab 100644
--- a/gpu/command_buffer/service/program_manager.cc
+++ b/gpu/command_buffer/service/program_manager.cc
@@ -578,6 +578,13 @@ void ProgramManager::ProgramInfo::Validate() {
 
 GLint ProgramManager::ProgramInfo::GetUniformFakeLocation(
     const std::string& name) const {
+  bool getting_array_location = false;
+  size_t open_pos = std::string::npos;
+  int index = 0;
+  if (!GLES2Util::ParseUniformName(
+      name, &open_pos, &index, &getting_array_location)) {
+    return -1;
+  }
   for (GLuint ii = 0; ii < uniform_infos_.size(); ++ii) {
     const UniformInfo& info = uniform_infos_[ii];
     if (!info.IsValid()) {
@@ -587,26 +594,12 @@ GLint ProgramManager::ProgramInfo::GetUniformFakeLocation(
         (info.is_array &&
          info.name.compare(0, info.name.size() - 3, name) == 0)) {
       return info.fake_location_base;
-    } else if (info.is_array &&
-               name.size() >= 3 && name[name.size() - 1] == ']') {
+    } else if (getting_array_location && info.is_array) {
       // Look for an array specification.
-      size_t open_pos = name.find_last_of('[');
-      if (open_pos != std::string::npos &&
-          open_pos < name.size() - 2 &&
-          info.name.size() > open_pos &&
+      size_t open_pos_2 = info.name.find_last_of('[');
+      if (open_pos_2 == open_pos &&
           name.compare(0, open_pos, info.name, 0, open_pos) == 0) {
-        GLint index = 0;
-        size_t last = name.size() - 1;
-        bool bad = false;
-        for (size_t pos = open_pos + 1; pos < last; ++pos) {
-          int8 digit = name[pos] - '0';
-          if (digit < 0 || digit > 9) {
-            bad = true;
-            break;
-          }
-          index = index * 10 + digit;
-        }
-        if (!bad && index >= 0 && index < info.size) {
+        if (index >= 0 && index < info.size) {
           return ProgramManager::MakeFakeLocation(
               info.fake_location_base, index);
         }
diff --git a/gpu/command_buffer/service/program_manager_unittest.cc b/gpu/command_buffer/service/program_manager_unittest.cc
index 7a9c9f4..227a81f 100644
--- a/gpu/command_buffer/service/program_manager_unittest.cc
+++ b/gpu/command_buffer/service/program_manager_unittest.cc
@@ -599,6 +599,69 @@ TEST_F(ProgramManagerWithShaderTest, GLDriverReturnsGLUnderscoreUniform) {
   EXPECT_EQ(strlen(kUniform3BadName) + 4u, static_cast<size_t>(value));
 }
 
+// Test the bug comparing similar array names is fixed.
+TEST_F(ProgramManagerWithShaderTest, SimilarArrayNames) {
+  static const char* kUniform2Name = "u_nameLong[0]";
+  static const char* kUniform3Name = "u_name[0]";
+  static const GLint kUniform2Size = 2;
+  static const GLint kUniform3Size = 2;
+  static ProgramManagerWithShaderTest::UniformInfo kUniforms[] = {
+    { kUniform1Name,
+      kUniform1Size,
+      kUniform1Type,
+      kUniform1FakeLocation,
+      kUniform1RealLocation,
+      kUniform1DesiredLocation,
+      kUniform1Name,
+    },
+    { kUniform2Name,
+      kUniform2Size,
+      kUniform2Type,
+      kUniform2FakeLocation,
+      kUniform2RealLocation,
+      kUniform2DesiredLocation,
+      kUniform2Name,
+    },
+    { kUniform3Name,
+      kUniform3Size,
+      kUniform3Type,
+      kUniform3FakeLocation,
+      kUniform3RealLocation,
+      kUniform3DesiredLocation,
+      kUniform3Name,
+    },
+  };
+  const size_t kNumUniforms = arraysize(kUniforms);
+  static const GLuint kClientProgramId = 1234;
+  static const GLuint kServiceProgramId = 5679;
+  const GLuint kVShaderClientId = 2001;
+  const GLuint kFShaderClientId = 2002;
+  const GLuint kVShaderServiceId = 3001;
+  const GLuint kFShaderServiceId = 3002;
+  SetupShader(
+      kAttribs, kNumAttribs, kUniforms, kNumUniforms, kServiceProgramId);
+  ShaderManager::ShaderInfo* vshader = shader_manager_.CreateShaderInfo(
+      kVShaderClientId, kVShaderServiceId, GL_VERTEX_SHADER);
+  ASSERT_TRUE(vshader != NULL);
+  vshader->SetStatus(true, "", NULL);
+  ShaderManager::ShaderInfo* fshader = shader_manager_.CreateShaderInfo(
+      kFShaderClientId, kFShaderServiceId, GL_FRAGMENT_SHADER);
+  ASSERT_TRUE(fshader != NULL);
+  fshader->SetStatus(true, "", NULL);
+  ProgramManager::ProgramInfo* program_info =
+      manager_.CreateProgramInfo(kClientProgramId, kServiceProgramId);
+  ASSERT_TRUE(program_info != NULL);
+  EXPECT_TRUE(program_info->AttachShader(&shader_manager_, vshader));
+  EXPECT_TRUE(program_info->AttachShader(&shader_manager_, fshader));
+  program_info->Link(NULL, NULL, NULL, NULL);
+
+  // Check that we get the correct locations.
+  EXPECT_EQ(kUniform2FakeLocation,
+            program_info->GetUniformFakeLocation(kUniform2Name));
+  EXPECT_EQ(kUniform3FakeLocation,
+            program_info->GetUniformFakeLocation(kUniform3Name));
+}
+
 // Some GL drivers incorrectly return the wrong type. For example they return
 // GL_FLOAT_VEC2 when they should return GL_FLOAT_MAT2. Check we handle this.
 TEST_F(ProgramManagerWithShaderTest, GLDriverReturnsWrongTypeInfo) {