Bug 1160914 - Make gmp-clearkey's decoders threadsafe refcounted, to handle DecodingComplete while GMPVideoHost::CreateFrame() is waiting. r=edwin
☠☠ backed out by 000c1797e8a0 ☠ ☠
authorChris Pearce <cpearce@mozilla.com>
Tue, 05 May 2015 11:21:55 +1200
changeset 273688 ddb936f4e78a30118099566c9722fbc812251e32
parent 273687 1fa448752bd9c1cf0a8bb343db06faf81ea4b357
child 273689 000c1797e8a02f243d10efd739237a03fed0392b
push id863
push userraliiev@mozilla.com
push dateMon, 03 Aug 2015 13:22:43 +0000
treeherdermozilla-release@f6321b14228d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersedwin
bugs1160914
milestone40.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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
media/gmp-clearkey/0.1/AudioDecoder.h
media/gmp-clearkey/0.1/ClearKeySessionManager.cpp
media/gmp-clearkey/0.1/RefCounted.h
media/gmp-clearkey/0.1/VideoDecoder.cpp
media/gmp-clearkey/0.1/VideoDecoder.h
media/gmp-clearkey/0.1/gmp-task-utils-generated.h
--- a/media/gmp-clearkey/0.1/AudioDecoder.cpp
+++ b/media/gmp-clearkey/0.1/AudioDecoder.cpp
@@ -27,16 +27,18 @@ using namespace wmf;
 AudioDecoder::AudioDecoder(GMPAudioHost *aHostAPI)
   : mHostAPI(aHostAPI)
   , mCallback(nullptr)
   , mWorkerThread(nullptr)
   , mMutex(nullptr)
   , mNumInputTasks(0)
   , mHasShutdown(false)
 {
+  // We drop the ref in DecodingComplete().
+  AddRef();
 }
 
 AudioDecoder::~AudioDecoder()
 {
   if (mMutex) {
     mMutex->Destroy();
   }
 }
