From 95e10121a857e7bdbd752077bc46c3c1cb3c6c2d Mon Sep 17 00:00:00 2001 From: Jonah Williams Date: Mon, 10 Mar 2025 15:43:36 -0700 Subject: [PATCH] [Impeller] Fixes to YUV imports on Android, Incomplete read of pipeline cache data, missing enabled extensions. (#164744) - Handle textures that require a YUV import but aren't an undefined format. - INCOMPLETE is actually a success case for the pipeline cache. CERTAIN drivers ALWAYS return incomplete, even when they wrote all the data. Probably an off by one or something like that... - Ensures Optional AndroidExtensions are enabled - Only creates a YUV conversion if necessary --- engine/src/flutter/.ci.yaml | 7 -- engine/src/flutter/BUILD.gn | 2 + .../ci/builders/linux_android_emulator.json | 4 + .../flutter/ci/licenses_golden/excluded_files | 1 + .../impeller/renderer/backend/vulkan/BUILD.gn | 48 +++++++++ .../vulkan/android/ahb_texture_source_vk.cc | 67 +++++++++---- .../ahb_texture_source_vk_unittests.cc | 97 +++++++++++++++++++ .../backend/vulkan/capabilities_vk.cc | 16 ++- .../backend/vulkan/pipeline_cache_data_vk.cc | 10 +- engine/src/flutter/testing/run_tests.py | 1 + 10 files changed, 221 insertions(+), 32 deletions(-) create mode 100644 engine/src/flutter/impeller/renderer/backend/vulkan/android/ahb_texture_source_vk_unittests.cc diff --git a/engine/src/flutter/.ci.yaml b/engine/src/flutter/.ci.yaml index 732fe855be..9de03afb33 100644 --- a/engine/src/flutter/.ci.yaml +++ b/engine/src/flutter/.ci.yaml @@ -67,13 +67,6 @@ targets: {"dependency": "goldctl", "version": "git_revision:720a542f6fe4f92922c3b8f0fdcc4d2ac6bb83cd"} ] timeout: 90 - runIf: - - DEPS - - engine/src/flutter/.ci.yaml - - engine/src/flutter/ci/builders/linux_android_emulator.json - - engine/src/flutter/lib/ui/** - - engine/src/flutter/shell/platform/android/** - - engine/src/flutter/testing/skia_gold_client/** - name: Linux builder_cache enabled_branches: diff --git a/engine/src/flutter/BUILD.gn b/engine/src/flutter/BUILD.gn index 8a854b24e0..12d69c65a5 100644 --- a/engine/src/flutter/BUILD.gn +++ b/engine/src/flutter/BUILD.gn @@ -170,6 +170,8 @@ group("unittests") { public_deps = [] if (is_android) { public_deps += [ + "//flutter/impeller/renderer/backend/vulkan:vulkan_android_apk_unittests", + "//flutter/impeller/renderer/backend/vulkan:vulkan_android_unittests", "//flutter/impeller/toolkit/android:apk_unittests", "//flutter/impeller/toolkit/android:unittests", "//flutter/shell/platform/android:flutter_shell_native_unittests", diff --git a/engine/src/flutter/ci/builders/linux_android_emulator.json b/engine/src/flutter/ci/builders/linux_android_emulator.json index 2ff2383ac4..53a4c90cce 100644 --- a/engine/src/flutter/ci/builders/linux_android_emulator.json +++ b/engine/src/flutter/ci/builders/linux_android_emulator.json @@ -30,6 +30,8 @@ "ninja": { "config": "ci/android_emulator_debug_x64", "targets": [ + "flutter/impeller/renderer/backend/vulkan:vulkan_android_unittests", + "flutter/impeller/renderer/backend/vulkan:vulkan_android_apk_unittests", "flutter/impeller/toolkit/android:unittests", "flutter/shell/platform/android:flutter_shell_native_unittests" ] @@ -97,6 +99,8 @@ "ninja": { "config": "ci/android_emulator_debug_x86", "targets": [ + "flutter/impeller/renderer/backend/vulkan:vulkan_android_unittests", + "flutter/impeller/renderer/backend/vulkan:vulkan_android_apk_unittests", "flutter/impeller/toolkit/android:unittests", "flutter/shell/platform/android:flutter_shell_native_unittests" ] diff --git a/engine/src/flutter/ci/licenses_golden/excluded_files b/engine/src/flutter/ci/licenses_golden/excluded_files index 6e43c1045f..3a2ce74781 100644 --- a/engine/src/flutter/ci/licenses_golden/excluded_files +++ b/engine/src/flutter/ci/licenses_golden/excluded_files @@ -192,6 +192,7 @@ ../../../flutter/impeller/renderer/backend/metal/swapchain_transients_mtl_unittests.mm ../../../flutter/impeller/renderer/backend/metal/texture_mtl_unittests.mm ../../../flutter/impeller/renderer/backend/vulkan/allocator_vk_unittests.cc +../../../flutter/impeller/renderer/backend/vulkan/android/ahb_texture_source_vk_unittests.cc ../../../flutter/impeller/renderer/backend/vulkan/command_encoder_vk_unittests.cc ../../../flutter/impeller/renderer/backend/vulkan/command_pool_vk_unittests.cc ../../../flutter/impeller/renderer/backend/vulkan/context_vk_unittests.cc diff --git a/engine/src/flutter/impeller/renderer/backend/vulkan/BUILD.gn b/engine/src/flutter/impeller/renderer/backend/vulkan/BUILD.gn index 6e8b85e9f6..cc800da0bc 100644 --- a/engine/src/flutter/impeller/renderer/backend/vulkan/BUILD.gn +++ b/engine/src/flutter/impeller/renderer/backend/vulkan/BUILD.gn @@ -2,6 +2,7 @@ # Use of this source code is governed by a BSD-style license that can be # found in the LICENSE file. +import("//flutter/testing/android/native_activity/native_activity.gni") import("//flutter/vulkan/config.gni") import("../../../tools/impeller.gni") @@ -161,3 +162,50 @@ impeller_component("vulkan") { public_deps += [ "../../../toolkit/android" ] } } + +if (is_android) { + config("public_android_config") { + defines = [ "__ANDROID_UNAVAILABLE_SYMBOLS_ARE_WEAK__" ] + } + + test_fixtures("unittests_fixtures") { + fixtures = [] + } + + source_set("unittests_lib") { + visibility = [ ":*" ] + + testonly = true + + sources = [ "android/ahb_texture_source_vk_unittests.cc" ] + + deps = [ + ":unittests_fixtures", + ":vulkan", + "//flutter/testing", + ] + + defines = [ "TESTING" ] + } + + executable("vulkan_android_unittests") { + assert(is_android) + + testonly = true + + output_name = "impeller_vulkan_android_unittests" + + deps = [ ":unittests_lib" ] + } + + native_activity_apk("vulkan_android_apk_unittests") { + apk_name = "impeller_vulkan_android_unittests" + + testonly = true + + deps = [ + ":unittests_lib", + "//flutter/testing/android/native_activity:gtest_activity", + ] + } +} diff --git a/engine/src/flutter/impeller/renderer/backend/vulkan/android/ahb_texture_source_vk.cc b/engine/src/flutter/impeller/renderer/backend/vulkan/android/ahb_texture_source_vk.cc index 9234eecf2f..39663ca326 100644 --- a/engine/src/flutter/impeller/renderer/backend/vulkan/android/ahb_texture_source_vk.cc +++ b/engine/src/flutter/impeller/renderer/backend/vulkan/android/ahb_texture_source_vk.cc @@ -11,13 +11,30 @@ namespace impeller { +namespace { + +bool RequiresYCBCRConversion(vk::Format format) { + switch (format) { + case vk::Format::eG8B8R83Plane420Unorm: + case vk::Format::eG8B8R82Plane420Unorm: + case vk::Format::eG8B8R83Plane422Unorm: + case vk::Format::eG8B8R82Plane422Unorm: + case vk::Format::eG8B8R83Plane444Unorm: + return true; + default: + // NOTE: NOT EXHAUSTIVE. + break; + } + return false; +} + using AHBProperties = vk::StructureChain< // For VK_ANDROID_external_memory_android_hardware_buffer vk::AndroidHardwareBufferPropertiesANDROID, // For VK_ANDROID_external_memory_android_hardware_buffer vk::AndroidHardwareBufferFormatPropertiesANDROID>; -static vk::UniqueImage CreateVKImageWrapperForAndroidHarwareBuffer( +vk::UniqueImage CreateVKImageWrapperForAndroidHarwareBuffer( const vk::Device& device, const AHBProperties& ahb_props, const AHardwareBuffer_Desc& ahb_desc) { @@ -94,7 +111,7 @@ static vk::UniqueImage CreateVKImageWrapperForAndroidHarwareBuffer( return std::move(image.value); } -static vk::UniqueDeviceMemory ImportVKDeviceMemoryFromAndroidHarwareBuffer( +vk::UniqueDeviceMemory ImportVKDeviceMemoryFromAndroidHarwareBuffer( const vk::Device& device, const vk::PhysicalDevice& physical_device, const vk::Image& image, @@ -138,7 +155,7 @@ static vk::UniqueDeviceMemory ImportVKDeviceMemoryFromAndroidHarwareBuffer( return std::move(device_memory.value); } -static std::shared_ptr CreateYUVConversion( +std::shared_ptr CreateYUVConversion( const ContextVK& context, const AHBProperties& ahb_props) { YUVConversionDescriptorVK conversion_chain; @@ -176,10 +193,10 @@ static std::shared_ptr CreateYUVConversion( return context.GetYUVConversionLibrary()->GetConversion(conversion_chain); } -static vk::UniqueImageView CreateVKImageView( +vk::UniqueImageView CreateVKImageView( const vk::Device& device, const vk::Image& image, - const vk::SamplerYcbcrConversion& yuv_conversion, + const std::shared_ptr& yuv_conversion_wrapper, const AHBProperties& ahb_props, const AHardwareBuffer_Desc& ahb_desc) { const auto& ahb_format = @@ -205,9 +222,10 @@ static vk::UniqueImageView CreateVKImageView( view_info.subresourceRange.layerCount = ahb_desc.layers; // We need a custom YUV conversion only if we don't recognize the format. - if (view_info.format == vk::Format::eUndefined) { + if (view_info.format == vk::Format::eUndefined || + RequiresYCBCRConversion(view_info.format)) { view_chain.get().conversion = - yuv_conversion; + yuv_conversion_wrapper->GetConversion(); } else { view_chain.unlink(); } @@ -222,7 +240,7 @@ static vk::UniqueImageView CreateVKImageView( return std::move(image_view.value); } -static PixelFormat ToPixelFormat(AHardwareBuffer_Format format) { +PixelFormat ToPixelFormat(AHardwareBuffer_Format format) { switch (format) { case AHARDWAREBUFFER_FORMAT_R8G8B8A8_UNORM: return PixelFormat::kR8G8B8A8UNormInt; @@ -256,7 +274,7 @@ static PixelFormat ToPixelFormat(AHardwareBuffer_Format format) { return PixelFormat::kR8G8B8A8UNormInt; } -static TextureType ToTextureType(const AHardwareBuffer_Desc& ahb_desc) { +TextureType ToTextureType(const AHardwareBuffer_Desc& ahb_desc) { if (ahb_desc.layers == 1u) { return TextureType::kTexture2D; } @@ -269,8 +287,7 @@ static TextureType ToTextureType(const AHardwareBuffer_Desc& ahb_desc) { return TextureType::kTexture2D; } -static TextureDescriptor ToTextureDescriptor( - const AHardwareBuffer_Desc& ahb_desc) { +TextureDescriptor ToTextureDescriptor(const AHardwareBuffer_Desc& ahb_desc) { const auto ahb_size = ISize{ahb_desc.width, ahb_desc.height}; TextureDescriptor desc; // We are not going to touch hardware buffers on the CPU or use them as @@ -290,6 +307,7 @@ static TextureDescriptor ToTextureDescriptor( } return desc; } +} // namespace AHBTextureSourceVK::AHBTextureSourceVK( const std::shared_ptr& p_context, @@ -339,23 +357,27 @@ AHBTextureSourceVK::AHBTextureSourceVK( } // Figure out how to perform YUV conversions. - auto yuv_conversion = CreateYUVConversion(context, ahb_props); - if (!yuv_conversion || !yuv_conversion->IsValid()) { - return; + needs_yuv_conversion_ = ahb_format.format == vk::Format::eUndefined || + RequiresYCBCRConversion(ahb_format.format); + std::shared_ptr yuv_conversion; + if (needs_yuv_conversion_) { + yuv_conversion = CreateYUVConversion(context, ahb_props); + if (!yuv_conversion || !yuv_conversion->IsValid()) { + return; + } } // Create image view for the newly created image. - auto image_view = CreateVKImageView(device, // - image.get(), // - yuv_conversion->GetConversion(), // - ahb_props, // - ahb_desc // + auto image_view = CreateVKImageView(device, // + image.get(), // + yuv_conversion, // + ahb_props, // + ahb_desc // ); if (!image_view) { return; } - needs_yuv_conversion_ = ahb_format.format == vk::Format::eUndefined; device_memory_ = std::move(device_memory); image_ = std::move(image); yuv_conversion_ = std::move(yuv_conversion); @@ -364,7 +386,10 @@ AHBTextureSourceVK::AHBTextureSourceVK( #ifdef IMPELLER_DEBUG context.SetDebugName(device_memory_.get(), "AHB Device Memory"); context.SetDebugName(image_.get(), "AHB Image"); - context.SetDebugName(yuv_conversion_->GetConversion(), "AHB YUV Conversion"); + if (yuv_conversion_) { + context.SetDebugName(yuv_conversion_->GetConversion(), + "AHB YUV Conversion"); + } context.SetDebugName(image_view_.get(), "AHB ImageView"); #endif // IMPELLER_DEBUG diff --git a/engine/src/flutter/impeller/renderer/backend/vulkan/android/ahb_texture_source_vk_unittests.cc b/engine/src/flutter/impeller/renderer/backend/vulkan/android/ahb_texture_source_vk_unittests.cc new file mode 100644 index 0000000000..4e40da07f5 --- /dev/null +++ b/engine/src/flutter/impeller/renderer/backend/vulkan/android/ahb_texture_source_vk_unittests.cc @@ -0,0 +1,97 @@ +// 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 +#include + +#include "flutter/testing/testing.h" +#include "gtest/gtest.h" +#include "impeller/renderer/backend/vulkan/android/ahb_texture_source_vk.h" +#include "impeller/renderer/backend/vulkan/context_vk.h" +#include "impeller/renderer/backend/vulkan/surface_context_vk.h" +#include "impeller/toolkit/android/hardware_buffer.h" +#include "impeller/toolkit/android/surface_transaction.h" + +namespace impeller::android::testing { + +// Set up context. +std::shared_ptr CreateContext() { + auto vulkan_dylib = fml::NativeLibrary::Create("libvulkan.so"); + auto instance_proc_addr = + vulkan_dylib->ResolveFunction( + "vkGetInstanceProcAddr"); + + if (!instance_proc_addr.has_value()) { + VALIDATION_LOG << "Could not setup Vulkan proc table."; + return nullptr; + } + + impeller::ContextVK::Settings settings; + settings.proc_address_callback = instance_proc_addr.value(); + settings.shader_libraries_data = {}; + settings.enable_validation = false; + settings.enable_gpu_tracing = false; + settings.enable_surface_control = false; + + return ContextVK::Create(std::move(settings)); +} + +TEST(AndroidVulkanTest, CanImportRGBA) { + if (!HardwareBuffer::IsAvailableOnPlatform()) { + GTEST_SKIP() << "Hardware buffers are not supported on this platform."; + } + + HardwareBufferDescriptor desc; + desc.size = ISize{1, 1}; + desc.format = HardwareBufferFormat::kR8G8B8A8UNormInt; + desc.usage = HardwareBufferUsageFlags::kSampledImage; + + auto ahb = std::make_unique(desc); + ASSERT_TRUE(ahb); + auto context_vk = CreateContext(); + ASSERT_TRUE(context_vk); + + AHBTextureSourceVK source(context_vk, std::move(ahb), + /*is_swapchain_image=*/false); + + EXPECT_TRUE(source.IsValid()); + EXPECT_EQ(source.GetYUVConversion(), nullptr); + + context_vk->Shutdown(); +} + +TEST(AndroidVulkanTest, CanImportWithYUB) { + if (!HardwareBuffer::IsAvailableOnPlatform()) { + GTEST_SKIP() << "Hardware buffers are not supported on this platform."; + } + + AHardwareBuffer_Desc desc; + desc.width = 16; + desc.height = 16; + desc.format = AHARDWAREBUFFER_FORMAT_Y8Cb8Cr8_420; + desc.stride = 0; + desc.layers = 1; + desc.rfu0 = 0; + desc.rfu1 = 0; + desc.usage = AHARDWAREBUFFER_USAGE_GPU_SAMPLED_IMAGE | + AHARDWAREBUFFER_USAGE_CPU_WRITE_MASK | + AHARDWAREBUFFER_USAGE_CPU_READ_MASK; + + EXPECT_EQ(AHardwareBuffer_isSupported(&desc), 1); + + AHardwareBuffer* buffer = nullptr; + ASSERT_EQ(AHardwareBuffer_allocate(&desc, &buffer), 0); + + auto context_vk = CreateContext(); + ASSERT_TRUE(context_vk); + + AHBTextureSourceVK source(context_vk, buffer, desc); + + EXPECT_TRUE(source.IsValid()); + EXPECT_NE(source.GetYUVConversion(), nullptr); + + context_vk->Shutdown(); +} + +} // namespace impeller::android::testing diff --git a/engine/src/flutter/impeller/renderer/backend/vulkan/capabilities_vk.cc b/engine/src/flutter/impeller/renderer/backend/vulkan/capabilities_vk.cc index 6e59d9f678..7cd75c7063 100644 --- a/engine/src/flutter/impeller/renderer/backend/vulkan/capabilities_vk.cc +++ b/engine/src/flutter/impeller/renderer/backend/vulkan/capabilities_vk.cc @@ -298,6 +298,17 @@ CapabilitiesVK::GetEnabledDeviceExtensions( return true; }; + auto for_each_optional_android_extension = + [&](OptionalAndroidDeviceExtensionVK ext) { +#ifdef FML_OS_ANDROID + auto name = GetExtensionName(ext); + if (exts.find(name) != exts.end()) { + enabled.push_back(name); + } +#endif // FML_OS_ANDROID + return true; + }; + auto for_each_optional_extension = [&](OptionalDeviceExtensionVK ext) { auto name = GetExtensionName(ext); if (exts.find(name) != exts.end()) { @@ -311,7 +322,10 @@ CapabilitiesVK::GetEnabledDeviceExtensions( for_each_common_extension) && IterateExtensions( for_each_android_extension) && - IterateExtensions(for_each_optional_extension); + IterateExtensions( + for_each_optional_extension) && + IterateExtensions( + for_each_optional_android_extension); if (!iterate_extensions) { VALIDATION_LOG << "Device not suitable since required extensions are not " diff --git a/engine/src/flutter/impeller/renderer/backend/vulkan/pipeline_cache_data_vk.cc b/engine/src/flutter/impeller/renderer/backend/vulkan/pipeline_cache_data_vk.cc index 9cdd199aed..ecb46b7831 100644 --- a/engine/src/flutter/impeller/renderer/backend/vulkan/pipeline_cache_data_vk.cc +++ b/engine/src/flutter/impeller/renderer/backend/vulkan/pipeline_cache_data_vk.cc @@ -36,9 +36,13 @@ bool PipelineCacheDataPersist(const fml::UniqueFD& cache_directory, } const auto header = PipelineCacheHeaderVK{props, data_size}; std::memcpy(allocation->GetBuffer(), &header, sizeof(header)); - if (cache.getOwner().getPipelineCacheData( - *cache, &data_size, allocation->GetBuffer() + sizeof(header)) != - vk::Result::eSuccess) { + vk::Result lookup_result = cache.getOwner().getPipelineCacheData( + *cache, &data_size, allocation->GetBuffer() + sizeof(header)); + + // Some drivers may return incomplete erroneously, but this is not an + // error condition as some/all data was still written. + if (lookup_result != vk::Result::eSuccess && + lookup_result != vk::Result::eIncomplete) { VALIDATION_LOG << "Could not copy pipeline cache data."; return false; } diff --git a/engine/src/flutter/testing/run_tests.py b/engine/src/flutter/testing/run_tests.py index 062b4b0231..8cd8776181 100755 --- a/engine/src/flutter/testing/run_tests.py +++ b/engine/src/flutter/testing/run_tests.py @@ -765,6 +765,7 @@ def run_android_tests(android_variant='android_debug_unopt', adb_path=None): run_android_unittest('flutter_shell_native_unittests', android_variant, adb_path) run_android_unittest('impeller_toolkit_android_unittests', android_variant, adb_path) + run_android_unittest('impeller_vulkan_android_unittests', android_variant, adb_path) def run_objc_tests(ios_variant='ios_debug_sim_unopt', test_filter=None):