Fix stats for encoder target bitrate when target rate is zero.

When the target bitrate is zero, currently  VideoSendStream.Stats.target_media_bitrate_bps show the last set rate before the target was set to zero.

BUG=webrtc::5687 b/29574845

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

webrtc/modules/video_coding/codec_database.cc

@@ -90,7 +90,6 @@ VCMExtDecoderMapItem::VCMExtDecoderMapItem(
       external_decoder_instance(external_decoder_instance) {}
 
 VCMCodecDataBase::VCMCodecDataBase(
-    VideoEncoderRateObserver* encoder_rate_observer,
     VCMEncodedFrameCallback* encoded_frame_callback)
     : number_of_cores_(0),
       max_payload_size_(kDefaultPayloadSize),
@@ -101,7 +100,6 @@ VCMCodecDataBase::VCMCodecDataBase(
       encoder_payload_type_(0),
       external_encoder_(nullptr),
       internal_source_(false),
-      encoder_rate_observer_(encoder_rate_observer),
       encoded_frame_callback_(encoded_frame_callback),
       ptr_decoder_(nullptr),
       dec_map_(),
@@ -245,9 +243,8 @@ bool VCMCodecDataBase::SetSendCodec(const VideoCodec* send_codec,
   DeleteEncoder();
   RTC_DCHECK_EQ(encoder_payload_type_, send_codec_.plType)
       << "Encoder not registered for payload type " << send_codec_.plType;
-  ptr_encoder_.reset(
+  ptr_encoder_.reset(new VCMGenericEncoder(
-      new VCMGenericEncoder(external_encoder_, encoder_rate_observer_,
+      external_encoder_, encoded_frame_callback_, internal_source_));
-                            encoded_frame_callback_, internal_source_));
   encoded_frame_callback_->SetInternalSource(internal_source_);
   if (ptr_encoder_->InitEncode(&send_codec_, number_of_cores_,
                                max_payload_size_) < 0) {

webrtc/modules/video_coding/codec_database.h

@@ -44,8 +44,7 @@ struct VCMExtDecoderMapItem {
 
 class VCMCodecDataBase {
  public:
-  VCMCodecDataBase(VideoEncoderRateObserver* encoder_rate_observer,
+  explicit VCMCodecDataBase(VCMEncodedFrameCallback* encoded_frame_callback);
-                   VCMEncodedFrameCallback* encoded_frame_callback);
   ~VCMCodecDataBase();
 
   // Sender Side
@@ -154,7 +153,6 @@ class VCMCodecDataBase {
   uint8_t encoder_payload_type_;
   VideoEncoder* external_encoder_;
   bool internal_source_;
-  VideoEncoderRateObserver* const encoder_rate_observer_;
   VCMEncodedFrameCallback* const encoded_frame_callback_;
   std::unique_ptr<VCMGenericEncoder> ptr_encoder_;
   VCMGenericDecoder* ptr_decoder_;

webrtc/modules/video_coding/generic_encoder.cc

@@ -23,11 +23,9 @@
 namespace webrtc {
 VCMGenericEncoder::VCMGenericEncoder(
     VideoEncoder* encoder,
-    VideoEncoderRateObserver* rate_observer,
     VCMEncodedFrameCallback* encoded_frame_callback,
     bool internal_source)
     : encoder_(encoder),
-      rate_observer_(rate_observer),
       vcm_encoded_frame_callback_(encoded_frame_callback),
       internal_source_(internal_source),
       encoder_params_({0, 0, 0, 0}),
@@ -102,10 +100,6 @@ void VCMGenericEncoder::SetEncoderParameters(const EncoderParameters& params) {
   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_) {
-      rate_observer_->OnSetRates(params.target_bitrate,
-                                 params.input_frame_rate);
-    }
   }
 }
 

webrtc/modules/video_coding/generic_encoder.h

@@ -59,7 +59,6 @@ class VCMGenericEncoder {
 
  public:
   VCMGenericEncoder(VideoEncoder* encoder,
-                    VideoEncoderRateObserver* rate_observer,
                     VCMEncodedFrameCallback* encoded_frame_callback,
                     bool internal_source);
   ~VCMGenericEncoder();
@@ -86,7 +85,6 @@ class VCMGenericEncoder {
   rtc::RaceChecker race_checker_;
 
   VideoEncoder* const encoder_ GUARDED_BY(race_checker_);
-  VideoEncoderRateObserver* const rate_observer_;
   VCMEncodedFrameCallback* const vcm_encoded_frame_callback_;
   const bool internal_source_;
   rtc::CriticalSection params_lock_;

webrtc/modules/video_coding/include/video_coding.h

@@ -72,21 +72,15 @@ class VideoCodingModule : public Module {
 
   enum ReceiverRobustness { kNone, kHardNack, kSoftNack, kReferenceSelection };
 
-  static VideoCodingModule* Create(
+  static VideoCodingModule* Create(Clock* clock, EventFactory* event_factory);
-      Clock* clock,
-      VideoEncoderRateObserver* encoder_rate_observer,
-      VCMQMSettingsCallback* qm_settings_callback);
 
   static VideoCodingModule* Create(
       Clock* clock,
-      VideoEncoderRateObserver* encoder_rate_observer,
       VCMQMSettingsCallback* qm_settings_callback,
       NackSender* nack_sender,
       KeyFrameRequestSender* keyframe_request_sender,
       EncodedImageCallback* pre_decode_image_callback);
 
-  static VideoCodingModule* Create(Clock* clock, EventFactory* event_factory);
-
   static VideoCodingModule* Create(
       Clock* clock,
       EventFactory* event_factory,

webrtc/modules/video_coding/include/video_coding_defines.h

@@ -126,12 +126,6 @@ class VCMProtectionCallback {
   virtual ~VCMProtectionCallback() {}
 };
 
-class VideoEncoderRateObserver {
- public:
-  virtual ~VideoEncoderRateObserver() {}
-  virtual void OnSetRates(uint32_t bitrate_bps, int framerate) = 0;
-};
-
 // Callback class used for telling the user about what frame type needed to
 // continue decoding.
 // Typically a key frame when the stream has been corrupted in some way.

webrtc/modules/video_coding/video_coding_impl.cc

@@ -73,12 +73,11 @@ class VideoCodingModuleImpl : public VideoCodingModule {
  public:
   VideoCodingModuleImpl(Clock* clock,
                         EventFactory* event_factory,
-                        VideoEncoderRateObserver* encoder_rate_observer,
                         NackSender* nack_sender,
                         KeyFrameRequestSender* keyframe_request_sender,
                         EncodedImageCallback* pre_decode_image_callback)
       : VideoCodingModule(),
-        sender_(clock, &post_encode_callback_, encoder_rate_observer, nullptr),
+        sender_(clock, &post_encode_callback_, nullptr),
         receiver_(clock,
                   event_factory,
                   pre_decode_image_callback,
@@ -263,29 +262,15 @@ void VideoCodingModule::Codec(VideoCodecType codecType, VideoCodec* codec) {
   VCMCodecDataBase::Codec(codecType, codec);
 }
 
-// Create method for current interface, will be removed when the
-// new jitter buffer is in place.
-VideoCodingModule* VideoCodingModule::Create(
-    Clock* clock,
-    VideoEncoderRateObserver* encoder_rate_observer,
-    VCMQMSettingsCallback* qm_settings_callback) {
-  return VideoCodingModule::Create(clock, encoder_rate_observer,
-                                   qm_settings_callback,
-                                   nullptr,   // NackSender
-                                   nullptr,   // KeyframeRequestSender
-                                   nullptr);  // Pre-decode image callback
-}
-
 // Create method for the new jitter buffer.
 VideoCodingModule* VideoCodingModule::Create(
     Clock* clock,
-    VideoEncoderRateObserver* encoder_rate_observer,
     VCMQMSettingsCallback* qm_settings_callback,
     NackSender* nack_sender,
     KeyFrameRequestSender* keyframe_request_sender,
     EncodedImageCallback* pre_decode_image_callback) {
-  return new VideoCodingModuleImpl(clock, nullptr, encoder_rate_observer,
+  return new VideoCodingModuleImpl(clock, nullptr, nack_sender,
-                                   nack_sender, keyframe_request_sender,
+                                   keyframe_request_sender,
                                    pre_decode_image_callback);
 }
 
@@ -306,7 +291,7 @@ VideoCodingModule* VideoCodingModule::Create(
     KeyFrameRequestSender* keyframe_request_sender) {
   assert(clock);
   assert(event_factory);
-  return new VideoCodingModuleImpl(clock, event_factory, nullptr, nack_sender,
+  return new VideoCodingModuleImpl(clock, event_factory, nack_sender,
                                    keyframe_request_sender, nullptr);
 }
 

webrtc/modules/video_coding/video_coding_impl.h

@@ -58,7 +58,6 @@ class VideoSender : public Module {
 
   VideoSender(Clock* clock,
               EncodedImageCallback* post_encode_callback,
-              VideoEncoderRateObserver* encoder_rate_observer,
               VCMSendStatisticsCallback* send_stats_callback);
 
   ~VideoSender();

webrtc/modules/video_coding/video_receiver.cc

@@ -45,7 +45,7 @@ VideoReceiver::VideoReceiver(Clock* clock,
       _scheduleKeyRequest(false),
       drop_frames_until_keyframe_(false),
       max_nack_list_size_(0),
-      _codecDataBase(nullptr, nullptr),
+      _codecDataBase(nullptr),
       pre_decode_image_callback_(pre_decode_image_callback),
       _receiveStatsTimer(1000, clock_),
       _retransmissionTimer(10, clock_),

webrtc/modules/video_coding/video_sender.cc

@@ -26,14 +26,13 @@ namespace vcm {
 
 VideoSender::VideoSender(Clock* clock,
                          EncodedImageCallback* post_encode_callback,
-                         VideoEncoderRateObserver* encoder_rate_observer,
                          VCMSendStatisticsCallback* send_stats_callback)
     : clock_(clock),
       _encoder(nullptr),
       _mediaOpt(clock_),
       _encodedFrameCallback(post_encode_callback, &_mediaOpt),
       send_stats_callback_(send_stats_callback),
-      _codecDataBase(encoder_rate_observer, &_encodedFrameCallback),
+      _codecDataBase(&_encodedFrameCallback),
       frame_dropper_enabled_(true),
       _sendStatsTimer(1000, clock_),
       current_codec_(),

webrtc/modules/video_coding/video_sender_unittest.cc

@@ -180,8 +180,7 @@ class TestVideoSender : public ::testing::Test {
   TestVideoSender() : clock_(1000), encoded_frame_callback_(&clock_) {}
 
   void SetUp() override {
-    sender_.reset(
+    sender_.reset(new VideoSender(&clock_, &encoded_frame_callback_, nullptr));
-        new VideoSender(&clock_, &encoded_frame_callback_, nullptr, nullptr));
   }
 
   void AddFrame() {

webrtc/video/end_to_end_tests.cc

@@ -2473,7 +2473,14 @@ TEST_F(EndToEndTest, ReportsSetEncoderRates) {
     void PerformTest() override {
       ASSERT_TRUE(Wait())
           << "Timed out while waiting for encoder SetRates() call.";
-      // Wait for GetStats to report a corresponding bitrate.
+      WaitForEncoderTargetBitrateMatchStats();
+      send_stream_->Stop();
+      WaitForStatsReportZeroTargetBitrate();
+      send_stream_->Start();
+      WaitForEncoderTargetBitrateMatchStats();
+    }
+
+    void WaitForEncoderTargetBitrateMatchStats() {
       for (int i = 0; i < kDefaultTimeoutMs; ++i) {
         VideoSendStream::Stats stats = send_stream_->GetStats();
         {
@@ -2489,6 +2496,16 @@ TEST_F(EndToEndTest, ReportsSetEncoderRates) {
           << "Timed out waiting for stats reporting the currently set bitrate.";
     }
 
+    void WaitForStatsReportZeroTargetBitrate() {
+      for (int i = 0; i < kDefaultTimeoutMs; ++i) {
+        if (send_stream_->GetStats().target_media_bitrate_bps == 0) {
+          return;
+        }
+        SleepMs(1);
+      }
+      FAIL() << "Timed out waiting for stats reporting zero bitrate.";
+    }
+
    private:
     rtc::CriticalSection crit_;
     VideoSendStream* send_stream_;

webrtc/video/send_statistics_proxy.cc

@@ -418,7 +418,7 @@ void SendStatisticsProxy::OnInactiveSsrc(uint32_t ssrc) {
   stats->width = 0;
 }
 
-void SendStatisticsProxy::OnSetRates(uint32_t bitrate_bps, int framerate) {
+void SendStatisticsProxy::OnSetEncoderTargetRate(uint32_t bitrate_bps) {
   rtc::CritScope lock(&crit_);
   stats_.target_media_bitrate_bps = bitrate_bps;
 }

webrtc/video/send_statistics_proxy.h

@@ -36,7 +36,6 @@ class SendStatisticsProxy : public CpuOveruseMetricsObserver,
                             public StreamDataCountersCallback,
                             public BitrateStatisticsObserver,
                             public FrameCountObserver,
-                            public VideoEncoderRateObserver,
                             public SendSideDelayObserver {
  public:
   static const int kStatsTimeoutMs;
@@ -63,8 +62,8 @@ class SendStatisticsProxy : public CpuOveruseMetricsObserver,
   // how stats are collected.
   void SetContentType(VideoEncoderConfig::ContentType content_type);
 
-  // Implements VideoEncoderRateObserver.
+  // Used to update the encoder target rate.
-  void OnSetRates(uint32_t bitrate_bps, int framerate) override;
+  void OnSetEncoderTargetRate(uint32_t bitrate_bps);
 
   // Implements CpuOveruseMetricsObserver.
   void OnEncodedFrameTimeMeasured(int encode_time_ms,

webrtc/video/video_send_stream.cc

@@ -396,6 +396,7 @@ VideoSendStream::VideoSendStream(
       encoder_thread_(EncoderThreadFunction, this, "EncoderThread"),
       encoder_wakeup_event_(false, false),
       stop_encoder_thread_(0),
+      encoder_max_bitrate_bps_(0),
       state_(State::kStopped),
       overuse_detector_(
           Clock::GetRealTimeClock(),
@@ -631,6 +632,7 @@ void VideoSendStream::EncoderProcess() {
       } else if (*pending_state_change == State::kStopped) {
         bitrate_allocator_->RemoveObserver(this);
         vie_encoder_.OnBitrateUpdated(0, 0, 0);
+        stats_proxy_.OnSetEncoderTargetRate(0);
         state_ = State::kStopped;
         LOG_F(LS_INFO) << "Encoder stopped.";
       }
@@ -684,6 +686,7 @@ void VideoSendStream::ReconfigureVideoEncoder(
       config_.encoder_settings.payload_type);
   {
     rtc::CritScope lock(&encoder_settings_crit_);
+    encoder_max_bitrate_bps_ = video_codec.maxBitrate * 1000;
     pending_encoder_settings_.reset(new EncoderSettings({video_codec, config}));
   }
   encoder_wakeup_event_.Set();
@@ -875,9 +878,18 @@ uint32_t VideoSendStream::OnBitrateUpdated(uint32_t bitrate_bps,
   uint32_t encoder_target_rate_bps =
       protection_bitrate_calculator_.SetTargetRates(
           bitrate_bps, stats_proxy_.GetSendFrameRate(), fraction_loss, rtt);
-  vie_encoder_.OnBitrateUpdated(encoder_target_rate_bps, fraction_loss, rtt);
 
-  return bitrate_bps - encoder_target_rate_bps;
+  uint32_t protection_bitrate = bitrate_bps - encoder_target_rate_bps;
+  {
+    // Limit the target bitrate to the configured max bitrate.
+    rtc::CritScope lock(&encoder_settings_crit_);
+    encoder_target_rate_bps =
+        std::min(encoder_max_bitrate_bps_, encoder_target_rate_bps);
+  }
+  vie_encoder_.OnBitrateUpdated(encoder_target_rate_bps, fraction_loss, rtt);
+  stats_proxy_.OnSetEncoderTargetRate(encoder_target_rate_bps);
+
+  return protection_bitrate;
 }
 
 int VideoSendStream::ProtectionRequest(const FecProtectionParams* delta_params,

webrtc/video/video_send_stream.h

@@ -138,6 +138,7 @@ class VideoSendStream : public webrtc::VideoSendStream,
   rtc::CriticalSection encoder_settings_crit_;
   std::unique_ptr<EncoderSettings> pending_encoder_settings_
       GUARDED_BY(encoder_settings_crit_);
+  uint32_t encoder_max_bitrate_bps_ GUARDED_BY(encoder_settings_crit_);
 
   enum class State {
     kStopped,  // VideoSendStream::Start has not yet been called.

webrtc/video/vie_encoder.cc

@@ -35,7 +35,7 @@ ViEEncoder::ViEEncoder(uint32_t number_of_cores,
     : number_of_cores_(number_of_cores),
       sink_(sink),
       vp_(VideoProcessing::Create()),
-      video_sender_(Clock::GetRealTimeClock(), this, this, this),
+      video_sender_(Clock::GetRealTimeClock(), this, this),
       stats_proxy_(stats_proxy),
       overuse_detector_(overuse_detector),
       time_of_last_frame_activity_ms_(std::numeric_limits<int64_t>::max()),
@@ -193,11 +193,6 @@ int64_t ViEEncoder::time_of_last_frame_activity_ms() {
   return time_of_last_frame_activity_ms_;
 }
 
-void ViEEncoder::OnSetRates(uint32_t bitrate_bps, int framerate) {
-  if (stats_proxy_)
-    stats_proxy_->OnSetRates(bitrate_bps, framerate);
-}
-
 int32_t ViEEncoder::Encoded(const EncodedImage& encoded_image,
                             const CodecSpecificInfo* codec_specific_info,
                             const RTPFragmentationHeader* fragmentation) {

webrtc/video/vie_encoder.h

@@ -48,8 +48,7 @@ class VideoEncoder;
 // 4. Call SetEncoder with the codec settings and the object that shall receive
 //    the encoded bit stream.
 // 5. For each available raw video frame call EncodeVideoFrame.
-class ViEEncoder : public VideoEncoderRateObserver,
+class ViEEncoder : public EncodedImageCallback,
-                   public EncodedImageCallback,
                    public VCMSendStatisticsCallback {
  public:
   friend class ViEBitrateObserver;
@@ -81,12 +80,6 @@ class ViEEncoder : public VideoEncoderRateObserver,
   // an encoded frame.
   int64_t time_of_last_frame_activity_ms();
 
-  // Implements VideoEncoderRateObserver.
-  // TODO(perkj): Refactor VideoEncoderRateObserver. This is only used for
-  // stats. The stats should be set in VideoSendStream instead.
-  // |bitrate_bps| is the target bitrate and |framerate| is the input frame
-  // rate so it has nothing to do with the actual encoder.
-  void OnSetRates(uint32_t bitrate_bps, int framerate) override;
 
   // Implements EncodedImageCallback.
   int32_t Encoded(const EncodedImage& encoded_image,