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

[Impeller] More efficient usage of transient onscreen attachments. #51206

Merged
auto-submit merged 12 commits into from
Mar 8, 2024
Merged

[Impeller] More efficient usage of transient onscreen attachments. #51206

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
d9b46eb
[Impeller] more efficient use of transient onscreen attachments.
Mar 5, 2024
2819e36
[Impeller] more efficient use of transient onscreen attachments.
Mar 5, 2024
c026f94
Merge branch 'onscreen_improvements' of github.com:jonahwilliams/engi…
Mar 5, 2024
1437114
fold in RP caching.
Mar 5, 2024
743899c
update mock vulkan.
Mar 5, 2024
4432aa1
++
Mar 5, 2024
91ae5d0
Merge branch 'main' of github.com:flutter/engine into onscreen_improv…
Mar 6, 2024
634bee0
merge fix.
Mar 6, 2024
0c5743d
++
Mar 6, 2024
5836a6b
Merge branch 'main' of github.com:flutter/engine into onscreen_improv…
Mar 7, 2024
e7b3a1b
matanl review.
Mar 7, 2024
2dd355a
++
Mar 7, 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
Prev Previous commit
Next Next commit
[Impeller] more efficient use of transient onscreen attachments.
  • Loading branch information
jonahwilliams committed Mar 5, 2024
commit 2819e3652dc0106536156ab35015b9177d52633c
7 changes: 5 additions & 2 deletions impeller/renderer/backend/vulkan/surface_vk.cc
View file
Open in desktop
Copy link
Contributor

@matanlurey matanlurey Mar 7, 2024

Choose a reason for hiding this comment

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

It would be nice to go and add a comment to SurfaceVK::WrapSwapchainImage (in the .h).

Based on what you told me, maybe something like:

Wraps a swapchain image for rendering with MSAA and depth/stencil support.

This function takes an device(?)-provided swapchain image (which is the resolve target
of a potential MSAA render process) and prepares it for use by creating necessary
supporting textures. This includes the MSAA texture (if enabled) and the depth/stencil
texture for rendering.

Copy link
Contributor Author

@jonahwilliams jonahwilliams Mar 7, 2024

Choose a reason for hiding this comment

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

Doc comment added

Copy link
Contributor

@matanlurey matanlurey Mar 7, 2024

Choose a reason for hiding this comment

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

Out of scope of this PR, but why do we have this code:

if (!context || !swapchain_image || !swap_callback) {
  return nullptr;
}

When is it valid not to have these things? I'm in particular confused about swap_callback, because below:

bool SurfaceVK::Present() const {
  return swap_callback_ ? swap_callback_() : false;
}

It's impossible for this to be null, right?

