Bug 786234 - Part 2.1: RTCP filtering logic. r=abr
☠☠ backed out by 098be567e0d4 ☠ ☠
authorByron Campen [:bwc] <docfaraday@gmail.com>
Thu, 09 Jan 2014 15:12:25 -0800
changeset 187413 1dea344fa608897709f2e674ee726ec858c21da2
parent 187412 4f7eac2b16ca293cbbfab61c1220b65990467186
child 187414 53d8b186e5742151279233834e73eb0eef02a6c9
push id3503
push userraliiev@mozilla.com
push dateMon, 28 Apr 2014 18:51:11 +0000
treeherdermozilla-beta@c95ac01e332e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersabr
bugs786234
milestone30.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 786234 - Part 2.1: RTCP filtering logic. r=abr
media/webrtc/signaling/src/mediapipeline/MediaPipelineFilter.cpp
media/webrtc/signaling/src/mediapipeline/MediaPipelineFilter.h
media/webrtc/signaling/test/mediapipeline_unittest.cpp
--- a/media/webrtc/signaling/src/mediapipeline/MediaPipelineFilter.cpp
+++ b/media/webrtc/signaling/src/mediapipeline/MediaPipelineFilter.cpp
@@ -4,18 +4,23 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this file,
  * You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 // Original author: bcampen@mozilla.com
 
 #include "MediaPipelineFilter.h"
 
+#include "logging.h"
+
 #include "webrtc/modules/interface/module_common_types.h"
 
