Bug 1489757 - Bug 1448863 causes video streams to take a very long time to recover from packet loss; r=bwc a=pascalc
authorAndrew Johnson <ajohnson@draster.com>
Thu, 20 Sep 2018 08:23:32 -0400
changeset 492673 fc45a1a514bd2c338150d537df8b6f092a8074cb
parent 492672 e71d111100026b11b611f304e3cad75dff77a303
child 492674 db44e1bed0c6905c11ef424250d3d0450cd97b81
push id1815
push userffxbld-merge
push dateMon, 15 Oct 2018 10:40:45 +0000
treeherdermozilla-release@18d4c09e9378 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbwc, pascalc
bugs1489757, 1448863
milestone63.0
Bug 1489757 - Bug 1448863 causes video streams to take a very long time to recover from packet loss; r=bwc a=pascalc This patch sets mDecoderStatus from the GMPThread so we can eventually report an error back to the caller. Since this done during an asynchronous call, there is no guarantee that the error will be associated with the correct frame, but this workaround should eventually cause an error to be signalled, so that a PLI can be requested and video will not freeze.
media/webrtc/signaling/src/media-conduit/WebrtcGmpVideoCodec.cpp
media/webrtc/signaling/src/media-conduit/WebrtcGmpVideoCodec.h
--- a/media/webrtc/signaling/src/media-conduit/WebrtcGmpVideoCodec.cpp
+++ b/media/webrtc/signaling/src/media-conduit/WebrtcGmpVideoCodec.cpp
@@ -784,16 +784,27 @@ WebrtcGmpVideoDecoder::GmpInitDone(GMPVi
     nsTArray<UniquePtr<GMPDecodeData>> temp;
     temp.SwapElements(mQueuedFrames);
     for (auto& queued : temp) {
       Decode_g(RefPtr<WebrtcGmpVideoDecoder>(this),
                nsAutoPtr<GMPDecodeData>(queued.release()));
     }
   }
 
