Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit a1b4d0fd authored by Hans Boehm's avatar Hans Boehm
Browse files

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.
Merged-In: I3cb65e9f880588abdb0712890e7a654311577b07
Change-Id: I3cb65e9f880588abdb0712890e7a654311577b07
(cherry picked from commit b421f6ea)
parent 71a2d52f

media/libmediaextractor/MediaBuffer.cpp

+7 −7
Original line number Diff line number Diff line
@@ -39,7 +39,7 @@ MediaBuffer::MediaBuffer(void *data, size_t size) 
      mRangeOffset(0),

      mRangeLength(size),

      mOwnsData(false),

      mMetaData(new MetaData),

      mMetaData(new MetaDataBase),

      mOriginal(NULL) {

}
@@ -51,7 +51,7 @@ MediaBuffer::MediaBuffer(size_t size) 
      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 @@ MediaBuffer::MediaBuffer(const sp<ABuffer> &buffer) 
      mRangeLength(mSize),

      mBuffer(buffer),

      mOwnsData(false),

      mMetaData(new MetaData),

      mMetaData(new MetaDataBase),

      mOriginal(NULL) {

}
@@ -96,7 +96,7 @@ void MediaBuffer::release() { 
        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::release() { 


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 {

media/libmediaextractor/include/media/stagefright/MediaBuffer.h

+4 −2
Original line number Diff line number Diff line
@@ -86,12 +86,14 @@ public: 
    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 @@ private: 
    void claim();



    MediaBufferObserver *mObserver;

    int mRefCount;

    std::atomic<int> mRefCount;



    void *mData;

    size_t mSize, mRangeOffset, mRangeLength;