Fix ImageReader may leak images when onDraw() not called (flutter#24272)

Author: eggfly

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

@@ -23,8 +23,6 @@
 import io.flutter.embedding.engine.renderer.FlutterRenderer;
 import io.flutter.embedding.engine.renderer.RenderSurface;
 import java.nio.ByteBuffer;
-import java.util.LinkedList;
-import java.util.Queue;
 
 /**
  * Paints a Flutter UI provided by an {@link android.media.ImageReader} onto a {@link
@@ -41,7 +39,6 @@
 @TargetApi(19)
 public class FlutterImageView extends View implements RenderSurface {
   @NonNull private ImageReader imageReader;
-  @Nullable private Queue<Image> imageQueue;
   @Nullable private Image currentImage;
   @Nullable private Bitmap currentBitmap;
   @Nullable private FlutterRenderer flutterRenderer;
@@ -57,13 +54,6 @@ public enum SurfaceKind {
   /** The kind of surface. */
   private SurfaceKind kind;
 
-  /**
-   * The number of images acquired from the current {@link android.media.ImageReader} that are
-   * waiting to be painted. This counter is decreased after calling {@link
-   * android.media.Image#close()}.
-   */
-  private int pendingImages = 0;
-
   /** Whether the view is attached to the Flutter render. */
   private boolean isAttachedToFlutterRenderer = false;
 
@@ -89,7 +79,6 @@ public FlutterImageView(@NonNull Context context, @NonNull AttributeSet attrs) {
     super(context, null);
     this.imageReader = imageReader;
     this.kind = kind;
-    this.imageQueue = new LinkedList<>();
     init();
   }
 
@@ -155,22 +144,14 @@ public void detachFromRenderer() {
       return;
     }
     setAlpha(0.0f);
-    // Drop the lastest image as it shouldn't render this image if this view is
+    // Drop the latest image as it shouldn't render this image if this view is
     // attached to the renderer again.
     acquireLatestImage();
     // Clear drawings.
     currentBitmap = null;
 
-    // Close the images in the queue and clear the queue.
-    for (final Image image : imageQueue) {
-      image.close();
-    }
-    imageQueue.clear();
     // Close and clear the current image if any.
-    if (currentImage != null) {
-      currentImage.close();
-      currentImage = null;
-    }
+    closeCurrentImage();
     invalidate();
     isAttachedToFlutterRenderer = false;
   }
