Update AvgCounter to have the ability to include last period metric for subsequent intervals withou…

Update AvgCounter to have the ability to include last period metric for subsequent intervals without samples (e.g. for non-periodic updated stats).

Integrate AvgCounter to be used for BWE stats in call.

Fixes for stats regression in:
WebRTC.Call.EstimatedSendBitrateInKbps
WebRTC.Call.PacerBitrateInKbps

Example:
BWE for a 15 seconds long call (with intervals of 1 sec):
|300|400|500|600|600|600|600| 0 | 0 | 0 | 0 | 0 |800|800|800|  // 0 - network state down

Reported via OnNetworkChanged:
|300|400|500|600| x | x | x | 0 | x | x | x | x |800| x | x |  // x - empty interval, 0 -> pauses stats

Stats:
|300|400|500|600|600|600|600| - | - | - | - | - |800|800|800|  // x -> last value used (intervals during pause ignored)

AvgCounter uses the average of samples within an interval (interval length is 2 sec).

BUG=webrtc:6244
Committed: https://crrev.com/ce2e13602e06c5f7df7245f8d0e66e7070c0be34
Cr-Commit-Position: refs/heads/master@{#14147}

[perkj_webrtc, 2016-09-05 07:59:58]

Description was changed from

==========
Update AvgCounter to have the ability to include last period metric for
subsequent intervals without samples (e.g. for non-periodic updated stats).

Integrate AvgCounter to be used for BWE stats in call.

Fixes for stats regression in:
WebRTC.Call.EstimatedSendBitrateInKbps
WebRTC.Call.PacerBitrateInKbps

webrtc:6244
==========

to

==========
Update AvgCounter to have the ability to include last period metric for
subsequent intervals without samples (e.g. for non-periodic updated stats).

Integrate AvgCounter to be used for BWE stats in call.

Fixes for stats regression in:
WebRTC.Call.EstimatedSendBitrateInKbps
WebRTC.Call.PacerBitrateInKbps

BUG=webrtc:6244
==========

[åsapersson, 2016-09-05 14:27:01]

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

[åsapersson, 2016-09-05 14:28:10]

Patchset #2 (id:20001) has been deleted

[stefan-webrtc, 2016-09-08 07:28:15]

It's not clear to me how this CL fixes the bug. Could you describe that in the
description?

My understanding is that because OnNetworkChanged never gets called once the BWE
has converged (and no longer changes), we won't get samples reported during that
time. In many calls that will be the majority of the call, which means the BWE
stats will only be an average of the start-up phase.

Let's say BWE looks like this for a 10 seconds long call:
300, 400, 500, 600, 600, 600, 600, 600, 600, 600

then this is what will be reported via OnNetworkChanged:
300, 400, 500, 600

So the average will be 450 instead of 540. I might be missing how this solves
that issue though, sorry in that case. :)

https://codereview.webrtc.org/2307913002/diff/40001/webrtc/video/stats_counte...
File webrtc/video/stats_counter.cc (right):

https://codereview.webrtc.org/2307913002/diff/40001/webrtc/video/stats_counte...
webrtc/video/stats_counter.cc:48: bool HasSamples() const { return
stats_.num_samples > 0; }
Maybe call this Empty() instead to align with interfaces of typical c++
containers.

https://codereview.webrtc.org/2307913002/diff/40001/webrtc/video/stats_counte...
webrtc/video/stats_counter.cc:157: (num_samples_ == 0) ? elapsed_intervals :
(elapsed_intervals - 1);
I don't follow this, maybe add a comment on why we treat this differently when
we don't have samples?

[åsapersson, 2016-09-08 10:26:19]

Description was changed from

==========
Update AvgCounter to have the ability to include last period metric for
subsequent intervals without samples (e.g. for non-periodic updated stats).

Integrate AvgCounter to be used for BWE stats in call.

Fixes for stats regression in:
WebRTC.Call.EstimatedSendBitrateInKbps
WebRTC.Call.PacerBitrateInKbps

BUG=webrtc:6244
==========

to

==========
Update AvgCounter to have the ability to include last period metric for
subsequent intervals without samples (e.g. for non-periodic updated stats).

Integrate AvgCounter to be used for BWE stats in call.

Fixes for stats regression in:
WebRTC.Call.EstimatedSendBitrateInKbps
WebRTC.Call.PacerBitrateInKbps


