diff --git a/talk/session/media/bundlefilter.cc b/talk/session/media/bundlefilter.cc index b47d47fb27..670befeb7d 100755 --- a/talk/session/media/bundlefilter.cc +++ b/talk/session/media/bundlefilter.cc @@ -32,78 +32,29 @@ namespace cricket { -static const uint32_t kSsrc01 = 0x01; - BundleFilter::BundleFilter() { } BundleFilter::~BundleFilter() { } -bool BundleFilter::DemuxPacket(const char* data, size_t len, bool rtcp) { - // For rtp packets, we check whether the payload type can be found. - // For rtcp packets, we check whether the ssrc can be found or is the special - // value 1 except for SDES packets which always pass through. Plus, if - // |streams_| is empty, we will allow all rtcp packets pass through provided - // that they are valid rtcp packets in case that they are for early media. - if (!rtcp) { - // It may not be a RTP packet (e.g. SCTP). - if (!IsRtpPacket(data, len)) - return false; - - int payload_type = 0; - if (!GetRtpPayloadType(data, len, &payload_type)) { - return false; - } - return FindPayloadType(payload_type); +bool BundleFilter::DemuxPacket(const uint8_t* data, size_t len) { + // For RTP packets, we check whether the payload type can be found. + if (!IsRtpPacket(data, len)) { + return false; } - // Rtcp packets using ssrc filter. - int pl_type = 0; - uint32_t ssrc = 0; - if (!GetRtcpType(data, len, &pl_type)) return false; - if (pl_type == kRtcpTypeSDES) { - // SDES packet parsing not supported. - LOG(LS_INFO) << "SDES packet received for demux."; - return true; - } else { - if (!GetRtcpSsrc(data, len, &ssrc)) return false; - if (ssrc == kSsrc01) { - // SSRC 1 has a special meaning and indicates generic feedback on - // some systems and should never be dropped. If it is forwarded - // incorrectly it will be ignored by lower layers anyway. - return true; - } + int payload_type = 0; + if (!GetRtpPayloadType(data, len, &payload_type)) { + return false; } - // Pass through if |streams_| is empty to allow early rtcp packets in. - return !HasStreams() || FindStream(ssrc); + return FindPayloadType(payload_type); } void BundleFilter::AddPayloadType(int payload_type) { payload_types_.insert(payload_type); } -bool BundleFilter::AddStream(const StreamParams& stream) { - if (GetStreamBySsrc(streams_, stream.first_ssrc())) { - LOG(LS_WARNING) << "Stream already added to filter"; - return false; - } - streams_.push_back(stream); - return true; -} - -bool BundleFilter::RemoveStream(uint32_t ssrc) { - return RemoveStreamBySsrc(&streams_, ssrc); -} - -bool BundleFilter::HasStreams() const { - return !streams_.empty(); -} - -bool BundleFilter::FindStream(uint32_t ssrc) const { - return ssrc == 0 ? false : GetStreamBySsrc(streams_, ssrc) != nullptr; -} - bool BundleFilter::FindPayloadType(int pl_type) const { return payload_types_.find(pl_type) != payload_types_.end(); } diff --git a/talk/session/media/bundlefilter.h b/talk/session/media/bundlefilter.h index 3717376668..d9d952f4ee 100755 --- a/talk/session/media/bundlefilter.h +++ b/talk/session/media/bundlefilter.h @@ -28,6 +28,8 @@ #ifndef TALK_SESSION_MEDIA_BUNDLEFILTER_H_ #define TALK_SESSION_MEDIA_BUNDLEFILTER_H_ +#include + #include #include @@ -37,42 +39,31 @@ namespace cricket { // In case of single RTP session and single transport channel, all session -// ( or media) channels share a common transport channel. Hence they all get +// (or media) channels share a common transport channel. Hence they all get // SignalReadPacket when packet received on transport channel. This requires // cricket::BaseChannel to know all the valid sources, else media channel // will decode invalid packets. // // This class determines whether a packet is destined for cricket::BaseChannel. -// For rtp packets, this is decided based on the payload type. For rtcp packets, -// this is decided based on the sender ssrc values. +// This is only to be used for RTP packets as RTCP packets are not filtered. +// For RTP packets, this is decided based on the payload type. class BundleFilter { public: BundleFilter(); ~BundleFilter(); - // Determines packet belongs to valid cricket::BaseChannel. - bool DemuxPacket(const char* data, size_t len, bool rtcp); + // Determines if a RTP packet belongs to valid cricket::BaseChannel. + bool DemuxPacket(const uint8_t* data, size_t len); // Adds the supported payload type. void AddPayloadType(int payload_type); - // Adding a valid source to the filter. - bool AddStream(const StreamParams& stream); - - // Removes source from the filter. - bool RemoveStream(uint32_t ssrc); - - // Utility methods added for unitest. - // True if |streams_| is not empty. - bool HasStreams() const; - bool FindStream(uint32_t ssrc) const; + // Public for unittests. bool FindPayloadType(int pl_type) const; void ClearAllPayloadTypes(); - private: std::set payload_types_; - std::vector streams_; }; } // namespace cricket diff --git a/talk/session/media/bundlefilter_unittest.cc b/talk/session/media/bundlefilter_unittest.cc index 806d6bab09..f2c35fc1d8 100755 --- a/talk/session/media/bundlefilter_unittest.cc +++ b/talk/session/media/bundlefilter_unittest.cc @@ -30,9 +30,6 @@ using cricket::StreamParams; -static const int kSsrc1 = 0x1111; -static const int kSsrc2 = 0x2222; -static const int kSsrc3 = 0x3333; static const int kPayloadType1 = 0x11; static const int kPayloadType2 = 0x22; static const int kPayloadType3 = 0x33; @@ -55,56 +52,6 @@ static const unsigned char kRtpPacketPt3Ssrc2[] = { 0x22, }; -// PT = 200 = SR, len = 28, SSRC of sender = 0x0001 -// NTP TS = 0, RTP TS = 0, packet count = 0 -static const unsigned char kRtcpPacketSrSsrc01[] = { - 0x80, 0xC8, 0x00, 0x1B, 0x00, 0x00, 0x00, 0x01, - 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, - 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, - 0x00, 0x00, 0x00, 0x00, -}; - -// PT = 200 = SR, len = 28, SSRC of sender = 0x2222 -// NTP TS = 0, RTP TS = 0, packet count = 0 -static const unsigned char kRtcpPacketSrSsrc2[] = { - 0x80, 0xC8, 0x00, 0x1B, 0x00, 0x00, 0x22, 0x22, - 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, - 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, - 0x00, 0x00, 0x00, 0x00, -}; - -// First packet - SR = PT = 200, len = 0, SSRC of sender = 0x1111 -// NTP TS = 0, RTP TS = 0, packet count = 0 -// second packet - SDES = PT = 202, count = 0, SSRC = 0x1111, cname len = 0 -static const unsigned char kRtcpPacketCompoundSrSdesSsrc1[] = { - 0x80, 0xC8, 0x00, 0x01, 0x00, 0x00, 0x11, 0x11, - 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, - 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, - 0x00, 0x00, 0x00, 0x00, - 0x81, 0xCA, 0x00, 0x00, 0x00, 0x00, 0x11, 0x11, 0x01, 0x00, -}; - -// SDES = PT = 202, count = 0, SSRC = 0x2222, cname len = 0 -static const unsigned char kRtcpPacketSdesSsrc2[] = { - 0x81, 0xCA, 0x00, 0x00, 0x00, 0x00, 0x22, 0x22, 0x01, 0x00, -}; - -// Packet has only mandatory fixed RTCP header -static const unsigned char kRtcpPacketFixedHeaderOnly[] = { - 0x80, 0xC8, 0x00, 0x00, -}; - -// Small packet for SSRC demux. -static const unsigned char kRtcpPacketTooSmall[] = { - 0x80, 0xC8, 0x00, 0x00, 0x00, 0x00, -}; - -// PT = 206, FMT = 1, Sender SSRC = 0x1111, Media SSRC = 0x1111 -// No FCI information is needed for PLI. -static const unsigned char kRtcpPacketNonCompoundRtcpPliFeedback[] = { - 0x81, 0xCE, 0x00, 0x0C, 0x00, 0x00, 0x11, 0x11, 0x00, 0x00, 0x11, 0x11, -}; - // An SCTP packet. static const unsigned char kSctpPacket[] = { 0x00, 0x01, 0x00, 0x01, @@ -114,100 +61,29 @@ static const unsigned char kSctpPacket[] = { 0x00, 0x00, 0x00, 0x00, }; -TEST(BundleFilterTest, AddRemoveStreamTest) { - cricket::BundleFilter bundle_filter; - EXPECT_FALSE(bundle_filter.HasStreams()); - EXPECT_TRUE(bundle_filter.AddStream(StreamParams::CreateLegacy(kSsrc1))); - StreamParams stream2; - stream2.ssrcs.push_back(kSsrc2); - stream2.ssrcs.push_back(kSsrc3); - EXPECT_TRUE(bundle_filter.AddStream(stream2)); - - EXPECT_TRUE(bundle_filter.HasStreams()); - EXPECT_TRUE(bundle_filter.FindStream(kSsrc1)); - EXPECT_TRUE(bundle_filter.FindStream(kSsrc2)); - EXPECT_TRUE(bundle_filter.FindStream(kSsrc3)); - EXPECT_TRUE(bundle_filter.RemoveStream(kSsrc1)); - EXPECT_FALSE(bundle_filter.FindStream(kSsrc1)); - EXPECT_TRUE(bundle_filter.RemoveStream(kSsrc3)); - EXPECT_FALSE(bundle_filter.RemoveStream(kSsrc2)); // Already removed. - EXPECT_FALSE(bundle_filter.HasStreams()); -} - TEST(BundleFilterTest, RtpPacketTest) { cricket::BundleFilter bundle_filter; bundle_filter.AddPayloadType(kPayloadType1); - EXPECT_TRUE(bundle_filter.DemuxPacket( - reinterpret_cast(kRtpPacketPt1Ssrc1), - sizeof(kRtpPacketPt1Ssrc1), false)); + EXPECT_TRUE(bundle_filter.DemuxPacket(kRtpPacketPt1Ssrc1, + sizeof(kRtpPacketPt1Ssrc1))); bundle_filter.AddPayloadType(kPayloadType2); - EXPECT_TRUE(bundle_filter.DemuxPacket( - reinterpret_cast(kRtpPacketPt2Ssrc2), - sizeof(kRtpPacketPt2Ssrc2), false)); + EXPECT_TRUE(bundle_filter.DemuxPacket(kRtpPacketPt2Ssrc2, + sizeof(kRtpPacketPt2Ssrc2))); // Payload type 0x33 is not added. - EXPECT_FALSE(bundle_filter.DemuxPacket( - reinterpret_cast(kRtpPacketPt3Ssrc2), - sizeof(kRtpPacketPt3Ssrc2), false)); + EXPECT_FALSE(bundle_filter.DemuxPacket(kRtpPacketPt3Ssrc2, + sizeof(kRtpPacketPt3Ssrc2))); // Size is too small. - EXPECT_FALSE(bundle_filter.DemuxPacket( - reinterpret_cast(kRtpPacketPt1Ssrc1), 11, false)); + EXPECT_FALSE(bundle_filter.DemuxPacket(kRtpPacketPt1Ssrc1, 11)); bundle_filter.ClearAllPayloadTypes(); - EXPECT_FALSE(bundle_filter.DemuxPacket( - reinterpret_cast(kRtpPacketPt1Ssrc1), - sizeof(kRtpPacketPt1Ssrc1), false)); - EXPECT_FALSE(bundle_filter.DemuxPacket( - reinterpret_cast(kRtpPacketPt2Ssrc2), - sizeof(kRtpPacketPt2Ssrc2), false)); -} - -TEST(BundleFilterTest, RtcpPacketTest) { - cricket::BundleFilter bundle_filter; - EXPECT_TRUE(bundle_filter.AddStream(StreamParams::CreateLegacy(kSsrc1))); - EXPECT_TRUE(bundle_filter.DemuxPacket( - reinterpret_cast(kRtcpPacketCompoundSrSdesSsrc1), - sizeof(kRtcpPacketCompoundSrSdesSsrc1), true)); - EXPECT_TRUE(bundle_filter.AddStream(StreamParams::CreateLegacy(kSsrc2))); - EXPECT_TRUE(bundle_filter.DemuxPacket( - reinterpret_cast(kRtcpPacketSrSsrc2), - sizeof(kRtcpPacketSrSsrc2), true)); - EXPECT_TRUE(bundle_filter.DemuxPacket( - reinterpret_cast(kRtcpPacketSdesSsrc2), - sizeof(kRtcpPacketSdesSsrc2), true)); - EXPECT_TRUE(bundle_filter.RemoveStream(kSsrc2)); - // RTCP Packets other than SR and RR are demuxed regardless of SSRC. - EXPECT_TRUE(bundle_filter.DemuxPacket( - reinterpret_cast(kRtcpPacketSdesSsrc2), - sizeof(kRtcpPacketSdesSsrc2), true)); - // RTCP Packets with 'special' SSRC 0x01 are demuxed also - EXPECT_TRUE(bundle_filter.DemuxPacket( - reinterpret_cast(kRtcpPacketSrSsrc01), - sizeof(kRtcpPacketSrSsrc01), true)); - EXPECT_FALSE(bundle_filter.DemuxPacket( - reinterpret_cast(kRtcpPacketSrSsrc2), - sizeof(kRtcpPacketSrSsrc2), true)); - EXPECT_FALSE(bundle_filter.DemuxPacket( - reinterpret_cast(kRtcpPacketFixedHeaderOnly), - sizeof(kRtcpPacketFixedHeaderOnly), true)); - EXPECT_FALSE(bundle_filter.DemuxPacket( - reinterpret_cast(kRtcpPacketTooSmall), - sizeof(kRtcpPacketTooSmall), true)); - EXPECT_TRUE(bundle_filter.DemuxPacket( - reinterpret_cast(kRtcpPacketNonCompoundRtcpPliFeedback), - sizeof(kRtcpPacketNonCompoundRtcpPliFeedback), true)); - // If the streams_ is empty, rtcp packet passes through - EXPECT_TRUE(bundle_filter.RemoveStream(kSsrc1)); - EXPECT_FALSE(bundle_filter.HasStreams()); - EXPECT_TRUE(bundle_filter.DemuxPacket( - reinterpret_cast(kRtcpPacketSrSsrc2), - sizeof(kRtcpPacketSrSsrc2), true)); + EXPECT_FALSE(bundle_filter.DemuxPacket(kRtpPacketPt1Ssrc1, + sizeof(kRtpPacketPt1Ssrc1))); + EXPECT_FALSE(bundle_filter.DemuxPacket(kRtpPacketPt2Ssrc2, + sizeof(kRtpPacketPt2Ssrc2))); } TEST(BundleFilterTest, InvalidRtpPacket) { cricket::BundleFilter bundle_filter; - EXPECT_TRUE(bundle_filter.AddStream(StreamParams::CreateLegacy(kSsrc1))); - EXPECT_FALSE(bundle_filter.DemuxPacket( - reinterpret_cast(kSctpPacket), - sizeof(kSctpPacket), false)); + EXPECT_FALSE(bundle_filter.DemuxPacket(kSctpPacket, sizeof(kSctpPacket))); } diff --git a/talk/session/media/channel.cc b/talk/session/media/channel.cc index ae27e332f8..8d3efec4fa 100644 --- a/talk/session/media/channel.cc +++ b/talk/session/media/channel.cc @@ -628,9 +628,12 @@ bool BaseChannel::WantsPacket(bool rtcp, rtc::Buffer* packet) { << " packet: wrong size=" << packet->size(); return false; } - - // Bundle filter handles both rtp and rtcp packets. - return bundle_filter_.DemuxPacket(packet->data(), packet->size(), rtcp); + if (rtcp) { + // Permit all (seemingly valid) RTCP packets. + return true; + } + // Check whether we handle this payload. + return bundle_filter_.DemuxPacket(packet->data(), packet->size()); } void BaseChannel::HandlePacket(bool rtcp, rtc::Buffer* packet, @@ -1075,15 +1078,11 @@ bool BaseChannel::SetRtcpMux_w(bool enable, ContentAction action, bool BaseChannel::AddRecvStream_w(const StreamParams& sp) { ASSERT(worker_thread() == rtc::Thread::Current()); - if (!media_channel()->AddRecvStream(sp)) - return false; - - return bundle_filter_.AddStream(sp); + return media_channel()->AddRecvStream(sp); } bool BaseChannel::RemoveRecvStream_w(uint32_t ssrc) { ASSERT(worker_thread() == rtc::Thread::Current()); - bundle_filter_.RemoveStream(ssrc); return media_channel()->RemoveRecvStream(ssrc); } diff --git a/talk/session/media/channel_unittest.cc b/talk/session/media/channel_unittest.cc index 28ab3d294a..6866ecea77 100644 --- a/talk/session/media/channel_unittest.cc +++ b/talk/session/media/channel_unittest.cc @@ -1474,12 +1474,6 @@ class ChannelTest : public testing::Test, public sigslot::has_slots<> { EXPECT_TRUE(channel2_->bundle_filter()->FindPayloadType(pl_type1)); EXPECT_FALSE(channel1_->bundle_filter()->FindPayloadType(pl_type2)); EXPECT_FALSE(channel2_->bundle_filter()->FindPayloadType(pl_type2)); - // channel1 - should only have media_content2 as remote. i.e. kSsrc2 - EXPECT_TRUE(channel1_->bundle_filter()->FindStream(kSsrc2)); - EXPECT_FALSE(channel1_->bundle_filter()->FindStream(kSsrc1)); - // channel2 - should only have media_content1 as remote. i.e. kSsrc1 - EXPECT_TRUE(channel2_->bundle_filter()->FindStream(kSsrc1)); - EXPECT_FALSE(channel2_->bundle_filter()->FindStream(kSsrc2)); // Both channels can receive pl_type1 only. EXPECT_TRUE(SendCustomRtp1(kSsrc1, ++sequence_number1_1, pl_type1)); @@ -1504,8 +1498,9 @@ class ChannelTest : public testing::Test, public sigslot::has_slots<> { EXPECT_TRUE(SendCustomRtcp1(kSsrc2)); EXPECT_TRUE(SendCustomRtcp2(kSsrc1)); - EXPECT_FALSE(CheckCustomRtcp1(kSsrc1)); - EXPECT_FALSE(CheckCustomRtcp2(kSsrc2)); + // Bundle filter shouldn't filter out any RTCP. + EXPECT_TRUE(CheckCustomRtcp1(kSsrc1)); + EXPECT_TRUE(CheckCustomRtcp2(kSsrc2)); } // Test that the media monitor can be run and gives timely callbacks.