From cb2129b3b568a4e31bcbda3545a468024bc972fe Mon Sep 17 00:00:00 2001 From: Andy Hung Date: Tue, 11 Nov 2014 12:17:22 -0800 Subject: 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 --- media/libmedia/AudioTrackShared.cpp | 60 +++++++++++++++++++++++-------------- 1 file changed, 38 insertions(+), 22 deletions(-) (limited to 'media/libmedia/AudioTrackShared.cpp') 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 +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 @@ StaticAudioTrackServerProxy::StaticAudioTrackServerProxy(audio_track_cblk_t* cbl 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 @@ void StaticAudioTrackServerProxy::framesReadyIsCalledByMultipleThreads() 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 @@ ssize_t StaticAudioTrackServerProxy::pollPosition() } // 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 @@ status_t StaticAudioTrackServerProxy::obtainBuffer(Buffer* buffer, bool ackFlush 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 @@ status_t StaticAudioTrackServerProxy::obtainBuffer(Buffer* buffer, bool ackFlush 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 @@ status_t StaticAudioTrackServerProxy::obtainBuffer(Buffer* buffer, bool ackFlush 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 @@ void StaticAudioTrackServerProxy::releaseBuffer(Buffer* buffer) 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 @@ void StaticAudioTrackServerProxy::releaseBuffer(Buffer* buffer) 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 -- cgit v1.1