From 089693066b6c038f19db2bce2762f38d9ca98267 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Fri, 6 Oct 2023 17:42:28 -0700 Subject: [PATCH 1/9] [Impeller] Cache location in metadata. --- impeller/core/shader_types.h | 3 + .../backend/gles/buffer_bindings_gles.cc | 85 ++++++++++++------- 2 files changed, 56 insertions(+), 32 deletions(-) diff --git a/impeller/core/shader_types.h b/impeller/core/shader_types.h index 9ce7954998e0c..f80dd31cc9c1e 100644 --- a/impeller/core/shader_types.h +++ b/impeller/core/shader_types.h @@ -71,11 +71,14 @@ struct ShaderStructMemberMetadata { size_t size; size_t byte_length; std::optional array_elements; + mutable std::optional location = std::nullopt; + ; }; struct ShaderMetadata { std::string name; std::vector members; + mutable std::optional location = std::nullopt; }; struct ShaderUniformSlot { diff --git a/impeller/renderer/backend/gles/buffer_bindings_gles.cc b/impeller/renderer/backend/gles/buffer_bindings_gles.cc index 65aa3ac26b6a9..af207fb942c2e 100644 --- a/impeller/renderer/backend/gles/buffer_bindings_gles.cc +++ b/impeller/renderer/backend/gles/buffer_bindings_gles.cc @@ -8,6 +8,7 @@ #include #include +#include "GLES3/gl3.h" #include "impeller/base/config.h" #include "impeller/base/validation.h" #include "impeller/renderer/backend/gles/device_buffer_gles.h" @@ -203,14 +204,26 @@ bool BufferBindingsGLES::BindUniformBuffer(const ProcTableGLES& gl, size_t element_count = member.array_elements.value_or(1); - const auto member_key = - CreateUniformMemberKey(metadata->name, member.name, element_count > 1); - const auto location = uniform_locations_.find(member_key); - if (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. - continue; + GLint location; + auto maybe_location = member.location; + 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(); } size_t element_stride = member.byte_length / element_count; @@ -238,34 +251,34 @@ bool BufferBindingsGLES::BindUniformBuffer(const ProcTableGLES& gl, case ShaderType::kFloat: switch (member.size) { case sizeof(Matrix): - gl.UniformMatrix4fv(location->second, // location - element_count, // count - GL_FALSE, // normalize - buffer_data // data + gl.UniformMatrix4fv(location, // location + element_count, // count + GL_FALSE, // normalize + buffer_data // data ); continue; case sizeof(Vector4): - gl.Uniform4fv(location->second, // location - element_count, // count - buffer_data // data + gl.Uniform4fv(location, // location + element_count, // count + buffer_data // data ); continue; case sizeof(Vector3): - gl.Uniform3fv(location->second, // location - element_count, // count - buffer_data // data + gl.Uniform3fv(location, // location + element_count, // count + buffer_data // data ); continue; case sizeof(Vector2): - gl.Uniform2fv(location->second, // location - element_count, // count - buffer_data // data + gl.Uniform2fv(location, // location + element_count, // count + buffer_data // data ); continue; case sizeof(Scalar): - gl.Uniform1fv(location->second, // location - element_count, // count - buffer_data // data + gl.Uniform1fv(location, // location + element_count, // count + buffer_data // data ); continue; } @@ -288,7 +301,7 @@ bool BufferBindingsGLES::BindUniformBuffer(const ProcTableGLES& gl, case ShaderType::kSampledImage: case ShaderType::kSampler: VALIDATION_LOG << "Could not bind uniform buffer data for key: " - << member_key; + << member.name; return false; } } @@ -306,12 +319,20 @@ bool BufferBindingsGLES::BindTextures(const ProcTableGLES& gl, return false; } - 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; + 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(); } //-------------------------------------------------------------------------- @@ -343,7 +364,7 @@ bool BufferBindingsGLES::BindTextures(const ProcTableGLES& gl, //-------------------------------------------------------------------------- /// Set the texture uniform location. /// - gl.Uniform1i(uniform->second, active_index); + gl.Uniform1i(location, active_index); //-------------------------------------------------------------------------- /// Bump up the active index at binding. From 66f8d19ca156197140fb8f2702639565c261db89 Mon Sep 17 00:00:00 2001 From: Jonah Williams Date: Fri, 6 Oct 2023 17:45:33 -0700 Subject: [PATCH 2/9] Update buffer_bindings_gles.cc --- impeller/renderer/backend/gles/buffer_bindings_gles.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/impeller/renderer/backend/gles/buffer_bindings_gles.cc b/impeller/renderer/backend/gles/buffer_bindings_gles.cc index af207fb942c2e..42c7e2e2f6d66 100644 --- a/impeller/renderer/backend/gles/buffer_bindings_gles.cc +++ b/impeller/renderer/backend/gles/buffer_bindings_gles.cc @@ -8,7 +8,6 @@ #include #include -#include "GLES3/gl3.h" #include "impeller/base/config.h" #include "impeller/base/validation.h" #include "impeller/renderer/backend/gles/device_buffer_gles.h" From 0793d8be530ac78912d5433a65c549347435438e Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Sat, 7 Oct 2023 13:08:40 -0700 Subject: [PATCH 3/9] Add docs --- impeller/core/shader_types.h | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/impeller/core/shader_types.h b/impeller/core/shader_types.h index f80dd31cc9c1e..6d373a6b8dddc 100644 --- a/impeller/core/shader_types.h +++ b/impeller/core/shader_types.h @@ -71,6 +71,11 @@ struct ShaderStructMemberMetadata { size_t size; size_t byte_length; std::optional array_elements; + /// @brief This field is used to cache the runtime location of this + /// Uniform data after the first time the shader metadata is used in + /// the OpenGLES backend. + /// + /// See buffer_binding_gles.cc. mutable std::optional location = std::nullopt; ; }; @@ -78,6 +83,11 @@ struct ShaderStructMemberMetadata { struct ShaderMetadata { std::string name; std::vector members; + /// @brief This field is used to cache the runtime location of this + /// Uniform data after the first time the shader metadata is used in + /// the OpenGLES backend. + /// + /// See buffer_binding_gles.cc. mutable std::optional location = std::nullopt; }; From 2d5618d787b32da58903ba2a8b5aa01f72063aa0 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Mon, 9 Oct 2023 19:19:18 -0700 Subject: [PATCH 4/9] sneaky cache. --- impeller/core/shader_types.h | 1 - impeller/renderer/backend/gles/BUILD.gn | 1 + .../backend/gles/buffer_bindings_gles.cc | 84 +++++++++++-------- .../backend/gles/buffer_bindings_gles.h | 15 ++++ .../test/buffer_bindings_gles_unittests.cc | 78 +++++++++++++++++ 5 files changed, 143 insertions(+), 36 deletions(-) create mode 100644 impeller/renderer/backend/gles/test/buffer_bindings_gles_unittests.cc diff --git a/impeller/core/shader_types.h b/impeller/core/shader_types.h index 6d373a6b8dddc..fd4f1b83ea947 100644 --- a/impeller/core/shader_types.h +++ b/impeller/core/shader_types.h @@ -77,7 +77,6 @@ struct ShaderStructMemberMetadata { /// /// See buffer_binding_gles.cc. mutable std::optional location = std::nullopt; - ; }; struct ShaderMetadata { diff --git a/impeller/renderer/backend/gles/BUILD.gn b/impeller/renderer/backend/gles/BUILD.gn index 2877f2c72a684..c073df036ce6d 100644 --- a/impeller/renderer/backend/gles/BUILD.gn +++ b/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", diff --git a/impeller/renderer/backend/gles/buffer_bindings_gles.cc b/impeller/renderer/backend/gles/buffer_bindings_gles.cc index af207fb942c2e..1e25feaa588a4 100644 --- a/impeller/renderer/backend/gles/buffer_bindings_gles.cc +++ b/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 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(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. diff --git a/impeller/renderer/backend/gles/buffer_bindings_gles.h b/impeller/renderer/backend/gles/buffer_bindings_gles.h index b556364b9ea86..018371f0931a4 100644 --- a/impeller/renderer/backend/gles/buffer_bindings_gles.h +++ b/impeller/renderer/backend/gles/buffer_bindings_gles.h @@ -8,6 +8,7 @@ #include #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 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& GetUniformLocationsForTesting() { + return uniform_locations_; + } + private: //---------------------------------------------------------------------------- /// @brief The arguments to glVertexAttribPointer. diff --git a/impeller/renderer/backend/gles/test/buffer_bindings_gles_unittests.cc b/impeller/renderer/backend/gles/test/buffer_bindings_gles_unittests.cc new file mode 100644 index 0000000000000..1e90aed08e444 --- /dev/null +++ b/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 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(); + + // 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 \ No newline at end of file From bb57fc571fc3a549f569f4bc41c61b205693b7f1 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Mon, 9 Oct 2023 19:27:29 -0700 Subject: [PATCH 5/9] ++ --- impeller/renderer/backend/gles/buffer_bindings_gles.cc | 2 -- impeller/renderer/backend/gles/buffer_bindings_gles.h | 1 - .../backend/gles/test/buffer_bindings_gles_unittests.cc | 1 - 3 files changed, 4 deletions(-) diff --git a/impeller/renderer/backend/gles/buffer_bindings_gles.cc b/impeller/renderer/backend/gles/buffer_bindings_gles.cc index 6a9b1122be5da..4b46d6a4efcf7 100644 --- a/impeller/renderer/backend/gles/buffer_bindings_gles.cc +++ b/impeller/renderer/backend/gles/buffer_bindings_gles.cc @@ -4,11 +4,9 @@ #include "impeller/renderer/backend/gles/buffer_bindings_gles.h" -#include #include #include -#include "impeller/base/config.h" #include "impeller/base/validation.h" #include "impeller/renderer/backend/gles/device_buffer_gles.h" #include "impeller/renderer/backend/gles/formats_gles.h" diff --git a/impeller/renderer/backend/gles/buffer_bindings_gles.h b/impeller/renderer/backend/gles/buffer_bindings_gles.h index 018371f0931a4..5d97960551b85 100644 --- a/impeller/renderer/backend/gles/buffer_bindings_gles.h +++ b/impeller/renderer/backend/gles/buffer_bindings_gles.h @@ -12,7 +12,6 @@ #include "impeller/renderer/backend/gles/gles.h" #include "impeller/renderer/backend/gles/proc_table_gles.h" #include "impeller/renderer/command.h" -#include "impeller/renderer/vertex_descriptor.h" namespace impeller { diff --git a/impeller/renderer/backend/gles/test/buffer_bindings_gles_unittests.cc b/impeller/renderer/backend/gles/test/buffer_bindings_gles_unittests.cc index 1e90aed08e444..2b663f3ac907a 100644 --- a/impeller/renderer/backend/gles/test/buffer_bindings_gles_unittests.cc +++ b/impeller/renderer/backend/gles/test/buffer_bindings_gles_unittests.cc @@ -6,7 +6,6 @@ #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 { From db573bff816ccdcbac5dce50c7466454cc3a963e Mon Sep 17 00:00:00 2001 From: Jonah Williams Date: Tue, 10 Oct 2023 06:26:57 -0700 Subject: [PATCH 6/9] Update buffer_bindings_gles_unittests.cc --- .../backend/gles/test/buffer_bindings_gles_unittests.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/impeller/renderer/backend/gles/test/buffer_bindings_gles_unittests.cc b/impeller/renderer/backend/gles/test/buffer_bindings_gles_unittests.cc index 2b663f3ac907a..cf502a4470a66 100644 --- a/impeller/renderer/backend/gles/test/buffer_bindings_gles_unittests.cc +++ b/impeller/renderer/backend/gles/test/buffer_bindings_gles_unittests.cc @@ -74,4 +74,4 @@ TEST(BufferBindingsGLES, CanCacheLocationDataInShaderMetadata) { } } // namespace testing -} // namespace impeller \ No newline at end of file +} // namespace impeller From a82e1a9ebc2001fb2408740d8c5bdc7b45618920 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Thu, 19 Oct 2023 15:49:13 -0700 Subject: [PATCH 7/9] ++ --- impeller/core/shader_types.h | 12 -- .../backend/gles/buffer_bindings_gles.cc | 121 +++++++++--------- .../backend/gles/buffer_bindings_gles.h | 33 ++--- .../renderer/backend/gles/pipeline_gles.cc | 2 +- .../renderer/backend/gles/pipeline_gles.h | 2 +- .../renderer/backend/gles/render_pass_gles.cc | 2 +- .../test/buffer_bindings_gles_unittests.cc | 77 ----------- 7 files changed, 79 insertions(+), 170 deletions(-) delete mode 100644 impeller/renderer/backend/gles/test/buffer_bindings_gles_unittests.cc diff --git a/impeller/core/shader_types.h b/impeller/core/shader_types.h index fd4f1b83ea947..9ce7954998e0c 100644 --- a/impeller/core/shader_types.h +++ b/impeller/core/shader_types.h @@ -71,23 +71,11 @@ struct ShaderStructMemberMetadata { size_t size; size_t byte_length; std::optional array_elements; - /// @brief This field is used to cache the runtime location of this - /// Uniform data after the first time the shader metadata is used in - /// the OpenGLES backend. - /// - /// See buffer_binding_gles.cc. - mutable std::optional location = std::nullopt; }; struct ShaderMetadata { std::string name; std::vector members; - /// @brief This field is used to cache the runtime location of this - /// Uniform data after the first time the shader metadata is used in - /// the OpenGLES backend. - /// - /// See buffer_binding_gles.cc. - mutable std::optional location = std::nullopt; }; struct ShaderUniformSlot { diff --git a/impeller/renderer/backend/gles/buffer_bindings_gles.cc b/impeller/renderer/backend/gles/buffer_bindings_gles.cc index 4b46d6a4efcf7..982d6c9f86600 100644 --- a/impeller/renderer/backend/gles/buffer_bindings_gles.cc +++ b/impeller/renderer/backend/gles/buffer_bindings_gles.cc @@ -88,6 +88,9 @@ bool BufferBindingsGLES::ReadUniformsBindings(const ProcTableGLES& gl, GLint uniform_count = 0; gl.GetProgramiv(program, GL_ACTIVE_UNIFORMS, &uniform_count); + + // Query the Program for all active uniform locations, and + // record this via normalized key. for (GLint i = 0; i < uniform_count; i++) { std::vector name; name.resize(max_name_size); @@ -137,11 +140,10 @@ bool BufferBindingsGLES::BindVertexAttributes(const ProcTableGLES& gl, return true; } -bool BufferBindingsGLES::BindUniformData( - const ProcTableGLES& gl, - Allocator& transients_allocator, - const Bindings& vertex_bindings, - const Bindings& fragment_bindings) const { +bool BufferBindingsGLES::BindUniformData(const ProcTableGLES& gl, + Allocator& transients_allocator, + const Bindings& vertex_bindings, + const Bindings& fragment_bindings) { for (const auto& buffer : vertex_bindings.buffers) { if (!BindUniformBuffer(gl, transients_allocator, buffer.second.view)) { return false; @@ -171,53 +173,58 @@ bool BufferBindingsGLES::UnbindVertexAttributes(const ProcTableGLES& gl) const { return true; } -// Visible for testing. -std::optional 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; +GLint BufferBindingsGLES::ComputeTextureLocation( + const ShaderMetadata* metadata) { + auto location = binding_map_.find(metadata->name); + if (location != binding_map_.end()) { + return location->second[0]; } - // Uniform was optimized out, continue. - if (maybe_location.value() == -1) { - return std::nullopt; + auto& locations = binding_map_[metadata->name] = {}; + auto computed_location = + uniform_locations_.find(CreateUniformMemberKey(metadata->name)); + if (computed_location == uniform_locations_.end()) { + locations.push_back(-1); + } else { + locations.push_back(computed_location->second); } - return maybe_location.value(); + return locations[0]; } -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; +const std::vector& BufferBindingsGLES::ComputeUniformLocations( + const ShaderMetadata* metadata) { + auto location = binding_map_.find(metadata->name); + if (location != binding_map_.end()) { + return location->second; + } + + // For each metadata member, look up the binding location and record + // it in the binding map. + auto& locations = binding_map_[metadata->name] = {}; + for (const auto member : metadata->members) { + if (member.type == ShaderType::kVoid) { + // Void types are used for padding. We are obviously not going to find + // mappings for these. Keep going. + locations.push_back(-1); + continue; } - metadata->location = uniform->second; - return uniform->second; + + size_t element_count = member.array_elements.value_or(1); + 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()) { + // Uniform was not active. + locations.push_back(-1); + continue; + } + locations.push_back(computed_location->second); } - return maybe_location.value(); + return locations; } bool BufferBindingsGLES::BindUniformBuffer(const ProcTableGLES& gl, Allocator& transients_allocator, - const BufferResource& buffer) const { + const BufferResource& buffer) { const auto* metadata = buffer.GetMetadata(); auto device_buffer = buffer.resource.buffer->GetDeviceBuffer(transients_allocator); @@ -236,31 +243,24 @@ bool BufferBindingsGLES::BindUniformBuffer(const ProcTableGLES& gl, return false; } - for (const auto& member : metadata->members) { - if (member.type == ShaderType::kVoid) { - // Void types are used for padding. We are obviously not going to find - // mappings for these. Keep going. + const auto& locations = ComputeUniformLocations(metadata); + for (auto i = 0u; i < metadata->members.size(); i++) { + const auto& member = metadata->members[i]; + auto location = locations[i]; + if (location == -1) { continue; } size_t element_count = member.array_elements.value_or(1); - - auto maybe_location = - LookupUniformLocation(metadata, member, element_count > 1); - if (!maybe_location.has_value()) { - continue; - } - auto location = maybe_location.value(); - size_t element_stride = member.byte_length / element_count; auto* buffer_data = reinterpret_cast(buffer_ptr + member.offset); std::vector array_element_buffer; if (element_count > 1) { - // When binding uniform arrays, the elements must be contiguous. Copy the - // uniforms to a temp buffer to eliminate any padding needed by the other - // backends. + // When binding uniform arrays, the elements must be contiguous. Copy + // the uniforms to a temp buffer to eliminate any padding needed by the + // other backends. array_element_buffer.resize(member.size * element_count); for (size_t element_i = 0; element_i < element_count; element_i++) { std::memcpy(array_element_buffer.data() + element_i * member.size, @@ -335,7 +335,7 @@ bool BufferBindingsGLES::BindUniformBuffer(const ProcTableGLES& gl, bool BufferBindingsGLES::BindTextures(const ProcTableGLES& gl, const Bindings& bindings, - ShaderStage stage) const { + ShaderStage stage) { size_t active_index = 0; for (const auto& data : bindings.sampled_images) { const auto& texture_gles = TextureGLES::Cast(*data.second.texture.resource); @@ -344,7 +344,10 @@ bool BufferBindingsGLES::BindTextures(const ProcTableGLES& gl, return false; } - GLint location = LookupTextureLocation(data.second.texture.GetMetadata()); + auto location = ComputeTextureLocation(data.second.texture.GetMetadata()); + if (location == -1) { + return false; + } //-------------------------------------------------------------------------- /// Set the active texture unit. diff --git a/impeller/renderer/backend/gles/buffer_bindings_gles.h b/impeller/renderer/backend/gles/buffer_bindings_gles.h index 5d97960551b85..78b64c9434b94 100644 --- a/impeller/renderer/backend/gles/buffer_bindings_gles.h +++ b/impeller/renderer/backend/gles/buffer_bindings_gles.h @@ -4,7 +4,7 @@ #pragma once -#include +#include #include #include "flutter/fml/macros.h" @@ -38,24 +38,10 @@ class BufferBindingsGLES { bool BindUniformData(const ProcTableGLES& gl, Allocator& transients_allocator, const Bindings& vertex_bindings, - const Bindings& fragment_bindings) const; + const Bindings& fragment_bindings); bool UnbindVertexAttributes(const ProcTableGLES& gl) const; - // Visible for testing. - std::optional 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& GetUniformLocationsForTesting() { - return uniform_locations_; - } - private: //---------------------------------------------------------------------------- /// @brief The arguments to glVertexAttribPointer. @@ -69,15 +55,24 @@ class BufferBindingsGLES { GLsizei offset = 0u; }; std::vector vertex_attrib_arrays_; - std::map uniform_locations_; + + std::unordered_map uniform_locations_; + + using BindingMap = std::unordered_map>; + BindingMap binding_map_ = {}; + + const std::vector& ComputeUniformLocations( + const ShaderMetadata* metadata); + + GLint ComputeTextureLocation(const ShaderMetadata* metadata); bool BindUniformBuffer(const ProcTableGLES& gl, Allocator& transients_allocator, - const BufferResource& buffer) const; + const BufferResource& buffer); bool BindTextures(const ProcTableGLES& gl, const Bindings& bindings, - ShaderStage stage) const; + ShaderStage stage); FML_DISALLOW_COPY_AND_ASSIGN(BufferBindingsGLES); }; diff --git a/impeller/renderer/backend/gles/pipeline_gles.cc b/impeller/renderer/backend/gles/pipeline_gles.cc index 79efe7454d5ae..eb996613f5a5c 100644 --- a/impeller/renderer/backend/gles/pipeline_gles.cc +++ b/impeller/renderer/backend/gles/pipeline_gles.cc @@ -35,7 +35,7 @@ const HandleGLES& PipelineGLES::GetProgramHandle() const { return handle_; } -const BufferBindingsGLES* PipelineGLES::GetBufferBindings() const { +BufferBindingsGLES* PipelineGLES::GetBufferBindings() const { return buffer_bindings_.get(); } diff --git a/impeller/renderer/backend/gles/pipeline_gles.h b/impeller/renderer/backend/gles/pipeline_gles.h index 4097684a24717..53559fc7d878e 100644 --- a/impeller/renderer/backend/gles/pipeline_gles.h +++ b/impeller/renderer/backend/gles/pipeline_gles.h @@ -28,7 +28,7 @@ class PipelineGLES final [[nodiscard]] bool UnbindProgram() const; - const BufferBindingsGLES* GetBufferBindings() const; + BufferBindingsGLES* GetBufferBindings() const; [[nodiscard]] bool BuildVertexDescriptor(const ProcTableGLES& gl, GLuint program); diff --git a/impeller/renderer/backend/gles/render_pass_gles.cc b/impeller/renderer/backend/gles/render_pass_gles.cc index 4816a2973b0da..2e1f41dcca3ca 100644 --- a/impeller/renderer/backend/gles/render_pass_gles.cc +++ b/impeller/renderer/backend/gles/render_pass_gles.cc @@ -364,7 +364,7 @@ struct RenderPassData { return false; } - const auto& vertex_desc_gles = pipeline.GetBufferBindings(); + auto vertex_desc_gles = pipeline.GetBufferBindings(); //-------------------------------------------------------------------------- /// Bind vertex and index buffers. diff --git a/impeller/renderer/backend/gles/test/buffer_bindings_gles_unittests.cc b/impeller/renderer/backend/gles/test/buffer_bindings_gles_unittests.cc deleted file mode 100644 index 2b663f3ac907a..0000000000000 --- a/impeller/renderer/backend/gles/test/buffer_bindings_gles_unittests.cc +++ /dev/null @@ -1,77 +0,0 @@ -// 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/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 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(); - - // 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 \ No newline at end of file From 205a60033bd4ecdcfe1df82252be43a19a5be234 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Fri, 20 Oct 2023 09:35:44 -0700 Subject: [PATCH 8/9] ++ --- impeller/renderer/backend/gles/BUILD.gn | 1 - 1 file changed, 1 deletion(-) diff --git a/impeller/renderer/backend/gles/BUILD.gn b/impeller/renderer/backend/gles/BUILD.gn index a440552351ab1..1d783c0f8c841 100644 --- a/impeller/renderer/backend/gles/BUILD.gn +++ b/impeller/renderer/backend/gles/BUILD.gn @@ -14,7 +14,6 @@ config("gles_config") { impeller_component("gles_unittests") { testonly = true sources = [ - "test/buffer_bindings_gles_unittests.cc", "test/capabilities_unittests.cc", "test/formats_gles_unittests.cc", "test/mock_gles.cc", From 9799b65dc3659b23c9d8e9c014a89de7ae66b628 Mon Sep 17 00:00:00 2001 From: Jonah Williams Date: Wed, 25 Oct 2023 09:16:32 -0700 Subject: [PATCH 9/9] Update buffer_bindings_gles.cc --- impeller/renderer/backend/gles/buffer_bindings_gles.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/impeller/renderer/backend/gles/buffer_bindings_gles.cc b/impeller/renderer/backend/gles/buffer_bindings_gles.cc index 982d6c9f86600..9335f4c0b3b77 100644 --- a/impeller/renderer/backend/gles/buffer_bindings_gles.cc +++ b/impeller/renderer/backend/gles/buffer_bindings_gles.cc @@ -247,6 +247,7 @@ bool BufferBindingsGLES::BindUniformBuffer(const ProcTableGLES& gl, for (auto i = 0u; i < metadata->members.size(); i++) { const auto& member = metadata->members[i]; auto location = locations[i]; + // Void type or inactive uniform. if (location == -1) { continue; }