Fix loop and position restoration in static AudioTracks

Allow restoration of loop and position.
Make position and loop synchronously readable.

Bug: 17964637
Change-Id: I8cfb5036e665f55fdff5c67d27e1363ce9a8665d

2 files changed, 74 insertions, 46 deletions

diff --git a/media/libmedia/AudioTrack.cpp b/media/libmedia/AudioTrack.cpp
index fdc31a1..422ead1 100644
--- a/media/libmedia/AudioTrack.cpp
+++ b/media/libmedia/AudioTrack.cpp
@@ -38,6 +38,11 @@
 namespace android {
 // ---------------------------------------------------------------------------
 
+template <typename T>
+const T &min(const T &x, const T &y) {
+    return x < y ? x : y;
+}
+
 static int64_t convertTimespecToUs(const struct timespec &tv)
 {
     return tv.tv_sec * 1000000ll + tv.tv_nsec / 1000;
@@ -420,7 +425,9 @@ status_t AudioTrack::set(
     mStatus = NO_ERROR;
     mState = STATE_STOPPED;
     mUserData = user;
-    mLoopPeriod = 0;
+    mLoopCount = 0;
+    mLoopStart = 0;
+    mLoopEnd = 0;
     mMarkerPosition = 0;
     mMarkerReached = false;
     mNewPosition = 0;
@@ -534,7 +541,6 @@ void AudioTrack::stop()
 
     if (mSharedBuffer != 0) {
         // clear buffer position and loop count.
-        mLoopPeriod = 0;
         mStaticProxy->setBufferPositionAndLoop(0 /* position */,
                 0 /* loopStart */, 0 /* loopEnd */, 0 /* loopCount */);
     }
@@ -739,9 +745,11 @@ status_t AudioTrack::setLoop(uint32_t loopStart, uint32_t loopEnd, int loopCount
 
 void AudioTrack::setLoop_l(uint32_t loopStart, uint32_t loopEnd, int loopCount)
 {
-    // Setting the loop will reset next notification update period (like setPosition).
-    mNewPosition = updateAndGetPosition_l() + mUpdatePeriod;
-    mLoopPeriod = loopCount != 0 ? loopEnd - loopStart : 0;
+    // We do not update the periodic notification point.
+    // mNewPosition = updateAndGetPosition_l() + mUpdatePeriod;
+    mLoopCount = loopCount;
+    mLoopEnd = loopEnd;
+    mLoopStart = loopStart;
     mStaticProxy->setLoop(loopStart, loopEnd, loopCount);
 }
 
@@ -889,7 +897,6 @@ status_t AudioTrack::reload()
         return INVALID_OPERATION;
     }
     mNewPosition = mUpdatePeriod;
-    mLoopPeriod = 0;
     (void) updateAndGetPosition_l();
     mPosition = 0;
     // The documentation is not clear on the behavior of reload() and the restoration
@@ -1610,7 +1617,7 @@ nsecs_t AudioTrack::processAudioBuffer()
     }
 
     // Cache other fields that will be needed soon
-    uint32_t loopPeriod = mLoopPeriod;
+    uint32_t loopPeriod = mLoopCount != 0 ? mLoopEnd - mLoopStart : 0;
     uint32_t sampleRate = mSampleRate;
     uint32_t notificationFrames = mNotificationFramesAct;
     if (mRefreshRemaining) {
@@ -1856,7 +1863,11 @@ status_t AudioTrack::restoreTrack_l(const char *from)
     }
 
     // save the old static buffer position
-    size_t bufferPosition = mStaticProxy != NULL ? mStaticProxy->getBufferPosition() : 0;
+    size_t bufferPosition = 0;
+    int loopCount = 0;
+    if (mStaticProxy != 0) {
+        mStaticProxy->getBufferPositionAndLoopCount(&bufferPosition, &loopCount);
+    }
 
     // If a new IAudioTrack is successfully created, createTrack_l() will modify the
     // following member variables: mAudioTrack, mCblkMemory and mCblk.
@@ -1873,27 +1884,15 @@ status_t AudioTrack::restoreTrack_l(const char *from)
     }
 
     if (result == NO_ERROR) {
-        // continue playback from last known position, but
-        // don't attempt to restore loop after invalidation; it's difficult and not worthwhile
-        if (mStaticProxy != NULL) {
-            mLoopPeriod = 0;
-            mStaticProxy->setBufferPositionAndLoop(bufferPosition,
-                    0 /* loopStart */, 0 /* loopEnd */, 0 /* loopCount */);
-        }
-        // FIXME How do we simulate the fact that all frames present in the buffer at the time of
-        //       track destruction have been played? This is critical for SoundPool implementation
-        //       This must be broken, and needs to be tested/debugged.
-#if 0
-        // restore write index and set other indexes to reflect empty buffer status
-        if (!strcmp(from, "start")) {
-            // Make sure that a client relying on callback events indicating underrun or
-            // the actual amount of audio frames played (e.g SoundPool) receives them.
-            if (mSharedBuffer == 0) {
-                // restart playback even if buffer is not completely filled.
-                android_atomic_or(CBLK_FORCEREADY, &mCblk->mFlags);
+        // Continue playback from last known position and restore loop.
+        if (mStaticProxy != 0) {
+            if (loopCount != 0) {
+                mStaticProxy->setBufferPositionAndLoop(bufferPosition,
+                        mLoopStart, mLoopEnd, loopCount);
+            } else {
+                mStaticProxy->setBufferPosition(bufferPosition);
             }
         }
-#endif
         if (mState == STATE_ACTIVE) {
             result = mAudioTrack->start();
         }
diff --git a/media/libmedia/AudioTrackShared.cpp b/media/libmedia/AudioTrackShared.cpp
index c44cdee..08241e2 100644
--- a/media/libmedia/AudioTrackShared.cpp
+++ b/media/libmedia/AudioTrackShared.cpp
@@ -499,9 +499,11 @@ end:
 StaticAudioTrackClientProxy::StaticAudioTrackClientProxy(audio_track_cblk_t* cblk, void *buffers,
         size_t frameCount, size_t frameSize)
     : AudioTrackClientProxy(cblk, buffers, frameCount, frameSize),
-      mMutator(&cblk->u.mStatic.mSingleStateQueue), mBufferPosition(0)
+      mMutator(&cblk->u.mStatic.mSingleStateQueue),
+      mPosLoopObserver(&cblk->u.mStatic.mPosLoopQueue)
 {
     memset(&mState, 0, sizeof(mState));
+    memset(&mPosLoop, 0, sizeof(mPosLoop));
 }
 
 void StaticAudioTrackClientProxy::flush()
@@ -523,11 +525,12 @@ void StaticAudioTrackClientProxy::setLoop(size_t loopStart, size_t loopEnd, int
     // set patch-up variables until the mState is acknowledged by the ServerProxy.
     // observed buffer position and loop count will freeze until then to give the
     // illusion of a synchronous change.
-    size_t bufferPosition = getBufferPosition();
+    getBufferPositionAndLoopCount(NULL, NULL);
     // preserve behavior to restart at mState.mLoopStart if position exceeds mState.mLoopEnd.
-    if (loopCount != 0 && bufferPosition >= mState.mLoopEnd) {
-        mBufferPosition = mState.mLoopStart;
+    if (mState.mLoopCount != 0 && mPosLoop.mBufferPosition >= mState.mLoopEnd) {
+        mPosLoop.mBufferPosition = mState.mLoopStart;
     }
+    mPosLoop.mLoopCount = mState.mLoopCount;
     (void) mMutator.push(mState);
 }
 
@@ -540,7 +543,17 @@ void StaticAudioTrackClientProxy::setBufferPosition(size_t position)
     }
     mState.mPosition = (uint32_t) position;
     mState.mPositionSequence = incrementSequence(mState.mPositionSequence, mState.mLoopSequence);
-    mBufferPosition = position;
+    // set patch-up variables until the mState is acknowledged by the ServerProxy.
+    // observed buffer position and loop count will freeze until then to give the
+    // illusion of a synchronous change.
+    if (mState.mLoopCount > 0) {  // only check if loop count is changing
+        getBufferPositionAndLoopCount(NULL, NULL); // get last position
+    }
+    mPosLoop.mBufferPosition = position;
+    if (position >= mState.mLoopEnd) {
+        // no ongoing loop is possible if position is greater than loopEnd.
+        mPosLoop.mLoopCount = 0;
+    }
     (void) mMutator.push(mState);
 }
 
@@ -553,18 +566,24 @@ void StaticAudioTrackClientProxy::setBufferPositionAndLoop(size_t position, size
 
 size_t StaticAudioTrackClientProxy::getBufferPosition()
 {
-    size_t bufferPosition;
-    if (mMutator.ack()) {
-        // There is a race condition here as ack may be signaled before
-        // the buffer position in mCblk is updated.  Will be fixed in a later CL.
-        bufferPosition = (size_t) mCblk->u.mStatic.mBufferPosition;
-        if (bufferPosition > mFrameCount) {
-            bufferPosition = mFrameCount;
-        }
-    } else {
-        bufferPosition = mBufferPosition;
+    getBufferPositionAndLoopCount(NULL, NULL);
+    return mPosLoop.mBufferPosition;
+}
+
+void StaticAudioTrackClientProxy::getBufferPositionAndLoopCount(
+        size_t *position, int *loopCount)
+{
+    if (mMutator.ack() == StaticAudioTrackSingleStateQueue::SSQ_DONE) {
+         if (mPosLoopObserver.poll(mPosLoop)) {
+             ; // a valid mPosLoop should be available if ackDone is true.
+         }
+    }
+    if (position != NULL) {
+        *position = mPosLoop.mBufferPosition;
+    }
+    if (loopCount != NULL) {
+        *loopCount = mPosLoop.mLoopCount;
     }
-    return bufferPosition;
 }
 
 // ---------------------------------------------------------------------------
@@ -780,7 +799,8 @@ void AudioTrackServerProxy::tallyUnderrunFrames(uint32_t frameCount)
 StaticAudioTrackServerProxy::StaticAudioTrackServerProxy(audio_track_cblk_t* cblk, void *buffers,
         size_t frameCount, size_t frameSize)
     : AudioTrackServerProxy(cblk, buffers, frameCount, frameSize),
-      mObserver(&cblk->u.mStatic.mSingleStateQueue), mPosition(0),
+      mObserver(&cblk->u.mStatic.mSingleStateQueue),
+      mPosLoopMutator(&cblk->u.mStatic.mPosLoopQueue),
       mFramesReadySafe(frameCount), mFramesReady(frameCount),
       mFramesReadyIsCalledByMultipleThreads(false)
 {
@@ -865,6 +885,7 @@ ssize_t StaticAudioTrackServerProxy::pollPosition()
                     updateStateWithLoop(&trystate, state) == OK;
         }
         if (!result) {
+            mObserver.done();
             // caution: no update occurs so server state will be inconsistent with client state.
             ALOGE("%s client pushed an invalid state, shutting down", __func__);
             mIsShutdown = true;
@@ -883,7 +904,12 @@ ssize_t StaticAudioTrackServerProxy::pollPosition()
         }
         mFramesReadySafe = clampToSize(mFramesReady);
         // This may overflow, but client is not supposed to rely on it
-        mCblk->u.mStatic.mBufferPosition = (uint32_t) mState.mPosition;
+        StaticAudioTrackPosLoop posLoop;
+
+        posLoop.mLoopCount = (int32_t) mState.mLoopCount;
+        posLoop.mBufferPosition = (uint32_t) mState.mPosition;
+        mPosLoopMutator.push(posLoop);
+        mObserver.done(); // safe to read mStatic variables.
     }
     return (ssize_t) mState.mPosition;
 }
@@ -969,7 +995,10 @@ void StaticAudioTrackServerProxy::releaseBuffer(Buffer* buffer)
 
     cblk->mServer += stepCount;
     // This may overflow, but client is not supposed to rely on it
-    cblk->u.mStatic.mBufferPosition = (uint32_t) mState.mPosition;
+    StaticAudioTrackPosLoop posLoop;
+    posLoop.mBufferPosition = mState.mPosition;
+    posLoop.mLoopCount = mState.mLoopCount;
+    mPosLoopMutator.push(posLoop);
     if (setFlags != 0) {
         (void) android_atomic_or(setFlags, &cblk->mFlags);
         // this would be a good place to wake a futex