Initial rate allocation should not use fps = 0

webrtc/modules/video_coding/video_sender.cc

 /*
  *  Copyright (c) 2013 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 19 matching lines @@
 VideoSender::VideoSender(Clock* clock,
                          EncodedImageCallback* post_encode_callback,
                          VCMSendStatisticsCallback* send_stats_callback)
     : clock_(clock),
       _encoder(nullptr),
       _mediaOpt(clock_),
       _encodedFrameCallback(post_encode_callback, &_mediaOpt),
       send_stats_callback_(send_stats_callback),
       _codecDataBase(&_encodedFrameCallback),
       frame_dropper_enabled_(true),
-      _sendStatsTimer(1000, clock_),
+      _sendStatsTimer(VCMProcessTimer::kDefaultProcessIntervalMs, clock_),
       current_codec_(),
       encoder_params_({BitrateAllocation(), 0, 0, 0}),
+      has_received_frame_(false),
       encoder_has_internal_source_(false),
       next_frame_types_(1, kVideoFrameDelta) {
   _mediaOpt.Reset();
   // 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).
   sequenced_checker_.Detach();
 }
 
 VideoSender::~VideoSender() {}
 
 void VideoSender::Process() {
   if (_sendStatsTimer.TimeUntilProcess() == 0) {
     // |_sendStatsTimer.Processed()| must be called. Otherwise
     // VideoSender::Process() will be called in an infinite loop.
     _sendStatsTimer.Processed();
     if (send_stats_callback_) {
       uint32_t bitRate = _mediaOpt.SentBitRate();
       uint32_t frameRate = _mediaOpt.SentFrameRate();
       send_stats_callback_->SendStatistics(bitRate, frameRate);
     }
   }
 
   {
     rtc::CritScope cs(&params_crit_);
     // 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();

[danilchap, 2016/11/21 12:45:42]

shouldn't you adjust frame_rate here too?
if not, may be avoid setting maxFramerate to encoder_params_.input_frame_rate
below (only pass maxFramerate to bitrate allocator)

[sprang_webrtc, 2016/11/21 13:01:20]

Looking close, SetEncoderParameters() actually treats 0 as invalid and caps to
codec.maxFramerate before calling the encoder, so I'll just simplify this a bit.

   }
 }
 
 int64_t VideoSender::TimeUntilNextProcess() {
   return _sendStatsTimer.TimeUntilProcess();
 }
 
 // Register the send codec to be used.
 int32_t VideoSender::RegisterSendCodec(const VideoCodec* sendCodec,
                                        uint32_t numberOfCores,
@@ skipping 38 matching lines @@
   if (disable_frame_dropper) {
     _mediaOpt.EnableFrameDropper(false);
   } else if (frame_dropper_enabled_) {
     _mediaOpt.EnableFrameDropper(true);
   }
   {
     rtc::CritScope cs(&params_crit_);
     next_frame_types_.clear();
     next_frame_types_.resize(VCM_MAX(sendCodec->numberOfSimulcastStreams, 1),
                              kVideoFrameKey);
+    has_received_frame_ = false;
     // Cache InternalSource() to have this available from IntraFrameRequest()
     // without having to acquire encoder_crit_ (avoid blocking on encoder use).
     encoder_has_internal_source_ = _encoder->InternalSource();
   }
 
   LOG(LS_VERBOSE) << " max bitrate " << sendCodec->maxBitrate
                   << " start bitrate " << sendCodec->startBitrate
                   << " max frame rate " << sendCodec->maxFramerate
                   << " max payload size " << maxPayloadSize;
   _mediaOpt.SetEncodingData(sendCodec->maxBitrate * 1000,
@@ skipping 53 matching lines @@
   return 0;
 }
 
 EncoderParameters VideoSender::UpdateEncoderParameters(
     const EncoderParameters& params,
     VideoBitrateAllocator* bitrate_allocator,
     uint32_t target_bitrate_bps) {
   uint32_t video_target_rate_bps = _mediaOpt.SetTargetRates(
       target_bitrate_bps, params.loss_rate, params.rtt);
   uint32_t input_frame_rate = _mediaOpt.InputFrameRate();
+  if (input_frame_rate > 0) {
+    has_received_frame_ = true;

[danilchap, 2016/11/21 12:45:42]

variable name make impression it should be set to true in AddVideoFrame.
May be a different name can explain better why it is set here:
has_calculated_framerate_
had_(nonzero_)framerate_
!use_fake_framerate_

[sprang_webrtc, 2016/11/21 13:01:20]

Ack, but removed instead.

+  } else if (input_frame_rate == 0 && !has_received_frame_) {

[danilchap, 2016/11/21 12:45:42]

This mean input_frame_rate ~= 0 mid call is a valid number?
can it be valid from the start of the call?

[sprang_webrtc, 2016/11/21 13:01:20]

My impression was that 0 could be a valid framerate during the call. In a
follow-up CL I would like to clean up media opts and remove that rate tracking,
since it's really poorly implemented, and reuse the RateStatistics class
instead, which returns an Optional with no value if there is no reliable value
available.

+    input_frame_rate = current_codec_.maxFramerate;
+  }
+
   BitrateAllocation bitrate_allocation;
   if (bitrate_allocator) {
     bitrate_allocation = bitrate_allocator->GetAllocation(video_target_rate_bps,
                                                           input_frame_rate);
   } else {
     DefaultVideoBitrateAllocator default_allocator(current_codec_);
     bitrate_allocation = default_allocator.GetAllocation(video_target_rate_bps,
                                                          input_frame_rate);
   }
 
@@ skipping 11 matching lines @@
 }
 
 int32_t VideoSender::SetChannelParameters(
     uint32_t target_bitrate_bps,
     uint8_t lossRate,
     int64_t rtt,
     VideoBitrateAllocator* bitrate_allocator) {
   EncoderParameters encoder_params;
   encoder_params.loss_rate = lossRate;
   encoder_params.rtt = rtt;
-  encoder_params = UpdateEncoderParameters(encoder_params, bitrate_allocator,
-                                           target_bitrate_bps);
 
   bool encoder_has_internal_source;
   {
     rtc::CritScope cs(&params_crit_);
-    encoder_params_ = encoder_params;
+    encoder_params_ = UpdateEncoderParameters(encoder_params, bitrate_allocator,
+                                              target_bitrate_bps);
+    encoder_params = encoder_params_;

[danilchap, 2016/11/21 12:45:42]

may be other way around make slightly more sense:
first prepare local variable encoder_params,
then assign it to member variable.

[sprang_webrtc, 2016/11/21 13:01:20]

Ack, but reverted instead.

     encoder_has_internal_source = encoder_has_internal_source_;
   }
 
   // For encoders with internal sources, we need to tell the encoder directly,
   // instead of waiting for an AddVideoFrame that will never come (internal
   // source encoders don't get input frames).
   if (encoder_has_internal_source) {
     rtc::CritScope cs(&encoder_crit_);
     if (_encoder) {
       SetEncoderParameters(encoder_params, encoder_has_internal_source);
@@ skipping 135 matching lines @@
 }
 
 int32_t VideoSender::EnableFrameDropper(bool enable) {
   rtc::CritScope lock(&encoder_crit_);
   frame_dropper_enabled_ = enable;
   _mediaOpt.EnableFrameDropper(enable);
   return VCM_OK;
 }
 }  // namespace vcm
 }  // namespace webrtc