Fix missing loop count for static tracks.

StaticAudioTrackServerProxy::framesReady() previously returned
only the contiguous frames, update to return the total
available frames. This resolves short-count looping in
SoundPool for FastTracks.

Also (1) Removes the racy condition of reading two variables
and (2) Fixes buffer->mNonContig to return the correct value
and (3) Restores behavior that loop count of 1 goes back to
loopStart once during playback.

Bug: 11830751
Bug: 12070295
Bug: 17456842
Change-Id: I64906e6036bb00a1d7375b03efe6deb69d6478ca

2 files changed, 46 insertions, 23 deletions

diff --git a/include/private/media/AudioTrackShared.h b/include/private/media/AudioTrackShared.h
index fa1b20a..31dff36 100644
--- a/include/private/media/AudioTrackShared.h
+++ b/include/private/media/AudioTrackShared.h
@@ -450,7 +450,14 @@ private:
     ssize_t             pollPosition(); // poll for state queue update, and return current position
     StaticAudioTrackSingleStateQueue::Observer  mObserver;
     size_t              mPosition;  // server's current play position in frames, relative to 0
-    size_t              mEnd;       // cached value computed from mState, safe for asynchronous read
+
+    size_t              mFramesReadySafe; // Assuming size_t read/writes are atomic on 32 / 64 bit
+                                          // processors, this is a thread-safe version of
+                                          // mFramesReady.
+    int64_t             mFramesReady;     // The number of frames ready in the static buffer
+                                          // including loops.  This is 64 bits since loop mode
+                                          // can cause a track to appear to have a large number
+                                          // of frames. INT64_MAX means an infinite loop.
     bool                mFramesReadyIsCalledByMultipleThreads;
     StaticAudioTrackState   mState;
 };
diff --git a/media/libmedia/AudioTrackShared.cpp b/media/libmedia/AudioTrackShared.cpp
index 561cb24..62362da 100644
--- a/media/libmedia/AudioTrackShared.cpp
+++ b/media/libmedia/AudioTrackShared.cpp
@@ -25,6 +25,12 @@
 
 namespace android {
 
+// used to clamp a value to size_t.  TODO: move to another file.
+template <typename T>
+size_t clampToSize(T x) {
+    return x > SIZE_MAX ? SIZE_MAX : x < 0 ? 0 : (size_t) x;
+}
+
 audio_track_cblk_t::audio_track_cblk_t()
     : mServer(0), mFutex(0), mMinimum(0),
     mVolumeLR(GAIN_MINIFLOAT_PACKED_UNITY), mSampleRate(0), mSendLevel(0), mFlags(0)
@@ -728,7 +734,8 @@ StaticAudioTrackServerProxy::StaticAudioTrackServerProxy(audio_track_cblk_t* cbl
         size_t frameCount, size_t frameSize)
     : AudioTrackServerProxy(cblk, buffers, frameCount, frameSize),
       mObserver(&cblk->u.mStatic.mSingleStateQueue), mPosition(0),
-      mEnd(frameCount), mFramesReadyIsCalledByMultipleThreads(false)
+      mFramesReadySafe(frameCount), mFramesReady(frameCount),
+      mFramesReadyIsCalledByMultipleThreads(false)
 {
     mState.mLoopStart = 0;
     mState.mLoopEnd = 0;
@@ -742,20 +749,11 @@ void StaticAudioTrackServerProxy::framesReadyIsCalledByMultipleThreads()
 
 size_t StaticAudioTrackServerProxy::framesReady()
 {
-    // FIXME
-    // This is racy if called by normal mixer thread,
-    // as we're reading 2 independent variables without a lock.
-    // Can't call mObserver.poll(), as we might be called from wrong thread.
-    // If looping is enabled, should return a higher number (since includes non-contiguous).
-    size_t position = mPosition;
+    // Can't call pollPosition() from multiple threads.
     if (!mFramesReadyIsCalledByMultipleThreads) {
-        ssize_t positionOrStatus = pollPosition();
-        if (positionOrStatus >= 0) {
-            position = (size_t) positionOrStatus;
-        }
+        (void) pollPosition();
     }
-    size_t end = mEnd;
-    return position < end ? end - position : 0;
+    return mFramesReadySafe;
 }
 
 ssize_t StaticAudioTrackServerProxy::pollPosition()
@@ -772,25 +770,35 @@ ssize_t StaticAudioTrackServerProxy::pollPosition()
             }
             // ignore loopEnd
             mPosition = position = loopStart;
-            mEnd = mFrameCount;
+            mFramesReady = mFrameCount - mPosition;
             mState.mLoopCount = 0;
             valid = true;
-        } else {
+        } else if (state.mLoopCount >= -1) {
             if (loopStart < loopEnd && loopEnd <= mFrameCount &&
                     loopEnd - loopStart >= MIN_LOOP) {
                 if (!(loopStart <= position && position < loopEnd)) {
                     mPosition = position = loopStart;
                 }
-                mEnd = loopEnd;
+                if (state.mLoopCount == -1) {
+                    mFramesReady = INT64_MAX;
+                } else {
+                    // mFramesReady is 64 bits to handle the effective number of frames
+                    // that the static audio track contains, including loops.
+                    // TODO: Later consider fixing overflow, but does not seem needed now
+                    // as will not overflow if loopStart and loopEnd are Java "ints".
+                    mFramesReady = int64_t(state.mLoopCount) * (loopEnd - loopStart)
+                            + mFrameCount - mPosition;
+                }
                 mState = state;
                 valid = true;
             }
         }
