Camera2: Correct error handling

- Report capture failures from service to application
- Only go to error state for device-level errors
- Adjust binder interface method names to match the service side names
- Reduce failed session creation logging
- Don't fire CaptureSession.onActive for CameraDevice.onBusy
- Check with session to determine capture failure reason

Bug: 17160301
Bug: 15524101
Bug: 14448494
Bug: 11272459

Change-Id: I9dd606004fd7845910dc865738fbe17f1640f07d

5 files changed, 114 insertions, 27 deletions

diff --git a/core/java/android/hardware/camera2/ICameraDeviceCallbacks.aidl b/core/java/android/hardware/camera2/ICameraDeviceCallbacks.aidl
index caabed3..ca0935c 100644
--- a/core/java/android/hardware/camera2/ICameraDeviceCallbacks.aidl
+++ b/core/java/android/hardware/camera2/ICameraDeviceCallbacks.aidl
@@ -26,8 +26,8 @@ interface ICameraDeviceCallbacks
      * Keep up-to-date with frameworks/av/include/camera/camera2/ICameraDeviceCallbacks.h
      */
 
-    oneway void onCameraError(int errorCode, in CaptureResultExtras resultExtras);
-    oneway void onCameraIdle();
+    oneway void onDeviceError(int errorCode, in CaptureResultExtras resultExtras);
+    oneway void onDeviceIdle();
     oneway void onCaptureStarted(in CaptureResultExtras resultExtras, long timestamp);
     oneway void onResultReceived(in CameraMetadataNative result,
                                  in CaptureResultExtras resultExtras);
