Update encoder settings periodically, not only on new bandwidth estimate

Also moved actual update call to encoder thread, and tweaked frame rate
estimate to be less noisy.

This is a re-upload of https://review.webrtc.org/47249004

BUG=chromium:476469
R=stefan@webrtc.org

Review URL: https://codereview.webrtc.org/1180623005.

Cr-Commit-Position: refs/heads/master@{#9417}

webrtc/modules/video_coding/main/source/media_optimization.cc

@@ -631,8 +631,9 @@ void MediaOptimization::ProcessIncomingFrameRate(int64_t now) {
     }
   }
   if (num > 1) {
-    const int64_t diff = now - incoming_frame_times_[num - 1];
+    const int64_t diff =
-    incoming_frame_rate_ = 1.0;
+        incoming_frame_times_[0] - incoming_frame_times_[num - 1];
+    incoming_frame_rate_ = 0.0;  // No frame rate estimate available.
     if (diff > 0) {
       incoming_frame_rate_ = nr_of_frames * 1000.0f / static_cast<float>(diff);
     }

webrtc/modules/video_coding/main/source/media_optimization.h

@@ -79,6 +79,7 @@ class MediaOptimization {
   // Informs Media Optimization of encoded output.
   int32_t UpdateWithEncodedData(const EncodedImage& encoded_image);
 
+  // InputFrameRate 0 = no frame rate estimate available.
   uint32_t InputFrameRate();
   uint32_t SentFrameRate();
   uint32_t SentBitRate();

webrtc/modules/video_coding/main/source/video_coding_impl.h

@@ -93,6 +93,7 @@ class VideoSender {
   int32_t SetChannelParameters(uint32_t target_bitrate,  // bits/s.
                                uint8_t lossRate,
                                int64_t rtt);
+  int32_t UpdateEncoderParameters();
 
   int32_t RegisterTransportCallback(VCMPacketizationCallback* transport);
   int32_t RegisterSendStatisticsCallback(VCMSendStatisticsCallback* sendStats);
@@ -132,6 +133,15 @@ class VideoSender {
 
   VCMQMSettingsCallback* const qm_settings_callback_;
   VCMProtectionCallback* protection_callback_;
+
+  rtc::CriticalSection params_lock_;
+  struct EncoderParameters {
+    uint32_t target_bitrate;
+    uint8_t loss_rate;
+    int64_t rtt;
+    uint32_t input_frame_rate;
+    bool updated;
+  } encoder_params_ GUARDED_BY(params_lock_);
 };
 
 class VideoReceiver {

webrtc/modules/video_coding/main/source/video_sender.cc

@@ -42,6 +42,7 @@ VideoSender::VideoSender(Clock* clock,
       current_codec_(),
       qm_settings_callback_(qm_settings_callback),
       protection_callback_(nullptr) {
+  encoder_params_ = {0, 0, 0, 0, false};
   // Allow VideoSender to be created on one thread but used on another, post
   // construction. This is currently how this class is being used by at least
   // one external project (diffractor).
@@ -67,6 +68,14 @@ int32_t VideoSender::Process() {
     }
   }
 
+  {
+    rtc::CritScope cs(&params_lock_);
+    // Force an encoder parameters update, so that incoming frame rate is
+    // updated even if bandwidth hasn't changed.
+    encoder_params_.input_frame_rate = _mediaOpt.InputFrameRate();
+    encoder_params_.updated = true;
+  }
+
   return returnValue;
 }
 
@@ -216,27 +225,42 @@ int VideoSender::FrameRate(unsigned int* framerate) const {
 int32_t VideoSender::SetChannelParameters(uint32_t target_bitrate,
                                           uint8_t lossRate,
                                           int64_t rtt) {
-  // TODO(tommi,mflodman): This method is called on the network thread via the
+  uint32_t target_rate =
-  // OnNetworkChanged event (ViEEncoder::OnNetworkChanged). Could we instead
+      _mediaOpt.SetTargetRates(target_bitrate, lossRate, rtt,
-  // post the updated information to the encoding thread and not grab a lock
+                               protection_callback_, qm_settings_callback_);
-  // here?  This effectively means that the network thread will be blocked for
-  // as much as frame encoding period.
 
-  uint32_t target_rate = _mediaOpt.SetTargetRates(target_bitrate,
-                                                  lossRate,
-                                                  rtt,
-                                                  protection_callback_,
-                                                  qm_settings_callback_);
   uint32_t input_frame_rate = _mediaOpt.InputFrameRate();
 
+  rtc::CritScope cs(&params_lock_);
+  encoder_params_ =
+      EncoderParameters{target_rate, lossRate, rtt, input_frame_rate, true};
+
+  return VCM_OK;
+}
+
+int32_t VideoSender::UpdateEncoderParameters() {
+  EncoderParameters params;
+  {
+    rtc::CritScope cs(&params_lock_);
+    params = encoder_params_;
+    encoder_params_.updated = false;
+  }
+
+  if (!params.updated || params.target_bitrate == 0)
+    return VCM_OK;
+
   CriticalSectionScoped sendCs(_sendCritSect);
   int32_t ret = VCM_UNINITIALIZED;
   static_assert(VCM_UNINITIALIZED < 0, "VCM_UNINITIALIZED must be negative.");
 
+  if (params.input_frame_rate == 0) {
+    // No frame rate estimate available, use default.
+    params.input_frame_rate = current_codec_.maxFramerate;
+  }
   if (_encoder != nullptr) {
-    ret = _encoder->SetChannelParameters(lossRate, rtt);
+    ret = _encoder->SetChannelParameters(params.loss_rate, params.rtt);
     if (ret >= 0) {
-      ret = _encoder->SetRates(target_rate, input_frame_rate);
+      ret = _encoder->SetRates(params.target_bitrate, params.input_frame_rate);
     }
   }
   return ret;
@@ -300,6 +324,7 @@ void VideoSender::SetVideoProtection(bool enable,
 int32_t VideoSender::AddVideoFrame(const VideoFrame& videoFrame,
                                    const VideoContentMetrics* contentMetrics,
                                    const CodecSpecificInfo* codecSpecificInfo) {
+  UpdateEncoderParameters();
   CriticalSectionScoped cs(_sendCritSect);
   if (_encoder == nullptr) {
     return VCM_UNINITIALIZED;

webrtc/modules/video_coding/main/source/video_sender_unittest.cc

@@ -314,6 +314,20 @@ TEST_F(TestVideoSenderWithMockEncoder, TestIntraRequestsInternalCapture) {
   EXPECT_EQ(-1, sender_->IntraFrameRequest(-1));
 }
 
+TEST_F(TestVideoSenderWithMockEncoder, EncoderFramerateUpdatedViaProcess) {
+  sender_->SetChannelParameters(settings_.startBitrate, 0, 200);
+  const int64_t kRateStatsWindowMs = 2000;
+  const uint32_t kInputFps = 20;
+  int64_t start_time = clock_.TimeInMilliseconds();
+  while (clock_.TimeInMilliseconds() < start_time + kRateStatsWindowMs) {
+    AddFrame();
+    clock_.AdvanceTimeMilliseconds(1000 / kInputFps);
+  }
+  EXPECT_CALL(encoder_, SetRates(_, kInputFps)).Times(1).WillOnce(Return(0));
+  sender_->Process();
+  AddFrame();
+}
+
 class TestVideoSenderWithVp8 : public TestVideoSender {
  public:
   TestVideoSenderWithVp8()
@@ -354,14 +368,12 @@ class TestVideoSenderWithVp8 : public TestVideoSender {
       EXPECT_CALL(post_encode_callback_, Encoded(_, NULL, NULL))
           .WillOnce(Return(0));
       AddFrame();
-
       // SetChannelParameters needs to be called frequently to propagate
       // framerate from the media optimization into the encoder.
       // Note: SetChannelParameters fails if less than 2 frames are in the
       // buffer since it will fail to calculate the framerate.
       if (i != 0) {
-        EXPECT_EQ(VCM_OK,
+        EXPECT_EQ(VCM_OK, sender_->SetChannelParameters(
-                  sender_->SetChannelParameters(
                               available_bitrate_kbps_ * 1000, 0, 200));
       }
     }