[Impeller] Create a drawable backed TextureMTL. #48049

jonahwilliams · 2023-11-15T01:52:38Z

Allow creating a TextureMTL instance that is backend by a CAMetalLayer, which only acquires the drawable (and texture) lazily.

I split TextureMTL into two subclasses, one that has the standard wrapping behavior of a MTLTexture, and the other that holds a drawable. This is part of the foundational work for "removing drawable acquisition latency", as we need to delay aquisition of the drawable texture to a worker thread.

The drawable backed texture is only used in tests for now.

Part of flutter/flutter#138490

jonahwilliams · 2023-11-15T16:05:33Z

impeller/renderer/backend/metal/texture_mtl.mm

-TextureMTL::TextureMTL(TextureDescriptor p_desc,
+/// @brief an implementation of TextureMTL that is backed by a real Metal
+/// texture.
+class BetterNameTextureMTL final : public TextureMTL {


Help me come up with a better name.

chinmaygarde

The std::async feedback is the crux of the review. Everything else is nits.

On a related note, I thought we discussed making pass attachments taking texture descriptors instead of textures and have the texture be lazily evaluation. This would move the lazy acquisition of the on-screen drawable above the HAL which should be more desirable. But that we can do that separately.

chinmaygarde · 2023-11-15T18:10:57Z

impeller/renderer/backend/metal/texture_mtl.mm

+      return;
+    }
+
+    if (desc.size != GetSize()) {


Do we not have a utility to just construct an impeller::TextureDescriptor from an MTLTextureDescriptor? Seems like one can be inferred from the other and we won't have to check if they disagree. Impeller texture descriptors are comparable.

This was an existing check 🤷

Also we need to relax this in order to make the texture cache more effective, but that is for another day.

chinmaygarde · 2023-11-15T18:12:24Z

impeller/renderer/backend/metal/texture_mtl.mm

+};
+
+/// @brief A class that holds the mutable state of the DrawableTextureMTL.
+class DrawableHolder {


I'd move these to their own TU. Generally prefer classes in their own TUs. Even for ones that are ostensibly "internal" to others for ease of code navigation IMO.

Moved this to lazy_drawable_holder.h

chinmaygarde · 2023-11-15T18:16:47Z

impeller/renderer/backend/metal/texture_mtl.mm

+/// the next drawable of the provided metal layer. This class is intended to be
+/// used to allow referencing the onscreen Metal drawable without blocking on
+/// drawable acquisition.
+class DrawableTextureMTL final : public TextureMTL {


LazilyAcquiredFromLayerTextureMTL? Is it bad if the only argument is that its silly-ly long?

Removed, only have one class now

chinmaygarde · 2023-11-15T18:37:13Z

impeller/renderer/backend/metal/texture_mtl.mm

+
+  ~DrawableHolder() = default;
+
+  bool AcquireNextDrawable(CAMetalLayer* layer) {


This seems to be a couple of levels of overkill. All we need is a lambda that is lazily evaluated and its results cached. std::async with std::launch::deferred will defer calling the lambda on the calling thread when the future is awaited upon. And, the future itself will hold the value around till you discard it.

So the DrawableHolder can just be std::future<std::shared_ptr<impeller::Texture>> (typedef it maybe). That can be obtained using a call to std::async(std::launch::deferred, [layer](){return WrapIntoAtexture([layer nextDrawable]);})

For style points, TextureMTL itself can take only a single argument which is a lambda that returns a texture. A lambda that will always be evaluated lazily. And in case you already have a texture, the lambda becomes [texture](){return texture};. Create a factory that creates the lambda if need be to keep the interface the same.

That way, you don't have to create a subclass hierarchy to LayerMTL. More importantly, you also don't have to think about naming the subclasses.

Unfortunately I need access to both the texture and its drawable, the former to add to the MTL attachment, the later to present in SurfaceMTL.

I can definitely compress this into a subclass, but I can't think of a way to do so without adding new API to query the drawable state.

Unfortunately I need access to both the texture and its drawable, the former to add to the MTL attachment, the later to present in SurfaceMTL.

Thats just a future to a std::pair or struct.

But the usage of the drawable isn't internal to the texture or to the callback, its done by the surface MTL. So roughly:

surface_mtl has access to CAMetalLayer.

Create TextureMTL from CAMetalLayer.
3a. Worker thread begins encoding root pass and waits on next drawable.
3b. surface_mtl waits on same drawable (without requesting it twice) and presents.

But thinking about it, I could probably have an object captured by that callback manage this state (essentially the state holder object I created)?

So I'll give that a shot. We do stil need an additional bit for whether or not the texture is lazy

chinmaygarde · 2023-11-15T18:37:22Z

impeller/renderer/backend/metal/texture_mtl_unittests.mm

+}
+
+}  // namespace testing
+}  // namespace impeller


Newline at EOF.

jonahwilliams · 2023-11-15T18:52:03Z

On a related note, I thought we discussed making pass attachments taking texture descriptors instead of textures and have the texture be lazily evaluation.

I don't recall discussing this, or even how we would do it. Can you elaborate a bit?

chinmaygarde · 2023-11-15T19:05:59Z

As I recall (this was during a tail end of one of the weekly syncs), the observation was that the presence of full blown textures on impeller::Attachments on render passes was overkill. And all we needed was a texture descriptor. Even if we did needed the texture, it could be a future to a texture that could be lazily evaluated. As long as the texture descriptors were compatible with the textures obtained later during drawable-present, the command buffer could be filled.

As you said, not sure this is possible. But we were wondering if impeller::Attachment::texture and impeller::Attachment::resolve_texture were optimal APIs.

jonahwilliams · 2023-11-15T22:47:07Z

Still need to do some refactoring to make this use std::async and std::launch::deferred

…ngine into drawable_wrapped_texture

jonahwilliams · 2023-11-16T00:32:52Z

Updated to use std::shared_future and std::async. The idea is that the surface_mtl will wrap the CAMetalLayer with this callback, then pass a copy to the TextureMTL used in the root pass resolve. Then to present the surface_mtl should be able to block on getting the drawable and it should be cached.

…gine#48049)

…138568) flutter/engine@22baa83...094a338 2023-11-16 [email protected] Roll Skia from 297049bbfc0d to 4c099aaa259f (5 revisions) (flutter/engine#48138) 2023-11-16 [email protected] Roll Dart SDK from b512191e9612 to 906f23c1cb20 (1 revision) (flutter/engine#48136) 2023-11-16 [email protected] Disable the `runIfNot` clauses in `.ci.yaml`, as they are unsafe. (flutter/engine#48132) 2023-11-16 [email protected] Fix race condition in Unobstructed Platform View Scenario tests (flutter/engine#48096) 2023-11-16 [email protected] [Impeller] store all path point data in single buffer. (flutter/engine#47896) 2023-11-16 [email protected] Roll Skia from b9ead4140f84 to 297049bbfc0d (2 revisions) (flutter/engine#48133) 2023-11-16 [email protected] [macOS] Replace pasteboard mock with fake (flutter/engine#48110) 2023-11-16 [email protected] [Impeller] Fix issue where the lock was not re-acquired when the wait exits on CV. (flutter/engine#48104) 2023-11-16 [email protected] [Impeller] Create a drawable backed TextureMTL. (flutter/engine#48049) 2023-11-16 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Make `flow/embedded_views.h` compatible with `.clang_tidy`." (flutter/engine#48130) 2023-11-16 [email protected] [macOS] Replace fixture subclasses with usings (flutter/engine#48111) 2023-11-16 [email protected] Make `flow/embedded_views.h` compatible with `.clang_tidy`. (flutter/engine#47994) 2023-11-16 [email protected] Make `flow/...` compatible with `.clang_tidy`. (flutter/engine#47995) 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],[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

[Impeller] drawable backed texture.

18a52d8

jonahwilliams marked this pull request as draft November 15, 2023 01:52

flutter deleted a comment from flutter-dashboard bot Nov 15, 2023

github-actions bot added the e: impeller label Nov 15, 2023

jonahwilliams added 4 commits November 14, 2023 18:43

++

a70d76c

++

90ff1c9

add unittests.

980767b

remove mutable.

acb7c68

jonahwilliams commented Nov 15, 2023

View reviewed changes

++

915b592

jonahwilliams marked this pull request as ready for review November 15, 2023 16:08

jonahwilliams changed the title ~~[Impeller][WIP] drawable backed texture.~~ [Impeller] Create a drawable backed TextureMTL. Nov 15, 2023

jonahwilliams requested review from chinmaygarde and dnfield November 15, 2023 16:16

jonahwilliams added 2 commits November 15, 2023 08:42

++

5651c46

++

f3ccf9a

chinmaygarde reviewed Nov 15, 2023

View reviewed changes

revert texture changes.

3063d65

jonahwilliams added 6 commits November 15, 2023 11:46

use single texture class.

fea4f0b

+

43e4820

++

19ef071

license and ignore

4264cdf

++

d00491e

Merge branch 'main' into drawable_wrapped_texture

e6dc393

jonahwilliams added 3 commits November 15, 2023 15:49

++

22f5730

++

bdef2bc

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

445386d

…ngine into drawable_wrapped_texture

revert unrelated change

f228243

jonahwilliams requested a review from chinmaygarde November 16, 2023 00:33

chinmaygarde approved these changes Nov 16, 2023

View reviewed changes

jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 16, 2023

auto-submit bot merged commit 0d348e7 into flutter:main Nov 16, 2023

engine-flutter-autoroll mentioned this pull request Nov 16, 2023

Roll Flutter Engine from 22baa83db63b to 13cdf933d641 (10 revisions) flutter/flutter#138563

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 16, 2023

0d348e7ef [Impeller] Create a drawable backed TextureMTL. (flutter/en…

ffa1694

…gine#48049)

engine-flutter-autoroll mentioned this pull request Nov 16, 2023

Roll Flutter Engine from 22baa83db63b to 094a3383a406 (13 revisions) flutter/flutter#138568

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 16, 2023

0d348e7ef [Impeller] Create a drawable backed TextureMTL. (flutter/en…

73e5fbe

…gine#48049)


		~DrawableHolder() = default;

		bool AcquireNextDrawable(CAMetalLayer* layer) {

[Impeller] Create a drawable backed TextureMTL. #48049

[Impeller] Create a drawable backed TextureMTL. #48049

Uh oh!

Conversation

jonahwilliams commented Nov 15, 2023 • 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!

chinmaygarde left a comment

Choose a reason for hiding this comment

Uh oh!

chinmaygarde Nov 15, 2023 • 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!

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!

jonahwilliams commented Nov 15, 2023

Uh oh!

chinmaygarde commented Nov 15, 2023

Uh oh!

jonahwilliams commented Nov 15, 2023

Uh oh!

jonahwilliams commented Nov 16, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jonahwilliams commented Nov 15, 2023 •

edited

Loading

chinmaygarde Nov 15, 2023 •

edited

Loading