From e357d1899d52846134e8965ae58a28f4e97b8c05 Mon Sep 17 00:00:00 2001 From: Jonah Williams Date: Mon, 28 Oct 2024 13:10:10 -0700 Subject: [PATCH] [Impeller] fix initial layout for loadOp load and incorrect usage of host visible textures. (flutter/engine#56148) Both changes were required to get playground tests validation free with moltenvk. becuase an initial state of undefined means "I don't care what was in this texture before" but that doesn't make sense if we're setting "kLoad" since that explicitly asks vulkan to load the previous contents. Fixes https://github.com/flutter/flutter/issues/157557 --- .../playground/imgui/imgui_impl_impeller.cc | 21 +++++++++++++--- .../flutter/impeller/playground/playground.cc | 8 +++--- .../backend/vulkan/render_pass_builder_vk.cc | 11 ++++++-- .../backend/vulkan/render_pass_builder_vk.h | 12 +++++---- .../render_pass_builder_vk_unittests.cc | 25 ++++++++++++++++++- .../renderer/backend/vulkan/render_pass_vk.cc | 3 ++- 6 files changed, 63 insertions(+), 17 deletions(-) diff --git a/engine/src/flutter/impeller/playground/imgui/imgui_impl_impeller.cc b/engine/src/flutter/impeller/playground/imgui/imgui_impl_impeller.cc index f6fd6c33f95..44af6fddbd4 100644 --- a/engine/src/flutter/impeller/playground/imgui/imgui_impl_impeller.cc +++ b/engine/src/flutter/impeller/playground/imgui/imgui_impl_impeller.cc @@ -9,6 +9,7 @@ #include #include +#include "fml/mapping.h" #include "impeller/core/buffer_view.h" #include "impeller/core/host_buffer.h" #include "impeller/core/platform.h" @@ -78,8 +79,8 @@ bool ImGui_ImplImpeller_Init( int width, height; io.Fonts->GetTexDataAsRGBA32(&pixels, &width, &height); - auto texture_descriptor = impeller::TextureDescriptor{}; - texture_descriptor.storage_mode = impeller::StorageMode::kHostVisible; + impeller::TextureDescriptor texture_descriptor; + texture_descriptor.storage_mode = impeller::StorageMode::kDevicePrivate; texture_descriptor.format = impeller::PixelFormat::kR8G8B8A8UNormInt; texture_descriptor.size = {width, height}; texture_descriptor.mip_count = 1u; @@ -90,8 +91,20 @@ bool ImGui_ImplImpeller_Init( "Could not allocate ImGui font texture."); bd->font_texture->SetLabel("ImGui Font Texture"); - [[maybe_unused]] bool uploaded = bd->font_texture->SetContents( - pixels, texture_descriptor.GetByteSizeOfBaseMipLevel()); + auto command_buffer = context->CreateCommandBuffer(); + auto blit_pass = command_buffer->CreateBlitPass(); + auto mapping = std::make_shared( + reinterpret_cast(pixels), + texture_descriptor.GetByteSizeOfBaseMipLevel()); + auto device_buffer = + context->GetResourceAllocator()->CreateBufferWithCopy(*mapping); + + blit_pass->AddCopy(impeller::DeviceBuffer::AsBufferView(device_buffer), + bd->font_texture); + blit_pass->EncodeCommands(context->GetResourceAllocator()); + + [[maybe_unused]] bool uploaded = + context->GetCommandQueue()->Submit({command_buffer}).ok(); IM_ASSERT(uploaded && "Could not upload ImGui font texture to device memory."); } diff --git a/engine/src/flutter/impeller/playground/playground.cc b/engine/src/flutter/impeller/playground/playground.cc index 880bed4feb1..7f6521b5395 100644 --- a/engine/src/flutter/impeller/playground/playground.cc +++ b/engine/src/flutter/impeller/playground/playground.cc @@ -388,8 +388,8 @@ static std::shared_ptr CreateTextureForDecompressedImage( const std::shared_ptr& context, DecompressedImage& decompressed_image, bool enable_mipmapping) { - auto texture_descriptor = TextureDescriptor{}; - texture_descriptor.storage_mode = StorageMode::kHostVisible; + TextureDescriptor texture_descriptor; + texture_descriptor.storage_mode = StorageMode::kDevicePrivate; texture_descriptor.format = PixelFormat::kR8G8B8A8UNormInt; texture_descriptor.size = decompressed_image.GetSize(); texture_descriptor.mip_count = @@ -463,8 +463,8 @@ std::shared_ptr Playground::CreateTextureCubeForFixture( images[i] = image.value(); } - auto texture_descriptor = TextureDescriptor{}; - texture_descriptor.storage_mode = StorageMode::kHostVisible; + TextureDescriptor texture_descriptor; + texture_descriptor.storage_mode = StorageMode::kDevicePrivate; texture_descriptor.type = TextureType::kTextureCube; texture_descriptor.format = PixelFormat::kR8G8B8A8UNormInt; texture_descriptor.size = images[0].GetSize(); diff --git a/engine/src/flutter/impeller/renderer/backend/vulkan/render_pass_builder_vk.cc b/engine/src/flutter/impeller/renderer/backend/vulkan/render_pass_builder_vk.cc index 1433f3811ba..77f5a3a1e9b 100644 --- a/engine/src/flutter/impeller/renderer/backend/vulkan/render_pass_builder_vk.cc +++ b/engine/src/flutter/impeller/renderer/backend/vulkan/render_pass_builder_vk.cc @@ -6,7 +6,9 @@ #include +#include "impeller/core/formats.h" #include "impeller/renderer/backend/vulkan/formats_vk.h" +#include "vulkan/vulkan_enums.hpp" namespace impeller { @@ -31,7 +33,8 @@ RenderPassBuilderVK& RenderPassBuilderVK::SetColorAttachment( PixelFormat format, SampleCount sample_count, LoadAction load_action, - StoreAction store_action) { + StoreAction store_action, + vk::ImageLayout current_layout) { vk::AttachmentDescription desc; desc.format = ToVKImageFormat(format); desc.samples = ToVKSampleCount(sample_count); @@ -39,7 +42,11 @@ RenderPassBuilderVK& RenderPassBuilderVK::SetColorAttachment( desc.storeOp = ToVKAttachmentStoreOp(store_action, false); desc.stencilLoadOp = vk::AttachmentLoadOp::eDontCare; desc.stencilStoreOp = vk::AttachmentStoreOp::eDontCare; - desc.initialLayout = vk::ImageLayout::eUndefined; + if (load_action == LoadAction::kLoad) { + desc.initialLayout = current_layout; + } else { + desc.initialLayout = vk::ImageLayout::eUndefined; + } desc.finalLayout = vk::ImageLayout::eGeneral; colors_[index] = desc; diff --git a/engine/src/flutter/impeller/renderer/backend/vulkan/render_pass_builder_vk.h b/engine/src/flutter/impeller/renderer/backend/vulkan/render_pass_builder_vk.h index 4ef1924e8fe..c3ed390e961 100644 --- a/engine/src/flutter/impeller/renderer/backend/vulkan/render_pass_builder_vk.h +++ b/engine/src/flutter/impeller/renderer/backend/vulkan/render_pass_builder_vk.h @@ -24,11 +24,13 @@ class RenderPassBuilderVK { RenderPassBuilderVK& operator=(const RenderPassBuilderVK&) = delete; - RenderPassBuilderVK& SetColorAttachment(size_t index, - PixelFormat format, - SampleCount sample_count, - LoadAction load_action, - StoreAction store_action); + RenderPassBuilderVK& SetColorAttachment( + size_t index, + PixelFormat format, + SampleCount sample_count, + LoadAction load_action, + StoreAction store_action, + vk::ImageLayout current_layout = vk::ImageLayout::eUndefined); RenderPassBuilderVK& SetDepthStencilAttachment(PixelFormat format, SampleCount sample_count, diff --git a/engine/src/flutter/impeller/renderer/backend/vulkan/render_pass_builder_vk_unittests.cc b/engine/src/flutter/impeller/renderer/backend/vulkan/render_pass_builder_vk_unittests.cc index 325dd826e5f..658ae2f54ac 100644 --- a/engine/src/flutter/impeller/renderer/backend/vulkan/render_pass_builder_vk_unittests.cc +++ b/engine/src/flutter/impeller/renderer/backend/vulkan/render_pass_builder_vk_unittests.cc @@ -27,6 +27,29 @@ TEST(RenderPassBuilder, CreatesRenderPassWithNoDepthStencil) { EXPECT_FALSE(builder.GetDepthStencil().has_value()); } +TEST(RenderPassBuilder, RenderPassWithLoadOpUsesCurrentLayout) { + RenderPassBuilderVK builder = RenderPassBuilderVK(); + auto const context = MockVulkanContextBuilder().Build(); + + builder.SetColorAttachment(0, PixelFormat::kR8G8B8A8UNormInt, + SampleCount::kCount1, LoadAction::kLoad, + StoreAction::kStore, + vk::ImageLayout::eColorAttachmentOptimal); + + auto render_pass = builder.Build(context->GetDevice()); + + EXPECT_TRUE(!!render_pass); + + auto maybe_color = builder.GetColorAttachments().find(0u); + ASSERT_NE(maybe_color, builder.GetColorAttachments().end()); + auto color = maybe_color->second; + + EXPECT_EQ(color.initialLayout, vk::ImageLayout::eColorAttachmentOptimal); + EXPECT_EQ(color.finalLayout, vk::ImageLayout::eGeneral); + EXPECT_EQ(color.loadOp, vk::AttachmentLoadOp::eLoad); + EXPECT_EQ(color.storeOp, vk::AttachmentStoreOp::eStore); +} + TEST(RenderPassBuilder, CreatesRenderPassWithCombinedDepthStencil) { RenderPassBuilderVK builder = RenderPassBuilderVK(); auto const context = MockVulkanContextBuilder().Build(); @@ -34,7 +57,7 @@ TEST(RenderPassBuilder, CreatesRenderPassWithCombinedDepthStencil) { // Create a single color attachment with a transient depth stencil. builder.SetColorAttachment(0, PixelFormat::kR8G8B8A8UNormInt, SampleCount::kCount1, LoadAction::kClear, - StoreAction::kStore); + StoreAction::kStore, vk::ImageLayout::eGeneral); builder.SetDepthStencilAttachment(PixelFormat::kD24UnormS8Uint, SampleCount::kCount1, LoadAction::kDontCare, StoreAction::kDontCare); diff --git a/engine/src/flutter/impeller/renderer/backend/vulkan/render_pass_vk.cc b/engine/src/flutter/impeller/renderer/backend/vulkan/render_pass_vk.cc index d9c01e1add0..ffb3f05fb6f 100644 --- a/engine/src/flutter/impeller/renderer/backend/vulkan/render_pass_vk.cc +++ b/engine/src/flutter/impeller/renderer/backend/vulkan/render_pass_vk.cc @@ -89,7 +89,8 @@ SharedHandleVK RenderPassVK::CreateVKRenderPass( color.texture->GetTextureDescriptor().format, // color.texture->GetTextureDescriptor().sample_count, // color.load_action, // - color.store_action // + color.store_action, // + TextureVK::Cast(*color.texture).GetLayout() // ); TextureVK::Cast(*color.texture) .SetLayoutWithoutEncoding(vk::ImageLayout::eGeneral);