From 1a2f92068e40b504a6b2053f65f20ae0f64fabd9 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Mon, 5 Feb 2024 15:05:48 -0800 Subject: [PATCH 1/4] Add IMPORTANT log level, change Impeller logging info to use it --- fml/log_level.h | 12 ++++++++++-- fml/logging.cc | 6 ++++-- fml/logging_unittests.cc | 5 +++++ .../android/android_context_gl_impeller.cc | 2 +- .../android/android_context_vulkan_impeller.cc | 6 +++--- .../FlutterDarwinContextMetalImpeller.mm | 1 - .../graphics/FlutterDarwinContextMetalSkia.mm | 2 ++ .../embedder/embedder_surface_gl_impeller.cc | 2 +- .../embedder_surface_metal_impeller.mm | 18 +++++++++--------- 9 files changed, 35 insertions(+), 19 deletions(-) diff --git a/fml/log_level.h b/fml/log_level.h index 8610aa5fe52df..a53b4ab44cde2 100644 --- a/fml/log_level.h +++ b/fml/log_level.h @@ -13,8 +13,11 @@ typedef int LogSeverity; constexpr LogSeverity kLogInfo = 0; constexpr LogSeverity kLogWarning = 1; constexpr LogSeverity kLogError = 2; -constexpr LogSeverity kLogFatal = 3; -constexpr LogSeverity kLogNumSeverities = 4; +// A log that is not an error, is important enough to display even if ordinary +// info is hidden. +constexpr LogSeverity kLogImportant = 3; +constexpr LogSeverity kLogFatal = 4; +constexpr LogSeverity kLogNumSeverities = 5; // DEPRECATED: Use |kLogInfo|. // Ignoring Clang Tidy because this is used in a very common substitution macro. @@ -31,6 +34,11 @@ constexpr LogSeverity LOG_WARNING = kLogWarning; // NOLINTNEXTLINE(readability-identifier-naming) constexpr LogSeverity LOG_ERROR = kLogError; +// DEPRECATED: Use |kLogImportant|. +// Ignoring Clang Tidy because this is used in a very common substitution macro. +// NOLINTNEXTLINE(readability-identifier-naming) +constexpr LogSeverity LOG_IMPORTANT = kLogImportant; + // DEPRECATED: Use |kLogFatal|. // Ignoring Clang Tidy because this is used in a very common substitution macro. // NOLINTNEXTLINE(readability-identifier-naming) diff --git a/fml/logging.cc b/fml/logging.cc index bedd82017f387..144ba3a7d5cec 100644 --- a/fml/logging.cc +++ b/fml/logging.cc @@ -27,8 +27,8 @@ namespace fml { namespace { #if !defined(OS_FUCHSIA) -const char* const kLogSeverityNames[kLogNumSeverities] = {"INFO", "WARNING", - "ERROR", "FATAL"}; +const char* const kLogSeverityNames[kLogNumSeverities] = { + "INFO", "WARNING", "ERROR", "FATAL", "IMPORTANT"}; const char* GetNameForLogSeverity(LogSeverity severity) { if (severity >= kLogInfo && severity < kLogNumSeverities) { @@ -153,6 +153,7 @@ LogMessage::~LogMessage() { android_LogPriority priority = (severity_ < 0) ? ANDROID_LOG_VERBOSE : ANDROID_LOG_UNKNOWN; switch (severity_) { + case kLogImportant: case kLogInfo: priority = ANDROID_LOG_INFO; break; @@ -172,6 +173,7 @@ LogMessage::~LogMessage() { #elif defined(OS_FUCHSIA) FuchsiaLogSeverity severity; switch (severity_) { + case kLogImportant: case kLogInfo: severity = FUCHSIA_LOG_INFO; break; diff --git a/fml/logging_unittests.cc b/fml/logging_unittests.cc index 812dbf67060a8..6b3935686d17e 100644 --- a/fml/logging_unittests.cc +++ b/fml/logging_unittests.cc @@ -7,11 +7,16 @@ #include "flutter/fml/build_config.h" #include "flutter/fml/log_settings.h" #include "flutter/fml/logging.h" +#include "fml/log_level.h" #include "gtest/gtest.h" namespace fml { namespace testing { +static_assert(fml::kLogFatal == fml::kLogNumSeverities - 1); +static_assert(fml::kLogImportant < fml::kLogFatal); +static_assert(fml::kLogImportant > fml::kLogError); + #ifndef OS_FUCHSIA class MakeSureFmlLogDoesNotSegfaultWhenStaticallyCalled { public: diff --git a/shell/platform/android/android_context_gl_impeller.cc b/shell/platform/android/android_context_gl_impeller.cc index d76910c4b317d..73cec9af7d9e1 100644 --- a/shell/platform/android/android_context_gl_impeller.cc +++ b/shell/platform/android/android_context_gl_impeller.cc @@ -84,7 +84,7 @@ static std::shared_ptr CreateImpellerContext( FML_LOG(ERROR) << "Could not add reactor worker."; return nullptr; } - FML_LOG(ERROR) << "Using the Impeller rendering backend (OpenGLES)."; + FML_LOG(IMPORTANT) << "Using the Impeller rendering backend (OpenGLES)."; return context; } diff --git a/shell/platform/android/android_context_vulkan_impeller.cc b/shell/platform/android/android_context_vulkan_impeller.cc index 7f4babb7ed984..e5ba19802e1b6 100644 --- a/shell/platform/android/android_context_vulkan_impeller.cc +++ b/shell/platform/android/android_context_vulkan_impeller.cc @@ -48,10 +48,10 @@ static std::shared_ptr CreateImpellerContext( if (context && impeller::CapabilitiesVK::Cast(*context->GetCapabilities()) .AreValidationsEnabled()) { - FML_LOG(ERROR) << "Using the Impeller rendering backend (Vulkan with " - "Validation Layers)."; + FML_LOG(IMPORTANT) << "Using the Impeller rendering backend (Vulkan with " + "Validation Layers)."; } else { - FML_LOG(ERROR) << "Using the Impeller rendering backend (Vulkan)."; + FML_LOG(IMPORTANT) << "Using the Impeller rendering backend (Vulkan)."; } return context; diff --git a/shell/platform/darwin/graphics/FlutterDarwinContextMetalImpeller.mm b/shell/platform/darwin/graphics/FlutterDarwinContextMetalImpeller.mm index b7b4a3d6b7b6f..76f710954c7db 100644 --- a/shell/platform/darwin/graphics/FlutterDarwinContextMetalImpeller.mm +++ b/shell/platform/darwin/graphics/FlutterDarwinContextMetalImpeller.mm @@ -39,7 +39,6 @@ FML_LOG(ERROR) << "Could not create Metal Impeller Context."; return nullptr; } - FML_LOG(ERROR) << "Using the Impeller rendering backend."; return context; } diff --git a/shell/platform/darwin/graphics/FlutterDarwinContextMetalSkia.mm b/shell/platform/darwin/graphics/FlutterDarwinContextMetalSkia.mm index c6e1321b19f8c..9457437918629 100644 --- a/shell/platform/darwin/graphics/FlutterDarwinContextMetalSkia.mm +++ b/shell/platform/darwin/graphics/FlutterDarwinContextMetalSkia.mm @@ -60,6 +60,8 @@ - (instancetype)initWithMTLDevice:(id)device return nil; } + FML_LOG(IMPORTANT) << "Using the Skia rendering backend (Metal)."; + _resourceContext->setResourceCacheLimit(0u); } return self; diff --git a/shell/platform/embedder/embedder_surface_gl_impeller.cc b/shell/platform/embedder/embedder_surface_gl_impeller.cc index c061c091a912b..afaed2a86b7f1 100644 --- a/shell/platform/embedder/embedder_surface_gl_impeller.cc +++ b/shell/platform/embedder/embedder_surface_gl_impeller.cc @@ -94,7 +94,7 @@ EmbedderSurfaceGLImpeller::EmbedderSurfaceGLImpeller( } gl_dispatch_table_.gl_clear_current_callback(); - FML_LOG(ERROR) << "Using the Impeller rendering backend (OpenGL)."; + FML_LOG(IMPORTANT) << "Using the Impeller rendering backend (OpenGL)."; valid_ = true; } diff --git a/shell/platform/embedder/embedder_surface_metal_impeller.mm b/shell/platform/embedder/embedder_surface_metal_impeller.mm index c9f463a059700..c36199a2a3fc0 100644 --- a/shell/platform/embedder/embedder_surface_metal_impeller.mm +++ b/shell/platform/embedder/embedder_surface_metal_impeller.mm @@ -34,16 +34,16 @@ metal_dispatch_table_(std::move(metal_dispatch_table)), external_view_embedder_(std::move(external_view_embedder)) { std::vector> shader_mappings = { - std::make_shared(impeller_entity_shaders_data, - impeller_entity_shaders_length), + std::make_shared(impeller_entity_shaders_data, + impeller_entity_shaders_length), #if IMPELLER_ENABLE_3D - std::make_shared(impeller_scene_shaders_data, - impeller_scene_shaders_length), + std::make_shared(impeller_scene_shaders_data, + impeller_scene_shaders_length), #endif // IMPELLER_ENABLE_3D - std::make_shared(impeller_modern_shaders_data, - impeller_modern_shaders_length), - std::make_shared(impeller_framebuffer_blend_shaders_data, - impeller_framebuffer_blend_shaders_length), + std::make_shared(impeller_modern_shaders_data, + impeller_modern_shaders_length), + std::make_shared(impeller_framebuffer_blend_shaders_data, + impeller_framebuffer_blend_shaders_length), }; context_ = impeller::ContextMTL::Create( (id)device, // device @@ -52,7 +52,7 @@ std::make_shared(false), // is_gpu_disabled_sync_switch "Impeller Library" // library_label ); - FML_LOG(ERROR) << "Using the Impeller rendering backend (Metal)."; + FML_LOG(IMPORTANT) << "Using the Impeller rendering backend (Metal)."; valid_ = !!context_; } From 4deaa544f2f2a1084a81e4816f2fbafbac32919c Mon Sep 17 00:00:00 2001 From: Dan Field Date: Wed, 7 Feb 2024 11:56:05 -0800 Subject: [PATCH 2/4] better test, important ordering --- fml/logging.cc | 15 ++------------- fml/logging_unittests.cc | 26 ++++++++++++++++++++++++++ 2 files changed, 28 insertions(+), 13 deletions(-) diff --git a/fml/logging.cc b/fml/logging.cc index 144ba3a7d5cec..15bfa9de368b4 100644 --- a/fml/logging.cc +++ b/fml/logging.cc @@ -28,7 +28,7 @@ namespace { #if !defined(OS_FUCHSIA) const char* const kLogSeverityNames[kLogNumSeverities] = { - "INFO", "WARNING", "ERROR", "FATAL", "IMPORTANT"}; + "INFO", "WARNING", "ERROR", "IMPORTANT", "FATAL"}; const char* GetNameForLogSeverity(LogSeverity severity) { if (severity >= kLogInfo && severity < kLogNumSeverities) { @@ -45,14 +45,6 @@ const char* StripDots(const char* path) { return path; } -const char* StripPath(const char* path) { - auto* p = strrchr(path, '/'); - if (p) { - return p + 1; - } - return path; -} - #if defined(OS_FUCHSIA) zx_koid_t GetKoid(zx_handle_t handle) { @@ -98,9 +90,7 @@ LogMessage::LogMessage(LogSeverity severity, const char* file, int line, const char* condition) - : severity_(severity), - file_(severity > kLogInfo ? StripDots(file) : StripPath(file)), - line_(line) { + : severity_(severity), file_(StripDots(file)), line_(line) { #if !defined(OS_FUCHSIA) stream_ << "["; if (severity >= kLogInfo) { @@ -144,7 +134,6 @@ LogMessage::~LogMessage() { #if !defined(OS_FUCHSIA) stream_ << std::endl; #endif - if (capture_next_log_stream_) { *capture_next_log_stream_ << stream_.str(); capture_next_log_stream_ = nullptr; diff --git a/fml/logging_unittests.cc b/fml/logging_unittests.cc index 6b3935686d17e..0b861aee08c69 100644 --- a/fml/logging_unittests.cc +++ b/fml/logging_unittests.cc @@ -72,5 +72,31 @@ TEST(LoggingTest, UnreachableKillProcessWithMacro) { ASSERT_DEATH({ FML_UNREACHABLE(); }, ""); } +TEST(LoggingTest, SanityTests) { +#if OS_FUCHSIA + GTEST_SKIP() << "Fuchsia does this differently."; +#endif // OS_FUCHSIA + ::testing::FLAGS_gtest_death_test_style = "threadsafe"; + std::vector severities = {"INFO", "WARNING", "ERROR", + "IMPORTANT"}; + + for (size_t i = 0; i < severities.size(); i++) { + LogCapture capture; + { + LogMessage log(i, "path/to/file.cc", 4, nullptr); + log.stream() << "Hello!"; + } + EXPECT_EQ(capture.str(), std::string("[" + severities[i] + + ":path/to/file.cc(4)] Hello!\n")); + } + + ASSERT_DEATH( + { + LogMessage log(kLogFatal, "path/to/file.cc", 5, nullptr); + log.stream() << "Goodbye"; + }, + R"(\[FATAL:path/to/file.cc\(5\)\] Goodbye)"); +} + } // namespace testing } // namespace fml From 8e20acc3d935d5577a6973605e33294630ee6356 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Wed, 7 Feb 2024 12:33:06 -0800 Subject: [PATCH 3/4] fuchsia... --- fml/logging_unittests.cc | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/fml/logging_unittests.cc b/fml/logging_unittests.cc index 0b861aee08c69..86dbd636c4800 100644 --- a/fml/logging_unittests.cc +++ b/fml/logging_unittests.cc @@ -72,10 +72,8 @@ TEST(LoggingTest, UnreachableKillProcessWithMacro) { ASSERT_DEATH({ FML_UNREACHABLE(); }, ""); } +#if !OS_FUCHSIA TEST(LoggingTest, SanityTests) { -#if OS_FUCHSIA - GTEST_SKIP() << "Fuchsia does this differently."; -#endif // OS_FUCHSIA ::testing::FLAGS_gtest_death_test_style = "threadsafe"; std::vector severities = {"INFO", "WARNING", "ERROR", "IMPORTANT"}; @@ -97,6 +95,7 @@ TEST(LoggingTest, SanityTests) { }, R"(\[FATAL:path/to/file.cc\(5\)\] Goodbye)"); } +#endif // !OS_FUCHSIA } // namespace testing } // namespace fml From c23e39bbb7ae12f2831353f751d9aaefd493b9fb Mon Sep 17 00:00:00 2001 From: Dan Field Date: Wed, 7 Feb 2024 12:33:27 -0800 Subject: [PATCH 4/4] .. --- fml/logging_unittests.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fml/logging_unittests.cc b/fml/logging_unittests.cc index 86dbd636c4800..fd0eca048e9ac 100644 --- a/fml/logging_unittests.cc +++ b/fml/logging_unittests.cc @@ -72,7 +72,7 @@ TEST(LoggingTest, UnreachableKillProcessWithMacro) { ASSERT_DEATH({ FML_UNREACHABLE(); }, ""); } -#if !OS_FUCHSIA +#ifndef OS_FUCHSIA TEST(LoggingTest, SanityTests) { ::testing::FLAGS_gtest_death_test_style = "threadsafe"; std::vector severities = {"INFO", "WARNING", "ERROR",