ImageReader: Fix rogue RuntimeException in #detachImage
ImageReader#detach documents that an IllegalStateException will be thrown
if there are any errors detaching Image from a Surface. As ImageReaders
are set up, it is common for this exception to be thrown. Applications
are expected to catch and recover from the exception. However,
ImageReader#nativeDetachImage, that ImageReader#detachImage calls into,
explicitly threw a RuntimeException causing applications that didn't
expect RuntimeException to crash.
This behavior has stuck around for a few years, so changing the behavior
to match the documentation will break backwards compatibility for many
apps. However, RuntimeException is an incredibly generic exception that
applications shouldn't be forced to catch as it can hide more serious
bugs.
This CL updates #nativeDetachImage to accept an additional flag.
When this flag is set to True, #nativeDetachImage only throws
IllegalStateException and preserves previous behavior when the flag is
set to False.
The flag is populated through the App Compatibility Framework which
changes the behavior only for apps that set their targetSdk to > 33.
Bug: 236825255
Bug: 204438677
Test: Manually tested that the flag is set to true for Apps targetting
Android SDK > 33
Change-Id: I20bd986f11dbe7acf4898cf0ce794c27f42e1ee2
diff --git a/media/java/android/media/ImageReader.java b/media/java/android/media/ImageReader.java
index 472586b..fde7afd 100644
--- a/media/java/android/media/ImageReader.java
+++ b/media/java/android/media/ImageReader.java
@@ -18,7 +18,11 @@
import android.annotation.IntRange;
import android.annotation.NonNull;
+import android.annotation.Nullable;
import android.annotation.SuppressLint;
+import android.app.compat.CompatChanges;
+import android.compat.annotation.ChangeId;
+import android.compat.annotation.EnabledAfter;
import android.graphics.GraphicBuffer;
import android.graphics.ImageFormat;
import android.graphics.ImageFormat.Format;
@@ -29,6 +33,7 @@
import android.hardware.HardwareBuffer.Usage;
import android.hardware.SyncFence;
import android.hardware.camera2.MultiResolutionImageReader;
+import android.os.Build;
import android.os.Handler;
import android.os.Looper;
import android.os.ParcelFileDescriptor;
@@ -87,6 +92,38 @@
/**
* <p>
+ * Flag to gate correct exception thrown by {@code #detachImage}.
+ * </p>
+ * <p>
+ * {@code #detachImage} is documented as throwing {@link java.lang.IllegalStateException} in
+ * the event of an error; a native helper method to this threw
+ * {@link java.lang.RuntimeException} if the surface was abandoned while detaching the
+ * {@code Image}.
+ * <p>
+ * This previously undocumented exception behavior continues through Android T.
+ * </p>
+ * <p>
+ * After Android T, the native helper method only throws {@code IllegalStateExceptions} in
+ * accordance with the documentation.
+ * </p>
+ * <p>
+ * {@code #detachImage} will now throw only ISEs if it runs into errors while detaching
+ * the image. Behavior on apps targeting API levels <= T remains unchanged.
+ * </p>
+ */
+ @ChangeId
+ @EnabledAfter(targetSdkVersion = Build.VERSION_CODES.TIRAMISU)
+ private static final long DETACH_THROWS_ISE_ONLY = 236825255L;
+
+ /**
+ * Cached value of {@link #DETACH_THROWS_ISE_ONLY} flag to prevent repeated calls when
+ * detaching image.
+ */
+ private final boolean mDetachThrowsIseOnly =
+ CompatChanges.isChangeEnabled(DETACH_THROWS_ISE_ONLY);
+
+ /**
+ * <p>
* Create a new reader for images of the desired size and format.
* </p>
* <p>
@@ -825,10 +862,10 @@
* </p>
* <p>
* After this call, the ImageReader no longer owns this image, and the image
- * ownership can be transfered to another entity like {@link ImageWriter}
+ * ownership can be transferred to another entity like {@link ImageWriter}
* via {@link ImageWriter#queueInputImage}. It's up to the new owner to
* release the resources held by this image. For example, if the ownership
- * of this image is transfered to an {@link ImageWriter}, the image will be
+ * of this image is transferred to an {@link ImageWriter}, the image will be
* freed by the ImageWriter after the image data consumption is done.
* </p>
* <p>
@@ -849,16 +886,22 @@
* @throws IllegalStateException If the ImageReader or image have been
* closed, or the has been detached, or has not yet been
* acquired.
+ * @throws RuntimeException If there is an error detaching {@code Image} from {@code Surface}.
+ * {@code RuntimeException} is only thrown for applications targeting SDK <=
+ * {@link android.os.Build.VERSION_CODES#TIRAMISU}.
+ * For applications targeting SDK >
+ * {@link android.os.Build.VERSION_CODES#TIRAMISU},
+ * this method only throws {@code IllegalStateException}.
* @hide
*/
- public void detachImage(Image image) {
- if (image == null) {
+ public void detachImage(@Nullable Image image) {
+ if (image == null) {
throw new IllegalArgumentException("input image must not be null");
- }
- if (!isImageOwnedbyMe(image)) {
+ }
+ if (!isImageOwnedbyMe(image)) {
throw new IllegalArgumentException("Trying to detach an image that is not owned by"
+ " this ImageReader");
- }
+ }
SurfaceImage si = (SurfaceImage) image;
si.throwISEIfImageIsInvalid();
@@ -867,7 +910,7 @@
throw new IllegalStateException("Image was already detached from this ImageReader");
}
- nativeDetachImage(image);
+ nativeDetachImage(image, mDetachThrowsIseOnly);
si.clearSurfacePlanes();
si.mPlanes = null;
si.setDetached(true);
@@ -1408,7 +1451,7 @@
private synchronized native void nativeClose();
private synchronized native void nativeReleaseImage(Image i);
private synchronized native Surface nativeGetSurface();
- private synchronized native int nativeDetachImage(Image i);
+ private synchronized native int nativeDetachImage(Image i, boolean throwISEOnly);
private synchronized native void nativeDiscardFreeBuffers();
/**
diff --git a/media/jni/android_media_ImageReader.cpp b/media/jni/android_media_ImageReader.cpp
index 62c0d55..556f98c 100644
--- a/media/jni/android_media_ImageReader.cpp
+++ b/media/jni/android_media_ImageReader.cpp
@@ -643,7 +643,8 @@
return ACQUIRE_SUCCESS;
}
-static jint ImageReader_detachImage(JNIEnv* env, jobject thiz, jobject image) {
+static jint ImageReader_detachImage(JNIEnv* env, jobject thiz, jobject image,
+ jboolean throwISEOnly) {
ALOGV("%s:", __FUNCTION__);
JNIImageReaderContext* ctx = ImageReader_getContext(env, thiz);
if (ctx == NULL) {
@@ -666,8 +667,12 @@
res = bufferConsumer->detachBuffer(buffer->mSlot);
if (res != OK) {
ALOGE("Image detach failed: %s (%d)!!!", strerror(-res), res);
- jniThrowRuntimeException(env,
- "nativeDetachImage failed for image!!!");
+ if ((bool) throwISEOnly) {
+ jniThrowException(env, "java/lang/IllegalStateException",
+ "nativeDetachImage failed for image!!!");
+ } else {
+ jniThrowRuntimeException(env, "nativeDetachImage failed for image!!!");
+ }
return res;
}
return OK;
@@ -965,7 +970,7 @@
{"nativeReleaseImage", "(Landroid/media/Image;)V", (void*)ImageReader_imageRelease },
{"nativeImageSetup", "(Landroid/media/Image;Z)I", (void*)ImageReader_imageSetup },
{"nativeGetSurface", "()Landroid/view/Surface;", (void*)ImageReader_getSurface },
- {"nativeDetachImage", "(Landroid/media/Image;)I", (void*)ImageReader_detachImage },
+ {"nativeDetachImage", "(Landroid/media/Image;Z)I", (void*)ImageReader_detachImage },
{"nativeCreateImagePlanes",
"(ILandroid/graphics/GraphicBuffer;IIIIII)[Landroid/media/ImageReader$ImagePlane;",
(void*)ImageReader_createImagePlanes },