From b14952e4b8e64a20736725ad8e2883a1e2693818 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Fri, 26 May 2023 08:45:55 -0700 Subject: [PATCH 1/6] [Impeller] Avoid encoding metal commands while the GPU is unavailable when decoding images. --- lib/ui/painting/image_decoder.cc | 6 +- lib/ui/painting/image_decoder.h | 3 +- lib/ui/painting/image_decoder_impeller.cc | 116 ++++++++++++++-------- lib/ui/painting/image_decoder_impeller.h | 9 +- shell/common/engine.cc | 18 ++-- shell/common/engine.h | 9 +- shell/common/engine_unittests.cc | 44 +++++--- shell/common/shell.cc | 37 ++++--- shell/common/shell.h | 4 +- 9 files changed, 156 insertions(+), 90 deletions(-) diff --git a/lib/ui/painting/image_decoder.cc b/lib/ui/painting/image_decoder.cc index 905ffe4b9d217..b598de3823058 100644 --- a/lib/ui/painting/image_decoder.cc +++ b/lib/ui/painting/image_decoder.cc @@ -16,14 +16,16 @@ std::unique_ptr ImageDecoder::Make( const Settings& settings, const TaskRunners& runners, std::shared_ptr concurrent_task_runner, - fml::WeakPtr io_manager) { + fml::WeakPtr io_manager, + const std::shared_ptr& gpu_disabled_switch) { #if IMPELLER_SUPPORTS_RENDERING if (settings.enable_impeller) { return std::make_unique( runners, // std::move(concurrent_task_runner), // std::move(io_manager), // - settings.enable_wide_gamut); + settings.enable_wide_gamut, // + gpu_disabled_switch); } #endif // IMPELLER_SUPPORTS_RENDERING return std::make_unique( diff --git a/lib/ui/painting/image_decoder.h b/lib/ui/painting/image_decoder.h index f85308e39fcbe..212255953325f 100644 --- a/lib/ui/painting/image_decoder.h +++ b/lib/ui/painting/image_decoder.h @@ -27,7 +27,8 @@ class ImageDecoder { const Settings& settings, const TaskRunners& runners, std::shared_ptr concurrent_task_runner, - fml::WeakPtr io_manager); + fml::WeakPtr io_manager, + const std::shared_ptr& gpu_disabled_switch = nullptr); virtual ~ImageDecoder(); diff --git a/lib/ui/painting/image_decoder_impeller.cc b/lib/ui/painting/image_decoder_impeller.cc index a2fce06f168d8..0eba7e21f9682 100644 --- a/lib/ui/painting/image_decoder_impeller.cc +++ b/lib/ui/painting/image_decoder_impeller.cc @@ -80,9 +80,11 @@ ImageDecoderImpeller::ImageDecoderImpeller( const TaskRunners& runners, std::shared_ptr concurrent_task_runner, const fml::WeakPtr& io_manager, - bool supports_wide_gamut) + bool supports_wide_gamut, + const std::shared_ptr& gpu_disabled_switch) : ImageDecoder(runners, std::move(concurrent_task_runner), io_manager), - supports_wide_gamut_(supports_wide_gamut) { + supports_wide_gamut_(supports_wide_gamut), + gpu_disabled_switch_(gpu_disabled_switch) { std::promise> context_promise; context_ = context_promise.get_future(); runners_.GetIOTaskRunner()->PostTask(fml::MakeCopyable( @@ -322,7 +324,8 @@ std::pair, std::string> ImageDecoderImpeller::UploadTextureToShared( const std::shared_ptr& context, std::shared_ptr bitmap, - bool create_mips) { + bool create_mips, + const std::shared_ptr& gpu_disabled_switch) { TRACE_EVENT0("impeller", __FUNCTION__); if (!context) { return std::make_pair(nullptr, "No Impeller context is available"); @@ -370,32 +373,44 @@ ImageDecoderImpeller::UploadTextureToShared( texture->SetLabel(impeller::SPrintF("ui.Image(%p)", texture.get()).c_str()); if (texture_descriptor.mip_count > 1u && create_mips) { - auto command_buffer = context->CreateCommandBuffer(); - if (!command_buffer) { - std::string decode_error( - "Could not create command buffer for mipmap generation."); - FML_DLOG(ERROR) << decode_error; - return std::make_pair(nullptr, decode_error); - } - command_buffer->SetLabel("Mipmap Command Buffer"); - - auto blit_pass = command_buffer->CreateBlitPass(); - if (!blit_pass) { - std::string decode_error( - "Could not create blit pass for mipmap generation."); - FML_DLOG(ERROR) << decode_error; - return std::make_pair(nullptr, decode_error); - } - blit_pass->SetLabel("Mipmap Blit Pass"); - blit_pass->GenerateMipmap(texture); - - blit_pass->EncodeCommands(context->GetResourceAllocator()); - if (!command_buffer->SubmitCommands()) { - std::string decode_error("Failed to submit blit pass command buffer."); - FML_DLOG(ERROR) << decode_error; - return std::make_pair(nullptr, decode_error); + std::optional decode_error; +#if FML_OS_IOS + gpu_disabled_switch->Execute(fml::SyncSwitch::Handlers().SetIfFalse( + [context, &texture, &decode_error] { +#else + [context, &texture, &decode_error] { +#endif // FML_OS_IOS + auto command_buffer = context->CreateCommandBuffer(); + if (!command_buffer) { + decode_error = + "Could not create command buffer for mipmap generation."; + return; + } + command_buffer->SetLabel("Mipmap Command Buffer"); + + auto blit_pass = command_buffer->CreateBlitPass(); + if (!blit_pass) { + decode_error = "Could not create blit pass for mipmap generation."; + return; + } + blit_pass->SetLabel("Mipmap Blit Pass"); + blit_pass->GenerateMipmap(texture); + + blit_pass->EncodeCommands(context->GetResourceAllocator()); + if (!command_buffer->SubmitCommands()) { + decode_error = "Failed to submit blit pass command buffer."; + return; + } + command_buffer->WaitUntilScheduled(); +#if FML_OS_IOS + })); +#else + }(); +#endif // FML_OS_IOS + if (decode_error.has_value()) { + FML_DLOG(ERROR) << decode_error.value(); + return std::make_pair(nullptr, decode_error.value()); } - command_buffer->WaitUntilScheduled(); } return std::make_pair(impeller::DlImageImpeller::Make(std::move(texture)), @@ -429,8 +444,8 @@ void ImageDecoderImpeller::Decode(fml::RefPtr descriptor, target_size = SkISize::Make(target_width, target_height), // io_runner = runners_.GetIOTaskRunner(), // result, - supports_wide_gamut = supports_wide_gamut_ // - ]() { + supports_wide_gamut = supports_wide_gamut_, // + gpu_disabled_switch = gpu_disabled_switch_]() { if (!context) { result(nullptr, "No Impeller context is available"); return; @@ -446,24 +461,39 @@ void ImageDecoderImpeller::Decode(fml::RefPtr descriptor, result(nullptr, bitmap_result.decode_error); return; } - auto upload_texture_and_invoke_result = [result, context, - bitmap_result]() { - // TODO(jonahwilliams): remove ifdef once blit from buffer to - // texture is implemented on other platforms. + auto upload_texture_and_invoke_result = [result, context, bitmap_result, + gpu_disabled_switch]() { + // TODO(jonahwilliams): remove ifdef once blit from buffer + // to texture is implemented on other platforms. sk_sp image; std::string decode_error; + bool needs_upload = true; + #ifdef FML_OS_IOS - std::tie(image, decode_error) = UploadTextureToPrivate( - context, bitmap_result.device_buffer, bitmap_result.image_info); -#else - std::tie(image, decode_error) = - UploadTextureToShared(context, bitmap_result.sk_bitmap); -#endif + FML_DCHECK(gpu_disabled_switch); + if (gpu_disabled_switch) { + gpu_disabled_switch->Execute(fml::SyncSwitch::Handlers().SetIfFalse( + [&needs_upload, &image, &decode_error, &context, + &bitmap_result] { + needs_upload = false; + FML_LOG(ERROR) << "Yay"; + std::tie(image, decode_error) = UploadTextureToPrivate( + context, bitmap_result.device_buffer, + bitmap_result.image_info); + })); + } +#endif // FML_OS_IOS + if (needs_upload) { + FML_LOG(ERROR) << "Boo"; + std::tie(image, decode_error) = UploadTextureToShared( + context, bitmap_result.sk_bitmap, true, gpu_disabled_switch); + } result(image, decode_error); }; - // TODO(jonahwilliams): https://github.com/flutter/flutter/issues/123058 - // Technically we don't need to post tasks to the io runner, but without - // this forced serialization we can end up overloading the GPU and/or + // TODO(jonahwilliams): + // https://github.com/flutter/flutter/issues/123058 Technically we + // don't need to post tasks to the io runner, but without this + // forced serialization we can end up overloading the GPU and/or // competing with raster workloads. io_runner->PostTask(upload_texture_and_invoke_result); }); diff --git a/lib/ui/painting/image_decoder_impeller.h b/lib/ui/painting/image_decoder_impeller.h index c764df6464d14..81d96a48499e1 100644 --- a/lib/ui/painting/image_decoder_impeller.h +++ b/lib/ui/painting/image_decoder_impeller.h @@ -50,7 +50,8 @@ class ImageDecoderImpeller final : public ImageDecoder { const TaskRunners& runners, std::shared_ptr concurrent_task_runner, const fml::WeakPtr& io_manager, - bool supports_wide_gamut); + bool supports_wide_gamut, + const std::shared_ptr& gpu_disabled_switch); ~ImageDecoderImpeller() override; @@ -83,16 +84,20 @@ class ImageDecoderImpeller final : public ImageDecoder { /// @param bitmap A bitmap containg the image to be uploaded. /// @param create_mips Whether mipmaps should be generated for the given /// image. + /// @param gpu_disabled_switch Whether the GPU is available for mipmap + /// creation. /// @return A DlImage. static std::pair, std::string> UploadTextureToShared( const std::shared_ptr& context, std::shared_ptr bitmap, - bool create_mips = true); + bool create_mips = true, + const std::shared_ptr& gpu_disabled_switch = nullptr); private: using FutureContext = std::shared_future>; FutureContext context_; const bool supports_wide_gamut_; + std::shared_ptr gpu_disabled_switch_; FML_DISALLOW_COPY_AND_ASSIGN(ImageDecoderImpeller); }; diff --git a/shell/common/engine.cc b/shell/common/engine.cc index 9107be5bacb92..2c0b22d1863f8 100644 --- a/shell/common/engine.cc +++ b/shell/common/engine.cc @@ -45,7 +45,8 @@ Engine::Engine( std::unique_ptr animator, fml::WeakPtr io_manager, const std::shared_ptr& font_collection, - std::unique_ptr runtime_controller) + std::unique_ptr runtime_controller, + const std::shared_ptr& gpu_disabled_switch) : delegate_(delegate), settings_(settings), animator_(std::move(animator)), @@ -54,7 +55,8 @@ Engine::Engine( image_decoder_(ImageDecoder::Make(settings_, task_runners, std::move(image_decoder_task_runner), - std::move(io_manager))), + std::move(io_manager), + gpu_disabled_switch)), task_runners_(task_runners), weak_factory_(this) { pointer_data_dispatcher_ = dispatcher_maker(*this); @@ -71,7 +73,8 @@ Engine::Engine(Delegate& delegate, fml::WeakPtr io_manager, fml::RefPtr unref_queue, fml::TaskRunnerAffineWeakPtr snapshot_delegate, - std::shared_ptr volatile_path_tracker) + std::shared_ptr volatile_path_tracker, + const std::shared_ptr& gpu_disabled_switch) : Engine(delegate, dispatcher_maker, vm.GetConcurrentWorkerTaskRunner(), @@ -80,7 +83,8 @@ Engine::Engine(Delegate& delegate, std::move(animator), io_manager, std::make_shared(), - nullptr) { + nullptr, + gpu_disabled_switch) { runtime_controller_ = std::make_unique( *this, // runtime delegate &vm, // VM @@ -112,7 +116,8 @@ std::unique_ptr Engine::Spawn( std::unique_ptr animator, const std::string& initial_route, const fml::WeakPtr& io_manager, - fml::TaskRunnerAffineWeakPtr snapshot_delegate) const { + fml::TaskRunnerAffineWeakPtr snapshot_delegate, + const std::shared_ptr& gpu_disabled_switch) const { auto result = std::make_unique( /*delegate=*/delegate, /*dispatcher_maker=*/dispatcher_maker, @@ -123,7 +128,8 @@ std::unique_ptr Engine::Spawn( /*animator=*/std::move(animator), /*io_manager=*/io_manager, /*font_collection=*/font_collection_, - /*runtime_controller=*/nullptr); + /*runtime_controller=*/nullptr, + /*gpu_disabled_switch=*/gpu_disabled_switch); result->runtime_controller_ = runtime_controller_->Spawn( /*p_client=*/*result, /*advisory_script_uri=*/settings.advisory_script_uri, diff --git a/shell/common/engine.h b/shell/common/engine.h index d4b4340658eae..1890589729b04 100644 --- a/shell/common/engine.h +++ b/shell/common/engine.h @@ -309,7 +309,8 @@ class Engine final : public RuntimeDelegate, PointerDataDispatcher::Delegate { std::unique_ptr animator, fml::WeakPtr io_manager, const std::shared_ptr& font_collection, - std::unique_ptr runtime_controller); + std::unique_ptr runtime_controller, + const std::shared_ptr& gpu_disabled_switch); //---------------------------------------------------------------------------- /// @brief Creates an instance of the engine. This is done by the Shell @@ -364,7 +365,8 @@ class Engine final : public RuntimeDelegate, PointerDataDispatcher::Delegate { fml::WeakPtr io_manager, fml::RefPtr unref_queue, fml::TaskRunnerAffineWeakPtr snapshot_delegate, - std::shared_ptr volatile_path_tracker); + std::shared_ptr volatile_path_tracker, + const std::shared_ptr& gpu_disabled_switch); //---------------------------------------------------------------------------- /// @brief Create a Engine that shares as many resources as @@ -382,7 +384,8 @@ class Engine final : public RuntimeDelegate, PointerDataDispatcher::Delegate { std::unique_ptr animator, const std::string& initial_route, const fml::WeakPtr& io_manager, - fml::TaskRunnerAffineWeakPtr snapshot_delegate) const; + fml::TaskRunnerAffineWeakPtr snapshot_delegate, + const std::shared_ptr& gpu_disabled_switch) const; //---------------------------------------------------------------------------- /// @brief Destroys the engine engine. Called by the shell on the UI task diff --git a/shell/common/engine_unittests.cc b/shell/common/engine_unittests.cc index 6d181b2773595..cf89e3967e0d3 100644 --- a/shell/common/engine_unittests.cc +++ b/shell/common/engine_unittests.cc @@ -162,7 +162,8 @@ TEST_F(EngineTest, Create) { /*animator=*/std::move(animator_), /*io_manager=*/io_manager_, /*font_collection=*/std::make_shared(), - /*runtime_controller=*/std::move(runtime_controller_)); + /*runtime_controller=*/std::move(runtime_controller_), + /*gpu_disabled_switch=*/nullptr); EXPECT_TRUE(engine); }); } @@ -183,7 +184,8 @@ TEST_F(EngineTest, DispatchPlatformMessageUnknown) { /*animator=*/std::move(animator_), /*io_manager=*/io_manager_, /*font_collection=*/std::make_shared(), - /*runtime_controller=*/std::move(mock_runtime_controller)); + /*runtime_controller=*/std::move(mock_runtime_controller), + /*gpu_disabled_switch=*/nullptr); fml::RefPtr response = fml::MakeRefCounted(); @@ -209,7 +211,8 @@ TEST_F(EngineTest, DispatchPlatformMessageInitialRoute) { /*animator=*/std::move(animator_), /*io_manager=*/io_manager_, /*font_collection=*/std::make_shared(), - /*runtime_controller=*/std::move(mock_runtime_controller)); + /*runtime_controller=*/std::move(mock_runtime_controller), + /*gpu_disabled_switch=*/nullptr); fml::RefPtr response = fml::MakeRefCounted(); @@ -242,7 +245,8 @@ TEST_F(EngineTest, DispatchPlatformMessageInitialRouteIgnored) { /*animator=*/std::move(animator_), /*io_manager=*/io_manager_, /*font_collection=*/std::make_shared(), - /*runtime_controller=*/std::move(mock_runtime_controller)); + /*runtime_controller=*/std::move(mock_runtime_controller), + /*gpu_disabled_switch=*/nullptr); fml::RefPtr response = fml::MakeRefCounted(); @@ -274,10 +278,12 @@ TEST_F(EngineTest, SpawnSharesFontLibrary) { /*animator=*/std::move(animator_), /*io_manager=*/io_manager_, /*font_collection=*/std::make_shared(), - /*runtime_controller=*/std::move(mock_runtime_controller)); + /*runtime_controller=*/std::move(mock_runtime_controller), + /*gpu_disabled_switch=*/nullptr); - auto spawn = engine->Spawn(delegate_, dispatcher_maker_, settings_, nullptr, - std::string(), io_manager_, snapshot_delegate_); + auto spawn = + engine->Spawn(delegate_, dispatcher_maker_, settings_, nullptr, + std::string(), io_manager_, snapshot_delegate_, nullptr); EXPECT_TRUE(spawn != nullptr); EXPECT_EQ(&engine->GetFontCollection(), &spawn->GetFontCollection()); }); @@ -300,10 +306,12 @@ TEST_F(EngineTest, SpawnWithCustomInitialRoute) { /*animator=*/std::move(animator_), /*io_manager=*/io_manager_, /*font_collection=*/std::make_shared(), - /*runtime_controller=*/std::move(mock_runtime_controller)); + /*runtime_controller=*/std::move(mock_runtime_controller), + /*gpu_disabled_switch=*/nullptr); - auto spawn = engine->Spawn(delegate_, dispatcher_maker_, settings_, nullptr, - "/foo", io_manager_, snapshot_delegate_); + auto spawn = + engine->Spawn(delegate_, dispatcher_maker_, settings_, nullptr, "/foo", + io_manager_, snapshot_delegate_, nullptr); EXPECT_TRUE(spawn != nullptr); ASSERT_EQ("/foo", spawn->InitialRoute()); }); @@ -332,14 +340,16 @@ TEST_F(EngineTest, SpawnResetsViewportMetrics) { /*animator=*/std::move(animator_), /*io_manager=*/io_manager_, /*font_collection=*/std::make_shared(), - /*runtime_controller=*/std::move(mock_runtime_controller)); + /*runtime_controller=*/std::move(mock_runtime_controller), + /*gpu_disabled_switch=*/nullptr); auto& old_platform_data = engine->GetRuntimeController()->GetPlatformData(); EXPECT_EQ(old_platform_data.viewport_metrics.physical_width, kViewWidth); EXPECT_EQ(old_platform_data.viewport_metrics.physical_height, kViewHeight); - auto spawn = engine->Spawn(delegate_, dispatcher_maker_, settings_, nullptr, - std::string(), io_manager_, snapshot_delegate_); + auto spawn = + engine->Spawn(delegate_, dispatcher_maker_, settings_, nullptr, + std::string(), io_manager_, snapshot_delegate_, nullptr); EXPECT_TRUE(spawn != nullptr); auto& new_viewport_metrics = spawn->GetRuntimeController()->GetPlatformData().viewport_metrics; @@ -365,14 +375,15 @@ TEST_F(EngineTest, SpawnWithCustomSettings) { /*animator=*/std::move(animator_), /*io_manager=*/io_manager_, /*font_collection=*/std::make_shared(), - /*runtime_controller=*/std::move(mock_runtime_controller)); + /*runtime_controller=*/std::move(mock_runtime_controller), + /*gpu_disabled_switch=*/nullptr); Settings custom_settings = settings_; custom_settings.persistent_isolate_data = std::make_shared("foo"); auto spawn = engine->Spawn(delegate_, dispatcher_maker_, custom_settings, nullptr, - std::string(), io_manager_, snapshot_delegate_); + std::string(), io_manager_, snapshot_delegate_, nullptr); EXPECT_TRUE(spawn != nullptr); auto new_persistent_isolate_data = const_cast(spawn->GetRuntimeController()) @@ -405,7 +416,8 @@ TEST_F(EngineTest, PassesLoadDartDeferredLibraryErrorToRuntime) { /*animator=*/std::move(animator_), /*io_manager=*/io_manager_, /*font_collection=*/std::make_shared(), - /*runtime_controller=*/std::move(mock_runtime_controller)); + /*runtime_controller=*/std::move(mock_runtime_controller), + /*gpu_disabled_switch=*/nullptr); engine->LoadDartDeferredLibraryError(error_id, error_message, true); }); diff --git a/shell/common/shell.cc b/shell/common/shell.cc index 247736517da12..c5785908215eb 100644 --- a/shell/common/shell.cc +++ b/shell/common/shell.cc @@ -54,19 +54,21 @@ std::unique_ptr CreateEngine( const fml::WeakPtr& io_manager, const fml::RefPtr& unref_queue, const fml::TaskRunnerAffineWeakPtr& snapshot_delegate, - const std::shared_ptr& volatile_path_tracker) { - return std::make_unique(delegate, // - dispatcher_maker, // - vm, // - isolate_snapshot, // - task_runners, // - platform_data, // - settings, // - std::move(animator), // - io_manager, // - unref_queue, // - snapshot_delegate, // - volatile_path_tracker); + const std::shared_ptr& volatile_path_tracker, + const std::shared_ptr& gpu_disabled_switch) { + return std::make_unique(delegate, // + dispatcher_maker, // + vm, // + isolate_snapshot, // + task_runners, // + platform_data, // + settings, // + std::move(animator), // + io_manager, // + unref_queue, // + snapshot_delegate, // + volatile_path_tracker, // + gpu_disabled_switch); } // Though there can be multiple shells, some settings apply to all components in @@ -301,7 +303,8 @@ std::unique_ptr Shell::CreateShellOnPlatformThread( weak_io_manager_future.get(), // unref_queue_future.get(), // snapshot_delegate_future.get(), // - shell->volatile_path_tracker_)); + shell->volatile_path_tracker_, + shell->is_gpu_disabled_sync_switch_)); })); if (!shell->Setup(std::move(platform_view), // @@ -538,7 +541,8 @@ std::unique_ptr Shell::Spawn( const fml::WeakPtr& io_manager, const fml::RefPtr& unref_queue, fml::TaskRunnerAffineWeakPtr snapshot_delegate, - const std::shared_ptr& volatile_path_tracker) { + const std::shared_ptr& volatile_path_tracker, + const std::shared_ptr& is_gpu_disabled_sync_switch) { return engine->Spawn( /*delegate=*/delegate, /*dispatcher_maker=*/dispatcher_maker, @@ -546,7 +550,8 @@ std::unique_ptr Shell::Spawn( /*animator=*/std::move(animator), /*initial_route=*/initial_route, /*io_manager=*/io_manager, - /*snapshot_delegate=*/std::move(snapshot_delegate)); + /*snapshot_delegate=*/std::move(snapshot_delegate), + /*gpu_disabled_switch=*/is_gpu_disabled_sync_switch); }, is_gpu_disabled); result->RunEngine(std::move(run_configuration)); diff --git a/shell/common/shell.h b/shell/common/shell.h index b5bef42ed75c1..6f6360625f412 100644 --- a/shell/common/shell.h +++ b/shell/common/shell.h @@ -126,7 +126,8 @@ class Shell final : public PlatformView::Delegate, fml::WeakPtr io_manager, fml::RefPtr unref_queue, fml::TaskRunnerAffineWeakPtr snapshot_delegate, - std::shared_ptr volatile_path_tracker)> + std::shared_ptr volatile_path_tracker, + const std::shared_ptr& gpu_disabled_switch)> EngineCreateCallback; //---------------------------------------------------------------------------- @@ -356,6 +357,7 @@ class Shell final : public PlatformView::Delegate, //---------------------------------------------------------------------------- /// @brief Accessor for the disable GPU SyncSwitch. + // |Rasterizer::Delegate| std::shared_ptr GetIsGpuDisabledSyncSwitch() const override; From 86e04119a97a69308713f6368a45b6f0d3b38590 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Fri, 26 May 2023 09:32:20 -0700 Subject: [PATCH 2/6] Aaron review --- lib/ui/painting/image_decoder.h | 2 +- lib/ui/painting/image_decoder_impeller.cc | 80 ++++++++++++---------- lib/ui/painting/image_decoder_impeller.h | 10 ++- lib/ui/painting/image_decoder_unittests.cc | 27 ++++---- lib/ui/painting/multi_frame_codec.cc | 1 + 5 files changed, 67 insertions(+), 53 deletions(-) diff --git a/lib/ui/painting/image_decoder.h b/lib/ui/painting/image_decoder.h index 212255953325f..ebb104efd44f7 100644 --- a/lib/ui/painting/image_decoder.h +++ b/lib/ui/painting/image_decoder.h @@ -28,7 +28,7 @@ class ImageDecoder { const TaskRunners& runners, std::shared_ptr concurrent_task_runner, fml::WeakPtr io_manager, - const std::shared_ptr& gpu_disabled_switch = nullptr); + const std::shared_ptr& gpu_disabled_switch); virtual ~ImageDecoder(); diff --git a/lib/ui/painting/image_decoder_impeller.cc b/lib/ui/painting/image_decoder_impeller.cc index 0eba7e21f9682..82dea476082f8 100644 --- a/lib/ui/painting/image_decoder_impeller.cc +++ b/lib/ui/painting/image_decoder_impeller.cc @@ -248,18 +248,11 @@ DecompressResult ImageDecoderImpeller::DecompressTexture( .image_info = scaled_bitmap->info()}; } -std::pair, std::string> -ImageDecoderImpeller::UploadTextureToPrivate( +/// Only call this method if the GPU is available. +static std::pair, std::string> UnsafeUploadTextureToPrivate( const std::shared_ptr& context, const std::shared_ptr& buffer, const SkImageInfo& image_info) { - TRACE_EVENT0("impeller", __FUNCTION__); - if (!context) { - return std::make_pair(nullptr, "No Impeller context is available"); - } - if (!buffer) { - return std::make_pair(nullptr, "No Impeller device buffer is available"); - } const auto pixel_format = impeller::skia_conversions::ToPixelFormat(image_info.colorType()); if (!pixel_format) { @@ -321,15 +314,44 @@ ImageDecoderImpeller::UploadTextureToPrivate( } std::pair, std::string> -ImageDecoderImpeller::UploadTextureToShared( +ImageDecoderImpeller::UploadTextureToPrivate( const std::shared_ptr& context, + const std::shared_ptr& buffer, + const SkImageInfo& image_info, std::shared_ptr bitmap, - bool create_mips, const std::shared_ptr& gpu_disabled_switch) { TRACE_EVENT0("impeller", __FUNCTION__); if (!context) { return std::make_pair(nullptr, "No Impeller context is available"); } + if (!buffer) { + return std::make_pair(nullptr, "No Impeller device buffer is available"); + } + + std::pair, std::string> result; + gpu_disabled_switch->Execute( + fml::SyncSwitch::Handlers() + .SetIfFalse([&result, context, buffer, image_info] { + result = UnsafeUploadTextureToPrivate(context, buffer, image_info); + }) + .SetIfTrue([&result, context, bitmap, gpu_disabled_switch] { + // create_mips is false because we already know the GPU is disabled. + result = UploadTextureToShared(context, bitmap, gpu_disabled_switch, + false); + })); + return result; +} + +std::pair, std::string> +ImageDecoderImpeller::UploadTextureToShared( + const std::shared_ptr& context, + std::shared_ptr bitmap, + const std::shared_ptr& gpu_disabled_switch, + bool create_mips) { + TRACE_EVENT0("impeller", __FUNCTION__); + if (!context) { + return std::make_pair(nullptr, "No Impeller context is available"); + } if (!bitmap) { return std::make_pair(nullptr, "No texture bitmap is available"); } @@ -374,12 +396,12 @@ ImageDecoderImpeller::UploadTextureToShared( if (texture_descriptor.mip_count > 1u && create_mips) { std::optional decode_error; -#if FML_OS_IOS + + // The only platform that needs mipmapping unconditionally is GL. + // GL based platforms never disable GPU access. + // This is only really needed for iOS. gpu_disabled_switch->Execute(fml::SyncSwitch::Handlers().SetIfFalse( [context, &texture, &decode_error] { -#else - [context, &texture, &decode_error] { -#endif // FML_OS_IOS auto command_buffer = context->CreateCommandBuffer(); if (!command_buffer) { decode_error = @@ -402,11 +424,7 @@ ImageDecoderImpeller::UploadTextureToShared( return; } command_buffer->WaitUntilScheduled(); -#if FML_OS_IOS })); -#else - }(); -#endif // FML_OS_IOS if (decode_error.has_value()) { FML_DLOG(ERROR) << decode_error.value(); return std::make_pair(nullptr, decode_error.value()); @@ -467,27 +485,15 @@ void ImageDecoderImpeller::Decode(fml::RefPtr descriptor, // to texture is implemented on other platforms. sk_sp image; std::string decode_error; - bool needs_upload = true; #ifdef FML_OS_IOS - FML_DCHECK(gpu_disabled_switch); - if (gpu_disabled_switch) { - gpu_disabled_switch->Execute(fml::SyncSwitch::Handlers().SetIfFalse( - [&needs_upload, &image, &decode_error, &context, - &bitmap_result] { - needs_upload = false; - FML_LOG(ERROR) << "Yay"; - std::tie(image, decode_error) = UploadTextureToPrivate( - context, bitmap_result.device_buffer, - bitmap_result.image_info); - })); - } + std::tie(image, decode_error) = UploadTextureToPrivate( + context, bitmap_result.device_buffer, bitmap_result.image_info, + bitmap_result.sk_bitmap, gpu_disabled_switch); +#else + std::tie(image, decode_error) = UploadTextureToShared( + context, bitmap_result.sk_bitmap, gpu_disabled_switch, true); #endif // FML_OS_IOS - if (needs_upload) { - FML_LOG(ERROR) << "Boo"; - std::tie(image, decode_error) = UploadTextureToShared( - context, bitmap_result.sk_bitmap, true, gpu_disabled_switch); - } result(image, decode_error); }; // TODO(jonahwilliams): diff --git a/lib/ui/painting/image_decoder_impeller.h b/lib/ui/painting/image_decoder_impeller.h index 81d96a48499e1..a9c1af42de9d0 100644 --- a/lib/ui/painting/image_decoder_impeller.h +++ b/lib/ui/painting/image_decoder_impeller.h @@ -73,11 +73,15 @@ class ImageDecoderImpeller final : public ImageDecoder { /// @param context The Impeller graphics context. /// @param buffer A host buffer containing the image to be uploaded. /// @param image_info Format information about the particular image. + /// @param bitmap A bitmap containg the image to be uploaded. + /// @param gpu_disabled_switch Whether the GPU is available command encoding. /// @return A DlImage. static std::pair, std::string> UploadTextureToPrivate( const std::shared_ptr& context, const std::shared_ptr& buffer, - const SkImageInfo& image_info); + const SkImageInfo& image_info, + std::shared_ptr bitmap, + const std::shared_ptr& gpu_disabled_switch); /// @brief Create a host visible texture from the provided bitmap. /// @param context The Impeller graphics context. @@ -90,8 +94,8 @@ class ImageDecoderImpeller final : public ImageDecoder { static std::pair, std::string> UploadTextureToShared( const std::shared_ptr& context, std::shared_ptr bitmap, - bool create_mips = true, - const std::shared_ptr& gpu_disabled_switch = nullptr); + const std::shared_ptr& gpu_disabled_switch, + bool create_mips = true); private: using FutureContext = std::shared_future>; diff --git a/lib/ui/painting/image_decoder_unittests.cc b/lib/ui/painting/image_decoder_unittests.cc index 486f1158f89e7..ad5c5b651a33b 100644 --- a/lib/ui/painting/image_decoder_unittests.cc +++ b/lib/ui/painting/image_decoder_unittests.cc @@ -280,7 +280,8 @@ TEST_F(ImageDecoderFixtureTest, CanCreateImageDecoder) { TestIOManager manager(runners.GetIOTaskRunner()); Settings settings; auto decoder = ImageDecoder::Make(settings, runners, loop->GetTaskRunner(), - manager.GetWeakIOManager()); + manager.GetWeakIOManager(), + std::make_shared()); ASSERT_NE(decoder, nullptr); }); } @@ -331,7 +332,8 @@ TEST_F(ImageDecoderFixtureTest, InvalidImageResultsError) { TestIOManager manager(runners.GetIOTaskRunner()); Settings settings; auto decoder = ImageDecoder::Make(settings, runners, loop->GetTaskRunner(), - manager.GetWeakIOManager()); + manager.GetWeakIOManager(), + std::make_shared()); auto data = OpenFixtureAsSkData("ThisDoesNotExist.jpg"); ASSERT_FALSE(data); @@ -370,9 +372,9 @@ TEST_F(ImageDecoderFixtureTest, ValidImageResultsInSuccess) { }; auto decode_image = [&]() { Settings settings; - std::unique_ptr image_decoder = - ImageDecoder::Make(settings, runners, loop->GetTaskRunner(), - io_manager->GetWeakIOManager()); + std::unique_ptr image_decoder = ImageDecoder::Make( + settings, runners, loop->GetTaskRunner(), + io_manager->GetWeakIOManager(), std::make_shared()); auto data = OpenFixtureAsSkData("DashInNooglerHat.jpg"); @@ -642,9 +644,9 @@ TEST_F(ImageDecoderFixtureTest, ExifDataIsRespectedOnDecode) { SkISize decoded_size = SkISize::MakeEmpty(); auto decode_image = [&]() { Settings settings; - std::unique_ptr image_decoder = - ImageDecoder::Make(settings, runners, loop->GetTaskRunner(), - io_manager->GetWeakIOManager()); + std::unique_ptr image_decoder = ImageDecoder::Make( + settings, runners, loop->GetTaskRunner(), + io_manager->GetWeakIOManager(), std::make_shared()); auto data = OpenFixtureAsSkData("Horizontal.jpg"); @@ -703,9 +705,9 @@ TEST_F(ImageDecoderFixtureTest, CanDecodeWithoutAGPUContext) { auto decode_image = [&]() { Settings settings; - std::unique_ptr image_decoder = - ImageDecoder::Make(settings, runners, loop->GetTaskRunner(), - io_manager->GetWeakIOManager()); + std::unique_ptr image_decoder = ImageDecoder::Make( + settings, runners, loop->GetTaskRunner(), + io_manager->GetWeakIOManager(), std::make_shared()); auto data = OpenFixtureAsSkData("DashInNooglerHat.jpg"); @@ -771,7 +773,8 @@ TEST_F(ImageDecoderFixtureTest, CanDecodeWithResizes) { PostTaskSync(runners.GetUITaskRunner(), [&]() { Settings settings; image_decoder = ImageDecoder::Make(settings, runners, loop->GetTaskRunner(), - io_manager->GetWeakIOManager()); + io_manager->GetWeakIOManager(), + std::make_shared()); }); // Setup a generic decoding utility that gives us the final decoded size. diff --git a/lib/ui/painting/multi_frame_codec.cc b/lib/ui/painting/multi_frame_codec.cc index 92a1687072c34..784355b84dd33 100644 --- a/lib/ui/painting/multi_frame_codec.cc +++ b/lib/ui/painting/multi_frame_codec.cc @@ -159,6 +159,7 @@ MultiFrameCodec::State::GetNextFrameImage( // without mipmap creation there is no command buffer encoding done. return ImageDecoderImpeller::UploadTextureToShared( impeller_context, std::make_shared(bitmap), + std::make_shared(), /*create_mips=*/false); } #endif // IMPELLER_SUPPORTS_RENDERING From 1cbd4cf14cf48a873286a7d14335e191ec546a87 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Fri, 26 May 2023 10:01:40 -0700 Subject: [PATCH 3/6] test --- lib/ui/painting/image_decoder_unittests.cc | 31 ++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/lib/ui/painting/image_decoder_unittests.cc b/lib/ui/painting/image_decoder_unittests.cc index ad5c5b651a33b..485fc8d3c8982 100644 --- a/lib/ui/painting/image_decoder_unittests.cc +++ b/lib/ui/painting/image_decoder_unittests.cc @@ -138,9 +138,12 @@ class TestImpellerContext : public impeller::Context { } std::shared_ptr CreateCommandBuffer() const override { + command_buffer_count_ += 1; return nullptr; } + mutable size_t command_buffer_count_ = 0; + private: std::shared_ptr capabilities_; }; @@ -428,6 +431,34 @@ float HalfToFloat(uint16_t half) { } } // namespace +TEST_F(ImageDecoderFixtureTest, ImpellerUploadToSharedNoGpu) { +#if !IMPELLER_SUPPORTS_RENDERING + GTEST_SKIP() << "Impeller only test."; +#endif // IMPELLER_SUPPORTS_RENDERING + + auto no_gpu_access_context = + std::make_shared(); + auto gpu_disabled_switch = std::make_shared(true); + + auto info = SkImageInfo::Make(10, 10, SkColorType::kRGBA_8888_SkColorType, + SkAlphaType::kPremul_SkAlphaType); + auto bitmap = std::make_shared(); + bitmap->allocPixels(info, 10 * 4); + impeller::DeviceBufferDescriptor desc; + desc.size = bitmap->computeByteSize(); + auto buffer = std::make_shared(desc); + + auto result = ImageDecoderImpeller::UploadTextureToPrivate( + no_gpu_access_context, buffer, info, bitmap, gpu_disabled_switch); + ASSERT_EQ(no_gpu_access_context->command_buffer_count_, 0ul); + ASSERT_EQ(result.second, ""); + + result = ImageDecoderImpeller::UploadTextureToShared( + no_gpu_access_context, bitmap, gpu_disabled_switch, true); + ASSERT_EQ(no_gpu_access_context->command_buffer_count_, 0ul); + ASSERT_EQ(result.second, ""); +} + TEST_F(ImageDecoderFixtureTest, ImpellerNullColorspace) { auto info = SkImageInfo::Make(10, 10, SkColorType::kRGBA_8888_SkColorType, SkAlphaType::kPremul_SkAlphaType); From 4ca1ed1c5cd78e029ad9af91487c845904e7f435 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Fri, 26 May 2023 10:57:24 -0700 Subject: [PATCH 4/6] Update lib/ui/painting/image_decoder_impeller.cc Co-authored-by: gaaclarke <30870216+gaaclarke@users.noreply.github.com> --- lib/ui/painting/image_decoder_impeller.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/ui/painting/image_decoder_impeller.cc b/lib/ui/painting/image_decoder_impeller.cc index 82dea476082f8..07446b26a29e8 100644 --- a/lib/ui/painting/image_decoder_impeller.cc +++ b/lib/ui/painting/image_decoder_impeller.cc @@ -337,7 +337,7 @@ ImageDecoderImpeller::UploadTextureToPrivate( .SetIfTrue([&result, context, bitmap, gpu_disabled_switch] { // create_mips is false because we already know the GPU is disabled. result = UploadTextureToShared(context, bitmap, gpu_disabled_switch, - false); + /*create_mips=*/false); })); return result; } From 020469e7bcc26e6b4090842fdf499380eeab89d7 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Fri, 26 May 2023 10:58:27 -0700 Subject: [PATCH 5/6] Update lib/ui/painting/image_decoder_impeller.cc Co-authored-by: gaaclarke <30870216+gaaclarke@users.noreply.github.com> --- lib/ui/painting/image_decoder_impeller.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/ui/painting/image_decoder_impeller.cc b/lib/ui/painting/image_decoder_impeller.cc index 07446b26a29e8..8523afc81c6c3 100644 --- a/lib/ui/painting/image_decoder_impeller.cc +++ b/lib/ui/painting/image_decoder_impeller.cc @@ -492,7 +492,7 @@ void ImageDecoderImpeller::Decode(fml::RefPtr descriptor, bitmap_result.sk_bitmap, gpu_disabled_switch); #else std::tie(image, decode_error) = UploadTextureToShared( - context, bitmap_result.sk_bitmap, gpu_disabled_switch, true); + context, bitmap_result.sk_bitmap, gpu_disabled_switch, /*create_mips=*/true); #endif // FML_OS_IOS result(image, decode_error); }; From 3f456def82e01d5b300ee79decd08e6561e25e12 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Fri, 26 May 2023 11:02:20 -0700 Subject: [PATCH 6/6] bdero/gaaclarke review --- .../renderer/backend/metal/render_pass_mtl.mm | 3 +++ lib/ui/painting/image_decoder_impeller.cc | 5 +++-- shell/common/engine_unittests.cc | 18 +++++++++--------- 3 files changed, 15 insertions(+), 11 deletions(-) diff --git a/impeller/renderer/backend/metal/render_pass_mtl.mm b/impeller/renderer/backend/metal/render_pass_mtl.mm index 317e46b98ca64..dc5e4fa621639 100644 --- a/impeller/renderer/backend/metal/render_pass_mtl.mm +++ b/impeller/renderer/backend/metal/render_pass_mtl.mm @@ -385,10 +385,13 @@ static bool Bind(PassBindingsCache& pass, } if (texture.NeedsMipmapGeneration()) { + // TODO(127697): generate mips when the GPU is available on iOS. +#if !FML_OS_IOS VALIDATION_LOG << "Texture at binding index " << bind_index << " has a mip count > 1, but the mipmap has not been generated."; return false; +#endif // !FML_OS_IOS } return pass.SetTexture(stage, bind_index, diff --git a/lib/ui/painting/image_decoder_impeller.cc b/lib/ui/painting/image_decoder_impeller.cc index 8523afc81c6c3..50e3d19a0b851 100644 --- a/lib/ui/painting/image_decoder_impeller.cc +++ b/lib/ui/painting/image_decoder_impeller.cc @@ -491,8 +491,9 @@ void ImageDecoderImpeller::Decode(fml::RefPtr descriptor, context, bitmap_result.device_buffer, bitmap_result.image_info, bitmap_result.sk_bitmap, gpu_disabled_switch); #else - std::tie(image, decode_error) = UploadTextureToShared( - context, bitmap_result.sk_bitmap, gpu_disabled_switch, /*create_mips=*/true); + std::tie(image, decode_error) = + UploadTextureToShared(context, bitmap_result.sk_bitmap, + gpu_disabled_switch, /*create_mips=*/true); #endif // FML_OS_IOS result(image, decode_error); }; diff --git a/shell/common/engine_unittests.cc b/shell/common/engine_unittests.cc index cf89e3967e0d3..5011f266557c8 100644 --- a/shell/common/engine_unittests.cc +++ b/shell/common/engine_unittests.cc @@ -163,7 +163,7 @@ TEST_F(EngineTest, Create) { /*io_manager=*/io_manager_, /*font_collection=*/std::make_shared(), /*runtime_controller=*/std::move(runtime_controller_), - /*gpu_disabled_switch=*/nullptr); + /*gpu_disabled_switch=*/std::make_shared()); EXPECT_TRUE(engine); }); } @@ -185,7 +185,7 @@ TEST_F(EngineTest, DispatchPlatformMessageUnknown) { /*io_manager=*/io_manager_, /*font_collection=*/std::make_shared(), /*runtime_controller=*/std::move(mock_runtime_controller), - /*gpu_disabled_switch=*/nullptr); + /*gpu_disabled_switch=*/std::make_shared()); fml::RefPtr response = fml::MakeRefCounted(); @@ -212,7 +212,7 @@ TEST_F(EngineTest, DispatchPlatformMessageInitialRoute) { /*io_manager=*/io_manager_, /*font_collection=*/std::make_shared(), /*runtime_controller=*/std::move(mock_runtime_controller), - /*gpu_disabled_switch=*/nullptr); + /*gpu_disabled_switch=*/std::make_shared()); fml::RefPtr response = fml::MakeRefCounted(); @@ -246,7 +246,7 @@ TEST_F(EngineTest, DispatchPlatformMessageInitialRouteIgnored) { /*io_manager=*/io_manager_, /*font_collection=*/std::make_shared(), /*runtime_controller=*/std::move(mock_runtime_controller), - /*gpu_disabled_switch=*/nullptr); + /*gpu_disabled_switch=*/std::make_shared()); fml::RefPtr response = fml::MakeRefCounted(); @@ -279,7 +279,7 @@ TEST_F(EngineTest, SpawnSharesFontLibrary) { /*io_manager=*/io_manager_, /*font_collection=*/std::make_shared(), /*runtime_controller=*/std::move(mock_runtime_controller), - /*gpu_disabled_switch=*/nullptr); + /*gpu_disabled_switch=*/std::make_shared()); auto spawn = engine->Spawn(delegate_, dispatcher_maker_, settings_, nullptr, @@ -307,7 +307,7 @@ TEST_F(EngineTest, SpawnWithCustomInitialRoute) { /*io_manager=*/io_manager_, /*font_collection=*/std::make_shared(), /*runtime_controller=*/std::move(mock_runtime_controller), - /*gpu_disabled_switch=*/nullptr); + /*gpu_disabled_switch=*/std::make_shared()); auto spawn = engine->Spawn(delegate_, dispatcher_maker_, settings_, nullptr, "/foo", @@ -341,7 +341,7 @@ TEST_F(EngineTest, SpawnResetsViewportMetrics) { /*io_manager=*/io_manager_, /*font_collection=*/std::make_shared(), /*runtime_controller=*/std::move(mock_runtime_controller), - /*gpu_disabled_switch=*/nullptr); + /*gpu_disabled_switch=*/std::make_shared()); auto& old_platform_data = engine->GetRuntimeController()->GetPlatformData(); EXPECT_EQ(old_platform_data.viewport_metrics.physical_width, kViewWidth); @@ -376,7 +376,7 @@ TEST_F(EngineTest, SpawnWithCustomSettings) { /*io_manager=*/io_manager_, /*font_collection=*/std::make_shared(), /*runtime_controller=*/std::move(mock_runtime_controller), - /*gpu_disabled_switch=*/nullptr); + /*gpu_disabled_switch=*/std::make_shared()); Settings custom_settings = settings_; custom_settings.persistent_isolate_data = @@ -417,7 +417,7 @@ TEST_F(EngineTest, PassesLoadDartDeferredLibraryErrorToRuntime) { /*io_manager=*/io_manager_, /*font_collection=*/std::make_shared(), /*runtime_controller=*/std::move(mock_runtime_controller), - /*gpu_disabled_switch=*/nullptr); + /*gpu_disabled_switch=*/std::make_shared()); engine->LoadDartDeferredLibraryError(error_id, error_message, true); });