Fix uncertainty of one normal mix buffer in AudioTrack::getTimestamp

The per-thread timestamp latch was not synchronized with the per-track
released frames.  Now the value of each track's released frames is
latched along with the timestamp.

Bug: 17531839
Bug: 17669342
Change-Id: I9d50c8c6a5de55a3f4561ac40e20d497376c1257

diff --git a/services/audioflinger/Threads.cpp b/services/audioflinger/Threads.cpp
index 12e09ab..b1e9c07 100644 (file)
--- a/services/audioflinger/Threads.cpp
+++ b/services/audioflinger/Threads.cpp
@@ -2101,6 +2101,7 @@ ssize_t AudioFlinger::PlaybackThread::threadLoop_write()
 
     // If an NBAIO sink is present, use it to write the normal mixer's submix
     if (mNormalSink != 0) {
+
         const size_t count = mBytesRemaining / mFrameSize;
 
         ATRACE_BEGIN("write");
@@ -2126,6 +2127,7 @@ ssize_t AudioFlinger::PlaybackThread::threadLoop_write()
             size_t totalFramesWritten = mNormalSink->framesWritten();
             if (totalFramesWritten >= mLatchD.mTimestamp.mPosition) {
                 mLatchD.mUnpresentedFrames = totalFramesWritten - mLatchD.mTimestamp.mPosition;
+                // mLatchD.mFramesReleased is set immediately before D is clocked into Q
                 mLatchDValid = true;
             }
         }
@@ -2418,6 +2420,18 @@ bool AudioFlinger::PlaybackThread::threadLoop()
                 logString = NULL;
             }
 
+            // Gather the framesReleased counters for all active tracks,
+            // and latch them atomically with the timestamp.
+            // FIXME We're using raw pointers as indices. A unique track ID would be a better index.
+            mLatchD.mFramesReleased.clear();
+            size_t size = mActiveTracks.size();
+            for (size_t i = 0; i < size; i++) {
+                sp<Track> t = mActiveTracks[i].promote();
+                if (t != 0) {
+                    mLatchD.mFramesReleased.add(t.get(),
+                            t->mAudioTrackServerProxy->framesReleased());
+                }
+            }
             if (mLatchDValid) {
                 mLatchQ = mLatchD;
                 mLatchDValid = false;
@@ -3093,6 +3107,7 @@ void AudioFlinger::MixerThread::threadLoop_mix()
     sleepTime = 0;
     standbyTime = systemTime() + standbyDelay;
     //TODO: delay standby when effects have a tail
+
 }
 
 void AudioFlinger::MixerThread::threadLoop_sleepTime()

diff --git a/services/audioflinger/Threads.h b/services/audioflinger/Threads.h
index 7af5264..bb9aa18 100644 (file)
--- a/services/audioflinger/Threads.h
+++ b/services/audioflinger/Threads.h
@@ -819,8 +819,11 @@ private:
     struct {
         AudioTimestamp  mTimestamp;
         uint32_t        mUnpresentedFrames;
+        KeyedVector<Track *, uint32_t> mFramesReleased;
     } mLatchD, mLatchQ;
-    bool mLatchDValid;  // true means mLatchD is valid, and clock it into latch at next opportunity
+    bool mLatchDValid;  // true means mLatchD is valid
+                        //     (except for mFramesReleased which is filled in later),
+                        //     and clock it into latch at next opportunity
     bool mLatchQValid;  // true means mLatchQ is valid
 };
 

diff --git a/services/audioflinger/Tracks.cpp b/services/audioflinger/Tracks.cpp
index 75190f3..b9308fa 100644 (file)
--- a/services/audioflinger/Tracks.cpp
+++ b/services/audioflinger/Tracks.cpp
@@ -898,7 +898,16 @@ status_t AudioFlinger::PlaybackThread::Track::getTimestamp(AudioTimestamp& times
         uint32_t unpresentedFrames =
                 ((int64_t) playbackThread->mLatchQ.mUnpresentedFrames * mSampleRate) /
                 playbackThread->mSampleRate;
-        uint32_t framesWritten = mAudioTrackServerProxy->framesReleased();
+        // FIXME Since we're using a raw pointer as the key, it is theoretically possible
+        //       for a brand new track to share the same address as a recently destroyed
+        //       track, and thus for us to get the frames released of the wrong track.
+        //       It is unlikely that we would be able to call getTimestamp() so quickly
+        //       right after creating a new track.  Nevertheless, the index here should
+        //       be changed to something that is unique.  Or use a completely different strategy.
+        ssize_t i = playbackThread->mLatchQ.mFramesReleased.indexOfKey(this);
+        uint32_t framesWritten = i >= 0 ?
+                playbackThread->mLatchQ.mFramesReleased[i] :
+                mAudioTrackServerProxy->framesReleased();
         bool checkPreviousTimestamp = mPreviousValid && framesWritten >= mPreviousFramesWritten;
         if (framesWritten < unpresentedFrames) {
             mPreviousValid = false;