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
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 @@
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 @@
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 @@
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 @@
}
// 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 @@
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 @@
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 @@
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 @@
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 @@
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