AudioTrack: Prevent server from reading client data after stop

Prior to this CL, if an AudioTrack client wrote audio data after
AudioTrack stop(), but before the track was drained by the server,
the newly written client data would be consumed in the drain.

We now limit the server read to the client write position on stop.
This interlocking is essential for rapid asynchronous AudioTrack
command processing.

Test: AudioTrack CTS, SoundPool looping, bug test case
Bug: 75788313
Change-Id: Ib70e3dc46afe047a8c6cf1fb906a618b3c66cc7f

include/private/media/AudioTrackShared.h

@@ -60,6 +60,8 @@ struct AudioTrackSharedStreaming {
     volatile int32_t mRear;     // written by producer (output: client, input: server)
     volatile int32_t mFlush;    // incremented by client to indicate a request to flush;
                                 // server notices and discards all data between mFront and mRear
+    volatile int32_t mStop;     // set by client to indicate a stop frame position; server
+                                // will not read beyond this position until start is called.
     volatile uint32_t mUnderrunFrames; // server increments for each unavailable but desired frame
     volatile uint32_t mUnderrunCount;  // server increments for each underrun occurrence
 };
@@ -335,6 +337,8 @@ public:
         mTimestamp.clear();
     }
 
+    virtual void stop() { }; // called by client in AudioTrack::stop()
+
 private:
     // This is a copy of mCblk->mBufferSizeInFrames
     uint32_t   mBufferSizeInFrames;  // effective size of the buffer
@@ -383,8 +387,14 @@ public:
         mPlaybackRateMutator.push(playbackRate);
     }
 
+    // Sends flush and stop position information from the client to the server,
+    // used by streaming AudioTrack flush() or stop().
+    void sendStreamingFlushStop(bool flush);
+
     virtual void flush();
 
+            void stop() override;
+
     virtual uint32_t    getUnderrunFrames() const {
         return mCblk->u.mStreaming.mUnderrunFrames;
     }
@@ -410,6 +420,8 @@ public:
 
     virtual void    flush();
 
+    void stop() override;
+
 #define MIN_LOOP    16  // minimum length of each loop iteration in frames
 
             // setLoop(), setBufferPosition(), and setBufferPositionAndLoop() set the
@@ -532,6 +544,10 @@ public:
     //   client will be notified via Futex
     virtual void    flushBufferIfNeeded();
 
+    // Returns the rear position of the AudioTrack shared ring buffer, limited by
+    // the stop frame position level.
+    virtual int32_t getRear() const = 0;
+
     // Total count of the number of flushed frames since creation (never reset).
     virtual int64_t     framesFlushed() const { return mFlushed; }
 
@@ -607,10 +623,18 @@ public:
         return mDrained.load();
     }
 
+    int32_t             getRear() const override;
+
+    // Called on server side track start().
+    virtual void        start();
+
 private:
     AudioPlaybackRate             mPlaybackRate;  // last observed playback rate
     PlaybackRateQueue::Observer   mPlaybackRateObserver;
 
+    // Last client stop-at position when start() was called. Used for streaming AudioTracks.
+    std::atomic<int32_t>          mStopLast{0};
+
     // The server keeps a copy here where it is safe from the client.
     uint32_t                      mUnderrunCount; // echoed to mCblk
     bool                          mUnderrunning;  // used to detect edge of underrun
@@ -634,6 +658,10 @@ public:
     virtual void        tallyUnderrunFrames(uint32_t frameCount);
     virtual uint32_t    getUnderrunFrames() const { return 0; }
 
+    int32_t getRear() const override;
+
+    void start() override { } // ignore for static tracks
+
 private:
     status_t            updateStateWithLoop(StaticAudioTrackState *localState,
                                             const StaticAudioTrackState &update) const;
@@ -661,6 +689,10 @@ public:
             size_t frameSize, bool clientInServer)
         : ServerProxy(cblk, buffers, frameCount, frameSize, false /*isOut*/, clientInServer) { }
 
+    int32_t getRear() const override {
+        return mCblk->u.mStreaming.mRear; // For completeness only; mRear written by server.
+    }
+
 protected:
     virtual ~AudioRecordServerProxy() { }
 };

media/libaudioclient/AudioTrack.cpp

@@ -770,6 +770,7 @@ void AudioTrack::stop()
         mReleased = 0;
     }
 
+    mProxy->stop(); // notify server not to read beyond current client position until start().
     mProxy->interrupt();
     mAudioTrack->stop();
 

media/libaudioclient/AudioTrackShared.cpp

@@ -393,19 +393,50 @@ size_t ClientProxy::getMisalignment()
 
 // ---------------------------------------------------------------------------
 
