Refactor NACK bitrate allocation

webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc

Index: webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc
diff --git a/webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc b/webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc
index 3affb318aa70428e2a20489abf14d1cffdd3f397..59b5e784499eec384c1c2382e2ffa8a5edb8e47f 100644
--- a/webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc
+++ b/webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc
@@ -31,6 +31,7 @@ enum { REDForFECHeaderLength = 1 };
 
 RTPSenderVideo::RTPSenderVideo(Clock* clock, RTPSenderInterface* rtpSender)
     : _rtpSender(*rtpSender),
+      clock_(clock),
       _videoType(kRtpVideoGeneric),
       _retransmissionSettings(kRetransmitBaseLayer),
       // Generic FEC
@@ -41,8 +42,8 @@ RTPSenderVideo::RTPSenderVideo(Clock* clock, RTPSenderInterface* rtpSender)
       delta_fec_params_(),
       key_fec_params_(),
       producer_fec_(&fec_),
-      _fecOverheadRate(clock, NULL),
-      _videoBitrate(clock, NULL) {
+      fec_bitrate_(1000, 8000),  // 1000ms window, bits/s scale.

[danilchap, 2016/06/23 12:46:12]

8000 is so popular constant for RateStatistic class. May be put it as constant
into it?
Or at least name this constants in this file, something like
namespace {
constexpr int64_t kMaxWindowSizeMs = 1000;
constexpr float kBpsScale = 8000.0;
}  // namespace

[sprang_webrtc, 2016/06/28 09:12:33]

Done.

+      video_bitrate_(1000, 8000) {
   memset(&delta_fec_params_, 0, sizeof(delta_fec_params_));
   memset(&key_fec_params_, 0, sizeof(key_fec_params_));
   delta_fec_params_.max_fec_frames = key_fec_params_.max_fec_frames = 1;
@@ -95,7 +96,9 @@ void RTPSenderVideo::SendVideoPacket(uint8_t* data_buffer,
   if (_rtpSender.SendToNetwork(data_buffer, payload_length, rtp_header_length,
                                capture_time_ms, storage,
                                RtpPacketSender::kLowPriority) == 0) {
-    _videoBitrate.Update(payload_length + rtp_header_length);
+    rtc::CritScope cs(&stats_crit_);
+    video_bitrate_.Update(payload_length + rtp_header_length,
+                          clock_->TimeInMilliseconds());
     TRACE_EVENT_INSTANT2(TRACE_DISABLED_BY_DEFAULT("webrtc_rtp"),
                          "Video::PacketNormal", "timestamp", capture_timestamp,
                          "seqnum", seq_num);
@@ -141,7 +144,8 @@ void RTPSenderVideo::SendVideoPacketAsRed(uint8_t* data_buffer,
           red_packet->data(), red_packet->length() - rtp_header_length,
           rtp_header_length, capture_time_ms, media_packet_storage,
           RtpPacketSender::kLowPriority) == 0) {
-    _videoBitrate.Update(red_packet->length());
+    rtc::CritScope cs(&stats_crit_);
+    video_bitrate_.Update(red_packet->length(), clock_->TimeInMilliseconds());
     TRACE_EVENT_INSTANT2(TRACE_DISABLED_BY_DEFAULT("webrtc_rtp"),
                          "Video::PacketRed", "timestamp", capture_timestamp,
                          "seqnum", media_seq_num);
@@ -153,7 +157,8 @@ void RTPSenderVideo::SendVideoPacketAsRed(uint8_t* data_buffer,
             fec_packet->data(), fec_packet->length() - rtp_header_length,
             rtp_header_length, capture_time_ms, fec_storage,
             RtpPacketSender::kLowPriority) == 0) {
-      _fecOverheadRate.Update(fec_packet->length());
+      rtc::CritScope cs(&stats_crit_);
+      fec_bitrate_.Update(fec_packet->length(), clock_->TimeInMilliseconds());
       TRACE_EVENT_INSTANT2(TRACE_DISABLED_BY_DEFAULT("webrtc_rtp"),
                            "Video::PacketFec", "timestamp", capture_timestamp,
                            "seqnum", next_fec_sequence_number);
@@ -337,17 +342,14 @@ int32_t RTPSenderVideo::SendVideo(const RtpVideoCodecTypes videoType,
   return 0;
 }
 
-void RTPSenderVideo::ProcessBitrate() {
-  _videoBitrate.Process();
-  _fecOverheadRate.Process();
+uint32_t RTPSenderVideo::VideoBitrateSent() {
+  rtc::CritScope cs(&stats_crit_);
+  return video_bitrate_.Rate(clock_->TimeInMilliseconds()).value_or(0);
 }
 
-uint32_t RTPSenderVideo::VideoBitrateSent() const {
-  return _videoBitrate.BitrateLast();
-}
-
-uint32_t RTPSenderVideo::FecOverheadRate() const {
-  return _fecOverheadRate.BitrateLast();
+uint32_t RTPSenderVideo::FecOverheadRate() {
+  rtc::CritScope cs(&stats_crit_);
+  return fec_bitrate_.Rate(clock_->TimeInMilliseconds()).value_or(0);

[danilchap, 2016/06/23 12:46:12]

may be better make RateStatistic::Rate function const? (e.g. using mutables)
that function may change some internal caching variables, but doesn't change
logical state of the RateStatistic, i.e. calling RateStatistic::Rate doesn't
affect results of any further calls... as long as time is monotonic.
(It is also possible to change Rate function to calculate same value without
changing internal variables, but it would make code more complicated and less
efficient)

[sprang_webrtc, 2016/06/28 09:12:33]

Yeah. It's a bit ugly but probably worth it.

 }
 
 int RTPSenderVideo::SelectiveRetransmissions() const {