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);