Manage resource and onscreen contexts using separate IOSGLContext objects #11798

gw280 · 2019-08-30T23:10:59Z

Initial attempt at managing the onscreen and resource GL contexts on iOS using separate IOSGLContext objects.

I still need to work out when to destroy the objects, but this seemed like a good place to get initial feedback.

My biggest concern with this patch is the static where I store the sharegroup to allow for the same sharegroup to be used to generate the contexts.

…ects

…ation. minor style updates. release onscreenContext at FlutterView destruction

xster · 2019-09-03T18:31:02Z

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

 @implementation FlutterView

 id<FlutterViewEngineDelegate> _delegate;
+std::shared_ptr<flutter::IOSGLContext> _onscreenContext;


thanks for renaming stuff too. This was always confusing to me.

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

shell/platform/darwin/ios/ios_gl_context.h

- camelCase for objc variables - switch to using a factory for creating contexts using the same sharedgroup -> hides implementation details of shared contexts from the caller

gw280 · 2019-09-03T21:42:12Z

So I'm in the process of running this through @xster's testcase and it looks like it's still leaking the IOSurface, so I'm investigating further.

…way. Refcounting fixes.

chinmaygarde

Other than the leaky context, just nits.

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

chinmaygarde · 2019-09-04T22:40:50Z

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

                          opaque:(BOOL)opaque NS_DESIGNATED_INITIALIZER;
 - (std::unique_ptr<flutter::IOSSurface>)createSurface:
-    (std::shared_ptr<flutter::IOSGLContext>)context;
+    (std::shared_ptr<flutter::IOSGLContext>)resourceContext;


Same comment about the ObjC naming convention.

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

shell/platform/darwin/ios/ios_gl_render_target.h

