Reverts "[Impeller] Encode directly to command buffer for Vulkan." (#…

…49818) Reverts #49780 Initiated by: jonahwilliams This change reverts the following previous change: Original Description: Part of flutter/flutter#140804 Rather than using impeller::Command, the impeller::RenderPass records most state directly into the Vulkan command buffer. This should remove allocation/free overhead of the intermediary structures and make further improvements to the backend even easier. This required a number of other changes to the renderer: 1. The render pass holds a strong ptr to the context. This helps avoid locking continually while encoding, which is quite slow. 2. barriers need to be encoded on the _producing_ side, and not the consuming side. This is because we'll actually run the consuming code before the producing code. i.e. we transition to shader read at the end of a render pass instead of when binding. 3. I've updated the binding code to also provide the descriptor type so that we don't need to look it up from the desc. set. 4. I added a test render pass class that records commands.
flutter · Jan 17, 2024 · 73a2de5 · 73a2de5
1 parent 60ce382
commit 73a2de5
Show file tree

Hide file tree

Showing 40 changed files with 445 additions and 805 deletions.
diff --git a/impeller/aiks/BUILD.gn b/impeller/aiks/BUILD.gn
@@ -76,7 +76,6 @@ impeller_component("context_spy") {
     "testing/context_spy.h",
   ]
   deps = [
-    "//flutter/impeller/entity:entity_test_helpers",
     "//flutter/impeller/renderer",
     "//flutter/testing:testing_lib",
   ]

diff --git a/impeller/aiks/testing/context_spy.cc b/impeller/aiks/testing/context_spy.cc
@@ -2,8 +2,6 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include <memory>
-
 #include "impeller/aiks/testing/context_spy.h"
 
 namespace impeller {
@@ -66,17 +64,14 @@ std::shared_ptr<ContextMock> ContextSpy::MakeContext(
             });
 
         ON_CALL(*spy, OnCreateRenderPass)
