Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions impeller/display_list/canvas.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -856,7 +857,7 @@ void Canvas::DrawAtlas(const std::shared_ptr<AtlasContents>& atlas_contents,

void Canvas::SetupRenderPass() {
renderer_.GetRenderTargetCache()->Start();
auto color0 = render_target_.GetColorAttachments().find(0u)->second;
ColorAttachment color0 = render_target_.GetColorAttachment(0);

auto& stencil_attachment = render_target_.GetStencilAttachment();
auto& depth_attachment = render_target_.GetDepthAttachment();
Expand Down Expand Up @@ -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.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()
Expand Down Expand Up @@ -1591,8 +1591,7 @@ std::shared_ptr<Texture> Canvas::FlipBackdrop(Point global_pass_position,
}

if (should_use_onscreen) {
ColorAttachment color0 =
render_target_.GetColorAttachments().find(0u)->second;
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.
Expand Down
2 changes: 1 addition & 1 deletion impeller/entity/entity_pass_target.cc
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ EntityPassTarget::EntityPassTarget(const RenderTarget& render_target,

std::shared_ptr<Texture> EntityPassTarget::Flip(
const ContentContext& renderer) {
auto color0 = target_.GetColorAttachments().find(0)->second;
ColorAttachment color0 = target_.GetColorAttachment(0);
if (!color0.resolve_texture) {
VALIDATION_LOG << "EntityPassTarget Flip should never be called for a "
"non-MSAA target.";
Expand Down
20 changes: 4 additions & 16 deletions impeller/entity/entity_pass_target_unittests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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().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()
.GetColorAttachments()
.find(0u)
->second;
color0 = entity_pass_target.GetRenderTarget().GetColorAttachment(0);

ASSERT_EQ(msaa_tex, color0.texture);
ASSERT_NE(resolve_tex, color0.resolve_texture);
Expand Down Expand Up @@ -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)
Comment on lines -93 to -94
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we instead change this to .GetColorAttachment(0u). That way the way the render target is storing the first color attachment isn't being leaked to its clients.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

->second;
auto color0 = entity_pass_target.GetRenderTarget().GetColorAttachment(0);
auto msaa_tex = color0.texture;
auto resolve_tex = color0.resolve_texture;

Expand All @@ -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().GetColorAttachment(0);

ASSERT_NE(msaa_tex, color0.texture);
ASSERT_NE(resolve_tex, color0.resolve_texture);
Expand Down
16 changes: 4 additions & 12 deletions impeller/entity/inline_pass_context.cc
Original file line number Diff line number Diff line change
Expand Up @@ -86,29 +86,21 @@ const std::shared_ptr<RenderPass>& 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().GetColorAttachment(0).resolve_texture !=
nullptr;
if (pass_count_ > 0 && is_msaa) {
pass_target_.Flip(renderer_);
}
}

// 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().GetColorAttachment(0);
bool is_msaa = color0.resolve_texture != nullptr;

if (pass_count_ > 0) {
Expand Down
17 changes: 9 additions & 8 deletions impeller/entity/render_target_cache.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -52,10 +53,10 @@ 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.GetColorAttachment(0);
std::optional<DepthAttachment> depth =
render_target_data.render_target.GetDepthAttachment();
std::shared_ptr<Texture> depth_tex = depth ? depth->texture : nullptr;
return RenderTargetAllocator::CreateOffscreen(
context, size, mip_count, label, color_attachment_config,
Expand Down Expand Up @@ -102,10 +103,10 @@ 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.GetColorAttachment(0);
std::optional<DepthAttachment> depth =
render_target_data.render_target.GetDepthAttachment();
std::shared_ptr<Texture> depth_tex = depth ? depth->texture : nullptr;
return RenderTargetAllocator::CreateOffscreenMSAA(
context, size, mip_count, label, color_attachment_config,
Expand Down
5 changes: 3 additions & 2 deletions impeller/entity/render_target_cache_unittests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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.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);
Expand Down
8 changes: 1 addition & 7 deletions impeller/playground/playground.cc
Original file line number Diff line number Diff line change
Expand Up @@ -284,20 +284,14 @@ 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.GetColorAttachment(0);
color0.load_action = LoadAction::kLoad;
if (color0.resolve_texture) {
color0.texture = color0.resolve_texture;
color0.resolve_texture = nullptr;
color0.store_action = StoreAction::kStore;
}
render_target.SetColorAttachment(color0, 0);

render_target.SetStencilAttachment(std::nullopt);
render_target.SetDepthAttachment(std::nullopt);

Expand Down
9 changes: 6 additions & 3 deletions impeller/renderer/backend/gles/render_pass_gles.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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.GetColorAttachment(0);
const std::optional<DepthAttachment>& depth0 =
render_target.GetDepthAttachment();
const std::optional<StencilAttachment>& stencil0 =
render_target.GetStencilAttachment();

auto pass_data = std::make_shared<RenderPassData>();
pass_data->label = label_;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ TEST_P(SurfaceGLESTest, CanWrapNonZeroFBO) {
ASSERT_TRUE(surface->IsValid());
ASSERT_TRUE(surface->GetRenderTarget().HasColorAttachment(0));
const auto& texture = TextureGLES::Cast(
*(surface->GetRenderTarget().GetColorAttachments().at(0).texture));
*(surface->GetRenderTarget().GetColorAttachment(0).texture));
auto wrapped = texture.GetFBO();
ASSERT_TRUE(wrapped.has_value());
// NOLINTNEXTLINE(bugprone-unchecked-optional-access)
Expand Down
18 changes: 9 additions & 9 deletions impeller/renderer/backend/metal/render_pass_mtl.mm
Original file line number Diff line number Diff line change
Expand Up @@ -104,15 +104,15 @@ static bool ConfigureStencilAttachment(
const RenderTarget& desc) {
auto result = [MTLRenderPassDescriptor renderPassDescriptor];

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;
}
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& depth = desc.GetDepthAttachment();
Expand Down
22 changes: 13 additions & 9 deletions impeller/renderer/backend/vulkan/context_vk.cc
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include <unordered_map>

#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"
Expand Down Expand Up @@ -666,15 +667,18 @@ 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 //
);
}

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(
Expand Down
58 changes: 51 additions & 7 deletions impeller/renderer/backend/vulkan/render_pass_builder_vk.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -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<vk::AttachmentDescription> attachments;

Expand All @@ -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());
}
Comment on lines +131 to +143
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove duplication

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't you use the Iterate method here to avoid the duplication?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't the render_target, this is the internal implementation of the RenderPassBuilderVK

}

for (const auto& color : colors_) {
vk::AttachmentReference color_ref;
color_ref.attachment = attachments.size();
Expand Down Expand Up @@ -207,4 +239,16 @@ RenderPassBuilderVK::GetDepthStencil() const {
return depth_stencil_;
}

// Visible for testing.
std::optional<vk::AttachmentDescription> RenderPassBuilderVK::GetColor0()
const {
return color0_;
}

// Visible for testing.
std::optional<vk::AttachmentDescription> RenderPassBuilderVK::GetColor0Resolve()
const {
return color0_resolve_;
}

} // namespace impeller
Loading
Loading