Reuse texture cache in ios_external_texture_gl. #9806

loongliu · 2019-07-12T03:36:28Z

In current implementation, external texture data flow is a producer-consumer model. When painting external texture, it always asks registered external texture object to produce new CVPixelBuffer, then transforms it to texture. MarkNewFrameAvailable function is ignored. This commit changes the dataflow. Now ios_external_texture_gl caches previous opengl texture, if no new frame are available, it do not copyPixelBuffer method, just uses cached opengl texture to draw.

googlebot · 2019-07-12T03:36:32Z

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 (e.g. 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-07-12T03:36:32Z

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 (e.g. 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.

loongliu · 2019-07-13T06:16:05Z

I signed it!

googlebot · 2019-07-13T06:16:07Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

googlebot · 2019-07-13T06:16:07Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

iskakaushik · 2019-08-05T21:55:41Z

shell/platform/darwin/ios/ios_external_texture_gl.mm

                                 GrContext* context) {
  EnsureTextureCacheExists();
-  if (!freeze) {
+  if ( (!freeze && new_frame_ready_ ) || !texture_ref_) {


Why do we need the || !texture_ref_ part? I'm trying to understand the following branches:

We are frozen (freeze = true) and texture_ref_ is nil.

No new frame is ready and texture_ref_ is nil.

In both these cases wouldn't we always have an older texture generated from previous frame's pixel buffer?

When app resigns and enters background, texture_ref_ resets to nil in OnGrContextDestroyed. Then, app becomes active, freeze is false, new_frame_ready_ is false, texture_ref is nil.
If texture_ref_ is nil, the texture rendering output is blank.
In this case, it is needed to regenerate texture_ref_.

This is useful. Could you factor this conditional into a method and document this behavior? We would ideally have a test for this, but since that is currently hard, lets document this behavior.

iskakaushik · 2019-08-12T18:14:10Z

@loongliu could you please add a comment explaining it as I mentioned above. We can proceed to land this PR after that.

loongliu · 2019-08-15T07:02:58Z

@iskakaushik Changes has done.

iskakaushik

LGTM, with minor nit.

iskakaushik · 2019-08-15T17:51:06Z

shell/platform/darwin/ios/ios_external_texture_gl.mm

                                 GrContext* context) {
  EnsureTextureCacheExists();
-  if (!freeze) {
+  if (NeedUpdateTexture(freeze)) {


I think a better name here would be TextureNeedsUpdate

iskakaushik

minor nit.

iskakaushik · 2019-08-15T18:00:03Z

Can you rebase on top of the new master, i think that will fix the build failures. If you could give me permission to commit to your fork, i can try and fix them.

loongliu · 2019-08-16T03:16:22Z

@iskakaushik Invitation is sent.

cbracken · 2019-08-26T17:46:46Z

@iskakaushik have you had time to look into doing a quick rebase of this PR? If not, I'm happy to do it, but assume I need permission.

loongliu · 2019-08-27T04:29:16Z

@cbracken I sent you an invitation, would you do a rebase?

In current implementation, external texture data flow is a producer-consumer model. When painting external texture, it always asks registered external texture object to produce new CVPixelBuffer, then transforms it to texture. `MarkNewFrameAvailable` function is ignored. This commit changes the dataflow. Now ios_external_texture_gl caches previous opengl texture, if no new frame are available, it do not `copyPixelBuffer` method, just uses cached opengl texture to draw.

cbracken · 2019-08-27T08:27:41Z

@loongliu Thanks! I've rebased the PR, squashed the commits, and re-pushed. Just waiting on the presubmits now.

) Broke iOS builds: ../../flutter/shell/platform/darwin/ios/ios_external_texture_gl.mm:56:28: error: out-of-line definition of 'NeedUpdateTexture' does not match any declaration in 'flutter::IOSExternalTextureGL' bool IOSExternalTextureGL::NeedUpdateTexture(bool freeze) { ^~~~~~~~~~~~~~~~~ This reverts commit 94b3174.

…ne#9806)

In current implementation, external texture data flow is a producer-consumer model. When painting external texture, it always asks registered external texture object to produce new CVPixelBuffer, then transforms it to texture. `MarkNewFrameAvailable` function is ignored. This commit changes the dataflow. Now ios_external_texture_gl caches previous opengl texture, if no new frame are available, it do not `copyPixelBuffer` method, just uses cached opengl texture to draw. This is a re-land of #9806, which was previously reverted in #11522.

[email protected]:flutter/engine.git/compare/4d7ea15cae36...bf77966 git log 4d7ea15..bf77966 --no-merges --oneline 2019-08-27 [email protected] Revert "Reuse texture cache in ios_external_texture_gl. (#9806)" (flutter/engine#11522) 2019-08-27 [email protected] Reuse texture cache in ios_external_texture_gl. (flutter/engine#9806) 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

googlebot added the cla: no label Jul 12, 2019

googlebot added cla: yes and removed cla: no labels Jul 13, 2019

cbracken requested review from amirh, chinmaygarde and iskakaushik August 5, 2019 18:07

iskakaushik reviewed Aug 5, 2019

View reviewed changes

iskakaushik approved these changes Aug 15, 2019

View reviewed changes

iskakaushik reviewed Aug 15, 2019

View reviewed changes

cbracken merged commit 94b3174 into flutter:master Aug 27, 2019

cbracken deleted the rework_io_external_texture branch August 27, 2019 09:00

cbracken restored the rework_io_external_texture branch August 27, 2019 09:00

cbracken mentioned this pull request Aug 27, 2019

Reuse texture cache in ios_external_texture_gl #11524

Merged

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

94b3174 Reuse texture cache in ios_external_texture_gl. (flutter/engi…

55ee3a8

…ne#9806)

engine-flutter-autoroll mentioned this pull request Aug 27, 2019

Roll engine 4d7ea15cae36..bf7796611dc1 (2 commits) flutter/flutter#39340

Merged

Reuse texture cache in ios_external_texture_gl. #9806

Reuse texture cache in ios_external_texture_gl. #9806

Uh oh!

Conversation

loongliu commented Jul 12, 2019

Uh oh!

googlebot commented Jul 12, 2019

What to do if you already signed the CLA

Individual signers

Corporate signers

Uh oh!

googlebot commented Jul 12, 2019

What to do if you already signed the CLA

Individual signers

Corporate signers

Uh oh!

loongliu commented Jul 13, 2019

Uh oh!

googlebot commented Jul 13, 2019

Uh oh!

googlebot commented Jul 13, 2019

Uh oh!

iskakaushik Aug 5, 2019

Choose a reason for hiding this comment

Uh oh!

loongliu Aug 6, 2019

Choose a reason for hiding this comment

Uh oh!

iskakaushik Aug 6, 2019

Choose a reason for hiding this comment

Uh oh!

iskakaushik commented Aug 12, 2019

Uh oh!

loongliu commented Aug 15, 2019

Uh oh!

iskakaushik left a comment • 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!

iskakaushik Aug 15, 2019

Choose a reason for hiding this comment

Uh oh!

iskakaushik left a comment

Choose a reason for hiding this comment

Uh oh!

iskakaushik commented Aug 15, 2019

Uh oh!

loongliu commented Aug 16, 2019

Uh oh!

cbracken commented Aug 26, 2019

Uh oh!

loongliu commented Aug 27, 2019

Uh oh!

cbracken commented Aug 27, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

iskakaushik left a comment •

edited

Loading