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

jonahwilliams · 2024-03-05T19:50:44Z

The primary trick here is making sure that the transient attachments for the onscreen (MSAA and Depth/Stencil) are effectively recycled. In fact, I think we can actually make a single Onscreen MSAA and Onscreen Depth/Stencil and share them across the swapchain images - as they should be effectively synchronized already.

Fixes flutter/flutter#141750

…ne into onscreen_improvements

…ements

matanlurey

Someone with more context on the implications of this change should give final LGTM - I mostly reviewed agnostically (though I remember some words!)

matanlurey · 2024-03-07T16:30:33Z

impeller/renderer/backend/vulkan/surface_vk.cc

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.

Doc comment added

matanlurey · 2024-03-07T16:39:55Z

impeller/renderer/backend/vulkan/surface_vk.cc

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?

matanlurey · 2024-03-07T16:40:38Z

impeller/renderer/backend/vulkan/surface_vk.cc


  RenderTarget render_target_desc;
  render_target_desc.SetColorAttachment(color0, 0u);
+  render_target_desc.SetupDepthStencilAttachments(


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

matanlurey · 2024-03-07T16:42:10Z

impeller/renderer/backend/vulkan/swapchain_image_vk.h

  vk::Image image_ = VK_NULL_HANDLE;
  vk::UniqueImageView image_view_ = {};
-  std::shared_ptr<Texture> msaa_tex_;
+  std::shared_ptr<Texture> msaa_texture_;


Thank you for spelling out texture.

This one was my fault initially

matanlurey · 2024-03-07T16:43:00Z

impeller/renderer/backend/vulkan/swapchain_impl_vk.cc

+  TextureDescriptor msaa_desc;
+  msaa_desc.storage_mode = StorageMode::kDeviceTransient;
+  msaa_desc.type = TextureType::kTexture2DMultisample;
+  msaa_desc.sample_count = SampleCount::kCount4;


How are all these values picked/known?

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

matanlurey · 2024-03-07T16:44:00Z

impeller/renderer/backend/vulkan/texture_source_vk.h

  ///
  virtual bool IsSwapchainImage() const = 0;

+  // These methods should only be used by render_pass_vk.h


Could friend be used instead of a comment?

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?

matanlurey · 2024-03-07T16:47:43Z

impeller/renderer/backend/vulkan/test/swapchain_unittests.cc

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


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.

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

matanlurey · 2024-03-07T16:48:13Z

impeller/renderer/backend/vulkan/test/swapchain_unittests.cc

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

It does now

matanlurey · 2024-03-07T16:48:47Z

impeller/renderer/backend/vulkan/test/swapchain_unittests.cc

+
+  EXPECT_TRUE(swapchain->IsValid());
+
+  // We should create 3 swapchain images with the current mock setup. However,


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

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.

matanlurey · 2024-03-07T16:50:05Z

impeller/renderer/backend/vulkan/test/swapchain_unittests.cc

+    auto texture = render_target.GetRenderTargetTexture();
+    auto& texture_vk = TextureVK::Cast(*texture);
+
+    EXPECT_NE(texture_vk.GetFramebuffer(), nullptr);


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

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

matanlurey

LGTM from a purely this is code and I think it is good (TM) perspective.

dnfield

Let's Get This Moving

…hments. (flutter/engine#51206)

…144839) flutter/engine@bc4abcd...05fdf94 2024-03-08 [email protected] [Impeller] More efficient usage of transient onscreen attachments. (flutter/engine#51206) 2024-03-08 6844906[email protected] [Fuchsia] Providing FUCHSIA_SDK_PATH env var for fuchsia lsc (flutter/engine#51234) 2024-03-08 [email protected] Migrate vulkan_memory_allocator to flutter/third_party (flutter/engine#51275) 2024-03-08 [email protected] Update CI scripts to use either src/flutter/third_party/dart or src/third_party/dart (flutter/engine#51276) 2024-03-08 [email protected] Add fuchsia_gn_sdk GN variable pointing to //flutter/tools/fuchsia/gn-sdk (flutter/engine#51287) 2024-03-08 [email protected] Roll Skia from 993a88a663c8 to a4fb847f47d9 (1 revision) (flutter/engine#51283) 2024-03-08 [email protected] Roll Skia from 15f8d03a9594 to 993a88a663c8 (1 revision) (flutter/engine#51282) 2024-03-08 [email protected] Roll Skia from 093994f8f078 to 15f8d03a9594 (1 revision) (flutter/engine#51280) 2024-03-08 [email protected] Roll Skia from bf3f9c5f0edb to 093994f8f078 (1 revision) (flutter/engine#51278) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

jonahwilliams added 4 commits March 5, 2024 09:26

[Impeller] more efficient use of transient onscreen attachments.

d9b46eb

[Impeller] more efficient use of transient onscreen attachments.

2819e36

Merge branch 'onscreen_improvements' of github.com:jonahwilliams/engi…

c026f94

…ne into onscreen_improvements

fold in RP caching.

1437114

github-actions bot added the e: impeller label Mar 5, 2024

jonahwilliams mentioned this pull request Mar 5, 2024

[Impeller] cache root render target and render pass. #51085

Closed

8 tasks

jonahwilliams changed the title ~~Onscreen improvements~~ [Impeller] More efficient usage of transient onscreen attachments. Mar 5, 2024

jonahwilliams added 2 commits March 5, 2024 12:07

update mock vulkan.

743899c

++

4432aa1

jonahwilliams mentioned this pull request Mar 5, 2024

[Impeller] Wire up Android Hardware Buffer backed swapchains. #51213

Closed

jonahwilliams marked this pull request as ready for review March 6, 2024 00:39

jonahwilliams added 4 commits March 6, 2024 12:26

Merge branch 'main' of github.com:flutter/engine into onscreen_improv…

91ae5d0

…ements

merge fix.

634bee0

++

0c5743d

Merge branch 'main' of github.com:flutter/engine into onscreen_improv…

5836a6b

…ements

jonahwilliams requested review from chinmaygarde and dnfield March 7, 2024 16:36

matanlurey reviewed Mar 7, 2024

View reviewed changes

matanl review.

e7b3a1b

jonahwilliams force-pushed the onscreen_improvements branch from 7c4b31e to e7b3a1b Compare March 7, 2024 17:21

jonahwilliams requested a review from matanlurey March 7, 2024 17:21

++

2dd355a

matanlurey approved these changes Mar 7, 2024

View reviewed changes

dnfield approved these changes Mar 8, 2024

View reviewed changes

jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 8, 2024

auto-submit bot merged commit 05fdf94 into flutter:main Mar 8, 2024

jonahwilliams deleted the onscreen_improvements branch March 8, 2024 17:36

engine-flutter-autoroll mentioned this pull request Mar 8, 2024

Roll Flutter Engine from bc4abcda6357 to 05fdf947f81b (9 revisions) flutter/flutter#144839

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 8, 2024

05fdf947f [Impeller] More efficient usage of transient onscreen attac…

f7bd993

…hments. (flutter/engine#51206)


		EXPECT_TRUE(swapchain->IsValid());

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

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

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

Uh oh!

Conversation

jonahwilliams commented Mar 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matanlurey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matanlurey left a comment

Choose a reason for hiding this comment

Uh oh!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jonahwilliams commented Mar 5, 2024 •

edited

Loading