Remove redudant encoder rate calls.

Moves EncoderParameters update checks into GenericEncoder before calling
SetRates/SetChannelParameters as applicable.

Also removes CodecConfigParameters as a bonus.

BUG=
R=stefan@webrtc.org
TBR=mflodman@webrtc.org

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

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

webrtc/modules/video_coding/codecs/i420/include/i420.h

@@ -73,10 +73,6 @@ class I420Encoder : public VideoEncoder {
     return WEBRTC_VIDEO_CODEC_OK;
   }
 
-  int CodecConfigParameters(uint8_t* /*buffer*/, int /*size*/) override {
-    return WEBRTC_VIDEO_CODEC_OK;
-  }
-
   void OnDroppedFrame() override {}
 
  private:

webrtc/modules/video_coding/codecs/interface/mock/mock_video_codec_interface.h

@@ -43,8 +43,6 @@ class MockVideoEncoder : public VideoEncoder {
   MOCK_METHOD2(SetChannelParameters, int32_t(uint32_t packetLoss, int64_t rtt));
   MOCK_METHOD2(SetRates, int32_t(uint32_t newBitRate, uint32_t frameRate));
   MOCK_METHOD1(SetPeriodicKeyFrames, int32_t(bool enable));
-  MOCK_METHOD2(CodecConfigParameters,
-               int32_t(uint8_t* /*buffer*/, int32_t));
 };
 
 class MockDecodedImageCallback : public DecodedImageCallback {
@@ -69,8 +67,6 @@ class MockVideoDecoder : public VideoDecoder {
                int32_t(DecodedImageCallback* callback));
   MOCK_METHOD0(Release, int32_t());
   MOCK_METHOD0(Reset, int32_t());
-  MOCK_METHOD2(SetCodecConfigParameters,
-               int32_t(const uint8_t* /*buffer*/, int32_t));
   MOCK_METHOD0(Copy, VideoDecoder*());
 };
 

webrtc/modules/video_coding/main/interface/video_coding.h

@@ -185,16 +185,6 @@ public:
                                             uint8_t payloadType,
                                             bool internalSource = false) = 0;
 
-    // API to get codec config parameters to be sent out-of-band to a receiver.
-    //
-    // Input:
-    //      - buffer          : Memory where the codec config parameters should be written.
-    //      - size            : Size of the memory available.
-    //
-    // Return value      : Number of bytes written, on success.
-    //                     < 0,                     on error.
-    virtual int32_t CodecConfigParameters(uint8_t* buffer, int32_t size) = 0;
-
     // API to get currently configured encoder target bitrate in bits/s.
     //
     // Return value      : 0,   on success.

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