+// Logging context
+MOZ_MTLOG_MODULE("mediapipeline")
+
 namespace mozilla {
 
 MediaPipelineFilter::MediaPipelineFilter() : correlator_(0) {
 }
 
 bool MediaPipelineFilter::Filter(const webrtc::RTPHeader& header,
                                  uint32_t correlator) {
   if (correlator) {
@@ -42,24 +47,16 @@ bool MediaPipelineFilter::Filter(const w
     // sender reports correctly (these use a different payload-type field)
     AddRemoteSSRC(header.ssrc);
     return true;
   }
 
   return false;
 }
 
-bool MediaPipelineFilter::FilterRTCP(uint32_t ssrc) {
-  return remote_ssrc_set_.count(ssrc) != 0;
-}
-
-bool MediaPipelineFilter::FilterRTCPReceiverReport(uint32_t ssrc) {
-  return local_ssrc_set_.count(ssrc) != 0;
-}
-
 void MediaPipelineFilter::AddLocalSSRC(uint32_t ssrc) {
   local_ssrc_set_.insert(ssrc);
 }
 
 void MediaPipelineFilter::AddRemoteSSRC(uint32_t ssrc) {
   remote_ssrc_set_.insert(ssrc);
 }
 
@@ -78,11 +75,108 @@ void MediaPipelineFilter::IncorporateRem
   if (!remote_filter.remote_ssrc_set_.empty()) {
     remote_ssrc_set_ = remote_filter.remote_ssrc_set_;
   }
 
   // We do not mess with the payload types or correlator here, since the remote
   // SDP doesn't tell us anything about what we will be receiving.
 }
 
+MediaPipelineFilter::Result
+MediaPipelineFilter::FilterRTCP(const unsigned char* data,
+                                size_t len) const {
+  if (len < FIRST_SSRC_OFFSET) {
+    return FAIL;
+  }
+
+  uint8_t payload_type = data[PT_OFFSET];
+
+  switch (payload_type) {
+    case SENDER_REPORT_T:
+      return CheckRtcpReport(data, len, RECEIVER_REPORT_START_SR) ? PASS : FAIL;
+    case RECEIVER_REPORT_T:
+      return CheckRtcpReport(data, len, SENDER_REPORT_START_RR) ? PASS : FAIL;
+    default:
+      return UNSUPPORTED;
+  }
+
+  return UNSUPPORTED;
+}
+
+bool MediaPipelineFilter::CheckRtcpSsrc(const unsigned char* data,
+                                        size_t len,
+                                        size_t ssrc_offset,
+                                        uint8_t flags) const {
+  if (ssrc_offset + 4 > len) {
+    return false;
+  }
+
+  uint32_t ssrc = 0;
+  ssrc += (uint32_t)data[ssrc_offset++] << 24;
+  ssrc += (uint32_t)data[ssrc_offset++] << 16;
+  ssrc += (uint32_t)data[ssrc_offset++] << 8;
+  ssrc += (uint32_t)data[ssrc_offset++];
+
+  if (flags | MAYBE_LOCAL_SSRC) {
+    if (local_ssrc_set_.count(ssrc)) {
+      return true;
+    }
+  }
+
+  if (flags | MAYBE_REMOTE_SSRC) {
+    if (remote_ssrc_set_.count(ssrc)) {
+      return true;
+    }
+  }
+  return false;
+}
+
+static uint8_t GetCount(const unsigned char* data, size_t len) {
+  // This might be a count of rr reports, or might be a count of stuff like
+  // SDES reports, or what-have-you. They all live in bits 3-7.
+  return data[0] & 0x1F;
+}
+
+bool MediaPipelineFilter::CheckRtcpReport(const unsigned char* data,
+                                          size_t len,
+                                          size_t first_rr_offset) const {
+  bool remote_ssrc_matched = CheckRtcpSsrc(data,
+                                           len,
+                                           FIRST_SSRC_OFFSET,
+                                           MAYBE_REMOTE_SSRC);
+
+  uint8_t rr_count = GetCount(data, len);
+
+  // We need to check for consistency. If the remote ssrc matched, the local
+  // ssrcs must too. If it did not, this may just be because our filter is
+  // incomplete, so the local ssrcs could go either way.
+  bool ssrcs_must_match = remote_ssrc_matched;
+  bool ssrcs_must_not_match = false;
+
+  for (uint8_t rr_num = 0; rr_num < rr_count; ++rr_num) {
+    size_t ssrc_offset = first_rr_offset + (rr_num * RECEIVER_REPORT_SIZE);
+
+    if (!CheckRtcpSsrc(data, len, ssrc_offset, MAYBE_LOCAL_SSRC)) {
+      ssrcs_must_not_match = true;
+      if (ssrcs_must_match) {
+        break;
+      }
+    } else {
+      ssrcs_must_match = true;
+      if (ssrcs_must_not_match) {
+        break;
+      }
+    }
+  }
+
+  if (ssrcs_must_match && ssrcs_must_not_match) {
+    MOZ_MTLOG(ML_ERROR, "Received an RTCP packet with SSRCs from "
+              "multiple m-lines! This is broken.");
+    return false;
+  }
+
+  // This is set if any ssrc matched
+  return ssrcs_must_match;
+}
+
 } // end namespace mozilla
 
 
--- a/media/webrtc/signaling/src/mediapipeline/MediaPipelineFilter.h
+++ b/media/webrtc/signaling/src/mediapipeline/MediaPipelineFilter.h
@@ -47,33 +47,62 @@ 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);
 
+  typedef enum {
+    FAIL,
+    PASS,
+    UNSUPPORTED
+  } Result;
+
   // RTCP doesn't have things like the RTP correlator, and uses its own
   // payload types too.
-  // RTCP receiver reports use _our_ ssrcs
-  bool FilterRTCPReceiverReport(uint32_t ssrc);
-  // The rest of RTCP uses their ssrcs
-  bool FilterRTCP(uint32_t ssrc);
+  Result FilterRTCP(const unsigned char* data, size_t len) const;
 
   void AddLocalSSRC(uint32_t ssrc);
   void AddRemoteSSRC(uint32_t ssrc);
 
   // 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 IncorporateRemoteDescription(const MediaPipelineFilter& remote_filter);
 
+  // Some payload types
+  static const uint8_t SENDER_REPORT_T = 200;
+  static const uint8_t RECEIVER_REPORT_T = 201;
+
  private:
