From 1b42097f38e72574ed853a35f4e8a66e4739c421 Mon Sep 17 00:00:00 2001 From: Phil Burk Date: Wed, 22 Apr 2015 10:52:21 -0700 Subject: AudioTrack: fix spurious retrograde messages The retrograde motion was confused by some positions coming from the DSP on offloaded tracks. So the retrograde check was moved up into AudioTrack.cpp. This allows us to take advantage of the checks for invalid positions based on timing. Bug: 2047891 Change-Id: Ifcad2349201443a7f1711347c203297100449536 Signed-off-by: Phil Burk --- include/media/AudioTrack.h | 3 +++ media/libmedia/AudioTrack.cpp | 44 +++++++++++++++++++++++++++++++ services/audioflinger/PlaybackTracks.h | 5 ---- services/audioflinger/Tracks.cpp | 47 ++-------------------------------- 4 files changed, 49 insertions(+), 50 deletions(-) diff --git a/include/media/AudioTrack.h b/include/media/AudioTrack.h index e7ee0ce..d361901 100644 --- a/include/media/AudioTrack.h +++ b/include/media/AudioTrack.h @@ -832,6 +832,9 @@ protected: int64_t mStartUs; // the start time after flush or stop. // only used for offloaded and direct tracks. + bool mPreviousTimestampValid;// true if mPreviousTimestamp is valid + AudioTimestamp mPreviousTimestamp; // used to detect retrograde motion + audio_output_flags_t mFlags; // const after set(), except for bits AUDIO_OUTPUT_FLAG_FAST and AUDIO_OUTPUT_FLAG_OFFLOAD. // mLock must be held to read or write those bits reliably. diff --git a/media/libmedia/AudioTrack.cpp b/media/libmedia/AudioTrack.cpp index 8555983..98c3cae 100644 --- a/media/libmedia/AudioTrack.cpp +++ b/media/libmedia/AudioTrack.cpp @@ -470,6 +470,7 @@ status_t AudioTrack::set( mSequence = 1; mObservedSequence = mSequence; mInUnderrun = false; + mPreviousTimestampValid = false; return NO_ERROR; } @@ -496,6 +497,8 @@ status_t AudioTrack::start() if (previousState == STATE_STOPPED || previousState == STATE_FLUSHED) { // reset current position as seen by client to 0 mPosition = 0; + mPreviousTimestampValid = false; + // For offloaded tracks, we don't know if the hardware counters are really zero here, // since the flush is asynchronous and stop may not fully drain. // We save the time when the track is started to later verify whether @@ -995,6 +998,7 @@ status_t AudioTrack::reload() mNewPosition = mUpdatePeriod; (void) updateAndGetPosition_l(); mPosition = 0; + mPreviousTimestampValid = false; #if 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, @@ -2089,6 +2093,11 @@ status_t AudioTrack::setParameters(const String8& keyValuePairs) status_t AudioTrack::getTimestamp(AudioTimestamp& timestamp) { AutoMutex lock(mLock); + + bool previousTimestampValid = mPreviousTimestampValid; + // Set false here to cover all the error return cases. + mPreviousTimestampValid = false; + // FIXME not implemented for fast tracks; should use proxy and SSQ if (mFlags & AUDIO_OUTPUT_FLAG_FAST) { return INVALID_OPERATION; @@ -2187,6 +2196,41 @@ status_t AudioTrack::getTimestamp(AudioTimestamp& timestamp) // IAudioTrack. And timestamp.mPosition is initially in server's // point of view, so we need to apply the same fudge factor to it. } + + // Prevent retrograde motion in timestamp. + // This is sometimes caused by erratic reports of the available space in the ALSA drivers. + if (status == NO_ERROR) { + if (previousTimestampValid) { +#define TIME_TO_NANOS(time) ((uint64_t)time.tv_sec * 1000000000 + time.tv_nsec) + const uint64_t previousTimeNanos = TIME_TO_NANOS(mPreviousTimestamp.mTime); + const uint64_t currentTimeNanos = TIME_TO_NANOS(timestamp.mTime); +#undef TIME_TO_NANOS + if (currentTimeNanos < previousTimeNanos) { + ALOGW("retrograde timestamp time"); + // FIXME Consider blocking this from propagating upwards. + } + + // Looking at signed delta will work even when the timestamps + // are wrapping around. + int32_t deltaPosition = static_cast(timestamp.mPosition + - mPreviousTimestamp.mPosition); + // position can bobble slightly as an artifact; this hides the bobble + static const int32_t MINIMUM_POSITION_DELTA = 8; + ALOGW_IF(deltaPosition < 0, + "retrograde timestamp position corrected, %d = %u - %u, (at %llu, %llu nanos)", + deltaPosition, + timestamp.mPosition, + mPreviousTimestamp.mPosition, + currentTimeNanos, + previousTimeNanos); + if (deltaPosition < MINIMUM_POSITION_DELTA) { + timestamp = mPreviousTimestamp; // Use last valid timestamp. + } + } + mPreviousTimestamp = timestamp; + mPreviousTimestampValid = true; + } + return status; } diff --git a/services/audioflinger/PlaybackTracks.h b/services/audioflinger/PlaybackTracks.h index c51021b..7bc6f0c 100644 --- a/services/audioflinger/PlaybackTracks.h +++ b/services/audioflinger/PlaybackTracks.h @@ -156,11 +156,6 @@ private: bool mResumeToStopping; // track was paused in stopping state. bool mFlushHwPending; // track requests for thread flush - // for last call to getTimestamp - bool mPreviousTimestampValid; - // This is either the first timestamp or one that has passed - // the check to prevent retrograde motion. - AudioTimestamp mPreviousTimestamp; }; // end of Track class TimedTrack : public Track { diff --git a/services/audioflinger/Tracks.cpp b/services/audioflinger/Tracks.cpp index c6e9745..1b03060 100644 --- a/services/audioflinger/Tracks.cpp +++ b/services/audioflinger/Tracks.cpp @@ -404,8 +404,7 @@ AudioFlinger::PlaybackThread::Track::Track( mIsInvalid(false), mAudioTrackServerProxy(NULL), mResumeToStopping(false), - mFlushHwPending(false), - mPreviousTimestampValid(false) + mFlushHwPending(false) { // client == 0 implies sharedBuffer == 0 ALOG_ASSERT(!(client == 0 && sharedBuffer != 0)); @@ -863,7 +862,6 @@ void AudioFlinger::PlaybackThread::Track::reset() if (mState == FLUSHED) { mState = IDLE; } - mPreviousTimestampValid = false; } } @@ -885,12 +883,10 @@ status_t AudioFlinger::PlaybackThread::Track::getTimestamp(AudioTimestamp& times { // Client should implement this using SSQ; the unpresented frame count in latch is irrelevant if (isFastTrack()) { - // FIXME no lock held to set mPreviousTimestampValid = false return INVALID_OPERATION; } sp thread = mThread.promote(); if (thread == 0) { - // FIXME no lock held to set mPreviousTimestampValid = false return INVALID_OPERATION; } @@ -900,7 +896,6 @@ status_t AudioFlinger::PlaybackThread::Track::getTimestamp(AudioTimestamp& times status_t result = INVALID_OPERATION; if (!isOffloaded() && !isDirect()) { if (!playbackThread->mLatchQValid) { - mPreviousTimestampValid = false; return INVALID_OPERATION; } // FIXME Not accurate under dynamic changes of sample rate and speed. @@ -919,10 +914,7 @@ status_t AudioFlinger::PlaybackThread::Track::getTimestamp(AudioTimestamp& times uint32_t framesWritten = i >= 0 ? playbackThread->mLatchQ.mFramesReleased[i] : mAudioTrackServerProxy->framesReleased(); - if (framesWritten < unpresentedFrames) { - mPreviousTimestampValid = false; - // return invalid result - } else { + if (framesWritten >= unpresentedFrames) { timestamp.mPosition = framesWritten - unpresentedFrames; timestamp.mTime = playbackThread->mLatchQ.mTimestamp.mTime; result = NO_ERROR; @@ -931,41 +923,6 @@ status_t AudioFlinger::PlaybackThread::Track::getTimestamp(AudioTimestamp& times result = playbackThread->getTimestamp_l(timestamp); } - // Prevent retrograde motion in timestamp. - if (result == NO_ERROR) { - if (mPreviousTimestampValid) { - if (timestamp.mTime.tv_sec < mPreviousTimestamp.mTime.tv_sec || - (timestamp.mTime.tv_sec == mPreviousTimestamp.mTime.tv_sec && - timestamp.mTime.tv_nsec < mPreviousTimestamp.mTime.tv_nsec)) { - ALOGW("WARNING - retrograde timestamp time"); - // FIXME Consider blocking this from propagating upwards. - } - - // Looking at signed delta will work even when the timestamps - // are wrapping around. - int32_t deltaPosition = static_cast(timestamp.mPosition - - mPreviousTimestamp.mPosition); - // position can bobble slightly as an artifact; this hides the bobble - static const int32_t MINIMUM_POSITION_DELTA = 8; - if (deltaPosition < 0) { -#define TIME_TO_NANOS(time) ((uint64_t)time.tv_sec * 1000000000 + time.tv_nsec) - ALOGW("WARNING - retrograde timestamp position corrected," - " %d = %u - %u, (at %llu, %llu nanos)", - deltaPosition, - timestamp.mPosition, - mPreviousTimestamp.mPosition, - TIME_TO_NANOS(timestamp.mTime), - TIME_TO_NANOS(mPreviousTimestamp.mTime)); -#undef TIME_TO_NANOS - } - if (deltaPosition < MINIMUM_POSITION_DELTA) { - // Current timestamp is bad. Use last valid timestamp. - timestamp = mPreviousTimestamp; - } - } - mPreviousTimestamp = timestamp; - mPreviousTimestampValid = true; - } return result; } -- cgit v1.1