Bug 1136986 - Fix unthreadsafe uses of GMPVideoHost in gmp-clearkey. r=edwin a=lmandel
authorChris Pearce <cpearce@mozilla.com>
Sat, 28 Feb 2015 10:23:33 +1300
changeset 250228 1fd982ec5296
parent 250227 9745aeeb920c
child 250229 8abdbdecd2d6
push id4521
push usercpearce@mozilla.com
push date2015-03-04 01:22 +0000
treeherdermozilla-beta@8abdbdecd2d6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersedwin, lmandel
bugs1136986
milestone37.0
Bug 1136986 - Fix unthreadsafe uses of GMPVideoHost in gmp-clearkey. r=edwin a=lmandel
dom/media/gmp/gmp-api/gmp-video-host.h
media/gmp-clearkey/0.1/VideoDecoder.cpp
media/gmp-clearkey/0.1/VideoDecoder.h
media/gmp-clearkey/0.1/WMFUtils.h
--- a/dom/media/gmp/gmp-api/gmp-video-host.h
+++ b/dom/media/gmp/gmp-api/gmp-video-host.h
@@ -34,18 +34,20 @@
 #ifndef GMP_VIDEO_HOST_h_
 #define GMP_VIDEO_HOST_h_
 
 #include "gmp-errors.h"
 #include "gmp-video-frame-i420.h"
 #include "gmp-video-frame-encoded.h"
 #include "gmp-video-codec.h"
 
+// This interface must be called on the main thread only.
 class GMPVideoHost
 {
 public:
   // Construct various video API objects. Host does not retain reference,
   // caller is owner and responsible for deleting.
+  // MAIN THREAD ONLY
   virtual GMPErr CreateFrame(GMPVideoFrameFormat aFormat, GMPVideoFrame** aFrame) = 0;
   virtual GMPErr CreatePlane(GMPPlane** aPlane) = 0;
 };
 
 #endif // GMP_VIDEO_HOST_h_
--- a/media/gmp-clearkey/0.1/VideoDecoder.cpp
+++ b/media/gmp-clearkey/0.1/VideoDecoder.cpp
@@ -18,16 +18,17 @@
 #include <limits>
 
 #include "AnnexB.h"
 #include "ClearKeyDecryptionManager.h"
 #include "ClearKeyUtils.h"
 #include "gmp-task-utils.h"
 #include "mozilla/Endian.h"
 #include "VideoDecoder.h"
+#include "mozilla/Move.h"
 
 using namespace wmf;
 
 VideoDecoder::VideoDecoder(GMPVideoHost *aHostAPI)
   : mHostAPI(aHostAPI)
   , mCallback(nullptr)
   , mWorkerThread(nullptr)
   , mMutex(nullptr)
@@ -107,20 +108,35 @@ VideoDecoder::Decode(GMPVideoEncodedFram
   // 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));
 }
 
+class AutoReleaseVideoFrame {
+public:
+  AutoReleaseVideoFrame(GMPVideoEncodedFrame* aFrame)
+    : mFrame(aFrame)
+  {
+  }
+  ~AutoReleaseVideoFrame()
+  {
+    GetPlatform()->runonmainthread(WrapTask(mFrame, &GMPVideoEncodedFrame::Destroy));
+  }
+private:
+  GMPVideoEncodedFrame* mFrame;
+};
+
 void
 VideoDecoder::DecodeTask(GMPVideoEncodedFrame* aInput)
 {
   CK_LOGD("VideoDecoder::DecodeTask");
+  AutoReleaseVideoFrame ensureFrameReleased(aInput);
   HRESULT hr;
 
   {
     AutoLock lock(mMutex);
     mNumInputTasks--;
     assert(mNumInputTasks >= 0);
   }
 
@@ -158,73 +174,84 @@ VideoDecoder::DecodeTask(GMPVideoEncoded
     buffer.insert(buffer.begin(), mAnnexB.begin(), mAnnexB.end());
   }
 
   hr = mDecoder->Input(buffer.data(),
                        buffer.size(),
                        aInput->TimeStamp(),
                        aInput->Duration());
 
-  // We must delete the input sample!
-  GetPlatform()->runonmainthread(WrapTask(aInput, &GMPVideoEncodedFrame::Destroy));
-
   CK_LOGD("VideoDecoder::DecodeTask() Input ret hr=0x%x\n", hr);
   if (FAILED(hr)) {
     CK_LOGE("VideoDecoder::DecodeTask() decode failed ret=0x%x%s\n",
         hr,
         ((hr == MF_E_NOTACCEPTING) ? " (MF_E_NOTACCEPTING)" : ""));
     return;
   }
 
   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) {
-      ReturnOutput(output);
+      GetPlatform()->runonmainthread(
+        WrapTask(this,
+                 &VideoDecoder::ReturnOutput,
+                 CComPtr<IMFSample>(mozilla::Move(output)),
+                 mDecoder->GetFrameWidth(),
+                 mDecoder->GetFrameHeight(),
+                 mDecoder->GetStride()));
+      assert(!output.Get());
     }
     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.
