Bug 786234 - Part 6: Move RTCP handling back to the transmit pipeline. r=jesup, r=ethanhugg

media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp

@@ -2282,21 +2282,6 @@ static int vcmTxStartICE_m(cc_mcapid_t mcap_id,
     }
   }
 
-  // This tells the receive MediaPipeline (if there is one) whether we are
-  // doing bundle, and if so, updates the filter. This does not affect the
-  // transmit MediaPipeline (created above) at all.
-  if (attrs->bundle_level) {
-    nsAutoPtr<mozilla::MediaPipelineFilter> filter (new MediaPipelineFilter);
-    for (int s = 0; s < attrs->num_ssrcs; ++s) {
-      filter->AddRemoteSSRC(attrs->ssrcs[s]);
-    }
-    pc.impl()->media()->SetUsingBundle_m(level, true);
-    pc.impl()->media()->UpdateFilterFromRemoteDescription_m(level, filter);
-  } else {
-    // This will also clear the filter.
-    pc.impl()->media()->SetUsingBundle_m(level, false);
-  }
-
 
   if (CC_IS_AUDIO(mcap_id)) {
     mozilla::AudioCodecConfig *config_raw;
@@ -2405,6 +2390,21 @@ static int vcmTxStartICE_m(cc_mcapid_t mcap_id,
     return VCM_ERROR;
   }
 
+  // This tells the receive MediaPipeline (if there is one) whether we are
+  // doing bundle, and if so, updates the filter. Once the filter is finalized,
+  // it is then copied to the transmit pipeline so it can filter RTCP.
+  if (attrs->bundle_level) {
+    nsAutoPtr<mozilla::MediaPipelineFilter> filter (new MediaPipelineFilter);
+    for (int s = 0; s < attrs->num_ssrcs; ++s) {
+      filter->AddRemoteSSRC(attrs->ssrcs[s]);
+    }
+    pc.impl()->media()->SetUsingBundle_m(level, true);
+    pc.impl()->media()->UpdateFilterFromRemoteDescription_m(level, filter);
+  } else {
+    // This will also clear the filter.
+    pc.impl()->media()->SetUsingBundle_m(level, false);
+  }
+
   CSFLogDebug( logTag, "%s success", __FUNCTION__);
   return 0;
 }

media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp

@@ -246,32 +246,28 @@ nsresult MediaPipeline::TransportReady_s(TransportInfo &info) {
     return NS_ERROR_FAILURE;
   }
 
-  if (direction_ == RECEIVE) {
-    // The TRANSMIT pipeline does not process _any_ RTCP. This is the RECEIVE
-    // pipeline's job, even for receiver reports.
     MOZ_MTLOG(ML_INFO, "Listening for " << ToString(info.type_)
                        << " packets received on " <<
                        static_cast<void *>(dtls->downward()));
 
-    switch (info.type_) {
-      case RTP:
-        dtls->downward()->SignalPacketReceived.connect(
-            this,
-            &MediaPipeline::RtpPacketReceived);
-        break;
-      case RTCP:
-        dtls->downward()->SignalPacketReceived.connect(
-            this,
-            &MediaPipeline::RtcpPacketReceived);
-        break;
-      case MUX:
-        dtls->downward()->SignalPacketReceived.connect(
-            this,
-            &MediaPipeline::PacketReceived);
-        break;
-      default:
-        MOZ_CRASH();
-    }
+  switch (info.type_) {
+    case RTP:
+      dtls->downward()->SignalPacketReceived.connect(
+          this,
+          &MediaPipeline::RtpPacketReceived);
+      break;
+    case RTCP:
+      dtls->downward()->SignalPacketReceived.connect(
+          this,
+          &MediaPipeline::RtcpPacketReceived);
+      break;
+    case MUX:
+      dtls->downward()->SignalPacketReceived.connect(
+          this,
+          &MediaPipeline::PacketReceived);
+      break;
+    default:
+      MOZ_CRASH();
   }
 
   info.state_ = MP_OPEN;
@@ -773,7 +769,7 @@ void MediaPipeline::SetUsingBundle_s(bool decision) {
   }
 }
 
