From 9addbebf427a8a49cd5a43cf4827c7c8b6e24c21 Mon Sep 17 00:00:00 2001 From: eladalon Date: Fri, 30 Jun 2017 06:26:54 -0700 Subject: [PATCH] Remove RtpDemuxer tweak for preventing multiple RSID inspections We have a tweak preventing multiple deep-examinations of packets; packets with a given SSRC are only inspected deeply (RSID) once (only the first received packet). Once we move to many-to-one stream-to-sink associations, this becomes less useful, and is better removed. BUG=webrtc:7135 Review-Url: https://codereview.webrtc.org/2955373002 Cr-Commit-Position: refs/heads/master@{#18859} --- webrtc/call/rtp_demuxer.cc | 34 +++++------------------ webrtc/call/rtp_demuxer.h | 13 --------- webrtc/call/rtp_demuxer_unittest.cc | 42 +++++++---------------------- 3 files changed, 15 insertions(+), 74 deletions(-) diff --git a/webrtc/call/rtp_demuxer.cc b/webrtc/call/rtp_demuxer.cc index b616f6f09e..205498fd84 100644 --- a/webrtc/call/rtp_demuxer.cc +++ b/webrtc/call/rtp_demuxer.cc @@ -20,10 +20,6 @@ namespace webrtc { -namespace { -constexpr size_t kMaxProcessedSsrcs = 1000; // Prevent memory overuse. -} // namespace - RtpDemuxer::RtpDemuxer() = default; RtpDemuxer::~RtpDemuxer() { @@ -43,9 +39,6 @@ void RtpDemuxer::AddSink(const std::string& rsid, RTC_DCHECK(!MultimapAssociationExists(rsid_sinks_, rsid, sink)); rsid_sinks_.emplace(rsid, sink); - - // This RSID might now map to an SSRC which we saw earlier. - processed_ssrcs_.clear(); } bool RtpDemuxer::RemoveSink(const RtpPacketSinkInterface* sink) { @@ -65,7 +58,12 @@ void RtpDemuxer::RecordSsrcToSinkAssociation(uint32_t ssrc, } bool RtpDemuxer::OnRtpPacket(const RtpPacketReceived& packet) { - ResolveAssociations(packet); + // TODO(eladalon): This will now check every single packet, but soon a CL will + // be added which will change the many-to-many association of packets to sinks + // to a many-to-one, meaning each packet will be associated with one sink + // at most. Then, only packets with an unknown SSRC will be checked for RSID. + ResolveRsidToSsrcAssociations(packet); + auto it_range = ssrc_sinks_.equal_range(packet.Ssrc()); for (auto it = it_range.first; it != it_range.second; ++it) { it->second->OnRtpPacket(packet); @@ -79,8 +77,6 @@ void RtpDemuxer::RegisterRsidResolutionObserver( RTC_DCHECK(!ContainerHasKey(rsid_resolution_observers_, observer)); rsid_resolution_observers_.push_back(observer); - - processed_ssrcs_.clear(); // New observer requires new notifications. } void RtpDemuxer::DeregisterRsidResolutionObserver( @@ -92,24 +88,6 @@ void RtpDemuxer::DeregisterRsidResolutionObserver( rsid_resolution_observers_.erase(it); } -void RtpDemuxer::ResolveAssociations(const RtpPacketReceived& packet) { - // Avoid expensive string comparisons for RSID by looking the sinks up only - // by SSRC whenever possible. - if (processed_ssrcs_.find(packet.Ssrc()) != processed_ssrcs_.cend()) { - return; - } - - ResolveRsidToSsrcAssociations(packet); - - if (processed_ssrcs_.size() < kMaxProcessedSsrcs) { // Prevent memory overuse - processed_ssrcs_.insert(packet.Ssrc()); // Avoid re-examining in-depth. - } else if (!logged_max_processed_ssrcs_exceeded_) { - LOG(LS_WARNING) << "More than " << kMaxProcessedSsrcs - << " different SSRCs seen."; - logged_max_processed_ssrcs_exceeded_ = true; - } -} - void RtpDemuxer::ResolveRsidToSsrcAssociations( const RtpPacketReceived& packet) { std::string rsid; diff --git a/webrtc/call/rtp_demuxer.h b/webrtc/call/rtp_demuxer.h index 5060a80a70..e557c4dbcc 100644 --- a/webrtc/call/rtp_demuxer.h +++ b/webrtc/call/rtp_demuxer.h @@ -12,7 +12,6 @@ #define WEBRTC_CALL_RTP_DEMUXER_H_ #include -#include #include #include @@ -60,9 +59,6 @@ class RtpDemuxer { // packet reception. void RecordSsrcToSinkAssociation(uint32_t ssrc, RtpPacketSinkInterface* sink); - // When a new packet arrives, we attempt to resolve extra associations. - void ResolveAssociations(const RtpPacketReceived& packet); - // Find the associations of RSID to SSRCs. void ResolveRsidToSsrcAssociations(const RtpPacketReceived& packet); @@ -79,15 +75,6 @@ class RtpDemuxer { // from this container, and moved into |ssrc_sinks_|. std::multimap rsid_sinks_; - // Iterating over |rsid_sinks_| for each incoming and performing multiple - // string comparisons is of non-trivial cost. To avoid this cost, we only - // check RSIDs for the first packet on each incoming SSRC stream. - // (If RSID associations are added later, we check again.) - std::set processed_ssrcs_; - - // Avoid an attack that would create excessive logging. - bool logged_max_processed_ssrcs_exceeded_ = false; - // Observers which will be notified when an RSID association to an SSRC is // resolved by this object. std::vector rsid_resolution_observers_; diff --git a/webrtc/call/rtp_demuxer_unittest.cc b/webrtc/call/rtp_demuxer_unittest.cc index 56bfe3c65f..6d61f57894 100644 --- a/webrtc/call/rtp_demuxer_unittest.cc +++ b/webrtc/call/rtp_demuxer_unittest.cc @@ -381,18 +381,22 @@ TEST(RtpDemuxerTest, FirstSsrcAssociatedWithAnRsidIsNotForgotten) { EXPECT_CALL(sink_a, OnRtpPacket(SamePacketAs(*packet_a))).Times(1); EXPECT_TRUE(demuxer.OnRtpPacket(*packet_a)); - // Second, a packet with |rsid_b| is received. Its RSID is ignored. + // Second, a packet with |rsid_b| is received. We guarantee that |sink_a| + // would receive it, and make no guarantees about |sink_b|. auto packet_b = CreateRtpPacketReceivedWithRsid(rsid_b, shared_ssrc, 20); EXPECT_CALL(sink_a, OnRtpPacket(SamePacketAs(*packet_b))).Times(1); + EXPECT_CALL(sink_b, OnRtpPacket(SamePacketAs(*packet_b))).Times(AtLeast(0)); EXPECT_TRUE(demuxer.OnRtpPacket(*packet_b)); // Known edge-case; adding a new RSID association makes us re-examine all // SSRCs. |sink_b| may or may not be associated with the SSRC now; we make // no promises on that. We do however still guarantee that |sink_a| still // receives the new packets. - MockRtpPacketSink sink_ignored; - demuxer.AddSink("ignored", &sink_ignored); - auto packet_c = CreateRtpPacketReceivedWithRsid(rsid_b, shared_ssrc, 30); + MockRtpPacketSink sink_c; + const std::string rsid_c = "c"; + constexpr uint32_t some_other_ssrc = shared_ssrc + 1; + demuxer.AddSink(some_other_ssrc, &sink_c); + auto packet_c = CreateRtpPacketReceivedWithRsid(rsid_c, shared_ssrc, 30); EXPECT_CALL(sink_a, OnRtpPacket(SamePacketAs(*packet_c))).Times(1); EXPECT_CALL(sink_b, OnRtpPacket(SamePacketAs(*packet_c))).Times(AtLeast(0)); EXPECT_TRUE(demuxer.OnRtpPacket(*packet_c)); @@ -400,7 +404,7 @@ TEST(RtpDemuxerTest, FirstSsrcAssociatedWithAnRsidIsNotForgotten) { // Test tear-down demuxer.RemoveSink(&sink_a); demuxer.RemoveSink(&sink_b); - demuxer.RemoveSink(&sink_ignored); + demuxer.RemoveSink(&sink_c); } TEST(RtpDemuxerTest, MultipleRsidsOnSameSink) { @@ -518,34 +522,6 @@ TEST(RtpDemuxerTest, RsidObserversInformedOfResolutions) { } } -// Normally, we only produce one notification per resolution (though no such -// guarantee is made), but when a new observer is added, we reset -// this suppression - we "re-resolve" associations for the benefit of the -// new observer.. -TEST(RtpDemuxerTest, NotificationSuppressionResetWhenNewObserverAdded) { - RtpDemuxer demuxer; - - constexpr uint32_t ssrc = 111; - const std::string rsid = "a"; - - // First observer registered, then gets a notification. - NiceMock first_observer; - demuxer.RegisterRsidResolutionObserver(&first_observer); - demuxer.OnRtpPacket(*CreateRtpPacketReceivedWithRsid(rsid, ssrc)); - - // Second observer registered, then gets a notification. No guarantee is made - // about whether the first observer would get an additional notification. - MockRsidResolutionObserver second_observer; - demuxer.RegisterRsidResolutionObserver(&second_observer); - EXPECT_CALL(first_observer, OnRsidResolved(rsid, ssrc)).Times(AtLeast(0)); - EXPECT_CALL(second_observer, OnRsidResolved(rsid, ssrc)).Times(1); - demuxer.OnRtpPacket(*CreateRtpPacketReceivedWithRsid(rsid, ssrc)); - - // Test tear-down - demuxer.DeregisterRsidResolutionObserver(&first_observer); - demuxer.DeregisterRsidResolutionObserver(&second_observer); -} - TEST(RtpDemuxerTest, DeregisteredRsidObserversNotInformedOfResolutions) { RtpDemuxer demuxer;