Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.
7 changes: 5 additions & 2 deletions impeller/renderer/backend/vulkan/surface_vk.cc
Copy link
Contributor

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

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

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

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

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
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
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
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

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

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
34 changes: 34 additions & 0 deletions impeller/renderer/backend/vulkan/swapchain_impl_vk.cc
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

#include "fml/synchronization/semaphore.h"
#include "impeller/base/validation.h"
#include "impeller/core/formats.h"
#include "impeller/renderer/backend/vulkan/command_buffer_vk.h"
#include "impeller/renderer/backend/vulkan/command_encoder_vk.h"
#include "impeller/renderer/backend/vulkan/context_vk.h"
Expand Down Expand Up @@ -228,6 +229,37 @@ 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

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

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;
if (enable_msaa) {
depth_stencil_desc.type = TextureType::kTexture2DMultisample;
depth_stencil_desc.sample_count = SampleCount::kCount4;
} else {
depth_stencil_desc.type = TextureType::kTexture2D;
depth_stencil_desc.sample_count = SampleCount::kCount1;
}
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;
if (enable_msaa) {
msaa_texture = context->GetResourceAllocator()->CreateTexture(msaa_desc);
}
std::shared_ptr<Texture> depth_stencil_texture =
context->GetResourceAllocator()->CreateTexture(depth_stencil_desc);

