Bug 1349233: allow SSRC changes in VideoConduits r=bwc a=gchang
authorRandell Jesup <rjesup@jesup.org>
Tue, 28 Mar 2017 10:43:10 -0400
changeset 396310 7f5bd5c96e77bc373b9ff1b3a7b363b49490ff56
parent 396309 93a43397cf6c874c2c1039e20d408ab9773cc815
child 396311 3ee9312263c7ffcc83949291181a12b0de0e56c7
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, gchang
bugs1349233
milestone54.0
Bug 1349233: allow SSRC changes in VideoConduits r=bwc a=gchang MozReview-Commit-ID: 6PNyjLyl9L0
media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
media/webrtc/signaling/src/media-conduit/VideoConduit.h
--- a/media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
+++ b/media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ -193,17 +193,17 @@ WebrtcVideoConduit::WebrtcVideoConduit(R
   , mStartBitrate(0)
   , mPrefMaxBitrate(0)
   , mNegotiatedMaxBitrate(0)
   , mMinBitrateEstimate(0)
   , mCodecMode(webrtc::kRealtimeVideo)
   , mCall(aCall) // refcounted store of the call object
   , mSendStreamConfig(this) // 'this' is stored but not  dereferenced in the constructor.
   , mRecvStreamConfig(this) // 'this' is stored but not  dereferenced in the constructor.
-  , mRecvSSRCSet(false)
+  , mRecvSSRC(0)
   , mRecvSSRCSetInProgress(false)
   , mSendCodecPlugin(nullptr)
   , mRecvCodecPlugin(nullptr)
   , mVideoStatsTimer(do_CreateInstance(NS_TIMER_CONTRACTID))
 {
   mRecvStreamConfig.renderer = this;
 
   // Video Stats Callback
@@ -698,17 +698,16 @@ bool
 WebrtcVideoConduit::SetRemoteSSRC(unsigned int ssrc)
 {
   mRecvStreamConfig.rtp.remote_ssrc = ssrc;
 
   unsigned int current_ssrc;
   if (!GetRemoteSSRC(&current_ssrc)) {
     return false;
   }
-  mRecvSSRCSet = true;
 
   if (current_ssrc == ssrc) {
     return true;
   }
 
   bool wasReceiving = mEngineReceiving;
   if (StopReceiving() != kMediaConduitNoError) {
     return false;
@@ -1176,30 +1175,34 @@ WebrtcVideoConduit::ConfigureRecvMediaCo
     mRecvStreamConfig.rtp.keyframe_method = kf_request_method;
 
     if (use_fec) {
       mRecvStreamConfig.rtp.fec.ulpfec_payload_type = ulpfec_payload_type;
       mRecvStreamConfig.rtp.fec.red_payload_type = red_payload_type;
       mRecvStreamConfig.rtp.fec.red_rtx_payload_type = -1;
     }
 
-    if (!mRecvSSRCSet) {
+    // XXX ugh! same SSRC==0 problem that webrtc.org has
+    if (mRecvSSRC == 0) {
       // Handle un-signalled SSRCs by creating a random one and then when it actually gets set,
       // we'll destroy and recreate.  Simpler than trying to unwind all the logic that assumes
       // the receive stream is created and started when we ConfigureRecvMediaCodecs()
       unsigned int ssrc;
       do {
         SECStatus rv = PK11_GenerateRandom(reinterpret_cast<unsigned char*>(&ssrc), sizeof(ssrc));
         if (rv != SECSuccess) {
           return kMediaConduitUnknownError;
         }
       } while (ssrc == 0); // webrtc.org code has fits if you select an SSRC of 0
 
       mRecvStreamConfig.rtp.remote_ssrc = ssrc;
     }
+    // Either set via SetRemoteSSRC, or temp one we created.
+    mRecvSSRC = mRecvStreamConfig.rtp.remote_ssrc;
+
     // 0 isn't allowed.  Would be best to ask for a random SSRC from the
     // RTP code.  Would need to call rtp_sender.cc -- GenerateNewSSRC(),
     // which isn't exposed.  It's called on collision, or when we decide to
     // send.  it should be called on receiver creation.  Here, we're
     // generating the SSRC value - but this causes ssrc_forced in set in
     // rtp_sender, which locks us into the SSRC - even a collision won't
     // change it!!!
     MOZ_ASSERT(!mSendStreamConfig.rtp.ssrcs.empty());
@@ -1802,20 +1805,25 @@ WebrtcVideoConduit::DeliverPacket(const 
 
   return kMediaConduitNoError;
 }
 
 MediaConduitErrorCode
 WebrtcVideoConduit::ReceivedRTPPacket(const void* data, int len, uint32_t ssrc)
 {
   bool queue = mRecvSSRCSetInProgress;
-  if (!mRecvSSRCSet && !mRecvSSRCSetInProgress) {
+  if (mRecvSSRC != ssrc && !queue) {
+    // we "switch" here immediately, but buffer until the queue is released
+    mRecvSSRC = ssrc;
     mRecvSSRCSetInProgress = true;
     queue = true;
-    // Handle the ssrc-not-signaled case; lock onto first ssrc
+    // any queued packets are from a previous switch that hasn't completed
+    // yet; drop them and only process the latest SSRC
+    mQueuedPackets.Clear();
+    // Handle the unknown ssrc (and ssrc-not-signaled case).
     // We can't just do this here; it has to happen on MainThread :-(
     // We also don't want to drop the packet, nor stall this thread, so we hold
     // the packet (and any following) for inserting once the SSRC is set.
 
     // Ensure lamba captures refs
     RefPtr<WebrtcVideoConduit> self = this;
     nsCOMPtr<nsIThread> thread;
     if (NS_WARN_IF(NS_FAILED(NS_GetCurrentThread(getter_AddRefs(thread))))) {
@@ -1825,29 +1833,33 @@ WebrtcVideoConduit::ReceivedRTPPacket(co
           // Normally this is done in CreateOrUpdateMediaPipeline() for
           // initial creation and renegotiation, but here we're rebuilding the
           // Receive channel at a lower level.  This is needed whenever we're
           // creating a GMPVideoCodec (in particular, H264) so it can communicate
           // errors to the PC.
           WebrtcGmpPCHandleSetter setter(self->mPCHandle);
           self->SetRemoteSSRC(ssrc); // this will likely re-create the VideoReceiveStream
           // We want to unblock the queued packets on the original thread
-          thread->Dispatch(media::NewRunnableFrom([self]() mutable {
-                self->mRecvSSRCSetInProgress = false;
-                // SSRC is set; insert queued packets
-                for (auto& packet : self->mQueuedPackets) {
-                  CSFLogDebug(logTag, "%s: seq# %u, Len %d ", __FUNCTION__,
-                              (uint16_t)ntohs(((uint16_t*) packet->mData)[1]), packet->mLen);
+          thread->Dispatch(media::NewRunnableFrom([self, ssrc]() mutable {
+                if (ssrc == self->mRecvSSRC) {
+                  // SSRC is set; insert queued packets
+                  for (auto& packet : self->mQueuedPackets) {
+                    CSFLogDebug(logTag, "%s: seq# %u, Len %d ", __FUNCTION__,
+                                (uint16_t)ntohs(((uint16_t*) packet->mData)[1]), packet->mLen);
 
-                  if (self->DeliverPacket(packet->mData, packet->mLen) != kMediaConduitNoError) {
-                    CSFLogError(logTag, "%s RTP Processing Failed", __FUNCTION__);
-                    // Keep delivering and then clear the queue
+                    if (self->DeliverPacket(packet->mData, packet->mLen) != kMediaConduitNoError) {
+                      CSFLogError(logTag, "%s RTP Processing Failed", __FUNCTION__);
+                      // Keep delivering and then clear the queue
+                    }
                   }
+                  self->mQueuedPackets.Clear();
+                  // we don't leave inprogress until there are no changes in-flight
+                  self->mRecvSSRCSetInProgress = false;
                 }
-                self->mQueuedPackets.Clear();
+                // else this is an intermediate switch; another is in-flight
 
                 return NS_OK;
               }), NS_DISPATCH_NORMAL);
           return NS_OK;
         }));
     // we'll return after queuing
   }
   if (queue) {
--- a/media/webrtc/signaling/src/media-conduit/VideoConduit.h
+++ b/media/webrtc/signaling/src/media-conduit/VideoConduit.h
@@ -498,20 +498,20 @@ private:
   // WEBRTC.ORG Call API
   RefPtr<WebRtcCallWrapper> mCall;
 
   webrtc::VideoSendStream::Config mSendStreamConfig;
   VideoEncoderConfigBuilder mEncoderConfig;
   webrtc::VideoCodecH264 mEncoderSpecificH264;
 
   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;
+
+  // accessed on creation, and when receiving packets
+  uint32_t mRecvSSRC; // this can change during a stream!
+
   // The runnable to set the SSRC is in-flight; queue packets until it's done.
   bool mRecvSSRCSetInProgress;
   struct QueuedPacket {
     int mLen;
     uint8_t mData[1];
   };
   nsTArray<UniquePtr<QueuedPacket>> mQueuedPackets;