@@ -188,26 +169,20 @@ public boolean acquireLatestImage() {
     if (!isAttachedToFlutterRenderer) {
       return false;
     }
-    // There's no guarantee that the image will be closed before the next call to
-    // `acquireLatestImage()`. For example, the device may not produce new frames if
-    // it's in sleep mode, so the calls to `invalidate()` will be queued up
+    // 1. `acquireLatestImage()` may return null if no new image is available.
+    // 2. There's no guarantee that `onDraw()` is called after `invalidate()`.
+    // For example, the device may not produce new frames if it's in sleep mode
+    // or some special Android devices so the calls to `invalidate()` queued up
     // until the device produces a new frame.
-    //
-    // While the engine will also stop producing frames, there is a race condition.
-    //
-    // To avoid exceptions, check if a new image can be acquired.
-    int imageOpenedCount = imageQueue.size();
-    if (currentImage != null) {
-      imageOpenedCount++;
+    // 3. While the engine will also stop producing frames, there is a race condition.
+    final Image newImage = imageReader.acquireLatestImage();
+    if (newImage != null) {
+      // Only close current image after acquiring valid new image
+      closeCurrentImage();
+      currentImage = newImage;
+      invalidate();
     }
-    if (imageOpenedCount < imageReader.getMaxImages()) {
-      final Image image = imageReader.acquireLatestImage();
-      if (image != null) {
-        imageQueue.add(image);
-      }
-    }
-    invalidate();
-    return !imageQueue.isEmpty();
+    return newImage != null;
   }
 
   /** Creates a new image reader with the provided size. */
@@ -218,32 +193,36 @@ public void resizeIfNeeded(int width, int height) {
     if (width == imageReader.getWidth() && height == imageReader.getHeight()) {
       return;
     }
-    imageQueue.clear();
-    currentImage = null;
+
+    // Close resources.
+    closeCurrentImage();
+
     // Close all the resources associated with the image reader,
     // including the images.
     imageReader.close();
     // Image readers cannot be resized once created.
     imageReader = createImageReader(width, height);
-    pendingImages = 0;
   }
 
   @Override
   protected void onDraw(Canvas canvas) {
     super.onDraw(canvas);
-
-    if (!imageQueue.isEmpty()) {
-      if (currentImage != null) {
-        currentImage.close();
-      }
-      currentImage = imageQueue.poll();
+    if (currentImage != null) {
       updateCurrentBitmap();
     }
     if (currentBitmap != null) {
       canvas.drawBitmap(currentBitmap, 0, 0, null);
     }
   }
 
+  private void closeCurrentImage() {
+    // Close and clear the current image if any.
+    if (currentImage != null) {
+      currentImage.close();
+      currentImage = null;
+    }
+  }
+
   @TargetApi(29)
   private void updateCurrentBitmap() {
     if (android.os.Build.VERSION.SDK_INT >= 29) {

shell/platform/android/test/io/flutter/embedding/android/FlutterViewTest.java

@@ -658,36 +658,19 @@ public void flutterImageView_acquiresImageAndInvalidates() {
     verify(imageView, times(1)).invalidate();
   }
 
-  @Test
-  public void flutterImageView_acquireLatestImageReturnsFalse() {
-    final ImageReader mockReader = mock(ImageReader.class);
-    when(mockReader.getMaxImages()).thenReturn(2);
-
-    final FlutterImageView imageView =
-        spy(
-            new FlutterImageView(
-                RuntimeEnvironment.application,
-                mockReader,
-                FlutterImageView.SurfaceKind.background));
-
-    assertFalse(imageView.acquireLatestImage());
-
-    final FlutterJNI jni = mock(FlutterJNI.class);
-    imageView.attachToRenderer(new FlutterRenderer(jni));
-
-    when(mockReader.acquireLatestImage()).thenReturn(null);
-    assertFalse(imageView.acquireLatestImage());
-  }
-
   @Test
   @SuppressLint("WrongCall") /*View#onDraw*/
-  public void flutterImageView_acquiresMaxImagesAtMost() {
+  public void flutterImageView_acquiresImageClosesPreviousImageUnlessNoNewImage() {
     final ImageReader mockReader = mock(ImageReader.class);
     when(mockReader.getMaxImages()).thenReturn(3);
 
     final Image mockImage = mock(Image.class);
     when(mockImage.getPlanes()).thenReturn(new Plane[0]);
-    when(mockReader.acquireLatestImage()).thenReturn(mockImage);
+    // Mock no latest image on the second time
+    when(mockReader.acquireLatestImage())
+        .thenReturn(mockImage)
+        .thenReturn(null)
+        .thenReturn(mockImage);
 
     final FlutterImageView imageView =
         spy(
@@ -700,33 +683,27 @@ public void flutterImageView_acquiresMaxImagesAtMost() {
     imageView.attachToRenderer(new FlutterRenderer(jni));
     doNothing().when(imageView).invalidate();
 
-    assertTrue(imageView.acquireLatestImage()); // 1 image
-    assertTrue(imageView.acquireLatestImage()); // 2 images
-    assertTrue(imageView.acquireLatestImage()); // 3 images
-    assertTrue(imageView.acquireLatestImage()); // 3 images
-    verify(mockReader, times(3)).acquireLatestImage();
+    assertTrue(imageView.acquireLatestImage()); // No previous, acquire latest image
+    assertFalse(
+        imageView.acquireLatestImage()); // Mock no image when acquire, don't close, and assertFalse
+    assertTrue(imageView.acquireLatestImage()); // Acquire latest image and close previous
+    assertTrue(imageView.acquireLatestImage()); // Acquire latest image and close previous
+    assertTrue(imageView.acquireLatestImage()); // Acquire latest image and close previous
+    verify(mockImage, times(3)).close(); // Close 3 times
 
-    imageView.onDraw(mock(Canvas.class)); // 3 images
-    assertTrue(imageView.acquireLatestImage()); // 3 images
-    verify(mockReader, times(3)).acquireLatestImage();
+    imageView.onDraw(mock(Canvas.class)); // Draw latest image
 
-    imageView.onDraw(mock(Canvas.class)); // 2 images
-    assertTrue(imageView.acquireLatestImage()); // 3 images
-    verify(mockReader, times(4)).acquireLatestImage();
+    assertTrue(imageView.acquireLatestImage()); // acquire latest image and close previous
 
-    imageView.onDraw(mock(Canvas.class)); // 2 images
-    imageView.onDraw(mock(Canvas.class)); // 1 image
-    imageView.onDraw(mock(Canvas.class)); // 1 image
+    imageView.onDraw(mock(Canvas.class)); // Draw latest image
+    imageView.onDraw(mock(Canvas.class)); // Draw latest image
+    imageView.onDraw(mock(Canvas.class)); // Draw latest image
 
-    assertTrue(imageView.acquireLatestImage()); // 2 images
-    assertTrue(imageView.acquireLatestImage()); // 3 images
-    assertTrue(imageView.acquireLatestImage()); // 3 images
-    assertTrue(imageView.acquireLatestImage()); // 3 images
     verify(mockReader, times(6)).acquireLatestImage();
   }
 
   @Test
-  public void flutterImageView_detachFromRendererClosesAllImages() {
+  public void flutterImageView_detachFromRendererClosesPreviousImage() {
     final ImageReader mockReader = mock(ImageReader.class);
     when(mockReader.getMaxImages()).thenReturn(2);
 
@@ -746,54 +723,12 @@ public void flutterImageView_detachFromRendererClosesAllImages() {
     doNothing().when(imageView).invalidate();
     imageView.acquireLatestImage();
     imageView.acquireLatestImage();
-    imageView.detachFromRenderer();
-
-    verify(mockImage, times(2)).close();
-  }
-
-  @Test
-  @SuppressLint("WrongCall") /*View#onDraw*/
-  public void flutterImageView_onDrawClosesAllImages() {
-    final ImageReader mockReader = mock(ImageReader.class);
-    when(mockReader.getMaxImages()).thenReturn(2);
-
-    final Image mockImage = mock(Image.class);
-    when(mockImage.getPlanes()).thenReturn(new Plane[0]);
-    when(mockReader.acquireLatestImage()).thenReturn(mockImage);
-
-    final FlutterImageView imageView =
-        spy(
-            new FlutterImageView(
-                RuntimeEnvironment.application,
-                mockReader,
-                FlutterImageView.SurfaceKind.background));
-
-    final FlutterJNI jni = mock(FlutterJNI.class);
-    imageView.attachToRenderer(new FlutterRenderer(jni));
-
-    doNothing().when(imageView).invalidate();
-    imageView.acquireLatestImage();
-    imageView.acquireLatestImage();
-
-    imageView.onDraw(mock(Canvas.class));
-    imageView.onDraw(mock(Canvas.class));
-
-    // 1 image is closed and 1 is active.
-    verify(mockImage, times(1)).close();
-    verify(mockReader, times(2)).acquireLatestImage();
-
-    // This call doesn't do anything because there isn't
-    // an image in the queue.
-    imageView.onDraw(mock(Canvas.class));
     verify(mockImage, times(1)).close();
 
-    // Aquire another image and push it to the queue.
-    imageView.acquireLatestImage();
-    verify(mockReader, times(3)).acquireLatestImage();
-
-    // Then, the second image is closed.
-    imageView.onDraw(mock(Canvas.class));
-    verify(mockImage, times(2)).close();
+    imageView.detachFromRenderer();
+    // There's an acquireLatestImage() in detachFromRenderer(),
+    // so it will be 2 times called close() inside detachFromRenderer()
+    verify(mockImage, times(3)).close();
   }
 
   @Test