Merge "Make the ImageProxy acquired from ImageCapture safe" into androidx-master-dev

4 files changed, 85 insertions, 18 deletions

diff --git a/camera/camera-core/src/main/java/androidx/camera/core/ImageCapture.java b/camera/camera-core/src/main/java/androidx/camera/core/ImageCapture.java
index 18eede83100..2a6c07eddf9 100644
--- a/camera/camera-core/src/main/java/androidx/camera/core/ImageCapture.java
+++ b/camera/camera-core/src/main/java/androidx/camera/core/ImageCapture.java
@@ -251,7 +251,11 @@ public final class ImageCapture extends UseCase {
     private final CaptureProcessor mCaptureProcessor;
     /** synthetic accessor */
     @SuppressWarnings("WeakerAccess")
-    ImageReaderProxy mImageReader;
+    SafeCloseImageReaderProxy mImageReader;
+
+    @SuppressWarnings("WeakerAccess")
+    ProcessingImageReader mProcessingImageReader;
+
     /** Callback used to match the {@link ImageProxy} with the {@link ImageInfo}. */
     private CameraCaptureCallback mMetadataMatchingCaptureCallback;
     private ImageCaptureConfig mConfig;
@@ -321,13 +325,15 @@ public final class ImageCapture extends UseCase {
 
         // Setup the ImageReader to do processing
         if (config.getImageReaderProxyProvider() != null) {
-            mImageReader = config.getImageReaderProxyProvider().newInstance(resolution.getWidth(),
-                    resolution.getHeight(), getImageFormat(), MAX_IMAGES, 0);
+            mImageReader =
+                    new SafeCloseImageReaderProxy(
+                            config.getImageReaderProxyProvider().newInstance(resolution.getWidth(),
+                                    resolution.getHeight(), getImageFormat(), MAX_IMAGES, 0));
             mMetadataMatchingCaptureCallback = new CameraCaptureCallback() {
             };
         } else if (mCaptureProcessor != null) {
             // TODO: To allow user to use an Executor for the image processing.
-            ProcessingImageReader processingImageReader =
+            mProcessingImageReader =
                     new ProcessingImageReader(
                             resolution.getWidth(),
                             resolution.getHeight(),
@@ -335,13 +341,13 @@ public final class ImageCapture extends UseCase {
                             /* postProcessExecutor */mExecutor,
                             getCaptureBundle(CaptureBundles.singleDefaultCaptureBundle()),
                             mCaptureProcessor);
-            mMetadataMatchingCaptureCallback = processingImageReader.getCameraCaptureCallback();
-            mImageReader = processingImageReader;
+            mMetadataMatchingCaptureCallback = mProcessingImageReader.getCameraCaptureCallback();
+            mImageReader = new SafeCloseImageReaderProxy(mProcessingImageReader);
         } else {
             MetadataImageReader metadataImageReader = new MetadataImageReader(resolution.getWidth(),
                     resolution.getHeight(), getImageFormat(), MAX_IMAGES);
             mMetadataMatchingCaptureCallback = metadataImageReader.getCameraCaptureCallback();
-            mImageReader = metadataImageReader;
+            mImageReader = new SafeCloseImageReaderProxy(metadataImageReader);
         }
         mImageCaptureRequestProcessor = new ImageCaptureRequestProcessor(MAX_IMAGES,
                 request -> takePictureInternal(request));
@@ -350,13 +356,13 @@ public final class ImageCapture extends UseCase {
         mImageReader.setOnImageAvailableListener(mClosingListener,
                 CameraXExecutors.mainThreadExecutor());
 
-        ImageReaderProxy imageReaderProxy = mImageReader;
+        SafeCloseImageReaderProxy imageReaderProxy = mImageReader;
         if (mDeferrableSurface != null) {
             mDeferrableSurface.close();
         }
         mDeferrableSurface = new ImmediateSurface(mImageReader.getSurface());
         mDeferrableSurface.getTerminationFuture().addListener(
-                () -> imageReaderProxy.close(), CameraXExecutors.mainThreadExecutor());
+                imageReaderProxy::safeClose, CameraXExecutors.mainThreadExecutor());
         sessionConfigBuilder.addNonRepeatingSurface(mDeferrableSurface);
 
         sessionConfigBuilder.addErrorListener((sessionConfig, error) -> {
@@ -385,6 +391,7 @@ public final class ImageCapture extends UseCase {
         DeferrableSurface deferrableSurface = mDeferrableSurface;
         mDeferrableSurface = null;
         mImageReader = null;
+        mProcessingImageReader = null;
 
         if (deferrableSurface != null) {
             deferrableSurface.close();
@@ -1218,7 +1225,7 @@ public final class ImageCapture extends UseCase {
         final List<ListenableFuture<Void>> futureList = new ArrayList<>();
         final List<CaptureConfig> captureConfigs = new ArrayList<>();
         CaptureBundle captureBundle;
-        if (mCaptureProcessor != null) {
+        if (mProcessingImageReader != null) {
             // If the Processor is provided, check if we have valid CaptureBundle and update
             // ProcessingImageReader before actually issuing a take picture request.
             captureBundle = getCaptureBundle(null);
@@ -1233,7 +1240,7 @@ public final class ImageCapture extends UseCase {
                         "ImageCapture has CaptureStages > Max CaptureStage size"));
             }
 
-            ((ProcessingImageReader) mImageReader).setCaptureBundle(captureBundle);
+            mProcessingImageReader.setCaptureBundle(captureBundle);
         } else {
             captureBundle = getCaptureBundle(CaptureBundles.singleDefaultCaptureBundle());
             if (captureBundle.getCaptureStages().size() > 1) {
diff --git a/camera/camera-core/src/test/java/androidx/camera/core/ImageCaptureTest.kt b/camera/camera-core/src/test/java/androidx/camera/core/ImageCaptureTest.kt
index bc02dcaf197..fe2d89a6eec 100644
--- a/camera/camera-core/src/test/java/androidx/camera/core/ImageCaptureTest.kt
+++ b/camera/camera-core/src/test/java/androidx/camera/core/ImageCaptureTest.kt
@@ -150,6 +150,26 @@ class ImageCaptureTest {
     }
 
     @Test
+    fun capturedImageValidAfterUnbind() {
+        // Arrange
+        val imageCapture = bindImageCapture(
+            ViewPort.Builder(Rational(1, 1), Surface.ROTATION_0).build()
+        )
+
+        // Act
+        imageCapture.takePicture(executor, onImageCapturedCallback)
+        // Send fake image.
+        fakeImageReaderProxy?.triggerImageAvailable("tag", 0)
+        flushHandler(callbackHandler)
+        CameraX.unbind(imageCapture)
+
+        // Assert.
+        // The captured image should still be valid even if the ImageCapture has been unbound. It
+        // is the consumer of the ImageProxy who determines when the ImageProxy will be closed.
+        capturedImage?.format
+    }
+
+    @Test
     fun capturedImageSize_isEqualToSurfaceSize() {
         // Act/arrange.
         val imageCapture = bindImageCapture()
diff --git a/camera/camera-testing/src/main/java/androidx/camera/testing/fakes/FakeImageProxy.java b/camera/camera-testing/src/main/java/androidx/camera/testing/fakes/FakeImageProxy.java
index 3f47890c426..f37960dac12 100644
--- a/camera/camera-testing/src/main/java/androidx/camera/testing/fakes/FakeImageProxy.java
+++ b/camera/camera-testing/src/main/java/androidx/camera/testing/fakes/FakeImageProxy.java
@@ -39,6 +39,7 @@ public final class FakeImageProxy implements ImageProxy {
     private int mHeight = 0;
     private int mWidth = 0;
     private PlaneProxy[] mPlaneProxy = new PlaneProxy[0];
+    private boolean mClosed = false;
 
     @NonNull
     private ImageInfo mImageInfo;
@@ -59,6 +60,7 @@ public final class FakeImageProxy implements ImageProxy {
     @Override
     public void close() {
         synchronized (mReleaseLock) {
+            mClosed = true;
             if (mReleaseCompleter != null) {
                 mReleaseCompleter.set(null);
                 mReleaseCompleter = null;
@@ -69,7 +71,12 @@ public final class FakeImageProxy implements ImageProxy {
     @Override
     @NonNull
     public Rect getCropRect() {
-        return mCropRect;
+        synchronized (mReleaseLock) {
+            if (mClosed) {
+                throw new IllegalStateException("FakeImageProxy already closed");
+            }
+            return mCropRect;
+        }
     }
 
     @Override
@@ -79,23 +86,43 @@ public final class FakeImageProxy implements ImageProxy {
 
     @Override
     public int getFormat() {
-        return mFormat;
+        synchronized (mReleaseLock) {
+            if (mClosed) {
+                throw new IllegalStateException("FakeImageProxy already closed");
+            }
+            return mFormat;
+        }
     }
 
     @Override
     public int getHeight() {
-        return mHeight;
+        synchronized (mReleaseLock) {
+            if (mClosed) {
+                throw new IllegalStateException("FakeImageProxy already closed");
+            }
+            return mHeight;
+        }
     }
 
     @Override
     public int getWidth() {
-        return mWidth;
+        synchronized (mReleaseLock) {
+            if (mClosed) {
+                throw new IllegalStateException("FakeImageProxy already closed");
+            }
+            return mWidth;
+        }
     }
 
     @Override
     @NonNull
     public PlaneProxy[] getPlanes() {
-        return mPlaneProxy;
+        synchronized (mReleaseLock) {
+            if (mClosed) {
+                throw new IllegalStateException("FakeImageProxy already closed");
+            }
+            return mPlaneProxy;
+        }
     }
 
     @Override
diff --git a/camera/camera-testing/src/main/java/androidx/camera/testing/fakes/FakeImageReaderProxy.java b/camera/camera-testing/src/main/java/androidx/camera/testing/fakes/FakeImageReaderProxy.java
index 27a3452dbab..3b1177ba00f 100644
--- a/camera/camera-testing/src/main/java/androidx/camera/testing/fakes/FakeImageReaderProxy.java
+++ b/camera/camera-testing/src/main/java/androidx/camera/testing/fakes/FakeImageReaderProxy.java
@@ -27,6 +27,8 @@ import androidx.camera.core.impl.utils.executor.CameraXExecutors;
 
 import com.google.common.util.concurrent.ListenableFuture;
 
+import java.util.ArrayList;
+import java.util.List;
 import java.util.NoSuchElementException;
 import java.util.concurrent.BlockingQueue;
 import java.util.concurrent.Executor;
@@ -56,6 +58,10 @@ public class FakeImageReaderProxy implements ImageReaderProxy {
     // Queue of ImageProxys which have not yet been acquired.
     private BlockingQueue<ImageProxy> mImageProxyAcquisitionQueue;
 
+    // List of all ImageProxy which have been acquired. Close them all once the ImageReader is
+    // closed
+    private List<ImageProxy> mOutboundImageProxy = new ArrayList<>();
+
     @SuppressWarnings("WeakerAccess") /* synthetic accessor */
     @Nullable
     ImageReaderProxy.OnImageAvailableListener mListener;
@@ -88,19 +94,23 @@ public class FakeImageReaderProxy implements ImageReaderProxy {
 
     @Override
     public ImageProxy acquireLatestImage() {
-        ImageProxy imageProxy;
+        ImageProxy imageProxy = null;
 
         try {
             // Remove and close all ImageProxy aside from last one
             do {
+                if (imageProxy != null) {
+                    imageProxy.close();
+                }
                 imageProxy = mImageProxyAcquisitionQueue.remove();
-                imageProxy.close();
             } while (mImageProxyAcquisitionQueue.size() > 1);
         } catch (NoSuchElementException e) {
             throw new IllegalStateException(
                     "Unable to acquire latest image from empty FakeImageReader");
         }
 
+        mOutboundImageProxy.add(imageProxy);
+
         return imageProxy;
     }
 
@@ -120,6 +130,9 @@ public class FakeImageReaderProxy implements ImageReaderProxy {
 
     @Override
     public void close() {
+        for (ImageProxy imageProxy : mOutboundImageProxy) {
+            imageProxy.close();
+        }
         mIsClosed = true;
     }