Example:
BWE for a 10 seconds long call (with intervals of 1 sec):
|300|400|500|600|600|600|600| 0 | 0 | 0 | 0 | 0 |800|800|800|  // 0 - network
state down

Reported via OnNetworkChanged:
|300|400|500|600| x | x | x | 0 | x | x | x | x |800| x | x |   // x - empty
interval, 0 -> pauses stats 

Stats:
|300|400|500|600|600|600|600| - | - | - | - | - |800|800|800|  // x -> last
value used (intervals during pause ignored)

The average of samples within an interval is used (interval length is 2 sec).

BUG=webrtc:6244
==========

[åsapersson, 2016-09-08 10:27:26]

Description was changed from

==========
Update AvgCounter to have the ability to include last period metric for
subsequent intervals without samples (e.g. for non-periodic updated stats).

Integrate AvgCounter to be used for BWE stats in call.

Fixes for stats regression in:
WebRTC.Call.EstimatedSendBitrateInKbps
WebRTC.Call.PacerBitrateInKbps


Example:
BWE for a 10 seconds long call (with intervals of 1 sec):
|300|400|500|600|600|600|600| 0 | 0 | 0 | 0 | 0 |800|800|800|  // 0 - network
state down

Reported via OnNetworkChanged:
|300|400|500|600| x | x | x | 0 | x | x | x | x |800| x | x |   // x - empty
interval, 0 -> pauses stats 

Stats:
|300|400|500|600|600|600|600| - | - | - | - | - |800|800|800|  // x -> last
value used (intervals during pause ignored)

The average of samples within an interval is used (interval length is 2 sec).

BUG=webrtc:6244
==========

to

==========
Update AvgCounter to have the ability to include last period metric for
subsequent intervals without samples (e.g. for non-periodic updated stats).

Integrate AvgCounter to be used for BWE stats in call.

Fixes for stats regression in:
WebRTC.Call.EstimatedSendBitrateInKbps
WebRTC.Call.PacerBitrateInKbps


Example:
BWE for a 10 seconds long call (with intervals of 1 sec):
|300|400|500|600|600|600|600| 0 | 0 | 0 | 0 | 0 |800|800|800|  // 0 - network
state down

Reported via OnNetworkChanged:
|300|400|500|600| x | x | x | 0 | x | x | x | x |800| x | x |   // x - empty
interval, 0 -> pauses stats 

Stats:
|300|400|500|600|600|600|600| - | - | - | - | - |800|800|800|  // x -> last
value used (intervals during pause ignored)

AvgCounter uses the average of samples within an interval (interval length is 2
sec).

BUG=webrtc:6244
==========

[åsapersson, 2016-09-08 13:15:02]

https://codereview.webrtc.org/2307913002/diff/40001/webrtc/video/stats_counte...
File webrtc/video/stats_counter.cc (right):

https://codereview.webrtc.org/2307913002/diff/40001/webrtc/video/stats_counte...
webrtc/video/stats_counter.cc:48: bool HasSamples() const { return
stats_.num_samples > 0; }
On 2016/09/08 07:28:14, stefan-webrtc (holmer) wrote:
> Maybe call this Empty() instead to align with interfaces of typical c++
> containers.

Done.

https://codereview.webrtc.org/2307913002/diff/40001/webrtc/video/stats_counte...
webrtc/video/stats_counter.cc:157: (num_samples_ == 0) ? elapsed_intervals :
(elapsed_intervals - 1);
On 2016/09/08 07:28:14, stefan-webrtc (holmer) wrote:
> I don't follow this, maybe add a comment on why we treat this differently when
> we don't have samples?

Done.

[åsapersson, 2016-09-08 13:16:18]

Description was changed from

==========
Update AvgCounter to have the ability to include last period metric for
subsequent intervals without samples (e.g. for non-periodic updated stats).

Integrate AvgCounter to be used for BWE stats in call.

Fixes for stats regression in:
WebRTC.Call.EstimatedSendBitrateInKbps
WebRTC.Call.PacerBitrateInKbps


Example:
BWE for a 10 seconds long call (with intervals of 1 sec):
|300|400|500|600|600|600|600| 0 | 0 | 0 | 0 | 0 |800|800|800|  // 0 - network
state down

Reported via OnNetworkChanged:
|300|400|500|600| x | x | x | 0 | x | x | x | x |800| x | x |   // x - empty
interval, 0 -> pauses stats 

