Fix race in getting duration

mDuration is protected by mSettingsLock because it is accessed from both
the ALooper thread and from the application thread, but only one of the
two "set"s was using the lock, and the "get" was not using the lock.

Also added some comments about the lock, and moved lock closer inside { }.

Change-Id: I7c96186f31baaad1b941d934549cb50d4f82d0c8

diff --git a/wilhelm/src/android/android_AudioSfDecoder.cpp b/wilhelm/src/android/android_AudioSfDecoder.cpp
index b11c2a7..a9fff4a 100644 (file)
--- a/wilhelm/src/android/android_AudioSfDecoder.cpp
+++ b/wilhelm/src/android/android_AudioSfDecoder.cpp
@@ -262,8 +262,10 @@ void AudioSfDecoder::onPrepare() {
     int32_t sr;
     bool hasSampleRate = meta->findInt32(kKeySampleRate, &sr);
 
+    // first compute the duration
     off64_t size;
     int64_t durationUs;
+    int32_t durationMsec;
     if (dataSource->getSize(&size) == OK
             && meta->findInt64(kKeyDuration, &durationUs)) {
         if (durationUs != 0) {
@@ -272,11 +274,17 @@ void AudioSfDecoder::onPrepare() {
             mBitrate = -1;
         }
         mDurationUsec = durationUs;
-        mDurationMsec = durationUs / 1000;
+        durationMsec = durationUs / 1000;
     } else {
         mBitrate = -1;
         mDurationUsec = ANDROID_UNKNOWN_TIME;
-        mDurationMsec = ANDROID_UNKNOWN_TIME;
+        durationMsec = ANDROID_UNKNOWN_TIME;
+    }
+
+    // then assign the duration under the settings lock
+    {
+        Mutex::Autolock _l(mSettingsLock);
+        mDurationMsec = durationMsec;
     }
 
     // the audio content is not raw PCM, so we need a decoder

diff --git a/wilhelm/src/android/android_GenericMediaPlayer.cpp b/wilhelm/src/android/android_GenericMediaPlayer.cpp
index 71b9357..8b111fd 100644 (file)
--- a/wilhelm/src/android/android_GenericMediaPlayer.cpp
+++ b/wilhelm/src/android/android_GenericMediaPlayer.cpp
@@ -495,9 +495,9 @@ void GenericMediaPlayer::afterMediaPlayerPreparedSuccessfully() {
     delete reply;
     // retrieve duration
     {
-        Mutex::Autolock _l(mSettingsLock);
         int msec = 0;
         if (OK == mPlayer->getDuration(&msec)) {
+            Mutex::Autolock _l(mSettingsLock);
             mDurationMsec = msec;
         }
     }

diff --git a/wilhelm/src/android/android_GenericPlayer.cpp b/wilhelm/src/android/android_GenericPlayer.cpp
index bfc9db2..ef469ae 100644 (file)
--- a/wilhelm/src/android/android_GenericPlayer.cpp
+++ b/wilhelm/src/android/android_GenericPlayer.cpp
@@ -192,6 +192,7 @@ void GenericPlayer::setBufferingUpdateThreshold(int16_t thresholdPercent) {
 
 //--------------------------------------------------
 void GenericPlayer::getDurationMsec(int* msec) {
+    Mutex::Autolock _l(mSettingsLock);
     *msec = mDurationMsec;
 }
 

diff --git a/wilhelm/src/android/android_GenericPlayer.h b/wilhelm/src/android/android_GenericPlayer.h
index 16ee14e..76ae919 100644 (file)
--- a/wilhelm/src/android/android_GenericPlayer.h
+++ b/wilhelm/src/android/android_GenericPlayer.h
@@ -87,7 +87,7 @@ public:
     void setPlayEvents(int32_t eventFlags, int32_t markerPosition, int32_t positionUpdatePeriod);
 
 protected:
-    // mutex used for set vs use of volume and cache (fill, threshold) settings
+    // mutex used for set vs use of volume, duration, and cache (fill, threshold) settings
     Mutex mSettingsLock;
 
     void resetDataLocator();
@@ -175,6 +175,8 @@ protected:
     AudioPlayback_Parameters mPlaybackParams;
 
     AndroidAudioLevels mAndroidAudioLevels;
+
+    // protected by mSettingsLock
     int32_t mDurationMsec;
 
     CacheStatus_t mCacheStatus;