From dc6bb24a3c2c830c909523e8d7d999fb556fb77d Mon Sep 17 00:00:00 2001 From: Zhijun He Date: Thu, 3 Dec 2015 15:34:34 -0800 Subject: [PATCH] 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 --- media/java/android/media/ImageReader.java | 110 ++++++++++------------ media/java/android/media/ImageUtils.java | 3 + media/java/android/media/ImageWriter.java | 30 +++++- media/jni/android_media_ImageReader.cpp | 5 +- media/jni/android_media_ImageWriter.cpp | 6 +- 5 files changed, 84 insertions(+), 70 deletions(-) diff --git a/media/java/android/media/ImageReader.java b/media/java/android/media/ImageReader.java index 2164eec48705b..66a174c35f5ce 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.lang.ref.WeakReference; 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 @@ public class ImageReader implements AutoCloseable { "NV21 format is not supported"); } - mNumPlanes = getNumPlanesFromFormat(); + mNumPlanes = ImageUtils.getNumPlanesForFormat(mFormat); nativeInit(new WeakReference(this), width, height, format, maxImages); @@ -323,21 +325,27 @@ public class ImageReader implements AutoCloseable { * @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 @@ public class ImageReader implements AutoCloseable { "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 @@ public class ImageReader implements AutoCloseable { si.clearSurfacePlanes(); nativeReleaseImage(i); si.mIsImageValid = false; + mAcquiredImages.remove(i); } /** @@ -475,7 +488,24 @@ public class ImageReader implements AutoCloseable { 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 @@ public class ImageReader implements AutoCloseable { 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 @@ public class ImageReader implements AutoCloseable { } } - private final int mWidth; private final int mHeight; private final int mFormat; @@ -622,8 +612,12 @@ public class ImageReader implements AutoCloseable { 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 mAcquiredImages = new CopyOnWriteArrayList(); /** * This field is used by native code, do not access or modify. @@ -657,9 +651,7 @@ public class ImageReader implements AutoCloseable { @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 ba3949a64e235..eefd69c4cb3c2 100644 --- a/media/java/android/media/ImageUtils.java +++ b/media/java/android/media/ImageUtils.java @@ -58,6 +58,9 @@ class ImageUtils { 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 9fb328627612d..851d436744588 100644 --- a/media/java/android/media/ImageWriter.java +++ b/media/java/android/media/ImageWriter.java @@ -18,17 +18,21 @@ package android.media; 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; /** *

@@ -83,8 +87,10 @@ public class ImageWriter implements AutoCloseable { private int mWriterFormat; private final int mMaxImages; - // Keep track of the currently dequeued Image. - private List mDequeuedImages = new ArrayList(); + // 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 mDequeuedImages = new CopyOnWriteArrayList(); + private int mEstimatedNativeAllocBytes; /** *

@@ -128,6 +134,16 @@ public class ImageWriter implements AutoCloseable { // Note that the underlying BufferQueue is working in synchronous mode // to avoid dropping any buffers. mNativeContext = nativeInit(new WeakReference(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 @@ public class ImageWriter implements AutoCloseable { mDequeuedImages.clear(); nativeClose(mNativeContext); mNativeContext = 0; + + if (mEstimatedNativeAllocBytes > 0) { + VMRuntime.getRuntime().registerNativeFree(mEstimatedNativeAllocBytes); + mEstimatedNativeAllocBytes = 0; + } } @Override @@ -569,9 +590,8 @@ public class ImageWriter implements AutoCloseable { } 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 3ffdb17f4ec0f..9a53186365e18 100644 --- a/media/jni/android_media_ImageReader.cpp +++ b/media/jni/android_media_ImageReader.cpp @@ -933,7 +933,7 @@ static void ImageReader_imageRelease(JNIEnv* env, jobject thiz, jobject image) 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 @@ static jint ImageReader_imageSetup(JNIEnv* env, jobject thiz, jobject image) { 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 f92a8eff3919d..f50da8547af21 100644 --- a/media/jni/android_media_ImageWriter.cpp +++ b/media/jni/android_media_ImageWriter.cpp @@ -403,8 +403,7 @@ static void ImageWriter_cancelImage(JNIEnv* env, jobject thiz, jlong nativeCtx, ALOGV("%s", __FUNCTION__); JNIImageWriterContext* const ctx = reinterpret_cast(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 @@ static void ImageWriter_cancelImage(JNIEnv* env, jobject thiz, jlong nativeCtx, 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; }