sneaky cache.

impeller/core/shader_types.h

@@ -77,7 +77,6 @@ struct ShaderStructMemberMetadata {
   ///
   ///        See buffer_binding_gles.cc.
   mutable std::optional<int> location = std::nullopt;
-  ;
 };
 
 struct ShaderMetadata {

impeller/renderer/backend/gles/BUILD.gn

@@ -14,6 +14,7 @@ config("gles_config") {
 impeller_component("gles_unittests") {
   testonly = true
   sources = [
+    "test/buffer_bindings_gles_unittests.cc",
     "test/capabilities_unittests.cc",
     "test/mock_gles.cc",
     "test/mock_gles.h",

impeller/renderer/backend/gles/buffer_bindings_gles.cc

@@ -174,6 +174,50 @@ bool BufferBindingsGLES::UnbindVertexAttributes(const ProcTableGLES& gl) const {
   return true;
 }
 
+// Visible for testing.
+std::optional<GLint> BufferBindingsGLES::LookupUniformLocation(
+    const ShaderMetadata* metadata,
+    const ShaderStructMemberMetadata& member,
+    bool is_array) const {
+  auto maybe_location = member.location;
+  if (!maybe_location.has_value()) {
+    const auto member_key =
+        CreateUniformMemberKey(metadata->name, member.name, is_array);
+    const auto computed_location = uniform_locations_.find(member_key);
+    if (computed_location == uniform_locations_.end()) {
+      // The list of uniform locations only contains "active" uniforms that
+      // are not optimized out. So this situation is expected to happen when
+      // unused uniforms are present in the shader.
+      member.location = -1;
+      return std::nullopt;
+    }
+    auto location = computed_location->second;
+    member.location = computed_location->second;
+    return location;
+  }
+  // Uniform was optimized out, continue.
+  if (maybe_location.value() == -1) {
+    return std::nullopt;
+  }
+  return maybe_location.value();
+}
+
+GLint BufferBindingsGLES::LookupTextureLocation(
+    const ShaderMetadata* metadata) const {
+  auto maybe_location = metadata->location;
+  if (!maybe_location.has_value()) {
+    const auto uniform_key = CreateUniformMemberKey(metadata->name);
+    auto uniform = uniform_locations_.find(uniform_key);
+    if (uniform == uniform_locations_.end()) {
+      VALIDATION_LOG << "Could not find uniform for key: " << uniform_key;
+      return false;
+    }
+    metadata->location = uniform->second;
+    return uniform->second;
+  }
+  return maybe_location.value();
+}
+
 bool BufferBindingsGLES::BindUniformBuffer(const ProcTableGLES& gl,
                                            Allocator& transients_allocator,
                                            const BufferResource& buffer) const {
@@ -204,30 +248,14 @@ bool BufferBindingsGLES::BindUniformBuffer(const ProcTableGLES& gl,
 
     size_t element_count = member.array_elements.value_or(1);
 
-    GLint location;
-    auto maybe_location = member.location;
+    auto maybe_location =
+        LookupUniformLocation(metadata, member, element_count > 1);
     if (!maybe_location.has_value()) {
-      const auto member_key = CreateUniformMemberKey(
-          metadata->name, member.name, element_count > 1);
-      const auto computed_location = uniform_locations_.find(member_key);
-      if (computed_location == uniform_locations_.end()) {
-        // The list of uniform locations only contains "active" uniforms that
-        // are not optimized out. So this situation is expected to happen when
-        // unused uniforms are present in the shader.
-        member.location = -1;
-        continue;
-      }
-      location = computed_location->second;
-      member.location = computed_location->second;
-    } else {
-      if (maybe_location.value() == -1) {
-        continue;
-      }
-      location = maybe_location.value();
+      continue;
     }
+    auto location = maybe_location.value();
 
     size_t element_stride = member.byte_length / element_count;
-
     auto* buffer_data =
         reinterpret_cast<const GLfloat*>(buffer_ptr + member.offset);
 
@@ -319,21 +347,7 @@ bool BufferBindingsGLES::BindTextures(const ProcTableGLES& gl,
       return false;
     }
 
-    GLint location;
-    auto maybe_location = data.second.texture.GetMetadata()->location;
-    if (!maybe_location.has_value()) {
-      const auto uniform_key =
-          CreateUniformMemberKey(data.second.texture.GetMetadata()->name);
-      auto uniform = uniform_locations_.find(uniform_key);
-      if (uniform == uniform_locations_.end()) {
-        VALIDATION_LOG << "Could not find uniform for key: " << uniform_key;
-        return false;
-      }
-      data.second.texture.GetMetadata()->location = uniform->second;
-      location = uniform->second;
-    } else {
-      location = maybe_location.value();
-    }
+    GLint location = LookupTextureLocation(data.second.texture.GetMetadata());
 
     //--------------------------------------------------------------------------
     /// Set the active texture unit.

impeller/renderer/backend/gles/buffer_bindings_gles.h

@@ -8,6 +8,7 @@
 #include <vector>
 
 #include "flutter/fml/macros.h"
+#include "impeller/core/shader_types.h"
 #include "impeller/renderer/backend/gles/gles.h"
 #include "impeller/renderer/backend/gles/proc_table_gles.h"
 #include "impeller/renderer/command.h"
@@ -42,6 +43,20 @@ class BufferBindingsGLES {
 
   bool UnbindVertexAttributes(const ProcTableGLES& gl) const;
 
+  // Visible for testing.
+  std::optional<GLint> LookupUniformLocation(
+      const ShaderMetadata* metadata,
+      const ShaderStructMemberMetadata& member,
+      bool is_array) const;
+
+  // Visible for testing.
+  GLint LookupTextureLocation(const ShaderMetadata* metadata) const;
+
+  // Visible for testing.
+  std::map<std::string, GLint>& GetUniformLocationsForTesting() {
+    return uniform_locations_;
+  }
+
  private:
   //----------------------------------------------------------------------------
   /// @brief      The arguments to glVertexAttribPointer.

impeller/renderer/backend/gles/test/buffer_bindings_gles_unittests.cc

@@ -0,0 +1,78 @@
+// Copyright 2013 The Flutter Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "flutter/testing/testing.h"  // IWYU pragma: keep
+#include "gtest/gtest.h"
+#include "impeller/core/shader_types.h"
+#include "impeller/renderer/backend/gles/buffer_bindings_gles.h"
+#include "impeller/renderer/backend/gles/proc_table_gles.h"
+#include "impeller/renderer/backend/gles/test/mock_gles.h"
+
+namespace impeller {
+namespace testing {
+
+ShaderType type;
+std::string name;
+size_t offset;
+size_t size;
+size_t byte_length;
+std::optional<size_t> array_elements;
+
+TEST(BufferBindingsGLES, CanCacheLocationDataInShaderMetadata) {
+  // Uniform metadata for
+  // struct Foo {
+  //   float Bar;
+  //   float Baz;
+  // }
+  const ShaderMetadata metadata =
+      ShaderMetadata{.name = "Foo",
+                     .members = {ShaderStructMemberMetadata{
+                                     .type = ShaderType::kFloat,
+                                     .name = "Bar",
+                                     .offset = 0u,
+                                     .size = sizeof(Scalar),
+                                     .byte_length = sizeof(Scalar),
+                                     .array_elements = std::nullopt,
+                                 },
+                                 ShaderStructMemberMetadata{
+                                     .type = ShaderType::kFloat,
+                                     .name = "Baz",
+                                     .offset = sizeof(Scalar),
+                                     .size = sizeof(Scalar),
+                                     .byte_length = sizeof(Scalar),
+                                     .array_elements = std::nullopt,
+                                 }}};
+  // Uniform metadata for
+  // sampler2D Fizz;
+  const ShaderMetadata sampler_metadata = ShaderMetadata{.name = "Fizz"};
+
+  auto buffer_bindings = std::make_shared<BufferBindingsGLES>();
+
+  // Mock uniform locations.
+  buffer_bindings->GetUniformLocationsForTesting()["FOO.BAR"] = 0;
+  buffer_bindings->GetUniformLocationsForTesting()["FOO.BAZ"] = 1;
+  buffer_bindings->GetUniformLocationsForTesting()["FIZZ"] = 2;
+
+  // Lookup locations.
+
+  auto bar_location = buffer_bindings->LookupUniformLocation(
+      &metadata, metadata.members[0], false);
+  auto baz_location = buffer_bindings->LookupUniformLocation(
+      &metadata, metadata.members[1], false);
+  auto fizz_location =
+      buffer_bindings->LookupTextureLocation(&sampler_metadata);
+
+  EXPECT_EQ(bar_location.value_or(-1), 0);
+  EXPECT_EQ(baz_location.value_or(-1), 1);
+  EXPECT_EQ(fizz_location, 2);
+
+  // values have been cached.
+
+  EXPECT_EQ(metadata.members[0].location.value_or(-1), 0);
+  EXPECT_EQ(metadata.members[1].location.value_or(-1), 1);
+  EXPECT_EQ(sampler_metadata.location.value_or(-1), 2);
+}
+
+}  // namespace testing
+}  // namespace impeller