[fuchsia][shader warmup] fix for fxbug.dev/90387 (flutter#30482)

Warmup surface was being initialized on the platform thread rather than
the raster thread which was causing races in the skia cache, this change
just moves the warmup surface intiialization to the raster thread.

This change also addresses another minor problem where invoking shader
warmup with 0 skps would drop the callback on the floor and that would
cause the dart future to hang forever. Simply invoke the dart future if
no SKP's are provided.

Author: freiling

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); });