Bug 1337777: ensure mSend/RecvStream access is locked r=bwc
authorRandell Jesup <rjesup@jesup.org>
Thu, 02 Mar 2017 15:11:28 -0500
changeset 394713 cfa8acce82af1ce85b4cbd8f341a4d64e9f3fa81
parent 394712 52bee438b042ee97becc3d6d126966c4b86d001f
child 394714 f24dc887296771ed1d36f96caaed0e326c6f3d25
push id1468
push userasasaki@mozilla.com
push dateMon, 05 Jun 2017 19:31:07 +0000
treeherdermozilla-release@0641fc6ee9d1 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbwc
bugs1337777
milestone54.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 1337777: ensure mSend/RecvStream access is locked r=bwc MozReview-Commit-ID: 5wBkKheve2K
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
@@ -171,16 +171,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)
@@ -189,19 +191,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;
@@ -267,16 +267,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;
 }
@@ -318,31 +319,34 @@ PayloadNameToEncoderType(const std::stri
     return webrtc::VideoEncoder::EncoderType::kH264;
   }
   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));
@@ -386,26 +390,29 @@ PayloadNameToDecoderType(const std::stri
     return webrtc::VideoDecoder::DecoderType::kH264;
   }
   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) {
@@ -646,16 +653,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();
@@ -694,23 +702,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);
@@ -942,16 +956,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);
@@ -1170,24 +1185,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
@@ -452,19 +452,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;
@@ -487,24 +493,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 {
@@ -517,16 +519,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
@@ -555,17 +555,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++;