-
Notifications
You must be signed in to change notification settings - Fork 6k
[Impeller] avoid heap allocation in RenderTarget object. #56829
Changes from 1 commit
8e740b5
5b4d07e
24450c4
61c4ff5
b6de222
c65046a
938b279
132748a
2f912f6
52d2674
2190282
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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; | ||
| } | ||
|
|
@@ -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; | ||
|
|
||
|
|
@@ -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
Member
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. Please remove duplication
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
Member
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. Can't you use the Iterate method here to avoid the duplication?
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 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(); | ||
|
|
@@ -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 | ||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -54,10 +54,19 @@ class RenderPassBuilderVK { | |||
| // Visible for testing. | ||||
| const std::optional<vk::AttachmentDescription>& GetDepthStencil() const; | ||||
|
|
||||
| // Visible for testing. | ||||
| std::optional<vk::AttachmentDescription> GetColor0() const; | ||||
|
|
||||
| // Visible for testing. | ||||
| std::optional<vk::AttachmentDescription> GetColor0Resolve() const; | ||||
|
Comment on lines
+58
to
+62
Member
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. Please make the test a friend instead of making these public (example
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. Listing out each test name seems like a pretty big step down, given that this is an internal library of an internal library. |
||||
|
|
||||
| private: | ||||
| std::optional<vk::AttachmentDescription> color0_; | ||||
| std::optional<vk::AttachmentDescription> color0_resolve_; | ||||
| std::optional<vk::AttachmentDescription> depth_stencil_; | ||||
|
|
||||
| std::map<size_t, vk::AttachmentDescription> colors_; | ||||
|
Member
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. We should add a comment here that colors_[0] is really at color0_.
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 |
||||
| std::map<size_t, vk::AttachmentDescription> resolves_; | ||||
| std::optional<vk::AttachmentDescription> depth_stencil_; | ||||
| }; | ||||
|
|
||||
| //------------------------------------------------------------------------------ | ||||
|
|
||||
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.
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.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.
Done