commit 92e5107a0668811729d4e06c94f9cf83b1b6629e
author Dichen Zhang <dichenzhang@google.com> Wed Jun 08 18:26:37 2022 -0700
committer Dichen Zhang <dichenzhang@google.com> Fri Jun 10 11:53:55 2022 -0700
tree 89bc799d834c6651ee6b99e392d959cafd5121bb
parent 87a4f272af378e789d67cfd48a3e8c652069ac6f

Keep mRetriever thread-save

Bug: b/233279751
Test: AImageDecoderTest
Change-Id: Iffe27b467f0a4d3bfe3d92e3ea8e660cd0db1ef4

media/libheif/HeifDecoderImpl.cpp

diff --git a/media/libheif/HeifDecoderImpl.cpp b/media/libheif/HeifDecoderImpl.cpp
index 923f5c1..691eede 100644
--- a/media/libheif/HeifDecoderImpl.cpp
+++ b/media/libheif/HeifDecoderImpl.cpp
@@ -344,19 +344,22 @@
     mFrameDecoded = false;
     mFrameMemory.clear();
 
-    mRetriever = new MediaMetadataRetriever();
-    status_t err = mRetriever->setDataSource(mDataSource, "image/heif");
+    sp<MediaMetadataRetriever> retriever = new MediaMetadataRetriever();
+    status_t err = retriever->setDataSource(mDataSource, "image/heif");
     if (err != OK) {
         ALOGE("failed to set data source!");
-
         mRetriever.clear();
         mDataSource.clear();
         return false;
     }
+    {
+        Mutex::Autolock _l(mRetrieverLock);
+        mRetriever = retriever;
+    }
     ALOGV("successfully set data source.");
 
-    const char* hasImage = mRetriever->extractMetadata(METADATA_KEY_HAS_IMAGE);
-    const char* hasVideo = mRetriever->extractMetadata(METADATA_KEY_HAS_VIDEO);
+    const char* hasImage = retriever->extractMetadata(METADATA_KEY_HAS_IMAGE);
+    const char* hasVideo = retriever->extractMetadata(METADATA_KEY_HAS_VIDEO);
 
     mHasImage = hasImage && !strcasecmp(hasImage, "yes");
     mHasVideo = hasVideo && !strcasecmp(hasVideo, "yes");
@@ -364,7 +367,7 @@
     HeifFrameInfo* defaultInfo = nullptr;
     if (mHasImage) {
         // image index < 0 to retrieve primary image
-        sp<IMemory> sharedMem = mRetriever->getImageAtIndex(
+        sp<IMemory> sharedMem = retriever->getImageAtIndex(
                 -1, mOutputColor, true /*metaOnly*/);
 
         if (sharedMem == nullptr || sharedMem->unsecurePointer() == nullptr) {
@@ -399,7 +402,7 @@
     }
 
     if (mHasVideo) {
-        sp<IMemory> sharedMem = mRetriever->getFrameAtTime(0,
+        sp<IMemory> sharedMem = retriever->getFrameAtTime(0,
                 MediaSource::ReadOptions::SEEK_PREVIOUS_SYNC,
                 mOutputColor, true /*metaOnly*/);
 
@@ -425,7 +428,7 @@
 
         initFrameInfo(&mSequenceInfo, videoFrame);
 
-        const char* frameCount = mRetriever->extractMetadata(METADATA_KEY_VIDEO_FRAME_COUNT);
+        const char* frameCount = retriever->extractMetadata(METADATA_KEY_VIDEO_FRAME_COUNT);
         if (frameCount == nullptr) {
             android_errorWriteWithInfoLog(0x534e4554, "215002587", -1, NULL, 0);
             ALOGD("No valid sequence information in metadata");
@@ -511,14 +514,27 @@
 }
 
 bool HeifDecoderImpl::decodeAsync() {
+    wp<MediaMetadataRetriever> weakRetriever;
+    {
+        Mutex::Autolock _l(mRetrieverLock);
+        weakRetriever = mRetriever;
+        mRetriever.clear();
+    }
+
     for (size_t i = 1; i < mNumSlices; i++) {
+        sp<MediaMetadataRetriever> retriever = weakRetriever.promote();
+        if (retriever == nullptr) {
+            return false;
+        }
+
         ALOGV("decodeAsync(): decoding slice %zu", i);
         size_t top = i * mSliceHeight;
         size_t bottom = (i + 1) * mSliceHeight;
         if (bottom > mImageInfo.mHeight) {
             bottom = mImageInfo.mHeight;
         }
-        sp<IMemory> frameMemory = mRetriever->getImageRectAtIndex(
+
+        sp<IMemory> frameMemory = retriever->getImageRectAtIndex(
                 -1, mOutputColor, 0, top, mImageInfo.mWidth, bottom);
         {
             Mutex::Autolock autolock(mLock);
@@ -534,9 +550,6 @@
             mScanlineReady.signal();
         }
     }
-    // Aggressive clear to avoid holding on to resources
-    mRetriever.clear();
-
     // Hold on to mDataSource in case the client wants to redecode.
     return false;
 }
@@ -549,6 +562,17 @@
         return true;
     }
 
+    sp<MediaMetadataRetriever> retriever;
+    {
+        Mutex::Autolock _l(mRetrieverLock);
+        if (mRetriever == nullptr) {
+            ALOGE("Failed to get MediaMetadataRetriever!");
+            return false;
+        }
+
+        retriever = mRetriever;
+    }
+
     // See if we want to decode in slices to allow client to start
     // scanline processing in parallel with decode. If this fails
     // we fallback to decoding the full frame.
@@ -563,7 +587,7 @@
 
         if (mNumSlices > 1) {
             // get first slice and metadata
-            sp<IMemory> frameMemory = mRetriever->getImageRectAtIndex(
+            sp<IMemory> frameMemory = retriever->getImageRectAtIndex(
                     -1, mOutputColor, 0, 0, mImageInfo.mWidth, mSliceHeight);
 
             if (frameMemory == nullptr || frameMemory->unsecurePointer() == nullptr) {
@@ -598,9 +622,9 @@
 
     if (mHasImage) {
         // image index < 0 to retrieve primary image
-        mFrameMemory = mRetriever->getImageAtIndex(-1, mOutputColor);
+        mFrameMemory = retriever->getImageAtIndex(-1, mOutputColor);
     } else if (mHasVideo) {
-        mFrameMemory = mRetriever->getFrameAtTime(0,
+        mFrameMemory = retriever->getFrameAtTime(0,
                 MediaSource::ReadOptions::SEEK_PREVIOUS_SYNC, mOutputColor);
     }
 
@@ -658,7 +682,17 @@
     // set total scanline to sequence height now
     mTotalScanline = mSequenceInfo.mHeight;
 
-    mFrameMemory = mRetriever->getFrameAtIndex(frameIndex, mOutputColor);
+    sp<MediaMetadataRetriever> retriever;
+    {
+        Mutex::Autolock _l(mRetrieverLock);
+        retriever = mRetriever;
+        if (retriever == nullptr) {
+            ALOGE("failed to get MediaMetadataRetriever!");
+            return false;
+        }
+    }
+
+    mFrameMemory = retriever->getFrameAtIndex(frameIndex, mOutputColor);
     if (mFrameMemory == nullptr || mFrameMemory->unsecurePointer() == nullptr) {
         ALOGE("decode: videoFrame is a nullptr");
         return false;