This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Wait before switching surfaces #20100

Merged

fluttergithubbot merged 7 commits into flutter:master from blasten:wait_before_switching_surfaces

Jul 31, 2020

blasten commented Jul 29, 2020 •

edited

Loading

Description

Once a platform view disappears, the embedding continues rendering the Flutter UI on FlutterSurfaceView. However, the embedding should wait for the first frame before displaying the SurfaceView.

Related Issues

Fixes flutter/flutter#62455

Tests

I added the following tests: Unit test

Replace this with a list of the tests that you added as part of this PR. A
change in behaviour with no test covering it will likely get reverted
accidentally sooner or later. PRs must include tests for all
changed/updated/fixed behaviors. See testing the engine for instructions on
writing and running engine tests.

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.

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

Emmanuel Garcia added 2 commits

July 28, 2020 18:24


          Wait until first frame is rendered back in surface


          final

54fca20

flutter-dashboard bot added the platform-android label

googlebot added the cla: yes label

flutter deleted a comment from flutter-dashboard bot

auto-assign bot requested a review from GaryQian

July 29, 2020 01:36


          Test

7b76886

blasten requested review from jason-simmons and tvolkert and removed request for GaryQian

July 30, 2020 02:50

tvolkert reviewed

View reviewed changes

shell/platform/android/io/flutter/embedding/android/FlutterView.java Outdated

+                    callSafely(onDone);
+                    return;
+                  }
+                  final FlutterRenderer render = flutterEngine.getRenderer();

Contributor

tvolkert Jul 30, 2020

Nit: renderer?

Author

blasten Jul 30, 2020

Done.

tvolkert reviewed

View reviewed changes

