[Impeller] allocate the impeller onscreen texture from the render target cache. #55943

jonahwilliams · 2024-10-18T02:19:27Z

Otherwise with backdrop filters we create and throw away a single fullscreen texture every frame. Very wasteful!

Does not impact iOS because it has the read from resolve option. Removed some unused parts of the inline_pass_context that were only used in the entity pass days.

…get cache.

…_rt_cache

jonahwilliams · 2024-10-18T16:29:47Z

impeller/entity/inline_pass_context.cc

-    pass_ = collapsed_parent_pass.value().pass;
-  }
-}
+InlinePassContext::InlinePassContext(const ContentContext& renderer,


entity_count, collapsed_parent_pass, is_collapsed_ are all unused

jonahwilliams · 2024-10-18T16:30:11Z

impeller/entity/inline_pass_context.h


 class InlinePassContext {
 public:
-  struct RenderPassResult {


None of these are used anymore

jonahwilliams · 2024-10-18T16:34:10Z

impeller/entity/entity_pass_target.cc

    TextureDescriptor new_descriptor =
        color0.resolve_texture->GetTextureDescriptor();
-    secondary_color_texture_ = allocator.CreateTexture(new_descriptor);
+    RenderTarget target = renderer.GetRenderTargetCache()->CreateOffscreenMSAA(


When we "flip" the backdrop texture, do the new allocation from the render target cache. Technically this creates some unused transients too but :shrug.

While we still only create a maximum of two backdrop textures per frame, this change ensures we recycle them across frames and avoid continual re-allocation

gaaclarke · 2024-10-18T20:13:48Z

Is there an issue associated with this change? Sounds similar to some of the others I've seen.

jonahwilliams · 2024-10-18T20:15:42Z

The related issues is flutter/flutter#149368 , we still have worst frame instability compared to skia which I suspect is related to the allocation.

gaaclarke

looks mostly good to me, I just have one question about msaa

gaaclarke · 2024-10-18T20:17:57Z

impeller/display_list/canvas.cc

  }

-  entity.Render(renderer_, *result.pass);
+  entity.Render(renderer_, *result);


Add a FML_DCHECK that we can dereference this.

its null checked on the previous line?

gaaclarke · 2024-10-18T20:20:46Z

impeller/entity/entity_pass_target.cc

    TextureDescriptor new_descriptor =
        color0.resolve_texture->GetTextureDescriptor();
-    secondary_color_texture_ = allocator.CreateTexture(new_descriptor);
+    RenderTarget target = renderer.GetRenderTargetCache()->CreateOffscreenMSAA(


Should there be a branch here if we aren't using MSAA?

See above, we'll never use this on a non-MSAA target.

auto color0 = target_.GetColorAttachments().find(0)->second; if (!color0.resolve_texture) { VALIDATION_LOG << "EntityPassTarget Flip should never be called for a " "non-MSAA target.";

gaaclarke · 2024-10-18T20:22:41Z

impeller/entity/entity_pass_target_unittests.cc


-  entity_pass_target.Flip(
-      *content_context->GetContext()->GetResourceAllocator());
+  entity_pass_target.Flip(*content_context);


Same here, DCHECK for nullptr.

gaaclarke · 2024-10-18T21:02:18Z

impeller/entity/inline_pass_context.h


  uint32_t GetPassCount() const;

-  RenderPassResult GetRenderPass(uint32_t pass_depth);


This is just straight up dead code regardless of your change? No need to change this PR but for future PRs it would be easier if refactoring and functional changes are split into 2 different prs =)

Yeah I get it. Though I didn't intend to do a large refactoring, I realized all this was dead when I started changing it.

gaaclarke

lgtm, modulo a few nits on dereferencing a pointer into a reference that's potentially null.

…ngine into allocation_from_rt_cache

…render target cache. (flutter/engine#55943)

…157215) flutter/engine@76d310e...dd37446 2024-10-19 [email protected] Move keymap from FlKeyboardViewDelegate to FlKeyboardManager (flutter/engine#55942) 2024-10-19 [email protected] [UI] fix scene builder parameter naming. (flutter/engine#55969) 2024-10-19 [email protected] iOS: Improve thread safety of first frame callback (flutter/engine#55966) 2024-10-18 [email protected] iOS: Fix flaky tests (remove timeouts) (flutter/engine#55961) 2024-10-18 [email protected] [Impeller] allocate the impeller onscreen texture from the render target cache. (flutter/engine#55943) 2024-10-18 [email protected] Roll Fuchsia Linux SDK from 9F_NaKPd2twhbPwP7... to tNQZ8d5mRYpe3--lk... (flutter/engine#55963) 2024-10-18 [email protected] Started filtering out close line segments in rrect polylines. (flutter/engine#55929) 2024-10-18 [email protected] [Impeller] libImpeller: Allow custom font registrations. (flutter/engine#55934) 2024-10-18 [email protected] Re-reland "iOS: Migrate FlutterEngine to ARC" (flutter/engine#55962) 2024-10-18 [email protected] [Impeller] libImpeller: Add a README. (flutter/engine#55940) 2024-10-18 [email protected] iOS: Eliminate needless profiler metrics ivar (flutter/engine#55957) 2024-10-18 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[Impeller] one descriptor pool per frame. (#55939)" (flutter/engine#55959) 2024-10-18 [email protected] Revert "Reland "iOS: Migrate FlutterEngine to ARC" (#55937)" (flutter/engine#55954) 2024-10-18 [email protected] Roll Dart SDK from 993d3069f42e to a51df90298ca (7 revisions) (flutter/engine#55951) 2024-10-18 [email protected] [engine] add back opt out for merged threads. (flutter/engine#55952) 2024-10-18 [email protected] [Impeller] one descriptor pool per frame. (flutter/engine#55939) 2024-10-18 [email protected] [Impeller] add mechanism for sharing bdf inputs. (flutter/engine#55701) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from 9F_NaKPd2twh to tNQZ8d5mRYpe 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

…get cache. (flutter/engine#55943) Otherwise with backdrop filters we create and throw away a single fullscreen texture every frame. Very wasteful! Does not impact iOS because it has the read from resolve option. Removed some unused parts of the inline_pass_context that were only used in the entity pass days.

[Impeller] allocate the impeller onscreen texture from the render tar…

d1800ba

…get cache.

github-actions bot added the e: impeller label Oct 18, 2024

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

ff7bc16

…_rt_cache

jonahwilliams commented Oct 18, 2024

View reviewed changes

jonahwilliams marked this pull request as ready for review October 18, 2024 16:41

Merge branch 'main' into allocation_from_rt_cache

b6053b7

jonahwilliams requested a review from gaaclarke October 18, 2024 20:12

gaaclarke reviewed Oct 18, 2024

View reviewed changes

gaaclarke approved these changes Oct 18, 2024

View reviewed changes

jonahwilliams added 2 commits October 18, 2024 14:13

add null checks.

a6b6b32

Merge branch 'allocation_from_rt_cache' of github.com:jonahwilliams/e…

302ca9b

…ngine into allocation_from_rt_cache

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

auto-submit bot merged commit cd8c39c into flutter:main Oct 18, 2024
30 checks passed

engine-flutter-autoroll mentioned this pull request Oct 19, 2024

Roll Flutter Engine from 76d310e42c06 to 0d8a8766cbff (16 revisions) flutter/flutter#157213

Closed

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

cd8c39c28 [Impeller] allocate the impeller onscreen texture from the …

b68c071

…render target cache. (flutter/engine#55943)

engine-flutter-autoroll mentioned this pull request Oct 19, 2024

Roll Flutter Engine from 76d310e42c06 to dd37446fbc55 (17 revisions) flutter/flutter#157215

Merged

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

cd8c39c28 [Impeller] allocate the impeller onscreen texture from the …

7161258

…render target cache. (flutter/engine#55943)

gaaclarke mentioned this pull request Aug 20, 2025

UI glitches when Impeller is enabled on some Android devices. flutter/flutter#171772

Closed


		uint32_t GetPassCount() const;

		RenderPassResult GetRenderPass(uint32_t pass_depth);

[Impeller] allocate the impeller onscreen texture from the render target cache. #55943

[Impeller] allocate the impeller onscreen texture from the render target cache. #55943

Uh oh!

Conversation

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

Uh oh!

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 commented Oct 18, 2024

Uh oh!

jonahwilliams commented Oct 18, 2024

Uh oh!

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

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

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jonahwilliams commented Oct 18, 2024 •

edited

Loading

jonahwilliams Oct 18, 2024 •

edited

Loading