OSDN Git Service

Bug 5293383 ~StreamSourceAppProxy wasn't reached
authorGlenn Kasten <gkasten@google.com>
Mon, 12 Sep 2011 14:26:34 +0000 (07:26 -0700)
committerGlenn Kasten <gkasten@google.com>
Mon, 10 Oct 2011 18:58:28 +0000 (11:58 -0700)
Fix bug where StreamSourceAppProxy destructor wasn't reached,
which caused all sorts of other problems later on.  To see this,
enable the logs StreamSourceAppProxy::~StreamSourceAppProxy and
StreamPlayer::~StreamPlayer.  You'll see that StreamPlayer was destroyed,
but not StreamSourceAppProxy.

As StreamSourceAppProxy is child of StreamPlayer, make the reference
from StreamSourceAppProxy to StreamPlayer a weak reference in case
StreamSourceAppProxy's lifetime exceeds StreamPlayer.  It is not supposed
to any more with this fix, but the wp<> provides extra safety.

StreamPlayer preDestroy no longer bypasses the preDestroy in
GenericMediaPlayer.

Do a full disconnect in GenericMediaPlayer::preDestroy.

Push decremented reference counts for strong pointer through binder
to workaround binder's "optimization".

Extra error-checking in setListener and setBuffers to verify
that mediaserver is calling them correctly.

Use mutex mLock consistently in StreamSourceAppProxy.

Add an explicit StreamSourceAppProxy::disconnect to break
a circular reference, and call it in StreamPlayer destructor.

Make methods private: receivedCmd_l and receivedBuffer_l.

Add explicit clear during preDestroy to give up references earlier.

Warning: setDataSource(NULL) is not supported by NuPlayer yet,
this depends on another change in frameworks/base, so it is
commented out for now.

Rename mPlayerPrepared to mPreparedPlayer to avoid confusion with the
enum mPlayerPrepared.

Change-Id: Ie5f554c206027d22204eb86edd15489c6281b512

wilhelm/src/android/android_GenericMediaPlayer.cpp
wilhelm/src/android/android_GenericMediaPlayer.h
wilhelm/src/android/android_StreamPlayer.cpp
wilhelm/src/android/android_StreamPlayer.h

