Allow external texture sources when using the Metal backend. #17154

chinmaygarde · 2020-03-14T22:50:02Z

Allow external texture sources when using the Metal backend.

This patch moves the responsibility of vending external texture proxies to the
client rendering API specific IOS context subclasses.

Some key changes in the Metal backend compared to OpenGL.

The Metal backend has a single texture cache for all external textures as
opposed to a separate cache for each texture proxy in the OpenGL backend. This
use is more desirable as cached entries may be reused by multiple proxies for
better cache utilization. This implementation is harder in the OpenGL backend
because the cache cannot be created upfront as thread local state is needed to
(re)initialize the cache. This update can be made to the OpenGL backend as
well but it is unrelated to the primary purpose of this patch.
Attempting to use external textures with the software backend now logs a
single message to the console that this feature is only available when using
the Metal or OpenGL backend on devices. Previously, an OpenGL texture proxy
creation would be attempted and it would fail in an undefined spot internally
with a confusing message.
The OpenGL backend seems to conflate rendering size with paint size. This puts
the unnecessary limitation that the embedders must provide textures at the
size at which they will be rendered. This limitation has been lifted in the
Metal backend and should probably be lifted in the OpenGL backend as well (in
a later patch).

There are opportunities to dry up the implementations of the
IOSExternalTextureMetal and IOSExternalTextureGL but that refactor can be
attempted separately.

Fixes flutter/flutter#41819

chinmaygarde · 2020-03-23T02:28:37Z

As c0deb23 has landed, this is good to go. All existing plugins that composite external textures have been tested to work with the Metal backend without modification.

fluttergithubbot · 2020-03-23T03:31:02Z

This pull request is not suitable for automatic merging in its current state.

Please get at least one approved review before re-applying this label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

dnfield · 2020-03-23T03:43:06Z

fml/platform/darwin/cf_utils.h

@@ -36,14 +51,20 @@ class CFRef {
    instance_ = instance;
  }

+  [[nodiscard]] T Release() {


Are we on C++ 17 everywhere we have to compile? Particularly google3?

dnfield · 2020-03-23T03:46:45Z

shell/platform/darwin/ios/ios_context_metal.mm

@@ -34,6 +35,19 @@
    return;
  }

