From dbfffa939a0e97f834ec671b6f8fa1be8e47c57b Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Wed, 27 Mar 2024 12:49:17 -0700 Subject: [PATCH 1/3] [Impeller] use optimal depth attachment, remove useless barrier. --- impeller/renderer/backend/vulkan/BUILD.gn | 1 + .../backend/vulkan/render_pass_builder_vk.cc | 23 ++++- .../backend/vulkan/render_pass_builder_vk.h | 9 ++ .../render_pass_builder_vk_unittests.cc | 98 +++++++++++++++++++ .../renderer/backend/vulkan/render_pass_vk.cc | 2 - 5 files changed, 127 insertions(+), 6 deletions(-) create mode 100644 impeller/renderer/backend/vulkan/render_pass_builder_vk_unittests.cc diff --git a/impeller/renderer/backend/vulkan/BUILD.gn b/impeller/renderer/backend/vulkan/BUILD.gn index 285c655acaa09..8a00a9c77c1af 100644 --- a/impeller/renderer/backend/vulkan/BUILD.gn +++ b/impeller/renderer/backend/vulkan/BUILD.gn @@ -16,6 +16,7 @@ impeller_component("vulkan_unittests") { "descriptor_pool_vk_unittests.cc", "driver_info_vk_unittests.cc", "fence_waiter_vk_unittests.cc", + "render_pass_builder_vk_unittests.cc", "render_pass_cache_unittests.cc", "resource_manager_vk_unittests.cc", "test/gpu_tracer_unittests.cc", diff --git a/impeller/renderer/backend/vulkan/render_pass_builder_vk.cc b/impeller/renderer/backend/vulkan/render_pass_builder_vk.cc index 5edd04441553f..662c64253bb4d 100644 --- a/impeller/renderer/backend/vulkan/render_pass_builder_vk.cc +++ b/impeller/renderer/backend/vulkan/render_pass_builder_vk.cc @@ -62,8 +62,8 @@ RenderPassBuilderVK& RenderPassBuilderVK::SetDepthStencilAttachment( desc.storeOp = ToVKAttachmentStoreOp(store_action); desc.stencilLoadOp = desc.loadOp; // Not separable in Impeller. desc.stencilStoreOp = desc.storeOp; // Not separable in Impeller. - desc.initialLayout = vk::ImageLayout::eGeneral; - desc.finalLayout = vk::ImageLayout::eGeneral; + desc.initialLayout = vk::ImageLayout::eUndefined; + desc.finalLayout = vk::ImageLayout::eDepthStencilAttachmentOptimal; depth_stencil_ = desc; return *this; } @@ -80,8 +80,8 @@ RenderPassBuilderVK& RenderPassBuilderVK::SetStencilAttachment( desc.storeOp = vk::AttachmentStoreOp::eDontCare; desc.stencilLoadOp = ToVKAttachmentLoadOp(load_action); desc.stencilStoreOp = ToVKAttachmentStoreOp(store_action); - desc.initialLayout = vk::ImageLayout::eGeneral; - desc.finalLayout = vk::ImageLayout::eGeneral; + desc.initialLayout = vk::ImageLayout::eUndefined; + desc.finalLayout = vk::ImageLayout::eDepthStencilAttachmentOptimal; depth_stencil_ = desc; return *this; } @@ -184,4 +184,19 @@ void InsertBarrierForInputAttachmentRead(const vk::CommandBuffer& buffer, ); } +const std::map& +RenderPassBuilderVK::GetColors() const { + return colors_; +} + +const std::map& +RenderPassBuilderVK::GetResolves() const { + return resolves_; +} + +const std::optional& +RenderPassBuilderVK::GetDepthStencil() const { + return depth_stencil_; +} + } // namespace impeller diff --git a/impeller/renderer/backend/vulkan/render_pass_builder_vk.h b/impeller/renderer/backend/vulkan/render_pass_builder_vk.h index 4ed9f1c9c5a39..a855898bb2ed6 100644 --- a/impeller/renderer/backend/vulkan/render_pass_builder_vk.h +++ b/impeller/renderer/backend/vulkan/render_pass_builder_vk.h @@ -42,6 +42,15 @@ class RenderPassBuilderVK { vk::UniqueRenderPass Build(const vk::Device& device) const; + // Visible for testing. + const std::map& GetColors() const; + + // Visible for testing. + const std::map& GetResolves() const; + + // Visible for testing. + const std::optional& GetDepthStencil() const; + private: std::map colors_; std::map resolves_; diff --git a/impeller/renderer/backend/vulkan/render_pass_builder_vk_unittests.cc b/impeller/renderer/backend/vulkan/render_pass_builder_vk_unittests.cc new file mode 100644 index 0000000000000..229692618205a --- /dev/null +++ b/impeller/renderer/backend/vulkan/render_pass_builder_vk_unittests.cc @@ -0,0 +1,98 @@ +// 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 "flutter/testing/testing.h" // IWYU pragma: keep +#include "gtest/gtest.h" +#include "impeller/core/formats.h" +#include "impeller/renderer/backend/vulkan/render_pass_builder_vk.h" +#include "impeller/renderer/backend/vulkan/test/mock_vulkan.h" +#include "vulkan/vulkan_core.h" +#include "vulkan/vulkan_enums.hpp" + +namespace impeller { +namespace testing { + +TEST(RenderPassBuilder, CreatesRenderPassWithNoDepthStencil) { + RenderPassBuilderVK builder = RenderPassBuilderVK(); + auto const context = MockVulkanContextBuilder().Build(); + + // Create a single color attachment with a transient depth stencil. + builder.SetColorAttachment(0, PixelFormat::kR8G8B8A8UNormInt, + SampleCount::kCount1, LoadAction::kClear, + StoreAction::kStore); + + auto render_pass = builder.Build(context->GetDevice()); + + EXPECT_TRUE(!!render_pass); + EXPECT_FALSE(builder.GetDepthStencil().has_value()); +} + +TEST(RenderPassBuilder, CreatesRenderPassWithCombinedDepthStencil) { + RenderPassBuilderVK builder = RenderPassBuilderVK(); + auto const context = MockVulkanContextBuilder().Build(); + + // Create a single color attachment with a transient depth stencil. + builder.SetColorAttachment(0, PixelFormat::kR8G8B8A8UNormInt, + SampleCount::kCount1, LoadAction::kClear, + StoreAction::kStore); + builder.SetDepthStencilAttachment(PixelFormat::kD24UnormS8Uint, + SampleCount::kCount1, LoadAction::kDontCare, + StoreAction::kDontCare); + + auto render_pass = builder.Build(context->GetDevice()); + + EXPECT_TRUE(!!render_pass); + + auto maybe_color = builder.GetColors().find(0u); + ASSERT_NE(maybe_color, builder.GetColors().end()); + auto color = maybe_color->second; + + EXPECT_EQ(color.initialLayout, vk::ImageLayout::eGeneral); + EXPECT_EQ(color.finalLayout, vk::ImageLayout::eGeneral); + EXPECT_EQ(color.loadOp, vk::AttachmentLoadOp::eClear); + EXPECT_EQ(color.storeOp, vk::AttachmentStoreOp::eStore); + + auto maybe_depth_stencil = builder.GetDepthStencil(); + ASSERT_TRUE(maybe_depth_stencil.has_value()); + auto depth_stencil = maybe_depth_stencil.value(); + + EXPECT_EQ(depth_stencil.initialLayout, vk::ImageLayout::eUndefined); + EXPECT_EQ(depth_stencil.finalLayout, + vk::ImageLayout::eDepthStencilAttachmentOptimal); + EXPECT_EQ(depth_stencil.loadOp, vk::AttachmentLoadOp::eDontCare); + EXPECT_EQ(depth_stencil.storeOp, vk::AttachmentStoreOp::eDontCare); + EXPECT_EQ(depth_stencil.stencilLoadOp, vk::AttachmentLoadOp::eDontCare); + EXPECT_EQ(depth_stencil.stencilStoreOp, vk::AttachmentStoreOp::eDontCare); +} + +TEST(RenderPassBuilder, CreatesRenderPassWithOnlyStencil) { + RenderPassBuilderVK builder = RenderPassBuilderVK(); + auto const context = MockVulkanContextBuilder().Build(); + + // Create a single color attachment with a transient depth stencil. + builder.SetColorAttachment(0, PixelFormat::kR8G8B8A8UNormInt, + SampleCount::kCount1, LoadAction::kClear, + StoreAction::kStore); + builder.SetStencilAttachment(PixelFormat::kS8UInt, SampleCount::kCount1, + LoadAction::kDontCare, StoreAction::kDontCare); + + auto render_pass = builder.Build(context->GetDevice()); + + EXPECT_TRUE(!!render_pass); + + auto maybe_depth_stencil = builder.GetDepthStencil(); + ASSERT_TRUE(maybe_depth_stencil.has_value()); + auto depth_stencil = maybe_depth_stencil.value(); + + EXPECT_EQ(depth_stencil.initialLayout, vk::ImageLayout::eUndefined); + EXPECT_EQ(depth_stencil.finalLayout, + vk::ImageLayout::eDepthStencilAttachmentOptimal); + EXPECT_EQ(depth_stencil.loadOp, vk::AttachmentLoadOp::eDontCare); + EXPECT_EQ(depth_stencil.storeOp, vk::AttachmentStoreOp::eDontCare); + EXPECT_EQ(depth_stencil.stencilLoadOp, vk::AttachmentLoadOp::eDontCare); + EXPECT_EQ(depth_stencil.stencilStoreOp, vk::AttachmentStoreOp::eDontCare); +} + +} // namespace testing +} // namespace impeller diff --git a/impeller/renderer/backend/vulkan/render_pass_vk.cc b/impeller/renderer/backend/vulkan/render_pass_vk.cc index 70dedb19a59b9..ba5d68f08e838 100644 --- a/impeller/renderer/backend/vulkan/render_pass_vk.cc +++ b/impeller/renderer/backend/vulkan/render_pass_vk.cc @@ -113,7 +113,6 @@ SharedHandleVK RenderPassVK::CreateVKRenderPass( depth->load_action, // depth->store_action // ); - TextureVK::Cast(*depth->texture).SetLayout(barrier); } else if (auto stencil = render_target_.GetStencilAttachment(); stencil.has_value()) { builder.SetStencilAttachment( @@ -122,7 +121,6 @@ SharedHandleVK RenderPassVK::CreateVKRenderPass( stencil->load_action, // stencil->store_action // ); - TextureVK::Cast(*stencil->texture).SetLayout(barrier); } if (recycled_renderpass != nullptr) { From 5dea4dffc72c546033398bbf0fa0eaca228ffd78 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Wed, 27 Mar 2024 13:28:26 -0700 Subject: [PATCH 2/3] ++ --- ci/licenses_golden/excluded_files | 1 + .../backend/vulkan/render_pass_builder_vk_unittests.cc | 6 ++++++ 2 files changed, 7 insertions(+) diff --git a/ci/licenses_golden/excluded_files b/ci/licenses_golden/excluded_files index caf32b13e4d1a..d5daa38cabb8f 100644 --- a/ci/licenses_golden/excluded_files +++ b/ci/licenses_golden/excluded_files @@ -177,6 +177,7 @@ ../../../flutter/impeller/renderer/backend/vulkan/descriptor_pool_vk_unittests.cc ../../../flutter/impeller/renderer/backend/vulkan/driver_info_vk_unittests.cc ../../../flutter/impeller/renderer/backend/vulkan/fence_waiter_vk_unittests.cc +../../../flutter/impeller/renderer/backend/vulkan/render_pass_builder_vk_unittests.cc ../../../flutter/impeller/renderer/backend/vulkan/render_pass_cache_unittests.cc ../../../flutter/impeller/renderer/backend/vulkan/resource_manager_vk_unittests.cc ../../../flutter/impeller/renderer/backend/vulkan/swapchain/README.md diff --git a/impeller/renderer/backend/vulkan/render_pass_builder_vk_unittests.cc b/impeller/renderer/backend/vulkan/render_pass_builder_vk_unittests.cc index 229692618205a..dcd7df1d2cb51 100644 --- a/impeller/renderer/backend/vulkan/render_pass_builder_vk_unittests.cc +++ b/impeller/renderer/backend/vulkan/render_pass_builder_vk_unittests.cc @@ -55,6 +55,9 @@ TEST(RenderPassBuilder, CreatesRenderPassWithCombinedDepthStencil) { auto maybe_depth_stencil = builder.GetDepthStencil(); ASSERT_TRUE(maybe_depth_stencil.has_value()); + if (!maybe_depth_stencil.has_value()) { + return; + } auto depth_stencil = maybe_depth_stencil.value(); EXPECT_EQ(depth_stencil.initialLayout, vk::ImageLayout::eUndefined); @@ -83,6 +86,9 @@ TEST(RenderPassBuilder, CreatesRenderPassWithOnlyStencil) { auto maybe_depth_stencil = builder.GetDepthStencil(); ASSERT_TRUE(maybe_depth_stencil.has_value()); + if (!maybe_depth_stencil.has_value()) { + return; + } auto depth_stencil = maybe_depth_stencil.value(); EXPECT_EQ(depth_stencil.initialLayout, vk::ImageLayout::eUndefined); From c7eea8b27e719fdca16cf5d26e98e183c5c79a08 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Wed, 27 Mar 2024 14:40:37 -0700 Subject: [PATCH 3/3] Rename to GetColorAttachments --- impeller/renderer/backend/vulkan/render_pass_builder_vk.cc | 2 +- impeller/renderer/backend/vulkan/render_pass_builder_vk.h | 3 ++- .../backend/vulkan/render_pass_builder_vk_unittests.cc | 5 ++--- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/impeller/renderer/backend/vulkan/render_pass_builder_vk.cc b/impeller/renderer/backend/vulkan/render_pass_builder_vk.cc index 662c64253bb4d..b0f29947533eb 100644 --- a/impeller/renderer/backend/vulkan/render_pass_builder_vk.cc +++ b/impeller/renderer/backend/vulkan/render_pass_builder_vk.cc @@ -185,7 +185,7 @@ void InsertBarrierForInputAttachmentRead(const vk::CommandBuffer& buffer, } const std::map& -RenderPassBuilderVK::GetColors() const { +RenderPassBuilderVK::GetColorAttachments() const { return colors_; } diff --git a/impeller/renderer/backend/vulkan/render_pass_builder_vk.h b/impeller/renderer/backend/vulkan/render_pass_builder_vk.h index a855898bb2ed6..4ef1924e8fe32 100644 --- a/impeller/renderer/backend/vulkan/render_pass_builder_vk.h +++ b/impeller/renderer/backend/vulkan/render_pass_builder_vk.h @@ -43,7 +43,8 @@ class RenderPassBuilderVK { vk::UniqueRenderPass Build(const vk::Device& device) const; // Visible for testing. - const std::map& GetColors() const; + const std::map& GetColorAttachments() + const; // Visible for testing. const std::map& GetResolves() const; diff --git a/impeller/renderer/backend/vulkan/render_pass_builder_vk_unittests.cc b/impeller/renderer/backend/vulkan/render_pass_builder_vk_unittests.cc index dcd7df1d2cb51..03e11a6725ffb 100644 --- a/impeller/renderer/backend/vulkan/render_pass_builder_vk_unittests.cc +++ b/impeller/renderer/backend/vulkan/render_pass_builder_vk_unittests.cc @@ -7,7 +7,6 @@ #include "impeller/core/formats.h" #include "impeller/renderer/backend/vulkan/render_pass_builder_vk.h" #include "impeller/renderer/backend/vulkan/test/mock_vulkan.h" -#include "vulkan/vulkan_core.h" #include "vulkan/vulkan_enums.hpp" namespace impeller { @@ -44,8 +43,8 @@ TEST(RenderPassBuilder, CreatesRenderPassWithCombinedDepthStencil) { EXPECT_TRUE(!!render_pass); - auto maybe_color = builder.GetColors().find(0u); - ASSERT_NE(maybe_color, builder.GetColors().end()); + 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::eGeneral);