Bug 1743638 [Linux] Make VideoFramePool thread safe r=alwu,media-playback-reviewers

Make VideoFramePool thread safe to avoid multiple access during software decode to DMABuf:

- Create Mutex for VideoFramePool access
- Mark surface as used when it's provided by VideoFramePool to avoid race conditions.

Differential Revision: https://phabricator.services.mozilla.com/D135557

dom/media/platforms/ffmpeg/FFmpegVideoDecoder.cpp

@@ -785,12 +785,12 @@ MediaResult FFmpegVideoDecoder<LIBAV_VER>::CreateImageVAAPI(
   }
 
   MOZ_ASSERT(mTaskQueue->IsOnCurrentThread());
-  auto surface = mVideoFramePool->GetVideoFrameSurface(vaDesc);
+  auto surface = mVideoFramePool->GetVideoFrameSurface(vaDesc, mCodecContext,
+                                                       mFrame, mLib);
   if (!surface) {
     return MediaResult(NS_ERROR_OUT_OF_MEMORY,
                        RESULT_DETAIL("VAAPI dmabuf allocation error"));
   }
-  surface->LockVAAPIData(mCodecContext, mFrame, mLib);
   surface->SetYUVColorSpace(GetFrameColorSpace());
 
   if (mLib->av_frame_get_color_range) {

dom/media/platforms/ffmpeg/FFmpegVideoFramePool.cpp

@@ -23,6 +23,7 @@ VideoFrameSurfaceDMABuf::VideoFrameSurfaceDMABuf(DMABufSurface* aSurface)
   MOZ_ASSERT(mSurface);
   MOZ_RELEASE_ASSERT(mSurface->GetAsDMABufSurfaceYUV());
   mSurface->GlobalRefCountCreate();
+  mSurface->GlobalRefAdd();
   FFMPEG_LOG("VideoFrameSurfaceDMABuf: creating surface UID = %d",
              mSurface->GetUID());
 }
@@ -68,17 +69,23 @@ VideoFrameSurfaceVAAPI::~VideoFrameSurfaceVAAPI() {
   ReleaseVAAPIData(/* aForFrameRecycle */ false);
 }
 
-VideoFramePool::VideoFramePool(bool aUseVAAPI) : mUseVAAPI(aUseVAAPI) {}
+VideoFramePool::VideoFramePool(bool aUseVAAPI)
+    : mUseVAAPI(aUseVAAPI), mSurfaceLock("VideoFramePoolSurfaceLock") {}
 