+  CVMetalTextureCacheRef texture_cache_raw = NULL;
+  auto cv_return = CVMetalTextureCacheCreate(kCFAllocatorDefault,  // allocator
+                                             NULL,           // cache attributes (NULL default)


clang format doesn't line these up?

Apparently not. I don't format code so its got to be clang-format. I think its because the comments are pushed up against the column limit.

dnfield · 2020-03-23T03:47:34Z

shell/platform/darwin/ios/ios_context_metal.mm

+                                             &texture_cache_raw  // [out] cache
+  );
+  if (cv_return != kCVReturnSuccess) {
+    FML_LOG(ERROR) << "Could not create Metal texture cache.";


What does this mean for a Flutter developer? What action should be taken if any?

Hah. Docs are woefully lacking here, apparently it just documents that when it succeeds it returns this and nothing else... https://developer.apple.com/documentation/corevideo/1456774-cvmetaltexturecachecreate?language=objc

should this really be FML_DLOG?

What action should be taken if any?

If I saw that in the a bug report, I'd know immediately where what went wrong and where. If a user filed an error for this with no logs, I probably wouldn't find the cause unless I reproduced the issue and set various breakpoint (if the issue caught my attention at all).

I am on the fence about its use. Probably means I should just get rid of it. Patching.

dnfield · 2020-03-23T03:50:55Z

shell/platform/darwin/ios/ios_external_texture_metal.mm

+  // External images in this backend have no thread affinity and are not tied to the context in any
+  // way. Instead, they are tied to the Metal device which is associated with the cache already and
+  // is consistent throughout the shell run.


This is exciting.

dnfield

LGTM with nit about logging. If a developer can take action on it,w e should clarify what action to take - otherwise we should probably make a DLOG.

This patch moves the responsibility of vending external texture proxies to the client rendering API specific IOS context subclasses. Some key changes in the Metal backend compared to OpenGL. * The Metal backend has a single texture cache for all external textures as opposed to a separate cache for each texture proxy in the OpenGL backend. This use is more desirable as cached entries may be reused by multiple proxies for better cache utilization. This implementation is harder in the OpenGL backend because the cache cannot be created upfront as thread local state is needed to (re)initialize the cache. This update can be made to the OpenGL backend as well but it is unrelated to the primary purpose of this patch. * Attempting to use external textures with the software backend now logs a single message to the console that this feature is only available when using the Metal or OpenGL backend on devices. Previously, an OpenGL texture proxy creation would be attempted and it would fail in an undefined spot internally with a confusing message. * The OpenGL backend seems to conflate rendering size with paint size. This puts the unnecessary limitation that the embedders must provide textures at the size at which they will be rendered. This limitation has been lifted in the Metal backend and should probably be lifted in the OpenGL backend as well (in a later patch). There are opportunities to dry up the implementations of the `IOSExternalTextureMetal` and `IOSExternalTextureGL` but that refactor can be attempted separately. Fixes flutter/flutter#41819

chinmaygarde · 2020-03-23T04:32:25Z

If a developer can take action on it

This is unlikely for anything in these components. I converted everything to DLOG.

…lutter/engine#17154)

chinmaygarde added the Work in progress (WIP) Not ready (yet) for review! label Mar 14, 2020

googlebot added the cla: yes label Mar 14, 2020

auto-assign bot requested a review from cbracken March 14, 2020 22:51

chinmaygarde force-pushed the external_textures branch 3 times, most recently from 72a015e to ead5edd Compare March 19, 2020 02:51

chinmaygarde force-pushed the external_textures branch 2 times, most recently from e353f09 to 520383c Compare March 23, 2020 02:26

chinmaygarde added waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. and removed Work in progress (WIP) Not ready (yet) for review! labels Mar 23, 2020

chinmaygarde requested review from dnfield and jason-simmons March 23, 2020 02:27

fluttergithubbot removed the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Mar 23, 2020

dnfield reviewed Mar 23, 2020

View reviewed changes

dnfield approved these changes Mar 23, 2020

View reviewed changes

chinmaygarde force-pushed the external_textures branch from 520383c to 453d6a3 Compare March 23, 2020 04:31

chinmaygarde added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Mar 23, 2020

fluttergithubbot merged commit e56b335 into flutter:master Mar 23, 2020

engine-flutter-autoroll mentioned this pull request Mar 24, 2020

Roll engine 2d42c74a348d..d7ad50c96f05 (54 commits) flutter/flutter#53178

Closed

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

e56b335 Allow external texture sources when using the Metal backend. (f…

80efb16

…lutter/engine#17154)

engine-flutter-autoroll mentioned this pull request Mar 24, 2020

Roll engine 2d42c74a348d..3a68f689996f (56 commits) flutter/flutter#53196

Closed

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

e56b335 Allow external texture sources when using the Metal backend. (f…

9517098

…lutter/engine#17154)

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

Roll engine 2d42c74a348d..3a68f689996f (56 commits) flutter/flutter#53210

Merged

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

e56b335 Allow external texture sources when using the Metal backend. (f…

ee34650

…lutter/engine#17154)

chinmaygarde deleted the external_textures branch July 7, 2022 22:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Allow external texture sources when using the Metal backend. #17154

Allow external texture sources when using the Metal backend. #17154

Uh oh!

chinmaygarde commented Mar 14, 2020 •

edited

Loading

Uh oh!

chinmaygarde commented Mar 23, 2020

Uh oh!

fluttergithubbot commented Mar 23, 2020

Uh oh!

dnfield Mar 23, 2020

Uh oh!

chinmaygarde Mar 23, 2020

Uh oh!

dnfield Mar 23, 2020

Uh oh!

chinmaygarde Mar 23, 2020

Uh oh!

dnfield Mar 23, 2020

Uh oh!

dnfield Mar 23, 2020

Uh oh!

dnfield Mar 23, 2020

Uh oh!

chinmaygarde Mar 23, 2020

Uh oh!

dnfield Mar 23, 2020

Uh oh!

dnfield left a comment

Uh oh!

chinmaygarde commented Mar 23, 2020

Uh oh!

Uh oh!

Allow external texture sources when using the Metal backend. #17154

Allow external texture sources when using the Metal backend. #17154

Uh oh!

Conversation

chinmaygarde commented Mar 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chinmaygarde commented Mar 23, 2020

Uh oh!

fluttergithubbot commented Mar 23, 2020

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!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

chinmaygarde commented Mar 23, 2020

Uh oh!

Uh oh!

chinmaygarde commented Mar 14, 2020 •

edited

Loading