Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 95037853 authored by Eino-Ville Talvala's avatar Eino-Ville Talvala
Browse files

Camera2: Clean up corner case error handling 

- If a session is closed, and a new session is created immediately
afterwards, but then fails to be configured, the first session sees an
onUnconfigured call which it wasn't expecting, and throws an
exception on an internal thread, leading to app death.
Add a guard against this case.

- If the lower levels skip a frame (illegal per design), be slightly more
robust to that by accepting any successful result as the latest completed
frame, instead of just incrementing the completed frame count. This will
lead to missing results, but should allow shutdown, etc, to complete
cleanly.

- Convert TIMED_OUT error codes to CAMERA_ERROR CameraAccessExceptions.
This is a common error code returned by waitUntilIdle.

Also, improve debug logging to log a session ID with verbose logging,
and add a few verbose logs.

Bug: 16899526
Change-Id: I7a31f0a12effc2611e1f9c2408224ee82c37c912
parent 8eec09a1

core/java/android/hardware/camera2/impl/CameraCaptureSessionImpl.java

+56 −25
Original line number Diff line number Diff line
@@ -42,6 +42,10 @@ public class CameraCaptureSessionImpl extends CameraCaptureSession { 
    private static final String TAG = "CameraCaptureSession";

    private static final boolean VERBOSE = Log.isLoggable(TAG, Log.VERBOSE);



    /** Simple integer ID for session for debugging */

    private final int mId;

    private final String mIdString;



    /** User-specified set of surfaces used as the configuration outputs */

    private final List<Surface> mOutputs;

    /**
@@ -83,7 +87,7 @@ public class CameraCaptureSessionImpl extends CameraCaptureSession { 
     * There must be no pending actions

     * (e.g. no pending captures, no repeating requests, no flush).</p>

     */

    CameraCaptureSessionImpl(List<Surface> outputs,

    CameraCaptureSessionImpl(int id, List<Surface> outputs,

            CameraCaptureSession.StateCallback callback, Handler stateHandler,

            android.hardware.camera2.impl.CameraDeviceImpl deviceImpl,

            Handler deviceStateHandler, boolean configureSuccess) {
@@ -93,6 +97,9 @@ public class CameraCaptureSessionImpl extends CameraCaptureSession { 
            throw new IllegalArgumentException("callback must not be null");

        }



        mId = id;

        mIdString = String.format("Session %d: ", mId);



        // TODO: extra verification of outputs

        mOutputs = outputs;

        mStateHandler = checkHandler(stateHandler);
@@ -120,12 +127,12 @@ public class CameraCaptureSessionImpl extends CameraCaptureSession { 


        if (configureSuccess) {

            mStateCallback.onConfigured(this);

            if (VERBOSE) Log.v(TAG, "ctor - Created session successfully");

            if (VERBOSE) Log.v(TAG, mIdString + "Created session successfully");

            mConfigureSuccess = true;

        } else {

            mStateCallback.onConfigureFailed(this);

            mClosed = true; // do not fire any other callbacks, do not allow any other work

            Log.e(TAG, "Failed to create capture session; configuration failed");

            Log.e(TAG, mIdString + "Failed to create capture session; configuration failed");

            mConfigureSuccess = false;

        }

    }
@@ -147,8 +154,8 @@ public class CameraCaptureSessionImpl extends CameraCaptureSession { 
        handler = checkHandler(handler, callback);



        if (VERBOSE) {

            Log.v(TAG, "capture - request " + request + ", callback " + callback + " handler" +

                    " " + handler);

            Log.v(TAG, mIdString + "capture - request " + request + ", callback " + callback +

                    " handler " + handler);

        }



        return addPendingSequence(mDeviceImpl.capture(request,
@@ -170,8 +177,8 @@ public class CameraCaptureSessionImpl extends CameraCaptureSession { 


        if (VERBOSE) {

            CaptureRequest[] requestArray = requests.toArray(new CaptureRequest[0]);

            Log.v(TAG, "captureBurst - requests " + Arrays.toString(requestArray) + ", callback " +

                    callback + " handler" + "" + handler);

            Log.v(TAG, mIdString + "captureBurst - requests " + Arrays.toString(requestArray) +

                    ", callback " + callback + " handler " + handler);

        }



        return addPendingSequence(mDeviceImpl.captureBurst(requests,
@@ -190,8 +197,8 @@ public class CameraCaptureSessionImpl extends CameraCaptureSession { 
        handler = checkHandler(handler, callback);



        if (VERBOSE) {

            Log.v(TAG, "setRepeatingRequest - request " + request + ", callback " + callback +

                    " handler" + " " + handler);

            Log.v(TAG, mIdString + "setRepeatingRequest - request " + request + ", callback " +

                    callback + " handler" + " " + handler);

        }



        return addPendingSequence(mDeviceImpl.setRepeatingRequest(request,
@@ -213,7 +220,7 @@ public class CameraCaptureSessionImpl extends CameraCaptureSession { 


        if (VERBOSE) {

            CaptureRequest[] requestArray = requests.toArray(new CaptureRequest[0]);

            Log.v(TAG, "setRepeatingBurst - requests " + Arrays.toString(requestArray) +

            Log.v(TAG, mIdString + "setRepeatingBurst - requests " + Arrays.toString(requestArray) +

                    ", callback " + callback + " handler" + "" + handler);

        }
@@ -226,7 +233,7 @@ public class CameraCaptureSessionImpl extends CameraCaptureSession { 
        checkNotClosed();



        if (VERBOSE) {

            Log.v(TAG, "stopRepeating");

            Log.v(TAG, mIdString + "stopRepeating");

        }



        mDeviceImpl.stopRepeating();
@@ -237,11 +244,11 @@ public class CameraCaptureSessionImpl extends CameraCaptureSession { 
        checkNotClosed();



        if (VERBOSE) {

            Log.v(TAG, "abortCaptures");

            Log.v(TAG, mIdString + "abortCaptures");

        }



        if (mAborting) {

            Log.w(TAG, "abortCaptures - Session is already aborting; doing nothing");

            Log.w(TAG, mIdString + "abortCaptures - Session is already aborting; doing nothing");

            return;

        }
@@ -279,7 +286,7 @@ public class CameraCaptureSessionImpl extends CameraCaptureSession { 
         * but this would introduce nondeterministic behavior.

         */



        if (VERBOSE) Log.v(TAG, "replaceSessionClose");

        if (VERBOSE) Log.v(TAG, mIdString + "replaceSessionClose");



        // Set up fast shutdown. Possible alternative paths:

        // - This session is active, so close() below starts the shutdown drain
@@ -299,11 +306,11 @@ public class CameraCaptureSessionImpl extends CameraCaptureSession { 
    public synchronized void close() {



        if (mClosed) {

            if (VERBOSE) Log.v(TAG, "close - reentering");

            if (VERBOSE) Log.v(TAG, mIdString + "close - reentering");

            return;

        }



        if (VERBOSE) Log.v(TAG, "close - first time");

        if (VERBOSE) Log.v(TAG, mIdString + "close - first time");



        mClosed = true;
@@ -321,7 +328,7 @@ public class CameraCaptureSessionImpl extends CameraCaptureSession { 
            mDeviceImpl.stopRepeating();

        } catch (IllegalStateException e) {

            // OK: Camera device may already be closed, nothing else to do

            Log.w(TAG, "The camera device was already closed: ", e);

            Log.w(TAG, mIdString + "The camera device was already closed: ", e);



            // TODO: Fire onClosed anytime we get the device onClosed or the ISE?

            // or just suppress the ISE only and rely onClosed.
@@ -332,7 +339,7 @@ public class CameraCaptureSessionImpl extends CameraCaptureSession { 
            return;

        } catch (CameraAccessException e) {

            // OK: close does not throw checked exceptions.

            Log.e(TAG, "Exception while stopping repeating: ", e);

            Log.e(TAG, mIdString + "Exception while stopping repeating: ", e);



            // TODO: call onError instead of onClosed if this happens

        }
@@ -453,13 +460,14 @@ public class CameraCaptureSessionImpl extends CameraCaptureSession { 


            @Override

            public void onDisconnected(CameraDevice camera) {

                if (VERBOSE) Log.v(TAG, mIdString + "onDisconnected");

                close();

            }



            @Override

            public void onError(CameraDevice camera, int error) {

                // TODO: Handle errors somehow.

                Log.wtf(TAG, "Got device error " + error);

                // Should not be reached, handled by device code

                Log.wtf(TAG, mIdString + "Got device error " + error);

            }



            @Override
@@ -467,12 +475,15 @@ public class CameraCaptureSessionImpl extends CameraCaptureSession { 
                mIdleDrainer.taskStarted();

                mActive = true;



                if (VERBOSE) Log.v(TAG, mIdString + "onActive");

                mStateCallback.onActive(session);

            }



            @Override

            public void onIdle(CameraDevice camera) {

                boolean isAborting;

                if (VERBOSE) Log.v(TAG, mIdString + "onIdle");



                synchronized (session) {

                    isAborting = mAborting;

                }
@@ -513,14 +524,29 @@ 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)");

                if (VERBOSE) Log.v(TAG, mIdString + "onBusy");

            }



            @Override

            public void onUnconfigured(CameraDevice camera) {

                if (VERBOSE) Log.v(TAG, mIdString + "onUnconfigured");

                synchronized (session) {

                    // Ignore #onUnconfigured before #close is called

                    if (mClosed && mConfigureSuccess) {

                    // Ignore #onUnconfigured before #close is called.

                    //

                    // Normally, this is reached when this session is closed and no immediate other

                    // activity happens for the camera, in which case the camera is configured to

                    // null streams by this session and the UnconfigureDrainer task is started.

                    // However, we can also end up here if

                    //

                    // 1) Session is closed

                    // 2) New session is created before this session finishes closing, setting

                    //    mSkipUnconfigure and therefore this session does not configure null or

                    //    start the UnconfigureDrainer task.

                    // 3) And then the new session fails to be created, so onUnconfigured fires

                    //    _anyway_.

                    // In this second case, need to not finish a task that was never started, so

                    // guard with mSkipUnconfigure

                    if (mClosed && mConfigureSuccess && !mSkipUnconfigure) {

                        mUnconfigureDrainer.taskFinished();

                    }

                }
@@ -580,6 +606,7 @@ public class CameraCaptureSessionImpl extends CameraCaptureSession { 
             * If the camera is already "IDLE" and no aborts are pending,

             * then the drain immediately finishes.

             */

            if (VERBOSE) Log.v(TAG, mIdString + "onSequenceDrained");

            mAbortDrainer.beginDrain();

        }

    }
@@ -587,6 +614,7 @@ public class CameraCaptureSessionImpl extends CameraCaptureSession { 
    private class AbortDrainListener implements TaskDrainer.DrainListener {

        @Override

        public void onDrained() {

            if (VERBOSE) Log.v(TAG, mIdString + "onAbortDrained");

            synchronized (CameraCaptureSessionImpl.this) {

                /*

                 * Any queued aborts have now completed.
@@ -604,6 +632,7 @@ public class CameraCaptureSessionImpl extends CameraCaptureSession { 
    private class IdleDrainListener implements TaskDrainer.DrainListener {

        @Override

        public void onDrained() {

            if (VERBOSE) Log.v(TAG, mIdString + "onIdleDrained");

            synchronized (CameraCaptureSessionImpl.this) {

                /*

                 * The device is now IDLE, and has settled. It will not transition to
@@ -613,7 +642,7 @@ 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: " +

                if (VERBOSE) Log.v(TAG, mIdString + "Session drain complete, skip unconfigure: " +

                        mSkipUnconfigure);



                // Fast path: A new capture session has replaced this one; don't unconfigure.
@@ -629,7 +658,7 @@ public class CameraCaptureSessionImpl extends CameraCaptureSession { 
                    mDeviceImpl.configureOutputsChecked(null); // begin transition to unconfigured

                } catch (CameraAccessException e) {

                    // OK: do not throw checked exceptions.

                    Log.e(TAG, "Exception while configuring outputs: ", e);

                    Log.e(TAG, mIdString + "Exception while configuring outputs: ", e);



                    // TODO: call onError instead of onClosed if this happens

                }
@@ -641,7 +670,9 @@ public class CameraCaptureSessionImpl extends CameraCaptureSession { 


    private class UnconfigureDrainListener implements TaskDrainer.DrainListener {

        @Override



        public void onDrained() {

            if (VERBOSE) Log.v(TAG, mIdString + "onUnconfigureDrained");

            synchronized (CameraCaptureSessionImpl.this) {

                // The device has finished unconfiguring. It's now fully closed.

                mStateCallback.onClosed(CameraCaptureSessionImpl.this);

core/java/android/hardware/camera2/impl/CameraDeviceImpl.java

+6 −2
Original line number Diff line number Diff line
@@ -99,6 +99,7 @@ public class CameraDeviceImpl extends CameraDevice { 
    private final FrameNumberTracker mFrameNumberTracker = new FrameNumberTracker();



    private CameraCaptureSessionImpl mCurrentSession;

    private int mNextSessionId = 0;



    // Runnables for all state transitions, except error, which needs the

    // error code argument
@@ -445,7 +446,8 @@ public class CameraDeviceImpl extends CameraDevice { 


            // Fire onConfigured if configureOutputs succeeded, fire onConfigureFailed otherwise.

            CameraCaptureSessionImpl newSession =

                    new CameraCaptureSessionImpl(outputs, callback, handler, this, mDeviceHandler,

                    new CameraCaptureSessionImpl(mNextSessionId++,

                            outputs, callback, handler, this, mDeviceHandler,

                            configureSuccess);



            // TODO: wait until current session closes, then create the new session
@@ -1003,8 +1005,10 @@ public class CameraDeviceImpl extends CameraDevice { 
                    Log.e(TAG, String.format(

                            "result frame number %d comes out of order, should be %d + 1",

                            frameNumber, mCompletedFrameNumber));

                    // Continue on to set the completed frame number to this frame anyway,

                    // to be robust to lower-level errors and allow for clean shutdowns.

                }

                mCompletedFrameNumber++;

                mCompletedFrameNumber = frameNumber;

            }

            update();

        }

core/java/android/hardware/camera2/utils/CameraBinderDecorator.java

+6 −1
Original line number Diff line number Diff line
@@ -19,6 +19,7 @@ package android.hardware.camera2.utils; 
import static android.hardware.camera2.CameraAccessException.CAMERA_DISABLED;

import static android.hardware.camera2.CameraAccessException.CAMERA_DISCONNECTED;

import static android.hardware.camera2.CameraAccessException.CAMERA_IN_USE;

import static android.hardware.camera2.CameraAccessException.CAMERA_ERROR;

import static android.hardware.camera2.CameraAccessException.MAX_CAMERAS_IN_USE;

import static android.hardware.camera2.CameraAccessException.CAMERA_DEPRECATED_HAL;
@@ -41,6 +42,7 @@ public class CameraBinderDecorator { 
    public static final int BAD_VALUE = -22;

    public static final int DEAD_OBJECT = -32;

    public static final int INVALID_OPERATION = -38;

    public static final int TIMED_OUT = -110;



    /**

     * TODO: add as error codes in Errors.h
@@ -112,6 +114,9 @@ public class CameraBinderDecorator { 
                throw new IllegalArgumentException("Bad argument passed to camera service");

            case DEAD_OBJECT:

                throw new CameraRuntimeException(CAMERA_DISCONNECTED);

            case TIMED_OUT:

                throw new CameraRuntimeException(CAMERA_ERROR,

                        "Operation timed out in camera service");

            case EACCES:

                throw new CameraRuntimeException(CAMERA_DISABLED);

            case EBUSY:
@@ -123,7 +128,7 @@ public class CameraBinderDecorator { 
            case EOPNOTSUPP:

                throw new CameraRuntimeException(CAMERA_DEPRECATED_HAL);

            case INVALID_OPERATION:

                throw new IllegalStateException(

                throw new CameraRuntimeException(CAMERA_ERROR,

                        "Illegal state encountered in camera service.");

        }