From e37870297fc45f1253dff4b1c59c85a50d2a8a97 Mon Sep 17 00:00:00 2001 From: mflodman Date: Wed, 21 Oct 2015 13:24:28 +0200 Subject: [PATCH] ChannelGroup cleanup. Move CallStats to Call, EncoderStateFeedback to VideoSendStream and remove last ViEChannel dependency from ChannelGroup. BUG=webrtc:5079 R=pbos@webrtc.org, stefan@webrtc.org Review URL: https://codereview.webrtc.org/1418613002 . Cr-Commit-Position: refs/heads/master@{#10355} --- webrtc/call/call.cc | 14 ++++++++--- webrtc/video/video_receive_stream.cc | 26 +++++++++----------- webrtc/video/video_receive_stream.h | 5 +++- webrtc/video/video_send_stream.cc | 23 ++++++++++-------- webrtc/video/video_send_stream.h | 5 ++++ webrtc/video_engine/vie_channel.cc | 4 --- webrtc/video_engine/vie_channel.h | 1 - webrtc/video_engine/vie_channel_group.cc | 31 ++++++------------------ webrtc/video_engine/vie_channel_group.h | 15 +++++------- 9 files changed, 56 insertions(+), 68 deletions(-) diff --git a/webrtc/call/call.cc b/webrtc/call/call.cc index 4b592583cd..eff441c5fa 100644 --- a/webrtc/call/call.cc +++ b/webrtc/call/call.cc @@ -34,6 +34,7 @@ #include "webrtc/system_wrappers/interface/trace.h" #include "webrtc/video/video_receive_stream.h" #include "webrtc/video/video_send_stream.h" +#include "webrtc/video_engine/call_stats.h" #include "webrtc/voice_engine/include/voe_codec.h" namespace webrtc { @@ -94,6 +95,7 @@ class Call : public webrtc::Call, public PacketReceiver { const int num_cpu_cores_; const rtc::scoped_ptr module_process_thread_; + const rtc::scoped_ptr call_stats_; const rtc::scoped_ptr channel_group_; Call::Config config_; rtc::ThreadChecker configuration_thread_checker_; @@ -138,7 +140,9 @@ namespace internal { Call::Call(const Call::Config& config) : num_cpu_cores_(CpuInfo::DetectNumberOfCores()), module_process_thread_(ProcessThread::Create("ModuleProcessThread")), - channel_group_(new ChannelGroup(module_process_thread_.get())), + call_stats_(new CallStats()), + channel_group_(new ChannelGroup(module_process_thread_.get(), + call_stats_.get())), config_(config), network_enabled_(true), receive_crit_(RWLockWrapper::CreateRWLock()), @@ -162,6 +166,7 @@ Call::Call(const Call::Config& config) Trace::CreateTrace(); module_process_thread_->Start(); + module_process_thread_->RegisterModule(call_stats_.get()); channel_group_->SetBweBitrates(config_.bitrate_config.min_bitrate_bps, config_.bitrate_config.start_bitrate_bps, @@ -177,6 +182,7 @@ Call::~Call() { RTC_CHECK(video_receive_ssrcs_.empty()); RTC_CHECK(video_receive_streams_.empty()); + module_process_thread_->DeRegisterModule(call_stats_.get()); module_process_thread_->Stop(); Trace::ReturnTrace(); } @@ -272,8 +278,8 @@ webrtc::VideoSendStream* Call::CreateVideoSendStream( // TODO(mflodman): Base the start bitrate on a current bandwidth estimate, if // the call has already started. VideoSendStream* send_stream = new VideoSendStream(num_cpu_cores_, - module_process_thread_.get(), channel_group_.get(), config, - encoder_config, suspended_video_send_ssrcs_); + module_process_thread_.get(), call_stats_.get(), channel_group_.get(), + config, encoder_config, suspended_video_send_ssrcs_); // This needs to be taken before send_crit_ as both locks need to be held // while changing network state. @@ -333,7 +339,7 @@ webrtc::VideoReceiveStream* Call::CreateVideoReceiveStream( RTC_DCHECK(configuration_thread_checker_.CalledOnValidThread()); VideoReceiveStream* receive_stream = new VideoReceiveStream( num_cpu_cores_, channel_group_.get(), config, config_.voice_engine, - module_process_thread_.get()); + module_process_thread_.get(), call_stats_.get()); // This needs to be taken before receive_crit_ as both locks need to be held // while changing network state. diff --git a/webrtc/video/video_receive_stream.cc b/webrtc/video/video_receive_stream.cc index e892989f16..380466ab07 100644 --- a/webrtc/video/video_receive_stream.cc +++ b/webrtc/video/video_receive_stream.cc @@ -141,12 +141,14 @@ VideoReceiveStream::VideoReceiveStream(int num_cpu_cores, ChannelGroup* channel_group, const VideoReceiveStream::Config& config, webrtc::VoiceEngine* voice_engine, - ProcessThread* process_thread) + ProcessThread* process_thread, + CallStats* call_stats) : transport_adapter_(config.rtcp_send_transport), encoded_frame_proxy_(config.pre_decode_callback), config_(config), clock_(Clock::GetRealTimeClock()), - channel_group_(channel_group) { + channel_group_(channel_group), + call_stats_(call_stats) { LOG(LS_INFO) << "VideoReceiveStream: " << config_.ToString(); bool send_side_bwe = UseSendSideBwe(config_.rtp.extensions); @@ -155,18 +157,15 @@ VideoReceiveStream::VideoReceiveStream(int num_cpu_cores, channel_group_->GetRemoteBitrateEstimator(send_side_bwe); vie_channel_.reset(new ViEChannel( - num_cpu_cores, &transport_adapter_, process_thread, - channel_group_->GetRtcpIntraFrameObserver(), + num_cpu_cores, &transport_adapter_, process_thread, nullptr, channel_group_->GetBitrateController()->CreateRtcpBandwidthObserver(), - nullptr, bitrate_estimator, - channel_group_->GetCallStats()->rtcp_rtt_stats(), channel_group_->pacer(), - channel_group_->packet_router(), 1, false)); + nullptr, bitrate_estimator, call_stats_->rtcp_rtt_stats(), + channel_group_->pacer(), channel_group_->packet_router(), 1, false)); RTC_CHECK(vie_channel_->Init() == 0); // Register the channel to receive stats updates. - channel_group_->GetCallStats()->RegisterStatsObserver( - vie_channel_->GetStatsObserver()); + call_stats_->RegisterStatsObserver(vie_channel_->GetStatsObserver()); // TODO(pbos): This is not fine grained enough... vie_channel_->SetProtectionMode(config_.rtp.nack.rtp_history_ms > 0, false, @@ -196,10 +195,8 @@ VideoReceiveStream::VideoReceiveStream(int num_cpu_cores, vie_channel_->SetUseRtxPayloadMappingOnRestore( config_.rtp.use_rtx_payload_mapping_on_restore); - // TODO(pbos): Remove channel_group_ usage from VideoReceiveStream. This - // should be configured in call.cc. channel_group_->SetChannelRembStatus(false, config_.rtp.remb, - vie_channel_.get()); + vie_channel_->rtp_rtcp()); for (size_t i = 0; i < config_.rtp.extensions.size(); ++i) { const std::string& extension = config_.rtp.extensions[i].name; @@ -289,9 +286,8 @@ VideoReceiveStream::~VideoReceiveStream() { for (size_t i = 0; i < config_.decoders.size(); ++i) vie_channel_->DeRegisterExternalDecoder(config_.decoders[i].payload_type); - channel_group_->GetCallStats()->DeregisterStatsObserver( - vie_channel_->GetStatsObserver()); - channel_group_->SetChannelRembStatus(false, false, vie_channel_.get()); + call_stats_->DeregisterStatsObserver(vie_channel_->GetStatsObserver()); + channel_group_->SetChannelRembStatus(false, false, vie_channel_->rtp_rtcp()); uint32_t remote_ssrc = vie_channel_->GetRemoteSSRC(); bool send_side_bwe = UseSendSideBwe(config_.rtp.extensions); diff --git a/webrtc/video/video_receive_stream.h b/webrtc/video/video_receive_stream.h index fbea604267..06adb725f7 100644 --- a/webrtc/video/video_receive_stream.h +++ b/webrtc/video/video_receive_stream.h @@ -30,6 +30,7 @@ namespace webrtc { +class CallStats; class VoiceEngine; namespace internal { @@ -43,7 +44,8 @@ class VideoReceiveStream : public webrtc::VideoReceiveStream, ChannelGroup* channel_group, const VideoReceiveStream::Config& config, webrtc::VoiceEngine* voice_engine, - ProcessThread* process_thread); + ProcessThread* process_thread, + CallStats* call_stats); ~VideoReceiveStream() override; // webrtc::ReceiveStream implementation. @@ -81,6 +83,7 @@ class VideoReceiveStream : public webrtc::VideoReceiveStream, Clock* const clock_; ChannelGroup* const channel_group_; + CallStats* const call_stats_; rtc::scoped_ptr incoming_video_stream_; rtc::scoped_ptr stats_proxy_; diff --git a/webrtc/video/video_send_stream.cc b/webrtc/video/video_send_stream.cc index 8e4ccbc733..b9edbba198 100644 --- a/webrtc/video/video_send_stream.cc +++ b/webrtc/video/video_send_stream.cc @@ -22,6 +22,7 @@ #include "webrtc/system_wrappers/interface/logging.h" #include "webrtc/video/video_capture_input.h" #include "webrtc/video_engine/call_stats.h" +#include "webrtc/video_engine/encoder_state_feedback.h" #include "webrtc/video_engine/payload_router.h" #include "webrtc/video_engine/vie_channel.h" #include "webrtc/video_engine/vie_channel_group.h" @@ -111,6 +112,7 @@ namespace internal { VideoSendStream::VideoSendStream( int num_cpu_cores, ProcessThread* module_process_thread, + CallStats* call_stats, ChannelGroup* channel_group, const VideoSendStream::Config& config, const VideoEncoderConfig& encoder_config, @@ -120,7 +122,9 @@ VideoSendStream::VideoSendStream( config_(config), suspended_ssrcs_(suspended_ssrcs), module_process_thread_(module_process_thread), + call_stats_(call_stats), channel_group_(channel_group), + encoder_feedback_(new EncoderStateFeedback()), use_config_bitrate_(true), stats_proxy_(Clock::GetRealTimeClock(), config) { LOG(LS_INFO) << "VideoSendStream: " << config_.ToString(); @@ -146,16 +150,15 @@ VideoSendStream::VideoSendStream( vie_channel_.reset(new ViEChannel( num_cpu_cores, config.send_transport, module_process_thread_, - channel_group_->GetRtcpIntraFrameObserver(), + encoder_feedback_->GetRtcpIntraFrameObserver(), channel_group_->GetBitrateController()->CreateRtcpBandwidthObserver(), transport_feedback_observer, channel_group_->GetRemoteBitrateEstimator(false), - channel_group_->GetCallStats()->rtcp_rtt_stats(), channel_group_->pacer(), + call_stats_->rtcp_rtt_stats(), channel_group_->pacer(), channel_group_->packet_router(), ssrcs.size(), true)); RTC_CHECK(vie_channel_->Init() == 0); - channel_group_->GetCallStats()->RegisterStatsObserver( - vie_channel_->GetStatsObserver()); + call_stats_->RegisterStatsObserver(vie_channel_->GetStatsObserver()); vie_encoder_->StartThreadsAndSetSharedMembers( vie_channel_->send_payload_router(), @@ -183,8 +186,7 @@ VideoSendStream::VideoSendStream( } } - // TODO(pbos): Consider configuring REMB in Call. - channel_group_->SetChannelRembStatus(true, false, vie_channel_.get()); + channel_group_->SetChannelRembStatus(true, false, vie_channel_->rtp_rtcp()); // Enable NACK, FEC or both. const bool enable_protection_nack = config_.rtp.nack.rtp_history_ms > 0; @@ -226,7 +228,8 @@ VideoSendStream::VideoSendStream( if (config_.suspend_below_min_bitrate) vie_encoder_->SuspendBelowMinBitrate(); - channel_group_->AddEncoder(ssrcs, vie_encoder_.get()); + channel_group_->AddEncoder(vie_encoder_.get()); + encoder_feedback_->AddEncoder(ssrcs, vie_encoder_.get()); vie_channel_->RegisterSendChannelRtcpStatisticsCallback(&stats_proxy_); vie_channel_->RegisterSendChannelRtpStatisticsCallback(&stats_proxy_); @@ -250,13 +253,13 @@ VideoSendStream::~VideoSendStream() { vie_encoder_->DeRegisterExternalEncoder( config_.encoder_settings.payload_type); - channel_group_->GetCallStats()->DeregisterStatsObserver( - vie_channel_->GetStatsObserver()); - channel_group_->SetChannelRembStatus(false, false, vie_channel_.get()); + call_stats_->DeregisterStatsObserver(vie_channel_->GetStatsObserver()); + channel_group_->SetChannelRembStatus(false, false, vie_channel_->rtp_rtcp()); // Remove the feedback, stop all encoding threads and processing. This must be // done before deleting the channel. channel_group_->RemoveEncoder(vie_encoder_.get()); + encoder_feedback_->RemoveEncoder(vie_encoder_.get()); vie_encoder_->StopThreadsAndRemoveSharedMembers(); uint32_t remote_ssrc = vie_channel_->GetRemoteSSRC(); diff --git a/webrtc/video/video_send_stream.h b/webrtc/video/video_send_stream.h index 68473b1f1d..d61e50f870 100644 --- a/webrtc/video/video_send_stream.h +++ b/webrtc/video/video_send_stream.h @@ -27,7 +27,9 @@ namespace webrtc { +class CallStats; class ChannelGroup; +class EncoderStateFeedback; class ProcessThread; class ViEChannel; class ViEEncoder; @@ -39,6 +41,7 @@ class VideoSendStream : public webrtc::VideoSendStream, public: VideoSendStream(int num_cpu_cores, ProcessThread* module_process_thread, + CallStats* call_stats, ChannelGroup* channel_group, const VideoSendStream::Config& config, const VideoEncoderConfig& encoder_config, @@ -76,11 +79,13 @@ class VideoSendStream : public webrtc::VideoSendStream, std::map suspended_ssrcs_; ProcessThread* const module_process_thread_; + CallStats* const call_stats_; ChannelGroup* const channel_group_; rtc::scoped_ptr input_; rtc::scoped_ptr vie_channel_; rtc::scoped_ptr vie_encoder_; + rtc::scoped_ptr encoder_feedback_; // Used as a workaround to indicate that we should be using the configured // start bitrate initially, instead of the one reported by VideoEngine (which diff --git a/webrtc/video_engine/vie_channel.cc b/webrtc/video_engine/vie_channel.cc index ba6d524b77..9a737113b9 100644 --- a/webrtc/video_engine/vie_channel.cc +++ b/webrtc/video_engine/vie_channel.cc @@ -609,10 +609,6 @@ int ViEChannel::GetRequiredNackListSize(int target_delay_ms) { return target_delay_ms * 40 * 30 / 1000; } -void ViEChannel::EnableRemb(bool enable) { - rtp_rtcp_modules_[0]->SetREMBStatus(enable); -} - int ViEChannel::SetSendTimestampOffsetStatus(bool enable, int id) { // Disable any previous registrations of this extension to avoid errors. for (RtpRtcp* rtp_rtcp : rtp_rtcp_modules_) { diff --git a/webrtc/video_engine/vie_channel.h b/webrtc/video_engine/vie_channel.h index cd0646b979..3e95cb1885 100644 --- a/webrtc/video_engine/vie_channel.h +++ b/webrtc/video_engine/vie_channel.h @@ -109,7 +109,6 @@ class ViEChannel : public VCMFrameTypeCallback, bool IsSendingFecEnabled(); int SetSenderBufferingMode(int target_delay_ms); int SetReceiverBufferingMode(int target_delay_ms); - void EnableRemb(bool enable); int SetSendTimestampOffsetStatus(bool enable, int id); int SetReceiveTimestampOffsetStatus(bool enable, int id); int SetSendAbsoluteSendTimeStatus(bool enable, int id); diff --git a/webrtc/video_engine/vie_channel_group.cc b/webrtc/video_engine/vie_channel_group.cc index 75865aec80..62d6040e01 100644 --- a/webrtc/video_engine/vie_channel_group.cc +++ b/webrtc/video_engine/vie_channel_group.cc @@ -25,9 +25,7 @@ #include "webrtc/system_wrappers/interface/critical_section_wrapper.h" #include "webrtc/system_wrappers/interface/logging.h" #include "webrtc/video_engine/call_stats.h" -#include "webrtc/video_engine/encoder_state_feedback.h" #include "webrtc/video_engine/payload_router.h" -#include "webrtc/video_engine/vie_channel.h" #include "webrtc/video_engine/vie_encoder.h" #include "webrtc/video_engine/vie_remb.h" #include "webrtc/voice_engine/include/voe_video_sync.h" @@ -145,10 +143,10 @@ class WrappingBitrateEstimator : public RemoteBitrateEstimator { } // namespace -ChannelGroup::ChannelGroup(ProcessThread* process_thread) +ChannelGroup::ChannelGroup(ProcessThread* process_thread, + CallStats* call_stats) : remb_(new VieRemb()), bitrate_allocator_(new BitrateAllocator()), - call_stats_(new CallStats()), packet_router_(new PacketRouter()), pacer_(new PacedSender(Clock::GetRealTimeClock(), packet_router_.get(), @@ -161,8 +159,8 @@ ChannelGroup::ChannelGroup(ProcessThread* process_thread) remote_estimator_proxy_( new RemoteEstimatorProxy(Clock::GetRealTimeClock(), packet_router_.get())), - encoder_state_feedback_(new EncoderStateFeedback()), process_thread_(process_thread), + call_stats_(call_stats), pacer_thread_(ProcessThread::Create("PacerThread")), // Constructed last as this object calls the provided callback on // construction. @@ -177,7 +175,6 @@ ChannelGroup::ChannelGroup(ProcessThread* process_thread) process_thread->RegisterModule(remote_estimator_proxy_.get()); process_thread->RegisterModule(remote_bitrate_estimator_.get()); - process_thread->RegisterModule(call_stats_.get()); process_thread->RegisterModule(bitrate_controller_.get()); } @@ -185,7 +182,6 @@ ChannelGroup::~ChannelGroup() { pacer_thread_->Stop(); pacer_thread_->DeRegisterModule(pacer_.get()); process_thread_->DeRegisterModule(bitrate_controller_.get()); - process_thread_->DeRegisterModule(call_stats_.get()); process_thread_->DeRegisterModule(remote_bitrate_estimator_.get()); process_thread_->DeRegisterModule(remote_estimator_proxy_.get()); call_stats_->DeregisterStatsObserver(remote_bitrate_estimator_.get()); @@ -195,15 +191,12 @@ ChannelGroup::~ChannelGroup() { RTC_DCHECK(encoders_.empty()); } -void ChannelGroup::AddEncoder(const std::vector& ssrcs, - ViEEncoder* encoder) { - encoder_state_feedback_->AddEncoder(ssrcs, encoder); +void ChannelGroup::AddEncoder(ViEEncoder* encoder) { rtc::CritScope lock(&encoder_crit_); encoders_.push_back(encoder); } void ChannelGroup::RemoveEncoder(ViEEncoder* encoder) { - encoder_state_feedback_->RemoveEncoder(encoder); rtc::CritScope lock(&encoder_crit_); for (auto it = encoders_.begin(); it != encoders_.end(); ++it) { if (*it == encoder) { @@ -240,10 +233,6 @@ RemoteBitrateEstimator* ChannelGroup::GetRemoteBitrateEstimator( return remote_bitrate_estimator_.get(); } -CallStats* ChannelGroup::GetCallStats() const { - return call_stats_.get(); -} - TransportFeedbackObserver* ChannelGroup::GetTransportFeedbackObserver() { if (transport_feedback_adapter_.get() == nullptr) { transport_feedback_adapter_.reset(new TransportFeedbackAdapter( @@ -259,21 +248,15 @@ TransportFeedbackObserver* ChannelGroup::GetTransportFeedbackObserver() { return transport_feedback_adapter_.get(); } -RtcpIntraFrameObserver* ChannelGroup::GetRtcpIntraFrameObserver() const { - return encoder_state_feedback_->GetRtcpIntraFrameObserver(); -} - int64_t ChannelGroup::GetPacerQueuingDelayMs() const { return pacer_->QueueInMs(); } +// TODO(mflodman): Move out of this class. void ChannelGroup::SetChannelRembStatus(bool sender, bool receiver, - ViEChannel* channel) { - // Update the channel state. - channel->EnableRemb(sender || receiver); - // Update the REMB instance with necessary RTP modules. - RtpRtcp* rtp_module = channel->rtp_rtcp(); + RtpRtcp* rtp_module) { + rtp_module->SetREMBStatus(sender || receiver); if (sender) { remb_->AddRembSender(rtp_module); } else { diff --git a/webrtc/video_engine/vie_channel_group.h b/webrtc/video_engine/vie_channel_group.h index 528a3a6bd9..d26ee10844 100644 --- a/webrtc/video_engine/vie_channel_group.h +++ b/webrtc/video_engine/vie_channel_group.h @@ -28,16 +28,14 @@ namespace webrtc { class BitrateAllocator; class CallStats; class Config; -class EncoderStateFeedback; -class I420FrameCallback; class PacedSender; class PacketRouter; class ProcessThread; class RemoteBitrateEstimator; class RemoteEstimatorProxy; +class RtpRtcp; class SendStatisticsProxy; class TransportFeedbackAdapter; -class ViEChannel; class ViEEncoder; class VieRemb; @@ -45,21 +43,20 @@ class VieRemb; // group are assumed to send/receive data to the same end-point. class ChannelGroup : public BitrateObserver { public: - explicit ChannelGroup(ProcessThread* process_thread); + ChannelGroup(ProcessThread* process_thread, CallStats* call_stats); ~ChannelGroup(); - void AddEncoder(const std::vector& ssrcs, ViEEncoder* encoder); + void AddEncoder(ViEEncoder* encoder); void RemoveEncoder(ViEEncoder* encoder); void SetBweBitrates(int min_bitrate_bps, int start_bitrate_bps, int max_bitrate_bps); - void SetChannelRembStatus(bool sender, bool receiver, ViEChannel* channel); + void SetChannelRembStatus(bool sender, bool receiver, RtpRtcp* rtp_module); void SignalNetworkState(NetworkState state); BitrateController* GetBitrateController() const; RemoteBitrateEstimator* GetRemoteBitrateEstimator(bool send_side_bwe) const; - CallStats* GetCallStats() const; int64_t GetPacerQueuingDelayMs() const; PacedSender* pacer() const { return pacer_.get(); } PacketRouter* packet_router() const { return packet_router_.get(); } @@ -78,18 +75,18 @@ class ChannelGroup : public BitrateObserver { private: rtc::scoped_ptr remb_; rtc::scoped_ptr bitrate_allocator_; - rtc::scoped_ptr call_stats_; rtc::scoped_ptr packet_router_; rtc::scoped_ptr pacer_; rtc::scoped_ptr remote_bitrate_estimator_; rtc::scoped_ptr remote_estimator_proxy_; - rtc::scoped_ptr encoder_state_feedback_; mutable rtc::CriticalSection encoder_crit_; std::vector encoders_ GUARDED_BY(encoder_crit_); // Registered at construct time and assumed to outlive this class. ProcessThread* const process_thread_; + CallStats* const call_stats_; + rtc::scoped_ptr pacer_thread_; rtc::scoped_ptr bitrate_controller_;