From 8e740b50bfde57103ef7e65fca0222a5837587e1 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Tue, 26 Nov 2024 17:14:40 -0800 Subject: [PATCH 01/10] [Impeller] avoid heap allocation in RenderTarget object. --- impeller/display_list/canvas.cc | 9 ++- impeller/entity/entity_pass_target.cc | 2 +- .../entity/entity_pass_target_unittests.cc | 20 ++----- impeller/entity/inline_pass_context.cc | 15 +---- impeller/entity/render_target_cache.cc | 15 +++-- .../entity/render_target_cache_unittests.cc | 5 +- impeller/playground/playground.cc | 8 +-- .../renderer/backend/gles/render_pass_gles.cc | 9 ++- .../gles/test/surface_gles_unittests.cc | 4 +- .../renderer/backend/metal/render_pass_mtl.mm | 7 ++- .../renderer/backend/vulkan/context_vk.cc | 19 +++--- .../backend/vulkan/render_pass_builder_vk.cc | 58 +++++++++++++++--- .../backend/vulkan/render_pass_builder_vk.h | 11 +++- .../render_pass_builder_vk_unittests.cc | 18 +++--- .../vulkan/render_pass_cache_unittests.cc | 3 +- .../vulkan/test/swapchain_unittests.cc | 3 +- impeller/renderer/render_target.cc | 59 ++++++++++++++++--- impeller/renderer/render_target.h | 14 ++--- impeller/renderer/renderer_unittests.cc | 8 --- 19 files changed, 176 insertions(+), 111 deletions(-) diff --git a/impeller/display_list/canvas.cc b/impeller/display_list/canvas.cc index 7b26f02892904..f311094489fa3 100644 --- a/impeller/display_list/canvas.cc +++ b/impeller/display_list/canvas.cc @@ -14,6 +14,7 @@ #include "flutter/fml/logging.h" #include "flutter/fml/trace_event.h" #include "impeller/base/validation.h" +#include "impeller/core/formats.h" #include "impeller/display_list/color_filter.h" #include "impeller/display_list/image_filter.h" #include "impeller/display_list/skia_conversions.h" @@ -856,7 +857,7 @@ void Canvas::DrawAtlas(const std::shared_ptr& atlas_contents, void Canvas::SetupRenderPass() { renderer_.GetRenderTargetCache()->Start(); - auto color0 = render_target_.GetColorAttachments().find(0u)->second; + ColorAttachment color0 = render_target_.GetColor0(); auto& stencil_attachment = render_target_.GetStencilAttachment(); auto& depth_attachment = render_target_.GetDepthAttachment(); @@ -1463,8 +1464,7 @@ void Canvas::AddRenderEntityToCurrentPass(Entity& entity, bool reuse_depth) { RenderTarget& render_target = render_passes_.back() .inline_pass_context->GetPassTarget() .GetRenderTarget(); - ColorAttachment attachment = - render_target.GetColorAttachments().find(0u)->second; + ColorAttachment attachment = render_target.GetColor0(); // Attachment.clear color needs to be premultiplied at all times, but the // Color::Blend function requires unpremultiplied colors. attachment.clear_color = attachment.clear_color.Unpremultiply() @@ -1591,8 +1591,7 @@ std::shared_ptr Canvas::FlipBackdrop(Point global_pass_position, } if (should_use_onscreen) { - ColorAttachment color0 = - render_target_.GetColorAttachments().find(0u)->second; + ColorAttachment color0 = render_target_.GetColor0(); // When MSAA is being used, we end up overriding the entire backdrop by // drawing the previous pass texture, and so we don't have to clear it and // can use kDontCare. diff --git a/impeller/entity/entity_pass_target.cc b/impeller/entity/entity_pass_target.cc index e9284de522675..a25d96e197d45 100644 --- a/impeller/entity/entity_pass_target.cc +++ b/impeller/entity/entity_pass_target.cc @@ -19,7 +19,7 @@ EntityPassTarget::EntityPassTarget(const RenderTarget& render_target, std::shared_ptr EntityPassTarget::Flip( const ContentContext& renderer) { - auto color0 = target_.GetColorAttachments().find(0)->second; + ColorAttachment color0 = target_.GetColor0(); if (!color0.resolve_texture) { VALIDATION_LOG << "EntityPassTarget Flip should never be called for a " "non-MSAA target."; diff --git a/impeller/entity/entity_pass_target_unittests.cc b/impeller/entity/entity_pass_target_unittests.cc index cd22acbffdb5a..28aa97d3fd9b3 100644 --- a/impeller/entity/entity_pass_target_unittests.cc +++ b/impeller/entity/entity_pass_target_unittests.cc @@ -31,20 +31,14 @@ TEST_P(EntityPassTargetTest, SwapWithMSAATexture) { auto entity_pass_target = EntityPassTarget(render_target, false, false); - auto color0 = entity_pass_target.GetRenderTarget() - .GetColorAttachments() - .find(0u) - ->second; + auto color0 = entity_pass_target.GetRenderTarget().GetColor0(); auto msaa_tex = color0.texture; auto resolve_tex = color0.resolve_texture; FML_DCHECK(content_context); entity_pass_target.Flip(*content_context); - color0 = entity_pass_target.GetRenderTarget() - .GetColorAttachments() - .find(0u) - ->second; + color0 = entity_pass_target.GetRenderTarget().GetColor0(); ASSERT_EQ(msaa_tex, color0.texture); ASSERT_NE(resolve_tex, color0.resolve_texture); @@ -89,10 +83,7 @@ TEST_P(EntityPassTargetTest, SwapWithMSAAImplicitResolve) { auto entity_pass_target = EntityPassTarget(render_target, false, true); - auto color0 = entity_pass_target.GetRenderTarget() - .GetColorAttachments() - .find(0u) - ->second; + auto color0 = entity_pass_target.GetRenderTarget().GetColor0(); auto msaa_tex = color0.texture; auto resolve_tex = color0.resolve_texture; @@ -101,10 +92,7 @@ TEST_P(EntityPassTargetTest, SwapWithMSAAImplicitResolve) { FML_DCHECK(content_context); entity_pass_target.Flip(*content_context); - color0 = entity_pass_target.GetRenderTarget() - .GetColorAttachments() - .find(0u) - ->second; + color0 = entity_pass_target.GetRenderTarget().GetColor0(); ASSERT_NE(msaa_tex, color0.texture); ASSERT_NE(resolve_tex, color0.resolve_texture); diff --git a/impeller/entity/inline_pass_context.cc b/impeller/entity/inline_pass_context.cc index ac86af2139143..8c00550d272de 100644 --- a/impeller/entity/inline_pass_context.cc +++ b/impeller/entity/inline_pass_context.cc @@ -86,20 +86,12 @@ const std::shared_ptr& InlinePassContext::GetRenderPass() { return pass_; } - if (pass_target_.GetRenderTarget().GetColorAttachments().empty()) { - VALIDATION_LOG << "Color attachment unexpectedly missing from the " - "EntityPass render target."; - return pass_; - } - command_buffer_->SetLabel("EntityPass Command Buffer"); { // If the pass target has a resolve texture, then we're using MSAA. - bool is_msaa = pass_target_.GetRenderTarget() - .GetColorAttachments() - .find(0) - ->second.resolve_texture != nullptr; + bool is_msaa = + pass_target_.GetRenderTarget().GetColor0().resolve_texture != nullptr; if (pass_count_ > 0 && is_msaa) { pass_target_.Flip(renderer_); } @@ -107,8 +99,7 @@ const std::shared_ptr& InlinePassContext::GetRenderPass() { // Find the color attachment a second time, since the target may have just // flipped. - auto color0 = - pass_target_.GetRenderTarget().GetColorAttachments().find(0)->second; + ColorAttachment color0 = pass_target_.GetRenderTarget().GetColor0(); bool is_msaa = color0.resolve_texture != nullptr; if (pass_count_ > 0) { diff --git a/impeller/entity/render_target_cache.cc b/impeller/entity/render_target_cache.cc index dbfe0a9b50c63..fb9c8098f4b82 100644 --- a/impeller/entity/render_target_cache.cc +++ b/impeller/entity/render_target_cache.cc @@ -3,6 +3,7 @@ // found in the LICENSE file. #include "impeller/entity/render_target_cache.h" +#include "impeller/core/formats.h" #include "impeller/renderer/render_target.h" namespace impeller { @@ -52,10 +53,9 @@ RenderTarget RenderTargetCache::CreateOffscreen( const auto other_config = render_target_data.config; if (!render_target_data.used_this_frame && other_config == config) { render_target_data.used_this_frame = true; - auto color0 = render_target_data.render_target.GetColorAttachments() - .find(0u) - ->second; - auto depth = render_target_data.render_target.GetDepthAttachment(); + ColorAttachment color0 = render_target_data.render_target.GetColor0(); + std::optional depth = + render_target_data.render_target.GetDepthAttachment(); std::shared_ptr depth_tex = depth ? depth->texture : nullptr; return RenderTargetAllocator::CreateOffscreen( context, size, mip_count, label, color_attachment_config, @@ -102,10 +102,9 @@ RenderTarget RenderTargetCache::CreateOffscreenMSAA( const auto other_config = render_target_data.config; if (!render_target_data.used_this_frame && other_config == config) { render_target_data.used_this_frame = true; - auto color0 = render_target_data.render_target.GetColorAttachments() - .find(0u) - ->second; - auto depth = render_target_data.render_target.GetDepthAttachment(); + ColorAttachment color0 = render_target_data.render_target.GetColor0(); + std::optional depth = + render_target_data.render_target.GetDepthAttachment(); std::shared_ptr depth_tex = depth ? depth->texture : nullptr; return RenderTargetAllocator::CreateOffscreenMSAA( context, size, mip_count, label, color_attachment_config, diff --git a/impeller/entity/render_target_cache_unittests.cc b/impeller/entity/render_target_cache_unittests.cc index 73493e7641c85..17ea9338f11e0 100644 --- a/impeller/entity/render_target_cache_unittests.cc +++ b/impeller/entity/render_target_cache_unittests.cc @@ -7,6 +7,7 @@ #include "flutter/testing/testing.h" #include "impeller/base/validation.h" #include "impeller/core/allocator.h" +#include "impeller/core/formats.h" #include "impeller/core/texture_descriptor.h" #include "impeller/entity/entity_playground.h" #include "impeller/entity/render_target_cache.h" @@ -104,8 +105,8 @@ TEST_P(RenderTargetCacheTest, CachedTextureGetsNewAttachmentConfig) { *GetContext(), {100, 100}, 1, "Offscreen2", color_attachment_config); render_target_cache.End(); - auto color1 = target1.GetColorAttachments().find(0)->second; - auto color2 = target2.GetColorAttachments().find(0)->second; + ColorAttachment color1 = target1.GetColor0(); + ColorAttachment color2 = target2.GetColor0(); // The second color attachment should reuse the first attachment's texture // but with attributes from the second AttachmentConfig. EXPECT_EQ(color2.texture, color1.texture); diff --git a/impeller/playground/playground.cc b/impeller/playground/playground.cc index f588a54049480..fc7cbcd3140ea 100644 --- a/impeller/playground/playground.cc +++ b/impeller/playground/playground.cc @@ -284,12 +284,7 @@ bool Playground::OpenPlaygroundHere( } buffer->SetLabel("ImGui Command Buffer"); - if (render_target.GetColorAttachments().empty()) { - VALIDATION_LOG << "render target attachments are empty."; - return false; - } - - auto color0 = render_target.GetColorAttachments().find(0)->second; + auto color0 = render_target.GetColor0(); color0.load_action = LoadAction::kLoad; if (color0.resolve_texture) { color0.texture = color0.resolve_texture; @@ -297,7 +292,6 @@ bool Playground::OpenPlaygroundHere( color0.store_action = StoreAction::kStore; } render_target.SetColorAttachment(color0, 0); - render_target.SetStencilAttachment(std::nullopt); render_target.SetDepthAttachment(std::nullopt); diff --git a/impeller/renderer/backend/gles/render_pass_gles.cc b/impeller/renderer/backend/gles/render_pass_gles.cc index 594fcfab0732f..6c9d45020dae8 100644 --- a/impeller/renderer/backend/gles/render_pass_gles.cc +++ b/impeller/renderer/backend/gles/render_pass_gles.cc @@ -11,6 +11,7 @@ #include "fml/closure.h" #include "fml/logging.h" #include "impeller/base/validation.h" +#include "impeller/core/formats.h" #include "impeller/renderer/backend/gles/context_gles.h" #include "impeller/renderer/backend/gles/device_buffer_gles.h" #include "impeller/renderer/backend/gles/formats_gles.h" @@ -537,9 +538,11 @@ bool RenderPassGLES::OnEncodeCommands(const Context& context) const { if (!render_target.HasColorAttachment(0u)) { return false; } - const auto& color0 = render_target.GetColorAttachments().at(0u); - const auto& depth0 = render_target.GetDepthAttachment(); - const auto& stencil0 = render_target.GetStencilAttachment(); + const ColorAttachment& color0 = render_target.GetColor0(); + const std::optional& depth0 = + render_target.GetDepthAttachment(); + const std::optional& stencil0 = + render_target.GetStencilAttachment(); auto pass_data = std::make_shared(); pass_data->label = label_; diff --git a/impeller/renderer/backend/gles/test/surface_gles_unittests.cc b/impeller/renderer/backend/gles/test/surface_gles_unittests.cc index dbef03075a088..14b90907e7d81 100644 --- a/impeller/renderer/backend/gles/test/surface_gles_unittests.cc +++ b/impeller/renderer/backend/gles/test/surface_gles_unittests.cc @@ -21,8 +21,8 @@ TEST_P(SurfaceGLESTest, CanWrapNonZeroFBO) { ASSERT_TRUE(!!surface); ASSERT_TRUE(surface->IsValid()); ASSERT_TRUE(surface->GetRenderTarget().HasColorAttachment(0)); - const auto& texture = TextureGLES::Cast( - *(surface->GetRenderTarget().GetColorAttachments().at(0).texture)); + const auto& texture = + TextureGLES::Cast(*(surface->GetRenderTarget().GetColor0().texture)); auto wrapped = texture.GetFBO(); ASSERT_TRUE(wrapped.has_value()); // NOLINTNEXTLINE(bugprone-unchecked-optional-access) diff --git a/impeller/renderer/backend/metal/render_pass_mtl.mm b/impeller/renderer/backend/metal/render_pass_mtl.mm index 80cf59bb5f1f7..ddda56a9ec84a 100644 --- a/impeller/renderer/backend/metal/render_pass_mtl.mm +++ b/impeller/renderer/backend/metal/render_pass_mtl.mm @@ -104,8 +104,13 @@ static bool ConfigureStencilAttachment( const RenderTarget& desc) { auto result = [MTLRenderPassDescriptor renderPassDescriptor]; - const auto& colors = desc.GetColorAttachments(); + const ColorAttachment& color0 = desc.GetColor0(); + if (!ConfigureColorAttachment(color0, result.colorAttachments[0u])) { + VALIDATION_LOG << "Could not configure color attachment at index 0"; + return nil; + } + const auto& colors = desc.GetColorAttachments(); for (const auto& color : colors) { if (!ConfigureColorAttachment(color.second, result.colorAttachments[color.first])) { diff --git a/impeller/renderer/backend/vulkan/context_vk.cc b/impeller/renderer/backend/vulkan/context_vk.cc index d793b3e9c61fd..8963fae083654 100644 --- a/impeller/renderer/backend/vulkan/context_vk.cc +++ b/impeller/renderer/backend/vulkan/context_vk.cc @@ -7,6 +7,7 @@ #include #include "fml/concurrent_message_loop.h" +#include "impeller/core/formats.h" #include "impeller/renderer/backend/vulkan/command_queue_vk.h" #include "impeller/renderer/backend/vulkan/descriptor_pool_vk.h" #include "impeller/renderer/backend/vulkan/render_pass_builder_vk.h" @@ -666,15 +667,15 @@ void ContextVK::InitializeCommonlyUsedShadersIfNeeded() const { rt_allocator.CreateOffscreenMSAA(*this, {1, 1}, 1); RenderPassBuilderVK builder; - for (const auto& [bind_point, color] : render_target.GetColorAttachments()) { - builder.SetColorAttachment( - bind_point, // - color.texture->GetTextureDescriptor().format, // - color.texture->GetTextureDescriptor().sample_count, // - color.load_action, // - color.store_action // - ); - } + + const ColorAttachment& color = render_target.GetColor0(); + builder.SetColorAttachment( + 0u, // + color.texture->GetTextureDescriptor().format, // + color.texture->GetTextureDescriptor().sample_count, // + color.load_action, // + color.store_action // + ); if (auto depth = render_target.GetDepthAttachment(); depth.has_value()) { builder.SetDepthStencilAttachment( diff --git a/impeller/renderer/backend/vulkan/render_pass_builder_vk.cc b/impeller/renderer/backend/vulkan/render_pass_builder_vk.cc index 77f5a3a1e9b14..32f62dddf1907 100644 --- a/impeller/renderer/backend/vulkan/render_pass_builder_vk.cc +++ b/impeller/renderer/backend/vulkan/render_pass_builder_vk.cc @@ -48,14 +48,27 @@ RenderPassBuilderVK& RenderPassBuilderVK::SetColorAttachment( desc.initialLayout = vk::ImageLayout::eUndefined; } desc.finalLayout = vk::ImageLayout::eGeneral; - colors_[index] = desc; - if (StoreActionPerformsResolve(store_action)) { - desc.storeOp = ToVKAttachmentStoreOp(store_action, true); - desc.samples = vk::SampleCountFlagBits::e1; - resolves_[index] = desc; + const bool performs_resolves = StoreActionPerformsResolve(store_action); + if (index == 0u) { + color0_ = desc; + + if (performs_resolves) { + desc.storeOp = ToVKAttachmentStoreOp(store_action, true); + desc.samples = vk::SampleCountFlagBits::e1; + color0_resolve_ = desc; + } else { + color0_resolve_ = std::nullopt; + } } else { - resolves_.erase(index); + colors_[index] = desc; + if (performs_resolves) { + desc.storeOp = ToVKAttachmentStoreOp(store_action, true); + desc.samples = vk::SampleCountFlagBits::e1; + resolves_[index] = desc; + } else { + resolves_.erase(index); + } } return *this; } @@ -100,8 +113,11 @@ vk::UniqueRenderPass RenderPassBuilderVK::Build( const vk::Device& device) const { // This must be less than `VkPhysicalDeviceLimits::maxColorAttachments` but we // are not checking. - const auto color_attachments_count = + auto color_attachments_count = colors_.empty() ? 0u : colors_.rbegin()->first + 1u; + if (color0_.has_value()) { + color_attachments_count++; + } std::vector attachments; @@ -111,6 +127,22 @@ vk::UniqueRenderPass RenderPassBuilderVK::Build( kUnusedAttachmentReference); vk::AttachmentReference depth_stencil_ref = kUnusedAttachmentReference; + if (color0_.has_value()) { + vk::AttachmentReference color_ref; + color_ref.attachment = attachments.size(); + color_ref.layout = vk::ImageLayout::eGeneral; + color_refs[0] = color_ref; + attachments.push_back(color0_.value()); + + if (color0_resolve_.has_value()) { + vk::AttachmentReference resolve_ref; + resolve_ref.attachment = attachments.size(); + resolve_ref.layout = vk::ImageLayout::eGeneral; + resolve_refs[0] = resolve_ref; + attachments.push_back(color0_resolve_.value()); + } + } + for (const auto& color : colors_) { vk::AttachmentReference color_ref; color_ref.attachment = attachments.size(); @@ -207,4 +239,16 @@ RenderPassBuilderVK::GetDepthStencil() const { return depth_stencil_; } +// Visible for testing. +std::optional RenderPassBuilderVK::GetColor0() + const { + return color0_; +} + +// Visible for testing. +std::optional RenderPassBuilderVK::GetColor0Resolve() + const { + return color0_resolve_; +} + } // 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 c3ed390e961b9..51c6313ec9583 100644 --- a/impeller/renderer/backend/vulkan/render_pass_builder_vk.h +++ b/impeller/renderer/backend/vulkan/render_pass_builder_vk.h @@ -54,10 +54,19 @@ class RenderPassBuilderVK { // Visible for testing. const std::optional& GetDepthStencil() const; + // Visible for testing. + std::optional GetColor0() const; + + // Visible for testing. + std::optional GetColor0Resolve() const; + private: + std::optional color0_; + std::optional color0_resolve_; + std::optional depth_stencil_; + std::map colors_; std::map resolves_; - std::optional depth_stencil_; }; //------------------------------------------------------------------------------ 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 658ae2f54ac88..d280cdb6dfc23 100644 --- a/impeller/renderer/backend/vulkan/render_pass_builder_vk_unittests.cc +++ b/impeller/renderer/backend/vulkan/render_pass_builder_vk_unittests.cc @@ -40,9 +40,9 @@ TEST(RenderPassBuilder, RenderPassWithLoadOpUsesCurrentLayout) { EXPECT_TRUE(!!render_pass); - auto maybe_color = builder.GetColorAttachments().find(0u); - ASSERT_NE(maybe_color, builder.GetColorAttachments().end()); - auto color = maybe_color->second; + auto maybe_color = builder.GetColor0(); + ASSERT_TRUE(maybe_color.has_value()); + auto color = maybe_color.value(); EXPECT_EQ(color.initialLayout, vk::ImageLayout::eColorAttachmentOptimal); EXPECT_EQ(color.finalLayout, vk::ImageLayout::eGeneral); @@ -66,9 +66,9 @@ TEST(RenderPassBuilder, CreatesRenderPassWithCombinedDepthStencil) { EXPECT_TRUE(!!render_pass); - auto maybe_color = builder.GetColorAttachments().find(0u); - ASSERT_NE(maybe_color, builder.GetColorAttachments().end()); - auto color = maybe_color->second; + auto maybe_color = builder.GetColor0(); + ASSERT_TRUE(maybe_color.has_value()); + auto color = maybe_color.value(); EXPECT_EQ(color.initialLayout, vk::ImageLayout::eUndefined); EXPECT_EQ(color.finalLayout, vk::ImageLayout::eGeneral); @@ -135,9 +135,9 @@ TEST(RenderPassBuilder, CreatesMSAAResolveWithCorrectStore) { EXPECT_TRUE(!!render_pass); - auto maybe_color = builder.GetColorAttachments().find(0u); - ASSERT_NE(maybe_color, builder.GetColorAttachments().end()); - auto color = maybe_color->second; + auto maybe_color = builder.GetColor0(); + ASSERT_TRUE(maybe_color.has_value()); + auto color = maybe_color.value(); // MSAA Texture. EXPECT_EQ(color.initialLayout, vk::ImageLayout::eUndefined); diff --git a/impeller/renderer/backend/vulkan/render_pass_cache_unittests.cc b/impeller/renderer/backend/vulkan/render_pass_cache_unittests.cc index 055c91a3ccb10..51d42aee512b8 100644 --- a/impeller/renderer/backend/vulkan/render_pass_cache_unittests.cc +++ b/impeller/renderer/backend/vulkan/render_pass_cache_unittests.cc @@ -23,8 +23,7 @@ TEST_P(RendererTest, CachesRenderPassAndFramebuffer) { auto render_target = allocator->CreateOffscreenMSAA(*GetContext(), {100, 100}, 1); - auto resolve_texture = - render_target.GetColorAttachments().find(0u)->second.resolve_texture; + auto resolve_texture = render_target.GetColor0().resolve_texture; auto& texture_vk = TextureVK::Cast(*resolve_texture); EXPECT_EQ(texture_vk.GetCachedFramebuffer(), nullptr); diff --git a/impeller/renderer/backend/vulkan/test/swapchain_unittests.cc b/impeller/renderer/backend/vulkan/test/swapchain_unittests.cc index 7589f849a15e4..9594e2d75103a 100644 --- a/impeller/renderer/backend/vulkan/test/swapchain_unittests.cc +++ b/impeller/renderer/backend/vulkan/test/swapchain_unittests.cc @@ -86,8 +86,7 @@ TEST(SwapchainTest, CachesRenderPassOnSwapchainImage) { auto& depth = render_target.GetDepthAttachment(); depth_stencil_textures.push_back(depth.has_value() ? depth->texture : nullptr); - msaa_textures.push_back( - render_target.GetColorAttachments().find(0u)->second.texture); + msaa_textures.push_back(render_target.GetColor0().texture); } for (auto i = 1; i < 3; i++) { diff --git a/impeller/renderer/render_target.cc b/impeller/renderer/render_target.cc index dd70f9f202a9d..7de9db51fe0d8 100644 --- a/impeller/renderer/render_target.cc +++ b/impeller/renderer/render_target.cc @@ -28,6 +28,7 @@ bool RenderTarget::IsValid() const { return false; } +#ifndef NDEBUG // Validate that all attachments are of the same size. { std::optional size; @@ -87,12 +88,18 @@ bool RenderTarget::IsValid() const { return false; } } +#endif // NDEBUG return true; } void RenderTarget::IterateAllAttachments( const std::function& iterator) const { + if (color0_.has_value()) { + if (!iterator(color0_.value())) { + return; + } + } for (const auto& color : colors_) { if (!iterator(color.second)) { return; @@ -113,13 +120,16 @@ void RenderTarget::IterateAllAttachments( } SampleCount RenderTarget::GetSampleCount() const { - if (auto found = colors_.find(0u); found != colors_.end()) { - return found->second.texture->GetTextureDescriptor().sample_count; + if (color0_.has_value()) { + return color0_.value().texture->GetTextureDescriptor().sample_count; } return SampleCount::kCount1; } bool RenderTarget::HasColorAttachment(size_t index) const { + if (index == 0u) { + return color0_.has_value(); + } if (auto found = colors_.find(index); found != colors_.end()) { return true; } @@ -127,6 +137,12 @@ bool RenderTarget::HasColorAttachment(size_t index) const { } std::optional RenderTarget::GetColorAttachmentSize(size_t index) const { + if (index == 0u) { + if (color0_.has_value()) { + return color0_.value().texture->GetSize(); + } + return std::nullopt; + } auto found = colors_.find(index); if (found == colors_.end()) { @@ -142,12 +158,10 @@ ISize RenderTarget::GetRenderTargetSize() const { } std::shared_ptr RenderTarget::GetRenderTargetTexture() const { - auto found = colors_.find(0u); - if (found == colors_.end()) { + if (!color0_.has_value()) { return nullptr; } - return found->second.resolve_texture ? found->second.resolve_texture - : found->second.texture; + return color0_->resolve_texture ? color0_->resolve_texture : color0_->texture; } PixelFormat RenderTarget::GetRenderTargetPixelFormat() const { @@ -169,7 +183,12 @@ size_t RenderTarget::GetMaxColorAttacmentBindIndex() const { RenderTarget& RenderTarget::SetColorAttachment( const ColorAttachment& attachment, size_t index) { - if (attachment.IsValid()) { + if (!attachment.IsValid()) { + return *this; + } + if (index == 0u) { + color0_ = attachment; + } else { colors_[index] = attachment; } return *this; @@ -195,6 +214,13 @@ RenderTarget& RenderTarget::SetStencilAttachment( return *this; } +ColorAttachment RenderTarget::GetColor0() const { + if (color0_.has_value()) { + return color0_.value(); + } + return ColorAttachment{}; +} + const std::map& RenderTarget::GetColorAttachments() const { return colors_; @@ -219,6 +245,9 @@ size_t RenderTarget::GetTotalAttachmentCount() const { count++; } } + if (color0_.has_value()) { + count++; + } if (depth_.has_value()) { count++; } @@ -231,6 +260,10 @@ size_t RenderTarget::GetTotalAttachmentCount() const { std::string RenderTarget::ToString() const { std::stringstream stream; + if (color0_.has_value()) { + stream << SPrintF("Color[%zu]=(%s)", 0ul, + ColorAttachmentToString(color0_.value()).c_str()); + } for (const auto& [index, color] : colors_) { stream << SPrintF("Color[%zu]=(%s)", index, ColorAttachmentToString(color).c_str()); @@ -248,6 +281,18 @@ std::string RenderTarget::ToString() const { return stream.str(); } +RenderTargetConfig RenderTarget::ToConfig() const { + if (color0_.has_value()) { + return RenderTargetConfig{}; + } + const auto& color_attachment = color0_.value(); + return RenderTargetConfig{ + .size = color_attachment.texture->GetSize(), + .mip_count = color_attachment.texture->GetMipCount(), + .has_msaa = color_attachment.resolve_texture != nullptr, + .has_depth_stencil = depth_.has_value() && stencil_.has_value()}; +} + RenderTargetAllocator::RenderTargetAllocator( std::shared_ptr allocator) : allocator_(std::move(allocator)) {} diff --git a/impeller/renderer/render_target.h b/impeller/renderer/render_target.h index 77f5bf43f5e38..b65ed33ff3e3d 100644 --- a/impeller/renderer/render_target.h +++ b/impeller/renderer/render_target.h @@ -109,6 +109,8 @@ class RenderTarget final { size_t GetMaxColorAttacmentBindIndex() const; + ColorAttachment GetColor0() const; + const std::map& GetColorAttachments() const; const std::optional& GetDepthAttachment() const; @@ -122,19 +124,13 @@ class RenderTarget final { std::string ToString() const; - RenderTargetConfig ToConfig() const { - auto& color_attachment = GetColorAttachments().find(0)->second; - return RenderTargetConfig{ - .size = color_attachment.texture->GetSize(), - .mip_count = color_attachment.texture->GetMipCount(), - .has_msaa = color_attachment.resolve_texture != nullptr, - .has_depth_stencil = depth_.has_value() && stencil_.has_value()}; - } + RenderTargetConfig ToConfig() const; private: - std::map colors_; + std::optional color0_; std::optional depth_; std::optional stencil_; + std::map colors_; }; /// @brief a wrapper around the impeller [Allocator] instance that can be used diff --git a/impeller/renderer/renderer_unittests.cc b/impeller/renderer/renderer_unittests.cc index 633ad6ddba897..0e84ea50cfd17 100644 --- a/impeller/renderer/renderer_unittests.cc +++ b/impeller/renderer/renderer_unittests.cc @@ -589,10 +589,6 @@ TEST_P(RendererTest, CanBlitTextureToTexture) { } pass->SetLabel("Playground Blit Pass"); - if (render_target.GetColorAttachments().empty()) { - return false; - } - // Blit `bridge` to the top left corner of the texture. pass->AddCopy(bridge, texture); @@ -709,10 +705,6 @@ TEST_P(RendererTest, CanBlitTextureToBuffer) { } pass->SetLabel("Playground Blit Pass"); - if (render_target.GetColorAttachments().empty()) { - return false; - } - // Blit `bridge` to the top left corner of the texture. pass->AddCopy(bridge, device_buffer); pass->EncodeCommands(context->GetResourceAllocator()); From 5b4d07e4aeecd98b6a5e143abbcec0ea22bf4fd0 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Tue, 26 Nov 2024 17:46:00 -0800 Subject: [PATCH 02/10] fix missing color attachments in vk render_pass. --- .../renderer/backend/vulkan/render_pass_vk.cc | 36 ++++++++++++++++--- 1 file changed, 32 insertions(+), 4 deletions(-) diff --git a/impeller/renderer/backend/vulkan/render_pass_vk.cc b/impeller/renderer/backend/vulkan/render_pass_vk.cc index 3808d84fd34c2..244795e21c912 100644 --- a/impeller/renderer/backend/vulkan/render_pass_vk.cc +++ b/impeller/renderer/backend/vulkan/render_pass_vk.cc @@ -56,6 +56,12 @@ static std::vector GetVKClearValues( const RenderTarget& target) { std::vector clears; + const ColorAttachment& color0 = target.GetColor0(); + clears.emplace_back(VKClearValueFromColor(color0.clear_color)); + if (color0.resolve_texture) { + clears.emplace_back(VKClearValueFromColor(color0.clear_color)); + } + for (const auto& [_, color] : target.GetColorAttachments()) { clears.emplace_back(VKClearValueFromColor(color.clear_color)); if (color.resolve_texture) { @@ -83,6 +89,22 @@ SharedHandleVK RenderPassVK::CreateVKRenderPass( const std::shared_ptr& command_buffer) const { RenderPassBuilderVK builder; + const ColorAttachment& color0 = render_target_.GetColor0(); + builder.SetColorAttachment( + 0u, // + color0.texture->GetTextureDescriptor().format, // + color0.texture->GetTextureDescriptor().sample_count, // + color0.load_action, // + color0.store_action, // + TextureVK::Cast(*color0.texture).GetLayout() // + ); + TextureVK::Cast(*color0.texture) + .SetLayoutWithoutEncoding(vk::ImageLayout::eGeneral); + if (color0.resolve_texture) { + TextureVK::Cast(*color0.resolve_texture) + .SetLayoutWithoutEncoding(vk::ImageLayout::eGeneral); + } + for (const auto& [bind_point, color] : render_target_.GetColorAttachments()) { builder.SetColorAttachment( bind_point, // @@ -137,10 +159,8 @@ RenderPassVK::RenderPassVK(const std::shared_ptr& context, const RenderTarget& target, std::shared_ptr command_buffer) : RenderPass(context, target), command_buffer_(std::move(command_buffer)) { - color_image_vk_ = - render_target_.GetColorAttachments().find(0u)->second.texture; - resolve_image_vk_ = - render_target_.GetColorAttachments().find(0u)->second.resolve_texture; + color_image_vk_ = render_target_.GetColor0().texture; + resolve_image_vk_ = render_target_.GetColor0().resolve_texture; const auto& vk_context = ContextVK::Cast(*context); command_buffer_vk_ = command_buffer_->GetCommandBuffer(); @@ -254,6 +274,14 @@ SharedHandleVK RenderPassVK::CreateVKFramebuffer( // This bit must be consistent to ensure compatibility with the pass created // earlier. Follow this order: Color attachments, then depth-stencil, then // stencil. + const ColorAttachment color0 = render_target_.GetColor0(); + attachments.emplace_back( + TextureVK::Cast(*color0.texture).GetRenderTargetView()); + if (color0.resolve_texture) { + attachments.emplace_back( + TextureVK::Cast(*color0.resolve_texture).GetRenderTargetView()); + } + for (const auto& [_, color] : render_target_.GetColorAttachments()) { // The bind point doesn't matter here since that information is present in // the render pass. From 24450c427295733b3ca99bcb93530eeefc8b4482 Mon Sep 17 00:00:00 2001 From: Jonah Williams Date: Tue, 26 Nov 2024 19:26:14 -0800 Subject: [PATCH 03/10] Update render_target.cc --- impeller/renderer/render_target.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/impeller/renderer/render_target.cc b/impeller/renderer/render_target.cc index 7de9db51fe0d8..80b1416e2ceb6 100644 --- a/impeller/renderer/render_target.cc +++ b/impeller/renderer/render_target.cc @@ -261,7 +261,7 @@ std::string RenderTarget::ToString() const { std::stringstream stream; if (color0_.has_value()) { - stream << SPrintF("Color[%zu]=(%s)", 0ul, + stream << SPrintF("Color[%d]=(%s)", 0, ColorAttachmentToString(color0_.value()).c_str()); } for (const auto& [index, color] : colors_) { From 61c4ff56c16765d299a718fb322d05a3d9a4cc54 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Tue, 26 Nov 2024 23:03:23 -0800 Subject: [PATCH 04/10] lint workarounds. --- .../backend/vulkan/render_pass_builder_vk_unittests.cc | 9 +++++++++ 1 file changed, 9 insertions(+) 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 d280cdb6dfc23..85d277385cbc5 100644 --- a/impeller/renderer/backend/vulkan/render_pass_builder_vk_unittests.cc +++ b/impeller/renderer/backend/vulkan/render_pass_builder_vk_unittests.cc @@ -42,6 +42,9 @@ TEST(RenderPassBuilder, RenderPassWithLoadOpUsesCurrentLayout) { auto maybe_color = builder.GetColor0(); ASSERT_TRUE(maybe_color.has_value()); + if (!maybe_color.has_value()) { + return; + } auto color = maybe_color.value(); EXPECT_EQ(color.initialLayout, vk::ImageLayout::eColorAttachmentOptimal); @@ -68,6 +71,9 @@ TEST(RenderPassBuilder, CreatesRenderPassWithCombinedDepthStencil) { auto maybe_color = builder.GetColor0(); ASSERT_TRUE(maybe_color.has_value()); + if (!maybe_color.has_value()) { + return; + } auto color = maybe_color.value(); EXPECT_EQ(color.initialLayout, vk::ImageLayout::eUndefined); @@ -137,6 +143,9 @@ TEST(RenderPassBuilder, CreatesMSAAResolveWithCorrectStore) { auto maybe_color = builder.GetColor0(); ASSERT_TRUE(maybe_color.has_value()); + if (!maybe_color.has_value()) { + return; + } auto color = maybe_color.value(); // MSAA Texture. From b6de222c1dcbe16685f4fe4fcb1ce46e5604d6fb Mon Sep 17 00:00:00 2001 From: Jonah Williams Date: Tue, 26 Nov 2024 23:41:31 -0800 Subject: [PATCH 05/10] Update render_target.cc --- impeller/renderer/render_target.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/impeller/renderer/render_target.cc b/impeller/renderer/render_target.cc index 80b1416e2ceb6..c133c74096ecc 100644 --- a/impeller/renderer/render_target.cc +++ b/impeller/renderer/render_target.cc @@ -282,7 +282,7 @@ std::string RenderTarget::ToString() const { } RenderTargetConfig RenderTarget::ToConfig() const { - if (color0_.has_value()) { + if (!color0_.has_value()) { return RenderTargetConfig{}; } const auto& color_attachment = color0_.value(); From c65046afe87988e93e67e209217bb60e5ecdcbe7 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Wed, 27 Nov 2024 11:44:13 -0800 Subject: [PATCH 06/10] ++ --- .../render_pass_builder_vk_unittests.cc | 29 +++++++++++-------- 1 file changed, 17 insertions(+), 12 deletions(-) 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 85d277385cbc5..2aa44c42de0db 100644 --- a/impeller/renderer/backend/vulkan/render_pass_builder_vk_unittests.cc +++ b/impeller/renderer/backend/vulkan/render_pass_builder_vk_unittests.cc @@ -40,12 +40,12 @@ TEST(RenderPassBuilder, RenderPassWithLoadOpUsesCurrentLayout) { EXPECT_TRUE(!!render_pass); - auto maybe_color = builder.GetColor0(); + std::optional maybe_color = builder.GetColor0(); ASSERT_TRUE(maybe_color.has_value()); if (!maybe_color.has_value()) { return; } - auto color = maybe_color.value(); + vk::AttachmentDescription color = maybe_color.value(); EXPECT_EQ(color.initialLayout, vk::ImageLayout::eColorAttachmentOptimal); EXPECT_EQ(color.finalLayout, vk::ImageLayout::eGeneral); @@ -69,24 +69,25 @@ TEST(RenderPassBuilder, CreatesRenderPassWithCombinedDepthStencil) { EXPECT_TRUE(!!render_pass); - auto maybe_color = builder.GetColor0(); + std::optional maybe_color = builder.GetColor0(); ASSERT_TRUE(maybe_color.has_value()); if (!maybe_color.has_value()) { return; } - auto color = maybe_color.value(); + vk::AttachmentDescription color = maybe_color.value(); EXPECT_EQ(color.initialLayout, vk::ImageLayout::eUndefined); 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(); + std::optional 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(); + vk::AttachmentDescription depth_stencil = maybe_depth_stencil.value(); EXPECT_EQ(depth_stencil.initialLayout, vk::ImageLayout::eUndefined); EXPECT_EQ(depth_stencil.finalLayout, @@ -112,12 +113,13 @@ TEST(RenderPassBuilder, CreatesRenderPassWithOnlyStencil) { EXPECT_TRUE(!!render_pass); - auto maybe_depth_stencil = builder.GetDepthStencil(); + std::optional 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(); + vk::AttachmentDescription depth_stencil = maybe_depth_stencil.value(); EXPECT_EQ(depth_stencil.initialLayout, vk::ImageLayout::eUndefined); EXPECT_EQ(depth_stencil.finalLayout, @@ -146,7 +148,7 @@ TEST(RenderPassBuilder, CreatesMSAAResolveWithCorrectStore) { if (!maybe_color.has_value()) { return; } - auto color = maybe_color.value(); + vk::AttachmentDescription color = maybe_color.value(); // MSAA Texture. EXPECT_EQ(color.initialLayout, vk::ImageLayout::eUndefined); @@ -154,9 +156,12 @@ TEST(RenderPassBuilder, CreatesMSAAResolveWithCorrectStore) { EXPECT_EQ(color.loadOp, vk::AttachmentLoadOp::eClear); EXPECT_EQ(color.storeOp, vk::AttachmentStoreOp::eDontCare); - auto maybe_resolve = builder.GetResolves().find(0u); - ASSERT_NE(maybe_resolve, builder.GetResolves().end()); - auto resolve = maybe_resolve->second; + auto maybe_resolve = builder.GetColor0Resolve(); + ASSERT_TRUE(maybe_resolve.has_value()); + if (!maybe_resolve.has_value()) { + return; + } + vk::AttachmentDescription resolve = maybe_resolve.value(); // MSAA Resolve Texture. EXPECT_EQ(resolve.initialLayout, vk::ImageLayout::eUndefined); From 132748a81ae9dd0a3a1e83d8b8e8542d41f734e3 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Mon, 2 Dec 2024 12:07:33 -0800 Subject: [PATCH 07/10] fixup flutter gpu. --- impeller/renderer/backend/metal/render_pass_mtl.mm | 2 +- impeller/renderer/render_target.cc | 4 ++++ impeller/renderer/render_target.h | 2 ++ lib/gpu/render_pass.cc | 4 ++++ 4 files changed, 11 insertions(+), 1 deletion(-) diff --git a/impeller/renderer/backend/metal/render_pass_mtl.mm b/impeller/renderer/backend/metal/render_pass_mtl.mm index ddda56a9ec84a..8365e6749aee8 100644 --- a/impeller/renderer/backend/metal/render_pass_mtl.mm +++ b/impeller/renderer/backend/metal/render_pass_mtl.mm @@ -105,7 +105,7 @@ static bool ConfigureStencilAttachment( auto result = [MTLRenderPassDescriptor renderPassDescriptor]; const ColorAttachment& color0 = desc.GetColor0(); - if (!ConfigureColorAttachment(color0, result.colorAttachments[0u])) { + if (!ConfigureColorAttachment(color0, result.colorAttachments[0])) { VALIDATION_LOG << "Could not configure color attachment at index 0"; return nil; } diff --git a/impeller/renderer/render_target.cc b/impeller/renderer/render_target.cc index c133c74096ecc..a280b683c91db 100644 --- a/impeller/renderer/render_target.cc +++ b/impeller/renderer/render_target.cc @@ -221,6 +221,10 @@ ColorAttachment RenderTarget::GetColor0() const { return ColorAttachment{}; } +bool RenderTarget::HasColor0() const { + return color0_.has_value(); +} + const std::map& RenderTarget::GetColorAttachments() const { return colors_; diff --git a/impeller/renderer/render_target.h b/impeller/renderer/render_target.h index b65ed33ff3e3d..0038d0901e75f 100644 --- a/impeller/renderer/render_target.h +++ b/impeller/renderer/render_target.h @@ -111,6 +111,8 @@ class RenderTarget final { ColorAttachment GetColor0() const; + bool HasColor0() const; + const std::map& GetColorAttachments() const; const std::optional& GetDepthAttachment() const; diff --git a/lib/gpu/render_pass.cc b/lib/gpu/render_pass.cc index 3987d823f5a22..154539edfaa01 100644 --- a/lib/gpu/render_pass.cc +++ b/lib/gpu/render_pass.cc @@ -105,6 +105,10 @@ RenderPass::GetOrCreatePipeline() { pipeline_desc.SetSampleCount(render_target_.GetSampleCount()); + if (render_target_.HasColor0()) { + auto& color = GetColorAttachmentDescriptor(0); + color.format = render_target_.GetRenderTargetPixelFormat(); + } for (const auto& it : render_target_.GetColorAttachments()) { auto& color = GetColorAttachmentDescriptor(it.first); color.format = render_target_.GetRenderTargetPixelFormat(); From 2f912f6263c9bbefd25c4099452dea6f1aaeed23 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Mon, 2 Dec 2024 17:34:42 -0800 Subject: [PATCH 08/10] hide specialization of color0 attachment. --- impeller/display_list/canvas.cc | 6 +- impeller/entity/entity_pass_target.cc | 2 +- .../entity/entity_pass_target_unittests.cc | 8 +- impeller/entity/inline_pass_context.cc | 5 +- impeller/entity/render_target_cache.cc | 6 +- .../entity/render_target_cache_unittests.cc | 4 +- impeller/playground/playground.cc | 2 +- .../renderer/backend/gles/render_pass_gles.cc | 2 +- .../gles/test/surface_gles_unittests.cc | 4 +- .../renderer/backend/metal/render_pass_mtl.mm | 21 ++-- .../renderer/backend/vulkan/context_vk.cc | 19 ++-- .../vulkan/render_pass_cache_unittests.cc | 2 +- .../renderer/backend/vulkan/render_pass_vk.cc | 104 +++++++----------- .../vulkan/test/swapchain_unittests.cc | 2 +- impeller/renderer/render_target.cc | 38 +++++-- impeller/renderer/render_target.h | 18 ++- lib/gpu/render_pass.cc | 15 ++- 17 files changed, 128 insertions(+), 130 deletions(-) diff --git a/impeller/display_list/canvas.cc b/impeller/display_list/canvas.cc index f311094489fa3..4aaa83372bf1a 100644 --- a/impeller/display_list/canvas.cc +++ b/impeller/display_list/canvas.cc @@ -857,7 +857,7 @@ void Canvas::DrawAtlas(const std::shared_ptr& atlas_contents, void Canvas::SetupRenderPass() { renderer_.GetRenderTargetCache()->Start(); - ColorAttachment color0 = render_target_.GetColor0(); + ColorAttachment color0 = render_target_.GetColorAttachment(0); auto& stencil_attachment = render_target_.GetStencilAttachment(); auto& depth_attachment = render_target_.GetDepthAttachment(); @@ -1464,7 +1464,7 @@ void Canvas::AddRenderEntityToCurrentPass(Entity& entity, bool reuse_depth) { RenderTarget& render_target = render_passes_.back() .inline_pass_context->GetPassTarget() .GetRenderTarget(); - ColorAttachment attachment = render_target.GetColor0(); + ColorAttachment attachment = render_target.GetColorAttachment(0); // Attachment.clear color needs to be premultiplied at all times, but the // Color::Blend function requires unpremultiplied colors. attachment.clear_color = attachment.clear_color.Unpremultiply() @@ -1591,7 +1591,7 @@ std::shared_ptr Canvas::FlipBackdrop(Point global_pass_position, } if (should_use_onscreen) { - ColorAttachment color0 = render_target_.GetColor0(); + ColorAttachment color0 = render_target_.GetColorAttachment(0); // When MSAA is being used, we end up overriding the entire backdrop by // drawing the previous pass texture, and so we don't have to clear it and // can use kDontCare. diff --git a/impeller/entity/entity_pass_target.cc b/impeller/entity/entity_pass_target.cc index a25d96e197d45..db9c917b27add 100644 --- a/impeller/entity/entity_pass_target.cc +++ b/impeller/entity/entity_pass_target.cc @@ -19,7 +19,7 @@ EntityPassTarget::EntityPassTarget(const RenderTarget& render_target, std::shared_ptr EntityPassTarget::Flip( const ContentContext& renderer) { - ColorAttachment color0 = target_.GetColor0(); + ColorAttachment color0 = target_.GetColorAttachment(0); if (!color0.resolve_texture) { VALIDATION_LOG << "EntityPassTarget Flip should never be called for a " "non-MSAA target."; diff --git a/impeller/entity/entity_pass_target_unittests.cc b/impeller/entity/entity_pass_target_unittests.cc index 28aa97d3fd9b3..4a72138a70e19 100644 --- a/impeller/entity/entity_pass_target_unittests.cc +++ b/impeller/entity/entity_pass_target_unittests.cc @@ -31,14 +31,14 @@ TEST_P(EntityPassTargetTest, SwapWithMSAATexture) { auto entity_pass_target = EntityPassTarget(render_target, false, false); - auto color0 = entity_pass_target.GetRenderTarget().GetColor0(); + auto color0 = entity_pass_target.GetRenderTarget().GetColorAttachment(0); auto msaa_tex = color0.texture; auto resolve_tex = color0.resolve_texture; FML_DCHECK(content_context); entity_pass_target.Flip(*content_context); - color0 = entity_pass_target.GetRenderTarget().GetColor0(); + color0 = entity_pass_target.GetRenderTarget().GetColorAttachment(0); ASSERT_EQ(msaa_tex, color0.texture); ASSERT_NE(resolve_tex, color0.resolve_texture); @@ -83,7 +83,7 @@ TEST_P(EntityPassTargetTest, SwapWithMSAAImplicitResolve) { auto entity_pass_target = EntityPassTarget(render_target, false, true); - auto color0 = entity_pass_target.GetRenderTarget().GetColor0(); + auto color0 = entity_pass_target.GetRenderTarget().GetColorAttachment(0); auto msaa_tex = color0.texture; auto resolve_tex = color0.resolve_texture; @@ -92,7 +92,7 @@ TEST_P(EntityPassTargetTest, SwapWithMSAAImplicitResolve) { FML_DCHECK(content_context); entity_pass_target.Flip(*content_context); - color0 = entity_pass_target.GetRenderTarget().GetColor0(); + color0 = entity_pass_target.GetRenderTarget().GetColorAttachment(0); ASSERT_NE(msaa_tex, color0.texture); ASSERT_NE(resolve_tex, color0.resolve_texture); diff --git a/impeller/entity/inline_pass_context.cc b/impeller/entity/inline_pass_context.cc index 8c00550d272de..82d94a614ec5d 100644 --- a/impeller/entity/inline_pass_context.cc +++ b/impeller/entity/inline_pass_context.cc @@ -91,7 +91,8 @@ const std::shared_ptr& InlinePassContext::GetRenderPass() { { // If the pass target has a resolve texture, then we're using MSAA. bool is_msaa = - pass_target_.GetRenderTarget().GetColor0().resolve_texture != nullptr; + pass_target_.GetRenderTarget().GetColorAttachment(0).resolve_texture != + nullptr; if (pass_count_ > 0 && is_msaa) { pass_target_.Flip(renderer_); } @@ -99,7 +100,7 @@ const std::shared_ptr& InlinePassContext::GetRenderPass() { // Find the color attachment a second time, since the target may have just // flipped. - ColorAttachment color0 = pass_target_.GetRenderTarget().GetColor0(); + ColorAttachment color0 = pass_target_.GetRenderTarget().GetColorAttachment(0); bool is_msaa = color0.resolve_texture != nullptr; if (pass_count_ > 0) { diff --git a/impeller/entity/render_target_cache.cc b/impeller/entity/render_target_cache.cc index fb9c8098f4b82..43c85fcbed790 100644 --- a/impeller/entity/render_target_cache.cc +++ b/impeller/entity/render_target_cache.cc @@ -53,7 +53,8 @@ RenderTarget RenderTargetCache::CreateOffscreen( const auto other_config = render_target_data.config; if (!render_target_data.used_this_frame && other_config == config) { render_target_data.used_this_frame = true; - ColorAttachment color0 = render_target_data.render_target.GetColor0(); + ColorAttachment color0 = + render_target_data.render_target.GetColorAttachment(0); std::optional depth = render_target_data.render_target.GetDepthAttachment(); std::shared_ptr depth_tex = depth ? depth->texture : nullptr; @@ -102,7 +103,8 @@ RenderTarget RenderTargetCache::CreateOffscreenMSAA( const auto other_config = render_target_data.config; if (!render_target_data.used_this_frame && other_config == config) { render_target_data.used_this_frame = true; - ColorAttachment color0 = render_target_data.render_target.GetColor0(); + ColorAttachment color0 = + render_target_data.render_target.GetColorAttachment(0); std::optional depth = render_target_data.render_target.GetDepthAttachment(); std::shared_ptr depth_tex = depth ? depth->texture : nullptr; diff --git a/impeller/entity/render_target_cache_unittests.cc b/impeller/entity/render_target_cache_unittests.cc index 17ea9338f11e0..2684e322c587e 100644 --- a/impeller/entity/render_target_cache_unittests.cc +++ b/impeller/entity/render_target_cache_unittests.cc @@ -105,8 +105,8 @@ TEST_P(RenderTargetCacheTest, CachedTextureGetsNewAttachmentConfig) { *GetContext(), {100, 100}, 1, "Offscreen2", color_attachment_config); render_target_cache.End(); - ColorAttachment color1 = target1.GetColor0(); - ColorAttachment color2 = target2.GetColor0(); + ColorAttachment color1 = target1.GetColorAttachment(0); + ColorAttachment color2 = target2.GetColorAttachment(0); // The second color attachment should reuse the first attachment's texture // but with attributes from the second AttachmentConfig. EXPECT_EQ(color2.texture, color1.texture); diff --git a/impeller/playground/playground.cc b/impeller/playground/playground.cc index fc7cbcd3140ea..1ef9f17f134d8 100644 --- a/impeller/playground/playground.cc +++ b/impeller/playground/playground.cc @@ -284,7 +284,7 @@ bool Playground::OpenPlaygroundHere( } buffer->SetLabel("ImGui Command Buffer"); - auto color0 = render_target.GetColor0(); + auto color0 = render_target.GetColorAttachment(0); color0.load_action = LoadAction::kLoad; if (color0.resolve_texture) { color0.texture = color0.resolve_texture; diff --git a/impeller/renderer/backend/gles/render_pass_gles.cc b/impeller/renderer/backend/gles/render_pass_gles.cc index 6c9d45020dae8..0da1c79c7bd6c 100644 --- a/impeller/renderer/backend/gles/render_pass_gles.cc +++ b/impeller/renderer/backend/gles/render_pass_gles.cc @@ -538,7 +538,7 @@ bool RenderPassGLES::OnEncodeCommands(const Context& context) const { if (!render_target.HasColorAttachment(0u)) { return false; } - const ColorAttachment& color0 = render_target.GetColor0(); + const ColorAttachment& color0 = render_target.GetColorAttachment(0); const std::optional& depth0 = render_target.GetDepthAttachment(); const std::optional& stencil0 = diff --git a/impeller/renderer/backend/gles/test/surface_gles_unittests.cc b/impeller/renderer/backend/gles/test/surface_gles_unittests.cc index 14b90907e7d81..a039e03786c22 100644 --- a/impeller/renderer/backend/gles/test/surface_gles_unittests.cc +++ b/impeller/renderer/backend/gles/test/surface_gles_unittests.cc @@ -21,8 +21,8 @@ TEST_P(SurfaceGLESTest, CanWrapNonZeroFBO) { ASSERT_TRUE(!!surface); ASSERT_TRUE(surface->IsValid()); ASSERT_TRUE(surface->GetRenderTarget().HasColorAttachment(0)); - const auto& texture = - TextureGLES::Cast(*(surface->GetRenderTarget().GetColor0().texture)); + const auto& texture = TextureGLES::Cast( + *(surface->GetRenderTarget().GetColorAttachment(0).texture)); auto wrapped = texture.GetFBO(); ASSERT_TRUE(wrapped.has_value()); // NOLINTNEXTLINE(bugprone-unchecked-optional-access) diff --git a/impeller/renderer/backend/metal/render_pass_mtl.mm b/impeller/renderer/backend/metal/render_pass_mtl.mm index 8365e6749aee8..cd0fbbeb0219c 100644 --- a/impeller/renderer/backend/metal/render_pass_mtl.mm +++ b/impeller/renderer/backend/metal/render_pass_mtl.mm @@ -104,22 +104,17 @@ static bool ConfigureStencilAttachment( const RenderTarget& desc) { auto result = [MTLRenderPassDescriptor renderPassDescriptor]; - const ColorAttachment& color0 = desc.GetColor0(); - if (!ConfigureColorAttachment(color0, result.colorAttachments[0])) { - VALIDATION_LOG << "Could not configure color attachment at index 0"; + bool configured_attachment = desc.IterateAllColorAttachments( + [&result](size_t index, const ColorAttachment& attachment) -> bool { + return ConfigureColorAttachment(attachment, + result.colorAttachments[index]); + }); + + if (!configured_attachment) { + VALIDATION_LOG << "Could not configure color attachments"; return nil; } - const auto& colors = desc.GetColorAttachments(); - for (const auto& color : colors) { - if (!ConfigureColorAttachment(color.second, - result.colorAttachments[color.first])) { - VALIDATION_LOG << "Could not configure color attachment at index " - << color.first; - return nil; - } - } - const auto& depth = desc.GetDepthAttachment(); if (depth.has_value() && diff --git a/impeller/renderer/backend/vulkan/context_vk.cc b/impeller/renderer/backend/vulkan/context_vk.cc index 8963fae083654..62da661cf7e84 100644 --- a/impeller/renderer/backend/vulkan/context_vk.cc +++ b/impeller/renderer/backend/vulkan/context_vk.cc @@ -668,14 +668,17 @@ void ContextVK::InitializeCommonlyUsedShadersIfNeeded() const { RenderPassBuilderVK builder; - const ColorAttachment& color = render_target.GetColor0(); - builder.SetColorAttachment( - 0u, // - color.texture->GetTextureDescriptor().format, // - color.texture->GetTextureDescriptor().sample_count, // - color.load_action, // - color.store_action // - ); + render_target.IterateAllColorAttachments( + [&builder](size_t index, const ColorAttachment& attachment) -> bool { + builder.SetColorAttachment( + index, // + attachment.texture->GetTextureDescriptor().format, // + attachment.texture->GetTextureDescriptor().sample_count, // + attachment.load_action, // + attachment.store_action // + ); + return true; + }); if (auto depth = render_target.GetDepthAttachment(); depth.has_value()) { builder.SetDepthStencilAttachment( diff --git a/impeller/renderer/backend/vulkan/render_pass_cache_unittests.cc b/impeller/renderer/backend/vulkan/render_pass_cache_unittests.cc index 51d42aee512b8..f4b470ebe2a3f 100644 --- a/impeller/renderer/backend/vulkan/render_pass_cache_unittests.cc +++ b/impeller/renderer/backend/vulkan/render_pass_cache_unittests.cc @@ -23,7 +23,7 @@ TEST_P(RendererTest, CachesRenderPassAndFramebuffer) { auto render_target = allocator->CreateOffscreenMSAA(*GetContext(), {100, 100}, 1); - auto resolve_texture = render_target.GetColor0().resolve_texture; + auto resolve_texture = render_target.GetColorAttachment(0).resolve_texture; auto& texture_vk = TextureVK::Cast(*resolve_texture); EXPECT_EQ(texture_vk.GetCachedFramebuffer(), nullptr); diff --git a/impeller/renderer/backend/vulkan/render_pass_vk.cc b/impeller/renderer/backend/vulkan/render_pass_vk.cc index 244795e21c912..1bbebd409cce6 100644 --- a/impeller/renderer/backend/vulkan/render_pass_vk.cc +++ b/impeller/renderer/backend/vulkan/render_pass_vk.cc @@ -56,18 +56,14 @@ static std::vector GetVKClearValues( const RenderTarget& target) { std::vector clears; - const ColorAttachment& color0 = target.GetColor0(); - clears.emplace_back(VKClearValueFromColor(color0.clear_color)); - if (color0.resolve_texture) { - clears.emplace_back(VKClearValueFromColor(color0.clear_color)); - } - - for (const auto& [_, color] : target.GetColorAttachments()) { - clears.emplace_back(VKClearValueFromColor(color.clear_color)); - if (color.resolve_texture) { - clears.emplace_back(VKClearValueFromColor(color.clear_color)); - } - } + target.IterateAllColorAttachments( + [&clears](size_t index, const ColorAttachment& attachment) -> bool { + clears.emplace_back(VKClearValueFromColor(attachment.clear_color)); + if (attachment.resolve_texture) { + clears.emplace_back(VKClearValueFromColor(attachment.clear_color)); + } + return true; + }); const auto& depth = target.GetDepthAttachment(); const auto& stencil = target.GetStencilAttachment(); @@ -89,38 +85,24 @@ SharedHandleVK RenderPassVK::CreateVKRenderPass( const std::shared_ptr& command_buffer) const { RenderPassBuilderVK builder; - const ColorAttachment& color0 = render_target_.GetColor0(); - builder.SetColorAttachment( - 0u, // - color0.texture->GetTextureDescriptor().format, // - color0.texture->GetTextureDescriptor().sample_count, // - color0.load_action, // - color0.store_action, // - TextureVK::Cast(*color0.texture).GetLayout() // - ); - TextureVK::Cast(*color0.texture) - .SetLayoutWithoutEncoding(vk::ImageLayout::eGeneral); - if (color0.resolve_texture) { - TextureVK::Cast(*color0.resolve_texture) - .SetLayoutWithoutEncoding(vk::ImageLayout::eGeneral); - } - - for (const auto& [bind_point, color] : render_target_.GetColorAttachments()) { - builder.SetColorAttachment( - bind_point, // - color.texture->GetTextureDescriptor().format, // - color.texture->GetTextureDescriptor().sample_count, // - color.load_action, // - color.store_action, // - TextureVK::Cast(*color.texture).GetLayout() // - ); - TextureVK::Cast(*color.texture) - .SetLayoutWithoutEncoding(vk::ImageLayout::eGeneral); - if (color.resolve_texture) { - TextureVK::Cast(*color.resolve_texture) - .SetLayoutWithoutEncoding(vk::ImageLayout::eGeneral); - } - } + render_target_.IterateAllColorAttachments( + [&](size_t bind_point, const ColorAttachment& attachment) -> bool { + builder.SetColorAttachment( + bind_point, // + attachment.texture->GetTextureDescriptor().format, // + attachment.texture->GetTextureDescriptor().sample_count, // + attachment.load_action, // + attachment.store_action, // + TextureVK::Cast(*attachment.texture).GetLayout() // + ); + TextureVK::Cast(*attachment.texture) + .SetLayoutWithoutEncoding(vk::ImageLayout::eGeneral); + if (attachment.resolve_texture) { + TextureVK::Cast(*attachment.resolve_texture) + .SetLayoutWithoutEncoding(vk::ImageLayout::eGeneral); + } + return true; + }); if (auto depth = render_target_.GetDepthAttachment(); depth.has_value()) { builder.SetDepthStencilAttachment( @@ -159,8 +141,9 @@ RenderPassVK::RenderPassVK(const std::shared_ptr& context, const RenderTarget& target, std::shared_ptr command_buffer) : RenderPass(context, target), command_buffer_(std::move(command_buffer)) { - color_image_vk_ = render_target_.GetColor0().texture; - resolve_image_vk_ = render_target_.GetColor0().resolve_texture; + const ColorAttachment& color0 = render_target_.GetColorAttachment(0); + color_image_vk_ = color0.texture; + resolve_image_vk_ = color0.resolve_texture; const auto& vk_context = ContextVK::Cast(*context); command_buffer_vk_ = command_buffer_->GetCommandBuffer(); @@ -274,24 +257,19 @@ SharedHandleVK RenderPassVK::CreateVKFramebuffer( // This bit must be consistent to ensure compatibility with the pass created // earlier. Follow this order: Color attachments, then depth-stencil, then // stencil. - const ColorAttachment color0 = render_target_.GetColor0(); - attachments.emplace_back( - TextureVK::Cast(*color0.texture).GetRenderTargetView()); - if (color0.resolve_texture) { - attachments.emplace_back( - TextureVK::Cast(*color0.resolve_texture).GetRenderTargetView()); - } + render_target_.IterateAllColorAttachments( + [&attachments](size_t index, const ColorAttachment& attachment) -> bool { + // The bind point doesn't matter here since that information is present + // in the render pass. + attachments.emplace_back( + TextureVK::Cast(*attachment.texture).GetRenderTargetView()); + if (attachment.resolve_texture) { + attachments.emplace_back(TextureVK::Cast(*attachment.resolve_texture) + .GetRenderTargetView()); + } + return true; + }); - for (const auto& [_, color] : render_target_.GetColorAttachments()) { - // The bind point doesn't matter here since that information is present in - // the render pass. - attachments.emplace_back( - TextureVK::Cast(*color.texture).GetRenderTargetView()); - if (color.resolve_texture) { - attachments.emplace_back( - TextureVK::Cast(*color.resolve_texture).GetRenderTargetView()); - } - } if (auto depth = render_target_.GetDepthAttachment(); depth.has_value()) { attachments.emplace_back( TextureVK::Cast(*depth->texture).GetRenderTargetView()); diff --git a/impeller/renderer/backend/vulkan/test/swapchain_unittests.cc b/impeller/renderer/backend/vulkan/test/swapchain_unittests.cc index 9594e2d75103a..4347df46aa561 100644 --- a/impeller/renderer/backend/vulkan/test/swapchain_unittests.cc +++ b/impeller/renderer/backend/vulkan/test/swapchain_unittests.cc @@ -86,7 +86,7 @@ TEST(SwapchainTest, CachesRenderPassOnSwapchainImage) { auto& depth = render_target.GetDepthAttachment(); depth_stencil_textures.push_back(depth.has_value() ? depth->texture : nullptr); - msaa_textures.push_back(render_target.GetColor0().texture); + msaa_textures.push_back(render_target.GetColorAttachment(0).texture); } for (auto i = 1; i < 3; i++) { diff --git a/impeller/renderer/render_target.cc b/impeller/renderer/render_target.cc index a280b683c91db..b3621e706b659 100644 --- a/impeller/renderer/render_target.cc +++ b/impeller/renderer/render_target.cc @@ -93,6 +93,22 @@ bool RenderTarget::IsValid() const { return true; } +bool RenderTarget::IterateAllColorAttachments( + const std::function& + iterator) const { + if (color0_.has_value()) { + if (!iterator(0, color0_.value())) { + return false; + } + } + for (const auto& [index, attachment] : colors_) { + if (!iterator(index, attachment)) { + return false; + } + } + return true; +} + void RenderTarget::IterateAllAttachments( const std::function& iterator) const { if (color0_.has_value()) { @@ -214,22 +230,20 @@ RenderTarget& RenderTarget::SetStencilAttachment( return *this; } -ColorAttachment RenderTarget::GetColor0() const { - if (color0_.has_value()) { - return color0_.value(); +ColorAttachment RenderTarget::GetColorAttachment(size_t index) const { + if (index == 0) { + if (color0_.has_value()) { + return color0_.value(); + } + return ColorAttachment{}; + } + std::map::const_iterator it = colors_.find(index); + if (it != colors_.end()) { + return it->second; } return ColorAttachment{}; } -bool RenderTarget::HasColor0() const { - return color0_.has_value(); -} - -const std::map& RenderTarget::GetColorAttachments() - const { - return colors_; -} - const std::optional& RenderTarget::GetDepthAttachment() const { return depth_; } diff --git a/impeller/renderer/render_target.h b/impeller/renderer/render_target.h index 0038d0901e75f..e7d1fb73df60c 100644 --- a/impeller/renderer/render_target.h +++ b/impeller/renderer/render_target.h @@ -102,6 +102,13 @@ class RenderTarget final { RenderTarget& SetColorAttachment(const ColorAttachment& attachment, size_t index); + /// @brief Get the color attachment at [index]. + /// + /// This function does not validate whether or not the attachment was + /// previously defined and will return a default constructed attachment + /// if none is set. + ColorAttachment GetColorAttachment(size_t index) const; + RenderTarget& SetDepthAttachment(std::optional attachment); RenderTarget& SetStencilAttachment( @@ -109,18 +116,17 @@ class RenderTarget final { size_t GetMaxColorAttacmentBindIndex() const; - ColorAttachment GetColor0() const; - - bool HasColor0() const; - - const std::map& GetColorAttachments() const; - const std::optional& GetDepthAttachment() const; const std::optional& GetStencilAttachment() const; size_t GetTotalAttachmentCount() const; + bool IterateAllColorAttachments( + const std::function& iterator) + const; + void IterateAllAttachments( const std::function& iterator) const; diff --git a/lib/gpu/render_pass.cc b/lib/gpu/render_pass.cc index 154539edfaa01..a566567a9484f 100644 --- a/lib/gpu/render_pass.cc +++ b/lib/gpu/render_pass.cc @@ -105,14 +105,13 @@ RenderPass::GetOrCreatePipeline() { pipeline_desc.SetSampleCount(render_target_.GetSampleCount()); - if (render_target_.HasColor0()) { - auto& color = GetColorAttachmentDescriptor(0); - color.format = render_target_.GetRenderTargetPixelFormat(); - } - for (const auto& it : render_target_.GetColorAttachments()) { - auto& color = GetColorAttachmentDescriptor(it.first); - color.format = render_target_.GetRenderTargetPixelFormat(); - } + render_target_.IterateAllColorAttachments( + [&](size_t index, const impeller::ColorAttachment& attachment) -> bool { + auto& color = GetColorAttachmentDescriptor(index); + color.format = render_target_.GetRenderTargetPixelFormat(); + return true; + }); + pipeline_desc.SetColorAttachmentDescriptors(color_descriptors_); { From 52d2674a859a95e2229f2e036ad3752cc2fa871e Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Mon, 2 Dec 2024 17:36:22 -0800 Subject: [PATCH 09/10] add comment. --- impeller/renderer/render_target.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/impeller/renderer/render_target.h b/impeller/renderer/render_target.h index e7d1fb73df60c..a7a1e0bd32c9a 100644 --- a/impeller/renderer/render_target.h +++ b/impeller/renderer/render_target.h @@ -138,6 +138,9 @@ class RenderTarget final { std::optional color0_; std::optional depth_; std::optional stencil_; + // Note: color0 is stored in the color0_ field above and not in this map, + // to avoid heap allocations for the commonly created render target formats + // in Flutter. std::map colors_; }; From 21902829d3e3412e5fc1f04bb7fb391a14748f2e Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Tue, 3 Dec 2024 09:20:44 -0800 Subject: [PATCH 10/10] ++ --- impeller/renderer/backend/vulkan/render_pass_builder_vk.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/impeller/renderer/backend/vulkan/render_pass_builder_vk.h b/impeller/renderer/backend/vulkan/render_pass_builder_vk.h index 51c6313ec9583..4e9632d9a6d13 100644 --- a/impeller/renderer/backend/vulkan/render_pass_builder_vk.h +++ b/impeller/renderer/backend/vulkan/render_pass_builder_vk.h @@ -8,6 +8,7 @@ #include #include +#include "flutter/fml/macros.h" #include "impeller/core/formats.h" #include "impeller/renderer/backend/vulkan/context_vk.h" #include "impeller/renderer/backend/vulkan/vk.h" @@ -65,6 +66,7 @@ class RenderPassBuilderVK { std::optional color0_resolve_; std::optional depth_stencil_; + // Color attachment 0 is stored in the field above and not in these maps. std::map colors_; std::map resolves_; };