From 375525ff4103081876dc64ec3af3247177c5d1ac Mon Sep 17 00:00:00 2001 From: Avichal Rakesh Date: Tue, 13 Dec 2022 10:08:23 -0800 Subject: [PATCH] Make ImageWriter thread safe The various functions in ImageWriter were not protected by a lock, which led to segfaults under certain race conditions. This CL adds a mutex to prevent ImageWriter methods from executing while ImageWriter is closing. This also prevent multiple calls to close from executing simultaneously. It also adds a simialar mutex to WriterImageSurface to prevent simultaneous calls to close(). Bug: 244262872 Test: mediaframeworktest:com.android.mediaframeworktest.stress Change-Id: I44976e5a36154ce98f64e2982865b2defcec7f5e --- media/java/android/media/ImageWriter.java | 152 +++++++++++++--------- 1 file changed, 92 insertions(+), 60 deletions(-) diff --git a/media/java/android/media/ImageWriter.java b/media/java/android/media/ImageWriter.java index 9f52bf18f4e37..308bb3eb723f0 100644 --- a/media/java/android/media/ImageWriter.java +++ b/media/java/android/media/ImageWriter.java @@ -100,6 +100,8 @@ public class ImageWriter implements AutoCloseable { private final Object mListenerLock = new Object(); private OnImageReleasedListener mListener; private ListenerHandler mListenerHandler; + private final Object mCloseLock = new Object(); + private boolean mIsWriterValid = false; private long mNativeContext; private int mWidth; @@ -305,6 +307,8 @@ public class ImageWriter implements AutoCloseable { ImageUtils.getEstimatedNativeAllocBytes(mWidth, mHeight, useLegacyImageFormat ? imageFormat : hardwareBufferFormat, /*buffer count*/ 1); VMRuntime.getRuntime().registerNativeAllocation(mEstimatedNativeAllocBytes); + + mIsWriterValid = true; } private ImageWriter(Surface surface, int maxImages, boolean useSurfaceImageFormatInfo, @@ -448,14 +452,17 @@ public class ImageWriter implements AutoCloseable { * @see Image#close */ public Image dequeueInputImage() { - if (mDequeuedImages.size() >= mMaxImages) { - throw new IllegalStateException("Already dequeued max number of Images " + mMaxImages); + synchronized (mCloseLock) { + if (mDequeuedImages.size() >= mMaxImages) { + throw new IllegalStateException( + "Already dequeued max number of Images " + mMaxImages); + } + WriterSurfaceImage newImage = new WriterSurfaceImage(this); + nativeDequeueInputImage(mNativeContext, newImage); + mDequeuedImages.add(newImage); + newImage.mIsImageValid = true; + return newImage; } - WriterSurfaceImage newImage = new WriterSurfaceImage(this); - nativeDequeueInputImage(mNativeContext, newImage); - mDequeuedImages.add(newImage); - newImage.mIsImageValid = true; - return newImage; } /** @@ -513,49 +520,53 @@ public class ImageWriter implements AutoCloseable { if (image == null) { throw new IllegalArgumentException("image shouldn't be null"); } - boolean ownedByMe = isImageOwnedByMe(image); - if (ownedByMe && !(((WriterSurfaceImage) image).mIsImageValid)) { - throw new IllegalStateException("Image from ImageWriter is invalid"); - } - // For images from other components that have non-null owner, need to detach first, - // then attach. Images without owners must already be attachable. - if (!ownedByMe) { - if ((image.getOwner() instanceof ImageReader)) { - ImageReader prevOwner = (ImageReader) image.getOwner(); - - prevOwner.detachImage(image); - } else if (image.getOwner() != null) { - throw new IllegalArgumentException("Only images from ImageReader can be queued to" - + " ImageWriter, other image source is not supported yet!"); + synchronized (mCloseLock) { + boolean ownedByMe = isImageOwnedByMe(image); + if (ownedByMe && !(((WriterSurfaceImage) image).mIsImageValid)) { + throw new IllegalStateException("Image from ImageWriter is invalid"); } - attachAndQueueInputImage(image); - // This clears the native reference held by the original owner. - // When this Image is detached later by this ImageWriter, the - // native memory won't be leaked. - image.close(); - return; - } + // For images from other components that have non-null owner, need to detach first, + // then attach. Images without owners must already be attachable. + if (!ownedByMe) { + if ((image.getOwner() instanceof ImageReader)) { + ImageReader prevOwner = (ImageReader) image.getOwner(); - Rect crop = image.getCropRect(); - nativeQueueInputImage(mNativeContext, image, image.getTimestamp(), image.getDataSpace(), - crop.left, crop.top, crop.right, crop.bottom, image.getTransform(), - image.getScalingMode()); + prevOwner.detachImage(image); + } else if (image.getOwner() != null) { + throw new IllegalArgumentException( + "Only images from ImageReader can be queued to" + + " ImageWriter, other image source is not supported yet!"); + } - /** - * Only remove and cleanup the Images that are owned by this - * ImageWriter. Images detached from other owners are only temporarily - * owned by this ImageWriter and will be detached immediately after they - * are released by downstream consumers, so there is no need to keep - * track of them in mDequeuedImages. - */ - if (ownedByMe) { - mDequeuedImages.remove(image); - // Do not call close here, as close is essentially cancel image. - WriterSurfaceImage wi = (WriterSurfaceImage) image; - wi.clearSurfacePlanes(); - wi.mIsImageValid = false; + attachAndQueueInputImage(image); + // This clears the native reference held by the original owner. + // When this Image is detached later by this ImageWriter, the + // native memory won't be leaked. + image.close(); + return; + } + + Rect crop = image.getCropRect(); + nativeQueueInputImage(mNativeContext, image, image.getTimestamp(), image.getDataSpace(), + crop.left, crop.top, crop.right, crop.bottom, image.getTransform(), + image.getScalingMode()); + + /** + * Only remove and cleanup the Images that are owned by this + * ImageWriter. Images detached from other owners are only temporarily + * owned by this ImageWriter and will be detached immediately after they + * are released by downstream consumers, so there is no need to keep + * track of them in mDequeuedImages. + */ + if (ownedByMe) { + mDequeuedImages.remove(image); + // Do not call close here, as close is essentially cancel image. + WriterSurfaceImage wi = (WriterSurfaceImage) image; + wi.clearSurfacePlanes(); + wi.mIsImageValid = false; + } } } @@ -691,17 +702,23 @@ public class ImageWriter implements AutoCloseable { */ @Override public void close() { - setOnImageReleasedListener(null, null); - for (Image image : mDequeuedImages) { - image.close(); - } - mDequeuedImages.clear(); - nativeClose(mNativeContext); - mNativeContext = 0; + synchronized (mCloseLock) { + if (!mIsWriterValid) { + return; + } + setOnImageReleasedListener(null, null); + for (Image image : mDequeuedImages) { + image.close(); + } + mDequeuedImages.clear(); + nativeClose(mNativeContext); + mNativeContext = 0; - if (mEstimatedNativeAllocBytes > 0) { - VMRuntime.getRuntime().registerNativeFree(mEstimatedNativeAllocBytes); - mEstimatedNativeAllocBytes = 0; + if (mEstimatedNativeAllocBytes > 0) { + VMRuntime.getRuntime().registerNativeFree(mEstimatedNativeAllocBytes); + mEstimatedNativeAllocBytes = 0; + } + mIsWriterValid = false; } } @@ -790,10 +807,16 @@ public class ImageWriter implements AutoCloseable { @Override public void handleMessage(Message msg) { OnImageReleasedListener listener; - synchronized (mListenerLock) { + boolean isWriterValid; + synchronized (ImageWriter.this.mListenerLock) { listener = mListener; } - if (listener != null) { + // Check to make sure we don't accidentally queue images after the writer is + // closed or closing + synchronized (ImageWriter.this.mCloseLock) { + isWriterValid = ImageWriter.this.mIsWriterValid; + } + if (listener != null && isWriterValid) { listener.onImageReleased(ImageWriter.this); } } @@ -813,10 +836,14 @@ public class ImageWriter implements AutoCloseable { } final Handler handler; + final boolean isWriterValid; synchronized (iw.mListenerLock) { handler = iw.mListenerHandler; } - if (handler != null) { + synchronized (iw.mCloseLock) { + isWriterValid = iw.mIsWriterValid; + } + if (handler != null && isWriterValid) { handler.sendEmptyMessage(0); } } @@ -1050,6 +1077,9 @@ public class ImageWriter implements AutoCloseable { private int mTransform = 0; //Default no transform private int mScalingMode = 0; //Default frozen scaling mode + private final Object mCloseLock = new Object(); // lock to protect against multiple + // simultaneous calls to close() + public WriterSurfaceImage(ImageWriter writer) { mOwner = writer; mWidth = writer.mWidth; @@ -1192,8 +1222,10 @@ public class ImageWriter implements AutoCloseable { @Override public void close() { - if (mIsImageValid) { - getOwner().abortImage(this); + synchronized (mCloseLock) { + if (mIsImageValid) { + getOwner().abortImage(this); + } } }