[fuchsia][shader warmup] fix for fxbug.dev/90387

shell/platform/fuchsia/flutter/engine.cc

@@ -860,26 +860,36 @@ void Engine::WarmupSkps(
     SkISize size,
     std::shared_ptr<flutter::AssetManager> asset_manager,
     std::optional<const std::vector<std::string>> skp_names,
-    std::optional<std::function<void(uint32_t)>> completion_callback) {
-  // We use a raw pointer here because we want to keep this alive until all gpu
-  // work is done and the callbacks skia takes for this are function pointers
-  // so we are unable to use a lambda that captures the smart pointer.
-  SurfaceProducerSurface* skp_warmup_surface =
-      surface_producer->ProduceOffscreenSurface(size).release();
-  if (!skp_warmup_surface) {
-    FML_LOG(ERROR) << "Failed to create offscreen warmup surface";
-    // Tell client that zero shaders were warmed up because warmup failed.
-    if (completion_callback.has_value() && completion_callback.value()) {
-      completion_callback.value()(0);
+    std::optional<std::function<void(uint32_t)>> maybe_completion_callback,
+    bool synchronous) {
+  // Wrap the optional validity checks up in a lambda to simplify the various
+  // callsites below
+  auto completion_callback = [maybe_completion_callback](uint32_t skp_count) {
+    if (maybe_completion_callback.has_value() &&
+        maybe_completion_callback.value()) {
+      maybe_completion_callback.value()(skp_count);
     }
-    return;
-  }
+  };
+
+  // We use this bizzare raw pointer to a smart pointer thing here because we
+  // want to keep the surface alive until all gpu work is done and the
+  // callbacks skia takes for this are function pointers so we are unable to
+  // use a lambda that captures the smart pointer. We need two levels of
+  // indirection because it needs to be the same across all invocations of the
+  // raster task lambda from a single invocation of WarmupSkps, but be
+  // different across different invocations of WarmupSkps (so we cant
+  // statically initialialize it in the lambda itself). Basically the result
+  // of a mashup of wierd call dynamics, multithreading, and lifecycle
+  // management with C style Skia callbacks.
+  std::unique_ptr<SurfaceProducerSurface>* skp_warmup_surface =
+      new std::unique_ptr<SurfaceProducerSurface>(nullptr);
 
   // tell concurrent task runner to deserialize all skps available from
   // the asset manager
-  concurrent_task_runner->PostTask([raster_task_runner, skp_warmup_surface,
-                                    surface_producer, asset_manager, skp_names,
-                                    completion_callback]() {
+  concurrent_task_runner->PostTask([raster_task_runner, size,
+                                    skp_warmup_surface, surface_producer,
+                                    asset_manager, skp_names,
+                                    completion_callback, synchronous]() {
     TRACE_DURATION("flutter", "DeserializeSkps");
     std::vector<std::unique_ptr<fml::Mapping>> skp_mappings;
     if (skp_names) {
@@ -895,6 +905,13 @@ void Engine::WarmupSkps(
       skp_mappings = asset_manager->GetAsMappings(".*\\.skp$", "shaders");
     }
 
+    if (skp_mappings.size() == 0) {
+      FML_LOG(WARNING)
+          << "Engine::WarmupSkps got zero SKP mappings, returning early";
+      completion_callback(0);
+      return;
+    }
+
     size_t total_size = 0;
     for (auto& mapping : skp_mappings) {
       total_size += mapping->GetSize();
@@ -920,20 +937,35 @@ void Engine::WarmupSkps(
 
       // Tell raster task runner to warmup have the compositor
       // context warm up the newly deserialized picture
-      raster_task_runner->PostTask([skp_warmup_surface, picture,
+      raster_task_runner->PostTask([picture, skp_warmup_surface, size,
                                     surface_producer, completion_callback, i,
-                                    count = skp_mappings.size()] {
+                                    count = skp_mappings.size(), synchronous] {
         TRACE_DURATION("flutter", "WarmupSkp");
-        skp_warmup_surface->GetSkiaSurface()->getCanvas()->drawPicture(picture);
+        if (*skp_warmup_surface == nullptr) {
+          skp_warmup_surface->reset(
+              surface_producer->ProduceOffscreenSurface(size).release());
+
+          if (*skp_warmup_surface == nullptr) {
+            FML_LOG(ERROR) << "Failed to create offscreen warmup surface";
+            // Tell client that zero shaders were warmed up because warmup
+            // failed.
+            completion_callback(0);
+            return;
+          }
+        }
+
+        // Do the actual warmup
+        (*skp_warmup_surface)
+            ->GetSkiaSurface()
+            ->getCanvas()
+            ->drawPicture(picture);
 
         if (i == count - 1) {
           // We call this here instead of inside fFinishedProc below because
-          // we want to unblock the dart animation code as soon as the raster
-          // thread is free to enque work, rather than waiting for the GPU work
-          // itself to finish.
-          if (completion_callback.has_value() && completion_callback.value()) {
-            completion_callback.value()(count);
-          }
+          // we want to unblock the dart animation code as soon as the
+          // raster thread is free to enque work, rather than waiting for
+          // the GPU work itself to finish.
+          completion_callback(count);
         }
 
         if (surface_producer->gr_context()) {
@@ -946,11 +978,12 @@ void Engine::WarmupSkps(
             struct GrFlushInfo flush_info;
             flush_info.fFinishedContext = skp_warmup_surface;
             flush_info.fFinishedProc = [](void* skp_warmup_surface) {
-              delete static_cast<SurfaceProducerSurface*>(skp_warmup_surface);
+              delete static_cast<std::unique_ptr<SurfaceProducerSurface>*>(
+                  skp_warmup_surface);
             };
 
             surface_producer->gr_context()->flush(flush_info);
-            surface_producer->gr_context()->submit();
+            surface_producer->gr_context()->submit(synchronous);
           }
         } else {
           if (i == count - 1) {

shell/platform/fuchsia/flutter/engine.h

@@ -108,7 +108,8 @@ class Engine final : public fuchsia::memorypressure::Watcher {
       SkISize size,
       std::shared_ptr<flutter::AssetManager> asset_manager,
       std::optional<const std::vector<std::string>> skp_names,
-      std::optional<std::function<void(uint32_t)>> completion_callback);
+      std::optional<std::function<void(uint32_t)>> completion_callback,
+      bool synchronous = false);
 
   void OnMainIsolateStart();
 

shell/platform/fuchsia/flutter/tests/engine_unittests.cc

@@ -66,10 +66,11 @@ class EngineTest : public ::testing::Test {
       uint64_t height,
       std::shared_ptr<flutter::AssetManager> asset_manager,
       std::optional<const std::vector<std::string>> skp_names,
-      std::optional<std::function<void(uint32_t)>> completion_callback) {
+      std::optional<std::function<void(uint32_t)>> completion_callback,
+      bool synchronous = true) {
     // Have to create a message loop so default async dispatcher gets set,
     // otherwise we segfault creating the VulkanSurfaceProducer
-    auto loop = fml::MessageLoopImpl::Create();
+    loop_ = fml::MessageLoopImpl::Create();
 
     context_ = sys::ComponentContext::CreateAndServeOutgoingDirectory();
     scenic_ = context_->svc()->Connect<fuchsia::ui::scenic::Scenic>();
@@ -79,10 +80,12 @@ class EngineTest : public ::testing::Test {
 
     Engine::WarmupSkps(&concurrent_task_runner_, &raster_task_runner_,
                        surface_producer_, SkISize::Make(width, height),
-                       asset_manager, std::nullopt, std::nullopt);
+                       asset_manager, skp_names, completion_callback,
+                       synchronous);
   }
 
  protected:
+  fml::RefPtr<fml::MessageLoopImpl> loop_;
   MockTaskRunner concurrent_task_runner_;
   MockTaskRunner raster_task_runner_;
   std::shared_ptr<VulkanSurfaceProducer> surface_producer_;
@@ -163,7 +166,7 @@ TEST_F(EngineTest, SkpWarmup) {
       std::make_unique<DirectoryAssetBundle>(std::move(asset_dir_fd), false));
 
   WarmupSkps(draw_size.width(), draw_size.height(), asset_manager, std::nullopt,
-             std::nullopt);
+             [](uint32_t count) { EXPECT_EQ(1u, count); });
   concurrent_task_runner_.Run();
   raster_task_runner_.Run();
 
@@ -196,8 +199,9 @@ TEST_F(EngineTest, SkpWarmupAsync) {
   fml::ScopedTemporaryDirectory asset_dir;
   fml::UniqueFD asset_dir_fd = fml::OpenDirectory(
       asset_dir.path().c_str(), false, fml::FilePermission::kRead);
-  fml::UniqueFD subdir_fd = fml::OpenDirectory(asset_dir_fd, "shaders", true,
-                                               fml::FilePermission::kReadWrite);
+  std::string subdir_name = "shaders";
+  fml::UniqueFD subdir_fd = fml::OpenDirectory(
+      asset_dir_fd, subdir_name.c_str(), true, fml::FilePermission::kReadWrite);
   std::string skp_name = "test.skp";
 
   bool success = fml::WriteAtomically(subdir_fd, skp_name.c_str(), mapping);
@@ -207,7 +211,7 @@ TEST_F(EngineTest, SkpWarmupAsync) {
   asset_manager->PushBack(
       std::make_unique<DirectoryAssetBundle>(std::move(asset_dir_fd), false));
 
-  std::vector<std::string> skp_names = {skp_name};
+  std::vector<std::string> skp_names = {subdir_name + "/" + skp_name};
 
   WarmupSkps(draw_size.width(), draw_size.height(), asset_manager, skp_names,
              [](uint32_t count) { EXPECT_EQ(1u, count); });