From 0245da0fa0eb370dd7b8d8fb1a2da89200fe46ff Mon Sep 17 00:00:00 2001 From: nisse Date: Wed, 30 Nov 2016 03:35:20 -0800 Subject: [PATCH] Move ownership of PacketRouter from CongestionController to Call. And delete the method CongestionController::packet_router. BUG=None Review-Url: https://codereview.webrtc.org/2516983004 Cr-Commit-Position: refs/heads/master@{#15323} --- webrtc/audio/audio_receive_stream.cc | 37 +++--------- webrtc/audio/audio_receive_stream.h | 7 ++- webrtc/audio/audio_receive_stream_unittest.cc | 57 ++++++++----------- webrtc/audio/audio_send_stream.cc | 4 +- webrtc/audio/audio_send_stream.h | 2 + webrtc/audio/audio_send_stream_unittest.cc | 35 ++++++------ webrtc/call/call.cc | 31 ++++++---- .../congestion_controller.cc | 53 ++++++++--------- .../congestion_controller_unittest.cc | 4 +- .../include/congestion_controller.h | 17 ++++-- .../include/mock/mock_congestion_controller.h | 7 ++- .../congestion_controller_feedback_fuzzer.cc | 4 +- webrtc/tools/event_log_visualizer/analyzer.cc | 4 +- webrtc/video/video_receive_stream.cc | 3 +- webrtc/video/video_receive_stream.h | 1 + webrtc/video/video_send_stream.cc | 22 ++++--- webrtc/video/video_send_stream.h | 2 + 17 files changed, 149 insertions(+), 141 deletions(-) diff --git a/webrtc/audio/audio_receive_stream.cc b/webrtc/audio/audio_receive_stream.cc index 81ffdade5c..8da0616f1d 100644 --- a/webrtc/audio/audio_receive_stream.cc +++ b/webrtc/audio/audio_receive_stream.cc @@ -20,7 +20,6 @@ #include "webrtc/base/checks.h" #include "webrtc/base/logging.h" #include "webrtc/base/timeutils.h" -#include "webrtc/modules/congestion_controller/include/congestion_controller.h" #include "webrtc/modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h" #include "webrtc/voice_engine/channel_proxy.h" #include "webrtc/voice_engine/include/voe_base.h" @@ -32,20 +31,6 @@ #include "webrtc/voice_engine/voice_engine_impl.h" namespace webrtc { -namespace { - -bool UseSendSideBwe(const webrtc::AudioReceiveStream::Config& config) { - if (!config.rtp.transport_cc) { - return false; - } - for (const auto& extension : config.rtp.extensions) { - if (extension.uri == RtpExtension::kTransportSequenceNumberUri) { - return true; - } - } - return false; -} -} // namespace std::string AudioReceiveStream::Config::Rtp::ToString() const { std::stringstream ss; @@ -80,17 +65,20 @@ std::string AudioReceiveStream::Config::ToString() const { namespace internal { AudioReceiveStream::AudioReceiveStream( - CongestionController* congestion_controller, + PacketRouter* packet_router, + RemoteBitrateEstimator* remote_bitrate_estimator, const webrtc::AudioReceiveStream::Config& config, const rtc::scoped_refptr& audio_state, webrtc::RtcEventLog* event_log) - : config_(config), + : remote_bitrate_estimator_(remote_bitrate_estimator), + config_(config), audio_state_(audio_state), rtp_header_parser_(RtpHeaderParser::Create()) { LOG(LS_INFO) << "AudioReceiveStream: " << config_.ToString(); RTC_DCHECK_NE(config_.voe_channel_id, -1); RTC_DCHECK(audio_state_.get()); - RTC_DCHECK(congestion_controller); + RTC_DCHECK(packet_router); + RTC_DCHECK(remote_bitrate_estimator); RTC_DCHECK(rtp_header_parser_); VoiceEngineImpl* voe_impl = static_cast(voice_engine()); @@ -129,12 +117,7 @@ AudioReceiveStream::AudioReceiveStream( } } // Configure bandwidth estimation. - channel_proxy_->RegisterReceiverCongestionControlObjects( - congestion_controller->packet_router()); - if (UseSendSideBwe(config)) { - remote_bitrate_estimator_ = - congestion_controller->GetRemoteBitrateEstimator(true); - } + channel_proxy_->RegisterReceiverCongestionControlObjects(packet_router); } AudioReceiveStream::~AudioReceiveStream() { @@ -147,9 +130,7 @@ AudioReceiveStream::~AudioReceiveStream() { channel_proxy_->DeRegisterExternalTransport(); channel_proxy_->ResetCongestionControlObjects(); channel_proxy_->SetRtcEventLog(nullptr); - if (remote_bitrate_estimator_) { - remote_bitrate_estimator_->RemoveStream(config_.rtp.remote_ssrc); - } + remote_bitrate_estimator_->RemoveStream(config_.rtp.remote_ssrc); } void AudioReceiveStream::Start() { @@ -288,7 +269,7 @@ bool AudioReceiveStream::DeliverRtp(const uint8_t* packet, // Only forward if the parsed header has one of the headers necessary for // bandwidth estimation. RTP timestamps has different rates for audio and // video and shouldn't be mixed. - if (remote_bitrate_estimator_ && + if (config_.rtp.transport_cc && header.extension.hasTransportSequenceNumber) { int64_t arrival_time_ms = rtc::TimeMillis(); if (packet_time.timestamp >= 0) diff --git a/webrtc/audio/audio_receive_stream.h b/webrtc/audio/audio_receive_stream.h index 6e7da09d1d..3bba54ebb8 100644 --- a/webrtc/audio/audio_receive_stream.h +++ b/webrtc/audio/audio_receive_stream.h @@ -22,9 +22,9 @@ #include "webrtc/modules/rtp_rtcp/include/rtp_header_parser.h" namespace webrtc { -class CongestionController; class RemoteBitrateEstimator; class RtcEventLog; +class PacketRouter; namespace voe { class ChannelProxy; @@ -36,7 +36,8 @@ class AudioSendStream; class AudioReceiveStream final : public webrtc::AudioReceiveStream, public AudioMixer::Source { public: - AudioReceiveStream(CongestionController* congestion_controller, + AudioReceiveStream(PacketRouter* packet_router, + RemoteBitrateEstimator* remote_bitrate_estimator, const webrtc::AudioReceiveStream::Config& config, const rtc::scoped_refptr& audio_state, webrtc::RtcEventLog* event_log); @@ -69,7 +70,7 @@ class AudioReceiveStream final : public webrtc::AudioReceiveStream, int SetVoiceEnginePlayout(bool playout); rtc::ThreadChecker thread_checker_; - RemoteBitrateEstimator* remote_bitrate_estimator_ = nullptr; + RemoteBitrateEstimator* const remote_bitrate_estimator_; const webrtc::AudioReceiveStream::Config config_; rtc::scoped_refptr audio_state_; std::unique_ptr rtp_header_parser_; diff --git a/webrtc/audio/audio_receive_stream_unittest.cc b/webrtc/audio/audio_receive_stream_unittest.cc index bc071970e1..4c443e0f2e 100644 --- a/webrtc/audio/audio_receive_stream_unittest.cc +++ b/webrtc/audio/audio_receive_stream_unittest.cc @@ -17,11 +17,9 @@ #include "webrtc/logging/rtc_event_log/mock/mock_rtc_event_log.h" #include "webrtc/modules/audio_coding/codecs/mock/mock_audio_decoder_factory.h" #include "webrtc/modules/bitrate_controller/include/mock/mock_bitrate_controller.h" -#include "webrtc/modules/congestion_controller/include/mock/mock_congestion_controller.h" #include "webrtc/modules/pacing/packet_router.h" #include "webrtc/modules/remote_bitrate_estimator/include/mock/mock_remote_bitrate_estimator.h" #include "webrtc/modules/rtp_rtcp/source/byte_io.h" -#include "webrtc/system_wrappers/include/clock.h" #include "webrtc/test/gtest.h" #include "webrtc/test/mock_voe_channel_proxy.h" #include "webrtc/test/mock_voice_engine.h" @@ -67,12 +65,7 @@ const AudioDecodingCallStats kAudioDecodeStats = MakeAudioDecodeStatsForTest(); struct ConfigHelper { ConfigHelper() - : simulated_clock_(123456), - decoder_factory_(new rtc::RefCountedObject), - congestion_controller_(&simulated_clock_, - &bitrate_observer_, - &remote_bitrate_observer_, - &event_log_), + : decoder_factory_(new rtc::RefCountedObject), audio_mixer_(new rtc::RefCountedObject()) { using testing::Invoke; @@ -104,8 +97,6 @@ struct ConfigHelper { EXPECT_CALL(*channel_proxy_, RegisterReceiverCongestionControlObjects(&packet_router_)) .Times(1); - EXPECT_CALL(congestion_controller_, packet_router()) - .WillOnce(Return(&packet_router_)); EXPECT_CALL(*channel_proxy_, ResetCongestionControlObjects()) .Times(1); EXPECT_CALL(*channel_proxy_, RegisterExternalTransport(nullptr)) @@ -134,9 +125,7 @@ struct ConfigHelper { stream_config_.decoder_factory = decoder_factory_; } - MockCongestionController* congestion_controller() { - return &congestion_controller_; - } + PacketRouter* packet_router() { return &packet_router_; } MockRemoteBitrateEstimator* remote_bitrate_estimator() { return &remote_bitrate_estimator_; } @@ -148,9 +137,6 @@ struct ConfigHelper { MockVoEChannelProxy* channel_proxy() { return channel_proxy_; } void SetupMockForBweFeedback(bool send_side_bwe) { - EXPECT_CALL(congestion_controller_, - GetRemoteBitrateEstimator(send_side_bwe)) - .WillOnce(Return(&remote_bitrate_estimator_)); EXPECT_CALL(remote_bitrate_estimator_, RemoveStream(stream_config_.rtp.remote_ssrc)); } @@ -176,12 +162,8 @@ struct ConfigHelper { } private: - SimulatedClock simulated_clock_; PacketRouter packet_router_; - testing::NiceMock bitrate_observer_; - testing::NiceMock remote_bitrate_observer_; rtc::scoped_refptr decoder_factory_; - MockCongestionController congestion_controller_; MockRemoteBitrateEstimator remote_bitrate_estimator_; MockRtcEventLog event_log_; testing::StrictMock voice_engine_; @@ -261,8 +243,9 @@ TEST(AudioReceiveStreamTest, ConfigToString) { TEST(AudioReceiveStreamTest, ConstructDestruct) { ConfigHelper helper; internal::AudioReceiveStream recv_stream( - helper.congestion_controller(), helper.config(), helper.audio_state(), - helper.event_log()); + helper.packet_router(), + helper.remote_bitrate_estimator(), + helper.config(), helper.audio_state(), helper.event_log()); } MATCHER_P(VerifyHeaderExtension, expected_extension, "") { @@ -277,8 +260,9 @@ TEST(AudioReceiveStreamTest, ReceiveRtpPacket) { helper.config().rtp.transport_cc = true; helper.SetupMockForBweFeedback(true); internal::AudioReceiveStream recv_stream( - helper.congestion_controller(), helper.config(), helper.audio_state(), - helper.event_log()); + helper.packet_router(), + helper.remote_bitrate_estimator(), + helper.config(), helper.audio_state(), helper.event_log()); const int kTransportSequenceNumberValue = 1234; std::vector rtp_packet = CreateRtpHeaderWithOneByteExtension( kTransportSequenceNumberId, kTransportSequenceNumberValue, 2); @@ -306,8 +290,9 @@ TEST(AudioReceiveStreamTest, ReceiveRtcpPacket) { helper.config().rtp.transport_cc = true; helper.SetupMockForBweFeedback(true); internal::AudioReceiveStream recv_stream( - helper.congestion_controller(), helper.config(), helper.audio_state(), - helper.event_log()); + helper.packet_router(), + helper.remote_bitrate_estimator(), + helper.config(), helper.audio_state(), helper.event_log()); std::vector rtcp_packet = CreateRtcpSenderReport(); EXPECT_CALL(*helper.channel_proxy(), @@ -319,8 +304,9 @@ TEST(AudioReceiveStreamTest, ReceiveRtcpPacket) { TEST(AudioReceiveStreamTest, GetStats) { ConfigHelper helper; internal::AudioReceiveStream recv_stream( - helper.congestion_controller(), helper.config(), helper.audio_state(), - helper.event_log()); + helper.packet_router(), + helper.remote_bitrate_estimator(), + helper.config(), helper.audio_state(), helper.event_log()); helper.SetupMockForGetStats(); AudioReceiveStream::Stats stats = recv_stream.GetStats(); EXPECT_EQ(kRemoteSsrc, stats.remote_ssrc); @@ -364,8 +350,9 @@ TEST(AudioReceiveStreamTest, GetStats) { TEST(AudioReceiveStreamTest, SetGain) { ConfigHelper helper; internal::AudioReceiveStream recv_stream( - helper.congestion_controller(), helper.config(), helper.audio_state(), - helper.event_log()); + helper.packet_router(), + helper.remote_bitrate_estimator(), + helper.config(), helper.audio_state(), helper.event_log()); EXPECT_CALL(*helper.channel_proxy(), SetChannelOutputVolumeScaling(FloatEq(0.765f))); recv_stream.SetGain(0.765f); @@ -374,8 +361,9 @@ TEST(AudioReceiveStreamTest, SetGain) { TEST(AudioReceiveStreamTest, StreamShouldNotBeAddedToMixerWhenVoEReturnsError) { ConfigHelper helper; internal::AudioReceiveStream recv_stream( - helper.congestion_controller(), helper.config(), helper.audio_state(), - helper.event_log()); + helper.packet_router(), + helper.remote_bitrate_estimator(), + helper.config(), helper.audio_state(), helper.event_log()); EXPECT_CALL(helper.voice_engine(), StartPlayout(_)).WillOnce(Return(-1)); EXPECT_CALL(*helper.audio_mixer(), AddSource(_)).Times(0); @@ -386,8 +374,9 @@ TEST(AudioReceiveStreamTest, StreamShouldNotBeAddedToMixerWhenVoEReturnsError) { TEST(AudioReceiveStreamTest, StreamShouldBeAddedToMixerOnStart) { ConfigHelper helper; internal::AudioReceiveStream recv_stream( - helper.congestion_controller(), helper.config(), helper.audio_state(), - helper.event_log()); + helper.packet_router(), + helper.remote_bitrate_estimator(), + helper.config(), helper.audio_state(), helper.event_log()); EXPECT_CALL(helper.voice_engine(), StartPlayout(_)).WillOnce(Return(0)); EXPECT_CALL(helper.voice_engine(), StopPlayout(_)); diff --git a/webrtc/audio/audio_send_stream.cc b/webrtc/audio/audio_send_stream.cc index b031762b4c..179edf2d3d 100644 --- a/webrtc/audio/audio_send_stream.cc +++ b/webrtc/audio/audio_send_stream.cc @@ -45,6 +45,7 @@ AudioSendStream::AudioSendStream( const webrtc::AudioSendStream::Config& config, const rtc::scoped_refptr& audio_state, rtc::TaskQueue* worker_queue, + PacketRouter* packet_router, CongestionController* congestion_controller, BitrateAllocator* bitrate_allocator, RtcEventLog* event_log) @@ -62,8 +63,7 @@ AudioSendStream::AudioSendStream( channel_proxy_->SetRtcEventLog(event_log); channel_proxy_->RegisterSenderCongestionControlObjects( congestion_controller->pacer(), - congestion_controller->GetTransportFeedbackObserver(), - congestion_controller->packet_router()); + congestion_controller->GetTransportFeedbackObserver(), packet_router); channel_proxy_->SetRTCPStatus(true); channel_proxy_->SetLocalSSRC(config.rtp.ssrc); channel_proxy_->SetRTCP_CNAME(config.rtp.c_name); diff --git a/webrtc/audio/audio_send_stream.h b/webrtc/audio/audio_send_stream.h index af0513ccf7..138f4fca7b 100644 --- a/webrtc/audio/audio_send_stream.h +++ b/webrtc/audio/audio_send_stream.h @@ -23,6 +23,7 @@ namespace webrtc { class CongestionController; class VoiceEngine; class RtcEventLog; +class PacketRouter; namespace voe { class ChannelProxy; @@ -35,6 +36,7 @@ class AudioSendStream final : public webrtc::AudioSendStream, AudioSendStream(const webrtc::AudioSendStream::Config& config, const rtc::scoped_refptr& audio_state, rtc::TaskQueue* worker_queue, + PacketRouter* packet_router, CongestionController* congestion_controller, BitrateAllocator* bitrate_allocator, RtcEventLog* event_log); diff --git a/webrtc/audio/audio_send_stream_unittest.cc b/webrtc/audio/audio_send_stream_unittest.cc index dbc49662e6..496a871d85 100644 --- a/webrtc/audio/audio_send_stream_unittest.cc +++ b/webrtc/audio/audio_send_stream_unittest.cc @@ -67,7 +67,8 @@ struct ConfigHelper { congestion_controller_(&simulated_clock_, &bitrate_observer_, &remote_bitrate_observer_, - &event_log_), + &event_log_, + &packet_router_), bitrate_allocator_(&limit_observer_), worker_queue_("ConfigHelper_worker_queue") { using testing::Invoke; @@ -110,6 +111,7 @@ struct ConfigHelper { AudioSendStream::Config& config() { return stream_config_; } rtc::scoped_refptr audio_state() { return audio_state_; } MockVoEChannelProxy* channel_proxy() { return channel_proxy_; } + PacketRouter* packet_router() { return &packet_router_; } CongestionController* congestion_controller() { return &congestion_controller_; } @@ -135,7 +137,7 @@ struct ConfigHelper { RegisterSenderCongestionControlObjects( congestion_controller_.pacer(), congestion_controller_.GetTransportFeedbackObserver(), - congestion_controller_.packet_router())) + packet_router())) .Times(1); EXPECT_CALL(*channel_proxy_, ResetCongestionControlObjects()).Times(1); EXPECT_CALL(*channel_proxy_, RegisterExternalTransport(nullptr)).Times(1); @@ -218,6 +220,7 @@ struct ConfigHelper { testing::NiceMock remote_bitrate_observer_; MockAudioProcessing audio_processing_; AudioProcessing::AudioProcessingStatistics audio_processing_stats_; + PacketRouter packet_router_; CongestionController congestion_controller_; MockRtcEventLog event_log_; testing::NiceMock limit_observer_; @@ -264,16 +267,16 @@ TEST(AudioSendStreamTest, ConstructDestruct) { ConfigHelper helper; internal::AudioSendStream send_stream( helper.config(), helper.audio_state(), helper.worker_queue(), - helper.congestion_controller(), helper.bitrate_allocator(), - helper.event_log()); + helper.packet_router(), helper.congestion_controller(), + helper.bitrate_allocator(), helper.event_log()); } TEST(AudioSendStreamTest, SendTelephoneEvent) { ConfigHelper helper; internal::AudioSendStream send_stream( helper.config(), helper.audio_state(), helper.worker_queue(), - helper.congestion_controller(), helper.bitrate_allocator(), - helper.event_log()); + helper.packet_router(), helper.congestion_controller(), + helper.bitrate_allocator(), helper.event_log()); helper.SetupMockForSendTelephoneEvent(); EXPECT_TRUE(send_stream.SendTelephoneEvent(kTelephoneEventPayloadType, kTelephoneEventPayloadFrequency, kTelephoneEventCode, @@ -284,8 +287,8 @@ TEST(AudioSendStreamTest, SetMuted) { ConfigHelper helper; internal::AudioSendStream send_stream( helper.config(), helper.audio_state(), helper.worker_queue(), - helper.congestion_controller(), helper.bitrate_allocator(), - helper.event_log()); + helper.packet_router(), helper.congestion_controller(), + helper.bitrate_allocator(), helper.event_log()); EXPECT_CALL(*helper.channel_proxy(), SetInputMute(true)); send_stream.SetMuted(true); } @@ -294,8 +297,8 @@ TEST(AudioSendStreamTest, GetStats) { ConfigHelper helper; internal::AudioSendStream send_stream( helper.config(), helper.audio_state(), helper.worker_queue(), - helper.congestion_controller(), helper.bitrate_allocator(), - helper.event_log()); + helper.packet_router(), helper.congestion_controller(), + helper.bitrate_allocator(), helper.event_log()); helper.SetupMockForGetStats(); AudioSendStream::Stats stats = send_stream.GetStats(); EXPECT_EQ(kSsrc, stats.local_ssrc); @@ -325,8 +328,8 @@ TEST(AudioSendStreamTest, GetStatsTypingNoiseDetected) { ConfigHelper helper; internal::AudioSendStream send_stream( helper.config(), helper.audio_state(), helper.worker_queue(), - helper.congestion_controller(), helper.bitrate_allocator(), - helper.event_log()); + helper.packet_router(), helper.congestion_controller(), + helper.bitrate_allocator(), helper.event_log()); helper.SetupMockForGetStats(); EXPECT_FALSE(send_stream.GetStats().typing_noise_detected); @@ -379,8 +382,8 @@ TEST(AudioSendStreamTest, SendCodecAppliesConfigParams) { EnableAudioNetworkAdaptor(*stream_config.audio_network_adaptor_config)); internal::AudioSendStream send_stream( stream_config, helper.audio_state(), helper.worker_queue(), - helper.congestion_controller(), helper.bitrate_allocator(), - helper.event_log()); + helper.packet_router(), helper.congestion_controller(), + helper.bitrate_allocator(), helper.event_log()); } // VAD is applied when codec is mono and the CNG frequency matches the codec @@ -396,8 +399,8 @@ TEST(AudioSendStreamTest, SendCodecCanApplyVad) { .WillOnce(Return(0)); internal::AudioSendStream send_stream( stream_config, helper.audio_state(), helper.worker_queue(), - helper.congestion_controller(), helper.bitrate_allocator(), - helper.event_log()); + helper.packet_router(), helper.congestion_controller(), + helper.bitrate_allocator(), helper.event_log()); } } // namespace test diff --git a/webrtc/call/call.cc b/webrtc/call/call.cc index d2bc153d32..2af817149e 100644 --- a/webrtc/call/call.cc +++ b/webrtc/call/call.cc @@ -213,6 +213,9 @@ class Call : public webrtc::Call, std::map network_routes_; VieRemb remb_; + PacketRouter packet_router_; + // TODO(nisse): Could be a direct member, except for constness + // issues with GetRemoteBitrateEstimator (and maybe others). const std::unique_ptr congestion_controller_; const std::unique_ptr video_send_delay_stats_; const int64_t start_ms_; @@ -267,8 +270,11 @@ Call::Call(const Call::Config& config) estimated_send_bitrate_kbps_counter_(clock_, nullptr, true), pacer_bitrate_kbps_counter_(clock_, nullptr, true), remb_(clock_), - congestion_controller_( - new CongestionController(clock_, this, &remb_, event_log_)), + congestion_controller_(new CongestionController(clock_, + this, + &remb_, + event_log_, + &packet_router_)), video_send_delay_stats_(new SendDelayStats(clock_)), start_ms_(clock_->TimeInMilliseconds()), worker_queue_("call_worker_queue") { @@ -412,8 +418,8 @@ webrtc::AudioSendStream* Call::CreateAudioSendStream( RTC_DCHECK(configuration_thread_checker_.CalledOnValidThread()); event_log_->LogAudioSendStreamConfig(config); AudioSendStream* send_stream = new AudioSendStream( - config, config_.audio_state, &worker_queue_, congestion_controller_.get(), - bitrate_allocator_.get(), event_log_); + config, config_.audio_state, &worker_queue_, &packet_router_, + congestion_controller_.get(), bitrate_allocator_.get(), event_log_); { WriteLockScoped write_lock(*send_crit_); RTC_DCHECK(audio_send_ssrcs_.find(config.rtp.ssrc) == @@ -466,7 +472,10 @@ webrtc::AudioReceiveStream* Call::CreateAudioReceiveStream( RTC_DCHECK(configuration_thread_checker_.CalledOnValidThread()); event_log_->LogAudioReceiveStreamConfig(config); AudioReceiveStream* receive_stream = new AudioReceiveStream( - congestion_controller_.get(), config, config_.audio_state, event_log_); + &packet_router_, + // TODO(nisse): Used only when UseSendSideBwe(config) is true. + congestion_controller_->GetRemoteBitrateEstimator(true), config, + config_.audio_state, event_log_); { WriteLockScoped write_lock(*receive_crit_); RTC_DCHECK(audio_receive_ssrcs_.find(config.rtp.remote_ssrc) == @@ -525,9 +534,10 @@ webrtc::VideoSendStream* Call::CreateVideoSendStream( std::vector ssrcs = config.rtp.ssrcs; VideoSendStream* send_stream = new VideoSendStream( num_cpu_cores_, module_process_thread_.get(), &worker_queue_, - call_stats_.get(), congestion_controller_.get(), bitrate_allocator_.get(), - video_send_delay_stats_.get(), &remb_, event_log_, std::move(config), - std::move(encoder_config), suspended_video_send_ssrcs_); + call_stats_.get(), congestion_controller_.get(), &packet_router_, + bitrate_allocator_.get(), video_send_delay_stats_.get(), &remb_, + event_log_, std::move(config), std::move(encoder_config), + suspended_video_send_ssrcs_); { WriteLockScoped write_lock(*send_crit_); @@ -583,8 +593,9 @@ webrtc::VideoReceiveStream* Call::CreateVideoReceiveStream( TRACE_EVENT0("webrtc", "Call::CreateVideoReceiveStream"); RTC_DCHECK(configuration_thread_checker_.CalledOnValidThread()); VideoReceiveStream* receive_stream = new VideoReceiveStream( - num_cpu_cores_, congestion_controller_.get(), std::move(configuration), - voice_engine(), module_process_thread_.get(), call_stats_.get(), &remb_); + num_cpu_cores_, congestion_controller_.get(), &packet_router_, + std::move(configuration), voice_engine(), module_process_thread_.get(), + call_stats_.get(), &remb_); const webrtc::VideoReceiveStream::Config& config = receive_stream->config(); { diff --git a/webrtc/modules/congestion_controller/congestion_controller.cc b/webrtc/modules/congestion_controller/congestion_controller.cc index 5a11f3242f..d2b45b7889 100644 --- a/webrtc/modules/congestion_controller/congestion_controller.cc +++ b/webrtc/modules/congestion_controller/congestion_controller.cc @@ -148,31 +148,30 @@ class WrappingBitrateEstimator : public RemoteBitrateEstimator { } // namespace +CongestionController::CongestionController( + Clock* clock, + Observer* observer, + RemoteBitrateObserver* remote_bitrate_observer, + RtcEventLog* event_log, + PacketRouter* packet_router) + : CongestionController( + clock, + observer, + remote_bitrate_observer, + event_log, + packet_router, + std::unique_ptr(new PacedSender(clock, packet_router))) { +} + CongestionController::CongestionController( Clock* clock, Observer* observer, RemoteBitrateObserver* remote_bitrate_observer, RtcEventLog* event_log) - : clock_(clock), - observer_(observer), - packet_router_(new PacketRouter()), - pacer_(new PacedSender(clock_, packet_router_.get())), - remote_bitrate_estimator_( - new WrappingBitrateEstimator(remote_bitrate_observer, clock_)), - bitrate_controller_( - BitrateController::CreateBitrateController(clock_, event_log)), - probe_controller_(new ProbeController(pacer_.get(), clock_)), - retransmission_rate_limiter_( - new RateLimiter(clock, kRetransmitWindowSizeMs)), - remote_estimator_proxy_(clock_, packet_router_.get()), - transport_feedback_adapter_(clock_, bitrate_controller_.get()), - min_bitrate_bps_(congestion_controller::GetMinBitrateBps()), - max_bitrate_bps_(0), - last_reported_bitrate_bps_(0), - last_reported_fraction_loss_(0), - last_reported_rtt_(0), - network_state_(kNetworkUp) { - Init(); + : CongestionController(clock, observer, remote_bitrate_observer, event_log, + new PacketRouter()) { + // Record ownership. + owned_packet_router_.reset(packet_router_); } CongestionController::CongestionController( @@ -180,11 +179,11 @@ CongestionController::CongestionController( Observer* observer, RemoteBitrateObserver* remote_bitrate_observer, RtcEventLog* event_log, - std::unique_ptr packet_router, + PacketRouter* packet_router, std::unique_ptr pacer) : clock_(clock), observer_(observer), - packet_router_(std::move(packet_router)), + packet_router_(packet_router), pacer_(std::move(pacer)), remote_bitrate_estimator_( new WrappingBitrateEstimator(remote_bitrate_observer, clock_)), @@ -195,7 +194,7 @@ CongestionController::CongestionController( probe_controller_(new ProbeController(pacer_.get(), clock_)), retransmission_rate_limiter_( new RateLimiter(clock, kRetransmitWindowSizeMs)), - remote_estimator_proxy_(clock_, packet_router_.get()), + remote_estimator_proxy_(clock_, packet_router_), transport_feedback_adapter_(clock_, bitrate_controller_.get()), min_bitrate_bps_(congestion_controller::GetMinBitrateBps()), max_bitrate_bps_(0), @@ -203,16 +202,12 @@ CongestionController::CongestionController( last_reported_fraction_loss_(0), last_reported_rtt_(0), network_state_(kNetworkUp) { - Init(); -} - -CongestionController::~CongestionController() {} - -void CongestionController::Init() { transport_feedback_adapter_.InitBwe(); transport_feedback_adapter_.SetMinBitrate(min_bitrate_bps_); } +CongestionController::~CongestionController() {} + void CongestionController::SetBweBitrates(int min_bitrate_bps, int start_bitrate_bps, int max_bitrate_bps) { diff --git a/webrtc/modules/congestion_controller/congestion_controller_unittest.cc b/webrtc/modules/congestion_controller/congestion_controller_unittest.cc index 42bb5267b2..9b12cf3f83 100644 --- a/webrtc/modules/congestion_controller/congestion_controller_unittest.cc +++ b/webrtc/modules/congestion_controller/congestion_controller_unittest.cc @@ -37,10 +37,9 @@ class CongestionControllerTest : public ::testing::Test { void SetUp() override { pacer_ = new NiceMock(); std::unique_ptr pacer(pacer_); // Passes ownership. - std::unique_ptr packet_router(new PacketRouter()); controller_.reset(new CongestionController( &clock_, &observer_, &remote_bitrate_observer_, &event_log_, - std::move(packet_router), std::move(pacer))); + &packet_router_, std::move(pacer))); bandwidth_observer_.reset( controller_->GetBitrateController()->CreateRtcpBandwidthObserver()); @@ -57,6 +56,7 @@ class CongestionControllerTest : public ::testing::Test { NiceMock remote_bitrate_observer_; NiceMock event_log_; std::unique_ptr bandwidth_observer_; + PacketRouter packet_router_; std::unique_ptr controller_; const uint32_t kInitialBitrateBps = 60000; }; diff --git a/webrtc/modules/congestion_controller/include/congestion_controller.h b/webrtc/modules/congestion_controller/include/congestion_controller.h index 4f9ce95e2c..ed37b9a4ce 100644 --- a/webrtc/modules/congestion_controller/include/congestion_controller.h +++ b/webrtc/modules/congestion_controller/include/congestion_controller.h @@ -53,6 +53,12 @@ class CongestionController : public CallStatsObserver, public Module { protected: virtual ~Observer() {} }; + CongestionController(Clock* clock, + Observer* observer, + RemoteBitrateObserver* remote_bitrate_observer, + RtcEventLog* event_log, + PacketRouter* packet_router); + // TODO(nisse): Deprecated. Will create and own a PacketRouter. CongestionController(Clock* clock, Observer* observer, RemoteBitrateObserver* remote_bitrate_observer, @@ -61,7 +67,7 @@ class CongestionController : public CallStatsObserver, public Module { Observer* observer, RemoteBitrateObserver* remote_bitrate_observer, RtcEventLog* event_log, - std::unique_ptr packet_router, + PacketRouter* packet_router, std::unique_ptr pacer); virtual ~CongestionController(); @@ -79,7 +85,8 @@ class CongestionController : public CallStatsObserver, public Module { bool send_side_bwe); virtual int64_t GetPacerQueuingDelayMs() const; virtual PacedSender* pacer() { return pacer_.get(); } - virtual PacketRouter* packet_router() { return packet_router_.get(); } + // TODO(nisse): Deprecated, but still used by downstream projects. + virtual PacketRouter* packet_router() { return packet_router_; } virtual TransportFeedbackObserver* GetTransportFeedbackObserver(); RateLimiter* GetRetransmissionRateLimiter(); void EnablePeriodicAlrProbing(bool enable); @@ -107,7 +114,6 @@ class CongestionController : public CallStatsObserver, public Module { void Process() override; private: - void Init(); void MaybeTriggerOnNetworkChanged(); bool IsSendQueueFull() const; @@ -117,7 +123,10 @@ class CongestionController : public CallStatsObserver, public Module { int64_t rtt); Clock* const clock_; Observer* const observer_; - const std::unique_ptr packet_router_; + // Used by the deprecated constructor, where caller doesn't provide + // the packet_router. + std::unique_ptr owned_packet_router_; + PacketRouter* const packet_router_; const std::unique_ptr pacer_; const std::unique_ptr remote_bitrate_estimator_; const std::unique_ptr bitrate_controller_; diff --git a/webrtc/modules/congestion_controller/include/mock/mock_congestion_controller.h b/webrtc/modules/congestion_controller/include/mock/mock_congestion_controller.h index 128d64ea98..593a36e7d8 100644 --- a/webrtc/modules/congestion_controller/include/mock/mock_congestion_controller.h +++ b/webrtc/modules/congestion_controller/include/mock/mock_congestion_controller.h @@ -32,11 +32,13 @@ class MockCongestionController : public CongestionController { MockCongestionController(Clock* clock, Observer* observer, RemoteBitrateObserver* remote_bitrate_observer, - RtcEventLog* event_log) + RtcEventLog* event_log, + PacketRouter* packet_router) : CongestionController(clock, observer, remote_bitrate_observer, - event_log) {} + event_log, + packet_router) {} MOCK_METHOD3(SetBweBitrates, void(int min_bitrate_bps, int start_bitrate_bps, @@ -47,7 +49,6 @@ class MockCongestionController : public CongestionController { RemoteBitrateEstimator*(bool send_side_bwe)); MOCK_CONST_METHOD0(GetPacerQueuingDelayMs, int64_t()); MOCK_METHOD0(pacer, PacedSender*()); - MOCK_METHOD0(packet_router, PacketRouter*()); MOCK_METHOD0(GetTransportFeedbackObserver, TransportFeedbackObserver*()); MOCK_METHOD3(UpdatePacerBitrate, void(int bitrate_kbps, diff --git a/webrtc/test/fuzzers/congestion_controller_feedback_fuzzer.cc b/webrtc/test/fuzzers/congestion_controller_feedback_fuzzer.cc index 496af90c69..42817fa40b 100644 --- a/webrtc/test/fuzzers/congestion_controller_feedback_fuzzer.cc +++ b/webrtc/test/fuzzers/congestion_controller_feedback_fuzzer.cc @@ -33,7 +33,9 @@ void FuzzOneInput(const uint8_t* data, size_t size) { SimulatedClock clock(data[i++]); NullBitrateObserver observer; RtcEventLogNullImpl event_log; - CongestionController cc(&clock, &observer, &observer, &event_log); + PacketRouter packet_router; + CongestionController cc(&clock, &observer, &observer, &event_log, + &packet_router); RemoteBitrateEstimator* rbe = cc.GetRemoteBitrateEstimator(true); RTPHeader header; header.ssrc = ByteReader::ReadBigEndian(&data[i]); diff --git a/webrtc/tools/event_log_visualizer/analyzer.cc b/webrtc/tools/event_log_visualizer/analyzer.cc index d9b772bcd6..f98c5443d7 100644 --- a/webrtc/tools/event_log_visualizer/analyzer.cc +++ b/webrtc/tools/event_log_visualizer/analyzer.cc @@ -938,7 +938,9 @@ void EventLogAnalyzer::CreateBweSimulationGraph(Plot* plot) { SimulatedClock clock(0); BitrateObserver observer; RtcEventLogNullImpl null_event_log; - CongestionController cc(&clock, &observer, &observer, &null_event_log); + PacketRouter packet_router; + CongestionController cc(&clock, &observer, &observer, &null_event_log, + &packet_router); // TODO(holmer): Log the call config and use that here instead. static const uint32_t kDefaultStartBitrateBps = 300000; cc.SetBweBitrates(0, kDefaultStartBitrateBps, -1); diff --git a/webrtc/video/video_receive_stream.cc b/webrtc/video/video_receive_stream.cc index 5fd79a7744..eb2f10eaf7 100644 --- a/webrtc/video/video_receive_stream.cc +++ b/webrtc/video/video_receive_stream.cc @@ -189,6 +189,7 @@ namespace internal { VideoReceiveStream::VideoReceiveStream( int num_cpu_cores, CongestionController* congestion_controller, + PacketRouter* packet_router, VideoReceiveStream::Config config, webrtc::VoiceEngine* voice_engine, ProcessThread* process_thread, @@ -212,7 +213,7 @@ VideoReceiveStream::VideoReceiveStream( &transport_adapter_, call_stats_->rtcp_rtt_stats(), congestion_controller_->pacer(), - congestion_controller_->packet_router(), + packet_router, remb, &config_, &stats_proxy_, diff --git a/webrtc/video/video_receive_stream.h b/webrtc/video/video_receive_stream.h index f1efc4b4b8..56685e20ee 100644 --- a/webrtc/video/video_receive_stream.h +++ b/webrtc/video/video_receive_stream.h @@ -50,6 +50,7 @@ class VideoReceiveStream : public webrtc::VideoReceiveStream, public: VideoReceiveStream(int num_cpu_cores, CongestionController* congestion_controller, + PacketRouter* packet_router, VideoReceiveStream::Config config, webrtc::VoiceEngine* voice_engine, ProcessThread* process_thread, diff --git a/webrtc/video/video_send_stream.cc b/webrtc/video/video_send_stream.cc index 04748f8d3f..4416fb99bb 100644 --- a/webrtc/video/video_send_stream.cc +++ b/webrtc/video/video_send_stream.cc @@ -295,6 +295,7 @@ class VideoSendStreamImpl : public webrtc::BitrateAllocatorObserver, rtc::TaskQueue* worker_queue, CallStats* call_stats, CongestionController* congestion_controller, + PacketRouter* packet_router, BitrateAllocator* bitrate_allocator, SendDelayStats* send_delay_stats, VieRemb* remb, @@ -374,6 +375,7 @@ class VideoSendStreamImpl : public webrtc::BitrateAllocatorObserver, CallStats* const call_stats_; CongestionController* const congestion_controller_; + PacketRouter* const packet_router_; BitrateAllocator* const bitrate_allocator_; VieRemb* const remb_; @@ -422,6 +424,7 @@ class VideoSendStream::ConstructionTask : public rtc::QueuedTask { ProcessThread* module_process_thread, CallStats* call_stats, CongestionController* congestion_controller, + PacketRouter* packet_router, BitrateAllocator* bitrate_allocator, SendDelayStats* send_delay_stats, VieRemb* remb, @@ -435,6 +438,7 @@ class VideoSendStream::ConstructionTask : public rtc::QueuedTask { vie_encoder_(vie_encoder), call_stats_(call_stats), congestion_controller_(congestion_controller), + packet_router_(packet_router), bitrate_allocator_(bitrate_allocator), send_delay_stats_(send_delay_stats), remb_(remb), @@ -449,9 +453,9 @@ class VideoSendStream::ConstructionTask : public rtc::QueuedTask { bool Run() override { send_stream_->reset(new VideoSendStreamImpl( stats_proxy_, rtc::TaskQueue::Current(), call_stats_, - congestion_controller_, bitrate_allocator_, send_delay_stats_, remb_, - vie_encoder_, event_log_, config_, initial_encoder_max_bitrate_, - std::move(suspended_ssrcs_))); + congestion_controller_, packet_router_, bitrate_allocator_, + send_delay_stats_, remb_, vie_encoder_, event_log_, config_, + initial_encoder_max_bitrate_, std::move(suspended_ssrcs_))); return true; } @@ -461,6 +465,7 @@ class VideoSendStream::ConstructionTask : public rtc::QueuedTask { ViEEncoder* const vie_encoder_; CallStats* const call_stats_; CongestionController* const congestion_controller_; + PacketRouter* const packet_router_; BitrateAllocator* const bitrate_allocator_; SendDelayStats* const send_delay_stats_; VieRemb* const remb_; @@ -575,6 +580,7 @@ VideoSendStream::VideoSendStream( rtc::TaskQueue* worker_queue, CallStats* call_stats, CongestionController* congestion_controller, + PacketRouter* packet_router, BitrateAllocator* bitrate_allocator, SendDelayStats* send_delay_stats, VieRemb* remb, @@ -594,7 +600,7 @@ VideoSendStream::VideoSendStream( worker_queue_->PostTask(std::unique_ptr(new ConstructionTask( &send_stream_, &thread_sync_event_, &stats_proxy_, vie_encoder_.get(), - module_process_thread, call_stats, congestion_controller, + module_process_thread, call_stats, congestion_controller, packet_router, bitrate_allocator, send_delay_stats, remb, event_log, &config_, encoder_config.max_bitrate_bps, suspended_ssrcs))); @@ -702,6 +708,7 @@ VideoSendStreamImpl::VideoSendStreamImpl( rtc::TaskQueue* worker_queue, CallStats* call_stats, CongestionController* congestion_controller, + PacketRouter* packet_router, BitrateAllocator* bitrate_allocator, SendDelayStats* send_delay_stats, VieRemb* remb, @@ -718,6 +725,7 @@ VideoSendStreamImpl::VideoSendStreamImpl( check_encoder_activity_task_(nullptr), call_stats_(call_stats), congestion_controller_(congestion_controller), + packet_router_(packet_router), bitrate_allocator_(bitrate_allocator), remb_(remb), flexfec_sender_(MaybeCreateFlexfecSender(*config_)), @@ -739,7 +747,7 @@ VideoSendStreamImpl::VideoSendStreamImpl( congestion_controller_->GetTransportFeedbackObserver(), call_stats_->rtcp_rtt_stats(), congestion_controller_->pacer(), - congestion_controller_->packet_router(), + packet_router_, flexfec_sender_.get(), stats_proxy_, send_delay_stats, @@ -767,7 +775,7 @@ VideoSendStreamImpl::VideoSendStreamImpl( // RTP/RTCP initialization. for (RtpRtcp* rtp_rtcp : rtp_rtcp_modules_) { - congestion_controller_->packet_router()->AddRtpModule(rtp_rtcp); + packet_router_->AddRtpModule(rtp_rtcp); } for (size_t i = 0; i < config_->rtp.extensions.size(); ++i) { @@ -848,7 +856,7 @@ VideoSendStreamImpl::~VideoSendStreamImpl() { remb_->RemoveRembSender(rtp_rtcp_modules_[0]); for (RtpRtcp* rtp_rtcp : rtp_rtcp_modules_) { - congestion_controller_->packet_router()->RemoveRtpModule(rtp_rtcp); + packet_router_->RemoveRtpModule(rtp_rtcp); delete rtp_rtcp; } } diff --git a/webrtc/video/video_send_stream.h b/webrtc/video/video_send_stream.h index 2845aedd0e..34b9d5c890 100644 --- a/webrtc/video/video_send_stream.h +++ b/webrtc/video/video_send_stream.h @@ -35,6 +35,7 @@ class BitrateAllocator; class CallStats; class CongestionController; class IvfFileWriter; +class PacketRouter; class ProcessThread; class RtpRtcp; class VieRemb; @@ -54,6 +55,7 @@ class VideoSendStream : public webrtc::VideoSendStream { rtc::TaskQueue* worker_queue, CallStats* call_stats, CongestionController* congestion_controller, + PacketRouter* packet_router, BitrateAllocator* bitrate_allocator, SendDelayStats* send_delay_stats, VieRemb* remb,