Bug 1358224 - pt 2 - change to RTP stream id filtering on simulcast mochitests. r=drno draft
authorMichael Froman <mfroman@mozilla.com>
Wed, 26 Apr 2017 10:51:00 -0500
changeset 569671 027616406f7de586bf130aa5e4d84acf9df80072
parent 569670 ce0b2440a9ded38ba8ffd52c00f29cdcdfb5b565
child 569672 6c65085b7b4da2f2f03930986f19547dc1115a07
push id56252
push userbmo:mfroman@nostrum.com
push dateThu, 27 Apr 2017 19:55:30 +0000
reviewersdrno
bugs1358224
milestone55.0a1
Bug 1358224 - pt 2 - change to RTP stream id filtering on simulcast mochitests. r=drno The simulcast mochitests setup the receiving PeerConnection to receive simulcast video streams which Firefox doesn't really support. Without a test media server, this is about the best we can do and still test simulcast. Unfortunately the two simulcast streams arriving with different ssrcs (as expect) exercises code we have to deal with some services switching ssrcs midstream. In the tests, this causes intermittent failures because the test is waiting to receive a certain ssrc, and the receiving VideoConduit has switched to the other ssrc. This change adds the ability to filter on RID at the MediaPipeline level, which we can setup prior to media flowing. This avoids the ssrc switching issue since the VideoConduit only receives one ssrc until we change the RID filter to the second RID. At that point, the VideoConduit sees a new ssrc and the switching code works as intended. The modified mochitests setup the RTP stream id header extension, and then filter on each of the RTP stream ids in turn. MozReview-Commit-ID: KApfaxMX8rl
dom/media/tests/mochitest/mochitest.ini
dom/media/tests/mochitest/test_peerConnection_simulcastAnswer.html
dom/media/tests/mochitest/test_peerConnection_simulcastOffer.html
media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
media/webrtc/signaling/src/mediapipeline/MediaPipeline.h
media/webrtc/signaling/src/mediapipeline/MediaPipelineFilter.cpp
media/webrtc/signaling/src/mediapipeline/MediaPipelineFilter.h
media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
--- a/dom/media/tests/mochitest/mochitest.ini
+++ b/dom/media/tests/mochitest/mochitest.ini
@@ -187,22 +187,20 @@ skip-if = toolkit == 'android'
 [test_peerConnection_restartIceNoRtcpMux.html]
 skip-if = toolkit == 'android'
 [test_peerConnection_restartIceLocalRollback.html]
 skip-if = toolkit == 'android'
 [test_peerConnection_restartIceLocalAndRemoteRollback.html]
 skip-if = toolkit == 'android'
 [test_peerConnection_scaleResolution.html]
 skip-if = (android_version == '18') # android(Bug 1189784, timeouts on 4.3 emulator)
-# disable test_peerConnection_simulcastOffer.html for Bug 1351590
-#[test_peerConnection_simulcastOffer.html]
-#skip-if = android_version # no simulcast support on android
-# disable test_peerConnection_simulcastAnswer.html for Bug 1351531
-#[test_peerConnection_simulcastAnswer.html]
-#skip-if = android_version # no simulcast support on android
+[test_peerConnection_simulcastOffer.html]
+skip-if = android_version # no simulcast support on android
+[test_peerConnection_simulcastAnswer.html]
+skip-if = android_version # no simulcast support on android
 #[test_peerConnection_relayOnly.html]
 [test_peerConnection_callbacks.html]
 skip-if = toolkit == 'android' # android(Bug 1189784, timeouts on 4.3 emulator)
 [test_peerConnection_replaceTrack.html]
 skip-if = toolkit == 'android'  # Bug 1189784
 [test_peerConnection_syncSetDescription.html]
 skip-if = android_version == '18' # android(Bug 1189784, timeouts on 4.3 emulator)
 [test_peerConnection_setLocalAnswerInHaveLocalOffer.html]
--- a/dom/media/tests/mochitest/test_peerConnection_simulcastAnswer.html
+++ b/dom/media/tests/mochitest/test_peerConnection_simulcastAnswer.html
@@ -11,22 +11,30 @@
     bug: "1231507",
     title: "Basic video-only peer connection with Simulcast answer",
     visible: true
   });
 
   var test;
   var pushPrefs = (...p) => SpecialPowers.pushPrefEnv({set: p});
 
