Add onUnregistered callback in 'Texture' and 'FlutterTexture' #12695

cyanglaz · 2019-09-30T15:07:37Z

Issue explained in detail here: flutter/flutter#41125
As it for now, when a FlutterTextureRegistry calls unregisterTexture on a non-GPU thread, the FlutterTexture doesn't know when the unregistering ends because the task is posted to the GPU thread.
Plugins like video player needs to know when the texture is unregistered in order to safely destruct the AVPlayer object.

Adding this callback provides a way for FlutterTexture to know when it is unregistered.

A WIP PR in video_player plugin demonstrates how it is used flutter/plugins#2124

Partial fix for flutter/flutter#41125

This reverts commit 7418e7e.

chinmaygarde

Some nits but LGTM otherwise. Now that we have an OpenGL capable test harness, can we also a test that asserts the following:

Platform registers a texture.
Platform says a texture frame is available.
ASSERT: The copy-texture call is made by the engine back to the embedder on the GPU thread.
Platform releases the texture.
ASSERT: This new unregistered call is made back to the embedder.

The shell or embedder unittests might be the right test harness for this as it asserts how the platform interacts with this mechanism. Just a suggestion and I'd understand if you want to attempt that in a separate patch.

chinmaygarde · 2019-09-30T19:05:00Z

flow/texture.h

  virtual void MarkNewFrameAvailable() = 0;

+  // Called on GPU thread.
+  virtual void OnUnregistered() = 0;


OnTextureUnregistered instead. Since a subclass is going to override this, OnUnregistered will read ambiguously in a class that has other non-texture handling responsibilities.

chinmaygarde · 2019-09-30T19:07:19Z

shell/platform/darwin/ios/framework/Headers/FlutterTexture.h

+ * Called on the GPU thread.
+ */
+@optional
+- (void)onUnregistered;


Ditto about the onUnregistered begin too ambiguous. In Objective-C the idiom is to also pass the object as an argument. So something like:

-(void) onTextureUnregistered:(FlutterTexture*) texture;

chinmaygarde · 2019-09-30T19:08:28Z

flow/texture_unittests.cc

+
+  ~MockTexture() override = default;
+
+  bool unregistered = false;


Per naming convention, this needs to be private and have an underscore suffix. You can add a getter to check the assertion.

chinmaygarde · 2019-09-30T19:09:59Z

flow/texture_unittests.cc

+             bool freeze,
+             GrContext* context) override {}
+
+  void OnGrContextCreated() override {}


Per convention, we note the class that declares the prototype for the method being overridden. So // |flutter::testing::Texture|.

cyanglaz · 2019-10-07T22:19:07Z

@chinmaygarde Added the shell test per your suggestion. The copy-texture call exists only on the embedder, so I didn't add it to the shell_unittest.
If you are ok with the new tests, please LGTM it. I'll wait for you to approve it before merging the PR.

…e' (flutter/engine#12695)

