[macos] Re-land FlutterOpenGLRenderer isolation #22607

iskakaushik · 2020-11-19T17:58:36Z

Isolate macOS OpenGL renderer config generation to FlutterOpenGLRenderer class.

This reverts commit be7f80e.

flutter-dashboard · 2020-11-19T17:58:38Z

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.

iskakaushik · 2020-11-19T18:00:52Z

@stuartmorgan I addressed your feedback in the earlier commit. I also made it a non-breaking change.

stuartmorgan-g

Now that we have the ability to write proc-table-based unit tests, is there a reason this change can't be tested?

stuartmorgan-g · 2020-11-19T18:45:46Z

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

+    [_openGLRenderer clearResourceContext];
    [self shutDownEngine];
-    _resourceContext = nil;
  }


There is a codepath here (nil controller, _allowHeadlessExecution true) wher you aren't updating the renderer.

Fixed it to clear the resource context when we don't have a view controller.

stuartmorgan-g · 2020-11-19T18:50:22Z

shell/platform/darwin/macos/framework/Source/FlutterOpenGLRenderer.mm

+
+  FlutterView* _flutterView;
+
+  // The context that is owned by the currently displayed FlutterView. This is stashed


Is this comment still accurate? This class is creating the context.

This was valid during an earlier iteration, fixed it now.

stuartmorgan-g · 2020-11-19T18:52:01Z

shell/platform/darwin/macos/framework/Source/FlutterOpenGLRenderer.mm

+  FlutterExternalTextureGL* FlutterTexture =
+      [[FlutterExternalTextureGL alloc] initWithFlutterTexture:texture];
+  int64_t textureID = [FlutterTexture textureID];
+  auto success = _embedderAPI.RegisterExternalTexture(_engine, textureID);


Please use explicit types rather than auto everywhere for embedder API results; the type of the return value is not obvious from just the code on the RHS, which is the suggested use case for auto in the style guide.

iskakaushik · 2020-11-19T19:23:01Z

Fixed the review comments modulo adding a test. I didn't realize that have proc-table based tests. Will add some for this.

shell/platform/darwin/macos/framework/Source/FlutterExternalTextureGL.mm

iskakaushik · 2020-11-19T23:22:31Z

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

 @implementation FlutterEngineRegistrar {
  NSString* _pluginKey;
  FlutterEngine* _flutterEngine;
-  FlutterEngineProcTable _embedderAPI;


This wasn't used.

iskakaushik · 2020-11-19T23:23:00Z

I've added a tests in FlutterEngineTests.mm.

stuartmorgan-g · 2020-11-23T19:46:34Z

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

+        return kSuccess;
+      });
+
+  [engine.openGLRenderer registerTexture:flutterTexture];


These tests are all testing the OpenGL renderer, so should be in a new test file for that class rather than going through the engine.

stuartmorgan-g · 2020-11-23T19:50:34Z

shell/platform/darwin/macos/framework/Source/FlutterOpenGLRenderer.mm

+      [[FlutterExternalTextureGL alloc] initWithFlutterTexture:texture];
+  int64_t textureID = [FlutterTexture textureID];
+  FlutterEngineResult success =
+      _flutterEngine.embedderAPI.RegisterExternalTexture(_engine, textureID);


These calls should be wrapped as private FlutterEngine methods, rather than having other classes calling into embedderAPI directly.

stuartmorgan-g · 2020-11-23T19:52:01Z

shell/platform/darwin/macos/framework/Source/FlutterOpenGLRenderer.mm

+  }
+}
+
+#pragma mark - Helper methods to create rendering config for embedder.


This kind of comment belongs on the specific declaration; if anyone adds another private method they are unlikely to notice and update this, at which point it will be actively misleading. You can just use #pragma mark - Private methods

stuartmorgan-g

Code mostly looks good; some nits below. On the testing front, why is so little of the new renderer class tested? It seems like with some simple ObjC mocks you could test many of the core methods. E.g., that fboForFrameInfo: works as expected depending on whether or not attachToFlutterView: has beend called.

