Bug 1160914 - Make gmp-clearkey's decoders threadsafe refcounted, to handle DecodingComplete while GMPVideoHost::CreateFrame() is waiting. r=edwin

media/gmp-clearkey/0.1/AudioDecoder.cpp

@@ -32,6 +32,8 @@ AudioDecoder::AudioDecoder(GMPAudioHost *aHostAPI)
   , mNumInputTasks(0)
   , mHasShutdown(false)
 {
+  // We drop the ref in DecodingComplete().
+  AddRef();
 }
 
 AudioDecoder::~AudioDecoder()
@@ -84,9 +86,9 @@ AudioDecoder::Decode(GMPAudioSamples* aInput)
     AutoLock lock(mMutex);
     mNumInputTasks++;
   }
-  mWorkerThread->Post(WrapTask(this,
-                               &AudioDecoder::DecodeTask,
-                               aInput));
+  mWorkerThread->Post(WrapTaskRefCounted(this,
+                                         &AudioDecoder::DecodeTask,
+                                         aInput));
 }
 
 void
@@ -258,8 +260,8 @@ AudioDecoder::Drain()
     return;
   }
   EnsureWorker();
-  mWorkerThread->Post(WrapTask(this,
-                               &AudioDecoder::DrainTask));
+  mWorkerThread->Post(WrapTaskRefCounted(this,
+                                         &AudioDecoder::DrainTask));
 }
 
 void
@@ -270,24 +272,19 @@ AudioDecoder::DecodingComplete()
   }
   mHasShutdown = true;
 
-  // Worker thread might have dispatched more tasks to the main thread that need this object.
-  // Append another task to delete |this|.
-  GetPlatform()->runonmainthread(WrapTask(this, &AudioDecoder::Destroy));
+  // Release the reference we added in the constructor. There may be
+  // WrapRefCounted tasks that also hold references to us, and keep
+  // us alive a little longer.
+  Release();
 }
 
 void
-AudioDecoder::Destroy()
-{
-  delete this;
-}
-
-void
-AudioDecoder::MaybeRunOnMainThread(gmp_task_args_base* aTask)
+AudioDecoder::MaybeRunOnMainThread(GMPTask* aTask)
 {
   class MaybeRunTask : public GMPTask
   {
   public:
-    MaybeRunTask(AudioDecoder* aDecoder, gmp_task_args_base* aTask)
+    MaybeRunTask(AudioDecoder* aDecoder, GMPTask* aTask)
       : mDecoder(aDecoder), mTask(aTask)
     { }
 
@@ -307,8 +304,8 @@ AudioDecoder::MaybeRunOnMainThread(gmp_task_args_base* aTask)
     }
 
   private:
-    AudioDecoder* mDecoder;
-    gmp_task_args_base* mTask;
+    RefPtr<AudioDecoder> mDecoder;
+    GMPTask* mTask;
   };
 
   GetPlatform()->runonmainthread(new MaybeRunTask(this, aTask));

media/gmp-clearkey/0.1/AudioDecoder.h

@@ -21,16 +21,16 @@
 #include "gmp-audio-host.h"
 #include "gmp-task-utils.h"
 #include "WMFAACDecoder.h"
+#include "RefCounted.h"
 
 #include "mfobjects.h"
 
 class AudioDecoder : public GMPAudioDecoder
+                   , public RefCounted
 {
 public:
   AudioDecoder(GMPAudioHost *aHostAPI);
 
-  virtual ~AudioDecoder();
-
   virtual void InitDecode(const GMPAudioCodec& aCodecSettings,
                           GMPAudioDecoderCallback* aCallback) override;
 
@@ -45,6 +45,7 @@ public:
   bool HasShutdown() { return mHasShutdown; }
 
 private:
+  virtual ~AudioDecoder();
 
   void EnsureWorker();
 
@@ -56,8 +57,7 @@ private:
   HRESULT MFToGMPSample(IMFSample* aSample,
                         GMPAudioSamples* aAudioFrame);
 
-  void MaybeRunOnMainThread(gmp_task_args_base* aTask);
-  void Destroy();
+  void MaybeRunOnMainThread(GMPTask* aTask);
 
   GMPAudioHost *mHostAPI; // host-owned, invalid at DecodingComplete
   GMPAudioDecoderCallback* mCallback; // host-owned, invalid at DecodingComplete

media/gmp-clearkey/0.1/ClearKeySessionManager.cpp

@@ -48,7 +48,6 @@ ClearKeySessionManager::ClearKeySessionManager()
 ClearKeySessionManager::~ClearKeySessionManager()
 {
   CK_LOGD("ClearKeySessionManager dtor %p", this);
-   assert(!mRefCount);
 }
 
 static bool
@@ -387,9 +386,9 @@ ClearKeySessionManager::Decrypt(GMPBuffer* aBuffer,
     return;
   }
 
-  mThread->Post(WrapTask(this,
-                         &ClearKeySessionManager::DoDecrypt,
-                         aBuffer, aMetadata));
+  mThread->Post(WrapTaskRefCounted(this,
+                                   &ClearKeySessionManager::DoDecrypt,
+                                   aBuffer, aMetadata));
 }
 
 void

