Bug 1337777: ensure mSend/RecvStream access is locked r=bwc a=lizzard
authorRandell Jesup <rjesup@jesup.org>
Thu, 02 Mar 2017 15:11:28 -0500
changeset 379024 c1babb29398b8bbef723dd17f575d616822b8327
parent 379023 f6e2a8eabd7ea17672a006d6e69a6718e1083002
child 379025 b6322b5f2b77fe988573ab663ec167ed2d716669
push id1419
push userjlund@mozilla.com
push dateMon, 10 Apr 2017 20:44:07 +0000
treeherdermozilla-release@5e6801b73ef6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbwc, lizzard
bugs1337777
milestone53.0
Bug 1337777: ensure mSend/RecvStream access is locked r=bwc a=lizzard
media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
media/webrtc/signaling/src/media-conduit/VideoConduit.h
media/webrtc/signaling/src/media-conduit/WebrtcGmpVideoCodec.cpp
--- a/media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
+++ b/media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ -175,16 +175,18 @@ VideoSessionConduit::Create(RefPtr<WebRt
 WebrtcVideoConduit::WebrtcVideoConduit(RefPtr<WebRtcCallWrapper> aCall)
   : mTransportMonitor("WebrtcVideoConduit")
   , mRenderer(nullptr)
   , mEngineTransmitting(false)
   , mEngineReceiving(false)
   , mCapId(-1)
   , mCodecMutex("VideoConduit codec db")
   , mInReconfig(false)
+  , mRecvStream(nullptr)
+  , mSendStream(nullptr)
   , mLastWidth(0)
   , mLastHeight(0) // initializing as 0 forces a check for reconfig at start
   , mSendingWidth(0)
   , mSendingHeight(0)
   , mReceivingWidth(0)
   , mReceivingHeight(0)
   , mSendingFramerate(DEFAULT_VIDEO_MAX_FRAMERATE)
   , mLastFramerateTenths(DEFAULT_VIDEO_MAX_FRAMERATE * 10)
@@ -193,19 +195,17 @@ WebrtcVideoConduit::WebrtcVideoConduit(R
   , mVideoLatencyAvg(0)
   , mMinBitrate(0)
   , mStartBitrate(0)
   , mPrefMaxBitrate(0)
   , mNegotiatedMaxBitrate(0)
   , mMinBitrateEstimate(0)
   , mCodecMode(webrtc::kRealtimeVideo)
   , mCall(aCall) // refcounted store of the call object
-  , mSendStream(nullptr)
   , mSendStreamConfig(this) // 'this' is stored but not  dereferenced in the constructor.
-  , mRecvStream(nullptr)
   , mRecvStreamConfig(this) // 'this' is stored but not  dereferenced in the constructor.
   , mRecvSSRCSet(false)
   , mRecvSSRCSetInProgress(false)
   , mSendCodecPlugin(nullptr)
   , mRecvCodecPlugin(nullptr)
   , mVideoStatsTimer(do_CreateInstance(NS_TIMER_CONTRACTID))
 {
   mRecvStreamConfig.renderer = this;
@@ -270,16 +270,17 @@ bool WebrtcVideoConduit::SetLocalSSRCs(c
   mSendStreamConfig.rtp.ssrcs = aSSRCs;
 
   bool wasTransmitting = mEngineTransmitting;
   if (StopTransmitting() != kMediaConduitNoError) {
     return false;
   }
 
   if (wasTransmitting) {
+    MutexAutoLock lock(mCodecMutex);
     DeleteSendStream();
     if (StartTransmitting() != kMediaConduitNoError) {
       return false;
     }
   }
 
   return true;
 }
@@ -322,31 +323,34 @@ PayloadNameToEncoderType(const std::stri
   }
 
   return webrtc::VideoEncoder::EncoderType::kUnsupportedCodec;
 }
 
 void
 WebrtcVideoConduit::DeleteSendStream()
 {
+  mCodecMutex.AssertCurrentThreadOwns();
   if (mSendStream) {
 
     if (mLoadManager && mSendStream->LoadStateObserver()) {
       mLoadManager->RemoveObserver(mSendStream->LoadStateObserver());
     }
 
     mCall->Call()->DestroyVideoSendStream(mSendStream);
     mSendStream = nullptr;
     mEncoder = nullptr;
   }
 }
 
 MediaConduitErrorCode
 WebrtcVideoConduit::CreateSendStream()
 {
+  mCodecMutex.AssertCurrentThreadOwns();
+
   webrtc::VideoEncoder::EncoderType encoder_type =
     PayloadNameToEncoderType(mSendStreamConfig.encoder_settings.payload_name);
   if (encoder_type == webrtc::VideoEncoder::EncoderType::kUnsupportedCodec) {
     return kMediaConduitInvalidSendCodec;
   }
 
   nsAutoPtr<webrtc::VideoEncoder> encoder(
     CreateEncoder(encoder_type, mEncoderConfig.StreamCount() > 0));
@@ -391,26 +395,29 @@ PayloadNameToDecoderType(const std::stri
   }
 
   return webrtc::VideoDecoder::DecoderType::kUnsupportedCodec;
 }
 
 void
 WebrtcVideoConduit::DeleteRecvStream()
 {
+  mCodecMutex.AssertCurrentThreadOwns();
   if (mRecvStream) {
     mCall->Call()->DestroyVideoReceiveStream(mRecvStream);
     mRecvStream = nullptr;
     mDecoders.clear();
   }
 }
 
 MediaConduitErrorCode
 WebrtcVideoConduit::CreateRecvStream()
 {
+  mCodecMutex.AssertCurrentThreadOwns();
+
   webrtc::VideoReceiveStream::Decoder decoder_desc;
   std::unique_ptr<webrtc::VideoDecoder> decoder;
   webrtc::VideoDecoder::DecoderType decoder_type;
 
   mRecvStreamConfig.decoders.clear();
   for (auto& config : mRecvCodecList) {
     decoder_type = PayloadNameToDecoderType(config->mName);
     if (decoder_type == webrtc::VideoDecoder::DecoderType::kUnsupportedCodec) {
@@ -651,16 +658,17 @@ WebrtcVideoConduit::ConfigureSendMediaCo
     }
 
     condError = StopTransmitting();
     if (condError != kMediaConduitNoError) {
       return condError;
     }
 
     // This will cause a new encoder to be created by StartTransmitting()
+    MutexAutoLock lock(mCodecMutex);
     DeleteSendStream();
   }
 
   mSendStreamConfig.encoder_settings.payload_name = codecConfig->mName;
   mSendStreamConfig.encoder_settings.payload_type = codecConfig->mType;
   mSendStreamConfig.rtp.rtcp_mode = webrtc::RtcpMode::kCompound;
   mSendStreamConfig.rtp.max_packet_size = kVideoMtu;
   mSendStreamConfig.overuse_callback = mLoadManager.get();
@@ -699,23 +707,29 @@ WebrtcVideoConduit::SetRemoteSSRC(unsign
   if (current_ssrc == ssrc || !mEngineReceiving) {
     return true;
   }
 
   if (StopReceiving() != kMediaConduitNoError) {
     return false;
   }
 
-  DeleteRecvStream();
-  MediaConduitErrorCode rval = CreateRecvStream();
-  if (rval != kMediaConduitNoError) {
-    CSFLogError(logTag, "%s Start Receive Error %d ", __FUNCTION__, rval);
-    return false;
+  // This will destroy mRecvStream and create a new one (argh, why can't we change
+  // it without a full destroy?)
+  // We're going to modify mRecvStream, we must lock.  Only modified on MainThread.
+  // All non-MainThread users must lock before reading/using
+  {
+    MutexAutoLock lock(mCodecMutex);
+    DeleteRecvStream();
+    MediaConduitErrorCode rval = CreateRecvStream();
+    if (rval != kMediaConduitNoError) {
+      CSFLogError(logTag, "%s Start Receive Error %d ", __FUNCTION__, rval);
+      return false;
+    }
   }
-
   return (StartReceiving() == kMediaConduitNoError);
 }
 
 bool
 WebrtcVideoConduit::GetRemoteSSRC(unsigned int* ssrc)
 {
   {
     MutexAutoLock lock(mCodecMutex);
@@ -949,16 +963,17 @@ WebrtcVideoConduit::Init()
 }
 
 void
 WebrtcVideoConduit::Destroy()
 {
   // We can't delete the VideoEngine until all these are released!
   // And we can't use a Scoped ptr, since the order is arbitrary
 
+  MutexAutoLock lock(mCodecMutex);
   DeleteSendStream();
   DeleteRecvStream();
 }
 
 void
 WebrtcVideoConduit::SyncTo(WebrtcAudioConduit* aConduit)
 {
   CSFLogDebug(logTag, "%s Synced to %p", __FUNCTION__, aConduit);
@@ -1177,24 +1192,27 @@ WebrtcVideoConduit::ConfigureRecvMediaCo
     // webrtc.org code has fits if you select an SSRC of 0
 
     mRecvStreamConfig.rtp.local_ssrc = ssrc;
 
     // XXX Copy over those that are the same and don't rebuild them
     mRecvCodecList.SwapElements(recv_codecs);
     recv_codecs.Clear();
     mRecvStreamConfig.rtp.rtx.clear();
-    DeleteRecvStream();
-    // Rebuilds mRecvStream from mRecvStreamConfig
-    MediaConduitErrorCode rval = CreateRecvStream();
-    if (rval != kMediaConduitNoError) {
-      CSFLogError(logTag, "%s Start Receive Error %d ", __FUNCTION__, rval);
-      return rval;
+
+    {
+      MutexAutoLock lock(mCodecMutex);
+      DeleteRecvStream();
+      // Rebuilds mRecvStream from mRecvStreamConfig
+      MediaConduitErrorCode rval = CreateRecvStream();
+      if (rval != kMediaConduitNoError) {
+        CSFLogError(logTag, "%s Start Receive Error %d ", __FUNCTION__, rval);
+        return rval;
+      }
     }
-
     return StartReceiving();
   }
   return kMediaConduitNoError;
 }
 
 webrtc::VideoDecoder*
 WebrtcVideoConduit::CreateDecoder(webrtc::VideoDecoder::DecoderType aType)
 {
--- a/media/webrtc/signaling/src/media-conduit/VideoConduit.h
+++ b/media/webrtc/signaling/src/media-conduit/VideoConduit.h
@@ -451,19 +451,25 @@ private:
   // Engine state we are concerned with.
   mozilla::Atomic<bool> mEngineTransmitting; // If true ==> Transmit Subsystem is up and running
   mozilla::Atomic<bool> mEngineReceiving;    // if true ==> Receive Subsystem up and running
 
   int mCapId;   // Capturer for this conduit
   //Local database of currently applied receive codecs
   nsTArray<UniquePtr<VideoCodecConfig>> mRecvCodecList;
 
-  Mutex mCodecMutex; // protects mCurrSendCodecConfig, mVideoSend/RecvStreamStats
+  // protects mCurrSendCodecConfig, mInReconfig,mVideoSend/RecvStreamStats, mSend/RecvStreams
+  Mutex mCodecMutex;
   nsAutoPtr<VideoCodecConfig> mCurSendCodecConfig;
   bool mInReconfig;
+  SendStreamStatistics mSendStreamStats;
+  ReceiveStreamStatistics mRecvStreamStats;
+  // Must call webrtc::Call::DestroyVideoReceive/SendStream to delete these:
+  webrtc::VideoReceiveStream* mRecvStream;
+  webrtc::VideoSendStream* mSendStream;
 
   unsigned short mLastWidth;
   unsigned short mLastHeight;
   unsigned short mSendingWidth;
   unsigned short mSendingHeight;
   unsigned short mReceivingWidth;
   unsigned short mReceivingHeight;
   unsigned int   mSendingFramerate;
@@ -489,24 +495,20 @@ private:
   RefPtr<WebrtcAudioConduit> mSyncedTo;
 
   nsAutoPtr<LoadManager> mLoadManager;
   webrtc::VideoCodecMode mCodecMode;
 
   // WEBRTC.ORG Call API
   RefPtr<WebRtcCallWrapper> mCall;
 
-  webrtc::VideoSendStream* mSendStream;
-  // Must call webrtc::Call::DestroyVideoSendStream to delete
   webrtc::VideoSendStream::Config mSendStreamConfig;
   VideoEncoderConfigBuilder mEncoderConfig;
   webrtc::VideoCodecH264 mEncoderSpecificH264;
 
-  webrtc::VideoReceiveStream* mRecvStream;
-  // Must call webrtc::Call::DestroyVideoReceiveStream to delete
   webrtc::VideoReceiveStream::Config mRecvStreamConfig;
   // We can't create mRecvStream without knowing the remote SSRC
   // Atomic since we key off this on packet insertion, which happens
   // on a different thread.
   Atomic<bool> mRecvSSRCSet;
   // The runnable to set the SSRC is in-flight; queue packets until it's done.
   bool mRecvSSRCSetInProgress;
   struct QueuedPacket {
@@ -519,16 +521,14 @@ private:
   // They are passed to the webrtc::VideoSendStream or VideoReceiveStream,
   // on construction.
   nsAutoPtr<webrtc::VideoEncoder> mEncoder; // only one encoder for now
   std::vector<std::unique_ptr<webrtc::VideoDecoder>> mDecoders;
   WebrtcVideoEncoder* mSendCodecPlugin;
   WebrtcVideoDecoder* mRecvCodecPlugin;
 
   nsCOMPtr<nsITimer> mVideoStatsTimer;
-  SendStreamStatistics mSendStreamStats;
-  ReceiveStreamStatistics mRecvStreamStats;
 
   std::string mPCHandle;
 };
 } // end namespace
 
 #endif
--- a/media/webrtc/signaling/src/media-conduit/WebrtcGmpVideoCodec.cpp
+++ b/media/webrtc/signaling/src/media-conduit/WebrtcGmpVideoCodec.cpp
@@ -566,17 +566,17 @@ WebrtcGmpVideoEncoder::Encoded(GMPVideoE
         return;
     }
 
     struct nal_entry {
       uint32_t offset;
       uint32_t size;
     };
     AutoTArray<nal_entry, 1> nals;
-    uint32_t size;
+    uint32_t size = 0;
     // make sure we don't read past the end of the buffer getting the size
     while (buffer+size_bytes < end) {
       switch (aEncodedFrame->BufferType()) {
         case GMP_BufferSingle:
           size = aEncodedFrame->Size();
           break;
         case GMP_BufferLength8:
           size = *buffer++;