Stats:
|300|400|500|600|600|600|600| - | - | - | - | - |800|800|800|  // x -> last
value used (intervals during pause ignored)

AvgCounter uses the average of samples within an interval (interval length is 2
sec).

BUG=webrtc:6244
==========

to

==========
Update AvgCounter to have the ability to include last period metric for
subsequent intervals without samples (e.g. for non-periodic updated stats).

Integrate AvgCounter to be used for BWE stats in call.

Fixes for stats regression in:
WebRTC.Call.EstimatedSendBitrateInKbps
WebRTC.Call.PacerBitrateInKbps


Example:
BWE for a 10 seconds long call (with intervals of 1 sec):
|300|400|500|600|600|600|600| 0 | 0 | 0 | 0 | 0 |800|800|800|  // 0 - network
state down

Reported via OnNetworkChanged:
|300|400|500|600| x | x | x | 0 | x | x | x | x |800| x | x |  // x - empty
interval, 0 -> pauses stats 

Stats:
|300|400|500|600|600|600|600| - | - | - | - | - |800|800|800|  // x -> last
value used (intervals during pause ignored)

AvgCounter uses the average of samples within an interval (interval length is 2
sec).

BUG=webrtc:6244
==========

[stefan-webrtc, 2016-09-08 14:21:15]

Thanks for the new description, it made things a lot clearer.

lgtm

[åsapersson, 2016-09-08 14:24:48]

Description was changed from

==========
Update AvgCounter to have the ability to include last period metric for
subsequent intervals without samples (e.g. for non-periodic updated stats).

Integrate AvgCounter to be used for BWE stats in call.

Fixes for stats regression in:
WebRTC.Call.EstimatedSendBitrateInKbps
WebRTC.Call.PacerBitrateInKbps


Example:
BWE for a 10 seconds long call (with intervals of 1 sec):
|300|400|500|600|600|600|600| 0 | 0 | 0 | 0 | 0 |800|800|800|  // 0 - network
state down

Reported via OnNetworkChanged:
|300|400|500|600| x | x | x | 0 | x | x | x | x |800| x | x |  // x - empty
interval, 0 -> pauses stats 

Stats:
|300|400|500|600|600|600|600| - | - | - | - | - |800|800|800|  // x -> last
value used (intervals during pause ignored)

AvgCounter uses the average of samples within an interval (interval length is 2
sec).

BUG=webrtc:6244
==========

to

==========
Update AvgCounter to have the ability to include last period metric for
subsequent intervals without samples (e.g. for non-periodic updated stats).

Integrate AvgCounter to be used for BWE stats in call.

Fixes for stats regression in:
WebRTC.Call.EstimatedSendBitrateInKbps
WebRTC.Call.PacerBitrateInKbps


Example:
BWE for a 15 seconds long call (with intervals of 1 sec):
|300|400|500|600|600|600|600| 0 | 0 | 0 | 0 | 0 |800|800|800|  // 0 - network
state down

Reported via OnNetworkChanged:
|300|400|500|600| x | x | x | 0 | x | x | x | x |800| x | x |  // x - empty
interval, 0 -> pauses stats 

Stats:
|300|400|500|600|600|600|600| - | - | - | - | - |800|800|800|  // x -> last
value used (intervals during pause ignored)

AvgCounter uses the average of samples within an interval (interval length is 2
sec).

BUG=webrtc:6244
==========

[åsapersson, 2016-09-09 06:26:22]

The CQ bit was checked by asapersson@webrtc.org

[commit-bot: I haz the power, 2016-09-09 06:26:29]

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-09-09 07:13:36]

Description was changed from

==========
Update AvgCounter to have the ability to include last period metric for
subsequent intervals without samples (e.g. for non-periodic updated stats).

Integrate AvgCounter to be used for BWE stats in call.

Fixes for stats regression in:
WebRTC.Call.EstimatedSendBitrateInKbps
WebRTC.Call.PacerBitrateInKbps


Example:
BWE for a 15 seconds long call (with intervals of 1 sec):
|300|400|500|600|600|600|600| 0 | 0 | 0 | 0 | 0 |800|800|800|  // 0 - network
state down

Reported via OnNetworkChanged:
|300|400|500|600| x | x | x | 0 | x | x | x | x |800| x | x |  // x - empty
interval, 0 -> pauses stats 

