[Impeller] Support access the descriptor of PipelineFuture directly (flutter#37415)

ColdPaleLight · schwa423 · commit 48a500227ed5 · 2022-11-16T16:26:32.000Z
* [Impeller] Support access the descriptor of PipelineFuture directly

* Tweak code

* Clean code
diff --git a/impeller/entity/contents/content_context.cc b/impeller/entity/contents/content_context.cc
@@ -219,11 +219,9 @@ ContentContext::ContentContext(std::shared_ptr<Context> context)
   yuv_to_rgb_filter_pipelines_[{}] =
       CreateDefaultPipeline<YUVToRGBFilterPipeline>(*context_);
 
-  // Pipelines that are variants of the base pipelines with custom descriptors.
-  // TODO(98684): Rework this API to allow fetching the descriptor without
-  //              waiting for the pipeline to build.
-  if (auto solid_fill_pipeline = solid_fill_pipelines_[{}]->WaitAndGet()) {
-    auto clip_pipeline_descriptor = solid_fill_pipeline->GetDescriptor();
+  if (solid_fill_pipelines_[{}]->GetDescriptor().has_value()) {
+    auto clip_pipeline_descriptor =
+        solid_fill_pipelines_[{}]->GetDescriptor().value();
     clip_pipeline_descriptor.SetLabel("Clip Pipeline");
     // Disable write to all color attachments.
     auto color_attachments =
diff --git a/impeller/entity/contents/runtime_effect_contents.cc b/impeller/entity/contents/runtime_effect_contents.cc
@@ -126,7 +126,7 @@ bool RuntimeEffectContents::Render(const ContentContext& renderer,
   options.primitive_type = geometry_result.type;
   options.ApplyToPipelineDescriptor(desc);
 
-  auto pipeline = context->GetPipelineLibrary()->GetPipeline(desc).get();
+  auto pipeline = context->GetPipelineLibrary()->GetPipeline(desc).Get();
   if (!pipeline) {
     VALIDATION_LOG << "Failed to get or create runtime effect pipeline.";
     return false;
diff --git a/impeller/playground/imgui/imgui_impl_impeller.cc b/impeller/playground/imgui/imgui_impl_impeller.cc
@@ -102,7 +102,7 @@ bool ImGui_ImplImpeller_Init(
     }
 
     bd->pipeline =
-        context->GetPipelineLibrary()->GetPipeline(std::move(desc)).get();
+        context->GetPipelineLibrary()->GetPipeline(std::move(desc)).Get();
     IM_ASSERT(bd->pipeline != nullptr && "Could not create ImGui pipeline.");
 
     bd->sampler = context->GetSamplerLibrary()->GetSampler({});
diff --git a/impeller/renderer/backend/gles/pipeline_library_gles.cc b/impeller/renderer/backend/gles/pipeline_library_gles.cc
@@ -180,8 +180,9 @@ PipelineFuture<PipelineDescriptor> PipelineLibraryGLES::GetPipeline(
   }
 
   if (!reactor_) {
-    return RealizedFuture<std::shared_ptr<Pipeline<PipelineDescriptor>>>(
-        nullptr);
+    return {
+        descriptor,
+        RealizedFuture<std::shared_ptr<Pipeline<PipelineDescriptor>>>(nullptr)};
   }
 
   auto vert_function = descriptor.GetEntrypointForStage(ShaderStage::kVertex);
@@ -190,14 +191,16 @@ PipelineFuture<PipelineDescriptor> PipelineLibraryGLES::GetPipeline(
   if (!vert_function || !frag_function) {
     VALIDATION_LOG
         << "Could not find stage entrypoint functions in pipeline descriptor.";
-    return RealizedFuture<std::shared_ptr<Pipeline<PipelineDescriptor>>>(
-        nullptr);
+    return {
+        descriptor,
+        RealizedFuture<std::shared_ptr<Pipeline<PipelineDescriptor>>>(nullptr)};
   }
 
   auto promise = std::make_shared<
       std::promise<std::shared_ptr<Pipeline<PipelineDescriptor>>>>();
-  auto future = PipelineFuture<PipelineDescriptor>{promise->get_future()};
-  pipelines_[descriptor] = future;
+  auto pipeline_future =
+      PipelineFuture<PipelineDescriptor>{descriptor, promise->get_future()};
+  pipelines_[descriptor] = pipeline_future;
   auto weak_this = weak_from_this();
 
   auto result = reactor_->AddOperation(
@@ -243,19 +246,17 @@ PipelineFuture<PipelineDescriptor> PipelineLibraryGLES::GetPipeline(
       });
   FML_CHECK(result);
 
-  return future;
+  return pipeline_future;
 }
 
 // |PipelineLibrary|
 PipelineFuture<ComputePipelineDescriptor> PipelineLibraryGLES::GetPipeline(
     ComputePipelineDescriptor descriptor) {
   auto promise = std::make_shared<
       std::promise<std::shared_ptr<Pipeline<ComputePipelineDescriptor>>>>();
-  auto future =
-      PipelineFuture<ComputePipelineDescriptor>{promise->get_future()};
   // TODO(dnfield): implement compute for GLES.
   promise->set_value(nullptr);
-  return future;
+  return {descriptor, promise->get_future()};
 }
 
 // |PipelineLibrary|
diff --git a/impeller/renderer/backend/metal/pipeline_library_mtl.mm b/impeller/renderer/backend/metal/pipeline_library_mtl.mm
@@ -93,14 +93,16 @@
   }
 
   if (!IsValid()) {
-    return RealizedFuture<std::shared_ptr<Pipeline<PipelineDescriptor>>>(
-        nullptr);
+    return {
+        descriptor,
+        RealizedFuture<std::shared_ptr<Pipeline<PipelineDescriptor>>>(nullptr)};
   }
 
   auto promise = std::make_shared<
       std::promise<std::shared_ptr<Pipeline<PipelineDescriptor>>>>();
-  auto future = PipelineFuture<PipelineDescriptor>{promise->get_future()};
-  pipelines_[descriptor] = future;
+  auto pipeline_future =
+      PipelineFuture<PipelineDescriptor>{descriptor, promise->get_future()};
+  pipelines_[descriptor] = pipeline_future;
   auto weak_this = weak_from_this();
 
   auto completion_handler =
@@ -132,7 +134,7 @@
   [device_ newRenderPipelineStateWithDescriptor:GetMTLRenderPipelineDescriptor(
                                                     descriptor)
                               completionHandler:completion_handler];
-  return future;
+  return pipeline_future;
 }
 
 PipelineFuture<ComputePipelineDescriptor> PipelineLibraryMTL::GetPipeline(
@@ -143,15 +145,17 @@
   }
 
   if (!IsValid()) {
-    return RealizedFuture<std::shared_ptr<Pipeline<ComputePipelineDescriptor>>>(
-        nullptr);
+    return {
+        descriptor,
+        RealizedFuture<std::shared_ptr<Pipeline<ComputePipelineDescriptor>>>(
+            nullptr)};
   }
 
   auto promise = std::make_shared<
       std::promise<std::shared_ptr<Pipeline<ComputePipelineDescriptor>>>>();
-  auto future =
-      PipelineFuture<ComputePipelineDescriptor>{promise->get_future()};
-  compute_pipelines_[descriptor] = future;
+  auto pipeline_future = PipelineFuture<ComputePipelineDescriptor>{
+      descriptor, promise->get_future()};
+  compute_pipelines_[descriptor] = pipeline_future;
   auto weak_this = weak_from_this();
 
   auto completion_handler =
@@ -185,7 +189,7 @@ new ComputePipelineMTL(weak_this,
                                                 descriptor)
                                     options:MTLPipelineOptionNone
                           completionHandler:completion_handler];
-  return future;
+  return pipeline_future;
 }
 
 // |PipelineLibrary|
diff --git a/impeller/renderer/backend/vulkan/pipeline_library_vk.cc b/impeller/renderer/backend/vulkan/pipeline_library_vk.cc
@@ -61,14 +61,16 @@ PipelineFuture<PipelineDescriptor> PipelineLibraryVK::GetPipeline(
   }
 
   if (!IsValid()) {
-    return RealizedFuture<std::shared_ptr<Pipeline<PipelineDescriptor>>>(
-        nullptr);
+    return {
+        descriptor,
+        RealizedFuture<std::shared_ptr<Pipeline<PipelineDescriptor>>>(nullptr)};
   }
 
   auto promise = std::make_shared<
       std::promise<std::shared_ptr<Pipeline<PipelineDescriptor>>>>();
-  auto future = PipelineFuture<PipelineDescriptor>{promise->get_future()};
-  pipelines_[descriptor] = future;
+  auto pipeline_future =
+      PipelineFuture<PipelineDescriptor>{descriptor, promise->get_future()};
+  pipelines_[descriptor] = pipeline_future;
 
   auto weak_this = weak_from_this();
 
@@ -86,19 +88,17 @@ PipelineFuture<PipelineDescriptor> PipelineLibraryVK::GetPipeline(
         weak_this, descriptor, std::move(pipeline_create_info)));
   });
 
