From dfced6d19950d6d2e9086f34c2dc54ae15c9f4b4 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Mon, 1 Apr 2024 10:33:02 -0700 Subject: [PATCH 1/3] Never panic in `finally { ... }`, check output logs on success only. (#51814) Work towards https://github.com/flutter/flutter/issues/145988. Should be a NO-OP in behavior, but hopefully make some of the false negatives less confusing (i.e. getting "missing X outputted files when the test is about to fail anyway". --- .../scenario_app/bin/run_android_tests.dart | 97 +++++++++++-------- testing/scenario_app/bin/utils/logs.dart | 8 +- 2 files changed, 61 insertions(+), 44 deletions(-) diff --git a/testing/scenario_app/bin/run_android_tests.dart b/testing/scenario_app/bin/run_android_tests.dart index c0b8ffaf4b0cc..71ff683d40b10 100644 --- a/testing/scenario_app/bin/run_android_tests.dart +++ b/testing/scenario_app/bin/run_android_tests.dart @@ -392,7 +392,49 @@ Future _run({ panic(['$comparisonsFailed Skia Gold comparisons failed']); } }); + + + if (enableImpeller) { + await step('Validating Impeller...', () async { + final _ImpellerBackend expectedImpellerBackend = impellerBackend ?? _ImpellerBackend.vulkan; + if (actualImpellerBackend != expectedImpellerBackend) { + panic([ + '--enable-impeller was specified and expected to find "${expectedImpellerBackend.name}", which did not match "${actualImpellerBackend?.name ?? ''}".', + ]); + } + }); + } + + await step('Wait for pending Skia gold comparisons...', () async { + await Future.wait(pendingComparisons); + }); + + final bool allTestsRun = smokeTestFullPath == null; + final bool checkGoldens = contentsGolden != null; + if (allTestsRun && checkGoldens) { + // Check the output here. + await step('Check output files...', () async { + // TODO(matanlurey): Resolve this in a better way. On CI this file always exists. + File(join(screenshotPath, 'noop.txt')).writeAsStringSync(''); + // TODO(gaaclarke): We should move this into dir_contents_diff. + final String diffScreenhotPath = absolute(screenshotPath); + _withTemporaryCwd(absolute(dirname(contentsGolden)), () { + final int exitCode = dirContentsDiff(basename(contentsGolden), diffScreenhotPath); + if (exitCode != 0) { + panic(['Output contents incorrect.']); + } + }); + }); + } } finally { + // The finally clause is entered if: + // - The tests have completed successfully. + // - Any step has failed. + // + // Do *NOT* throw exceptions or errors in this block, as these are cleanup + // steps and the program is about to exit. Instead, just log the error and + // continue with the cleanup. + await server.close(); for (final Socket client in pendingConnections.toList()) { client.close(); @@ -401,7 +443,7 @@ Future _run({ await step('Killing test app and test runner...', () async { final int exitCode = await pm.runAndForward([adb.path, 'shell', 'am', 'force-stop', 'dev.flutter.scenarios']); if (exitCode != 0) { - panic(['could not kill test app']); + logError('could not kill test app'); } }); @@ -443,17 +485,6 @@ Future _run({ ); }); - if (enableImpeller) { - await step('Validating Impeller...', () async { - final _ImpellerBackend expectedImpellerBackend = impellerBackend ?? _ImpellerBackend.vulkan; - if (actualImpellerBackend != expectedImpellerBackend) { - panic([ - '--enable-impeller was specified and expected to find "${expectedImpellerBackend.name}", which did not match "${actualImpellerBackend?.name ?? ''}".', - ]); - } - }); - } - await step('Symbolize stack traces', () async { final ProcessResult result = await pm.run( [ @@ -477,47 +508,31 @@ Future _run({ 'tcp:3000', ]); if (exitCode != 0) { - panic(['could not unforward port']); + logError('could not unforward port'); } }); await step('Uninstalling app APK...', () async { - final int exitCode = await pm.runAndForward( - [adb.path, 'uninstall', 'dev.flutter.scenarios']); + final int exitCode = await pm.runAndForward([ + adb.path, + 'uninstall', + 'dev.flutter.scenarios', + ]); if (exitCode != 0) { - panic(['could not uninstall app apk']); + logError('could not uninstall app apk'); } }); await step('Uninstalling test APK...', () async { - final int exitCode = await pm.runAndForward( - [adb.path, 'uninstall', 'dev.flutter.scenarios.test']); + final int exitCode = await pm.runAndForward([ + adb.path, + 'uninstall', + 'dev.flutter.scenarios.test', + ]); if (exitCode != 0) { - panic(['could not uninstall app apk']); + logError('could not uninstall app apk'); } }); - - await step('Wait for Skia gold comparisons...', () async { - await Future.wait(pendingComparisons); - }); - - final bool allTestsRun = smokeTestFullPath == null; - final bool checkGoldens = contentsGolden != null; - if (allTestsRun && checkGoldens) { - // Check the output here. - await step('Check output files...', () async { - // TODO(matanlurey): Resolve this in a better way. On CI this file always exists. - File(join(screenshotPath, 'noop.txt')).writeAsStringSync(''); - // TODO(gaaclarke): We should move this into dir_contents_diff. - final String diffScreenhotPath = absolute(screenshotPath); - _withTemporaryCwd(absolute(dirname(contentsGolden)), () { - final int exitCode = dirContentsDiff(basename(contentsGolden), diffScreenhotPath); - if (exitCode != 0) { - panic(['Output contents incorrect.']); - } - }); - }); - } } } diff --git a/testing/scenario_app/bin/utils/logs.dart b/testing/scenario_app/bin/utils/logs.dart index 4d883d340e261..e082211cefabb 100644 --- a/testing/scenario_app/bin/utils/logs.dart +++ b/testing/scenario_app/bin/utils/logs.dart @@ -39,11 +39,13 @@ void logWarning(String msg) { _logWithColor(_yellow, msg); } +void logError(String msg) { + _logWithColor(_red, msg); +} + final class Panic extends Error {} Never panic(List messages) { - for (final String message in messages) { - _logWithColor(_red, message); - } + messages.forEach(logError); throw Panic(); } From 74d93dc38922035a402bcc054688269aff79ceae Mon Sep 17 00:00:00 2001 From: Jason Simmons Date: Mon, 1 Apr 2024 10:33:05 -0700 Subject: [PATCH 2/3] [Impeller] Set RGBA8888 as the default Vulkan color format before the app acquires a surface (#51770) An app may render offscreen images before it gets a window. The ContextVK needs to have a default color format so that render passes can be created before the ContextVK is updated to use the window's format. --- impeller/renderer/backend/vulkan/capabilities_vk.cc | 8 +++++++- .../renderer/backend/vulkan/context_vk_unittests.cc | 12 ++++++++++-- impeller/renderer/backend/vulkan/test/mock_vulkan.cc | 2 +- 3 files changed, 18 insertions(+), 4 deletions(-) diff --git a/impeller/renderer/backend/vulkan/capabilities_vk.cc b/impeller/renderer/backend/vulkan/capabilities_vk.cc index b5ad0aba23f93..0ee92dc55abdf 100644 --- a/impeller/renderer/backend/vulkan/capabilities_vk.cc +++ b/impeller/renderer/backend/vulkan/capabilities_vk.cc @@ -297,7 +297,7 @@ static bool HasSuitableDepthStencilFormat(const vk::PhysicalDevice& device, static bool PhysicalDeviceSupportsRequiredFormats( const vk::PhysicalDevice& device) { const auto has_color_format = - HasSuitableColorFormat(device, vk::Format::eB8G8R8A8Unorm); + HasSuitableColorFormat(device, vk::Format::eR8G8B8A8Unorm); const auto has_stencil_format = HasSuitableDepthStencilFormat(device, vk::Format::eD32SfloatS8Uint) || HasSuitableDepthStencilFormat(device, vk::Format::eD24UnormS8Uint); @@ -412,6 +412,12 @@ void CapabilitiesVK::SetOffscreenFormat(PixelFormat pixel_format) const { } bool CapabilitiesVK::SetPhysicalDevice(const vk::PhysicalDevice& device) { + if (HasSuitableColorFormat(device, vk::Format::eR8G8B8A8Unorm)) { + default_color_format_ = PixelFormat::kR8G8B8A8UNormInt; + } else { + default_color_format_ = PixelFormat::kUnknown; + } + if (HasSuitableDepthStencilFormat(device, vk::Format::eD32SfloatS8Uint)) { default_depth_stencil_format_ = PixelFormat::kD32FloatS8UInt; } else if (HasSuitableDepthStencilFormat(device, diff --git a/impeller/renderer/backend/vulkan/context_vk_unittests.cc b/impeller/renderer/backend/vulkan/context_vk_unittests.cc index c0d41e7ea6da4..835c7a1389307 100644 --- a/impeller/renderer/backend/vulkan/context_vk_unittests.cc +++ b/impeller/renderer/backend/vulkan/context_vk_unittests.cc @@ -167,7 +167,7 @@ TEST(CapabilitiesVKTest, ContextInitializesWithNoStencilFormat) { .SetPhysicalDeviceFormatPropertiesCallback( [](VkPhysicalDevice physicalDevice, VkFormat format, VkFormatProperties* pFormatProperties) { - if (format == VK_FORMAT_B8G8R8A8_UNORM) { + if (format == VK_FORMAT_R8G8B8A8_UNORM) { pFormatProperties->optimalTilingFeatures = static_cast( vk::FormatFeatureFlagBits::eColorAttachment); @@ -200,7 +200,7 @@ TEST(CapabilitiesVKTest, .SetPhysicalDeviceFormatPropertiesCallback( [](VkPhysicalDevice physicalDevice, VkFormat format, VkFormatProperties* pFormatProperties) { - if (format == VK_FORMAT_B8G8R8A8_UNORM) { + if (format == VK_FORMAT_R8G8B8A8_UNORM) { pFormatProperties->optimalTilingFeatures = static_cast( vk::FormatFeatureFlagBits::eColorAttachment); @@ -222,5 +222,13 @@ TEST(ContextVKTest, WarmUpFunctionCreatesRenderPass) { "vkCreateRenderPass") != functions->end()); } +TEST(ContextVKTest, HasDefaultColorFormat) { + std::shared_ptr context = MockVulkanContextBuilder().Build(); + + const CapabilitiesVK* capabilites_vk = + reinterpret_cast(context->GetCapabilities().get()); + ASSERT_NE(capabilites_vk->GetDefaultColorFormat(), PixelFormat::kUnknown); +} + } // namespace testing } // namespace impeller diff --git a/impeller/renderer/backend/vulkan/test/mock_vulkan.cc b/impeller/renderer/backend/vulkan/test/mock_vulkan.cc index 77cd8cec88f7a..c18a569315863 100644 --- a/impeller/renderer/backend/vulkan/test/mock_vulkan.cc +++ b/impeller/renderer/backend/vulkan/test/mock_vulkan.cc @@ -894,7 +894,7 @@ MockVulkanContextBuilder::MockVulkanContextBuilder() format_properties_callback_([](VkPhysicalDevice physicalDevice, VkFormat format, VkFormatProperties* pFormatProperties) { - if (format == VK_FORMAT_B8G8R8A8_UNORM) { + if (format == VK_FORMAT_R8G8B8A8_UNORM) { pFormatProperties->optimalTilingFeatures = static_cast( vk::FormatFeatureFlagBits::eColorAttachment); From 8dff6b833fe254fdee18f5ca1a2a3b77dca44a70 Mon Sep 17 00:00:00 2001 From: Jason Simmons Date: Mon, 1 Apr 2024 10:48:50 -0700 Subject: [PATCH 3/3] Use the stripped Vulkan validation library in Android engine builds by default (#51628) The unstripped build will be used if GN is configured with the --no-stripped flag --- shell/platform/android/BUILD.gn | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/shell/platform/android/BUILD.gn b/shell/platform/android/BUILD.gn index b1a0f428e860b..4b537f4df48b7 100644 --- a/shell/platform/android/BUILD.gn +++ b/shell/platform/android/BUILD.gn @@ -524,12 +524,19 @@ action("android_jar") { apilevel26_toolchain = "//build/toolchain/android:clang_arm64_apilevel26" validation_layer_target = "//third_party/vulkan_validation_layers($apilevel26_toolchain)" - validation_layer_out_dir = - get_label_info(validation_layer_target, "root_out_dir") deps += [ validation_layer_target ] + if (stripped_symbols) { + validation_library = "lib.stripped/libVkLayer_khronos_validation.so" + } else { + validation_layer_out_dir = + get_label_info(validation_layer_target, "root_out_dir") + validation_library = rebase_path( + "$validation_layer_out_dir/libVkLayer_khronos_validation.so") + } + args += [ "--native_lib", - rebase_path("$validation_layer_out_dir/libVkLayer_khronos_validation.so"), + validation_library, ] if (current_cpu != "arm64") { # This may not be necessarily required anymore. It was kept to maintain