Improve MediaBuffer robustness for remote clients am: 9bd3c9b0e8 am: e2538f2f9b
am: ff97bcaf88
Change-Id: I97a2b06342b90949b489f72b9165fe618479fd82
diff --git a/include/media/stagefright/MediaBuffer.h b/include/media/stagefright/MediaBuffer.h
index 6cdd0fa..2c0ebe7 100644
--- a/include/media/stagefright/MediaBuffer.h
+++ b/include/media/stagefright/MediaBuffer.h
@@ -68,11 +68,16 @@
mMemory = mem;
}
- // Decrements the reference count and returns the buffer to its
- // associated MediaBufferGroup if the reference count drops to 0.
+ // If MediaBufferGroup is set, decrement the local reference count;
+ // if the local reference count drops to 0, return the buffer to the
+ // associated MediaBufferGroup.
+ //
+ // If no MediaBufferGroup is set, the local reference count must be zero
+ // when called, whereupon the MediaBuffer is deleted.
virtual void release();
- // Increments the reference count.
+ // Increments the local reference count.
+ // Use only when MediaBufferGroup is set.
virtual void add_ref();
void *data() const;
@@ -97,7 +102,28 @@
// MetaData.
MediaBuffer *clone();
- int refcount() const;
+ // sum of localRefcount() and remoteRefcount()
+ int refcount() const {
+ return localRefcount() + remoteRefcount();
+ }
+
+ int localRefcount() const {
+ return mRefCount;
+ }
+
+ int remoteRefcount() const {
+ if (mMemory.get() == nullptr || mMemory->pointer() == nullptr) return 0;
+ int32_t remoteRefcount =
+ reinterpret_cast<SharedControl *>(mMemory->pointer())->getRemoteRefcount();
+ // Sanity check so that remoteRefCount() is non-negative.
+ return remoteRefcount >= 0 ? remoteRefcount : 0; // do not allow corrupted data.
+ }
+
+ // returns old value
+ int addRemoteRefcount(int32_t value) {
+ if (mMemory.get() == nullptr || mMemory->pointer() == nullptr) return 0;
+ return reinterpret_cast<SharedControl *>(mMemory->pointer())->addRemoteRefcount(value);
+ }
bool isDeadObject() const {
return isDeadObject(mMemory);
@@ -117,25 +143,6 @@
}
protected:
- // MediaBuffer remote releases are handled through a
- // pending release count variable stored in a SharedControl block
- // at the start of the IMemory.
-
- // Returns old value of pending release count.
- inline int32_t addPendingRelease(int32_t value) {
- return getSharedControl()->addPendingRelease(value);
- }
-
- // Issues all pending releases (works in parallel).
- // Assumes there is a MediaBufferObserver.
- inline void resolvePendingRelease() {
- if (mMemory.get() == nullptr) return;
- while (addPendingRelease(-1) > 0) {
- release();
- }
- addPendingRelease(1);
- }
-
// true if MediaBuffer is observed (part of a MediaBufferGroup).
inline bool isObserved() const {
return mObserver != nullptr;
@@ -181,18 +188,18 @@
};
// returns old value
- inline int32_t addPendingRelease(int32_t value) {
+ inline int32_t addRemoteRefcount(int32_t value) {
return std::atomic_fetch_add_explicit(
- &mPendingRelease, (int_least32_t)value, std::memory_order_seq_cst);
+ &mRemoteRefcount, (int_least32_t)value, std::memory_order_seq_cst);
}
- inline int32_t getPendingRelease() const {
- return std::atomic_load_explicit(&mPendingRelease, std::memory_order_seq_cst);
+ inline int32_t getRemoteRefcount() const {
+ return std::atomic_load_explicit(&mRemoteRefcount, std::memory_order_seq_cst);
}
- inline void setPendingRelease(int32_t value) {
+ inline void setRemoteRefcount(int32_t value) {
std::atomic_store_explicit(
- &mPendingRelease, (int_least32_t)value, std::memory_order_seq_cst);
+ &mRemoteRefcount, (int_least32_t)value, std::memory_order_seq_cst);
}
inline bool isDeadObject() const {
@@ -209,13 +216,13 @@
std::atomic_store_explicit(
&mFlags, (int_least32_t)0, std::memory_order_seq_cst);
std::atomic_store_explicit(
- &mPendingRelease, (int_least32_t)0, std::memory_order_seq_cst);
+ &mRemoteRefcount, (int_least32_t)0, std::memory_order_seq_cst);
}
private:
// Caution: atomic_int_fast32_t is 64 bits on LP64.
std::atomic_int_least32_t mFlags;
- std::atomic_int_least32_t mPendingRelease;
+ std::atomic_int_least32_t mRemoteRefcount;
int32_t unused[6]; // additional buffer space
};
diff --git a/include/media/stagefright/MediaBufferGroup.h b/include/media/stagefright/MediaBufferGroup.h
index dfa31b2..3051406 100644
--- a/include/media/stagefright/MediaBufferGroup.h
+++ b/include/media/stagefright/MediaBufferGroup.h
@@ -53,10 +53,7 @@
size_t buffers() const { return mBuffers.size(); }
- // freeBuffers is the number of free buffers allowed to remain.
- void gc(size_t freeBuffers = 0);
-
-protected:
+ // If buffer is nullptr, have acquire_buffer() check for remote release.
virtual void signalBufferReturned(MediaBuffer *buffer);
private:
diff --git a/media/libmedia/IMediaSource.cpp b/media/libmedia/IMediaSource.cpp
index c9aae07..061f0d5 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) {