-        if (!valid) {
+        if (!valid || mPosition > mFrameCount) {
             ALOGE("%s client pushed an invalid state, shutting down", __func__);
             mIsShutdown = true;
             return (ssize_t) NO_INIT;
         }
+        mFramesReadySafe = clampToSize(mFramesReady);
         // This may overflow, but client is not supposed to rely on it
         mCblk->u.mStatic.mBufferPosition = (uint32_t) position;
     }
@@ -815,9 +823,10 @@ status_t StaticAudioTrackServerProxy::obtainBuffer(Buffer* buffer, bool ackFlush
         return (status_t) positionOrStatus;
     }
     size_t position = (size_t) positionOrStatus;
+    size_t end = mState.mLoopCount != 0 ? mState.mLoopEnd : mFrameCount;
     size_t avail;
-    if (position < mEnd) {
-        avail = mEnd - position;
+    if (position < end) {
+        avail = end - position;
         size_t wanted = buffer->mFrameCount;
         if (avail < wanted) {
             buffer->mFrameCount = avail;
@@ -830,7 +839,10 @@ status_t StaticAudioTrackServerProxy::obtainBuffer(Buffer* buffer, bool ackFlush
         buffer->mFrameCount = 0;
         buffer->mRaw = NULL;
     }
-    buffer->mNonContig = 0;     // FIXME should be > 0 for looping
+    // As mFramesReady is the total remaining frames in the static audio track,
+    // it is always larger or equal to avail.
+    LOG_ALWAYS_FATAL_IF(mFramesReady < avail);
+    buffer->mNonContig = mFramesReady == INT64_MAX ? SIZE_MAX : clampToSize(mFramesReady - avail);
     mUnreleased = avail;
     return NO_ERROR;
 }
@@ -838,6 +850,7 @@ status_t StaticAudioTrackServerProxy::obtainBuffer(Buffer* buffer, bool ackFlush
 void StaticAudioTrackServerProxy::releaseBuffer(Buffer* buffer)
 {
     size_t stepCount = buffer->mFrameCount;
+    LOG_ALWAYS_FATAL_IF(!(stepCount <= mFramesReady));
     LOG_ALWAYS_FATAL_IF(!(stepCount <= mUnreleased));
     if (stepCount == 0) {
         // prevent accidental re-use of buffer
@@ -854,11 +867,10 @@ void StaticAudioTrackServerProxy::releaseBuffer(Buffer* buffer)
         ALOGW("%s newPosition %zu outside [%zu, %zu]", __func__, newPosition, position, mFrameCount);
         newPosition = mFrameCount;
     } else if (mState.mLoopCount != 0 && newPosition == mState.mLoopEnd) {
+        newPosition = mState.mLoopStart;
         if (mState.mLoopCount == -1 || --mState.mLoopCount != 0) {
-            newPosition = mState.mLoopStart;
             setFlags = CBLK_LOOP_CYCLE;
         } else {
-            mEnd = mFrameCount;     // this is what allows playback to continue after the loop
             setFlags = CBLK_LOOP_FINAL;
         }
     }
@@ -866,6 +878,10 @@ void StaticAudioTrackServerProxy::releaseBuffer(Buffer* buffer)
         setFlags |= CBLK_BUFFER_END;
     }
     mPosition = newPosition;
+    if (mFramesReady != INT64_MAX) {
+        mFramesReady -= stepCount;
+    }
+    mFramesReadySafe = clampToSize(mFramesReady);
 
     cblk->mServer += stepCount;
     // This may overflow, but client is not supposed to rely on it