[Impeller] remove transfer barriers from render pass, drop blit, tighten up graphics on level 3. #165584

jonahwilliams · 2025-03-20T17:55:31Z

The change to fix PowerVR barriers regressed Mali (see #165538)

We need to tighten the barriers to remove the transfer barrier to get back mali performance. however, this requires us to drop the usage of the blit for onscreen restore. not a huge loss imo.

At any rate, all of these changes should be visible in benchmarks.

…ten up.

flutter-dashboard · 2025-03-20T17:55:34Z

It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group.

jonahwilliams · 2025-03-20T17:56:01Z

engine/src/flutter/impeller/renderer/backend/vulkan/texture_source_vk.cc


 fml::Status TextureSourceVK::SetLayout(const BarrierVK& barrier) const {
  const auto old_layout = SetLayoutWithoutEncoding(barrier.new_layout);
-  if (barrier.new_layout == old_layout) {


This isn't correct because we can transition across the same layout with different stage masks - dropping the barrier makes it incorrect.

jonahwilliams · 2025-03-20T17:56:54Z

engine/src/flutter/impeller/renderer/backend/vulkan/texture_source_vk.h

 private:
  SharedHandleVK<vk::Framebuffer> framebuffer_;
  SharedHandleVK<vk::RenderPass> render_pass_;
-  mutable RWMutex layout_mutex_;


This mutex is incorrect. The layout depends on code submitted to the queue, not the order of encoded operations. so layout_ is only a best guess and isn't correct if there is shared texture usage across more than one thread.

We don't do that today so there aren't any drawbacks to the bug.

We don't do what today? We don't use the textures across more than one thread?

Correct. And if we did, this mutex would still be wrong because it records layout at recording time whereas the layout only actually changes at submit time.

jonahwilliams · 2025-03-20T17:57:11Z

engine/src/flutter/impeller/renderer/backend/vulkan/android/ahb_texture_source_vk.cc

  }
  if (ahb_desc.usage & AHARDWAREBUFFER_USAGE_GPU_FRAMEBUFFER) {
    image_usage_flags |= vk::ImageUsageFlagBits::eColorAttachment;
-  }


This was wrong, AHARDWAREBUFFER_USAGE_COMPOSER_OVERLAY isn't used by vulkan.

jonahwilliams · 2025-03-20T17:57:17Z

engine/src/flutter/impeller/renderer/backend/vulkan/android/ahb_texture_source_vk.cc

                       ? ahb_size.MipCount()
                       : 1u;
-  if (ahb_desc.usage & AHARDWAREBUFFER_USAGE_COMPOSER_OVERLAY) {
+  if (ahb_desc.usage & AHARDWAREBUFFER_USAGE_GPU_FRAMEBUFFER) {


typo should be framebuffer

gaaclarke · 2025-03-21T16:37:57Z

engine/src/flutter/impeller/renderer/backend/vulkan/render_pass_builder_vk.cc

-  deps[0].srcAccessMask = vk::AccessFlagBits::eShaderRead |
-                          vk::AccessFlagBits::eTransferRead |
-                          vk::AccessFlagBits::eColorAttachmentWrite;
+  if (is_swapchain_) {


A comment wouldn't hurt here.

gaaclarke · 2025-03-21T16:40:01Z

engine/src/flutter/impeller/renderer/backend/vulkan/texture_source_vk.h

 private:
  SharedHandleVK<vk::Framebuffer> framebuffer_;
  SharedHandleVK<vk::RenderPass> render_pass_;
-  mutable RWMutex layout_mutex_;


We don't do what today? We don't use the textures across more than one thread?

gaaclarke

This stuff is hard to review. LGTM as long as there is automated tests that are executing this, preferably with validation layers turned on.

jonahwilliams · 2025-03-25T16:27:51Z

reason for revert: render pass compatibility issue.

…it, tighten up graphics on level 3. (#165584)" This reverts commit 33cf3b6.

…lit, tighten up graphics on level 3. (#165584)" (#165898)  Reverts: #165584   Initiated by: jonahwilliams   Reason for reverting: render pass compatibility issue.   Original PR Author: jonahwilliams   Reviewed By: {gaaclarke}   This change reverts the following previous change: The change to fix PowerVR barriers regressed Mali (see #165538) We need to tighten the barriers to remove the transfer barrier to get back mali performance. however, this requires us to drop the usage of the blit for onscreen restore. not a huge loss imo. At any rate, all of these changes should be visible in benchmarks.  Co-authored-by: auto-submit[bot] <[email protected]>

…it, tighten up graphics on level 3. (flutter/flutter#165584)

…ten up graphics on level 3. (flutter#165584) The change to fix PowerVR barriers regressed Mali (see flutter#165538) We need to tighten the barriers to remove the transfer barrier to get back mali performance. however, this requires us to drop the usage of the blit for onscreen restore. not a huge loss imo. At any rate, all of these changes should be visible in benchmarks.

…lit, tighten up graphics on level 3. (flutter#165584)" (flutter#165898)  Reverts: flutter#165584   Initiated by: jonahwilliams   Reason for reverting: render pass compatibility issue.   Original PR Author: jonahwilliams   Reviewed By: {gaaclarke}   This change reverts the following previous change: The change to fix PowerVR barriers regressed Mali (see flutter#165538) We need to tighten the barriers to remove the transfer barrier to get back mali performance. however, this requires us to drop the usage of the blit for onscreen restore. not a huge loss imo. At any rate, all of these changes should be visible in benchmarks.  Co-authored-by: auto-submit[bot] <[email protected]>

relands flutter#165584 We can't have separate barriers for onscreen because it makes the render pass incompatible.

…ten up graphics on level 3. (flutter#165584) The change to fix PowerVR barriers regressed Mali (see flutter#165538) We need to tighten the barriers to remove the transfer barrier to get back mali performance. however, this requires us to drop the usage of the blit for onscreen restore. not a huge loss imo. At any rate, all of these changes should be visible in benchmarks.

…lit, tighten up graphics on level 3. (flutter#165584)" (flutter#165898)  Reverts: flutter#165584   Initiated by: jonahwilliams   Reason for reverting: render pass compatibility issue.   Original PR Author: jonahwilliams   Reviewed By: {gaaclarke}   This change reverts the following previous change: The change to fix PowerVR barriers regressed Mali (see flutter#165538) We need to tighten the barriers to remove the transfer barrier to get back mali performance. however, this requires us to drop the usage of the blit for onscreen restore. not a huge loss imo. At any rate, all of these changes should be visible in benchmarks.  Co-authored-by: auto-submit[bot] <[email protected]>

relands flutter#165584 We can't have separate barriers for onscreen because it makes the render pass incompatible.

[Impeller] remove transfer barriers from render pass, drop blit, tigh…

1e87fff

…ten up.

github-actions bot added engine flutter/engine related. See also e: labels. e: impeller Impeller rendering backend issues and features requests labels Mar 20, 2025

jonahwilliams commented Mar 20, 2025

View reviewed changes

jonahwilliams requested a review from gaaclarke March 20, 2025 17:57

jonahwilliams added 2 commits March 20, 2025 12:30

++

962ce70

Merge branch 'master' into tighten_up_barriers

305d47e

gaaclarke reviewed Mar 21, 2025

View reviewed changes

gaaclarke approved these changes Mar 21, 2025

View reviewed changes

Jonah Williams added 3 commits March 21, 2025 09:50

Update render_pass_builder_vk.cc

bca5fb3

Merge branch 'master' into tighten_up_barriers

83e9b11

Merge branch 'master' into tighten_up_barriers

eacbfeb

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

jonahwilliams added this pull request to the merge queue Mar 25, 2025

Merged via the queue into flutter:master with commit 33cf3b6 Mar 25, 2025
172 checks passed

jonahwilliams deleted the tighten_up_barriers branch March 25, 2025 15:44

flutter-dashboard bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Mar 25, 2025

jonahwilliams added the revert Autorevert PR (with "Reason for revert:" comment) label Mar 25, 2025

auto-submit bot pushed a commit that referenced this pull request Mar 25, 2025

Revert "[Impeller] remove transfer barriers from render pass, drop bl…

91436c7

…it, tighten up graphics on level 3. (#165584)" This reverts commit 33cf3b6.

auto-submit bot mentioned this pull request Mar 25, 2025

Reverts "[Impeller] remove transfer barriers from render pass, drop blit, tighten up graphics on level 3. (#165584)" #165898

Merged

auto-submit bot removed the revert Autorevert PR (with "Reason for revert:" comment) label Mar 25, 2025

engine-flutter-autoroll mentioned this pull request Mar 25, 2025

Roll Flutter from b16430b2fd57 to 8893670e1596 (245 revisions) flutter/packages#8898

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 25, 2025

33cf3b6 [Impeller] remove transfer barriers from render pass, drop bl…

9ec155b

…it, tighten up graphics on level 3. (flutter/flutter#165584)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 27, 2025

33cf3b6 [Impeller] remove transfer barriers from render pass, drop bl…

617d92c

…it, tighten up graphics on level 3. (flutter/flutter#165584)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 27, 2025

33cf3b6 [Impeller] remove transfer barriers from render pass, drop bl…

d305d0a

…it, tighten up graphics on level 3. (flutter/flutter#165584)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 28, 2025

33cf3b6 [Impeller] remove transfer barriers from render pass, drop bl…

913c5d6

…it, tighten up graphics on level 3. (flutter/flutter#165584)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 28, 2025

33cf3b6 [Impeller] remove transfer barriers from render pass, drop bl…

9c9c66f

…it, tighten up graphics on level 3. (flutter/flutter#165584)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 28, 2025

33cf3b6 [Impeller] remove transfer barriers from render pass, drop bl…

8a75b62

…it, tighten up graphics on level 3. (flutter/flutter#165584)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 28, 2025

33cf3b6 [Impeller] remove transfer barriers from render pass, drop bl…

7ac5d95

…it, tighten up graphics on level 3. (flutter/flutter#165584)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 29, 2025

33cf3b6 [Impeller] remove transfer barriers from render pass, drop bl…

9219271

…it, tighten up graphics on level 3. (flutter/flutter#165584)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 29, 2025

33cf3b6 [Impeller] remove transfer barriers from render pass, drop bl…

7b0cf1e

…it, tighten up graphics on level 3. (flutter/flutter#165584)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 30, 2025

33cf3b6 [Impeller] remove transfer barriers from render pass, drop bl…

6167fb1

…it, tighten up graphics on level 3. (flutter/flutter#165584)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 30, 2025

33cf3b6 [Impeller] remove transfer barriers from render pass, drop bl…

cd7f03f

…it, tighten up graphics on level 3. (flutter/flutter#165584)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 31, 2025

33cf3b6 [Impeller] remove transfer barriers from render pass, drop bl…

20e9a4a

…it, tighten up graphics on level 3. (flutter/flutter#165584)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 31, 2025

33cf3b6 [Impeller] remove transfer barriers from render pass, drop bl…

9484564

…it, tighten up graphics on level 3. (flutter/flutter#165584)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 31, 2025

33cf3b6 [Impeller] remove transfer barriers from render pass, drop bl…

54b096c

…it, tighten up graphics on level 3. (flutter/flutter#165584)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 31, 2025

33cf3b6 [Impeller] remove transfer barriers from render pass, drop bl…

698a0f7

…it, tighten up graphics on level 3. (flutter/flutter#165584)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 31, 2025

33cf3b6 [Impeller] remove transfer barriers from render pass, drop bl…

6b8c82a

…it, tighten up graphics on level 3. (flutter/flutter#165584)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 31, 2025

33cf3b6 [Impeller] remove transfer barriers from render pass, drop bl…

0fe71d9

…it, tighten up graphics on level 3. (flutter/flutter#165584)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 31, 2025

33cf3b6 [Impeller] remove transfer barriers from render pass, drop bl…

86802a9

…it, tighten up graphics on level 3. (flutter/flutter#165584)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 1, 2025

33cf3b6 [Impeller] remove transfer barriers from render pass, drop bl…

39fc6ee

…it, tighten up graphics on level 3. (flutter/flutter#165584)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 1, 2025

33cf3b6 [Impeller] remove transfer barriers from render pass, drop bl…

b69ddc4

…it, tighten up graphics on level 3. (flutter/flutter#165584)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 1, 2025

33cf3b6 [Impeller] remove transfer barriers from render pass, drop bl…

6fef640

…it, tighten up graphics on level 3. (flutter/flutter#165584)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 1, 2025

33cf3b6 [Impeller] remove transfer barriers from render pass, drop bl…

6d994b2

…it, tighten up graphics on level 3. (flutter/flutter#165584)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 20, 2025

33cf3b6 [Impeller] remove transfer barriers from render pass, drop bl…

2ca5333

…it, tighten up graphics on level 3. (flutter/flutter#165584)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 20, 2025

33cf3b6 [Impeller] remove transfer barriers from render pass, drop bl…

97a0bcd

…it, tighten up graphics on level 3. (flutter/flutter#165584)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 21, 2025

33cf3b6 [Impeller] remove transfer barriers from render pass, drop bl…

129a9b0

…it, tighten up graphics on level 3. (flutter/flutter#165584)

[Impeller] remove transfer barriers from render pass, drop blit, tighten up graphics on level 3. #165584

[Impeller] remove transfer barriers from render pass, drop blit, tighten up graphics on level 3. #165584

Uh oh!

Conversation

jonahwilliams commented Mar 20, 2025

Uh oh!

flutter-dashboard bot commented Mar 20, 2025

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!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonahwilliams commented Mar 25, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants