Use RateAccCounter for sent bitrate stats. Reports average of periodically computed stats over a ca…

Use RateAccCounter for sent bitrate stats. Reports average of periodically computed stats over a call.

Intervals when video is paused is no longer included in the stats:
"WebRTC.Video.BitrateSentInKbps"
"WebRTC.Video.MediaBitrateSentInKbps"
"WebRTC.Video.PaddingBitrateSentInKbps"
"WebRTC.Video.RetransmittedBitrateSentInKbps"
"WebRTC.Video.RtxBitrateSentInKbps"
"WebRTC.Video.FecBitrateSentInKbps"

BUG=webrtc:5283
Review-Url: https://codereview.webrtc.org/2536613002
Cr-Commit-Position: refs/heads/master@{#16447}
Committed: https://chromium.googlesource.com/external/webrtc/+/93e1e23537431893c34b5c4fd879532c7bba839a

[åsapersson, 2016-12-02 13:38:54]

Patchset #4 (id:60001) has been deleted

[åsapersson, 2016-12-02 13:39:31]

Patchset #3 (id:40001) has been deleted

[åsapersson, 2016-12-02 13:45:21]

Patchset #3 (id:80001) has been deleted

[åsapersson, 2016-12-02 13:57:33]

The CQ bit was checked by asapersson@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 13:57:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-12-02 14:02:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-02 14:02:50]

Dry run: Try jobs failed on following builders:
  win_x64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/20288)

[åsapersson, 2016-12-02 14:41:10]

The CQ bit was checked by asapersson@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 14:41:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[åsapersson, 2016-12-02 14:47:41]

Patchset #3 (id:100001) has been deleted

[commit-bot: I haz the power, 2016-12-02 15:01:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-02 15:01:06]

Dry run: This issue passed the CQ dry run.

[åsapersson, 2016-12-02 15:54:12]

Description was changed from

==========
Use RateAccCounter for sent bitrate stats. Reports average of periodically
computed stats over a call.

Intervals when video is paused is no longer included in the stats:
"WebRTC.Video.BitrateSentInKbps"
"WebRTC.Video.MediaBitrateSentInKbps"
"WebRTC.Video.PaddingBitrateSentInKbps"
"WebRTC.Video.RetransmittedBitrateSentInKbps"
"WebRTC.Video.RtxBitrateSentInKbps"
"WebRTC.Video.FecBitrateSentInKbps"

BUG=webrtc:5283
==========

to

==========
Use RateAccCounter for sent bitrate stats. Reports average of periodically
computed stats over a call.

Intervals when video is paused is no longer included in the stats:
"WebRTC.Video.BitrateSentInKbps"
"WebRTC.Video.MediaBitrateSentInKbps"
"WebRTC.Video.PaddingBitrateSentInKbps"
"WebRTC.Video.RetransmittedBitrateSentInKbps"
"WebRTC.Video.RtxBitrateSentInKbps"
"WebRTC.Video.FecBitrateSentInKbps"

BUG=webrtc:5283
==========

[åsapersson, 2016-12-02 15:54:12]

asapersson@webrtc.org changed reviewers:
+ stefan@webrtc.org

[åsapersson, 2016-12-02 16:44:40]

asapersson@webrtc.org changed reviewers:
- stefan@webrtc.org

[åsapersson, 2016-12-06 08:15:39]

Patchset #4 (id:140001) has been deleted

[åsapersson, 2016-12-06 08:17:30]

The CQ bit was checked by asapersson@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-06 08:17:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-12-06 08:35:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-06 08:35:40]

Dry run: This issue passed the CQ dry run.

[åsapersson, 2016-12-06 17:21:01]

Patchset #4 (id:160001) has been deleted

[åsapersson, 2016-12-06 17:21:52]

asapersson@webrtc.org changed reviewers:
+ stefan@webrtc.org

[stefan-webrtc, 2017-01-10 09:16:23]

lgtm

https://codereview.webrtc.org/2536613002/diff/180001/webrtc/video/send_statis...
File webrtc/video/send_statistics_proxy.cc (right):

https://codereview.webrtc.org/2536613002/diff/180001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy.cc:366: }
A lot of code duplication here. can we create a method that we call 4 times here
instead?

https://codereview.webrtc.org/2536613002/diff/180001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy.cc:375: rtx_bytes_per_sec_avg = 0;
Please comment on why we do this. It's not clear why this should be set to zero
because the total bytes per sec samples is too small.

And why shouldn't it be set to -1?

https://codereview.webrtc.org/2536613002/diff/180001/webrtc/video/stats_count...
File webrtc/video/stats_counter.cc (right):

https://codereview.webrtc.org/2536613002/diff/180001/webrtc/video/stats_count...
webrtc/video/stats_counter.cc:199: void
StatsCounter::ProcessAndPauseWithMin(int64_t min_pause_time_ms) {
Can we have a better name for this? Maybe something like
ProcessAndPauseForDuration. It's hard to understand what min really means. Will
stats be paused for at least min_pause_time_ms, but it can be paused for a
longer time?

https://codereview.webrtc.org/2536613002/diff/180001/webrtc/video/stats_count...
webrtc/video/stats_counter.cc:297: void
StatsCounter::ResetPauseIfMinTimePassed() {
ResumeIfMinTimePassed

https://codereview.webrtc.org/2536613002/diff/180001/webrtc/video/stats_count...
webrtc/video/stats_counter.cc:304: void StatsCounter::ResetPause() {
Should this be called Resume()

https://codereview.webrtc.org/2536613002/diff/180001/webrtc/video/stats_count...
File webrtc/video/stats_counter.h (right):

https://codereview.webrtc.org/2536613002/diff/180001/webrtc/video/stats_count...
webrtc/video/stats_counter.h:280: // Sets the value for previous interval. Used
initially, zero is default.
I think it would have been good with an example of how this is supposed to be
used in a typical case.

[stefan-webrtc, 2017-01-10 09:16:44]

accidentaly gave lg.

not lgtm

[åsapersson, 2017-01-13 11:47:47]

Patchset #5 (id:200001) has been deleted

[åsapersson, 2017-01-13 13:02:57]

https://codereview.webrtc.org/2536613002/diff/180001/webrtc/video/send_statis...
File webrtc/video/send_statistics_proxy.cc (right):

https://codereview.webrtc.org/2536613002/diff/180001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy.cc:366: }
On 2017/01/10 09:16:23, stefan-webrtc wrote:
> A lot of code duplication here. can we create a method that we call 4 times
here
> instead?

For the histograms, the name needs to be unique per macro (since the histogram
pointer is cached), e.g.

void Method(const std::string& name) {
  RTC_HISTOGRAM(name);
}

Method("name1");
Method("name2");

is not allowed.

https://codereview.webrtc.org/2536613002/diff/180001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy.cc:375: rtx_bytes_per_sec_avg = 0;
On 2017/01/10 09:16:23, stefan-webrtc wrote:
> Please comment on why we do this. It's not clear why this should be set to
zero
> because the total bytes per sec samples is too small.
> 
> And why shouldn't it be set to -1?

Added a comment.

https://codereview.webrtc.org/2536613002/diff/180001/webrtc/video/stats_count...
File webrtc/video/stats_counter.cc (right):

https://codereview.webrtc.org/2536613002/diff/180001/webrtc/video/stats_count...
webrtc/video/stats_counter.cc:199: void
StatsCounter::ProcessAndPauseWithMin(int64_t min_pause_time_ms) {
On 2017/01/10 09:16:23, stefan-webrtc wrote:
> Can we have a better name for this? Maybe something like
> ProcessAndPauseForDuration. It's hard to understand what min really means.
Will
> stats be paused for at least min_pause_time_ms, but it can be paused for a
> longer time?

Right, stats will be paused for at least min_pause_time_ms. Rename method.

https://codereview.webrtc.org/2536613002/diff/180001/webrtc/video/stats_count...
webrtc/video/stats_counter.cc:297: void
StatsCounter::ResetPauseIfMinTimePassed() {
On 2017/01/10 09:16:23, stefan-webrtc wrote:
> ResumeIfMinTimePassed

Done.

https://codereview.webrtc.org/2536613002/diff/180001/webrtc/video/stats_count...
webrtc/video/stats_counter.cc:304: void StatsCounter::ResetPause() {
On 2017/01/10 09:16:23, stefan-webrtc wrote:
> Should this be called Resume()

Done.

https://codereview.webrtc.org/2536613002/diff/180001/webrtc/video/stats_count...
File webrtc/video/stats_counter.h (right):

https://codereview.webrtc.org/2536613002/diff/180001/webrtc/video/stats_count...
webrtc/video/stats_counter.h:280: // Sets the value for previous interval. Used
initially, zero is default.
On 2017/01/10 09:16:23, stefan-webrtc wrote:
> I think it would have been good with an example of how this is supposed to be
> used in a typical case.

Updated comment, hopefully more clear now.

[stefan-webrtc, 2017-01-13 13:26:46]

lgtm

[åsapersson, 2017-02-06 10:09:54]

The CQ bit was checked by asapersson@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-06 10:10:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-02-06 11:13:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-06 11:13:18]

Dry run: This issue passed the CQ dry run.

[åsapersson, 2017-02-06 13:16:08]

The CQ bit was checked by asapersson@webrtc.org

[åsapersson, 2017-02-06 13:16:09]

The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2536613002/#ps240001 (title:
"rebase")

[commit-bot: I haz the power, 2017-02-06 13:16:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-02-06 13:18:36]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1486386968688510,
"parent_rev": "447dba958681dc3054c3fd92ef571176ab52c279", "commit_rev":
"93e1e23537431893c34b5c4fd879532c7bba839a"}

[commit-bot: I haz the power, 2017-02-06 13:18:39]

Description was changed from

==========
Use RateAccCounter for sent bitrate stats. Reports average of periodically
computed stats over a call.

Intervals when video is paused is no longer included in the stats:
"WebRTC.Video.BitrateSentInKbps"
"WebRTC.Video.MediaBitrateSentInKbps"
"WebRTC.Video.PaddingBitrateSentInKbps"
"WebRTC.Video.RetransmittedBitrateSentInKbps"
"WebRTC.Video.RtxBitrateSentInKbps"
"WebRTC.Video.FecBitrateSentInKbps"

BUG=webrtc:5283
==========

to

==========
Use RateAccCounter for sent bitrate stats. Reports average of periodically
computed stats over a call.

Intervals when video is paused is no longer included in the stats:
"WebRTC.Video.BitrateSentInKbps"
"WebRTC.Video.MediaBitrateSentInKbps"
"WebRTC.Video.PaddingBitrateSentInKbps"
"WebRTC.Video.RetransmittedBitrateSentInKbps"
"WebRTC.Video.RtxBitrateSentInKbps"
"WebRTC.Video.FecBitrateSentInKbps"

BUG=webrtc:5283

Review-Url: https://codereview.webrtc.org/2536613002
Cr-Commit-Position: refs/heads/master@{#16447}
Committed:
https://chromium.googlesource.com/external/webrtc/+/93e1e23537431893c34b5c4fd...
==========

[commit-bot: I haz the power, 2017-02-06 13:18:41]

Committed patchset #6 (id:240001) as
https://chromium.googlesource.com/external/webrtc/+/93e1e23537431893c34b5c4fd...