From ff0f38e6fe2402b1320bc5faeea3f7bec27a3a94 Mon Sep 17 00:00:00 2001
From: Dave Sparks <davidsparks@android.com>
Date: Tue, 10 Nov 2009 17:08:08 -0800
Subject: [PATCH] Fix potential deadlock in stopPreview/stopRecord.

Some camera HALs spin up a preview thread and need to wait for
the thread to exit. This can create a potential deadlock. In
stopPreview, we take the main lock. If a preview callback occurs
while the lock is held, the preview thread will block. If the
camera HAL is waiting for the preview thread to exit, this will
cause a deadlock.

This patch breaks out the preview buffer heap into a separate
mutex. This mutex is never held when the main lock is held, thus
preventing the deadlock from occuring.
---
 camera/libcameraservice/CameraService.cpp | 71 ++++++++++++++---------
 camera/libcameraservice/CameraService.h   |  4 +-
 2 files changed, 46 insertions(+), 29 deletions(-)

diff --git a/camera/libcameraservice/CameraService.cpp b/camera/libcameraservice/CameraService.cpp
index e54852446..df59dcff0 100644
--- a/camera/libcameraservice/CameraService.cpp
+++ b/camera/libcameraservice/CameraService.cpp
@@ -683,22 +683,30 @@ void CameraService::Client::stopPreview()
 {
     LOGD("stopPreview (pid %d)", getCallingPid());
 
-    Mutex::Autolock lock(mLock);
-    if (checkPid() != NO_ERROR) return;
+    // hold main lock during state transition
+    {
+        Mutex::Autolock lock(mLock);
+        if (checkPid() != NO_ERROR) return;
 
-    if (mHardware == 0) {
-        LOGE("mHardware is NULL, returning.");
-        return;
+        if (mHardware == 0) {
+            LOGE("mHardware is NULL, returning.");
+            return;
+        }
+
+        mHardware->stopPreview();
+        mHardware->disableMsgType(CAMERA_MSG_PREVIEW_FRAME);
+        LOGD("stopPreview(), hardware stopped OK");
+
+        if (mSurface != 0 && !mUseOverlay) {
+            mSurface->unregisterBuffers();
+        }
     }
 
-    mHardware->stopPreview();
-    mHardware->disableMsgType(CAMERA_MSG_PREVIEW_FRAME);
-    LOGD("stopPreview(), hardware stopped OK");
-
-    if (mSurface != 0 && !mUseOverlay) {
-        mSurface->unregisterBuffers();
+    // hold preview buffer lock
+    {
+        Mutex::Autolock lock(mPreviewLock);
+        mPreviewBuffer.clear();
     }
-    mPreviewBuffer.clear();
 }
 
 // stop recording mode
@@ -706,24 +714,31 @@ void CameraService::Client::stopRecording()
 {
     LOGD("stopRecording (pid %d)", getCallingPid());
 
-    Mutex::Autolock lock(mLock);
-    if (checkPid() != NO_ERROR) return;
+    // hold main lock during state transition
+    {
+        Mutex::Autolock lock(mLock);
+        if (checkPid() != NO_ERROR) return;
 
-    if (mHardware == 0) {
-        LOGE("mHardware is NULL, returning.");
-        return;
+        if (mHardware == 0) {
+            LOGE("mHardware is NULL, returning.");
+            return;
+        }
+
+        if (mMediaPlayerBeep.get() != NULL) {
+            mMediaPlayerBeep->seekTo(0);
+            mMediaPlayerBeep->start();
+        }
+
+        mHardware->stopRecording();
+        mHardware->disableMsgType(CAMERA_MSG_VIDEO_FRAME);
+        LOGD("stopRecording(), hardware stopped OK");
     }
 
-    if (mMediaPlayerBeep.get() != NULL) {
-        mMediaPlayerBeep->seekTo(0);
-        mMediaPlayerBeep->start();
+    // hold preview buffer lock
+    {
+        Mutex::Autolock lock(mPreviewLock);
+        mPreviewBuffer.clear();
     }
-
-    mHardware->stopRecording();
-    mHardware->disableMsgType(CAMERA_MSG_VIDEO_FRAME);
-    LOGD("stopRecording(), hardware stopped OK");
-
-    mPreviewBuffer.clear();
 }
 
 // release a recording frame
@@ -1216,10 +1231,10 @@ void CameraService::Client::copyFrameAndPostCopiedFrame(const sp<ICameraClient>&
     // provided it's big enough. Don't allocate the memory or
     // perform the copy if there's no callback.
 
-    // hold the lock while we grab a reference to the preview buffer
+    // hold the preview lock while we grab a reference to the preview buffer
     sp<MemoryHeapBase> previewBuffer;
     {
-        Mutex::Autolock lock(mLock);
+        Mutex::Autolock lock(mPreviewLock);
         if (mPreviewBuffer == 0) {
             mPreviewBuffer = new MemoryHeapBase(size, 0, NULL);
         } else if (size > mPreviewBuffer->virtualSize()) {
diff --git a/camera/libcameraservice/CameraService.h b/camera/libcameraservice/CameraService.h
index 41c5d99fa..3e3e54f0a 100644
--- a/camera/libcameraservice/CameraService.h
+++ b/camera/libcameraservice/CameraService.h
@@ -181,7 +181,6 @@ private:
         mutable     Condition                   mReady;
                     sp<CameraService>           mCameraService;
                     sp<ISurface>                mSurface;
-                    sp<MemoryHeapBase>          mPreviewBuffer;
                     int                         mPreviewCallbackFlag;
 
                     sp<MediaPlayer>             mMediaPlayerClick;
@@ -197,6 +196,9 @@ private:
                     sp<OverlayRef>              mOverlayRef;
                     int                         mOverlayW;
                     int                         mOverlayH;
+
+        mutable     Mutex                       mPreviewLock;
+                    sp<MemoryHeapBase>          mPreviewBuffer;
     };
 
 // ----------------------------------------------------------------------------