Merge "AudioTrack: Prevent server from reading client data after stop" into pi-dev

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

@@ -764,6 +764,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;