std::vector<std::shared_ptr<SwapchainImageVK>> swapchain_images;
for (const auto& image : images) {
auto swapchain_image =
Expand All @@ -239,6 +271,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
20 changes: 20 additions & 0 deletions impeller/renderer/backend/vulkan/test/mock_vulkan.cc
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ struct MockSwapchainKHR {

struct MockSemaphore {};

struct MockFramebuffer {};

static ISize currentImageSize = ISize{1, 1};

class MockDevice final {
Expand Down Expand Up @@ -719,6 +721,20 @@ VkResult vkAcquireNextImageKHR(VkDevice device,
return VK_SUCCESS;
}

VkResult vkCreateFramebuffer(VkDevice device,
const VkFramebufferCreateInfo* pCreateInfo,
const VkAllocationCallbacks* pAllocator,
VkFramebuffer* pFramebuffer) {
*pFramebuffer = reinterpret_cast<VkFramebuffer>(new MockFramebuffer());
return VK_SUCCESS;
}

void vkDestroyFramebuffer(VkDevice device,
VkFramebuffer framebuffer,
const VkAllocationCallbacks* pAllocator) {
delete reinterpret_cast<MockFramebuffer*>(framebuffer);
}

PFN_vkVoidFunction GetMockVulkanProcAddress(VkInstance instance,
const char* pName) {
if (strcmp("vkEnumerateInstanceExtensionProperties", pName) == 0) {
Expand Down Expand Up @@ -855,6 +871,10 @@ PFN_vkVoidFunction GetMockVulkanProcAddress(VkInstance instance,
return (PFN_vkVoidFunction)vkDestroySurfaceKHR;
} else if (strcmp("vkAcquireNextImageKHR", pName) == 0) {
return (PFN_vkVoidFunction)vkAcquireNextImageKHR;
} else if (strcmp("vkCreateFramebuffer", pName) == 0) {
return (PFN_vkVoidFunction)vkCreateFramebuffer;
} else if (strcmp("vkDestroyFramebuffer", pName) == 0) {
return (PFN_vkVoidFunction)vkDestroyFramebuffer;
}
return noop;
}
Expand Down
39 changes: 39 additions & 0 deletions impeller/renderer/backend/vulkan/test/swapchain_unittests.cc
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the test directly verify the "primary trick" outlined in the change context (recycling a single MSAA and Depth/Stencil attachment)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does now

Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#include "gtest/gtest.h"
#include "impeller/renderer/backend/vulkan/swapchain_vk.h"
#include "impeller/renderer/backend/vulkan/test/mock_vulkan.h"
#include "impeller/renderer/backend/vulkan/texture_vk.h"
#include "vulkan/vulkan_enums.hpp"

namespace impeller {
Expand Down Expand Up @@ -52,5 +53,43 @@ TEST(SwapchainTest, RecreateSwapchainWhenSizeChanges) {
EXPECT_EQ(image_b->GetSize(), expected_size);
}

TEST(SwapchainTest, CachesRenderPassOnSwapchainImage) {
auto const context = MockVulkanContextBuilder().Build();

auto surface = CreateSurface(*context);
auto swapchain =
SwapchainVK::Create(context, std::move(surface), ISize{1, 1});

EXPECT_TRUE(swapchain->IsValid());

// We should create 3 swapchain images with the current mock setup. However,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this enforced in the test somehow or an implied assumption?

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 can actually be fixed now. There were some global state problems, but @jason-simmons fixed up parts of the mock vulkan when he was doing the ASAN changes that make this easier to implemented.

Updated the test so we create all 3 swapchain images and can make assertions on them.

// we will only ever return the first image, so the render pass and
// framebuffer will be cached after one call to AcquireNextDrawable.
auto drawable = swapchain->AcquireNextDrawable();
RenderTarget render_target = drawable->GetTargetRenderPassDescriptor();

auto texture = render_target.GetRenderTargetTexture();
auto& texture_vk = TextureVK::Cast(*texture);
EXPECT_EQ(texture_vk.GetFramebuffer(), nullptr);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the caching behavior truly imply these should start null?

If so, I wonder if the name is misleading (i.e. should be GetCachedFramebuffer). I might be missing something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes these start null, I updated the name to GetCachedFramebuffer.

EXPECT_EQ(texture_vk.GetRenderPass(), nullptr);

auto command_buffer = context->CreateCommandBuffer();
auto render_pass = command_buffer->CreateRenderPass(render_target);

render_pass->EncodeCommands();

EXPECT_NE(texture_vk.GetFramebuffer(), nullptr);
EXPECT_NE(texture_vk.GetRenderPass(), nullptr);

{
auto drawable = swapchain->AcquireNextDrawable();
auto texture = render_target.GetRenderTargetTexture();
auto& texture_vk = TextureVK::Cast(*texture);

EXPECT_NE(texture_vk.GetFramebuffer(), nullptr);
Copy link
Contributor

Choose a reason for hiding this comment

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

We should improve the mocks if we're not able to make a more definitive assertion (i.e. if, as I think you are trying to do, assert that the frame buffer stays the same).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh yeah, I can go through once more and verify they are the same.

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

EXPECT_NE(texture_vk.GetRenderPass(), nullptr);
}
}

} // namespace testing
} // namespace impeller
18 changes: 18 additions & 0 deletions impeller/renderer/backend/vulkan/texture_source_vk.cc
Original file line number Diff line number Diff line change
Expand Up @@ -62,4 +62,22 @@ fml::Status TextureSourceVK::SetLayout(const BarrierVK& barrier) const {
return {};
}

void TextureSourceVK::SetFramebuffer(
const SharedHandleVK<vk::Framebuffer>& framebuffer) {
framebuffer_ = framebuffer;
}

void TextureSourceVK::SetRenderPass(
const SharedHandleVK<vk::RenderPass>& render_pass) {
render_pass_ = render_pass;
}

SharedHandleVK<vk::Framebuffer> TextureSourceVK::GetFramebuffer() const {
return framebuffer_;
}

SharedHandleVK<vk::RenderPass> TextureSourceVK::GetRenderPass() const {
return render_pass_;
}

} // namespace impeller
28 changes: 28 additions & 0 deletions impeller/renderer/backend/vulkan/texture_source_vk.h
Original file line number Diff line number Diff line change
Expand Up @@ -125,12 +125,40 @@ class TextureSourceVK {
///
virtual bool IsSwapchainImage() const = 0;

// These methods should only be used by render_pass_vk.h
Copy link
Contributor

Choose a reason for hiding this comment

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

Could friend be used instead of a comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would have to be friend for the class and all of the test classes. TBH I haven't figured out how to do that consistently for test classes, since the name is macro generated?


/// Store the last framebuffer object used with this texture.
///
/// This field is only set if this texture is used as the resolve texture
/// of a render pass. By construction, this framebuffer should be compatible
/// with any future render passes.
void SetFramebuffer(const SharedHandleVK<vk::Framebuffer>& framebuffer);

/// Store the last render pass object used with this texture.
///
/// This field is only set if this texture is used as the resolve texture
/// of a render pass. By construction, this framebuffer should be compatible
/// with any future render passes.
void SetRenderPass(const SharedHandleVK<vk::RenderPass>& render_pass);

/// Retrieve the last framebuffer object used with this texture.
///
/// May be nullptr if no previous framebuffer existed.
SharedHandleVK<vk::Framebuffer> GetFramebuffer() const;

/// Retrieve the last render pass object used with this texture.
///
/// May be nullptr if no previous render pass existed.
SharedHandleVK<vk::RenderPass> GetRenderPass() const;

protected:
const TextureDescriptor desc_;

explicit TextureSourceVK(TextureDescriptor desc);

private:
SharedHandleVK<vk::Framebuffer> framebuffer_;
SharedHandleVK<vk::RenderPass> render_pass_;
mutable RWMutex layout_mutex_;
mutable vk::ImageLayout layout_ IPLR_GUARDED_BY(layout_mutex_) =
vk::ImageLayout::eUndefined;
Expand Down
8 changes: 4 additions & 4 deletions impeller/renderer/backend/vulkan/texture_vk.cc
Original file line number Diff line number Diff line change
Expand Up @@ -176,20 +176,20 @@ vk::ImageView TextureVK::GetRenderTargetView() const {

void TextureVK::SetFramebuffer(
const SharedHandleVK<vk::Framebuffer>& framebuffer) {
framebuffer_ = framebuffer;
source_->SetFramebuffer(framebuffer);
}

void TextureVK::SetRenderPass(
const SharedHandleVK<vk::RenderPass>& render_pass) {
render_pass_ = render_pass;
source_->SetRenderPass(render_pass);
}

SharedHandleVK<vk::Framebuffer> TextureVK::GetFramebuffer() const {
return framebuffer_;
return source_->GetFramebuffer();
}

SharedHandleVK<vk::RenderPass> TextureVK::GetRenderPass() const {
return render_pass_;
return source_->GetRenderPass();
}

void TextureVK::SetMipMapGenerated() {
Expand Down
2 changes: 0 additions & 2 deletions impeller/renderer/backend/vulkan/texture_vk.h
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,6 @@ class TextureVK final : public Texture, public BackendCast<TextureVK, Texture> {
private:
std::weak_ptr<Context> context_;
std::shared_ptr<TextureSourceVK> source_;
SharedHandleVK<vk::Framebuffer> framebuffer_ = nullptr;
SharedHandleVK<vk::RenderPass> render_pass_ = nullptr;

// |Texture|
void SetLabel(std::string_view label) override;
Expand Down
1 change: 1 addition & 0 deletions impeller/renderer/render_target.cc
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include "impeller/core/allocator.h"
#include "impeller/core/formats.h"
#include "impeller/core/texture.h"
#include "impeller/core/texture_descriptor.h"
#include "impeller/renderer/context.h"

namespace impeller {
Expand Down