Bug 1266938 - Prevent WidevineVideoDecoder emitting out of order frames. r=cpearce
authorBryce Van Dyk <bvandyk@mozilla.com>
Tue, 26 Jul 2016 11:23:10 +1200
changeset 346844 212b7e507a61f1aff4db9d175fc7ca6bd2e65538
parent 346843 d7e39be85498d9e3803e9259f92c4b93beb679d1
child 346845 13fe047c4015a86a79e45d42413e9f7f71fddd95
push id6389
push userraliiev@mozilla.com
push dateMon, 19 Sep 2016 13:38:22 +0000
treeherdermozilla-beta@01d67bfe6c81 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerscpearce
bugs1266938
milestone50.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 1266938 - Prevent WidevineVideoDecoder emitting out of order frames. r=cpearce It was possible for WidevineVideoDeocder when allocating shmems for frames to become reentrant. If a further frame was allocated during this reentrancy it would be returned before the first frame being allocated. As frames are fed into the decoder in order, altering this order for returned frames would result in an out of order display. This is addressed by keeping a deque of frames and allocating them in order. There are changes to guard again reentracy issues with draining also. Previously it was possible to drain a decoder, but still have a frame allocation be pending, resulting in a drain being completed, and a frame being returned follow that. This has been addressed by not draining during reentrancy. MozReview-Commit-ID: LcIsmyabqAv
dom/media/gmp/widevine-adapter/WidevineVideoDecoder.cpp
dom/media/gmp/widevine-adapter/WidevineVideoDecoder.h
dom/media/gmp/widevine-adapter/WidevineVideoFrame.cpp
dom/media/gmp/widevine-adapter/WidevineVideoFrame.h
--- a/dom/media/gmp/widevine-adapter/WidevineVideoDecoder.cpp
+++ b/dom/media/gmp/widevine-adapter/WidevineVideoDecoder.cpp
@@ -3,27 +3,31 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "WidevineVideoDecoder.h"
 
 #include "mp4_demuxer/AnnexB.h"
 #include "WidevineUtils.h"
 #include "WidevineVideoFrame.h"
