From 5bbf23efdeff5fe151b43b8d3c92d2f169037c0c Mon Sep 17 00:00:00 2001
From: Matan Lurey <matan@lurey.org>
Date: Fri, 17 Nov 2023 16:54:45 -0800
Subject: [PATCH 1/2] [Impeller] Try replacing *desc with value/is_valid
 checks.

---
 impeller/scene/scene_context.cc | 17 +++++++++++++++--
 impeller/scene/scene_context.h  | 26 ++++++++++++++++++++++++--
 2 files changed, 39 insertions(+), 4 deletions(-)

diff --git a/impeller/scene/scene_context.cc b/impeller/scene/scene_context.cc
index 66c8b07db1989..baece932832a2 100644
--- a/impeller/scene/scene_context.cc
+++ b/impeller/scene/scene_context.cc
@@ -40,11 +40,24 @@ SceneContext::SceneContext(std::shared_ptr<Context> context)
     return;
   }
 
-  pipelines_[{PipelineKey{GeometryType::kUnskinned, MaterialType::kUnlit}}] =
+  auto unskinned_variant =
       MakePipelineVariants<UnskinnedVertexShader, UnlitFragmentShader>(
           *context_);
-  pipelines_[{PipelineKey{GeometryType::kSkinned, MaterialType::kUnlit}}] =
+  if (!unskinned_variant) {
+    FML_LOG(ERROR) << "Could not create unskinned pipeline variant.";
+    return;
+  }
+  pipelines_[{PipelineKey{GeometryType::kUnskinned, MaterialType::kUnlit}}] =
+      std::move(unskinned_variant);
+
+  auto skinned_variant =
       MakePipelineVariants<SkinnedVertexShader, UnlitFragmentShader>(*context_);
+  if (!skinned_variant) {
+    FML_LOG(ERROR) << "Could not create skinned pipeline variant.";
+    return;
+  }
+  pipelines_[{PipelineKey{GeometryType::kSkinned, MaterialType::kUnlit}}] =
+      std::move(skinned_variant);
 
   {
     impeller::TextureDescriptor texture_descriptor;
diff --git a/impeller/scene/scene_context.h b/impeller/scene/scene_context.h
index 615ec4a4be5ed..6f1f36071a15b 100644
--- a/impeller/scene/scene_context.h
+++ b/impeller/scene/scene_context.h
@@ -67,9 +67,19 @@ class SceneContext {
    public:
     explicit PipelineVariantsT(Context& context) {
       auto desc = PipelineT::Builder::MakeDefaultPipelineDescriptor(context);
+      if (!desc.has_value()) {
+        is_valid_ = false;
+        return;
+      }
       // Apply default ContentContextOptions to the descriptor.
+      // TODO(matanlurey): Consider failing if the descriptor is empty.
+      //
+      // Note that previously we used to dereference *desc, which is undefined
+      // behavior if desc is empty. Ideally we would signal failure upwards
+      // and not create the PipelineVariantsT at all.
       SceneContextOptions{}.ApplyToPipelineDescriptor(
-          *context.GetCapabilities(), *desc);
+          /*capabilities=*/*context.GetCapabilities(),
+          /*desc=*/desc.value());
       variants_[{}] = std::make_unique<PipelineT>(context, desc);
     };
 
@@ -99,7 +109,10 @@ class SceneContext {
       return variant_pipeline;
     }
 
+    bool IsValid() const { return is_valid_; }
+
    private:
+    bool is_valid_ = true;
     std::unordered_map<SceneContextOptions,
                        std::unique_ptr<PipelineT>,
                        SceneContextOptions::Hash,
@@ -108,10 +121,19 @@ class SceneContext {
   };
 
   template <typename VertexShader, typename FragmentShader>
+  /// Creates a PipelineVariantsT for the given vertex and fragment shaders.
+  ///
+  /// If a pipeline could not be created, returns nullptr.
   std::unique_ptr<PipelineVariants> MakePipelineVariants(Context& context) {
+    auto pipeline =
+        PipelineVariantsT<RenderPipelineT<VertexShader, FragmentShader>>(
+            context);
+    if (!pipeline.IsValid()) {
+      return nullptr;
+    }
     return std::make_unique<
         PipelineVariantsT<RenderPipelineT<VertexShader, FragmentShader>>>(
-        context);
+        std::move(pipeline));
   }
 
   std::unordered_map<PipelineKey,

From 11a87f3c46c27e52303f7253ebf17fa0d8ccb92d Mon Sep 17 00:00:00 2001
From: Matan Lurey <matan@lurey.org>
Date: Fri, 17 Nov 2023 16:58:08 -0800
Subject: [PATCH 2/2] Remove TODO.

---
 impeller/scene/scene_context.h | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/impeller/scene/scene_context.h b/impeller/scene/scene_context.h
index 6f1f36071a15b..f85663b426770 100644
--- a/impeller/scene/scene_context.h
+++ b/impeller/scene/scene_context.h
@@ -72,11 +72,6 @@ class SceneContext {
         return;
       }
       // Apply default ContentContextOptions to the descriptor.
-      // TODO(matanlurey): Consider failing if the descriptor is empty.
-      //
-      // Note that previously we used to dereference *desc, which is undefined
-      // behavior if desc is empty. Ideally we would signal failure upwards
-      // and not create the PipelineVariantsT at all.
       SceneContextOptions{}.ApplyToPipelineDescriptor(
           /*capabilities=*/*context.GetCapabilities(),
           /*desc=*/desc.value());