Camera2: Fix session shutdown race, frequent warning log

- Make sure that session.close followed by device.createCaptureSession cannot race on
  configureOutputs calls
- Silence warning about RAW_OPAQUE format

Change-Id: I02e4a048e8b26ea61aadcf115b029e9fbb58ad4e

diff --git a/core/java/android/hardware/camera2/impl/CameraCaptureSessionImpl.java b/core/java/android/hardware/camera2/impl/CameraCaptureSessionImpl.java
index dffff9a..a15028c 100644 (file)
--- a/core/java/android/hardware/camera2/impl/CameraCaptureSessionImpl.java
+++ b/core/java/android/hardware/camera2/impl/CameraCaptureSessionImpl.java
@@ -261,9 +261,12 @@ public class CameraCaptureSessionImpl extends CameraCaptureSession {
      * <p>The semantics are identical to {@link #close}, except that unconfiguring will be skipped.
      * <p>
      *
+     * <p>After this call completes, the session will not call any further methods on the camera
+     * device.</p>
+     *
      * @see CameraCaptureSession#close
      */
-    synchronized void replaceSessionClose(CameraCaptureSession other) {
+    synchronized void replaceSessionClose() {
         /*
          * In order for creating new sessions to be fast, the new session should be created
          * before the old session is closed.
@@ -278,13 +281,17 @@ public class CameraCaptureSessionImpl extends CameraCaptureSession {
 
         if (VERBOSE) Log.v(TAG, "replaceSessionClose");
 
-        // #close was already called explicitly, keep going the slow route
-        if (mClosed) {
-            if (VERBOSE) Log.v(TAG, "replaceSessionClose - close was already called");
-            return;
-        }
-
+        // Set up fast shutdown. Possible alternative paths:
+        // - This session is active, so close() below starts the shutdown drain
+        // - This session is mid-shutdown drain, and hasn't yet reached the idle drain listener.
+        // - This session is already closed and has executed the idle drain listener, and
+        //   configureOutputs(null) has already been called.
+        //
+        // Do not call configureOutputs(null) going forward, since it would race with the
+        // configuration for the new session. If it was already called, then we don't care, since it
+        // won't get called again.
         mSkipUnconfigure = true;
+
         close();
     }
 
@@ -599,6 +606,8 @@ public class CameraCaptureSessionImpl extends CameraCaptureSession {
                  *
                  * This operation is idempotent; a session will not be closed twice.
                  */
+                if (VERBOSE) Log.v(TAG, "Session drain complete, skip unconfigure: " +
+                        mSkipUnconfigure);
 
                 // Fast path: A new capture session has replaced this one; don't unconfigure.
                 if (mSkipUnconfigure) {

diff --git a/core/java/android/hardware/camera2/impl/CameraDeviceImpl.java b/core/java/android/hardware/camera2/impl/CameraDeviceImpl.java
index 63e9644..71eb0e9 100644 (file)
--- a/core/java/android/hardware/camera2/impl/CameraDeviceImpl.java
+++ b/core/java/android/hardware/camera2/impl/CameraDeviceImpl.java
@@ -387,7 +387,11 @@ public class CameraDeviceImpl extends CameraDevice {
 
             checkIfCameraClosedOrInError();
 
-            // TODO: we must be in UNCONFIGURED mode to begin with, or using another session
+            // Notify current session that it's going away, before starting camera operations
+            // After this call completes, the session is not allowed to call into CameraDeviceImpl
+            if (mCurrentSession != null) {
+                mCurrentSession.replaceSessionClose();
+            }
 
             // TODO: dont block for this
             boolean configureSuccess = true;
@@ -407,10 +411,6 @@ public class CameraDeviceImpl extends CameraDevice {
                     new CameraCaptureSessionImpl(outputs, listener, handler, this, mDeviceHandler,
                             configureSuccess);
 
-            if (mCurrentSession != null) {
-                mCurrentSession.replaceSessionClose(newSession);
-            }
-
             // TODO: wait until current session closes, then create the new session
             mCurrentSession = newSession;
 

diff --git a/core/java/android/hardware/camera2/params/StreamConfigurationMap.java b/core/java/android/hardware/camera2/params/StreamConfigurationMap.java
index 1efabb1..2e6b9ae 100644 (file)
--- a/core/java/android/hardware/camera2/params/StreamConfigurationMap.java
+++ b/core/java/android/hardware/camera2/params/StreamConfigurationMap.java
@@ -813,6 +813,7 @@ public final class StreamConfigurationMap {
         switch (format) {
             case HAL_PIXEL_FORMAT_IMPLEMENTATION_DEFINED:
             case HAL_PIXEL_FORMAT_BLOB:
+            case HAL_PIXEL_FORMAT_RAW_OPAQUE:
                 return format;
             case ImageFormat.JPEG:
                 throw new IllegalArgumentException(
@@ -843,12 +844,6 @@ public final class StreamConfigurationMap {
      * @throws IllegalArgumentException if the format was not user-defined
      */
     static int checkArgumentFormat(int format) {
-        // TODO: remove this hack , CTS shouldn't have been using internal constants
-        if (format == HAL_PIXEL_FORMAT_RAW_OPAQUE) {
-            Log.w(TAG, "RAW_OPAQUE is not yet a published format; allowing it anyway");
-            return format;
-        }
-
         if (!ImageFormat.isPublicFormat(format) && !PixelFormat.isPublicFormat(format)) {
             throw new IllegalArgumentException(String.format(
                     "format 0x%x was not defined in either ImageFormat or PixelFormat", format));