Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[Impeller] allocate the impeller onscreen texture from the render target cache. #55943

Merged
merged 5 commits into from
Oct 18, 2024
Merged

[Impeller] allocate the impeller onscreen texture from the render target cache. #55943

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d1800ba
[Impeller] allocate the impeller onscreen texture from the render tar…
Oct 18, 2024
ff7bc16
Merge branch 'main' of github.com:flutter/engine into allocation_from…
Oct 18, 2024
b6053b7
Merge branch 'main' into allocation_from_rt_cache
Oct 18, 2024
a6b6b32
add null checks.
Oct 18, 2024
302ca9b
Merge branch 'allocation_from_rt_cache' of github.com:jonahwilliams/e…
Oct 18, 2024
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
33 changes: 16 additions & 17 deletions impeller/display_list/canvas.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ static std::shared_ptr<Texture> FlipBackdrop(
EntityPassClipStack& clip_coverage_stack,
ContentContext& renderer,
bool should_remove_texture = false) {
auto rendering_config = std::move(render_passes.back());
LazyRenderingConfig rendering_config = std::move(render_passes.back());
render_passes.pop_back();

// If the very first thing we render in this EntityPass is a subpass that
Expand All @@ -119,7 +119,7 @@ static std::shared_ptr<Texture> FlipBackdrop(
// In cases where there are no contents, we
// could instead check the clear color and initialize a 1x2 CPU texture
// instead of ending the pass.
rendering_config.inline_pass_context->GetRenderPass(0);
rendering_config.inline_pass_context->GetRenderPass();
if (!rendering_config.inline_pass_context->EndPass()) {
VALIDATION_LOG
<< "Failed to end the current render pass in order to read from "
Expand All @@ -134,7 +134,7 @@ static std::shared_ptr<Texture> FlipBackdrop(
return nullptr;
}

std::shared_ptr<Texture> input_texture =
const std::shared_ptr<Texture>& input_texture =
rendering_config.inline_pass_context->GetTexture();

if (!input_texture) {
Expand All @@ -157,7 +157,7 @@ static std::shared_ptr<Texture> FlipBackdrop(
render_passes.back().entity_pass_target->RemoveSecondary();
}
RenderPass& current_render_pass =
*render_passes.back().inline_pass_context->GetRenderPass(0).pass;
*render_passes.back().inline_pass_context->GetRenderPass();

// Eagerly restore the BDF contents.

Expand Down Expand Up @@ -1202,7 +1202,6 @@ void Canvas::SaveLayer(const Paint& paint,
backdrop_entity.SetTransform(
Matrix::MakeTranslation(Vector3(-local_position)));
backdrop_entity.SetClipDepth(std::numeric_limits<uint32_t>::max());

backdrop_entity.Render(renderer_, GetCurrentRenderPass());
}

Expand Down Expand Up @@ -1241,7 +1240,7 @@ bool Canvas::Restore() {
auto lazy_render_pass = std::move(render_passes_.back());
render_passes_.pop_back();
// Force the render pass to be constructed if it never was.
lazy_render_pass.inline_pass_context->GetRenderPass(0);
lazy_render_pass.inline_pass_context->GetRenderPass();

SaveLayerState save_layer_state = save_layer_state_.back();
save_layer_state_.pop_back();
Expand Down Expand Up @@ -1315,8 +1314,8 @@ bool Canvas::Restore() {
}

element_entity.Render(
renderer_, //
*render_passes_.back().inline_pass_context->GetRenderPass(0).pass //
renderer_, //
*render_passes_.back().inline_pass_context->GetRenderPass() //
);
clip_coverage_stack_.PopSubpass();
transform_stack_.pop_back();
Expand Down Expand Up @@ -1363,9 +1362,9 @@ bool Canvas::Restore() {
if (clip_state_result.clip_did_change) {
// We only need to update the pass scissor if the clip state has changed.
SetClipScissor(
clip_coverage_stack_.CurrentClipCoverage(), //
*render_passes_.back().inline_pass_context->GetRenderPass(0).pass, //
GetGlobalPassPosition() //
clip_coverage_stack_.CurrentClipCoverage(), //
*render_passes_.back().inline_pass_context->GetRenderPass(), //
GetGlobalPassPosition() //
);
}

Expand Down Expand Up @@ -1580,16 +1579,16 @@ void Canvas::AddRenderEntityToCurrentPass(Entity& entity, bool reuse_depth) {
}
}

InlinePassContext::RenderPassResult result =
render_passes_.back().inline_pass_context->GetRenderPass(0);
if (!result.pass) {
const std::shared_ptr<RenderPass>& result =
render_passes_.back().inline_pass_context->GetRenderPass();
if (!result) {
// Failure to produce a render pass should be explained by specific errors
// in `InlinePassContext::GetRenderPass()`, so avoid log spam and don't
// append a validation log here.
return;
}

entity.Render(renderer_, *result.pass);
entity.Render(renderer_, *result);
Copy link
Member

@gaaclarke gaaclarke Oct 18, 2024

Choose a reason for hiding this comment

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

Add a FML_DCHECK that we can dereference this.

Copy link
Contributor Author

@jonahwilliams jonahwilliams Oct 18, 2024

Choose a reason for hiding this comment

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

its null checked on the previous line?

}

void Canvas::AddClipEntityToCurrentPass(Entity& entity) {
Expand Down Expand Up @@ -1647,7 +1646,7 @@ void Canvas::AddClipEntityToCurrentPass(Entity& entity) {
}

RenderPass& Canvas::GetCurrentRenderPass() const {
return *render_passes_.back().inline_pass_context->GetRenderPass(0).pass;
return *render_passes_.back().inline_pass_context->GetRenderPass();
}

void Canvas::SetBackdropData(
Expand Down Expand Up @@ -1716,7 +1715,7 @@ bool Canvas::BlitToOnscreen() {

void Canvas::EndReplay() {
FML_DCHECK(render_passes_.size() == 1u);
render_passes_.back().inline_pass_context->GetRenderPass(0);
render_passes_.back().inline_pass_context->GetRenderPass();
render_passes_.back().inline_pass_context->EndPass();
backdrop_data_.clear();

Expand Down
2 changes: 1 addition & 1 deletion impeller/display_list/canvas.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ struct LazyRenderingConfig {
std::unique_ptr<EntityPassTarget> p_entity_pass_target)
: entity_pass_target(std::move(p_entity_pass_target)) {
inline_pass_context =
std::make_unique<InlinePassContext>(renderer, *entity_pass_target, 0);
std::make_unique<InlinePassContext>(renderer, *entity_pass_target);
}

LazyRenderingConfig(ContentContext& renderer,
Expand Down
7 changes: 5 additions & 2 deletions impeller/entity/entity_pass_target.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ EntityPassTarget::EntityPassTarget(const RenderTarget& render_target,
supports_read_from_resolve_(supports_read_from_resolve),
supports_implicit_msaa_(supports_implicit_msaa) {}

std::shared_ptr<Texture> EntityPassTarget::Flip(Allocator& allocator) {
std::shared_ptr<Texture> EntityPassTarget::Flip(
const ContentContext& renderer) {
auto color0 = target_.GetColorAttachments().find(0)->second;
if (!color0.resolve_texture) {
VALIDATION_LOG << "EntityPassTarget Flip should never be called for a "
Expand All @@ -40,7 +41,9 @@ std::shared_ptr<Texture> EntityPassTarget::Flip(Allocator& allocator) {
// The second texture is allocated lazily to avoid unused allocations.
TextureDescriptor new_descriptor =
color0.resolve_texture->GetTextureDescriptor();
secondary_color_texture_ = allocator.CreateTexture(new_descriptor);
RenderTarget target = renderer.GetRenderTargetCache()->CreateOffscreenMSAA(
Copy link
Contributor Author

@jonahwilliams jonahwilliams Oct 18, 2024

Choose a reason for hiding this comment

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

When we "flip" the backdrop texture, do the new allocation from the render target cache. Technically this creates some unused transients too but :shrug.

While we still only create a maximum of two backdrop textures per frame, this change ensures we recycle them across frames and avoid continual re-allocation

Copy link
Member

@gaaclarke gaaclarke Oct 18, 2024

Choose a reason for hiding this comment

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

Should there be a branch here if we aren't using MSAA?

Copy link
Contributor Author

@jonahwilliams jonahwilliams Oct 18, 2024

Choose a reason for hiding this comment

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

See above, we'll never use this on a non-MSAA target.

  auto color0 = target_.GetColorAttachments().find(0)->second;
  if (!color0.resolve_texture) {
    VALIDATION_LOG << "EntityPassTarget Flip should never be called for a "
                      "non-MSAA target.";

*renderer.GetContext(), new_descriptor.size, 1);
secondary_color_texture_ = target.GetRenderTargetTexture();

if (!secondary_color_texture_) {
return nullptr;
Expand Down
3 changes: 2 additions & 1 deletion impeller/entity/entity_pass_target.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#ifndef FLUTTER_IMPELLER_ENTITY_ENTITY_PASS_TARGET_H_
#define FLUTTER_IMPELLER_ENTITY_ENTITY_PASS_TARGET_H_

#include "impeller/entity/contents/content_context.h"
#include "impeller/renderer/render_target.h"

namespace impeller {
Expand All @@ -24,7 +25,7 @@ class EntityPassTarget {
/// result of `GetRenderTarget` is guaranteed to be able to read the
/// previous pass's backdrop texture (which is returned by this
/// method).
std::shared_ptr<Texture> Flip(Allocator& allocator);
std::shared_ptr<Texture> Flip(const ContentContext& renderer);

RenderTarget& GetRenderTarget();

Expand Down
8 changes: 4 additions & 4 deletions impeller/entity/entity_pass_target_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,8 @@ TEST_P(EntityPassTargetTest, SwapWithMSAATexture) {
auto msaa_tex = color0.texture;
auto resolve_tex = color0.resolve_texture;

entity_pass_target.Flip(
*content_context->GetContext()->GetResourceAllocator());
FML_DCHECK(content_context);
entity_pass_target.Flip(*content_context);
Copy link
Member

@gaaclarke gaaclarke Oct 18, 2024

Choose a reason for hiding this comment

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

Same here, DCHECK for nullptr.

Copy link
Contributor Author

@jonahwilliams jonahwilliams Oct 18, 2024

Choose a reason for hiding this comment

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

Done


color0 = entity_pass_target.GetRenderTarget()
.GetColorAttachments()
Expand Down Expand Up @@ -98,8 +98,8 @@ TEST_P(EntityPassTargetTest, SwapWithMSAAImplicitResolve) {

ASSERT_EQ(msaa_tex, resolve_tex);

entity_pass_target.Flip(
*content_context->GetContext()->GetResourceAllocator());
FML_DCHECK(content_context);
entity_pass_target.Flip(*content_context);

color0 = entity_pass_target.GetRenderTarget()
.GetColorAttachments()
Expand Down
60 changes: 14 additions & 46 deletions impeller/entity/inline_pass_context.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,34 +7,22 @@
#include <utility>

#include "flutter/fml/status.h"
#include "impeller/base/allocation.h"
#include "impeller/base/validation.h"
#include "impeller/core/formats.h"
#include "impeller/entity/contents/content_context.h"
#include "impeller/entity/entity_pass_target.h"
#include "impeller/renderer/command_buffer.h"
#include "impeller/renderer/render_pass.h"
#include "impeller/renderer/texture_mipmap.h"

namespace impeller {

InlinePassContext::InlinePassContext(
const ContentContext& renderer,
EntityPassTarget& pass_target,
uint32_t entity_count,
std::optional<RenderPassResult> collapsed_parent_pass)
: renderer_(renderer),
pass_target_(pass_target),
entity_count_(entity_count),
is_collapsed_(collapsed_parent_pass.has_value()) {
if (collapsed_parent_pass.has_value()) {
pass_ = collapsed_parent_pass.value().pass;
}
}
InlinePassContext::InlinePassContext(const ContentContext& renderer,
Copy link
Contributor Author

@jonahwilliams jonahwilliams Oct 18, 2024

Choose a reason for hiding this comment

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

entity_count, collapsed_parent_pass, is_collapsed_ are all unused

EntityPassTarget& pass_target)
: renderer_(renderer), pass_target_(pass_target) {}

InlinePassContext::~InlinePassContext() {
if (!is_collapsed_) {
EndPass();
}
EndPass();
}

bool InlinePassContext::IsValid() const {
Expand Down Expand Up @@ -90,10 +78,9 @@ EntityPassTarget& InlinePassContext::GetPassTarget() const {
return pass_target_;
}

InlinePassContext::RenderPassResult InlinePassContext::GetRenderPass(
uint32_t pass_depth) {
const std::shared_ptr<RenderPass>& InlinePassContext::GetRenderPass() {
if (IsActive()) {
return {.pass = pass_};
return pass_;
}

/// Create a new render pass if one isn't active. This path will run the first
Expand All @@ -103,30 +90,25 @@ InlinePassContext::RenderPassResult InlinePassContext::GetRenderPass(
command_buffer_ = renderer_.GetContext()->CreateCommandBuffer();
if (!command_buffer_) {
VALIDATION_LOG << "Could not create command buffer.";
return {};
return pass_;
}

if (pass_target_.GetRenderTarget().GetColorAttachments().empty()) {
VALIDATION_LOG << "Color attachment unexpectedly missing from the "
"EntityPass render target.";
return {};
return pass_;
}

command_buffer_->SetLabel("EntityPass Command Buffer");

RenderPassResult result;
{
// 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;
if (pass_count_ > 0 && is_msaa) {
result.backdrop_texture =
pass_target_.Flip(*renderer_.GetContext()->GetResourceAllocator());
if (!result.backdrop_texture) {
VALIDATION_LOG << "Could not flip the EntityPass render target.";
}
pass_target_.Flip(renderer_);
}
}

Expand All @@ -152,7 +134,7 @@ InlinePassContext::RenderPassResult InlinePassContext::GetRenderPass(
if (!depth.has_value()) {
VALIDATION_LOG << "Depth attachment unexpectedly missing from the "
"EntityPass render target.";
return {};
return pass_;
}
depth->load_action = LoadAction::kClear;
depth->store_action = StoreAction::kDontCare;
Expand All @@ -162,7 +144,7 @@ InlinePassContext::RenderPassResult InlinePassContext::GetRenderPass(
if (!depth.has_value() || !stencil.has_value()) {
VALIDATION_LOG << "Stencil/Depth attachment unexpectedly missing from the "
"EntityPass render target.";
return {};
return pass_;
}
stencil->load_action = LoadAction::kClear;
stencil->store_action = StoreAction::kDontCare;
Expand All @@ -175,26 +157,12 @@ InlinePassContext::RenderPassResult InlinePassContext::GetRenderPass(
pass_ = command_buffer_->CreateRenderPass(pass_target_.GetRenderTarget());
if (!pass_) {
VALIDATION_LOG << "Could not create render pass.";
return {};
return pass_;
}
// Commands are fairly large (500B) objects, so re-allocation of the command
// buffer while encoding can add a surprising amount of overhead. We make a
// conservative npot estimate to avoid this case.
pass_->ReserveCommands(Allocation::NextPowerOfTwoSize(entity_count_));
pass_->SetLabel("EntityPass Render Pass");

result.pass = pass_;
result.just_created = true;

if (!renderer_.GetContext()->GetCapabilities()->SupportsReadFromResolve() &&
result.backdrop_texture ==
result.pass->GetRenderTarget().GetRenderTargetTexture()) {
VALIDATION_LOG << "EntityPass backdrop restore configuration is not valid "
"for the current graphics backend.";
}

++pass_count_;
return result;
return pass_;
}

uint32_t InlinePassContext::GetPassCount() const {
Expand Down
19 changes: 3 additions & 16 deletions impeller/entity/inline_pass_context.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,17 +16,8 @@ namespace impeller {

class InlinePassContext {
public:
struct RenderPassResult {
Copy link
Contributor Author

@jonahwilliams jonahwilliams Oct 18, 2024

Choose a reason for hiding this comment

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

None of these are used anymore

bool just_created = false;
std::shared_ptr<RenderPass> pass;
std::shared_ptr<Texture> backdrop_texture;
};

InlinePassContext(
const ContentContext& renderer,
EntityPassTarget& pass_target,
uint32_t entity_count,
std::optional<RenderPassResult> collapsed_parent_pass = std::nullopt);
InlinePassContext(const ContentContext& renderer,
EntityPassTarget& pass_target);

~InlinePassContext();

Expand All @@ -42,18 +33,14 @@ class InlinePassContext {

uint32_t GetPassCount() const;

RenderPassResult GetRenderPass(uint32_t pass_depth);
Copy link
Member

@gaaclarke gaaclarke Oct 18, 2024

Choose a reason for hiding this comment

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

This is just straight up dead code regardless of your change? No need to change this PR but for future PRs it would be easier if refactoring and functional changes are split into 2 different prs =)

Copy link
Contributor Author

@jonahwilliams jonahwilliams Oct 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah I get it. Though I didn't intend to do a large refactoring, I realized all this was dead when I started changing it.

const std::shared_ptr<RenderPass>& GetRenderPass();

private:
const ContentContext& renderer_;
EntityPassTarget& pass_target_;
std::shared_ptr<CommandBuffer> command_buffer_;
std::shared_ptr<RenderPass> pass_;
uint32_t pass_count_ = 0;
uint32_t entity_count_ = 0;

// Whether this context is collapsed into a parent entity pass.
bool is_collapsed_ = false;

InlinePassContext(const InlinePassContext&) = delete;

Expand Down