Stats:
|300|400|500|600|600|600|600| - | - | - | - | - |800|800|800|  // x -> last
value used (intervals during pause ignored)

AvgCounter uses the average of samples within an interval (interval length is 2
sec).

BUG=webrtc:6244
==========

to

==========
Update AvgCounter to have the ability to include last period metric for
subsequent intervals without samples (e.g. for non-periodic updated stats).

Integrate AvgCounter to be used for BWE stats in call.

Fixes for stats regression in:
WebRTC.Call.EstimatedSendBitrateInKbps
WebRTC.Call.PacerBitrateInKbps


Example:
BWE for a 15 seconds long call (with intervals of 1 sec):
|300|400|500|600|600|600|600| 0 | 0 | 0 | 0 | 0 |800|800|800|  // 0 - network
state down

Reported via OnNetworkChanged:
|300|400|500|600| x | x | x | 0 | x | x | x | x |800| x | x |  // x - empty
interval, 0 -> pauses stats 

Stats:
|300|400|500|600|600|600|600| - | - | - | - | - |800|800|800|  // x -> last
value used (intervals during pause ignored)

AvgCounter uses the average of samples within an interval (interval length is 2
sec).

BUG=webrtc:6244
==========

[commit-bot: I haz the power, 2016-09-09 07:13:39]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-09-09 07:13:41]

Description was changed from

==========
Update AvgCounter to have the ability to include last period metric for
subsequent intervals without samples (e.g. for non-periodic updated stats).

Integrate AvgCounter to be used for BWE stats in call.

Fixes for stats regression in:
WebRTC.Call.EstimatedSendBitrateInKbps
WebRTC.Call.PacerBitrateInKbps


Example:
BWE for a 15 seconds long call (with intervals of 1 sec):
|300|400|500|600|600|600|600| 0 | 0 | 0 | 0 | 0 |800|800|800|  // 0 - network
state down

Reported via OnNetworkChanged:
|300|400|500|600| x | x | x | 0 | x | x | x | x |800| x | x |  // x - empty
interval, 0 -> pauses stats 

Stats:
|300|400|500|600|600|600|600| - | - | - | - | - |800|800|800|  // x -> last
value used (intervals during pause ignored)

AvgCounter uses the average of samples within an interval (interval length is 2
sec).

BUG=webrtc:6244
==========

to

==========
Update AvgCounter to have the ability to include last period metric for
subsequent intervals without samples (e.g. for non-periodic updated stats).

Integrate AvgCounter to be used for BWE stats in call.

Fixes for stats regression in:
WebRTC.Call.EstimatedSendBitrateInKbps
WebRTC.Call.PacerBitrateInKbps

Example:
BWE for a 15 seconds long call (with intervals of 1 sec):
|300|400|500|600|600|600|600| 0 | 0 | 0 | 0 | 0 |800|800|800|  // 0 - network
state down

Reported via OnNetworkChanged:
|300|400|500|600| x | x | x | 0 | x | x | x | x |800| x | x |  // x - empty
interval, 0 -> pauses stats

Stats:
|300|400|500|600|600|600|600| - | - | - | - | - |800|800|800|  // x -> last
value used (intervals during pause ignored)

AvgCounter uses the average of samples within an interval (interval length is 2
sec).

BUG=webrtc:6244

Committed: https://crrev.com/ce2e13602e06c5f7df7245f8d0e66e7070c0be34
Cr-Commit-Position: refs/heads/master@{#14147}
==========

[commit-bot: I haz the power, 2016-09-09 07:13:42]

Patchset 4 (id:??) landed as
https://crrev.com/ce2e13602e06c5f7df7245f8d0e66e7070c0be34
Cr-Commit-Position: refs/heads/master@{#14147}

[perkj_webrtc, 2016-09-10 07:12:53]

perkj@webrtc.org changed reviewers:
+ perkj@webrtc.org

[perkj_webrtc, 2016-09-10 07:12:54]

a bit late but can you remove the need for locking now?

https://codereview.webrtc.org/2307913002/diff/80001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/2307913002/diff/80001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:762: rtc::CritScope lock(&bitrate_crit_);
is this lock still needed?

https://codereview.webrtc.org/2307913002/diff/80001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:768: bool sending_video;
dito- lock needed?

https://codereview.webrtc.org/2307913002/diff/80001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:774: rtc::CritScope lock(&bitrate_crit_);
and this?