From 2608f64c156e89186bb215f24b227570f3c4676b Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Fri, 10 May 2024 10:03:32 -0700 Subject: [PATCH 1/6] [impeller] made ability to save out display lists --- shell/common/BUILD.gn | 2 ++ shell/common/display_list_debugger.cc | 40 +++++++++++++++++++++++++ shell/common/display_list_debugger.h | 22 ++++++++++++++ shell/common/shell.cc | 6 ++++ shell/gpu/gpu_surface_metal_impeller.mm | 4 +++ 5 files changed, 74 insertions(+) create mode 100644 shell/common/display_list_debugger.cc create mode 100644 shell/common/display_list_debugger.h diff --git a/shell/common/BUILD.gn b/shell/common/BUILD.gn index 8e2d9f2174b3a..356c5c79bb325 100644 --- a/shell/common/BUILD.gn +++ b/shell/common/BUILD.gn @@ -86,6 +86,8 @@ source_set("common") { "animator.h", "context_options.cc", "context_options.h", + "display_list_debugger.cc", + "display_list_debugger.h", "display_manager.cc", "display_manager.h", "dl_op_spy.cc", diff --git a/shell/common/display_list_debugger.cc b/shell/common/display_list_debugger.cc new file mode 100644 index 0000000000000..f40c68851e379 --- /dev/null +++ b/shell/common/display_list_debugger.cc @@ -0,0 +1,40 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "flutter/shell/common/display_list_debugger.h" + +#include + +#include "flutter/fml/logging.h" + +namespace { +std::atomic saveDisplayListPath; +} + +namespace flutter { +void DisplayListDebugger::HandleMessage( + std::unique_ptr message) { + const fml::MallocMapping& data = message->data(); + char* old_path = saveDisplayListPath.exchange( + strdup(reinterpret_cast(data.GetMapping()))); + if (old_path) { + free(old_path); + } +} + +void DisplayListDebugger::SaveDisplayList( + const sk_sp& display_list) { + if (char* path = saveDisplayListPath.exchange(nullptr)) { + fml::UniqueFD dir = fml::OpenDirectory(path, /*create_if_necessary=*/false, + fml::FilePermission::kWrite); + // DisplayList doesn't have an accessor for the byte size of the storage, + // adding one may confuse the api so we derive it with bytes(). + size_t size = display_list->bytes(/*nested=*/false) - sizeof(DisplayList); + fml::NonOwnedMapping mapping(display_list->GetStorage().get(), size); + bool success = fml::WriteAtomically(dir, "display_list.dat", mapping); + FML_LOG(ERROR) << "store display_list (" << success << "):" << path; + free(path); + } +} +} // namespace flutter diff --git a/shell/common/display_list_debugger.h b/shell/common/display_list_debugger.h new file mode 100644 index 0000000000000..4adc12fe4400a --- /dev/null +++ b/shell/common/display_list_debugger.h @@ -0,0 +1,22 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef FLUTTER_SHELL_COMMON_DISPLAY_LIST_DEBUGGER_H_ +#define FLUTTER_SHELL_COMMON_DISPLAY_LIST_DEBUGGER_H_ + +#include "flutter/display_list/display_list.h" +#include "flutter/lib/ui/window/platform_message.h" + +namespace flutter { + +class DisplayListDebugger { + public: + static constexpr char kChannelName[] = "flutter/display_list"; + static void HandleMessage(std::unique_ptr message); + static void SaveDisplayList(const sk_sp& display_list); +}; + +} // namespace flutter + +#endif // FLUTTER_SHELL_COMMON_DISPLAY_LIST_DEBUGGER_H_ diff --git a/shell/common/shell.cc b/shell/common/shell.cc index 9cb5735c6a14c..7f8ac70bd12e1 100644 --- a/shell/common/shell.cc +++ b/shell/common/shell.cc @@ -24,6 +24,7 @@ #include "flutter/fml/trace_event.h" #include "flutter/runtime/dart_vm.h" #include "flutter/shell/common/base64.h" +#include "flutter/shell/common/display_list_debugger.h" #include "flutter/shell/common/engine.h" #include "flutter/shell/common/skia_event_tracer_impl.h" #include "flutter/shell/common/switches.h" @@ -1328,6 +1329,11 @@ void Shell::OnEngineHandlePlatformMessage( return; } + if (message->channel() == DisplayListDebugger::kChannelName) { + DisplayListDebugger::HandleMessage(std::move(message)); + return; + } + if (platform_message_handler_) { if (route_messages_through_platform_thread_ && !platform_message_handler_ diff --git a/shell/gpu/gpu_surface_metal_impeller.mm b/shell/gpu/gpu_surface_metal_impeller.mm index 01093fe37612f..508e0ab2e0b16 100644 --- a/shell/gpu/gpu_surface_metal_impeller.mm +++ b/shell/gpu/gpu_surface_metal_impeller.mm @@ -10,8 +10,10 @@ #include "flutter/common/settings.h" #include "flutter/fml/make_copyable.h" #include "flutter/fml/mapping.h" +#include "flutter/fml/paths.h" #include "flutter/fml/trace_event.h" #include "impeller/display_list/dl_dispatcher.h" +#include "flutter/shell/common/display_list_debugger.h" #include "impeller/renderer/backend/metal/surface_mtl.h" #include "impeller/typographer/backends/skia/typographer_context_skia.h" @@ -127,6 +129,8 @@ return false; } + DisplayListDebugger::SaveDisplayList(display_list); + if (!disable_partial_repaint && damage) { uintptr_t texture = reinterpret_cast(last_texture); From fdbc9de473afa2d157e19037bade54d8e5f76503 Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Fri, 10 May 2024 11:02:50 -0700 Subject: [PATCH 2/6] removed raw pointers --- shell/common/display_list_debugger.cc | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/shell/common/display_list_debugger.cc b/shell/common/display_list_debugger.cc index f40c68851e379..11ebc8e5c7873 100644 --- a/shell/common/display_list_debugger.cc +++ b/shell/common/display_list_debugger.cc @@ -10,31 +10,31 @@ namespace { std::atomic saveDisplayListPath; -} +template +using FreePtr = std::unique_ptr; +} // namespace namespace flutter { void DisplayListDebugger::HandleMessage( std::unique_ptr message) { const fml::MallocMapping& data = message->data(); - char* old_path = saveDisplayListPath.exchange( - strdup(reinterpret_cast(data.GetMapping()))); - if (old_path) { - free(old_path); - } + FreePtr(saveDisplayListPath.exchange( + strdup(reinterpret_cast(data.GetMapping()))), + &free); } void DisplayListDebugger::SaveDisplayList( const sk_sp& display_list) { - if (char* path = saveDisplayListPath.exchange(nullptr)) { - fml::UniqueFD dir = fml::OpenDirectory(path, /*create_if_necessary=*/false, - fml::FilePermission::kWrite); + if (FreePtr path = + FreePtr(saveDisplayListPath.exchange(nullptr), &free)) { + fml::UniqueFD dir = fml::OpenDirectory( + path.get(), /*create_if_necessary=*/false, fml::FilePermission::kWrite); // DisplayList doesn't have an accessor for the byte size of the storage, // adding one may confuse the api so we derive it with bytes(). size_t size = display_list->bytes(/*nested=*/false) - sizeof(DisplayList); fml::NonOwnedMapping mapping(display_list->GetStorage().get(), size); bool success = fml::WriteAtomically(dir, "display_list.dat", mapping); - FML_LOG(ERROR) << "store display_list (" << success << "):" << path; - free(path); + FML_LOG(ERROR) << "store display_list (" << success << "):" << path.get(); } } } // namespace flutter From f995f3b2411155ed47f4d205b6a824fc1075ce8b Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Fri, 10 May 2024 11:23:28 -0700 Subject: [PATCH 3/6] added test --- shell/common/BUILD.gn | 1 + shell/common/display_list_debugger.cc | 5 ++- shell/common/display_list_debugger.h | 4 ++- .../common/display_list_debugger_unittests.cc | 34 +++++++++++++++++++ shell/gpu/gpu_surface_metal_impeller.mm | 2 +- 5 files changed, 43 insertions(+), 3 deletions(-) create mode 100644 shell/common/display_list_debugger_unittests.cc diff --git a/shell/common/BUILD.gn b/shell/common/BUILD.gn index 356c5c79bb325..ae6a999dd2597 100644 --- a/shell/common/BUILD.gn +++ b/shell/common/BUILD.gn @@ -314,6 +314,7 @@ if (enable_unittests) { "animator_unittests.cc", "base64_unittests.cc", "context_options_unittests.cc", + "display_list_debugger_unittests.cc", "dl_op_spy_unittests.cc", "engine_animator_unittests.cc", "engine_unittests.cc", diff --git a/shell/common/display_list_debugger.cc b/shell/common/display_list_debugger.cc index 11ebc8e5c7873..2b4ddc71f1e56 100644 --- a/shell/common/display_list_debugger.cc +++ b/shell/common/display_list_debugger.cc @@ -23,7 +23,7 @@ void DisplayListDebugger::HandleMessage( &free); } -void DisplayListDebugger::SaveDisplayList( +fml::Status DisplayListDebugger::SaveDisplayList( const sk_sp& display_list) { if (FreePtr path = FreePtr(saveDisplayListPath.exchange(nullptr), &free)) { @@ -35,6 +35,9 @@ void DisplayListDebugger::SaveDisplayList( fml::NonOwnedMapping mapping(display_list->GetStorage().get(), size); bool success = fml::WriteAtomically(dir, "display_list.dat", mapping); FML_LOG(ERROR) << "store display_list (" << success << "):" << path.get(); + return success ? fml::Status() + : fml::Status(fml::StatusCode::kUnknown, "write failed"); } + return fml::Status(fml::StatusCode::kUnavailable, "unavailable"); } } // namespace flutter diff --git a/shell/common/display_list_debugger.h b/shell/common/display_list_debugger.h index 4adc12fe4400a..1cf3ec36bae15 100644 --- a/shell/common/display_list_debugger.h +++ b/shell/common/display_list_debugger.h @@ -6,15 +6,17 @@ #define FLUTTER_SHELL_COMMON_DISPLAY_LIST_DEBUGGER_H_ #include "flutter/display_list/display_list.h" +#include "flutter/fml/status.h" #include "flutter/lib/ui/window/platform_message.h" namespace flutter { +/// Utility for saving display lists to disk in response to platform messages. class DisplayListDebugger { public: static constexpr char kChannelName[] = "flutter/display_list"; static void HandleMessage(std::unique_ptr message); - static void SaveDisplayList(const sk_sp& display_list); + static fml::Status SaveDisplayList(const sk_sp& display_list); }; } // namespace flutter diff --git a/shell/common/display_list_debugger_unittests.cc b/shell/common/display_list_debugger_unittests.cc new file mode 100644 index 0000000000000..6cf94b2c62020 --- /dev/null +++ b/shell/common/display_list_debugger_unittests.cc @@ -0,0 +1,34 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "flutter/display_list/dl_builder.h" +#include "flutter/shell/common/display_list_debugger.h" + +#include "gtest/gtest.h" + +namespace flutter { +namespace testing { + +TEST(DisplayListDebuggerTest, Noop) { + DisplayListBuilder builder; + sk_sp display_list = builder.Build(); + EXPECT_FALSE(DisplayListDebugger::SaveDisplayList(display_list).ok()); +} + +TEST(DisplayListDebuggerTest, Simple) { + DisplayListBuilder builder; + builder.Scale(0.2, 0.2); + sk_sp display_list = builder.Build(); + fml::ScopedTemporaryDirectory temp_dir; + auto message = std::make_unique( + DisplayListDebugger::kChannelName, + fml::MallocMapping::Copy(temp_dir.path().c_str(), + temp_dir.path().size() + 1), + /*response=*/nullptr); + DisplayListDebugger::HandleMessage(std::move(message)); + EXPECT_TRUE(DisplayListDebugger::SaveDisplayList(display_list).ok()); +} + +} // namespace testing +} // namespace flutter diff --git a/shell/gpu/gpu_surface_metal_impeller.mm b/shell/gpu/gpu_surface_metal_impeller.mm index 508e0ab2e0b16..7db4f2b786d3c 100644 --- a/shell/gpu/gpu_surface_metal_impeller.mm +++ b/shell/gpu/gpu_surface_metal_impeller.mm @@ -12,8 +12,8 @@ #include "flutter/fml/mapping.h" #include "flutter/fml/paths.h" #include "flutter/fml/trace_event.h" -#include "impeller/display_list/dl_dispatcher.h" #include "flutter/shell/common/display_list_debugger.h" +#include "impeller/display_list/dl_dispatcher.h" #include "impeller/renderer/backend/metal/surface_mtl.h" #include "impeller/typographer/backends/skia/typographer_context_skia.h" From c5745cad1e06d4a3c933da5e98be85349d274522 Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Fri, 10 May 2024 11:44:04 -0700 Subject: [PATCH 4/6] made it only available for debug builds --- shell/gpu/gpu_surface_metal_impeller.mm | 2 ++ 1 file changed, 2 insertions(+) diff --git a/shell/gpu/gpu_surface_metal_impeller.mm b/shell/gpu/gpu_surface_metal_impeller.mm index 7db4f2b786d3c..5dac3e2565631 100644 --- a/shell/gpu/gpu_surface_metal_impeller.mm +++ b/shell/gpu/gpu_surface_metal_impeller.mm @@ -129,7 +129,9 @@ return false; } +#ifndef NDEBUG DisplayListDebugger::SaveDisplayList(display_list); +#endif if (!disable_partial_repaint && damage) { uintptr_t texture = reinterpret_cast(last_texture); From 858289660254cb98e59b42918e8646f3c162a70d Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Fri, 10 May 2024 13:16:41 -0700 Subject: [PATCH 5/6] licenses --- ci/licenses_golden/excluded_files | 1 + ci/licenses_golden/licenses_flutter | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/ci/licenses_golden/excluded_files b/ci/licenses_golden/excluded_files index baeb05a703512..0c350100bfb21 100644 --- a/ci/licenses_golden/excluded_files +++ b/ci/licenses_golden/excluded_files @@ -257,6 +257,7 @@ ../../../flutter/shell/common/animator_unittests.cc ../../../flutter/shell/common/base64_unittests.cc ../../../flutter/shell/common/context_options_unittests.cc +../../../flutter/shell/common/display_list_debugger_unittests.cc ../../../flutter/shell/common/dl_op_spy_unittests.cc ../../../flutter/shell/common/engine_animator_unittests.cc ../../../flutter/shell/common/engine_unittests.cc diff --git a/ci/licenses_golden/licenses_flutter b/ci/licenses_golden/licenses_flutter index b8c15596edb2c..486606e92ac2f 100644 --- a/ci/licenses_golden/licenses_flutter +++ b/ci/licenses_golden/licenses_flutter @@ -41507,6 +41507,8 @@ ORIGIN: ../../../flutter/shell/common/context_options.h + ../../../flutter/LICEN ORIGIN: ../../../flutter/shell/common/dart_native_benchmarks.cc + ../../../flutter/LICENSE ORIGIN: ../../../flutter/shell/common/display.cc + ../../../flutter/LICENSE ORIGIN: ../../../flutter/shell/common/display.h + ../../../flutter/LICENSE +ORIGIN: ../../../flutter/shell/common/display_list_debugger.cc + ../../../flutter/LICENSE +ORIGIN: ../../../flutter/shell/common/display_list_debugger.h + ../../../flutter/LICENSE ORIGIN: ../../../flutter/shell/common/display_manager.cc + ../../../flutter/LICENSE ORIGIN: ../../../flutter/shell/common/display_manager.h + ../../../flutter/LICENSE ORIGIN: ../../../flutter/shell/common/dl_op_spy.cc + ../../../flutter/LICENSE @@ -44395,6 +44397,8 @@ FILE: ../../../flutter/shell/common/context_options.h FILE: ../../../flutter/shell/common/dart_native_benchmarks.cc FILE: ../../../flutter/shell/common/display.cc FILE: ../../../flutter/shell/common/display.h +FILE: ../../../flutter/shell/common/display_list_debugger.cc +FILE: ../../../flutter/shell/common/display_list_debugger.h FILE: ../../../flutter/shell/common/display_manager.cc FILE: ../../../flutter/shell/common/display_manager.h FILE: ../../../flutter/shell/common/dl_op_spy.cc From 7cd0a0c8f35baa31d19f180e4290c593bed3a2ef Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Fri, 10 May 2024 13:17:22 -0700 Subject: [PATCH 6/6] changed channel name --- shell/common/display_list_debugger.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/shell/common/display_list_debugger.h b/shell/common/display_list_debugger.h index 1cf3ec36bae15..1c8e622e5f9c5 100644 --- a/shell/common/display_list_debugger.h +++ b/shell/common/display_list_debugger.h @@ -14,7 +14,7 @@ namespace flutter { /// Utility for saving display lists to disk in response to platform messages. class DisplayListDebugger { public: - static constexpr char kChannelName[] = "flutter/display_list"; + static constexpr char kChannelName[] = "flutter/display_list_debugger"; static void HandleMessage(std::unique_ptr message); static fml::Status SaveDisplayList(const sk_sp& display_list); };