From 3dd5d1d84af02f8cdd08ea4198aa5392cb3d906d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Peter=20Bostr=C3=B6m?= Date: Thu, 25 Feb 2016 16:56:48 +0100 Subject: [PATCH] Remove PacketRouter sender distinction. Instead relies on SetSendingMediaStatus() to filter out receiving RTP modules. This status is now set in VoiceEngine's SetSend() for senders along with SetSendingStatus(). BUG= R=solenberg@webrtc.org, stefan@webrtc.org Review URL: https://codereview.webrtc.org/1705763002 . Cr-Commit-Position: refs/heads/master@{#11768} --- webrtc/modules/pacing/packet_router.cc | 60 +++++-------------- webrtc/modules/pacing/packet_router.h | 7 +-- .../modules/pacing/packet_router_unittest.cc | 41 +++++++++---- webrtc/video/vie_channel.cc | 8 +-- webrtc/voice_engine/channel.cc | 17 +++--- 5 files changed, 59 insertions(+), 74 deletions(-) diff --git a/webrtc/modules/pacing/packet_router.cc b/webrtc/modules/pacing/packet_router.cc index 571f35d184..5c7a7ab29a 100644 --- a/webrtc/modules/pacing/packet_router.cc +++ b/webrtc/modules/pacing/packet_router.cc @@ -18,55 +18,26 @@ namespace webrtc { -namespace { -void AddModule(RtpRtcp* rtp_module, std::list* rtp_modules) { - RTC_DCHECK(std::find(rtp_modules->begin(), rtp_modules->end(), rtp_module) == - rtp_modules->end()); - rtp_modules->push_back(rtp_module); -} - -void RemoveModule(RtpRtcp* rtp_module, std::list* rtp_modules) { - RTC_DCHECK(std::find(rtp_modules->begin(), rtp_modules->end(), rtp_module) != - rtp_modules->end()); - rtp_modules->remove(rtp_module); -} - -bool SendFeedback(rtcp::TransportFeedback* packet, - std::list* rtp_modules) { - for (auto* rtp_module : *rtp_modules) { - packet->WithPacketSenderSsrc(rtp_module->SSRC()); - if (rtp_module->SendFeedbackPacket(*packet)) - return true; - } - return false; -} -} // namespace - PacketRouter::PacketRouter() : transport_seq_(0) { pacer_thread_checker_.DetachFromThread(); } PacketRouter::~PacketRouter() { - RTC_DCHECK(send_rtp_modules_.empty()); - RTC_DCHECK(recv_rtp_modules_.empty()); + RTC_DCHECK(rtp_modules_.empty()); } -void PacketRouter::AddRtpModule(RtpRtcp* rtp_module, bool sender) { +void PacketRouter::AddRtpModule(RtpRtcp* rtp_module) { rtc::CritScope cs(&modules_crit_); - if (sender) { - AddModule(rtp_module, &send_rtp_modules_); - } else { - AddModule(rtp_module, &recv_rtp_modules_); - } + RTC_DCHECK(std::find(rtp_modules_.begin(), rtp_modules_.end(), rtp_module) == + rtp_modules_.end()); + rtp_modules_.push_back(rtp_module); } -void PacketRouter::RemoveRtpModule(RtpRtcp* rtp_module, bool sender) { +void PacketRouter::RemoveRtpModule(RtpRtcp* rtp_module) { rtc::CritScope cs(&modules_crit_); - if (sender) { - RemoveModule(rtp_module, &send_rtp_modules_); - } else { - RemoveModule(rtp_module, &recv_rtp_modules_); - } + RTC_DCHECK(std::find(rtp_modules_.begin(), rtp_modules_.end(), rtp_module) != + rtp_modules_.end()); + rtp_modules_.remove(rtp_module); } bool PacketRouter::TimeToSendPacket(uint32_t ssrc, @@ -75,7 +46,7 @@ bool PacketRouter::TimeToSendPacket(uint32_t ssrc, bool retransmission) { RTC_DCHECK(pacer_thread_checker_.CalledOnValidThread()); rtc::CritScope cs(&modules_crit_); - for (auto* rtp_module : send_rtp_modules_) { + for (auto* rtp_module : rtp_modules_) { if (rtp_module->SendingMedia() && ssrc == rtp_module->SSRC()) { return rtp_module->TimeToSendPacket(ssrc, sequence_number, capture_timestamp, retransmission); @@ -88,7 +59,7 @@ size_t PacketRouter::TimeToSendPadding(size_t bytes_to_send) { RTC_DCHECK(pacer_thread_checker_.CalledOnValidThread()); size_t total_bytes_sent = 0; rtc::CritScope cs(&modules_crit_); - for (RtpRtcp* module : send_rtp_modules_) { + for (RtpRtcp* module : rtp_modules_) { if (module->SendingMedia()) { size_t bytes_sent = module->TimeToSendPadding(bytes_to_send - total_bytes_sent); @@ -124,10 +95,11 @@ uint16_t PacketRouter::AllocateSequenceNumber() { bool PacketRouter::SendFeedback(rtcp::TransportFeedback* packet) { rtc::CritScope cs(&modules_crit_); - if (::webrtc::SendFeedback(packet, &recv_rtp_modules_)) - return true; - if (::webrtc::SendFeedback(packet, &send_rtp_modules_)) - return true; + for (auto* rtp_module : rtp_modules_) { + packet->WithPacketSenderSsrc(rtp_module->SSRC()); + if (rtp_module->SendFeedbackPacket(*packet)) + return true; + } return false; } diff --git a/webrtc/modules/pacing/packet_router.h b/webrtc/modules/pacing/packet_router.h index dd58faf4ea..12f0a90c3a 100644 --- a/webrtc/modules/pacing/packet_router.h +++ b/webrtc/modules/pacing/packet_router.h @@ -37,8 +37,8 @@ class PacketRouter : public PacedSender::Callback, PacketRouter(); virtual ~PacketRouter(); - void AddRtpModule(RtpRtcp* rtp_module, bool sender); - void RemoveRtpModule(RtpRtcp* rtp_module, bool sender); + void AddRtpModule(RtpRtcp* rtp_module); + void RemoveRtpModule(RtpRtcp* rtp_module); // Implements PacedSender::Callback. bool TimeToSendPacket(uint32_t ssrc, @@ -57,8 +57,7 @@ class PacketRouter : public PacedSender::Callback, private: rtc::ThreadChecker pacer_thread_checker_; rtc::CriticalSection modules_crit_; - std::list send_rtp_modules_ GUARDED_BY(modules_crit_); - std::list recv_rtp_modules_ GUARDED_BY(modules_crit_); + std::list rtp_modules_ GUARDED_BY(modules_crit_); volatile int transport_seq_; diff --git a/webrtc/modules/pacing/packet_router_unittest.cc b/webrtc/modules/pacing/packet_router_unittest.cc index e5e05896c2..4649358f4c 100644 --- a/webrtc/modules/pacing/packet_router_unittest.cc +++ b/webrtc/modules/pacing/packet_router_unittest.cc @@ -36,8 +36,8 @@ class PacketRouterTest : public ::testing::Test { TEST_F(PacketRouterTest, TimeToSendPacket) { MockRtpRtcp rtp_1; MockRtpRtcp rtp_2; - packet_router_->AddRtpModule(&rtp_1, true); - packet_router_->AddRtpModule(&rtp_2, true); + packet_router_->AddRtpModule(&rtp_1); + packet_router_->AddRtpModule(&rtp_2); const uint16_t kSsrc1 = 1234; uint16_t sequence_number = 17; @@ -89,7 +89,7 @@ TEST_F(PacketRouterTest, TimeToSendPacket) { EXPECT_TRUE(packet_router_->TimeToSendPacket(kSsrc1 + kSsrc2, sequence_number, timestamp, retransmission)); - packet_router_->RemoveRtpModule(&rtp_1, true); + packet_router_->RemoveRtpModule(&rtp_1); // rtp_1 has been removed, try sending a packet on that ssrc and make sure // it is dropped as expected by not expecting any calls to rtp_1. @@ -99,7 +99,7 @@ TEST_F(PacketRouterTest, TimeToSendPacket) { EXPECT_TRUE(packet_router_->TimeToSendPacket(kSsrc1, sequence_number, timestamp, retransmission)); - packet_router_->RemoveRtpModule(&rtp_2, true); + packet_router_->RemoveRtpModule(&rtp_2); } TEST_F(PacketRouterTest, TimeToSendPadding) { @@ -110,8 +110,8 @@ TEST_F(PacketRouterTest, TimeToSendPadding) { EXPECT_CALL(rtp_1, SSRC()).WillRepeatedly(Return(kSsrc1)); MockRtpRtcp rtp_2; EXPECT_CALL(rtp_2, SSRC()).WillRepeatedly(Return(kSsrc2)); - packet_router_->AddRtpModule(&rtp_1, true); - packet_router_->AddRtpModule(&rtp_2, true); + packet_router_->AddRtpModule(&rtp_1); + packet_router_->AddRtpModule(&rtp_2); // Default configuration, sending padding on all modules sending media, // ordered by SSRC. @@ -147,7 +147,7 @@ TEST_F(PacketRouterTest, TimeToSendPadding) { EXPECT_CALL(rtp_2, TimeToSendPadding(_)).Times(0); EXPECT_EQ(0u, packet_router_->TimeToSendPadding(requested_padding_bytes)); - packet_router_->RemoveRtpModule(&rtp_1, true); + packet_router_->RemoveRtpModule(&rtp_1); // rtp_1 has been removed, try sending padding and make sure rtp_1 isn't asked // to send by not expecting any calls. Instead verify rtp_2 is called. @@ -155,7 +155,24 @@ TEST_F(PacketRouterTest, TimeToSendPadding) { EXPECT_CALL(rtp_2, TimeToSendPadding(requested_padding_bytes)).Times(1); EXPECT_EQ(0u, packet_router_->TimeToSendPadding(requested_padding_bytes)); - packet_router_->RemoveRtpModule(&rtp_2, true); + packet_router_->RemoveRtpModule(&rtp_2); +} + +TEST_F(PacketRouterTest, SenderOnlyFunctionsRespectSendingMedia) { + MockRtpRtcp rtp; + packet_router_->AddRtpModule(&rtp); + static const uint16_t kSsrc = 1234; + EXPECT_CALL(rtp, SSRC()).WillRepeatedly(Return(kSsrc)); + EXPECT_CALL(rtp, SendingMedia()).WillRepeatedly(Return(false)); + + // Verify that TimeToSendPacket does not end up in a receiver. + EXPECT_CALL(rtp, TimeToSendPacket(_, _, _, _)).Times(0); + EXPECT_TRUE(packet_router_->TimeToSendPacket(kSsrc, 1, 1, false)); + // Verify that TimeToSendPadding does not end up in a receiver. + EXPECT_CALL(rtp, TimeToSendPadding(_)).Times(0); + EXPECT_EQ(0u, packet_router_->TimeToSendPadding(200)); + + packet_router_->RemoveRtpModule(&rtp); } TEST_F(PacketRouterTest, AllocateSequenceNumbers) { @@ -174,15 +191,15 @@ TEST_F(PacketRouterTest, AllocateSequenceNumbers) { TEST_F(PacketRouterTest, SendFeedback) { MockRtpRtcp rtp_1; MockRtpRtcp rtp_2; - packet_router_->AddRtpModule(&rtp_1, false); - packet_router_->AddRtpModule(&rtp_2, true); + packet_router_->AddRtpModule(&rtp_1); + packet_router_->AddRtpModule(&rtp_2); rtcp::TransportFeedback feedback; EXPECT_CALL(rtp_1, SendFeedbackPacket(_)).Times(1); packet_router_->SendFeedback(&feedback); - packet_router_->RemoveRtpModule(&rtp_1, false); + packet_router_->RemoveRtpModule(&rtp_1); EXPECT_CALL(rtp_2, SendFeedbackPacket(_)).Times(1); packet_router_->SendFeedback(&feedback); - packet_router_->RemoveRtpModule(&rtp_2, true); + packet_router_->RemoveRtpModule(&rtp_2); } } // namespace webrtc diff --git a/webrtc/video/vie_channel.cc b/webrtc/video/vie_channel.cc index 97957352fe..15dafcb708 100644 --- a/webrtc/video/vie_channel.cc +++ b/webrtc/video/vie_channel.cc @@ -146,7 +146,7 @@ int32_t ViEChannel::Init() { for (RtpRtcp* rtp_rtcp : rtp_rtcp_modules_) rtp_rtcp->SetStorePacketsStatus(true, kMinSendSidePacketHistorySize); } - packet_router_->AddRtpModule(rtp_rtcp_modules_[0], sender_); + packet_router_->AddRtpModule(rtp_rtcp_modules_[0]); if (sender_) { send_payload_router_->SetSendingRtpModules( std::vector(1, rtp_rtcp_modules_[0])); @@ -172,7 +172,7 @@ ViEChannel::~ViEChannel() { send_payload_router_->SetSendingRtpModules(std::vector()); } for (size_t i = 0; i < num_active_rtp_rtcp_modules_; ++i) - packet_router_->RemoveRtpModule(rtp_rtcp_modules_[i], sender_); + packet_router_->RemoveRtpModule(rtp_rtcp_modules_[i]); for (RtpRtcp* rtp_rtcp : rtp_rtcp_modules_) { module_process_thread_->DeRegisterModule(rtp_rtcp); delete rtp_rtcp; @@ -294,10 +294,10 @@ int32_t ViEChannel::SetSendCodec(const VideoCodec& video_codec, // Deregister previously registered modules. for (size_t i = num_active_modules; i < num_prev_active_modules; ++i) - packet_router_->RemoveRtpModule(rtp_rtcp_modules_[i], true); + packet_router_->RemoveRtpModule(rtp_rtcp_modules_[i]); // Register new active modules. for (size_t i = num_prev_active_modules; i < num_active_modules; ++i) - packet_router_->AddRtpModule(rtp_rtcp_modules_[i], true); + packet_router_->AddRtpModule(rtp_rtcp_modules_[i]); return 0; } diff --git a/webrtc/voice_engine/channel.cc b/webrtc/voice_engine/channel.cc index 9e27ce8b7b..11af45edc8 100644 --- a/webrtc/voice_engine/channel.cc +++ b/webrtc/voice_engine/channel.cc @@ -124,12 +124,6 @@ class RtpPacketSenderProxy : public RtpPacketSender { rtp_packet_sender_ = rtp_packet_sender; } - bool HasPacketSender() const { - RTC_DCHECK(thread_checker_.CalledOnValidThread()); - rtc::CritScope lock(&crit_); - return rtp_packet_sender_ != nullptr; - } - // Implements RtpPacketSender. void InsertPacket(Priority priority, uint32_t ssrc, @@ -855,6 +849,7 @@ Channel::Channel(int32_t channelId, configuration.event_log = event_log; _rtpRtcpModule.reset(RtpRtcp::CreateRtpRtcp(configuration)); + _rtpRtcpModule->SetSendingMediaStatus(false); statistics_proxy_.reset(new StatisticsProxy(_rtpRtcpModule->SSRC())); rtp_receive_statistics_->RegisterRtcpStatisticsCallback( @@ -1136,10 +1131,12 @@ int32_t Channel::StartSend() { } channel_state_.SetSending(true); + _rtpRtcpModule->SetSendingMediaStatus(true); if (_rtpRtcpModule->SetSendingStatus(true) != 0) { _engineStatisticsPtr->SetLastError( VE_RTP_RTCP_MODULE_ERROR, kTraceError, "StartSend() RTP/RTCP failed to start sending"); + _rtpRtcpModule->SetSendingMediaStatus(false); rtc::CritScope cs(&_callbackCritSect); channel_state_.SetSending(false); return -1; @@ -1171,6 +1168,7 @@ int32_t Channel::StopSend() { VE_RTP_RTCP_MODULE_ERROR, kTraceWarning, "StartSend() RTP/RTCP failed to stop sending"); } + _rtpRtcpModule->SetSendingMediaStatus(false); return 0; } @@ -2599,14 +2597,14 @@ void Channel::RegisterSenderCongestionControlObjects( seq_num_allocator_proxy_->SetSequenceNumberAllocator(packet_router); rtp_packet_sender_proxy_->SetPacketSender(rtp_packet_sender); _rtpRtcpModule->SetStorePacketsStatus(true, 600); - packet_router->AddRtpModule(_rtpRtcpModule.get(), true); + packet_router->AddRtpModule(_rtpRtcpModule.get()); packet_router_ = packet_router; } void Channel::RegisterReceiverCongestionControlObjects( PacketRouter* packet_router) { RTC_DCHECK(packet_router && !packet_router_); - packet_router->AddRtpModule(_rtpRtcpModule.get(), false); + packet_router->AddRtpModule(_rtpRtcpModule.get()); packet_router_ = packet_router; } @@ -2615,8 +2613,7 @@ void Channel::ResetCongestionControlObjects() { _rtpRtcpModule->SetStorePacketsStatus(false, 600); feedback_observer_proxy_->SetTransportFeedbackObserver(nullptr); seq_num_allocator_proxy_->SetSequenceNumberAllocator(nullptr); - const bool sender = rtp_packet_sender_proxy_->HasPacketSender(); - packet_router_->RemoveRtpModule(_rtpRtcpModule.get(), sender); + packet_router_->RemoveRtpModule(_rtpRtcpModule.get()); packet_router_ = nullptr; rtp_packet_sender_proxy_->SetPacketSender(nullptr); }