Define RtpTransportControllerSendInterface.

webrtc/call/call.cc

Index: webrtc/call/call.cc
diff --git a/webrtc/call/call.cc b/webrtc/call/call.cc
index e21b0762fe8429f21b87b38916c5c5b772ddb119..f92adb65013ecdd9ff22e1237e063bb9e98d6fad 100644
--- a/webrtc/call/call.cc
+++ b/webrtc/call/call.cc
@@ -32,6 +32,7 @@
 #include "webrtc/call/bitrate_allocator.h"
 #include "webrtc/call/call.h"
 #include "webrtc/call/flexfec_receive_stream_impl.h"
+#include "webrtc/call/rtp_transport_controller.h"
 #include "webrtc/config.h"
 #include "webrtc/logging/rtc_event_log/rtc_event_log.h"
 #include "webrtc/modules/bitrate_controller/include/bitrate_controller.h"
@@ -86,6 +87,39 @@ bool UseSendSideBwe(const FlexfecReceiveStream::Config& config) {
   return UseSendSideBwe(config.rtp_header_extensions, config.transport_cc);
 }
 
+class RtpTransportController : public RtpTransportControllerSendInterface {

[stefan-webrtc, 2017/02/17 11:59:28]

Maybe create a new file for this already?

[nisse-webrtc, 2017/02/17 12:35:14]

Could do that. Then I'd need to declare it (or at least some way to create it)
in the header file.

+ public:
+  RtpTransportController(Clock* clock, webrtc::RtcEventLog* event_log);
+  void InitCongestionControl(CongestionController::Observer* observer);
+  VieRemb* remb() override { return &remb_; }
+  PacketRouter* packet_router() override { return &packet_router_; }
+  CongestionController* congestion_controller() override {
+    return congestion_controller_.get();
+  }
+
+ private:
+  Clock* const clock_;
+  webrtc::RtcEventLog* const event_log_;
+  VieRemb remb_;
+  PacketRouter packet_router_;
+  // Construction delayed until InitCongestionControl, since the
+  // CongestionController wants its observer as a construction time
+  // argument, and setting it later seems no-trivial.

[stefan-webrtc, 2017/02/17 11:59:28]

Doesn't this just mean that we're creating the RtpTransportController in the
wrong place? If we create Call and/or the BandwidthAllocator before the
RtpTransportController this wouldn't be a problem, right? Seems to me that we
might want to enforce that call order.

[nisse-webrtc, 2017/02/17 12:35:14]

That would make sense for now, but longer term, I think one should be able to
create the transport and pass it to the Call constructor. And then one would
need to register the CongestionController's observer later, in some way.

+  std::unique_ptr<CongestionController> congestion_controller_;
+};
+
+RtpTransportController::RtpTransportController(Clock* clock,
+                                               webrtc::RtcEventLog* event_log)
+    : clock_(clock), event_log_(event_log), remb_(clock) {}
+
+void RtpTransportController::InitCongestionControl(
+    CongestionController::Observer* observer) {
+  // Must be called only once.
+  RTC_CHECK(!congestion_controller_);
+  congestion_controller_.reset(new CongestionController(
+      clock_, observer, &remb_, event_log_, &packet_router_));
+}
+
 }  // namespace
 
 namespace internal {
@@ -96,7 +130,8 @@ class Call : public webrtc::Call,
              public CongestionController::Observer,
              public BitrateAllocator::LimitObserver {
  public:
-  explicit Call(const Call::Config& config);
+  Call(const Call::Config& config,
+       std::unique_ptr<RtpTransportController> transport);
   virtual ~Call();
 
   // Implements webrtc::Call.
@@ -270,11 +305,7 @@ class Call : public webrtc::Call,
 
   std::map<std::string, rtc::NetworkRoute> 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<CongestionController> congestion_controller_;
+  std::unique_ptr<RtpTransportController> transport_;
   const std::unique_ptr<SendDelayStats> video_send_delay_stats_;
   const int64_t start_ms_;
   // TODO(perkj): |worker_queue_| is supposed to replace
@@ -300,12 +331,16 @@ std::string Call::Stats::ToString(int64_t time_ms) const {
 }
 
 Call* Call::Create(const Call::Config& config) {
-  return new internal::Call(config);
+  return new internal::Call(
+      config,
+      std::unique_ptr<RtpTransportController>(new RtpTransportController(
+          Clock::GetRealTimeClock(), config.event_log)));
 }
 
 namespace internal {
 
-Call::Call(const Call::Config& config)
+Call::Call(const Call::Config& config,
+           std::unique_ptr<RtpTransportController> transport)
     : clock_(Clock::GetRealTimeClock()),
       num_cpu_cores_(CpuInfo::DetectNumberOfCores()),
       module_process_thread_(ProcessThread::Create("ModuleProcessThread")),
@@ -327,12 +362,7 @@ Call::Call(const Call::Config& config)
       configured_max_padding_bitrate_bps_(0),
       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_,
-                                                      &packet_router_)),
+      transport_(std::move(transport)),
       video_send_delay_stats_(new SendDelayStats(clock_)),
       start_ms_(clock_->TimeInMilliseconds()),
       worker_queue_("call_worker_queue") {
@@ -346,25 +376,25 @@ Call::Call(const Call::Config& config)
                   config.bitrate_config.start_bitrate_bps);
   }
   Trace::CreateTrace();
