CameraHAL: Prevent deadlock in AppCallbackNotifier::stop()

There is a small chance that a stopPreview call can come from
CameraService right around the same time Encoder thread will
send a data callback for the video snapshot. Currently, we
are waiting for the encoder thread to join in AppCallbackNofier::
stop(), so we could deadlock if CameraService locks in
lockIfMessageWanted for the video snapshot.

Instead of waiting for Encoder thread to join, we can make
cancel() block until Encoder thread is done canceling the
encode. After cancel() returns, we can free up the cookies
that we passed to it, so Encoder thread does not need to call
the callback function to AppCallbackNotifier.

Change-Id: Ib453d49d91077925b143c812d43a7d1b782c181c
Signed-off-by: Iliyan Malchev <malchev@google.com>

2 files changed, 51 insertions, 25 deletions

diff --git a/camera/AppCallbackNotifier.cpp b/camera/AppCallbackNotifier.cpp
index c845796..18f48f3 100644
--- a/camera/AppCallbackNotifier.cpp
+++ b/camera/AppCallbackNotifier.cpp
@@ -38,12 +38,24 @@ void AppCallbackNotifierEncoderCallback(void* main_jpeg,
                                         CameraFrame::FrameType type,
                                         void* cookie1,
                                         void* cookie2,
-                                        void* cookie3)
+                                        void* cookie3,
+                                        bool canceled)
 {
-    if (cookie1) {
+    if (cookie1 && !canceled) {
         AppCallbackNotifier* cb = (AppCallbackNotifier*) cookie1;
         cb->EncoderDoneCb(main_jpeg, thumb_jpeg, type, cookie2, cookie3);
     }
+
+    if (main_jpeg) {
+        free(main_jpeg);
+    }
+
+    if (thumb_jpeg) {
+       if (((Encoder_libjpeg::params *) thumb_jpeg)->dst) {
+           free(((Encoder_libjpeg::params *) thumb_jpeg)->dst);
+       }
+       free(thumb_jpeg);
+    }
 }
 
 /*--------------------NotificationHandler Class STARTS here-----------------------------*/
@@ -129,30 +141,17 @@ void AppCallbackNotifier::EncoderDoneCb(void* main_jpeg, void* thumb_jpeg, Camer
 
  exit:
 
-    if (main_jpeg) {
-        free(main_jpeg);
-    }
-
-    if (thumb_jpeg) {
-       if (((Encoder_libjpeg::params *) thumb_jpeg)->dst) {
-           free(((Encoder_libjpeg::params *) thumb_jpeg)->dst);
-       }
-       free(thumb_jpeg);
-    }
-
-    if (encoded_mem) {
-        encoded_mem->release(encoded_mem);
-    }
-
     if (picture) {
         picture->release(picture);
     }
 
-    if (cookie2) {
-        delete (ExifElementsTable*) cookie2;
-    }
-
     if (mNotifierState == AppCallbackNotifier::NOTIFIER_STARTED) {
+        if (encoded_mem) {
+            encoded_mem->release(encoded_mem);
+        }
+        if (cookie2) {
+            delete (ExifElementsTable*) cookie2;
+        }
         encoder = gEncoderQueue.valueFor(src);
         if (encoder.get()) {
             gEncoderQueue.removeItem(src);
@@ -1733,9 +1732,20 @@ status_t AppCallbackNotifier::stop()
 
     while(!gEncoderQueue.isEmpty()) {
         sp<Encoder_libjpeg> encoder = gEncoderQueue.valueAt(0);
+        camera_memory_t* encoded_mem = NULL;
+        ExifElementsTable* exif = NULL;
+
         if(encoder.get()) {
             encoder->cancel();
-            encoder->join();
+
+            encoder->getCookies(NULL, (void**) &encoded_mem, (void**) &exif);
+            if (encoded_mem) {
+                encoded_mem->release(encoded_mem);
+            }
+            if (exif) {
+                delete exif;
+            }
+
             encoder.clear();
         }
         gEncoderQueue.removeItemsAt(0);
diff --git a/camera/inc/Encoder_libjpeg.h b/camera/inc/Encoder_libjpeg.h
index 457a0dc..26136fa 100644
--- a/camera/inc/Encoder_libjpeg.h
+++ b/camera/inc/Encoder_libjpeg.h
@@ -30,6 +30,9 @@
 extern "C" {
 #include "jhead.h"
 }
+
+#define CANCEL_TIMEOUT 3000000 // 3 seconds
+
 namespace android {
 /**
  * libjpeg encoder class - uses libjpeg to encode yuv
@@ -41,7 +44,8 @@ typedef void (*encoder_libjpeg_callback_t) (void* main_jpeg,
                                             CameraFrame::FrameType type,
                                             void* cookie1,
                                             void* cookie2,
-                                            void* cookie3);
+                                            void* cookie3,
+                                            bool canceled);
 
 // these have to match strings defined in external/jhead/exif.c
 static const char TAG_MODEL[] = "Model";
@@ -131,6 +135,7 @@ class Encoder_libjpeg : public Thread {
               mCancelEncoding(false), mCookie1(cookie1), mCookie2(cookie2), mCookie3(cookie3),
               mType(type), mThumb(NULL) {
             this->incStrong(this);
+            mCancelSem.Create(0);
         }
 
         ~Encoder_libjpeg() {
@@ -149,6 +154,9 @@ class Encoder_libjpeg : public Thread {
             // encode our main image
             size = encode(mMainInput);
 
+            // signal cancel semaphore incase somebody is waiting
+            mCancelSem.Signal();
+
             // check if it is main jpeg thread
             if(mThumb.get()) {
                 // wait until tn jpeg thread exits.
@@ -158,7 +166,7 @@ class Encoder_libjpeg : public Thread {
             }
 
             if(mCb) {
-                mCb(mMainInput, mThumbnailInput, mType, mCookie1, mCookie2, mCookie3);
+                mCb(mMainInput, mThumbnailInput, mType, mCookie1, mCookie2, mCookie3, mCancelEncoding);
             }
 
             // encoder thread runs, self-destructs, and then exits
@@ -167,10 +175,17 @@ class Encoder_libjpeg : public Thread {
         }
 
         void cancel() {
+           mCancelEncoding = true;
            if (mThumb.get()) {
                mThumb->cancel();
+               mCancelSem.WaitTimeout(CANCEL_TIMEOUT);
            }
-           mCancelEncoding = true;
+        }
+
+        void getCookies(void **cookie1, void **cookie2, void **cookie3) {
+            if (cookie1) *cookie1 = mCookie1;
+            if (cookie2) *cookie2 = mCookie2;
+            if (cookie3) *cookie3 = mCookie3;
         }
 
     private:
@@ -183,6 +198,7 @@ class Encoder_libjpeg : public Thread {
         void* mCookie3;
         CameraFrame::FrameType mType;
         sp<Encoder_libjpeg> mThumb;
+        Semaphore mCancelSem;
 
         size_t encode(params*);
 };