From 6231472fb15c9f35249ec4e621a2b2115a6a2141 Mon Sep 17 00:00:00 2001 From: Emilian Peev Date: Thu, 23 Feb 2023 16:48:53 -0800 Subject: [PATCH] Camera: Avoid holding locks during extension initialization Holding the interface lock while initializing the extension is not really necessary and can potentially result in a deadlock in case of failure where callbacks try to close the camera capture session in the handler thread, and another thread tries to close the camera device acquiring the interface lock there: Thread1: CameraDeviceImpl.close() acquire the device interface lock release advanced extension try to acquire the extension characteristics lock Thread2: InitializationSessionHandler.onFailure() while holding the extension characteristics lock mCaptureSession.close() try to acquire the device interface lock Additionally re-use the device interface lock for the extension session synchronization. There is no real need to use a separate lock which complicates the lock acquire order. Bug: 270276341 Test: atest -c -d cts/tests/camera/src/android/hardware/camera2/cts/CameraExtensionSessionTest.java Change-Id: Ice808c8d5538e3adb3220d7ea51dea4b14cc2f44 --- .../CameraAdvancedExtensionSessionImpl.java | 101 ++++++++++-------- .../impl/CameraExtensionSessionImpl.java | 85 ++++++++------- 2 files changed, 105 insertions(+), 81 deletions(-) diff --git a/core/java/android/hardware/camera2/impl/CameraAdvancedExtensionSessionImpl.java b/core/java/android/hardware/camera2/impl/CameraAdvancedExtensionSessionImpl.java index b52daab284b5e..41370e37f0924 100644 --- a/core/java/android/hardware/camera2/impl/CameraAdvancedExtensionSessionImpl.java +++ b/core/java/android/hardware/camera2/impl/CameraAdvancedExtensionSessionImpl.java @@ -98,15 +98,15 @@ public final class CameraAdvancedExtensionSessionImpl extends CameraExtensionSes // Lock to synchronize cross-thread access to device public interface - final Object mInterfaceLock = new Object(); // access from this class and Session only! + final Object mInterfaceLock; /** * @hide */ @RequiresPermission(android.Manifest.permission.CAMERA) public static CameraAdvancedExtensionSessionImpl createCameraAdvancedExtensionSession( - @NonNull CameraDevice cameraDevice, @NonNull Context ctx, - @NonNull ExtensionSessionConfiguration config, int sessionId) + @NonNull android.hardware.camera2.impl.CameraDeviceImpl cameraDevice, + @NonNull Context ctx, @NonNull ExtensionSessionConfiguration config, int sessionId) throws CameraAccessException, RemoteException { long clientId = CameraExtensionCharacteristics.registerClient(ctx); if (clientId < 0) { @@ -183,7 +183,8 @@ public final class CameraAdvancedExtensionSessionImpl extends CameraExtensionSes } private CameraAdvancedExtensionSessionImpl(long extensionClientId, - @NonNull IAdvancedExtenderImpl extender, @NonNull CameraDevice cameraDevice, + @NonNull IAdvancedExtenderImpl extender, + @NonNull android.hardware.camera2.impl.CameraDeviceImpl cameraDevice, @Nullable Surface repeatingRequestSurface, @Nullable Surface burstCaptureSurface, @NonNull CameraExtensionSession.StateCallback callback, @NonNull Executor executor, int sessionId) { @@ -200,6 +201,7 @@ public final class CameraAdvancedExtensionSessionImpl extends CameraExtensionSes mInitialized = false; mInitializeHandler = new InitializeSessionHandler(); mSessionId = sessionId; + mInterfaceLock = cameraDevice.mInterfaceLock; } /** @@ -511,13 +513,14 @@ public final class CameraAdvancedExtensionSessionImpl extends CameraExtensionSes public void onConfigured(@NonNull CameraCaptureSession session) { synchronized (mInterfaceLock) { mCaptureSession = session; - try { - CameraExtensionCharacteristics.initializeSession(mInitializeHandler); - } catch (RemoteException e) { - Log.e(TAG, "Failed to initialize session! Extension service does" - + " not respond!"); - notifyConfigurationFailure(); - } + } + + try { + CameraExtensionCharacteristics.initializeSession(mInitializeHandler); + } catch (RemoteException e) { + Log.e(TAG, "Failed to initialize session! Extension service does" + + " not respond!"); + notifyConfigurationFailure(); } } } @@ -525,46 +528,56 @@ public final class CameraAdvancedExtensionSessionImpl extends CameraExtensionSes private class InitializeSessionHandler extends IInitializeSessionCallback.Stub { @Override public void onSuccess() { - boolean status = true; - synchronized (mInterfaceLock) { - try { - if (mSessionProcessor != null) { - mSessionProcessor.onCaptureSessionStart(mRequestProcessor); - mInitialized = true; - } else { - Log.v(TAG, "Failed to start capture session, session released before " + - "extension start!"); - status = false; - mCaptureSession.close(); + mHandler.post(new Runnable() { + @Override + public void run() { + boolean status = true; + synchronized (mInterfaceLock) { + try { + if (mSessionProcessor != null) { + mSessionProcessor.onCaptureSessionStart(mRequestProcessor); + mInitialized = true; + } else { + Log.v(TAG, "Failed to start capture session, session " + + " released before extension start!"); + status = false; + } + } catch (RemoteException e) { + Log.e(TAG, "Failed to start capture session," + + " extension service does not respond!"); + status = false; + mInitialized = false; + } } - } catch (RemoteException e) { - Log.e(TAG, "Failed to start capture session," - + " extension service does not respond!"); - status = false; - mCaptureSession.close(); - } - } - if (status) { - final long ident = Binder.clearCallingIdentity(); - try { - mExecutor.execute( - () -> mCallbacks.onConfigured(CameraAdvancedExtensionSessionImpl.this)); - } finally { - Binder.restoreCallingIdentity(ident); + if (status) { + final long ident = Binder.clearCallingIdentity(); + try { + mExecutor.execute(() -> mCallbacks.onConfigured( + CameraAdvancedExtensionSessionImpl.this)); + } finally { + Binder.restoreCallingIdentity(ident); + } + } else { + onFailure(); + } } - } else { - notifyConfigurationFailure(); - } + }); } @Override public void onFailure() { - mCaptureSession.close(); - Log.e(TAG, "Failed to initialize proxy service session!" - + " This can happen when trying to configure multiple " - + "concurrent extension sessions!"); - notifyConfigurationFailure(); + mHandler.post(new Runnable() { + @Override + public void run() { + mCaptureSession.close(); + + Log.e(TAG, "Failed to initialize proxy service session!" + + " This can happen when trying to configure multiple " + + "concurrent extension sessions!"); + notifyConfigurationFailure(); + } + }); } } diff --git a/core/java/android/hardware/camera2/impl/CameraExtensionSessionImpl.java b/core/java/android/hardware/camera2/impl/CameraExtensionSessionImpl.java index 41822e77f9536..6bca732feca52 100644 --- a/core/java/android/hardware/camera2/impl/CameraExtensionSessionImpl.java +++ b/core/java/android/hardware/camera2/impl/CameraExtensionSessionImpl.java @@ -114,7 +114,7 @@ public final class CameraExtensionSessionImpl extends CameraExtensionSession { private boolean mInternalRepeatingRequestEnabled = true; // Lock to synchronize cross-thread access to device public interface - final Object mInterfaceLock = new Object(); // access from this class and Session only! + final Object mInterfaceLock; private static int nativeGetSurfaceFormat(Surface surface) { return SurfaceUtils.getSurfaceFormat(surface); @@ -125,7 +125,7 @@ public final class CameraExtensionSessionImpl extends CameraExtensionSession { */ @RequiresPermission(android.Manifest.permission.CAMERA) public static CameraExtensionSessionImpl createCameraExtensionSession( - @NonNull CameraDevice cameraDevice, + @NonNull android.hardware.camera2.impl.CameraDeviceImpl cameraDevice, @NonNull Context ctx, @NonNull ExtensionSessionConfiguration config, int sessionId) @@ -223,7 +223,7 @@ public final class CameraExtensionSessionImpl extends CameraExtensionSession { @NonNull IPreviewExtenderImpl previewExtender, @NonNull List previewSizes, long extensionClientId, - @NonNull CameraDevice cameraDevice, + @NonNull android.hardware.camera2.impl.CameraDeviceImpl cameraDevice, @Nullable Surface repeatingRequestSurface, @Nullable Surface burstCaptureSurface, @NonNull StateCallback callback, @@ -249,6 +249,7 @@ public final class CameraExtensionSessionImpl extends CameraExtensionSession { mSupportedRequestKeys = requestKeys; mSupportedResultKeys = resultKeys; mCaptureResultsSupported = !resultKeys.isEmpty(); + mInterfaceLock = cameraDevice.mInterfaceLock; } private void initializeRepeatingRequestPipeline() throws RemoteException { @@ -856,46 +857,56 @@ public final class CameraExtensionSessionImpl extends CameraExtensionSession { private class InitializeSessionHandler extends IInitializeSessionCallback.Stub { @Override public void onSuccess() { - boolean status = true; - ArrayList initialRequestList = - compileInitialRequestList(); - if (!initialRequestList.isEmpty()) { - try { - setInitialCaptureRequest(initialRequestList, - new InitialRequestHandler( - mRepeatingRequestImageCallback)); - } catch (CameraAccessException e) { - Log.e(TAG, - "Failed to initialize the initial capture " - + "request!"); - status = false; - } - } else { - try { - setRepeatingRequest(mPreviewExtender.getCaptureStage(), - new PreviewRequestHandler(null, null, null, - mRepeatingRequestImageCallback)); - } catch (CameraAccessException | RemoteException e) { - Log.e(TAG, - "Failed to initialize internal repeating " - + "request!"); - status = false; - } + mHandler.post(new Runnable() { + @Override + public void run() { + boolean status = true; + ArrayList initialRequestList = + compileInitialRequestList(); + if (!initialRequestList.isEmpty()) { + try { + setInitialCaptureRequest(initialRequestList, + new InitialRequestHandler( + mRepeatingRequestImageCallback)); + } catch (CameraAccessException e) { + Log.e(TAG, + "Failed to initialize the initial capture " + + "request!"); + status = false; + } + } else { + try { + setRepeatingRequest(mPreviewExtender.getCaptureStage(), + new PreviewRequestHandler(null, null, null, + mRepeatingRequestImageCallback)); + } catch (CameraAccessException | RemoteException e) { + Log.e(TAG, + "Failed to initialize internal repeating " + + "request!"); + status = false; + } - } + } - if (!status) { - notifyConfigurationFailure(); - } + if (!status) { + notifyConfigurationFailure(); + } + } + }); } @Override public void onFailure() { - mCaptureSession.close(); - Log.e(TAG, "Failed to initialize proxy service session!" - + " This can happen when trying to configure multiple " - + "concurrent extension sessions!"); - notifyConfigurationFailure(); + mHandler.post(new Runnable() { + @Override + public void run() { + mCaptureSession.close(); + Log.e(TAG, "Failed to initialize proxy service session!" + + " This can happen when trying to configure multiple " + + "concurrent extension sessions!"); + notifyConfigurationFailure(); + } + }); } }