-  return future;
+  return pipeline_future;
 }
 
 // |PipelineLibrary|
 PipelineFuture<ComputePipelineDescriptor> PipelineLibraryVK::GetPipeline(
     ComputePipelineDescriptor descriptor) {
   auto promise = std::make_shared<
       std::promise<std::shared_ptr<Pipeline<ComputePipelineDescriptor>>>>();
-  auto future =
-      PipelineFuture<ComputePipelineDescriptor>{promise->get_future()};
   // TODO(dnfield): implement compute for GLES.
   promise->set_value(nullptr);
-  return future;
+  return {descriptor, promise->get_future()};
 }
 
 static vk::AttachmentDescription CreatePlaceholderAttachmentDescription(
diff --git a/impeller/renderer/compute_pipeline_descriptor.h b/impeller/renderer/compute_pipeline_descriptor.h
@@ -53,10 +53,4 @@ class ComputePipelineDescriptor final
   std::shared_ptr<const ShaderFunction> entrypoint_;
 };
 
-using ComputePipelineMap = std::unordered_map<
-    ComputePipelineDescriptor,
-    std::shared_future<std::shared_ptr<Pipeline<ComputePipelineDescriptor>>>,
-    ComparableHash<ComputePipelineDescriptor>,
-    ComparableEqual<ComputePipelineDescriptor>>;
-
 }  // namespace impeller