-__attribute__((no_sanitize("integer")))
 void AudioTrackClientProxy::flush()
 {
+    sendStreamingFlushStop(true /* flush */);
+}
+
+void AudioTrackClientProxy::stop()
+{
+    sendStreamingFlushStop(false /* flush */);
+}
+
+// Sets the client-written mFlush and mStop positions, which control server behavior.
+//
+// @param flush indicates whether the operation is a flush or stop.
+// A client stop sets mStop to the current write position;
+// the server will not read past this point until start() or subsequent flush().
+// A client flush sets both mStop and mFlush to the current write position.
+// This advances the server read limit (if previously set) and on the next
+// server read advances the server read position to this limit.
+//
+void AudioTrackClientProxy::sendStreamingFlushStop(bool flush)
+{
+    // TODO: Replace this by 64 bit counters - avoids wrap complication.
     // This works for mFrameCountP2 <= 2^30
-    size_t increment = mFrameCountP2 << 1;
-    size_t mask = increment - 1;
-    audio_track_cblk_t* cblk = mCblk;
     // mFlush is 32 bits concatenated as [ flush_counter ] [ newfront_offset ]
     // Should newFlush = cblk->u.mStreaming.mRear?  Only problem is
     // if you want to flush twice to the same rear location after a 32 bit wrap.
-    int32_t newFlush = (cblk->u.mStreaming.mRear & mask) |
-                        ((cblk->u.mStreaming.mFlush & ~mask) + increment);
-    android_atomic_release_store(newFlush, &cblk->u.mStreaming.mFlush);
+
+    const size_t increment = mFrameCountP2 << 1;
+    const size_t mask = increment - 1;
+    // No need for client atomic synchronization on mRear, mStop, mFlush
+    // as AudioTrack client only read/writes to them under client lock. Server only reads.
+    const int32_t rearMasked = mCblk->u.mStreaming.mRear & mask;
+
+    // update stop before flush so that the server front
+    // never advances beyond a (potential) previous stop's rear limit.
+    int32_t stopBits; // the following add can overflow
+    __builtin_add_overflow(mCblk->u.mStreaming.mStop & ~mask, increment, &stopBits);
+    android_atomic_release_store(rearMasked | stopBits, &mCblk->u.mStreaming.mStop);
+
+    if (flush) {
+        int32_t flushBits; // the following add can overflow
+        __builtin_add_overflow(mCblk->u.mStreaming.mFlush & ~mask, increment, &flushBits);
+        android_atomic_release_store(rearMasked | flushBits, &mCblk->u.mStreaming.mFlush);
+    }
 }
 
 bool AudioTrackClientProxy::clearStreamEndDone() {
@@ -540,6 +571,11 @@ void StaticAudioTrackClientProxy::flush()
     LOG_ALWAYS_FATAL("static flush");
 }
 
+void StaticAudioTrackClientProxy::stop()
+{
+    ; // no special handling required for static tracks.
+}
+
 void StaticAudioTrackClientProxy::setLoop(size_t loopStart, size_t loopEnd, int loopCount)
 {
     // This can only happen on a 64-bit client
@@ -638,6 +674,7 @@ void ServerProxy::flushBufferIfNeeded()
     if (flush != mFlush) {
         ALOGV("ServerProxy::flushBufferIfNeeded() mStreaming.mFlush = 0x%x, mFlush = 0x%0x",
                 flush, mFlush);
+        // shouldn't matter, but for range safety use mRear instead of getRear().
         int32_t rear = android_atomic_acquire_load(&cblk->u.mStreaming.mRear);
         int32_t front = cblk->u.mStreaming.mFront;
 
@@ -676,6 +713,45 @@ void ServerProxy::flushBufferIfNeeded()
     }
 }
 