media/gmp-clearkey/0.1/RefCounted.h

@@ -18,8 +18,10 @@
 #define __RefCount_h__
 
 #include <stdint.h>
+#include <atomic>
+#include <assert.h>
 
-// Note: Not thread safe!
+// Note: Thread safe.
 class RefCounted {
 public:
   void AddRef() {
@@ -41,8 +43,9 @@ protected:
   }
   virtual ~RefCounted()
   {
+    assert(!mRefCount);
   }
-  uint32_t mRefCount;
+  std::atomic<uint32_t> mRefCount;
 };
 
 template<class T>

media/gmp-clearkey/0.1/VideoDecoder.cpp

@@ -35,6 +35,8 @@ VideoDecoder::VideoDecoder(GMPVideoHost *aHostAPI)
   , mSentExtraData(false)
   , mHasShutdown(false)
 {
+  // We drop the ref in DecodingComplete().
+  AddRef();
 }
 
 VideoDecoder::~VideoDecoder()
@@ -112,9 +114,9 @@ VideoDecoder::Decode(GMPVideoEncodedFrame* aInputFrame,
   // Note: we don't need the codec specific info on a per-frame basis.
   // It's mostly useful for WebRTC use cases.
 
-  mWorkerThread->Post(WrapTask(this,
-                               &VideoDecoder::DecodeTask,
-                               aInputFrame));
+  mWorkerThread->Post(WrapTaskRefCounted(this,
+                                         &VideoDecoder::DecodeTask,
+                                         aInputFrame));
 }
 
 class AutoReleaseVideoFrame {
@@ -197,12 +199,12 @@ VideoDecoder::DecodeTask(GMPVideoEncodedFrame* aInput)
     CK_LOGD("VideoDecoder::DecodeTask() output ret=0x%x\n", hr);
     if (hr == S_OK) {
       MaybeRunOnMainThread(
-        WrapTask(this,
-                 &VideoDecoder::ReturnOutput,
-                 CComPtr<IMFSample>(output),
-                 mDecoder->GetFrameWidth(),
-                 mDecoder->GetFrameHeight(),
-                 mDecoder->GetStride()));
+        WrapTaskRefCounted(this,
+                           &VideoDecoder::ReturnOutput,
+                           CComPtr<IMFSample>(output),
+                           mDecoder->GetFrameWidth(),
+                           mDecoder->GetFrameHeight(),
+                           mDecoder->GetStride()));
     }
     if (hr == MF_E_TRANSFORM_NEED_MORE_INPUT) {
       AutoLock lock(mMutex);
@@ -232,11 +234,20 @@ VideoDecoder::ReturnOutput(IMFSample* aSample,
   HRESULT hr;
 
   GMPVideoFrame* f = nullptr;
-  mHostAPI->CreateFrame(kGMPI420VideoFrame, &f);
-  if (!f) {
+  auto err = mHostAPI->CreateFrame(kGMPI420VideoFrame, &f);
+  if (GMP_FAILED(err) || !f) {
     CK_LOGE("Failed to create i420 frame!\n");
     return;
   }
+  if (HasShutdown()) {
+    // Note: GMPVideoHost::CreateFrame() can process messages before returning,
+    // including a message that calls VideoDecoder::DecodingComplete(), i.e.
+    // we can shutdown during the call!
+    CK_LOGD("Shutdown while waiting on GMPVideoHost::CreateFrame()!\n");
+    f->Destroy();
+    return;
+  }
+
   auto vf = static_cast<GMPVideoi420Frame*>(f);
 
   hr = SampleToVideoFrame(aSample, aWidth, aHeight, aStride, vf);
@@ -291,9 +302,10 @@ VideoDecoder::SampleToVideoFrame(IMFSample* aSample,
   int32_t halfStride = (stride + 1) / 2;
   int32_t halfHeight = (aHeight + 1) / 2;
 
-  aVideoFrame->CreateEmptyFrame(stride, aHeight, stride, halfStride, halfStride);
+  auto err = aVideoFrame->CreateEmptyFrame(stride, aHeight, stride, halfStride, halfStride);
+  ENSURE(GMP_SUCCEEDED(err), E_FAIL);
 
-  auto err = aVideoFrame->SetWidth(aWidth);
+  err = aVideoFrame->SetWidth(aWidth);
   ENSURE(GMP_SUCCEEDED(err), E_FAIL);
   err = aVideoFrame->SetHeight(aHeight);
   ENSURE(GMP_SUCCEEDED(err), E_FAIL);
@@ -355,12 +367,12 @@ VideoDecoder::DrainTask()
     CK_LOGD("VideoDecoder::DrainTask() output ret=0x%x\n", hr);
     if (hr == S_OK) {
       MaybeRunOnMainThread(
-        WrapTask(this,
-                 &VideoDecoder::ReturnOutput,
-                 CComPtr<IMFSample>(output),
-                 mDecoder->GetFrameWidth(),
-                 mDecoder->GetFrameHeight(),
-                 mDecoder->GetStride()));
+        WrapTaskRefCounted(this,
+                           &VideoDecoder::ReturnOutput,
+                           CComPtr<IMFSample>(output),
+                           mDecoder->GetFrameWidth(),
+                           mDecoder->GetFrameHeight(),
+                           mDecoder->GetStride()));
     }
   }
   MaybeRunOnMainThread(WrapTask(mCallback, &GMPVideoDecoderCallback::DrainComplete));
@@ -376,8 +388,8 @@ VideoDecoder::Drain()
     return;
   }
   EnsureWorker();
-  mWorkerThread->Post(WrapTask(this,
-                               &VideoDecoder::DrainTask));
+  mWorkerThread->Post(WrapTaskRefCounted(this,
+                                         &VideoDecoder::DrainTask));
 }
 
 void
