From 8b79b07a55a857370848724385f3d1f38c52213f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Peter=20Bostr=C3=B6m?= Date: Fri, 26 Feb 2016 16:31:37 +0100 Subject: [PATCH] Move RTP module activation into PayloadRouter. Simplifies PayloadRouter to not accept dynamically-changing modules as well as usage of PayloadRouter inside ViEChannel::SetSendCodec. BUG=webrtc:5494 R=stefan@webrtc.org TBR=mflodman@webrtc.org Review URL: https://codereview.webrtc.org/1725363003 . Cr-Commit-Position: refs/heads/master@{#11787} --- webrtc/common_types.h | 2 +- webrtc/modules/rtp_rtcp/include/rtp_rtcp.h | 3 + webrtc/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h | 2 + .../modules/rtp_rtcp/source/rtp_rtcp_impl.cc | 9 ++- .../modules/rtp_rtcp/source/rtp_rtcp_impl.h | 4 +- webrtc/modules/rtp_rtcp/source/rtp_sender.cc | 2 +- webrtc/modules/rtp_rtcp/source/rtp_sender.h | 9 +-- webrtc/video/payload_router.cc | 59 ++++++++++++------ webrtc/video/payload_router.h | 20 +++--- webrtc/video/payload_router_unittest.cc | 29 ++++----- webrtc/video/video_send_stream.cc | 4 ++ webrtc/video/vie_channel.cc | 61 +++++-------------- 12 files changed, 100 insertions(+), 104 deletions(-) diff --git a/webrtc/common_types.h b/webrtc/common_types.h index 2999bc95bc..16174dcf75 100644 --- a/webrtc/common_types.h +++ b/webrtc/common_types.h @@ -37,7 +37,7 @@ #define NULL 0 #endif -#define RTP_PAYLOAD_NAME_SIZE 32 +#define RTP_PAYLOAD_NAME_SIZE 32u #if defined(WEBRTC_WIN) || defined(WIN32) // Compares two strings without regard to case. diff --git a/webrtc/modules/rtp_rtcp/include/rtp_rtcp.h b/webrtc/modules/rtp_rtcp/include/rtp_rtcp.h index a47d3fefae..50bb6a0878 100644 --- a/webrtc/modules/rtp_rtcp/include/rtp_rtcp.h +++ b/webrtc/modules/rtp_rtcp/include/rtp_rtcp.h @@ -168,6 +168,9 @@ class RtpRtcp : public Module { virtual int32_t RegisterSendPayload( const VideoCodec& videoCodec) = 0; + virtual void RegisterVideoSendPayload(int payload_type, + const char* payload_name) = 0; + /* * Unregister a send payload * diff --git a/webrtc/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h b/webrtc/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h index d4d6e1491c..2281e36db1 100644 --- a/webrtc/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h +++ b/webrtc/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h @@ -74,6 +74,8 @@ class MockRtpRtcp : public RtpRtcp { int32_t(const CodecInst& voiceCodec)); MOCK_METHOD1(RegisterSendPayload, int32_t(const VideoCodec& videoCodec)); + MOCK_METHOD2(RegisterVideoSendPayload, + void(int payload_type, const char* payload_name)); MOCK_METHOD1(DeRegisterSendPayload, int32_t(const int8_t payloadType)); MOCK_METHOD2(RegisterSendRtpHeaderExtension, diff --git a/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc b/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc index cbb085b5df..003a134678 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc @@ -118,8 +118,6 @@ ModuleRtpRtcpImpl::ModuleRtpRtcpImpl(const Configuration& configuration) rtt_stats_(configuration.rtt_stats), critical_section_rtt_(CriticalSectionWrapper::CreateCriticalSection()), rtt_ms_(0) { - send_video_codec_.codecType = kVideoCodecUnknown; - // Make sure that RTCP objects are aware of our SSRC. uint32_t SSRC = rtp_sender_.SSRC(); rtcp_sender_.SetSSRC(SSRC); @@ -269,11 +267,16 @@ int32_t ModuleRtpRtcpImpl::RegisterSendPayload( } int32_t ModuleRtpRtcpImpl::RegisterSendPayload(const VideoCodec& video_codec) { - send_video_codec_ = video_codec; return rtp_sender_.RegisterPayload(video_codec.plName, video_codec.plType, 90000, 0, 0); } +void ModuleRtpRtcpImpl::RegisterVideoSendPayload(int payload_type, + const char* payload_name) { + RTC_CHECK_EQ( + 0, rtp_sender_.RegisterPayload(payload_name, payload_type, 90000, 0, 0)); +} + int32_t ModuleRtpRtcpImpl::DeRegisterSendPayload(const int8_t payload_type) { return rtp_sender_.DeRegisterSendPayload(payload_type); } diff --git a/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h b/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h index 8c211e4d9c..8fb7349a52 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h +++ b/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h @@ -51,6 +51,9 @@ class ModuleRtpRtcpImpl : public RtpRtcp { int32_t RegisterSendPayload(const VideoCodec& video_codec) override; + void RegisterVideoSendPayload(int payload_type, + const char* payload_name) override; + int32_t DeRegisterSendPayload(int8_t payload_type) override; int8_t SendPayloadType() const; @@ -369,7 +372,6 @@ class ModuleRtpRtcpImpl : public RtpRtcp { uint32_t nack_last_time_sent_full_prev_; uint16_t nack_last_seq_number_sent_; - VideoCodec send_video_codec_; KeyFrameRequestMethod key_frame_req_method_; RemoteBitrateEstimator* remote_bitrate_; diff --git a/webrtc/modules/rtp_rtcp/source/rtp_sender.cc b/webrtc/modules/rtp_rtcp/source/rtp_sender.cc index 97469ca35e..3197d60f92 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_sender.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_sender.cc @@ -304,7 +304,7 @@ int32_t RTPSender::RegisterPayload( uint32_t frequency, size_t channels, uint32_t rate) { - assert(payload_name); + RTC_DCHECK_LT(strlen(payload_name), RTP_PAYLOAD_NAME_SIZE); rtc::CritScope lock(&send_critsect_); std::map::iterator it = diff --git a/webrtc/modules/rtp_rtcp/source/rtp_sender.h b/webrtc/modules/rtp_rtcp/source/rtp_sender.h index 0e1242837c..5b6d4f7af7 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_sender.h +++ b/webrtc/modules/rtp_rtcp/source/rtp_sender.h @@ -114,10 +114,11 @@ class RTPSender : public RTPSenderInterface { // Includes size of RTP and FEC headers. size_t MaxDataPayloadLength() const override; - int32_t RegisterPayload( - const char payload_name[RTP_PAYLOAD_NAME_SIZE], - const int8_t payload_type, const uint32_t frequency, - const size_t channels, const uint32_t rate); + int32_t RegisterPayload(const char* payload_name, + const int8_t payload_type, + const uint32_t frequency, + const size_t channels, + const uint32_t rate); int32_t DeRegisterSendPayload(const int8_t payload_type); diff --git a/webrtc/video/payload_router.cc b/webrtc/video/payload_router.cc index a70da244db..968d82df62 100644 --- a/webrtc/video/payload_router.cc +++ b/webrtc/video/payload_router.cc @@ -17,7 +17,7 @@ namespace webrtc { PayloadRouter::PayloadRouter() - : active_(false) {} + : active_(false), num_sending_modules_(0) {} PayloadRouter::~PayloadRouter() {} @@ -26,15 +26,18 @@ size_t PayloadRouter::DefaultMaxPayloadLength() { return IP_PACKET_SIZE - kIpUdpSrtpLength; } -void PayloadRouter::SetSendingRtpModules( +void PayloadRouter::Init( const std::vector& rtp_modules) { - rtc::CritScope lock(&crit_); + RTC_DCHECK(rtp_modules_.empty()); rtp_modules_ = rtp_modules; } void PayloadRouter::set_active(bool active) { rtc::CritScope lock(&crit_); + if (active_ == active) + return; active_ = active; + UpdateModuleSendingState(); } bool PayloadRouter::active() { @@ -42,6 +45,25 @@ bool PayloadRouter::active() { return active_ && !rtp_modules_.empty(); } +void PayloadRouter::SetSendingRtpModules(size_t num_sending_modules) { + RTC_DCHECK_LE(num_sending_modules, rtp_modules_.size()); + rtc::CritScope lock(&crit_); + num_sending_modules_ = num_sending_modules; + UpdateModuleSendingState(); +} + +void PayloadRouter::UpdateModuleSendingState() { + for (size_t i = 0; i < num_sending_modules_; ++i) { + rtp_modules_[i]->SetSendingStatus(active_); + rtp_modules_[i]->SetSendingMediaStatus(active_); + } + // Disable inactive modules. + for (size_t i = num_sending_modules_; i < rtp_modules_.size(); ++i) { + rtp_modules_[i]->SetSendingStatus(false); + rtp_modules_[i]->SetSendingMediaStatus(false); + } +} + bool PayloadRouter::RoutePayload(FrameType frame_type, int8_t payload_type, uint32_t time_stamp, @@ -51,18 +73,19 @@ bool PayloadRouter::RoutePayload(FrameType frame_type, const RTPFragmentationHeader* fragmentation, const RTPVideoHeader* rtp_video_hdr) { rtc::CritScope lock(&crit_); - if (!active_ || rtp_modules_.empty()) - return false; - - // The simulcast index might actually be larger than the number of modules in - // case the encoder was processing a frame during a codec reconfig. - if (rtp_video_hdr != NULL && - rtp_video_hdr->simulcastIdx >= rtp_modules_.size()) + RTC_DCHECK(!rtp_modules_.empty()); + if (!active_ || num_sending_modules_ == 0) return false; int stream_idx = 0; - if (rtp_video_hdr != NULL) + if (rtp_video_hdr) { + RTC_DCHECK_LT(rtp_video_hdr->simulcastIdx, rtp_modules_.size()); + // The simulcast index might actually be larger than the number of modules + // in case the encoder was processing a frame during a codec reconfig. + if (rtp_video_hdr->simulcastIdx >= num_sending_modules_) + return false; stream_idx = rtp_video_hdr->simulcastIdx; + } return rtp_modules_[stream_idx]->SendOutgoingData( frame_type, payload_type, time_stamp, capture_time_ms, payload_data, payload_length, fragmentation, rtp_video_hdr) == 0 ? true : false; @@ -71,21 +94,17 @@ bool PayloadRouter::RoutePayload(FrameType frame_type, void PayloadRouter::SetTargetSendBitrates( const std::vector& stream_bitrates) { rtc::CritScope lock(&crit_); - if (stream_bitrates.size() < rtp_modules_.size()) { - // There can be a size mis-match during codec reconfiguration. - return; - } - int idx = 0; - for (auto* rtp_module : rtp_modules_) { - rtp_module->SetTargetSendBitrate(stream_bitrates[idx++]); + RTC_DCHECK_LE(stream_bitrates.size(), rtp_modules_.size()); + for (size_t i = 0; i < stream_bitrates.size(); ++i) { + rtp_modules_[i]->SetTargetSendBitrate(stream_bitrates[i]); } } size_t PayloadRouter::MaxPayloadLength() const { size_t min_payload_length = DefaultMaxPayloadLength(); rtc::CritScope lock(&crit_); - for (auto* rtp_module : rtp_modules_) { - size_t module_payload_length = rtp_module->MaxDataPayloadLength(); + for (size_t i = 0; i < num_sending_modules_; ++i) { + size_t module_payload_length = rtp_modules_[i]->MaxDataPayloadLength(); if (module_payload_length < min_payload_length) min_payload_length = module_payload_length; } diff --git a/webrtc/video/payload_router.h b/webrtc/video/payload_router.h index ad570fabed..661856d2a5 100644 --- a/webrtc/video/payload_router.h +++ b/webrtc/video/payload_router.h @@ -36,7 +36,9 @@ class PayloadRouter { static size_t DefaultMaxPayloadLength(); // Rtp modules are assumed to be sorted in simulcast index order. - void SetSendingRtpModules(const std::vector& rtp_modules); + void Init(const std::vector& rtp_modules); + + void SetSendingRtpModules(size_t num_sending_modules); // PayloadRouter will only route packets if being active, all packets will be // dropped otherwise. @@ -62,19 +64,15 @@ class PayloadRouter { // and RTP headers. size_t MaxPayloadLength() const; - void AddRef() { ++ref_count_; } - void Release() { if (--ref_count_ == 0) { delete this; } } - private: - // TODO(mflodman): When the new video API has launched, remove crit_ and - // assume rtp_modules_ will never change during a call. + void UpdateModuleSendingState() EXCLUSIVE_LOCKS_REQUIRED(crit_); + + // TODO(pbos): Set once and for all on construction and make const. + std::vector rtp_modules_; + rtc::CriticalSection crit_; - - // Active sending RTP modules, in layer order. - std::vector rtp_modules_ GUARDED_BY(crit_); bool active_ GUARDED_BY(crit_); - - Atomic32 ref_count_; + size_t num_sending_modules_ GUARDED_BY(crit_); RTC_DISALLOW_COPY_AND_ASSIGN(PayloadRouter); }; diff --git a/webrtc/video/payload_router_unittest.cc b/webrtc/video/payload_router_unittest.cc index 2bc096dbaa..9b831a3ef7 100644 --- a/webrtc/video/payload_router_unittest.cc +++ b/webrtc/video/payload_router_unittest.cc @@ -34,7 +34,8 @@ TEST_F(PayloadRouterTest, SendOnOneModule) { MockRtpRtcp rtp; std::vector modules(1, &rtp); - payload_router_->SetSendingRtpModules(modules); + payload_router_->Init(modules); + payload_router_->SetSendingRtpModules(modules.size()); uint8_t payload = 'a'; FrameType frame_type = kVideoFrameKey; @@ -67,8 +68,7 @@ TEST_F(PayloadRouterTest, SendOnOneModule) { EXPECT_TRUE(payload_router_->RoutePayload(frame_type, payload_type, 0, 0, &payload, 1, NULL, NULL)); - modules.clear(); - payload_router_->SetSendingRtpModules(modules); + payload_router_->SetSendingRtpModules(0); EXPECT_CALL(rtp, SendOutgoingData(frame_type, payload_type, 0, 0, _, 1, NULL, NULL)) .Times(0); @@ -83,7 +83,8 @@ TEST_F(PayloadRouterTest, SendSimulcast) { modules.push_back(&rtp_1); modules.push_back(&rtp_2); - payload_router_->SetSendingRtpModules(modules); + payload_router_->Init(modules); + payload_router_->SetSendingRtpModules(modules.size()); uint8_t payload_1 = 'a'; FrameType frame_type_1 = kVideoFrameKey; @@ -125,12 +126,13 @@ TEST_F(PayloadRouterTest, SendSimulcast) { &payload_2, 1, NULL, &rtp_hdr_2)); // Invalid simulcast index. + payload_router_->SetSendingRtpModules(1); payload_router_->set_active(true); EXPECT_CALL(rtp_1, SendOutgoingData(_, _, _, _, _, _, _, _)) .Times(0); EXPECT_CALL(rtp_2, SendOutgoingData(_, _, _, _, _, _, _, _)) .Times(0); - rtp_hdr_1.simulcastIdx = 2; + rtp_hdr_1.simulcastIdx = 1; EXPECT_FALSE(payload_router_->RoutePayload(frame_type_1, payload_type_1, 0, 0, &payload_1, 1, NULL, &rtp_hdr_1)); } @@ -147,7 +149,8 @@ TEST_F(PayloadRouterTest, MaxPayloadLength) { std::vector modules; modules.push_back(&rtp_1); modules.push_back(&rtp_2); - payload_router_->SetSendingRtpModules(modules); + payload_router_->Init(modules); + payload_router_->SetSendingRtpModules(modules.size()); // Modules return a higher length than the default value. EXPECT_CALL(rtp_1, MaxDataPayloadLength()) @@ -175,7 +178,8 @@ TEST_F(PayloadRouterTest, SetTargetSendBitrates) { std::vector modules; modules.push_back(&rtp_1); modules.push_back(&rtp_2); - payload_router_->SetSendingRtpModules(modules); + payload_router_->Init(modules); + payload_router_->SetSendingRtpModules(modules.size()); const uint32_t bitrate_1 = 10000; const uint32_t bitrate_2 = 76543; @@ -188,19 +192,10 @@ TEST_F(PayloadRouterTest, SetTargetSendBitrates) { payload_router_->SetTargetSendBitrates(bitrates); bitrates.resize(1); - EXPECT_CALL(rtp_1, SetTargetSendBitrate(bitrate_1)) - .Times(0); - EXPECT_CALL(rtp_2, SetTargetSendBitrate(bitrate_2)) - .Times(0); - payload_router_->SetTargetSendBitrates(bitrates); - - bitrates.resize(3); - bitrates[1] = bitrate_2; - bitrates[2] = bitrate_1 + bitrate_2; EXPECT_CALL(rtp_1, SetTargetSendBitrate(bitrate_1)) .Times(1); EXPECT_CALL(rtp_2, SetTargetSendBitrate(bitrate_2)) - .Times(1); + .Times(0); payload_router_->SetTargetSendBitrates(bitrates); } } // namespace webrtc diff --git a/webrtc/video/video_send_stream.cc b/webrtc/video/video_send_stream.cc index 66a6a321a1..b7b130209e 100644 --- a/webrtc/video/video_send_stream.cc +++ b/webrtc/video/video_send_stream.cc @@ -215,6 +215,7 @@ VideoSendStream::VideoSendStream( RTC_DCHECK(congestion_controller_); RTC_DCHECK(remb_); + payload_router_.Init(rtp_rtcp_modules_); RTC_CHECK(vie_encoder_.Init()); encoder_feedback_.Init(config_.rtp.ssrcs, &vie_encoder_); RTC_CHECK(vie_channel_.Init() == 0); @@ -274,6 +275,9 @@ VideoSendStream::VideoSendStream( rtp_rtcp->RegisterRtcpStatisticsCallback(&stats_proxy_); rtp_rtcp->RegisterSendChannelRtpStatisticsCallback(&stats_proxy_); rtp_rtcp->SetMaxTransferUnit(mtu); + rtp_rtcp->RegisterVideoSendPayload( + config_.encoder_settings.payload_type, + config_.encoder_settings.payload_name.c_str()); } RTC_DCHECK(config.encoder_settings.encoder != nullptr); diff --git a/webrtc/video/vie_channel.cc b/webrtc/video/vie_channel.cc index d91f20b7a6..aa34d0dc0f 100644 --- a/webrtc/video/vie_channel.cc +++ b/webrtc/video/vie_channel.cc @@ -148,8 +148,7 @@ int32_t ViEChannel::Init() { } packet_router_->AddRtpModule(rtp_rtcp_modules_[0]); if (sender_) { - send_payload_router_->SetSendingRtpModules( - std::vector(1, rtp_rtcp_modules_[0])); + send_payload_router_->SetSendingRtpModules(1); RTC_DCHECK(!send_payload_router_->active()); } else { if (vcm_->RegisterReceiveCallback(this) != 0) { @@ -169,7 +168,7 @@ ViEChannel::~ViEChannel() { module_process_thread_->DeRegisterModule( vie_receiver_.GetReceiveStatistics()); if (sender_) { - send_payload_router_->SetSendingRtpModules(std::vector()); + send_payload_router_->SetSendingRtpModules(0); } for (size_t i = 0; i < num_active_rtp_rtcp_modules_; ++i) packet_router_->RemoveRtpModule(rtp_rtcp_modules_[i]); @@ -244,12 +243,10 @@ int32_t ViEChannel::SetSendCodec(const VideoCodec& video_codec, // The first layer is always active, so the first module can be checked for // sending status. bool is_sending = rtp_rtcp_modules_[0]->Sending(); - bool router_was_active = send_payload_router_->active(); send_payload_router_->set_active(false); - send_payload_router_->SetSendingRtpModules(std::vector()); + send_payload_router_->SetSendingRtpModules(0); std::vector registered_modules; - std::vector deregistered_modules; size_t num_active_modules = video_codec.numberOfSimulcastStreams > 0 ? video_codec.numberOfSimulcastStreams : 1; @@ -263,34 +260,14 @@ int32_t ViEChannel::SetSendCodec(const VideoCodec& video_codec, for (size_t i = 0; i < num_active_modules; ++i) registered_modules.push_back(rtp_rtcp_modules_[i]); - for (size_t i = num_active_modules; i < rtp_rtcp_modules_.size(); ++i) - deregistered_modules.push_back(rtp_rtcp_modules_[i]); - - // Disable inactive modules. - for (RtpRtcp* rtp_rtcp : deregistered_modules) { - rtp_rtcp->SetSendingStatus(false); - rtp_rtcp->SetSendingMediaStatus(false); - } - - // Configure active modules. - for (RtpRtcp* rtp_rtcp : registered_modules) { - rtp_rtcp->DeRegisterSendPayload(video_codec.plType); - if (rtp_rtcp->RegisterSendPayload(video_codec) != 0) { - return -1; - } - rtp_rtcp->SetSendingStatus(is_sending); - rtp_rtcp->SetSendingMediaStatus(is_sending); - } - // |RegisterSimulcastRtpRtcpModules| resets all old weak pointers and old // modules can be deleted after this step. vie_receiver_.RegisterRtpRtcpModules(registered_modules); // Update the packet and payload routers with the sending RtpRtcp modules. - send_payload_router_->SetSendingRtpModules(registered_modules); + send_payload_router_->SetSendingRtpModules(num_active_modules); - if (router_was_active) - send_payload_router_->set_active(true); + send_payload_router_->set_active(is_sending); // Deregister previously registered modules. for (size_t i = num_active_modules; i < num_prev_active_modules; ++i) @@ -418,35 +395,27 @@ void ViEChannel::RegisterSendBitrateObserver( } int32_t ViEChannel::StartSend() { - rtc::CritScope lock(&crit_); - if (rtp_rtcp_modules_[0]->Sending()) return -1; - for (size_t i = 0; i < num_active_rtp_rtcp_modules_; ++i) { - RtpRtcp* rtp_rtcp = rtp_rtcp_modules_[i]; - // Only have senders send media. - rtp_rtcp->SetSendingMediaStatus(sender_); - rtp_rtcp->SetSendingStatus(true); - } - if (sender_) + if (!sender_) { + rtp_rtcp_modules_[0]->SetSendingStatus(true); + } else { send_payload_router_->set_active(true); + } return 0; } int32_t ViEChannel::StopSend() { - if (sender_) - send_payload_router_->set_active(false); - for (RtpRtcp* rtp_rtcp : rtp_rtcp_modules_) - rtp_rtcp->SetSendingMediaStatus(false); - - if (!rtp_rtcp_modules_[0]->Sending()) { + if (!rtp_rtcp_modules_[0]->Sending()) return -1; + + if (!sender_) { + rtp_rtcp_modules_[0]->SetSendingStatus(false); + } else { + send_payload_router_->set_active(false); } - for (RtpRtcp* rtp_rtcp : rtp_rtcp_modules_) { - rtp_rtcp->SetSendingStatus(false); - } return 0; }