Skip to content

Commit

Permalink
libstagefright_wfd: video encoder does not actually release MediaBuff…
Browse files Browse the repository at this point in the history
…erBase when done

* This fixes buffer flow SurfaceMediaSource -> MediaPuller -> Converted
  freezing at mMediaBuffersAvailableCondition.wait(), due to this
  condition never being broadcast. This was supposed to happen from within
  SurfaceMediaSource::signalBufferReturned(), but this was never called.
  The Converter class does feedEncoderInputBuffers(), and after the
  encoder does its job, it should return the video buffer to the
  SurfaceMediaSource in ACodec::BaseState::onOMXEmptyBufferDone().
* There (in ACodec class), the code for doing that used to be:

    // We're in "store-metadata-in-buffers" mode, the underlying
    // OMX component had access to data that's implicitly refcounted
    // by this "MediaBuffer" object. Now that the OMX component has
    // told us that it's done with the input buffer, we can decrement
    // the mediaBuffer's reference count.
    info->mData->setMediaBufferBase(NULL);

  This means that if there was already a MediaBufferBase assigned to
  this mediaBuffer, then it got released when explicitly setting it to NULL:

  void MediaCodecBuffer::setMediaBufferBase(MediaBufferBase *mediaBuffer) {
      if (mMediaBufferBase != NULL) {
          mMediaBufferBase->release();
      }
      mMediaBufferBase = mediaBuffer;
  }

  Then in MediaBuffer::release(), which is a subclass of
  MediaBufferBase, there is code that does

        mObserver->signalBufferReturned(this);

  This should have went on to call SurfaceMediaSource::signalBufferReturned(),
  as it was setting itself as observer on the buffers sent to the video
  encoder. Stay tuned to find out why the call path was broken.

* Now, after Mr. Dongwon Kang's commit
  "f03606d9 Move MediaBufferXXX from foundation to libmediaextractor",
  the setMediaBufferBase and getMediaBufferBase functions no longer
  exist, and reference counting on MediaBuffer's is different.
  The direct replacement of setMediaBufferBase(mbuf) is now
  meta()->setObject("mediaBufferHolder", new MediaBufferHolder(mbuf)).
  The reference counting seems to now be managed through the constructor
  and destructor of this new MediaBufferHolder class (the code for
  release() is now in the holder's destructor). Now the issue seems to
  be that the lifetime of these new MediaBufferHolder's is not quite
  what it should be, because their destructor never gets called, hence
  the buffers never get returned.

* This might be an API problem that Mr. Dongwon Kang himself acknowledged,
  since in the aforementioned patch, he forcefully called mbuf->release()
  right below a comment where it clearly said that "video encoder will
  release MediaBuffer when done with underlying data":

  https://android.googlesource.com/platform/frameworks/av/+/f03606d9034730bea1a394e6803f9ebc36f3d2eb%5E%21/#F13

* Without addressing the root cause of the issue, in this commit we are
  simply mirroring a workaround for what appears to be broken media
  buffer reference counting.

Change-Id: Ie540e6dcf5536f93091ced2af2e121b71f70bb83
Signed-off-by: Vladimir Oltean <[email protected]>
Signed-off-by: DennySPb <[email protected]>
Signed-off-by: Mayur <[email protected]>
  • Loading branch information
vladimiroltean authored and marshmello61 committed Jun 28, 2021
1 parent 19d1b03 commit e2ff81b
Showing 1 changed file with 2 additions and 0 deletions.
2 changes: 2 additions & 0 deletions media/libstagefright/wifi-display/source/MediaPuller.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,8 @@ void MediaPuller::onMessageReceived(const sp<AMessage> &msg) {
// video encoder will release MediaBufferBase when done
// with underlying data.
accessUnit->meta()->setObject("mediaBufferHolder", new MediaBufferHolder(mbuf));
mbuf->release();
mbuf = NULL;
}

sp<AMessage> notify = mNotify->dup();
Expand Down

0 comments on commit e2ff81b

Please sign in to comment.