-        GetPlatform()->runonmainthread(WrapTask(mCallback, &GMPVideoDecoderCallback::InputDataExhausted));
+        GetPlatform()->runonmainthread(
+          WrapTask(mCallback,
+                   &GMPVideoDecoderCallback::InputDataExhausted));
       }
     }
     if (FAILED(hr)) {
       CK_LOGE("VideoDecoder::DecodeTask() output failed hr=0x%x\n", hr);
     }
   }
 }
 
 void
-VideoDecoder::ReturnOutput(IMFSample* aSample)
+VideoDecoder::ReturnOutput(IMFSample* aSample,
+                           int32_t aWidth,
+                           int32_t aHeight,
+                           int32_t aStride)
 {
   CK_LOGD("[%p] VideoDecoder::ReturnOutput()\n", this);
   assert(aSample);
 
   HRESULT hr;
 
   GMPVideoFrame* f = nullptr;
   mHostAPI->CreateFrame(kGMPI420VideoFrame, &f);
   if (!f) {
     CK_LOGE("Failed to create i420 frame!\n");
     return;
   }
   auto vf = static_cast<GMPVideoi420Frame*>(f);
 
-  hr = SampleToVideoFrame(aSample, vf);
+  hr = SampleToVideoFrame(aSample, aWidth, aHeight, aStride, vf);
   ENSURE(SUCCEEDED(hr), /*void*/);
 
-  GetPlatform()->runonmainthread(WrapTask(mCallback, &GMPVideoDecoderCallback::Decoded, vf));
-
+  mCallback->Decoded(vf);
 }
 
 HRESULT
 VideoDecoder::SampleToVideoFrame(IMFSample* aSample,
+                                 int32_t aWidth,
+                                 int32_t aHeight,
+                                 int32_t aStride,
                                  GMPVideoi420Frame* aVideoFrame)
 {
   ENSURE(aSample != nullptr, E_POINTER);
   ENSURE(aVideoFrame != nullptr, E_POINTER);
 
   HRESULT hr;
   CComPtr<IMFMediaBuffer> mediaBuffer;
 
@@ -240,47 +267,43 @@ VideoDecoder::SampleToVideoFrame(IMFSamp
   CComPtr<IMF2DBuffer> twoDBuffer;
   hr = mediaBuffer->QueryInterface(static_cast<IMF2DBuffer**>(&twoDBuffer));
   if (SUCCEEDED(hr)) {
     hr = twoDBuffer->Lock2D(&data, &stride);
     ENSURE(SUCCEEDED(hr), hr);
   } else {
     hr = mediaBuffer->Lock(&data, NULL, NULL);
     ENSURE(SUCCEEDED(hr), hr);
-    stride = mDecoder->GetStride();
+    stride = aStride;
   }
-  int32_t width = mDecoder->GetFrameWidth();
-  int32_t height = mDecoder->GetFrameHeight();
 
   // The V and U planes are stored 16-row-aligned, so we need to add padding
   // to the row heights to ensure the Y'CbCr planes are referenced properly.
   // YV12, planar format: [YYYY....][VVVV....][UUUU....]
   // i.e., Y, then V, then U.
   uint32_t padding = 0;
-  if (height % 16 != 0) {
-    padding = 16 - (height % 16);
+  if (aHeight % 16 != 0) {
+    padding = 16 - (aHeight % 16);
   }
-  int32_t y_size = stride * (height + padding);
-  int32_t v_size = stride * (height + padding) / 4;
+  int32_t y_size = stride * (aHeight + padding);
+  int32_t v_size = stride * (aHeight + padding) / 4;
   int32_t halfStride = (stride + 1) / 2;
-  int32_t halfHeight = (height + 1) / 2;
+  int32_t halfHeight = (aHeight + 1) / 2;
 
-  GetPlatform()->syncrunonmainthread(WrapTask(aVideoFrame,
-                                  &GMPVideoi420Frame::CreateEmptyFrame,
-                                  stride, height, stride, halfStride, halfStride));
+  aVideoFrame->CreateEmptyFrame(stride, aHeight, stride, halfStride, halfStride);
 
-  auto err = aVideoFrame->SetWidth(width);
+  auto err = aVideoFrame->SetWidth(aWidth);
   ENSURE(GMP_SUCCEEDED(err), E_FAIL);
-  err = aVideoFrame->SetHeight(height);
+  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*height);
-  memcpy(outBuffer, data, stride*height);
+  assert(aVideoFrame->AllocatedSize(kGMPYPlane) >= stride*aHeight);
+  memcpy(outBuffer, data, stride*aHeight);
 
   outBuffer = aVideoFrame->Buffer(kGMPUPlane);
   ENSURE(outBuffer != nullptr, E_FAIL);
   assert(aVideoFrame->AllocatedSize(kGMPUPlane) >= halfStride*halfHeight);
   memcpy(outBuffer, data+y_size, halfStride*halfHeight);
 
   outBuffer = aVideoFrame->Buffer(kGMPVPlane);
   ENSURE(outBuffer != nullptr, E_FAIL);
@@ -310,31 +333,36 @@ VideoDecoder::Reset()
 {
   mDecoder->Reset();
   mCallback->ResetComplete();
 }
 
 void
 VideoDecoder::DrainTask()
 {
-  if (FAILED(mDecoder->Drain())) {
-    GetPlatform()->syncrunonmainthread(WrapTask(mCallback, &GMPVideoDecoderCallback::DrainComplete));
-  }
+  mDecoder->Drain();
 
   // 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) {
-      ReturnOutput(output);
+      GetPlatform()->runonmainthread(
+        WrapTask(this,
+                 &VideoDecoder::ReturnOutput,
+                 CComPtr<IMFSample>(mozilla::Move(output)),
+                 mDecoder->GetFrameWidth(),
+                 mDecoder->GetFrameHeight(),
+                 mDecoder->GetStride()));
+      assert(!output.Get());
     }
   }
-  GetPlatform()->syncrunonmainthread(WrapTask(mCallback, &GMPVideoDecoderCallback::DrainComplete));
+  GetPlatform()->runonmainthread(WrapTask(mCallback, &GMPVideoDecoderCallback::DrainComplete));
 }
 
 void
 VideoDecoder::Drain()
 {
   EnsureWorker();
   mWorkerThread->Post(WrapTask(this,
                                &VideoDecoder::DrainTask));
--- a/media/gmp-clearkey/0.1/VideoDecoder.h
+++ b/media/gmp-clearkey/0.1/VideoDecoder.h
@@ -51,19 +51,25 @@ public:
 private:
 
   void EnsureWorker();
 
   void DrainTask();
 
   void DecodeTask(GMPVideoEncodedFrame* aInputFrame);
 
-  void ReturnOutput(IMFSample* aSample);
+  void ReturnOutput(IMFSample* aSample,
+                    int32_t aWidth,
+                    int32_t aHeight,
+                    int32_t aStride);
 
   HRESULT SampleToVideoFrame(IMFSample* aSample,
+                             int32_t aWidth,
+                             int32_t aHeight,
+                             int32_t aStride,
                              GMPVideoi420Frame* aVideoFrame);
 
   GMPVideoHost *mHostAPI; // host-owned, invalid at DecodingComplete
   GMPVideoDecoderCallback* mCallback; // host-owned, invalid at DecodingComplete
   GMPThread* mWorkerThread;
   GMPMutex* mMutex;
   wmf::AutoPtr<wmf::WMFH264Decoder> mDecoder;
 
--- a/media/gmp-clearkey/0.1/WMFUtils.h
+++ b/media/gmp-clearkey/0.1/WMFUtils.h
@@ -44,39 +44,60 @@ extern "C" const CLSID CLSID_CMSAACDecMF
 
 namespace wmf {
 
 // Reimplementation of CComPtr to reduce dependence on system
 // shared libraries.
 template<class T>
 class CComPtr {
 public:
+  CComPtr(CComPtr&& aOther) : mPtr(aOther.Detach()) { }
+  CComPtr& operator=(CComPtr&& other) { mPtr = other.Detach(); }
+
+  CComPtr(const CComPtr& aOther) : mPtr(nullptr) { Set(aOther.Get()); }
   CComPtr() : mPtr(nullptr) { }
-  CComPtr(T* const & aPtr) : mPtr(aPtr) { }
+  CComPtr(T* const & aPtr) : mPtr(nullptr) { Set(aPtr); }
   CComPtr(const nullptr_t& aNullPtr) : mPtr(aNullPtr) { }
   T** operator&() { return &mPtr; }
   T* operator->(){ return mPtr; }
   operator T*() { return mPtr; }
-  T* operator=(T* const & aPtr) { return mPtr = aPtr; }
+  T* operator=(T* const & aPtr) { return Set(aPtr); }
   T* operator=(const nullptr_t& aPtr) { return mPtr = aPtr; }
 
+  T* Get() const { return mPtr; }
+
   T* Detach() {
     T* tmp = mPtr;
     mPtr = nullptr;
     return tmp;
   }
 
   ~CComPtr() {
     if (mPtr) {
       mPtr->Release();
     }
     mPtr = nullptr;
   }
 
 private:
+
+  T* Set(T* aPtr) {
+    if (mPtr == aPtr) {
+      return aPtr;
+    }
+    if (mPtr) {
+      mPtr->Release();
+    }
+    mPtr = aPtr;
+    if (mPtr) {
+      mPtr->AddRef();
+    }
+    return mPtr;
+  }
+
   T* mPtr;
 };
 
 class IntRect {
 public:
   IntRect(int32_t _x, int32_t _y, int32_t _w, int32_t _h)
     : x(_x), y(_y), width(_w), height(_h) {}
   IntRect()