diff --git a/impeller/renderer/compute_unittests.cc b/impeller/renderer/compute_unittests.cc
@@ -29,7 +29,7 @@ TEST_P(ComputeTest, CanCreateComputePass) {
       SamplePipelineBuilder::MakeDefaultPipelineDescriptor(*context);
   ASSERT_TRUE(pipeline_desc.has_value());
   auto compute_pipeline =
-      context->GetPipelineLibrary()->GetPipeline(pipeline_desc).get();
+      context->GetPipelineLibrary()->GetPipeline(pipeline_desc).Get();
   ASSERT_TRUE(compute_pipeline);
 
   auto cmd_buffer = context->CreateCommandBuffer();
diff --git a/impeller/renderer/pipeline.cc b/impeller/renderer/pipeline.cc
@@ -3,6 +3,7 @@
 // found in the LICENSE file.
 
 #include "impeller/renderer/pipeline.h"
+#include <optional>
 
 #include "compute_pipeline_descriptor.h"
 #include "impeller/base/promise.h"
@@ -24,8 +25,8 @@ PipelineFuture<PipelineDescriptor> CreatePipelineFuture(
     const Context& context,
     std::optional<PipelineDescriptor> desc) {
   if (!context.IsValid()) {
-    return RealizedFuture<std::shared_ptr<Pipeline<PipelineDescriptor>>>(
-        nullptr);
+    return {desc, RealizedFuture<std::shared_ptr<Pipeline<PipelineDescriptor>>>(
+                      nullptr)};
   }
 
   return context.GetPipelineLibrary()->GetPipeline(std::move(desc));
@@ -35,8 +36,10 @@ PipelineFuture<ComputePipelineDescriptor> CreatePipelineFuture(
     const Context& context,
     std::optional<ComputePipelineDescriptor> desc) {
   if (!context.IsValid()) {
-    return RealizedFuture<std::shared_ptr<Pipeline<ComputePipelineDescriptor>>>(
-        nullptr);
+    return {
+        desc,
+        RealizedFuture<std::shared_ptr<Pipeline<ComputePipelineDescriptor>>>(
+            nullptr)};
   }
 
   return context.GetPipelineLibrary()->GetPipeline(std::move(desc));
@@ -51,7 +54,8 @@ template <typename T>
 PipelineFuture<T> Pipeline<T>::CreateVariant(
     std::function<void(T& desc)> descriptor_callback) const {
   if (!descriptor_callback) {
-    return RealizedFuture<std::shared_ptr<Pipeline<T>>>(nullptr);
+    return {std::nullopt,
+            RealizedFuture<std::shared_ptr<Pipeline<T>>>(nullptr)};
   }
 
   auto copied_desc = desc_;
@@ -62,7 +66,7 @@ PipelineFuture<T> Pipeline<T>::CreateVariant(
   if (!library) {
     VALIDATION_LOG << "The library from which this pipeline was created was "
                       "already collected.";
-    return RealizedFuture<std::shared_ptr<Pipeline<T>>>(nullptr);
+    return {desc_, RealizedFuture<std::shared_ptr<Pipeline<T>>>(nullptr)};
   }
 
   return library->GetPipeline(std::move(copied_desc));
diff --git a/impeller/renderer/pipeline.h b/impeller/renderer/pipeline.h
@@ -20,14 +20,15 @@ class PipelineLibrary;
 template <typename PipelineDescriptor_>
 class Pipeline;
 
-// TODO(csg): Using a simple future is sub-optimal since callers that want to
-// eagerly create and cache pipeline variants will have to await on the future
-// to get its pipeline descriptor (unless they have explicitly cached it). This
-// would be a concurrency pessimization.
-//
-// Use a struct that stores the future and the descriptor separately.
-template <class T>
-using PipelineFuture = std::shared_future<std::shared_ptr<Pipeline<T>>>;
+template <typename T>
+struct PipelineFuture {
+  std::optional<T> descriptor;
+  std::shared_future<std::shared_ptr<Pipeline<T>>> future;
+
+  const std::shared_ptr<Pipeline<T>> Get() const { return future.get(); }
+
+  bool IsValid() const { return future.valid(); }
+};
 
 //------------------------------------------------------------------------------
 /// @brief      Describes the fixed function and programmable aspects of
@@ -107,12 +108,16 @@ class RenderPipelineT {
       return pipeline_;
     }
     did_wait_ = true;
-    if (pipeline_future_.valid()) {
-      pipeline_ = pipeline_future_.get();
+    if (pipeline_future_.IsValid()) {
+      pipeline_ = pipeline_future_.Get();
     }
     return pipeline_;
   }
 
+  std::optional<PipelineDescriptor> GetDescriptor() const {
+    return pipeline_future_.descriptor;
+  }
+
  private:
   PipelineFuture<PipelineDescriptor> pipeline_future_;
   std::shared_ptr<Pipeline<PipelineDescriptor>> pipeline_;
@@ -145,8 +150,8 @@ class ComputePipelineT {
       return pipeline_;
     }
     did_wait_ = true;
-    if (pipeline_future_.valid()) {
-      pipeline_ = pipeline_future_.get();
+    if (pipeline_future_.IsValid()) {
+      pipeline_ = pipeline_future_.Get();
     }
     return pipeline_;
   }
diff --git a/impeller/renderer/pipeline_descriptor.h b/impeller/renderer/pipeline_descriptor.h
@@ -138,10 +138,4 @@ class PipelineDescriptor final : public Comparable<PipelineDescriptor> {
   PrimitiveType primitive_type_ = PrimitiveType::kTriangle;
 };
 
-using PipelineMap = std::unordered_map<
-    PipelineDescriptor,
-    std::shared_future<std::shared_ptr<Pipeline<PipelineDescriptor>>>,
-    ComparableHash<PipelineDescriptor>,
-    ComparableEqual<PipelineDescriptor>>;
-
 }  // namespace impeller
diff --git a/impeller/renderer/pipeline_library.cc b/impeller/renderer/pipeline_library.cc
@@ -18,7 +18,7 @@ PipelineFuture<PipelineDescriptor> PipelineLibrary::GetPipeline(
   auto promise = std::make_shared<
       std::promise<std::shared_ptr<Pipeline<PipelineDescriptor>>>>();
   promise->set_value(nullptr);
-  return promise->get_future();
+  return {descriptor, promise->get_future()};
 }
 
 PipelineFuture<ComputePipelineDescriptor> PipelineLibrary::GetPipeline(
@@ -29,7 +29,7 @@ PipelineFuture<ComputePipelineDescriptor> PipelineLibrary::GetPipeline(
   auto promise = std::make_shared<
       std::promise<std::shared_ptr<Pipeline<ComputePipelineDescriptor>>>>();
   promise->set_value(nullptr);
-  return promise->get_future();
+  return {descriptor, promise->get_future()};
 }
 
 }  // namespace impeller
diff --git a/impeller/renderer/pipeline_library.h b/impeller/renderer/pipeline_library.h
@@ -15,6 +15,17 @@ namespace impeller {
 
 class Context;
 
+using PipelineMap = std::unordered_map<PipelineDescriptor,
+                                       PipelineFuture<PipelineDescriptor>,
+                                       ComparableHash<PipelineDescriptor>,
+                                       ComparableEqual<PipelineDescriptor>>;
+
+using ComputePipelineMap =
+    std::unordered_map<ComputePipelineDescriptor,
+                       PipelineFuture<ComputePipelineDescriptor>,
+                       ComparableHash<ComputePipelineDescriptor>,
+                       ComparableEqual<ComputePipelineDescriptor>>;
+
 class PipelineLibrary : public std::enable_shared_from_this<PipelineLibrary> {
  public:
   virtual ~PipelineLibrary();
diff --git a/impeller/renderer/renderer_unittests.cc b/impeller/renderer/renderer_unittests.cc
diff --git a/impeller/runtime_stage/runtime_stage_unittests.cc b/impeller/runtime_stage/runtime_stage_unittests.cc

Original file line number	Diff line number	Diff line change
`@@ -102,7 +102,7 @@ bool ImGui_ImplImpeller_Init(`
`102`	`102`	`}`
`103`	`103`
`104`	`104`	`bd->pipeline =`
`105`		`- context->GetPipelineLibrary()->GetPipeline(std::move(desc)).get();`
	`105`	`+ context->GetPipelineLibrary()->GetPipeline(std::move(desc)).Get();`
`106`	`106`	`IM_ASSERT(bd->pipeline != nullptr && "Could not create ImGui pipeline.");`
`107`	`107`
`108`	`108`	`bd->sampler = context->GetSamplerLibrary()->GetSampler({});`