-            .WillByDefault([real_buffer, shared_this,
-                            real_context](const RenderTarget& render_target) {
-              std::shared_ptr<RenderPass> result =
-                  CommandBufferMock::ForwardOnCreateRenderPass(
-                      real_buffer.get(), render_target);
-              std::shared_ptr<RecordingRenderPass> recorder =
-                  std::make_shared<RecordingRenderPass>(result, real_context,
-                                                        render_target);
-              shared_this->render_passes_.push_back(recorder);
-              return recorder;
-            });
+            .WillByDefault(
+                [real_buffer, shared_this](const RenderTarget& render_target) {
+                  std::shared_ptr<RenderPass> result =
+                      CommandBufferMock::ForwardOnCreateRenderPass(
+                          real_buffer.get(), render_target);
+                  shared_this->render_passes_.push_back(result);
+                  return result;
+                });
 
         ON_CALL(*spy, OnCreateBlitPass).WillByDefault([real_buffer]() {
           return CommandBufferMock::ForwardOnCreateBlitPass(real_buffer.get());

diff --git a/impeller/aiks/testing/context_spy.h b/impeller/aiks/testing/context_spy.h
@@ -6,9 +6,7 @@
 #define FLUTTER_IMPELLER_AIKS_TESTING_CONTEXT_SPY_H_
 
 #include <memory>
-
 #include "impeller/aiks/testing/context_mock.h"
-#include "impeller/entity/contents/test/recording_render_pass.h"
 
 namespace impeller {
 namespace testing {
@@ -22,7 +20,7 @@ class ContextSpy : public std::enable_shared_from_this<ContextSpy> {
   std::shared_ptr<ContextMock> MakeContext(
       const std::shared_ptr<Context>& real_context);
 
-  std::vector<std::shared_ptr<RecordingRenderPass>> render_passes_;
+  std::vector<std::shared_ptr<RenderPass>> render_passes_;
 
  private:
   ContextSpy() = default;

diff --git a/impeller/compiler/code_gen_template.h b/impeller/compiler/code_gen_template.h
@@ -161,7 +161,7 @@ struct {{camel_case(shader_name)}}{{camel_case(shader_stage)}}Shader {
 {% endfor %}) {
     return {{ proto.args.0.argument_name }}.BindResource({% for arg in proto.args %}
   {% if loop.is_first %}
-{{to_shader_stage(shader_stage)}}, {{ proto.descriptor_type }}, kResource{{ proto.name }}, kMetadata{{ proto.name }}, {% else %}
+{{to_shader_stage(shader_stage)}}, kResource{{ proto.name }}, kMetadata{{ proto.name }}, {% else %}
 std::move({{ arg.argument_name }}){% if not loop.is_last %}, {% endif %}
   {% endif %}
   {% endfor %});

diff --git a/impeller/compiler/reflector.cc b/impeller/compiler/reflector.cc
@@ -1308,7 +1308,6 @@ std::vector<Reflector::BindPrototype> Reflector::ReflectBindPrototypes(
     auto& proto = prototypes.emplace_back(BindPrototype{});
     proto.return_type = "bool";
     proto.name = ToCamelCase(uniform_buffer.name);
-    proto.descriptor_type = "DescriptorType::kUniformBuffer";
     {
       std::stringstream stream;
       stream << "Bind uniform buffer for resource named " << uniform_buffer.name
@@ -1328,7 +1327,6 @@ std::vector<Reflector::BindPrototype> Reflector::ReflectBindPrototypes(
     auto& proto = prototypes.emplace_back(BindPrototype{});
     proto.return_type = "bool";
     proto.name = ToCamelCase(storage_buffer.name);
-    proto.descriptor_type = "DescriptorType::kStorageBuffer";
     {
       std::stringstream stream;
       stream << "Bind storage buffer for resource named " << storage_buffer.name
@@ -1348,7 +1346,6 @@ std::vector<Reflector::BindPrototype> Reflector::ReflectBindPrototypes(
     auto& proto = prototypes.emplace_back(BindPrototype{});
     proto.return_type = "bool";
     proto.name = ToCamelCase(sampled_image.name);
-    proto.descriptor_type = "DescriptorType::kSampledImage";
     {
       std::stringstream stream;
       stream << "Bind combined image sampler for resource named "
@@ -1372,7 +1369,6 @@ std::vector<Reflector::BindPrototype> Reflector::ReflectBindPrototypes(
     auto& proto = prototypes.emplace_back(BindPrototype{});
     proto.return_type = "bool";
     proto.name = ToCamelCase(separate_image.name);
-    proto.descriptor_type = "DescriptorType::kImage";
     {
       std::stringstream stream;
       stream << "Bind separate image for resource named " << separate_image.name
@@ -1392,7 +1388,6 @@ std::vector<Reflector::BindPrototype> Reflector::ReflectBindPrototypes(
     auto& proto = prototypes.emplace_back(BindPrototype{});
     proto.return_type = "bool";
     proto.name = ToCamelCase(separate_sampler.name);
-    proto.descriptor_type = "DescriptorType::kSampler";
     {
       std::stringstream stream;
       stream << "Bind separate sampler for resource named "
@@ -1421,7 +1416,6 @@ nlohmann::json::array_t Reflector::EmitBindPrototypes(
     item["return_type"] = res.return_type;
     item["name"] = res.name;
     item["docstring"] = res.docstring;
-    item["descriptor_type"] = res.descriptor_type;
     auto& args = item["args"] = nlohmann::json::array_t{};
     for (const auto& arg : res.args) {
       auto& json_arg = args.emplace_back(nlohmann::json::object_t{});

diff --git a/impeller/compiler/reflector.h b/impeller/compiler/reflector.h
@@ -187,7 +187,6 @@ class Reflector {
     std::string name;
     std::string return_type;
     std::string docstring;
-    std::string descriptor_type = "";
     std::vector<BindPrototypeArgument> args;
   };
 

diff --git a/impeller/core/resource_binder.h b/impeller/core/resource_binder.h
@@ -24,13 +24,11 @@ struct ResourceBinder {
   virtual ~ResourceBinder() = default;
 
   virtual bool BindResource(ShaderStage stage,
-                            DescriptorType type,
                             const ShaderUniformSlot& slot,
                             const ShaderMetadata& metadata,
                             BufferView view) = 0;
 
   virtual bool BindResource(ShaderStage stage,
-                            DescriptorType type,
                             const SampledImageSlot& slot,
                             const ShaderMetadata& metadata,
                             std::shared_ptr<const Texture> texture,

diff --git a/impeller/entity/BUILD.gn b/impeller/entity/BUILD.gn
@@ -257,8 +257,6 @@ impeller_component("entity_test_helpers") {
   sources = [
     "contents/test/contents_test_helpers.cc",
     "contents/test/contents_test_helpers.h",
-    "contents/test/recording_render_pass.cc",
-    "contents/test/recording_render_pass.h",
   ]
 
   deps = [ ":entity" ]

diff --git a/impeller/entity/contents/checkerboard_contents_unittests.cc b/impeller/entity/contents/checkerboard_contents_unittests.cc
@@ -9,7 +9,6 @@
 
 #include "impeller/entity/contents/checkerboard_contents.h"
 #include "impeller/entity/contents/contents.h"
-#include "impeller/entity/contents/test/recording_render_pass.h"
 #include "impeller/entity/entity.h"
 #include "impeller/entity/entity_playground.h"
 #include "impeller/renderer/render_target.h"
@@ -39,14 +38,11 @@ TEST_P(EntityTest, RendersWithoutError) {
       *GetContentContext()->GetRenderTargetCache(), {100, 100},
       /*mip_count=*/1);
   auto render_pass = buffer->CreateRenderPass(render_target);
-  auto recording_pass = std::make_shared<RecordingRenderPass>(
-      render_pass, GetContext(), render_target);
-
   Entity entity;
 
-  ASSERT_TRUE(recording_pass->GetCommands().empty());
-  ASSERT_TRUE(contents->Render(*content_context, entity, *recording_pass));
-  ASSERT_FALSE(recording_pass->GetCommands().empty());
+  ASSERT_TRUE(render_pass->GetCommands().empty());
+  ASSERT_TRUE(contents->Render(*content_context, entity, *render_pass));
+  ASSERT_FALSE(render_pass->GetCommands().empty());
 }
 #endif  // IMPELLER_DEBUG
 

diff --git a/impeller/entity/contents/runtime_effect_contents.cc b/impeller/entity/contents/runtime_effect_contents.cc
@@ -199,9 +199,8 @@ bool RuntimeEffectContents::Render(const ContentContext& renderer,
         ShaderUniformSlot uniform_slot;
         uniform_slot.name = uniform.name.c_str();
         uniform_slot.ext_res_0 = uniform.location;
-        pass.BindResource(ShaderStage::kFragment,
-                          DescriptorType::kUniformBuffer, uniform_slot,
-                          metadata, buffer_view);
+        pass.BindResource(ShaderStage::kFragment, uniform_slot, metadata,
+                          buffer_view);
         buffer_index++;
         buffer_offset += uniform.GetSize();
         break;
@@ -238,8 +237,7 @@ bool RuntimeEffectContents::Render(const ContentContext& renderer,
         auto buffer_view = renderer.GetTransientsBuffer().Emplace(
             reinterpret_cast<const void*>(uniform_buffer.data()),
             sizeof(float) * uniform_buffer.size(), alignment);
-        pass.BindResource(ShaderStage::kFragment,
-                          DescriptorType::kUniformBuffer, uniform_slot,
+        pass.BindResource(ShaderStage::kFragment, uniform_slot,
                           ShaderMetadata{}, buffer_view);
       }
     }
@@ -273,8 +271,8 @@ bool RuntimeEffectContents::Render(const ContentContext& renderer,
 
         image_slot.binding = sampler_binding_location;
         image_slot.texture_index = uniform.location - minimum_sampler_index;
-        pass.BindResource(ShaderStage::kFragment, DescriptorType::kSampledImage,
-                          image_slot, *metadata, input.texture, sampler);
+        pass.BindResource(ShaderStage::kFragment, image_slot, *metadata,
+                          input.texture, sampler);
 
         sampler_index++;
         break;

diff --git a/impeller/entity/contents/test/recording_render_pass.cc b/impeller/entity/contents/test/recording_render_pass.cc