ImageWriter: fix and cleanup the closed Image Handling
Attempting to access an Image after it is closed will result in an ISE.
Also fixed some minor doc issues.
Bug: 19872785
Change-Id: I91f037b2b2f243fcbd905d5a646b505bc9c10638
diff --git a/media/java/android/media/Image.java b/media/java/android/media/Image.java
index 9ae468a..f74834b 100644
--- a/media/java/android/media/Image.java
+++ b/media/java/android/media/Image.java
@@ -47,6 +47,8 @@
* @see ImageReader
*/
public abstract class Image implements AutoCloseable {
+ protected boolean mIsImageValid = false;
+
/**
* @hide
*/
@@ -54,6 +56,14 @@
}
/**
+ * Throw IllegalStateException if the image is invalid (already closed).
+ */
+ protected void throwISEIfImageIsInvalid() {
+ if (!mIsImageValid) {
+ throw new IllegalStateException("Image is already closed");
+ }
+ }
+ /**
* Get the format for this image. This format determines the number of
* ByteBuffers needed to represent the image, and the general layout of the
* pixel data in each in ByteBuffer.
@@ -128,7 +138,7 @@
* Set the timestamp associated with this frame.
* <p>
* The timestamp is measured in nanoseconds, and is normally monotonically
- * increasing. However, However, the behavior of the timestamp depends on
+ * increasing. However, the behavior of the timestamp depends on
* the destination of this image. See {@link android.hardware.Camera Camera}
* , {@link android.hardware.camera2.CameraDevice CameraDevice},
* {@link MediaPlayer} and {@link MediaCodec} for more details.
@@ -144,6 +154,7 @@
* @param timestamp The timestamp to be set for this image.
*/
public void setTimestamp(long timestamp) {
+ throwISEIfImageIsInvalid();
return;
}
@@ -155,6 +166,7 @@
* </p>
*/
public boolean isOpaque() {
+ throwISEIfImageIsInvalid();
return false;
}
@@ -167,6 +179,8 @@
* using coordinates in the largest-resolution plane.
*/
public Rect getCropRect() {
+ throwISEIfImageIsInvalid();
+
if (mCropRect == null) {
return new Rect(0, 0, getWidth(), getHeight());
} else {
@@ -181,6 +195,8 @@
* using coordinates in the largest-resolution plane.
*/
public void setCropRect(Rect cropRect) {
+ throwISEIfImageIsInvalid();
+
if (cropRect != null) {
cropRect = new Rect(cropRect); // make a copy
cropRect.intersect(0, 0, getWidth(), getHeight());
@@ -228,6 +244,8 @@
* a new owner.
*/
boolean isAttachable() {
+ throwISEIfImageIsInvalid();
+
return false;
}
@@ -247,6 +265,8 @@
* @return The owner of the Image.
*/
Object getOwner() {
+ throwISEIfImageIsInvalid();
+
return null;
}
@@ -262,6 +282,8 @@
* @return native context associated with this Image.
*/
long getNativeContext() {
+ throwISEIfImageIsInvalid();
+
return 0;
}
diff --git a/media/java/android/media/ImageReader.java b/media/java/android/media/ImageReader.java
index 3d8f9a0..6904f46 100644
--- a/media/java/android/media/ImageReader.java
+++ b/media/java/android/media/ImageReader.java
@@ -368,7 +368,7 @@
switch (status) {
case ACQUIRE_SUCCESS:
si.createSurfacePlanes();
- si.setImageValid(true);
+ si.mIsImageValid = true;
case ACQUIRE_NO_BUFS:
case ACQUIRE_MAX_IMAGES:
break;
@@ -444,7 +444,7 @@
si.clearSurfacePlanes();
nativeReleaseImage(i);
- si.setImageValid(false);
+ si.mIsImageValid = false;
}
/**
@@ -686,7 +686,6 @@
private class SurfaceImage extends android.media.Image {
public SurfaceImage(int format) {
- mIsImageValid = false;
mFormat = format;
}
@@ -784,16 +783,6 @@
mIsDetached.getAndSet(detached);
}
- private void setImageValid(boolean isValid) {
- mIsImageValid = isValid;
- }
-
- private void throwISEIfImageIsInvalid() {
- if (!mIsImageValid) {
- throw new IllegalStateException("Image is already closed");
- }
- }
-
private void clearSurfacePlanes() {
if (mIsImageValid) {
for (int i = 0; i < mPlanes.length; i++) {
@@ -877,7 +866,6 @@
private long mTimestamp;
private SurfacePlane[] mPlanes;
- private boolean mIsImageValid;
private int mHeight = -1;
private int mWidth = -1;
private int mFormat = ImageFormat.UNKNOWN;
diff --git a/media/java/android/media/ImageWriter.java b/media/java/android/media/ImageWriter.java
index c18b463..f805339 100644
--- a/media/java/android/media/ImageWriter.java
+++ b/media/java/android/media/ImageWriter.java
@@ -29,7 +29,6 @@
import java.nio.NioUtils;
import java.util.ArrayList;
import java.util.List;
-import java.util.concurrent.atomic.AtomicBoolean;
/**
* <p>
@@ -204,7 +203,7 @@
WriterSurfaceImage newImage = new WriterSurfaceImage(this);
nativeDequeueInputImage(mNativeContext, newImage);
mDequeuedImages.add(newImage);
- newImage.setImageValid(true);
+ newImage.mIsImageValid = true;
return newImage;
}
@@ -260,7 +259,7 @@
throw new IllegalArgumentException("image shouldn't be null");
}
boolean ownedByMe = isImageOwnedByMe(image);
- if (ownedByMe && !(((WriterSurfaceImage) image).isImageValid())) {
+ if (ownedByMe && !(((WriterSurfaceImage) image).mIsImageValid)) {
throw new IllegalStateException("Image from ImageWriter is invalid");
}
@@ -312,7 +311,7 @@
// Do not call close here, as close is essentially cancel image.
WriterSurfaceImage wi = (WriterSurfaceImage) image;
wi.clearSurfacePlanes();
- wi.setImageValid(false);
+ wi.mIsImageValid = false;
}
}
@@ -555,7 +554,7 @@
WriterSurfaceImage wi = (WriterSurfaceImage) image;
- if (!wi.isImageValid()) {
+ if (!wi.mIsImageValid) {
throw new IllegalStateException("Image is invalid");
}
@@ -568,7 +567,7 @@
cancelImage(mNativeContext, image);
mDequeuedImages.remove(image);
wi.clearSurfacePlanes();
- wi.setImageValid(false);
+ wi.mIsImageValid = false;
}
private boolean isImageOwnedByMe(Image image) {
@@ -585,7 +584,6 @@
private static class WriterSurfaceImage extends android.media.Image {
private ImageWriter mOwner;
- private AtomicBoolean mIsImageValid = new AtomicBoolean(false);
// This field is used by native code, do not access or modify.
private long mNativeBuffer;
private int mNativeFenceFd = -1;
@@ -604,9 +602,8 @@
@Override
public int getFormat() {
- if (!mIsImageValid.get()) {
- throw new IllegalStateException("Image is already released");
- }
+ throwISEIfImageIsInvalid();
+
if (mFormat == -1) {
mFormat = nativeGetFormat();
}
@@ -615,9 +612,7 @@
@Override
public int getWidth() {
- if (!mIsImageValid.get()) {
- throw new IllegalStateException("Image is already released");
- }
+ throwISEIfImageIsInvalid();
if (mWidth == -1) {
mWidth = nativeGetWidth();
@@ -628,9 +623,7 @@
@Override
public int getHeight() {
- if (!mIsImageValid.get()) {
- throw new IllegalStateException("Image is already released");
- }
+ throwISEIfImageIsInvalid();
if (mHeight == -1) {
mHeight = nativeGetHeight();
@@ -641,36 +634,28 @@
@Override
public long getTimestamp() {
- if (!mIsImageValid.get()) {
- throw new IllegalStateException("Image is already released");
- }
+ throwISEIfImageIsInvalid();
return mTimestamp;
}
@Override
public void setTimestamp(long timestamp) {
- if (!mIsImageValid.get()) {
- throw new IllegalStateException("Image is already released");
- }
+ throwISEIfImageIsInvalid();
mTimestamp = timestamp;
}
@Override
public boolean isOpaque() {
- if (!mIsImageValid.get()) {
- throw new IllegalStateException("Image is already released");
- }
+ throwISEIfImageIsInvalid();
return getFormat() == ImageFormat.PRIVATE;
}
@Override
public Plane[] getPlanes() {
- if (!mIsImageValid.get()) {
- throw new IllegalStateException("Image is already released");
- }
+ throwISEIfImageIsInvalid();
if (mPlanes == null) {
int numPlanes = ImageUtils.getNumPlanesForFormat(getFormat());
@@ -682,9 +667,7 @@
@Override
boolean isAttachable() {
- if (!mIsImageValid.get()) {
- throw new IllegalStateException("Image is already released");
- }
+ throwISEIfImageIsInvalid();
// Don't allow Image to be detached from ImageWriter for now, as no
// detach API is exposed.
return false;
@@ -692,17 +675,21 @@
@Override
ImageWriter getOwner() {
+ throwISEIfImageIsInvalid();
+
return mOwner;
}
@Override
long getNativeContext() {
+ throwISEIfImageIsInvalid();
+
return mNativeBuffer;
}
@Override
public void close() {
- if (mIsImageValid.get()) {
+ if (mIsImageValid) {
getOwner().abortImage(this);
}
}
@@ -716,16 +703,8 @@
}
}
- private boolean isImageValid() {
- return mIsImageValid.get();
- }
-
- private void setImageValid(boolean isValid) {
- mIsImageValid.getAndSet(isValid);
- }
-
private void clearSurfacePlanes() {
- if (mIsImageValid.get()) {
+ if (mIsImageValid) {
for (int i = 0; i < mPlanes.length; i++) {
if (mPlanes[i] != null) {
mPlanes[i].clearBuffer();
@@ -756,26 +735,19 @@
@Override
public int getRowStride() {
- if (WriterSurfaceImage.this.isImageValid() == false) {
- throw new IllegalStateException("Image is already released");
- }
+ throwISEIfImageIsInvalid();
return mRowStride;
}
@Override
public int getPixelStride() {
- if (WriterSurfaceImage.this.isImageValid() == false) {
- throw new IllegalStateException("Image is already released");
- }
+ throwISEIfImageIsInvalid();
return mPixelStride;
}
@Override
public ByteBuffer getBuffer() {
- if (WriterSurfaceImage.this.isImageValid() == false) {
- throw new IllegalStateException("Image is already released");
- }
-
+ throwISEIfImageIsInvalid();
return mBuffer;
}
diff --git a/media/java/android/media/MediaCodec.java b/media/java/android/media/MediaCodec.java
index b0cd3e4..c621195 100644
--- a/media/java/android/media/MediaCodec.java
+++ b/media/java/android/media/MediaCodec.java
@@ -1817,7 +1817,6 @@
/** @hide */
public static class MediaImage extends Image {
private final boolean mIsReadOnly;
- private boolean mIsValid;
private final int mWidth;
private final int mHeight;
private final int mFormat;
@@ -1830,36 +1829,42 @@
private final static int TYPE_YUV = 1;
+ @Override
public int getFormat() {
- checkValid();
+ throwISEIfImageIsInvalid();
return mFormat;
}
+ @Override
public int getHeight() {
- checkValid();
+ throwISEIfImageIsInvalid();
return mHeight;
}
+ @Override
public int getWidth() {
- checkValid();
+ throwISEIfImageIsInvalid();
return mWidth;
}
+ @Override
public long getTimestamp() {
- checkValid();
+ throwISEIfImageIsInvalid();
return mTimestamp;
}
+ @Override
@NonNull
public Plane[] getPlanes() {
- checkValid();
+ throwISEIfImageIsInvalid();
return Arrays.copyOf(mPlanes, mPlanes.length);
}
+ @Override
public void close() {
- if (mIsValid) {
+ if (mIsImageValid) {
java.nio.NioUtils.freeDirectBuffer(mBuffer);
- mIsValid = false;
+ mIsImageValid = false;
}
}
@@ -1869,6 +1874,7 @@
* The crop rectangle specifies the region of valid pixels in the image,
* using coordinates in the largest-resolution plane.
*/
+ @Override
public void setCropRect(@Nullable Rect cropRect) {
if (mIsReadOnly) {
throw new ReadOnlyBufferException();
@@ -1876,11 +1882,6 @@
super.setCropRect(cropRect);
}
- private void checkValid() {
- if (!mIsValid) {
- throw new IllegalStateException("Image is already released");
- }
- }
private int readInt(@NonNull ByteBuffer buffer, boolean asLong) {
if (asLong) {
@@ -1895,7 +1896,7 @@
long timestamp, int xOffset, int yOffset, @Nullable Rect cropRect) {
mFormat = ImageFormat.YUV_420_888;
mTimestamp = timestamp;
- mIsValid = true;
+ mIsImageValid = true;
mIsReadOnly = buffer.isReadOnly();
mBuffer = buffer.duplicate();
@@ -1966,20 +1967,20 @@
@Override
public int getRowStride() {
- checkValid();
+ throwISEIfImageIsInvalid();
return mRowInc;
}
@Override
public int getPixelStride() {
- checkValid();
+ throwISEIfImageIsInvalid();
return mColInc;
}
@Override
@NonNull
public ByteBuffer getBuffer() {
- checkValid();
+ throwISEIfImageIsInvalid();
return mData;
}
diff --git a/media/jni/android_media_ImageWriter.cpp b/media/jni/android_media_ImageWriter.cpp
index d2c614e..8c76665 100644
--- a/media/jni/android_media_ImageWriter.cpp
+++ b/media/jni/android_media_ImageWriter.cpp
@@ -338,7 +338,7 @@
status_t res = anw->dequeueBuffer(anw.get(), &anb, &fenceFd);
if (res != OK) {
// TODO: handle different error cases here.
- ALOGE("%s: Set buffer count failed: %s (%d)", __FUNCTION__, strerror(-res), res);
+ ALOGE("%s: Dequeue buffer failed: %s (%d)", __FUNCTION__, strerror(-res), res);
jniThrowRuntimeException(env, "dequeue buffer failed");
return;
}