[email protected]:flutter/engine.git/compare/c635d70c7266...21b8224 git log c635d70..21b8224 --no-merges --oneline 2019-10-08 [email protected] Send AccessibilityEvent.TYPE_VIEW_FOCUSED when input focus is set. (flutter/engine#12746) 2019-10-08 [email protected] Fix for a11y crash on iOS (flutter/engine#12990) 2019-10-08 [email protected] Link Semantics Typo (flutter/engine#13009) 2019-10-08 [email protected] [web] Add support for path transform (flutter/engine#12794) 2019-10-08 [email protected] Auto-formatter fixes for BUILD.gn files (flutter/engine#13005) 2019-10-08 [email protected] Unblock SIGPROF on flutter_tester start (flutter/engine#12813) 2019-10-08 [email protected] [web] Update the url when route is replaced (flutter/engine#13003) 2019-10-08 [email protected] Missing link flag (flutter/engine#13001) 2019-10-08 [email protected] Started setting our debug background task id to invalid after completion. (flutter/engine#12999) 2019-10-08 [email protected] Add `onUnregistered` callback in 'Texture' and 'FlutterTexture' (flutter/engine#12695) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected] on the revert to ensure that a human is aware of the problem. To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

Chris Yang added 3 commits August 27, 2019 15:30

wait for platform view appear

7418e7e

Merge branch 'master' of github.com:flutter/engine

ab43f55

prototype adding unregistered callback

6236792

googlebot added the cla: yes label Sep 30, 2019

cyanglaz mentioned this pull request Sep 30, 2019

video_player plugin can cause a crash on iPhone X running iOS 12 when disposing the video controller flutter/flutter#41125

Closed

Chris Yang added 5 commits September 30, 2019 08:32

remove import

9de6e23

add documentation

ef340fb

Merge branch 'master' of github.com:flutter/engine

e360c9d

Merge branch 'master' into texture_race

91d023a

Revert "wait for platform view appear"

2417658

This reverts commit 7418e7e.

cyanglaz marked this pull request as ready for review September 30, 2019 16:44

lisence golden fix, make onUnregistered optional

1c2fa91

cyanglaz mentioned this pull request Sep 30, 2019

[video_player] Move [player dispose] to onUnregistered flutter/plugins#2124

Merged

13 tasks

cyanglaz requested a review from chinmaygarde September 30, 2019 16:58

chinmaygarde approved these changes Sep 30, 2019

View reviewed changes

Chris Yang added 6 commits September 30, 2019 13:48

rename method

2ab9edd

Merge branch 'master' of github.com:flutter/engine into texture_race

fceb313

review fixes

1e0a9c5

change texture test to mm

11e3847

add texture testing in shell_unittest

01d6d09

formatting

04059b3

chinmaygarde approved these changes Oct 7, 2019

View reviewed changes

cyanglaz merged commit 0d59c9e into flutter:master Oct 8, 2019

cyanglaz deleted the texture_race branch October 8, 2019 19:46

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

0d59c9e Add onUnregistered callback in 'Texture' and 'FlutterTextur…

729858d

…e' (flutter/engine#12695)

engine-flutter-autoroll mentioned this pull request Oct 8, 2019

Roll engine c635d70c7266..414ad3866251 (3 commits) flutter/flutter#42268

Closed

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

0d59c9e Add onUnregistered callback in 'Texture' and 'FlutterTextur…

4b545a9

…e' (flutter/engine#12695)

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

Roll engine c635d70c7266..21b8224ae12d (10 commits) flutter/flutter#42285

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add onUnregistered callback in 'Texture' and 'FlutterTexture' #12695

Add onUnregistered callback in 'Texture' and 'FlutterTexture' #12695

Uh oh!

cyanglaz commented Sep 30, 2019 •

edited

Loading

Uh oh!

chinmaygarde left a comment

Uh oh!

chinmaygarde Sep 30, 2019

Uh oh!

chinmaygarde Sep 30, 2019

Uh oh!

chinmaygarde Sep 30, 2019

Uh oh!

chinmaygarde Sep 30, 2019

Uh oh!

cyanglaz commented Oct 7, 2019 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants


		~MockTexture() override = default;

		bool unregistered = false;

Add onUnregistered callback in 'Texture' and 'FlutterTexture' #12695

Add onUnregistered callback in 'Texture' and 'FlutterTexture' #12695

Uh oh!

Conversation

cyanglaz commented Sep 30, 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!

chinmaygarde Sep 30, 2019

Choose a reason for hiding this comment

Uh oh!

chinmaygarde Sep 30, 2019

Choose a reason for hiding this comment

Uh oh!

chinmaygarde Sep 30, 2019

Choose a reason for hiding this comment

Uh oh!

chinmaygarde Sep 30, 2019

Choose a reason for hiding this comment

Uh oh!

cyanglaz commented Oct 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cyanglaz commented Sep 30, 2019 •

edited

Loading

cyanglaz commented Oct 7, 2019 •

edited

Loading