admin/webrtc_m130
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Remove RtpDemuxer tweak for preventing multiple RSID inspections

Browse Source 
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}
This commit is contained in:
eladalon 2017-06-30 06:26:54 -07:00 committed by Commit Bot
parent 49085ef280
commit 9addbebf42
3 changed files with 15 additions and 74 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

34
webrtc/call/rtp_demuxer.cc
View File

@ -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;

13
webrtc/call/rtp_demuxer.h
View File

@ -12,7 +12,6 @@
#define WEBRTC_CALL_RTP_DEMUXER_H_
#include <map>
#include <set>
#include <string>
#include <vector>
@ -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<std::string, RtpPacketSinkInterface*> 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<uint32_t> 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<RsidResolutionObserver*> rsid_resolution_observers_;

42
webrtc/call/rtp_demuxer_unittest.cc
View File

@ -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<MockRsidResolutionObserver> 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;