From 2f75e7d7ef2ef29ac4b1287dc44cf62cfbaf50b3 Mon Sep 17 00:00:00 2001 From: Eino-Ville Talvala Date: Tue, 15 Jul 2014 10:31:54 -0700 Subject: [PATCH] Camera2: Fix callback operation - Remove CloseableLock use; looks to be incompatible with invocations during callbacks - Replace with basic interface lock to be thread-safe - Add intermediate callback thread to legacy mode to match cross-process one-way Binder semantics Change-Id: Iecd4ff6cf260c5a13bd11b850177ccea93e25933 --- .../camera2/impl/CameraDeviceImpl.java | 158 +++++++++--------- .../camera2/legacy/CameraDeviceUserShim.java | 133 ++++++++++++++- 2 files changed, 207 insertions(+), 84 deletions(-) diff --git a/core/java/android/hardware/camera2/impl/CameraDeviceImpl.java b/core/java/android/hardware/camera2/impl/CameraDeviceImpl.java index 02be22f73ee73..fb1bc1551372d 100644 --- a/core/java/android/hardware/camera2/impl/CameraDeviceImpl.java +++ b/core/java/android/hardware/camera2/impl/CameraDeviceImpl.java @@ -28,8 +28,6 @@ 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; @@ -59,7 +57,8 @@ 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 CloseableLock mCloseLock; + // Lock to synchronize cross-thread access to device public interface + private final Object mInterfaceLock = new Object(); private final CameraDeviceCallbacks mCallbacks = new CameraDeviceCallbacks(); private final StateListener mDeviceListener; @@ -104,60 +103,64 @@ public class CameraDeviceImpl extends android.hardware.camera2.CameraDevice { private final Runnable mCallOnOpened = new Runnable() { @Override public void run() { - try (ScopedLock scopedLock = mCloseLock.acquireLock()) { - if (scopedLock == null) return; // Camera already closed + StateListener sessionListener = null; + synchronized(mInterfaceLock) { + if (mRemoteDevice == null) return; // Camera already closed - StateListener sessionListener = mSessionStateListener; - if (sessionListener != null) { - sessionListener.onOpened(CameraDeviceImpl.this); - } - mDeviceListener.onOpened(CameraDeviceImpl.this); + sessionListener = mSessionStateListener; } + if (sessionListener != null) { + sessionListener.onOpened(CameraDeviceImpl.this); + } + mDeviceListener.onOpened(CameraDeviceImpl.this); } }; private final Runnable mCallOnUnconfigured = new Runnable() { @Override public void run() { - try (ScopedLock scopedLock = mCloseLock.acquireLock()) { - if (scopedLock == null) return; // Camera already closed + StateListener sessionListener = null; + synchronized(mInterfaceLock) { + if (mRemoteDevice == null) return; // Camera already closed - StateListener sessionListener = mSessionStateListener; - if (sessionListener != null) { - sessionListener.onUnconfigured(CameraDeviceImpl.this); - } - mDeviceListener.onUnconfigured(CameraDeviceImpl.this); + sessionListener = mSessionStateListener; } + if (sessionListener != null) { + sessionListener.onUnconfigured(CameraDeviceImpl.this); + } + mDeviceListener.onUnconfigured(CameraDeviceImpl.this); } }; private final Runnable mCallOnActive = new Runnable() { @Override public void run() { - try (ScopedLock scopedLock = mCloseLock.acquireLock()) { - if (scopedLock == null) return; // Camera already closed + StateListener sessionListener = null; + synchronized(mInterfaceLock) { + if (mRemoteDevice == null) return; // Camera already closed - StateListener sessionListener = mSessionStateListener; - if (sessionListener != null) { - sessionListener.onActive(CameraDeviceImpl.this); - } - mDeviceListener.onActive(CameraDeviceImpl.this); + sessionListener = mSessionStateListener; } + if (sessionListener != null) { + sessionListener.onActive(CameraDeviceImpl.this); + } + mDeviceListener.onActive(CameraDeviceImpl.this); } }; private final Runnable mCallOnBusy = new Runnable() { @Override public void run() { - try (ScopedLock scopedLock = mCloseLock.acquireLock()) { - if (scopedLock == null) return; // Camera already closed + StateListener sessionListener = null; + synchronized(mInterfaceLock) { + if (mRemoteDevice == null) return; // Camera already closed - StateListener sessionListener = mSessionStateListener; - if (sessionListener != null) { - sessionListener.onBusy(CameraDeviceImpl.this); - } - mDeviceListener.onBusy(CameraDeviceImpl.this); + sessionListener = mSessionStateListener; } + if (sessionListener != null) { + sessionListener.onBusy(CameraDeviceImpl.this); + } + mDeviceListener.onBusy(CameraDeviceImpl.this); } }; @@ -169,8 +172,10 @@ public class CameraDeviceImpl extends android.hardware.camera2.CameraDevice { if (mClosedOnce) { throw new AssertionError("Don't post #onClosed more than once"); } - - StateListener sessionListener = mSessionStateListener; + StateListener sessionListener = null; + synchronized(mInterfaceLock) { + sessionListener = mSessionStateListener; + } if (sessionListener != null) { sessionListener.onClosed(CameraDeviceImpl.this); } @@ -182,30 +187,32 @@ public class CameraDeviceImpl extends android.hardware.camera2.CameraDevice { private final Runnable mCallOnIdle = new Runnable() { @Override public void run() { - try (ScopedLock scopedLock = mCloseLock.acquireLock()) { - if (scopedLock == null) return; // Camera already closed + StateListener sessionListener = null; + synchronized(mInterfaceLock) { + if (mRemoteDevice == null) return; // Camera already closed - StateListener sessionListener = mSessionStateListener; - if (sessionListener != null) { - sessionListener.onIdle(CameraDeviceImpl.this); - } - mDeviceListener.onIdle(CameraDeviceImpl.this); + sessionListener = mSessionStateListener; } + if (sessionListener != null) { + sessionListener.onIdle(CameraDeviceImpl.this); + } + mDeviceListener.onIdle(CameraDeviceImpl.this); } }; private final Runnable mCallOnDisconnected = new Runnable() { @Override public void run() { - try (ScopedLock scopedLock = mCloseLock.acquireLock()) { - if (scopedLock == null) return; // Camera already closed + StateListener sessionListener = null; + synchronized(mInterfaceLock) { + if (mRemoteDevice == null) return; // Camera already closed - StateListener sessionListener = mSessionStateListener; - if (sessionListener != null) { - sessionListener.onDisconnected(CameraDeviceImpl.this); - } - mDeviceListener.onDisconnected(CameraDeviceImpl.this); + sessionListener = mSessionStateListener; } + if (sessionListener != null) { + sessionListener.onDisconnected(CameraDeviceImpl.this); + } + mDeviceListener.onDisconnected(CameraDeviceImpl.this); } }; @@ -218,7 +225,6 @@ 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); @@ -243,8 +249,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(mInterfaceLock) { + // TODO: Move from decorator to direct binder-mediated exceptions // If setRemoteFailure already called, do nothing if (mInError) return; @@ -287,8 +293,8 @@ public class CameraDeviceImpl extends android.hardware.camera2.CameraDevice { } final int code = failureCode; final boolean isError = failureIsError; - try (ScopedLock scopedLock = mCloseLock.acquireLock()) { - if (scopedLock == null) return; // Camera already closed, can't go to error state + synchronized(mInterfaceLock) { + if (mRemoteDevice == null) return; // Camera already closed, can't go to error state mInError = true; mDeviceHandler.post(new Runnable() { @@ -314,7 +320,7 @@ public class CameraDeviceImpl extends android.hardware.camera2.CameraDevice { if (outputs == null) { outputs = new ArrayList(); } - try (ScopedLock scopedLock = mCloseLock.acquireExclusiveLock()) { + synchronized(mInterfaceLock) { checkIfCameraClosedOrInError(); HashSet addSet = new HashSet(outputs); // Streams to create @@ -378,7 +384,7 @@ public class CameraDeviceImpl extends android.hardware.camera2.CameraDevice { public void createCaptureSession(List outputs, CameraCaptureSession.StateListener listener, Handler handler) throws CameraAccessException { - try (ScopedLock scopedLock = mCloseLock.acquireExclusiveLock()) { + synchronized(mInterfaceLock) { if (DEBUG) { Log.d(TAG, "createCaptureSession"); } @@ -423,7 +429,7 @@ public class CameraDeviceImpl extends android.hardware.camera2.CameraDevice { @Override public CaptureRequest.Builder createCaptureRequest(int templateType) throws CameraAccessException { - try (ScopedLock scopedLock = mCloseLock.acquireExclusiveLock()) { + synchronized(mInterfaceLock) { checkIfCameraClosedOrInError(); CameraMetadataNative templatedRequest = new CameraMetadataNative(); @@ -554,7 +560,7 @@ public class CameraDeviceImpl extends android.hardware.camera2.CameraDevice { } } - try (ScopedLock scopedLock = mCloseLock.acquireExclusiveLock()) { + synchronized(mInterfaceLock) { checkIfCameraClosedOrInError(); int requestId; @@ -623,7 +629,7 @@ public class CameraDeviceImpl extends android.hardware.camera2.CameraDevice { public void stopRepeating() throws CameraAccessException { - try (ScopedLock scopedLock = mCloseLock.acquireExclusiveLock()) { + synchronized(mInterfaceLock) { checkIfCameraClosedOrInError(); if (mRepeatingRequestId != REQUEST_ID_NONE) { @@ -654,12 +660,12 @@ public class CameraDeviceImpl extends android.hardware.camera2.CameraDevice { private void waitUntilIdle() throws CameraAccessException { - try (ScopedLock scopedLock = mCloseLock.acquireExclusiveLock()) { + synchronized(mInterfaceLock) { checkIfCameraClosedOrInError(); + if (mRepeatingRequestId != REQUEST_ID_NONE) { throw new IllegalStateException("Active repeating request ongoing"); } - try { mRemoteDevice.waitUntilIdle(); } catch (CameraRuntimeException e) { @@ -668,13 +674,11 @@ public class CameraDeviceImpl extends android.hardware.camera2.CameraDevice { // impossible return; } - - mRepeatingRequestId = REQUEST_ID_NONE; } } public void flush() throws CameraAccessException { - try (ScopedLock scopedLock = mCloseLock.acquireExclusiveLock()) { + synchronized(mInterfaceLock) { checkIfCameraClosedOrInError(); mDeviceHandler.post(mCallOnBusy); @@ -697,15 +701,7 @@ public class CameraDeviceImpl extends android.hardware.camera2.CameraDevice { @Override public void close() { - 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) - */ - { + synchronized (mInterfaceLock) { try { if (mRemoteDevice != null) { mRemoteDevice.disconnect(); @@ -853,8 +849,8 @@ public class CameraDeviceImpl extends android.hardware.camera2.CameraDevice { // remove request from mCaptureListenerMap final int requestId = frameNumberRequestPair.getValue(); final CaptureListenerHolder holder; - try (ScopedLock scopedLock = mCloseLock.acquireLock()) { - if (scopedLock == null) { + synchronized(mInterfaceLock) { + if (mRemoteDevice == null) { Log.w(TAG, "Camera closed while checking sequences"); return; } @@ -944,8 +940,8 @@ public class CameraDeviceImpl extends android.hardware.camera2.CameraDevice { public void onCameraError(final int errorCode, CaptureResultExtras resultExtras) { Runnable r = null; - try (ScopedLock scopedLock = mCloseLock.acquireLock()) { - if (scopedLock == null) { + synchronized(mInterfaceLock) { + if (mRemoteDevice == null) { return; // Camera already closed } @@ -985,8 +981,8 @@ public class CameraDeviceImpl extends android.hardware.camera2.CameraDevice { if (DEBUG) { Log.d(TAG, "Camera now idle"); } - try (ScopedLock scopedLock = mCloseLock.acquireLock()) { - if (scopedLock == null) return; // Camera already closed + synchronized(mInterfaceLock) { + if (mRemoteDevice == null) return; // Camera already closed if (!CameraDeviceImpl.this.mIdle) { CameraDeviceImpl.this.mDeviceHandler.post(mCallOnIdle); @@ -1003,8 +999,8 @@ public class CameraDeviceImpl extends android.hardware.camera2.CameraDevice { } final CaptureListenerHolder holder; - try (ScopedLock scopedLock = mCloseLock.acquireLock()) { - if (scopedLock == null) return; // Camera already closed + synchronized(mInterfaceLock) { + if (mRemoteDevice == null) return; // Camera already closed // Get the listener for this frame ID, if there is one holder = CameraDeviceImpl.this.mCaptureListenerMap.get(requestId); @@ -1042,8 +1038,8 @@ public class CameraDeviceImpl extends android.hardware.camera2.CameraDevice { + requestId); } - try (ScopedLock scopedLock = mCloseLock.acquireLock()) { - if (scopedLock == null) return; // Camera already closed + synchronized(mInterfaceLock) { + if (mRemoteDevice == null) return; // Camera already closed // TODO: Handle CameraCharacteristics access from CaptureResult correctly. result.set(CameraCharacteristics.LENS_INFO_SHADING_MAP_SIZE, diff --git a/core/java/android/hardware/camera2/legacy/CameraDeviceUserShim.java b/core/java/android/hardware/camera2/legacy/CameraDeviceUserShim.java index 5bdef4ab5efe2..c68d8c3733133 100644 --- a/core/java/android/hardware/camera2/legacy/CameraDeviceUserShim.java +++ b/core/java/android/hardware/camera2/legacy/CameraDeviceUserShim.java @@ -25,11 +25,15 @@ import android.hardware.camera2.ICameraDeviceCallbacks; import android.hardware.camera2.ICameraDeviceUser; import android.hardware.camera2.utils.LongParcelable; import android.hardware.camera2.impl.CameraMetadataNative; +import android.hardware.camera2.impl.CaptureResultExtras; import android.hardware.camera2.utils.CameraBinderDecorator; import android.hardware.camera2.utils.CameraRuntimeException; import android.os.ConditionVariable; import android.os.IBinder; import android.os.Looper; +import android.os.Handler; +import android.os.HandlerThread; +import android.os.Message; import android.os.RemoteException; import android.util.Log; import android.util.SparseArray; @@ -66,14 +70,18 @@ public class CameraDeviceUserShim implements ICameraDeviceUser { private final SparseArray mSurfaces; private final CameraCharacteristics mCameraCharacteristics; private final CameraLooper mCameraInit; + private final CameraCallbackThread mCameraCallbacks; + protected CameraDeviceUserShim(int cameraId, LegacyCameraDevice legacyCamera, - CameraCharacteristics characteristics, CameraLooper cameraInit) { + CameraCharacteristics characteristics, CameraLooper cameraInit, + CameraCallbackThread cameraCallbacks) { mLegacyDevice = legacyCamera; mConfiguring = false; mSurfaces = new SparseArray(); mCameraCharacteristics = characteristics; mCameraInit = cameraInit; + mCameraCallbacks = cameraCallbacks; mSurfaceIdCounter = 0; } @@ -173,6 +181,122 @@ public class CameraDeviceUserShim implements ICameraDeviceUser { } } + /** + * A thread to process callbacks to send back to the camera client. + * + *

