From 35839407bcb7c86e070fc8c5b5d04624f1009cff Mon Sep 17 00:00:00 2001 From: Forrest Reiling Date: Thu, 23 Dec 2021 21:38:32 +0000 Subject: [PATCH] [fuchsia][shader warmup] fix for fxbug.dev/90387 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. --- shell/platform/fuchsia/flutter/engine.cc | 87 +++++++++++++------ shell/platform/fuchsia/flutter/engine.h | 3 +- .../fuchsia/flutter/tests/engine_unittests.cc | 18 ++-- 3 files changed, 73 insertions(+), 35 deletions(-) diff --git a/shell/platform/fuchsia/flutter/engine.cc b/shell/platform/fuchsia/flutter/engine.cc index 011df3aa4b676..ccad16a902d50 100644 --- a/shell/platform/fuchsia/flutter/engine.cc +++ b/shell/platform/fuchsia/flutter/engine.cc @@ -860,26 +860,36 @@ void Engine::WarmupSkps( SkISize size, std::shared_ptr asset_manager, std::optional> skp_names, - std::optional> 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> 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* skp_warmup_surface = + new std::unique_ptr(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> 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(skp_warmup_surface); + delete static_cast*>( + 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) { diff --git a/shell/platform/fuchsia/flutter/engine.h b/shell/platform/fuchsia/flutter/engine.h index 76e4caa16ce35..e11c41e878c14 100644 --- a/shell/platform/fuchsia/flutter/engine.h +++ b/shell/platform/fuchsia/flutter/engine.h @@ -108,7 +108,8 @@ class Engine final : public fuchsia::memorypressure::Watcher { SkISize size, std::shared_ptr asset_manager, std::optional> skp_names, - std::optional> completion_callback); + std::optional> completion_callback, + bool synchronous = false); void OnMainIsolateStart(); diff --git a/shell/platform/fuchsia/flutter/tests/engine_unittests.cc b/shell/platform/fuchsia/flutter/tests/engine_unittests.cc index 10afd5fae5706..ef88ca17e9b6b 100644 --- a/shell/platform/fuchsia/flutter/tests/engine_unittests.cc +++ b/shell/platform/fuchsia/flutter/tests/engine_unittests.cc @@ -66,10 +66,11 @@ class EngineTest : public ::testing::Test { uint64_t height, std::shared_ptr asset_manager, std::optional> skp_names, - std::optional> completion_callback) { + std::optional> 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(); @@ -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 loop_; MockTaskRunner concurrent_task_runner_; MockTaskRunner raster_task_runner_; std::shared_ptr surface_producer_; @@ -163,7 +166,7 @@ TEST_F(EngineTest, SkpWarmup) { std::make_unique(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(std::move(asset_dir_fd), false)); - std::vector skp_names = {skp_name}; + std::vector skp_names = {subdir_name + "/" + skp_name}; WarmupSkps(draw_size.width(), draw_size.height(), asset_manager, skp_names, [](uint32_t count) { EXPECT_EQ(1u, count); });