Fix issues in GenericPlayer destruction

Added a preDestroy() mechanism for the subclasses of
 GenericPlayer.
Added a lock around the update and use of the callback
 GenericPlayer is using for notifications to the
 OpenSL ES framework. This prevents notifications after
 the GenericPlayer is flagged for destruction.
Added a lock to protect the update of the audio source
 running state, so it doesn't get used after the
 AudioSfDecoder is flagged for destruction.
Before stopping the audio source of an AudioSfDecoder
 object, release the code buffer if necessary, to
 ensure proper teardown of the audio source (OMXCodec)

Change-Id: I6cf08d169a6da622552dda5101dbc61e663ce6aa

diff --git a/wilhelm/src/android/AudioPlayer_to_android.cpp b/wilhelm/src/android/AudioPlayer_to_android.cpp
index e6dbfc4..3ada688 100644 (file)
--- a/wilhelm/src/android/AudioPlayer_to_android.cpp
+++ b/wilhelm/src/android/AudioPlayer_to_android.cpp
@@ -1433,9 +1433,14 @@ SLresult android_audioPlayer_realize(CAudioPlayer *pAudioPlayer, SLboolean async
  * Called with a lock on AudioPlayer
  */
 SLresult android_audioPlayer_preDestroy(CAudioPlayer *pAudioPlayer) {
-    SL_LOGV("android_audioPlayer_preDestroy(%p)", pAudioPlayer);
+    SL_LOGD("android_audioPlayer_preDestroy(%p)", pAudioPlayer);
     SLresult result = SL_RESULT_SUCCESS;
 
+    if (pAudioPlayer->mAPlayer != 0) {
+        pAudioPlayer->mAPlayer->preDestroy();
+    }
+    SL_LOGD("android_audioPlayer_preDestroy(%p) after mAPlayer->preDestroy()", pAudioPlayer);
+
     object_unlock_exclusive(&pAudioPlayer->mObject);
     if (pAudioPlayer->mCallbackProtector != 0) {
         pAudioPlayer->mCallbackProtector->requestCbExitAndWait();

diff --git a/wilhelm/src/android/android_AudioSfDecoder.cpp b/wilhelm/src/android/android_AudioSfDecoder.cpp
index c62ade8..6a6bcf1 100644 (file)
--- a/wilhelm/src/android/android_AudioSfDecoder.cpp
+++ b/wilhelm/src/android/android_AudioSfDecoder.cpp
@@ -40,6 +40,7 @@ namespace android {
 AudioSfDecoder::AudioSfDecoder(const AudioPlayback_Parameters* params) : GenericPlayer(params),
         mDataSource(0),
         mAudioSource(0),
+        mAudioSourceStarted(false),
         mBitrate(-1),
         mChannelMask(ANDROID_UNKNOWN_CHANNELMASK),
         mDurationUsec(-1),
@@ -56,8 +57,24 @@ AudioSfDecoder::AudioSfDecoder(const AudioPlayback_Parameters* params) : Generic
 
 AudioSfDecoder::~AudioSfDecoder() {
     SL_LOGV("AudioSfDecoder::~AudioSfDecoder()");
-    if (mAudioSource != 0) {
-        mAudioSource->stop();
+}
+
+
+void AudioSfDecoder::preDestroy() {
+    GenericPlayer::preDestroy();
+    SL_LOGD("AudioSfDecoder::preDestroy()");
+    {
+        Mutex::Autolock _l(mBufferSourceLock);
+
+        if (NULL != mDecodeBuffer) {
+            mDecodeBuffer->release();
+            mDecodeBuffer = NULL;
+        }
+
+        if ((mAudioSource != 0) && mAudioSourceStarted) {
+            mAudioSource->stop();
+            mAudioSourceStarted = false;
+        }
     }
 }
 
@@ -156,17 +173,30 @@ bool AudioSfDecoder::getPcmFormatKeyValue(uint32_t index, uint32_t size, uint32_
 
 //--------------------------------------------------
 // Event handlers
+//  it is strictly verboten to call those methods outside of the event loop
+
+// Initializes the data and audio sources, and update the PCM format info
+// post-condition: upon successful initialization based on the player data locator
+//    GenericPlayer::onPrepare() was called
+//    mDataSource != 0
+//    mAudioSource != 0
+//    mAudioSourceStarted == true
 void AudioSfDecoder::onPrepare() {
     SL_LOGD("AudioSfDecoder::onPrepare()");
+    Mutex::Autolock _l(mBufferSourceLock);
 
+    // Initialize the PCM format info with the known parameters before the start of the decode
     mPcmFormatValues[ANDROID_KEY_INDEX_PCMFORMAT_BITSPERSAMPLE] = SL_PCMSAMPLEFORMAT_FIXED_16;
     mPcmFormatValues[ANDROID_KEY_INDEX_PCMFORMAT_CONTAINERSIZE] = 16;
     mPcmFormatValues[ANDROID_KEY_INDEX_PCMFORMAT_ENDIANNESS] = SL_BYTEORDER_LITTLEENDIAN;
-    // initialization with the default values
+    //    initialization with the default values: they will be replaced by the actual values
+    //      once the decoder has figured them out
     mPcmFormatValues[ANDROID_KEY_INDEX_PCMFORMAT_NUMCHANNELS] = mChannelCount;
     mPcmFormatValues[ANDROID_KEY_INDEX_PCMFORMAT_SAMPLESPERSEC] = mSampleRateHz;
     mPcmFormatValues[ANDROID_KEY_INDEX_PCMFORMAT_CHANNELMASK] = mChannelMask;
 
+    //---------------------------------
+    // Instantiate and initialize the data source for the decoder
     sp<DataSource> dataSource;
 
     switch (mDataLocatorType) {
@@ -201,6 +231,8 @@ void AudioSfDecoder::onPrepare() {
         TRESPASS();
     }
 
+    //---------------------------------
+    // Instanciate and initialize the decoder attached to the data source
     sp<MediaExtractor> extractor = MediaExtractor::Create(dataSource);
     if (extractor == NULL) {
         SL_LOGE("AudioSfDecoder::onPrepare: Could not instantiate extractor.");
@@ -252,6 +284,7 @@ void AudioSfDecoder::onPrepare() {
         mDurationUsec = -1;
     }
 
+    // the audio content is not raw PCM, so we need a decoder
     if (!isRawAudio) {
         OMXClient client;
         CHECK_EQ(client.connect(), (status_t)OK);
@@ -276,8 +309,11 @@ void AudioSfDecoder::onPrepare() {
         return;
     }
 
+    //---------------------------------
+    // The data source, and audio source (a decoder if required) are ready to be used
     mDataSource = dataSource;
     mAudioSource = source;
+    mAudioSourceStarted = true;
 
     if (!hasChannelCount) {
         // even though the channel count was already queried above, there are issues with some
@@ -412,6 +448,7 @@ void AudioSfDecoder::onDecode() {
         // application set play state to paused which failed, then set play state to playing
         return;
     }
+
     if (wantPrefetch()
             && (getCacheRemaining(&eos) == kStatusLow)
             && !eos) {
@@ -439,12 +476,16 @@ void AudioSfDecoder::onDecode() {
     }
 
     {
-        Mutex::Autolock _l(mDecodeBufferLock);
+        Mutex::Autolock _l(mBufferSourceLock);
+
         if (NULL != mDecodeBuffer) {
             // the current decoded buffer hasn't been rendered, drop it
             mDecodeBuffer->release();
             mDecodeBuffer = NULL;
         }
+        if(!mAudioSourceStarted) {
+            return;
+        }
         err = mAudioSource->read(&mDecodeBuffer, &readOptions);
         if (err == OK) {
             CHECK(mDecodeBuffer->meta_data()->findInt64(kKeyTime, &mLastDecodedPositionUs));
@@ -509,7 +550,7 @@ void AudioSfDecoder::onDecode() {
 void AudioSfDecoder::onRender() {
     //SL_LOGV("AudioSfDecoder::onRender");
 
-    Mutex::Autolock _l(mDecodeBufferLock);
+    Mutex::Autolock _l(mBufferSourceLock);
 
     if (NULL == mDecodeBuffer) {
         // nothing to render, move along
@@ -580,21 +621,29 @@ void AudioSfDecoder::notifyPrepared(status_t prepareRes) {
 
 
 void AudioSfDecoder::onNotify(const sp<AMessage> &msg) {
-    if (NULL == mNotifyClient) {
-        return;
+    notif_cbf_t notifyClient;
+    void*       notifyUser;
+    {
+        android::Mutex::Autolock autoLock(mNotifyClientLock);
+        if (NULL == mNotifyClient) {
+            return;
+        } else {
+            notifyClient = mNotifyClient;
+            notifyUser   = mNotifyUser;
+        }
     }
     int32_t val;
     if (msg->findInt32(PLAYEREVENT_PREFETCHSTATUSCHANGE, &val)) {
         SL_LOGV("\tASfPlayer notifying %s = %d", PLAYEREVENT_PREFETCHSTATUSCHANGE, val);
-        mNotifyClient(kEventPrefetchStatusChange, val, 0, mNotifyUser);
+        notifyClient(kEventPrefetchStatusChange, val, 0, notifyUser);
     }
     else if (msg->findInt32(PLAYEREVENT_PREFETCHFILLLEVELUPDATE, &val)) {
         SL_LOGV("\tASfPlayer notifying %s = %d", PLAYEREVENT_PREFETCHFILLLEVELUPDATE, val);
-        mNotifyClient(kEventPrefetchFillLevelUpdate, val, 0, mNotifyUser);
+        notifyClient(kEventPrefetchFillLevelUpdate, val, 0, notifyUser);
     }
     else if (msg->findInt32(PLAYEREVENT_ENDOFSTREAM, &val)) {
         SL_LOGV("\tASfPlayer notifying %s = %d", PLAYEREVENT_ENDOFSTREAM, val);
-        mNotifyClient(kEventEndOfStream, val, 0, mNotifyUser);
+        notifyClient(kEventEndOfStream, val, 0, notifyUser);
     }
     else {
         GenericPlayer::onNotify(msg);

diff --git a/wilhelm/src/android/android_AudioSfDecoder.h b/wilhelm/src/android/android_AudioSfDecoder.h
index 05628e7..cf280de 100644 (file)
--- a/wilhelm/src/android/android_AudioSfDecoder.h
+++ b/wilhelm/src/android/android_AudioSfDecoder.h
@@ -46,6 +46,8 @@ public:
     AudioSfDecoder(const AudioPlayback_Parameters* params);
     virtual ~AudioSfDecoder();
 
+    virtual void preDestroy();
+
     // overridden from GenericPlayer
     virtual void play();
 
@@ -89,8 +91,10 @@ protected:
     virtual void startAudioSink() = 0;
     virtual void pauseAudioSink() = 0;
 
-    sp<DataSource> mDataSource;
-    sp<MediaSource> mAudioSource;
+    sp<DataSource>  mDataSource; // where the raw data comes from
+    sp<MediaSource> mAudioSource;// the decoder reading from the data source
+    // used to indicate mAudioSource was successfully started, but wasn't stopped
+    bool            mAudioSourceStarted;
 
     // negative values indicate invalid value
     int64_t mBitrate;  // in bits/sec
@@ -99,8 +103,9 @@ protected:
 
     // buffer passed from decoder to renderer
     MediaBuffer *mDecodeBuffer;
-    // mutex used to protect the decode buffer
-    Mutex       mDecodeBufferLock;
+
+    // mutex used to protect the decode buffer, the audio source and its running state
+    Mutex       mBufferSourceLock;
 
 private:
 

diff --git a/wilhelm/src/android/android_AudioToCbRenderer.cpp b/wilhelm/src/android/android_AudioToCbRenderer.cpp
index 2ed54d6..caa3d4b 100644 (file)
--- a/wilhelm/src/android/android_AudioToCbRenderer.cpp
+++ b/wilhelm/src/android/android_AudioToCbRenderer.cpp
@@ -53,9 +53,9 @@ void AudioToCbRenderer::onPrepare() {
 
 
 void AudioToCbRenderer::onRender() {
-    SL_LOGV("AudioToCbRenderer::onRender");
+    SL_LOGD("AudioToCbRenderer::onRender");
 
-    Mutex::Autolock _l(mDecodeBufferLock);
+    Mutex::Autolock _l(mBufferSourceLock);
 
     if (NULL == mDecodeBuffer) {
         // nothing to render, move along
@@ -93,14 +93,17 @@ void AudioToCbRenderer::onRender() {
 //--------------------------------------------------
 // Audio output
 void AudioToCbRenderer::createAudioSink() {
-    SL_LOGD("AudioToCbRenderer::createAudioSink()");
-    SL_LOGV("sample rate = %d nb channels = %d", mSampleRateHz, mNumChannels);
+    SL_LOGD("AudioToCbRenderer::createAudioSink() sample rate = %d, nb channels = %d",
+            mSampleRateHz, mChannelCount);
 }
 
 
 void AudioToCbRenderer::updateAudioSink() {
     SL_LOGD("AudioToCbRenderer::updateAudioSink()");
-    if (mAudioSource != 0) {
+
+    Mutex::Autolock _l(mBufferSourceLock);
+
+    if ((mAudioSource != 0) && mAudioSourceStarted) {
         sp<MetaData> meta = mAudioSource->getFormat();
 
         SL_LOGV("old sample rate = %d", mSampleRateHz);

diff --git a/wilhelm/src/android/android_GenericPlayer.cpp b/wilhelm/src/android/android_GenericPlayer.cpp
index 815a3c8..d39e1dc 100644 (file)
--- a/wilhelm/src/android/android_GenericPlayer.cpp
+++ b/wilhelm/src/android/android_GenericPlayer.cpp
@@ -53,24 +53,36 @@ GenericPlayer::GenericPlayer(const AudioPlayback_Parameters* params) :
 GenericPlayer::~GenericPlayer() {
     SL_LOGV("GenericPlayer::~GenericPlayer()");
 
-    mLooper->stop();
-    mLooper->unregisterHandler(id());
-    mLooper.clear();
-
 }
 
 
 void GenericPlayer::init(const notif_cbf_t cbf, void* notifUser) {
     SL_LOGD("GenericPlayer::init()");
 
-    mNotifyClient = cbf;
-    mNotifyUser = notifUser;
+    {
+        android::Mutex::Autolock autoLock(mNotifyClientLock);
+        mNotifyClient = cbf;
+        mNotifyUser = notifUser;
+    }
 
     mLooper->registerHandler(this);
     mLooper->start(false /*runOnCallingThread*/, false /*canCallJava*/, mLooperPriority);
 }
 
 
+void GenericPlayer::preDestroy() {
+    SL_LOGD("GenericPlayer::preDestroy()");
+    {
+        android::Mutex::Autolock autoLock(mNotifyClientLock);
+        mNotifyClient = NULL;
+        mNotifyUser = NULL;
+    }
+
+    mLooper->stop();
+    mLooper->unregisterHandler(id());
+}
+
+
 void GenericPlayer::setDataSource(const char *uri) {
     resetDataLocator();
 
@@ -323,26 +335,34 @@ void GenericPlayer::onPrepare() {
 
 
 void GenericPlayer::onNotify(const sp<AMessage> &msg) {
-    if (NULL == mNotifyClient) {
-        return;
+    notif_cbf_t notifClient;
+    void*       notifUser;
+    {
+        android::Mutex::Autolock autoLock(mNotifyClientLock);
+        if (NULL == mNotifyClient) {
+            return;
+        } else {
+            notifClient = mNotifyClient;
+            notifUser   = mNotifyUser;
+        }
     }
 
     int32_t val1, val2;
     if (msg->findInt32(PLAYEREVENT_PREFETCHSTATUSCHANGE, &val1)) {
         SL_LOGV("GenericPlayer notifying %s = %d", PLAYEREVENT_PREFETCHSTATUSCHANGE, val1);
-        mNotifyClient(kEventPrefetchStatusChange, val1, 0, mNotifyUser);
+        notifClient(kEventPrefetchStatusChange, val1, 0, notifUser);
     } else if (msg->findInt32(PLAYEREVENT_PREFETCHFILLLEVELUPDATE, &val1)) {
         SL_LOGV("GenericPlayer notifying %s = %d", PLAYEREVENT_PREFETCHFILLLEVELUPDATE, val1);
-        mNotifyClient(kEventPrefetchFillLevelUpdate, val1, 0, mNotifyUser);
+        notifClient(kEventPrefetchFillLevelUpdate, val1, 0, notifUser);
     } else if (msg->findInt32(PLAYEREVENT_ENDOFSTREAM, &val1)) {
         SL_LOGV("GenericPlayer notifying %s = %d", PLAYEREVENT_ENDOFSTREAM, val1);
-        mNotifyClient(kEventEndOfStream, val1, 0, mNotifyUser);
+        notifClient(kEventEndOfStream, val1, 0, notifUser);
     } else if (msg->findInt32(PLAYEREVENT_PREPARED, &val1)) {
         SL_LOGV("GenericPlayer notifying %s = %d", PLAYEREVENT_PREPARED, val1);
-        mNotifyClient(kEventPrepared, val1, 0, mNotifyUser);
+        notifClient(kEventPrepared, val1, 0, notifUser);
     } else if (msg->findRect(PLAYEREVENT_VIDEO_SIZE_UPDATE, &val1, &val2, &val1, &val2)) {
         SL_LOGV("GenericPlayer notifying %s = %d, %d", PLAYEREVENT_VIDEO_SIZE_UPDATE, val1, val2);
-        mNotifyClient(kEventHasVideoSize, val1, val2, mNotifyUser);
+        notifClient(kEventHasVideoSize, val1, val2, notifUser);
     }
 }
 

diff --git a/wilhelm/src/android/android_GenericPlayer.h b/wilhelm/src/android/android_GenericPlayer.h
index 5518a3c..b73de6f 100644 (file)
--- a/wilhelm/src/android/android_GenericPlayer.h
+++ b/wilhelm/src/android/android_GenericPlayer.h
@@ -49,6 +49,7 @@ public:
     virtual ~GenericPlayer();
 
     virtual void init(const notif_cbf_t cbf, void* notifUser);
+    virtual void preDestroy();
 
     virtual void setDataSource(const char *uri);
     virtual void setDataSource(int fd, int64_t offset, int64_t length);
@@ -120,6 +121,8 @@ protected:
     // Event notification from GenericPlayer to OpenSL ES / OpenMAX AL framework
     notif_cbf_t mNotifyClient;
     void*       mNotifyUser;
+    // lock to protect mNotifyClient and mNotifyUser updates
+    Mutex       mNotifyClientLock;
 
     enum {
         kFlagPrepared  = 1 <<0,
@@ -148,7 +151,6 @@ protected:
     int16_t mLastNotifiedCacheFill; // last cache fill level communicated to the listener
     int16_t mCacheFillNotifThreshold; // threshold in cache fill level for cache fill to be reported
 
-
 private:
     DISALLOW_EVIL_CONSTRUCTORS(GenericPlayer);
 };