This reland https://codereview.webrtc.org/1932683002/.

Remove ViEEncoder::SetNetworkStatus.

Original cl description:
This cl removed ViEEncoder::SetNetworkStatus. Instead the PacedSender will report that frames can not be sent when the network is down and the BitrateController will report an estimated available bandwidth of 0 bps.

Patchset #1 is a pure reland.
Patchset #2 change the bitrate allocator to always return an initial bitrate >0 regardless of the estimates. The observer will be notified though if the network is down.

BUG=webrtc:5687

Review-Url: https://codereview.webrtc.org/1972183004
Cr-Commit-Position: refs/heads/master@{#12743}

webrtc/call/bitrate_allocator.cc

@@ -28,6 +28,7 @@ BitrateAllocator::BitrateAllocator()
     : bitrate_observer_configs_(),
       enforce_min_bitrate_(true),
       last_bitrate_bps_(kDefaultBitrateBps),
+      last_non_zero_bitrate_bps_(kDefaultBitrateBps),
       last_fraction_loss_(0),
       last_rtt_(0) {}
 
@@ -36,11 +37,13 @@ uint32_t BitrateAllocator::OnNetworkChanged(uint32_t bitrate,
                                             int64_t rtt) {
   rtc::CritScope lock(&crit_sect_);
   last_bitrate_bps_ = bitrate;
+  last_non_zero_bitrate_bps_ =
+      bitrate > 0 ? bitrate : last_non_zero_bitrate_bps_;
   last_fraction_loss_ = fraction_loss;
   last_rtt_ = rtt;
 
   uint32_t allocated_bitrate_bps = 0;
-  ObserverAllocation allocation = AllocateBitrates();
+  ObserverAllocation allocation = AllocateBitrates(bitrate);
   for (const auto& kv : allocation) {
     kv.first->OnBitrateUpdated(kv.second, last_fraction_loss_, last_rtt_);
     allocated_bitrate_bps += kv.second;
@@ -73,13 +76,24 @@ int BitrateAllocator::AddObserver(BitrateAllocatorObserver* observer,
         observer, min_bitrate_bps, max_bitrate_bps, enforce_min_bitrate));
   }
 
-  ObserverAllocation allocation = AllocateBitrates();
   int new_observer_bitrate_bps = 0;
+  if (last_bitrate_bps_ > 0) {  // We have a bitrate to allocate.
+    ObserverAllocation allocation = AllocateBitrates(last_bitrate_bps_);
     for (auto& kv : allocation) {
+      // Update all observers with the new allocation.
       kv.first->OnBitrateUpdated(kv.second, last_fraction_loss_, last_rtt_);
       if (kv.first == observer)
         new_observer_bitrate_bps = kv.second;
     }
+  } else {
+    // Currently, an encoder is not allowed to produce frames.
+    // But we still have to return the initial config bitrate + let the
+    // observer know that it can not produce frames.
+    ObserverAllocation allocation =
+        AllocateBitrates(last_non_zero_bitrate_bps_);
+    observer->OnBitrateUpdated(0, last_fraction_loss_, last_rtt_);
+    new_observer_bitrate_bps = allocation[observer];
+  }
   return new_observer_bitrate_bps;
 }
 
@@ -106,20 +120,21 @@ BitrateAllocator::FindObserverConfig(
   return bitrate_observer_configs_.end();
 }
 
