Don't re-initialize the default RenderSurface when returning from hybrid composition mode (flutter/engine#47358)

When we enter hybrid composition mode we 'pause' the default RenderSurface (implemented by SurfaceView or TextureView) and swap to an ImageReader based RenderSurface.

When we return from hybrid composition mode we recreate and re-initialize the real RenderSurface as if it was being used for the first time.

This broke Platform Views in an internal app b/306122497 because we would incorrectly tell the texture to attach when it was never detached.

This CL changes the protocol so that when we return from hybrid composition mode we only swap the RenderSurface and do not re-create it. This avoids doing a bunch of unnecessary work and fixes the logic error of re-attaching textures that were never detached.

*Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.*

*List which issues are fixed by this PR. You must list at least one issue.*

*If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].*

[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
This commit is contained in:
John McCutchan
2023-10-26 17:33:23 -07:00
committed by GitHub
parent 9572116d73
commit 632d65292d
9 changed files with 157 additions and 81 deletions

View File

@@ -814,7 +814,6 @@ void Shell::OnPlatformViewCreated(std::unique_ptr<Surface> surface) {
const bool should_post_raster_task =
!task_runners_.GetRasterTaskRunner()->RunsTasksOnCurrentThread();
fml::AutoResetWaitableEvent latch;
auto raster_task = fml::MakeCopyable(
[&waiting_for_first_frame = waiting_for_first_frame_, //
rasterizer = rasterizer_->GetWeakPtr(), //
@@ -841,8 +840,8 @@ void Shell::OnPlatformViewCreated(std::unique_ptr<Surface> surface) {
// weak pointer. However, we are preventing the platform view from being
// collected by using a latch.
auto* platform_view = platform_view_.get();
FML_DCHECK(platform_view);
fml::AutoResetWaitableEvent latch;
auto io_task = [io_manager = io_manager_->GetWeakPtr(), platform_view,
ui_task_runner = task_runners_.GetUITaskRunner(), ui_task,

View File

@@ -186,6 +186,10 @@ public class FlutterImageView extends View implements RenderSurface {
// Not supported.
}
public void resume() {
// Not supported.
}
/**
* Acquires the next image to be drawn to the {@link android.graphics.Canvas}. Returns true if
* there's an image available in the queue.

View File

@@ -38,9 +38,12 @@ public class FlutterSurfaceView extends SurfaceView implements RenderSurface {
private final boolean renderTransparently;
private boolean isSurfaceAvailableForRendering = false;
private boolean isPaused = false;
private boolean isAttachedToFlutterRenderer = false;
@Nullable private FlutterRenderer flutterRenderer;
private boolean shouldNotify() {
return flutterRenderer != null && !isPaused;
}
// Connects the {@code Surface} beneath this {@code SurfaceView} with Flutter's native code.
// Callbacks are received by this Object and then those messages are forwarded to our
// FlutterRenderer, and then on to the JNI bridge over to native Flutter code.
@@ -51,7 +54,7 @@ public class FlutterSurfaceView extends SurfaceView implements RenderSurface {
Log.v(TAG, "SurfaceHolder.Callback.startRenderingToSurface()");
isSurfaceAvailableForRendering = true;
if (isAttachedToFlutterRenderer) {
if (shouldNotify()) {
connectSurfaceToRenderer();
}
}
@@ -60,7 +63,7 @@ public class FlutterSurfaceView extends SurfaceView implements RenderSurface {
public void surfaceChanged(
@NonNull SurfaceHolder holder, int format, int width, int height) {
Log.v(TAG, "SurfaceHolder.Callback.surfaceChanged()");
if (isAttachedToFlutterRenderer) {
if (shouldNotify()) {
changeSurfaceSize(width, height);
}
}
@@ -70,7 +73,7 @@ public class FlutterSurfaceView extends SurfaceView implements RenderSurface {
Log.v(TAG, "SurfaceHolder.Callback.stopRenderingToSurface()");
isSurfaceAvailableForRendering = false;
if (isAttachedToFlutterRenderer) {
if (shouldNotify()) {
disconnectSurfaceFromRenderer();
}
}
@@ -183,25 +186,15 @@ public class FlutterSurfaceView extends SurfaceView implements RenderSurface {
if (this.flutterRenderer != null) {
Log.v(
TAG,
"Already connected to a FlutterRenderer. Detaching from old one and attaching to new one.");
"Already connected to a FlutterRenderer. Detaching from old one and attaching to new"
+ " one.");
this.flutterRenderer.stopRenderingToSurface();
this.flutterRenderer.removeIsDisplayingFlutterUiListener(flutterUiDisplayListener);
}
this.flutterRenderer = flutterRenderer;
isAttachedToFlutterRenderer = true;
this.flutterRenderer.addIsDisplayingFlutterUiListener(flutterUiDisplayListener);
// If we're already attached to an Android window then we're now attached to both a renderer
// and the Android window. We can begin rendering now.
if (isSurfaceAvailableForRendering) {
Log.v(
TAG,
"Surface is available for rendering. Connecting FlutterRenderer to Android surface.");
connectSurfaceToRenderer();
}
isPaused = false;
resume();
}
/**
@@ -222,13 +215,13 @@ public class FlutterSurfaceView extends SurfaceView implements RenderSurface {
disconnectSurfaceFromRenderer();
}
pause();
// Make the SurfaceView invisible to avoid showing a black rectangle.
setAlpha(0.0f);
flutterRenderer.removeIsDisplayingFlutterUiListener(flutterUiDisplayListener);
flutterRenderer = null;
isAttachedToFlutterRenderer = false;
} else {
Log.w(TAG, "detachFromRenderer() invoked when no FlutterRenderer was attached.");
}
@@ -239,22 +232,37 @@ public class FlutterSurfaceView extends SurfaceView implements RenderSurface {
* UI to this {@code FlutterSurfaceView}.
*/
public void pause() {
if (flutterRenderer != null) {
// Don't remove the `flutterUiDisplayListener` as `onFlutterUiDisplayed()` will make
// the `FlutterSurfaceView` visible.
flutterRenderer = null;
isPaused = true;
isAttachedToFlutterRenderer = false;
} else {
if (flutterRenderer == null) {
Log.w(TAG, "pause() invoked when no FlutterRenderer was attached.");
return;
}
isPaused = true;
}
public void resume() {
if (flutterRenderer == null) {
Log.w(TAG, "resume() invoked when no FlutterRenderer was attached.");
return;
}
this.flutterRenderer.addIsDisplayingFlutterUiListener(flutterUiDisplayListener);
// If we're already attached to an Android window then we're now attached to both a renderer
// and the Android window. We can begin rendering now.
if (isSurfaceAvailableForRendering) {
Log.v(
TAG,
"Surface is available for rendering. Connecting FlutterRenderer to Android surface.");
connectSurfaceToRenderer();
}
isPaused = false;
}
// FlutterRenderer and getSurfaceTexture() must both be non-null.
private void connectSurfaceToRenderer() {
if (flutterRenderer == null || getHolder() == null) {
throw new IllegalStateException(
"connectSurfaceToRenderer() should only be called when flutterRenderer and getHolder() are non-null.");
"connectSurfaceToRenderer() should only be called when flutterRenderer and getHolder()"
+ " are non-null.");
}
// When connecting the surface to the renderer, it's possible that the surface is currently
// paused. For instance, when a platform view is displayed, the current FlutterSurfaceView
@@ -285,9 +293,10 @@ public class FlutterSurfaceView extends SurfaceView implements RenderSurface {
private void disconnectSurfaceFromRenderer() {
if (flutterRenderer == null) {
throw new IllegalStateException(
"disconnectSurfaceFromRenderer() should only be called when flutterRenderer is non-null.");
"disconnectSurfaceFromRenderer() should only be called when flutterRenderer is"
+ " non-null.");
}
flutterRenderer.stopRenderingToSurface();
}
}
}

View File

@@ -35,11 +35,14 @@ public class FlutterTextureView extends TextureView implements RenderSurface {
private static final String TAG = "FlutterTextureView";
private boolean isSurfaceAvailableForRendering = false;
private boolean isAttachedToFlutterRenderer = false;
private boolean isPaused = false;
@Nullable private FlutterRenderer flutterRenderer;
@Nullable private Surface renderSurface;
private boolean shouldNotify() {
return flutterRenderer != null && !isPaused;
}
// Connects the {@code SurfaceTexture} beneath this {@code TextureView} with Flutter's native
// code.
// Callbacks are received by this Object and then those messages are forwarded to our
@@ -55,7 +58,7 @@ public class FlutterTextureView extends TextureView implements RenderSurface {
// If we're already attached to a FlutterRenderer then we're now attached to both a
// renderer
// and the Android window, so we can begin rendering now.
if (isAttachedToFlutterRenderer) {
if (shouldNotify()) {
connectSurfaceToRenderer();
}
}
@@ -64,7 +67,7 @@ public class FlutterTextureView extends TextureView implements RenderSurface {
public void onSurfaceTextureSizeChanged(
@NonNull SurfaceTexture surface, int width, int height) {
Log.v(TAG, "SurfaceTextureListener.onSurfaceTextureSizeChanged()");
if (isAttachedToFlutterRenderer) {
if (shouldNotify()) {
changeSurfaceSize(width, height);
}
}
@@ -82,7 +85,7 @@ public class FlutterTextureView extends TextureView implements RenderSurface {
// If we're attached to a FlutterRenderer then we need to notify it that our
// SurfaceTexture
// has been destroyed.
if (isAttachedToFlutterRenderer) {
if (shouldNotify()) {
disconnectSurfaceFromRenderer();
}
@@ -139,21 +142,14 @@ public class FlutterTextureView extends TextureView implements RenderSurface {
if (this.flutterRenderer != null) {
Log.v(
TAG,
"Already connected to a FlutterRenderer. Detaching from old one and attaching to new one.");
"Already connected to a FlutterRenderer. Detaching from old one and attaching to new"
+ " one.");
this.flutterRenderer.stopRenderingToSurface();
}
this.flutterRenderer = flutterRenderer;
isAttachedToFlutterRenderer = true;
// If we're already attached to an Android window then we're now attached to both a renderer
// and the Android window. We can begin rendering now.
if (isSurfaceAvailableForRendering) {
Log.v(
TAG,
"Surface is available for rendering. Connecting FlutterRenderer to Android surface.");
connectSurfaceToRenderer();
}
resume();
}
/**
@@ -174,8 +170,9 @@ public class FlutterTextureView extends TextureView implements RenderSurface {
disconnectSurfaceFromRenderer();
}
pause();
flutterRenderer = null;
isAttachedToFlutterRenderer = false;
} else {
Log.w(TAG, "detachFromRenderer() invoked when no FlutterRenderer was attached.");
}
@@ -186,13 +183,28 @@ public class FlutterTextureView extends TextureView implements RenderSurface {
* UI to this {@code FlutterTextureView}.
*/
public void pause() {
if (flutterRenderer != null) {
flutterRenderer = null;
isPaused = true;
isAttachedToFlutterRenderer = false;
} else {
if (flutterRenderer == null) {
Log.w(TAG, "pause() invoked when no FlutterRenderer was attached.");
return;
}
isPaused = true;
}
public void resume() {
if (flutterRenderer == null) {
Log.w(TAG, "resume() invoked when no FlutterRenderer was attached.");
return;
}
// If we're already attached to an Android window then we're now attached to both a renderer
// and the Android window. We can begin rendering now.
if (isSurfaceAvailableForRendering) {
Log.v(
TAG,
"Surface is available for rendering. Connecting FlutterRenderer to Android surface.");
connectSurfaceToRenderer();
}
isPaused = false;
}
/**
@@ -209,7 +221,8 @@ public class FlutterTextureView extends TextureView implements RenderSurface {
private void connectSurfaceToRenderer() {
if (flutterRenderer == null || getSurfaceTexture() == null) {
throw new IllegalStateException(
"connectSurfaceToRenderer() should only be called when flutterRenderer and getSurfaceTexture() are non-null.");
"connectSurfaceToRenderer() should only be called when flutterRenderer and"
+ " getSurfaceTexture() are non-null.");
}
// Definitively release the surface to avoid leaked closeables, just in case
@@ -220,7 +233,6 @@ public class FlutterTextureView extends TextureView implements RenderSurface {
renderSurface = new Surface(getSurfaceTexture());
flutterRenderer.startRenderingToSurface(renderSurface, isPaused);
isPaused = false;
}
// FlutterRenderer must be non-null.
@@ -243,7 +255,8 @@ public class FlutterTextureView extends TextureView implements RenderSurface {
private void disconnectSurfaceFromRenderer() {
if (flutterRenderer == null) {
throw new IllegalStateException(
"disconnectSurfaceFromRenderer() should only be called when flutterRenderer is non-null.");
"disconnectSurfaceFromRenderer() should only be called when flutterRenderer is"
+ " non-null.");
}
flutterRenderer.stopRenderingToSurface();
@@ -252,4 +265,4 @@ public class FlutterTextureView extends TextureView implements RenderSurface {
renderSurface = null;
}
}
}
}

View File

@@ -1369,9 +1369,9 @@ public class FlutterView extends FrameLayout
onDone.run();
return;
}
// Start rendering on the previous surface.
// Resume rendering to the previous surface.
// This surface is typically `FlutterSurfaceView` or `FlutterTextureView`.
renderSurface.attachToRenderer(renderer);
renderSurface.resume();
// Install a Flutter UI listener to wait until the first frame is rendered
// in the new surface to call the `onDone` callback.

View File

@@ -467,20 +467,26 @@ public class FlutterRenderer implements TextureRegistry {
* android.view.TextureView.SurfaceTextureListener}
*
* @param surface The render surface.
* @param keepCurrentSurface True if the current active surface should not be released.
* @param onlySwap True if the current active surface should not be detached.
*/
public void startRenderingToSurface(@NonNull Surface surface, boolean keepCurrentSurface) {
// Don't stop rendering the surface if it's currently paused.
// Stop rendering to the surface releases the associated native resources, which
// causes a glitch when showing platform views.
// For more, https://github.com/flutter/flutter/issues/95343
if (this.surface != null && !keepCurrentSurface) {
public void startRenderingToSurface(@NonNull Surface surface, boolean onlySwap) {
if (!onlySwap) {
// Stop rendering to the surface releases the associated native resources, which
// causes a glitch when toggling between rendering to an image view (hybrid composition) and
// rendering directly to a Surface or Texture view. For more,
// https://github.com/flutter/flutter/issues/95343
stopRenderingToSurface();
}
this.surface = surface;
flutterJNI.onSurfaceCreated(surface);
if (onlySwap) {
// In the swap case we are just swapping the surface that we render to.
flutterJNI.onSurfaceWindowChanged(surface);
} else {
// In the non-swap case we are creating a new surface to render to.
flutterJNI.onSurfaceCreated(surface);
}
}
/**

View File

@@ -60,4 +60,11 @@ public interface RenderSurface {
* #attachToRenderer(FlutterRenderer)}.
*/
void pause();
/**
* Instructs this {@code RenderSurface} to resume forwarding {@code Surface} notifications to the
* {@code FlutterRenderer} that was previously connected with {@link
* #attachToRenderer(FlutterRenderer)}.
*/
void resume();
}

View File

@@ -197,6 +197,8 @@ void PlatformViewAndroid::NotifySurfaceWindowChanged(
});
latch.Wait();
}
PlatformView::ScheduleFrame();
}
void PlatformViewAndroid::NotifyDestroyed() {

View File

@@ -36,11 +36,13 @@ public class FlutterRendererTest {
private FlutterJNI fakeFlutterJNI;
private Surface fakeSurface;
private Surface fakeSurface2;
@Before
public void setup() {
fakeFlutterJNI = mock(FlutterJNI.class);
fakeSurface = mock(Surface.class);
fakeSurface2 = mock(Surface.class);
}
@Test
@@ -50,7 +52,7 @@ public class FlutterRendererTest {
FlutterRenderer flutterRenderer = new FlutterRenderer(fakeFlutterJNI);
// Execute the behavior under test.
flutterRenderer.startRenderingToSurface(fakeSurface, /*keepCurrentSurface=*/ false);
flutterRenderer.startRenderingToSurface(fakeSurface, false);
// Verify the behavior under test.
verify(fakeFlutterJNI, times(1)).onSurfaceCreated(eq(fakeSurface));
@@ -62,7 +64,7 @@ public class FlutterRendererTest {
Surface fakeSurface = mock(Surface.class);
FlutterRenderer flutterRenderer = new FlutterRenderer(fakeFlutterJNI);
flutterRenderer.startRenderingToSurface(fakeSurface, /*keepCurrentSurface=*/ false);
flutterRenderer.startRenderingToSurface(fakeSurface, false);
// Execute the behavior under test.
flutterRenderer.surfaceChanged(100, 50);
@@ -77,7 +79,7 @@ public class FlutterRendererTest {
Surface fakeSurface = mock(Surface.class);
FlutterRenderer flutterRenderer = new FlutterRenderer(fakeFlutterJNI);
flutterRenderer.startRenderingToSurface(fakeSurface, /*keepCurrentSurface=*/ false);
flutterRenderer.startRenderingToSurface(fakeSurface, false);
// Execute the behavior under test.
flutterRenderer.stopRenderingToSurface();
@@ -92,10 +94,10 @@ public class FlutterRendererTest {
Surface fakeSurface2 = mock(Surface.class);
FlutterRenderer flutterRenderer = new FlutterRenderer(fakeFlutterJNI);
flutterRenderer.startRenderingToSurface(fakeSurface, /*keepCurrentSurface=*/ false);
flutterRenderer.startRenderingToSurface(fakeSurface, false);
// Execute behavior under test.
flutterRenderer.startRenderingToSurface(fakeSurface2, /*keepCurrentSurface=*/ false);
flutterRenderer.startRenderingToSurface(fakeSurface2, false);
// Verify behavior under test.
verify(fakeFlutterJNI, times(1)).onSurfaceDestroyed(); // notification of 1st surface's removal.
@@ -106,7 +108,7 @@ public class FlutterRendererTest {
// Setup the test.
FlutterRenderer flutterRenderer = new FlutterRenderer(fakeFlutterJNI);
flutterRenderer.startRenderingToSurface(fakeSurface, /*keepCurrentSurface=*/ false);
flutterRenderer.startRenderingToSurface(fakeSurface, false);
// Execute the behavior under test.
flutterRenderer.stopRenderingToSurface();
@@ -120,9 +122,9 @@ public class FlutterRendererTest {
// Setup the test.
FlutterRenderer flutterRenderer = new FlutterRenderer(fakeFlutterJNI);
flutterRenderer.startRenderingToSurface(fakeSurface, /*keepCurrentSurface=*/ false);
flutterRenderer.startRenderingToSurface(fakeSurface, false);
flutterRenderer.startRenderingToSurface(fakeSurface, /*keepCurrentSurface=*/ false);
flutterRenderer.startRenderingToSurface(fakeSurface, false);
// Verify behavior under test.
verify(fakeFlutterJNI, times(1)).onSurfaceDestroyed();
@@ -133,9 +135,9 @@ public class FlutterRendererTest {
// Setup the test.
FlutterRenderer flutterRenderer = new FlutterRenderer(fakeFlutterJNI);
flutterRenderer.startRenderingToSurface(fakeSurface, /*keepCurrentSurface=*/ false);
flutterRenderer.startRenderingToSurface(fakeSurface, false);
flutterRenderer.startRenderingToSurface(fakeSurface, /*keepCurrentSurface=*/ true);
flutterRenderer.startRenderingToSurface(fakeSurface, true);
// Verify behavior under test.
verify(fakeFlutterJNI, never()).onSurfaceDestroyed();
@@ -151,7 +153,7 @@ public class FlutterRendererTest {
FlutterRenderer.SurfaceTextureRegistryEntry entry =
(FlutterRenderer.SurfaceTextureRegistryEntry) flutterRenderer.createSurfaceTexture();
flutterRenderer.startRenderingToSurface(fakeSurface, /*keepCurrentSurface=*/ false);
flutterRenderer.startRenderingToSurface(fakeSurface, false);
// Execute the behavior under test.
flutterRenderer.stopRenderingToSurface();
@@ -174,7 +176,7 @@ public class FlutterRendererTest {
(FlutterRenderer.SurfaceTextureRegistryEntry)
flutterRenderer.registerSurfaceTexture(surfaceTexture);
flutterRenderer.startRenderingToSurface(fakeSurface, /*keepCurrentSurface=*/ false);
flutterRenderer.startRenderingToSurface(fakeSurface, false);
// Verify behavior under test.
assertEquals(surfaceTexture, entry.surfaceTexture());
@@ -195,7 +197,7 @@ public class FlutterRendererTest {
(FlutterRenderer.SurfaceTextureRegistryEntry) flutterRenderer.createSurfaceTexture();
long id = entry.id();
flutterRenderer.startRenderingToSurface(fakeSurface, /*keepCurrentSurface=*/ false);
flutterRenderer.startRenderingToSurface(fakeSurface, false);
// Execute the behavior under test.
runFinalization(entry);
@@ -221,7 +223,7 @@ public class FlutterRendererTest {
(FlutterRenderer.SurfaceTextureRegistryEntry) flutterRenderer.createSurfaceTexture();
long id = entry.id();
flutterRenderer.startRenderingToSurface(fakeSurface, /*keepCurrentSurface=*/ false);
flutterRenderer.startRenderingToSurface(fakeSurface, false);
flutterRenderer.stopRenderingToSurface();
@@ -399,7 +401,7 @@ public class FlutterRendererTest {
// Setup the test.
FlutterRenderer flutterRenderer = new FlutterRenderer(fakeFlutterJNI);
flutterRenderer.startRenderingToSurface(fakeSurface, /*keepCurrentSurface=*/ false);
flutterRenderer.startRenderingToSurface(fakeSurface, false);
// Execute the behavior under test.
// Simulate calling |FlutterRenderer#stopRenderingToSurface| twice with different code paths.
@@ -409,4 +411,38 @@ public class FlutterRendererTest {
// Verify behavior under test.
verify(fakeFlutterJNI, times(1)).onSurfaceDestroyed();
}
@Test
public void itInvokesCreatesSurfaceWhenStartingRendering() {
FlutterRenderer flutterRenderer = new FlutterRenderer(fakeFlutterJNI);
flutterRenderer.startRenderingToSurface(fakeSurface, false);
verify(fakeFlutterJNI, times(1)).onSurfaceCreated(eq(fakeSurface));
}
@Test
public void itDoesNotInvokeCreatesSurfaceWhenResumingRendering() {
FlutterRenderer flutterRenderer = new FlutterRenderer(fakeFlutterJNI);
// The following call sequence mimics the behaviour of FlutterView when it exits from hybrid
// composition mode.
// Install initial rendering surface.
flutterRenderer.startRenderingToSurface(fakeSurface, false);
verify(fakeFlutterJNI, times(1)).onSurfaceCreated(eq(fakeSurface));
verify(fakeFlutterJNI, times(0)).onSurfaceWindowChanged(eq(fakeSurface));
// Install the image view.
flutterRenderer.startRenderingToSurface(fakeSurface2, true);
verify(fakeFlutterJNI, times(1)).onSurfaceCreated(eq(fakeSurface));
verify(fakeFlutterJNI, times(0)).onSurfaceWindowChanged(eq(fakeSurface));
verify(fakeFlutterJNI, times(0)).onSurfaceCreated(eq(fakeSurface2));
verify(fakeFlutterJNI, times(1)).onSurfaceWindowChanged(eq(fakeSurface2));
flutterRenderer.startRenderingToSurface(fakeSurface, true);
verify(fakeFlutterJNI, times(1)).onSurfaceCreated(eq(fakeSurface));
verify(fakeFlutterJNI, times(1)).onSurfaceWindowChanged(eq(fakeSurface));
verify(fakeFlutterJNI, times(0)).onSurfaceCreated(eq(fakeSurface2));
verify(fakeFlutterJNI, times(1)).onSurfaceWindowChanged(eq(fakeSurface2));
}
}