-  call_stats_->RegisterStatsObserver(congestion_controller_.get());
-
-  congestion_controller_->SignalNetworkState(kNetworkDown);
-  congestion_controller_->SetBweBitrates(
+  transport_->InitCongestionControl(this);
+  transport_->congestion_controller()->SignalNetworkState(kNetworkDown);
+  transport_->congestion_controller()->SetBweBitrates(
       config_.bitrate_config.min_bitrate_bps,
       config_.bitrate_config.start_bitrate_bps,
       config_.bitrate_config.max_bitrate_bps);
+  call_stats_->RegisterStatsObserver(transport_->congestion_controller());
 
   module_process_thread_->Start();
   module_process_thread_->RegisterModule(call_stats_.get());
-  module_process_thread_->RegisterModule(congestion_controller_.get());
-  pacer_thread_->RegisterModule(congestion_controller_->pacer());
+  module_process_thread_->RegisterModule(transport_->congestion_controller());
+  pacer_thread_->RegisterModule(transport_->congestion_controller()->pacer());
   pacer_thread_->RegisterModule(
-      congestion_controller_->GetRemoteBitrateEstimator(true));
+      transport_->congestion_controller()->GetRemoteBitrateEstimator(true));
   pacer_thread_->Start();
 }
 
 Call::~Call() {
-  RTC_DCHECK(!remb_.InUse());
+  RTC_DCHECK(!transport_->remb()->InUse());
   RTC_DCHECK(configuration_thread_checker_.CalledOnValidThread());
 
   RTC_CHECK(audio_send_ssrcs_.empty());
@@ -375,13 +405,13 @@ Call::~Call() {
   RTC_CHECK(video_receive_streams_.empty());
 
   pacer_thread_->Stop();
-  pacer_thread_->DeRegisterModule(congestion_controller_->pacer());
+  pacer_thread_->DeRegisterModule(transport_->congestion_controller()->pacer());
   pacer_thread_->DeRegisterModule(
-      congestion_controller_->GetRemoteBitrateEstimator(true));
-  module_process_thread_->DeRegisterModule(congestion_controller_.get());
+      transport_->congestion_controller()->GetRemoteBitrateEstimator(true));
+  module_process_thread_->DeRegisterModule(transport_->congestion_controller());
   module_process_thread_->DeRegisterModule(call_stats_.get());
   module_process_thread_->Stop();
-  call_stats_->DeregisterStatsObserver(congestion_controller_.get());
+  call_stats_->DeregisterStatsObserver(transport_->congestion_controller());
 
   // Only update histograms after process threads have been shut down, so that
   // they won't try to concurrently update stats.
@@ -499,9 +529,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_, &packet_router_,
-      congestion_controller_.get(), bitrate_allocator_.get(), event_log_,
-      call_stats_->rtcp_rtt_stats());
+      config, config_.audio_state, &worker_queue_, transport_.get(),
+      bitrate_allocator_.get(), event_log_, call_stats_->rtcp_rtt_stats());
   {
     WriteLockScoped write_lock(*send_crit_);
     RTC_DCHECK(audio_send_ssrcs_.find(config.rtp.ssrc) ==
@@ -554,8 +583,7 @@ webrtc::AudioReceiveStream* Call::CreateAudioReceiveStream(
   RTC_DCHECK(configuration_thread_checker_.CalledOnValidThread());
   event_log_->LogAudioReceiveStreamConfig(config);
   AudioReceiveStream* receive_stream = new AudioReceiveStream(
-      &packet_router_, config,
-      config_.audio_state, event_log_);
+      transport_->packet_router(), config, config_.audio_state, event_log_);
   {
     WriteLockScoped write_lock(*receive_crit_);
     RTC_DCHECK(audio_receive_ssrcs_.find(config.rtp.remote_ssrc) ==
@@ -589,7 +617,8 @@ void Call::DestroyAudioReceiveStream(
     WriteLockScoped write_lock(*receive_crit_);
     const AudioReceiveStream::Config& config = audio_receive_stream->config();
     uint32_t ssrc = config.rtp.remote_ssrc;
-    congestion_controller_->GetRemoteBitrateEstimator(UseSendSideBwe(config))
+    transport_->congestion_controller()
+        ->GetRemoteBitrateEstimator(UseSendSideBwe(config))
         ->RemoveStream(ssrc);
     size_t num_deleted = audio_receive_ssrcs_.erase(ssrc);
     RTC_DCHECK(num_deleted == 1);
@@ -621,10 +650,9 @@ webrtc::VideoSendStream* Call::CreateVideoSendStream(
   std::vector<uint32_t> ssrcs = config.rtp.ssrcs;
   VideoSendStream* send_stream = new VideoSendStream(
       num_cpu_cores_, module_process_thread_.get(), &worker_queue_,
-      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_);
+      call_stats_.get(), transport_.get(), bitrate_allocator_.get(),
+      video_send_delay_stats_.get(), event_log_, std::move(config),
+      std::move(encoder_config), suspended_video_send_ssrcs_);
 
   {
     WriteLockScoped write_lock(*send_crit_);
@@ -688,9 +716,9 @@ webrtc::VideoReceiveStream* Call::CreateVideoReceiveStream(
         flexfec_receive_ssrcs_media_.end();
   }
   VideoReceiveStream* receive_stream = new VideoReceiveStream(
-      num_cpu_cores_, protected_by_flexfec,
-      &packet_router_, std::move(configuration), module_process_thread_.get(),
-      call_stats_.get(), &remb_);
+      num_cpu_cores_, protected_by_flexfec, transport_->packet_router(),
+      std::move(configuration), module_process_thread_.get(), call_stats_.get(),
+      transport_->remb());
 
   const webrtc::VideoReceiveStream::Config& config = receive_stream->config();
   ReceiveRtpConfig receive_config(config.rtp.extensions,
@@ -746,7 +774,8 @@ void Call::DestroyVideoReceiveStream(
   }
   const VideoReceiveStream::Config& config = receive_stream_impl->config();
 
-  congestion_controller_->GetRemoteBitrateEstimator(UseSendSideBwe(config))
+  transport_->congestion_controller()
+      ->GetRemoteBitrateEstimator(UseSendSideBwe(config))

[stefan-webrtc, 2017/02/17 11:59:28]

What would it take to get rid of GetRemoteBitrateEstimator? Would it be enough
to expose methods on CongestionController, such as, RemoveStream(uint32_t id)
and LatestEstimate() (which probably should be called
GetEstimatedRecvBandwidth() or something)?

I think that would be a nice cleanup.

[nisse-webrtc, 2017/02/17 12:35:14]

I agree this could use some cleanup, but that's for another cl.

It's also not clear to me which methods should be advertised by
CongestionController, and which should be attached to the
RtpTransportController.

       ->RemoveStream(config.rtp.remote_ssrc);
 
   UpdateAggregateNetworkState();
@@ -822,7 +851,8 @@ void Call::DestroyFlexfecReceiveStream(FlexfecReceiveStream* receive_stream) {
         ++media_it;
     }
 
-    congestion_controller_->GetRemoteBitrateEstimator(UseSendSideBwe(config))
+    transport_->congestion_controller()
+        ->GetRemoteBitrateEstimator(UseSendSideBwe(config))
         ->RemoveStream(ssrc);
 
     flexfec_receive_streams_.erase(receive_stream_impl);
@@ -838,15 +868,18 @@ Call::Stats Call::GetStats() const {
   Stats stats;
   // Fetch available send/receive bitrates.
   uint32_t send_bandwidth = 0;
-  congestion_controller_->GetBitrateController()->AvailableBandwidth(
-      &send_bandwidth);
+  transport_->congestion_controller()
+      ->GetBitrateController()
+      ->AvailableBandwidth(&send_bandwidth);
   std::vector<unsigned int> ssrcs;
   uint32_t recv_bandwidth = 0;
-  congestion_controller_->GetRemoteBitrateEstimator(false)->LatestEstimate(
-      &ssrcs, &recv_bandwidth);
+  transport_->congestion_controller()
+      ->GetRemoteBitrateEstimator(false)
+      ->LatestEstimate(&ssrcs, &recv_bandwidth);
   stats.send_bandwidth_bps = send_bandwidth;
   stats.recv_bandwidth_bps = recv_bandwidth;
-  stats.pacer_delay_ms = congestion_controller_->GetPacerQueuingDelayMs();
+  stats.pacer_delay_ms =
+      transport_->congestion_controller()->GetPacerQueuingDelayMs();
   stats.rtt_ms = call_stats_->rtcp_rtt_stats()->LastProcessedRtt();
   {
     rtc::CritScope cs(&bitrate_crit_);
@@ -879,9 +912,9 @@ void Call::SetBitrateConfig(
     config_.bitrate_config.start_bitrate_bps = bitrate_config.start_bitrate_bps;
   config_.bitrate_config.max_bitrate_bps = bitrate_config.max_bitrate_bps;
   RTC_DCHECK_NE(bitrate_config.start_bitrate_bps, 0);
-  congestion_controller_->SetBweBitrates(bitrate_config.min_bitrate_bps,
-                                         bitrate_config.start_bitrate_bps,
-                                         bitrate_config.max_bitrate_bps);
+  transport_->congestion_controller()->SetBweBitrates(
+      bitrate_config.min_bitrate_bps, bitrate_config.start_bitrate_bps,
+      bitrate_config.max_bitrate_bps);
 }
 
 void Call::SignalChannelNetworkState(MediaType media, NetworkState state) {
@@ -976,7 +1009,7 @@ void Call::OnNetworkRouteChanged(const std::string& transport_name,
                  << " bps,  max: " << config_.bitrate_config.start_bitrate_bps
                  << " bps.";
     RTC_DCHECK_GT(config_.bitrate_config.start_bitrate_bps, 0);
-    congestion_controller_->ResetBweAndBitrates(
+    transport_->congestion_controller()->ResetBweAndBitrates(
         config_.bitrate_config.start_bitrate_bps,
         config_.bitrate_config.min_bitrate_bps,
         config_.bitrate_config.max_bitrate_bps);
@@ -1012,7 +1045,7 @@ void Call::UpdateAggregateNetworkState() {
   LOG(LS_INFO) << "UpdateAggregateNetworkState: aggregate_state="
                << (aggregate_state == kNetworkUp ? "up" : "down");
 
-  congestion_controller_->SignalNetworkState(aggregate_state);
+  transport_->congestion_controller()->SignalNetworkState(aggregate_state);
 }
 
 void Call::OnSentPacket(const rtc::SentPacket& sent_packet) {
@@ -1020,7 +1053,7 @@ void Call::OnSentPacket(const rtc::SentPacket& sent_packet) {
     first_packet_sent_ms_ = clock_->TimeInMilliseconds();
   video_send_delay_stats_->OnSentPacket(sent_packet.packet_id,
                                         clock_->TimeInMilliseconds());
-  congestion_controller_->OnSentPacket(sent_packet);
+  transport_->congestion_controller()->OnSentPacket(sent_packet);
 }
 
 void Call::OnNetworkChanged(uint32_t target_bitrate_bps,
@@ -1071,7 +1104,7 @@ void Call::OnNetworkChanged(uint32_t target_bitrate_bps,
 
 void Call::OnAllocationLimitsChanged(uint32_t min_send_bitrate_bps,
                                      uint32_t max_padding_bitrate_bps) {
-  congestion_controller_->SetAllocatedSendBitrateLimits(
+  transport_->congestion_controller()->SetAllocatedSendBitrateLimits(
       min_send_bitrate_bps, max_padding_bitrate_bps);
   rtc::CritScope lock(&bitrate_crit_);
   min_allocated_send_bitrate_bps_ = min_send_bitrate_bps;
@@ -1296,11 +1329,12 @@ void Call::NotifyBweOfReceivedPacket(const RtpPacketReceived& packet,
   // should be fixed to use the same MediaType as the production code.
   if (media_type != MediaType::AUDIO ||
       (use_send_side_bwe && header.extension.hasTransportSequenceNumber)) {
-    congestion_controller_->OnReceivedPacket(
+    transport_->congestion_controller()->OnReceivedPacket(
         packet.arrival_time_ms(), packet.payload_size() + packet.padding_size(),
         header);
   }
 }
 
 }  // namespace internal
+
 }  // namespace webrtc