This effectively emulates one-way binder semantics when in the same process as the + * callee.

+ */ + private static class CameraCallbackThread implements ICameraDeviceCallbacks { + private static final int CAMERA_ERROR = 0; + private static final int CAMERA_IDLE = 1; + private static final int CAPTURE_STARTED = 2; + private static final int RESULT_RECEIVED = 3; + + private final HandlerThread mHandlerThread; + private Handler mHandler; + + private final ICameraDeviceCallbacks mCallbacks; + + public CameraCallbackThread(ICameraDeviceCallbacks callbacks) { + mCallbacks = callbacks; + + mHandlerThread = new HandlerThread("LegacyCameraCallback"); + mHandlerThread.start(); + } + + public void close() { + mHandlerThread.quitSafely(); + } + + @Override + public void onCameraError(final int errorCode, final CaptureResultExtras resultExtras) { + Message msg = getHandler().obtainMessage(CAMERA_ERROR, + /*arg1*/ errorCode, /*arg2*/ 0, + /*obj*/ resultExtras); + getHandler().sendMessage(msg); + } + + @Override + public void onCameraIdle() { + Message msg = getHandler().obtainMessage(CAMERA_IDLE); + getHandler().sendMessage(msg); + } + + @Override + public void onCaptureStarted(final CaptureResultExtras resultExtras, final long timestamp) { + Message msg = getHandler().obtainMessage(CAPTURE_STARTED, + /*arg1*/ (int) (timestamp & 0xFFFFFFFFL), + /*arg2*/ (int) ( (timestamp >> 32) & 0xFFFFFFFFL), + /*obj*/ resultExtras); + getHandler().sendMessage(msg); + } + + @Override + public void onResultReceived(final CameraMetadataNative result, + final CaptureResultExtras resultExtras) { + Object[] resultArray = new Object[] { result, resultExtras }; + Message msg = getHandler().obtainMessage(RESULT_RECEIVED, + /*obj*/ resultArray); + getHandler().sendMessage(msg); + } + + @Override + public IBinder asBinder() { + // This is solely intended to be used for in-process binding. + return null; + } + + private Handler getHandler() { + if (mHandler == null) { + mHandler = new CallbackHandler(mHandlerThread.getLooper()); + } + return mHandler; + } + + private class CallbackHandler extends Handler { + public CallbackHandler(Looper l) { + super(l); + } + + public void handleMessage(Message msg) { + try { + switch (msg.what) { + case CAMERA_ERROR: { + int errorCode = msg.arg1; + CaptureResultExtras resultExtras = (CaptureResultExtras) msg.obj; + mCallbacks.onCameraError(errorCode, resultExtras); + break; + } + case CAMERA_IDLE: + mCallbacks.onCameraIdle(); + break; + case CAPTURE_STARTED: { + long timestamp = msg.arg2 & 0xFFFFFFFFL; + timestamp = (timestamp << 32) | (msg.arg1 & 0xFFFFFFFFL); + CaptureResultExtras resultExtras = (CaptureResultExtras) msg.obj; + mCallbacks.onCaptureStarted(resultExtras, timestamp); + break; + } + case RESULT_RECEIVED: { + Object[] resultArray = (Object[]) msg.obj; + CameraMetadataNative result = (CameraMetadataNative) resultArray[0]; + CaptureResultExtras resultExtras = (CaptureResultExtras) resultArray[1]; + mCallbacks.onResultReceived(result, resultExtras); + break; + } + default: + throw new IllegalArgumentException( + "Unknown callback message " + msg.what); + } + } catch (RemoteException e) { + throw new IllegalStateException( + "Received remote exception during camera callback " + msg.what, e); + } + } + } + } + public static CameraDeviceUserShim connectBinderShim(ICameraDeviceCallbacks callbacks, int cameraId) { if (DEBUG) { @@ -187,6 +311,8 @@ public class CameraDeviceUserShim implements ICameraDeviceUser { CameraLooper init = new CameraLooper(cameraId); + CameraCallbackThread threadCallbacks = new CameraCallbackThread(callbacks); + // TODO: Make this async instead of blocking int initErrors = init.waitForOpen(OPEN_CAMERA_TIMEOUT_MS); Camera legacyCamera = init.getCamera(); @@ -200,8 +326,8 @@ public class CameraDeviceUserShim implements ICameraDeviceUser { CameraCharacteristics characteristics = LegacyMetadataMapper.createCharacteristics(legacyCamera.getParameters(), info); LegacyCameraDevice device = new LegacyCameraDevice( - cameraId, legacyCamera, characteristics, callbacks); - return new CameraDeviceUserShim(cameraId, device, characteristics, init); + cameraId, legacyCamera, characteristics, threadCallbacks); + return new CameraDeviceUserShim(cameraId, device, characteristics, init, threadCallbacks); } @Override @@ -214,6 +340,7 @@ public class CameraDeviceUserShim implements ICameraDeviceUser { mLegacyDevice.close(); } finally { mCameraInit.close(); + mCameraCallbacks.close(); } }