shell/platform/android/io/flutter/embedding/android/FlutterView.java Outdated

		@@ -970,9 +976,12 @@ public void convertToImageView() {

		/**
		* If the surface is rendered by a {@code FlutterImageView}. Then, calling this method will stop

Contributor

tvolkert Jul 30, 2020

Nit: s/. Then,/, then/

Author

blasten Jul 30, 2020

nice catch

tvolkert reviewed

View reviewed changes

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

@@ @@ -949,13 +950,18 @@ public void detachFromFlutterEngine() { @@
                   flutterEngine = null;
                 }
+                @VisibleForTesting

Contributor

tvolkert Jul 30, 2020

Are the tests in the same package (can this be package private)?

Author

blasten Jul 30, 2020

different packages unfortunately.io.flutter.embedding.android vs io.flutter.plugin.platform

tvolkert reviewed

View reviewed changes

shell/platform/android/io/flutter/embedding/android/FlutterView.java Outdated

		@@ -970,9 +976,12 @@ public void convertToImageView() {

		/**
		* If the surface is rendered by a {@code FlutterImageView}. Then, calling this method will stop

Contributor

tvolkert Jul 30, 2020

Are there docs you can link to about what surface this method's talking about?

Author

blasten Jul 30, 2020

I changed the annotation to {@link}. The TDD is internal only, but flutter/flutter#61976 tracks making it public.

tvolkert reviewed

View reviewed changes

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

@@ @@ -981,11 +990,46 @@ public void revertImageView() { @@
                     Log.v(TAG, "Tried to revert the image view, but no previous surface was used.");
                     return;
                   }
-                  flutterImageView.detachFromRenderer();
                   renderSurface = previousRenderSurface;
                   previousRenderSurface = null;

Contributor

tvolkert Jul 30, 2020

The fact that there's a single instance variable for the previous surface seems weird... can you render an image view over an image view? should we keep a stack of surfaces?

Author

blasten Jul 30, 2020

ack.

can you render an image view over an image view?

No.

Only FlutterSurfaceView/FlutterTextureView <-> FlutterImage, so a stack isn't needed as there could be 2 surfaces max.

tvolkert reviewed

View reviewed changes

shell/platform/android/io/flutter/embedding/engine/FlutterJNI.java

+                  nativePlatformViewId = performNativeAttach(this, isBackgroundView);
+                }
+                @VisibleForTesting

Contributor

tvolkert Jul 30, 2020

Can this be package private?

Author

blasten Jul 30, 2020

Unfortunately, it can't because FlutterJNI is a dependency of the PlatformViewsController, which for historic reasons is a different package.

tvolkert reviewed

View reviewed changes

shell/platform/android/io/flutter/embedding/engine/FlutterJNI.java Outdated

-                // TODO(mattcarroll): add javadocs
+                private native boolean nativeGetIsSoftwareRenderingEnabled();
+                @VisibleForTesting

Contributor

tvolkert Jul 30, 2020

Can this be package private?

Contributor

tvolkert Jul 30, 2020

It looks like it's not just visible for testing - it's used below in FlutterRenderer.isSoftwareRenderingEnabled()

Author

blasten Jul 30, 2020

right on. Fixed

tvolkert reviewed

View reviewed changes

shell/platform/android/io/flutter/embedding/engine/FlutterJNI.java

                 @SuppressWarnings("unused")
                 @VisibleForTesting
                 @UiThread
-                void onFirstFrame() {
+                public void onFirstFrame() {

Contributor

tvolkert Jul 30, 2020

Can this be package private?

Author

blasten Jul 30, 2020

ditto

tvolkert reviewed

View reviewed changes

shell/platform/android/io/flutter/plugin/platform/PlatformViewsController.java Outdated

+                        () -> {
+                          // Destroy overlay surfaces once the surface reversion is completed.
+                          finishFrame(false);
+                          return null;

Contributor

tvolkert Jul 30, 2020

Is this return needed?

Author

blasten Jul 30, 2020

Done.

tvolkert approved these changes

View reviewed changes

Emmanuel Garcia added 3 commits

July 30, 2020 13:26


          Feedback

3f93967


          javadocs

daf41be


          Callable->Runnable

94503c1

blasten added the waiting for tree to go green label

tvolkert approved these changes

View reviewed changes

Contributor

fluttergithubbot commented Jul 30, 2020

This pull request is not suitable for automatic merging in its current state.

The status or check suite Linux Android Debug Engine has failed. Please fix the issues identified (or deflake) before re-applying this label.

fluttergithubbot removed the waiting for tree to go green label


          Retrigger LUCI checks

ba2c8b7

blasten added the waiting for tree to go green label

Contributor

fluttergithubbot commented Jul 31, 2020

This pull request is not suitable for automatic merging in its current state.

The status or check suite Mac Android AOT Engine has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite Mac Android Debug Engine has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite Mac Host Engine has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite Mac Web Engine has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite Mac iOS Engine has failed. Please fix the issues identified (or deflake) before re-applying this label.

fluttergithubbot removed the waiting for tree to go green label

Author

blasten commented Jul 31, 2020

@godofredoc is there any infra issue? Some of the LUCI checks seem to have failed twice.

Contributor

godofredoc commented Jul 31, 2020

@godofredoc is there any infra issue? Some of the LUCI checks seem to have failed twice.

There were so many pending tasks that will be timing out, I cancelled all the mac tasks older than 1 hour starting at 7:00. Re-triggered the tasks for this PR

godofredoc added the waiting for tree to go green label

fluttergithubbot merged commit 7f5d044 into flutter:master

blasten deleted the wait_before_switching_surfaces branch

July 31, 2020 17:02

pcsosinski pushed a commit to pcsosinski/engine that referenced this pull request


          Wait before switching surfaces (flutter#20100)

engine-flutter-autoroll mentioned this pull request

Roll Engine from 280bbfc763cf to 5513273d0feb (2 revisions) flutter/flutter#62683

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request


          7f5d044 Wait before switching surfaces (flutter/engine#20100)

68f03e6

engine-flutter-autoroll mentioned this pull request

Roll Engine from 280bbfc763cf to 3b0e697db64c (4 revisions) flutter/flutter#62688

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request


          7f5d044 Wait before switching surfaces (flutter/engine#20100)

00eecd9

pcsosinski pushed a commit that referenced this pull request


          Wait before switching surfaces (#20100) (#20180)

d6ee149

Co-authored-by: Emmanuel Garcia <egarciad@google.com>

engine-flutter-autoroll mentioned this pull request

Roll Engine from 280bbfc763cf to ed36b1aa25d1 (6 revisions) flutter/flutter#62692

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request


          7f5d044 Wait before switching surfaces (flutter/engine#20100)

engine-flutter-autoroll mentioned this pull request

Roll Engine from 280bbfc763cf to fcc1eaf8ffff (7 revisions) flutter/flutter#62699

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request


          7f5d044 Wait before switching surfaces (flutter/engine#20100)

83cd7af

engine-flutter-autoroll mentioned this pull request

Roll Engine from 280bbfc763cf to 083282e33bbb (22 revisions) flutter/flutter#62768

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request


          7f5d044 Wait before switching surfaces (flutter/engine#20100)

0c69ede

zanderso pushed a commit to flutter/flutter that referenced this pull request


          Roll Engine from 280bbfc to 083282e (22 revisions) (#62768)

d261f1e

* 7f5d044 Wait before switching surfaces (flutter/engine#20100)

* 5513273 Reland: Avoid a copy in EncodeImage (flutter/engine#20003)

* 1efdd95 Roll Dart SDK from bd528bfbd69d to ea6bde577d1c (19 revisions) (flutter/engine#20172)

* 3b0e697 Roll Skia from 8cc118dce813 to c3794dd52778 (27 revisions) (flutter/engine#20173)

* cb1a374 Roll Fuchsia Mac SDK from T2xc0OuiK... to i0zTcQ8Qb... (flutter/engine#20175)

* ed36b1a Roll Skia from c3794dd52778 to 2d01ed94605a (10 revisions) (flutter/engine#20179)

* fcc1eaf Fix iOS Keyboard stuck as UIKeyboardTypeNamePhonePad (flutter/engine#20181)

* 9c6837c Roll Skia from 2d01ed94605a to 7225788b9070 (6 revisions) (flutter/engine#20183)

* 13e993e Fix Typos (flutter/engine#19691)

* 98cfd1d Move platform specific information to `PlatformConfiguration` class (flutter/engine#19652)

* 22fb58b update nullability of drawAtlas methods and flesh out docs (flutter/engine#20176)

* bcc43df Roll Dart SDK from ea6bde577d1c to 033a81d924b9 (23 revisions) (flutter/engine#20186)

* cb4bb93 [web] increase number of shards. sync engine web tests same as flutter repo (flutter/engine#20164)

* d986b8d Enable linting in several files (flutter/engine#20134)

* 7dd092d Enable more linting (flutter/engine#20187)

* 3cc86ac Roll Dart SDK from 033a81d924b9 to ad5bcf16f1c8 (9 revisions) (flutter/engine#20191)

* 5ca8a2a Roll Dart SDK from ad5bcf16f1c8 to d169af6f7d8f (1 revision) (flutter/engine#20192)

* 4de0c04 Roll Dart SDK from d169af6f7d8f to 7e6c55e3aaf5 (1 revision) (flutter/engine#20196)

* 908fe01 Fix navigation message relay. (flutter/engine#20193)

* f1b3b69 Roll Dart SDK from 7e6c55e3aaf5 to 365525432a70 (2 revisions) (flutter/engine#20197)

* 8fbdd3f Fix parameter names

* 083282e Fix Implments typo

Pragya007 pushed a commit to Pragya007/flutter that referenced this pull request


          Roll Engine from 280bbfc to 083282e (22 revisions) (flutter#62768)

13b14d3

* 7f5d044 Wait before switching surfaces (flutter/engine#20100)

* 5513273 Reland: Avoid a copy in EncodeImage (flutter/engine#20003)

* 1efdd95 Roll Dart SDK from bd528bfbd69d to ea6bde577d1c (19 revisions) (flutter/engine#20172)

* 3b0e697 Roll Skia from 8cc118dce813 to c3794dd52778 (27 revisions) (flutter/engine#20173)

* cb1a374 Roll Fuchsia Mac SDK from T2xc0OuiK... to i0zTcQ8Qb... (flutter/engine#20175)

* ed36b1a Roll Skia from c3794dd52778 to 2d01ed94605a (10 revisions) (flutter/engine#20179)

* fcc1eaf Fix iOS Keyboard stuck as UIKeyboardTypeNamePhonePad (flutter/engine#20181)

* 9c6837c Roll Skia from 2d01ed94605a to 7225788b9070 (6 revisions) (flutter/engine#20183)

* 13e993e Fix Typos (flutter/engine#19691)

* 98cfd1d Move platform specific information to `PlatformConfiguration` class (flutter/engine#19652)

* 22fb58b update nullability of drawAtlas methods and flesh out docs (flutter/engine#20176)

* bcc43df Roll Dart SDK from ea6bde577d1c to 033a81d924b9 (23 revisions) (flutter/engine#20186)

* cb4bb93 [web] increase number of shards. sync engine web tests same as flutter repo (flutter/engine#20164)

* d986b8d Enable linting in several files (flutter/engine#20134)

* 7dd092d Enable more linting (flutter/engine#20187)

* 3cc86ac Roll Dart SDK from 033a81d924b9 to ad5bcf16f1c8 (9 revisions) (flutter/engine#20191)

* 5ca8a2a Roll Dart SDK from ad5bcf16f1c8 to d169af6f7d8f (1 revision) (flutter/engine#20192)

* 4de0c04 Roll Dart SDK from d169af6f7d8f to 7e6c55e3aaf5 (1 revision) (flutter/engine#20196)

* 908fe01 Fix navigation message relay. (flutter/engine#20193)

* f1b3b69 Roll Dart SDK from 7e6c55e3aaf5 to 365525432a70 (2 revisions) (flutter/engine#20197)

* 8fbdd3f Fix parameter names

* 083282e Fix Implments typo

mingwandroid pushed a commit to mingwandroid/flutter that referenced this pull request


          Roll Engine from 280bbfc to 083282e (22 revisions) (flutter#62768)

a822754

* 7f5d044 Wait before switching surfaces (flutter/engine#20100)

* 5513273 Reland: Avoid a copy in EncodeImage (flutter/engine#20003)

* 1efdd95 Roll Dart SDK from bd528bfbd69d to ea6bde577d1c (19 revisions) (flutter/engine#20172)

* 3b0e697 Roll Skia from 8cc118dce813 to c3794dd52778 (27 revisions) (flutter/engine#20173)

* cb1a374 Roll Fuchsia Mac SDK from T2xc0OuiK... to i0zTcQ8Qb... (flutter/engine#20175)

* ed36b1a Roll Skia from c3794dd52778 to 2d01ed94605a (10 revisions) (flutter/engine#20179)

* fcc1eaf Fix iOS Keyboard stuck as UIKeyboardTypeNamePhonePad (flutter/engine#20181)

* 9c6837c Roll Skia from 2d01ed94605a to 7225788b9070 (6 revisions) (flutter/engine#20183)

* 13e993e Fix Typos (flutter/engine#19691)

* 98cfd1d Move platform specific information to `PlatformConfiguration` class (flutter/engine#19652)

* 22fb58b update nullability of drawAtlas methods and flesh out docs (flutter/engine#20176)

* bcc43df Roll Dart SDK from ea6bde577d1c to 033a81d924b9 (23 revisions) (flutter/engine#20186)

* cb4bb93 [web] increase number of shards. sync engine web tests same as flutter repo (flutter/engine#20164)

* d986b8d Enable linting in several files (flutter/engine#20134)

* 7dd092d Enable more linting (flutter/engine#20187)

* 3cc86ac Roll Dart SDK from 033a81d924b9 to ad5bcf16f1c8 (9 revisions) (flutter/engine#20191)

* 5ca8a2a Roll Dart SDK from ad5bcf16f1c8 to d169af6f7d8f (1 revision) (flutter/engine#20192)

* 4de0c04 Roll Dart SDK from d169af6f7d8f to 7e6c55e3aaf5 (1 revision) (flutter/engine#20196)

* 908fe01 Fix navigation message relay. (flutter/engine#20193)

* f1b3b69 Roll Dart SDK from 7e6c55e3aaf5 to 365525432a70 (2 revisions) (flutter/engine#20197)

* 8fbdd3f Fix parameter names

* 083282e Fix Implments typo

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yes platform-android waiting for tree to go green