From 8fddf1f7b34bae7d5a3eb45170f9a827664c4eba Mon Sep 17 00:00:00 2001 From: Danil Chapovalov Date: Wed, 8 Jul 2020 18:32:52 +0200 Subject: [PATCH] Delete callbacks from RtpDemuxer on ssrc binding It was only used by RtcpDemuxer that is now deleted Bug: None Change-Id: Ief2c285e293cde3ed7576b194d2df137d6cbad38 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/178902 Reviewed-by: Sebastian Jansson Commit-Queue: Danil Chapovalov Cr-Commit-Position: refs/heads/master@{#31760} --- call/BUILD.gn | 1 - call/rtp_demuxer.cc | 68 +--------- call/rtp_demuxer.h | 22 +--- call/rtp_demuxer_unittest.cc | 245 ----------------------------------- call/ssrc_binding_observer.h | 40 ------ 5 files changed, 9 insertions(+), 367 deletions(-) delete mode 100644 call/ssrc_binding_observer.h diff --git a/call/BUILD.gn b/call/BUILD.gn index db4261a4a5..65b545c11e 100644 --- a/call/BUILD.gn +++ b/call/BUILD.gn @@ -116,7 +116,6 @@ rtc_library("rtp_receiver") { "rtp_stream_receiver_controller.h", "rtx_receive_stream.cc", "rtx_receive_stream.h", - "ssrc_binding_observer.h", ] deps = [ ":rtp_interfaces", diff --git a/call/rtp_demuxer.cc b/call/rtp_demuxer.cc index 86a6e7f87b..9fc4ba1c16 100644 --- a/call/rtp_demuxer.cc +++ b/call/rtp_demuxer.cc @@ -11,7 +11,6 @@ #include "call/rtp_demuxer.h" #include "call/rtp_packet_sink_interface.h" -#include "call/ssrc_binding_observer.h" #include "modules/rtp_rtcp/source/rtp_header_extensions.h" #include "modules/rtp_rtcp/source/rtp_packet_received.h" #include "rtc_base/checks.h" @@ -49,11 +48,6 @@ size_t RemoveFromMapByValue(Map* map, const Value& value) { return count; } -template -bool ContainerHasKey(const Container& c, const Key& k) { - return std::find(c.cbegin(), c.cend(), k) != c.cend(); -} - } // namespace RtpDemuxerCriteria::RtpDemuxerCriteria() = default; @@ -105,7 +99,6 @@ RtpDemuxer::~RtpDemuxer() { RTC_DCHECK(sinks_by_pt_.empty()); RTC_DCHECK(sink_by_mid_and_rsid_.empty()); RTC_DCHECK(sink_by_rsid_.empty()); - RTC_DCHECK(ssrc_binding_observers_.empty()); } bool RtpDemuxer::AddSink(const RtpDemuxerCriteria& criteria, @@ -362,12 +355,7 @@ RtpPacketSinkInterface* RtpDemuxer::ResolveSinkByMid(const std::string& mid, const auto it = sink_by_mid_.find(mid); if (it != sink_by_mid_.end()) { RtpPacketSinkInterface* sink = it->second; - bool notify = AddSsrcSinkBinding(ssrc, sink); - if (notify) { - for (auto* observer : ssrc_binding_observers_) { - observer->OnSsrcBoundToMid(mid, ssrc); - } - } + AddSsrcSinkBinding(ssrc, sink); return sink; } return nullptr; @@ -380,39 +368,22 @@ RtpPacketSinkInterface* RtpDemuxer::ResolveSinkByMidRsid( const auto it = sink_by_mid_and_rsid_.find(std::make_pair(mid, rsid)); if (it != sink_by_mid_and_rsid_.end()) { RtpPacketSinkInterface* sink = it->second; - bool notify = AddSsrcSinkBinding(ssrc, sink); - if (notify) { - for (auto* observer : ssrc_binding_observers_) { - observer->OnSsrcBoundToMidRsid(mid, rsid, ssrc); - } - } + AddSsrcSinkBinding(ssrc, sink); return sink; } return nullptr; } -void RtpDemuxer::RegisterRsidResolutionObserver(SsrcBindingObserver* observer) { - RegisterSsrcBindingObserver(observer); -} RtpPacketSinkInterface* RtpDemuxer::ResolveSinkByRsid(const std::string& rsid, uint32_t ssrc) { const auto it = sink_by_rsid_.find(rsid); if (it != sink_by_rsid_.end()) { RtpPacketSinkInterface* sink = it->second; - bool notify = AddSsrcSinkBinding(ssrc, sink); - if (notify) { - for (auto* observer : ssrc_binding_observers_) { - observer->OnSsrcBoundToRsid(rsid, ssrc); - } - } + AddSsrcSinkBinding(ssrc, sink); return sink; } return nullptr; } -void RtpDemuxer::DeregisterRsidResolutionObserver( - const SsrcBindingObserver* observer) { - DeregisterSsrcBindingObserver(observer); -} RtpPacketSinkInterface* RtpDemuxer::ResolveSinkByPayloadType( uint8_t payload_type, @@ -423,25 +394,20 @@ RtpPacketSinkInterface* RtpDemuxer::ResolveSinkByPayloadType( const auto end = range.second; if (std::next(it) == end) { RtpPacketSinkInterface* sink = it->second; - bool notify = AddSsrcSinkBinding(ssrc, sink); - if (notify) { - for (auto* observer : ssrc_binding_observers_) { - observer->OnSsrcBoundToPayloadType(payload_type, ssrc); - } - } + AddSsrcSinkBinding(ssrc, sink); return sink; } } return nullptr; } -bool RtpDemuxer::AddSsrcSinkBinding(uint32_t ssrc, +void RtpDemuxer::AddSsrcSinkBinding(uint32_t ssrc, RtpPacketSinkInterface* sink) { if (sink_by_ssrc_.size() >= kMaxSsrcBindings) { RTC_LOG(LS_WARNING) << "New SSRC=" << ssrc << " sink binding ignored; limit of" << kMaxSsrcBindings << " bindings has been reached."; - return false; + return; } auto result = sink_by_ssrc_.emplace(ssrc, sink); @@ -450,31 +416,11 @@ bool RtpDemuxer::AddSsrcSinkBinding(uint32_t ssrc, if (inserted) { RTC_LOG(LS_INFO) << "Added sink = " << sink << " binding with SSRC=" << ssrc; - return true; - } - if (it->second != sink) { + } else if (it->second != sink) { RTC_LOG(LS_INFO) << "Updated sink = " << sink << " binding with SSRC=" << ssrc; it->second = sink; - return true; } - return false; -} - -void RtpDemuxer::RegisterSsrcBindingObserver(SsrcBindingObserver* observer) { - RTC_DCHECK(observer); - RTC_DCHECK(!ContainerHasKey(ssrc_binding_observers_, observer)); - - ssrc_binding_observers_.push_back(observer); -} - -void RtpDemuxer::DeregisterSsrcBindingObserver( - const SsrcBindingObserver* observer) { - RTC_DCHECK(observer); - auto it = std::find(ssrc_binding_observers_.begin(), - ssrc_binding_observers_.end(), observer); - RTC_DCHECK(it != ssrc_binding_observers_.end()); - ssrc_binding_observers_.erase(it); } } // namespace webrtc diff --git a/call/rtp_demuxer.h b/call/rtp_demuxer.h index dae7a53b70..3aa7e9df26 100644 --- a/call/rtp_demuxer.h +++ b/call/rtp_demuxer.h @@ -21,7 +21,6 @@ namespace webrtc { class RtpPacketReceived; class RtpPacketSinkInterface; -class SsrcBindingObserver; // This struct describes the criteria that will be used to match packets to a // specific sink. @@ -133,17 +132,6 @@ class RtpDemuxer { // if the packet was forwarded and false if the packet was dropped. bool OnRtpPacket(const RtpPacketReceived& packet); - // The Observer will be notified when an attribute (e.g., RSID, MID, etc.) is - // bound to an SSRC. - void RegisterSsrcBindingObserver(SsrcBindingObserver* observer); - // Deprecated: Use the above method. - void RegisterRsidResolutionObserver(SsrcBindingObserver* observer); - - // Undo a previous RegisterSsrcBindingObserver(). - void DeregisterSsrcBindingObserver(const SsrcBindingObserver* observer); - // Deprecated: Use the above method. - void DeregisterRsidResolutionObserver(const SsrcBindingObserver* observer); - // Configure whether to look at the MID header extension when demuxing // incoming RTP packets. By default this is enabled. void set_use_mid(bool use_mid) { use_mid_ = use_mid; } @@ -200,14 +188,8 @@ class RtpDemuxer { std::map mid_by_ssrc_; std::map rsid_by_ssrc_; - // Adds a binding from the SSRC to the given sink. Returns true if there was - // not already a sink bound to the SSRC or if the sink replaced a different - // sink. Returns false if the binding was unchanged. - bool AddSsrcSinkBinding(uint32_t ssrc, RtpPacketSinkInterface* sink); - - // Observers which will be notified when an RSID association to an SSRC is - // resolved by this object. - std::vector ssrc_binding_observers_; + // Adds a binding from the SSRC to the given sink. + void AddSsrcSinkBinding(uint32_t ssrc, RtpPacketSinkInterface* sink); bool use_mid_ = true; }; diff --git a/call/rtp_demuxer_unittest.cc b/call/rtp_demuxer_unittest.cc index 59baafe9ff..a4abab73ed 100644 --- a/call/rtp_demuxer_unittest.cc +++ b/call/rtp_demuxer_unittest.cc @@ -14,7 +14,6 @@ #include #include -#include "call/ssrc_binding_observer.h" #include "call/test/mock_rtp_packet_sink_interface.h" #include "modules/rtp_rtcp/include/rtp_header_extension_map.h" #include "modules/rtp_rtcp/source/rtp_header_extensions.h" @@ -31,39 +30,15 @@ namespace { using ::testing::_; using ::testing::AtLeast; -using ::testing::AtMost; using ::testing::InSequence; using ::testing::NiceMock; -class MockSsrcBindingObserver : public SsrcBindingObserver { - public: - MOCK_METHOD(void, - OnSsrcBoundToRsid, - (const std::string& rsid, uint32_t ssrc), - (override)); - MOCK_METHOD(void, - OnSsrcBoundToMid, - (const std::string& mid, uint32_t ssrc), - (override)); - MOCK_METHOD(void, - OnSsrcBoundToMidRsid, - (const std::string& mid, const std::string& rsid, uint32_t ssrc), - (override)); - MOCK_METHOD(void, - OnSsrcBoundToPayloadType, - (uint8_t payload_type, uint32_t ssrc), - (override)); -}; - class RtpDemuxerTest : public ::testing::Test { protected: ~RtpDemuxerTest() { for (auto* sink : sinks_to_tear_down_) { demuxer_.RemoveSink(sink); } - for (auto* observer : observers_to_tear_down_) { - demuxer_.DeregisterSsrcBindingObserver(observer); - } } // These are convenience methods for calling demuxer.AddSink with different @@ -111,20 +86,6 @@ class RtpDemuxerTest : public ::testing::Test { return demuxer_.RemoveSink(sink); } - // These are convenience methods for calling - // demuxer.{Register|Unregister}SsrcBindingObserver such that observers are - // automatically removed when the test finishes. - - void RegisterSsrcBindingObserver(SsrcBindingObserver* observer) { - demuxer_.RegisterSsrcBindingObserver(observer); - observers_to_tear_down_.insert(observer); - } - - void DeregisterSsrcBindingObserver(SsrcBindingObserver* observer) { - demuxer_.DeregisterSsrcBindingObserver(observer); - observers_to_tear_down_.erase(observer); - } - // The CreatePacket* methods are helpers for creating new RTP packets with // various attributes set. Tests should use the helper that provides the // minimum information needed to exercise the behavior under test. Tests also @@ -214,7 +175,6 @@ class RtpDemuxerTest : public ::testing::Test { RtpDemuxer demuxer_; std::set sinks_to_tear_down_; - std::set observers_to_tear_down_; uint16_t next_sequence_number_ = 1; }; @@ -756,73 +716,6 @@ TEST_F(RtpDemuxerTest, AssociatingByRsidAndBySsrcCannotTriggerDoubleCall) { EXPECT_TRUE(demuxer_.OnRtpPacket(*packet)); } -TEST_F(RtpDemuxerTest, ObserversNotifiedOfSsrcBoundToMid) { - const std::string mid = "v"; - constexpr uint32_t ssrc = 10; - - NiceMock sink; - AddSinkOnlyMid(mid, &sink); - - MockSsrcBindingObserver observer; - RegisterSsrcBindingObserver(&observer); - - auto packet = CreatePacketWithSsrcMid(ssrc, mid); - EXPECT_CALL(observer, OnSsrcBoundToMid(mid, ssrc)); - EXPECT_TRUE(demuxer_.OnRtpPacket(*packet)); -} - -TEST_F(RtpDemuxerTest, ObserversNotifiedOfSsrcBoundToRsid) { - const std::string rsid = "1"; - constexpr uint32_t ssrc = 111; - - // Only RSIDs which the demuxer knows may be resolved. - NiceMock sink; - AddSinkOnlyRsid(rsid, &sink); - - NiceMock rsid_resolution_observers[3]; - for (auto& observer : rsid_resolution_observers) { - RegisterSsrcBindingObserver(&observer); - EXPECT_CALL(observer, OnSsrcBoundToRsid(rsid, ssrc)).Times(1); - } - - // The expected calls to OnSsrcBoundToRsid() will be triggered by this. - auto packet = CreatePacketWithSsrcRsid(ssrc, rsid); - EXPECT_TRUE(demuxer_.OnRtpPacket(*packet)); -} - -TEST_F(RtpDemuxerTest, ObserversNotifiedOfSsrcBoundToMidRsid) { - const std::string mid = "v"; - const std::string rsid = "1"; - constexpr uint32_t ssrc = 10; - - NiceMock sink; - AddSinkBothMidRsid(mid, rsid, &sink); - - MockSsrcBindingObserver observer; - RegisterSsrcBindingObserver(&observer); - - auto packet = CreatePacketWithSsrcMidRsid(ssrc, mid, rsid); - EXPECT_CALL(observer, OnSsrcBoundToMidRsid(mid, rsid, ssrc)); - EXPECT_TRUE(demuxer_.OnRtpPacket(*packet)); -} - -TEST_F(RtpDemuxerTest, ObserversNotifiedOfSsrcBoundToPayloadType) { - constexpr uint8_t payload_type = 3; - constexpr uint32_t ssrc = 10; - - RtpDemuxerCriteria criteria; - criteria.payload_types = {payload_type}; - NiceMock sink; - AddSink(criteria, &sink); - - MockSsrcBindingObserver observer; - RegisterSsrcBindingObserver(&observer); - - auto packet = CreatePacketWithSsrc(ssrc); - packet->SetPayloadType(payload_type); - EXPECT_CALL(observer, OnSsrcBoundToPayloadType(payload_type, ssrc)); - EXPECT_TRUE(demuxer_.OnRtpPacket(*packet)); -} // If one sink is associated with SSRC x, and another sink with RSID y, then if // we receive a packet with both SSRC x and RSID y, route that to only the sink @@ -857,9 +750,6 @@ TEST_F(RtpDemuxerTest, NiceMock rsid_sink; AddSinkOnlyRsid(rsid, &rsid_sink); - NiceMock observer; - RegisterSsrcBindingObserver(&observer); - // The SSRC was mapped to an SSRC sink, but was even active (packets flowed // over it). auto packet = CreatePacketWithSsrcRsid(ssrc, rsid); @@ -870,7 +760,6 @@ TEST_F(RtpDemuxerTest, // is guaranteed. RemoveSink(&ssrc_sink); EXPECT_CALL(rsid_sink, OnRtpPacket(SamePacketAs(*packet))).Times(AtLeast(0)); - EXPECT_CALL(observer, OnSsrcBoundToRsid(rsid, ssrc)).Times(AtLeast(0)); EXPECT_TRUE(demuxer_.OnRtpPacket(*packet)); } @@ -1365,127 +1254,6 @@ TEST_F(RtpDemuxerTest, PacketWithMidAndUnknownRsidIsNotRoutedByPayloadType) { EXPECT_FALSE(demuxer_.OnRtpPacket(*packet)); } -// Observers are only notified of an SSRC binding to an RSID if we care about -// the RSID (i.e., have a sink added for that RSID). -TEST_F(RtpDemuxerTest, ObserversNotNotifiedOfUntrackedRsids) { - const std::string rsid = "1"; - constexpr uint32_t ssrc = 111; - - MockSsrcBindingObserver rsid_resolution_observers[3]; - for (auto& observer : rsid_resolution_observers) { - RegisterSsrcBindingObserver(&observer); - EXPECT_CALL(observer, OnSsrcBoundToRsid(_, _)).Times(0); - } - - // Since no sink is registered for this SSRC/RSID, expect the packet to not be - // routed and no observers notified of the SSRC -> RSID binding. - EXPECT_FALSE(demuxer_.OnRtpPacket(*CreatePacketWithSsrcRsid(ssrc, rsid))); -} - -// Ensure that observers are notified of SSRC bindings only once per unique -// binding source (e.g., SSRC -> MID, SSRC -> RSID, etc.) -TEST_F(RtpDemuxerTest, ObserversNotifiedOfSsrcBoundtoMidOnlyOnce) { - const std::string mid = "v"; - constexpr uint32_t ssrc = 10; - - NiceMock sink; - AddSinkOnlyMid(mid, &sink); - - MockSsrcBindingObserver observer; - RegisterSsrcBindingObserver(&observer); - - EXPECT_CALL(observer, OnSsrcBoundToMid(mid, ssrc)).Times(1); - - demuxer_.OnRtpPacket(*CreatePacketWithSsrcMid(ssrc, mid)); - demuxer_.OnRtpPacket(*CreatePacketWithSsrcMid(ssrc, mid)); -} - -// Ensure that when a new SSRC -> MID binding is discovered observers are also -// notified of that, even if there has already been an SSRC bound to the MID. -TEST_F(RtpDemuxerTest, ObserversNotifiedOfSsrcBoundtoMidWhenSsrcChanges) { - const std::string mid = "v"; - constexpr uint32_t ssrc1 = 10; - constexpr uint32_t ssrc2 = 11; - - NiceMock sink; - AddSinkOnlyMid(mid, &sink); - - MockSsrcBindingObserver observer; - RegisterSsrcBindingObserver(&observer); - - InSequence seq; - EXPECT_CALL(observer, OnSsrcBoundToMid(mid, ssrc1)).Times(1); - EXPECT_CALL(observer, OnSsrcBoundToMid(mid, ssrc2)).Times(1); - - auto p1 = CreatePacketWithSsrcMid(ssrc1, mid); - demuxer_.OnRtpPacket(*p1); - - auto p2 = CreatePacketWithSsrcMid(ssrc2, mid); - demuxer_.OnRtpPacket(*p2); -} - -TEST_F(RtpDemuxerTest, DeregisteredRsidObserversNotInformedOfResolutions) { - constexpr uint32_t ssrc = 111; - const std::string rsid = "a"; - NiceMock sink; - AddSinkOnlyRsid(rsid, &sink); - - // Register several, then deregister only one, to show that not all of the - // observers had been forgotten when one was removed. - MockSsrcBindingObserver observer_1; - MockSsrcBindingObserver observer_2_removed; - MockSsrcBindingObserver observer_3; - - RegisterSsrcBindingObserver(&observer_1); - RegisterSsrcBindingObserver(&observer_2_removed); - RegisterSsrcBindingObserver(&observer_3); - - DeregisterSsrcBindingObserver(&observer_2_removed); - - EXPECT_CALL(observer_1, OnSsrcBoundToRsid(rsid, ssrc)).Times(1); - EXPECT_CALL(observer_2_removed, OnSsrcBoundToRsid(_, _)).Times(0); - EXPECT_CALL(observer_3, OnSsrcBoundToRsid(rsid, ssrc)).Times(1); - - // The expected calls to OnSsrcBoundToRsid() will be triggered by this. - demuxer_.OnRtpPacket(*CreatePacketWithSsrcRsid(ssrc, rsid)); -} - -TEST_F(RtpDemuxerTest, - PacketFittingBothRsidSinkAndSsrcSinkTriggersResolutionCallbacks) { - constexpr uint32_t ssrc = 111; - NiceMock ssrc_sink; - AddSinkOnlySsrc(ssrc, &ssrc_sink); - - const std::string rsid = "a"; - NiceMock rsid_sink; - AddSinkOnlyRsid(rsid, &rsid_sink); - - MockSsrcBindingObserver observer; - RegisterSsrcBindingObserver(&observer); - - auto packet = CreatePacketWithSsrcRsid(ssrc, rsid); - EXPECT_CALL(observer, OnSsrcBoundToRsid(rsid, ssrc)).Times(1); - demuxer_.OnRtpPacket(*packet); -} - -TEST_F(RtpDemuxerTest, MaliciousPeerCannotCauseMemoryOveruse) { - const std::string mid = "v"; - - NiceMock sink; - AddSinkOnlyMid(mid, &sink); - - MockSsrcBindingObserver observer; - RegisterSsrcBindingObserver(&observer); - - EXPECT_CALL(observer, OnSsrcBoundToMid(_, _)) - .Times(AtMost(RtpDemuxer::kMaxSsrcBindings)); - - for (int i = 0; i < RtpDemuxer::kMaxSsrcBindings + 1; i++) { - auto packet = CreatePacketWithSsrcMid(i, mid); - EXPECT_TRUE(demuxer_.OnRtpPacket(*packet)); - } -} - #if RTC_DCHECK_IS_ON && GTEST_HAS_DEATH_TEST && !defined(WEBRTC_ANDROID) TEST_F(RtpDemuxerDeathTest, CriteriaMustBeNonEmpty) { @@ -1516,19 +1284,6 @@ TEST_F(RtpDemuxerDeathTest, MidMustNotExceedMaximumLength) { EXPECT_DEATH(AddSinkOnlyMid(mid, &sink), ""); } -TEST_F(RtpDemuxerDeathTest, - DoubleRegisterationOfSsrcBindingObserverDisallowed) { - MockSsrcBindingObserver observer; - RegisterSsrcBindingObserver(&observer); - EXPECT_DEATH(RegisterSsrcBindingObserver(&observer), ""); -} - -TEST_F(RtpDemuxerDeathTest, - DregisterationOfNeverRegisteredSsrcBindingObserverDisallowed) { - MockSsrcBindingObserver observer; - EXPECT_DEATH(DeregisterSsrcBindingObserver(&observer), ""); -} - #endif } // namespace diff --git a/call/ssrc_binding_observer.h b/call/ssrc_binding_observer.h deleted file mode 100644 index ada505610f..0000000000 --- a/call/ssrc_binding_observer.h +++ /dev/null @@ -1,40 +0,0 @@ -/* - * Copyright (c) 2017 The WebRTC project authors. All Rights Reserved. - * - * Use of this source code is governed by a BSD-style license - * that can be found in the LICENSE file in the root of the source - * tree. An additional intellectual property rights grant can be found - * in the file PATENTS. All contributing project authors may - * be found in the AUTHORS file in the root of the source tree. - */ -#ifndef CALL_SSRC_BINDING_OBSERVER_H_ -#define CALL_SSRC_BINDING_OBSERVER_H_ - -#include - -namespace webrtc { - -// With newer versions of SDP, SSRC is often not explicitly signaled and must -// be learned on the fly. This happens by correlating packet SSRCs with included -// RTP extension headers like MID and RSID, or by receiving information from -// RTCP messages. -// SsrcBindingObservers will be notified when a new binding is learned, which -// can happen during call setup and/or during the call. -class SsrcBindingObserver { - public: - virtual ~SsrcBindingObserver() = default; - - virtual void OnSsrcBoundToRsid(const std::string& rsid, uint32_t ssrc) {} - - virtual void OnSsrcBoundToMid(const std::string& mid, uint32_t ssrc) {} - - virtual void OnSsrcBoundToMidRsid(const std::string& mid, - const std::string& rsid, - uint32_t ssrc) {} - - virtual void OnSsrcBoundToPayloadType(uint8_t payload_type, uint32_t ssrc) {} -}; - -} // namespace webrtc - -#endif // CALL_SSRC_BINDING_OBSERVER_H_