From 7b2e67e77aadc7bcc08596f0be0f9e0fb77845d9 Mon Sep 17 00:00:00 2001 From: Emmanuel Garcia Date: Thu, 27 Aug 2020 18:34:46 -0700 Subject: [PATCH 1/5] Ensure all images are closed in FlutterImageView --- .../embedding/android/FlutterImageView.java | 57 ++++++++------- .../embedding/android/FlutterViewTest.java | 71 +++++++++++++++++++ 2 files changed, 99 insertions(+), 29 deletions(-) diff --git a/shell/platform/android/io/flutter/embedding/android/FlutterImageView.java b/shell/platform/android/io/flutter/embedding/android/FlutterImageView.java index 46079d429f56c..dfa1095d04feb 100644 --- a/shell/platform/android/io/flutter/embedding/android/FlutterImageView.java +++ b/shell/platform/android/io/flutter/embedding/android/FlutterImageView.java @@ -4,7 +4,6 @@ package io.flutter.embedding.android; -import android.annotation.SuppressLint; import android.annotation.TargetApi; import android.content.Context; import android.graphics.Bitmap; @@ -22,6 +21,8 @@ import androidx.annotation.VisibleForTesting; import io.flutter.embedding.engine.renderer.FlutterRenderer; import io.flutter.embedding.engine.renderer.RenderSurface; +import java.util.LinkedList; +import java.util.Queue; /** * Paints a Flutter UI provided by an {@link android.media.ImageReader} onto a {@link @@ -35,11 +36,10 @@ * an {@link android.media.Image} and renders it to the {@link android.graphics.Canvas} in {@code * onDraw}. */ -@SuppressLint("ViewConstructor") @TargetApi(19) public class FlutterImageView extends View implements RenderSurface { @NonNull private ImageReader imageReader; - @Nullable private Image nextImage; + @Nullable private Queue imageQueue; @Nullable private Image currentImage; @Nullable private Bitmap currentBitmap; @Nullable private FlutterRenderer flutterRenderer; @@ -70,10 +70,7 @@ public enum SurfaceKind { * the Flutter UI. */ public FlutterImageView(@NonNull Context context, int width, int height, SurfaceKind kind) { - super(context, null); - this.imageReader = createImageReader(width, height); - this.kind = kind; - init(); + this(context, createImageReader(width, height), kind); } @VisibleForTesting @@ -81,6 +78,7 @@ public FlutterImageView(@NonNull Context context, int width, int height, Surface super(context, null); this.imageReader = imageReader; this.kind = kind; + this.imageQueue = new LinkedList<>(); init(); } @@ -150,12 +148,14 @@ public void detachFromRenderer() { // attached to the renderer again. acquireLatestImage(); // Clear drawings. - pendingImages = 0; currentBitmap = null; - if (nextImage != null) { - nextImage.close(); - nextImage = 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; @@ -168,7 +168,10 @@ public void pause() { // Not supported. } - /** Acquires the next image to be drawn to the {@link android.graphics.Canvas}. */ + /** + * Acquires the next image to be drawn to the {@link android.graphics.Canvas}. Returns true if + * there's an image available in the queue. + */ @TargetApi(19) public boolean acquireLatestImage() { if (!isAttachedToFlutterRenderer) { @@ -182,14 +185,14 @@ public boolean acquireLatestImage() { // While the engine will also stop producing frames, there is a race condition. // // To avoid exceptions, check if a new image can be acquired. - if (pendingImages < imageReader.getMaxImages()) { - nextImage = imageReader.acquireLatestImage(); - if (nextImage != null) { - pendingImages++; + if (imageQueue.size() < imageReader.getMaxImages()) { + final Image image = imageReader.acquireLatestImage(); + if (image != null) { + imageQueue.add(image); } } invalidate(); - return nextImage != null; + return !imageQueue.isEmpty(); } /** Creates a new image reader with the provided size. */ @@ -200,15 +203,12 @@ public void resizeIfNeeded(int width, int height) { if (width == imageReader.getWidth() && height == imageReader.getHeight()) { return; } - // Close resources. - if (nextImage != null) { - nextImage.close(); - nextImage = null; - } + imageQueue.clear(); if (currentImage != null) { - currentImage.close(); currentImage = null; } + // 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); @@ -218,16 +218,15 @@ public void resizeIfNeeded(int width, int height) { @Override protected void onDraw(Canvas canvas) { super.onDraw(canvas); - if (nextImage != null) { + + if (!imageQueue.isEmpty()) { if (currentImage != null) { currentImage.close(); - pendingImages--; + currentImage = null; } - currentImage = nextImage; - nextImage = null; + currentImage = imageQueue.poll(); updateCurrentBitmap(); } - if (currentBitmap != null) { canvas.drawBitmap(currentBitmap, 0, 0, null); } @@ -238,6 +237,7 @@ private void updateCurrentBitmap() { if (android.os.Build.VERSION.SDK_INT >= 29) { final HardwareBuffer buffer = currentImage.getHardwareBuffer(); currentBitmap = Bitmap.wrapHardwareBuffer(buffer, ColorSpace.get(ColorSpace.Named.SRGB)); + buffer.close(); } else { final Plane[] imagePlanes = currentImage.getPlanes(); if (imagePlanes.length != 1) { @@ -255,7 +255,6 @@ private void updateCurrentBitmap() { Bitmap.createBitmap( desiredWidth, desiredHeight, android.graphics.Bitmap.Config.ARGB_8888); } - currentBitmap.copyPixelsFromBuffer(imagePlane.getBuffer()); } } diff --git a/shell/platform/android/test/io/flutter/embedding/android/FlutterViewTest.java b/shell/platform/android/test/io/flutter/embedding/android/FlutterViewTest.java index 28c12f831af43..12ef93c2f4b4f 100644 --- a/shell/platform/android/test/io/flutter/embedding/android/FlutterViewTest.java +++ b/shell/platform/android/test/io/flutter/embedding/android/FlutterViewTest.java @@ -16,8 +16,10 @@ import android.content.Context; import android.content.res.Configuration; import android.content.res.Resources; +import android.graphics.Canvas; import android.graphics.Insets; import android.media.Image; +import android.media.Image.Plane; import android.media.ImageReader; import android.view.View; import android.view.ViewGroup; @@ -550,6 +552,75 @@ public void flutterImageView_acquiresMaxImagesAtMost() { verify(mockReader, times(2)).acquireLatestImage(); } + @Test + public void flutterImageView_detachFromRendererClosesAllImages() { + final ImageReader mockReader = mock(ImageReader.class); + when(mockReader.getMaxImages()).thenReturn(2); + + final Image mockImage = mock(Image.class); + 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.detachFromRenderer(); + + verify(mockImage, times(2)).close(); + } + + @Test + 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(); + } /* * A custom shadow that reports fullscreen flag for system UI visibility */ From fc81d61ce851e9b6486b9056afb3ca291292ad18 Mon Sep 17 00:00:00 2001 From: Emmanuel Garcia Date: Tue, 1 Sep 2020 10:23:42 -0700 Subject: [PATCH 2/5] Lint --- .../io/flutter/embedding/android/FlutterImageView.java | 8 ++++++++ .../io/flutter/embedding/android/FlutterViewTest.java | 8 ++++---- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/shell/platform/android/io/flutter/embedding/android/FlutterImageView.java b/shell/platform/android/io/flutter/embedding/android/FlutterImageView.java index dfa1095d04feb..497a111dab6e6 100644 --- a/shell/platform/android/io/flutter/embedding/android/FlutterImageView.java +++ b/shell/platform/android/io/flutter/embedding/android/FlutterImageView.java @@ -73,6 +73,14 @@ public FlutterImageView(@NonNull Context context, int width, int height, Surface this(context, createImageReader(width, height), kind); } + public FlutterImageView(@NonNull Context context) { + this(context, 1, 1, SurfaceKind.background); + } + + public FlutterImageView(@NonNull Context context, @NonNull AttributeSet attrs) { + this(context, 1, 1, SurfaceKind.background); + } + @VisibleForTesting FlutterImageView(@NonNull Context context, @NonNull ImageReader imageReader, SurfaceKind kind) { super(context, null); diff --git a/shell/platform/android/test/io/flutter/embedding/android/FlutterViewTest.java b/shell/platform/android/test/io/flutter/embedding/android/FlutterViewTest.java index 12ef93c2f4b4f..ad0d065b91684 100644 --- a/shell/platform/android/test/io/flutter/embedding/android/FlutterViewTest.java +++ b/shell/platform/android/test/io/flutter/embedding/android/FlutterViewTest.java @@ -601,8 +601,8 @@ public void flutterImageView_onDrawClosesAllImages() { imageView.acquireLatestImage(); imageView.acquireLatestImage(); - imageView.onDraw(mock(Canvas.class)); - imageView.onDraw(mock(Canvas.class)); + imageView.draw(mock(Canvas.class)); + imageView.draw(mock(Canvas.class)); // 1 image is closed and 1 is active. verify(mockImage, times(1)).close(); @@ -610,7 +610,7 @@ public void flutterImageView_onDrawClosesAllImages() { // This call doesn't do anything because there isn't // an image in the queue. - imageView.onDraw(mock(Canvas.class)); + imageView.draw(mock(Canvas.class)); verify(mockImage, times(1)).close(); // Aquire another image and push it to the queue. @@ -618,7 +618,7 @@ public void flutterImageView_onDrawClosesAllImages() { verify(mockReader, times(3)).acquireLatestImage(); // Then, the second image is closed. - imageView.onDraw(mock(Canvas.class)); + imageView.draw(mock(Canvas.class)); verify(mockImage, times(2)).close(); } /* From 43b9aee0fe35c3615e850e295157240bb09efb71 Mon Sep 17 00:00:00 2001 From: Emmanuel Garcia Date: Tue, 1 Sep 2020 10:23:54 -0700 Subject: [PATCH 3/5] Feedback --- .../android/io/flutter/embedding/android/FlutterImageView.java | 1 - 1 file changed, 1 deletion(-) diff --git a/shell/platform/android/io/flutter/embedding/android/FlutterImageView.java b/shell/platform/android/io/flutter/embedding/android/FlutterImageView.java index 497a111dab6e6..77074631e8ebf 100644 --- a/shell/platform/android/io/flutter/embedding/android/FlutterImageView.java +++ b/shell/platform/android/io/flutter/embedding/android/FlutterImageView.java @@ -230,7 +230,6 @@ protected void onDraw(Canvas canvas) { if (!imageQueue.isEmpty()) { if (currentImage != null) { currentImage.close(); - currentImage = null; } currentImage = imageQueue.poll(); updateCurrentBitmap(); From a43b79a74ad422841bd1ad49d7354a23877909e4 Mon Sep 17 00:00:00 2001 From: Emmanuel Garcia Date: Tue, 1 Sep 2020 11:17:44 -0700 Subject: [PATCH 4/5] Fixes --- .../io/flutter/embedding/android/FlutterImageView.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/shell/platform/android/io/flutter/embedding/android/FlutterImageView.java b/shell/platform/android/io/flutter/embedding/android/FlutterImageView.java index 77074631e8ebf..7482c1f398e64 100644 --- a/shell/platform/android/io/flutter/embedding/android/FlutterImageView.java +++ b/shell/platform/android/io/flutter/embedding/android/FlutterImageView.java @@ -14,6 +14,7 @@ import android.media.Image; import android.media.Image.Plane; import android.media.ImageReader; +import android.util.AttributeSet; import android.view.Surface; import android.view.View; import androidx.annotation.NonNull; @@ -82,7 +83,8 @@ public FlutterImageView(@NonNull Context context, @NonNull AttributeSet attrs) { } @VisibleForTesting - FlutterImageView(@NonNull Context context, @NonNull ImageReader imageReader, SurfaceKind kind) { + /*package*/ FlutterImageView( + @NonNull Context context, @NonNull ImageReader imageReader, SurfaceKind kind) { super(context, null); this.imageReader = imageReader; this.kind = kind; @@ -212,9 +214,7 @@ public void resizeIfNeeded(int width, int height) { return; } imageQueue.clear(); - if (currentImage != null) { - currentImage = null; - } + currentImage = null; // Close all the resources associated with the image reader, // including the images. imageReader.close(); From f05d4f9585c1511116602d73f08b17f1dba00308 Mon Sep 17 00:00:00 2001 From: Emmanuel Garcia Date: Tue, 1 Sep 2020 11:41:42 -0700 Subject: [PATCH 5/5] Use View#onDraw and suppress lint --- .../io/flutter/embedding/android/FlutterViewTest.java | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/shell/platform/android/test/io/flutter/embedding/android/FlutterViewTest.java b/shell/platform/android/test/io/flutter/embedding/android/FlutterViewTest.java index ad0d065b91684..8129ecc9a3ed3 100644 --- a/shell/platform/android/test/io/flutter/embedding/android/FlutterViewTest.java +++ b/shell/platform/android/test/io/flutter/embedding/android/FlutterViewTest.java @@ -12,6 +12,7 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +import android.annotation.SuppressLint; import android.annotation.TargetApi; import android.content.Context; import android.content.res.Configuration; @@ -579,6 +580,7 @@ public void flutterImageView_detachFromRendererClosesAllImages() { } @Test + @SuppressLint("WrongCall") /*View#onDraw*/ public void flutterImageView_onDrawClosesAllImages() { final ImageReader mockReader = mock(ImageReader.class); when(mockReader.getMaxImages()).thenReturn(2); @@ -601,8 +603,8 @@ public void flutterImageView_onDrawClosesAllImages() { imageView.acquireLatestImage(); imageView.acquireLatestImage(); - imageView.draw(mock(Canvas.class)); - imageView.draw(mock(Canvas.class)); + imageView.onDraw(mock(Canvas.class)); + imageView.onDraw(mock(Canvas.class)); // 1 image is closed and 1 is active. verify(mockImage, times(1)).close(); @@ -610,7 +612,7 @@ public void flutterImageView_onDrawClosesAllImages() { // This call doesn't do anything because there isn't // an image in the queue. - imageView.draw(mock(Canvas.class)); + imageView.onDraw(mock(Canvas.class)); verify(mockImage, times(1)).close(); // Aquire another image and push it to the queue. @@ -618,9 +620,10 @@ public void flutterImageView_onDrawClosesAllImages() { verify(mockReader, times(3)).acquireLatestImage(); // Then, the second image is closed. - imageView.draw(mock(Canvas.class)); + imageView.onDraw(mock(Canvas.class)); verify(mockImage, times(2)).close(); } + /* * A custom shadow that reports fullscreen flag for system UI visibility */