Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

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

Merged
merged 1 commit into from
Feb 18, 2021
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 27 additions & 48 deletions shell/platform/android/io/flutter/embedding/android/FlutterImageView.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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;
Expand All @@ -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;

Expand All @@ -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();
}

Expand Down Expand Up @@ -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;
}
Expand All @@ -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();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like there is some misunderstanding from my side. The error message " Unable to acquire a buffer item, very likely client tried to acquire more than maxImages buffers" suggests that imageReader.acquireLatestImage() was called even though imageOpenedCount is less than imageReader.getMaxImages(). Unless there's a bug in this logic, shouldn't it be preventing the acquireLatestImage() call?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@blasten I guess maybe the newest available image will be also counted by the code in ImageReader.cpp.
Let me have a deeper view in ImageReader.cpp

Copy link
Member Author

@eggfly eggfly Feb 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@blasten I read the code and document of acquireLatestImage() in ImageReader.java carefully and I finally know why the error message comes even though the openedCount is less than maxImages.

The code:
https://cs.android.com/android/platform/superproject/+/master:frameworks/base/media/java/android/media/ImageReader.java;drc=master;l=410?q=two

The doc of acquireLatestImage() says:

     * Note that {@link #getMaxImages maxImages} should be at least 2 for
     * {@link #acquireLatestImage} to be any different than {@link #acquireNextImage} -
     * discarding all-but-the-newest {@link Image} requires temporarily acquiring two
     * {@link Image Images} at once. Or more generally, calling {@link #acquireLatestImage}
     * with less than two images of margin, that is
     * {@code (maxImages - currentAcquiredImages < 2)} will not discard as expected.
     * </p>

So it needs at least two reserved slots in buffer before one calling of acquireLatestImage(). And the implementation code is as same as it described:

    public Image acquireLatestImage() {
        Image image = acquireNextImage();
        if (image == null) {
            return null;
        }
        try {
            for (;;) {
                Image next = acquireNextImageNoThrowISE();
                if (next == null) {
                    Image result = image;
                    image = null;
                    return result;
                }
                image.close();
                image = next;
            }
        } finally {
            if (image != null) {
                image.close();
            }
        }
    }

Every nativeImageSetup() will need a new BufferItem in cpp, acquireNextSurfaceImage() calls nativeImageSetup(). then looking at the acquireLatestImage() logic, it calls acquireNextImage() and calls acquireNextImageNoThrowISE in the for-loop and close the last image unless the next is null. So It can really need two slots.

The code of this pull request fixs onDraw() may be lesser then invalidate(), and try to make the code logic neat and easy to maintain by closing the previous image if exists after reader.acquireLatestImages() and removing the queue and the counter.

if (image != null) {
imageQueue.add(image);
}
}
invalidate();
return !imageQueue.isEmpty();
return newImage != null;
}

/** Creates a new image reader with the provided size. */
Expand All @@ -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) {
Expand Down
111 changes: 23 additions & 88 deletions shell/platform/android/test/io/flutter/embedding/android/FlutterViewTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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);

Expand All @@ -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
Expand Down