Initialize mMetaData correctly, fix ref count
mMetaData was inconsistently initialized to either a MetaData or
MetaDataBase object. The former resulted in the destruction of
a RefBase object without ever incrementing its reference count.
This is normally very brittle, since the accidental creation of
an sp<> results in duplicate deallocation. Hence we are starting
to diagnose it. In this case it appears to just add extra overhead.
Since mRefCount can be concurrently accessed, declare it as atomic.
This is a minimal fix, which can be improved further. Local and remote
reference counts are currently still read with inconsistent memory
orders. Since the results appear to only be trusted when they
guarantee that there is no concurrent access (as they should be),
memory_order_relaxed on loads seems sufficient. Generally reference
count increments can also be relaxed, while decrements should be
acq_rel to ensure that all accesses to the object become effective
before deallocation. I believe that's correct here as well. Since
I'm unfamiliar with the code, I made only essential changes.
Bug: 79265389
Test: Built and booted master.
Change-Id: I3cb65e9f880588abdb0712890e7a654311577b07
diff --git a/media/libmediaextractor/MediaBuffer.cpp b/media/libmediaextractor/MediaBuffer.cpp
index 39f8d6e..d197b3f 100644
--- a/media/libmediaextractor/MediaBuffer.cpp
+++ b/media/libmediaextractor/MediaBuffer.cpp
@@ -39,7 +39,7 @@
mRangeOffset(0),
mRangeLength(size),
mOwnsData(false),
- mMetaData(new MetaData),
+ mMetaData(new MetaDataBase),
mOriginal(NULL) {
}
@@ -51,7 +51,7 @@
mRangeOffset(0),
mRangeLength(size),
mOwnsData(true),
- mMetaData(new MetaData),
+ mMetaData(new MetaDataBase),
mOriginal(NULL) {
if (size < kSharedMemThreshold
|| std::atomic_load_explicit(&mUseSharedMemory, std::memory_order_seq_cst) == 0) {
@@ -84,7 +84,7 @@
mRangeLength(mSize),
mBuffer(buffer),
mOwnsData(false),
- mMetaData(new MetaData),
+ mMetaData(new MetaDataBase),
mOriginal(NULL) {
}
@@ -96,7 +96,7 @@
return;
}
- int prevCount = __sync_fetch_and_sub(&mRefCount, 1);
+ int prevCount = mRefCount.fetch_sub(1);
if (prevCount == 1) {
if (mObserver == NULL) {
delete this;
@@ -110,13 +110,13 @@
void MediaBuffer::claim() {
CHECK(mObserver != NULL);
- CHECK_EQ(mRefCount, 1);
+ CHECK_EQ(mRefCount.load(std::memory_order_relaxed), 1);
- mRefCount = 0;
+ mRefCount.store(0, std::memory_order_relaxed);
}
void MediaBuffer::add_ref() {
- (void) __sync_fetch_and_add(&mRefCount, 1);
+ (void) mRefCount.fetch_add(1);
}
void *MediaBuffer::data() const {
diff --git a/media/libmediaextractor/include/media/stagefright/MediaBuffer.h b/media/libmediaextractor/include/media/stagefright/MediaBuffer.h
index f944d51..5a25965 100644
--- a/media/libmediaextractor/include/media/stagefright/MediaBuffer.h
+++ b/media/libmediaextractor/include/media/stagefright/MediaBuffer.h
@@ -86,12 +86,14 @@
virtual MediaBufferBase *clone();
// sum of localRefcount() and remoteRefcount()
+ // Result should be treated as approximate unless the result precludes concurrent accesses.
virtual int refcount() const {
return localRefcount() + remoteRefcount();
}
+ // Result should be treated as approximate unless the result precludes concurrent accesses.
virtual int localRefcount() const {
- return mRefCount;
+ return mRefCount.load(std::memory_order_relaxed);
}
virtual int remoteRefcount() const {
@@ -146,7 +148,7 @@
void claim();
MediaBufferObserver *mObserver;
- int mRefCount;
+ std::atomic<int> mRefCount;
void *mData;
size_t mSize, mRangeOffset, mRangeLength;