From c8aa0844f22ebfccc6a1db6c15f3ba459c90f89b Mon Sep 17 00:00:00 2001 From: gaaclarke <30870216+gaaclarke@users.noreply.github.com> Date: Mon, 16 Oct 2023 11:31:13 -0700 Subject: [PATCH] [Impeller] implements a retry mechanism for dart:ui/Image.toByteData. (flutter/engine#46840) Design doc: [link](https://docs.google.com/document/d/1Uuiw3pdQxNFTA8OQuZ-kuvYg1NB42XgccQCZeqr4oII/edit#heading=h.hn6wreyrz6fm) fixes: https://github.com/flutter/flutter/issues/135245 One slight deviation from the design doc is that I decided to make ContextMTL respond to changes to the SyncSwitch instead of having it observe the app state directly. The benefits are: 1) This keeps that functionality in one location 1) It makes writing tests much easier 1) There's no need of conditional compilation between macos and ios 1) There is no need to add an objc class [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style --- .../fml/synchronization/sync_switch.cc | 22 +++++- .../flutter/fml/synchronization/sync_switch.h | 17 +++++ .../renderer/backend/metal/context_mtl.h | 19 ++++- .../renderer/backend/metal/context_mtl.mm | 31 +++++++- .../src/flutter/impeller/renderer/context.h | 21 ++++++ .../src/flutter/lib/ui/fixtures/ui_test.dart | 40 +++++++++- .../ui/painting/image_encoding_impeller.cc | 75 +++++++++++++++---- .../ui/painting/image_encoding_unittests.cc | 70 ++++++++++++++++- engine/src/flutter/shell/common/shell_test.cc | 4 +- engine/src/flutter/shell/common/shell_test.h | 2 +- .../Source/FlutterEnginePlatformViewTest.mm | 6 +- 11 files changed, 280 insertions(+), 27 deletions(-) diff --git a/engine/src/flutter/fml/synchronization/sync_switch.cc b/engine/src/flutter/fml/synchronization/sync_switch.cc index ac95106226..7340086af9 100644 --- a/engine/src/flutter/fml/synchronization/sync_switch.cc +++ b/engine/src/flutter/fml/synchronization/sync_switch.cc @@ -32,8 +32,26 @@ void SyncSwitch::Execute(const SyncSwitch::Handlers& handlers) const { } void SyncSwitch::SetSwitch(bool value) { - fml::UniqueLock lock(*mutex_); - value_ = value; + { + fml::UniqueLock lock(*mutex_); + value_ = value; + } + for (Observer* observer : observers_) { + observer->OnSyncSwitchUpdate(value); + } } +void SyncSwitch::AddObserver(Observer* observer) const { + fml::UniqueLock lock(*mutex_); + if (std::find(observers_.begin(), observers_.end(), observer) == + observers_.end()) { + observers_.push_back(observer); + } +} + +void SyncSwitch::RemoveObserver(Observer* observer) const { + fml::UniqueLock lock(*mutex_); + observers_.erase(std::remove(observers_.begin(), observers_.end(), observer), + observers_.end()); +} } // namespace fml diff --git a/engine/src/flutter/fml/synchronization/sync_switch.h b/engine/src/flutter/fml/synchronization/sync_switch.h index 470072fe4f..c01c05aac2 100644 --- a/engine/src/flutter/fml/synchronization/sync_switch.h +++ b/engine/src/flutter/fml/synchronization/sync_switch.h @@ -21,6 +21,16 @@ namespace fml { /// at a time. class SyncSwitch { public: + /// Observes changes to the SyncSwitch. + class Observer { + public: + virtual ~Observer() = default; + /// `new_is_disabled` not guaranteed to be the value of the SyncSwitch + /// during execution, it should be checked with calls to + /// SyncSwitch::Execute. + virtual void OnSyncSwitchUpdate(bool new_is_disabled) = 0; + }; + /// Represents the 2 code paths available when calling |SyncSwitch::Execute|. struct Handlers { /// Sets the handler that will be executed if the |SyncSwitch| is true. @@ -53,8 +63,15 @@ class SyncSwitch { /// @param[in] value New value for the |SyncSwitch|. void SetSwitch(bool value); + /// Threadsafe. + void AddObserver(Observer* observer) const; + + /// Threadsafe. + void RemoveObserver(Observer* observer) const; + private: mutable std::unique_ptr mutex_; + mutable std::vector observers_; bool value_; FML_DISALLOW_COPY_AND_ASSIGN(SyncSwitch); diff --git a/engine/src/flutter/impeller/renderer/backend/metal/context_mtl.h b/engine/src/flutter/impeller/renderer/backend/metal/context_mtl.h index c272c68769..165ff722e2 100644 --- a/engine/src/flutter/impeller/renderer/backend/metal/context_mtl.h +++ b/engine/src/flutter/impeller/renderer/backend/metal/context_mtl.h @@ -6,8 +6,8 @@ #include +#include #include -#include #include "flutter/fml/concurrent_message_loop.h" #include "flutter/fml/macros.h" @@ -93,7 +93,20 @@ class ContextMTL final : public Context, std::shared_ptr GetIsGpuDisabledSyncSwitch() const; + // |Context| + void StoreTaskForGPU(std::function task) override; + private: + class SyncSwitchObserver : public fml::SyncSwitch::Observer { + public: + SyncSwitchObserver(ContextMTL& parent); + virtual ~SyncSwitchObserver() = default; + void OnSyncSwitchUpdate(bool new_value) override; + + private: + ContextMTL& parent_; + }; + id device_ = nullptr; id command_queue_ = nullptr; std::shared_ptr shader_library_; @@ -103,6 +116,8 @@ class ContextMTL final : public Context, std::shared_ptr device_capabilities_; std::shared_ptr raster_message_loop_; std::shared_ptr is_gpu_disabled_sync_switch_; + std::deque> tasks_awaiting_gpu_; + std::unique_ptr sync_switch_observer_; bool is_valid_ = false; ContextMTL( @@ -114,6 +129,8 @@ class ContextMTL final : public Context, std::shared_ptr CreateCommandBufferInQueue( id queue) const; + void FlushTasksAwaitingGPU(); + FML_DISALLOW_COPY_AND_ASSIGN(ContextMTL); }; diff --git a/engine/src/flutter/impeller/renderer/backend/metal/context_mtl.mm b/engine/src/flutter/impeller/renderer/backend/metal/context_mtl.mm index 528c185480..5297ab9871 100644 --- a/engine/src/flutter/impeller/renderer/backend/metal/context_mtl.mm +++ b/engine/src/flutter/impeller/renderer/backend/metal/context_mtl.mm @@ -83,6 +83,9 @@ ContextMTL::ContextMTL( return; } + sync_switch_observer_.reset(new SyncSwitchObserver(*this)); + is_gpu_disabled_sync_switch_->AddObserver(sync_switch_observer_.get()); + // Worker task runner. { raster_message_loop_ = fml::ConcurrentMessageLoop::Create( @@ -284,7 +287,9 @@ std::shared_ptr ContextMTL::Create( return context; } -ContextMTL::~ContextMTL() = default; +ContextMTL::~ContextMTL() { + is_gpu_disabled_sync_switch_->RemoveObserver(sync_switch_observer_.get()); +} Context::BackendType ContextMTL::GetBackendType() const { return Context::BackendType::kMetal; @@ -376,4 +381,28 @@ id ContextMTL::CreateMTLCommandBuffer( return buffer; } +void ContextMTL::StoreTaskForGPU(std::function task) { + tasks_awaiting_gpu_.emplace_back(std::move(task)); + while (tasks_awaiting_gpu_.size() > kMaxTasksAwaitingGPU) { + tasks_awaiting_gpu_.front()(); + tasks_awaiting_gpu_.pop_front(); + } +} + +void ContextMTL::FlushTasksAwaitingGPU() { + for (const auto& task : tasks_awaiting_gpu_) { + task(); + } + tasks_awaiting_gpu_.clear(); +} + +ContextMTL::SyncSwitchObserver::SyncSwitchObserver(ContextMTL& parent) + : parent_(parent) {} + +void ContextMTL::SyncSwitchObserver::OnSyncSwitchUpdate(bool new_is_disabled) { + if (!new_is_disabled) { + parent_.FlushTasksAwaitingGPU(); + } +} + } // namespace impeller diff --git a/engine/src/flutter/impeller/renderer/context.h b/engine/src/flutter/impeller/renderer/context.h index 3cc847235c..3d0cb9d97a 100644 --- a/engine/src/flutter/impeller/renderer/context.h +++ b/engine/src/flutter/impeller/renderer/context.h @@ -52,6 +52,14 @@ class Context { kVulkan, }; + /// The maximum number of tasks that should ever be stored for + /// `StoreTaskForGPU`. + /// + /// This number was arbitrarily chosen. The idea is that this is a somewhat + /// rare situation where tasks happen to get executed in that tiny amount of + /// time while an app is being backgrounded but still executing. + static constexpr int32_t kMaxTasksAwaitingGPU = 10; + //---------------------------------------------------------------------------- /// @brief Destroys an Impeller context. /// @@ -176,6 +184,19 @@ class Context { CaptureContext capture; + /// Stores a task on the `ContextMTL` that is awaiting access for the GPU. + /// + /// The task will be executed in the event that the GPU access has changed to + /// being available or that the task has been canceled. The task should + /// operate with the `SyncSwitch` to make sure the GPU is accessible. + /// + /// Threadsafe. + /// + /// `task` will be executed on the platform thread. + virtual void StoreTaskForGPU(std::function task) { + FML_CHECK(false && "not supported in this context"); + } + protected: Context(); diff --git a/engine/src/flutter/lib/ui/fixtures/ui_test.dart b/engine/src/flutter/lib/ui/fixtures/ui_test.dart index 38866fb476..dc4f75bb67 100644 --- a/engine/src/flutter/lib/ui/fixtures/ui_test.dart +++ b/engine/src/flutter/lib/ui/fixtures/ui_test.dart @@ -325,7 +325,11 @@ external void _validateExternal(Uint8List result); @pragma('vm:external-name', 'ValidateError') external void _validateError(String? error); @pragma('vm:external-name', 'TurnOffGPU') -external void _turnOffGPU(); +external void _turnOffGPU(bool value); +@pragma('vm:external-name', 'FlushGpuAwaitingTasks') +external void _flushGpuAwaitingTasks(); +@pragma('vm:external-name', 'ValidateNotNull') +external void _validateNotNull(Object? object); @pragma('vm:entry-point') Future toByteDataWithoutGPU() async { @@ -338,12 +342,40 @@ Future toByteDataWithoutGPU() async { canvas.drawCircle(c, 25.0, paint); final Picture picture = pictureRecorder.endRecording(); final Image image = await picture.toImage(100, 100); - _turnOffGPU(); + _turnOffGPU(true); + Timer flusher = Timer.periodic(Duration(milliseconds: 1), (timer) { + _flushGpuAwaitingTasks(); + }); try { ByteData? byteData = await image.toByteData(); _validateError(null); - } catch (ex) { - _validateError(ex.toString()); + } catch (error) { + _validateError(error.toString()); + } finally { + flusher.cancel(); + } +} + +@pragma('vm:entry-point') +Future toByteDataRetries() async { + final PictureRecorder pictureRecorder = PictureRecorder(); + final Canvas canvas = Canvas(pictureRecorder); + final Paint paint = Paint() + ..color = Color.fromRGBO(255, 255, 255, 1.0) + ..style = PaintingStyle.fill; + final Offset c = Offset(50.0, 50.0); + canvas.drawCircle(c, 25.0, paint); + final Picture picture = pictureRecorder.endRecording(); + final Image image = await picture.toImage(100, 100); + _turnOffGPU(true); + Future.delayed(Duration(milliseconds: 10), () { + _turnOffGPU(false); + }); + try { + ByteData? byteData = await image.toByteData(); + _validateNotNull(byteData); + } catch (error) { + _validateNotNull(null); } } diff --git a/engine/src/flutter/lib/ui/painting/image_encoding_impeller.cc b/engine/src/flutter/lib/ui/painting/image_encoding_impeller.cc index 4e403d5d3f..0801f7930d 100644 --- a/engine/src/flutter/lib/ui/painting/image_encoding_impeller.cc +++ b/engine/src/flutter/lib/ui/painting/image_encoding_impeller.cc @@ -56,21 +56,68 @@ sk_sp ConvertBufferToSkImage( return raster_image; } -void DoConvertImageToRasterImpeller( +[[nodiscard]] fml::Status DoConvertImageToRasterImpeller( const sk_sp& dl_image, - std::function>)> encode_task, + const std::function>)>& encode_task, const std::shared_ptr& is_gpu_disabled_sync_switch, const std::shared_ptr& impeller_context) { + fml::Status result; is_gpu_disabled_sync_switch->Execute( fml::SyncSwitch::Handlers() - .SetIfTrue([&encode_task] { - encode_task( - fml::Status(fml::StatusCode::kUnavailable, "GPU unavailable.")); + .SetIfTrue([&result] { + result = + fml::Status(fml::StatusCode::kUnavailable, "GPU unavailable."); }) .SetIfFalse([&dl_image, &encode_task, &impeller_context] { ImageEncodingImpeller::ConvertDlImageToSkImage( - dl_image, std::move(encode_task), impeller_context); + dl_image, encode_task, impeller_context); })); + return result; +} + +/// Same as `DoConvertImageToRasterImpeller` but it will attempt to retry the +/// operation if `DoConvertImageToRasterImpeller` returns kUnavailable when the +/// GPU becomes available again. +void DoConvertImageToRasterImpellerWithRetry( + const sk_sp& dl_image, + std::function>)>&& encode_task, + const std::shared_ptr& is_gpu_disabled_sync_switch, + const std::shared_ptr& impeller_context, + const fml::RefPtr& retry_runner) { + fml::Status status = DoConvertImageToRasterImpeller( + dl_image, encode_task, is_gpu_disabled_sync_switch, impeller_context); + if (!status.ok()) { + // If the conversion failed because of the GPU is unavailable, store the + // task on the Context so it can be executed when the GPU becomes available. + if (status.code() == fml::StatusCode::kUnavailable) { + impeller_context->StoreTaskForGPU( + [dl_image, encode_task = std::move(encode_task), + is_gpu_disabled_sync_switch, impeller_context, + retry_runner]() mutable { + auto retry_task = [dl_image, encode_task = std::move(encode_task), + is_gpu_disabled_sync_switch, impeller_context] { + fml::Status retry_status = DoConvertImageToRasterImpeller( + dl_image, encode_task, is_gpu_disabled_sync_switch, + impeller_context); + if (!retry_status.ok()) { + // The retry failed for some reason, maybe the GPU became + // unavailable again. Don't retry again, just fail in this case. + encode_task(retry_status); + } + }; + // If a `retry_runner` is specified, post the retry to it, otherwise + // execute it directly. + if (retry_runner) { + retry_runner->PostTask(retry_task); + } else { + retry_task(); + } + }); + } else { + // Pass on errors that are not `kUnavailable`. + encode_task(status); + } + } } } // namespace @@ -153,18 +200,20 @@ void ImageEncodingImpeller::ConvertImageToRaster( }; if (dl_image->owning_context() != DlImage::OwningContext::kRaster) { - DoConvertImageToRasterImpeller(dl_image, std::move(encode_task), - is_gpu_disabled_sync_switch, - impeller_context); + DoConvertImageToRasterImpellerWithRetry(dl_image, std::move(encode_task), + is_gpu_disabled_sync_switch, + impeller_context, + /*retry_runner=*/nullptr); return; } raster_task_runner->PostTask([dl_image, encode_task = std::move(encode_task), io_task_runner, is_gpu_disabled_sync_switch, - impeller_context]() mutable { - DoConvertImageToRasterImpeller(dl_image, std::move(encode_task), - is_gpu_disabled_sync_switch, - impeller_context); + impeller_context, + raster_task_runner]() mutable { + DoConvertImageToRasterImpellerWithRetry( + dl_image, std::move(encode_task), is_gpu_disabled_sync_switch, + impeller_context, raster_task_runner); }); } diff --git a/engine/src/flutter/lib/ui/painting/image_encoding_unittests.cc b/engine/src/flutter/lib/ui/painting/image_encoding_unittests.cc index cf8abf76b2..8de593ce4b 100644 --- a/engine/src/flutter/lib/ui/painting/image_encoding_unittests.cc +++ b/engine/src/flutter/lib/ui/painting/image_encoding_unittests.cc @@ -226,6 +226,55 @@ std::shared_ptr MakeConvertDlImageToSkImageContext( } } // namespace +TEST_F(ShellTest, EncodeImageRetries) { +#ifndef FML_OS_MACOSX + // Only works on macos currently. + GTEST_SKIP(); +#endif + Settings settings = CreateSettingsForFixture(); + settings.enable_impeller = true; + TaskRunners task_runners("test", // label + GetCurrentTaskRunner(), // platform + CreateNewThread(), // raster + CreateNewThread(), // ui + CreateNewThread() // io + ); + + std::unique_ptr shell = CreateShell({ + .settings = settings, + .task_runners = task_runners, + }); + + auto turn_off_gpu = [&](Dart_NativeArguments args) { + auto handle = Dart_GetNativeArgument(args, 0); + bool value = true; + ASSERT_TRUE(Dart_IsBoolean(handle)); + Dart_BooleanValue(handle, &value); + TurnOffGPU(shell.get(), value); + }; + + AddNativeCallback("TurnOffGPU", CREATE_NATIVE_ENTRY(turn_off_gpu)); + + auto validate_not_null = [&](Dart_NativeArguments args) { + auto handle = Dart_GetNativeArgument(args, 0); + EXPECT_FALSE(Dart_IsNull(handle)); + message_latch.Signal(); + }; + + AddNativeCallback("ValidateNotNull", CREATE_NATIVE_ENTRY(validate_not_null)); + + ASSERT_TRUE(shell->IsSetup()); + auto configuration = RunConfiguration::InferFromSettings(settings); + configuration.SetEntrypoint("toByteDataRetries"); + + shell->RunEngine(std::move(configuration), [&](auto result) { + ASSERT_EQ(result, Engine::RunStatus::Success); + }); + + message_latch.Wait(); + DestroyShell(std::move(shell), task_runners); +} + TEST_F(ShellTest, EncodeImageFailsWithoutGPUImpeller) { #ifndef FML_OS_MACOSX // Only works on macos currently. @@ -257,11 +306,30 @@ TEST_F(ShellTest, EncodeImageFailsWithoutGPUImpeller) { }); auto turn_off_gpu = [&](Dart_NativeArguments args) { - TurnOffGPU(shell.get()); + auto handle = Dart_GetNativeArgument(args, 0); + bool value = true; + ASSERT_TRUE(Dart_IsBoolean(handle)); + Dart_BooleanValue(handle, &value); + TurnOffGPU(shell.get(), true); }; AddNativeCallback("TurnOffGPU", CREATE_NATIVE_ENTRY(turn_off_gpu)); + auto flush_awaiting_tasks = [&](Dart_NativeArguments args) { + task_runners.GetIOTaskRunner()->PostTask([&] { + std::shared_ptr impeller_context = + shell->GetIOManager()->GetImpellerContext(); + // This will cause the stored tasks to overflow and start throwing them + // away. + for (int i = 0; i < impeller::Context::kMaxTasksAwaitingGPU; ++i) { + impeller_context->StoreTaskForGPU([] {}); + } + }); + }; + + AddNativeCallback("FlushGpuAwaitingTasks", + CREATE_NATIVE_ENTRY(flush_awaiting_tasks)); + ASSERT_TRUE(shell->IsSetup()); auto configuration = RunConfiguration::InferFromSettings(settings); configuration.SetEntrypoint("toByteDataWithoutGPU"); diff --git a/engine/src/flutter/shell/common/shell_test.cc b/engine/src/flutter/shell/common/shell_test.cc index f9f59f7366..c673d4b429 100644 --- a/engine/src/flutter/shell/common/shell_test.cc +++ b/engine/src/flutter/shell/common/shell_test.cc @@ -382,8 +382,8 @@ size_t ShellTest::GetLiveTrackedPathCount( }); } -void ShellTest::TurnOffGPU(Shell* shell) { - shell->is_gpu_disabled_sync_switch_->SetSwitch(true); +void ShellTest::TurnOffGPU(Shell* shell, bool value) { + shell->is_gpu_disabled_sync_switch_->SetSwitch(value); } } // namespace testing diff --git a/engine/src/flutter/shell/common/shell_test.h b/engine/src/flutter/shell/common/shell_test.h index dfcc22223e..7ded997fbc 100644 --- a/engine/src/flutter/shell/common/shell_test.h +++ b/engine/src/flutter/shell/common/shell_test.h @@ -135,7 +135,7 @@ class ShellTest : public FixtureTest { static size_t GetLiveTrackedPathCount( const std::shared_ptr& tracker); - static void TurnOffGPU(Shell* shell); + static void TurnOffGPU(Shell* shell, bool value); private: ThreadHost thread_host_; diff --git a/engine/src/flutter/shell/platform/darwin/ios/framework/Source/FlutterEnginePlatformViewTest.mm b/engine/src/flutter/shell/platform/darwin/ios/framework/Source/FlutterEnginePlatformViewTest.mm index 0b5526a534..51c76156b4 100644 --- a/engine/src/flutter/shell/platform/darwin/ios/framework/Source/FlutterEnginePlatformViewTest.mm +++ b/engine/src/flutter/shell/platform/darwin/ios/framework/Source/FlutterEnginePlatformViewTest.mm @@ -65,6 +65,7 @@ flutter::FakeDelegate fake_delegate; - (void)setUp { fml::MessageLoop::EnsureInitializedForCurrentThread(); auto thread_task_runner = fml::MessageLoop::GetCurrent().GetTaskRunner(); + auto sync_switch = std::make_shared(); flutter::TaskRunners runners(/*label=*/self.name.UTF8String, /*platform=*/thread_task_runner, /*raster=*/thread_task_runner, @@ -76,7 +77,7 @@ flutter::FakeDelegate fake_delegate; /*platform_views_controller=*/nil, /*task_runners=*/runners, /*worker_task_runner=*/nil, - /*is_gpu_disabled_sync_switch=*/nil); + /*is_gpu_disabled_sync_switch=*/sync_switch); weak_factory = std::make_unique>(platform_view.get()); } @@ -98,6 +99,7 @@ flutter::FakeDelegate fake_delegate; fake_delegate.settings_.msaa_samples = 4; auto thread_task_runner = fml::MessageLoop::GetCurrent().GetTaskRunner(); + auto sync_switch = std::make_shared(); flutter::TaskRunners runners(/*label=*/self.name.UTF8String, /*platform=*/thread_task_runner, /*raster=*/thread_task_runner, @@ -109,7 +111,7 @@ flutter::FakeDelegate fake_delegate; /*platform_views_controller=*/nil, /*task_runners=*/runners, /*worker_task_runner=*/nil, - /*is_gpu_disabled_sync_switch=*/nil); + /*is_gpu_disabled_sync_switch=*/sync_switch); XCTAssertEqual(msaa_4x_platform_view->GetIosContext()->GetMsaaSampleCount(), MsaaSampleCount::kFour);