Fix loop and position setting in static AudioTracks
Allow independent setting of position and loop.
Bug: 17964637
Change-Id: I8b3bd97a244b932728b68da7684044f2636984a5
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 @@
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 @@
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 @@
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 @@
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 @@
// 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 @@
// 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 @@
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 @@
}
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 @@
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 @@
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 @@
: AudioTrackClientProxy(cblk, buffers, frameCount, frameSize),
mMutator(&cblk->u.mStatic.mSingleStateQueue), mBufferPosition(0)
{
+ memset(&mState, 0, sizeof(mState));
}
void StaticAudioTrackClientProxy::flush()
@@ -501,22 +516,47 @@
// 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;
}
- mBufferPosition = bufferPosition; // snapshot buffer position until loop is acknowledged.
- (void) mMutator.push(newState);
+ (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;
+ }
+ 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 @@
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 @@
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;
+ 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;
- }
- 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 @@
}
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 @@
if (newPosition == mFrameCount) {
setFlags |= CBLK_BUFFER_END;
}
- mPosition = newPosition;
+ mState.mPosition = newPosition;
if (mFramesReady != INT64_MAX) {
mFramesReady -= stepCount;
}
@@ -895,7 +969,7 @@
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