index 9388867..44c9c63 100644 (file)
@@ -182,9 +182,22 @@ GenericMediaPlayer::~GenericMediaPlayer() {
 }
 
 void GenericMediaPlayer::preDestroy() {
-    SL_LOGD("GenericMediaPlayer::preDestroy()");
-    if (mPlayer != 0) {
-        mPlayer->stop();
+    // FIXME can't access mPlayer from outside the looper (no mutex!) so using mPreparedPlayer
+    sp<IMediaPlayer> player;
+    getPreparedPlayer(player);
+    if (player != NULL) {
+        player->stop();
+        // causes CHECK failure in Nuplayer, but commented out in the subclass preDestroy
+        player->setDataSource(NULL);
+        player->setVideoSurface(NULL);
+        player->disconnect();
+        // release all references to the IMediaPlayer
+        // FIXME illegal if not on looper
+        //mPlayer.clear();
+        {
+            Mutex::Autolock _l(mPreparedPlayerLock);
+            mPreparedPlayer.clear();
+        }
     }
     GenericPlayer::preDestroy();
 }
@@ -200,7 +213,7 @@ void GenericMediaPlayer::preDestroy() {
 void GenericMediaPlayer::getPositionMsec(int* msec) {
     SL_LOGD("GenericMediaPlayer::getPositionMsec()");
     sp<IMediaPlayer> player;
-    getPlayerPrepared(player);
+    getPreparedPlayer(player);
     // To avoid deadlock, directly call the MediaPlayer object
     if (player == 0 || player->getCurrentPosition(msec) != NO_ERROR) {
         *msec = ANDROID_UNKNOWN_TIME;
@@ -451,9 +464,9 @@ void GenericMediaPlayer::afterMediaPlayerPreparedSuccessfully() {
     assert(mStateFlags & kFlagPrepared);
     // Mark this player as prepared successfully, so safe to directly call getCurrentPosition
     {
-        Mutex::Autolock _l(mPlayerPreparedLock);
-        assert(mPlayerPrepared == 0);
-        mPlayerPrepared = mPlayer;
+        Mutex::Autolock _l(mPreparedPlayerLock);
+        assert(mPreparedPlayer == 0);
+        mPreparedPlayer = mPlayer;
     }
     // retrieve channel count
     assert(UNKNOWN_NUMCHANNELS == mChannelCount);
@@ -507,10 +520,10 @@ void GenericMediaPlayer::afterMediaPlayerPreparedSuccessfully() {
 
 //--------------------------------------------------
 // If player is prepared successfully, set output parameter to that reference, otherwise NULL
-void GenericMediaPlayer::getPlayerPrepared(sp<IMediaPlayer> &playerPrepared)
+void GenericMediaPlayer::getPreparedPlayer(sp<IMediaPlayer> &preparedPlayer)
 {
-    Mutex::Autolock _l(mPlayerPreparedLock);
-    playerPrepared = mPlayerPrepared;
+    Mutex::Autolock _l(mPreparedPlayerLock);
+    preparedPlayer = mPreparedPlayer;
 }
 
 } // namespace android
index 6d8c857..c3a9d00 100644 (file)
@@ -95,6 +95,7 @@ protected:
     sp<Surface> mVideoSurface;
     sp<ISurfaceTexture> mVideoSurfaceTexture;
 
+    // only safe to access from within Realize and looper
     sp<IMediaPlayer> mPlayer;
     // Receives Android MediaPlayer events from mPlayer
     const sp<MediaPlayerNotificationClient> mPlayerClient;
@@ -108,9 +109,11 @@ private:
     DISALLOW_EVIL_CONSTRUCTORS(GenericMediaPlayer);
     void afterMediaPlayerPreparedSuccessfully();
 
-    Mutex mPlayerPreparedLock;          // protects mPlayerPrepared
-    sp<IMediaPlayer> mPlayerPrepared;   // non-NULL if MediaPlayer exists and prepared, write once
-    void getPlayerPrepared(sp<IMediaPlayer> &playerPrepared);   // safely read mPlayerPrepared
+protected:  // FIXME temporary
+    Mutex mPreparedPlayerLock;          // protects mPreparedPlayer
+    sp<IMediaPlayer> mPreparedPlayer;   // non-NULL if MediaPlayer exists and prepared, write once
+private:
+    void getPreparedPlayer(sp<IMediaPlayer> &preparedPlayer);   // safely read mPreparedPlayer
 
 };
 
index 9307a98..a806290 100644 (file)
@@ -22,6 +22,7 @@
 #include <media/IStreamSource.h>
 #include <media/IMediaPlayerService.h>
 #include <media/stagefright/foundation/ADebug.h>
+#include <binder/IPCThreadState.h>
 
 
 //--------------------------------------------------------------------------------------------------
@@ -30,7 +31,12 @@ namespace android {
 StreamSourceAppProxy::StreamSourceAppProxy(
         IAndroidBufferQueue *androidBufferQueue,
         const sp<CallbackProtector> &callbackProtector,
-        const sp<StreamPlayer> &player) :
+        // sp<StreamPlayer> would cause StreamPlayer's destructor to run during it's own
+        // construction.   If you pass in a sp<> to 'this' inside a constructor, then first the
+        // refcount is increased from 0 to 1, then decreased from 1 to 0, which causes the object's
+        // destructor to run from inside it's own constructor.
+        StreamPlayer * /* const sp<StreamPlayer> & */ player) :
+    mBuffersHasBeenSet(false),
     mAndroidBufferQueue(androidBufferQueue),
     mCallbackProtector(callbackProtector),
     mPlayer(player)
@@ -39,9 +45,9 @@ StreamSourceAppProxy::StreamSourceAppProxy(
 }
 
 StreamSourceAppProxy::~StreamSourceAppProxy() {
-    SL_LOGD("StreamSourceAppProxy::~StreamSourceAppProxy()");
-    mListener.clear();
-    mBuffers.clear();
+    // FIXME make this an SL_LOGV later; this just proves that the bug is fixed
+    SL_LOGI("StreamSourceAppProxy::~StreamSourceAppProxy()");
+    disconnect();
 }
 
 const SLuint32 StreamSourceAppProxy::kItemProcessed[NB_BUFFEREVENT_ITEM_FIELDS] = {
@@ -53,26 +59,34 @@ const SLuint32 StreamSourceAppProxy::kItemProcessed[NB_BUFFEREVENT_ITEM_FIELDS]
 //--------------------------------------------------
 // IStreamSource implementation
 void StreamSourceAppProxy::setListener(const sp<IStreamListener> &listener) {
+    assert(listener != NULL);
     Mutex::Autolock _l(mLock);
+    assert(mListener == NULL);
     mListener = listener;
 }
 
 void StreamSourceAppProxy::setBuffers(const Vector<sp<IMemory> > &buffers) {
+    Mutex::Autolock _l(mLock);
+    assert(!mBuffersHasBeenSet);
     mBuffers = buffers;
+    mBuffersHasBeenSet = true;
 }
 
 void StreamSourceAppProxy::onBufferAvailable(size_t index) {
     //SL_LOGD("StreamSourceAppProxy::onBufferAvailable(%d)", index);
 
-    CHECK_LT(index, mBuffers.size());
-    sp<IMemory> mem = mBuffers.itemAt(index);
-    SLAint64 length = (SLAint64) mem->size();
-
     {
         Mutex::Autolock _l(mLock);
+        // assert not needed because if not set, size() will be zero and the CHECK_LT will also fail
+        // assert(mBuffersHasBeenSet);
+        CHECK_LT(index, mBuffers.size());
+#if 0   // enable if needed for debugging
+        sp<IMemory> mem = mBuffers.itemAt(index);
+        SLAint64 length = (SLAint64) mem->size();
+#endif
         mAvailableBuffers.push_back(index);
+        //SL_LOGD("onBufferAvailable() now %d buffers available in queue", mAvailableBuffers.size());
     }
-    //SL_LOGD("onBufferAvailable() now %d buffers available in queue", mAvailableBuffers.size());
 
     // a new shared mem buffer is available: let's try to fill immediately
     pullFromBuffQueue();
@@ -90,6 +104,18 @@ void StreamSourceAppProxy::receivedBuffer_l(size_t buffIndex, size_t buffLength)
     }
 }
 
+void StreamSourceAppProxy::disconnect() {
+    Mutex::Autolock _l(mLock);
+    mListener.clear();
+    // Force binder to push the decremented reference count for sp<IStreamListener>.
+    // mediaserver and client both have sp<> to the other. When you decrement an sp<>
+    // reference count, binder doesn't push that to the other process immediately.
+    IPCThreadState::self()->flushCommands();
+    mBuffers.clear();
+    mBuffersHasBeenSet = false;
+    mAvailableBuffers.clear();
+}
+
 //--------------------------------------------------
 // consumption from ABQ: pull from the ABQ, and push to shared memory (media server)
 void StreamSourceAppProxy::pullFromBuffQueue() {
@@ -141,8 +167,11 @@ void StreamSourceAppProxy::pullFromBuffQueue() {
             }
             if (oldFront->mItems.mTsCmdData.mTsCmdCode & (ANDROID_MP2TSEVENT_DISCONTINUITY |
                     ANDROID_MP2TSEVENT_DISCON_NEWPTS | ANDROID_MP2TSEVENT_FORMAT_CHANGE)) {
-                // FIXME see note at onSeek
-                mPlayer->seek(ANDROID_UNKNOWN_TIME);
+                const sp<StreamPlayer> player(mPlayer.promote());
+                if (player != NULL) {
+                    // FIXME see note at onSeek
+                    player->seek(ANDROID_UNKNOWN_TIME);
+                }
             }
             oldFront->mItems.mTsCmdData.mTsCmdCode = ANDROID_MP2TSEVENT_NONE;
         }
@@ -217,7 +246,10 @@ void StreamSourceAppProxy::pullFromBuffQueue() {
             if (!mAvailableBuffers.empty()) {
                 // there is still room in the shared memory, recheck later if we can pull
                 // data from the buffer queue and write it to shared memory
-                mPlayer->queueRefilled();
+                const sp<StreamPlayer> player(mPlayer.promote());
+                if (player != NULL) {
+                    player->queueRefilled();
+                }
             }
         }
 
@@ -261,6 +293,7 @@ StreamPlayer::StreamPlayer(AudioPlayback_Parameters* params, bool hasVideo,
 
 StreamPlayer::~StreamPlayer() {
     SL_LOGD("StreamPlayer::~StreamPlayer()");
+    mAppProxy->disconnect();
 }
 
 
@@ -290,14 +323,24 @@ void StreamPlayer::preDestroy() {
             mStopForDestroyCondition.wait(mStopForDestroyLock);
         }
     }
-    // skipping past GenericMediaPlayer::preDestroy
-    GenericPlayer::preDestroy();
+    // GenericMediaPlayer::preDestroy will repeat some of what we've done, but that's benign
+    GenericMediaPlayer::preDestroy();
 }
 
 
 void StreamPlayer::onStopForDestroy() {
     if (mPlayer != 0) {
         mPlayer->stop();
+        // causes CHECK failure in Nuplayer
+        //mPlayer->setDataSource(NULL);
+        mPlayer->setVideoSurface(NULL);
+        mPlayer->disconnect();
+        mPlayer.clear();
+        {
+            // FIXME ugh make this a method
+            Mutex::Autolock _l(mPreparedPlayerLock);
+            mPreparedPlayer.clear();
+        }
     }
     mStopForDestroyCompleted = true;
     mStopForDestroyCondition.signal();
index 51cad7d..1f520ca 100644 (file)
@@ -33,37 +33,45 @@ public:
     StreamSourceAppProxy(
             IAndroidBufferQueue *androidBufferQueue,
             const sp<CallbackProtector> &callbackProtector,
-            const sp<StreamPlayer> &player);
+            StreamPlayer *player);
     virtual ~StreamSourceAppProxy();
 
     // store an item structure to indicate a processed buffer
     static const SLuint32 kItemProcessed[NB_BUFFEREVENT_ITEM_FIELDS];
 
     // IStreamSource implementation
-    virtual void setListener(const sp<IStreamListener> &listener);
-    virtual void setBuffers(const Vector<sp<IMemory> > &buffers);
+    virtual void setListener(const sp<IStreamListener> &listener); // mediaserver calls exactly once
+    virtual void setBuffers(const Vector<sp<IMemory> > &buffers);  // mediaserver calls exactly once
     virtual void onBufferAvailable(size_t index);
 
     // Consumption from ABQ
     void pullFromBuffQueue();
 
+private:
     void receivedCmd_l(IStreamListener::Command cmd, const sp<AMessage> &msg = NULL);
     void receivedBuffer_l(size_t buffIndex, size_t buffLength);
 
+public:
+    // Call at least once prior to releasing the last strong reference to this object. It causes
+    // the player to release all of its resources, similar to android.media.MediaPlayer disconnect.
+    void disconnect();
+
 private:
-    // for mListener and mAvailableBuffers
+    // protects mListener, mBuffers, mBuffersHasBeenSet, and mAvailableBuffers
     Mutex mLock;
+
     sp<IStreamListener> mListener;
     // array of shared memory buffers
     Vector<sp<IMemory> > mBuffers;
+    bool mBuffersHasBeenSet;
     // list of available buffers in shared memory, identified by their index
     List<size_t> mAvailableBuffers;
 
     // the Android Buffer Queue from which data is consumed and written to shared memory
-    IAndroidBufferQueue* mAndroidBufferQueue;
+    IAndroidBufferQueue* const mAndroidBufferQueue;
 
     const sp<CallbackProtector> mCallbackProtector;
-    const sp<StreamPlayer> mPlayer;
+    const wp<StreamPlayer> mPlayer;
 
     DISALLOW_EVIL_CONSTRUCTORS(StreamSourceAppProxy);
 };