-  function selectRecvSsrc(pc, index) {
+  function addRIDExtension(pc, extensionId) {
     var receivers = pc._pc.getReceivers();
     is(receivers.length, 1, "We have exactly one RTP receiver");
     var receiver = receivers[0];
 
-    SpecialPowers.wrap(pc._pc).mozSelectSsrc(receiver, index);
+    SpecialPowers.wrap(pc._pc).mozAddRIDExtension(receiver, extensionId);
+  }
+
+  function selectRecvRID(pc, rid) {
+    var receivers = pc._pc.getReceivers();
+    is(receivers.length, 1, "We have exactly one RTP receiver");
+    var receiver = receivers[0];
+
+    SpecialPowers.wrap(pc._pc).mozAddRIDFilter(receiver, rid);
   }
 
   runNetworkTest(() =>
     pushPrefs(['media.peerconnection.simulcast', true],
               // 180Kbps was determined empirically, set well-higher than
               // the 80Kbps+overhead needed for the two simulcast streams.
               // 100Kbps was apparently too low.
               ['media.peerconnection.video.min_bitrate_estimate', 180*1000]).then(() => {
@@ -71,21 +79,27 @@
 
       test.chain.insertAfter('PC_LOCAL_GET_ANSWER',[
         function PC_LOCAL_REMOVE_SIMULCAST_ATTRS_FROM_ANSWER(test) {
           test._remote_answer.sdp =
             sdputils.removeSimulcastProperties(test._remote_answer.sdp);
         }
       ]);
 
-      test.chain.insertAfter('PC_REMOTE_WAIT_FOR_MEDIA_FLOW',[
-        function PC_REMOTE_SET_RTP_FIRST_RID(test) {
-          // Cause pcLocal to filter out everything but the first SSRC. This
-          // lets only one of the simulcast streams through.
-          selectRecvSsrc(test.pcLocal, 0);
+      // do this after set remote description so the MediaPipeline
+      // has been created.
+      test.chain.insertAfter('PC_LOCAL_SET_REMOTE_DESCRIPTION',[
+        function PC_LOCAL_SET_RTP_FIRST_RID(test) {
+          var extmap_id = test._local_offer.sdp.match(
+              "a=extmap:([0-9+])/recvonly urn:ietf:params:rtp-hdrext:sdes:rtp-stream-id");
+          ok(extmap_id, "Local offer has extmap id for simulcast: " + extmap_id[1]);
+          // Cause pcLocal to filter out everything but RID "bar", only
+          // allowing one of the simulcast streams through.
+          addRIDExtension(test.pcLocal, extmap_id[1]);
+          selectRecvRID(test.pcLocal, "bar");
         }
       ]);
 
       test.chain.append([
         function PC_LOCAL_WAIT_FOR_FRAMES() {
           var vremote = test.pcLocal.remoteMediaElements[0];
           ok(vremote, "Should have remote video element for pcLocal");
           return helper.waitForFrames(vremote);
@@ -96,19 +110,19 @@
           ok(vlocal, "Should have local video element for pcRemote");
           ok(vremote, "Should have remote video element for pcLocal");
           ok(vlocal.videoWidth > 0, "source width is positive");
           ok(vlocal.videoHeight > 0, "source height is positive");
           is(vremote.videoWidth, vlocal.videoWidth / 2, "sink is 1/2 width of source");
           is(vremote.videoHeight, vlocal.videoHeight / 2, "sink is 1/2 height of source");
         },
         function PC_LOCAL_SET_RTP_SECOND_RID(test) {
-          // Now, cause pcLocal to filter out everything but the second SSRC.
-          // This lets only the other simulcast stream through.
-          selectRecvSsrc(test.pcLocal, 1);
+          // Now, cause pcLocal to filter out everything but RID "foo", only
+          // allowing the other simulcast stream through.
+          selectRecvRID(test.pcLocal, "foo");
         },
         function PC_LOCAL_WAIT_FOR_SECOND_MEDIA_FLOW(test) {
           return test.pcLocal.waitForMediaFlow();
         },
         function PC_LOCAL_WAIT_FOR_FRAMES_2() {
           var vremote = test.pcLocal.remoteMediaElements[0];
           ok(vremote, "Should have remote video element for pcLocal");
           return helper.waitForFrames(vremote);
--- a/dom/media/tests/mochitest/test_peerConnection_simulcastOffer.html
+++ b/dom/media/tests/mochitest/test_peerConnection_simulcastOffer.html
@@ -11,22 +11,30 @@
     bug: "1231507",
     title: "Basic video-only peer connection with Simulcast offer",
     visible: true
   });
 
   var test;
   var pushPrefs = (...p) => SpecialPowers.pushPrefEnv({set: p});
 
-  function selectRecvSsrc(pc, index) {
+  function addRIDExtension(pc, extensionId) {
     var receivers = pc._pc.getReceivers();
     is(receivers.length, 1, "We have exactly one RTP receiver");
     var receiver = receivers[0];
 
-    SpecialPowers.wrap(pc._pc).mozSelectSsrc(receiver, index);
+    SpecialPowers.wrap(pc._pc).mozAddRIDExtension(receiver, extensionId);
+  }
+
+  function selectRecvRID(pc, rid) {
+    var receivers = pc._pc.getReceivers();
+    is(receivers.length, 1, "We have exactly one RTP receiver");
+    var receiver = receivers[0];
+
+    SpecialPowers.wrap(pc._pc).mozAddRIDFilter(receiver, rid);
   }
 
   runNetworkTest(() =>
     pushPrefs(['media.peerconnection.simulcast', true],
               // 180Kbps was determined empirically, set well-higher than
               // the 80Kbps+overhead needed for the two simulcast streams.
               // 100Kbps was apparently too low.
               ['media.peerconnection.video.min_bitrate_estimate', 180*1000]).then(() => {
@@ -63,44 +71,50 @@
           info("Answer with RIDs: " + JSON.stringify(test._remote_answer));
           ok(test._remote_answer.sdp.match(/a=simulcast:/), "Modified answer has simulcast");
           ok(test._remote_answer.sdp.match(/a=rid:foo/), "Modified answer has rid foo");
           ok(test._remote_answer.sdp.match(/a=rid:bar/), "Modified answer has rid bar");
           ok(test._remote_answer.sdp.match(/urn:ietf:params:rtp-hdrext:sdes:rtp-stream-id/), "Modified answer has RID");
         }
       ]);
 
-      test.chain.insertAfter('PC_REMOTE_WAIT_FOR_MEDIA_FLOW',[
+      // do this after set local description so the MediaPipeline
+      // has been created.
+      test.chain.insertAfter('PC_REMOTE_SET_LOCAL_DESCRIPTION',[
         function PC_REMOTE_SET_RTP_FIRST_RID(test) {
-          // Cause pcRemote to filter out everything but the first SSRC. This
-          // lets only one of the simulcast streams through.
-          selectRecvSsrc(test.pcRemote, 0);
+          var extmap_id = test.originalOffer.sdp.match(
+              "a=extmap:([0-9+])/sendonly urn:ietf:params:rtp-hdrext:sdes:rtp-stream-id");
+          ok(extmap_id, "Original offer has extmap id for simulcast: " + extmap_id[1]);
+          // Cause pcRemote to filter out everything but RID "foo", only
+          // allowing one of the simulcast streams through.
+          addRIDExtension(test.pcRemote, extmap_id[1]);
+          selectRecvRID(test.pcRemote, "foo");
         }
       ]);
 
       test.chain.append([
         function PC_REMOTE_WAIT_FOR_FRAMES() {
           var vremote = test.pcRemote.remoteMediaElements[0];
           ok(vremote, "Should have remote video element for pcRemote");
           return helper.waitForFrames(vremote);
         },
         function PC_REMOTE_CHECK_SIZE_1() {
           var vlocal = test.pcLocal.localMediaElements[0];
           var vremote = test.pcRemote.remoteMediaElements[0];
           ok(vlocal, "Should have local video element for pcLocal");
           ok(vremote, "Should have remote video element for pcRemote");
           ok(vlocal.videoWidth > 0, "source width is positive");
           ok(vlocal.videoHeight > 0, "source height is positive");
-          is(vremote.videoWidth, vlocal.videoWidth / 2, "sink is 1/2 width of source");
-          is(vremote.videoHeight, vlocal.videoHeight / 2, "sink is 1/2 height of source");
+          is(vremote.videoWidth, vlocal.videoWidth, "sink is same width as source");
+          is(vremote.videoHeight, vlocal.videoHeight,  "sink is same height as source");
         },
         function PC_REMOTE_SET_RTP_SECOND_RID(test) {
-          // Now, cause pcRemote to filter out everything but the second SSRC.
-          // This lets only the other simulcast stream through.
-          selectRecvSsrc(test.pcRemote, 1);
+          // Now, cause pcRemote to filter out everything but RID "bar", only
+          // allowing the other simulcast stream through.
+          selectRecvRID(test.pcRemote, "bar");
         },
         function PC_REMOTE_WAIT_FOR_SECOND_MEDIA_FLOW(test) {
           return test.pcRemote.waitForMediaFlow();
         },
         function PC_REMOTE_WAIT_FOR_FRAMES_2() {
           var vremote = test.pcRemote.remoteMediaElements[0];
           ok(vremote, "Should have remote video element for pcRemote");
           return helper.waitForFrames(vremote);
@@ -114,18 +128,18 @@
         },
         function PC_REMOTE_CHECK_SIZE_2() {
           var vlocal = test.pcLocal.localMediaElements[0];
           var vremote = test.pcRemote.remoteMediaElements[0];
           ok(vlocal, "Should have local video element for pcLocal");
           ok(vremote, "Should have remote video element for pcRemote");
           ok(vlocal.videoWidth > 0, "source width is positive");
           ok(vlocal.videoHeight > 0, "source height is positive");
-          is(vremote.videoWidth, vlocal.videoWidth, "sink is same width as source");
-          is(vremote.videoHeight, vlocal.videoHeight,  "sink is same height as source");
+          is(vremote.videoWidth, vlocal.videoWidth / 2, "sink is 1/2 width of source");
+          is(vremote.videoHeight, vlocal.videoHeight / 2, "sink is 1/2 height of source");
         },
       ]);
 
       return test.run();
   })
   .catch(e => ok(false, "unexpected failure: " + e)));
 </script>
 </pre>
--- a/media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
+++ b/media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ -731,16 +731,50 @@ MediaPipeline::UpdateTransport_s(int lev
     // by receiving traffic.
     filter_->Update(*filter);
   } else {
     filter_ = filter;
   }
 }
 
 void
+MediaPipeline::AddRIDExtension_m(size_t extension_id)
+{
+  RUN_ON_THREAD(sts_thread_,
+                WrapRunnable(this,
+                             &MediaPipeline::AddRIDExtension_s,
+                             extension_id),
+                NS_DISPATCH_NORMAL);
+}
+
+void
+MediaPipeline::AddRIDExtension_s(size_t extension_id)
+{
+  rtp_parser_->RegisterRtpHeaderExtension(webrtc::kRtpExtensionRtpStreamId,
+                                          extension_id);
+}
+
+void
+MediaPipeline::AddRIDFilter_m(const std::string& rid)
+{
+  RUN_ON_THREAD(sts_thread_,
+                WrapRunnable(this,
+                             &MediaPipeline::AddRIDFilter_s,
+                             rid),
+                NS_DISPATCH_NORMAL);
+}
+
+void
+MediaPipeline::AddRIDFilter_s(const std::string& rid)
+{
+  filter_ = new MediaPipelineFilter;
+  filter_->AddRemoteRtpStreamId(rid);
+}
+
+void
 MediaPipeline::SelectSsrc_m(size_t ssrc_index)
 {
   if (ssrc_index < ssrcs_received_.size()) {
     uint32_t ssrc = ssrcs_received_[ssrc_index];
     RUN_ON_THREAD(sts_thread_,
                   WrapRunnable(
                                this,
                                &MediaPipeline::SelectSsrc_s,
--- a/media/webrtc/signaling/src/mediapipeline/MediaPipeline.h
+++ b/media/webrtc/signaling/src/mediapipeline/MediaPipeline.h
@@ -113,16 +113,24 @@ class MediaPipeline : public sigslot::ha
                          RefPtr<TransportFlow> rtp_transport,
                          RefPtr<TransportFlow> rtcp_transport,
                          nsAutoPtr<MediaPipelineFilter> filter);
 
   // Used only for testing; installs a MediaPipelineFilter that filters
   // everything but the nth ssrc
   void SelectSsrc_m(size_t ssrc_index);
   void SelectSsrc_s(uint32_t ssrc);
+  // Used only for testing; adds RTP header extension for RTP Stream Id with
+  // the given id.
+  void AddRIDExtension_m(size_t extension_id);
+  void AddRIDExtension_s(size_t extension_id);
+  // Used only for testing; installs a MediaPipelineFilter that filters
+  // everything but the given RID
+  void AddRIDFilter_m(const std::string& rid);
+  void AddRIDFilter_s(const std::string& rid);
 
   virtual Direction direction() const { return direction_; }
   virtual const std::string& trackid() const { return track_id_; }
   virtual int level() const { return level_; }
   virtual bool IsVideo() const = 0;
 
   bool IsDoingRtcpMux() const {
     return (rtp_.type_ == MUX);
--- a/media/webrtc/signaling/src/mediapipeline/MediaPipelineFilter.cpp
+++ b/media/webrtc/signaling/src/mediapipeline/MediaPipelineFilter.cpp
@@ -26,16 +26,22 @@ bool MediaPipelineFilter::Filter(const w
       return true;
     }
     // Some other stream; it is possible that an SSRC has moved, so make sure
     // we don't have that SSRC in our filter any more.
     remote_ssrc_set_.erase(header.ssrc);
     return false;
   }
 
+  if (header.extension.hasRID &&
+      remote_rid_set_.size() &&
+      remote_rid_set_.count(header.extension.rid)) {
+    return true;
+  }
+
   if (remote_ssrc_set_.count(header.ssrc)) {
     return true;
   }
 
   // Last ditch effort...
   if (payload_type_set_.count(header.payloadType)) {
     // Actual match. We need to update the ssrc map so we can route rtcp
     // sender reports correctly (these use a different payload-type field)
@@ -45,16 +51,20 @@ bool MediaPipelineFilter::Filter(const w
 
   return false;
 }
 
 void MediaPipelineFilter::AddRemoteSSRC(uint32_t ssrc) {
   remote_ssrc_set_.insert(ssrc);
 }
 
+void MediaPipelineFilter::AddRemoteRtpStreamId(const std::string& rtp_strm_id) {
+  remote_rid_set_.insert(rtp_strm_id);
+}
+
 void MediaPipelineFilter::AddUniquePT(uint8_t payload_type) {
   payload_type_set_.insert(payload_type);
 }
 
 void MediaPipelineFilter::SetCorrelator(uint32_t correlator) {
   correlator_ = correlator;
 }
 
--- a/media/webrtc/signaling/src/mediapipeline/MediaPipelineFilter.h
+++ b/media/webrtc/signaling/src/mediapipeline/MediaPipelineFilter.h
@@ -7,16 +7,17 @@
 
 // Original author: bcampen@mozilla.com
 
 #ifndef mediapipelinefilter_h__
 #define mediapipelinefilter_h__
 
 #include <cstddef>
 #include <stdint.h>
+#include <string>
 
 #include <set>
 
 namespace webrtc {
 struct RTPHeader;
 }
 
 namespace mozilla {
@@ -52,16 +53,17 @@ class MediaPipelineFilter {
   // the filter about ssrcs)
   bool Filter(const webrtc::RTPHeader& header, uint32_t correlator = 0);
 
   // RTCP doesn't have things like the RTP correlator, and uses its own
   // payload types too.
   bool FilterSenderReport(const unsigned char* data, size_t len) const;
 
   void AddRemoteSSRC(uint32_t ssrc);
+  void AddRemoteRtpStreamId(const std::string& rtp_strm_id);
 
   // When a payload type id is unique to our media section, add it here.
   void AddUniquePT(uint8_t payload_type);
   void SetCorrelator(uint32_t correlator);
 
   void Update(const MediaPipelineFilter& filter_update);
 
   // Some payload types
@@ -73,14 +75,15 @@ class MediaPipelineFilter {
   // First SSRC always starts at the fifth byte.
   static const size_t FIRST_SSRC_OFFSET = 4;
 
   uint32_t correlator_;
   // The number of filters we manage here is quite small, so I am optimizing
   // for readability.
   std::set<uint32_t> remote_ssrc_set_;
   std::set<uint8_t> payload_type_set_;
+  std::set<std::string> remote_rid_set_;
 };
 
 } // end namespace mozilla
 
 #endif // mediapipelinefilter_h__
 
--- a/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
+++ b/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ -2343,45 +2343,64 @@ PeerConnectionImpl::AddTrack(MediaStream
       return res;
     }
     mNumVideoStreams++;
   }
   OnNegotiationNeeded();
   return NS_OK;
 }
 
-nsresult
-PeerConnectionImpl::SelectSsrc(MediaStreamTrack& aRecvTrack,
-                               unsigned short aSsrcIndex)
+RefPtr<MediaPipeline>
+PeerConnectionImpl::GetMediaPipelineForTrack(MediaStreamTrack& aRecvTrack)
 {
   for (size_t i = 0; i < mMedia->RemoteStreamsLength(); ++i) {
     if (mMedia->GetRemoteStreamByIndex(i)->GetMediaStream()->
         HasTrack(aRecvTrack)) {
       auto& pipelines = mMedia->GetRemoteStreamByIndex(i)->GetPipelines();
       std::string trackId = PeerConnectionImpl::GetTrackId(aRecvTrack);
       auto it = pipelines.find(trackId);
       if (it != pipelines.end()) {
-        it->second->SelectSsrc_m(aSsrcIndex);
+        return it->second;
       }
     }
   }
+
+  return nullptr;
+}
+
+nsresult
+PeerConnectionImpl::SelectSsrc(MediaStreamTrack& aRecvTrack,
+                               unsigned short aSsrcIndex)
+{
+  RefPtr<MediaPipeline> pipeline = GetMediaPipelineForTrack(aRecvTrack);
+  if (pipeline) {
+    pipeline->SelectSsrc_m(aSsrcIndex);
+  }
   return NS_OK;
 }
 
 nsresult
 PeerConnectionImpl::AddRIDExtension(MediaStreamTrack& aRecvTrack,
                                     unsigned short aExtensionId)
 {
+  RefPtr<MediaPipeline> pipeline = GetMediaPipelineForTrack(aRecvTrack);
+  if (pipeline) {
+    pipeline->AddRIDExtension_m(aExtensionId);
+  }
   return NS_OK;
 }
 
 nsresult
 PeerConnectionImpl::AddRIDFilter(MediaStreamTrack& aRecvTrack,
                                  const nsAString& aRid)
 {
+  RefPtr<MediaPipeline> pipeline = GetMediaPipelineForTrack(aRecvTrack);
+  if (pipeline) {
+    pipeline->AddRIDFilter_m(NS_ConvertUTF16toUTF8(aRid).get());
+  }
   return NS_OK;
 }
 
 NS_IMETHODIMP
 PeerConnectionImpl::RemoveTrack(MediaStreamTrack& aTrack) {
   PC_AUTO_ENTER_API_CALL(true);
 
   std::string trackId = PeerConnectionImpl::GetTrackId(aTrack);
--- a/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
+++ b/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
@@ -660,16 +660,19 @@ private:
   }
   bool CheckThreadInt() const {
     bool on;
     NS_ENSURE_SUCCESS(mThread->IsOnCurrentThread(&on), false);
     NS_ENSURE_TRUE(on, false);
     return true;
   }
 
+  RefPtr<MediaPipeline> GetMediaPipelineForTrack(
+      dom::MediaStreamTrack& aRecvTrack);
+
   nsresult GetTimeSinceEpoch(DOMHighResTimeStamp *result);
 
   // Shut down media - called on main thread only
   void ShutdownMedia();
 
   void CandidateReady(const std::string& candidate, uint16_t level);
   void SendLocalIceCandidateToContent(uint16_t level,
                                       const std::string& mid,