diff --git a/core/java/android/hardware/camera2/impl/CameraCaptureSessionImpl.java b/core/java/android/hardware/camera2/impl/CameraCaptureSessionImpl.java
index 621968b..9ca1fba 100644
--- a/core/java/android/hardware/camera2/impl/CameraCaptureSessionImpl.java
+++ b/core/java/android/hardware/camera2/impl/CameraCaptureSessionImpl.java
@@ -74,7 +74,7 @@ public class CameraCaptureSessionImpl extends CameraCaptureSession {
     private boolean mSkipUnconfigure = false;
 
     /** Is the session in the process of aborting? Pay attention to BUSY->IDLE transitions. */
-    private boolean mAborting;
+    private volatile boolean mAborting;
 
     /**
      * Create a new CameraCaptureSession.
@@ -346,6 +346,20 @@ public class CameraCaptureSessionImpl extends CameraCaptureSession {
     }
 
     /**
+     * Whether currently in mid-abort.
+     *
+     * <p>This is used by the implementation to set the capture failure
+     * reason, in lieu of more accurate error codes from the camera service.
+     * Unsynchronized to avoid deadlocks between simultaneous session->device,
+     * device->session calls.</p>
+     *
+     * <p>Package-private.</p>
+     */
+    boolean isAborting() {
+        return mAborting;
+    }
+
+    /**
      * Post calls into a CameraCaptureSession.StateListener to the user-specified {@code handler}.
      */
     private StateListener createUserStateListenerProxy(Handler handler, StateListener listener) {
@@ -502,8 +516,8 @@ public class CameraCaptureSessionImpl extends CameraCaptureSession {
 
                 // TODO: Queue captures during abort instead of failing them
                 // since the app won't be able to distinguish the two actives
+                // Don't signal the application since there's no clean mapping here
                 Log.w(TAG, "Device is now busy; do not submit new captures (TODO: allow this)");
-                mStateListener.onActive(session);
             }
 
             @Override
diff --git a/core/java/android/hardware/camera2/impl/CameraDeviceImpl.java b/core/java/android/hardware/camera2/impl/CameraDeviceImpl.java
index 79ce9df..513d222 100644
--- a/core/java/android/hardware/camera2/impl/CameraDeviceImpl.java
+++ b/core/java/android/hardware/camera2/impl/CameraDeviceImpl.java
@@ -384,7 +384,7 @@ public class CameraDeviceImpl extends CameraDevice {
                 catch (IllegalArgumentException e) {
                     // OK. camera service can reject stream config if it's not supported by HAL
                     // This is only the result of a programmer misusing the camera2 api.
-                    Log.e(TAG, "Stream configuration failed", e);
+                    Log.w(TAG, "Stream configuration failed");
                     return false;
                 }
 
@@ -1097,31 +1097,51 @@ public class CameraDeviceImpl extends CameraDevice {
          */
         static final int ERROR_CAMERA_SERVICE = 2;
 
+        /**
+         * Camera has encountered an error processing a single request.
+         */
+        static final int ERROR_CAMERA_REQUEST = 3;
+
+        /**
+         * Camera has encountered an error producing metadata for a single capture
+         */
+        static final int ERROR_CAMERA_RESULT = 4;
+
+        /**
+         * Camera has encountered an error producing an image buffer for a single capture
+         */
+        static final int ERROR_CAMERA_BUFFER = 5;
+
         @Override
         public IBinder asBinder() {
             return this;
         }
 
         @Override
-        public void onCameraError(final int errorCode, CaptureResultExtras resultExtras) {
-            Runnable r = null;
+        public void onDeviceError(final int errorCode, CaptureResultExtras resultExtras) {
+            if (DEBUG) {
+                Log.d(TAG, String.format(
+                    "Device error received, code %d, frame number %d, request ID %d, subseq ID %d",
+                    errorCode, resultExtras.getFrameNumber(), resultExtras.getRequestId(),
+                    resultExtras.getSubsequenceId()));
+            }
 
             synchronized(mInterfaceLock) {
                 if (mRemoteDevice == null) {
                     return; // Camera already closed
                 }
 
-                mInError = true;
                 switch (errorCode) {
                     case ERROR_CAMERA_DISCONNECTED:
-                        r = mCallOnDisconnected;
+                        CameraDeviceImpl.this.mDeviceHandler.post(mCallOnDisconnected);
                         break;
                     default:
                         Log.e(TAG, "Unknown error from camera device: " + errorCode);
                         // no break
                     case ERROR_CAMERA_DEVICE:
                     case ERROR_CAMERA_SERVICE:
-                        r = new Runnable() {
+                        mInError = true;
+                        Runnable r = new Runnable() {
                             @Override
                             public void run() {
                                 if (!CameraDeviceImpl.this.isClosed()) {
@@ -1129,21 +1149,19 @@ public class CameraDeviceImpl extends CameraDevice {
                                 }
                             }
                         };
+                        CameraDeviceImpl.this.mDeviceHandler.post(r);
+                        break;
+                    case ERROR_CAMERA_REQUEST:
+                    case ERROR_CAMERA_RESULT:
+                    case ERROR_CAMERA_BUFFER:
+                        onCaptureErrorLocked(errorCode, resultExtras);
                         break;
                 }
-                CameraDeviceImpl.this.mDeviceHandler.post(r);
-
-                // Fire onCaptureSequenceCompleted
-                if (DEBUG) {
-                    Log.v(TAG, String.format("got error frame %d", resultExtras.getFrameNumber()));
-                }
-                mFrameNumberTracker.updateTracker(resultExtras.getFrameNumber(), /*error*/true);
-                checkAndFireSequenceComplete();
             }
         }
 
         @Override
-        public void onCameraIdle() {
+        public void onDeviceIdle() {
             if (DEBUG) {
                 Log.d(TAG, "Camera now idle");
             }
@@ -1246,7 +1264,6 @@ public class CameraDeviceImpl extends CameraDevice {
 
                 final CaptureRequest request = holder.getRequest(resultExtras.getSubsequenceId());
 
-
                 Runnable resultDispatch = null;
 
                 // Either send a partial result or the final capture completed result
@@ -1290,11 +1307,67 @@ public class CameraDeviceImpl extends CameraDevice {
                 if (!isPartialResult) {
                     checkAndFireSequenceComplete();
                 }
+            }
+        }
 
+        /**
+         * Called by onDeviceError for handling single-capture failures.
+         */
+        private void onCaptureErrorLocked(int errorCode, CaptureResultExtras resultExtras) {
+
+            final int requestId = resultExtras.getRequestId();
+            final int subsequenceId = resultExtras.getSubsequenceId();
+            final long frameNumber = resultExtras.getFrameNumber();
+            final CaptureListenerHolder holder =
+                    CameraDeviceImpl.this.mCaptureListenerMap.get(requestId);
+
+            final CaptureRequest request = holder.getRequest(subsequenceId);
+
+            // No way to report buffer errors right now
+            if (errorCode == ERROR_CAMERA_BUFFER) {
+                Log.e(TAG, String.format("Lost output buffer reported for frame %d", frameNumber));
+                return;
+            }
+
+            boolean mayHaveBuffers = (errorCode == ERROR_CAMERA_RESULT);
+
+            // This is only approximate - exact handling needs the camera service and HAL to
+            // disambiguate between request failures to due abort and due to real errors.
+            // For now, assume that if the session believes we're mid-abort, then the error
+            // is due to abort.
+            int reason = (mCurrentSession != null && mCurrentSession.isAborting()) ?
+                    CaptureFailure.REASON_FLUSHED :
+                    CaptureFailure.REASON_ERROR;
+
+            final CaptureFailure failure = new CaptureFailure(
+                request,
+                reason,
+                /*dropped*/ mayHaveBuffers,
+                requestId,
+                frameNumber);
+
+            Runnable failureDispatch = new Runnable() {
+                @Override
+                public void run() {
+                    if (!CameraDeviceImpl.this.isClosed()){
+                        holder.getListener().onCaptureFailed(
+                            CameraDeviceImpl.this,
+                            request,
+                            failure);
+                    }
+                }
+            };
+            holder.getHandler().post(failureDispatch);
+
+            // Fire onCaptureSequenceCompleted if appropriate
+            if (DEBUG) {
+                Log.v(TAG, String.format("got error frame %d", frameNumber));
             }
+            mFrameNumberTracker.updateTracker(frameNumber, /*error*/true);
+            checkAndFireSequenceComplete();
         }
 
-    }
+    } // public class CameraDeviceCallbacks
 
     /**
      * Default handler management.
diff --git a/core/java/android/hardware/camera2/legacy/CameraDeviceUserShim.java b/core/java/android/hardware/camera2/legacy/CameraDeviceUserShim.java
index 5cbf109..410934e 100644
--- a/core/java/android/hardware/camera2/legacy/CameraDeviceUserShim.java
+++ b/core/java/android/hardware/camera2/legacy/CameraDeviceUserShim.java
@@ -211,7 +211,7 @@ public class CameraDeviceUserShim implements ICameraDeviceUser {
         }
 
         @Override
-        public void onCameraError(final int errorCode, final CaptureResultExtras resultExtras) {
+        public void onDeviceError(final int errorCode, final CaptureResultExtras resultExtras) {
             Message msg = getHandler().obtainMessage(CAMERA_ERROR,
                 /*arg1*/ errorCode, /*arg2*/ 0,
                 /*obj*/ resultExtras);
@@ -219,7 +219,7 @@ public class CameraDeviceUserShim implements ICameraDeviceUser {
         }
 
         @Override
-        public void onCameraIdle() {
+        public void onDeviceIdle() {
             Message msg = getHandler().obtainMessage(CAMERA_IDLE);
             getHandler().sendMessage(msg);
         }
@@ -267,11 +267,11 @@ public class CameraDeviceUserShim implements ICameraDeviceUser {
                         case CAMERA_ERROR: {
                             int errorCode = msg.arg1;
                             CaptureResultExtras resultExtras = (CaptureResultExtras) msg.obj;
-                            mCallbacks.onCameraError(errorCode, resultExtras);
+                            mCallbacks.onDeviceError(errorCode, resultExtras);
                             break;
                         }
                         case CAMERA_IDLE:
-                            mCallbacks.onCameraIdle();
+                            mCallbacks.onDeviceIdle();
                             break;
                         case CAPTURE_STARTED: {
                             long timestamp = msg.arg2 & 0xFFFFFFFFL;
diff --git a/core/java/android/hardware/camera2/legacy/LegacyCameraDevice.java b/core/java/android/hardware/camera2/legacy/LegacyCameraDevice.java
index 1cf7797..ffc55f1 100644
--- a/core/java/android/hardware/camera2/legacy/LegacyCameraDevice.java
+++ b/core/java/android/hardware/camera2/legacy/LegacyCameraDevice.java
@@ -97,7 +97,7 @@ public class LegacyCameraDevice implements AutoCloseable {
                         Log.d(TAG, "doing onError callback.");
                     }
                     try {
-                        mDeviceCallbacks.onCameraError(errorCode, extras);
+                        mDeviceCallbacks.onDeviceError(errorCode, extras);
                     } catch (RemoteException e) {
                         throw new IllegalStateException(
                                 "Received remote exception during onCameraError callback: ", e);
@@ -125,7 +125,7 @@ public class LegacyCameraDevice implements AutoCloseable {
                         Log.d(TAG, "doing onIdle callback.");
                     }
                     try {
-                        mDeviceCallbacks.onCameraIdle();
+                        mDeviceCallbacks.onDeviceIdle();
                     } catch (RemoteException e) {
                         throw new IllegalStateException(
                                 "Received remote exception during onCameraIdle callback: ", e);