-VideoFramePool::~VideoFramePool() { mDMABufSurfaces.Clear(); }
+VideoFramePool::~VideoFramePool() {
+  MutexAutoLock lock(mSurfaceLock);
+  mDMABufSurfaces.Clear();
+}
 
 void VideoFramePool::ReleaseUnusedVAAPIFrames() {
   if (!mUseVAAPI) {
     return;
   }
+  MutexAutoLock lock(mSurfaceLock);
   for (const auto& surface : mDMABufSurfaces) {
-    if (!surface->IsUsed()) {
-      surface->ReleaseVAAPIData();
+    auto* dmabufSurface = surface->AsVideoFrameSurfaceVAAPI();
+    if (!dmabufSurface->IsUsed()) {
+      dmabufSurface->ReleaseVAAPIData();
     }
   }
 }
@@ -86,7 +93,13 @@ void VideoFramePool::ReleaseUnusedVAAPIFrames() {
 RefPtr<VideoFrameSurface> VideoFramePool::GetFreeVideoFrameSurface() {
   int len = mDMABufSurfaces.Length();
   for (int i = 0; i < len; i++) {
-    if (!mDMABufSurfaces[i]->IsUsed()) {
+    auto* dmabufSurface = mDMABufSurfaces[i]->AsVideoFrameSurfaceDMABuf();
+    if (!dmabufSurface->IsUsed()) {
+      auto* vaapiSurface = dmabufSurface->AsVideoFrameSurfaceVAAPI();
+      if (vaapiSurface) {
+        vaapiSurface->ReleaseVAAPIData();
+      }
+      dmabufSurface->MarkAsUsed();
       return mDMABufSurfaces[i];
     }
   }
@@ -94,7 +107,8 @@ RefPtr<VideoFrameSurface> VideoFramePool::GetFreeVideoFrameSurface() {
 }
 
 RefPtr<VideoFrameSurface> VideoFramePool::GetVideoFrameSurface(
-    VADRMPRIMESurfaceDescriptor& aVaDesc) {
+    VADRMPRIMESurfaceDescriptor& aVaDesc, AVCodecContext* aAVCodecContext,
+    AVFrame* aAVFrame, FFmpegLibWrapper* aLib) {
   // VADRMPRIMESurfaceDescriptor can be used with VA-API only.
   MOZ_ASSERT(mUseVAAPI);
 
@@ -104,6 +118,7 @@ RefPtr<VideoFrameSurface> VideoFramePool::GetVideoFrameSurface(
     return nullptr;
   }
 
+  MutexAutoLock lock(mSurfaceLock);
   auto videoSurface = GetFreeVideoFrameSurface();
   if (!videoSurface) {
     RefPtr<DMABufSurfaceYUV> surface =
@@ -114,17 +129,18 @@ RefPtr<VideoFrameSurface> VideoFramePool::GetVideoFrameSurface(
     FFMPEG_LOG("Created new VA-API DMABufSurface UID = %d", surface->GetUID());
     videoSurface = new VideoFrameSurfaceVAAPI(surface);
     mDMABufSurfaces.AppendElement(videoSurface);
-    return videoSurface;
+  } else {
+    RefPtr<DMABufSurfaceYUV> surface = videoSurface->GetDMABufSurface();
+    if (!surface->UpdateYUVData(aVaDesc)) {
+      return nullptr;
+    }
+    FFMPEG_LOG("Reusing VA-API DMABufSurface UID = %d", surface->GetUID());
   }
 
-  // Release VAAPI surface data before we reuse it.
-  videoSurface->ReleaseVAAPIData();
-
-  RefPtr<DMABufSurfaceYUV> surface = videoSurface->GetDMABufSurface();
-  if (!surface->UpdateYUVData(aVaDesc)) {
-    return nullptr;
+  auto vaapiSurface = videoSurface->AsVideoFrameSurfaceVAAPI();
+  if (vaapiSurface) {
+    vaapiSurface->LockVAAPIData(aAVCodecContext, aAVFrame, aLib);
   }
-  FFMPEG_LOG("Reusing VA-API DMABufSurface UID = %d", surface->GetUID());
   return videoSurface;
 }
 
@@ -139,6 +155,7 @@ RefPtr<VideoFrameSurface> VideoFramePool::GetVideoFrameSurface(
     return nullptr;
   }
 
+  MutexAutoLock lock(mSurfaceLock);
   auto videoSurface = GetFreeVideoFrameSurface();
   if (!videoSurface) {
     RefPtr<DMABufSurfaceYUV> surface = DMABufSurfaceYUV::CreateYUVSurface(

dom/media/platforms/ffmpeg/FFmpegVideoFramePool.h

@@ -16,22 +16,25 @@
 
 namespace mozilla {
 
+class VideoFramePool;
+
 class VideoFrameSurface {
  public:
   NS_INLINE_DECL_THREADSAFE_REFCOUNTING(VideoFrameSurface)
 
-  VideoFrameSurface(){};
+  VideoFrameSurface() = default;
 
-  virtual void LockVAAPIData(AVCodecContext* aAVCodecContext, AVFrame* aAVFrame,
-                             FFmpegLibWrapper* aLib){};
-  virtual void ReleaseVAAPIData(bool aForFrameRecycle = true){};
-  virtual bool IsUsed() const = 0;
+  virtual class VideoFrameSurfaceDMABuf* AsVideoFrameSurfaceDMABuf() {
+    return nullptr;
+  }
+  virtual class VideoFrameSurfaceVAAPI* AsVideoFrameSurfaceVAAPI() {
+    return nullptr;
+  }
 
   virtual void SetYUVColorSpace(mozilla::gfx::YUVColorSpace aColorSpace) = 0;
   virtual void SetColorRange(mozilla::gfx::ColorRange aColorRange) = 0;
 
   virtual RefPtr<DMABufSurfaceYUV> GetDMABufSurface() { return nullptr; };
-
   virtual RefPtr<layers::Image> GetAsImage() = 0;
 
   // Don't allow VideoFrameSurface plain copy as it leads to
@@ -47,17 +50,18 @@ class VideoFrameSurface {
 // VideoFrameSurfaceDMABuf is YUV dmabuf surface used for SW video decoding.
 // Stores decoded video data in GPU memory.
 class VideoFrameSurfaceDMABuf : public VideoFrameSurface {
+  friend class VideoFramePool;
+
  public:
   explicit VideoFrameSurfaceDMABuf(DMABufSurface* aSurface);
 
-  // Check if DMABufSurface is used by any gecko rendering process
-  // (WebRender or GL compositor) or by DMABUFSurfaceImage/VideoData.
-  bool IsUsed() const { return mSurface->IsGlobalRefSet(); }
+  class VideoFrameSurfaceDMABuf* AsVideoFrameSurfaceDMABuf() {
+    return this;
+  }
 
   void SetYUVColorSpace(mozilla::gfx::YUVColorSpace aColorSpace) {
     mSurface->GetAsDMABufSurfaceYUV()->SetYUVColorSpace(aColorSpace);
   }
-
   void SetColorRange(mozilla::gfx::ColorRange aColorRange) {
     mSurface->GetAsDMABufSurfaceYUV()->SetColorRange(aColorRange);
   }
@@ -69,9 +73,14 @@ class VideoFrameSurfaceDMABuf : public VideoFrameSurface {
   RefPtr<layers::Image> GetAsImage();
 
  protected:
-  const RefPtr<DMABufSurface> mSurface;
+  // Check if DMABufSurface is used by any gecko rendering process
+  // (WebRender or GL compositor) or by DMABUFSurfaceImage/VideoData.
+  bool IsUsed() const { return mSurface->IsGlobalRefSet(); }
+  void MarkAsUsed() { mSurface->GlobalRefAdd(); }
 
  protected:
+  const RefPtr<DMABufSurface> mSurface;
+
   ~VideoFrameSurfaceDMABuf(){};
 };
 
@@ -104,18 +113,23 @@ class VideoFrameSurfaceDMABuf : public VideoFrameSurface {
 // Unfortunately there isn't any obvious way how to mark particular VASurface
 // as used. The best we can do is to hold a reference to particular AVBuffer
 // from decoded AVFrame and AVHWFramesContext which owns the AVBuffer.
-
 class VideoFrameSurfaceVAAPI : public VideoFrameSurfaceDMABuf {
+  friend class VideoFramePool;
+
  public:
   explicit VideoFrameSurfaceVAAPI(DMABufSurface* aSurface);
 
+  virtual class VideoFrameSurfaceVAAPI* AsVideoFrameSurfaceVAAPI() {
+    return this;
+  }
+
+ protected:
   // Lock VAAPI related data
   void LockVAAPIData(AVCodecContext* aAVCodecContext, AVFrame* aAVFrame,
                      FFmpegLibWrapper* aLib);
-
   // Release VAAPI related data, DMABufSurface can be reused
   // for another frame.
-  void ReleaseVAAPIData(bool aForFrameRecycle);
+  void ReleaseVAAPIData(bool aForFrameRecycle = true);
 
  private:
   ~VideoFrameSurfaceVAAPI();
@@ -125,13 +139,15 @@ class VideoFrameSurfaceVAAPI : public VideoFrameSurfaceDMABuf {
   AVBufferRef* mHWAVBuffer;
 };
 
+// VideoFramePool class is thread-safe.
 class VideoFramePool final {
  public:
   explicit VideoFramePool(bool aUseVAAPI);
   ~VideoFramePool();
 
   RefPtr<VideoFrameSurface> GetVideoFrameSurface(
-      VADRMPRIMESurfaceDescriptor& aVaDesc);
+      VADRMPRIMESurfaceDescriptor& aVaDesc, AVCodecContext* aAVCodecContext,
+      AVFrame* aAVFrame, FFmpegLibWrapper* aLib);
   RefPtr<VideoFrameSurface> GetVideoFrameSurface(AVPixelFormat aPixelFormat,
                                                  AVFrame* aFrame);
   void ReleaseUnusedVAAPIFrames();
@@ -141,6 +157,8 @@ class VideoFramePool final {
 
  private:
   const bool mUseVAAPI;
+  // Protect mDMABufSurfaces pool access
+  mozilla::Mutex mSurfaceLock;
   nsTArray<RefPtr<VideoFrameSurface>> mDMABufSurfaces;
 };
 

gfx/layers/DMABUFSurfaceImage.cpp

@@ -28,10 +28,14 @@ using namespace mozilla::gl;
 
 DMABUFSurfaceImage::DMABUFSurfaceImage(DMABufSurface* aSurface)
     : Image(nullptr, ImageFormat::DMABUF), mSurface(aSurface) {
-  mSurface->GlobalRefAdd();
+  MOZ_DIAGNOSTIC_ASSERT(mSurface->IsGlobalRefSet(),
+                        "DMABufSurface must be marked as used!");
 }
 
-DMABUFSurfaceImage::~DMABUFSurfaceImage() { mSurface->GlobalRefRelease(); }
+DMABUFSurfaceImage::~DMABUFSurfaceImage() {
+  // Unref as we're done with this surface.
+  mSurface->GlobalRefRelease();
+}
 
 StaticRefPtr<GLContext> sSnapshotContext;
 static StaticMutex sSnapshotContextMutex;