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

4 files changed, 108 insertions, 55 deletions

diff --git a/include/media/AudioTrack.h b/include/media/AudioTrack.h
index da6d4e3..0f9df4f 100644
--- a/include/media/AudioTrack.h
+++ b/include/media/AudioTrack.h
@@ -732,13 +732,16 @@ protected:
     bool                    mRefreshRemaining;      // processAudioBuffer() should refresh
                                                     // mRemainingFrames and mRetryOnPartialBuffer
 
+                                                    // used for static track cbf and restoration
+    int32_t                 mLoopCount;             // last setLoop loopCount; zero means disabled
+    uint32_t                mLoopStart;             // last setLoop loopStart
+    uint32_t                mLoopEnd;               // last setLoop loopEnd
+
     // These are private to processAudioBuffer(), and are not protected by a lock
     uint32_t                mRemainingFrames;       // number of frames to request in obtainBuffer()
     bool                    mRetryOnPartialBuffer;  // sleep and retry after partial obtainBuffer()
     uint32_t                mObservedSequence;      // last observed value of mSequence
 
-    uint32_t                mLoopPeriod;            // in frames, zero means looping is disabled
-
     uint32_t                mMarkerPosition;        // in wrapping (overflow) frame units
     bool                    mMarkerReached;
     uint32_t                mNewPosition;           // in frames
diff --git a/include/private/media/AudioTrackShared.h b/include/private/media/AudioTrackShared.h
index 8389773..0d87cd2 100644
--- a/include/private/media/AudioTrackShared.h
+++ b/include/private/media/AudioTrackShared.h
@@ -85,13 +85,32 @@ struct StaticAudioTrackState {
 
 typedef SingleStateQueue<StaticAudioTrackState> StaticAudioTrackSingleStateQueue;
 
+struct StaticAudioTrackPosLoop {
+    // Do not define constructors, destructors, or virtual methods as this is part of a
+    // union in shared memory and will not get called properly.
+
+    // These fields should both be size_t, but since they are located in shared memory we
+    // force to 32-bit.  The client and server may have different typedefs for size_t.
+
+    // This struct information is stored in a single state queue to communicate the
+    // static AudioTrack server state to the client while data is consumed.
+    // It is smaller than StaticAudioTrackState to prevent unnecessary information from
+    // being sent.
+
+    uint32_t mBufferPosition;
+    int32_t  mLoopCount;
+};
+
+typedef SingleStateQueue<StaticAudioTrackPosLoop> StaticAudioTrackPosLoopQueue;
+
 struct AudioTrackSharedStatic {
+    // client requests to the server for loop or position changes.
     StaticAudioTrackSingleStateQueue::Shared
                     mSingleStateQueue;
-    // This field should be a size_t, but since it is located in shared memory we
-    // force to 32-bit.  The client and server may have different typedefs for size_t.
-    uint32_t        mBufferPosition;    // updated asynchronously by server,
-                                        // "for entertainment purposes only"
+    // position info updated asynchronously by server and read by client,
+    // "for entertainment purposes only"
+    StaticAudioTrackPosLoopQueue::Shared
+                    mPosLoopQueue;
 };
 
 // ----------------------------------------------------------------------------
@@ -355,6 +374,9 @@ public:
             void    setBufferPositionAndLoop(size_t position, size_t loopStart, size_t loopEnd,
                                              int loopCount);
             size_t  getBufferPosition();
+                    // getBufferPositionAndLoopCount() provides the proper snapshot of
+                    // position and loopCount together.
+            void    getBufferPositionAndLoopCount(size_t *position, int *loopCount);
 
     virtual size_t  getMisalignment() {
         return 0;
@@ -366,8 +388,9 @@ public:
 
 private:
     StaticAudioTrackSingleStateQueue::Mutator   mMutator;
+    StaticAudioTrackPosLoopQueue::Observer      mPosLoopObserver;
                         StaticAudioTrackState   mState;   // last communicated state to server
-    size_t          mBufferPosition;    // so that getBufferPosition() appears to be synchronous
+                        StaticAudioTrackPosLoop mPosLoop; // snapshot of position and loop.
 };
 
 // ----------------------------------------------------------------------------
@@ -494,8 +517,7 @@ private:
                                                 const StaticAudioTrackState &update) const;
     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
-
+    StaticAudioTrackPosLoopQueue::Mutator       mPosLoopMutator;
     size_t              mFramesReadySafe; // Assuming size_t read/writes are atomic on 32 / 64 bit
                                           // processors, this is a thread-safe version of
                                           // mFramesReady.
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