Bug 1136986 - Fix unthreadsafe uses of GMPVideoHost in gmp-clearkey. r=edwin
authorChris Pearce <cpearce@mozilla.com>
Sat, 28 Feb 2015 10:23:33 +1300
changeset 261521 f9050bfa1679c5d03e022785075f25df46b53413
parent 261520 268374d29c6d87e04cfd2b59e0cb6e9c231f8043
child 261522 b83ecdab252a0d1af26a419ff88f3a05c51ccf1f
push id830
push userraliiev@mozilla.com
push dateFri, 19 Jun 2015 19:24:37 +0000
treeherdermozilla-release@932614382a68 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersedwin
bugs1136986
milestone39.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 1136986 - Fix unthreadsafe uses of GMPVideoHost in gmp-clearkey. r=edwin
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()