+  // This is an ugly solution to asynchronous decoding errors
+  // from Decode_g() not being returned to the synchronous Decode() method.
+  // If we don't return an error code at this point, our caller ultimately won't know to request
+  // a PLI and the video stream will remain frozen unless an IDR happens to arrive for other reasons.
+  // Bug 1492852 tracks implementing a proper solution.
+  if(mDecoderStatus != GMPNoErr){
+    LOG(LogLevel::Error, ("%s: Decoder status is bad (%u)!",
+          __PRETTY_FUNCTION__, static_cast<unsigned>(mDecoderStatus)));
+    return WEBRTC_VIDEO_CODEC_ERROR;
+  }
+
   return WEBRTC_VIDEO_CODEC_OK;
 }
 
 void
 WebrtcGmpVideoDecoder::Close_g()
 {
   GMPVideoDecoderProxy* gmp(mGMP);
   mGMP = nullptr;
@@ -814,25 +825,36 @@ WebrtcGmpVideoDecoder::Decode(const webr
                               int64_t aRenderTimeMs)
 {
   MOZ_ASSERT(mGMPThread);
   MOZ_ASSERT(!NS_IsMainThread());
   if (!aInputImage._length) {
     return WEBRTC_VIDEO_CODEC_ERROR;
   }
 
+  // This is an ugly solution to asynchronous decoding errors
+  // from Decode_g() not being returned to the synchronous Decode() method.
+  // If we don't return an error code at this point, our caller ultimately won't know to request
+  // a PLI and the video stream will remain frozen unless an IDR happens to arrive for other reasons.
+  // Bug 1492852 tracks implementing a proper solution.
   nsAutoPtr<GMPDecodeData> decodeData(new GMPDecodeData(aInputImage,
                                                         aMissingFrames,
                                                         aRenderTimeMs));
 
   mGMPThread->Dispatch(WrapRunnableNM(&WebrtcGmpVideoDecoder::Decode_g,
                                       RefPtr<WebrtcGmpVideoDecoder>(this),
                                       decodeData),
                        NS_DISPATCH_NORMAL);
 
+  if(mDecoderStatus != GMPNoErr){
+    LOG(LogLevel::Error, ("%s: Decoder status is bad (%u)!",
+          __PRETTY_FUNCTION__, static_cast<unsigned>(mDecoderStatus)));
+    return WEBRTC_VIDEO_CODEC_ERROR;
+  }
+
   return WEBRTC_VIDEO_CODEC_OK;
 }
 
 /* static */
 // Using nsAutoPtr because WrapRunnable doesn't support move semantics
 void
 WebrtcGmpVideoDecoder::Decode_g(const RefPtr<WebrtcGmpVideoDecoder>& aThis,
                                 nsAutoPtr<GMPDecodeData> aDecodeData)
@@ -840,35 +862,39 @@ WebrtcGmpVideoDecoder::Decode_g(const Re
   if (!aThis->mGMP) {
     if (aThis->mInitting) {
       // InitDone hasn't been called yet (race)
       aThis->mQueuedFrames.AppendElement(aDecodeData.forget());
       return;
     }
     // destroyed via Terminate(), failed to init, or just not initted yet
     LOGD(("GMP Decode: not initted yet"));
+
+    aThis->mDecoderStatus = GMPDecodeErr;
     return;
   }
 
   MOZ_ASSERT(aThis->mQueuedFrames.IsEmpty());
   MOZ_ASSERT(aThis->mHost);
 
   GMPVideoFrame* ftmp = nullptr;
   GMPErr err = aThis->mHost->CreateFrame(kGMPEncodedVideoFrame, &ftmp);
   if (err != GMPNoErr) {
     LOG(LogLevel::Error, ("%s: CreateFrame failed (%u)!",
         __PRETTY_FUNCTION__, static_cast<unsigned>(err)));
+    aThis->mDecoderStatus = err;
     return;
   }
 
   GMPUniquePtr<GMPVideoEncodedFrame> frame(static_cast<GMPVideoEncodedFrame*>(ftmp));
   err = frame->CreateEmptyFrame(aDecodeData->mImage._length);
   if (err != GMPNoErr) {
     LOG(LogLevel::Error, ("%s: CreateEmptyFrame failed (%u)!",
         __PRETTY_FUNCTION__, static_cast<unsigned>(err)));
+    aThis->mDecoderStatus = err;
     return;
   }
 
   // XXX At this point, we only will get mode1 data (a single length and a buffer)
   // Session_info.cc/etc code needs to change to support mode 0.
   *(reinterpret_cast<uint32_t*>(frame->Buffer())) = frame->Size();
 
   // XXX It'd be wonderful not to have to memcpy the encoded data!
@@ -880,16 +906,17 @@ WebrtcGmpVideoDecoder::Decode_g(const Re
   frame->SetCompleteFrame(aDecodeData->mImage._completeFrame);
   frame->SetBufferType(GMP_BufferLength32);
 
   GMPVideoFrameType ft;
   int32_t ret = WebrtcFrameTypeToGmpFrameType(aDecodeData->mImage._frameType, &ft);
   if (ret != WEBRTC_VIDEO_CODEC_OK) {
     LOG(LogLevel::Error, ("%s: WebrtcFrameTypeToGmpFrameType failed (%u)!",
         __PRETTY_FUNCTION__, static_cast<unsigned>(ret)));
+    aThis->mDecoderStatus = GMPDecodeErr;
     return;
   }
 
   // Bug XXXXXX: Set codecSpecific info
   GMPCodecSpecificInfo info;
   memset(&info, 0, sizeof(info));
   info.mCodecType = kGMPVideoCodecH264;
   info.mCodecSpecific.mH264.mSimulcastIdx = 0;
@@ -901,23 +928,21 @@ WebrtcGmpVideoDecoder::Decode_g(const Re
 
   nsresult rv = aThis->mGMP->Decode(std::move(frame),
                                     aDecodeData->mMissingFrames,
                                     codecSpecificInfo,
                                     aDecodeData->mRenderTimeMs);
   if (NS_FAILED(rv)) {
     LOG(LogLevel::Error, ("%s: Decode failed (rv=%u)!",
         __PRETTY_FUNCTION__, static_cast<unsigned>(rv)));
+    aThis->mDecoderStatus = GMPDecodeErr;
+    return;
   }
 
-  if(aThis->mDecoderStatus != GMPNoErr){
-    aThis->mDecoderStatus = GMPNoErr;
-    LOG(LogLevel::Error, ("%s: Decoder status is bad (%u)!",
-        __PRETTY_FUNCTION__, static_cast<unsigned>(aThis->mDecoderStatus)));
-  }
+  aThis->mDecoderStatus = GMPNoErr;
 }
 
 int32_t
 WebrtcGmpVideoDecoder::RegisterDecodeCompleteCallback( webrtc::DecodedImageCallback* aCallback)
 {
   MutexAutoLock lock(mCallbackMutex);
   mCallback = aCallback;
 
--- a/media/webrtc/signaling/src/media-conduit/WebrtcGmpVideoCodec.h
+++ b/media/webrtc/signaling/src/media-conduit/WebrtcGmpVideoCodec.h
@@ -479,17 +479,17 @@ private:
   bool mInitting;
   // Frames queued for decode while mInitting is true
   nsTArray<UniquePtr<GMPDecodeData>> mQueuedFrames;
   GMPVideoHost* mHost;
   // Protects mCallback
   Mutex mCallbackMutex;
   webrtc::DecodedImageCallback* mCallback;
   Atomic<uint64_t> mCachedPluginId;
-  GMPErr mDecoderStatus;
+  Atomic<GMPErr, ReleaseAcquire> mDecoderStatus;
   std::string mPCHandle;
 };
 
 // Basically a strong ref to a WebrtcGmpVideoDecoder, that also translates
 // from Release() to WebrtcGmpVideoDecoder::ReleaseGmp(), since we need
 // WebrtcGmpVideoDecoder::Release() for managing the refcount.
 // The webrtc.org code gets one of these, so it doesn't unilaterally delete
 // the "real" encoder.