-void MediaPipeline::UpdateFilterFromRemoteDescription_s(
+MediaPipelineFilter* MediaPipeline::UpdateFilterFromRemoteDescription_s(
     nsAutoPtr<MediaPipelineFilter> filter) {
   ASSERT_ON_THREAD(sts_thread_);
   // This is only supposed to relax the filter. Relaxing a missing filter is
@@ -785,6 +781,8 @@ void MediaPipeline::UpdateFilterFromRemoteDescription_s(
   } else {
     filter_->IncorporateRemoteDescription(*filter);
   }
+
+  return filter_.get();
 }
 
 nsresult MediaPipeline::PipelineTransport::SendRtpPacket(

media/webrtc/signaling/src/mediapipeline/MediaPipeline.h

@@ -128,7 +128,7 @@ class MediaPipeline : public sigslot::has_slots<> {
   // packet is meant for us. We want to get out of this indeterminate state
   // ASAP, which is what this function can be used for.
   void SetUsingBundle_s(bool decision);
-  void UpdateFilterFromRemoteDescription_s(
+  MediaPipelineFilter* UpdateFilterFromRemoteDescription_s(
       nsAutoPtr<MediaPipelineFilter> filter);
 
   virtual Direction direction() const { return direction_; }

media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp

@@ -412,21 +412,59 @@ PeerConnectionMedia::SetUsingBundle_m(int level, bool decision)
   return false;
 }
 
+static void
+UpdateFilterFromRemoteDescription_s(
+  RefPtr<mozilla::MediaPipeline> receive,
+  RefPtr<mozilla::MediaPipeline> transmit,
+  nsAutoPtr<mozilla::MediaPipelineFilter> filter) {
+
+  // Update filter, and make a copy of the final version.
+  mozilla::MediaPipelineFilter *finalFilter(
+    receive->UpdateFilterFromRemoteDescription_s(filter));
+
+  if (finalFilter) {
+    filter = new mozilla::MediaPipelineFilter(*finalFilter);
+  }
+
+  // Set same filter on transmit pipeline too.
+  transmit->UpdateFilterFromRemoteDescription_s(filter);
+}
+
 bool
 PeerConnectionMedia::UpdateFilterFromRemoteDescription_m(
     int level,
     nsAutoPtr<mozilla::MediaPipelineFilter> filter)
 {
   ASSERT_ON_THREAD(mMainThread);
-  for (size_t i = 0; i < mRemoteSourceStreams.Length(); ++i) {
-    if (mRemoteSourceStreams[i]->UpdateFilterFromRemoteDescription_m(level,
-                                                                     filter)) {
-      // Found the MediaPipeline for |level|
-      return true;
-    }
+
+  RefPtr<mozilla::MediaPipeline> receive;
+  for (size_t i = 0; !receive && i < mRemoteSourceStreams.Length(); ++i) {
+    receive = mRemoteSourceStreams[i]->GetPipelineByLevel_m(level);
+  }
+
+  RefPtr<mozilla::MediaPipeline> transmit;
+  for (size_t i = 0; !transmit && i < mLocalSourceStreams.Length(); ++i) {
+    transmit = mLocalSourceStreams[i]->GetPipelineByLevel_m(level);
+  }
+
+  if (receive && transmit) {
+    // GetPipelineByLevel_m will return nullptr if shutdown is in progress;
+    // since shutdown is initiated in main, and involves a dispatch to STS
+    // before the pipelines are released, our dispatch to STS will complete
+    // before any release can happen due to a shutdown that hasn't started yet.
+    RUN_ON_THREAD(GetSTSThread(),
+                  WrapRunnableNM(
+                      &UpdateFilterFromRemoteDescription_s,
+                      receive,
+                      transmit,
+                      filter
+                  ),
+                  NS_DISPATCH_NORMAL);
+    return true;
+  } else {
+    CSFLogWarn(logTag, "Could not locate level %d to update filter",
+        static_cast<int>(level));
   }
-  CSFLogWarn(logTag, "Could not locate level %d to update filter",
-                     static_cast<int>(level));
   return false;
 }
 
@@ -537,55 +575,28 @@ RemoteSourceStreamInfo::StorePipeline(int aTrack,
   mTypes[aTrack] = aIsVideo;
 }
 
-RefPtr<MediaPipeline> RemoteSourceStreamInfo::GetPipelineByLevel_m(int level) {
+RefPtr<MediaPipeline> SourceStreamInfo::GetPipelineByLevel_m(int level) {
   ASSERT_ON_THREAD(mParent->GetMainThread());
-  for (auto p = mPipelines.begin(); p != mPipelines.end(); ++p) {
-    if (p->second->level() == level) {
-      return p->second;
+
+  // Refuse to hand out references if we're tearing down.
+  // (Since teardown involves a dispatch to and from STS before MediaPipelines
+  // are released, it is safe to start other dispatches to and from STS with a
+  // RefPtr<MediaPipeline>, since that reference won't be the last one
+  // standing)
+  if (mMediaStream) {
+    for (auto p = mPipelines.begin(); p != mPipelines.end(); ++p) {
+      if (p->second->level() == level) {
+        return p->second;
+      }
     }
   }
+
   return nullptr;
 }
 
-bool RemoteSourceStreamInfo::UpdateFilterFromRemoteDescription_m(
-    int aLevel,
-    nsAutoPtr<mozilla::MediaPipelineFilter> aFilter) {
-  ASSERT_ON_THREAD(mParent->GetMainThread());
-
-  if (!mMediaStream) {
-    // Guard against dispatching once we've started teardown, since we don't
-    // want the RefPtr<MediaPipeline> being the last one standing on the call
-    // to MediaPipeline::UpdateFilterFromRemoteDescription_s; it is not safe
-    // to delete a MediaPipeline anywhere other than the main thread.
-    return false;
-  }
-
-  RefPtr<MediaPipeline> pipeline(GetPipelineByLevel_m(aLevel));
-
-  if (pipeline) {
-    RUN_ON_THREAD(mParent->GetSTSThread(),
-                  WrapRunnable(
-                      pipeline,
-                      &MediaPipeline::UpdateFilterFromRemoteDescription_s,
-                      aFilter
-                  ),
-                  NS_DISPATCH_NORMAL);
-    return true;
-  }
-  return false;
-}
-
 bool RemoteSourceStreamInfo::SetUsingBundle_m(int aLevel, bool decision) {
   ASSERT_ON_THREAD(mParent->GetMainThread());
 
-  if (!mMediaStream) {
-    // Guard against dispatching once we've started teardown, since we don't
-    // want the RefPtr<MediaPipeline> being the last one standing on the call
-    // to MediaPipeline::SetUsingBundle_s; it is not safe
-    // to delete a MediaPipeline anywhere other than the main thread.
-    return false;
-  }
-
   RefPtr<MediaPipeline> pipeline(GetPipelineByLevel_m(aLevel));
 
   if (pipeline) {

media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.h

@@ -190,6 +190,7 @@ public:
   // It allows visibility into the pipelines and flows.
   const std::map<mozilla::TrackID, mozilla::RefPtr<mozilla::MediaPipeline>>&
   GetPipelines() const { return mPipelines; }
+  mozilla::RefPtr<mozilla::MediaPipeline> GetPipelineByLevel_m(int level);
 
 protected:
   std::map<mozilla::TrackID, mozilla::RefPtr<mozilla::MediaPipeline>> mPipelines;
@@ -244,9 +245,6 @@ class RemoteSourceStreamInfo : public SourceStreamInfo {
   void StorePipeline(int aTrack, bool aIsVideo,
                      mozilla::RefPtr<mozilla::MediaPipeline> aPipeline);
 
-  bool UpdateFilterFromRemoteDescription_m(
-      int aLevel,
-      nsAutoPtr<mozilla::MediaPipelineFilter> aFilter);
   bool SetUsingBundle_m(int aLevel, bool decision);
 
   void DetachTransport_s();
@@ -257,7 +255,6 @@ class RemoteSourceStreamInfo : public SourceStreamInfo {
 public:
   DOMMediaStream::TrackTypeHints mTrackTypeHints;
  private:
-  mozilla::RefPtr<mozilla::MediaPipeline> GetPipelineByLevel_m(int level);
   std::map<int, bool> mTypes;
 };