Flutter External Texture Based On Share Texture Underlying Share Con… #11276

kaisa695275735 · 2019-08-20T09:57:38Z

Currently, external textures are very differently on iOS (CoreVideo) and Android (GL_OES_EGL_image_external).
In China, Portrait beauty effects are basically a must-have for a camera tool. We process our video frame on the GPU to generate a texture to be rendered. If flutter needs to display this texture, it requires a GPU->CPU->GPU process, wasting performance.
So we need to process the CoreVideo pixel buffer using OpenGL and give Flutter that texture directly instead of the pixel buffer
So we added an interface to Get Flutter GLContext on our Flutter branch. Based on this modification, we implemented the Flutter plugins such as Album, Video Capture, Video Editor.
And those plugins has been used by hundreds of millions of users on the Xianyu APP to verify its effectiveness。
We hope we can merge this change back to the flutter master so that more develop can use our plugins to develop video App。

googlebot · 2019-08-20T09:57:41Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

googlebot · 2019-08-20T09:57:41Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

googlebot · 2019-08-20T11:54:07Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

googlebot · 2019-08-20T11:54:08Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

kaisa695275735 · 2019-08-20T12:03:27Z

@googlebot
I signed it!

dnfield

Added some comments. This really badly needs a test of some sort - and ideally a benchmark. You could look at the tests in testing/scenarios_app and add some similar ones to what is there on the iOS side. The android side needs some work for testing unfortunately.

shell/platform/android/android_context_gl.cc

shell/platform/android/android_external_texture_gl_sc.cc

shell/platform/android/android_external_texture_gl_sc.h

shell/platform/android/io/flutter/embedding/engine/renderer/FlutterRenderer.java

shell/platform/android/io/flutter/view/FlutterView.java

shell/platform/android/io/flutter/view/TextureRegistry.java

shell/platform/android/platform_view_android_jni.cc

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

shell/platform/darwin/ios/ios_external_texture_gl_sc.mm

shell/platform/darwin/ios/ios_gl_context.mm

kaisa695275735 · 2019-08-28T02:41:47Z

@dnfield hello, we got an error when running format_and_dart_text:
License script got different results than expected for out/license_script_output/licenses_flutter.
and we tried to resolve it using the way
https://github.com/flutter/engine/tree/master/tools/licenses mentioned.

but we got an error on last step:

dnfield · 2019-08-28T04:28:25Z

The license tool can't handle random temp files, .DS_Store in particular makes it nutty. Just delete the file.

You very likely just need to add the new files you created to the licneses_golden - there's an alphabetical list in there that you can just pull from and add to manually (just look for android_context_gl.h and add yours alphabetically next to it).

dnfield · 2019-08-28T06:50:17Z

One other issue with this - it looks like @ lujunchen has not signed a CLA, but commits in here appear to have been authored by that user.

kaisa695275735 · 2019-08-28T07:18:51Z

One other issue with this - it looks like @ lujunchen has not signed a CLA, but commits in here appear to have been authored by that user.

YES, my mistake.
I use my company's username lujunchen(lujun.clj@alibaba-inc.com) on my commit.
So should i revert my changes and commit them again use my github account kaisa695275735(lujunchen.hit@gmail.com)?

googlebot · 2019-08-28T12:55:51Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

googlebot · 2019-08-28T12:55:51Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

gaaclarke

I've added a few topical comments around the code. There isn't a working example of FlutterShareTexture (test or otherwise) and there is no docstrings for it, so it's a bit hard to reason about. Could you please add a test or a dependent PR that uses your change?

Generally speaking, the problem you are trying to address is a good one and something we should address. It's going to take a bit more polish on the PR to get a better understanding if we are attacking it the correct way.

gaaclarke · 2019-08-29T20:50:33Z

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

 #include "FlutterMacros.h"

 NS_ASSUME_NONNULL_BEGIN

 FLUTTER_EXPORT
+@protocol FlutterShareTexture <NSObject>


Please add interface docstring.

gaaclarke · 2019-08-29T20:51:03Z

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

 #include "FlutterMacros.h"

 NS_ASSUME_NONNULL_BEGIN

 FLUTTER_EXPORT
