Fix SoundPool and MediaPlayerService buffer overflow

Overflow occurs when SoundPool sample tracks cannot
fit in the MediaPlayerService AudioCache buffer.

Unnecessary decoding occurred with AwesomePlayer and
an assert failure occurred with NuPlayer.  NuPlayerRenderer
is also tweaked to handle the latter case.

Bug: 17122639
Change-Id: I4d25d3e2c0c62e36a91da6bf969edabddc2ebbb0

diff --git a/media/libmediaplayerservice/MediaPlayerService.cpp b/media/libmediaplayerservice/MediaPlayerService.cpp
index 2c48306..b5bd988 100644 (file)
--- a/media/libmediaplayerservice/MediaPlayerService.cpp
+++ b/media/libmediaplayerservice/MediaPlayerService.cpp
@@ -1798,7 +1798,9 @@ ssize_t MediaPlayerService::AudioOutput::write(const void* buffer, size_t size)
     //ALOGV("write(%p, %u)", buffer, size);
     if (mTrack != 0) {
         ssize_t ret = mTrack->write(buffer, size);
-        mBytesWritten += ret;
+        if (ret >= 0) {
+            mBytesWritten += ret;
+        }
         return ret;
     }
     return NO_INIT;
@@ -1945,7 +1947,7 @@ uint32_t MediaPlayerService::AudioOutput::getSampleRate() const
 #define LOG_TAG "AudioCache"
 MediaPlayerService::AudioCache::AudioCache(const sp<IMemoryHeap>& heap) :
     mHeap(heap), mChannelCount(0), mFrameCount(1024), mSampleRate(0), mSize(0),
-    mError(NO_ERROR),  mCommandComplete(false)
+    mFrameSize(1), mError(NO_ERROR),  mCommandComplete(false)
 {
 }
 
@@ -1962,14 +1964,14 @@ float MediaPlayerService::AudioCache::msecsPerFrame() const
 status_t MediaPlayerService::AudioCache::getPosition(uint32_t *position) const
 {
     if (position == 0) return BAD_VALUE;
-    *position = mSize;
+    *position = mSize / mFrameSize;
     return NO_ERROR;
 }
 
 status_t MediaPlayerService::AudioCache::getFramesWritten(uint32_t *written) const
 {
     if (written == 0) return BAD_VALUE;
-    *written = mSize;
+    *written = mSize / mFrameSize;
     return NO_ERROR;
 }
 
