Fix issue 4111672: AudioTrack control block flags

Make sure that all read/modify/write operations on the AudioTrack
and AudioRecord control block flags field are protected by the
control block's mutex.

Also fix potential infinite loop in AudioTrack::write() if the
written size is not a multiple of frame size.

Change-Id: Ib3d557eb45dcc3abeb32c9aa56058e2873afee27

media/libmedia/AudioRecord.cpp

@@ -722,9 +722,12 @@ bool AudioRecord::processAudioBuffer(const sp<ClientRecordThread>& thread)
     // Manage overrun callback
     if (mActive && (cblk->framesAvailable() == 0)) {
         LOGV("Overrun user: %x, server: %x, flags %04x", cblk->user, cblk->server, cblk->flags);
+        AutoMutex _l(cblk->lock);
         if ((cblk->flags & CBLK_UNDERRUN_MSK) == CBLK_UNDERRUN_OFF) {
-            mCbf(EVENT_OVERRUN, mUserData, 0);
             cblk->flags |= CBLK_UNDERRUN_ON;
+            cblk->lock.unlock();
+            mCbf(EVENT_OVERRUN, mUserData, 0);
+            cblk->lock.lock();
         }
     }
 

media/libmedia/AudioTrack.cpp

@@ -329,6 +329,7 @@ void AudioTrack::start()
     if (mActive == 0) {
         mActive = 1;
         mNewPosition = cblk->server + mUpdatePeriod;
+        cblk->lock.lock();
         cblk->bufferTimeoutMs = MAX_STARTUP_TIMEOUT_MS;
         cblk->waitTimeMs = 0;
         cblk->flags &= ~CBLK_DISABLED_ON;
@@ -339,7 +340,6 @@ void AudioTrack::start()
         }
 
         LOGV("start %p before lock cblk %p", this, mCblk);
-        cblk->lock.lock();
         if (!(cblk->flags & CBLK_INVALID_MSK)) {
             cblk->lock.unlock();
             status = mAudioTrack->start();
@@ -893,9 +893,14 @@ create_new_track:
 
     // restart track if it was disabled by audioflinger due to previous underrun
     if (mActive && (cblk->flags & CBLK_DISABLED_MSK)) {
-        cblk->flags &= ~CBLK_DISABLED_ON;
+        AutoMutex _l(cblk->lock);
-        LOGW("obtainBuffer() track %p disabled, restarting", this);
+        if (mActive && (cblk->flags & CBLK_DISABLED_MSK)) {
-        mAudioTrack->start();
+            cblk->flags &= ~CBLK_DISABLED_ON;
+            cblk->lock.unlock();
+            LOGW("obtainBuffer() track %p disabled, restarting", this);
+            mAudioTrack->start();
+            cblk->lock.lock();
+        }
     }
 
     cblk->waitTimeMs = 0;
@@ -957,9 +962,10 @@ ssize_t AudioTrack::write(const void* buffer, size_t userSize)
     ssize_t written = 0;
     const int8_t *src = (const int8_t *)buffer;
     Buffer audioBuffer;
+    size_t frameSz = (size_t)frameSize();
 
     do {
-        audioBuffer.frameCount = userSize/frameSize();
+        audioBuffer.frameCount = userSize/frameSz;
 
         // Calling obtainBuffer() with a negative wait count causes
         // an (almost) infinite wait time.
@@ -991,7 +997,7 @@ ssize_t AudioTrack::write(const void* buffer, size_t userSize)
         written += toWrite;
 
         releaseBuffer(&audioBuffer);
-    } while (userSize);
+    } while (userSize >= frameSz);
 
     return written;
 }
@@ -1015,12 +1021,15 @@ bool AudioTrack::processAudioBuffer(const sp<AudioTrackThread>& thread)
     // Manage underrun callback
     if (mActive && (cblk->framesReady() == 0)) {
         LOGV("Underrun user: %x, server: %x, flags %04x", cblk->user, cblk->server, cblk->flags);
+        AutoMutex _l(cblk->lock);
         if ((cblk->flags & CBLK_UNDERRUN_MSK) == CBLK_UNDERRUN_OFF) {
+            cblk->flags |= CBLK_UNDERRUN_ON;
+            cblk->lock.unlock();
             mCbf(EVENT_UNDERRUN, mUserData, 0);
             if (cblk->server == cblk->frameCount) {
                 mCbf(EVENT_BUFFER_END, mUserData, 0);
             }
-            cblk->flags |= CBLK_UNDERRUN_ON;
+            cblk->lock.lock();
             if (mSharedBuffer != 0) return false;
         }
     }