+@protocol FlutterShareTexture <NSObject>
+/*
+ *copy OpenGL Texture directly


Please fix docstring formatting and use complete English sentences.

gaaclarke · 2019-08-29T20:52:10Z

shell/platform/darwin/ios/ios_external_texture_gl_share_context.h

+
+namespace flutter {
+
+class IOSExternalTextureShareContext : public flutter::Texture {


Please add class docstring.

gaaclarke · 2019-08-29T20:54:11Z

shell/platform/darwin/ios/ios_external_texture_gl_share_context.mm

+IOSExternalTextureShareContext::IOSExternalTextureShareContext(
+    int64_t textureId,
+    NSObject<FlutterShareTexture>* externalTexture)
+    : Texture(textureId), external_texture_(externalTexture) {


Shouldn't this have a retain on the external_texture_?

YES, their are other issues about the crash caused by external texture.
#10326
Just as metioned by @chinmaygarde , having a retain here is not thread safe.
So we recommend users avoiding to release the external_texture_ object too early.

gaaclarke · 2019-08-29T20:54:55Z

shell/platform/darwin/ios/ios_external_texture_gl_share_context.mm

+  textureInfo.fTarget = GL_TEXTURE_2D;
+
+  GrBackendTexture backendTexture(bounds.width(), bounds.height(), GrMipMapped::kNo, textureInfo);
+  printf("paint test back\n");


Stray print statement.

gaaclarke · 2019-08-29T20:57:23Z

shell/platform/darwin/ios/platform_view_ios.mm

+
+EAGLSharegroup* PlatformViewIOS::GetGLShareGroup() {
+  if (ios_surface_.get() == NULL)
+    return NULL;


return nullptr;

Add brackets.

Opening a new pr is fine

OK , i've created a new pr, #11819 .
@gaaclarke @dnfield @chinmaygarde @gw280

kaisa695275735 · 2019-08-30T01:39:59Z

I've added a few topical comments around the code. There isn't a working example of FlutterShareTexture (test or otherwise) and there is no docstrings for it, so it's a bit hard to reason about. Could you please add a test or a dependent PR that uses your change?

Generally speaking, the problem you are trying to address is a good one and something we should address. It's going to take a bit more polish on the PR to get a better understanding if we are attacking it the correct way.

Yes, we are looking at the tests in testing/scenarios_app and trying to add some similar ones.

kaisa695275735 · 2019-08-30T06:50:17Z

I've added a few topical comments around the code. There isn't a working example of FlutterShareTexture (test or otherwise) and there is no docstrings for it, so it's a bit hard to reason about. Could you please add a test or a dependent PR that uses your change?

Generally speaking, the problem you are trying to address is a good one and something we should address. It's going to take a bit more polish on the PR to get a better understanding if we are attacking it the correct way.

I'm very sorry that I accidentally deleted the external_texture_new branch, and result in the PR closed.
I don't know how can I reopen it, or should I commit a new PR ?

dnfield · 2019-08-30T14:18:23Z

Opening a new pr is fine

googlebot added cla: no labels Aug 20, 2019

cbracken requested review from dnfield, chinmaygarde, gaaclarke and gw280 August 26, 2019 18:00

dnfield suggested changes Aug 26, 2019

View reviewed changes

canewsin mentioned this pull request Aug 27, 2019

Some Audio Formats are not able to play on this library darwinvtomy/drm_wv_fp_player#2

Open

kaisa695275735 mentioned this pull request Aug 28, 2019

External Texture Underlying Share Context flutter/flutter#35515

Closed

Flutter External Texture Based On Share Texture

a386a66

kaisa695275735 force-pushed the external_texture_new branch from 934cd6e to a386a66 Compare August 28, 2019 12:55

googlebot added cla: yes and removed cla: no labels Aug 28, 2019

gaaclarke reviewed Aug 29, 2019

View reviewed changes

kaisa695275735 closed this Aug 30, 2019

kaisa695275735 deleted the external_texture_new branch August 30, 2019 02:45

kaisa695275735 mentioned this pull request Sep 2, 2019

Flutter External Texture Based On OpenGL Texture Underlying Share Context #11819

Closed


		namespace flutter {

		class IOSExternalTextureShareContext : public flutter::Texture {

Flutter External Texture Based On Share Texture Underlying Share Con… #11276

Flutter External Texture Based On Share Texture Underlying Share Con… #11276

Uh oh!

Conversation

kaisa695275735 commented Aug 20, 2019

Uh oh!

googlebot commented Aug 20, 2019

What to do if you already signed the CLA

Individual signers

Corporate signers

Uh oh!

googlebot commented Aug 20, 2019

What to do if you already signed the CLA

Individual signers

Corporate signers

Uh oh!

googlebot commented Aug 20, 2019

Uh oh!

googlebot commented Aug 20, 2019

Uh oh!

kaisa695275735 commented Aug 20, 2019

Uh oh!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kaisa695275735 commented Aug 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnfield commented Aug 28, 2019

Uh oh!

dnfield commented Aug 28, 2019

Uh oh!

kaisa695275735 commented Aug 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

googlebot commented Aug 28, 2019

Uh oh!

googlebot commented Aug 28, 2019

Uh oh!

gaaclarke 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!

kaisa695275735 commented Aug 30, 2019

Uh oh!

kaisa695275735 commented Aug 30, 2019

Uh oh!

dnfield commented Aug 30, 2019

Uh oh!

Uh oh!

kaisa695275735 commented Aug 28, 2019 •

edited

Loading

kaisa695275735 commented Aug 28, 2019 •

edited

Loading