+__attribute__((no_sanitize("integer")))
+int32_t AudioTrackServerProxy::getRear() const
+{
+    const int32_t stop = android_atomic_acquire_load(&mCblk->u.mStreaming.mStop);
+    const int32_t rear = android_atomic_acquire_load(&mCblk->u.mStreaming.mRear);
+    const int32_t stopLast = mStopLast.load(std::memory_order_acquire);
+    if (stop != stopLast) {
+        const int32_t front = mCblk->u.mStreaming.mFront;
+        const size_t overflowBit = mFrameCountP2 << 1;
+        const size_t mask = overflowBit - 1;
+        int32_t newRear = (rear & ~mask) | (stop & mask);
+        ssize_t filled = newRear - front;
+        if (filled < 0) {
+            // front and rear offsets span the overflow bit of the p2 mask
+            // so rebasing newrear.
+            ALOGV("stop wrap: filled %zx >= overflowBit %zx", filled, overflowBit);
+            newRear += overflowBit;
+            filled += overflowBit;
+        }
+        if (0 <= filled && (size_t) filled <= mFrameCount) {
+            // we're stopped, return the stop level as newRear
+            return newRear;
+        }
+
+        // A corrupt stop. Log error and ignore.
+        ALOGE("mStopLast %#x -> stop %#x, front %#x, rear %#x, mask %#x, newRear %#x, "
+                "filled %zd=%#x",
+                stopLast, stop, front, rear,
+                (unsigned)mask, newRear, filled, (unsigned)filled);
+        // Don't reset mStopLast as this is const.
+    }
+    return rear;
+}
+
+void AudioTrackServerProxy::start()
+{
+    mStopLast = android_atomic_acquire_load(&mCblk->u.mStreaming.mStop);
+}
+
 __attribute__((no_sanitize("integer")))
 status_t ServerProxy::obtainBuffer(Buffer* buffer, bool ackFlush)
 {
@@ -693,7 +769,7 @@ status_t ServerProxy::obtainBuffer(Buffer* buffer, bool ackFlush)
     // See notes on barriers at ClientProxy::obtainBuffer()
     if (mIsOut) {
         flushBufferIfNeeded(); // might modify mFront
-        rear = android_atomic_acquire_load(&cblk->u.mStreaming.mRear);
+        rear = getRear();
         front = cblk->u.mStreaming.mFront;
     } else {
         front = android_atomic_acquire_load(&cblk->u.mStreaming.mFront);
@@ -825,8 +901,7 @@ size_t AudioTrackServerProxy::framesReady()
         // FIXME should return an accurate value, but over-estimate is better than under-estimate
         return mFrameCount;
     }
-    // the acquire might not be necessary since not doing a subsequent read
-    int32_t rear = android_atomic_acquire_load(&cblk->u.mStreaming.mRear);
+    const int32_t rear = getRear();
     ssize_t filled = rear - cblk->u.mStreaming.mFront;
     // pipe should not already be overfull
     if (!(0 <= filled && (size_t) filled <= mFrameCount)) {
@@ -852,7 +927,7 @@ size_t AudioTrackServerProxy::framesReadySafe() const
     if (flush != mFlush) {
         return mFrameCount;
     }
-    const int32_t rear = android_atomic_acquire_load(&cblk->u.mStreaming.mRear);
+    const int32_t rear = getRear();
     const ssize_t filled = rear - cblk->u.mStreaming.mFront;
     if (!(0 <= filled && (size_t) filled <= mFrameCount)) {
         return 0; // error condition, silently return 0.
@@ -1149,6 +1224,12 @@ void StaticAudioTrackServerProxy::tallyUnderrunFrames(uint32_t frameCount)
     }
 }
 
+int32_t StaticAudioTrackServerProxy::getRear() const
+{
+    LOG_ALWAYS_FATAL("getRear() not permitted for static tracks");
+    return 0;
+}
+
 // ---------------------------------------------------------------------------
 
 }   // namespace android

media/libmediaplayerservice/nuplayer/NuPlayerRenderer.cpp

@@ -1617,14 +1617,7 @@ void NuPlayer::Renderer::onFlush(const sp<AMessage> &msg) {
             // internal buffer before resuming playback.
             // FIXME: this is ignored after flush().
             mAudioSink->stop();
-            if (mPaused) {
-                // Race condition: if renderer is paused and audio sink is stopped,
-                // we need to make sure that the audio track buffer fully drains
-                // before delivering data.
-                // FIXME: remove this if we can detect if stop() is complete.
-                const int delayUs = 2 * 50 * 1000; // (2 full mixer thread cycles at 50ms)
-                mPauseDrainAudioAllowedUs = ALooper::GetNowUs() + delayUs;
-            } else {
+            if (!mPaused) {
                 mAudioSink->start();
             }
             mNumFramesWritten = 0;

services/audioflinger/Tracks.cpp

@@ -761,6 +761,12 @@ status_t AudioFlinger::PlaybackThread::Track::start(AudioSystem::sync_event_t ev
                 mState = state;
             }
         }
+
+        if (status == NO_ERROR || status == ALREADY_EXISTS) {
+            // for streaming tracks, remove the buffer read stop limit.
+            mAudioTrackServerProxy->start();
+        }
+
         // track was already in the active list, not a problem
         if (status == ALREADY_EXISTS) {
             status = NO_ERROR;