@@ -94,12 +94,10 @@ VCMGenericEncoder::VCMGenericEncoder(VideoEncoder* encoder,
     : encoder_(encoder),
       rate_observer_(rate_observer),
       vcm_encoded_frame_callback_(nullptr),
-      bit_rate_(0),
-      frame_rate_(0),
+      encoder_params_({0, 0, 0, 0}),
       internal_source_(internalSource),
       rotation_(kVideoRotation_0),
-      is_screenshare_(false) {
-}
+      is_screenshare_(false) {}
 
 VCMGenericEncoder::~VCMGenericEncoder()
 {
@@ -108,9 +106,8 @@ VCMGenericEncoder::~VCMGenericEncoder()
 int32_t VCMGenericEncoder::Release()
 {
     {
-      rtc::CritScope lock(&rates_lock_);
-      bit_rate_ = 0;
-      frame_rate_ = 0;
+      rtc::CritScope lock(&params_lock_);
+      encoder_params_ = {0, 0, 0, 0};
       vcm_encoded_frame_callback_ = nullptr;
     }
 
@@ -123,9 +120,9 @@ VCMGenericEncoder::InitEncode(const VideoCodec* settings,
                               size_t maxPayloadSize)
 {
     {
-      rtc::CritScope lock(&rates_lock_);
-      bit_rate_ = settings->startBitrate * 1000;
-      frame_rate_ = settings->maxFramerate;
+      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;
@@ -162,54 +159,34 @@ int32_t VCMGenericEncoder::Encode(const VideoFrame& inputFrame,
   return result;
 }
 
-int32_t
-VCMGenericEncoder::SetChannelParameters(int32_t packetLoss, int64_t rtt)
-{
-    return encoder_->SetChannelParameters(packetLoss, rtt);
-}
-
-int32_t
-VCMGenericEncoder::SetRates(uint32_t newBitRate, uint32_t frameRate)
-{
-    uint32_t target_bitrate_kbps = (newBitRate + 500) / 1000;
-    int32_t ret = encoder_->SetRates(target_bitrate_kbps, frameRate);
-    if (ret < 0)
-    {
-        return ret;
+void VCMGenericEncoder::SetEncoderParameters(const EncoderParameters& params) {
+  bool channel_parameters_have_changed;
+  bool rates_have_changed;
+  {
+    rtc::CritScope lock(&params_lock_);
+    channel_parameters_have_changed =
+        params.loss_rate != encoder_params_.loss_rate ||
+        params.rtt != encoder_params_.rtt;
+    rates_have_changed =
+        params.target_bitrate != encoder_params_.target_bitrate ||
+        params.input_frame_rate != encoder_params_.input_frame_rate;
+    encoder_params_ = params;
+  }
+  if (channel_parameters_have_changed)
+    encoder_->SetChannelParameters(params.loss_rate, params.rtt);
+  if (rates_have_changed) {
+    uint32_t target_bitrate_kbps = (params.target_bitrate + 500) / 1000;
+    encoder_->SetRates(target_bitrate_kbps, params.input_frame_rate);
+    if (rate_observer_ != nullptr) {
+      rate_observer_->OnSetRates(params.target_bitrate,
+                                 params.input_frame_rate);
     }
-
-    {
-      rtc::CritScope lock(&rates_lock_);
-      bit_rate_ = newBitRate;
-      frame_rate_ = frameRate;
-    }
-
-    if (rate_observer_ != nullptr)
-      rate_observer_->OnSetRates(newBitRate, frameRate);
-    return VCM_OK;
+  }
 }
 
-int32_t
-VCMGenericEncoder::CodecConfigParameters(uint8_t* buffer, int32_t size)
-{
-    int32_t ret = encoder_->CodecConfigParameters(buffer, size);
-    if (ret < 0)
-    {
-        return ret;
-    }
-    return ret;
-}
-
-uint32_t VCMGenericEncoder::BitRate() const
-{
-    rtc::CritScope lock(&rates_lock_);
-    return bit_rate_;
-}
-
-uint32_t VCMGenericEncoder::FrameRate() const
-{
-    rtc::CritScope lock(&rates_lock_);
-    return frame_rate_;
+EncoderParameters VCMGenericEncoder::GetEncoderParameters() const {
+  rtc::CritScope lock(&params_lock_);
+  return encoder_params_;
 }
 
 int32_t

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

@@ -26,6 +26,13 @@ namespace media_optimization {
 class MediaOptimization;
 }  // namespace media_optimization
 
+struct EncoderParameters {
+  uint32_t target_bitrate;
+  uint8_t loss_rate;
+  int64_t rtt;
+  uint32_t input_frame_rate;
+};
+
 /*************************************/
 /* VCMEncodeFrameCallback class     */
 /***********************************/
@@ -102,33 +109,16 @@ public:
     int32_t Encode(const VideoFrame& inputFrame,
                    const CodecSpecificInfo* codecSpecificInfo,
                    const std::vector<FrameType>& frameTypes);
-    /**
-    * Set new target bitrate (bits/s) and framerate.
-    * Return Value: new bit rate if OK, otherwise <0s.
-    */
-    // TODO(tommi): We could replace BitRate and FrameRate below with a GetRates
-    // method that matches SetRates. For fetching current rates, we'd then only
-    // grab the lock once instead of twice.
-    int32_t SetRates(uint32_t target_bitrate, uint32_t frameRate);
-    /**
-    * Set a new packet loss rate and a new round-trip time in milliseconds.
-    */
-    int32_t SetChannelParameters(int32_t packetLoss, int64_t rtt);
-    int32_t CodecConfigParameters(uint8_t* buffer, int32_t size);
+
+    void SetEncoderParameters(const EncoderParameters& params);
     /**
     * Register a transport callback which will be called to deliver the encoded
     * buffers
     */
     int32_t RegisterEncodeCallback(
         VCMEncodedFrameCallback* VCMencodedFrameCallback);
-    /**
-    * Get encoder bit rate
-    */
-    uint32_t BitRate() const;
-     /**
-    * Get encoder frame rate
-    */
-    uint32_t FrameRate() const;
+
+    EncoderParameters GetEncoderParameters() const;
 
     int32_t SetPeriodicKeyFrames(bool enable);
 
@@ -146,10 +136,9 @@ private:
     VideoEncoder* const encoder_;
     VideoEncoderRateObserver* const rate_observer_;
     VCMEncodedFrameCallback*  vcm_encoded_frame_callback_;
-    uint32_t bit_rate_;
-    uint32_t frame_rate_;
+    EncoderParameters encoder_params_ GUARDED_BY(params_lock_);
     const bool internal_source_;
-    mutable rtc::CriticalSection rates_lock_;
+    mutable rtc::CriticalSection params_lock_;
     VideoRotation rotation_;
     bool is_screenshare_;
 }; // end of VCMGenericEncoder class

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

@@ -133,10 +133,6 @@ class VideoCodingModuleImpl : public VideoCodingModule {
         externalEncoder, payloadType, internalSource);
   }
 
-  int32_t CodecConfigParameters(uint8_t* buffer, int32_t size) override {
-    return sender_->CodecConfigParameters(buffer, size);
-  }
-
   int Bitrate(unsigned int* bitrate) const override {
     return sender_->Bitrate(bitrate);
   }

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

@@ -86,7 +86,6 @@ class VideoSender {
                                   uint8_t payloadType,
                                   bool internalSource);
 
-  int32_t CodecConfigParameters(uint8_t* buffer, int32_t size) const;
   int Bitrate(unsigned int* bitrate) const;
   int FrameRate(unsigned int* framerate) const;
 
@@ -113,14 +112,6 @@ class VideoSender {
   int32_t Process();
 
  private:
-  struct EncoderParameters {
-    uint32_t target_bitrate;
-    uint8_t loss_rate;
-    int64_t rtt;
-    uint32_t input_frame_rate;
-    bool updated;
-  };
-
   void SetEncoderParameters(EncoderParameters params)
       EXCLUSIVE_LOCKS_REQUIRED(send_crit_);
 

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

@@ -40,8 +40,8 @@ VideoSender::VideoSender(Clock* clock,
       _sendStatsTimer(1000, clock_),
       current_codec_(),
       qm_settings_callback_(qm_settings_callback),
-      protection_callback_(nullptr) {
-  encoder_params_ = {0, 0, 0, 0, false};
+      protection_callback_(nullptr),
+      encoder_params_({0, 0, 0, 0}) {
   // 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).
@@ -70,7 +70,6 @@ int32_t VideoSender::Process() {
     // 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;
@@ -180,27 +179,15 @@ int32_t VideoSender::RegisterExternalEncoder(VideoEncoder* externalEncoder,
   return 0;
 }
 
-// Get codec config parameters
-int32_t VideoSender::CodecConfigParameters(uint8_t* buffer,
-                                           int32_t size) const {
-  rtc::CritScope lock(&send_crit_);
-  if (_encoder != nullptr) {
-    return _encoder->CodecConfigParameters(buffer, size);
-  }
-  return VCM_UNINITIALIZED;
-}
-
 // Get encode bitrate
 int VideoSender::Bitrate(unsigned int* bitrate) const {
   RTC_DCHECK(main_thread_.CalledOnValidThread());
   // Since we're running on the thread that's the only thread known to modify
   // the value of _encoder, we don't need to grab the lock here.
 
-  // return the bit rate which the encoder is set to
-  if (!_encoder) {
+  if (!_encoder)
     return VCM_UNINITIALIZED;
-  }
-  *bitrate = _encoder->BitRate();
+  *bitrate = _encoder->GetEncoderParameters().target_bitrate;
   return 0;
 }
 
@@ -210,11 +197,10 @@ int VideoSender::FrameRate(unsigned int* framerate) const {
   // Since we're running on the thread that's the only thread known to modify
   // the value of _encoder, we don't need to grab the lock here.
 
-  // input frame rate, not compensated
-  if (!_encoder) {
+  if (!_encoder)
     return VCM_UNINITIALIZED;
-  }
-  *framerate = _encoder->FrameRate();
+
+  *framerate = _encoder->GetEncoderParameters().input_frame_rate;
   return 0;
 }
 
@@ -228,25 +214,21 @@ int32_t VideoSender::SetChannelParameters(uint32_t target_bitrate,
   uint32_t input_frame_rate = _mediaOpt.InputFrameRate();
 
   rtc::CritScope cs(&params_lock_);
-  encoder_params_ =
-      EncoderParameters{target_rate, lossRate, rtt, input_frame_rate, true};
+  encoder_params_ = {target_rate, lossRate, rtt, input_frame_rate};
 
   return VCM_OK;
 }
 
 void VideoSender::SetEncoderParameters(EncoderParameters params) {
-  if (!params.updated || params.target_bitrate == 0)
+  if (params.target_bitrate == 0)
     return;
 
   if (params.input_frame_rate == 0) {
     // No frame rate estimate available, use default.
     params.input_frame_rate = current_codec_.maxFramerate;
   }
-  if (_encoder != nullptr) {
-    _encoder->SetChannelParameters(params.loss_rate, params.rtt);
-    _encoder->SetRates(params.target_bitrate, params.input_frame_rate);
-  }
-  return;
+  if (_encoder != nullptr)
+    _encoder->SetEncoderParameters(params);
 }
 
 int32_t VideoSender::RegisterTransportCallback(
@@ -304,13 +286,11 @@ int32_t VideoSender::AddVideoFrame(const VideoFrame& videoFrame,
   {
     rtc::CritScope lock(&params_lock_);
     encoder_params = encoder_params_;
-    encoder_params_.updated = false;
   }
   rtc::CritScope lock(&send_crit_);
-  SetEncoderParameters(encoder_params);
-  if (_encoder == nullptr) {
+  if (_encoder == nullptr)
     return VCM_UNINITIALIZED;
-  }
+  SetEncoderParameters(encoder_params);
   // TODO(holmer): Add support for dropping frames per stream. Currently we
   // only have one frame dropper for all streams.
   if (_nextFrameTypes[0] == kEmptyFrame) {

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

@@ -318,7 +318,7 @@ TEST_F(TestVideoSenderWithMockEncoder, TestIntraRequestsInternalCapture) {
 }
 
 TEST_F(TestVideoSenderWithMockEncoder, EncoderFramerateUpdatedViaProcess) {
-  sender_->SetChannelParameters(settings_.startBitrate, 0, 200);
+  sender_->SetChannelParameters(settings_.startBitrate * 1000, 0, 200);
   const int64_t kRateStatsWindowMs = 2000;
   const uint32_t kInputFps = 20;
   int64_t start_time = clock_.TimeInMilliseconds();
@@ -331,6 +331,39 @@ TEST_F(TestVideoSenderWithMockEncoder, EncoderFramerateUpdatedViaProcess) {
   AddFrame();
 }
 
+TEST_F(TestVideoSenderWithMockEncoder,
+       NoRedundantSetChannelParameterOrSetRatesCalls) {
+  const uint8_t kLossRate = 4;
+  const uint8_t kRtt = 200;
+  const int64_t kRateStatsWindowMs = 2000;
+  const uint32_t kInputFps = 20;
+  int64_t start_time = clock_.TimeInMilliseconds();
+  // Expect initial call to SetChannelParameters. Rates are initialized through
+  // InitEncode and expects no additional call before the framerate (or bitrate)
+  // updates.
+  EXPECT_CALL(encoder_, SetChannelParameters(kLossRate, kRtt))
+      .Times(1)
+      .WillOnce(Return(0));
+  sender_->SetChannelParameters(settings_.startBitrate * 1000, kLossRate, kRtt);
+  while (clock_.TimeInMilliseconds() < start_time + kRateStatsWindowMs) {
+    AddFrame();
+    clock_.AdvanceTimeMilliseconds(1000 / kInputFps);
+  }
+  // After process, input framerate should be updated but not ChannelParameters
+  // as they are the same as before.
+  EXPECT_CALL(encoder_, SetRates(_, kInputFps)).Times(1).WillOnce(Return(0));
+  sender_->Process();
+  AddFrame();
+  // Call to SetChannelParameters with changed bitrate should call encoder
+  // SetRates but not encoder SetChannelParameters (that are unchanged).
+  EXPECT_CALL(encoder_, SetRates(2 * settings_.startBitrate, kInputFps))
+      .Times(1)
+      .WillOnce(Return(0));
+  sender_->SetChannelParameters(2 * settings_.startBitrate * 1000, kLossRate,
+                                kRtt);
+  AddFrame();
+}
+
 class TestVideoSenderWithVp8 : public TestVideoSender {
  public:
   TestVideoSenderWithVp8()