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 <philburk@google.com>

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<int32_t>(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<ThreadBase> 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<int32_t>(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;
 }