From 2251d6e17438e1a085ff4f88ad19de513214bec1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Peter=20Bostr=C3=B6m?= Date: Thu, 28 May 2015 14:10:39 +0200 Subject: [PATCH] Remove ViESender. Registers transport on construction removing the need for ViESender as a hop and removing a potential deadlock by removing RegisterSendTransport. BUG=1695, 2999 R=stefan@webrtc.org Review URL: https://webrtc-codereview.appspot.com/57449004 Cr-Commit-Position: refs/heads/master@{#9309} --- .../valgrind-webrtc/memcheck/suppressions.txt | 17 ------ .../sanitizers/tsan_suppressions_webrtc.cc | 1 - webrtc/video/call.cc | 8 ++- webrtc/video/video_receive_stream.cc | 7 +-- webrtc/video/video_send_stream.cc | 6 +- webrtc/video_engine/BUILD.gn | 2 - webrtc/video_engine/video_engine_core.gypi | 2 - webrtc/video_engine/vie_channel.cc | 60 +++---------------- webrtc/video_engine/vie_channel.h | 11 +--- webrtc/video_engine/vie_channel_group.cc | 37 +++++------- webrtc/video_engine/vie_channel_group.h | 9 +-- webrtc/video_engine/vie_sender.cc | 60 ------------------- webrtc/video_engine/vie_sender.h | 48 --------------- 13 files changed, 40 insertions(+), 228 deletions(-) delete mode 100644 webrtc/video_engine/vie_sender.cc delete mode 100644 webrtc/video_engine/vie_sender.h diff --git a/tools/valgrind-webrtc/memcheck/suppressions.txt b/tools/valgrind-webrtc/memcheck/suppressions.txt index f80236196..4e290aaa7 100644 --- a/tools/valgrind-webrtc/memcheck/suppressions.txt +++ b/tools/valgrind-webrtc/memcheck/suppressions.txt @@ -185,23 +185,6 @@ fun:StartThread } -{ - bug_329_5 - Memcheck:Param - socketcall.sendto(msg) - obj:*libpthread-*.so - fun:_ZN6webrtc14UdpSocketPosix6SendToEPKaiRKNS_13SocketAddressE - fun:_ZN6webrtc16UdpTransportImpl14SendRTCPPacketEiPKvi - fun:_ZN6webrtc9ViESender14SendRTCPPacketEiPKvi - fun:_ZN6webrtc10RTCPSender13SendToNetworkEPKht - fun:_ZN6webrtc10RTCPSender8SendRTCPEjiPKtbm - fun:_ZN6webrtc17ModuleRtpRtcpImpl7ProcessEv - fun:_ZN6webrtc17ProcessThreadImpl7ProcessEv - fun:_ZN6webrtc17ProcessThreadImpl3RunEPv - fun:_ZN6webrtc11ThreadPosix3RunEv - fun:StartThread -} - { bug_329_7 Memcheck:Unaddressable diff --git a/webrtc/build/sanitizers/tsan_suppressions_webrtc.cc b/webrtc/build/sanitizers/tsan_suppressions_webrtc.cc index 38e62e927..de4d29ae5 100644 --- a/webrtc/build/sanitizers/tsan_suppressions_webrtc.cc +++ b/webrtc/build/sanitizers/tsan_suppressions_webrtc.cc @@ -75,7 +75,6 @@ char kTSanDefaultSuppressions[] = "deadlock:webrtc::ViEChannel::StartSend\n" "deadlock:webrtc::ViECodecImpl::GetSendSideDelay\n" "deadlock:webrtc::ViEEncoder::OnLocalSsrcChanged\n" -"deadlock:webrtc::ViESender::RegisterSendTransport\n" // TODO(pbos): Trace events are racy due to lack of proper POD atomics. // https://code.google.com/p/webrtc/issues/detail?id=2497 diff --git a/webrtc/video/call.cc b/webrtc/video/call.cc index 37f9f2d60..bd9673421 100644 --- a/webrtc/video/call.cc +++ b/webrtc/video/call.cc @@ -117,6 +117,7 @@ class Call : public webrtc::Call, public PacketReceiver { // and receivers. rtc::CriticalSection network_enabled_crit_; bool network_enabled_ GUARDED_BY(network_enabled_crit_); + TransportAdapter transport_adapter_; rtc::scoped_ptr receive_crit_; std::map audio_receive_ssrcs_ @@ -147,11 +148,12 @@ namespace internal { Call::Call(const Call::Config& config) : num_cpu_cores_(CpuInfo::DetectNumberOfCores()), module_process_thread_(ProcessThread::Create()), - channel_group_(new ChannelGroup(module_process_thread_.get(), nullptr)), + channel_group_(new ChannelGroup(module_process_thread_.get())), base_channel_id_(0), next_channel_id_(base_channel_id_ + 1), config_(config), network_enabled_(true), + transport_adapter_(nullptr), receive_crit_(RWLockWrapper::CreateRWLock()), send_crit_(RWLockWrapper::CreateRWLock()) { DCHECK(config.send_transport != nullptr); @@ -169,8 +171,8 @@ Call::Call(const Call::Config& config) // TODO(pbos): Remove base channel when CreateReceiveChannel no longer // requires one. - CHECK(channel_group_->CreateSendChannel(base_channel_id_, 0, num_cpu_cores_, - true)); + CHECK(channel_group_->CreateSendChannel( + base_channel_id_, 0, &transport_adapter_, num_cpu_cores_, true)); if (config.overuse_callback) { overuse_observer_proxy_.reset( diff --git a/webrtc/video/video_receive_stream.cc b/webrtc/video/video_receive_stream.cc index 090aad39f..c3abbe43e 100644 --- a/webrtc/video/video_receive_stream.cc +++ b/webrtc/video/video_receive_stream.cc @@ -138,7 +138,8 @@ VideoReceiveStream::VideoReceiveStream(int num_cpu_cores, channel_id_(channel_id), voe_sync_interface_(nullptr) { CHECK(channel_group_->CreateReceiveChannel(channel_id_, 0, base_channel_id, - num_cpu_cores, true)); + &transport_adapter_, num_cpu_cores, + true)); vie_channel_ = channel_group_->GetChannel(channel_id_); @@ -184,8 +185,6 @@ VideoReceiveStream::VideoReceiveStream(int num_cpu_cores, } } - vie_channel_->RegisterSendTransport(&transport_adapter_); - if (config_.rtp.fec.ulpfec_payload_type != -1) { // ULPFEC without RED doesn't make sense. DCHECK(config_.rtp.fec.red_payload_type != -1); @@ -260,8 +259,6 @@ VideoReceiveStream::~VideoReceiveStream() { for (size_t i = 0; i < config_.decoders.size(); ++i) vie_channel_->DeRegisterExternalDecoder(config_.decoders[i].payload_type); - vie_channel_->DeregisterSendTransport(); - if (voe_sync_interface_ != nullptr) { vie_channel_->SetVoiceChannel(-1, nullptr); voe_sync_interface_->Release(); diff --git a/webrtc/video/video_send_stream.cc b/webrtc/video/video_send_stream.cc index 8fc03c50a..3ea47adc1 100644 --- a/webrtc/video/video_send_stream.cc +++ b/webrtc/video/video_send_stream.cc @@ -119,7 +119,8 @@ VideoSendStream::VideoSendStream( channel_id_(channel_id), use_config_bitrate_(true), stats_proxy_(Clock::GetRealTimeClock(), config) { - CHECK(channel_group->CreateSendChannel(channel_id_, 0, num_cpu_cores, true)); + CHECK(channel_group->CreateSendChannel(channel_id_, 0, &transport_adapter_, + num_cpu_cores, true)); vie_channel_ = channel_group_->GetChannel(channel_id_); vie_encoder_ = channel_group_->GetEncoder(channel_id_); @@ -180,7 +181,6 @@ VideoSendStream::VideoSendStream( vie_capturer_ = new ViECapturer(module_process_thread_, vie_encoder_); - vie_channel_->RegisterSendTransport(&transport_adapter_); // 28 to match packet overhead in ModuleRtpRtcpImpl. vie_channel_->SetMTU( static_cast(config_.rtp.max_packet_size + 28)); @@ -238,8 +238,6 @@ VideoSendStream::~VideoSendStream() { vie_encoder_->RegisterPreEncodeCallback(nullptr); vie_encoder_->RegisterPostEncodeImageCallback(nullptr); - vie_channel_->DeregisterSendTransport(); - vie_capturer_->RegisterCpuOveruseObserver(nullptr); delete vie_capturer_; diff --git a/webrtc/video_engine/BUILD.gn b/webrtc/video_engine/BUILD.gn index d0090ffa4..8a805f7bf 100644 --- a/webrtc/video_engine/BUILD.gn +++ b/webrtc/video_engine/BUILD.gn @@ -41,8 +41,6 @@ source_set("video_engine_core") { "vie_receiver.h", "vie_remb.cc", "vie_remb.h", - "vie_sender.cc", - "vie_sender.h", "vie_sync_module.cc", "vie_sync_module.h", ] diff --git a/webrtc/video_engine/video_engine_core.gypi b/webrtc/video_engine/video_engine_core.gypi index b642e5ed8..dc956714f 100644 --- a/webrtc/video_engine/video_engine_core.gypi +++ b/webrtc/video_engine/video_engine_core.gypi @@ -49,7 +49,6 @@ 'vie_channel_group.h', 'vie_encoder.h', 'vie_receiver.h', - 'vie_sender.h', 'vie_sync_module.h', # ViE @@ -65,7 +64,6 @@ 'vie_encoder.cc', 'vie_receiver.cc', 'vie_remb.cc', - 'vie_sender.cc', 'vie_sync_module.cc', ], # source # TODO(jschuh): Bug 1348: fix size_t to int truncations. diff --git a/webrtc/video_engine/vie_channel.cc b/webrtc/video_engine/vie_channel.cc index 0ee0e97ef..ffbaadd25 100644 --- a/webrtc/video_engine/vie_channel.cc +++ b/webrtc/video_engine/vie_channel.cc @@ -83,6 +83,7 @@ ViEChannel::ViEChannel(int32_t channel_id, int32_t engine_id, uint32_t number_of_cores, const Config& config, + Transport* transport, ProcessThread& module_process_thread, RtcpIntraFrameObserver* intra_frame_observer, RtcpBandwidthObserver* bandwidth_observer, @@ -119,7 +120,7 @@ ViEChannel::ViEChannel(int32_t channel_id, send_timestamp_extension_id_(kInvalidRtpExtensionId), absolute_send_time_extension_id_(kInvalidRtpExtensionId), video_rotation_extension_id_(kInvalidRtpExtensionId), - external_transport_(NULL), + transport_(transport), decoder_reset_(true), wait_for_key_frame_(false), mtu_(0), @@ -1294,10 +1295,6 @@ void ViEChannel::RegisterSendBitrateObserver( int32_t ViEChannel::StartSend() { CriticalSectionScoped cs(callback_cs_.get()); - if (!external_transport_) { - LOG(LS_ERROR) << "No transport set."; - return -1; - } rtp_rtcp_->SetSendingMediaStatus(true); if (rtp_rtcp_->Sending()) { @@ -1368,56 +1365,15 @@ int32_t ViEChannel::StopReceive() { return 0; } -int32_t ViEChannel::RegisterSendTransport(Transport* transport) { - if (rtp_rtcp_->Sending()) { - return -1; - } - - CriticalSectionScoped cs(callback_cs_.get()); - if (external_transport_) { - LOG_F(LS_ERROR) << "Transport already registered."; - return -1; - } - external_transport_ = transport; - vie_sender_.RegisterSendTransport(transport); - return 0; -} - -int32_t ViEChannel::DeregisterSendTransport() { - CriticalSectionScoped cs(callback_cs_.get()); - if (!external_transport_) { - return 0; - } - if (rtp_rtcp_->Sending()) { - LOG_F(LS_ERROR) << "Can't deregister transport when sending."; - return -1; - } - external_transport_ = NULL; - vie_sender_.DeregisterSendTransport(); - return 0; -} - -int32_t ViEChannel::ReceivedRTPPacket( - const void* rtp_packet, const size_t rtp_packet_length, - const PacketTime& packet_time) { - { - CriticalSectionScoped cs(callback_cs_.get()); - if (!external_transport_) { - return -1; - } - } +int32_t ViEChannel::ReceivedRTPPacket(const void* rtp_packet, + size_t rtp_packet_length, + const PacketTime& packet_time) { return vie_receiver_.ReceivedRTPPacket( rtp_packet, rtp_packet_length, packet_time); } -int32_t ViEChannel::ReceivedRTCPPacket( - const void* rtcp_packet, const size_t rtcp_packet_length) { - { - CriticalSectionScoped cs(callback_cs_.get()); - if (!external_transport_) { - return -1; - } - } +int32_t ViEChannel::ReceivedRTCPPacket(const void* rtcp_packet, + size_t rtcp_packet_length) { return vie_receiver_.ReceivedRTCPPacket(rtcp_packet, rtcp_packet_length); } @@ -1636,7 +1592,7 @@ RtpRtcp::Configuration ViEChannel::CreateRtpRtcpConfiguration() { configuration.id = ViEModuleId(engine_id_, channel_id_); configuration.audio = false; configuration.receiver_only = !sender_; - configuration.outgoing_transport = &vie_sender_; + configuration.outgoing_transport = transport_; if (sender_) configuration.intra_frame_callback = intra_frame_observer_; configuration.rtt_stats = rtt_stats_; diff --git a/webrtc/video_engine/vie_channel.h b/webrtc/video_engine/vie_channel.h index 57e187a7a..6016024f0 100644 --- a/webrtc/video_engine/vie_channel.h +++ b/webrtc/video_engine/vie_channel.h @@ -24,7 +24,6 @@ #include "webrtc/typedefs.h" #include "webrtc/video_engine/vie_defines.h" #include "webrtc/video_engine/vie_receiver.h" -#include "webrtc/video_engine/vie_sender.h" #include "webrtc/video_engine/vie_sync_module.h" namespace webrtc { @@ -105,6 +104,7 @@ class ViEChannel : public VCMFrameTypeCallback, int32_t engine_id, uint32_t number_of_cores, const Config& config, + Transport* transport, ProcessThread& module_process_thread, RtcpIntraFrameObserver* intra_frame_observer, RtcpBandwidthObserver* bandwidth_observer, @@ -268,15 +268,9 @@ class ViEChannel : public VCMFrameTypeCallback, int32_t StartReceive(); int32_t StopReceive(); - int32_t RegisterSendTransport(Transport* transport); - int32_t DeregisterSendTransport(); - - // Incoming packet from external transport. int32_t ReceivedRTPPacket(const void* rtp_packet, const size_t rtp_packet_length, const PacketTime& packet_time); - - // Incoming packet from external transport. int32_t ReceivedRTCPPacket(const void* rtcp_packet, const size_t rtcp_packet_length); @@ -487,7 +481,6 @@ class ViEChannel : public VCMFrameTypeCallback, VideoCodingModule* const vcm_; ViEReceiver vie_receiver_; - ViESender vie_sender_; ViESyncModule vie_sync_; // Helper to report call statistics. @@ -511,7 +504,7 @@ class ViEChannel : public VCMFrameTypeCallback, int absolute_send_time_extension_id_; int video_rotation_extension_id_; - Transport* external_transport_; + Transport* const transport_; bool decoder_reset_; // Current receive codec used for codec change callback. diff --git a/webrtc/video_engine/vie_channel_group.cc b/webrtc/video_engine/vie_channel_group.cc index 9330c4463..f5e83ac36 100644 --- a/webrtc/video_engine/vie_channel_group.cc +++ b/webrtc/video_engine/vie_channel_group.cc @@ -141,7 +141,7 @@ class WrappingBitrateEstimator : public RemoteBitrateEstimator { }; } // namespace -ChannelGroup::ChannelGroup(ProcessThread* process_thread, const Config* config) +ChannelGroup::ChannelGroup(ProcessThread* process_thread) : remb_(new VieRemb()), bitrate_allocator_(new BitrateAllocator()), call_stats_(new CallStats()), @@ -154,8 +154,7 @@ ChannelGroup::ChannelGroup(ProcessThread* process_thread, const Config* config) BitrateController::kDefaultStartBitrateKbps, 0)), encoder_map_cs_(CriticalSectionWrapper::CreateCriticalSection()), - config_(config), - own_config_(), + config_(new Config), process_thread_(process_thread), pacer_thread_(ProcessThread::Create()), // Constructed last as this object calls the provided callback on @@ -163,16 +162,8 @@ ChannelGroup::ChannelGroup(ProcessThread* process_thread, const Config* config) bitrate_controller_( BitrateController::CreateBitrateController(Clock::GetRealTimeClock(), this)) { - if (!config) { - own_config_.reset(new Config); - config_ = own_config_.get(); - } - DCHECK(config_); // Must have a valid config pointer here. - - remote_bitrate_estimator_.reset( - new WrappingBitrateEstimator(remb_.get(), - Clock::GetRealTimeClock(), - *config_)); + remote_bitrate_estimator_.reset(new WrappingBitrateEstimator( + remb_.get(), Clock::GetRealTimeClock(), *config_.get())); call_stats_->RegisterStatsObserver(remote_bitrate_estimator_.get()); @@ -200,16 +191,18 @@ ChannelGroup::~ChannelGroup() { bool ChannelGroup::CreateSendChannel(int channel_id, int engine_id, + Transport* transport, int number_of_cores, bool disable_default_encoder) { - rtc::scoped_ptr vie_encoder(new ViEEncoder( - channel_id, number_of_cores, *config_, *process_thread_, pacer_.get(), - bitrate_allocator_.get(), bitrate_controller_.get(), false)); + rtc::scoped_ptr vie_encoder( + new ViEEncoder(channel_id, number_of_cores, *config_.get(), + *process_thread_, pacer_.get(), bitrate_allocator_.get(), + bitrate_controller_.get(), false)); if (!vie_encoder->Init()) { return false; } ViEEncoder* encoder = vie_encoder.get(); - if (!CreateChannel(channel_id, engine_id, number_of_cores, + if (!CreateChannel(channel_id, engine_id, transport, number_of_cores, vie_encoder.release(), true, disable_default_encoder)) { return false; } @@ -232,15 +225,17 @@ bool ChannelGroup::CreateSendChannel(int channel_id, bool ChannelGroup::CreateReceiveChannel(int channel_id, int engine_id, int base_channel_id, + Transport* transport, int number_of_cores, bool disable_default_encoder) { ViEEncoder* encoder = GetEncoder(base_channel_id); - return CreateChannel(channel_id, engine_id, number_of_cores, encoder, false, - disable_default_encoder); + return CreateChannel(channel_id, engine_id, transport, number_of_cores, + encoder, false, disable_default_encoder); } bool ChannelGroup::CreateChannel(int channel_id, int engine_id, + Transport* transport, int number_of_cores, ViEEncoder* vie_encoder, bool sender, @@ -248,8 +243,8 @@ bool ChannelGroup::CreateChannel(int channel_id, DCHECK(vie_encoder); rtc::scoped_ptr channel(new ViEChannel( - channel_id, engine_id, number_of_cores, *config_, *process_thread_, - encoder_state_feedback_->GetRtcpIntraFrameObserver(), + channel_id, engine_id, number_of_cores, *config_.get(), transport, + *process_thread_, encoder_state_feedback_->GetRtcpIntraFrameObserver(), bitrate_controller_->CreateRtcpBandwidthObserver(), remote_bitrate_estimator_.get(), call_stats_->rtcp_rtt_stats(), pacer_.get(), packet_router_.get(), sender, disable_default_encoder)); diff --git a/webrtc/video_engine/vie_channel_group.h b/webrtc/video_engine/vie_channel_group.h index 36bf683a9..094c12179 100644 --- a/webrtc/video_engine/vie_channel_group.h +++ b/webrtc/video_engine/vie_channel_group.h @@ -40,15 +40,17 @@ typedef std::list ChannelList; // group are assumed to send/receive data to the same end-point. class ChannelGroup : public BitrateObserver { public: - ChannelGroup(ProcessThread* process_thread, const Config* config); + explicit ChannelGroup(ProcessThread* process_thread); ~ChannelGroup(); bool CreateSendChannel(int channel_id, int engine_id, + Transport* transport, int number_of_cores, bool disable_default_encoder); bool CreateReceiveChannel(int channel_id, int engine_id, int base_channel_id, + Transport* transport, int number_of_cores, bool disable_default_encoder); void DeleteChannel(int channel_id); @@ -84,6 +86,7 @@ class ChannelGroup : public BitrateObserver { bool CreateChannel(int channel_id, int engine_id, + Transport* transport, int number_of_cores, ViEEncoder* vie_encoder, bool sender, @@ -105,9 +108,7 @@ class ChannelGroup : public BitrateObserver { EncoderMap send_encoders_; rtc::scoped_ptr encoder_map_cs_; - const Config* config_; - // Placeholder for the case where this owns the config. - rtc::scoped_ptr own_config_; + const rtc::scoped_ptr config_; // Registered at construct time and assumed to outlive this class. ProcessThread* process_thread_; diff --git a/webrtc/video_engine/vie_sender.cc b/webrtc/video_engine/vie_sender.cc deleted file mode 100644 index b703e9dff..000000000 --- a/webrtc/video_engine/vie_sender.cc +++ /dev/null @@ -1,60 +0,0 @@ -/* - * Copyright (c) 2012 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. - */ - -#include "webrtc/video_engine/vie_sender.h" - -#include "webrtc/modules/rtp_rtcp/source/rtp_sender.h" -#include "webrtc/system_wrappers/interface/critical_section_wrapper.h" -#include "webrtc/system_wrappers/interface/trace.h" - -namespace webrtc { - -ViESender::ViESender() - : critsect_(CriticalSectionWrapper::CreateCriticalSection()), - transport_(NULL) { -} - -int ViESender::RegisterSendTransport(Transport* transport) { - CriticalSectionScoped cs(critsect_.get()); - if (transport_) { - return -1; - } - transport_ = transport; - return 0; -} - -int ViESender::DeregisterSendTransport() { - CriticalSectionScoped cs(critsect_.get()); - if (transport_ == NULL) { - return -1; - } - transport_ = NULL; - return 0; -} - -int ViESender::SendPacket(int id, const void* data, size_t len) { - CriticalSectionScoped cs(critsect_.get()); - if (!transport_) { - // No transport - return -1; - } - return transport_->SendPacket(id, data, len); -} - -int ViESender::SendRTCPPacket(int id, const void* data, size_t len) { - CriticalSectionScoped cs(critsect_.get()); - if (!transport_) { - return -1; - } - - return transport_->SendRTCPPacket(id, data, len); -} - -} // namespace webrtc diff --git a/webrtc/video_engine/vie_sender.h b/webrtc/video_engine/vie_sender.h deleted file mode 100644 index 1589ce0e0..000000000 --- a/webrtc/video_engine/vie_sender.h +++ /dev/null @@ -1,48 +0,0 @@ -/* - * Copyright (c) 2012 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. - */ - -// ViESender is responsible for sending packets to network. - -#ifndef WEBRTC_VIDEO_ENGINE_VIE_SENDER_H_ -#define WEBRTC_VIDEO_ENGINE_VIE_SENDER_H_ - -#include "webrtc/base/scoped_ptr.h" -#include "webrtc/common_types.h" -#include "webrtc/engine_configurations.h" -#include "webrtc/typedefs.h" -#include "webrtc/video_engine/vie_defines.h" - -namespace webrtc { - -class CriticalSectionWrapper; -class Transport; -class VideoCodingModule; - -class ViESender: public Transport { - public: - ViESender(); - - // Registers transport to use for sending RTP and RTCP. - int RegisterSendTransport(Transport* transport); - int DeregisterSendTransport(); - - // Implements Transport. - int SendPacket(int vie_id, const void* data, size_t len) override; - int SendRTCPPacket(int vie_id, const void* data, size_t len) override; - - private: - rtc::scoped_ptr critsect_; - - Transport* transport_; -}; - -} // namespace webrtc - -#endif // WEBRTC_VIDEO_ENGINE_VIE_SENDER_H_