+#include "mozilla/Move.h"
 
 using namespace cdm;
 
 namespace mozilla {
 
 WidevineVideoDecoder::WidevineVideoDecoder(GMPVideoHost* aVideoHost,
                                            RefPtr<CDMWrapper> aCDMWrapper)
   : mVideoHost(aVideoHost)
   , mCDMWrapper(Move(aCDMWrapper))
   , mExtraData(new MediaByteBuffer())
   , mSentInput(false)
+  , mReturnOutputCallDepth(0)
+  , mDrainPending(false)
+  , mResetInProgress(false)
 {
   // Expect to start with a CDM wrapper, will release it in DecodingComplete().
   MOZ_ASSERT(mCDMWrapper);
   Log("WidevineVideoDecoder created this=%p", this);
 
   // Corresponding Release is in DecodingComplete().
   AddRef();
 }
@@ -77,16 +81,18 @@ WidevineVideoDecoder::InitDecode(const G
 
 void
 WidevineVideoDecoder::Decode(GMPVideoEncodedFrame* aInputFrame,
                              bool aMissingFrames,
                              const uint8_t* aCodecSpecificInfo,
                              uint32_t aCodecSpecificInfoLength,
                              int64_t aRenderTimeMs)
 {
+  // We should not be given new input if a drain has been initiated
+  MOZ_ASSERT(!mDrainPending);
   // We may not get the same out of the CDM decoder as we put in, and there
   // may be some latency, i.e. we may need to input (say) 30 frames before
   // we receive output. So we need to store the durations of the frames input,
   // and retrieve them on output.
   mFrameDurations[aInputFrame->TimeStamp()] = aInputFrame->Duration();
 
   mSentInput = true;
   InputBuffer sample;
@@ -119,121 +125,247 @@ WidevineVideoDecoder::Decode(GMPVideoEnc
   aInputFrame = nullptr;
 
   if (rv == kSuccess) {
     if (!ReturnOutput(frame)) {
       Log("WidevineVideoDecoder::Decode() Failed in ReturnOutput()");
       mCallback->Error(GMPDecodeErr);
       return;
     }
-    mCallback->InputDataExhausted();
+    // A reset should only be started at most at level mReturnOutputCallDepth 1,
+    // and if it's started it should be finished by that call by the time
+    // the it returns, so it should always be false by this point.
+    MOZ_ASSERT(!mResetInProgress);
+    // Only request more data if we don't have pending samples.
+    if (mFrameAllocationQueue.empty()) {
+      MOZ_ASSERT(mCDMWrapper);
+      mCallback->InputDataExhausted();
+    }
   } else if (rv == kNeedMoreData) {
+    MOZ_ASSERT(mCDMWrapper);
     mCallback->InputDataExhausted();
   } else {
     mCallback->Error(ToGMPErr(rv));
   }
+  // Finish a drain if pending and we have no pending ReturnOutput calls on the stack.
+  if (mDrainPending && mReturnOutputCallDepth == 0) {
+    Drain();
+  }
 }
 
+// Util class to assist with counting mReturnOutputCallDepth.
+class CounterHelper {
+public:
+  // RAII, increment counter
+  CounterHelper(int32_t& counter)
+    : mCounter(counter)
+  {
+    mCounter++;
+  }
+
+  // RAII, decrement counter
+  ~CounterHelper()
+  {
+    mCounter--;
+  }
+
+private:
+  int32_t& mCounter;
+};
+
+// Util class to make sure GMP frames are freed. Holds a GMPVideoi420Frame*
+// and will destroy it when the helper is destroyed unless the held frame
+// if forgotten with ForgetFrame.
+class FrameDestroyerHelper {
+public:
+  FrameDestroyerHelper(GMPVideoi420Frame*& frame)
+    : frame(frame)
+  {
+  }
+
+  // RAII, destroy frame if held.
+  ~FrameDestroyerHelper()
+  {
+    if (frame) {
+      frame->Destroy();
+    }
+    frame = nullptr;
+  }
+
+  // Forget the frame without destroying it.
+  void ForgetFrame()
+  {
+    frame = nullptr;
+  }
+
+private:
+  GMPVideoi420Frame* frame;
+};
+
+
+// Special handing is needed around ReturnOutput as it spins the IPC message
+// queue when creating an empty frame and can end up with reentrant calls into
+// the class methods.
 bool
 WidevineVideoDecoder::ReturnOutput(WidevineVideoFrame& aCDMFrame)
 {
-  GMPVideoFrame* f = nullptr;
-  auto err = mVideoHost->CreateFrame(kGMPI420VideoFrame, &f);
-  if (GMP_FAILED(err) || !f) {
-    Log("Failed to create i420 frame!\n");
-    return false;
+  MOZ_ASSERT(mReturnOutputCallDepth >= 0);
+  CounterHelper counterHelper(mReturnOutputCallDepth);
+  mFrameAllocationQueue.push_back(Move(aCDMFrame));
+  if (mReturnOutputCallDepth > 1) {
+    // In a reentrant call.
+    return true;
   }
-  auto gmpFrame = static_cast<GMPVideoi420Frame*>(f);
-  Size size = aCDMFrame.Size();
-  const int32_t yStride = aCDMFrame.Stride(VideoFrame::kYPlane);
-  const int32_t uStride = aCDMFrame.Stride(VideoFrame::kUPlane);
-  const int32_t vStride = aCDMFrame.Stride(VideoFrame::kVPlane);
-  const int32_t halfHeight = size.height / 2;
-  err = gmpFrame->CreateEmptyFrame(size.width,
-                                   size.height,
-                                   yStride,
-                                   uStride,
-                                   vStride);
-  ENSURE_GMP_SUCCESS(err, false);
-
-  err = gmpFrame->SetWidth(size.width);
-  ENSURE_GMP_SUCCESS(err, false);
-
-  err = gmpFrame->SetHeight(size.height);
-  ENSURE_GMP_SUCCESS(err, false);
+  while (!mFrameAllocationQueue.empty()) {
+    MOZ_ASSERT(mReturnOutputCallDepth == 1);
+    // If we're at call level 1 a reset should not have been started. A
+    // reset may be received during CreateEmptyFrame below, but we should not
+    // be in a reset at this stage -- this would indicate receiving decode
+    // messages before completing our reset, which we should not.
+    MOZ_ASSERT(!mResetInProgress);
+    WidevineVideoFrame currentCDMFrame = Move(mFrameAllocationQueue.front());
+    mFrameAllocationQueue.pop_front();
+    GMPVideoFrame* f = nullptr;
+    auto err = mVideoHost->CreateFrame(kGMPI420VideoFrame, &f);
+    if (GMP_FAILED(err) || !f) {
+      Log("Failed to create i420 frame!\n");
+      return false;
+    }
+    auto gmpFrame = static_cast<GMPVideoi420Frame*>(f);
+    FrameDestroyerHelper frameDestroyerHelper(gmpFrame);
+    Size size = currentCDMFrame.Size();
+    const int32_t yStride = currentCDMFrame.Stride(VideoFrame::kYPlane);
+    const int32_t uStride = currentCDMFrame.Stride(VideoFrame::kUPlane);
+    const int32_t vStride = currentCDMFrame.Stride(VideoFrame::kVPlane);
+    const int32_t halfHeight = size.height / 2;
+    // This call can cause a shmem alloc, during this alloc other calls
+    // may be made to this class and placed on the stack. ***WARNING***:
+    // other IPC calls can happen during this call, resulting in calls
+    // being made to the CDM. After this call state can have changed,
+    // and should be reevaluated.
+    err = gmpFrame->CreateEmptyFrame(size.width,
+                                     size.height,
+                                     yStride,
+                                     uStride,
+                                     vStride);
+    // Assert possible reentrant calls or resets haven't altered level unexpectedly.
+    MOZ_ASSERT(mReturnOutputCallDepth == 1);
+    ENSURE_GMP_SUCCESS(err, false);
 
-  Buffer* buffer = aCDMFrame.FrameBuffer();
-  uint8_t* outBuffer = gmpFrame->Buffer(kGMPYPlane);
-  ENSURE_TRUE(outBuffer != nullptr, false);
-  MOZ_ASSERT(gmpFrame->AllocatedSize(kGMPYPlane) >= yStride*size.height);
-  memcpy(outBuffer,
-         buffer->Data() + aCDMFrame.PlaneOffset(VideoFrame::kYPlane),
-         yStride * size.height);
+    // If a reset started we need to dump the current frame and complete the reset.
+    if (mResetInProgress) {
+      MOZ_ASSERT(mCDMWrapper);
+      MOZ_ASSERT(mFrameAllocationQueue.empty());
+      CompleteReset();
+      return true;
+    }
+
+    err = gmpFrame->SetWidth(size.width);
+    ENSURE_GMP_SUCCESS(err, false);
 
-  outBuffer = gmpFrame->Buffer(kGMPUPlane);
-  ENSURE_TRUE(outBuffer != nullptr, false);
-  MOZ_ASSERT(gmpFrame->AllocatedSize(kGMPUPlane) >= uStride * halfHeight);
-  memcpy(outBuffer,
-         buffer->Data() + aCDMFrame.PlaneOffset(VideoFrame::kUPlane),
-         uStride * halfHeight);
+    err = gmpFrame->SetHeight(size.height);
+    ENSURE_GMP_SUCCESS(err, false);
+
+    Buffer* buffer = currentCDMFrame.FrameBuffer();
+    uint8_t* outBuffer = gmpFrame->Buffer(kGMPYPlane);
+    ENSURE_TRUE(outBuffer != nullptr, false);
+    MOZ_ASSERT(gmpFrame->AllocatedSize(kGMPYPlane) >= yStride*size.height);
+    memcpy(outBuffer,
+           buffer->Data() + currentCDMFrame.PlaneOffset(VideoFrame::kYPlane),
+           yStride * size.height);
 
-  outBuffer = gmpFrame->Buffer(kGMPVPlane);
-  ENSURE_TRUE(outBuffer != nullptr, false);
-  MOZ_ASSERT(gmpFrame->AllocatedSize(kGMPVPlane) >= vStride * halfHeight);
-  memcpy(outBuffer,
-         buffer->Data() + aCDMFrame.PlaneOffset(VideoFrame::kVPlane),
-         vStride * halfHeight);
+    outBuffer = gmpFrame->Buffer(kGMPUPlane);
+    ENSURE_TRUE(outBuffer != nullptr, false);
+    MOZ_ASSERT(gmpFrame->AllocatedSize(kGMPUPlane) >= uStride * halfHeight);
+    memcpy(outBuffer,
+           buffer->Data() + currentCDMFrame.PlaneOffset(VideoFrame::kUPlane),
+           uStride * halfHeight);
 
-  gmpFrame->SetTimestamp(aCDMFrame.Timestamp());
+    outBuffer = gmpFrame->Buffer(kGMPVPlane);
+    ENSURE_TRUE(outBuffer != nullptr, false);
+    MOZ_ASSERT(gmpFrame->AllocatedSize(kGMPVPlane) >= vStride * halfHeight);
+    memcpy(outBuffer,
+           buffer->Data() + currentCDMFrame.PlaneOffset(VideoFrame::kVPlane),
+           vStride * halfHeight);
+
+    gmpFrame->SetTimestamp(currentCDMFrame.Timestamp());
 
-  auto d = mFrameDurations.find(aCDMFrame.Timestamp());
-  if (d != mFrameDurations.end()) {
-    gmpFrame->SetDuration(d->second);
-    mFrameDurations.erase(d);
+    auto d = mFrameDurations.find(currentCDMFrame.Timestamp());
+    if (d != mFrameDurations.end()) {
+      gmpFrame->SetDuration(d->second);
+      mFrameDurations.erase(d);
+    }
+
+    // Forget frame so it's not deleted, call back taking ownership.
+    frameDestroyerHelper.ForgetFrame();
+    mCallback->Decoded(gmpFrame);
   }
 
-  mCallback->Decoded(gmpFrame);
-
   return true;
 }
 
 void
 WidevineVideoDecoder::Reset()
 {
   Log("WidevineVideoDecoder::Reset() mSentInput=%d", mSentInput);
+  // We shouldn't reset if a drain is pending.
+  MOZ_ASSERT(!mDrainPending);
+  mResetInProgress = true;
   if (mSentInput) {
     CDM()->ResetDecoder(kStreamTypeVideo);
   }
+  // Remove queued frames, but do not reset mReturnOutputCallDepth, let the
+  // ReturnOutput calls unwind and decrement the counter as needed.
+  mFrameAllocationQueue.clear();
   mFrameDurations.clear();
+  // Only if no ReturnOutput calls are in progress can we complete, otherwise
+  // ReturnOutput needs to finalize the reset.
+  if (mReturnOutputCallDepth == 0) {
+    CompleteReset();
+  }
+}
+
+void
+WidevineVideoDecoder::CompleteReset()
+{
   mCallback->ResetComplete();
   mSentInput = false;
+  mResetInProgress = false;
 }
 
 void
 WidevineVideoDecoder::Drain()
 {
   Log("WidevineVideoDecoder::Drain()");
+  if (mReturnOutputCallDepth > 0) {
+    Log("Drain call is reentrant, postponing drain");
+    mDrainPending = true;
+    return;
+  }
 
   Status rv = kSuccess;
   while (rv == kSuccess) {
     WidevineVideoFrame frame;
     InputBuffer sample;
     Status rv = CDM()->DecryptAndDecodeFrame(sample, &frame);
     Log("WidevineVideoDecoder::Drain();  DecryptAndDecodeFrame() rv=%d", rv);
     if (frame.Format() == kUnknownVideoFormat) {
       break;
     }
     if (rv == kSuccess) {
       if (!ReturnOutput(frame)) {
         Log("WidevineVideoDecoder::Decode() Failed in ReturnOutput()");
       }
     }
   }
+  // Shouldn't be reset while draining.
+  MOZ_ASSERT(!mResetInProgress);
 
   CDM()->ResetDecoder(kStreamTypeVideo);
+  mDrainPending = false;
   mCallback->DrainComplete();
 }
 
 void
 WidevineVideoDecoder::DecodingComplete()
 {
   Log("WidevineVideoDecoder::DecodingComplete()");
   if (mCDMWrapper) {
--- a/dom/media/gmp/widevine-adapter/WidevineVideoDecoder.h
+++ b/dom/media/gmp/widevine-adapter/WidevineVideoDecoder.h
@@ -11,16 +11,17 @@
 #include "gmp-api/gmp-video-decode.h"
 #include "gmp-api/gmp-video-host.h"
 #include "MediaData.h"
 #include "nsISupportsImpl.h"
 #include "nsTArray.h"
 #include "WidevineDecryptor.h"
 #include "WidevineVideoFrame.h"
 #include <map>
+#include <deque>
 
 namespace mozilla {
 
 class WidevineVideoDecoder : public GMPVideoDecoder {
 public:
 
   NS_INLINE_DECL_THREADSAFE_REFCOUNTING(WidevineVideoDecoder)
 
@@ -47,21 +48,32 @@ private:
   cdm::ContentDecryptionModule_8* CDM() const {
     // CDM should only be accessed before 'DecodingComplete'.
     MOZ_ASSERT(mCDMWrapper);
     // CDMWrapper ensure the CDM is non-null, no need to check again.
     return mCDMWrapper->GetCDM();
   }
 
   bool ReturnOutput(WidevineVideoFrame& aFrame);
+  void CompleteReset();
 
   GMPVideoHost* mVideoHost;
   RefPtr<CDMWrapper> mCDMWrapper;
   RefPtr<MediaByteBuffer> mExtraData;
   RefPtr<MediaByteBuffer> mAnnexB;
   GMPVideoDecoderCallback* mCallback;
   std::map<uint64_t, uint64_t> mFrameDurations;
   bool mSentInput;
+  // Frames waiting on allocation
+  std::deque<WidevineVideoFrame> mFrameAllocationQueue;
+  // Number of calls of ReturnOutput currently in progress.
+  int32_t mReturnOutputCallDepth;
+  // If we're waiting to drain. Used to prevent drain completing while
+  // ReturnOutput calls are still on the stack.
+  bool mDrainPending;
+  // If a reset is being performed. Used to track if ReturnOutput should
+  // dump current frame.
+  bool mResetInProgress;
 };
 
 } // namespace mozilla
 
 #endif // WidevineVideoDecoder_h_
--- a/dom/media/gmp/widevine-adapter/WidevineVideoFrame.cpp
+++ b/dom/media/gmp/widevine-adapter/WidevineVideoFrame.cpp
@@ -17,16 +17,29 @@ WidevineVideoFrame::WidevineVideoFrame()
   , mBuffer(nullptr)
   , mTimestamp(0)
 {
   Log("WidevineVideoFrame::WidevineVideoFrame() this=%p", this);
   memset(mPlaneOffsets, 0, sizeof(mPlaneOffsets));
   memset(mPlaneStrides, 0, sizeof(mPlaneStrides));
 }
 
+WidevineVideoFrame::WidevineVideoFrame(WidevineVideoFrame&& aOther)
+  : mFormat(aOther.mFormat)
+  , mSize(aOther.mSize)
+  , mBuffer(aOther.mBuffer)
+  , mTimestamp(aOther.mTimestamp)
+{
+  Log("WidevineVideoFrame::WidevineVideoFrame(WidevineVideoFrame&&) this=%p, other=%p",
+      this, &aOther);
+  memcpy(mPlaneOffsets, aOther.mPlaneOffsets, sizeof(mPlaneOffsets));
+  memcpy(mPlaneStrides, aOther.mPlaneStrides, sizeof(mPlaneStrides));
+  aOther.mBuffer = nullptr;
+}
+
 WidevineVideoFrame::~WidevineVideoFrame()
 {
   if (mBuffer) {
     mBuffer->Destroy();
     mBuffer = nullptr;
   }
 }
 
--- a/dom/media/gmp/widevine-adapter/WidevineVideoFrame.h
+++ b/dom/media/gmp/widevine-adapter/WidevineVideoFrame.h
@@ -10,16 +10,17 @@
 #include "content_decryption_module.h"
 #include <vector>
 
 namespace mozilla {
 
 class WidevineVideoFrame : public cdm::VideoFrame {
 public:
   WidevineVideoFrame();
+  WidevineVideoFrame(WidevineVideoFrame&& other);
   ~WidevineVideoFrame();
 
   void SetFormat(cdm::VideoFormat aFormat) override;
   cdm::VideoFormat Format() const override;
 
   void SetSize(cdm::Size aSize) override;
   cdm::Size Size() const override;