From ae00dada53a5b96a7b103c5a573eeb506d679268 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Thu, 7 Mar 2024 13:00:55 -0800 Subject: [PATCH 1/5] [Impeller] fix heap selection process for YUV textures. --- ...droid_hardware_buffer_texture_source_vk.cc | 36 ++++++++++++++----- .../android/image_external_texture_vk.cc | 4 +++ 2 files changed, 31 insertions(+), 9 deletions(-) diff --git a/impeller/renderer/backend/vulkan/android_hardware_buffer_texture_source_vk.cc b/impeller/renderer/backend/vulkan/android_hardware_buffer_texture_source_vk.cc index 0ec9e2725ff02..484cf73975c46 100644 --- a/impeller/renderer/backend/vulkan/android_hardware_buffer_texture_source_vk.cc +++ b/impeller/renderer/backend/vulkan/android_hardware_buffer_texture_source_vk.cc @@ -94,25 +94,42 @@ static vk::UniqueImage CreateVKImageWrapperForAndroidHarwareBuffer( // Returns -1 if not found. static int FindMemoryTypeIndex( - const vk::AndroidHardwareBufferPropertiesANDROID& props) { - uint32_t memory_type_bits = props.memoryTypeBits; + const vk::AndroidHardwareBufferPropertiesANDROID& props, + vk::PhysicalDeviceMemoryProperties& memory_properties) { int32_t type_index = -1; - for (uint32_t i = 0; memory_type_bits; - memory_type_bits = memory_type_bits >> 0x1, ++i) { - if (memory_type_bits & 0x1) { - type_index = i; - break; + uint32_t memory_type_bits_requirement = props.memoryTypeBits; + vk::MemoryPropertyFlagBits required_properties = + vk::MemoryPropertyFlagBits::eDeviceLocal; + + const uint32_t memory_count = memory_properties.memoryTypeCount; + for (uint32_t memory_index = 0; memory_index < memory_count; ++memory_index) { + const uint32_t memory_type_bits = (1 << memory_index); + const bool is_required_memory_type = + memory_type_bits_requirement & memory_type_bits; + + const auto properties = + memory_properties.memoryTypes[memory_index].propertyFlags; + const bool has_required_properties = + (properties & required_properties) == required_properties; + + if (is_required_memory_type && has_required_properties) { + return static_cast(memory_index); } } + return type_index; } static vk::UniqueDeviceMemory ImportVKDeviceMemoryFromAndroidHarwareBuffer( const vk::Device& device, + const vk::PhysicalDevice& physical_device, const vk::Image& image, struct AHardwareBuffer* hardware_buffer, const AHBProperties& ahb_props) { - const int memory_type_index = FindMemoryTypeIndex(ahb_props.get()); + vk::PhysicalDeviceMemoryProperties memory_properties; + physical_device.getMemoryProperties(&memory_properties); + int memory_type_index = + FindMemoryTypeIndex(ahb_props.get(), memory_properties); if (memory_type_index < 0) { VALIDATION_LOG << "Could not find memory type of external image."; return {}; @@ -304,6 +321,7 @@ AndroidHardwareBufferTextureSourceVK::AndroidHardwareBufferTextureSourceVK( } const auto& device = context->GetDevice(); + const auto& physical_device = context->GetPhysicalDevice(); AHBProperties ahb_props; @@ -326,7 +344,7 @@ AndroidHardwareBufferTextureSourceVK::AndroidHardwareBufferTextureSourceVK( // Create a device memory allocation to refer to our external image. auto device_memory = ImportVKDeviceMemoryFromAndroidHarwareBuffer( - device, image.get(), ahb, ahb_props); + device, physical_device, image.get(), ahb, ahb_props); if (!device_memory) { return; } diff --git a/shell/platform/android/image_external_texture_vk.cc b/shell/platform/android/image_external_texture_vk.cc index 906f4a5ee0561..1c23c639d225f 100644 --- a/shell/platform/android/image_external_texture_vk.cc +++ b/shell/platform/android/image_external_texture_vk.cc @@ -57,6 +57,10 @@ void ImageExternalTextureVK::ProcessFrame(PaintContext& context, auto texture_source = std::make_shared( impeller_context_, latest_hardware_buffer, hb_desc); + if (!texture_source->IsValid()) { + CloseHardwareBuffer(hardware_buffer); + return; + } auto texture = std::make_shared(impeller_context_, texture_source); From 37620e167399fddad0bcdd9eacc0abe60c173397 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Thu, 7 Mar 2024 13:52:57 -0800 Subject: [PATCH 2/5] rename golden to see what gets generated. --- testing/scenario_app/android/README.md | 4 ++-- .../java/dev/flutter/scenariosui/ExternalTextureTests.java | 2 +- testing/scenario_app/android/expected_golden_output.txt | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/testing/scenario_app/android/README.md b/testing/scenario_app/android/README.md index c3d5e0cb7fc46..79a09d888cb5f 100644 --- a/testing/scenario_app/android/README.md +++ b/testing/scenario_app/android/README.md @@ -73,9 +73,9 @@ or [platform views](https://docs.flutter.dev/platform-integration/android/platfo and as such, the tests in this package use golden screenshot file comparisons to verify the correctness of the engine's output. -For example, in [`ExternalTextureTests_testMediaSurface`](https://flutter-engine-gold.skia.org/search?corpus=flutter-engine&include_ignored=false&left_filter=name%3DExternalTextureTests_testMediaSurface&max_rgba=0&min_rgba=0&negative=true¬_at_head=false&positive=true&reference_image_required=false&right_filter=&sort=descending&untriaged=true), a [video](app/src/main/assets/sample.mp4) is converted to an external texture and displayed in a Flutter app. The test takes a screenshot of the app and compares it to a golden file: +For example, in [`ExternalTextureTests_testMediaSurface2`](https://flutter-engine-gold.skia.org/search?corpus=flutter-engine&include_ignored=false&left_filter=name%3DExternalTextureTests_testMediaSurface2&max_rgba=0&min_rgba=0&negative=true¬_at_head=false&positive=true&reference_image_required=false&right_filter=&sort=descending&untriaged=true), a [video](app/src/main/assets/sample.mp4) is converted to an external texture and displayed in a Flutter app. The test takes a screenshot of the app and compares it to a golden file: -Two pictures, the top one Flutter and the bottom Android Date: Fri, 8 Mar 2024 12:10:22 -0800 Subject: [PATCH 3/5] revert golden changes. --- testing/scenario_app/android/README.md | 4 ++-- .../java/dev/flutter/scenariosui/ExternalTextureTests.java | 2 +- testing/scenario_app/android/expected_golden_output.txt | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/testing/scenario_app/android/README.md b/testing/scenario_app/android/README.md index 79a09d888cb5f..c3d5e0cb7fc46 100644 --- a/testing/scenario_app/android/README.md +++ b/testing/scenario_app/android/README.md @@ -73,9 +73,9 @@ or [platform views](https://docs.flutter.dev/platform-integration/android/platfo and as such, the tests in this package use golden screenshot file comparisons to verify the correctness of the engine's output. -For example, in [`ExternalTextureTests_testMediaSurface2`](https://flutter-engine-gold.skia.org/search?corpus=flutter-engine&include_ignored=false&left_filter=name%3DExternalTextureTests_testMediaSurface2&max_rgba=0&min_rgba=0&negative=true¬_at_head=false&positive=true&reference_image_required=false&right_filter=&sort=descending&untriaged=true), a [video](app/src/main/assets/sample.mp4) is converted to an external texture and displayed in a Flutter app. The test takes a screenshot of the app and compares it to a golden file: +For example, in [`ExternalTextureTests_testMediaSurface`](https://flutter-engine-gold.skia.org/search?corpus=flutter-engine&include_ignored=false&left_filter=name%3DExternalTextureTests_testMediaSurface&max_rgba=0&min_rgba=0&negative=true¬_at_head=false&positive=true&reference_image_required=false&right_filter=&sort=descending&untriaged=true), a [video](app/src/main/assets/sample.mp4) is converted to an external texture and displayed in a Flutter app. The test takes a screenshot of the app and compares it to a golden file: -Two pictures, the top one Flutter and the bottom Android Date: Fri, 8 Mar 2024 12:38:05 -0800 Subject: [PATCH 4/5] add testing and refactor lookup into allocator. --- .../renderer/backend/vulkan/allocator_vk.cc | 26 +++++++++++++ .../renderer/backend/vulkan/allocator_vk.h | 11 ++++-- .../backend/vulkan/allocator_vk_unittests.cc | 38 +++++++++++++++++++ ...droid_hardware_buffer_texture_source_vk.cc | 33 ++-------------- 4 files changed, 75 insertions(+), 33 deletions(-) diff --git a/impeller/renderer/backend/vulkan/allocator_vk.cc b/impeller/renderer/backend/vulkan/allocator_vk.cc index 851d78e48716b..5dcadbe9df40e 100644 --- a/impeller/renderer/backend/vulkan/allocator_vk.cc +++ b/impeller/renderer/backend/vulkan/allocator_vk.cc @@ -170,6 +170,32 @@ ISize AllocatorVK::GetMaxTextureSizeSupported() const { return max_texture_size_; } +int32_t AllocatorVK::FindMemoryTypeIndex( + uint32_t memory_type_bits_requirement, + vk::PhysicalDeviceMemoryProperties& memory_properties) { + int32_t type_index = -1; + vk::MemoryPropertyFlagBits required_properties = + vk::MemoryPropertyFlagBits::eDeviceLocal; + + const uint32_t memory_count = memory_properties.memoryTypeCount; + for (uint32_t memory_index = 0; memory_index < memory_count; ++memory_index) { + const uint32_t memory_type_bits = (1 << memory_index); + const bool is_required_memory_type = + memory_type_bits_requirement & memory_type_bits; + + const auto properties = + memory_properties.memoryTypes[memory_index].propertyFlags; + const bool has_required_properties = + (properties & required_properties) == required_properties; + + if (is_required_memory_type && has_required_properties) { + return static_cast(memory_index); + } + } + + return type_index; +} + static constexpr vk::ImageUsageFlags ToVKImageUsageFlags( PixelFormat format, TextureUsageMask usage, diff --git a/impeller/renderer/backend/vulkan/allocator_vk.h b/impeller/renderer/backend/vulkan/allocator_vk.h index 339e496ffd7db..a19834466ee55 100644 --- a/impeller/renderer/backend/vulkan/allocator_vk.h +++ b/impeller/renderer/backend/vulkan/allocator_vk.h @@ -5,15 +5,12 @@ #ifndef FLUTTER_IMPELLER_RENDERER_BACKEND_VULKAN_ALLOCATOR_VK_H_ #define FLUTTER_IMPELLER_RENDERER_BACKEND_VULKAN_ALLOCATOR_VK_H_ -#include "flutter/fml/macros.h" -#include "flutter/fml/memory/ref_ptr.h" #include "impeller/core/allocator.h" #include "impeller/renderer/backend/vulkan/context_vk.h" #include "impeller/renderer/backend/vulkan/device_buffer_vk.h" #include "impeller/renderer/backend/vulkan/device_holder.h" #include "impeller/renderer/backend/vulkan/vk.h" -#include #include #include @@ -27,6 +24,14 @@ class AllocatorVK final : public Allocator { // Visible for testing size_t DebugGetHeapUsage() const; + /// @brief Select a matching memory type for the given + /// [memory_type_bits_requirement], or -1 if none is found. + /// + /// This only returns memory types with deviceLocal allocations. + static int32_t FindMemoryTypeIndex( + uint32_t memory_type_bits_requirement, + vk::PhysicalDeviceMemoryProperties& memory_properties); + private: friend class ContextVK; diff --git a/impeller/renderer/backend/vulkan/allocator_vk_unittests.cc b/impeller/renderer/backend/vulkan/allocator_vk_unittests.cc index 26856987e20de..1b0b85871231c 100644 --- a/impeller/renderer/backend/vulkan/allocator_vk_unittests.cc +++ b/impeller/renderer/backend/vulkan/allocator_vk_unittests.cc @@ -13,6 +13,44 @@ namespace impeller { namespace testing { +TEST(AllocatorVKTest, MemoryTypeSelectionSingleHeap) { + vk::PhysicalDeviceMemoryProperties properties; + properties.memoryTypeCount = 1; + properties.memoryHeapCount = 1; + properties.memoryTypes[0].heapIndex = 0; + properties.memoryTypes[0].propertyFlags = + vk::MemoryPropertyFlagBits::eDeviceLocal; + properties.memoryHeaps[0].size = 1024 * 1024 * 1024; + properties.memoryHeaps[0].flags = vk::MemoryHeapFlagBits::eDeviceLocal; + + EXPECT_EQ(AllocatorVK::FindMemoryTypeIndex(1, properties), 0); + EXPECT_EQ(AllocatorVK::FindMemoryTypeIndex(2, properties), -1); + EXPECT_EQ(AllocatorVK::FindMemoryTypeIndex(3, properties), 0); +} + +TEST(AllocatorVKTest, MemoryTypeSelectionTwoHeap) { + vk::PhysicalDeviceMemoryProperties properties; + properties.memoryTypeCount = 2; + properties.memoryHeapCount = 2; + properties.memoryTypes[0].heapIndex = 0; + properties.memoryTypes[0].propertyFlags = + vk::MemoryPropertyFlagBits::eHostVisible; + properties.memoryHeaps[0].size = 1024 * 1024 * 1024; + properties.memoryHeaps[0].flags = vk::MemoryHeapFlagBits::eDeviceLocal; + + properties.memoryTypes[1].heapIndex = 1; + properties.memoryTypes[1].propertyFlags = + vk::MemoryPropertyFlagBits::eDeviceLocal; + properties.memoryHeaps[1].size = 1024 * 1024 * 1024; + properties.memoryHeaps[1].flags = vk::MemoryHeapFlagBits::eDeviceLocal; + + // First fails because this only looks for eDeviceLocal. + EXPECT_EQ(AllocatorVK::FindMemoryTypeIndex(1, properties), -1); + EXPECT_EQ(AllocatorVK::FindMemoryTypeIndex(2, properties), 1); + EXPECT_EQ(AllocatorVK::FindMemoryTypeIndex(3, properties), 1); + EXPECT_EQ(AllocatorVK::FindMemoryTypeIndex(4, properties), -1); +} + #ifdef IMPELLER_DEBUG TEST(AllocatorVKTest, RecreateSwapchainWhenSizeChanges) { diff --git a/impeller/renderer/backend/vulkan/android_hardware_buffer_texture_source_vk.cc b/impeller/renderer/backend/vulkan/android_hardware_buffer_texture_source_vk.cc index 484cf73975c46..98b42324464ca 100644 --- a/impeller/renderer/backend/vulkan/android_hardware_buffer_texture_source_vk.cc +++ b/impeller/renderer/backend/vulkan/android_hardware_buffer_texture_source_vk.cc @@ -6,6 +6,7 @@ #include +#include "impeller/renderer/backend/vulkan/allocator_vk.h" #include "impeller/renderer/backend/vulkan/context_vk.h" #include "impeller/renderer/backend/vulkan/texture_source_vk.h" #include "impeller/renderer/backend/vulkan/yuv_conversion_library_vk.h" @@ -92,34 +93,6 @@ static vk::UniqueImage CreateVKImageWrapperForAndroidHarwareBuffer( return std::move(image.value); } -// Returns -1 if not found. -static int FindMemoryTypeIndex( - const vk::AndroidHardwareBufferPropertiesANDROID& props, - vk::PhysicalDeviceMemoryProperties& memory_properties) { - int32_t type_index = -1; - uint32_t memory_type_bits_requirement = props.memoryTypeBits; - vk::MemoryPropertyFlagBits required_properties = - vk::MemoryPropertyFlagBits::eDeviceLocal; - - const uint32_t memory_count = memory_properties.memoryTypeCount; - for (uint32_t memory_index = 0; memory_index < memory_count; ++memory_index) { - const uint32_t memory_type_bits = (1 << memory_index); - const bool is_required_memory_type = - memory_type_bits_requirement & memory_type_bits; - - const auto properties = - memory_properties.memoryTypes[memory_index].propertyFlags; - const bool has_required_properties = - (properties & required_properties) == required_properties; - - if (is_required_memory_type && has_required_properties) { - return static_cast(memory_index); - } - } - - return type_index; -} - static vk::UniqueDeviceMemory ImportVKDeviceMemoryFromAndroidHarwareBuffer( const vk::Device& device, const vk::PhysicalDevice& physical_device, @@ -128,8 +101,8 @@ static vk::UniqueDeviceMemory ImportVKDeviceMemoryFromAndroidHarwareBuffer( const AHBProperties& ahb_props) { vk::PhysicalDeviceMemoryProperties memory_properties; physical_device.getMemoryProperties(&memory_properties); - int memory_type_index = - FindMemoryTypeIndex(ahb_props.get(), memory_properties); + int memory_type_index = AllocatorVK::FindMemoryTypeIndex( + ahb_props.get().memoryTypeBits, memory_properties); if (memory_type_index < 0) { VALIDATION_LOG << "Could not find memory type of external image."; return {}; From b5b90b03569339a80fefab09dec616b75e08ea3e Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Wed, 13 Mar 2024 10:38:06 -0700 Subject: [PATCH 5/5] just try stuff. --- .../backend/vulkan/android/ahb_texture_source_vk.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/impeller/renderer/backend/vulkan/android/ahb_texture_source_vk.h b/impeller/renderer/backend/vulkan/android/ahb_texture_source_vk.h index 14c027b901d89..c36a7c5652e4e 100644 --- a/impeller/renderer/backend/vulkan/android/ahb_texture_source_vk.h +++ b/impeller/renderer/backend/vulkan/android/ahb_texture_source_vk.h @@ -58,10 +58,10 @@ class AHBTextureSourceVK final : public TextureSourceVK { std::shared_ptr GetYUVConversion() const override; private: - vk::UniqueDeviceMemory device_memory_; - vk::UniqueImage image_; - vk::UniqueImageView image_view_; - std::shared_ptr yuv_conversion_; + vk::UniqueDeviceMemory device_memory_ = {}; + vk::UniqueImage image_ = {}; + vk::UniqueImageView image_view_ = {}; + std::shared_ptr yuv_conversion_ = {}; bool needs_yuv_conversion_ = false; bool is_valid_ = false;