From 8893670e159698ae57307bf72922f1f1c8ee7d48 Mon Sep 17 00:00:00 2001 From: "auto-submit[bot]" <98614782+auto-submit[bot]@users.noreply.github.com> Date: Tue, 25 Mar 2025 09:40:57 -0700 Subject: [PATCH] Reverts "[Impeller] remove transfer barriers from render pass, drop blit, tighten up graphics on level 3. (#165584)" (#165898) Reverts: flutter/flutter#165584 Initiated by: jonahwilliams Reason for reverting: render pass compatibility issue. Original PR Author: jonahwilliams Reviewed By: {gaaclarke} This change reverts the following previous change: The change to fix PowerVR barriers regressed Mali (see https://github.com/flutter/flutter/issues/165538) We need to tighten the barriers to remove the transfer barrier to get back mali performance. however, this requires us to drop the usage of the blit for onscreen restore. not a huge loss imo. At any rate, all of these changes should be visible in benchmarks. Co-authored-by: auto-submit[bot] --- .../flutter/impeller/display_list/canvas.cc | 4 +- .../impeller/display_list/canvas_unittests.cc | 2 +- .../vulkan/android/ahb_texture_source_vk.cc | 6 ++- .../backend/vulkan/render_pass_builder_vk.cc | 37 ++++++------------- .../backend/vulkan/render_pass_builder_vk.h | 5 --- .../renderer/backend/vulkan/render_pass_vk.cc | 1 - .../swapchain/khr/khr_swapchain_impl_vk.cc | 5 ++- .../backend/vulkan/texture_source_vk.cc | 6 +++ .../backend/vulkan/texture_source_vk.h | 5 ++- 9 files changed, 33 insertions(+), 38 deletions(-) diff --git a/engine/src/flutter/impeller/display_list/canvas.cc b/engine/src/flutter/impeller/display_list/canvas.cc index f7df07c785..78f3b7bf05 100644 --- a/engine/src/flutter/impeller/display_list/canvas.cc +++ b/engine/src/flutter/impeller/display_list/canvas.cc @@ -1718,8 +1718,8 @@ bool Canvas::SupportsBlitToOnscreen() const { return renderer_.GetContext() ->GetCapabilities() ->SupportsTextureToTextureBlits() && - renderer_.GetContext()->GetBackendType() == - Context::BackendType::kMetal; + renderer_.GetContext()->GetBackendType() != + Context::BackendType::kOpenGLES; } bool Canvas::BlitToOnscreen(bool is_onscreen) { diff --git a/engine/src/flutter/impeller/display_list/canvas_unittests.cc b/engine/src/flutter/impeller/display_list/canvas_unittests.cc index 70487a3132..2f313a4df9 100644 --- a/engine/src/flutter/impeller/display_list/canvas_unittests.cc +++ b/engine/src/flutter/impeller/display_list/canvas_unittests.cc @@ -379,7 +379,7 @@ TEST_P(AiksTest, SupportsBlitToOnscreen) { auto canvas = CreateTestCanvas(context, Rect::MakeLTRB(0, 0, 100, 100), /*requires_readback=*/true); - if (GetBackend() != PlaygroundBackend::kMetal) { + if (GetBackend() == PlaygroundBackend::kOpenGLES) { EXPECT_FALSE(canvas->SupportsBlitToOnscreen()); } else { EXPECT_TRUE(canvas->SupportsBlitToOnscreen()); 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 8fe4766448..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 @@ -56,7 +56,11 @@ vk::UniqueImage CreateVKImageWrapperForAndroidHarwareBuffer( } if (ahb_desc.usage & AHARDWAREBUFFER_USAGE_GPU_FRAMEBUFFER) { image_usage_flags |= vk::ImageUsageFlagBits::eColorAttachment; + } + if (ahb_desc.usage & AHARDWAREBUFFER_USAGE_COMPOSER_OVERLAY) { + image_usage_flags |= vk::ImageUsageFlagBits::eColorAttachment; image_usage_flags |= vk::ImageUsageFlagBits::eInputAttachment; + image_usage_flags |= vk::ImageUsageFlagBits::eTransferDst; } vk::ImageCreateFlags image_create_flags; @@ -298,7 +302,7 @@ TextureDescriptor ToTextureDescriptor(const AHardwareBuffer_Desc& ahb_desc) { desc.mip_count = (ahb_desc.usage & AHARDWAREBUFFER_USAGE_GPU_MIPMAP_COMPLETE) ? ahb_size.MipCount() : 1u; - if (ahb_desc.usage & AHARDWAREBUFFER_USAGE_GPU_FRAMEBUFFER) { + if (ahb_desc.usage & AHARDWAREBUFFER_USAGE_COMPOSER_OVERLAY) { desc.usage = TextureUsage::kRenderTarget; } return desc; 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 393d35a6ea..2d369e0059 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 @@ -196,20 +196,12 @@ vk::UniqueRenderPass RenderPassBuilderVK::Build( // to the onscreen. deps[0].srcSubpass = VK_SUBPASS_EXTERNAL; deps[0].dstSubpass = 0u; - // If this render pass is performed using the onscreen attachment, then we - // know that the previous stage does not include sampling - only color - // attachment. According to various vulkan documentation, the correct access - // flag bits for this stage with a queue submit in-between is `{}` as the - // access is not otherwise expressable. - if (is_swapchain_) { - deps[0].srcStageMask = vk::PipelineStageFlagBits::eColorAttachmentOutput; - deps[0].srcAccessMask = {}; - } else { - deps[0].srcStageMask = vk::PipelineStageFlagBits::eColorAttachmentOutput | - vk::PipelineStageFlagBits::eFragmentShader; - deps[0].srcAccessMask = vk::AccessFlagBits::eShaderRead | - vk::AccessFlagBits::eColorAttachmentWrite; - } + deps[0].srcStageMask = vk::PipelineStageFlagBits::eColorAttachmentOutput | + vk::PipelineStageFlagBits::eFragmentShader | + vk::PipelineStageFlagBits::eTransfer; + deps[0].srcAccessMask = vk::AccessFlagBits::eShaderRead | + vk::AccessFlagBits::eTransferRead | + vk::AccessFlagBits::eColorAttachmentWrite; deps[0].dstStageMask = vk::PipelineStageFlagBits::eColorAttachmentOutput; deps[0].dstAccessMask = vk::AccessFlagBits::eColorAttachmentWrite; deps[0].dependencyFlags = kSelfDependencyFlags; @@ -225,23 +217,22 @@ vk::UniqueRenderPass RenderPassBuilderVK::Build( deps[1].dependencyFlags = kSelfDependencyFlags; // Outgoing dependency. The resolve step or color attachment must complete - // before we can sample from the image. This dependency is ignored for the - // onscreen as we will already insert a barrier before presenting the - // swapchain. + // before we can sample from the image, or use it as a blit src. deps[2].srcSubpass = 0u; // first subpass deps[2].dstSubpass = VK_SUBPASS_EXTERNAL; deps[2].srcStageMask = vk::PipelineStageFlagBits::eColorAttachmentOutput; deps[2].srcAccessMask = vk::AccessFlagBits::eColorAttachmentWrite; - deps[2].dstStageMask = vk::PipelineStageFlagBits::eFragmentShader; - deps[2].dstAccessMask = vk::AccessFlagBits::eShaderRead; + deps[2].dstStageMask = vk::PipelineStageFlagBits::eFragmentShader | + vk::PipelineStageFlagBits::eTransfer; + deps[2].dstAccessMask = + vk::AccessFlagBits::eShaderRead | vk::AccessFlagBits::eTransferRead; deps[2].dependencyFlags = kSelfDependencyFlags; vk::RenderPassCreateInfo render_pass_desc; render_pass_desc.setPAttachments(attachments.data()); render_pass_desc.setAttachmentCount(attachments_index); render_pass_desc.setSubpasses(subpass0); - render_pass_desc.setPDependencies(deps); - render_pass_desc.setDependencyCount(3); + render_pass_desc.setDependencies(deps); auto [result, pass] = device.createRenderPassUnique(render_pass_desc); if (result != vk::Result::eSuccess) { @@ -281,10 +272,6 @@ void InsertBarrierForInputAttachmentRead(const vk::CommandBuffer& buffer, ); } -void RenderPassBuilderVK::SetSwapchain(bool value) { - is_swapchain_ = value; -} - const std::map& RenderPassBuilderVK::GetColorAttachments() const { return colors_; 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 65e376c178..6021f32a2b 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 @@ -28,10 +28,6 @@ class RenderPassBuilderVK { RenderPassBuilderVK& operator=(const RenderPassBuilderVK&) = delete; - /// Whether this builder should configure barriers for the onscreen render - /// pass. - void SetSwapchain(bool value); - RenderPassBuilderVK& SetColorAttachment( size_t index, PixelFormat format, @@ -70,7 +66,6 @@ class RenderPassBuilderVK { std::optional GetColor0Resolve() const; private: - bool is_swapchain_ = false; std::optional color0_; std::optional color0_resolve_; std::optional depth_stencil_; 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 d3ab3979de..29a78abdc1 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 @@ -81,7 +81,6 @@ SharedHandleVK RenderPassVK::CreateVKRenderPass( const std::shared_ptr& command_buffer, bool is_swapchain) const { RenderPassBuilderVK builder; - builder.SetSwapchain(is_swapchain); render_target_.IterateAllColorAttachments([&](size_t bind_point, const ColorAttachment& diff --git a/engine/src/flutter/impeller/renderer/backend/vulkan/swapchain/khr/khr_swapchain_impl_vk.cc b/engine/src/flutter/impeller/renderer/backend/vulkan/swapchain/khr/khr_swapchain_impl_vk.cc index 84ebc9e944..cd305e98e8 100644 --- a/engine/src/flutter/impeller/renderer/backend/vulkan/swapchain/khr/khr_swapchain_impl_vk.cc +++ b/engine/src/flutter/impeller/renderer/backend/vulkan/swapchain/khr/khr_swapchain_impl_vk.cc @@ -186,9 +186,10 @@ KHRSwapchainImplVK::KHRSwapchainImplVK(const std::shared_ptr& context, : surface_caps.maxImageCount // max zero means no limit ); swapchain_info.imageArrayLayers = 1u; - // Swapchain images are primarily used as color attachments (via resolve) or - // input attachments. + // Swapchain images are primarily used as color attachments (via resolve), + // blit targets, or input attachments. swapchain_info.imageUsage = vk::ImageUsageFlagBits::eColorAttachment | + vk::ImageUsageFlagBits::eTransferDst | vk::ImageUsageFlagBits::eInputAttachment; swapchain_info.preTransform = vk::SurfaceTransformFlagBitsKHR::eIdentity; swapchain_info.compositeAlpha = composite.value(); diff --git a/engine/src/flutter/impeller/renderer/backend/vulkan/texture_source_vk.cc b/engine/src/flutter/impeller/renderer/backend/vulkan/texture_source_vk.cc index d8b6f6d6f3..7fdf5caf92 100644 --- a/engine/src/flutter/impeller/renderer/backend/vulkan/texture_source_vk.cc +++ b/engine/src/flutter/impeller/renderer/backend/vulkan/texture_source_vk.cc @@ -19,11 +19,13 @@ std::shared_ptr TextureSourceVK::GetYUVConversion() const { } vk::ImageLayout TextureSourceVK::GetLayout() const { + ReaderLock lock(layout_mutex_); return layout_; } vk::ImageLayout TextureSourceVK::SetLayoutWithoutEncoding( vk::ImageLayout layout) const { + WriterLock lock(layout_mutex_); const auto old_layout = layout_; layout_ = layout; return old_layout; @@ -31,6 +33,10 @@ vk::ImageLayout TextureSourceVK::SetLayoutWithoutEncoding( fml::Status TextureSourceVK::SetLayout(const BarrierVK& barrier) const { const auto old_layout = SetLayoutWithoutEncoding(barrier.new_layout); + if (barrier.new_layout == old_layout) { + return {}; + } + vk::ImageMemoryBarrier image_barrier; image_barrier.srcAccessMask = barrier.src_access; image_barrier.dstAccessMask = barrier.dst_access; diff --git a/engine/src/flutter/impeller/renderer/backend/vulkan/texture_source_vk.h b/engine/src/flutter/impeller/renderer/backend/vulkan/texture_source_vk.h index d1498d0ba2..374907cae1 100644 --- a/engine/src/flutter/impeller/renderer/backend/vulkan/texture_source_vk.h +++ b/engine/src/flutter/impeller/renderer/backend/vulkan/texture_source_vk.h @@ -6,6 +6,7 @@ #define FLUTTER_IMPELLER_RENDERER_BACKEND_VULKAN_TEXTURE_SOURCE_VK_H_ #include "flutter/fml/status.h" +#include "impeller/base/thread.h" #include "impeller/core/texture_descriptor.h" #include "impeller/renderer/backend/vulkan/barrier_vk.h" #include "impeller/renderer/backend/vulkan/formats_vk.h" @@ -158,7 +159,9 @@ class TextureSourceVK { private: SharedHandleVK framebuffer_; SharedHandleVK render_pass_; - mutable vk::ImageLayout layout_ = vk::ImageLayout::eUndefined; + mutable RWMutex layout_mutex_; + mutable vk::ImageLayout layout_ IPLR_GUARDED_BY(layout_mutex_) = + vk::ImageLayout::eUndefined; }; } // namespace impeller