-BitrateAllocator::ObserverAllocation BitrateAllocator::AllocateBitrates() {
+BitrateAllocator::ObserverAllocation BitrateAllocator::AllocateBitrates(
+    uint32_t bitrate) {
   if (bitrate_observer_configs_.empty())
     return ObserverAllocation();
 
-  if (last_bitrate_bps_ == 0)
+  if (bitrate == 0)
     return ZeroRateAllocation();
 
   uint32_t sum_min_bitrates = 0;
   for (const auto& observer_config : bitrate_observer_configs_)
     sum_min_bitrates += observer_config.min_bitrate_bps;
-  if (last_bitrate_bps_ <= sum_min_bitrates)
+  if (bitrate <= sum_min_bitrates)
-    return LowRateAllocation(last_bitrate_bps_);
+    return LowRateAllocation(bitrate);
 
-  return NormalRateAllocation(last_bitrate_bps_, sum_min_bitrates);
+  return NormalRateAllocation(bitrate, sum_min_bitrates);
 }
 
 BitrateAllocator::ObserverAllocation BitrateAllocator::NormalRateAllocation(

webrtc/call/bitrate_allocator.h

@@ -56,7 +56,11 @@ class BitrateAllocator {
   // |enforce_min_bitrate| = 'true' will allocate at least |min_bitrate_bps| for
   //    this observer, even if the BWE is too low, 'false' will allocate 0 to
   //    the observer if BWE doesn't allow |min_bitrate_bps|.
-  // Returns bitrate allocated for |observer|.
+  // Returns initial bitrate allocated for |observer|.
+  // Note that |observer|->OnBitrateUpdated() will be called within the scope of
+  // this method with the current rtt, fraction_loss and available bitrate and
+  // that the bitrate in OnBitrateUpdated will be zero if the |observer| is
+  // currently not allowed to send data.
   int AddObserver(BitrateAllocatorObserver* observer,
                   uint32_t min_bitrate_bps,
                   uint32_t max_bitrate_bps,
@@ -96,7 +100,8 @@ class BitrateAllocator {
   typedef std::multimap<uint32_t, const ObserverConfig*> ObserverSortingMap;
   typedef std::map<BitrateAllocatorObserver*, int> ObserverAllocation;
 
-  ObserverAllocation AllocateBitrates() EXCLUSIVE_LOCKS_REQUIRED(crit_sect_);
+  ObserverAllocation AllocateBitrates(uint32_t bitrate)
+      EXCLUSIVE_LOCKS_REQUIRED(crit_sect_);
   ObserverAllocation NormalRateAllocation(uint32_t bitrate,
                                           uint32_t sum_min_bitrates)
       EXCLUSIVE_LOCKS_REQUIRED(crit_sect_);
@@ -110,6 +115,7 @@ class BitrateAllocator {
   ObserverConfigList bitrate_observer_configs_;
   bool enforce_min_bitrate_ GUARDED_BY(crit_sect_);
   uint32_t last_bitrate_bps_ GUARDED_BY(crit_sect_);
+  uint32_t last_non_zero_bitrate_bps_ GUARDED_BY(crit_sect_);
   uint8_t last_fraction_loss_ GUARDED_BY(crit_sect_);
   int64_t last_rtt_ GUARDED_BY(crit_sect_);
 };

webrtc/call/bitrate_allocator_unittest.cc

@@ -222,4 +222,32 @@ TEST_F(BitrateAllocatorTest, ThreeBitrateObserversLowRembEnforceMin) {
   allocator_->RemoveObserver(&bitrate_observer_2);
   allocator_->RemoveObserver(&bitrate_observer_3);
 }
+
+TEST_F(BitrateAllocatorTest, AddObserverWhileNetworkDown) {
+  TestBitrateObserver bitrate_observer_1;
+  int start_bitrate =
+      allocator_->AddObserver(&bitrate_observer_1, 50000, 400000, true);
+  EXPECT_EQ(300000, start_bitrate);
+
+  // Set network down, ie, no available bitrate.
+  allocator_->OnNetworkChanged(0, 0, 0);
+
+  EXPECT_EQ(0u, bitrate_observer_1.last_bitrate_);
+
+  TestBitrateObserver bitrate_observer_2;
+  start_bitrate =
+      allocator_->AddObserver(&bitrate_observer_2, 50000, 400000, true);
+
+  // Expect the start_bitrate to be set as if the network was still up but that
+  // the new observer have been notified that the network is down.
+  EXPECT_EQ(300000 / 2, start_bitrate);
+  EXPECT_EQ(0u, bitrate_observer_1.last_bitrate_);
+  EXPECT_EQ(0u, bitrate_observer_2.last_bitrate_);
+
+  // Set network back up.
+  allocator_->OnNetworkChanged(1500000, 0, 50);
+  EXPECT_EQ(750000u, bitrate_observer_1.last_bitrate_);
+  EXPECT_EQ(750000u, bitrate_observer_2.last_bitrate_);
+}
+
 }  // namespace webrtc

webrtc/modules/congestion_controller/congestion_controller.cc

@@ -142,8 +142,7 @@ CongestionController::CongestionController(
     : clock_(clock),
       observer_(nullptr),
       packet_router_(new PacketRouter()),
-      pacer_(new PacedSender(clock_,
+      pacer_(new PacedSender(clock_, packet_router_.get())),
-                             packet_router_.get())),
       remote_bitrate_estimator_(
           new WrappingBitrateEstimator(remote_bitrate_observer, clock_)),
       bitrate_controller_(
@@ -151,7 +150,10 @@ CongestionController::CongestionController(
       remote_estimator_proxy_(clock_, packet_router_.get()),
       transport_feedback_adapter_(bitrate_controller_.get(), clock_),
       min_bitrate_bps_(RemoteBitrateEstimator::kDefaultMinBitrateBps),
-      send_queue_is_full_(false) {
+      last_reported_bitrate_bps_(0),
+      last_reported_fraction_loss_(0),
+      last_reported_rtt_(0),
+      network_state_(kNetworkUp) {
   Init();
 }
 
@@ -162,15 +164,17 @@ CongestionController::CongestionController(
     : clock_(clock),
       observer_(observer),
       packet_router_(new PacketRouter()),
-      pacer_(new PacedSender(clock_,
+      pacer_(new PacedSender(clock_, packet_router_.get())),
-                             packet_router_.get())),
       remote_bitrate_estimator_(
           new WrappingBitrateEstimator(remote_bitrate_observer, clock_)),
       bitrate_controller_(BitrateController::CreateBitrateController(clock_)),
       remote_estimator_proxy_(clock_, packet_router_.get()),
       transport_feedback_adapter_(bitrate_controller_.get(), clock_),
       min_bitrate_bps_(RemoteBitrateEstimator::kDefaultMinBitrateBps),
-      send_queue_is_full_(false) {
+      last_reported_bitrate_bps_(0),
+      last_reported_fraction_loss_(0),
+      last_reported_rtt_(0),
+      network_state_(kNetworkUp) {
   Init();
 }
 
@@ -192,7 +196,10 @@ CongestionController::CongestionController(
       remote_estimator_proxy_(clock_, packet_router_.get()),
       transport_feedback_adapter_(bitrate_controller_.get(), clock_),
       min_bitrate_bps_(RemoteBitrateEstimator::kDefaultMinBitrateBps),
-      send_queue_is_full_(false) {
+      last_reported_bitrate_bps_(0),
+      last_reported_fraction_loss_(0),
+      last_reported_rtt_(0),
+      network_state_(kNetworkUp) {
   Init();
 }
 
@@ -265,6 +272,11 @@ void CongestionController::SignalNetworkState(NetworkState state) {
   } else {
     pacer_->Pause();
   }
+  {
+    rtc::CritScope cs(&critsect_);
+    network_state_ = state;
+  }
+  MaybeTriggerOnNetworkChanged();
 }
 
 void CongestionController::OnSentPacket(const rtc::SentPacket& sent_packet) {
@@ -297,24 +309,40 @@ void CongestionController::MaybeTriggerOnNetworkChanged() {
   uint32_t bitrate_bps;
   uint8_t fraction_loss;
   int64_t rtt;
-  bool network_changed = bitrate_controller_->GetNetworkParameters(
+  bool estimate_changed = bitrate_controller_->GetNetworkParameters(
       &bitrate_bps, &fraction_loss, &rtt);
-  if (network_changed)
+  if (estimate_changed)
     pacer_->SetEstimatedBitrate(bitrate_bps);
-  bool send_queue_is_full =
+
-      pacer_->ExpectedQueueTimeMs() > PacedSender::kMaxQueueLengthMs;
+  bitrate_bps = IsNetworkDown() || IsSendQueueFull() ? 0 : bitrate_bps;
-  bitrate_bps = send_queue_is_full ? 0 : bitrate_bps;
+
-  if ((network_changed && !send_queue_is_full) ||
+  if (HasNetworkParametersToReportChanged(bitrate_bps, fraction_loss, rtt)) {
-      UpdateSendQueueStatus(send_queue_is_full)) {
     observer_->OnNetworkChanged(bitrate_bps, fraction_loss, rtt);
   }
 }
 
-bool CongestionController::UpdateSendQueueStatus(bool send_queue_is_full) {
+bool CongestionController::HasNetworkParametersToReportChanged(
+    uint32_t bitrate_bps,
+    uint8_t fraction_loss,
+    int64_t rtt) {
   rtc::CritScope cs(&critsect_);
-  bool result = send_queue_is_full_ != send_queue_is_full;
+  bool changed =
-  send_queue_is_full_ = send_queue_is_full;
+      last_reported_bitrate_bps_ != bitrate_bps ||
-  return result;
+      (bitrate_bps > 0 && (last_reported_fraction_loss_ != fraction_loss ||
+                           last_reported_rtt_ != rtt));
+  last_reported_bitrate_bps_ = bitrate_bps;
+  last_reported_fraction_loss_ = fraction_loss;
+  last_reported_rtt_ = rtt;
+  return changed;
+}
+
+bool CongestionController::IsSendQueueFull() const {
+  return pacer_->ExpectedQueueTimeMs() > PacedSender::kMaxQueueLengthMs;
+}
+
+bool CongestionController::IsNetworkDown() const {
+  rtc::CritScope cs(&critsect_);
+  return network_state_ == kNetworkDown;
 }
 
 }  // namespace webrtc

webrtc/modules/congestion_controller/congestion_controller_unittest.cc

@@ -113,5 +113,45 @@ TEST_F(CongestionControllerTest, OnSendQueueFullAndEstimateChange) {
   controller_->Process();
 }
 
+TEST_F(CongestionControllerTest, SignalNetworkState) {
+  EXPECT_CALL(observer_, OnNetworkChanged(0, _, _));
+  controller_->SignalNetworkState(kNetworkDown);
+
+  EXPECT_CALL(observer_, OnNetworkChanged(kInitialBitrateBps, _, _));
+  controller_->SignalNetworkState(kNetworkUp);
+
+  EXPECT_CALL(observer_, OnNetworkChanged(0, _, _));
+  controller_->SignalNetworkState(kNetworkDown);
+}
+
+TEST_F(CongestionControllerTest,
+       SignalNetworkStateAndQueueIsFullAndEstimateChange) {
+  // Send queue is full
+  EXPECT_CALL(*pacer_, ExpectedQueueTimeMs())
+      .WillRepeatedly(Return(PacedSender::kMaxQueueLengthMs + 1));
+  EXPECT_CALL(observer_, OnNetworkChanged(0, _, _));
+  controller_->Process();
+
+  // Queue is full and network is down. Expect no bitrate change.
+  controller_->SignalNetworkState(kNetworkDown);
+  controller_->Process();
+
+  // Queue is full but network is up. Expect no bitrate change.
+  controller_->SignalNetworkState(kNetworkUp);
+  controller_->Process();
+
+  // Receive new estimate but let the queue still be full.
+  EXPECT_CALL(*pacer_, SetEstimatedBitrate(kInitialBitrateBps * 2));
+  bandwidth_observer_->OnReceivedEstimatedBitrate(kInitialBitrateBps * 2);
+  clock_.AdvanceTimeMilliseconds(25);
+  controller_->Process();
+
+  // Let the pacer not be full next time the controller checks.
+  EXPECT_CALL(*pacer_, ExpectedQueueTimeMs())
+      .WillOnce(Return(PacedSender::kMaxQueueLengthMs - 1));
+  EXPECT_CALL(observer_, OnNetworkChanged(kInitialBitrateBps * 2, _, _));
+  controller_->Process();
+}
+
 }  // namespace test
 }  // namespace webrtc

webrtc/modules/congestion_controller/include/congestion_controller.h

@@ -95,10 +95,12 @@ class CongestionController : public CallStatsObserver, public Module {
  private:
   void Init();
   void MaybeTriggerOnNetworkChanged();
-  // Updates |send_queue_is_full_|. Returns true if |send_queue_is_full_|
-  // has changed.
-  bool UpdateSendQueueStatus(bool send_queue_is_full);
 
+  bool IsSendQueueFull() const;
+  bool IsNetworkDown() const;
+  bool HasNetworkParametersToReportChanged(uint32_t bitrate_bps,
+                                           uint8_t fraction_loss,
+                                           int64_t rtt);
   Clock* const clock_;
   Observer* const observer_;
   const std::unique_ptr<PacketRouter> packet_router_;
@@ -109,7 +111,10 @@ class CongestionController : public CallStatsObserver, public Module {
   TransportFeedbackAdapter transport_feedback_adapter_;
   int min_bitrate_bps_;
   rtc::CriticalSection critsect_;
-  bool send_queue_is_full_ GUARDED_BY(critsect_);
+  uint32_t last_reported_bitrate_bps_ GUARDED_BY(critsect_);
+  uint8_t last_reported_fraction_loss_ GUARDED_BY(critsect_);
+  int64_t last_reported_rtt_ GUARDED_BY(critsect_);
+  NetworkState network_state_ GUARDED_BY(critsect_);
 
   RTC_DISALLOW_IMPLICIT_CONSTRUCTORS(CongestionController);
 };

webrtc/modules/video_coding/video_sender.cc

@@ -228,6 +228,10 @@ int32_t VideoSender::SetChannelParameters(uint32_t target_bitrate,
 }
 
 void VideoSender::SetEncoderParameters(EncoderParameters params) {
+  // |target_bitrate == 0 | means that the network is down or the send pacer is
+  // full.
+  // TODO(perkj): Consider setting |target_bitrate| == 0 to the encoders.
+  // Especially if |encoder_has_internal_source_ | == true.
   if (params.target_bitrate == 0)
     return;
 

webrtc/modules/video_coding/video_sender_unittest.cc

@@ -292,6 +292,18 @@ TEST_F(TestVideoSenderWithMockEncoder, TestIntraRequests) {
   AddFrame();
 }
 
+TEST_F(TestVideoSenderWithMockEncoder, TestSetRate) {
+  const uint32_t new_bitrate = settings_.startBitrate + 300;
+  EXPECT_CALL(encoder_, SetRates(new_bitrate, _)).Times(1).WillOnce(Return(0));
+  sender_->SetChannelParameters(new_bitrate * 1000, 0, 200);
+  AddFrame();
+
+  // Expect no call to encoder_.SetRates if the new bitrate is zero.
+  EXPECT_CALL(encoder_, SetRates(new_bitrate, _)).Times(0);
+  sender_->SetChannelParameters(0, 0, 200);
+  AddFrame();
+}
+
 TEST_F(TestVideoSenderWithMockEncoder, TestIntraRequestsInternalCapture) {
   // De-register current external encoder.
   sender_->RegisterExternalEncoder(nullptr, kUnusedPayloadType, false);

webrtc/video/end_to_end_tests.cc

@@ -3341,6 +3341,13 @@ TEST_F(EndToEndTest, NewVideoSendStreamsRespectVideoNetworkDown) {
   class UnusedEncoder : public test::FakeEncoder {
    public:
     UnusedEncoder() : FakeEncoder(Clock::GetRealTimeClock()) {}
+
+    int32_t InitEncode(const VideoCodec* config,
+                       int32_t number_of_cores,
+                       size_t max_payload_size) override {
+      EXPECT_GT(config->startBitrate, 0u);
+      return 0;
+    }
     int32_t Encode(const VideoFrame& input_image,
                    const CodecSpecificInfo* codec_specific_info,
                    const std::vector<FrameType>* frame_types) override {

webrtc/video/video_send_stream.cc

@@ -491,21 +491,6 @@ VideoSendStream::~VideoSendStream() {
   }
 }
 
-void VideoSendStream::SignalNetworkState(NetworkState state) {
-  // When network goes up, enable RTCP status before setting transmission state.
-  // When it goes down, disable RTCP afterwards. This ensures that any packets
-  // sent due to the network state changed will not be dropped.
-  if (state == kNetworkUp) {
-    for (RtpRtcp* rtp_rtcp : rtp_rtcp_modules_)
-      rtp_rtcp->SetRTCPStatus(config_.rtp.rtcp_mode);
-  }
-  vie_encoder_.SetNetworkTransmissionState(state == kNetworkUp);
-  if (state == kNetworkDown) {
-    for (RtpRtcp* rtp_rtcp : rtp_rtcp_modules_)
-      rtp_rtcp->SetRTCPStatus(RtcpMode::kOff);
-  }
-}
-
 bool VideoSendStream::DeliverRtcp(const uint8_t* packet, size_t length) {
   for (RtpRtcp* rtp_rtcp : rtp_rtcp_modules_)
     rtp_rtcp->IncomingRtcpPacket(packet, length);
@@ -780,6 +765,13 @@ std::map<uint32_t, RtpState> VideoSendStream::GetRtpStates() const {
   return rtp_states;
 }
 
+void VideoSendStream::SignalNetworkState(NetworkState state) {
+  for (RtpRtcp* rtp_rtcp : rtp_rtcp_modules_) {
+    rtp_rtcp->SetRTCPStatus(state == kNetworkUp ? config_.rtp.rtcp_mode
+                                                : RtcpMode::kOff);
+  }
+}
+
 int VideoSendStream::GetPaddingNeededBps() const {
   return vie_encoder_.GetPaddingNeededBps();
 }

webrtc/video/vie_encoder.cc

@@ -43,7 +43,6 @@ ViEEncoder::ViEEncoder(uint32_t number_of_cores,
       encoder_config_(),
       min_transmit_bitrate_bps_(0),
       last_observed_bitrate_bps_(0),
-      network_is_transmitting_(true),
       encoder_paused_(true),
       encoder_paused_and_dropped_frame_(false),
       module_process_thread_(module_process_thread),
@@ -64,13 +63,6 @@ ViEEncoder::~ViEEncoder() {
   module_process_thread_->DeRegisterModule(&video_sender_);
 }
 
-void ViEEncoder::SetNetworkTransmissionState(bool is_transmitting) {
-  {
-    rtc::CritScope lock(&data_cs_);
-    network_is_transmitting_ = is_transmitting;
-  }
-}
-
 void ViEEncoder::Pause() {
   rtc::CritScope lock(&data_cs_);
   encoder_paused_ = true;
@@ -199,13 +191,9 @@ int ViEEncoder::GetPaddingNeededBps() const {
 bool ViEEncoder::EncoderPaused() const {
   // Pause video if paused by caller or as long as the network is down or the
   // pacer queue has grown too large in buffered mode.
-  if (encoder_paused_) {
+  // If the pacer queue has grown to large or the network is down,
-    return true;
+  // last_observed_bitrate_bps_ will be 0.
-  }
+  return encoder_paused_ || video_suspended_ || last_observed_bitrate_bps_ == 0;
-  if (video_suspended_ || last_observed_bitrate_bps_ == 0) {
-    return true;
-  }
-  return !network_is_transmitting_;
 }
 
 void ViEEncoder::TraceFrameDropStart() {

webrtc/video/vie_encoder.h

@@ -63,8 +63,6 @@ class ViEEncoder : public VideoEncoderRateObserver,
 
   vcm::VideoSender* video_sender();
 
-  void SetNetworkTransmissionState(bool is_transmitting);
-
   // Returns the id of the owning channel.
   int Owner() const;
 
@@ -137,7 +135,6 @@ class ViEEncoder : public VideoEncoderRateObserver,
   VideoCodec encoder_config_ GUARDED_BY(data_cs_);
   int min_transmit_bitrate_bps_ GUARDED_BY(data_cs_);
   uint32_t last_observed_bitrate_bps_ GUARDED_BY(data_cs_);
-  bool network_is_transmitting_ GUARDED_BY(data_cs_);
   bool encoder_paused_ GUARDED_BY(data_cs_);
   bool encoder_paused_and_dropped_frame_ GUARDED_BY(data_cs_);