Remove ViEEncoder::Pause / Start

This cl change so that VideoSendStream::Start adds the stream as a BitrateObserver and VideoSendStream::Stop removes the stream as observer.

That also means that start will trigger a VideoEncoder::SetRate call with the most recent bitrate estimate.
VideoSendStream::Stop will trigger a VideoEncoder::SetRate with bitrate  = 0.

BUG=webrtc:5687 b/28636240

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

webrtc/call/bitrate_allocator.cc

@@ -53,7 +53,7 @@ void BitrateAllocator::OnNetworkChanged(uint32_t target_bitrate_bps,
   last_allocation_ = allocation;
 }
 
-int BitrateAllocator::AddObserver(BitrateAllocatorObserver* observer,
+void BitrateAllocator::AddObserver(BitrateAllocatorObserver* observer,
                                    uint32_t min_bitrate_bps,
                                    uint32_t max_bitrate_bps,
                                    uint32_t pad_up_bitrate_bps,
@@ -89,7 +89,6 @@ int BitrateAllocator::AddObserver(BitrateAllocatorObserver* observer,
   UpdateAllocationLimits();
 
   last_allocation_ = allocation;
-  return allocation[observer];
 }
 
 void BitrateAllocator::UpdateAllocationLimits() {
@@ -121,6 +120,18 @@ void BitrateAllocator::RemoveObserver(BitrateAllocatorObserver* observer) {
   UpdateAllocationLimits();
 }
 
+int BitrateAllocator::GetStartBitrate(BitrateAllocatorObserver* observer) {
+  rtc::CritScope lock(&crit_sect_);
+  const auto& it = last_allocation_.find(observer);
+  if (it != last_allocation_.end())
+    return it->second;
+
+  // This is a new observer that has not yet been started. Assume that if it is
+  // added, all observers would split the available bitrate evenly.
+  return last_non_zero_bitrate_bps_ /
+         static_cast<int>((bitrate_observer_configs_.size() + 1));
+}
+
 BitrateAllocator::ObserverConfigList::iterator
 BitrateAllocator::FindObserverConfig(
     const BitrateAllocatorObserver* observer) {

webrtc/call/bitrate_allocator.h

@@ -68,12 +68,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 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,
+  void AddObserver(BitrateAllocatorObserver* observer,
                    uint32_t min_bitrate_bps,
                    uint32_t max_bitrate_bps,
                    uint32_t pad_up_bitrate_bps,
@@ -83,6 +82,10 @@ class BitrateAllocator {
   // allocation.
   void RemoveObserver(BitrateAllocatorObserver* observer);
 
+  // Returns initial bitrate allocated for |observer|. If |observer| is not in
+  // the list of added observers, a best guess is returned.
+  int GetStartBitrate(BitrateAllocatorObserver* observer);
+
  private:
   // Note: All bitrates for member variables and methods are in bps.
   struct ObserverConfig {

webrtc/call/bitrate_allocator_unittest.cc

@@ -63,29 +63,29 @@ TEST_F(BitrateAllocatorTest, UpdatingBitrateObserver) {
 
   EXPECT_CALL(limit_observer_, OnAllocationLimitsChanged(kMinSendBitrateBps,
                                                          kPadUpToBitrateBps));
-  int start_bitrate = allocator_->AddObserver(
+  allocator_->AddObserver(&bitrate_observer, kMinSendBitrateBps, 1500000,
-      &bitrate_observer, kMinSendBitrateBps, 1500000, kPadUpToBitrateBps, true);
+                          kPadUpToBitrateBps, true);
-  EXPECT_EQ(300000, start_bitrate);
+  EXPECT_EQ(300000, allocator_->GetStartBitrate(&bitrate_observer));
   allocator_->OnNetworkChanged(200000, 0, 0);
-  EXPECT_EQ(200000u, bitrate_observer.last_bitrate_bps_);
+  EXPECT_EQ(200000, allocator_->GetStartBitrate(&bitrate_observer));
 
   // TODO(pbos): Expect capping to 1.5M instead of 3M when not boosting the max
   // bitrate for FEC/retransmissions (see todo in BitrateAllocator).
   allocator_->OnNetworkChanged(4000000, 0, 0);
-  EXPECT_EQ(3000000u, bitrate_observer.last_bitrate_bps_);
+  EXPECT_EQ(3000000, allocator_->GetStartBitrate(&bitrate_observer));
 
   // Expect |max_padding_bitrate_bps| to change to 0 if the observer is updated.
   EXPECT_CALL(limit_observer_,
               OnAllocationLimitsChanged(kMinSendBitrateBps, 0));
-  start_bitrate = allocator_->AddObserver(&bitrate_observer, kMinSendBitrateBps,
+  allocator_->AddObserver(&bitrate_observer, kMinSendBitrateBps, 4000000, 0,
-                                          4000000, 0, true);
+                          true);
-  EXPECT_EQ(4000000, start_bitrate);
+  EXPECT_EQ(4000000, allocator_->GetStartBitrate(&bitrate_observer));
 
   EXPECT_CALL(limit_observer_,
               OnAllocationLimitsChanged(kMinSendBitrateBps, 0));
-  start_bitrate = allocator_->AddObserver(&bitrate_observer, kMinSendBitrateBps,
+  allocator_->AddObserver(&bitrate_observer, kMinSendBitrateBps, 1500000, 0,
-                                          1500000, 0, true);
+                          true);
-  EXPECT_EQ(3000000, start_bitrate);
+  EXPECT_EQ(3000000, allocator_->GetStartBitrate(&bitrate_observer));
   EXPECT_EQ(3000000u, bitrate_observer.last_bitrate_bps_);
   allocator_->OnNetworkChanged(1500000, 0, 0);
   EXPECT_EQ(1500000u, bitrate_observer.last_bitrate_bps_);
@@ -95,15 +95,15 @@ TEST_F(BitrateAllocatorTest, TwoBitrateObserversOneRtcpObserver) {
   TestBitrateObserver bitrate_observer_1;
   TestBitrateObserver bitrate_observer_2;
   EXPECT_CALL(limit_observer_, OnAllocationLimitsChanged(100000, 0));
-  int start_bitrate =
       allocator_->AddObserver(&bitrate_observer_1, 100000, 300000, 0, true);
-  EXPECT_EQ(300000, start_bitrate);
+      EXPECT_EQ(300000, allocator_->GetStartBitrate(&bitrate_observer_1));
-  EXPECT_CALL(limit_observer_, OnAllocationLimitsChanged(100000 + 200000, 0));
+      EXPECT_CALL(limit_observer_,
-  start_bitrate =
+                  OnAllocationLimitsChanged(100000 + 200000, 0));
       allocator_->AddObserver(&bitrate_observer_2, 200000, 300000, 0, true);
-  EXPECT_EQ(200000, start_bitrate);
+      EXPECT_EQ(200000, allocator_->GetStartBitrate(&bitrate_observer_2));
 
-  // Test too low start bitrate, hence lower than sum of min. Min bitrates will
+      // Test too low start bitrate, hence lower than sum of min. Min bitrates
+      // will
       // be allocated to all observers.
       allocator_->OnNetworkChanged(200000, 0, 50);
       EXPECT_EQ(100000u, bitrate_observer_1.last_bitrate_bps_);
@@ -121,12 +121,14 @@ TEST_F(BitrateAllocatorTest, TwoBitrateObserversOneRtcpObserver) {
       EXPECT_EQ(200000u + kBitrateToShare / 2,
                 bitrate_observer_2.last_bitrate_bps_);
 
-  // Limited by 2x max bitrates since we leave room for FEC and retransmissions.
+      // Limited by 2x max bitrates since we leave room for FEC and
+      // retransmissions.
       allocator_->OnNetworkChanged(1500000, 0, 50);
       EXPECT_EQ(600000u, bitrate_observer_1.last_bitrate_bps_);
       EXPECT_EQ(600000u, bitrate_observer_2.last_bitrate_bps_);
 
-  // Verify that if the bandwidth estimate is set to zero, the allocated rate is
+      // Verify that if the bandwidth estimate is set to zero, the allocated
+      // rate is
       // zero.
       allocator_->OnNetworkChanged(0, 0, 50);
       EXPECT_EQ(0u, bitrate_observer_1.last_bitrate_bps_);
@@ -165,9 +167,8 @@ TEST_F(BitrateAllocatorTestNoEnforceMin, OneBitrateObserver) {
   // Expect OnAllocationLimitsChanged with |min_send_bitrate_bps| = 0 since
   // AddObserver is called with |enforce_min_bitrate| = false.
   EXPECT_CALL(limit_observer_, OnAllocationLimitsChanged(0, 0));
-  int start_bitrate =
       allocator_->AddObserver(&bitrate_observer_1, 100000, 400000, 0, false);
-  EXPECT_EQ(300000, start_bitrate);
+      EXPECT_EQ(300000, allocator_->GetStartBitrate(&bitrate_observer_1));
 
       // High BWE.
       allocator_->OnNetworkChanged(150000, 0, 0);
@@ -186,18 +187,15 @@ TEST_F(BitrateAllocatorTestNoEnforceMin, ThreeBitrateObservers) {
   TestBitrateObserver bitrate_observer_2;
   TestBitrateObserver bitrate_observer_3;
   // Set up the observers with min bitrates at 100000, 200000, and 300000.
-  int start_bitrate =
   allocator_->AddObserver(&bitrate_observer_1, 100000, 400000, 0, false);
-  EXPECT_EQ(300000, start_bitrate);
+  EXPECT_EQ(300000, allocator_->GetStartBitrate(&bitrate_observer_1));
 
-  start_bitrate =
   allocator_->AddObserver(&bitrate_observer_2, 200000, 400000, 0, false);
-  EXPECT_EQ(200000, start_bitrate);
+  EXPECT_EQ(200000, allocator_->GetStartBitrate(&bitrate_observer_2));
   EXPECT_EQ(100000u, bitrate_observer_1.last_bitrate_bps_);
 
-  start_bitrate =
   allocator_->AddObserver(&bitrate_observer_3, 300000, 400000, 0, false);
-  EXPECT_EQ(0, start_bitrate);
+  EXPECT_EQ(0, allocator_->GetStartBitrate(&bitrate_observer_3));
   EXPECT_EQ(100000u, bitrate_observer_1.last_bitrate_bps_);
   EXPECT_EQ(200000u, bitrate_observer_2.last_bitrate_bps_);
 
@@ -249,22 +247,21 @@ TEST_F(BitrateAllocatorTest, ThreeBitrateObserversLowBweEnforceMin) {
   TestBitrateObserver bitrate_observer_1;
   TestBitrateObserver bitrate_observer_2;
   TestBitrateObserver bitrate_observer_3;
-  int start_bitrate =
-      allocator_->AddObserver(&bitrate_observer_1, 100000, 400000, 0, true);
-  EXPECT_EQ(300000, start_bitrate);
 
-  start_bitrate =
+  allocator_->AddObserver(&bitrate_observer_1, 100000, 400000, 0, true);
+  EXPECT_EQ(300000, allocator_->GetStartBitrate(&bitrate_observer_1));
+
   allocator_->AddObserver(&bitrate_observer_2, 200000, 400000, 0, true);
-  EXPECT_EQ(200000, start_bitrate);
+  EXPECT_EQ(200000, allocator_->GetStartBitrate(&bitrate_observer_2));
   EXPECT_EQ(100000u, bitrate_observer_1.last_bitrate_bps_);
 
-  start_bitrate =
       allocator_->AddObserver(&bitrate_observer_3, 300000, 400000, 0, true);
-  EXPECT_EQ(300000, start_bitrate);
+      EXPECT_EQ(300000, allocator_->GetStartBitrate(&bitrate_observer_3));
       EXPECT_EQ(100000, static_cast<int>(bitrate_observer_1.last_bitrate_bps_));
       EXPECT_EQ(200000, static_cast<int>(bitrate_observer_2.last_bitrate_bps_));
 
-  // Low BWE. Verify that all observers still get their respective min bitrate.
+      // Low BWE. Verify that all observers still get their respective min
+      // bitrate.
       allocator_->OnNetworkChanged(1000, 0, 0);
       EXPECT_EQ(100000u, bitrate_observer_1.last_bitrate_bps_);  // Min cap.
       EXPECT_EQ(200000u, bitrate_observer_2.last_bitrate_bps_);  // Min cap.
@@ -278,9 +275,9 @@ TEST_F(BitrateAllocatorTest, ThreeBitrateObserversLowBweEnforceMin) {
 TEST_F(BitrateAllocatorTest, AddObserverWhileNetworkDown) {
   TestBitrateObserver bitrate_observer_1;
   EXPECT_CALL(limit_observer_, OnAllocationLimitsChanged(50000, 0));
-  int start_bitrate =
+
   allocator_->AddObserver(&bitrate_observer_1, 50000, 400000, 0, true);
-  EXPECT_EQ(300000, start_bitrate);
+  EXPECT_EQ(300000, allocator_->GetStartBitrate(&bitrate_observer_1));
 
   // Set network down, ie, no available bitrate.
   allocator_->OnNetworkChanged(0, 0, 0);
@@ -290,12 +287,11 @@ TEST_F(BitrateAllocatorTest, AddObserverWhileNetworkDown) {
   TestBitrateObserver bitrate_observer_2;
   // Adding an observer while the network is down should not affect the limits.
   EXPECT_CALL(limit_observer_, OnAllocationLimitsChanged(50000 + 50000, 0));
-  start_bitrate =
   allocator_->AddObserver(&bitrate_observer_2, 50000, 400000, 0, 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(300000 / 2, allocator_->GetStartBitrate(&bitrate_observer_2));
   EXPECT_EQ(0u, bitrate_observer_1.last_bitrate_bps_);
   EXPECT_EQ(0u, bitrate_observer_2.last_bitrate_bps_);
 
@@ -307,14 +303,12 @@ TEST_F(BitrateAllocatorTest, AddObserverWhileNetworkDown) {
 
 TEST_F(BitrateAllocatorTest, MixedEnforecedConfigs) {
   TestBitrateObserver enforced_observer;
-  int start_bitrate =
       allocator_->AddObserver(&enforced_observer, 6000, 30000, 0, true);
-  EXPECT_EQ(60000, start_bitrate);
+      EXPECT_EQ(60000, allocator_->GetStartBitrate(&enforced_observer));
 
       TestBitrateObserver not_enforced_observer;
-  start_bitrate =
       allocator_->AddObserver(&not_enforced_observer, 30000, 2500000, 0, false);
-  EXPECT_EQ(270000, start_bitrate);
+      EXPECT_EQ(270000, allocator_->GetStartBitrate(&not_enforced_observer));
       EXPECT_EQ(30000u, enforced_observer.last_bitrate_bps_);
 
       allocator_->OnNetworkChanged(36000, 0, 50);
@@ -351,9 +345,8 @@ TEST_F(BitrateAllocatorTest, MixedEnforecedConfigs) {
 
 TEST_F(BitrateAllocatorTest, AvoidToggleAbsolute) {
   TestBitrateObserver observer;
-  int start_bitrate =
       allocator_->AddObserver(&observer, 30000, 300000, 0, false);
-  EXPECT_EQ(300000, start_bitrate);
+      EXPECT_EQ(300000, allocator_->GetStartBitrate(&observer));
 
       allocator_->OnNetworkChanged(30000, 0, 50);
       EXPECT_EQ(30000u, observer.last_bitrate_bps_);
@@ -378,9 +371,8 @@ TEST_F(BitrateAllocatorTest, AvoidToggleAbsolute) {
 
 TEST_F(BitrateAllocatorTest, AvoidTogglePercent) {
   TestBitrateObserver observer;
-  int start_bitrate =
       allocator_->AddObserver(&observer, 300000, 600000, 0, false);
-  EXPECT_EQ(300000, start_bitrate);
+      EXPECT_EQ(300000, allocator_->GetStartBitrate(&observer));
 
       allocator_->OnNetworkChanged(300000, 0, 50);
       EXPECT_EQ(300000u, observer.last_bitrate_bps_);

webrtc/modules/video_coding/generic_encoder.cc

@@ -44,12 +44,6 @@ int32_t VCMGenericEncoder::InitEncode(const VideoCodec* settings,
                                       int32_t number_of_cores,
                                       size_t max_payload_size) {
   TRACE_EVENT0("webrtc", "VCMGenericEncoder::InitEncode");
-  {
-    rtc::CritScope lock(&params_lock_);
-    encoder_params_.target_bitrate = settings->startBitrate * 1000;
-    encoder_params_.input_frame_rate = settings->maxFramerate;
-  }
-
   is_screenshare_ = settings->mode == VideoCodecMode::kScreensharing;
   if (encoder_->InitEncode(settings, number_of_cores, max_payload_size) != 0) {
     LOG(LS_ERROR) << "Failed to initialize the encoder associated with "

webrtc/modules/video_coding/protection_bitrate_calculator.cc

@@ -8,7 +8,7 @@
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
-#include <webrtc/modules/video_coding/protection_bitrate_calculator.h>
+#include "webrtc/modules/video_coding/protection_bitrate_calculator.h"
 
 namespace webrtc {
 

webrtc/modules/video_coding/protection_bitrate_calculator.h

@@ -59,7 +59,6 @@ class ProtectionBitrateCalculator {
                           int actual_framerate,
                           uint8_t fraction_lost,
                           int64_t round_trip_time_ms);
-
   // Informs of encoded output.
   void UpdateWithEncodedData(const EncodedImage& encoded_image);
 

webrtc/modules/video_coding/protection_bitrate_calculator_unittest.cc

@@ -86,4 +86,15 @@ TEST_F(ProtectionBitrateCalculatorTest, ProtectsUsingNackBitrate) {
   EXPECT_EQ(kMaxBitrateBps / 2, target_bitrate);
 }
 
+TEST_F(ProtectionBitrateCalculatorTest, NoProtection) {
+  static const uint32_t kMaxBitrateBps = 130000;
+
+  media_opt_.SetProtectionMethod(false /*enable_fec*/, false /* enable_nack */);
+  media_opt_.SetEncodingData(kCodecBitrateBps, 640, 480, 30, 1, 1000);
+
+  uint32_t target_bitrate =
+      media_opt_.SetTargetRates(kMaxBitrateBps, 30, 128, 100);
+  EXPECT_EQ(kMaxBitrateBps, target_bitrate);
+}
+
 }  // namespace webrtc

webrtc/modules/video_coding/video_coding_impl.h

@@ -96,7 +96,7 @@ class VideoSender : public Module {
   void Process() override;
 
  private:
-  void SetEncoderParameters(EncoderParameters params)
+  void SetEncoderParameters(EncoderParameters params, bool has_internal_source)
       EXCLUSIVE_LOCKS_REQUIRED(encoder_crit_);
 
   Clock* const clock_;

webrtc/modules/video_coding/video_sender.cc

@@ -219,19 +219,23 @@ int32_t VideoSender::SetChannelParameters(uint32_t target_bitrate,
   if (encoder_has_internal_source) {
     rtc::CritScope cs(&encoder_crit_);
     if (_encoder) {
-      SetEncoderParameters(encoder_params);
+      SetEncoderParameters(encoder_params, encoder_has_internal_source);
     }
   }
 
   return VCM_OK;
 }
 
-void VideoSender::SetEncoderParameters(EncoderParameters params) {
+void VideoSender::SetEncoderParameters(EncoderParameters params,
+                                       bool has_internal_source) {
   // |target_bitrate == 0 | means that the network is down or the send pacer is
-  // full.
+  // full. We currently only report this if the encoder has an internal source.
-  // TODO(perkj): Consider setting |target_bitrate| == 0 to the encoders.
+  // If the encoder does not have an internal source, higher levels are expected
-  // Especially if |encoder_has_internal_source_ | == true.
+  // to not call AddVideoFrame. We do this since its unclear how current
-  if (params.target_bitrate == 0)
+  // encoder implementations behave when given a zero target bitrate.
+  // TODO(perkj): Make sure all known encoder implementations handle zero
+  // target bitrate and remove this check.
+  if (!has_internal_source && params.target_bitrate == 0)
     return;
 
   if (params.input_frame_rate == 0) {
@@ -258,15 +262,17 @@ int32_t VideoSender::AddVideoFrame(const VideoFrame& videoFrame,
                                    const CodecSpecificInfo* codecSpecificInfo) {
   EncoderParameters encoder_params;
   std::vector<FrameType> next_frame_types;
+  bool encoder_has_internal_source = false;
   {
     rtc::CritScope lock(&params_crit_);
     encoder_params = encoder_params_;
     next_frame_types = next_frame_types_;
+    encoder_has_internal_source = encoder_has_internal_source_;
   }
   rtc::CritScope lock(&encoder_crit_);
   if (_encoder == nullptr)
     return VCM_UNINITIALIZED;
-  SetEncoderParameters(encoder_params);
+  SetEncoderParameters(encoder_params, encoder_has_internal_source);
   if (_mediaOpt.DropFrame()) {
     LOG(LS_VERBOSE) << "Drop Frame "
                     << "target bitrate " << encoder_params.target_bitrate

webrtc/video/end_to_end_tests.cc

@@ -2447,6 +2447,7 @@ TEST_F(EndToEndTest, ReportsSetEncoderRates) {
         std::vector<VideoReceiveStream::Config>* receive_configs,
         VideoEncoderConfig* encoder_config) override {
       send_config->encoder_settings.encoder = this;
+      RTC_DCHECK_EQ(1u, encoder_config->streams.size());
     }
 
     int32_t SetRates(uint32_t new_target_bitrate, uint32_t framerate) override {

webrtc/video/video_send_stream.cc

@@ -396,7 +396,7 @@ VideoSendStream::VideoSendStream(
       encoder_thread_(EncoderThreadFunction, this, "EncoderThread"),
       encoder_wakeup_event_(false, false),
       stop_encoder_thread_(0),
-      send_stream_registered_as_observer_(false),
+      state_(State::kStopped),
       overuse_detector_(
           Clock::GetRealTimeClock(),
           GetCpuOveruseOptions(config.encoder_settings.full_overuse_time),
@@ -440,7 +440,6 @@ VideoSendStream::VideoSendStream(
   RTC_DCHECK(congestion_controller_);
   RTC_DCHECK(remb_);
 
-
   // RTP/RTCP initialization.
   for (RtpRtcp* rtp_rtcp : rtp_rtcp_modules_) {
     module_process_thread_->RegisterModule(rtp_rtcp);
@@ -489,6 +488,7 @@ VideoSendStream::VideoSendStream(
 
   module_process_thread_->RegisterModule(&overuse_detector_);
 
+  encoder_thread_checker_.DetachFromThread();
   encoder_thread_.Start();
   encoder_thread_.SetPriority(rtc::kHighPriority);
 }
@@ -530,17 +530,23 @@ void VideoSendStream::Start() {
     return;
   TRACE_EVENT_INSTANT0("webrtc", "VideoSendStream::Start");
   payload_router_.set_active(true);
-  // Was not already started, trigger a keyframe.
+  {
-  vie_encoder_.SendKeyFrame();
+    rtc::CritScope lock(&encoder_settings_crit_);
-  vie_encoder_.Start();
+    pending_state_change_ = rtc::Optional<State>(State::kStarted);
+  }
+  encoder_wakeup_event_.Set();
 }
 
 void VideoSendStream::Stop() {
   if (!payload_router_.active())
     return;
   TRACE_EVENT_INSTANT0("webrtc", "VideoSendStream::Stop");
-  vie_encoder_.Pause();
   payload_router_.set_active(false);
+  {
+    rtc::CritScope lock(&encoder_settings_crit_);
+    pending_state_change_ = rtc::Optional<State>(State::kStopped);
+  }
+  encoder_wakeup_event_.Set();
 }
 
 VideoCaptureInput* VideoSendStream::Input() {
@@ -558,7 +564,7 @@ void VideoSendStream::EncoderProcess() {
                       config_.encoder_settings.encoder,
                       config_.encoder_settings.payload_type,
                       config_.encoder_settings.internal_source));
-
+  RTC_DCHECK_RUN_ON(&encoder_thread_checker_);
   while (true) {
     // Wake up every kEncodeCheckForActivityPeriodMs to check if the encoder is
     // active. If not, deregister as BitrateAllocatorObserver.
@@ -567,25 +573,21 @@ void VideoSendStream::EncoderProcess() {
     if (rtc::AtomicOps::AcquireLoad(&stop_encoder_thread_))
       break;
     bool change_settings = false;
+    rtc::Optional<State> pending_state_change;
     {
       rtc::CritScope lock(&encoder_settings_crit_);
       if (pending_encoder_settings_) {
         std::swap(current_encoder_settings_, pending_encoder_settings_);
         pending_encoder_settings_.reset();
         change_settings = true;
+      } else if (pending_state_change_) {
+        swap(pending_state_change, pending_state_change_);
       }
     }
     if (change_settings) {
-      current_encoder_settings_->video_codec.startBitrate =
+      current_encoder_settings_->video_codec.startBitrate = std::max(
-          bitrate_allocator_->AddObserver(
+          bitrate_allocator_->GetStartBitrate(this) / 1000,
-              this, current_encoder_settings_->video_codec.minBitrate * 1000,
+          static_cast<int>(current_encoder_settings_->video_codec.minBitrate));
-              current_encoder_settings_->video_codec.maxBitrate * 1000,
-              CalulcateMaxPadBitrateBps(current_encoder_settings_->config,
-                                        config_.suspend_below_min_bitrate),
-              !config_.suspend_below_min_bitrate) /
-          1000;
-      send_stream_registered_as_observer_ = true;
-
       payload_router_.SetSendStreams(current_encoder_settings_->config.streams);
       vie_encoder_.SetEncoder(current_encoder_settings_->video_codec,
                               payload_router_.MaxPayloadLength());
@@ -614,27 +616,39 @@ void VideoSendStream::EncoderProcess() {
       continue;
     }
 
-    VideoFrame frame;
+    if (pending_state_change) {
-    if (input_.GetVideoFrame(&frame)) {
+      if (*pending_state_change == State::kStarted &&
-      // TODO(perkj): |pre_encode_callback| is only used by tests. Tests should
+          state_ == State::kStopped) {
-      // register as a sink to the VideoSource instead.
+        bitrate_allocator_->AddObserver(
-      if (config_.pre_encode_callback) {
+            this, current_encoder_settings_->video_codec.minBitrate * 1000,
-        config_.pre_encode_callback->OnFrame(frame);
+            current_encoder_settings_->video_codec.maxBitrate * 1000,
+            CalulcateMaxPadBitrateBps(current_encoder_settings_->config,
+                                      config_.suspend_below_min_bitrate),
+            !config_.suspend_below_min_bitrate);
+        vie_encoder_.SendKeyFrame();
+        state_ = State::kStarted;
+        LOG_F(LS_INFO) << "Encoder started.";
+      } else if (*pending_state_change == State::kStopped) {
+        bitrate_allocator_->RemoveObserver(this);
+        vie_encoder_.OnBitrateUpdated(0, 0, 0);
+        state_ = State::kStopped;
+        LOG_F(LS_INFO) << "Encoder stopped.";
       }
-      vie_encoder_.EncodeVideoFrame(frame);
+      encoder_wakeup_event_.Set();
+      continue;
     }
 
     // Check if the encoder has produced anything the last kEncoderTimeOutMs.
     // If not, deregister as BitrateAllocatorObserver.
-    if (send_stream_registered_as_observer_ &&
+    if (state_ == State::kStarted &&
         vie_encoder_.time_of_last_frame_activity_ms() <
             rtc::TimeMillis() - kEncoderTimeOutMs) {
       // The encoder has timed out.
       LOG_F(LS_INFO) << "Encoder timed out.";
       bitrate_allocator_->RemoveObserver(this);
-      send_stream_registered_as_observer_ = false;
+      state_ = State::kEncoderTimedOut;
     }
-    if (!send_stream_registered_as_observer_ &&
+    if (state_ == State::kEncoderTimedOut &&
         vie_encoder_.time_of_last_frame_activity_ms() >
             rtc::TimeMillis() - kEncoderTimeOutMs) {
       LOG_F(LS_INFO) << "Encoder is active.";
@@ -644,7 +658,17 @@ void VideoSendStream::EncoderProcess() {
           CalulcateMaxPadBitrateBps(current_encoder_settings_->config,
                                     config_.suspend_below_min_bitrate),
           !config_.suspend_below_min_bitrate);
-      send_stream_registered_as_observer_ = true;
+      state_ = State::kStarted;
+    }
+
+    VideoFrame frame;
+    if (input_.GetVideoFrame(&frame)) {
+      // TODO(perkj): |pre_encode_callback| is only used by tests. Tests should
+      // register as a sink to the VideoSource instead.
+      if (config_.pre_encode_callback) {
+        config_.pre_encode_callback->OnFrame(frame);
+      }
+      vie_encoder_.EncodeVideoFrame(frame);
     }
   }
   vie_encoder_.DeRegisterExternalEncoder(config_.encoder_settings.payload_type);
@@ -850,7 +874,6 @@ void VideoSendStream::OnBitrateUpdated(uint32_t bitrate_bps,
   // protection overhead.
   uint32_t encoder_target_rate = protection_bitrate_calculator_.SetTargetRates(
       bitrate_bps, stats_proxy_.GetSendFrameRate(), fraction_loss, rtt);
-
   vie_encoder_.OnBitrateUpdated(encoder_target_rate, fraction_loss, rtt);
 }
 

webrtc/video/video_send_stream.h

@@ -138,9 +138,20 @@ class VideoSendStream : public webrtc::VideoSendStream,
   rtc::CriticalSection encoder_settings_crit_;
   std::unique_ptr<EncoderSettings> pending_encoder_settings_
       GUARDED_BY(encoder_settings_crit_);
+
+  enum class State {
+    kStopped,  // VideoSendStream::Start has not yet been called.
+    kStarted,  // VideoSendStream::Start has been called.
+    // VideoSendStream::Start has been called but the encoder have timed out.
+    kEncoderTimedOut,
+  };
+  rtc::Optional<State> pending_state_change_ GUARDED_BY(encoder_settings_crit_);
+
   // Only used on the encoder thread.
-  bool send_stream_registered_as_observer_;
+  rtc::ThreadChecker encoder_thread_checker_;
-  std::unique_ptr<EncoderSettings> current_encoder_settings_;
+  State state_ ACCESS_ON(&encoder_thread_checker_);
+  std::unique_ptr<EncoderSettings> current_encoder_settings_
+      ACCESS_ON(&encoder_thread_checker_);
 
   OveruseFrameDetector overuse_detector_;
   ViEEncoder vie_encoder_;

webrtc/video/video_send_stream_tests.cc

@@ -1193,6 +1193,79 @@ TEST_F(VideoSendStreamTest, CanReconfigureToUseStartBitrateAbovePreviousMax) {
   DestroyStreams();
 }
 
+// This test that if the encoder use an internal source, VideoEncoder::SetRates
+// will be called with zero bitrate during initialization and that
+// VideoSendStream::Stop also triggers VideoEncoder::SetRates Start to be called
+// with zero bitrate.
+TEST_F(VideoSendStreamTest, VideoSendStreamStopSetEncoderRateToZero) {
+  class StartStopBitrateObserver : public test::FakeEncoder {
+   public:
+    StartStopBitrateObserver()
+        : FakeEncoder(Clock::GetRealTimeClock()),
+          encoder_init_(false, false),
+          bitrate_changed_(false, false),
+          bitrate_kbps_(0) {}
+    int32_t InitEncode(const VideoCodec* config,
+                       int32_t number_of_cores,
+                       size_t max_payload_size) override {
+      rtc::CritScope lock(&crit_);
+      bitrate_kbps_ = config->startBitrate;
+      encoder_init_.Set();
+      return FakeEncoder::InitEncode(config, number_of_cores, max_payload_size);
+    }
+
+    int32_t SetRates(uint32_t new_target_bitrate, uint32_t framerate) override {
+      rtc::CritScope lock(&crit_);
+      bitrate_kbps_ = new_target_bitrate;
+      bitrate_changed_.Set();
+      return FakeEncoder::SetRates(new_target_bitrate, framerate);
+    }
+
+    int GetBitrateKbps() const {
+      rtc::CritScope lock(&crit_);
+      return bitrate_kbps_;
+    }
+
+    bool WaitForEncoderInit() {
+      return encoder_init_.Wait(VideoSendStreamTest::kDefaultTimeoutMs);
+    }
+    bool WaitBitrateChanged() {
+      return bitrate_changed_.Wait(VideoSendStreamTest::kDefaultTimeoutMs);
+    }
+
+   private:
+    rtc::CriticalSection crit_;
+    rtc::Event encoder_init_;
+    rtc::Event bitrate_changed_;
+    int bitrate_kbps_ GUARDED_BY(crit_);
+  };
+
+  CreateSenderCall(Call::Config());
+
+  test::NullTransport transport;
+  CreateSendConfig(1, 0, &transport);
+
+  StartStopBitrateObserver encoder;
+  video_send_config_.encoder_settings.encoder = &encoder;
+  video_send_config_.encoder_settings.internal_source = true;
+
+  CreateVideoStreams();
+
+  EXPECT_TRUE(encoder.WaitForEncoderInit());
+  EXPECT_GT(encoder.GetBitrateKbps(), 0);
+  video_send_stream_->Start();
+  EXPECT_TRUE(encoder.WaitBitrateChanged());
+  EXPECT_GT(encoder.GetBitrateKbps(), 0);
+  video_send_stream_->Stop();
+  EXPECT_TRUE(encoder.WaitBitrateChanged());
+  EXPECT_EQ(0, encoder.GetBitrateKbps());
+  video_send_stream_->Start();
+  EXPECT_TRUE(encoder.WaitBitrateChanged());
+  EXPECT_GT(encoder.GetBitrateKbps(), 0);
+
+  DestroyStreams();
+}
+
 TEST_F(VideoSendStreamTest, CapturesTextureAndVideoFrames) {
   class FrameObserver : public rtc::VideoSinkInterface<VideoFrame> {
    public:

webrtc/video/vie_encoder.cc

@@ -10,9 +10,8 @@
 
 #include "webrtc/video/vie_encoder.h"
 
-#include <assert.h>
-
 #include <algorithm>
+#include <limits>
 
 #include "webrtc/base/checks.h"
 #include "webrtc/base/logging.h"
@@ -39,10 +38,9 @@ ViEEncoder::ViEEncoder(uint32_t number_of_cores,
       video_sender_(Clock::GetRealTimeClock(), this, this, this),
       stats_proxy_(stats_proxy),
       overuse_detector_(overuse_detector),
-      time_of_last_frame_activity_ms_(0),
+      time_of_last_frame_activity_ms_(std::numeric_limits<int64_t>::max()),
       encoder_config_(),
       last_observed_bitrate_bps_(0),
-      encoder_paused_(true),
       encoder_paused_and_dropped_frame_(false),
       module_process_thread_(module_process_thread),
       has_received_sli_(false),
@@ -62,16 +60,6 @@ ViEEncoder::~ViEEncoder() {
   module_process_thread_->DeRegisterModule(&video_sender_);
 }
 
-void ViEEncoder::Pause() {
-  rtc::CritScope lock(&data_cs_);
-  encoder_paused_ = true;
-}
-
-void ViEEncoder::Start() {
-  rtc::CritScope lock(&data_cs_);
-  encoder_paused_ = false;
-}
-
 int32_t ViEEncoder::RegisterExternalEncoder(webrtc::VideoEncoder* encoder,
                                             uint8_t pl_type,
                                             bool internal_source) {
@@ -90,9 +78,6 @@ void ViEEncoder::SetEncoder(const webrtc::VideoCodec& video_codec,
   RTC_CHECK_EQ(VPM_OK,
                vp_->SetTargetResolution(video_codec.width, video_codec.height,
                                         video_codec.maxFramerate));
-
-  // Cache codec before calling AddBitrateObserver (which calls OnBitrateUpdated
-  // that makes use of the number of simulcast streams configured).
   {
     rtc::CritScope lock(&data_cs_);
     encoder_config_ = video_codec;
@@ -101,6 +86,7 @@ void ViEEncoder::SetEncoder(const webrtc::VideoCodec& video_codec,
   bool success = video_sender_.RegisterSendCodec(
                      &video_codec, number_of_cores_,
                      static_cast<uint32_t>(max_data_payload_length)) == VCM_OK;
+
   if (!success) {
     LOG(LS_ERROR) << "Failed to configure encoder.";
     RTC_DCHECK(success);
@@ -127,9 +113,9 @@ void ViEEncoder::SetEncoder(const webrtc::VideoCodec& video_codec,
 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 the pacer queue has grown to large or the network is down,
+  // If the pacer queue has grown too large or the network is down,
   // last_observed_bitrate_bps_ will be 0.
-  return encoder_paused_ || video_suspended_ || last_observed_bitrate_bps_ == 0;
+  return video_suspended_ || last_observed_bitrate_bps_ == 0;
 }
 
 void ViEEncoder::TraceFrameDropStart() {
@@ -270,6 +256,16 @@ void ViEEncoder::OnBitrateUpdated(uint32_t bitrate_bps,
     last_observed_bitrate_bps_ = bitrate_bps;
     video_suspension_changed = video_suspended_ != video_is_suspended;
     video_suspended_ = video_is_suspended;
+    // Set |time_of_last_frame_activity_ms_| to now if this is the first time
+    // the encoder is supposed to produce encoded frames.
+    // TODO(perkj): Remove this hack. It is here to avoid a race that the
+    // encoder report that it has timed out before it has processed the first
+    // frame.
+    if (last_observed_bitrate_bps_ != 0 &&
+        time_of_last_frame_activity_ms_ ==
+            std::numeric_limits<int64_t>::max()) {
+      time_of_last_frame_activity_ms_ = rtc::TimeMillis();
+    }
   }
 
   if (stats_proxy_ && video_suspension_changed) {

webrtc/video/vie_encoder.h

@@ -47,8 +47,7 @@ class VideoEncoder;
 // 3. Call RegisterExternalEncoder if available.
 // 4. Call SetEncoder with the codec settings and the object that shall receive
 //    the encoded bit stream.
-// 5. Call Start.
+// 5. For each available raw video frame call EncodeVideoFrame.
-// 6. For each available raw video frame call EncodeVideoFrame.
 class ViEEncoder : public VideoEncoderRateObserver,
                    public EncodedImageCallback,
                    public VCMSendStatisticsCallback {
@@ -67,10 +66,6 @@ class ViEEncoder : public VideoEncoderRateObserver,
   // Returns the id of the owning channel.
   int Owner() const;
 
-  void Start();
-  // Drops incoming packets before they get to the encoder.
-  void Pause();
-
   // Codec settings.
   int32_t RegisterExternalEncoder(VideoEncoder* encoder,
                                   uint8_t pl_type,
@@ -134,7 +129,6 @@ class ViEEncoder : public VideoEncoderRateObserver,
   int64_t time_of_last_frame_activity_ms_ GUARDED_BY(data_cs_);
   VideoCodec encoder_config_ GUARDED_BY(data_cs_);
   uint32_t last_observed_bitrate_bps_ GUARDED_BY(data_cs_);
-  bool encoder_paused_ GUARDED_BY(data_cs_);
   bool encoder_paused_and_dropped_frame_ GUARDED_BY(data_cs_);
 
   ProcessThread* module_process_thread_;