implemented GetMainContext() for opengl #23634

gaaclarke · 2021-01-12T22:04:15Z

Description

For Metal, the IOSContext owns the skia context, for OpenGL it was the GPUSurface that owned the context. I've made the IOSContextGL behave like the IOSContextMetal which allowed us to percolate up a shared interface GetMainContext(). This effectively solves the sharing GPU context problem for OpenGL as well.

Related Issues

fixes flutter/flutter#72021
#23539

Tests

I added the following tests:

No new functionality other than changing who has ownership of the skia context. Existing tests should cover it.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the contributor guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the C++, Objective-C, Java style guides for the engine.
I read the tree hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation.
All existing and new tests are passing.
I am willing to follow-up on review comments in a timely manner.

Reviewer Checklist

I have submitted any presubmit flakes in this PR using the engine presubmit flakes form before re-triggering the failure.

Breaking Change

Did any tests fail when you ran them? Please read handling breaking changes.

No, no existing tests failed, so this is not a breaking change.
Yes, this is a breaking change. If not, delete the remainder of this section.
- I wrote a design doc: https://flutter.dev/go/template Replace this with a link to your design doc's short link
- I got input from the developer relations team, specifically from: Replace with the names of who gave advice
- I wrote a migration guide: https://flutter.dev/go/breaking-changes-template Replace this with a link to a pull request that adds the migration guide to https://flutter.dev/docs/release/breaking-changes

flutter-dashboard · 2021-01-12T23:43:39Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

gaaclarke · 2021-01-15T21:33:46Z

@xster @iskakaushik friendly ping

xster

Doh sorry missed this. LGTM. Could any of this be tested?

gaaclarke · 2021-01-15T21:46:58Z

The new functionality would be covered under an existing test but since we don't run that test on OpenGL with the simulator it won't get exercised.

As for the refactoring of the setup for OpenGL there are golden image tests that run on opengl devices as far as I know.

I'll give unit tests directly on the opengl classes another look later today.

cleaned up and that the class behaves consistently

xster · 2021-01-20T13:49:20Z

shell/gpu/gpu_surface_gl.h

