Bug 1266938 - "Frames are displayed out of order on Shaka Player's demo video "Sintel 4K (multicodec, widevine)"". r=cpearce, a=sylvestre
authorBryce Van Dyk <bvandyk@mozilla.com>
Wed, 03 Aug 2016 14:24:00 +0200
changeset 342248 efa6e16e9e4d3618728c76464b0c7c6c50640755
parent 342247 b1bdd0f3482ae07d38a2ec5fbf190a2c4f2310ee
child 342249 f98982223d2376942edfb8b52d095873b00142db
push id1183
push userraliiev@mozilla.com
push dateMon, 05 Sep 2016 20:01:49 +0000
treeherdermozilla-release@3148731bed45 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerscpearce, sylvestre
bugs1266938
milestone49.0
Bug 1266938 - "Frames are displayed out of order on Shaka Player's demo video "Sintel 4K (multicodec, widevine)"". r=cpearce, a=sylvestre
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
+  explicit 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:
+  explicit 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;