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 170540 1dea344fa608897709f2e674ee726ec858c21da2
parent 170539 4f7eac2b16ca293cbbfab61c1220b65990467186
child 170541 53d8b186e5742151279233834e73eb0eef02a6c9
push id270
push userpvanderbeken@mozilla.com
push dateThu, 06 Mar 2014 09:24:21 +0000
reviewersabr
bugs786234
milestone30.0a1
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);