From 4ede21d9c1f957baf5e561849ff9bbe4bcbefc20 Mon Sep 17 00:00:00 2001 From: Andy Hung Date: Fri, 12 Dec 2014 15:37:34 -0800 Subject: 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 --- include/media/AudioTrack.h | 7 +++- include/private/media/AudioTrackShared.h | 36 +++++++++++++---- media/libmedia/AudioTrack.cpp | 53 +++++++++++++------------ media/libmedia/AudioTrackShared.cpp | 67 +++++++++++++++++++++++--------- 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 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 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 +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 -- cgit v1.1