-
Notifications
You must be signed in to change notification settings - Fork 6k
[Impeller] More efficient usage of transient onscreen attachments. #51206
Changes from all commits
d9b46eb
2819e36
c026f94
1437114
743899c
4432aa1
91ae5d0
634bee0
0c5743d
5836a6b
e7b3a1b
2dd355a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
|---|---|---|
|
|
@@ -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(); | ||
| } | ||
|
|
@@ -77,6 +76,16 @@ std::unique_ptr<SurfaceVK> SurfaceVK::WrapSwapchainImage( | |
|
|
||
| RenderTarget render_target_desc; | ||
| render_target_desc.SetColorAttachment(color0, 0u); | ||
| render_target_desc.SetupDepthStencilAttachments( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we expand this with
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
| /*context=*/*context, // | ||
| /*allocator=*/*context->GetResourceAllocator(), // | ||
| /*size=*/swapchain_image->GetSize(), // | ||
| /*msaa=*/enable_msaa, // | ||
| /*label=*/"Onscreen", // | ||
| /*stencil_attachment_config=*/ | ||
| RenderTarget::kDefaultStencilAttachmentConfig, // | ||
| /*depth_stencil_texture=*/swapchain_image->GetDepthStencilTexture() // | ||
| ); | ||
|
|
||
| // The constructor is private. So make_unique may not be used. | ||
| return std::unique_ptr<SurfaceVK>( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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_; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for spelling out texture.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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" | ||
|
|
@@ -228,6 +229,40 @@ 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; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How are all these values picked/known?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
|
|
||
| // The depth+stencil configuration matches the configuration used by | ||
| // RenderTarget::SetupDepthStencilAttachments and matching the swapchain | ||
| // image dimensions and sample count. | ||
| 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 = | ||
|
|
@@ -239,6 +274,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(), | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It does now |
There was a problem hiding this comment.
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:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doc comment added