Reland [ios] Refactor IOSSurface factory and unify surface creation #22016

iskakaushik · 2020-10-20T22:53:55Z

This refactor is to enable the separation of Surface from External View Embedder. As it stands Surface has GetExternalViewEmbedder which inturn has the ability to create surfaces. This refactor enables us to separate these concepts.

This is driven by the goal to share some components between ios and macOS.

iskakaushik · 2020-10-21T20:09:17Z

I've added a test that would've caught this problem.

gaaclarke · 2020-10-22T20:13:25Z

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

@@ -27,7 +27,7 @@

 static const char* kApplicationKernelSnapshotFileName = "kernel_blob.bin";

-static flutter::Settings DefaultSettingsForProcess(NSBundle* bundle = nil) {
+flutter::Settings FLTDefaultSettingsForProcess(NSBundle* bundle) {


Nit: this name is goofy, a bundle isn't a process. If we are going to make this function visible we should maybe give it an appropriate name.

Good call, will do

gaaclarke · 2020-10-22T20:17:15Z

shell/platform/darwin/ios/framework/Source/FlutterDartProject_Internal.h

+/**
+ * This is currently used for *only for tests* to override settings.
+ */
+- (instancetype)initWithSettings:(flutter::Settings)settings;


nit: const flutter::Settings&

Yup, will do

gaaclarke · 2020-10-22T20:18:20Z

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

@@ -485,6 +501,7 @@ - (BOOL)createShell:(NSString*)entrypoint

  auto settings = [_dartProject.get() settings];
  FlutterView.forceSoftwareRendering = settings.enable_software_rendering;
+  NSLog(@"software!!! > %@", @(FlutterView.forceSoftwareRendering));


stray print statement

gaaclarke · 2020-10-22T20:22:33Z

shell/platform/darwin/ios/ios_surface_factory.h

+  ~IOSSurfaceFactory();
+
+  void SetPlatformViewsController(
+      FlutterPlatformViewsController* platform_views_controller);


You aren't suppose to pass objects via raw pointers: https://google.github.io/styleguide/cppguide.html#Ownership_and_Smart_Pointers

This style isn't really respected in the rest of the codebase. There are multiple places where platform_views_controller is passed as a raw pointer. I looked at changing it to a smart pointer, it's a pretty big refactor.

What's the motto on the team, embrace Zach's shave?

The codebase purports to follow Google C++ Style of which this is in violation. This violate isn't really style like where to put curly braces, but is policy to avoid creating runtime minefields. I don't even think there is a good justification for this class but if you want to keep it and not use smart pointers you could pass the FlutterPlatformViewsController as an argument to the CreateSurface. That way you would be avoiding the creation of another mine.

gaaclarke · 2020-10-22T20:27:03Z

shell/platform/darwin/ios/ios_surface_factory.h

+  void SetPlatformViewsController(
+      FlutterPlatformViewsController* platform_views_controller);
+
+  std::unique_ptr<IOSSurface> CreateSurface(


What's the point of a factory that doesn't have virtual methods? This could just be a function call.

Can you elaborate?

Yep, right now IOSSurfaceFactory is just an obfuscation on flutter::IOSSurface::Create

this:

IOSSurfaceFactory factory = IOSSurfaceFactory.create(renderingApi); factory.setPlatformViewsController(platformViewsController); return factory.createSurface(layer);

could just be this:

return CreateSurface(renderingApi, platformViewsController, layer);

The factory object doesn't actually add any functionality, it just obscures the logic and worse it creates a mine by storing a raw pointer. Typically factories are used to allow us to swap out the creation of an object, since this is a c++ class without virtual methods, it can't actually be swapped out.

gaaclarke · 2020-10-22T20:28:36Z

shell/platform/darwin/ios/framework/Source/FlutterPlatformViews_Internal.h

  FML_DISALLOW_COPY_AND_ASSIGN(FlutterPlatformViewLayerPool);
 };

 class FlutterPlatformViewsController {
 public:
-  FlutterPlatformViewsController();
+  FlutterPlatformViewsController(std::shared_ptr<IOSSurfaceFactory> surface_factory);


nit: const std::shared_ptr<IOSurfaceFactory>& shared_ptr's are threadsafe so copying the objects incurs locking primitives which are system calls that slow down execution.

This is interesting, I've been passing shared_ptr<T> by value rather than const shared_ptr<T>& as a convention. But clearly that has very little use, I will change it to const shared_ptr<T>& given that FlutterPlatformViewsController has no intention of sharing ownership of surface_factory. Also came across this which makes sense.

Yea, the engine doesn't use const refs for passing arguments when it should. I got yelled at in the past for passing std::functions by const reference. We don't use shared_ptr's much but they are particularly important to pass by const reference because of the mutex inside them. I've worked on projects that have been crippled by that overhead.

xster

Which test covers flutter/flutter#68399?

xster · 2020-10-22T19:02:53Z

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

@@ -485,6 +501,7 @@ - (BOOL)createShell:(NSString*)entrypoint

  auto settings = [_dartProject.get() settings];
  FlutterView.forceSoftwareRendering = settings.enable_software_rendering;
+  NSLog(@"software!!! > %@", @(FlutterView.forceSoftwareRendering));


Is this intentional?

xster · 2020-10-22T19:03:39Z

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

@@ -63,6 +65,8 @@ @implementation FlutterEngine {
  fml::WeakPtr<FlutterViewController> _viewController;
  fml::scoped_nsobject<FlutterObservatoryPublisher> _publisher;

+  flutter::IOSRenderingAPI _renderingApi;
+  std::shared_ptr<flutter::IOSSurfaceFactory> _surfaceFactory;


@gaaclarke might relate to the work you're doing now

Yea, it's probably going to create merge conflicts for me. I don't think it necessarily helps or hurts what I was doing.

xster · 2020-10-22T20:00:54Z

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

+  FlutterEngine* engine = [[FlutterEngine alloc] init];
+
+  // Run with an initial route.
+  [engine runWithEntrypoint:FlutterDefaultDartEntrypoint initialRoute:@"test"];


seems like the entrypoint/initialroute doesn't really matter? If so, you can just call run

Yup, will change it.

iskakaushik · 2020-10-22T20:44:43Z

Which test covers flutter/flutter#68399?

https://github.com/flutter/engine/pull/22016/files#diff-32d133f42e4c4efe9a1d50c25f673993fafd3b22c2fa8d6bf990e5aa6edf0383R105 the last test in this file captures the issue. The problem earlier was once the platform view controller was set, it wasn't recreated once the rendering api changes. This happens on simulators where createShell is when the rendering api is set.

xster · 2020-10-23T18:11:33Z

ah interesting. For future archeologists, can you describe what the issue was on flutter/flutter#68399 and also fill out the PR description with a FR bug or describe a bit what the intent of the PR is and the nature of the fix for flutter/flutter#68399?

Otherwise, test LGTM. Please address @gaaclarke's review.

iskakaushik · 2020-10-23T19:39:48Z

The issue flutter/flutter#68399 was because my original attempt at the refactor inadvertently made it so that we weren't re-creating the platform views controller once it was created. This is fine on real devices but on simulators this flow would break as follows:

initWithName on FlutterEngine gets called and that would create the platform views controller with the wrong rendering API as FlutterView.forceSoftwareRendering isn't yet set.
createShell gets called which sets FlutterView.forceSoftwareRendering, we ensure the platform views controller exists, which it already is.
Fail.

My current attempt at the refactor #22016 makes it so that instead of just ensuring that platform views controller exists, we re-create it after createShell gets called, this ensures that the platform views controller is created for the right rendering api. I've also added a test for this over here.

iskakaushik · 2020-10-26T17:57:01Z

@gaaclarke I've rebased it on-top of the PR that moves FlutterPlatformViewsController to be a smart pointer. PTAL

gaaclarke · 2020-10-26T18:02:45Z

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

+- (void)recreatePlatformViewController {
+  _renderingApi = flutter::GetRenderingAPIForProcess(FlutterView.forceSoftwareRendering);
+  _surfaceFactory = flutter::IOSSurfaceFactory::Create(_renderingApi);
+  auto pvc = new flutter::FlutterPlatformViewsController(_surfaceFactory);


nit: You aren't suppose to use abbreviations in variable names

https://google.github.io/styleguide/cppguide.html#General_Naming_Rules

Good catch. Removed this now.

gaaclarke · 2020-10-26T18:05:12Z

shell/platform/darwin/ios/framework/Source/FlutterPlatformViews_Internal.h

@@ -61,6 +61,7 @@ void ResetAnchor(CALayer* layer);

 class IOSContextGL;
 class IOSSurface;
+class IOSSurfaceFactory;


Nit: You are suppose to avoid forward declarations in C++

https://google.github.io/styleguide/cppguide.html#Forward_Declarations

There is a cyclical dependency here. IOSSurfaceFactory holds a platform views controller via SetPlatformViewsController and platform views controller also needs a surface factory for creating surfaces. I will simplify this in the follow-up PRs.

gaaclarke

Thanks for the changes, LGTM

fluttergithubbot · 2020-10-27T17:52:02Z

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

The status or check suite Mac iOS Engine has failed. Please fix the issues identified (or deflake) before re-applying this label.

This reverts commit 0c7c477.

On simulator for example we instantiate the app with `initWithName` after this is done, we might still not have set the `FlutterView.forceSoftwareRendering` which is done in `createShell`. This will ensure that we create the platform view controller with the right rendering api.

see: flutter/flutter#69305

…eation (flutter/engine#22016)

* 53d5d68 Add dart-lang/sdk's new package:clock dependency (flutter/engine#22142) * c32e3d8 Roll Skia from 7737a5bd2510 to 5567a6091ceb (8 revisions) (flutter/engine#22146) * 376045c Roll Fuchsia Linux SDK from XYN02FThN... to UKgKCjxSA... (flutter/engine#22147) * 03395de Roll Fuchsia Mac SDK from GKPwGj1Ux... to xHjtLQPQ5... (flutter/engine#22151) * 0c7e952 Manual Dart SDK roll 6e015bd9cddb to 9c6e76468ca4 (6 revisions (flutter/engine#22153) * e5f168a Update constraint to latest integration test (flutter/engine#22169) * e61e8c2 Smooth window resizing on macOS (flutter/engine#21525) * acece00 Allow parse_and_send to use access tokens (flutter/engine#22019) * 0270632 Includes roles for links, checkboxes, and radio buttons in semantics (flutter/engine#22061) * 632354d Roll Fuchsia Linux SDK from UKgKCjxSA... to PY5hNI-wY... (flutter/engine#22175) * 62d50af Add plumbing for command line arguments on Windows (flutter/engine#22094) * 06b0910 Fix possible use of std::moved value in Rasterizer (flutter/engine#22125) * 005dec4 [web] Clean up unused previousStyle logic (flutter/engine#22150) * ca05bdc Roll Skia from 5567a6091ceb to f548a028ce70 (7 revisions) (flutter/engine#22155) * 5b07ac4 Roll Fuchsia Mac SDK from xHjtLQPQ5... to ICK_JlnKJ... (flutter/engine#22188) * d615a97 Roll Fuchsia Linux SDK from PY5hNI-wY... to Usec4YBzR... (flutter/engine#22197) * 11ed711 Invalidate the cached SkParagraph font collection when a new font manager is installed (flutter/engine#22157) * 07c780b [web] Assign default natural width/height for svgs that report 0,0 on firefox and ie11 (flutter/engine#22184) * b54bb88 Migrate runZoned to runZonedGuarded (flutter/engine#22198) * 247139a [web] Fix transform not invalidating path bounds causing debugValidate failure (flutter/engine#22172) * e4dffc1 [web] Fix scroll wheel line delta on Firefox. (flutter/engine#21928) * d6627c6 Reland [ios] Refactor IOSSurface factory and unify surface creation (flutter/engine#22016) * ce1dd11 Standardize macro names for dartdoc macros (flutter/engine#22180) * f81bc37 [profiling] Handle thread_info to account for killed threads (flutter/engine#22170) * fd94c86 Fix for firefox custom clipping (flutter/engine#22182) * 9ccf9f1 bring back build_test to ensure we validate licenses (flutter/engine#22201) * 38f6665 Set strut font to Roboto if the given font hasn't been registered (flutter/engine#22129) * caf32d5 Add a proc table version of embedder API (flutter/engine#21813) * ed0f477 Use preTranslate when applying offset to matrix (flutter/engine#21648) * b457e2d Refactor make_mock_engine into fl_test (flutter/engine#21585) * 28497c8 Fix typos in FlValue docs (flutter/engine#21875) * bb32446 Fix FlTextInputPlugin tear down (flutter/engine#22007) * 7a7804b Add "input shield" to capture pointer input for reinjection (flutter/engine#22067) * fe85f94 Update painting.dart (flutter/engine#22195) * 99cc50d [ios] Surface factory holds the canonical reference to the external view embedder (flutter/engine#22206)

…eation (flutter/engine#22016)

xster · 2020-11-17T08:16:20Z

@iskakaushik @gaaclarke

I was just checking our performance and noticed we're making a lot of metal contexts.

Looks like we make one in recreatePlatformViewController in initWithName, one in a recreatePlatformViewController in FlutterEngine createShell and our normal metal context. Looks like all 3 are persisted too. These things are expensive. Do we really need 3? Could we create the first platform view metal context when users actually use platform view?

…lutter#22016)

flutter-dashboard bot added the platform-ios label Oct 20, 2020

google-cla bot added the cla: yes label Oct 20, 2020

iskakaushik force-pushed the ios-refactor-eve branch from 680489c to 366f333 Compare October 21, 2020 20:07

iskakaushik requested review from xster and blasten October 21, 2020 20:08

iskakaushik force-pushed the ios-refactor-eve branch 2 times, most recently from 3d78d0b to fd3fdc2 Compare October 22, 2020 16:08

xster requested a review from gaaclarke October 22, 2020 18:56

gaaclarke reviewed Oct 22, 2020

View reviewed changes

xster reviewed Oct 22, 2020

View reviewed changes

iskakaushik mentioned this pull request Oct 23, 2020

iOS simulator displays a blank screen flutter/flutter#68399

Closed

iskakaushik force-pushed the ios-refactor-eve branch 2 times, most recently from 2923694 to 2a1a735 Compare October 26, 2020 17:55

gaaclarke reviewed Oct 26, 2020

View reviewed changes

gaaclarke approved these changes Oct 26, 2020

View reviewed changes

iskakaushik force-pushed the ios-refactor-eve branch from 2a1a735 to 1718b47 Compare October 26, 2020 18:11

iskakaushik 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 Oct 26, 2020

iskakaushik force-pushed the ios-refactor-eve branch 2 times, most recently from 2f24554 to 3c63cd7 Compare October 27, 2020 16:47

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 Oct 27, 2020

iskakaushik added 3 commits October 29, 2020 10:31

Reland "[ios] Refactor IOSSurface factory and unify surface creation"

004b2a5

This reverts commit 0c7c477.

temporary fix for a memory leak

e4fbbb1

see: flutter/flutter#69305

iskakaushik force-pushed the ios-refactor-eve branch from e5e3e54 to e4fbbb1 Compare October 29, 2020 17:32

iskakaushik 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 Oct 29, 2020

fluttergithubbot merged commit d6627c6 into flutter:master Oct 29, 2020

engine-flutter-autoroll mentioned this pull request Oct 29, 2020

Roll Engine from 073263e39d1d to 9ccf9f120ddc (26 revisions) flutter/flutter#69335

Closed

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

d6627c6 Reland [ios] Refactor IOSSurface factory and unify surface cr…

66f9d15

…eation (flutter/engine#22016)

engine-flutter-autoroll mentioned this pull request Oct 29, 2020

Roll Engine from 073263e39d1d to fe85f946db04 (34 revisions) flutter/flutter#69343

Closed

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

d6627c6 Reland [ios] Refactor IOSSurface factory and unify surface cr…

0c4ff3e

…eation (flutter/engine#22016)

GaryQian mentioned this pull request Oct 29, 2020

Roll engine from 073263e39d1d to fe85f946db04 (34 revisions) manual flutter/flutter#69347

Closed

engine-flutter-autoroll mentioned this pull request Oct 29, 2020

Roll Engine from 073263e39d1d to fe85f946db04 (34 revisions) flutter/flutter#69349

Closed

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

d6627c6 Reland [ios] Refactor IOSSurface factory and unify surface cr…

bb8da83

…eation (flutter/engine#22016)

engine-flutter-autoroll mentioned this pull request Oct 30, 2020

Roll Engine from 073263e39d1d to 99cc50dfffab (35 revisions) flutter/flutter#69353

Merged

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

d6627c6 Reland [ios] Refactor IOSSurface factory and unify surface cr…

3d15837

…eation (flutter/engine#22016)

engine-flutter-autoroll mentioned this pull request Oct 30, 2020

Roll Engine from 073263e39d1d to 7d6180c3014a (37 revisions) flutter/flutter#69357

Closed

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

d6627c6 Reland [ios] Refactor IOSSurface factory and unify surface cr…

ef577d9

…eation (flutter/engine#22016)

engine-flutter-autoroll mentioned this pull request Oct 30, 2020

Roll Engine from 99cc50dfffab1 to 7d6180c3014a (2 revisions) flutter/flutter#69365

Closed

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

d6627c6 Reland [ios] Refactor IOSSurface factory and unify surface cr…

704a30d

…eation (flutter/engine#22016)

xster mentioned this pull request Nov 18, 2020

recreatePlatformViewController shouldn't create new metal contexts before use flutter/flutter#70783

Closed

chaselatta pushed a commit to chaselatta/engine that referenced this pull request Nov 30, 2020

Reland [ios] Refactor IOSSurface factory and unify surface creation (f…

5b1a2a7

…lutter#22016)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reland [ios] Refactor IOSSurface factory and unify surface creation #22016

Reland [ios] Refactor IOSSurface factory and unify surface creation #22016

iskakaushik commented Oct 20, 2020 •

edited

Loading

iskakaushik commented Oct 21, 2020

gaaclarke Oct 22, 2020

iskakaushik Oct 22, 2020

gaaclarke Oct 22, 2020

iskakaushik Oct 22, 2020

gaaclarke Oct 22, 2020

iskakaushik Oct 22, 2020

gaaclarke Oct 22, 2020

iskakaushik Oct 22, 2020

gaaclarke Oct 22, 2020

gaaclarke Oct 22, 2020

iskakaushik Oct 22, 2020

gaaclarke Oct 22, 2020

gaaclarke Oct 22, 2020

iskakaushik Oct 22, 2020

gaaclarke Oct 22, 2020

xster left a comment

xster Oct 22, 2020

iskakaushik Oct 22, 2020

xster Oct 22, 2020

gaaclarke Oct 22, 2020

xster Oct 22, 2020

iskakaushik Oct 22, 2020

iskakaushik commented Oct 22, 2020

xster commented Oct 23, 2020

iskakaushik commented Oct 23, 2020

iskakaushik commented Oct 26, 2020

gaaclarke Oct 26, 2020

iskakaushik Oct 26, 2020

gaaclarke Oct 26, 2020

iskakaushik Oct 26, 2020

gaaclarke left a comment

fluttergithubbot commented Oct 27, 2020

xster commented Nov 17, 2020

Reland [ios] Refactor IOSSurface factory and unify surface creation #22016

Reland [ios] Refactor IOSSurface factory and unify surface creation #22016

Conversation

iskakaushik commented Oct 20, 2020 • edited Loading

iskakaushik commented Oct 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iskakaushik commented Oct 22, 2020

xster commented Oct 23, 2020

iskakaushik commented Oct 23, 2020

iskakaushik commented Oct 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaaclarke left a comment

Choose a reason for hiding this comment

fluttergithubbot commented Oct 27, 2020

xster commented Nov 17, 2020

iskakaushik commented Oct 20, 2020 •

edited

Loading