stuartmorgan-g · 2020-12-08T12:56:34Z

shell/platform/darwin/macos/framework/Source/FlutterEngine_Internal.h

+/**
+ * Registers an external texture with the given id. Returns true on success.
+ */
+- (bool)embedderRegisterTextureWithId:(int64_t)textureId;


Nit for all three: use BOOL for ObjC interfaces unless there's a compelling reason not to. (Comments should then be updated to s/true/YES/)

stuartmorgan-g · 2020-12-08T12:59:07Z

shell/platform/darwin/macos/framework/Source/FlutterEngine_Internal.h

+/**
+ * Registers an external texture with the given id. Returns true on success.
+ */
+- (bool)embedderRegisterTextureWithId:(int64_t)textureId;


Starting the names with embedder is unusual ObjC naming (other than for cases like protocols), and all of the methods in this header are for talking to the embedder API so it's odd to prefix some of them. I would just call them registerTextureWithID:, etc.

stuartmorgan-g · 2020-12-08T12:59:35Z

shell/platform/darwin/macos/framework/Source/FlutterEngine_Internal.h

+/**
+ * Registers an external texture with the given id. Returns true on success.
+ */
+- (bool)embedderRegisterTextureWithId:(int64_t)textureId;


s/Id/ID/ in both method name and param name. ObjC capitalizes acronyms differently than C++ style. (Or use Identifier; either is fine.)

stuartmorgan-g · 2020-12-08T13:03:11Z

shell/platform/darwin/macos/framework/Source/FlutterOpenGLRenderer.h

+#import "flutter/shell/platform/darwin/macos/framework/Source/FlutterView.h"
+#import "flutter/shell/platform/embedder/embedder.h"
+
+NS_ASSUME_NONNULL_BEGIN


I despise this macro; it makes important information non-local when people are reading code--especially when reviewing and context is collapsed by default--and have never used it in the macOS code. Please use explicit annotations.

stuartmorgan-g · 2020-12-08T13:07:05Z

shell/platform/darwin/macos/framework/Source/FlutterOpenGLRenderer.mm