@@ -79,19 +81,19 @@ AudioDecoder::EnsureWorker()
 void
 AudioDecoder::Decode(GMPAudioSamples* aInput)
 {
   EnsureWorker();
   {
     AutoLock lock(mMutex);
     mNumInputTasks++;
   }
-  mWorkerThread->Post(WrapTask(this,
-                               &AudioDecoder::DecodeTask,
-                               aInput));
+  mWorkerThread->Post(WrapTaskRefCounted(this,
+                                         &AudioDecoder::DecodeTask,
+                                         aInput));
 }
 
 void
 AudioDecoder::DecodeTask(GMPAudioSamples* aInput)
 {
   HRESULT hr;
 
   {
@@ -253,46 +255,41 @@ AudioDecoder::DrainTask()
 
 void
 AudioDecoder::Drain()
 {
   if (!mDecoder) {
     return;
   }
   EnsureWorker();
-  mWorkerThread->Post(WrapTask(this,
-                               &AudioDecoder::DrainTask));
+  mWorkerThread->Post(WrapTaskRefCounted(this,
+                                         &AudioDecoder::DrainTask));
 }
 
 void
 AudioDecoder::DecodingComplete()
 {
   if (mWorkerThread) {
     mWorkerThread->Join();
   }
   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)
     { }
 
     virtual void Run(void) {
       if (mDecoder->HasShutdown()) {
         CK_LOGD("Trying to dispatch to main thread after AudioDecoder has shut down");
         return;
       }
@@ -302,14 +299,14 @@ AudioDecoder::MaybeRunOnMainThread(gmp_t
 
     virtual void Destroy()
     {
       mTask->Destroy();
       delete this;
     }
 
   private:
-    AudioDecoder* mDecoder;
-    gmp_task_args_base* mTask;
+    RefPtr<AudioDecoder> mDecoder;
+    GMPTask* mTask;
   };
 
   GetPlatform()->runonmainthread(new MaybeRunTask(this, aTask));
 }
--- a/media/gmp-clearkey/0.1/AudioDecoder.h
+++ b/media/gmp-clearkey/0.1/AudioDecoder.h
@@ -16,53 +16,53 @@
 
 #ifndef __AudioDecoder_h__
 #define __AudioDecoder_h__
 
 #include "gmp-audio-decode.h"
 #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;
 
   virtual void Decode(GMPAudioSamples* aEncodedSamples);
 
   virtual void Reset() override;
 
   virtual void Drain() override;
 
   virtual void DecodingComplete() override;
 
   bool HasShutdown() { return mHasShutdown; }
 
 private:
+  virtual ~AudioDecoder();
 
   void EnsureWorker();
 
   void DecodeTask(GMPAudioSamples* aEncodedSamples);
   void DrainTask();
 
   void ReturnOutput(IMFSample* aSample);
 
   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
   GMPThread* mWorkerThread;
   GMPMutex* mMutex;
   wmf::AutoPtr<wmf::WMFAACDecoder> mDecoder;
 
   int32_t mNumInputTasks;
--- a/media/gmp-clearkey/0.1/ClearKeySessionManager.cpp
+++ b/media/gmp-clearkey/0.1/ClearKeySessionManager.cpp
@@ -43,17 +43,16 @@ ClearKeySessionManager::ClearKeySessionM
     CK_LOGD("failed to create thread in clearkey cdm");
     mThread = nullptr;
   }
 }
 
 ClearKeySessionManager::~ClearKeySessionManager()
 {
   CK_LOGD("ClearKeySessionManager dtor %p", this);
-   assert(!mRefCount);
 }
 
 static bool
 ShouldBeAbleToDecode()
 {
 #if !defined(ENABLE_WMF)
   return false;
 #else
@@ -382,19 +381,19 @@ ClearKeySessionManager::Decrypt(GMPBuffe
   CK_LOGD("ClearKeySessionManager::Decrypt");
 
   if (!mThread) {
     CK_LOGW("No decrypt thread");
     mCallback->Decrypted(aBuffer, GMPGenericErr);
     return;
   }
 
-  mThread->Post(WrapTask(this,
-                         &ClearKeySessionManager::DoDecrypt,
-                         aBuffer, aMetadata));
+  mThread->Post(WrapTaskRefCounted(this,
+                                   &ClearKeySessionManager::DoDecrypt,
+                                   aBuffer, aMetadata));
 }
 
 void
 ClearKeySessionManager::DoDecrypt(GMPBuffer* aBuffer,
                                   GMPEncryptedBufferMetadata* aMetadata)
 {
   CK_LOGD("ClearKeySessionManager::DoDecrypt");
 
--- a/media/gmp-clearkey/0.1/RefCounted.h
+++ b/media/gmp-clearkey/0.1/RefCounted.h
@@ -13,18 +13,20 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
 
 #ifndef __RefCount_h__
 #define __RefCount_h__
 
 #include <stdint.h>
+#include <atomic>
+#include <assert.h>
 
-// Note: Not thread safe!
+// Note: Thread safe.
 class RefCounted {
 public:
   void AddRef() {
     ++mRefCount;
   }
 
   uint32_t Release() {
     uint32_t newCount = --mRefCount;
@@ -36,18 +38,19 @@ public:
 
 protected:
   RefCounted()
     : mRefCount(0)
   {
   }
   virtual ~RefCounted()
   {
+    assert(!mRefCount);
   }
-  uint32_t mRefCount;
+  std::atomic<uint32_t> mRefCount;
 };
 
 template<class T>
 class RefPtr {
 public:
   explicit RefPtr(T* aPtr) : mPtr(nullptr) {
     Assign(aPtr);
   }
--- a/media/gmp-clearkey/0.1/VideoDecoder.cpp
+++ b/media/gmp-clearkey/0.1/VideoDecoder.cpp
@@ -30,16 +30,18 @@ VideoDecoder::VideoDecoder(GMPVideoHost 
   : mHostAPI(aHostAPI)
   , mCallback(nullptr)
   , mWorkerThread(nullptr)
   , mMutex(nullptr)
   , mNumInputTasks(0)
   , mSentExtraData(false)
   , mHasShutdown(false)
 {
+  // We drop the ref in DecodingComplete().
+  AddRef();
 }
 
 VideoDecoder::~VideoDecoder()
 {
   if (mMutex) {
     mMutex->Destroy();
   }
 }
@@ -107,19 +109,19 @@ VideoDecoder::Decode(GMPVideoEncodedFram
   {
     AutoLock lock(mMutex);
     mNumInputTasks++;
   }
 
   // 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 {
 public:
   AutoReleaseVideoFrame(GMPVideoEncodedFrame* aFrame)
     : mFrame(aFrame)
   {
   }
@@ -192,22 +194,22 @@ VideoDecoder::DecodeTask(GMPVideoEncoded
   }
 
   while (hr == S_OK) {
     CComPtr<IMFSample> output;
     hr = mDecoder->Output(&output);
     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);
       if (mNumInputTasks == 0) {
         // We have run all input tasks. We *must* notify Gecko so that it will
         // send us more data.
         MaybeRunOnMainThread(
           WrapTask(mCallback,
@@ -227,21 +229,30 @@ VideoDecoder::ReturnOutput(IMFSample* aS
                            int32_t aStride)
 {
   CK_LOGD("[%p] VideoDecoder::ReturnOutput()\n", this);
   assert(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);
   ENSURE(SUCCEEDED(hr), /*void*/);
 
   mCallback->Decoded(vf);
 }
 
@@ -286,19 +297,20 @@ VideoDecoder::SampleToVideoFrame(IMFSamp
   if (aHeight % 16 != 0) {
     padding = 16 - (aHeight % 16);
   }
   int32_t y_size = stride * (aHeight + padding);
   int32_t v_size = stride * (aHeight + padding) / 4;
   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);
 
   uint8_t* outBuffer = aVideoFrame->Buffer(kGMPYPlane);
   ENSURE(outBuffer != nullptr, E_FAIL);
   assert(aVideoFrame->AllocatedSize(kGMPYPlane) >= stride*aHeight);
   memcpy(outBuffer, data, stride*aHeight);
@@ -350,67 +362,62 @@ VideoDecoder::DrainTask()
   // Return any pending output.
   HRESULT hr = S_OK;
   while (hr == S_OK) {
     CComPtr<IMFSample> output;
     hr = mDecoder->Output(&output);
     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));
 }
 
 void
 VideoDecoder::Drain()
 {
   if (!mDecoder) {
     if (mCallback) {
       mCallback->DrainComplete();
     }
     return;
   }
   EnsureWorker();
-  mWorkerThread->Post(WrapTask(this,
-                               &VideoDecoder::DrainTask));
+  mWorkerThread->Post(WrapTaskRefCounted(this,
+                                         &VideoDecoder::DrainTask));
 }
 
 void
 VideoDecoder::DecodingComplete()
 {
   if (mWorkerThread) {
     mWorkerThread->Join();
   }
   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)
     { }
 
     virtual void Run(void) {
       if (mDecoder->HasShutdown()) {
         CK_LOGD("Trying to dispatch to main thread after VideoDecoder has shut down");
         return;
       }
@@ -420,14 +427,14 @@ VideoDecoder::MaybeRunOnMainThread(gmp_t
 
     virtual void Destroy()
     {
       mTask->Destroy();
       delete this;
     }
 
   private:
-    VideoDecoder* mDecoder;
-    gmp_task_args_base* mTask;
+    RefPtr<VideoDecoder> mDecoder;
+    GMPTask* mTask;
   };
 
   GetPlatform()->runonmainthread(new MaybeRunTask(this, aTask));
 }
--- a/media/gmp-clearkey/0.1/VideoDecoder.h
+++ b/media/gmp-clearkey/0.1/VideoDecoder.h
@@ -20,22 +20,21 @@
 #include "gmp-task-utils.h"
 #include "gmp-video-decode.h"
 #include "gmp-video-host.h"
 #include "WMFH264Decoder.h"
 
 #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,
                           GMPVideoDecoderCallback* aCallback,
                           int32_t aCoreCount) override;
 
   virtual void Decode(GMPVideoEncodedFrame* aInputFrame,
                       bool aMissingFrames,
@@ -48,16 +47,18 @@ public:
   virtual void Drain() override;
 
   virtual void DecodingComplete() override;
 
   bool HasShutdown() { return mHasShutdown; }
 
 private:
 
+  virtual ~VideoDecoder();
+
   void EnsureWorker();
 
   void DrainTask();
 
   void DecodeTask(GMPVideoEncodedFrame* aInputFrame);
 
   void ReturnOutput(IMFSample* aSample,
                     int32_t aWidth,
@@ -65,18 +66,17 @@ private:
                     int32_t aStride);
 
   HRESULT SampleToVideoFrame(IMFSample* aSample,
                              int32_t aWidth,
                              int32_t aHeight,
                              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
   GMPThread* mWorkerThread;
   GMPMutex* mMutex;
   wmf::AutoPtr<wmf::WMFH264Decoder> mDecoder;
 
   std::vector<uint8_t> mExtraData;
--- a/media/gmp-clearkey/0.1/gmp-task-utils-generated.h
+++ b/media/gmp-clearkey/0.1/gmp-task-utils-generated.h
@@ -9,16 +9,17 @@
  *
  * Unless required by applicable law or agreed to in writing, software
  * distributed under the License is distributed on an "AS IS" BASIS,
  * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
 
+#include "RefCounted.h"
 
 // 0 arguments --
 template<typename M> class gmp_task_args_nm_0 : public gmp_task_args_base {
  public:
   explicit gmp_task_args_nm_0(M m) :
     m_(m)  {}
 
   void Run() {
@@ -1903,8 +1904,35 @@ gmp_task_args_m_14<C, M, A0, A1, A2, A3,
 
 // 14 arguments --
 template<typename C, typename M, typename A0, typename A1, typename A2, typename A3, typename A4, typename A5, typename A6, typename A7, typename A8, typename A9, typename A10, typename A11, typename A12, typename A13, typename R>
 gmp_task_args_m_14_ret<C, M, A0, A1, A2, A3, A4, A5, A6, A7, A8, A9, A10, A11, A12, A13, R>* WrapTaskRet(C o, M m, A0 a0, A1 a1, A2 a2, A3 a3, A4 a4, A5 a5, A6 a6, A7 a7, A8 a8, A9 a9, A10 a10, A11 a11, A12 a12, A13 a13, R* r) {
   return new gmp_task_args_m_14_ret<C, M, A0, A1, A2, A3, A4, A5, A6, A7, A8, A9, A10, A11, A12, A13, R>
     (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);
+}