From 290cb56dcaf29dc15cc3fffc10565d2c3fb7095d Mon Sep 17 00:00:00 2001 From: "mflodman@webrtc.org" Date: Tue, 17 Feb 2015 10:15:06 +0000 Subject: [PATCH] Remove TimeToSendPacket and TimeToSendPadding from the default module. Thie CL moves the default RTP module logic for TimeToSendPacket and TimeToSendPadding to PayloadRouter class and asserts on usage of the default module. BUG=769 TEST=New unittest. R=stefan@webrtc.org Review URL: https://webrtc-codereview.appspot.com/33319004 Cr-Commit-Position: refs/heads/master@{#8383} git-svn-id: http://webrtc.googlecode.com/svn/trunk@8383 4adac7df-926f-26a2-2b94-8c16560cd09d --- .../modules/rtp_rtcp/source/rtp_rtcp_impl.cc | 36 +---- webrtc/video_engine/payload_router.cc | 23 +++ webrtc/video_engine/payload_router.h | 12 ++ .../video_engine/payload_router_unittest.cc | 141 ++++++++++++++++++ webrtc/video_engine/vie_channel_manager.cc | 8 +- webrtc/video_engine/vie_encoder.cc | 37 ++--- webrtc/video_engine/vie_encoder.h | 11 +- 7 files changed, 216 insertions(+), 52 deletions(-) diff --git a/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc b/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc index f821c0272a..89ac9a0204 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc @@ -501,42 +501,18 @@ bool ModuleRtpRtcpImpl::TimeToSendPacket(uint32_t ssrc, uint16_t sequence_number, int64_t capture_time_ms, bool retransmission) { - if (!IsDefaultModule()) { - // Don't send from default module. - if (SendingMedia() && ssrc == rtp_sender_.SSRC()) { - return rtp_sender_.TimeToSendPacket(sequence_number, capture_time_ms, - retransmission); - } - } else { - CriticalSectionScoped lock(critical_section_module_ptrs_.get()); - std::vector::iterator it = child_modules_.begin(); - while (it != child_modules_.end()) { - if ((*it)->SendingMedia() && ssrc == (*it)->rtp_sender_.SSRC()) { - return (*it)->rtp_sender_.TimeToSendPacket(sequence_number, - capture_time_ms, - retransmission); - } - ++it; - } + assert(!IsDefaultModule()); + if (SendingMedia() && ssrc == rtp_sender_.SSRC()) { + return rtp_sender_.TimeToSendPacket( + sequence_number, capture_time_ms, retransmission); } // No RTP sender is interested in sending this packet. return true; } size_t ModuleRtpRtcpImpl::TimeToSendPadding(size_t bytes) { - if (!IsDefaultModule()) { - // Don't send from default module. - return rtp_sender_.TimeToSendPadding(bytes); - } else { - CriticalSectionScoped lock(critical_section_module_ptrs_.get()); - for (size_t i = 0; i < child_modules_.size(); ++i) { - // Send padding on one of the modules sending media. - if (child_modules_[i]->SendingMedia()) { - return child_modules_[i]->rtp_sender_.TimeToSendPadding(bytes); - } - } - } - return 0; + assert(!IsDefaultModule()); + return rtp_sender_.TimeToSendPadding(bytes); } bool ModuleRtpRtcpImpl::GetSendSideDelay(int* avg_send_delay_ms, diff --git a/webrtc/video_engine/payload_router.cc b/webrtc/video_engine/payload_router.cc index 00c64ad4b2..58a2fb4dc7 100644 --- a/webrtc/video_engine/payload_router.cc +++ b/webrtc/video_engine/payload_router.cc @@ -71,6 +71,29 @@ bool PayloadRouter::RoutePayload(FrameType frame_type, payload_length, fragmentation, rtp_video_hdr) == 0 ? true : false; } +bool PayloadRouter::TimeToSendPacket(uint32_t ssrc, + uint16_t sequence_number, + int64_t capture_timestamp, + bool retransmission) { + CriticalSectionScoped cs(crit_.get()); + for (auto* rtp_module : rtp_modules_) { + if (rtp_module->SendingMedia() && ssrc == rtp_module->SSRC()) { + return rtp_module->TimeToSendPacket(ssrc, sequence_number, + capture_timestamp, retransmission); + } + } + return true; +} + +size_t PayloadRouter::TimeToSendPadding(size_t bytes) { + CriticalSectionScoped cs(crit_.get()); + for(auto* rtp_module : rtp_modules_) { + if (rtp_module->SendingMedia()) + return rtp_module->TimeToSendPadding(bytes); + } + return 0; +} + size_t PayloadRouter::MaxPayloadLength() const { size_t min_payload_length = DefaultMaxPayloadLength(); CriticalSectionScoped cs(crit_.get()); diff --git a/webrtc/video_engine/payload_router.h b/webrtc/video_engine/payload_router.h index baad38f5f6..60f4747223 100644 --- a/webrtc/video_engine/payload_router.h +++ b/webrtc/video_engine/payload_router.h @@ -54,11 +54,23 @@ class PayloadRouter { const RTPFragmentationHeader* fragmentation, const RTPVideoHeader* rtp_video_hdr); + // Called when it's time to send a stored packet. + bool TimeToSendPacket(uint32_t ssrc, + uint16_t sequence_number, + int64_t capture_timestamp, + bool retransmission); + + // Called when it's time to send padding, returns the number of bytes actually + // sent. + size_t TimeToSendPadding(size_t bytes); + // Returns the maximum allowed data payload length, given the configured MTU // and RTP headers. size_t MaxPayloadLength() const; private: + // TODO(mflodman): When the new video API has launched, remove crit_ and + // assume rtp_modules_ will never change during a call. scoped_ptr crit_; // Active sending RTP modules, in layer order. diff --git a/webrtc/video_engine/payload_router_unittest.cc b/webrtc/video_engine/payload_router_unittest.cc index abab965ac4..ff4f9b37d4 100644 --- a/webrtc/video_engine/payload_router_unittest.cc +++ b/webrtc/video_engine/payload_router_unittest.cc @@ -161,4 +161,145 @@ TEST_F(PayloadRouterTest, MaxPayloadLength) { EXPECT_EQ(kTestMinPayloadLength, payload_router_->MaxPayloadLength()); } +TEST_F(PayloadRouterTest, TimeToSendPacket) { + MockRtpRtcp rtp_1; + MockRtpRtcp rtp_2; + std::list modules; + modules.push_back(&rtp_1); + modules.push_back(&rtp_2); + payload_router_->SetSendingRtpModules(modules); + + const uint16_t kSsrc1 = 1234; + uint16_t sequence_number = 17; + uint64_t timestamp = 7890; + bool retransmission = false; + + // Send on the first module by letting rtp_1 be sending with correct ssrc. + EXPECT_CALL(rtp_1, SendingMedia()) + .Times(1) + .WillOnce(Return(true)); + EXPECT_CALL(rtp_1, SSRC()) + .Times(1) + .WillOnce(Return(kSsrc1)); + EXPECT_CALL(rtp_1, TimeToSendPacket(kSsrc1, sequence_number, timestamp, + retransmission)) + .Times(1) + .WillOnce(Return(true)); + EXPECT_CALL(rtp_2, TimeToSendPacket(_, _, _, _)) + .Times(0); + EXPECT_TRUE(payload_router_->TimeToSendPacket( + kSsrc1, sequence_number, timestamp, retransmission)); + + + // Send on the second module by letting rtp_2 be sending, but not rtp_1. + ++sequence_number; + timestamp += 30; + retransmission = true; + const uint16_t kSsrc2 = 4567; + EXPECT_CALL(rtp_1, SendingMedia()) + .Times(1) + .WillOnce(Return(false)); + EXPECT_CALL(rtp_2, SendingMedia()) + .Times(1) + .WillOnce(Return(true)); + EXPECT_CALL(rtp_2, SSRC()) + .Times(1) + .WillOnce(Return(kSsrc2)); + EXPECT_CALL(rtp_1, TimeToSendPacket(_, _, _, _)) + .Times(0); + EXPECT_CALL(rtp_2, TimeToSendPacket(kSsrc2, sequence_number, timestamp, + retransmission)) + .Times(1) + .WillOnce(Return(true)); + EXPECT_TRUE(payload_router_->TimeToSendPacket( + kSsrc2, sequence_number, timestamp, retransmission)); + + // No module is sending, hence no packet should be sent. + EXPECT_CALL(rtp_1, SendingMedia()) + .Times(1) + .WillOnce(Return(false)); + EXPECT_CALL(rtp_1, TimeToSendPacket(_, _, _,_)) + .Times(0); + EXPECT_CALL(rtp_2, SendingMedia()) + .Times(1) + .WillOnce(Return(false)); + EXPECT_CALL(rtp_2, TimeToSendPacket(_, _, _, _)) + .Times(0); + EXPECT_TRUE(payload_router_->TimeToSendPacket( + kSsrc1, sequence_number, timestamp, retransmission)); + + // Add a packet with incorrect ssrc and test it's dropped in the router. + EXPECT_CALL(rtp_1, SendingMedia()) + .Times(1) + .WillOnce(Return(true)); + EXPECT_CALL(rtp_1, SSRC()) + .Times(1) + .WillOnce(Return(kSsrc1)); + EXPECT_CALL(rtp_2, SendingMedia()) + .Times(1) + .WillOnce(Return(true)); + EXPECT_CALL(rtp_2, SSRC()) + .Times(1) + .WillOnce(Return(kSsrc2)); + EXPECT_CALL(rtp_1, TimeToSendPacket(_, _, _,_)) + .Times(0); + EXPECT_CALL(rtp_2, TimeToSendPacket(_, _, _, _)) + .Times(0); + EXPECT_TRUE(payload_router_->TimeToSendPacket( + kSsrc1 + kSsrc2, sequence_number, timestamp, retransmission)); +} + +TEST_F(PayloadRouterTest, TimeToSendPadding) { + MockRtpRtcp rtp_1; + MockRtpRtcp rtp_2; + std::list modules; + modules.push_back(&rtp_1); + modules.push_back(&rtp_2); + payload_router_->SetSendingRtpModules(modules); + + + // Default configuration, sending padding on the first sending module. + const size_t requested_padding_bytes = 1000; + const size_t sent_padding_bytes = 890; + EXPECT_CALL(rtp_1, SendingMedia()) + .Times(1) + .WillOnce(Return(true)); + EXPECT_CALL(rtp_1, TimeToSendPadding(requested_padding_bytes)) + .Times(1) + .WillOnce(Return(sent_padding_bytes)); + EXPECT_CALL(rtp_2, TimeToSendPadding(_)) + .Times(0); + EXPECT_EQ(sent_padding_bytes, + payload_router_->TimeToSendPadding(requested_padding_bytes)); + + // Let only the second module be sending and verify the padding request is + // routed there. + EXPECT_CALL(rtp_1, SendingMedia()) + .Times(1) + .WillOnce(Return(false)); + EXPECT_CALL(rtp_1, TimeToSendPadding(requested_padding_bytes)) + .Times(0); + EXPECT_CALL(rtp_2, SendingMedia()) + .Times(1) + .WillOnce(Return(true)); + EXPECT_CALL(rtp_2, TimeToSendPadding(_)) + .Times(1) + .WillOnce(Return(sent_padding_bytes)); + EXPECT_EQ(sent_padding_bytes, + payload_router_->TimeToSendPadding(requested_padding_bytes)); + + // No sending module at all. + EXPECT_CALL(rtp_1, SendingMedia()) + .Times(1) + .WillOnce(Return(false)); + EXPECT_CALL(rtp_1, TimeToSendPadding(requested_padding_bytes)) + .Times(0); + EXPECT_CALL(rtp_2, SendingMedia()) + .Times(1) + .WillOnce(Return(false)); + EXPECT_CALL(rtp_2, TimeToSendPadding(_)) + .Times(0); + EXPECT_EQ(static_cast(0), + payload_router_->TimeToSendPadding(requested_padding_bytes)); +} } // namespace webrtc diff --git a/webrtc/video_engine/vie_channel_manager.cc b/webrtc/video_engine/vie_channel_manager.cc index f2788cf60c..c9f0f4ef0a 100644 --- a/webrtc/video_engine/vie_channel_manager.cc +++ b/webrtc/video_engine/vie_channel_manager.cc @@ -117,7 +117,7 @@ int ViEChannelManager::CreateChannel(int* channel_id, return -1; } // Connect the encoder with the send packet router, to enable sending. - vie_encoder->SetSendPayloadRouter( + vie_encoder->StartThreadsAndSetSendPayloadRouter( channel_map_[new_channel_id]->send_payload_router()); // Add ViEEncoder to EncoderFeedBackObserver. @@ -183,7 +183,7 @@ int ViEChannelManager::CreateChannel(int* channel_id, vie_encoder = NULL; } // Connect the encoder with the send packet router, to enable sending. - vie_encoder->SetSendPayloadRouter( + vie_encoder->StartThreadsAndSetSendPayloadRouter( channel_map_[new_channel_id]->send_payload_router()); // Register the ViEEncoder to get key frame requests for this channel. @@ -249,9 +249,11 @@ int ViEChannelManager::DeleteChannel(int channel_id) { vie_channel->GetStatsObserver()); group->SetChannelRembStatus(channel_id, false, false, vie_channel); - // Remove the feedback if we're owning the encoder. + // If we're owning the encoder, remove the feedback and stop all encoding + // threads and processing. This must be done before deleting the channel. if (vie_encoder->channel_id() == channel_id) { group->GetEncoderStateFeedback()->RemoveEncoder(vie_encoder); + vie_encoder->StopThreadsAndRemovePayloadRouter(); } unsigned int remote_ssrc = 0; diff --git a/webrtc/video_engine/vie_encoder.cc b/webrtc/video_engine/vie_encoder.cc index e2947aaf5a..7e010b1f06 100644 --- a/webrtc/video_engine/vie_encoder.cc +++ b/webrtc/video_engine/vie_encoder.cc @@ -107,6 +107,7 @@ class ViEBitrateObserver : public BitrateObserver { ViEEncoder* owner_; }; +// TODO(mflodman): Move this observer to PayloadRouter class. class ViEPacedSenderCallback : public PacedSender::Callback { public: explicit ViEPacedSenderCallback(ViEEncoder* owner) @@ -191,14 +192,6 @@ bool ViEEncoder::Init() { // Enable/disable content analysis: off by default for now. vpm_.EnableContentAnalysis(false); - if (module_process_thread_.RegisterModule(&vcm_) != 0 || - module_process_thread_.RegisterModule(default_rtp_rtcp_.get()) != 0) { - return false; - } - if (pacer_thread_->RegisterModule(paced_sender_.get()) != 0 || - pacer_thread_->Start() != 0) { - return false; - } if (qm_callback_) { delete qm_callback_; } @@ -235,9 +228,24 @@ bool ViEEncoder::Init() { return true; } -void ViEEncoder::SetSendPayloadRouter(PayloadRouter* send_payload_router) { +void ViEEncoder::StartThreadsAndSetSendPayloadRouter( + PayloadRouter* send_payload_router) { DCHECK(send_payload_router_ == NULL); send_payload_router_ = send_payload_router; + + module_process_thread_.RegisterModule(&vcm_); + module_process_thread_.RegisterModule(default_rtp_rtcp_.get()); + pacer_thread_->RegisterModule(paced_sender_.get()); + pacer_thread_->Start(); +} + +void ViEEncoder::StopThreadsAndRemovePayloadRouter() { + pacer_thread_->Stop(); + pacer_thread_->DeRegisterModule(paced_sender_.get()); + module_process_thread_.DeRegisterModule(&vcm_); + module_process_thread_.DeRegisterModule(&vpm_); + module_process_thread_.DeRegisterModule(default_rtp_rtcp_.get()); + send_payload_router_ = nullptr; } ViEEncoder::~ViEEncoder() { @@ -245,11 +253,6 @@ ViEEncoder::~ViEEncoder() { if (bitrate_controller_) { bitrate_controller_->RemoveBitrateObserver(bitrate_observer_.get()); } - pacer_thread_->Stop(); - pacer_thread_->DeRegisterModule(paced_sender_.get()); - module_process_thread_.DeRegisterModule(&vcm_); - module_process_thread_.DeRegisterModule(&vpm_); - module_process_thread_.DeRegisterModule(default_rtp_rtcp_.get()); VideoCodingModule::Destroy(&vcm_); VideoProcessingModule::Destroy(&vpm_); delete qm_callback_; @@ -453,8 +456,8 @@ bool ViEEncoder::TimeToSendPacket(uint32_t ssrc, uint16_t sequence_number, int64_t capture_time_ms, bool retransmission) { - return default_rtp_rtcp_->TimeToSendPacket(ssrc, sequence_number, - capture_time_ms, retransmission); + return send_payload_router_->TimeToSendPacket( + ssrc, sequence_number, capture_time_ms, retransmission); } size_t ViEEncoder::TimeToSendPadding(size_t bytes) { @@ -465,7 +468,7 @@ size_t ViEEncoder::TimeToSendPadding(size_t bytes) { send_padding_ || video_suspended_ || min_transmit_bitrate_kbps_ > 0; } if (send_padding) { - return default_rtp_rtcp_->TimeToSendPadding(bytes); + return send_payload_router_->TimeToSendPadding(bytes); } return 0; } diff --git a/webrtc/video_engine/vie_encoder.h b/webrtc/video_engine/vie_encoder.h index 3777adba6f..d9c6847f0a 100644 --- a/webrtc/video_engine/vie_encoder.h +++ b/webrtc/video_engine/vie_encoder.h @@ -65,8 +65,15 @@ class ViEEncoder bool Init(); - // This function is assumed to be called before any frames are delivered. - void SetSendPayloadRouter(PayloadRouter* send_payload_router); + // This function is assumed to be called before any frames are delivered and + // only once. + // Ideally this would be done in Init, but the dependencies between ViEEncoder + // and ViEChannel makes it really hard to do in a good way. + void StartThreadsAndSetSendPayloadRouter(PayloadRouter* send_payload_router); + + // This function must be called before the corresponding ViEChannel is + // deleted. + void StopThreadsAndRemovePayloadRouter(); void SetNetworkTransmissionState(bool is_transmitting);