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}
diff --git a/webrtc/call/rtp_demuxer.cc b/webrtc/call/rtp_demuxer.cc
index b616f6f..205498f 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 @@
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 @@
}
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 @@
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 @@
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 5060a80..e557c4d 100644
--- a/webrtc/call/rtp_demuxer.h
+++ b/webrtc/call/rtp_demuxer.h
@@ -12,7 +12,6 @@
#define WEBRTC_CALL_RTP_DEMUXER_H_
#include <map>
-#include <set>
#include <string>
#include <vector>
@@ -60,9 +59,6 @@
// 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 @@
// 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_;
diff --git a/webrtc/call/rtp_demuxer_unittest.cc b/webrtc/call/rtp_demuxer_unittest.cc
index 56bfe3c..6d61f57 100644
--- a/webrtc/call/rtp_demuxer_unittest.cc
+++ b/webrtc/call/rtp_demuxer_unittest.cc
@@ -381,18 +381,22 @@
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 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 @@
}
}
-// 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;
