Improve MediaBuffer robustness for remote clients
Allow remote process to die or behave incorrectly.
Bug: 31060086
Change-Id: I01bc8984287fed61a46083ec090b7773e49158a9
diff --git a/media/libmedia/IMediaSource.cpp b/media/libmedia/IMediaSource.cpp
index dd94ccf..5289c5f 100644
--- a/media/libmedia/IMediaSource.cpp
+++ b/media/libmedia/IMediaSource.cpp
@@ -58,9 +58,9 @@
protected:
virtual ~RemoteMediaBufferWrapper() {
- // Indicate to MediaBufferGroup to release.
- int32_t old = addPendingRelease(1);
- ALOGV("RemoteMediaBufferWrapper: releasing %p, old %d", this, old);
+ // Release our interest in the MediaBuffer's shared memory.
+ int32_t old = addRemoteRefcount(-1);
+ ALOGV("RemoteMediaBufferWrapper: releasing %p, refcount %d", this, old - 1);
mMemory.clear(); // don't set the dead object flag.
}
};
@@ -296,8 +296,8 @@
case STOP: {
ALOGV("stop");
CHECK_INTERFACE(IMediaSource, data, reply);
+ mGroup->signalBufferReturned(nullptr);
status_t status = stop();
- mGroup->gc();
mIndexCache.reset();
mBuffersSinceStop = 0;
return status;
@@ -305,6 +305,7 @@
case PAUSE: {
ALOGV("pause");
CHECK_INTERFACE(IMediaSource, data, reply);
+ mGroup->signalBufferReturned(nullptr);
return pause();
}
case GETFORMAT: {
@@ -336,7 +337,7 @@
&& len == sizeof(opts)
&& data.read((void *)&opts, len) == NO_ERROR;
- mGroup->gc(kBinderMediaBuffers /* freeBuffers */);
+ mGroup->signalBufferReturned(nullptr);
mIndexCache.gc();
size_t inlineTransferSize = 0;
status_t ret = NO_ERROR;
@@ -411,10 +412,11 @@
reply->writeInt32(offset);
reply->writeInt32(length);
buf->meta_data()->writeToParcel(*reply);
- if (transferBuf != buf) {
- buf->release();
- } else if (!supportNonblockingRead()) {
- maxNumBuffers = 0; // stop readMultiple with one shared buffer.
+ if (transferBuf == buf) {
+ buf->addRemoteRefcount(1);
+ if (!supportNonblockingRead()) {
+ maxNumBuffers = 0; // stop readMultiple with one shared buffer.
+ }
}
} else {
ALOGV_IF(buf->mMemory != nullptr,
@@ -423,12 +425,12 @@
reply->writeInt32(INLINE_BUFFER);
reply->writeByteArray(length, (uint8_t*)buf->data() + offset);
buf->meta_data()->writeToParcel(*reply);
- buf->release();
inlineTransferSize += length;
if (inlineTransferSize > kInlineMaxTransfer) {
maxNumBuffers = 0; // stop readMultiple if inline transfer is too large.
}
}
+ buf->release();
}
reply->writeInt32(NULL_BUFFER); // Indicate no more MediaBuffers.
reply->writeInt32(ret);
diff --git a/media/libstagefright/foundation/MediaBuffer.cpp b/media/libstagefright/foundation/MediaBuffer.cpp
index 718b7e5..16000ef 100644
--- a/media/libstagefright/foundation/MediaBuffer.cpp
+++ b/media/libstagefright/foundation/MediaBuffer.cpp
@@ -105,14 +105,7 @@
void MediaBuffer::release() {
if (mObserver == NULL) {
- if (mMemory.get() != nullptr) {
- // See if there is a pending release and there are no observers.
- // Ideally this never happens.
- while (addPendingRelease(-1) > 0) {
- __sync_fetch_and_sub(&mRefCount, 1);
- }
- addPendingRelease(1);
- }
+ // Legacy contract for MediaBuffer without a MediaBufferGroup.
CHECK_EQ(mRefCount, 0);
delete this;
return;
@@ -205,10 +198,6 @@
mObserver = observer;
}
-int MediaBuffer::refcount() const {
- return mRefCount;
-}
-
MediaBuffer *MediaBuffer::clone() {
CHECK(mGraphicBuffer == NULL);
diff --git a/media/libstagefright/foundation/MediaBufferGroup.cpp b/media/libstagefright/foundation/MediaBufferGroup.cpp
index cb78879..54f768a 100644
--- a/media/libstagefright/foundation/MediaBufferGroup.cpp
+++ b/media/libstagefright/foundation/MediaBufferGroup.cpp
@@ -51,7 +51,7 @@
for (size_t i = 0; i < buffers; ++i) {
sp<IMemory> mem = memoryDealer->allocate(augmented_size);
- if (mem.get() == nullptr) {
+ if (mem.get() == nullptr || mem->pointer() == nullptr) {
ALOGW("Only allocated %zu shared buffers of size %zu", i, buffer_size);
break;
}
@@ -76,11 +76,24 @@
MediaBufferGroup::~MediaBufferGroup() {
for (MediaBuffer *buffer : mBuffers) {
- buffer->resolvePendingRelease();
- // If we don't release it, perhaps noone will release it.
- LOG_ALWAYS_FATAL_IF(buffer->refcount() != 0,
- "buffer refcount %p = %d != 0", buffer, buffer->refcount());
- // actually delete it.
+ if (buffer->refcount() != 0) {
+ const int localRefcount = buffer->localRefcount();
+ const int remoteRefcount = buffer->remoteRefcount();
+
+ // Fatal if we have a local refcount.
+ LOG_ALWAYS_FATAL_IF(localRefcount != 0,
+ "buffer(%p) localRefcount %d != 0, remoteRefcount %d",
+ buffer, localRefcount, remoteRefcount);
+
+ // Log an error if we have a remaining remote refcount,
+ // as the remote process may have died or may have inappropriate behavior.
+ // The shared memory associated with the MediaBuffer will
+ // automatically be reclaimed when there are no remaining fds
+ // associated with it.
+ ALOGE("buffer(%p) has residual remoteRefcount %d",
+ buffer, remoteRefcount);
+ }
+ // gracefully delete.
buffer->setObserver(nullptr);
buffer->release();
}
@@ -94,32 +107,11 @@
// optionally: mGrowthLimit = max(mGrowthLimit, mBuffers.size());
}
-void MediaBufferGroup::gc(size_t freeBuffers) {
- Mutex::Autolock autoLock(mLock);
-
- size_t freeCount = 0;
- for (auto it = mBuffers.begin(); it != mBuffers.end(); ) {
- (*it)->resolvePendingRelease();
- if ((*it)->isDeadObject()) {
- // The MediaBuffer has been deleted, why is it in the MediaBufferGroup?
- LOG_ALWAYS_FATAL("buffer(%p) has dead object with refcount %d",
- (*it), (*it)->refcount());
- } else if ((*it)->refcount() == 0 && ++freeCount > freeBuffers) {
- (*it)->setObserver(nullptr);
- (*it)->release();
- it = mBuffers.erase(it);
- } else {
- ++it;
- }
- }
-}
-
bool MediaBufferGroup::has_buffers() {
if (mBuffers.size() < mGrowthLimit) {
return true; // We can add more buffers internally.
}
for (MediaBuffer *buffer : mBuffers) {
- buffer->resolvePendingRelease();
if (buffer->refcount() == 0) {
return true;
}
@@ -135,7 +127,6 @@
MediaBuffer *buffer = nullptr;
auto free = mBuffers.end();
for (auto it = mBuffers.begin(); it != mBuffers.end(); ++it) {
- (*it)->resolvePendingRelease();
if ((*it)->refcount() == 0) {
const size_t size = (*it)->size();
if (size >= requestedSize) {