Bug 1465473: Disable RTCP bundle filtering, because webrtc.org can do that for us. draft
authorByron Campen [:bwc] <docfaraday@gmail.com>
Wed, 18 Jul 2018 15:23:12 -0500
changeset 820831 5afe5f346a16dc5f902f90f4c51da5336f37c139
parent 813062 9c02d2ecf22050bfee5d70c04a359d8aaff6eb91
push id116951
push userbcampen@mozilla.com
push dateFri, 20 Jul 2018 13:36:14 +0000
bugs1465473
milestone63.0a1
Bug 1465473: Disable RTCP bundle filtering, because webrtc.org can do that for us. MozReview-Commit-ID: EptqgMeMcoT
media/webrtc/signaling/gtest/mediapipeline_unittest.cpp
media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
media/webrtc/signaling/src/mediapipeline/MediaPipelineFilter.cpp
media/webrtc/signaling/src/mediapipeline/MediaPipelineFilter.h
--- a/media/webrtc/signaling/gtest/mediapipeline_unittest.cpp
+++ b/media/webrtc/signaling/gtest/mediapipeline_unittest.cpp
@@ -510,23 +510,23 @@ class MediaPipelineTest : public ::testi
     p1_.Shutdown();
     p2_.Shutdown();
 
     if (!bundle) {
       // If we are filtering, allow the test-case to do this checking.
       ASSERT_GE(p1_.GetAudioRtpCountSent(), 40);
       ASSERT_EQ(p1_.GetAudioRtpCountReceived(), p2_.GetAudioRtpCountSent());
       ASSERT_EQ(p1_.GetAudioRtpCountSent(), p2_.GetAudioRtpCountReceived());
-
-      // Calling ShutdownMedia_m on both pipelines does not stop the flow of
-      // RTCP. So, we might be off by one here.
-      ASSERT_LE(p2_.GetAudioRtcpCountReceived(), p1_.GetAudioRtcpCountSent());
-      ASSERT_GE(p2_.GetAudioRtcpCountReceived() + 1, p1_.GetAudioRtcpCountSent());
     }
 