Original file line number Diff line number Diff line change
Expand Up @@ -30,14 +30,13 @@ std::unique_ptr<SurfaceVK> SurfaceVK::WrapSwapchainImage(
msaa_tex_desc.size = swapchain_image->GetSize();
msaa_tex_desc.usage = static_cast<uint64_t>(TextureUsage::kRenderTarget);

if (!swapchain_image->HasMSAATexture()) {
if (!swapchain_image->GetMSAATexture()) {
msaa_tex = context->GetResourceAllocator()->CreateTexture(msaa_tex_desc);
msaa_tex->SetLabel("ImpellerOnscreenColorMSAA");
if (!msaa_tex) {
VALIDATION_LOG << "Could not allocate MSAA color texture.";
return nullptr;
}
swapchain_image->SetMSAATexture(msaa_tex);
} else {
msaa_tex = swapchain_image->GetMSAATexture();
}
Expand Down Expand Up @@ -77,6 +76,10 @@ std::unique_ptr<SurfaceVK> SurfaceVK::WrapSwapchainImage(

RenderTarget render_target_desc;
render_target_desc.SetColorAttachment(color0, 0u);
render_target_desc.SetupDepthStencilAttachments(
Copy link
Contributor

@matanlurey matanlurey Mar 7, 2024

Choose a reason for hiding this comment

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

Could we expand this with /*=param*/ comments?

Copy link
Contributor Author

@jonahwilliams jonahwilliams Mar 7, 2024

Choose a reason for hiding this comment

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

Done

*context, *context->GetResourceAllocator(), swapchain_image->GetSize(),
enable_msaa, "Onscreen", RenderTarget::kDefaultStencilAttachmentConfig,
swapchain_image->GetDepthStencilTexture());

// The constructor is private. So make_unique may not be used.
return std::unique_ptr<SurfaceVK>(
Expand Down
1 change: 0 additions & 1 deletion impeller/renderer/backend/vulkan/surface_vk.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@

#include <memory>

#include "flutter/fml/macros.h"
#include "impeller/renderer/backend/vulkan/context_vk.h"
#include "impeller/renderer/backend/vulkan/swapchain_image_vk.h"
#include "impeller/renderer/surface.h"
Expand Down
15 changes: 10 additions & 5 deletions impeller/renderer/backend/vulkan/swapchain_image_vk.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,15 +36,20 @@ bool SwapchainImageVK::IsValid() const {
}

std::shared_ptr<Texture> SwapchainImageVK::GetMSAATexture() const {
return msaa_tex_;
return msaa_texture_;
}

bool SwapchainImageVK::HasMSAATexture() const {
return msaa_tex_ != nullptr;
std::shared_ptr<Texture> SwapchainImageVK::GetDepthStencilTexture() const {
return depth_stencil_texture_;
}

void SwapchainImageVK::SetMSAATexture(std::shared_ptr<Texture> msaa_tex) {
msaa_tex_ = std::move(msaa_tex);
void SwapchainImageVK::SetMSAATexture(std::shared_ptr<Texture> texture) {
msaa_texture_ = std::move(texture);
}

void SwapchainImageVK::SetDepthStencilTexture(
std::shared_ptr<Texture> texture) {
depth_stencil_texture_ = std::move(texture);
}

PixelFormat SwapchainImageVK::GetPixelFormat() const {
Expand Down
9 changes: 6 additions & 3 deletions impeller/renderer/backend/vulkan/swapchain_image_vk.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,21 +33,24 @@ class SwapchainImageVK final : public TextureSourceVK {

std::shared_ptr<Texture> GetMSAATexture() const;

bool HasMSAATexture() const;
std::shared_ptr<Texture> GetDepthStencilTexture() const;

// |TextureSourceVK|
vk::ImageView GetImageView() const override;

vk::ImageView GetRenderTargetView() const override;

void SetMSAATexture(std::shared_ptr<Texture> msaa_tex);
void SetMSAATexture(std::shared_ptr<Texture> texture);

void SetDepthStencilTexture(std::shared_ptr<Texture> texture);

bool IsSwapchainImage() const override { return true; }

private:
vk::Image image_ = VK_NULL_HANDLE;
vk::UniqueImageView image_view_ = {};
std::shared_ptr<Texture> msaa_tex_;
std::shared_ptr<Texture> msaa_texture_;
Copy link
Contributor

@matanlurey matanlurey Mar 7, 2024

Choose a reason for hiding this comment

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

Thank you for spelling out texture.

Copy link
Contributor Author

@jonahwilliams jonahwilliams Mar 7, 2024

Choose a reason for hiding this comment

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

This one was my fault initially

std::shared_ptr<Texture> depth_stencil_texture_;
bool is_valid_ = false;

SwapchainImageVK(const SwapchainImageVK&) = delete;
Expand Down
31 changes: 31 additions & 0 deletions impeller/renderer/backend/vulkan/swapchain_impl_vk.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,35 @@ SwapchainImplVK::SwapchainImplVK(const std::shared_ptr<Context>& context,
texture_desc.size = ISize::MakeWH(swapchain_info.imageExtent.width,
swapchain_info.imageExtent.height);

// Allocate a single onscreen MSAA texture and Depth+Stencil Texture to
// be shared by all swapchain images.
TextureDescriptor msaa_desc;
msaa_desc.storage_mode = StorageMode::kDeviceTransient;
msaa_desc.type = TextureType::kTexture2DMultisample;
msaa_desc.sample_count = SampleCount::kCount4;
Copy link
Contributor

@matanlurey matanlurey Mar 7, 2024

Choose a reason for hiding this comment

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

How are all these values picked/known?

Copy link
Contributor Author

@jonahwilliams jonahwilliams Mar 7, 2024

Choose a reason for hiding this comment

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

I added a comment, but mostly it matches the swapchain + the convention we use.

msaa_desc.format = texture_desc.format;
msaa_desc.size = texture_desc.size;
msaa_desc.usage = static_cast<uint64_t>(TextureUsage::kRenderTarget);

TextureDescriptor depth_stencil_desc;
depth_stencil_desc.storage_mode = StorageMode::kDeviceTransient;
depth_stencil_desc.type = TextureType::kTexture2DMultisample;
depth_stencil_desc.sample_count = SampleCount::kCount4;
depth_stencil_desc.format =
context->GetCapabilities()->GetDefaultDepthStencilFormat();
depth_stencil_desc.size = texture_desc.size;
depth_stencil_desc.usage = static_cast<uint64_t>(TextureUsage::kRenderTarget);

std::shared_ptr<Texture> msaa_texture =
context->GetResourceAllocator()->CreateTexture(msaa_desc);
std::shared_ptr<Texture> depth_stencil_texture =
context->GetResourceAllocator()->CreateTexture(depth_stencil_desc);
if (!msaa_texture || !depth_stencil_texture) {
VALIDATION_LOG
<< "Failed to allocate onscreen MSAA or depth+stencil texture.";
return;
}

std::vector<std::shared_ptr<SwapchainImageVK>> swapchain_images;
for (const auto& image : images) {
auto swapchain_image =
Expand All @@ -239,6 +268,8 @@ SwapchainImplVK::SwapchainImplVK(const std::shared_ptr<Context>& context,
VALIDATION_LOG << "Could not create swapchain image.";
return;
}
swapchain_image->SetMSAATexture(msaa_texture);
swapchain_image->SetDepthStencilTexture(depth_stencil_texture);

ContextVK::SetDebugName(
vk_context.GetDevice(), swapchain_image->GetImage(),
Expand Down
18 changes: 12 additions & 6 deletions impeller/renderer/render_target.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -398,7 +398,8 @@ void RenderTarget::SetupDepthStencilAttachments(
ISize size,
bool msaa,
const std::string& label,
RenderTarget::AttachmentConfig stencil_attachment_config) {
RenderTarget::AttachmentConfig stencil_attachment_config,
const std::shared_ptr<Texture>& depth_stencil_texture) {
TextureDescriptor depth_stencil_texture_desc;
depth_stencil_texture_desc.storage_mode =
stencil_attachment_config.storage_mode;
Expand All @@ -412,23 +413,28 @@ void RenderTarget::SetupDepthStencilAttachments(
depth_stencil_texture_desc.usage =
static_cast<TextureUsageMask>(TextureUsage::kRenderTarget);

auto depth_stencil_texture =
allocator.CreateTexture(depth_stencil_texture_desc);
if (!depth_stencil_texture) {
std::shared_ptr<Texture> new_depth_stencil_texture =
depth_stencil_texture == nullptr
? allocator.CreateTexture(depth_stencil_texture_desc)
: depth_stencil_texture;
FML_DCHECK(depth_stencil_texture == nullptr ||
depth_stencil_texture->GetTextureDescriptor() ==
depth_stencil_texture_desc);
if (!new_depth_stencil_texture) {
return; // Error messages are handled by `Allocator::CreateTexture`.
}

DepthAttachment depth0;
depth0.load_action = stencil_attachment_config.load_action;
depth0.store_action = stencil_attachment_config.store_action;
depth0.clear_depth = 0u;
depth0.texture = depth_stencil_texture;
depth0.texture = new_depth_stencil_texture;

StencilAttachment stencil0;
stencil0.load_action = stencil_attachment_config.load_action;
stencil0.store_action = stencil_attachment_config.store_action;
stencil0.clear_stencil = 0u;
stencil0.texture = std::move(depth_stencil_texture);
stencil0.texture = std::move(new_depth_stencil_texture);

stencil0.texture->SetLabel(
SPrintF("%s Depth+Stencil Texture", label.c_str()));
Expand Down
12 changes: 2 additions & 10 deletions impeller/renderer/render_target.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,8 @@ class RenderTarget final {
bool msaa,
const std::string& label = "Offscreen",
RenderTarget::AttachmentConfig stencil_attachment_config =
RenderTarget::kDefaultStencilAttachmentConfig);
RenderTarget::kDefaultStencilAttachmentConfig,
const std::shared_ptr<Texture>& depth_stencil_texture = nullptr);

SampleCount GetSampleCount() const;

Expand Down Expand Up @@ -176,15 +177,6 @@ class RenderTargetAllocator {
virtual void End();

private:
void SetupDepthStencilAttachments(
Allocator& allocator,
const Context& context,
ISize size,
bool msaa,
const std::string& label = "Offscreen",
RenderTarget::AttachmentConfig stencil_attachment_config =
RenderTarget::kDefaultStencilAttachmentConfig);

std::shared_ptr<Allocator> allocator_;
};

Expand Down