From 43f0f4e1ce31aba8f11256d3afcf4a521bbaff1b Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Mon, 29 Jan 2024 11:21:05 -0800 Subject: [PATCH] Manually revert TLHC optimizations, holding on to width/height changes. (flutter/engine#50144) Reverts https://github.com/flutter/engine/pull/50065 due to https://github.com/flutter/flutter/issues/142459. [Discord](https://discord.com/channels/608014603317936148/608020293944082452/1201589744690270338): > It looks like the failing tests are the resize tests for webview_flutter and google_maps_flutter I can reproduce the failure locally for: https://github.com/flutter/packages/blob/main/packages/webview_flutter/webview_flutter_android/example/integration_test/webview_flutter_test.dart#L296 I manually merged this to keep https://github.com/flutter/engine/pull/50066. --- .../android/android_shell_holder_unittests.cc | 4 - .../android/image_external_texture.cc | 6 +- .../platform/android/image_external_texture.h | 1 + .../flutter/embedding/engine/FlutterJNI.java | 10 - .../engine/renderer/FlutterRenderer.java | 487 +++++++----------- .../SurfaceTextureSurfaceProducer.java | 5 - .../renderer/SurfaceTextureWrapper.java | 14 - .../platform/PlatformViewRenderTarget.java | 3 - .../plugin/platform/PlatformViewWrapper.java | 1 - ...rfaceProducerPlatformViewRenderTarget.java | 4 - .../io/flutter/view/TextureRegistry.java | 2 - .../shell/platform/android/jni/jni_mock.h | 5 - .../android/jni/platform_view_android_jni.h | 5 - .../android/platform_view_android_jni_impl.cc | 44 +- .../android/platform_view_android_jni_impl.h | 2 - .../surface_texture_external_texture.cc | 10 +- .../surface_texture_external_texture.h | 2 +- .../engine/renderer/FlutterRendererTest.java | 56 +- .../platform/PlatformViewsControllerTest.java | 2 - 19 files changed, 215 insertions(+), 448 deletions(-) diff --git a/engine/src/flutter/shell/platform/android/android_shell_holder_unittests.cc b/engine/src/flutter/shell/platform/android/android_shell_holder_unittests.cc index 2a7216a258a..2efa6660cd8 100644 --- a/engine/src/flutter/shell/platform/android/android_shell_holder_unittests.cc +++ b/engine/src/flutter/shell/platform/android/android_shell_holder_unittests.cc @@ -35,10 +35,6 @@ class MockPlatformViewAndroidJNI : public PlatformViewAndroidJNI { SurfaceTextureAttachToGLContext, (JavaLocalRef surface_texture, int textureId), (override)); - MOCK_METHOD(bool, - SurfaceTextureShouldUpdate, - (JavaLocalRef surface_texture), - (override)); MOCK_METHOD(void, SurfaceTextureUpdateTexImage, (JavaLocalRef surface_texture), diff --git a/engine/src/flutter/shell/platform/android/image_external_texture.cc b/engine/src/flutter/shell/platform/android/image_external_texture.cc index 87a8704c111..d045ca91ffd 100644 --- a/engine/src/flutter/shell/platform/android/image_external_texture.cc +++ b/engine/src/flutter/shell/platform/android/image_external_texture.cc @@ -26,9 +26,11 @@ void ImageExternalTexture::Paint(PaintContext& context, return; } Attach(context); - const bool should_process_frame = !freeze; + const bool should_process_frame = + (!freeze && new_frame_ready_) || dl_image_ == nullptr; if (should_process_frame) { ProcessFrame(context, bounds); + new_frame_ready_ = false; } if (dl_image_) { context.canvas->DrawImageRect( @@ -46,7 +48,7 @@ void ImageExternalTexture::Paint(PaintContext& context, // Implementing flutter::Texture. void ImageExternalTexture::MarkNewFrameAvailable() { - // NOOP. + new_frame_ready_ = true; } // Implementing flutter::Texture. diff --git a/engine/src/flutter/shell/platform/android/image_external_texture.h b/engine/src/flutter/shell/platform/android/image_external_texture.h index 251310cf537..3de077706ff 100644 --- a/engine/src/flutter/shell/platform/android/image_external_texture.h +++ b/engine/src/flutter/shell/platform/android/image_external_texture.h @@ -61,6 +61,7 @@ class ImageExternalTexture : public flutter::Texture { enum class AttachmentState { kUninitialized, kAttached, kDetached }; AttachmentState state_ = AttachmentState::kUninitialized; + bool new_frame_ready_ = false; sk_sp dl_image_; diff --git a/engine/src/flutter/shell/platform/android/io/flutter/embedding/engine/FlutterJNI.java b/engine/src/flutter/shell/platform/android/io/flutter/embedding/engine/FlutterJNI.java index 756298a777e..42eb25848e0 100644 --- a/engine/src/flutter/shell/platform/android/io/flutter/embedding/engine/FlutterJNI.java +++ b/engine/src/flutter/shell/platform/android/io/flutter/embedding/engine/FlutterJNI.java @@ -951,16 +951,6 @@ public class FlutterJNI { private native void nativeMarkTextureFrameAvailable(long nativeShellHolderId, long textureId); - /** Schedule the engine to draw a frame but does not invalidate the layout tree. */ - @UiThread - public void scheduleFrame() { - ensureRunningOnMainThread(); - ensureAttachedToNative(); - nativeScheduleFrame(nativeShellHolderId); - } - - private native void nativeScheduleFrame(long nativeShellHolderId); - /** * Unregisters a texture that was registered with {@link #registerTexture(long, * SurfaceTextureWrapper)}. diff --git a/engine/src/flutter/shell/platform/android/io/flutter/embedding/engine/renderer/FlutterRenderer.java b/engine/src/flutter/shell/platform/android/io/flutter/embedding/engine/renderer/FlutterRenderer.java index 5659a8612e4..a551cb19491 100644 --- a/engine/src/flutter/shell/platform/android/io/flutter/embedding/engine/renderer/FlutterRenderer.java +++ b/engine/src/flutter/shell/platform/android/io/flutter/embedding/engine/renderer/FlutterRenderer.java @@ -27,10 +27,8 @@ import java.io.IOException; import java.lang.ref.WeakReference; import java.nio.ByteBuffer; import java.util.ArrayList; -import java.util.HashMap; import java.util.HashSet; import java.util.Iterator; -import java.util.LinkedList; import java.util.List; import java.util.Set; import java.util.concurrent.atomic.AtomicLong; @@ -174,29 +172,21 @@ public class FlutterRenderer implements TextureRegistry { @NonNull @Override public SurfaceProducer createSurfaceProducer() { - // Prior to Impeller, Flutter on Android *only* ran on OpenGLES (via Skia). That - // meant that + // Prior to Impeller, Flutter on Android *only* ran on OpenGLES (via Skia). That meant that // plugins (i.e. end-users) either explicitly created a SurfaceTexture (via // createX/registerX) or an ImageTexture (via createX/registerX). // - // In an Impeller world, which for the first time uses (if available) a Vulkan - // rendering - // backend, it is no longer possible (at least not trivially) to render an - // OpenGLES-provided + // In an Impeller world, which for the first time uses (if available) a Vulkan rendering + // backend, it is no longer possible (at least not trivially) to render an OpenGLES-provided // texture (SurfaceTexture) in a Vulkan context. // - // This function picks the "best" rendering surface based on the Android - // runtime, and - // provides a consumer-agnostic SurfaceProducer (which in turn vends a Surface), - // and has - // plugins (i.e. end-users) use the Surface instead, letting us "hide" the - // consumer-side + // This function picks the "best" rendering surface based on the Android runtime, and + // provides a consumer-agnostic SurfaceProducer (which in turn vends a Surface), and has + // plugins (i.e. end-users) use the Surface instead, letting us "hide" the consumer-side // of the implementation. // - // tl;dr: If ImageTexture is available, we use it, otherwise we use a - // SurfaceTexture. - // Coincidentally, if ImageTexture is available, we are also on an Android - // version that is + // tl;dr: If ImageTexture is available, we use it, otherwise we use a SurfaceTexture. + // Coincidentally, if ImageTexture is available, we are also on an Android version that is // running Vulkan, so we don't have to worry about it not being supported. final long id = nextTextureId.getAndIncrement(); final SurfaceProducer entry; @@ -312,8 +302,7 @@ public class FlutterRenderer implements TextureRegistry { // mNativeView==null. return; } - textureWrapper.markDirty(); - scheduleEngineFrame(); + markTextureFrameAvailable(id); }; if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP) { // The callback relies on being executed on the UI thread (unsynchronised read @@ -415,10 +404,6 @@ public class FlutterRenderer implements TextureRegistry { } } - // Keep a queue of ImageReaders. - // Each ImageReader holds acquired Images. - // When we acquire the next image, close any ImageReaders that don't have any - // more pending images. @Keep @TargetApi(29) final class ImageReaderSurfaceProducer @@ -426,204 +411,204 @@ public class FlutterRenderer implements TextureRegistry { private static final String TAG = "ImageReaderSurfaceProducer"; private static final int MAX_IMAGES = 4; - // Flip when debugging to see verbose logs. - private static final boolean VERBOSE_LOGS = false; - private final long id; private boolean released; - // Will be true in tests and on Android API < 33. private boolean ignoringFence = false; // The requested width and height are updated by setSize. private int requestedWidth = 1; private int requestedHeight = 1; - // Whenever the requested width and height change we set this to be true so we - // create a new ImageReader (inside getSurface) with the correct width and height. - // We use this flag so that we lazily create the ImageReader only when a frame - // will be produced at that size. - private boolean createNewReader = true; - // State held to track latency of various stages. - private long lastDequeueTime = 0; - private long lastQueueTime = 0; - private long lastScheduleTime = 0; - - private Object lock = new Object(); - // REQUIRED: The following fields must only be accessed when lock is held. - private final LinkedList imageReaderQueue = new LinkedList(); - private final HashMap perImageReaders = - new HashMap(); - - /** Internal class: state held per Image produced by ImageReaders. */ + /** Internal class: state held per image produced by image readers. */ private class PerImage { - public final Image image; - public final long queuedTime; - - public PerImage(Image image, long queuedTime) { - this.image = image; - this.queuedTime = queuedTime; - } - } - - /** Internal class: state held per ImageReader. */ - private class PerImageReader { public final ImageReader reader; - private final LinkedList imageQueue = new LinkedList(); + public final Image image; - private final ImageReader.OnImageAvailableListener onImageAvailableListener = - reader -> { - Image image = null; - try { - image = reader.acquireLatestImage(); - } catch (IllegalStateException e) { - Log.e(TAG, "onImageAvailable acquireLatestImage failed: " + e); - } - if (released) { - return; - } - if (image == null) { - return; - } - onImage(reader, image); - }; - - public PerImageReader(ImageReader reader) { + public PerImage(ImageReader reader, Image image) { this.reader = reader; - reader.setOnImageAvailableListener(onImageAvailableListener, new Handler()); + this.image = image; } - PerImage queueImage(Image image) { - PerImage perImage = new PerImage(image, System.nanoTime()); - imageQueue.add(perImage); - // If we fall too far behind we will skip some frames. - while (imageQueue.size() > 2) { - PerImage r = imageQueue.removeFirst(); - r.image.close(); - } - return perImage; - } - - PerImage dequeueImage() { - if (imageQueue.size() == 0) { - return null; - } - PerImage r = imageQueue.removeFirst(); - return r; - } - - /** returns true if we can prune this reader */ - boolean canPrune() { - return imageQueue.size() == 0; - } - - void close() { - reader.close(); - imageQueue.clear(); + /** Call close when you are done with an the image. */ + public void close() { + this.image.close(); + maybeCloseReader(reader); } } - double deltaMillis(long deltaNanos) { - double ms = (double) deltaNanos / (double) 1000000.0; - return ms; - } + // Active image reader. + private ImageReader activeReader; + // Set of image readers that should be closed. + private final Set readersToClose = new HashSet<>(); + // Last image produced. We keep this around until a new image is produced or the + // consumer consumes this image. + private PerImage lastProducedImage; + // Last image consumed. We only close this at the next image consumption point to avoid + // a race condition with the raster thread accessing an image we closed. + private PerImage lastConsumedImage; - PerImageReader getOrCreatePerImageReader(ImageReader reader) { - PerImageReader r = perImageReaders.get(reader); - if (r == null) { - r = new PerImageReader(reader); - perImageReaders.put(reader, r); - imageReaderQueue.add(r); - } - return r; - } - - void pruneImageReaderQueue() { - // Prune nodes from the head of the ImageReader queue. - while (imageReaderQueue.size() > 1) { - PerImageReader r = imageReaderQueue.peekFirst(); - if (!r.canPrune()) { - // No more ImageReaders can be pruned this round. - break; - } - imageReaderQueue.removeFirst(); - perImageReaders.remove(r.reader); - r.close(); - } - } - - void onImage(ImageReader reader, Image image) { - PerImage queuedImage = null; - synchronized (lock) { - PerImageReader perReader = getOrCreatePerImageReader(reader); - queuedImage = perReader.queueImage(image); - } - if (queuedImage == null) { - return; - } - if (VERBOSE_LOGS) { - if (lastQueueTime != 0) { - long now = System.nanoTime(); - long queueDelta = now - lastQueueTime; - Log.i( - TAG, - "enqueued image=" - + queuedImage.image.hashCode() - + " queueDelta=" - + deltaMillis(queueDelta)); - lastQueueTime = now; - } else { - lastQueueTime = System.nanoTime(); - } - } - scheduleEngineFrame(); - } - - PerImage dequeueImage() { - PerImage r = null; - synchronized (lock) { - for (PerImageReader reader : imageReaderQueue) { - r = reader.dequeueImage(); - if (r == null) { - // This reader is probably about to get pruned. - continue; + private final Handler onImageAvailableHandler = new Handler(); + private final ImageReader.OnImageAvailableListener onImageAvailableListener = + reader -> { + Image image = null; + try { + image = reader.acquireLatestImage(); + } catch (IllegalStateException e) { + Log.e(TAG, "onImageAvailable acquireLatestImage failed: " + e); } - if (VERBOSE_LOGS) { - if (lastDequeueTime != 0) { - long now = System.nanoTime(); - long dequeueDelta = now - lastDequeueTime; - long queuedFor = now - r.queuedTime; - long scheduleDelay = now - lastScheduleTime; - Log.i( - TAG, - "dequeued image=" - + r.image.hashCode() - + " queuedFor= " - + deltaMillis(queuedFor) - + " dequeueDelta=" - + deltaMillis(dequeueDelta) - + " scheduleDelay=" - + deltaMillis(scheduleDelay)); - lastDequeueTime = now; - } else { - lastDequeueTime = System.nanoTime(); - } + if (image == null) { + return; } - // We have dequeued the first image. - break; - } - pruneImageReaderQueue(); - } - return r; + onImage(new PerImage(reader, image)); + }; + + ImageReaderSurfaceProducer(long id) { + this.id = id; + } + + @Override + public long id() { + return id; } private void releaseInternal() { released = true; - for (PerImageReader pir : perImageReaders.values()) { - pir.close(); + if (this.lastProducedImage != null) { + this.lastProducedImage.close(); + this.lastProducedImage = null; } - perImageReaders.clear(); - imageReaderQueue.clear(); + if (this.lastConsumedImage != null) { + this.lastConsumedImage.close(); + this.lastConsumedImage = null; + } + for (ImageReader reader : readersToClose) { + reader.close(); + } + readersToClose.clear(); + if (this.activeReader != null) { + this.activeReader.close(); + this.activeReader = null; + } + } + + @Override + public void release() { + if (released) { + return; + } + releaseInternal(); + unregisterTexture(id); + } + + @Override + public void setSize(int width, int height) { + // Clamp to a minimum of 1. A 0x0 texture is a runtime exception in ImageReader. + width = Math.max(1, width); + height = Math.max(1, height); + if (requestedWidth == width && requestedHeight == height) { + // No size change. + return; + } + this.requestedHeight = height; + this.requestedWidth = width; + synchronized (this) { + if (this.activeReader != null) { + // Schedule the old activeReader to be closed. + readersToClose.add(this.activeReader); + this.activeReader = null; + } + } + } + + @Override + public int getWidth() { + return this.requestedWidth; + } + + @Override + public int getHeight() { + return this.requestedHeight; + } + + @Override + public Surface getSurface() { + maybeCreateReader(); + return activeReader.getSurface(); + } + + @Override + @TargetApi(29) + public Image acquireLatestImage() { + PerImage r; + PerImage toClose; + synchronized (this) { + r = this.lastProducedImage; + this.lastProducedImage = null; + toClose = this.lastConsumedImage; + this.lastConsumedImage = r; + } + if (toClose != null) { + toClose.close(); + } + if (r == null) { + return null; + } + maybeWaitOnFence(r.image); + return r.image; + } + + private void maybeCloseReader(ImageReader reader) { + synchronized (this) { + if (!readersToClose.contains(reader)) { + return; + } + if (this.lastConsumedImage != null && this.lastConsumedImage.reader == reader) { + // There is still a consumed image in flight for this reader. Don't close. + return; + } + if (this.lastProducedImage != null && this.lastProducedImage.reader == reader) { + // There is still a pending image for this reader. Don't close. + return; + } + readersToClose.remove(reader); + } + // Close the reader. + reader.close(); + } + + private void maybeCreateReader() { + synchronized (this) { + if (this.activeReader != null) { + return; + } + this.activeReader = createImageReader(); + } + } + + /** Invoked for each method that is available. */ + private void onImage(PerImage image) { + if (released) { + return; + } + PerImage toClose; + synchronized (this) { + if (this.readersToClose.contains(image.reader)) { + Log.i(TAG, "Skipped frame because resize is in flight."); + image.close(); + return; + } + toClose = this.lastProducedImage; + this.lastProducedImage = image; + } + // Close the previously pushed buffer. + if (toClose != null) { + Log.i(TAG, "Dropping rendered frame that was not acquired in time."); + toClose.close(); + } + // Mark that we have a new frame available. Eventually the raster thread will + // call acquireLatestImage. + markTextureFrameAvailable(id); } @TargetApi(33) @@ -656,90 +641,6 @@ public class FlutterRenderer implements TextureRegistry { Log.w(TAG, "ImageTextureEntry can't wait on the fence on Android < 33"); } - ImageReaderSurfaceProducer(long id) { - this.id = id; - } - - @Override - public long id() { - return id; - } - - @Override - public void release() { - if (released) { - return; - } - releaseInternal(); - unregisterTexture(id); - } - - @Override - public void setSize(int width, int height) { - // Clamp to a minimum of 1. A 0x0 texture is a runtime exception in ImageReader. - width = Math.max(1, width); - height = Math.max(1, height); - - if (requestedWidth == width && requestedHeight == height) { - // No size change. - return; - } - this.createNewReader = true; - this.requestedHeight = height; - this.requestedWidth = width; - } - - @Override - public int getWidth() { - return this.requestedWidth; - } - - @Override - public int getHeight() { - return this.requestedHeight; - } - - @Override - public Surface getSurface() { - PerImageReader pir = getActiveReader(); - return pir.reader.getSurface(); - } - - @Override - public void scheduleFrame() { - if (VERBOSE_LOGS) { - long now = System.nanoTime(); - if (lastScheduleTime != 0) { - long delta = now - lastScheduleTime; - Log.v(TAG, "scheduleFrame delta=" + deltaMillis(delta)); - } - lastScheduleTime = now; - } - scheduleEngineFrame(); - } - - @Override - @TargetApi(29) - public Image acquireLatestImage() { - PerImage r = dequeueImage(); - if (r == null) { - return null; - } - maybeWaitOnFence(r.image); - return r.image; - } - - private PerImageReader getActiveReader() { - synchronized (lock) { - if (createNewReader) { - createNewReader = false; - // Create a new ImageReader and add it to the queue. - return getOrCreatePerImageReader(createImageReader()); - } - return imageReaderQueue.peekLast(); - } - } - @Override protected void finalize() throws Throwable { try { @@ -772,6 +673,7 @@ public class FlutterRenderer implements TextureRegistry { // Hint that consumed images will only be read by GPU. builder.setUsage(HardwareBuffer.USAGE_GPU_SAMPLED_IMAGE); final ImageReader reader = builder.build(); + reader.setOnImageAvailableListener(this.onImageAvailableListener, onImageAvailableHandler); return reader; } @@ -784,6 +686,7 @@ public class FlutterRenderer implements TextureRegistry { ImageFormat.PRIVATE, MAX_IMAGES, HardwareBuffer.USAGE_GPU_SAMPLED_IMAGE); + reader.setOnImageAvailableListener(this.onImageAvailableListener, onImageAvailableHandler); return reader; } @@ -804,21 +707,8 @@ public class FlutterRenderer implements TextureRegistry { } @VisibleForTesting - public int numImageReaders() { - synchronized (lock) { - return imageReaderQueue.size(); - } - } - - @VisibleForTesting - public int numImages() { - int r = 0; - synchronized (lock) { - for (PerImageReader reader : imageReaderQueue) { - r += reader.imageQueue.size(); - } - } - return r; + public int readersToCloseSize() { + return readersToClose.size(); } } @@ -871,7 +761,8 @@ public class FlutterRenderer implements TextureRegistry { toClose.close(); } if (image != null) { - scheduleEngineFrame(); + // Mark that we have a new frame available. + markTextureFrameAvailable(id); } } @@ -1136,10 +1027,6 @@ public class FlutterRenderer implements TextureRegistry { flutterJNI.registerImageTexture(textureId, imageTexture); } - private void scheduleEngineFrame() { - flutterJNI.scheduleFrame(); - } - // TODO(mattcarroll): describe the native behavior that this invokes private void markTextureFrameAvailable(long textureId) { flutterJNI.markTextureFrameAvailable(textureId); diff --git a/engine/src/flutter/shell/platform/android/io/flutter/embedding/engine/renderer/SurfaceTextureSurfaceProducer.java b/engine/src/flutter/shell/platform/android/io/flutter/embedding/engine/renderer/SurfaceTextureSurfaceProducer.java index 8153fc55826..50205f51d54 100644 --- a/engine/src/flutter/shell/platform/android/io/flutter/embedding/engine/renderer/SurfaceTextureSurfaceProducer.java +++ b/engine/src/flutter/shell/platform/android/io/flutter/embedding/engine/renderer/SurfaceTextureSurfaceProducer.java @@ -81,9 +81,4 @@ final class SurfaceTextureSurfaceProducer } return surface; } - - @Override - public void scheduleFrame() { - flutterJNI.markTextureFrameAvailable(id); - } } diff --git a/engine/src/flutter/shell/platform/android/io/flutter/embedding/engine/renderer/SurfaceTextureWrapper.java b/engine/src/flutter/shell/platform/android/io/flutter/embedding/engine/renderer/SurfaceTextureWrapper.java index a4468a58d31..fcd6c1694c6 100644 --- a/engine/src/flutter/shell/platform/android/io/flutter/embedding/engine/renderer/SurfaceTextureWrapper.java +++ b/engine/src/flutter/shell/platform/android/io/flutter/embedding/engine/renderer/SurfaceTextureWrapper.java @@ -22,7 +22,6 @@ public class SurfaceTextureWrapper { private boolean released; private boolean attached; private Runnable onFrameConsumed; - private boolean newFrameAvailable = false; public SurfaceTextureWrapper(@NonNull SurfaceTexture surfaceTexture) { this(surfaceTexture, null); @@ -48,23 +47,10 @@ public class SurfaceTextureWrapper { return surfaceTexture; } - public void markDirty() { - synchronized (this) { - newFrameAvailable = true; - } - } - - public boolean shouldUpdate() { - synchronized (this) { - return newFrameAvailable; - } - } - // Called by native. @SuppressWarnings("unused") public void updateTexImage() { synchronized (this) { - newFrameAvailable = false; if (!released) { surfaceTexture.updateTexImage(); if (onFrameConsumed != null) { diff --git a/engine/src/flutter/shell/platform/android/io/flutter/plugin/platform/PlatformViewRenderTarget.java b/engine/src/flutter/shell/platform/android/io/flutter/plugin/platform/PlatformViewRenderTarget.java index 7cb6c7a7840..d04240374c3 100644 --- a/engine/src/flutter/shell/platform/android/io/flutter/plugin/platform/PlatformViewRenderTarget.java +++ b/engine/src/flutter/shell/platform/android/io/flutter/plugin/platform/PlatformViewRenderTarget.java @@ -32,7 +32,4 @@ public interface PlatformViewRenderTarget { // Returns the Surface to be rendered on to. public Surface getSurface(); - - // Schedules a frame to be drawn. - public default void scheduleFrame() {} } diff --git a/engine/src/flutter/shell/platform/android/io/flutter/plugin/platform/PlatformViewWrapper.java b/engine/src/flutter/shell/platform/android/io/flutter/plugin/platform/PlatformViewWrapper.java index 9fac8a553eb..9ffb59877eb 100644 --- a/engine/src/flutter/shell/platform/android/io/flutter/plugin/platform/PlatformViewWrapper.java +++ b/engine/src/flutter/shell/platform/android/io/flutter/plugin/platform/PlatformViewWrapper.java @@ -167,7 +167,6 @@ public class PlatformViewWrapper extends FrameLayout { // Override the canvas that this subtree of views will use to draw. super.draw(targetCanvas); } finally { - renderTarget.scheduleFrame(); targetSurface.unlockCanvasAndPost(targetCanvas); } } diff --git a/engine/src/flutter/shell/platform/android/io/flutter/plugin/platform/SurfaceProducerPlatformViewRenderTarget.java b/engine/src/flutter/shell/platform/android/io/flutter/plugin/platform/SurfaceProducerPlatformViewRenderTarget.java index b37b53f1561..430bad3c89d 100644 --- a/engine/src/flutter/shell/platform/android/io/flutter/plugin/platform/SurfaceProducerPlatformViewRenderTarget.java +++ b/engine/src/flutter/shell/platform/android/io/flutter/plugin/platform/SurfaceProducerPlatformViewRenderTarget.java @@ -48,8 +48,4 @@ public class SurfaceProducerPlatformViewRenderTarget implements PlatformViewRend public Surface getSurface() { return this.producer.getSurface(); } - - public void scheduleFrame() { - this.producer.scheduleFrame(); - } } diff --git a/engine/src/flutter/shell/platform/android/io/flutter/view/TextureRegistry.java b/engine/src/flutter/shell/platform/android/io/flutter/view/TextureRegistry.java index add6cebaeac..7387e39a379 100644 --- a/engine/src/flutter/shell/platform/android/io/flutter/view/TextureRegistry.java +++ b/engine/src/flutter/shell/platform/android/io/flutter/view/TextureRegistry.java @@ -94,8 +94,6 @@ public interface TextureRegistry { * @return a Surface to use for a drawing target for various APIs. */ Surface getSurface(); - - void scheduleFrame(); }; /** A registry entry for a managed SurfaceTexture. */ diff --git a/engine/src/flutter/shell/platform/android/jni/jni_mock.h b/engine/src/flutter/shell/platform/android/jni/jni_mock.h index dd6c9737eae..20c64317dc5 100644 --- a/engine/src/flutter/shell/platform/android/jni/jni_mock.h +++ b/engine/src/flutter/shell/platform/android/jni/jni_mock.h @@ -49,11 +49,6 @@ class JNIMock final : public PlatformViewAndroidJNI { (JavaLocalRef surface_texture, int textureId), (override)); - MOCK_METHOD(bool, - SurfaceTextureShouldUpdate, - (JavaLocalRef surface_texture), - (override)); - MOCK_METHOD(void, SurfaceTextureUpdateTexImage, (JavaLocalRef surface_texture), diff --git a/engine/src/flutter/shell/platform/android/jni/platform_view_android_jni.h b/engine/src/flutter/shell/platform/android/jni/platform_view_android_jni.h index dcce4fd569e..54aaa814fea 100644 --- a/engine/src/flutter/shell/platform/android/jni/platform_view_android_jni.h +++ b/engine/src/flutter/shell/platform/android/jni/platform_view_android_jni.h @@ -91,11 +91,6 @@ class PlatformViewAndroidJNI { virtual void SurfaceTextureAttachToGLContext(JavaLocalRef surface_texture, int textureId) = 0; - //---------------------------------------------------------------------------- - /// @brief Returns true if surface_texture should be updated. - /// - virtual bool SurfaceTextureShouldUpdate(JavaLocalRef surface_texture) = 0; - //---------------------------------------------------------------------------- /// @brief Updates the texture image to the most recent frame from the /// image stream. diff --git a/engine/src/flutter/shell/platform/android/platform_view_android_jni_impl.cc b/engine/src/flutter/shell/platform/android/platform_view_android_jni_impl.cc index a928fb08cc2..4fe0a585290 100644 --- a/engine/src/flutter/shell/platform/android/platform_view_android_jni_impl.cc +++ b/engine/src/flutter/shell/platform/android/platform_view_android_jni_impl.cc @@ -111,8 +111,6 @@ static jmethodID g_java_weak_reference_get_method = nullptr; static jmethodID g_attach_to_gl_context_method = nullptr; -static jmethodID g_surface_texture_wrapper_should_update = nullptr; - static jmethodID g_update_tex_image_method = nullptr; static jmethodID g_get_transform_matrix_method = nullptr; @@ -523,10 +521,6 @@ static void MarkTextureFrameAvailable(JNIEnv* env, static_cast(texture_id)); } -static void ScheduleFrame(JNIEnv* env, jobject jcaller, jlong shell_holder) { - ANDROID_SHELL_HOLDER->GetPlatformView()->ScheduleFrame(); -} - static void InvokePlatformMessageResponseCallback(JNIEnv* env, jobject jcaller, jlong shell_holder, @@ -801,16 +795,12 @@ bool RegisterApi(JNIEnv* env) { .signature = "(JJ)V", .fnPtr = reinterpret_cast(&MarkTextureFrameAvailable), }, - { - .name = "nativeScheduleFrame", - .signature = "(J)V", - .fnPtr = reinterpret_cast(&ScheduleFrame), - }, { .name = "nativeUnregisterTexture", .signature = "(JJ)V", .fnPtr = reinterpret_cast(&UnregisterTexture), }, + // Methods for Dart callback functionality. { .name = "nativeLookupCallbackInformation", @@ -1169,15 +1159,6 @@ bool PlatformViewAndroid::Register(JNIEnv* env) { return false; } - g_surface_texture_wrapper_should_update = - env->GetMethodID(g_texture_wrapper_class->obj(), "shouldUpdate", "()Z"); - - if (g_surface_texture_wrapper_should_update == nullptr) { - FML_LOG(ERROR) - << "Could not locate SurfaceTextureWrapper.shouldUpdate method"; - return false; - } - g_update_tex_image_method = env->GetMethodID(g_texture_wrapper_class->obj(), "updateTexImage", "()V"); @@ -1472,29 +1453,6 @@ void PlatformViewAndroidJNIImpl::SurfaceTextureAttachToGLContext( FML_CHECK(fml::jni::CheckException(env)); } -bool PlatformViewAndroidJNIImpl::SurfaceTextureShouldUpdate( - JavaLocalRef surface_texture) { - JNIEnv* env = fml::jni::AttachCurrentThread(); - - if (surface_texture.is_null()) { - return false; - } - - fml::jni::ScopedJavaLocalRef surface_texture_local_ref( - env, env->CallObjectMethod(surface_texture.obj(), - g_java_weak_reference_get_method)); - if (surface_texture_local_ref.is_null()) { - return false; - } - - jboolean shouldUpdate = env->CallBooleanMethod( - surface_texture_local_ref.obj(), g_surface_texture_wrapper_should_update); - - FML_CHECK(fml::jni::CheckException(env)); - - return shouldUpdate; -} - void PlatformViewAndroidJNIImpl::SurfaceTextureUpdateTexImage( JavaLocalRef surface_texture) { JNIEnv* env = fml::jni::AttachCurrentThread(); diff --git a/engine/src/flutter/shell/platform/android/platform_view_android_jni_impl.h b/engine/src/flutter/shell/platform/android/platform_view_android_jni_impl.h index 5cf432559f9..0ca073dab33 100644 --- a/engine/src/flutter/shell/platform/android/platform_view_android_jni_impl.h +++ b/engine/src/flutter/shell/platform/android/platform_view_android_jni_impl.h @@ -45,8 +45,6 @@ class PlatformViewAndroidJNIImpl final : public PlatformViewAndroidJNI { void SurfaceTextureAttachToGLContext(JavaLocalRef surface_texture, int textureId) override; - bool SurfaceTextureShouldUpdate(JavaLocalRef surface_texture) override; - void SurfaceTextureUpdateTexImage(JavaLocalRef surface_texture) override; void SurfaceTextureGetTransformMatrix(JavaLocalRef surface_texture, diff --git a/engine/src/flutter/shell/platform/android/surface_texture_external_texture.cc b/engine/src/flutter/shell/platform/android/surface_texture_external_texture.cc index f0bddae496e..c02387276e4 100644 --- a/engine/src/flutter/shell/platform/android/surface_texture_external_texture.cc +++ b/engine/src/flutter/shell/platform/android/surface_texture_external_texture.cc @@ -37,7 +37,7 @@ void SurfaceTextureExternalTexture::OnGrContextCreated() { } void SurfaceTextureExternalTexture::MarkNewFrameAvailable() { - // NOOP. + new_frame_ready_ = true; } void SurfaceTextureExternalTexture::Paint(PaintContext& context, @@ -48,9 +48,10 @@ void SurfaceTextureExternalTexture::Paint(PaintContext& context, return; } const bool should_process_frame = - !freeze || ShouldUpdate() || dl_image_ == nullptr; + (!freeze && new_frame_ready_) || dl_image_ == nullptr; if (should_process_frame) { ProcessFrame(context, bounds); + new_frame_ready_ = false; } FML_CHECK(state_ == AttachmentState::kAttached); @@ -110,11 +111,6 @@ void SurfaceTextureExternalTexture::Attach(int gl_tex_id) { state_ = AttachmentState::kAttached; } -bool SurfaceTextureExternalTexture::ShouldUpdate() { - return jni_facade_->SurfaceTextureShouldUpdate( - fml::jni::ScopedJavaLocalRef(surface_texture_)); -} - void SurfaceTextureExternalTexture::Update() { jni_facade_->SurfaceTextureUpdateTexImage( fml::jni::ScopedJavaLocalRef(surface_texture_)); diff --git a/engine/src/flutter/shell/platform/android/surface_texture_external_texture.h b/engine/src/flutter/shell/platform/android/surface_texture_external_texture.h index e3400a1a649..de4210dd4e9 100644 --- a/engine/src/flutter/shell/platform/android/surface_texture_external_texture.h +++ b/engine/src/flutter/shell/platform/android/surface_texture_external_texture.h @@ -40,7 +40,6 @@ class SurfaceTextureExternalTexture : public flutter::Texture { virtual void Detach(); void Attach(int gl_tex_id); - bool ShouldUpdate(); void Update(); enum class AttachmentState { kUninitialized, kAttached, kDetached }; @@ -49,6 +48,7 @@ class SurfaceTextureExternalTexture : public flutter::Texture { fml::jni::ScopedJavaGlobalRef surface_texture_; AttachmentState state_ = AttachmentState::kUninitialized; SkMatrix transform_; + bool new_frame_ready_ = false; sk_sp dl_image_; FML_DISALLOW_COPY_AND_ASSIGN(SurfaceTextureExternalTexture); diff --git a/engine/src/flutter/shell/platform/android/test/io/flutter/embedding/engine/renderer/FlutterRendererTest.java b/engine/src/flutter/shell/platform/android/test/io/flutter/embedding/engine/renderer/FlutterRendererTest.java index 8cfb46ebaa3..682b0276128 100644 --- a/engine/src/flutter/shell/platform/android/test/io/flutter/embedding/engine/renderer/FlutterRendererTest.java +++ b/engine/src/flutter/shell/platform/android/test/io/flutter/embedding/engine/renderer/FlutterRendererTest.java @@ -511,7 +511,7 @@ public class FlutterRendererTest { } @Test - public void ImageReaderSurfaceProducerDoesNotDropFramesWhenResizeInflight() { + public void ImageReaderSurfaceProducerSkipsFramesWhenResizeInflight() { FlutterRenderer flutterRenderer = new FlutterRenderer(fakeFlutterJNI); FlutterRenderer.ImageReaderSurfaceProducer texture = flutterRenderer.new ImageReaderSurfaceProducer(0); @@ -533,15 +533,15 @@ public class FlutterRendererTest { // Resize. texture.setSize(4, 4); - // Let callbacks run. The rendered frame will manifest here. + // Let callbacks run. shadowOf(Looper.getMainLooper()).idle(); - // We acquired the frame produced above. - assertNotNull(texture.acquireLatestImage()); + // We should not get a new frame because the produced frame was for the previous size. + assertNull(texture.acquireLatestImage()); } @Test - public void ImageReaderSurfaceProducerImageReadersAndImagesCount() { + public void ImageReaderSurfaceProducerHandlesLateFrameWhenResizeInflight() { FlutterRenderer flutterRenderer = new FlutterRenderer(fakeFlutterJNI); FlutterRenderer.ImageReaderSurfaceProducer texture = flutterRenderer.new ImageReaderSurfaceProducer(0); @@ -563,9 +563,6 @@ public class FlutterRendererTest { // Let callbacks run, this will produce a single frame. shadowOf(Looper.getMainLooper()).idle(); - assertEquals(1, texture.numImageReaders()); - assertEquals(1, texture.numImages()); - // Resize. texture.setSize(4, 4); @@ -577,8 +574,15 @@ public class FlutterRendererTest { // Let callbacks run. shadowOf(Looper.getMainLooper()).idle(); - assertEquals(1, texture.numImageReaders()); - assertEquals(2, texture.numImages()); + // We will acquire a frame that is the old size. + Image produced = texture.acquireLatestImage(); + assertNotNull(produced); + assertEquals(produced.getWidth(), 1); + assertEquals(produced.getHeight(), 1); + + // We will still have one pending reader to be closed. + // If we didn't we would have closed the reader that owned the image we just acquired. + assertEquals(1, texture.readersToCloseSize()); // Render a new frame with the current size. surface = texture.getSurface(); @@ -590,35 +594,11 @@ public class FlutterRendererTest { // Let callbacks run. shadowOf(Looper.getMainLooper()).idle(); - assertEquals(2, texture.numImageReaders()); - assertEquals(3, texture.numImages()); + // Acquire the new image. + assertNotNull(texture.acquireLatestImage()); - // Acquire first frame. - Image produced = texture.acquireLatestImage(); - assertNotNull(produced); - assertEquals(1, produced.getWidth()); - assertEquals(1, produced.getHeight()); - assertEquals(2, texture.numImageReaders()); - assertEquals(2, texture.numImages()); - // Acquire second frame. This should result in closing the first ImageReader. - produced = texture.acquireLatestImage(); - assertNotNull(produced); - assertEquals(1, produced.getWidth()); - assertEquals(1, produced.getHeight()); - assertEquals(1, texture.numImageReaders()); - assertEquals(1, texture.numImages()); - // Acquire third frame. We should not close the active ImageReader. - produced = texture.acquireLatestImage(); - assertNotNull(produced); - assertEquals(4, produced.getWidth()); - assertEquals(4, produced.getHeight()); - assertEquals(1, texture.numImageReaders()); - assertEquals(0, texture.numImages()); - - // Returns null image when no more images are queued. - assertNull(texture.acquireLatestImage()); - assertEquals(1, texture.numImageReaders()); - assertEquals(0, texture.numImages()); + // We will have no pending readers to close. + assertEquals(0, texture.readersToCloseSize()); } // A 0x0 ImageReader is a runtime error. diff --git a/engine/src/flutter/shell/platform/android/test/io/flutter/plugin/platform/PlatformViewsControllerTest.java b/engine/src/flutter/shell/platform/android/test/io/flutter/plugin/platform/PlatformViewsControllerTest.java index c5871db27bf..81e264232b3 100644 --- a/engine/src/flutter/shell/platform/android/test/io/flutter/plugin/platform/PlatformViewsControllerTest.java +++ b/engine/src/flutter/shell/platform/android/test/io/flutter/plugin/platform/PlatformViewsControllerTest.java @@ -1614,8 +1614,6 @@ public class PlatformViewsControllerTest { public Surface getSurface() { return null; } - - public void scheduleFrame() {} }; } };