Implement asynchronous texture uploads when using the Metal backend on iOS. #17039

chinmaygarde · 2020-03-09T22:11:47Z

This moves the Metal GrContext creation utilities from GPUSurfaceMetal into
a separate IOSContext object subclass. An analogue of this object was used in
the GL regime for the management of onscreen and offscreen contexts that were
not tied to the lifecycle of the GPUSurface. This pattern has now been
generalized for use with all backends that need a resource context
(IOSContextGL and IOContextMetal). Drawable size selection now has been
moved to be just in time before the layer is wrapped by Skia. This makes it
resilient to size updates.

The platform views controller management in the ExternalViewEmbedder interface
implementation was repeated three times for Metal, OpenGL and
Software rendering. This repetition has been removed and a single
implementation present in the base IOSSurface and used on all platforms.
Addition of new client rendering APIs should not affect how the engine renders
into the platform view interleaving levels.

All rendering API selection logic has been moved into a single set of utilities
in rendering_api_selection.h. This enables the removal of a lot of code blocks
guarded by FLUTTER_SHELL_ENABLE_METAL. The remaining uses of this will be
removed when unified builds are enabled.

The Metal backend now also adds traces similar to the GL backend.

The IOGLContext has been renamed to IOContextGL to be more in line with the
convention used in this library.

Fixes flutter/flutter#41827
Adds flutter/flutter#52150

…n iOS. This moves the Metal `GrContext` creation utilities from `GPUSurfaceMetal` into a separate `IOSContext` object subclass. An analogue of this object was used in the GL regime for the management of onscreen and offscreen contexts that were not tied to the lifecycle of the `GPUSurface`. This pattern has now been generalized for use with all backends that need a resource context (`IOSContextGL` and `IOContextMetal`). The platform views controller management in the `ExternalViewEmbedder` interface implementation was repeated three times for [Metal][metal], [OpenGL](opengl) and [Software](software) rendering. This repetition has been removed and a single implementation present in the base `IOSSurface` and used on all platforms. Addition of new client rendering APIs should not affect how the engine renders into the platform view interleaving levels. All rendering API selection logic has been moved into a single set of utilities in `rendering_api_selection.h`. This enables the removal of a lot of code blocks guarded by `FLUTTER_SHELL_ENABLE_METAL`. The remaining uses of this will be removed when unified builds are enabled. The Metal backend now also adds traces similar to the GL backend. The `IOGLContext` has been renamed to `IOContextGL` to be more in line with the convention used in this library. Fixes flutter/flutter#41827 Adds flutter/flutter#52150 [metal]: https://github.com/flutter/engine/blob/1194ba2b218706a201c5d2c5325b55a5932546c5/shell/platform/darwin/ios/ios_surface_metal.mm#L55 [opengl]: https://github.com/flutter/engine/blob/1194ba2b218706a201c5d2c5325b55a5932546c5/shell/platform/darwin/ios/ios_surface_gl.mm#L95 [software]: https://github.com/flutter/engine/blob/1194ba2b218706a201c5d2c5325b55a5932546c5/shell/platform/darwin/ios/ios_surface_software.mm#L146

dnfield

I didn't quite finish reviewing the code, but could we split this up a bit?

Right now, this PR appears to do a lot more than just work in async texture uploads - it also:

Adds more tracing
Renames several files/classes to line up in a more pleasing manner (e.g. ios_context_gl instead of ios_gl_context)
Refactors the way we set scale on the metal texture
Removes some defaults from opacity and transaction handling on layers

All of these have some places that could go wrong make it a bit difficult to figure out exactly where things went wrong. In particular, some of them require manual tests to verify (e.g. you need to manually use an iOS device and rotate it and verify the rendering is correct).

There'd be a bit less to test/verify here if we could split some of this out into a few separate PRs.

dnfield · 2020-03-09T22:15:00Z

