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
diff --git a/include/private/media/AudioTrackShared.h b/include/private/media/AudioTrackShared.h
index b4fa3c5..ca119d5 100644
--- a/include/private/media/AudioTrackShared.h
+++ b/include/private/media/AudioTrackShared.h
@@ -60,6 +60,8 @@
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 @@
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 @@
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 @@
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 @@
// 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 @@
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 @@
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 @@
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() { }
};
diff --git a/media/libaudioclient/AudioTrack.cpp b/media/libaudioclient/AudioTrack.cpp
index 50c1295..46d9fde 100644
--- a/media/libaudioclient/AudioTrack.cpp
+++ b/media/libaudioclient/AudioTrack.cpp
@@ -770,6 +770,7 @@
mReleased = 0;
}
+ mProxy->stop(); // notify server not to read beyond current client position until start().
mProxy->interrupt();
mAudioTrack->stop();
diff --git a/media/libaudioclient/AudioTrackShared.cpp b/media/libaudioclient/AudioTrackShared.cpp
index 7bf4f99..b4c179d 100644
--- a/media/libaudioclient/AudioTrackShared.cpp
+++ b/media/libaudioclient/AudioTrackShared.cpp
@@ -393,19 +393,50 @@
// ---------------------------------------------------------------------------
-__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 @@
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 @@
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;
@@ -677,6 +714,45 @@
}
__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)
{
LOG_ALWAYS_FATAL_IF(buffer == NULL || buffer->mFrameCount == 0,
@@ -693,7 +769,7 @@
// 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 @@
// 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 @@
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 @@
}
}
+int32_t StaticAudioTrackServerProxy::getRear() const
+{
+ LOG_ALWAYS_FATAL("getRear() not permitted for static tracks");
+ return 0;
+}
+
// ---------------------------------------------------------------------------
} // namespace android
diff --git a/media/libmediaplayerservice/nuplayer/NuPlayerRenderer.cpp b/media/libmediaplayerservice/nuplayer/NuPlayerRenderer.cpp
index cc7f688..a762e76 100644
--- a/media/libmediaplayerservice/nuplayer/NuPlayerRenderer.cpp
+++ b/media/libmediaplayerservice/nuplayer/NuPlayerRenderer.cpp
@@ -1617,14 +1617,7 @@
// 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;
diff --git a/services/audioflinger/Tracks.cpp b/services/audioflinger/Tracks.cpp
index 9b93939..da61dd7 100644
--- a/services/audioflinger/Tracks.cpp
+++ b/services/audioflinger/Tracks.cpp
@@ -761,6 +761,12 @@
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;