@@ -20,6 +20,8 @@ namespace flutter {

 class GPUSurfaceGL : public Surface {
 public:
+  static sk_sp<GrDirectContext> MakeGLContext(GPUSurfaceGLDelegate* delegate);


If possible, can we add some consistency between the metal version and this one? In metal, the IOSContextMetal just decides to make a GrDirectContext the way it wants without asking anyone. Here, it seems like the IOSSurfaceGL asks the GPUSurfaceGL to make one, then pushes it into IOSContextGL which then owns it.

xster · 2021-01-20T13:51:31Z

shell/platform/darwin/ios/ios_context_gl.mm

-IOSContextGL::~IOSContextGL() = default;
+IOSContextGL::~IOSContextGL() {
+  if (main_context_) {
+    main_context_->releaseResourcesAndAbandonContext();


I don't know if it's possible. The various GPUSurfaceGL still has this shared pointer right? Could there be any risks?

xster · 2021-01-20T14:01:12Z

shell/platform/darwin/ios/ios_surface_gl.mm

+    sk_sp<GrDirectContext> context = gl_context->GetMainContext();
+    if (!context) {
+      context = GPUSurfaceGL::MakeGLContext(this);
+      gl_context->SetMainContext(context);


Ditto here. Seems like we have 2 timing characteristics. The metal case just makes this GrDirectContext way earlier when creating the shell's on_create_platform_view, whereas this makes it way later in FlutterViewController's viewWillAppear. There could be minutes in between if the engine was prewarmed.

I'm a bit worried that there might be side-effects involving platform views or memory etc that are hard to reproduce due to the rendering API differences.

It doesn't have to be in this PR, but I suppose this way works better memory wise. Can we make metal work this way too?

Though on the flip side, this likely shifts more of the latency away from the engine prewarming into the critical path for showing the first frame.

This reverts commit 296902b.

* 5b2defd Roll Fuchsia Mac SDK from MrtHftV0U... to 7nZajqutF... (flutter/engine#23827) * c8ffb20 Roll Skia from 87a055b02027 to e0d023562bd9 (1 revision) (flutter/engine#23829) * 296902b implemented GetMainContext() for opengl (flutter/engine#23634) * 5cf3eae Roll Skia from e0d023562bd9 to 982127b7d57d (4 revisions) (flutter/engine#23831) * fd9a079 [iOS] Fixes DisplayLinkManager leaks (flutter/engine#22194) * b241537 Roll Fuchsia Linux Toolchain from git_revision:2c0536b76b35fa592ac7b4a0e4bb176eaf55af75 to IJxh_9dNS... (flutter/engine#23832) * 5c2003f Roll Skia from 982127b7d57d to 6de1e52d0b12 (1 revision) (flutter/engine#23834) * 5b9cd44 Automatically download Noto fonts as backup fonts in CanvasKit mode (flutter/engine#23728) * 70a6824 Roll Dart SDK from 5e24a66b1bb8 to 704928da5702 (2 revisions) (flutter/engine#23838) * b0c46d8 Roll Skia from 6de1e52d0b12 to 8a37fb2c605b (5 revisions) (flutter/engine#23836) * d4132ea Use references when iterating over SkParagraph text boxes (flutter/engine#23837) * 87960d8 Fix typo in embedder unit tests (flutter/engine#23783) * 7f66714 iOS deeplink sends "path + query" instead of just path (flutter/engine#23562) * 1474d08 Roll Skia from 8a37fb2c605b to 37d16f135265 (4 revisions) (flutter/engine#23839) * 3da13fc Make android more lenient when it comes to out-of-order key event responses (flutter/engine#23604) * 9223073 Fix background crash when FlutterView going appear while app goes background (flutter/engine#23175) * 7c19824 Pass the filename directly to JNI for loading deferred component. (flutter/engine#23824) * 5dc2469 Reland path vol tracker (flutter/engine#23840) * e7e76f1 Roll Skia from 37d16f135265 to e89d8ea20b62 (2 revisions) (flutter/engine#23841) * 07f4861 Roll Dart SDK from 704928da5702 to 1db2d4d95562 (1 revision) (flutter/engine#23846) * 993ab78 Roll Skia from e89d8ea20b62 to c09761f57605 (1 revision) (flutter/engine#23843) * a4836a6 Call Dart plugin registrant if available (flutter/engine#23813) * 475a234 Roll Fuchsia Linux SDK from UGavhI1zv... to mODEe2CNk... (flutter/engine#23848) * b51da31 Roll Skia from c09761f57605 to 450f8565c7f3 (5 revisions) (flutter/engine#23851) * cb7106d Roll Skia from 450f8565c7f3 to 372791761157 (1 revision) (flutter/engine#23855) * 69980e5 Roll Fuchsia Mac SDK from 7nZajqutF... to tuJCioUf3... (flutter/engine#23857) * 20ff574 Roll Skia from 372791761157 to ce75036b3eaf (4 revisions) (flutter/engine#23858) * 0118b54 Implements accessibility bridge in common library (flutter/engine#23491) * ffc77f0 Search multiple paths when loading deferred component .so files. (flutter/engine#23849) * 71d264d Revert "implemented GetMainContext() for opengl (#23634)" (flutter/engine#23859) * fb48735 Roll Skia from ce75036b3eaf to cc6961b9ac5e (3 revisions) (flutter/engine#23860) * fdddf87 Roll Dart SDK from 1db2d4d95562 to 82b4c77fb17f (2 revisions) (flutter/engine#23861)

* 004d8ad Roll Skia from cc6961b9ac5e to 6a272434c2b2 (3 revisions) (flutter/engine#23864) * 7c2da3b reland of flutter/engine#23634 (flutter/engine#23865) * a4f02b7 Share Android surface GrDirectContext (flutter/engine#23798) * 31ef1dd Roll Skia from 6a272434c2b2 to bfc9be0f773f (2 revisions) (flutter/engine#23867) * 6dec57f Remove workarounds now that type promotion accounts for local boolean variables. (flutter/engine#23862) * a787229 Roll Skia from bfc9be0f773f to 3193ff271628 (5 revisions) (flutter/engine#23872) * a242dd8 [web] Fix shadows for arbitrary paths on PhysicalShape (flutter/engine#23830) * 05b4bec Started using Dart_CreateInGroup when using spawn on a release build (flutter/engine#23782)

…tter#23859) This reverts commit 296902b.

flutter-dashboard bot added the platform-ios label Jan 12, 2021

googlebot added the cla: yes label Jan 12, 2021

gaaclarke force-pushed the lfe-share-context-opengl branch 3 times, most recently from 0a97981 to 1dbb704 Compare January 12, 2021 22:49

gaaclarke marked this pull request as ready for review January 12, 2021 23:43

gaaclarke requested review from xster and iskakaushik January 12, 2021 23:43

xster reviewed Jan 15, 2021

View reviewed changes

iskakaushik approved these changes Jan 15, 2021

View reviewed changes

gaaclarke force-pushed the lfe-share-context-opengl branch from 50dcf08 to 3cad490 Compare January 19, 2021 21:36

gaaclarke added 2 commits January 19, 2021 14:56

implemented GetMainContext() for opengl

3d63f6b

Added initializers for instance variables to make sure contexts are

079b09e

cleaned up and that the class behaves consistently

gaaclarke force-pushed the lfe-share-context-opengl branch from 3cad490 to 079b09e Compare January 19, 2021 22:56

xster reviewed Jan 20, 2021

View reviewed changes

xster mentioned this pull request Jan 20, 2021

Share Android surface GrDirectContext #23798

Merged

9 tasks

gaaclarke merged commit 296902b into flutter:master Jan 21, 2021

engine-flutter-autoroll mentioned this pull request Jan 22, 2021

Roll Engine from 90641fa92385 to 92230737069a (16 revisions) flutter/flutter#74457

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 22, 2021

296902b implemented GetMainContext() for opengl (flutter/engine#23634)

c34ee87

engine-flutter-autoroll mentioned this pull request Jan 22, 2021

Roll Engine from 90641fa92385 to 7c19824c6d08 (17 revisions) flutter/flutter#74461

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 22, 2021

296902b implemented GetMainContext() for opengl (flutter/engine#23634)

25e5c63

engine-flutter-autoroll mentioned this pull request Jan 22, 2021

Roll Engine from 90641fa92385 to 993ab786eeff (21 revisions) flutter/flutter#74474

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 22, 2021

296902b implemented GetMainContext() for opengl (flutter/engine#23634)

3769a1b

engine-flutter-autoroll mentioned this pull request Jan 22, 2021

Roll Engine from 90641fa92385 to 475a2349d53e (23 revisions) flutter/flutter#74478

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 22, 2021

296902b implemented GetMainContext() for opengl (flutter/engine#23634)

d56c895

engine-flutter-autoroll mentioned this pull request Jan 22, 2021

Roll Engine from 90641fa92385 to b51da31eb93c (24 revisions) flutter/flutter#74487

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 22, 2021

296902b implemented GetMainContext() for opengl (flutter/engine#23634)

37925f3

engine-flutter-autoroll mentioned this pull request Jan 22, 2021

Roll Engine from 90641fa92385 to cb7106dd7c39 (25 revisions) flutter/flutter#74501

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 22, 2021

296902b implemented GetMainContext() for opengl (flutter/engine#23634)

b3d5e84

zanderso mentioned this pull request Jan 22, 2021

firebase_release_smoke_test crashing consistently on Engine roll attempts flutter/flutter#74502

Closed

zanderso added a commit that referenced this pull request Jan 22, 2021

Revert "implemented GetMainContext() for opengl (#23634)"

22665ea

This reverts commit 296902b.

zanderso mentioned this pull request Jan 22, 2021

Revert "implemented GetMainContext() for opengl" #23859

Merged

engine-flutter-autoroll mentioned this pull request Jan 22, 2021

Roll Engine from 90641fa92385 to 69980e5b8342 (26 revisions) flutter/flutter#74506

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 22, 2021

296902b implemented GetMainContext() for opengl (flutter/engine#23634)

ea1dd01

zanderso added a commit that referenced this pull request Jan 22, 2021

Revert "implemented GetMainContext() for opengl (#23634)" (#23859)

71d264d

This reverts commit 296902b.

engine-flutter-autoroll mentioned this pull request Jan 22, 2021

Roll Engine from 90641fa92385 to 20ff574d871d (27 revisions) flutter/flutter#74510

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 22, 2021

296902b implemented GetMainContext() for opengl (flutter/engine#23634)

a502315

gaaclarke added a commit to gaaclarke/engine that referenced this pull request Jan 22, 2021

reland of flutter#23634

1a01cee

gaaclarke mentioned this pull request Jan 22, 2021

implemented GetMainContext() for opengl - reland #23865

Merged

9 tasks

engine-flutter-autoroll mentioned this pull request Jan 22, 2021

Roll Engine from 90641fa92385 to ffc77f0da35b (29 revisions) flutter/flutter#74515

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 22, 2021

296902b implemented GetMainContext() for opengl (flutter/engine#23634)

510c8ce

gaaclarke added a commit that referenced this pull request Jan 22, 2021

reland of #23634 (#23865)

7c2da3b

engine-flutter-autoroll mentioned this pull request Jan 22, 2021

Roll Engine from 90641fa92385 to fdddf8708039 (32 revisions) flutter/flutter#74517

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 22, 2021

296902b implemented GetMainContext() for opengl (flutter/engine#23634)

10c819d

engine-flutter-autoroll mentioned this pull request Jan 23, 2021

Roll Engine from fdddf8708039 to 05b4bec8b3c1 (8 revisions) flutter/flutter#74540

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 23, 2021

7c2da3b reland of flutter/engine#23634 (flutter/engine#23865)

5cf0551

hjfreyer pushed a commit to hjfreyer/engine that referenced this pull request Mar 22, 2021

implemented GetMainContext() for opengl (flutter#23634)

894eaaa

hjfreyer pushed a commit to hjfreyer/engine that referenced this pull request Mar 22, 2021

Revert "implemented GetMainContext() for opengl (flutter#23634)" (flu…

72db80e

…tter#23859) This reverts commit 296902b.

hjfreyer pushed a commit to hjfreyer/engine that referenced this pull request Mar 22, 2021

reland of flutter#23634 (flutter#23865)

a92998d

linroid mentioned this pull request Sep 8, 2021

[WIP]Fix OpenGL leak on Android and iOS #28506

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

implemented GetMainContext() for opengl #23634

implemented GetMainContext() for opengl #23634

Uh oh!

gaaclarke commented Jan 12, 2021

Uh oh!

flutter-dashboard bot commented Jan 12, 2021

Uh oh!

gaaclarke commented Jan 15, 2021

Uh oh!

xster left a comment

Uh oh!

gaaclarke commented Jan 15, 2021

Uh oh!

xster Jan 20, 2021

Uh oh!

xster Jan 20, 2021

Uh oh!

xster Jan 20, 2021 •

edited

Loading

Uh oh!

Uh oh!

implemented GetMainContext() for opengl #23634

implemented GetMainContext() for opengl #23634

Uh oh!

Conversation

gaaclarke commented Jan 12, 2021

Description

Related Issues

Tests

Checklist

Reviewer Checklist

Breaking Change

Uh oh!

flutter-dashboard bot commented Jan 12, 2021

Uh oh!

gaaclarke commented Jan 15, 2021

Uh oh!

xster left a comment

Choose a reason for hiding this comment

Uh oh!

gaaclarke commented Jan 15, 2021

Uh oh!

xster Jan 20, 2021

Choose a reason for hiding this comment

Uh oh!

xster Jan 20, 2021

Choose a reason for hiding this comment

Uh oh!

xster Jan 20, 2021 • 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!

Uh oh!

xster Jan 20, 2021 •

edited

Loading