am ef8ae4cb: Merge "Fix SoundPool and MediaPlayerService buffer overflow" into lmp-dev

* commit 'ef8ae4cbec0c9f49a24625d4316ec9bfde4e75c3':
  Fix SoundPool and MediaPlayerService buffer overflow

3 files changed, 57 insertions, 13 deletions

diff --git a/media/libmediaplayerservice/MediaPlayerService.cpp b/media/libmediaplayerservice/MediaPlayerService.cpp
index 2c48306..b5bd988 100644
--- 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
--- 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
--- 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();