+  static const uint8_t MAYBE_LOCAL_SSRC = 1;
+  static const uint8_t MAYBE_REMOTE_SSRC = 2;
+
+  // 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;
+
+  static const size_t RECEIVER_REPORT_START_SR = 7*4;
+  static const size_t SENDER_REPORT_START_RR = 2*4;
+  static const size_t RECEIVER_REPORT_SIZE = 6*4;
+
+  bool CheckRtcpSsrc(const unsigned char* data,
+                     size_t len,
+                     size_t ssrc_offset,
+                     uint8_t flags) const;
+
+
+  bool CheckRtcpReport(const unsigned char* data,
+                        size_t len,
+                        size_t first_rr_offset) const;
+
   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<uint32_t> local_ssrc_set_;
   std::set<uint8_t> payload_type_set_;
 };
 
--- a/media/webrtc/signaling/test/mediapipeline_unittest.cpp
+++ b/media/webrtc/signaling/test/mediapipeline_unittest.cpp
@@ -370,58 +370,288 @@ TEST_F(MediaPipelineFilterTest, TestDefa
 
 TEST_F(MediaPipelineFilterTest, TestSSRCFilter) {
   MediaPipelineFilter filter;
   filter.AddRemoteSSRC(555);
   ASSERT_TRUE(Filter(filter, 0, 555, 110));
   ASSERT_FALSE(Filter(filter, 0, 556, 110));
 }
 
-TEST_F(MediaPipelineFilterTest, TestSSRCFilterRTCP) {
+#define SSRC(ssrc) \
+  ((ssrc >> 24) & 0xFF), \
+  ((ssrc >> 16) & 0xFF), \
+  ((ssrc >> 8 ) & 0xFF), \
+  (ssrc         & 0xFF)
+
+#define REPORT_FRAGMENT(ssrc) \
+  SSRC(ssrc), \
+  0,0,0,0, \
+  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_rr_s16[] = {
+  // zero rrs, size 1 words
+  RTCP_TYPEINFO(0, MediaPipelineFilter::RECEIVER_REPORT_T, 1),
+  SSRC(16)
+};
+
+const unsigned char rtcp_rr_s16_r17[] = {
+  // one rr, 7 words
+  RTCP_TYPEINFO(1, MediaPipelineFilter::RECEIVER_REPORT_T, 7),
+  SSRC(16),
+  REPORT_FRAGMENT(17)
+};
+
+const unsigned char rtcp_rr_s16_r17_18[] = {
+  // two rrs, size 13 words
+  RTCP_TYPEINFO(2, MediaPipelineFilter::RECEIVER_REPORT_T, 13),
+  SSRC(16),
+  REPORT_FRAGMENT(17),
+  REPORT_FRAGMENT(18)
+};
+
+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 rtcp_sr_s16_r17_18[] = {
+  // two rrs, size 18 words
+  RTCP_TYPEINFO(2, MediaPipelineFilter::SENDER_REPORT_T, 18),
+  REPORT_FRAGMENT(16),
+  REPORT_FRAGMENT(17),
+  REPORT_FRAGMENT(18)
+};
+
+const unsigned char unknown_type[] = {
+  RTCP_TYPEINFO(1, 222, 0)
+};
+
+TEST_F(MediaPipelineFilterTest, TestEmptyFilterReport0) {
   MediaPipelineFilter filter;
-  filter.AddRemoteSSRC(555);
-  ASSERT_TRUE(filter.FilterRTCP(555));
-  ASSERT_FALSE(filter.FilterRTCP(556));
-  ASSERT_FALSE(filter.FilterRTCPReceiverReport(555));
+  ASSERT_EQ(MediaPipelineFilter::FAIL,
+            filter.FilterRTCP(rtcp_sr_s16, sizeof(rtcp_sr_s16)));
+  ASSERT_EQ(MediaPipelineFilter::FAIL,
+            filter.FilterRTCP(rtcp_rr_s16, sizeof(rtcp_rr_s16)));
+}
+
+TEST_F(MediaPipelineFilterTest, TestFilterReport0) {
+  MediaPipelineFilter filter;
+  filter.AddRemoteSSRC(16);
+  ASSERT_EQ(MediaPipelineFilter::PASS,
+            filter.FilterRTCP(rtcp_sr_s16, sizeof(rtcp_sr_s16)));
+  ASSERT_EQ(MediaPipelineFilter::PASS,
+            filter.FilterRTCP(rtcp_rr_s16, sizeof(rtcp_rr_s16)));
+}
+
+TEST_F(MediaPipelineFilterTest, TestFilterReport0SSRCTruncated) {
+  MediaPipelineFilter filter;
+  filter.AddRemoteSSRC(16);
+  const unsigned char data[] = {
+    RTCP_TYPEINFO(0, MediaPipelineFilter::RECEIVER_REPORT_T, 1),
+    0,0,0
+  };
+  ASSERT_EQ(MediaPipelineFilter::FAIL,
+            filter.FilterRTCP(data, sizeof(data)));
+}
+
+TEST_F(MediaPipelineFilterTest, TestFilterReport0PTTruncated) {
+  MediaPipelineFilter filter;
+  filter.AddRemoteSSRC(16);
+  const unsigned char data[] = {0x80};
+  ASSERT_EQ(MediaPipelineFilter::FAIL,
+            filter.FilterRTCP(data, sizeof(data)));
+}
+
+TEST_F(MediaPipelineFilterTest, TestFilterReport0CountTruncated) {
+  MediaPipelineFilter filter;
+  filter.AddRemoteSSRC(16);
+  const unsigned char data[] = {};
+  ASSERT_EQ(MediaPipelineFilter::FAIL,
+            filter.FilterRTCP(data, sizeof(data)));
+}
+
+TEST_F(MediaPipelineFilterTest, TestFilterReport1BothMatch) {
+  MediaPipelineFilter filter;
+  filter.AddRemoteSSRC(16);
+  filter.AddLocalSSRC(17);
+  ASSERT_EQ(MediaPipelineFilter::PASS,
+            filter.FilterRTCP(rtcp_sr_s16_r17, sizeof(rtcp_sr_s16_r17)));
+  ASSERT_EQ(MediaPipelineFilter::PASS,
+            filter.FilterRTCP(rtcp_rr_s16_r17, sizeof(rtcp_rr_s16_r17)));
 }
 
-TEST_F(MediaPipelineFilterTest, TestSSRCFilterReceiverReport) {
+TEST_F(MediaPipelineFilterTest, TestFilterReport1SSRCTruncated) {
+  MediaPipelineFilter filter;
+  filter.AddRemoteSSRC(16);
+  filter.AddLocalSSRC(17);
+  const unsigned char rr[] = {
+    RTCP_TYPEINFO(1, MediaPipelineFilter::RECEIVER_REPORT_T, 7),
+    SSRC(16),
+    0,0,0
+  };
+  ASSERT_EQ(MediaPipelineFilter::FAIL,
+            filter.FilterRTCP(rr, sizeof(rr)));
+  const unsigned char sr[] = {
+    RTCP_TYPEINFO(1, MediaPipelineFilter::RECEIVER_REPORT_T, 12),
+    REPORT_FRAGMENT(16),
+    0,0,0
+  };
+  ASSERT_EQ(MediaPipelineFilter::FAIL,
+            filter.FilterRTCP(sr, sizeof(rr)));
+}
+
+TEST_F(MediaPipelineFilterTest, TestFilterReport1BigSSRC) {
+  MediaPipelineFilter filter;
+  filter.AddRemoteSSRC(0x01020304);
+  filter.AddLocalSSRC(0x11121314);
+  const unsigned char rr[] = {
+    RTCP_TYPEINFO(1, MediaPipelineFilter::RECEIVER_REPORT_T, 7),
+    SSRC(0x01020304),
+    REPORT_FRAGMENT(0x11121314)
+  };
+  ASSERT_EQ(MediaPipelineFilter::PASS,
+            filter.FilterRTCP(rr, sizeof(rr)));
+  const unsigned char sr[] = {
+    RTCP_TYPEINFO(1, MediaPipelineFilter::RECEIVER_REPORT_T, 12),
+    SSRC(0x01020304),
+    REPORT_FRAGMENT(0x11121314)
+  };
+  ASSERT_EQ(MediaPipelineFilter::PASS,
+            filter.FilterRTCP(sr, sizeof(rr)));
+}
+
+TEST_F(MediaPipelineFilterTest, TestFilterReport1LocalMatch) {
+  MediaPipelineFilter filter;
+  filter.AddLocalSSRC(17);
+  ASSERT_EQ(MediaPipelineFilter::PASS,
+            filter.FilterRTCP(rtcp_sr_s16_r17, sizeof(rtcp_sr_s16_r17)));
+  ASSERT_EQ(MediaPipelineFilter::PASS,
+            filter.FilterRTCP(rtcp_rr_s16_r17, sizeof(rtcp_rr_s16_r17)));
+}
+
+TEST_F(MediaPipelineFilterTest, TestFilterReport1Inconsistent) {
   MediaPipelineFilter filter;
-  filter.AddLocalSSRC(555);
-  ASSERT_TRUE(filter.FilterRTCPReceiverReport(555));
-  ASSERT_FALSE(filter.FilterRTCPReceiverReport(556));
-  ASSERT_FALSE(filter.FilterRTCP(555));
+  filter.AddRemoteSSRC(16);
+  // We assume that the filter is exactly correct in terms of local ssrcs.
+  // So, when RTCP shows up with a remote SSRC that matches, and a local
+  // ssrc that doesn't, we assume the other end has messed up and put ssrcs
+  // from more than one m-line in the packet.
+  ASSERT_EQ(MediaPipelineFilter::FAIL,
+            filter.FilterRTCP(rtcp_sr_s16_r17, sizeof(rtcp_sr_s16_r17)));
+  ASSERT_EQ(MediaPipelineFilter::FAIL,
+            filter.FilterRTCP(rtcp_rr_s16_r17, sizeof(rtcp_rr_s16_r17)));
+}
+
+TEST_F(MediaPipelineFilterTest, TestFilterReport1NeitherMatch) {
+  MediaPipelineFilter filter;
+  filter.AddRemoteSSRC(17);
+  filter.AddLocalSSRC(18);
+  ASSERT_EQ(MediaPipelineFilter::PASS,
+            filter.FilterRTCP(rtcp_sr_s16_r17, sizeof(rtcp_sr_s16_r17)));
+  ASSERT_EQ(MediaPipelineFilter::PASS,
+            filter.FilterRTCP(rtcp_rr_s16_r17, sizeof(rtcp_rr_s16_r17)));
+}
+
+TEST_F(MediaPipelineFilterTest, TestFilterReport2AllMatch) {
+  MediaPipelineFilter filter;
+  filter.AddRemoteSSRC(16);
+  filter.AddLocalSSRC(17);
+  filter.AddLocalSSRC(18);
+  ASSERT_EQ(MediaPipelineFilter::PASS,
+            filter.FilterRTCP(rtcp_sr_s16_r17_18,
+                              sizeof(rtcp_sr_s16_r17_18)));
+}
+
+TEST_F(MediaPipelineFilterTest, TestFilterReport2LocalMatch) {
+  MediaPipelineFilter filter;
+  filter.AddLocalSSRC(17);
+  filter.AddLocalSSRC(18);
+  ASSERT_EQ(MediaPipelineFilter::PASS,
+            filter.FilterRTCP(rtcp_sr_s16_r17_18,
+                              sizeof(rtcp_sr_s16_r17_18)));
+  ASSERT_EQ(MediaPipelineFilter::PASS,
+            filter.FilterRTCP(rtcp_rr_s16_r17_18,
+                              sizeof(rtcp_rr_s16_r17_18)));
+}
+
+TEST_F(MediaPipelineFilterTest, TestFilterReport2Inconsistent101) {
+  MediaPipelineFilter filter;
+  filter.AddRemoteSSRC(16);
+  filter.AddLocalSSRC(18);
+  ASSERT_EQ(MediaPipelineFilter::FAIL,
+            filter.FilterRTCP(rtcp_sr_s16_r17_18,
+                              sizeof(rtcp_sr_s16_r17_18)));
+  ASSERT_EQ(MediaPipelineFilter::FAIL,
+            filter.FilterRTCP(rtcp_rr_s16_r17_18,
+                              sizeof(rtcp_rr_s16_r17_18)));
+}
+
+TEST_F(MediaPipelineFilterTest, TestFilterReport2Inconsistent001) {
+  MediaPipelineFilter filter;
+  filter.AddLocalSSRC(18);
+  ASSERT_EQ(MediaPipelineFilter::FAIL,
+            filter.FilterRTCP(rtcp_sr_s16_r17_18,
+                              sizeof(rtcp_sr_s16_r17_18)));
+  ASSERT_EQ(MediaPipelineFilter::FAIL,
+            filter.FilterRTCP(rtcp_rr_s16_r17_18,
+                              sizeof(rtcp_rr_s16_r17_18)));
+}
+
+TEST_F(MediaPipelineFilterTest, TestFilterUnknownRTCPType) {
+  MediaPipelineFilter filter;
+  filter.AddLocalSSRC(18);
+  ASSERT_EQ(MediaPipelineFilter::UNSUPPORTED,
+            filter.FilterRTCP(unknown_type, sizeof(unknown_type)));
 }
 
 TEST_F(MediaPipelineFilterTest, TestCorrelatorFilter) {
   MediaPipelineFilter filter;
   filter.SetCorrelator(7777);
-  ASSERT_TRUE(Filter(filter, 7777, 555, 110));
-  ASSERT_FALSE(Filter(filter, 7778, 556, 110));
-  // This should also have resulted in the SSRC 555 being added to the filter
-  ASSERT_TRUE(Filter(filter, 0, 555, 110));
-  ASSERT_FALSE(Filter(filter, 0, 556, 110));
+  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));
 
-  ASSERT_TRUE(filter.FilterRTCP(555));
+  // rtcp_sr_s16 has 16 as an SSRC
+  ASSERT_EQ(MediaPipelineFilter::PASS,
+            filter.FilterRTCP(rtcp_sr_s16, sizeof(rtcp_sr_s16)));
+  ASSERT_EQ(MediaPipelineFilter::PASS,
+            filter.FilterRTCP(rtcp_rr_s16, sizeof(rtcp_rr_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, 555, 110));
-  ASSERT_TRUE(filter.FilterRTCP(555));
-  ASSERT_FALSE(filter.FilterRTCP(556));
-  ASSERT_FALSE(filter.FilterRTCPReceiverReport(555));
+  ASSERT_TRUE(Filter(filter, 0, 16, 110));
+
+  // rtcp_sr_s16 has 16 as an SSRC
+  ASSERT_EQ(MediaPipelineFilter::PASS,
+            filter.FilterRTCP(rtcp_sr_s16, sizeof(rtcp_sr_s16)));
 }
 
 TEST_F(MediaPipelineFilterTest, TestAnswerAddsSSRCs) {
   MediaPipelineFilter filter;
   filter.SetCorrelator(7777);
   ASSERT_TRUE(Filter(filter, 7777, 555, 110));
   ASSERT_FALSE(Filter(filter, 7778, 556, 110));
   // This should also have resulted in the SSRC 555 being added to the filter
@@ -478,18 +708,18 @@ TEST_F(MediaPipelineFilterTest, TestSSRC
   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));
 }
 
 TEST_F(MediaPipelineFilterTest, TestRemoteSDPNoSSRCs) {
   // If the remote SDP doesn't have SSRCs, right now this is a no-op and
-  // there is no point of even incorporating a filter, but we make the behavior
-  // consistent to avoid confusion.
+  // there is no point of even incorporating a filter, but we make the 
+  // behavior consistent to avoid confusion.
   MediaPipelineFilter filter;
   filter.SetCorrelator(7777);
   filter.AddUniquePT(111);
   ASSERT_TRUE(Filter(filter, 7777, 555, 110));
 
   MediaPipelineFilter filter2;
 
   filter.IncorporateRemoteDescription(filter2);