@@ -2031,6 +2033,8 @@ bool CallbackThread::threadLoop() {
 
     if (actualSize > 0) {
         sink->write(mBuffer, actualSize);
+        // Could return false on sink->write() error or short count.
+        // Not necessarily appropriate but would work for AudioCache behavior.
     }
 
     return true;
@@ -2053,6 +2057,9 @@ status_t MediaPlayerService::AudioCache::open(
     mChannelCount = (uint16_t)channelCount;
     mFormat = format;
     mMsecsPerFrame = 1.e3 / (float) sampleRate;
+    mFrameSize =  audio_is_linear_pcm(mFormat)
+            ? mChannelCount * audio_bytes_per_sample(mFormat) : 1;
+    mFrameCount = mHeap->getSize() / mFrameSize;
 
     if (cb != NULL) {
         mCallbackThread = new CallbackThread(this, cb, cookie);
@@ -2082,12 +2089,26 @@ ssize_t MediaPlayerService::AudioCache::write(const void* buffer, size_t size)
     if (p == NULL) return NO_INIT;
     p += mSize;
     ALOGV("memcpy(%p, %p, %u)", p, buffer, size);
-    if (mSize + size > mHeap->getSize()) {
+
+    bool overflow = mSize + size > mHeap->getSize();
+    if (overflow) {
         ALOGE("Heap size overflow! req size: %d, max size: %d", (mSize + size), mHeap->getSize());
         size = mHeap->getSize() - mSize;
     }
+    size -= size % mFrameSize; // consume only integral amounts of frame size
     memcpy(p, buffer, size);
     mSize += size;
+
+    if (overflow) {
+        // Signal heap filled here (last frame may be truncated).
+        // After this point, no more data should be written as the
+        // heap is filled and the AudioCache should be effectively
+        // immutable with respect to future writes.
+        //
+        // It is thus safe for another thread to read the AudioCache.
+        mCommandComplete = true;
+        mSignal.signal();
+    }
     return size;
 }
 

diff --git a/media/libmediaplayerservice/MediaPlayerService.h b/media/libmediaplayerservice/MediaPlayerService.h
index 406e3f6..4fe7075 100644 (file)
--- a/media/libmediaplayerservice/MediaPlayerService.h
+++ b/media/libmediaplayerservice/MediaPlayerService.h
@@ -194,7 +194,7 @@ class MediaPlayerService : public BnMediaPlayerService
         virtual ssize_t         bufferSize() const { return frameSize() * mFrameCount; }
         virtual ssize_t         frameCount() const { return mFrameCount; }
         virtual ssize_t         channelCount() const { return (ssize_t)mChannelCount; }
-        virtual ssize_t         frameSize() const { return ssize_t(mChannelCount * ((mFormat == AUDIO_FORMAT_PCM_16_BIT)?sizeof(int16_t):sizeof(u_int8_t))); }
+        virtual ssize_t         frameSize() const { return (ssize_t)mFrameSize; }
         virtual uint32_t        latency() const;
         virtual float           msecsPerFrame() const;
         virtual status_t        getPosition(uint32_t *position) const;
@@ -244,6 +244,7 @@ class MediaPlayerService : public BnMediaPlayerService
         ssize_t             mFrameCount;
         uint32_t            mSampleRate;
         uint32_t            mSize;
+        size_t              mFrameSize;
         int                 mError;
         bool                mCommandComplete;
 

diff --git a/media/libmediaplayerservice/nuplayer/NuPlayerRenderer.cpp b/media/libmediaplayerservice/nuplayer/NuPlayerRenderer.cpp
index 1213a18..a3c976d 100644 (file)
--- a/media/libmediaplayerservice/nuplayer/NuPlayerRenderer.cpp
+++ b/media/libmediaplayerservice/nuplayer/NuPlayerRenderer.cpp
@@ -448,11 +448,13 @@ bool NuPlayer::Renderer::onDrainAudioQueue() {
             copy = numBytesAvailableToWrite;
         }
 
-        CHECK_EQ(mAudioSink->write(
-                    entry->mBuffer->data() + entry->mOffset, copy),
-                 (ssize_t)copy);
+        ssize_t written = mAudioSink->write(entry->mBuffer->data() + entry->mOffset, copy);
+        if (written < 0) {
+            // An error in AudioSink write is fatal here.
+            LOG_ALWAYS_FATAL("AudioSink write error(%zd) when writing %zu bytes", written, copy);
+        }
 
-        entry->mOffset += copy;
+        entry->mOffset += written;
         if (entry->mOffset == entry->mBuffer->size()) {
             entry->mNotifyConsumed->post();
             mAudioQueue.erase(mAudioQueue.begin());
@@ -460,13 +462,33 @@ bool NuPlayer::Renderer::onDrainAudioQueue() {
             entry = NULL;
         }
 
-        numBytesAvailableToWrite -= copy;
-        size_t copiedFrames = copy / mAudioSink->frameSize();
+        numBytesAvailableToWrite -= written;
+        size_t copiedFrames = written / mAudioSink->frameSize();
         mNumFramesWritten += copiedFrames;
 
         notifyIfMediaRenderingStarted();
-    }
 
+        if (written != (ssize_t)copy) {
+            // A short count was received from AudioSink::write()
+            //
+            // AudioSink write should block until exactly the number of bytes are delivered.
+            // But it may return with a short count (without an error) when:
+            //
+            // 1) Size to be copied is not a multiple of the frame size. We consider this fatal.
+            // 2) AudioSink is an AudioCache for data retrieval, and the AudioCache is exceeded.
+
+            // (Case 1)
+            // Must be a multiple of the frame size.  If it is not a multiple of a frame size, it
+            // needs to fail, as we should not carry over fractional frames between calls.
+            CHECK_EQ(copy % mAudioSink->frameSize(), 0);
+
+            // (Case 2)
+            // Return early to the caller.
+            // Beware of calling immediately again as this may busy-loop if you are not careful.
+            ALOGW("AudioSink write short frame count %zd < %zu", written, copy);
+            break;
+        }
+    }
     notifyPosition();
 
     return !mAudioQueue.empty();