MTP: Remove an unnecessary thread from the MtpClient class.

Now a single thread is used for passing USB host events up to MtpClient.

Change-Id: I0e3a277956cb3d1036da122ea10acb03a27844d6
Signed-off-by: Mike Lockwood <lockwood@android.com>

4 files changed, 110 insertions, 132 deletions

diff --git a/media/java/android/media/MtpClient.java b/media/java/android/media/MtpClient.java
index ad6640f65c71..0fe9bb4f4216 100644
--- a/media/java/android/media/MtpClient.java
+++ b/media/java/android/media/MtpClient.java
@@ -44,9 +44,12 @@ public class MtpClient {
         native_finalize();
     }
 
-    public void start() {
-        mEventThread = new MtpEventThread();
-        mEventThread.start();
+    public boolean start() {
+        return native_start();
+    }
+
+    public void stop() {
+        native_stop();
     }
 
     public boolean deleteObject(int deviceID, int objectID) {
@@ -61,24 +64,6 @@ public class MtpClient {
         return native_get_storage_id(deviceID, objectID);
     }
 
-    private class MtpEventThread extends Thread {
-
-        private boolean mDone;
-
-        public MtpEventThread() {
-            super("MtpEventThread");
-        }
-
-        public void run() {
-            Log.d(TAG, "MtpEventThread starting");
-            while (!mDone) {
-                // this will wait for an event from an MTP device
-                native_wait_for_event();
-            }
-            Log.d(TAG, "MtpEventThread exiting");
-        }
-    }
-
     public interface Listener {
         // called when a new MTP device has been discovered
         void deviceAdded(int id);
@@ -99,14 +84,13 @@ public class MtpClient {
         mListener.deviceRemoved(id);
     }
 
-    private MtpEventThread mEventThread;
-
     // used by the JNI code
     private int mNativeContext;
 
     private native final void native_setup();
     private native final void native_finalize();
-    private native void native_wait_for_event();
+    private native boolean native_start();
+    private native void native_stop();
     private native boolean native_delete_object(int deviceID, int objectID);
     private native int native_get_parent(int deviceID, int objectID);
     private native int native_get_storage_id(int deviceID, int objectID);
diff --git a/media/jni/android_media_MtpClient.cpp b/media/jni/android_media_MtpClient.cpp
index 2328889687e4..5c397a6d3db3 100644
--- a/media/jni/android_media_MtpClient.cpp
+++ b/media/jni/android_media_MtpClient.cpp
@@ -22,7 +22,6 @@
 #include <limits.h>
 #include <unistd.h>
 #include <fcntl.h>
-#include <utils/threads.h>
 
 #include "jni.h"
 #include "JNIHelp.h"
@@ -39,34 +38,25 @@ static jmethodID method_deviceAdded;
 static jmethodID method_deviceRemoved;
 static jfieldID field_context;
 
+static void checkAndClearExceptionFromCallback(JNIEnv* env, const char* methodName) {
+    if (env->ExceptionCheck()) {
+        LOGE("An exception was thrown by callback '%s'.", methodName);
+        LOGE_EX(env);
+        env->ExceptionClear();
+    }
+}
+
 class MyClient : public MtpClient {
 private:
-
-    enum {
-        kDeviceAdded = 1,
-        kDeviceRemoved,
-    };
-
     virtual void    deviceAdded(MtpDevice *device);
     virtual void    deviceRemoved(MtpDevice *device);
 
-    bool            reportDeviceAdded(JNIEnv *env, MtpDevice *device);
-    bool            reportDeviceRemoved(JNIEnv *env, MtpDevice *device);
-
-    JNIEnv*         mEnv;
     jobject         mClient;
-    Mutex           mEventLock;
-    Condition       mEventCondition;
-    Mutex           mAckLock;
-    Condition       mAckCondition;
-    int             mEvent;
     MtpDevice*      mEventDevice;
 
 public:
                     MyClient(JNIEnv *env, jobject client);
-    virtual         ~MyClient();
-    void            waitForEvent(JNIEnv *env);
-
+    void            cleanup(JNIEnv *env);
 };
 
 MtpClient* get_client_from_object(JNIEnv* env, jobject javaClient)
@@ -76,87 +66,36 @@ MtpClient* get_client_from_object(JNIEnv* env, jobject javaClient)
 
 
 MyClient::MyClient(JNIEnv *env, jobject client)
-    :   mEnv(env),
-        mClient(env->NewGlobalRef(client)),
-        mEvent(0),
-        mEventDevice(NULL)
+    :   mClient(env->NewGlobalRef(client))
 {
 }
 
-MyClient::~MyClient() {
-    mEnv->DeleteGlobalRef(mClient);
+void MyClient::cleanup(JNIEnv *env) {
+    env->DeleteGlobalRef(mClient);
 }
 
-
 void MyClient::deviceAdded(MtpDevice *device) {
-    LOGD("MyClient::deviceAdded\n");
-    mAckLock.lock();
-    mEventLock.lock();
-    mEvent = kDeviceAdded;
-    mEventDevice = device;
-    mEventCondition.signal();
-    mEventLock.unlock();
-    mAckCondition.wait(mAckLock);
-    mAckLock.unlock();
-}
-
-void MyClient::deviceRemoved(MtpDevice *device) {
-    LOGD("MyClient::deviceRemoved\n");
-    mAckLock.lock();
-    mEventLock.lock();
-    mEvent = kDeviceRemoved;
-    mEventDevice = device;
-    mEventCondition.signal();
-    mEventLock.unlock();
-    mAckCondition.wait(mAckLock);
-    mAckLock.unlock();
-}
-
-bool MyClient::reportDeviceAdded(JNIEnv *env, MtpDevice *device) {
+    JNIEnv* env = AndroidRuntime::getJNIEnv();
     const char* name = device->getDeviceName();
-    LOGD("MyClient::reportDeviceAdded %s\n", name);
+    LOGD("MyClient::deviceAdded %s\n", name);
 
     env->CallVoidMethod(mClient, method_deviceAdded, device->getID());
 
-    return (!env->ExceptionCheck());
+    checkAndClearExceptionFromCallback(env, __FUNCTION__);
 }
 
-bool MyClient::reportDeviceRemoved(JNIEnv *env, MtpDevice *device) {
-   const char* name = device->getDeviceName();
-    LOGD("MyClient::reportDeviceRemoved %s\n", name);
+void MyClient::deviceRemoved(MtpDevice *device) {
+    JNIEnv* env = AndroidRuntime::getJNIEnv();
+    const char* name = device->getDeviceName();
+    LOGD("MyClient::deviceRemoved %s\n", name);
 
     env->CallVoidMethod(mClient, method_deviceRemoved, device->getID());
 
-    return (!env->ExceptionCheck());
+    checkAndClearExceptionFromCallback(env, __FUNCTION__);
 }
 
-void MyClient::waitForEvent(JNIEnv *env) {
-    mEventLock.lock();
-    mEventCondition.wait(mEventLock);
-    mEventLock.unlock();
-
-    switch (mEvent) {
-        case kDeviceAdded:
-            reportDeviceAdded(env, mEventDevice);
-            break;
-        case kDeviceRemoved:
-            reportDeviceRemoved(env, mEventDevice);
-            break;
-    }
-
-    mAckLock.lock();
-    mAckCondition.signal();
-    mAckLock.unlock();
-}
-
-
 // ----------------------------------------------------------------------------
 
-static bool ExceptionCheck(void* env)
-{
-    return ((JNIEnv *)env)->ExceptionCheck();
-}
-
 static void
 android_media_MtpClient_setup(JNIEnv *env, jobject thiz)
 {
@@ -171,15 +110,25 @@ android_media_MtpClient_finalize(JNIEnv *env, jobject thiz)
 {
     LOGD("finalize\n");
     MyClient *client = (MyClient *)env->GetIntField(thiz, field_context);
+    client->cleanup(env);
     delete client;
+    env->SetIntField(thiz, field_context, 0);
+}
+
+static jboolean
+android_media_MtpClient_start(JNIEnv *env, jobject thiz)
+{
+    LOGD("start\n");
+    MyClient *client = (MyClient *)env->GetIntField(thiz, field_context);
+    return client->start();
 }
 
 static void
-android_media_MtpClient_wait_for_event(JNIEnv *env, jobject thiz)
+android_media_MtpClient_stop(JNIEnv *env, jobject thiz)
 {
-    LOGD("wait_for_event\n");
+    LOGD("stop\n");
     MyClient *client = (MyClient *)env->GetIntField(thiz, field_context);
-    client->waitForEvent(env);
+    client->stop();
 }
 
 static jboolean
@@ -223,7 +172,8 @@ android_media_MtpClient_get_storage_id(JNIEnv *env, jobject thiz,
 static JNINativeMethod gMethods[] = {
     {"native_setup",            "()V",  (void *)android_media_MtpClient_setup},
     {"native_finalize",         "()V",  (void *)android_media_MtpClient_finalize},
-    {"native_wait_for_event",   "()V",  (void *)android_media_MtpClient_wait_for_event},
+    {"native_start",            "()Z",  (void *)android_media_MtpClient_start},
+    {"native_stop",             "()V",  (void *)android_media_MtpClient_stop},
     {"native_delete_object",   "(II)Z", (void *)android_media_MtpClient_delete_object},
     {"native_get_parent",      "(II)I", (void *)android_media_MtpClient_get_parent},
     {"native_get_storage_id",  "(II)I", (void *)android_media_MtpClient_get_storage_id},
diff --git a/media/mtp/MtpClient.cpp b/media/mtp/MtpClient.cpp
index fbf1c779ee0a..b4c47af88e7e 100644
--- a/media/mtp/MtpClient.cpp
+++ b/media/mtp/MtpClient.cpp
@@ -27,6 +27,7 @@
 #include <sys/stat.h>
 #include <fcntl.h>
 #include <errno.h>
+#include <utils/threads.h>
 
 #include <usbhost/usbhost.h>
 #include <linux/version.h>
@@ -38,34 +39,59 @@
 
 namespace android {
 
+class MtpClientThread : public Thread {
+private:
+    MtpClient*   mClient;
+
+public:
+    MtpClientThread(MtpClient* client)
+        : mClient(client)
+    {
+    }
+
+    virtual bool threadLoop() {
+        return mClient->threadLoop();
+    }
+};
+
+
 MtpClient::MtpClient()
-    :   mStarted(false)
+    :   mThread(NULL),
+        mUsbHostContext(NULL),
+        mDone(false)
 {
 }
 
 MtpClient::~MtpClient() {
+    usb_host_cleanup(mUsbHostContext);
 }
 
 bool MtpClient::start() {
-    if (mStarted)
+    if (mThread)
         return true;
 
-    if (usb_host_init(usb_device_added, usb_device_removed, this)) {
-        LOGE("MtpClient::start failed\n");
+    mUsbHostContext = usb_host_init();
+    if (!mUsbHostContext)
         return false;
-    }
-    mStarted = true;
+
+    mThread = new MtpClientThread(this);
+    mThread->run("MtpClientThread");
+
     return true;
 }
 
-void MtpClient::usbDeviceAdded(const char *devname) {
+void MtpClient::stop() {
+    mDone = true;
+}
+
+bool MtpClient::usbDeviceAdded(const char *devname) {
     struct usb_descriptor_header* desc;
     struct usb_descriptor_iter iter;
 
     struct usb_device *device = usb_device_open(devname);
     if (!device) {
         LOGE("usb_device_open failed\n");
-        return;
+        return mDone;
     }
 
     usb_descriptor_iter_init(device, &iter);
@@ -90,7 +116,7 @@ void MtpClient::usbDeviceAdded(const char *devname) {
                     ep = (struct usb_endpoint_descriptor *)usb_descriptor_iter_next(&iter);
                     if (!ep || ep->bDescriptorType != USB_DT_ENDPOINT) {
                         LOGE("endpoints not found\n");
-                        return;
+                        return mDone;
                     }
                     if (ep->bmAttributes == USB_ENDPOINT_XFER_BULK) {
                         if (ep->bEndpointAddress & USB_ENDPOINT_DIR_MASK)
@@ -104,7 +130,7 @@ void MtpClient::usbDeviceAdded(const char *devname) {
                 }
                 if (!ep_in_desc || !ep_out_desc || !ep_intr_desc) {
                     LOGE("endpoints not found\n");
-                    return;
+                    return mDone;
                 }
 
                 struct usb_endpoint *ep_in = usb_endpoint_open(device, ep_in_desc);
@@ -116,7 +142,7 @@ void MtpClient::usbDeviceAdded(const char *devname) {
                     usb_endpoint_close(ep_in);
                     usb_endpoint_close(ep_out);
                     usb_endpoint_close(ep_intr);
-                    return;
+                    return mDone;
                 }
 
                 MtpDevice* mtpDevice = new MtpDevice(device, interface->bInterfaceNumber,
@@ -124,12 +150,13 @@ void MtpClient::usbDeviceAdded(const char *devname) {
                 mDeviceList.add(mtpDevice);
                 mtpDevice->initialize();
                 deviceAdded(mtpDevice);
-                return;
+                return mDone;
             }
         }
     }
 
     usb_device_close(device);
+    return mDone;
 }
 
 MtpDevice* MtpClient::getDevice(int id) {
@@ -141,7 +168,7 @@ MtpDevice* MtpClient::getDevice(int id) {
     return NULL;
 }
 
-void MtpClient::usbDeviceRemoved(const char *devname) {
+bool MtpClient::usbDeviceRemoved(const char *devname) {
     for (int i = 0; i < mDeviceList.size(); i++) {
         MtpDevice* device = mDeviceList[i];
         if (!strcmp(devname, device->getDeviceName())) {
@@ -152,16 +179,22 @@ void MtpClient::usbDeviceRemoved(const char *devname) {
             break;
         }
     }
+    return mDone;
+}
+
+bool MtpClient::threadLoop() {
+    usb_host_run(mUsbHostContext, usb_device_added, usb_device_removed, this);
+    return false;
 }
 
-void MtpClient::usb_device_added(const char *devname, void* client_data) {
+int MtpClient::usb_device_added(const char *devname, void* client_data) {
     LOGD("usb_device_added %s\n", devname);
-    ((MtpClient *)client_data)->usbDeviceAdded(devname);
+    return ((MtpClient *)client_data)->usbDeviceAdded(devname);
 }
 
-void MtpClient::usb_device_removed(const char *devname, void* client_data) {
+int MtpClient::usb_device_removed(const char *devname, void* client_data) {
     LOGD("usb_device_removed %s\n", devname);
-    ((MtpClient *)client_data)->usbDeviceRemoved(devname);
+    return ((MtpClient *)client_data)->usbDeviceRemoved(devname);
 }
 
 }  // namespace android
diff --git a/media/mtp/MtpClient.h b/media/mtp/MtpClient.h
index d87c22648e7f..907a80bd94c3 100644
--- a/media/mtp/MtpClient.h
+++ b/media/mtp/MtpClient.h
@@ -19,18 +19,25 @@
 
 #include "MtpTypes.h"
 
+struct usb_host_context;
+
 namespace android {
 
+class MtpClientThread;
+
 class MtpClient {
 private:
-    MtpDeviceList           mDeviceList;
-    bool                    mStarted;
+    MtpDeviceList               mDeviceList;
+    MtpClientThread*            mThread;
+    struct usb_host_context*    mUsbHostContext;
+    bool                        mDone;
 
 public:
                             MtpClient();
     virtual                 ~MtpClient();
 
     bool                    start();
+    void                    stop();
 
     inline MtpDeviceList&   getDeviceList() { return mDeviceList; }
     MtpDevice*              getDevice(int id);
@@ -40,10 +47,14 @@ public:
     virtual void            deviceRemoved(MtpDevice *device) = 0;
 
 private:
-    void                    usbDeviceAdded(const char *devname);
-    void                    usbDeviceRemoved(const char *devname);
-    static void             usb_device_added(const char *devname, void* client_data);
-    static void             usb_device_removed(const char *devname, void* client_data);
+    // these return true if we should stop monitoring USB and clean up
+    bool                    usbDeviceAdded(const char *devname);
+    bool                    usbDeviceRemoved(const char *devname);
+
+    friend class MtpClientThread;
+    bool                    threadLoop();
+    static int              usb_device_added(const char *devname, void* client_data);
+    static int              usb_device_removed(const char *devname, void* client_data);
 };
 
 }; // namespace android