Removing AvailableBandwidth method on transport controller.

Removing the Synchronous call AvailableBandwidth from the
RtpTransportControllerSend interface. The bandwidth estimate is
provided trough a new interface that communicates with a struct
making it easier to add parameters in the future.

This prepares for removing locking behavior in
SendSideCongestionController that exists just to support this feature.

To keep backwards compatibility with the old
SendSideCongestionController, the struct TargetTransferRate
is constructed in RtpTransportControllerSend. This step can be
removed in the future when the old SendSideCongestionController
 is deprecated.

Bug: webrtc:8415
Change-Id: I06f64a89848157de412901c989650d1ecf35246b
Reviewed-on: https://webrtc-review.googlesource.com/60800
Commit-Queue: Sebastian Jansson <srte@webrtc.org>
Reviewed-by: Stefan Holmer <stefan@webrtc.org>
Reviewed-by: Niels Moller <nisse@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#22387}

call/BUILD.gn

@@ -96,6 +96,7 @@ rtc_source_set("rtp_sender") {
     ":rtp_interfaces",
     "..:webrtc_common",
     "../modules/congestion_controller",
+    "../modules/congestion_controller/network_control",
     "../modules/pacing",
     "../modules/utility",
     "../rtc_base:rtc_base",
@@ -168,6 +169,7 @@ rtc_static_library("call") {
     "../logging:rtc_stream_config",
     "../modules/bitrate_controller",
     "../modules/congestion_controller",
+    "../modules/congestion_controller/network_control",
     "../modules/pacing",
     "../modules/rtp_rtcp",
     "../modules/rtp_rtcp:rtp_rtcp_format",
@@ -177,6 +179,7 @@ rtc_static_library("call") {
     "../rtc_base:rate_limiter",
     "../rtc_base:rtc_base_approved",
     "../rtc_base:rtc_task_queue",
+    "../rtc_base:safe_minmax",
     "../rtc_base:sequenced_task_checker",
     "../system_wrappers",
     "../system_wrappers:metrics_api",

call/call.cc

@@ -35,8 +35,8 @@
 #include "logging/rtc_event_log/rtc_event_log.h"
 #include "logging/rtc_event_log/rtc_stream_config.h"
 #include "modules/bitrate_controller/include/bitrate_controller.h"
-#include "modules/congestion_controller/include/network_changed_observer.h"
 #include "modules/congestion_controller/include/receive_side_congestion_controller.h"
+#include "modules/congestion_controller/network_control/include/network_control.h"
 #include "modules/rtp_rtcp/include/flexfec_receiver.h"
 #include "modules/rtp_rtcp/include/rtp_header_extension_map.h"
 #include "modules/rtp_rtcp/include/rtp_header_parser.h"
@@ -49,6 +49,7 @@
 #include "rtc_base/constructormagic.h"
 #include "rtc_base/location.h"
 #include "rtc_base/logging.h"
+#include "rtc_base/numerics/safe_minmax.h"
 #include "rtc_base/ptr_util.h"
 #include "rtc_base/rate_limiter.h"
 #include "rtc_base/sequenced_task_checker.h"
@@ -166,7 +167,7 @@ namespace internal {
 class Call : public webrtc::Call,
              public PacketReceiver,
              public RecoveredPacketReceiver,
-             public NetworkChangedObserver,
+             public TargetTransferRateObserver,
              public BitrateAllocator::LimitObserver {
  public:
   Call(const Call::Config& config,
@@ -227,11 +228,8 @@ class Call : public webrtc::Call,
 
   void OnSentPacket(const rtc::SentPacket& sent_packet) override;
 
-  // Implements BitrateObserver.
-  void OnNetworkChanged(uint32_t bitrate_bps,
-                        uint8_t fraction_loss,
-                        int64_t rtt_ms,
-                        int64_t probing_interval_ms) override;
+  // Implements TargetTransferRateObserver,
+  void OnTargetTransferRate(TargetTransferRate msg) override;
 
   // Implements BitrateAllocator::LimitObserver.
   void OnAllocationLimitsChanged(uint32_t min_send_bitrate_bps,
@@ -344,6 +342,8 @@ class Call : public webrtc::Call,
   rtc::Optional<int64_t> last_received_rtp_video_ms_;
   TimeInterval sent_rtp_audio_timer_ms_;
 
+  rtc::CriticalSection last_bandwidth_bps_crit_;
+  uint32_t last_bandwidth_bps_ RTC_GUARDED_BY(&last_bandwidth_bps_crit_);
   // TODO(holmer): Remove this lock once BitrateController no longer calls
   // OnNetworkChanged from multiple threads.
   rtc::CriticalSection bitrate_crit_;
@@ -423,6 +423,7 @@ Call::Call(const Call::Config& config,
       received_audio_bytes_per_second_counter_(clock_, nullptr, true),
       received_video_bytes_per_second_counter_(clock_, nullptr, true),
       received_rtcp_bytes_per_second_counter_(clock_, nullptr, true),
+      last_bandwidth_bps_(0),
       min_allocated_send_bitrate_bps_(0),
       configured_max_padding_bitrate_bps_(0),
       estimated_send_bitrate_kbps_counter_(clock_, nullptr, true),
@@ -433,7 +434,7 @@ Call::Call(const Call::Config& config,
       start_ms_(clock_->TimeInMilliseconds()),
       worker_queue_("call_worker_queue") {
   RTC_DCHECK(config.event_log != nullptr);
-  transport_send->RegisterNetworkObserver(this);
+  transport_send->RegisterTargetTransferRateObserver(this);
   transport_send_ = std::move(transport_send);
 
   call_stats_->RegisterStatsObserver(&receive_side_cc_);
@@ -904,13 +905,15 @@ Call::Stats Call::GetStats() const {
   // RTC_DCHECK_CALLED_SEQUENTIALLY(&configuration_sequence_checker_);
   Stats stats;
   // Fetch available send/receive bitrates.
-  uint32_t send_bandwidth = 0;
-  transport_send_->AvailableBandwidth(&send_bandwidth);
   std::vector<unsigned int> ssrcs;
   uint32_t recv_bandwidth = 0;
   receive_side_cc_.GetRemoteBitrateEstimator(false)->LatestEstimate(
       &ssrcs, &recv_bandwidth);
-  stats.send_bandwidth_bps = send_bandwidth;
+
+  {
+    rtc::CritScope cs(&last_bandwidth_bps_crit_);
+    stats.send_bandwidth_bps = last_bandwidth_bps_;
+  }
   stats.recv_bandwidth_bps = recv_bandwidth;
   // TODO(srte): It is unclear if we only want to report queues if network is
   // available.
@@ -1045,26 +1048,26 @@ void Call::OnSentPacket(const rtc::SentPacket& sent_packet) {
   transport_send_->OnSentPacket(sent_packet);
 }
 
-void Call::OnNetworkChanged(uint32_t target_bitrate_bps,
-                            uint8_t fraction_loss,
-                            int64_t rtt_ms,
-                            int64_t probing_interval_ms) {
+void Call::OnTargetTransferRate(TargetTransferRate msg) {
   // TODO(perkj): Consider making sure CongestionController operates on
   // |worker_queue_|.
   if (!worker_queue_.IsCurrent()) {
-    worker_queue_.PostTask(
-        [this, target_bitrate_bps, fraction_loss, rtt_ms, probing_interval_ms] {
-          OnNetworkChanged(target_bitrate_bps, fraction_loss, rtt_ms,
-                           probing_interval_ms);
-        });
+    worker_queue_.PostTask([this, msg] { OnTargetTransferRate(msg); });
     return;
   }
   RTC_DCHECK_RUN_ON(&worker_queue_);
-  // TODO(srte): communicate bandwidth in the OnNetworkChanged event, or
-  // evaluate the feasability of using target bitrate _bps instead.
-  uint32_t bandwidth_bps;
-  if (transport_send_->AvailableBandwidth(&bandwidth_bps))
-    retransmission_rate_limiter_.SetMaxRate(bandwidth_bps);
+  uint32_t target_bitrate_bps = msg.target_rate.bps();
+  int loss_ratio_255 = msg.network_estimate.loss_rate_ratio * 255;
+  uint8_t fraction_loss =
+      rtc::dchecked_cast<uint8_t>(rtc::SafeClamp(loss_ratio_255, 0, 255));
+  int64_t rtt_ms = msg.network_estimate.round_trip_time.ms();
+  int64_t probing_interval_ms = msg.network_estimate.bwe_period.ms();
+  uint32_t bandwidth_bps = msg.network_estimate.bandwidth.bps();
+  {
+    rtc::CritScope cs(&last_bandwidth_bps_crit_);
+    last_bandwidth_bps_ = bandwidth_bps;
+  }
+  retransmission_rate_limiter_.SetMaxRate(bandwidth_bps);
   // For controlling the rate of feedback messages.
   receive_side_cc_.OnBitrateChanged(target_bitrate_bps);
   bitrate_allocator_->OnNetworkChanged(target_bitrate_bps, fraction_loss,

call/rtp_transport_controller_send.cc

@@ -11,6 +11,7 @@
 
 #include "call/rtp_transport_controller_send.h"
 #include "modules/congestion_controller/include/send_side_congestion_controller.h"
+#include "modules/congestion_controller/network_control/include/network_control.h"
 #include "rtc_base/location.h"
 #include "rtc_base/logging.h"
 #include "rtc_base/ptr_util.h"
@@ -21,14 +22,16 @@ RtpTransportControllerSend::RtpTransportControllerSend(
     Clock* clock,
     webrtc::RtcEventLog* event_log,
     const BitrateConstraints& bitrate_config)
-    : pacer_(clock, &packet_router_, event_log),
+    : clock_(clock),
+      pacer_(clock, &packet_router_, event_log),
       send_side_cc_(
           rtc::MakeUnique<SendSideCongestionController>(clock,
                                                         nullptr /* observer */,
                                                         event_log,
                                                         &pacer_)),
       bitrate_configurator_(bitrate_config),
-      process_thread_(ProcessThread::Create("SendControllerThread")) {
+      process_thread_(ProcessThread::Create("SendControllerThread")),
+      observer_(nullptr) {
   send_side_cc_->SignalNetworkState(kNetworkDown);
   send_side_cc_->SetBweBitrates(bitrate_config.min_bitrate_bps,
                                 bitrate_config.start_bitrate_bps,
@@ -45,6 +48,28 @@ RtpTransportControllerSend::~RtpTransportControllerSend() {
   process_thread_->DeRegisterModule(&pacer_);
 }
 
+void RtpTransportControllerSend::OnNetworkChanged(uint32_t bitrate_bps,
+                                                  uint8_t fraction_loss,
+                                                  int64_t rtt_ms,
+                                                  int64_t probing_interval_ms) {
+  // TODO(srte): Skip this step when old SendSideCongestionController is
+  // deprecated.
+  TargetTransferRate msg;
+  msg.at_time = Timestamp::ms(clock_->TimeInMilliseconds());
+  msg.target_rate = DataRate::bps(bitrate_bps);
+  msg.network_estimate.at_time = msg.at_time;
+  msg.network_estimate.bwe_period = TimeDelta::ms(probing_interval_ms);
+  uint32_t bandwidth_bps;
+  if (send_side_cc_->AvailableBandwidth(&bandwidth_bps))
+    msg.network_estimate.bandwidth = DataRate::bps(bandwidth_bps);
+  msg.network_estimate.loss_rate_ratio = fraction_loss / 255.0;
+  msg.network_estimate.round_trip_time = TimeDelta::ms(rtt_ms);
+  rtc::CritScope cs(&observer_crit_);
+  // We wont register as observer until we have an observer.
+  RTC_DCHECK(observer_ != nullptr);
+  observer_->OnTargetTransferRate(msg);
+}
+
 PacketRouter* RtpTransportControllerSend::packet_router() {
   return &packet_router_;
 }
@@ -91,9 +116,15 @@ void RtpTransportControllerSend::DeRegisterPacketFeedbackObserver(
     PacketFeedbackObserver* observer) {
   send_side_cc_->DeRegisterPacketFeedbackObserver(observer);
 }
-void RtpTransportControllerSend::RegisterNetworkObserver(
-    NetworkChangedObserver* observer) {
-  send_side_cc_->RegisterNetworkObserver(observer);
+
+void RtpTransportControllerSend::RegisterTargetTransferRateObserver(
+    TargetTransferRateObserver* observer) {
+  {
+    rtc::CritScope cs(&observer_crit_);
+    RTC_DCHECK(observer_ == nullptr);
+    observer_ = observer;
+  }
+  send_side_cc_->RegisterNetworkObserver(this);
 }
 void RtpTransportControllerSend::OnNetworkRouteChanged(
     const std::string& transport_name,
@@ -141,9 +172,6 @@ void RtpTransportControllerSend::OnNetworkAvailability(bool network_available) {
 RtcpBandwidthObserver* RtpTransportControllerSend::GetBandwidthObserver() {
   return send_side_cc_->GetBandwidthObserver();
 }
-bool RtpTransportControllerSend::AvailableBandwidth(uint32_t* bandwidth) const {
-  return send_side_cc_->AvailableBandwidth(bandwidth);
-}
 int64_t RtpTransportControllerSend::GetPacerQueuingDelayMs() const {
   return pacer_.QueueInMs();
 }

call/rtp_transport_controller_send.h

@@ -31,12 +31,20 @@ class RtcEventLog;
 // TODO(nisse): When we get the underlying transports here, we should
 // have one object implementing RtpTransportControllerSendInterface
 // per transport, sharing the same congestion controller.
-class RtpTransportControllerSend : public RtpTransportControllerSendInterface {
+class RtpTransportControllerSend : public RtpTransportControllerSendInterface,
+                                   public NetworkChangedObserver {
  public:
   RtpTransportControllerSend(Clock* clock,
                              RtcEventLog* event_log,
                              const BitrateConstraints& bitrate_config);
   ~RtpTransportControllerSend() override;
+
+  // Implements NetworkChangedObserver interface.
+  void OnNetworkChanged(uint32_t bitrate_bps,
+                        uint8_t fraction_loss,
+                        int64_t rtt_ms,
+                        int64_t probing_interval_ms) override;
+
   // Implements RtpTransportControllerSendInterface
   PacketRouter* packet_router() override;
 
@@ -56,12 +64,12 @@ class RtpTransportControllerSend : public RtpTransportControllerSendInterface {
       PacketFeedbackObserver* observer) override;
   void DeRegisterPacketFeedbackObserver(
       PacketFeedbackObserver* observer) override;
-  void RegisterNetworkObserver(NetworkChangedObserver* observer) override;
+  void RegisterTargetTransferRateObserver(
+      TargetTransferRateObserver* observer) override;
   void OnNetworkRouteChanged(const std::string& transport_name,
                              const rtc::NetworkRoute& network_route) override;
   void OnNetworkAvailability(bool network_available) override;
   RtcpBandwidthObserver* GetBandwidthObserver() override;
-  bool AvailableBandwidth(uint32_t* bandwidth) const override;
   int64_t GetPacerQueuingDelayMs() const override;
   int64_t GetFirstPacketTimeMs() const override;
   void EnablePeriodicAlrProbing(bool enable) override;
@@ -72,6 +80,7 @@ class RtpTransportControllerSend : public RtpTransportControllerSendInterface {
       const BitrateConstraintsMask& preferences) override;
 
  private:
+  const Clock* const clock_;
   PacketRouter packet_router_;
   PacedSender pacer_;
   const std::unique_ptr<SendSideCongestionControllerInterface> send_side_cc_;
@@ -79,6 +88,8 @@ class RtpTransportControllerSend : public RtpTransportControllerSendInterface {
   RtpBitrateConfigurator bitrate_configurator_;
   std::map<std::string, rtc::NetworkRoute> network_routes_;
   const std::unique_ptr<ProcessThread> process_thread_;
+  rtc::CriticalSection observer_crit_;
+  TargetTransferRateObserver* observer_ RTC_GUARDED_BY(observer_crit_);
 
   RTC_DISALLOW_COPY_AND_ASSIGN(RtpTransportControllerSend);
 };

call/rtp_transport_controller_send_interface.h

@@ -25,7 +25,7 @@ struct NetworkRoute;
 namespace webrtc {
 
 class CallStatsObserver;
-class NetworkChangedObserver;
+class TargetTransferRateObserver;
 class Module;
 class PacedSender;
 class PacketFeedbackObserver;
@@ -90,13 +90,13 @@ class RtpTransportControllerSendInterface {
       PacketFeedbackObserver* observer) = 0;
   virtual void DeRegisterPacketFeedbackObserver(
       PacketFeedbackObserver* observer) = 0;
-  virtual void RegisterNetworkObserver(NetworkChangedObserver* observer) = 0;
+  virtual void RegisterTargetTransferRateObserver(
+      TargetTransferRateObserver* observer) = 0;
   virtual void OnNetworkRouteChanged(
       const std::string& transport_name,
       const rtc::NetworkRoute& network_route) = 0;
   virtual void OnNetworkAvailability(bool network_available) = 0;
   virtual RtcpBandwidthObserver* GetBandwidthObserver() = 0;
-  virtual bool AvailableBandwidth(uint32_t* bandwidth) const = 0;
   virtual int64_t GetPacerQueuingDelayMs() const = 0;
   virtual int64_t GetFirstPacketTimeMs() const = 0;
   virtual void EnablePeriodicAlrProbing(bool enable) = 0;

call/test/mock_rtp_transport_controller_send.h

@@ -39,7 +39,8 @@ class MockRtpTransportControllerSend
   MOCK_METHOD0(GetCallStatsObserver, CallStatsObserver*());
   MOCK_METHOD1(RegisterPacketFeedbackObserver, void(PacketFeedbackObserver*));
   MOCK_METHOD1(DeRegisterPacketFeedbackObserver, void(PacketFeedbackObserver*));
-  MOCK_METHOD1(RegisterNetworkObserver, void(NetworkChangedObserver*));
+  MOCK_METHOD1(RegisterTargetTransferRateObserver,
+               void(TargetTransferRateObserver*));
   MOCK_METHOD2(OnNetworkRouteChanged,
                void(const std::string&, const rtc::NetworkRoute&));
   MOCK_METHOD1(OnNetworkAvailability, void(bool));

modules/congestion_controller/include/network_changed_observer.h

@@ -15,11 +15,18 @@
 
 namespace webrtc {
 
+// Note: This interface will be deprecated in favor of the
+// TargetTransferRateObserver interface. The new interface provides more
+// information about the network connection and uses structs to make it easier
+// to add fields.
+
 // Observer class for bitrate changes announced due to change in bandwidth
 // estimate or due to that the send pacer is full. Fraction loss and rtt is
 // also part of this callback to allow the observer to optimize its settings
 // for different types of network environments. The bitrate does not include
 // packet headers and is measured in bits per second.
+// TODO(srte): Deprecate and remove this class when SendSideCongestionController
+// is no longer using this as part of our public API.
 class NetworkChangedObserver {
  public:
   virtual void OnNetworkChanged(uint32_t bitrate_bps,

modules/congestion_controller/network_control/include/network_control.h

@@ -18,10 +18,20 @@
 
 namespace webrtc {
 
+class TargetTransferRateObserver {
+ public:
+  // Called to indicate target transfer rate as well as giving information about
+  // the current estimate of network parameters.
+  virtual void OnTargetTransferRate(TargetTransferRate) = 0;
+
+ protected:
+  virtual ~TargetTransferRateObserver() = default;
+};
+
 // NetworkControllerObserver is an interface implemented by observers of network
 // controllers. It contains declarations of the possible configuration messages
 // that can be sent from a network controller implementation.
-class NetworkControllerObserver {
+class NetworkControllerObserver : public TargetTransferRateObserver {
  public:
   // Called when congestion window configutation is changed.
   virtual void OnCongestionWindow(CongestionWindow) = 0;
@@ -29,12 +39,6 @@ class NetworkControllerObserver {
   virtual void OnPacerConfig(PacerConfig) = 0;
   // Called to indicate that a new probe should be sent.
   virtual void OnProbeClusterConfig(ProbeClusterConfig) = 0;
-  // Called to indicate target transfer rate as well as giving information about
-  // the current estimate of network parameters.
-  virtual void OnTargetTransferRate(TargetTransferRate) = 0;
-
- protected:
-  virtual ~NetworkControllerObserver() = default;
 };
 
 // NetworkControllerInterface is implemented by network controllers. A network