lib/ui/painting/image_decoder.cc

          })
          .SetIfFalse([&result, context = io_manager->GetResourceContext(),
                       &pixmap, queue = io_manager->GetSkiaUnrefQueue()] {
+            TRACE_EVENT0("flutter", "MakeCrossContextImageFromPixmap");


nit can we split tracing that does not depend on this PR into a separate PR?

We've had some issues recently where adding more tracing has run into timeouts in the devicelab, and if that happens here it'll be nice to know that's all it is and not some other change in this PR.

Also, if this has to get reverted for some reason the tracing will stay.

dnfield · 2020-03-09T22:16:47Z

shell/gpu/gpu_surface_gl.h

               GPUSurfaceGLDelegate* delegate,
               bool render_to_surface);

+  // |Surface|


Similar nit on these.

dnfield · 2020-03-09T22:25:03Z

shell/platform/darwin/ios/framework/Source/FlutterOverlayView.mm


-  if ([self.layer isKindOfClass:[CAEAGLLayer class]]) {
-    CAEAGLLayer* layer = reinterpret_cast<CAEAGLLayer*>(self.layer);
-    layer.allowsGroupOpacity = NO;


Why are we dropping this here? According to https://developer.apple.com/documentation/quartzcore/calayer/1621277-allowsgroupopacity?language=objc this defaults to YES on iOS 7.0+.

dnfield · 2020-03-09T22:25:39Z

shell/platform/darwin/ios/framework/Source/FlutterOverlayView.mm

-    fml::scoped_nsobject<CAEAGLLayer> eagl_layer(
-        reinterpret_cast<CAEAGLLayer*>([self.layer retain]));
-    if (@available(iOS 9.0, *)) {
-      eagl_layer.get().presentsWithTransaction = YES;


Doesn't this break platform views on iOS?

dnfield · 2020-03-09T22:28:59Z

shell/platform/darwin/ios/framework/Source/FlutterOverlayView.mm

-    return std::make_unique<flutter::IOSSurfaceSoftware>(std::move(layer), nullptr);
-  }
+    (std::shared_ptr<flutter::IOSContext>)ios_context {
+  return flutter::IOSSurface::Create(std::move(ios_context),                              // context


This will now throw if we're on a simulator, right?

dnfield · 2020-03-09T22:29:53Z

shell/platform/darwin/ios/ios_context.mm

+    case IOSRenderingAPI::kSoftware:
+      break;


In particular right here, we'll fall into FML_CHECK(false) when running on a simulator. Or am I missing something?

chinmaygarde · 2020-03-09T23:32:20Z

Will re-open after consideration.

chinmaygarde · 2020-03-10T01:02:11Z

For whatever reason, I could not just update the branch on a closed PR and just re-open the PR. I have opened #17046 with updates and clarifications described in this comment.

chinmaygarde requested review from cbracken, dnfield and jason-simmons March 9, 2020 22:11

auto-assign bot requested a review from iskakaushik March 9, 2020 22:11

googlebot added the cla: yes label Mar 9, 2020

Licenses.

1cf871f

dnfield suggested changes Mar 9, 2020

View reviewed changes

chinmaygarde closed this Mar 9, 2020

chinmaygarde mentioned this pull request Mar 10, 2020

Implement asynchronous texture uploads when using the Metal backend on iOS. #17046

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Implement asynchronous texture uploads when using the Metal backend on iOS. #17039

Implement asynchronous texture uploads when using the Metal backend on iOS. #17039

Uh oh!

chinmaygarde commented Mar 9, 2020

Uh oh!

dnfield left a comment

Uh oh!

dnfield Mar 9, 2020

Uh oh!

dnfield Mar 9, 2020

Uh oh!

dnfield Mar 9, 2020

Uh oh!

dnfield Mar 9, 2020

Uh oh!

dnfield Mar 9, 2020

Uh oh!

dnfield Mar 9, 2020

Uh oh!

chinmaygarde commented Mar 9, 2020

Uh oh!

chinmaygarde commented Mar 10, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Implement asynchronous texture uploads when using the Metal backend on iOS. #17039

Implement asynchronous texture uploads when using the Metal backend on iOS. #17039

Uh oh!

Conversation

chinmaygarde commented Mar 9, 2020

Uh oh!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

dnfield Mar 9, 2020

Choose a reason for hiding this comment

Uh oh!

dnfield Mar 9, 2020

Choose a reason for hiding this comment

Uh oh!

dnfield Mar 9, 2020

Choose a reason for hiding this comment

Uh oh!

dnfield Mar 9, 2020

Choose a reason for hiding this comment

Uh oh!

dnfield Mar 9, 2020

Choose a reason for hiding this comment

Uh oh!

dnfield Mar 9, 2020

Choose a reason for hiding this comment

Uh oh!

chinmaygarde commented Mar 9, 2020

Uh oh!

chinmaygarde commented Mar 10, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants