Fix loop and position setting in static AudioTracks

Allow independent setting of position and loop.

Bug: 17964637
Change-Id: I8b3bd97a244b932728b68da7684044f2636984a5

4 files changed, 187 insertions, 102 deletions

diff --git a/include/private/media/AudioTrackShared.h b/include/private/media/AudioTrackShared.h
index 97448c1..8389773 100644
--- a/include/private/media/AudioTrackShared.h
+++ b/include/private/media/AudioTrackShared.h
@@ -26,7 +26,6 @@
 #include <utils/RefBase.h>
 #include <media/nbaio/roundup.h>
 #include <media/SingleStateQueue.h>
-#include <private/media/StaticAudioTrackState.h>
 
 namespace android {
 
@@ -61,6 +60,29 @@ struct AudioTrackSharedStreaming {
     volatile uint32_t mUnderrunFrames;  // server increments for each unavailable but desired frame
 };
 
+// Represents a single state of an AudioTrack that was created in static mode (shared memory buffer
+// supplied by the client).  This state needs to be communicated from the client to server.  As this
+// state is too large to be updated atomically without a mutex, and mutexes aren't allowed here, the
+// state is wrapped by a SingleStateQueue.
+struct StaticAudioTrackState {
+    // Do not define constructors, destructors, or virtual methods as this is part of a
+    // union in shared memory and they 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.
+
+    // The state has a sequence counter to indicate whether changes are made to loop or position.
+    // The sequence counter also currently indicates whether loop or position is first depending
+    // on which is greater; it jumps by max(mLoopSequence, mPositionSequence) + 1.
+
+    uint32_t    mLoopStart;
+    uint32_t    mLoopEnd;
+    int32_t     mLoopCount;
+    uint32_t    mLoopSequence; // a sequence counter to indicate changes to loop
+    uint32_t    mPosition;
+    uint32_t    mPositionSequence; // a sequence counter to indicate changes to position
+};
+
 typedef SingleStateQueue<StaticAudioTrackState> StaticAudioTrackSingleStateQueue;
 
 struct AudioTrackSharedStatic {
@@ -314,7 +336,24 @@ public:
     virtual void    flush();
 
 #define MIN_LOOP    16  // minimum length of each loop iteration in frames
+
+            // setLoop(), setBufferPosition(), and setBufferPositionAndLoop() set the
+            // static buffer position and looping parameters.  These commands are not
+            // synchronous (they do not wait or block); instead they take effect at the
+            // next buffer data read from the server side. However, the client side
+            // getters will read a cached version of the position and loop variables
+            // until the setting takes effect.
+            //
+            // setBufferPositionAndLoop() is equivalent to calling, in order, setLoop() and
+            // setBufferPosition().
+            //
+            // The functions should not be relied upon to do parameter or state checking.
+            // That is done at the AudioTrack level.
+
             void    setLoop(size_t loopStart, size_t loopEnd, int loopCount);
+            void    setBufferPosition(size_t position);
+            void    setBufferPositionAndLoop(size_t position, size_t loopStart, size_t loopEnd,
+                                             int loopCount);
             size_t  getBufferPosition();
 
     virtual size_t  getMisalignment() {
@@ -327,6 +366,7 @@ public:
 
 private:
     StaticAudioTrackSingleStateQueue::Mutator   mMutator;
+                        StaticAudioTrackState   mState;   // last communicated state to server
     size_t          mBufferPosition;    // so that getBufferPosition() appears to be synchronous
 };
 
@@ -448,6 +488,10 @@ public:
     virtual uint32_t    getUnderrunFrames() const { return 0; }
 
 private:
+    status_t            updateStateWithLoop(StaticAudioTrackState *localState,
+                                            const StaticAudioTrackState &update) const;
+    status_t            updateStateWithPosition(StaticAudioTrackState *localState,
+                                                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
@@ -460,7 +504,8 @@ private:
                                           // can cause a track to appear to have a large number
                                           // of frames. INT64_MAX means an infinite loop.
     bool                mFramesReadyIsCalledByMultipleThreads;
-    StaticAudioTrackState   mState;
+    StaticAudioTrackState mState;         // Server side state. Any updates from client must be
+                                          // passed by the mObserver SingleStateQueue.
 };
 
 // Proxy used by AudioFlinger for servicing AudioRecord
diff --git a/include/private/media/StaticAudioTrackState.h b/include/private/media/StaticAudioTrackState.h
deleted file mode 100644
index d483061..0000000
--- a/include/private/media/StaticAudioTrackState.h
+++ /dev/null
@@ -1,39 +0,0 @@
-/*
- * Copyright (C) 2012 The Android Open Source Project
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- *      http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-#ifndef STATIC_AUDIO_TRACK_STATE_H
-#define STATIC_AUDIO_TRACK_STATE_H
-
-namespace android {
-
-// Represents a single state of an AudioTrack that was created in static mode (shared memory buffer
-// supplied by the client).  This state needs to be communicated from the client to server.  As this
-// state is too large to be updated atomically without a mutex, and mutexes aren't allowed here, the
-// state is wrapped by a SingleStateQueue.
-struct StaticAudioTrackState {
-    // do not define constructors, destructors, or virtual methods
-
-    // 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.
-    uint32_t    mLoopStart;
-    uint32_t    mLoopEnd;
-
-    int         mLoopCount;
-};
-
-}   // namespace android
-
-#endif  // STATIC_AUDIO_TRACK_STATE_H
diff --git a/media/libmedia/AudioTrack.cpp b/media/libmedia/AudioTrack.cpp
index 735db5c..fdc31a1 100644
--- a/media/libmedia/AudioTrack.cpp
+++ b/media/libmedia/AudioTrack.cpp
@@ -531,14 +531,13 @@ void AudioTrack::stop()
     // the playback head position will reset to 0, so if a marker is set, we need
     // to activate it again
     mMarkerReached = false;
-#if 0
-    // Force flush if a shared buffer is used otherwise audioflinger
-    // will not stop before end of buffer is reached.
-    // It may be needed to make sure that we stop playback, likely in case looping is on.
+
     if (mSharedBuffer != 0) {
-        flush_l();
+        // clear buffer position and loop count.
+        mLoopPeriod = 0;
+        mStaticProxy->setBufferPositionAndLoop(0 /* position */,
+                0 /* loopStart */, 0 /* loopEnd */, 0 /* loopCount */);
     }
-#endif
 
     sp<AudioTrackThread> t = mAudioTrackThread;
     if (t != 0) {
@@ -823,12 +822,9 @@ status_t AudioTrack::setPosition(uint32_t position)
     if (mState == STATE_ACTIVE) {
         return INVALID_OPERATION;
     }
+    // After setting the position, use full update period before notification.
     mNewPosition = updateAndGetPosition_l() + mUpdatePeriod;
-    mLoopPeriod = 0;
-    // FIXME Check whether loops and setting position are incompatible in old code.
-    // If we use setLoop for both purposes we lose the capability to set the position while looping.
-    mStaticProxy->setLoop(position, mFrameCount, 0);
-
+    mStaticProxy->setBufferPosition(position);
     return NO_ERROR;
 }
 
@@ -894,9 +890,13 @@ status_t AudioTrack::reload()
     }
     mNewPosition = mUpdatePeriod;
     mLoopPeriod = 0;
-    // FIXME The new code cannot reload while keeping a loop specified.
-    // Need to check how the old code handled this, and whether it's a significant change.
-    mStaticProxy->setLoop(0, mFrameCount, 0);
+    (void) updateAndGetPosition_l();
+    mPosition = 0;
+    // The documentation is not clear on the behavior of reload() and the restoration
+    // of loop count. Historically we have not restored loop count, start, end;
+    // rather we just reset the buffer position.  However, restoring the last setLoop
+    // information makes sense if one desires to repeat playing a particular sound.
+    mStaticProxy->setBufferPosition(0);
     return NO_ERROR;
 }
 
@@ -1865,15 +1865,20 @@ status_t AudioTrack::restoreTrack_l(const char *from)
     result = createTrack_l();
 
     // take the frames that will be lost by track recreation into account in saved position
+    // For streaming tracks, this is the amount we obtained from the user/client
+    // (not the number actually consumed at the server - those are already lost).
     (void) updateAndGetPosition_l();
-    mPosition = mReleased;
+    if (mStaticProxy != 0) {
+        mPosition = mReleased;
+    }
 
     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->setLoop(bufferPosition, mFrameCount, 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
diff --git a/media/libmedia/AudioTrackShared.cpp b/media/libmedia/AudioTrackShared.cpp
index ee02710..c44cdee 100644
--- a/media/libmedia/AudioTrackShared.cpp
+++ b/media/libmedia/AudioTrackShared.cpp
@@ -31,6 +31,20 @@ size_t clampToSize(T x) {
     return sizeof(T) > sizeof(size_t) && x > (T) SIZE_MAX ? SIZE_MAX : x < 0 ? 0 : (size_t) x;
 }
 
+// incrementSequence is used to determine the next sequence value
+// for the loop and position sequence counters.  It should return
+// a value between "other" + 1 and "other" + INT32_MAX, the choice of
+// which needs to be the "least recently used" sequence value for "self".
+// In general, this means (new_self) returned is max(self, other) + 1.
+
+static uint32_t incrementSequence(uint32_t self, uint32_t other) {
+    int32_t diff = self - other;
+    if (diff >= 0 && diff < INT32_MAX) {
+        return self + 1; // we're already ahead of other.
+    }
+    return other + 1; // we're behind, so move just ahead of other.
+}
+
 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)
@@ -487,6 +501,7 @@ StaticAudioTrackClientProxy::StaticAudioTrackClientProxy(audio_track_cblk_t* cbl
     : AudioTrackClientProxy(cblk, buffers, frameCount, frameSize),
       mMutator(&cblk->u.mStatic.mSingleStateQueue), mBufferPosition(0)
 {
+    memset(&mState, 0, sizeof(mState));
 }
 
 void StaticAudioTrackClientProxy::flush()
@@ -501,22 +516,47 @@ void StaticAudioTrackClientProxy::setLoop(size_t loopStart, size_t loopEnd, int
         // FIXME Should return an error status
         return;
     }
-    StaticAudioTrackState newState;
-    newState.mLoopStart = (uint32_t) loopStart;
-    newState.mLoopEnd = (uint32_t) loopEnd;
-    newState.mLoopCount = loopCount;
-    size_t bufferPosition;
-    if (loopCount == 0 || (bufferPosition = getBufferPosition()) >= loopEnd) {
-        bufferPosition = loopStart;
+    mState.mLoopStart = (uint32_t) loopStart;
+    mState.mLoopEnd = (uint32_t) loopEnd;
+    mState.mLoopCount = loopCount;
+    mState.mLoopSequence = incrementSequence(mState.mLoopSequence, mState.mPositionSequence);
+    // 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();
+    // preserve behavior to restart at mState.mLoopStart if position exceeds mState.mLoopEnd.
+    if (loopCount != 0 && bufferPosition >= mState.mLoopEnd) {
+        mBufferPosition = mState.mLoopStart;
+    }
+    (void) mMutator.push(mState);
+}
+
+void StaticAudioTrackClientProxy::setBufferPosition(size_t position)
+{
+    // This can only happen on a 64-bit client
+    if (position > UINT32_MAX) {
+        // FIXME Should return an error status
+        return;
     }
-    mBufferPosition = bufferPosition; // snapshot buffer position until loop is acknowledged.
-    (void) mMutator.push(newState);
+    mState.mPosition = (uint32_t) position;
+    mState.mPositionSequence = incrementSequence(mState.mPositionSequence, mState.mLoopSequence);
+    mBufferPosition = position;
+    (void) mMutator.push(mState);
+}
+
+void StaticAudioTrackClientProxy::setBufferPositionAndLoop(size_t position, size_t loopStart,
+        size_t loopEnd, int loopCount)
+{
+    setLoop(loopStart, loopEnd, loopCount);
+    setBufferPosition(position);
 }
 
 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;
@@ -744,9 +784,7 @@ StaticAudioTrackServerProxy::StaticAudioTrackServerProxy(audio_track_cblk_t* cbl
       mFramesReadySafe(frameCount), mFramesReady(frameCount),
       mFramesReadyIsCalledByMultipleThreads(false)
 {
-    mState.mLoopStart = 0;
-    mState.mLoopEnd = 0;
-    mState.mLoopCount = 0;
+    memset(&mState, 0, sizeof(mState));
 }
 
 void StaticAudioTrackServerProxy::framesReadyIsCalledByMultipleThreads()
@@ -763,55 +801,91 @@ size_t StaticAudioTrackServerProxy::framesReady()
     return mFramesReadySafe;
 }
 
-ssize_t StaticAudioTrackServerProxy::pollPosition()
+status_t StaticAudioTrackServerProxy::updateStateWithLoop(
+        StaticAudioTrackState *localState, const StaticAudioTrackState &update) const
 {
-    size_t position = mPosition;
-    StaticAudioTrackState state;
-    if (mObserver.poll(state)) {
+    if (localState->mLoopSequence != update.mLoopSequence) {
         bool valid = false;
-        size_t loopStart = state.mLoopStart;
-        size_t loopEnd = state.mLoopEnd;
-        if (state.mLoopCount == 0) {
-            if (loopStart > mFrameCount) {
-                loopStart = mFrameCount;
-            }
-            // ignore loopEnd
-            mPosition = position = loopStart;
-            mFramesReady = mFrameCount - mPosition;
-            mState.mLoopCount = 0;
+        const size_t loopStart = update.mLoopStart;
+        const size_t loopEnd = update.mLoopEnd;
+        size_t position = localState->mPosition;
+        if (update.mLoopCount == 0) {
             valid = true;
-        } else if (state.mLoopCount >= -1) {
+        } else if (update.mLoopCount >= -1) {
             if (loopStart < loopEnd && loopEnd <= mFrameCount &&
                     loopEnd - loopStart >= MIN_LOOP) {
                 // If the current position is greater than the end of the loop
                 // we "wrap" to the loop start. This might cause an audible pop.
                 if (position >= loopEnd) {
-                    mPosition = position = loopStart;
-                }
-                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;
+                    position = loopStart;
                 }
-                mState = state;
                 valid = true;
             }
         }
-        if (!valid || mPosition > mFrameCount) {
+        if (!valid || position > mFrameCount) {
+            return NO_INIT;
+        }
+        localState->mPosition = position;
+        localState->mLoopCount = update.mLoopCount;
+        localState->mLoopEnd = loopEnd;
+        localState->mLoopStart = loopStart;
+        localState->mLoopSequence = update.mLoopSequence;
+    }
+    return OK;
+}
+
+status_t StaticAudioTrackServerProxy::updateStateWithPosition(
+        StaticAudioTrackState *localState, const StaticAudioTrackState &update) const
+{
+    if (localState->mPositionSequence != update.mPositionSequence) {
+        if (update.mPosition > mFrameCount) {
+            return NO_INIT;
+        } else if (localState->mLoopCount != 0 && update.mPosition >= localState->mLoopEnd) {
+            localState->mLoopCount = 0; // disable loop count if position is beyond loop end.
+        }
+        localState->mPosition = update.mPosition;
+        localState->mPositionSequence = update.mPositionSequence;
+    }
+    return OK;
+}
+
+ssize_t StaticAudioTrackServerProxy::pollPosition()
+{
+    StaticAudioTrackState state;
+    if (mObserver.poll(state)) {
+        StaticAudioTrackState trystate = mState;
+        bool result;
+        const int32_t diffSeq = state.mLoopSequence - state.mPositionSequence;
+
+        if (diffSeq < 0) {
+            result = updateStateWithLoop(&trystate, state) == OK &&
+                    updateStateWithPosition(&trystate, state) == OK;
+        } else {
+            result = updateStateWithPosition(&trystate, state) == OK &&
+                    updateStateWithLoop(&trystate, state) == OK;
+        }
+        if (!result) {
+            // 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;
             return (ssize_t) NO_INIT;
         }
+        mState = trystate;
+        if (mState.mLoopCount == -1) {
+            mFramesReady = INT64_MAX;
+        } else if (mState.mLoopCount == 0) {
+            mFramesReady = mFrameCount - mState.mPosition;
+        } else if (mState.mLoopCount > 0) {
+            // 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(mState.mLoopCount) * (mState.mLoopEnd - mState.mLoopStart)
+                    + mFrameCount - mState.mPosition;
+        }
         mFramesReadySafe = clampToSize(mFramesReady);
         // This may overflow, but client is not supposed to rely on it
-        mCblk->u.mStatic.mBufferPosition = (uint32_t) position;
+        mCblk->u.mStatic.mBufferPosition = (uint32_t) mState.mPosition;
     }
-    return (ssize_t) position;
+    return (ssize_t) mState.mPosition;
 }
 
 status_t StaticAudioTrackServerProxy::obtainBuffer(Buffer* buffer, bool ackFlush __unused)
@@ -869,7 +943,7 @@ void StaticAudioTrackServerProxy::releaseBuffer(Buffer* buffer)
     }
     mUnreleased -= stepCount;
     audio_track_cblk_t* cblk = mCblk;
-    size_t position = mPosition;
+    size_t position = mState.mPosition;
     size_t newPosition = position + stepCount;
     int32_t setFlags = 0;
     if (!(position <= newPosition && newPosition <= mFrameCount)) {
@@ -887,7 +961,7 @@ void StaticAudioTrackServerProxy::releaseBuffer(Buffer* buffer)
     if (newPosition == mFrameCount) {
         setFlags |= CBLK_BUFFER_END;
     }
-    mPosition = newPosition;
+    mState.mPosition = newPosition;
     if (mFramesReady != INT64_MAX) {
         mFramesReady -= stepCount;
     }
@@ -895,7 +969,7 @@ 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) newPosition;
+    cblk->u.mStatic.mBufferPosition = (uint32_t) mState.mPosition;
     if (setFlags != 0) {
         (void) android_atomic_or(setFlags, &cblk->mFlags);
         // this would be a good place to wake a futex