+
+@implementation FlutterOpenGLRenderer {
+  // The embedding-API-level engine object.
+  FLUTTER_API_SYMBOL(FlutterEngine) _engine;


You should be able to remove this now, including from the constructor making it just initWithEngine: (and thus also remove the embedder.h include in the header, I believe.)

Removed this, but not embedder.h from the header since there is a helper method in this class that returuns FlutterRendererConfig.

stuartmorgan-g · 2020-12-08T13:10:06Z

shell/platform/darwin/macos/framework/Source/FlutterOpenGLRenderer.h

+/**
+ * Called by the engine to make the context the engine should draw into current.
+ */
+- (bool)makeCurrent;


These can all be BOOLs as well, and the implementations updated to YES/NO. (I should really have done that before when the statics were updated to call directly to ObjC versions; this is a good time to fix it.)

stuartmorgan-g · 2020-12-08T13:11:10Z

shell/platform/darwin/macos/framework/Source/FlutterOpenGLRendererTest.mm

+namespace flutter::testing {
+
+namespace {
+// Returns an engine configured for the text fixture resource configuration.


Typo: test->text

iskakaushik · 2020-12-09T18:35:56Z

@stuartmorgan I've added more tests for the renderer. PTAL

chinmaygarde · 2020-12-10T22:52:23Z

Stuart is OOO till Monday.

iskakaushik · 2020-12-30T18:29:02Z

@stuartmorgan friendly ping on this

stuartmorgan-g · 2021-01-05T16:29:25Z

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

-      .open_gl.make_resource_current = (BoolCallback)OnMakeResourceCurrent,
-      .open_gl.gl_external_texture_frame_callback = (TextureFrameCallback)OnAcquireExternalTexture,
-  };
+  [_openGLRenderer attachToFlutterView:_viewController.flutterView];


attachToFlutterView:'s parameter is marked non-null, but there's no guarantee that there's a _viewController. You need the same logic you have in setViewController: here as well. Alternately, you could make this kind of mistake impossible and remove duplicated code by having attachToFlutterView: allow nil (maybe changing its name to setFlutterView:), and remove clearResourceContext.

stuartmorgan-g · 2021-01-05T16:33:44Z

shell/platform/darwin/macos/framework/Source/FlutterOpenGLRenderer.mm

+
+- (BOOL)makeResourceCurrent {
+  [self.resourceContext makeCurrentContext];
+  return true;


s/true/YES/

stuartmorgan-g · 2021-01-05T16:34:15Z

shell/platform/darwin/macos/framework/Source/FlutterOpenGLRenderer.mm

+}
+
+- (void)textureFrameAvailable:(int64_t)textureID {
+  bool success = [_flutterEngine markTextureFrameAvailable:textureID];


s/bool/BOOL/

stuartmorgan-g · 2021-01-05T16:34:23Z

shell/platform/darwin/macos/framework/Source/FlutterOpenGLRenderer.mm

+}
+
+- (void)unregisterTexture:(int64_t)textureID {
+  bool success = [_flutterEngine unregisterTextureWithID:textureID];


s/bool/BOOL/

This reverts commit be7f80e.

stuartmorgan-g

Changes LGTM

…ne#22607)

google-cla bot added the cla: yes label Nov 19, 2020

iskakaushik requested a review from stuartmorgan-g November 19, 2020 17:58

iskakaushik force-pushed the reland_gl_isolation branch from 767967a to d677997 Compare November 19, 2020 18:09

stuartmorgan-g suggested changes Nov 19, 2020

View reviewed changes

iskakaushik force-pushed the reland_gl_isolation branch from d9d893e to 07e866e Compare November 19, 2020 23:20

iskakaushik commented Nov 19, 2020

View reviewed changes

shell/platform/darwin/macos/framework/Source/FlutterExternalTextureGL.mm Show resolved Hide resolved

iskakaushik commented Nov 19, 2020

View reviewed changes

stuartmorgan-g suggested changes Nov 23, 2020

View reviewed changes

iskakaushik requested a review from cbracken December 2, 2020 17:16

iskakaushik force-pushed the reland_gl_isolation branch from 0daf9a2 to 64183a6 Compare December 7, 2020 18:38

stuartmorgan-g suggested changes Dec 8, 2020

View reviewed changes

stuartmorgan-g suggested changes Jan 5, 2021

View reviewed changes

iskakaushik added 8 commits January 5, 2021 09:03

[macos] Reland FlutterOpenGLRenderer isolation

e9cf7b6

This reverts commit be7f80e.

CR fixes modulo adding a test.

31bd3f9

add tests for external textures

39e0bc0

fix cr comments

5c6dc11

fix merge confligts

f265eb3

fix return types

81c3e06

Add more tests for renderer

07f6410

unify setFlutterView

a7f1609

iskakaushik force-pushed the reland_gl_isolation branch from 68ca69d to a7f1609 Compare January 5, 2021 17:48

iskakaushik requested a review from stuartmorgan-g January 6, 2021 23:19

stuartmorgan-g approved these changes Jan 7, 2021

View reviewed changes

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

be8839f [macos] Re-land FlutterOpenGLRenderer isolation (flutter/engi…

5d82517

…ne#22607)

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

Roll Engine from 2e5833b7c572 to 73363dd25826 (13 revisions) flutter/flutter#73519

Closed

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

be8839f [macos] Re-land FlutterOpenGLRenderer isolation (flutter/engi…

5c03b9c

…ne#22607)

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

Roll Engine from 2e5833b7c572 to 3a1b62b36276 (14 revisions) flutter/flutter#73523

Closed

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

be8839f [macos] Re-land FlutterOpenGLRenderer isolation (flutter/engi…

6bfa610

…ne#22607)

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

Roll Engine from 2e5833b7c572 to 3a1b62b36276 (14 revisions) flutter/flutter#73524

Closed

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

be8839f [macos] Re-land FlutterOpenGLRenderer isolation (flutter/engi…

3e6ac68

…ne#22607)

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

Roll Engine from 2e5833b7c572 to 5f7a12097fc7 (18 revisions) flutter/flutter#73530

Closed

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

be8839f [macos] Re-land FlutterOpenGLRenderer isolation (flutter/engi…

b59bcd7

…ne#22607)

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

Roll Engine from 2e5833b7c572 to 72dddb47b44e (19 revisions) flutter/flutter#73537

Closed

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

be8839f [macos] Re-land FlutterOpenGLRenderer isolation (flutter/engi…

b89abae

…ne#22607)

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

Roll Engine from 2e5833b7c572 to c6a8bd257a13 (20 revisions) flutter/flutter#73539

Closed

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

be8839f [macos] Re-land FlutterOpenGLRenderer isolation (flutter/engi…

1ef2d6e

…ne#22607)

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

Roll Engine from 2e5833b7c572 to 1eb28331ba6d (21 revisions) flutter/flutter#73540

Closed

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

be8839f [macos] Re-land FlutterOpenGLRenderer isolation (flutter/engi…

24cb800

…ne#22607)

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

Roll Engine from 2e5833b7c572 to 5f85389c24a1 (24 revisions) flutter/flutter#73542

Closed

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

be8839f [macos] Re-land FlutterOpenGLRenderer isolation (flutter/engi…

0b26371

…ne#22607)

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

Roll Engine from 2e5833b7c572 to 96f3c3cf26f3 (26 revisions) flutter/flutter#73546

Closed

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

be8839f [macos] Re-land FlutterOpenGLRenderer isolation (flutter/engi…

2623934

…ne#22607)

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

Roll Engine from 2e5833b7c572 to 5381ab8c8d9e (27 revisions) flutter/flutter#73564

Closed

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

be8839f [macos] Re-land FlutterOpenGLRenderer isolation (flutter/engi…

16b7a9b

…ne#22607)

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

Roll Engine from 2e5833b7c572 to f07fb869907c (28 revisions) flutter/flutter#73569

Closed

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

be8839f [macos] Re-land FlutterOpenGLRenderer isolation (flutter/engi…

cceaaa9

…ne#22607)

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

Roll Engine from 2e5833b7c572 to cc813b9afb71 (29 revisions) flutter/flutter#73572

Closed

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

be8839f [macos] Re-land FlutterOpenGLRenderer isolation (flutter/engi…

8b55a4f

…ne#22607)

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

Roll Engine from 2e5833b7c572 to facd18f85441 (30 revisions) flutter/flutter#73584

Closed

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

be8839f [macos] Re-land FlutterOpenGLRenderer isolation (flutter/engi…

f04a004

…ne#22607)

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

Roll Engine from 2e5833b7c572 to 9cb4d2dd243d (32 revisions) flutter/flutter#73594

Merged

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

be8839f [macos] Re-land FlutterOpenGLRenderer isolation (flutter/engi…

f01f641

…ne#22607)

jmagman mentioned this pull request Jan 19, 2021

hot_mode_dev_cycle_macos_target__benchmark timing out when user not logged in flutter/flutter#74201

Closed


		FlutterView* _flutterView;

		// The context that is owned by the currently displayed FlutterView. This is stashed

[macos] Re-land FlutterOpenGLRenderer isolation #22607

[macos] Re-land FlutterOpenGLRenderer isolation #22607

Uh oh!

Conversation

iskakaushik commented Nov 19, 2020

Uh oh!

flutter-dashboard bot commented Nov 19, 2020

Uh oh!

iskakaushik commented Nov 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stuartmorgan-g left a comment

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!

iskakaushik commented Nov 19, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iskakaushik commented Nov 19, 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!

stuartmorgan-g left a comment

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!

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!

iskakaushik commented Dec 9, 2020

Uh oh!

chinmaygarde commented Dec 10, 2020

Uh oh!

iskakaushik commented Dec 30, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iskakaushik commented Nov 19, 2020 •

edited

Loading