@@ -1279,7 +1288,12 @@ uint32_t audio_track_cblk_t::stepUser(uint32_t frameCount)
     this->user = u;
 
     // Clear flow control error condition as new data has been written/read to/from buffer.
-    flags &= ~CBLK_UNDERRUN_MSK;
+    if (flags & CBLK_UNDERRUN_MSK) {
+        AutoMutex _l(lock);
+        if (flags & CBLK_UNDERRUN_MSK) {
+            flags &= ~CBLK_UNDERRUN_MSK;
+        }
+    }
 
     return u;
 }

services/audioflinger/AudioFlinger.cpp

@@ -1738,7 +1738,10 @@ uint32_t AudioFlinger::MixerThread::prepareTracks_l(const SortedVector< wp<Track
                     LOGV("BUFFER TIMEOUT: remove(%d) from active list on thread %p", track->name(), this);
                     tracksToRemove->add(track);
                     // indicate to client process that the track was disabled because of underrun
-                    cblk->flags |= CBLK_DISABLED_ON;
+                    {
+                        AutoMutex _l(cblk->lock);
+                        cblk->flags |= CBLK_DISABLED_ON;
+                    }
                 } else if (mixerStatus != MIXER_TRACKS_READY) {
                     mixerStatus = MIXER_TRACKS_ENABLED;
                 }
@@ -1787,10 +1790,9 @@ void AudioFlinger::MixerThread::invalidateTracks(int streamType)
     for (size_t i = 0; i < size; i++) {
         sp<Track> t = mTracks[i];
         if (t->type() == streamType) {
-            t->mCblk->lock.lock();
+            AutoMutex _lcblk(t->mCblk->lock);
             t->mCblk->flags |= CBLK_INVALID_ON;
             t->mCblk->cv.signal();
-            t->mCblk->lock.unlock();
         }
     }
 }
@@ -2948,6 +2950,7 @@ bool AudioFlinger::PlaybackThread::Track::isReady() const {
 
     if (mCblk->framesReady() >= mCblk->frameCount ||
             (mCblk->flags & CBLK_FORCEREADY_MSK)) {
+        AutoMutex _l(mCblk->lock);
         mFillingUpStatus = FS_FILLED;
         mCblk->flags &= ~CBLK_FORCEREADY_MSK;
         return true;
@@ -3063,19 +3066,18 @@ void AudioFlinger::PlaybackThread::Track::flush()
         // STOPPED state
         mState = STOPPED;
 
-        mCblk->lock.lock();
         // NOTE: reset() will reset cblk->user and cblk->server with
         // the risk that at the same time, the AudioMixer is trying to read
         // data. In this case, getNextBuffer() would return a NULL pointer
         // as audio buffer => the AudioMixer code MUST always test that pointer
         // returned by getNextBuffer() is not NULL!
         reset();
-        mCblk->lock.unlock();
     }
 }
 
 void AudioFlinger::PlaybackThread::Track::reset()
 {
+    AutoMutex _l(mCblk->lock);
     // Do not reset twice to avoid discarding data written just after a flush and before
     // the audioflinger thread detects the track is stopped.
     if (!mResetDone) {
@@ -3209,10 +3211,13 @@ void AudioFlinger::RecordThread::RecordTrack::stop()
     if (thread != 0) {
         RecordThread *recordThread = (RecordThread *)thread.get();
         recordThread->stop(this);
-        TrackBase::reset();
+        {
-        // Force overerrun condition to avoid false overrun callback until first data is
+            AutoMutex _l(mCblk->lock);
-        // read from buffer
+            TrackBase::reset();
-        mCblk->flags |= CBLK_UNDERRUN_ON;
+            // Force overerrun condition to avoid false overrun callback until first data is
+            // read from buffer
+            mCblk->flags |= CBLK_UNDERRUN_ON;
+        }
     }
 }