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

4 files changed, 15 insertions, 4 deletions

diff --git a/wilhelm/src/android/android_AudioSfDecoder.cpp b/wilhelm/src/android/android_AudioSfDecoder.cpp
index b11c2a7a..a9fff4a5 100644
--- 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 71b93575..8b111fd2 100644
--- 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 bfc9db23..ef469ae0 100644
--- 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 16ee14e9..76ae919f 100644
--- 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;