media: improve ImageReader/Writer native memory management
* Hook up the native allocation registration with ImageWriter, such that GC
can get some hint when clean up the large memory object.
* Close all pending images when closing ImageReader. This could avoid native
mem leaks for some bad app practice. For example, some apps may hold images
in background service when activity is paused/destroyed, which could cause
huge native memory leaks even ImageReader is closed.
* make Image close thread safe: it is possible the clients close the image
in listener thread and the client main thread.
* Some minor code refactor to reduce the code duplication.
Bug: 25088440
Change-Id: I37d22b52aeb8d2521bf9c702b0f54c05905473e0
diff --git a/media/java/android/media/ImageReader.java b/media/java/android/media/ImageReader.java
index 2164eec..66a174c 100644
--- a/media/java/android/media/ImageReader.java
+++ b/media/java/android/media/ImageReader.java
@@ -17,10 +17,10 @@
package android.media;
import android.graphics.ImageFormat;
-import android.graphics.PixelFormat;
import android.os.Handler;
import android.os.Looper;
import android.os.Message;
+import android.util.Log;
import android.view.Surface;
import dalvik.system.VMRuntime;
@@ -29,6 +29,8 @@
import java.nio.ByteBuffer;
import java.nio.ByteOrder;
import java.nio.NioUtils;
+import java.util.List;
+import java.util.concurrent.CopyOnWriteArrayList;
import java.util.concurrent.atomic.AtomicBoolean;
/**
@@ -145,7 +147,7 @@
"NV21 format is not supported");
}
- mNumPlanes = getNumPlanesFromFormat();
+ mNumPlanes = ImageUtils.getNumPlanesForFormat(mFormat);
nativeInit(new WeakReference<ImageReader>(this), width, height, format, maxImages);
@@ -323,21 +325,27 @@
* @see #ACQUIRE_SUCCESS
*/
private int acquireNextSurfaceImage(SurfaceImage si) {
+ synchronized (mCloseLock) {
+ int status = nativeImageSetup(si);
- int status = nativeImageSetup(si);
+ switch (status) {
+ case ACQUIRE_SUCCESS:
+ si.createSurfacePlanes();
+ si.mIsImageValid = true;
+ case ACQUIRE_NO_BUFS:
+ case ACQUIRE_MAX_IMAGES:
+ break;
+ default:
+ throw new AssertionError("Unknown nativeImageSetup return code " + status);
+ }
- switch (status) {
- case ACQUIRE_SUCCESS:
- si.createSurfacePlanes();
- si.mIsImageValid = true;
- case ACQUIRE_NO_BUFS:
- case ACQUIRE_MAX_IMAGES:
- break;
- default:
- throw new AssertionError("Unknown nativeImageSetup return code " + status);
+ // Only keep track the successfully acquired image, as the native buffer is only mapped
+ // for such case.
+ if (status == ACQUIRE_SUCCESS) {
+ mAcquiredImages.add(si);
+ }
+ return status;
}
-
- return status;
}
/**
@@ -398,7 +406,11 @@
"This image was not produced by an ImageReader");
}
SurfaceImage si = (SurfaceImage) i;
- if (si.getReader() != this) {
+ if (si.mIsImageValid == false) {
+ return;
+ }
+
+ if (si.getReader() != this || !mAcquiredImages.contains(i)) {
throw new IllegalArgumentException(
"This image was not produced by this ImageReader");
}
@@ -406,6 +418,7 @@
si.clearSurfacePlanes();
nativeReleaseImage(i);
si.mIsImageValid = false;
+ mAcquiredImages.remove(i);
}
/**
@@ -475,7 +488,24 @@
public void close() {
setOnImageAvailableListener(null, null);
if (mSurface != null) mSurface.release();
- nativeClose();
+
+ /**
+ * Close all outstanding acquired images before closing the ImageReader. It is a good
+ * practice to close all the images as soon as it is not used to reduce system instantaneous
+ * memory pressure. CopyOnWrite list will use a copy of current list content. For the images
+ * being closed by other thread (e.g., GC thread), doubling the close call is harmless. For
+ * the image being acquired by other threads, mCloseLock is used to synchronize close and
+ * acquire operations.
+ */
+ synchronized (mCloseLock) {
+ for (Image image : mAcquiredImages) {
+ image.close();
+ }
+ mAcquiredImages.clear();
+
+ nativeClose();
+ }
+
if (mEstimatedNativeAllocBytes > 0) {
VMRuntime.getRuntime().registerNativeFree(mEstimatedNativeAllocBytes);
mEstimatedNativeAllocBytes = 0;
@@ -542,45 +572,6 @@
si.setDetached(true);
}
- /**
- * Only a subset of the formats defined in
- * {@link android.graphics.ImageFormat ImageFormat} and
- * {@link android.graphics.PixelFormat PixelFormat} are supported by
- * ImageReader. When reading RGB data from a surface, the formats defined in
- * {@link android.graphics.PixelFormat PixelFormat} can be used, when
- * reading YUV, JPEG or raw sensor data (for example, from camera or video
- * decoder), formats from {@link android.graphics.ImageFormat ImageFormat}
- * are used.
- */
- private int getNumPlanesFromFormat() {
- switch (mFormat) {
- case ImageFormat.YV12:
- case ImageFormat.YUV_420_888:
- case ImageFormat.NV21:
- return 3;
- case ImageFormat.NV16:
- return 2;
- case PixelFormat.RGB_565:
- case PixelFormat.RGBA_8888:
- case PixelFormat.RGBX_8888:
- case PixelFormat.RGB_888:
- case ImageFormat.JPEG:
- case ImageFormat.YUY2:
- case ImageFormat.Y8:
- case ImageFormat.Y16:
- case ImageFormat.RAW_SENSOR:
- case ImageFormat.RAW10:
- case ImageFormat.DEPTH16:
- case ImageFormat.DEPTH_POINT_CLOUD:
- return 1;
- case ImageFormat.PRIVATE:
- return 0;
- default:
- throw new UnsupportedOperationException(
- String.format("Invalid format specified %d", mFormat));
- }
- }
-
private boolean isImageOwnedbyMe(Image image) {
if (!(image instanceof SurfaceImage)) {
return false;
@@ -612,7 +603,6 @@
}
}
-
private final int mWidth;
private final int mHeight;
private final int mFormat;
@@ -622,8 +612,12 @@
private int mEstimatedNativeAllocBytes;
private final Object mListenerLock = new Object();
+ private final Object mCloseLock = new Object();
private OnImageAvailableListener mListener;
private ListenerHandler mListenerHandler;
+ // Keep track of the successfully acquired Images. This need to be thread safe as the images
+ // could be closed by different threads (e.g., application thread and GC thread).
+ private List<Image> mAcquiredImages = new CopyOnWriteArrayList<Image>();
/**
* This field is used by native code, do not access or modify.
@@ -657,9 +651,7 @@
@Override
public void close() {
- if (mIsImageValid) {
- ImageReader.this.releaseImage(this);
- }
+ ImageReader.this.releaseImage(this);
}
public ImageReader getReader() {
diff --git a/media/java/android/media/ImageUtils.java b/media/java/android/media/ImageUtils.java
index ba3949a..eefd69c 100644
--- a/media/java/android/media/ImageUtils.java
+++ b/media/java/android/media/ImageUtils.java
@@ -58,6 +58,9 @@
case ImageFormat.Y16:
case ImageFormat.RAW_SENSOR:
case ImageFormat.RAW10:
+ case ImageFormat.RAW12:
+ case ImageFormat.DEPTH16:
+ case ImageFormat.DEPTH_POINT_CLOUD:
return 1;
case ImageFormat.PRIVATE:
return 0;
diff --git a/media/java/android/media/ImageWriter.java b/media/java/android/media/ImageWriter.java
index 9fb3286..851d436 100644
--- a/media/java/android/media/ImageWriter.java
+++ b/media/java/android/media/ImageWriter.java
@@ -18,17 +18,21 @@
import android.graphics.ImageFormat;
import android.graphics.Rect;
+import android.hardware.camera2.utils.SurfaceUtils;
import android.os.Handler;
import android.os.Looper;
import android.os.Message;
+import android.util.Size;
import android.view.Surface;
+import dalvik.system.VMRuntime;
+
import java.lang.ref.WeakReference;
import java.nio.ByteBuffer;
import java.nio.ByteOrder;
import java.nio.NioUtils;
-import java.util.ArrayList;
import java.util.List;
+import java.util.concurrent.CopyOnWriteArrayList;
/**
* <p>
@@ -83,8 +87,10 @@
private int mWriterFormat;
private final int mMaxImages;
- // Keep track of the currently dequeued Image.
- private List<Image> mDequeuedImages = new ArrayList<Image>();
+ // Keep track of the currently dequeued Image. This need to be thread safe as the images
+ // could be closed by different threads (e.g., application thread and GC thread).
+ private List<Image> mDequeuedImages = new CopyOnWriteArrayList<Image>();
+ private int mEstimatedNativeAllocBytes;
/**
* <p>
@@ -128,6 +134,16 @@
// Note that the underlying BufferQueue is working in synchronous mode
// to avoid dropping any buffers.
mNativeContext = nativeInit(new WeakReference<ImageWriter>(this), surface, maxImages);
+
+ // Estimate the native buffer allocation size and register it so it gets accounted for
+ // during GC. Note that this doesn't include the buffers required by the buffer queue
+ // itself and the buffers requested by the producer.
+ Size surfSize = SurfaceUtils.getSurfaceSize(surface);
+ int format = SurfaceUtils.getSurfaceFormat(surface);
+ mEstimatedNativeAllocBytes =
+ ImageUtils.getEstimatedNativeAllocBytes(surfSize.getWidth(),surfSize.getHeight(),
+ format, maxImages);
+ VMRuntime.getRuntime().registerNativeAllocation(mEstimatedNativeAllocBytes);
}
/**
@@ -432,6 +448,11 @@
mDequeuedImages.clear();
nativeClose(mNativeContext);
mNativeContext = 0;
+
+ if (mEstimatedNativeAllocBytes > 0) {
+ VMRuntime.getRuntime().registerNativeFree(mEstimatedNativeAllocBytes);
+ mEstimatedNativeAllocBytes = 0;
+ }
}
@Override
@@ -569,9 +590,8 @@
}
WriterSurfaceImage wi = (WriterSurfaceImage) image;
-
if (!wi.mIsImageValid) {
- throw new IllegalStateException("Image is invalid");
+ return;
}
/**
diff --git a/media/jni/android_media_ImageReader.cpp b/media/jni/android_media_ImageReader.cpp
index 3ffdb17..9a53186 100644
--- a/media/jni/android_media_ImageReader.cpp
+++ b/media/jni/android_media_ImageReader.cpp
@@ -933,7 +933,7 @@
CpuConsumer* consumer = ctx->getCpuConsumer();
CpuConsumer::LockedBuffer* buffer = Image_getLockedBuffer(env, image);
if (!buffer) {
- ALOGW("Image already released!!!");
+ // Release an already closed image is harmless.
return;
}
consumer->unlockBuffer(*buffer);
@@ -1078,7 +1078,8 @@
ALOGV("%s:", __FUNCTION__);
JNIImageReaderContext* ctx = ImageReader_getContext(env, thiz);
if (ctx == NULL) {
- jniThrowRuntimeException(env, "ImageReaderContext is not initialized");
+ jniThrowException(env, "java/lang/IllegalStateException",
+ "ImageReader is not initialized or was already closed");
return -1;
}
diff --git a/media/jni/android_media_ImageWriter.cpp b/media/jni/android_media_ImageWriter.cpp
index f92a8ef..f50da85 100644
--- a/media/jni/android_media_ImageWriter.cpp
+++ b/media/jni/android_media_ImageWriter.cpp
@@ -403,8 +403,7 @@
ALOGV("%s", __FUNCTION__);
JNIImageWriterContext* const ctx = reinterpret_cast<JNIImageWriterContext *>(nativeCtx);
if (ctx == NULL || thiz == NULL) {
- jniThrowException(env, "java/lang/IllegalStateException",
- "ImageWriterContext is not initialized");
+ ALOGW("ImageWriter#close called before Image#close, consider calling Image#close first");
return;
}
@@ -414,8 +413,7 @@
int fenceFd = -1;
Image_getNativeContext(env, image, &buffer, &fenceFd);
if (buffer == NULL) {
- jniThrowException(env, "java/lang/IllegalStateException",
- "Image is not initialized");
+ // Cancel an already cancelled image is harmless.
return;
}