- Use WeakPtr to reference the onscreen/resource contexts outside of FlutterView - FlutterView now explicitly owns the onscreen context through a unique_ptr - (hacky for now - need to assess whether it's the right call) add a nullptr_t constructor for fml::WeakPtr - ObjC naming convention nits

…nstead, as per existing convention

dnfield

Seems like we're getting close. I haven't audited this for thread safety closely but nothing is jumping out as wrong, would defer to Chinmay on that one.

dnfield · 2019-09-05T18:07:59Z

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

-    (std::shared_ptr<flutter::IOSGLContext>)gl_context {
+- (std::unique_ptr<flutter::IOSSurface>)
+    createSurfaceWithOnscreenContext:(fml::WeakPtr<flutter::IOSGLContext>)onscreen_gl_context
+                     resourceContext:(fml::WeakPtr<flutter::IOSGLContext>)resource_gl_context {


nit: these should be onscreenGLContext and resourceGLContext respectively

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

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

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

shell/platform/darwin/ios/ios_gl_render_target.h

shell/platform/darwin/ios/ios_gl_render_target.mm

chinmaygarde

Other than the nit about the target argument not being used, looks good to me. Thanks.

chinmaygarde · 2019-09-05T18:45:09Z

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

-@implementation FlutterView
-
-id<FlutterViewEngineDelegate> _delegate;
+@implementation FlutterView {


Can we file a bug to audit the codebase for existing issues like this? I the _delegate being static got past code review, there may be more instance of this kind of misuse.

flutter/flutter#39933

shell/platform/darwin/ios/ios_gl_context.mm

dnfield

LGTM

chinmaygarde · 2019-09-07T00:44:03Z

This is causing build failures on LUCI.

…text objects (flutter#11798)" This reverts commit a353f93.

xster · 2019-09-07T00:58:27Z

Can we add a note to https://github.com/flutter/flutter/wiki/Setting-up-the-Engine-development-environment or some such for how to run the LUCI tests locally before submitting until we have pre-submit LUCI tests on iOS?

…text objects (#11798)" (#12055) This reverts commit a353f93.

iskakaushik · 2019-09-07T01:19:55Z

I had a similar thing happen to me a few days ago. I filed flutter/flutter#39336 to re-enable iOS tests. The unfortunate situation is that MacStadium cirrus' provider for ios containers got so flaky that every test had to be triggered twice. It was a reasonable trade-off that we made at the time.

This looks like it was a compile failure -- I haven't dug into this too much. Did building locally work?

gw280 · 2019-09-07T01:24:28Z

Yes, building locally worked (I loaded the Xcode project locally and built it and ran it on device). I will back out the change and sort it out on Monday.

…

On Fri, Sep 6, 2019 at 6:20 PM Kaushik Iska ***@***.***> wrote: I had a similar thing happen to me a few days ago. I filed flutter/flutter#39336 <flutter/flutter#39336> to re-enable iOS tests. The unfortunate situation is that MacStadium cirrus' provider for ios containers got so flaky that every test had to be triggered twice. It was a reasonable trade-off that we made at the time. This looks like it was a compile failure -- I haven't dug into this too much. Did building locally work? — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#11798?email_source=notifications&email_token=AAJKDQIXRZYM7MAXFHIGWFTQIL6UXA5CNFSM4ISRY7C2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6EMYJY#issuecomment-529058855>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJKDQLDSYSMWUWNABZ2RK3QIL6UXANCNFSM4ISRY7CQ> . -- You received this message because you are subscribed to the Google Groups "gw280" group. To unsubscribe from this group and stop receiving emails from it, send an email to ***@***.***

chinmaygarde · 2019-09-07T01:24:42Z

Running LUCI tests locally is documented at go/flutter-luci#heading=h.80m6y2c7elmx. That test target does not use the GN build system and needs a specific engine variant to be built before invoking an extra step to build the test harness. So building just the iOS target and using run_tests.py would not have been sufficient anyway. @dnfield mentioned that this was going to be moved to use the GN build system and integrated with the rest of the build/test setup.

dnfield · 2019-09-07T02:26:16Z

Chinmay's doc link is correct - and you'd need internal access AFAIK to run it, so not sure it makes sense to document publicly. We also have limited resources for running these right now - but that should change.

I'd be interested in seeing why this passed locally vs. CI, but my guesses are it may not have been run on the simulator locally, or that some warning wasn't enabled...

Once I get Fuchsia tests running I can work on wiring this up to GN. It might be a little funky because we need both host dart and the target framework/gen_snapshot to run these tests, but it should also make it clearer on how to run them on CI for physical devices.

…text objects (flutter/engine#11798)

…ects (flutter#11798) Manage resource and onscreen contexts using separate IOSGLContext objects

…text objects (flutter/engine#11798)

…ects (flutter#11798) Manage resource and onscreen contexts using separate IOSGLContext objects

…text objects (flutter/engine#11798)

Manage resource and onscreen contexts using separate IOSGLContext obj…

8cdc516

…ects

googlebot added the cla: yes label Aug 30, 2019

gw280 requested review from cbracken and chinmaygarde August 30, 2019 23:18

formatting

9f34f1a

gw280 requested a review from dnfield September 3, 2019 17:15

get rid of sharegroup static, plumb it through at time of surface cre…

f62387b

…ation. minor style updates. release onscreenContext at FlutterView destruction

gw280 mentioned this pull request Sep 3, 2019

Calling PlatformView.NotifyDestroyed still leaks IOSurfaces flutter/flutter#38736

Closed

xster reviewed Sep 3, 2019

View reviewed changes

dnfield reviewed Sep 3, 2019

View reviewed changes

review updates

78ef3a2

- camelCase for objc variables - switch to using a factory for creating contexts using the same sharedgroup -> hides implementation details of shared contexts from the caller

George Wright added 2 commits September 4, 2019 14:33

Ensure we properly destroy the IOSSurface when the FlutterView goes a…

2b9e022

…way. Refcounting fixes.

formatting

ff01e4b

chinmaygarde reviewed Sep 4, 2019

View reviewed changes

dnfield reviewed Sep 4, 2019

View reviewed changes

shell/platform/darwin/ios/ios_gl_render_target.h Outdated Show resolved Hide resolved

George Wright added 3 commits September 5, 2019 10:55

WeakPtr -> GetWeakPtr

531e86a

get rid of nullptr_t ctor and explicitly construct an empty WeakRef i…

1fee160

…nstead, as per existing convention

dnfield suggested changes Sep 5, 2019

View reviewed changes

chinmaygarde reviewed Sep 5, 2019

View reviewed changes

George Wright added 2 commits September 5, 2019 13:29

review comments - minor style updates and remove errant std::move

30ec6c8

more nits

7f8ca9c

dnfield approved these changes Sep 5, 2019

View reviewed changes

objc naming update

a897c51

gw280 mentioned this pull request Sep 5, 2019

Audit engine objective-c[++] code for correct instance variable usage flutter/flutter#39933

Closed

George Wright added 4 commits September 6, 2019 15:11

unit test

230a40c

unit tests with surface checking

73a7c9a

style updates

89fdd6b

call selector directly instead of using performSelector

97f41c4

ensure we manually release the engine/viewcontroller now that ARC is off

4386478

gw280 merged commit a353f93 into flutter:master Sep 6, 2019

iskakaushik pushed a commit to iskakaushik/engine that referenced this pull request Sep 7, 2019

Revert "Manage resource and onscreen contexts using separate IOSGLCon…

120ca01

…text objects (flutter#11798)" This reverts commit a353f93.

iskakaushik mentioned this pull request Sep 7, 2019

Revert "Manage resource and onscreen contexts using separate IOSGLCon… #12055

Merged

iskakaushik added a commit that referenced this pull request Sep 7, 2019

Revert "Manage resource and onscreen contexts using separate IOSGLCon…

35875e0

…text objects (#11798)" (#12055) This reverts commit a353f93.

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 7, 2019

a353f93 Manage resource and onscreen contexts using separate IOSGLCon…

edf0824

…text objects (flutter/engine#11798)

engine-flutter-autoroll mentioned this pull request Sep 7, 2019

Roll engine c9ea4dba8da6..075a61fc4f70 (8 commits) flutter/flutter#40043

Closed

gw280 added a commit to gw280/engine that referenced this pull request Sep 9, 2019

Manage resource and onscreen contexts using separate IOSGLContext obj…

1307033

…ects (flutter#11798) Manage resource and onscreen contexts using separate IOSGLContext objects

gw280 mentioned this pull request Sep 9, 2019

Manage iOS contexts separately #12078

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 9, 2019

a353f93 Manage resource and onscreen contexts using separate IOSGLCon…

4d5bd16

…text objects (flutter/engine#11798)

engine-flutter-autoroll mentioned this pull request Sep 9, 2019

Roll engine c9ea4dba8da6..a4de006f2139 (17 commits) flutter/flutter#40113

Closed

gw280 added a commit to gw280/engine that referenced this pull request Sep 11, 2019

Manage resource and onscreen contexts using separate IOSGLContext obj…

7c75bcf

…ects (flutter#11798) Manage resource and onscreen contexts using separate IOSGLContext objects

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 11, 2019

a353f93 Manage resource and onscreen contexts using separate IOSGLCon…

b763078

…text objects (flutter/engine#11798)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 11, 2019

a353f93 Manage resource and onscreen contexts using separate IOSGLCon…

193f3ba

…text objects (flutter/engine#11798)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 11, 2019

a353f93 Manage resource and onscreen contexts using separate IOSGLCon…

41073c0

…text objects (flutter/engine#11798)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 11, 2019

a353f93 Manage resource and onscreen contexts using separate IOSGLCon…

631ff48

…text objects (flutter/engine#11798)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 12, 2019

a353f93 Manage resource and onscreen contexts using separate IOSGLCon…

6821298

…text objects (flutter/engine#11798)

Manage resource and onscreen contexts using separate IOSGLContext objects #11798

Manage resource and onscreen contexts using separate IOSGLContext objects #11798

Uh oh!

Conversation

gw280 commented Aug 30, 2019

Uh oh!

xster Sep 3, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gw280 commented Sep 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chinmaygarde left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chinmaygarde Sep 4, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

dnfield Sep 5, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chinmaygarde left a comment

Choose a reason for hiding this comment

Uh oh!

chinmaygarde Sep 5, 2019

Choose a reason for hiding this comment

Uh oh!

gw280 Sep 5, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

chinmaygarde commented Sep 7, 2019

Uh oh!

xster commented Sep 7, 2019

Uh oh!

iskakaushik commented Sep 7, 2019

Uh oh!

gw280 commented Sep 7, 2019 via email

Uh oh!

chinmaygarde commented Sep 7, 2019

Uh oh!

dnfield commented Sep 7, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

gw280 commented Sep 3, 2019 •

edited

Loading