+    // No RTCP packets should have been dropped, because we do not filter them.
+    // Calling ShutdownMedia_m on both pipelines does not stop the flow of
+    // RTCP. So, we might be off by one here.
+    ASSERT_LE(p2_.GetAudioRtcpCountReceived(), p1_.GetAudioRtcpCountSent());
+    ASSERT_GE(p2_.GetAudioRtcpCountReceived() + 1, p1_.GetAudioRtcpCountSent());
   }
 
   void TestAudioReceiverBundle(bool bundle_accepted,
       nsAutoPtr<MediaPipelineFilter> initialFilter,
       nsAutoPtr<MediaPipelineFilter> refinedFilter =
           nsAutoPtr<MediaPipelineFilter>(nullptr),
       unsigned int ms_until_answer = 500,
       unsigned int ms_of_traffic_after_answer = 10000) {
@@ -583,128 +583,33 @@ TEST_F(MediaPipelineFilterTest, TestSSRC
   0,0,0,0, \
   0,0,0,0, \
   0,0,0,0, \
   0,0,0,0
 
 #define RTCP_TYPEINFO(num_rrs, type, size) \
   0x80 + num_rrs, type, 0, size
 
-const unsigned char rtcp_sr_s16[] = {
-  // zero rrs, size 6 words
-  RTCP_TYPEINFO(0, MediaPipelineFilter::SENDER_REPORT_T, 6),
-  REPORT_FRAGMENT(16)
-};
-
-const unsigned char rtcp_sr_s16_r17[] = {
-  // one rr, size 12 words
-  RTCP_TYPEINFO(1, MediaPipelineFilter::SENDER_REPORT_T, 12),
-  REPORT_FRAGMENT(16),
-  REPORT_FRAGMENT(17)
-};
-
-const unsigned char unknown_type[] = {
-  RTCP_TYPEINFO(1, 222, 0)
-};
-
-TEST_F(MediaPipelineFilterTest, TestEmptyFilterReport0) {
-  MediaPipelineFilter filter;
-  ASSERT_FALSE(filter.FilterSenderReport(rtcp_sr_s16, sizeof(rtcp_sr_s16)));
-}
-
-TEST_F(MediaPipelineFilterTest, TestFilterReport0) {
-  MediaPipelineFilter filter;
-  filter.AddRemoteSSRC(16);
-  ASSERT_TRUE(filter.FilterSenderReport(rtcp_sr_s16, sizeof(rtcp_sr_s16)));
-}
-
-TEST_F(MediaPipelineFilterTest, TestFilterReport0PTTruncated) {
-  MediaPipelineFilter filter;
-  filter.AddRemoteSSRC(16);
-  const unsigned char data[] = {0x80};
-  ASSERT_FALSE(filter.FilterSenderReport(data, sizeof(data)));
-}
-
-TEST_F(MediaPipelineFilterTest, TestFilterReport0CountTruncated) {
-  MediaPipelineFilter filter;
-  filter.AddRemoteSSRC(16);
-  const unsigned char* data = {};
-  ASSERT_FALSE(filter.FilterSenderReport(data, sizeof(data)));
-}
-
-TEST_F(MediaPipelineFilterTest, TestFilterReport1SSRCTruncated) {
-  MediaPipelineFilter filter;
-  filter.AddRemoteSSRC(16);
-  const unsigned char sr[] = {
-    RTCP_TYPEINFO(1, MediaPipelineFilter::SENDER_REPORT_T, 12),
-    REPORT_FRAGMENT(16),
-    0,0,0
-  };
-  ASSERT_TRUE(filter.FilterSenderReport(sr, sizeof(sr)));
-}
-
-TEST_F(MediaPipelineFilterTest, TestFilterReport1BigSSRC) {
-  MediaPipelineFilter filter;
-  filter.AddRemoteSSRC(0x01020304);
-  const unsigned char sr[] = {
-    RTCP_TYPEINFO(1, MediaPipelineFilter::SENDER_REPORT_T, 12),
-    SSRC(0x01020304),
-    REPORT_FRAGMENT(0x11121314)
-  };
-  ASSERT_TRUE(filter.FilterSenderReport(sr, sizeof(sr)));
-}
-
-TEST_F(MediaPipelineFilterTest, TestFilterReportMatch) {
-  MediaPipelineFilter filter;
-  filter.AddRemoteSSRC(16);
-  ASSERT_TRUE(filter.FilterSenderReport(rtcp_sr_s16_r17,
-                                        sizeof(rtcp_sr_s16_r17)));
-}
-
-TEST_F(MediaPipelineFilterTest, TestFilterReportNoMatch) {
-  MediaPipelineFilter filter;
-  filter.AddRemoteSSRC(17);
-  ASSERT_FALSE(filter.FilterSenderReport(rtcp_sr_s16_r17,
-                                         sizeof(rtcp_sr_s16_r17)));
-}
-
-TEST_F(MediaPipelineFilterTest, TestFilterUnknownRTCPType) {
-  MediaPipelineFilter filter;
-  ASSERT_FALSE(filter.FilterSenderReport(unknown_type, sizeof(unknown_type)));
-}
-
 TEST_F(MediaPipelineFilterTest, TestCorrelatorFilter) {
   MediaPipelineFilter filter;
   filter.SetCorrelator(7777);
   ASSERT_TRUE(Filter(filter, 7777, 16, 110));
   ASSERT_FALSE(Filter(filter, 7778, 17, 110));
   // This should also have resulted in the SSRC 16 being added to the filter
   ASSERT_TRUE(Filter(filter, 0, 16, 110));
   ASSERT_FALSE(Filter(filter, 0, 17, 110));
-
-  // rtcp_sr_s16 has 16 as an SSRC
-  ASSERT_TRUE(filter.FilterSenderReport(rtcp_sr_s16, sizeof(rtcp_sr_s16)));
 }
 
 TEST_F(MediaPipelineFilterTest, TestPayloadTypeFilter) {
   MediaPipelineFilter filter;
   filter.AddUniquePT(110);
   ASSERT_TRUE(Filter(filter, 0, 555, 110));
   ASSERT_FALSE(Filter(filter, 0, 556, 111));
 }
 
-TEST_F(MediaPipelineFilterTest, TestPayloadTypeFilterSSRCUpdate) {
-  MediaPipelineFilter filter;
-  filter.AddUniquePT(110);
-  ASSERT_TRUE(Filter(filter, 0, 16, 110));
-
-  // rtcp_sr_s16 has 16 as an SSRC
-  ASSERT_TRUE(filter.FilterSenderReport(rtcp_sr_s16, sizeof(rtcp_sr_s16)));
-}
-
 TEST_F(MediaPipelineFilterTest, TestSSRCMovedWithCorrelator) {
   MediaPipelineFilter filter;
   filter.SetCorrelator(7777);
   ASSERT_TRUE(Filter(filter, 7777, 555, 110));
   ASSERT_TRUE(Filter(filter, 0, 555, 110));
   ASSERT_FALSE(Filter(filter, 7778, 555, 110));
   ASSERT_FALSE(Filter(filter, 0, 555, 110));
 }
@@ -747,18 +652,16 @@ TEST_F(MediaPipelineTest, TestAudioSendB
                           nsAutoPtr<MediaPipelineFilter>(),
                           10000,
                           10000);
 
   // Some packets should have been dropped, but not all
   ASSERT_GT(p1_.GetAudioRtpCountSent(), p2_.GetAudioRtpCountReceived());
   ASSERT_GT(p2_.GetAudioRtpCountReceived(), 40);
   ASSERT_GT(p1_.GetAudioRtcpCountSent(), 1);
-  ASSERT_GT(p1_.GetAudioRtcpCountSent(), p2_.GetAudioRtcpCountReceived());
-  ASSERT_GT(p2_.GetAudioRtcpCountReceived(), 0);
 }
 
 TEST_F(MediaPipelineTest, TestAudioSendEmptyBundleFilter) {
   nsAutoPtr<MediaPipelineFilter> filter(new MediaPipelineFilter);
   nsAutoPtr<MediaPipelineFilter> bad_answer_filter(new MediaPipelineFilter);
   TestAudioReceiverBundle(true, filter, bad_answer_filter);
   // Filter is empty, so should drop everything.
   ASSERT_EQ(0, p2_.GetAudioRtpCountReceived());
--- a/media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
+++ b/media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ -1164,23 +1164,21 @@ MediaPipeline::RtcpPacketReceived(Transp
     CSFLogError(LOGTAG, "Discarding incoming packet; transport not open");
     return;
   }
 
   if (!packet.len()) {
     return;
   }
 
-  // We do not filter receiver reports, since the webrtc.org code for
-  // senders already has logic to ignore RRs that do not apply.
-  // TODO bug 1279153: remove SR check for reduced size RTCP
-  if (mFilter && !mFilter->FilterSenderReport(packet.data(), packet.len())) {
-    CSFLogWarn(LOGTAG, "Dropping incoming RTCP packet; filtered out");
-    return;
-  }
+  // We do not filter RTCP. This is because a compound RTCP packet can contain
+  // any collection of RTCP packets, and webrtc.org already knows how to filter
+  // out what it is interested in, and what it is not. Maybe someday we should
+  // have a TransportLayer that breaks up compound RTCP so we can filter them
+  // individually, but I doubt that will matter much.
 
   CSFLogDebug(LOGTAG, "%s received RTCP packet.", mDescription.c_str());
   IncrementRtcpPacketsReceived();
 
   RtpLogger::LogPacket(packet, true, mDescription);
 
   // Might be nice to pass ownership of the buffer in this case, but it is a
   // small optimization in a rare case.
--- a/media/webrtc/signaling/src/mediapipeline/MediaPipelineFilter.cpp
+++ b/media/webrtc/signaling/src/mediapipeline/MediaPipelineFilter.cpp
@@ -89,38 +89,10 @@ void MediaPipelineFilter::Update(const M
   if (!filter_update.remote_ssrc_set_.empty()) {
     remote_ssrc_set_ = filter_update.remote_ssrc_set_;
   }
 
   payload_type_set_ = filter_update.payload_type_set_;
   correlator_ = filter_update.correlator_;
 }
 
-bool
-MediaPipelineFilter::FilterSenderReport(const unsigned char* data,
-                                        size_t len) const {
-
-  if (!data) {
-    return false;
-  }
-
-  if (len < FIRST_SSRC_OFFSET + 4) {
-    return false;
-  }
-
-  uint8_t payload_type = data[PT_OFFSET];
-
-  if (payload_type != SENDER_REPORT_T) {
-    // Not a sender report, let it through
-    return true;
-  }
-
-  uint32_t ssrc = 0;
-  ssrc += (uint32_t)data[FIRST_SSRC_OFFSET] << 24;
-  ssrc += (uint32_t)data[FIRST_SSRC_OFFSET + 1] << 16;
-  ssrc += (uint32_t)data[FIRST_SSRC_OFFSET + 2] << 8;
-  ssrc += (uint32_t)data[FIRST_SSRC_OFFSET + 3];
-
-  return !!remote_ssrc_set_.count(ssrc);
-}
-
 } // end namespace mozilla
 
--- a/media/webrtc/signaling/src/mediapipeline/MediaPipelineFilter.h
+++ b/media/webrtc/signaling/src/mediapipeline/MediaPipelineFilter.h
@@ -48,32 +48,25 @@ class MediaPipelineFilter {
  public:
   MediaPipelineFilter();
 
   // Checks whether this packet passes the filter, possibly updating the filter
   // in the process (if the correlator or payload types are used, they can teach
   // 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
-  static const uint8_t SENDER_REPORT_T = 200;
-
  private:
   // Payload type is always in the second byte
   static const size_t PT_OFFSET = 1;
   // 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