From 40786d52587cd3f0895521632b6b984159adb519 Mon Sep 17 00:00:00 2001
From: John McCutchan <john@johnmccutchan.com>
Date: Wed, 3 Jan 2024 09:41:14 -0800
Subject: [PATCH] Fix a crash in the new SurfaceProducer external texture

The following sequence of events would lead to a crash:

- Reader A is created.
- Reader A produces a frame (A0)
- Texture is resized.
- Reader B is created and reader A is scheduled to be closed.
- Reader A produces a frame (A1). This is skipped.
- Reader A is closed.
- Frame A0 is acquired.

Because we closed Reader A the frame A0 is invalid.

The fix is to not close Reader A when it is the last reader to produce a frame.

Fixes internal bug b/318458306
---
 .../engine/renderer/FlutterRenderer.java      | 13 +++-
 .../engine/renderer/FlutterRendererTest.java  | 67 +++++++++++++++++++
 2 files changed, 78 insertions(+), 2 deletions(-)

diff --git a/shell/platform/android/io/flutter/embedding/engine/renderer/FlutterRenderer.java b/shell/platform/android/io/flutter/embedding/engine/renderer/FlutterRenderer.java
index 34ea34086e8af..9fb0b12244d32 100644
--- a/shell/platform/android/io/flutter/embedding/engine/renderer/FlutterRenderer.java
+++ b/shell/platform/android/io/flutter/embedding/engine/renderer/FlutterRenderer.java
@@ -511,11 +511,15 @@ public Image acquireLatestImage() {
 
     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 (!readersToClose.contains(reader)) {
+        if (this.lastProducedImage != null && this.lastProducedImage.reader == reader) {
+          // There is still a pending image for this reader. Don't close.
           return;
         }
         readersToClose.remove(reader);
@@ -550,7 +554,7 @@ private void onImage(PerImage image) {
       }
       // Close the previously pushed buffer.
       if (toClose != null) {
-        Log.i(TAG, "Dropped frame.");
+        Log.i(TAG, "Dropping rendered frame that was not acquired in time.");
         toClose.close();
       }
       if (image != null) {
@@ -656,6 +660,11 @@ public void disableFenceForTest() {
       // Roboelectric's implementation of SyncFence is borked.
       ignoringFence = true;
     }
+
+    @VisibleForTesting
+    public int readersToCloseSize() {
+      return readersToClose.size();
+    }
   }
 
   @Keep
diff --git a/shell/platform/android/test/io/flutter/embedding/engine/renderer/FlutterRendererTest.java b/shell/platform/android/test/io/flutter/embedding/engine/renderer/FlutterRendererTest.java
index 05ab3777de2aa..3330d34f822f5 100644
--- a/shell/platform/android/test/io/flutter/embedding/engine/renderer/FlutterRendererTest.java
+++ b/shell/platform/android/test/io/flutter/embedding/engine/renderer/FlutterRendererTest.java
@@ -42,6 +42,12 @@ public class FlutterRendererTest {
   private Surface fakeSurface;
   private Surface fakeSurface2;
 
+  @Before
+  public void init() {
+    // Uncomment the following line to enable logging output in test.
+    // ShadowLog.stream = System.out;
+  }
+
   @Before
   public void setup() {
     fakeFlutterJNI = mock(FlutterJNI.class);
@@ -532,4 +538,65 @@ public void ImageReaderSurfaceProducerSkipsFramesWhenResizeInflight() {
     // We should not get a new frame because the produced frame was for the previous size.
     assertNull(texture.acquireLatestImage());
   }
+
+  @Test
+  public void ImageReaderSurfaceProducerHandlesLateFrameWhenResizeInflight() {
+    FlutterRenderer flutterRenderer = new FlutterRenderer(fakeFlutterJNI);
+    FlutterRenderer.ImageReaderSurfaceProducer texture =
+        flutterRenderer.new ImageReaderSurfaceProducer(0);
+    texture.disableFenceForTest();
+
+    // Returns a null image when one hasn't been produced.
+    assertNull(texture.acquireLatestImage());
+
+    // Give the texture an initial size.
+    texture.setSize(1, 1);
+
+    // Grab the surface so we can render a frame at 1x1 after resizing.
+    Surface surface = texture.getSurface();
+    assertNotNull(surface);
+    Canvas canvas = surface.lockHardwareCanvas();
+    canvas.drawARGB(255, 255, 0, 0);
+    surface.unlockCanvasAndPost(canvas);
+
+    // Let callbacks run, this will produce a single frame.
+    shadowOf(Looper.getMainLooper()).idle();
+
+    // Resize.
+    texture.setSize(4, 4);
+
+    // Render a frame at the old size (by using the pre-resized Surface)
+    canvas = surface.lockHardwareCanvas();
+    canvas.drawARGB(255, 255, 0, 0);
+    surface.unlockCanvasAndPost(canvas);
+
+    // Let callbacks run.
+    shadowOf(Looper.getMainLooper()).idle();
+
+    // 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();
+    assertNotNull(surface);
+    canvas = surface.lockHardwareCanvas();
+    canvas.drawARGB(255, 255, 0, 0);
+    surface.unlockCanvasAndPost(canvas);
+
+    // Let callbacks run.
+    shadowOf(Looper.getMainLooper()).idle();
+
+    // Acquire the new image.
+    assertNotNull(texture.acquireLatestImage());
+
+    // We will have no pending readers to close.
+    assertEquals(0, texture.readersToCloseSize());
+  }
 }