@@ -388,24 +400,19 @@ VideoDecoder::DecodingComplete()
   }
   mHasShutdown = true;
 
-  // Worker thread might have dispatched more tasks to the main thread that need this object.
-  // Append another task to delete |this|.
-  GetPlatform()->runonmainthread(WrapTask(this, &VideoDecoder::Destroy));
+  // Release the reference we added in the constructor. There may be
+  // WrapRefCounted tasks that also hold references to us, and keep
+  // us alive a little longer.
+  Release();
 }
 
 void
-VideoDecoder::Destroy()
-{
-  delete this;
-}
-
-void
-VideoDecoder::MaybeRunOnMainThread(gmp_task_args_base* aTask)
+VideoDecoder::MaybeRunOnMainThread(GMPTask* aTask)
 {
   class MaybeRunTask : public GMPTask
   {
   public:
-    MaybeRunTask(VideoDecoder* aDecoder, gmp_task_args_base* aTask)
+    MaybeRunTask(VideoDecoder* aDecoder, GMPTask* aTask)
       : mDecoder(aDecoder), mTask(aTask)
     { }
 
@@ -425,8 +432,8 @@ VideoDecoder::MaybeRunOnMainThread(gmp_task_args_base* aTask)
     }
 
   private:
-    VideoDecoder* mDecoder;
-    gmp_task_args_base* mTask;
+    RefPtr<VideoDecoder> mDecoder;
+    GMPTask* mTask;
   };
 
   GetPlatform()->runonmainthread(new MaybeRunTask(this, aTask));

media/gmp-clearkey/0.1/VideoDecoder.h

@@ -25,12 +25,11 @@
 #include "mfobjects.h"
 
 class VideoDecoder : public GMPVideoDecoder
+                   , public RefCounted
 {
 public:
   VideoDecoder(GMPVideoHost *aHostAPI);
 
-  virtual ~VideoDecoder();
-
   virtual void InitDecode(const GMPVideoCodec& aCodecSettings,
                           const uint8_t* aCodecSpecific,
                           uint32_t aCodecSpecificLength,
@@ -53,6 +52,8 @@ public:
 
 private:
 
+  virtual ~VideoDecoder();
+
   void EnsureWorker();
 
   void DrainTask();
@@ -70,8 +71,7 @@ private:
                              int32_t aStride,
                              GMPVideoi420Frame* aVideoFrame);
 
-  void MaybeRunOnMainThread(gmp_task_args_base* aTask);
-  void Destroy();
+  void MaybeRunOnMainThread(GMPTask* aTask);
 
   GMPVideoHost *mHostAPI; // host-owned, invalid at DecodingComplete
   GMPVideoDecoderCallback* mCallback; // host-owned, invalid at DecodingComplete

media/gmp-clearkey/0.1/gmp-task-utils-generated.h

@@ -14,6 +14,7 @@
  * limitations under the License.
  */
 
+#include "RefCounted.h"
 
 // 0 arguments --
 template<typename M> class gmp_task_args_nm_0 : public gmp_task_args_base {
@@ -1908,3 +1909,30 @@ gmp_task_args_m_14_ret<C, M, A0, A1, A2, A3, A4, A5, A6, A7, A8, A9, A10, A11, A
     (o, m, a0, a1, a2, a3, a4, a5, a6, a7, a8, a9, a10, a11, a12, a13, r);
 }
 
+class RefCountTaskWrapper : public gmp_task_args_base {
+public:
+  RefCountTaskWrapper(GMPTask* aTask, RefCounted* aRefCounted)
+    : mTask(aTask)
+    , mRefCounted(aRefCounted)
+  {}
+  virtual void Run() override {
+    mTask->Run();
+  }
+  virtual void Destroy() {
+    mTask->Destroy();
+    gmp_task_args_base::Destroy();
+  }
+private:
+  ~RefCountTaskWrapper() {}
+
+  GMPTask* mTask;
+  RefPtr<RefCounted> mRefCounted;
+};
+
+template<typename Type, typename Method, typename... Args>
+GMPTask*
+WrapTaskRefCounted(Type* aType, Method aMethod, Args... args)
+{
+  GMPTask* t = WrapTask(aType, aMethod, args...);
+  return new RefCountTaskWrapper(t, aType);
+}