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

Commit 49b2b135 authored by Igor Murashkin's avatar Igor Murashkin Committed by Ruben Brunk
Browse files

camera2: Fix deadlocks in shim #close and make #testInvalidCapture pass 

* Also fixes configureOutputs to allow it to unconfigure
* Adds IAE checks in a few spots to validate surfaces aren't null

Bug: 15116722
Change-Id: I9ec88bccb3600eb12747d84436ead27952e87646
parent b0586c92

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

+160 −116
Original line number Diff line number Diff line
@@ -28,6 +28,8 @@ import android.hardware.camera2.ICameraDeviceUser; 
import android.hardware.camera2.TotalCaptureResult;

import android.hardware.camera2.utils.CameraBinderDecorator;

import android.hardware.camera2.utils.CameraRuntimeException;

import android.hardware.camera2.utils.CloseableLock;

import android.hardware.camera2.utils.CloseableLock.ScopedLock;

import android.hardware.camera2.utils.LongParcelable;

import android.os.Handler;

import android.os.IBinder;
@@ -57,13 +59,14 @@ public class CameraDeviceImpl extends android.hardware.camera2.CameraDevice { 
    // TODO: guard every function with if (!mRemoteDevice) check (if it was closed)

    private ICameraDeviceUser mRemoteDevice;



    private final Object mLock = new Object();

    private final CloseableLock mCloseLock;

    private final CameraDeviceCallbacks mCallbacks = new CameraDeviceCallbacks();



    private final StateListener mDeviceListener;

    private volatile StateListener mSessionStateListener;

    private final Handler mDeviceHandler;



    private volatile boolean mClosing = false;

    private boolean mInError = false;

    private boolean mIdle = true;
@@ -100,7 +103,9 @@ public class CameraDeviceImpl extends android.hardware.camera2.CameraDevice { 
    private final Runnable mCallOnOpened = new Runnable() {

        @Override

        public void run() {

            if (!CameraDeviceImpl.this.isClosed()) {

            try (ScopedLock scopedLock = mCloseLock.acquireLock()) {

                if (scopedLock == null) return; // Camera already closed



                StateListener sessionListener = mSessionStateListener;

                if (sessionListener != null) {

                    sessionListener.onOpened(CameraDeviceImpl.this);
@@ -113,7 +118,9 @@ public class CameraDeviceImpl extends android.hardware.camera2.CameraDevice { 
    private final Runnable mCallOnUnconfigured = new Runnable() {

        @Override

        public void run() {

            if (!CameraDeviceImpl.this.isClosed()) {

            try (ScopedLock scopedLock = mCloseLock.acquireLock()) {

                if (scopedLock == null) return; // Camera already closed



                StateListener sessionListener = mSessionStateListener;

                if (sessionListener != null) {

                    sessionListener.onUnconfigured(CameraDeviceImpl.this);
@@ -126,7 +133,9 @@ public class CameraDeviceImpl extends android.hardware.camera2.CameraDevice { 
    private final Runnable mCallOnActive = new Runnable() {

        @Override

        public void run() {

            if (!CameraDeviceImpl.this.isClosed()) {

            try (ScopedLock scopedLock = mCloseLock.acquireLock()) {

                if (scopedLock == null) return; // Camera already closed



                StateListener sessionListener = mSessionStateListener;

                if (sessionListener != null) {

                    sessionListener.onActive(CameraDeviceImpl.this);
@@ -139,7 +148,9 @@ public class CameraDeviceImpl extends android.hardware.camera2.CameraDevice { 
    private final Runnable mCallOnBusy = new Runnable() {

        @Override

        public void run() {

            if (!CameraDeviceImpl.this.isClosed()) {

            try (ScopedLock scopedLock = mCloseLock.acquireLock()) {

                if (scopedLock == null) return; // Camera already closed



                StateListener sessionListener = mSessionStateListener;

                if (sessionListener != null) {

                    sessionListener.onBusy(CameraDeviceImpl.this);
@@ -150,20 +161,29 @@ public class CameraDeviceImpl extends android.hardware.camera2.CameraDevice { 
    };



    private final Runnable mCallOnClosed = new Runnable() {

        private boolean mClosedOnce = false;



        @Override

        public void run() {

            if (mClosedOnce) {

                throw new AssertionError("Don't post #onClosed more than once");

            }



            StateListener sessionListener = mSessionStateListener;

            if (sessionListener != null) {

                sessionListener.onClosed(CameraDeviceImpl.this);

            }

            mDeviceListener.onClosed(CameraDeviceImpl.this);

            mClosedOnce = true;

        }

    };



    private final Runnable mCallOnIdle = new Runnable() {

        @Override

        public void run() {

            if (!CameraDeviceImpl.this.isClosed()) {

            try (ScopedLock scopedLock = mCloseLock.acquireLock()) {

                if (scopedLock == null) return; // Camera already closed



                StateListener sessionListener = mSessionStateListener;

                if (sessionListener != null) {

                    sessionListener.onIdle(CameraDeviceImpl.this);
@@ -176,7 +196,9 @@ public class CameraDeviceImpl extends android.hardware.camera2.CameraDevice { 
    private final Runnable mCallOnDisconnected = new Runnable() {

        @Override

        public void run() {

            if (!CameraDeviceImpl.this.isClosed()) {

            try (ScopedLock scopedLock = mCloseLock.acquireLock()) {

                if (scopedLock == null) return; // Camera already closed



                StateListener sessionListener = mSessionStateListener;

                if (sessionListener != null) {

                    sessionListener.onDisconnected(CameraDeviceImpl.this);
@@ -195,6 +217,7 @@ public class CameraDeviceImpl extends android.hardware.camera2.CameraDevice { 
        mDeviceListener = listener;

        mDeviceHandler = handler;

        mCharacteristics = characteristics;

        mCloseLock = new CloseableLock(/*name*/"CD-" + mCameraId);



        final int MAX_TAG_LEN = 23;

        String tag = String.format("CameraDevice-JV-%s", mCameraId);
@@ -210,8 +233,8 @@ public class CameraDeviceImpl extends android.hardware.camera2.CameraDevice { 
    }



    public void setRemoteDevice(ICameraDeviceUser remoteDevice) {

        try (ScopedLock scopedLock = mCloseLock.acquireLock()) {

        // TODO: Move from decorator to direct binder-mediated exceptions

        synchronized(mLock) {

            // If setRemoteFailure already called, do nothing

            if (mInError) return;
@@ -254,7 +277,9 @@ public class CameraDeviceImpl extends android.hardware.camera2.CameraDevice { 
        }

        final int code = failureCode;

        final boolean isError = failureIsError;

        synchronized (mLock) {

        try (ScopedLock scopedLock = mCloseLock.acquireLock()) {

            if (scopedLock == null) return; // Camera already closed, can't go to error state



            mInError = true;

            mDeviceHandler.post(new Runnable() {

                @Override
@@ -280,7 +305,7 @@ public class CameraDeviceImpl extends android.hardware.camera2.CameraDevice { 
        if (outputs == null) {

            outputs = new ArrayList<Surface>();

        }

        synchronized (mLock) {

        try (ScopedLock scopedLock = mCloseLock.acquireExclusiveLock()) {

            checkIfCameraClosedOrInError();



            HashSet<Surface> addSet = new HashSet<Surface>(outputs);    // Streams to create
@@ -344,7 +369,7 @@ public class CameraDeviceImpl extends android.hardware.camera2.CameraDevice { 
    public void createCaptureSession(List<Surface> outputs,

            CameraCaptureSession.StateListener listener, Handler handler)

            throws CameraAccessException {

        synchronized (mLock) {

        try (ScopedLock scopedLock = mCloseLock.acquireExclusiveLock()) {

            if (DEBUG) {

                Log.d(TAG, "createCaptureSession");

            }
@@ -389,7 +414,7 @@ public class CameraDeviceImpl extends android.hardware.camera2.CameraDevice { 
    @Override

    public CaptureRequest.Builder createCaptureRequest(int templateType)

            throws CameraAccessException {

        synchronized (mLock) {

        try (ScopedLock scopedLock = mCloseLock.acquireExclusiveLock()) {

            checkIfCameraClosedOrInError();



            CameraMetadataNative templatedRequest = new CameraMetadataNative();
@@ -509,7 +534,7 @@ public class CameraDeviceImpl extends android.hardware.camera2.CameraDevice { 
            handler = checkHandler(handler);

        }



        synchronized (mLock) {

        try (ScopedLock scopedLock = mCloseLock.acquireExclusiveLock()) {

            checkIfCameraClosedOrInError();

            int requestId;
@@ -581,7 +606,7 @@ public class CameraDeviceImpl extends android.hardware.camera2.CameraDevice { 
    @Override

    public void stopRepeating() throws CameraAccessException {



        synchronized (mLock) {

        try (ScopedLock scopedLock = mCloseLock.acquireExclusiveLock()) {

            checkIfCameraClosedOrInError();

            if (mRepeatingRequestId != REQUEST_ID_NONE) {
@@ -612,7 +637,7 @@ public class CameraDeviceImpl extends android.hardware.camera2.CameraDevice { 


    private void waitUntilIdle() throws CameraAccessException {



        synchronized (mLock) {

        try (ScopedLock scopedLock = mCloseLock.acquireExclusiveLock()) {

            checkIfCameraClosedOrInError();

            if (mRepeatingRequestId != REQUEST_ID_NONE) {

                throw new IllegalStateException("Active repeating request ongoing");
@@ -633,7 +658,7 @@ public class CameraDeviceImpl extends android.hardware.camera2.CameraDevice { 


    @Override

    public void flush() throws CameraAccessException {

        synchronized (mLock) {

        try (ScopedLock scopedLock = mCloseLock.acquireExclusiveLock()) {

            checkIfCameraClosedOrInError();



            mDeviceHandler.post(mCallOnBusy);
@@ -656,8 +681,15 @@ public class CameraDeviceImpl extends android.hardware.camera2.CameraDevice { 


    @Override

    public void close() {

        synchronized (mLock) {

        mClosing = true;

        // Acquire exclusive lock, close, release (idempotent)

        mCloseLock.close();



        /*

         *  The rest of this is safe, since no other methods will be able to execute

         *  (they will throw ISE instead; the callbacks will get dropped)

         */

        {

            try {

                if (mRemoteDevice != null) {

                    mRemoteDevice.disconnect();
@@ -805,7 +837,12 @@ public class CameraDeviceImpl extends android.hardware.camera2.CameraDevice { 
                // remove request from mCaptureListenerMap

                final int requestId = frameNumberRequestPair.getValue();

                final CaptureListenerHolder holder;

                synchronized (mLock) {

                try (ScopedLock scopedLock = mCloseLock.acquireLock()) {

                    if (scopedLock == null) {

                        Log.w(TAG, "Camera closed while checking sequences");

                        return;

                    }



                    int index = mCaptureListenerMap.indexOfKey(requestId);

                    holder = (index >= 0) ? mCaptureListenerMap.valueAt(index)

                            : null;
@@ -890,9 +927,12 @@ public class CameraDeviceImpl extends android.hardware.camera2.CameraDevice { 
        @Override

        public void onCameraError(final int errorCode, CaptureResultExtras resultExtras) {

            Runnable r = null;

            if (isClosed()) return;



            synchronized(mLock) {

            try (ScopedLock scopedLock = mCloseLock.acquireLock()) {

                if (scopedLock == null) {

                    return; // Camera already closed

                }



                mInError = true;

                switch (errorCode) {

                    case ERROR_CAMERA_DISCONNECTED:
@@ -914,7 +954,6 @@ public class CameraDeviceImpl extends android.hardware.camera2.CameraDevice { 
                        break;

                }

                CameraDeviceImpl.this.mDeviceHandler.post(r);

            }



                // Fire onCaptureSequenceCompleted

                if (DEBUG) {
@@ -922,17 +961,17 @@ public class CameraDeviceImpl extends android.hardware.camera2.CameraDevice { 
                }

                mFrameNumberTracker.updateTracker(resultExtras.getFrameNumber(), /*error*/true);

                checkAndFireSequenceComplete();



            }

        }



        @Override

        public void onCameraIdle() {

            if (isClosed()) return;



            if (DEBUG) {

                Log.d(TAG, "Camera now idle");

            }

            synchronized (mLock) {

            try (ScopedLock scopedLock = mCloseLock.acquireLock()) {

                if (scopedLock == null) return; // Camera already closed



                if (!CameraDeviceImpl.this.mIdle) {

                    CameraDeviceImpl.this.mDeviceHandler.post(mCallOnIdle);

                }
@@ -948,10 +987,11 @@ public class CameraDeviceImpl extends android.hardware.camera2.CameraDevice { 
            }

            final CaptureListenerHolder holder;



            try (ScopedLock scopedLock = mCloseLock.acquireLock()) {

                if (scopedLock == null) return; // Camera already closed



                // Get the listener for this frame ID, if there is one

            synchronized (mLock) {

                holder = CameraDeviceImpl.this.mCaptureListenerMap.get(requestId);

            }



                if (holder == null) {

                    return;
@@ -972,6 +1012,8 @@ public class CameraDeviceImpl extends android.hardware.camera2.CameraDevice { 
                            }

                        }

                    });



            }

        }



        @Override
@@ -984,22 +1026,23 @@ public class CameraDeviceImpl extends android.hardware.camera2.CameraDevice { 
                        + requestId);

            }



            try (ScopedLock scopedLock = mCloseLock.acquireLock()) {

                if (scopedLock == null) return; // Camera already closed



                // TODO: Handle CameraCharacteristics access from CaptureResult correctly.

                result.set(CameraCharacteristics.LENS_INFO_SHADING_MAP_SIZE,

                        getCharacteristics().get(CameraCharacteristics.LENS_INFO_SHADING_MAP_SIZE));



            final CaptureListenerHolder holder;

            synchronized (mLock) {

                holder = CameraDeviceImpl.this.mCaptureListenerMap.get(requestId);

            }

                final CaptureListenerHolder holder =

                        CameraDeviceImpl.this.mCaptureListenerMap.get(requestId);



                Boolean quirkPartial = result.get(CaptureResult.QUIRKS_PARTIAL_RESULT);

                boolean quirkIsPartialResult = (quirkPartial != null && quirkPartial);



                // Update tracker (increment counter) when it's not a partial result.

                if (!quirkIsPartialResult) {

                mFrameNumberTracker.updateTracker(resultExtras.getFrameNumber(), /*error*/false);

                    mFrameNumberTracker.updateTracker(resultExtras.getFrameNumber(),

                            /*error*/false);

                }



                // Check if we have a listener for this
@@ -1067,6 +1110,8 @@ public class CameraDeviceImpl extends android.hardware.camera2.CameraDevice { 
                if (!quirkIsPartialResult) {

                    checkAndFireSequenceComplete();

                }



            }

        }



    }
@@ -1101,10 +1146,9 @@ public class CameraDeviceImpl extends android.hardware.camera2.CameraDevice { 
        }

    }



    /** Whether the camera device has started to close (may not yet have finished) */

    private boolean isClosed() {

        synchronized(mLock) {

            return (mRemoteDevice == null);

        }

        return mClosing;

    }



    private CameraCharacteristics getCharacteristics() {

core/java/android/hardware/camera2/legacy/LegacyCameraDevice.java

+53 −5
Original line number Diff line number Diff line
@@ -23,7 +23,6 @@ import android.hardware.camera2.impl.CaptureResultExtras; 
import android.hardware.camera2.ICameraDeviceCallbacks;

import android.hardware.camera2.utils.LongParcelable;

import android.hardware.camera2.impl.CameraMetadataNative;

import android.hardware.camera2.utils.CameraBinderDecorator;

import android.hardware.camera2.utils.CameraRuntimeException;

import android.os.ConditionVariable;

import android.os.Handler;
@@ -34,7 +33,8 @@ import android.view.Surface; 


import java.util.ArrayList;

import java.util.List;

import java.util.concurrent.atomic.AtomicInteger;



import static android.hardware.camera2.utils.CameraBinderDecorator.*;



/**

 * This class emulates the functionality of a Camera2 device using a the old Camera class.
@@ -54,6 +54,7 @@ public class LegacyCameraDevice implements AutoCloseable { 
    private final int mCameraId;

    private final ICameraDeviceCallbacks mDeviceCallbacks;

    private final CameraDeviceState mDeviceState = new CameraDeviceState();

    private List<Surface> mConfiguredSurfaces;



    private final ConditionVariable mIdle = new ConditionVariable(/*open*/true);
@@ -213,16 +214,35 @@ public class LegacyCameraDevice implements AutoCloseable { 
    /**

     * Configure the device with a set of output surfaces.

     *

     * <p>Using empty or {@code null} {@code outputs} is the same as unconfiguring.</p>

     *

     * <p>Every surface in {@code outputs} must be non-{@code null}.</p>

     *

     * @param outputs a list of surfaces to set.

     * @return an error code for this binder operation, or {@link CameraBinderDecorator.NO_ERROR}

     * @return an error code for this binder operation, or {@link NO_ERROR}

     *          on success.

     */

    public int configureOutputs(List<Surface> outputs) {

        if (outputs != null) {

            for (Surface output : outputs) {

                if (output == null) {

                    Log.e(TAG, "configureOutputs - null outputs are not allowed");

                    return BAD_VALUE;

                }

            }

        }



        int error = mDeviceState.setConfiguring();

        if (error == CameraBinderDecorator.NO_ERROR) {

        if (error == NO_ERROR) {

            mRequestThreadManager.configure(outputs);

            error = mDeviceState.setIdle();

        }



        // TODO: May also want to check the surfaces more deeply (e.g. state, formats, sizes..)

        if (error == NO_ERROR) {

            mConfiguredSurfaces = outputs != null ? new ArrayList<>(outputs) : null;

        }



        return error;

    }
@@ -239,7 +259,35 @@ public class LegacyCameraDevice implements AutoCloseable { 
     */

    public int submitRequestList(List<CaptureRequest> requestList, boolean repeating,

            /*out*/LongParcelable frameNumber) {

        // TODO: validate request here

        if (requestList == null || requestList.isEmpty()) {

            Log.e(TAG, "submitRequestList - Empty/null requests are not allowed");

            return BAD_VALUE;

        }



        // Make sure that there all requests have at least 1 surface; all surfaces are non-null

        for (CaptureRequest request : requestList) {

            if (request.getTargets().isEmpty()) {

                Log.e(TAG, "submitRequestList - "

                        + "Each request must have at least one Surface target");

                return BAD_VALUE;

            }



            for (Surface surface : request.getTargets()) {

                if (surface == null) {

                    Log.e(TAG, "submitRequestList - Null Surface targets are not allowed");

                    return BAD_VALUE;

                } else if (mConfiguredSurfaces == null) {

                    Log.e(TAG, "submitRequestList - must configure " +

                            " device with valid surfaces before submitting requests");

                    return INVALID_OPERATION;

                } else if (!mConfiguredSurfaces.contains(surface)) {

                    Log.e(TAG, "submitRequestList - cannot use a surface that wasn't configured");

                    return BAD_VALUE;

                }

            }

        }



        // TODO: further validation of request here

        mIdle.close();

        return mRequestThreadManager.submitCaptureRequests(requestList, repeating,

                frameNumber);

core/java/android/hardware/camera2/legacy/RequestThreadManager.java

+19 −18
Original line number Diff line number Diff line
@@ -16,7 +16,6 @@ 


package android.hardware.camera2.legacy;



import android.graphics.ImageFormat;

import android.graphics.SurfaceTexture;

import android.hardware.Camera;

import android.hardware.camera2.CaptureRequest;
@@ -76,8 +75,8 @@ public class RequestThreadManager { 
    private volatile RequestHolder mInFlightPreview;

    private volatile RequestHolder mInFlightJpeg;



    private List<Surface> mPreviewOutputs = new ArrayList<Surface>();

    private List<Surface> mCallbackOutputs = new ArrayList<Surface>();

    private final List<Surface> mPreviewOutputs = new ArrayList<Surface>();

    private final List<Surface> mCallbackOutputs = new ArrayList<Surface>();

    private GLThreadManager mGLThreadManager;

    private SurfaceTexture mPreviewTexture;

    private Camera.Parameters mParams;
@@ -315,7 +314,7 @@ public class RequestThreadManager { 
        mInFlightPreview = null;

        mInFlightJpeg = null;





        if (outputs != null) {

            for (Surface s : outputs) {

                int format = LegacyCameraDevice.nativeDetectSurfaceType(s);

                switch (format) {
@@ -327,6 +326,7 @@ public class RequestThreadManager { 
                        break;

                }

            }

        }

        mParams = mCamera.getParameters();

        if (mPreviewOutputs.size() > 0) {

            List<Size> outputSizes = new ArrayList<>(outputs.size());
@@ -370,8 +370,6 @@ public class RequestThreadManager { 
            }

        }







        // TODO: Detect and optimize single-output paths here to skip stream teeing.

        if (mGLThreadManager == null) {

            mGLThreadManager = new GLThreadManager(mCameraId);
@@ -432,7 +430,7 @@ public class RequestThreadManager { 


    private final Handler.Callback mRequestHandlerCb = new Handler.Callback() {

        private boolean mCleanup = false;

        private List<RequestHolder> mRepeating = null;

        private final List<RequestHolder> mRepeating = null;



        @SuppressWarnings("unchecked")

        @Override
@@ -447,7 +445,8 @@ public class RequestThreadManager { 
            switch (msg.what) {

                case MSG_CONFIGURE_OUTPUTS:

                    ConfigureHolder config = (ConfigureHolder) msg.obj;

                    Log.i(TAG, "Configure outputs: " + config.surfaces.size() +

                    int sizes = config.surfaces != null ? config.surfaces.size() : 0;

                    Log.i(TAG, "Configure outputs: " + sizes +

                            " surfaces configured.");

                    try {

                        configureOutputs(config.surfaces);
@@ -620,12 +619,14 @@ public class RequestThreadManager { 




    /**

     * Configure with the current output Surfaces.

     * Configure with the current list of output Surfaces.

     *

     * <p>

     * This operation blocks until the configuration is complete.

     * </p>

     *

     * <p>Using a {@code null} or empty {@code outputs} list is the equivalent of unconfiguring.</p>

     *

     * @param outputs a {@link java.util.Collection} of outputs to configure.

     */

    public void configure(Collection<Surface> outputs) {

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

0 → 100644
+330 −0

File added.

Preview size limit exceeded, changes collapsed.