Add class which periodically computes statistics.

Add class which periodically computes statistics.

During a period (e.g. 2 seconds), different metrics can be computed, e.g.:
- average of samples
- percentage/permille of samples
- units per second

Each periodic metric can be either:
- reported to an observer each period
- aggregated during the call (e.g. min, max, average)

BUG=webrtc:5283
Committed: https://crrev.com/0e9d6d9e0c2e553ddd79d497b2e9e47a612d2ee1
Cr-Commit-Position: refs/heads/master@{#12847}

[åsapersson, 2016-01-28 08:38:54]

Patchset #3 (id:40001) has been deleted

[åsapersson, 2016-01-28 08:39:02]

Patchset #2 (id:20001) has been deleted

[åsapersson, 2016-01-28 08:41:32]

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

[stefan-webrtc, 2016-01-29 15:22:46]

A first pass, I'll probably have to go through this in detail again though

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

https://codereview.webrtc.org/1640053003/diff/60001/webrtc/video/stats_counte...
webrtc/video/stats_counter.cc:42: bucket_size1_(bucket_count == bucket_max),
I don't think I fully understand what this variable is based on reading its
name.

https://codereview.webrtc.org/1640053003/diff/60001/webrtc/video/stats_counte...
webrtc/video/stats_counter.cc:65: double log_next = log_cur + log_x;
I'm trying to follow this and I have a bit of a hard time understanding what x
is here. Is it the number of percent each bucket is supposed to increase?

Based on my computations below I think line 66 can be rewritten to
next = cur * exp(log(x)) and line 65 can be removed, is that correct? Either way
maybe we should have a comment or two here to explain the derivation?

log(max / cur) = log(x)*remaining
log(x) = log(max/cur) / remaining
log(x) = (log(max) - log(cur)) / remaining

log(cur) + log(x) = log(cur*x)
next = exp(log(cur*x)) = cur*x

https://codereview.webrtc.org/1640053003/diff/60001/webrtc/video/stats_counter.h
File webrtc/video/stats_counter.h (right):

https://codereview.webrtc.org/1640053003/diff/60001/webrtc/video/stats_counte...
webrtc/video/stats_counter.h:56: Stats stats();
ComputeStats() or ComputeAndGetStats()? Should probably not be stats() since it
does computations.

https://codereview.webrtc.org/1640053003/diff/60001/webrtc/video/stats_counte...
webrtc/video/stats_counter.h:67: const bool bucket_size1_;
bucket_size_?

https://codereview.webrtc.org/1640053003/diff/60001/webrtc/video/stats_counte...
webrtc/video/stats_counter.h:99: };
You probably want to disable the copy constructor of this class with
RTC_DISALLOW_IMPLICIT_CONSTRUCTORS() since copying it will copy the scoped_ptr

[stefan-webrtc, 2016-01-29 15:24:25]

Btw, could you comment on how this is going to be used and for what (maybe a
design doc)? I think that would simplify the review.

[åsapersson, 2016-02-09 08:18:04]

Patchset #6 (id:140001) has been deleted

[åsapersson, 2016-02-09 08:49:17]

Description was changed from

==========
Add class which holds periodically computed video stats.

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

to

==========
Add class which periodically computes video stats.

Add histogram for input framerate:
"WebRTC.Video.Periodic.InputFramesPerSecond"

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

[åsapersson, 2016-02-09 08:49:17]

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

[åsapersson, 2016-02-09 08:50:11]

Patchset #6 (id:160001) has been deleted

[åsapersson, 2016-02-09 09:21:48]

Patchset #6 (id:180001) has been deleted

[åsapersson, 2016-02-09 09:24:33]

On 2016/01/29 15:24:25, stefan-webrtc (holmer) wrote:
> Btw, could you comment on how this is going to be used and for what (maybe a
> design doc)? I think that would simplify the review.

Removed histogram code, PTAL.

[åsapersson, 2016-04-29 11:36:28]

Patchset #7 (id:220001) has been deleted

[åsapersson, 2016-04-29 14:07:46]

Patchset #8 (id:260001) has been deleted

[åsapersson, 2016-04-29 14:09:00]

Description was changed from

==========
Add class which periodically computes video stats.

Add histogram for input framerate:
"WebRTC.Video.Periodic.InputFramesPerSecond"

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

to

==========
Add class which periodically computes statistics.

During a period (e.g. 2 seconds), different metrics can be computed, e.g.:
average of samples
percentage/permille of samples
units per second

Each periodic metric can be either:
reported to an observer each period
aggregated during the call (e.g. min, max, average)


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

[åsapersson, 2016-04-29 14:09:00]

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

[åsapersson, 2016-04-29 14:09:28]

Description was changed from

==========
Add class which periodically computes statistics.

During a period (e.g. 2 seconds), different metrics can be computed, e.g.:
average of samples
percentage/permille of samples
units per second

Each periodic metric can be either:
reported to an observer each period
aggregated during the call (e.g. min, max, average)


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

to

==========
Add class which periodically computes statistics.

During a period (e.g. 2 seconds), different metrics can be computed, e.g.:
- average of samples
- percentage/permille of samples
- units per second

Each periodic metric can be either:
- reported to an observer each period
- aggregated during the call (e.g. min, max, average)


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

[mflodman, 2016-05-06 07:33:30]

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

https://codereview.webrtc.org/1640053003/diff/300001/webrtc/video/stats_count...
webrtc/video/stats_counter.h:38: class AggStats {
What does 'Agg' stand for? Can you write the whole name, I'm don't fully
understand what this class does.

I'm not a big fan of nested classes in this way, it get's a bit unclear who is
supposed to call what methods. I guess a StatsCounter user shouldn't call
StatsCounter::AggStats::Add(...) directly, right? If that is true, I'd prefer to
not have this class nested in StatsCounter, to make it more clear what a user of
StatsCounter is supposed to call and what not to call.

https://codereview.webrtc.org/1640053003/diff/300001/webrtc/video/stats_count...
webrtc/video/stats_counter.h:45: int64_t num_samples;
You can set default values here instead of using a ctor.

https://codereview.webrtc.org/1640053003/diff/300001/webrtc/video/stats_count...
webrtc/video/stats_counter.h:70: void Add(int sample);
It would be good to add a comment for these two methods, what they do and maybe
the difference between them.

https://codereview.webrtc.org/1640053003/diff/300001/webrtc/video/stats_count...
webrtc/video/stats_counter.h:156: class RateAccCounter : public StatsCounter {
It would be good with a class description for these classes, describing the
intent with each of them.

[stefan-webrtc, 2016-05-06 11:10:17]

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

https://codereview.webrtc.org/1640053003/diff/300001/webrtc/video/stats_count...
webrtc/video/stats_counter.cc:49: stats_.average = (sum_ + (stats_.num_samples /
2)) / stats_.num_samples;
Remove parentheses around stats_.num_samples / 2

https://codereview.webrtc.org/1640053003/diff/300001/webrtc/video/stats_count...
webrtc/video/stats_counter.cc:132: *metric = (sum_ + (num_samples_ / 2)) /
num_samples_;
Remove parentheses for the second term here and below.

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

https://codereview.webrtc.org/1640053003/diff/300001/webrtc/video/stats_count...
webrtc/video/stats_counter.h:53: // Computes and returns |stats_|.
Empty line above.

[åsapersson, 2016-05-09 13:50:01]

Patchset #10 (id:320001) has been deleted

[åsapersson, 2016-05-09 14:15:37]

Patchset #10 (id:340001) has been deleted

[åsapersson, 2016-05-09 14:32:29]

Patchset #10 (id:360001) has been deleted

[åsapersson, 2016-05-09 14:36:51]

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

https://codereview.webrtc.org/1640053003/diff/300001/webrtc/video/stats_count...
webrtc/video/stats_counter.cc:49: stats_.average = (sum_ + (stats_.num_samples /
2)) / stats_.num_samples;
On 2016/05/06 11:10:17, stefan-webrtc (holmer) wrote:
> Remove parentheses around stats_.num_samples / 2

Done.

https://codereview.webrtc.org/1640053003/diff/300001/webrtc/video/stats_count...
webrtc/video/stats_counter.cc:132: *metric = (sum_ + (num_samples_ / 2)) /
num_samples_;
On 2016/05/06 11:10:17, stefan-webrtc (holmer) wrote:
> Remove parentheses for the second term here and below.

Done.

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

https://codereview.webrtc.org/1640053003/diff/300001/webrtc/video/stats_count...
webrtc/video/stats_counter.h:38: class AggStats {
On 2016/05/06 07:33:29, mflodman wrote:
> What does 'Agg' stand for? Can you write the whole name, I'm don't fully
> understand what this class does.
> 
> I'm not a big fan of nested classes in this way, it get's a bit unclear who is
> supposed to call what methods. I guess a StatsCounter user shouldn't call
> StatsCounter::AggStats::Add(...) directly, right? If that is true, I'd prefer
to
> not have this class nested in StatsCounter, to make it more clear what a user
of
> StatsCounter is supposed to call and what not to call.

Have moved the nested classes and renamed classes.

https://codereview.webrtc.org/1640053003/diff/300001/webrtc/video/stats_count...
webrtc/video/stats_counter.h:45: int64_t num_samples;
On 2016/05/06 07:33:29, mflodman wrote:
> You can set default values here instead of using a ctor.

Done.

https://codereview.webrtc.org/1640053003/diff/300001/webrtc/video/stats_count...
webrtc/video/stats_counter.h:53: // Computes and returns |stats_|.
On 2016/05/06 11:10:17, stefan-webrtc (holmer) wrote:
> Empty line above.

Done.

https://codereview.webrtc.org/1640053003/diff/300001/webrtc/video/stats_count...
webrtc/video/stats_counter.h:70: void Add(int sample);
On 2016/05/06 07:33:29, mflodman wrote:
> It would be good to add a comment for these two methods, what they do and
maybe
> the difference between them.

Added more comments and examples.

https://codereview.webrtc.org/1640053003/diff/300001/webrtc/video/stats_count...
webrtc/video/stats_counter.h:156: class RateAccCounter : public StatsCounter {
On 2016/05/06 07:33:29, mflodman wrote:
> It would be good with a class description for these classes, describing the
> intent with each of them.

Done.

[åsapersson, 2016-05-10 09:43:01]

Patchset #11 (id:400001) has been deleted

[åsapersson, 2016-05-10 09:44:46]

Patchset #11 (id:420001) has been deleted

[mflodman, 2016-05-17 11:56:18]

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

https://codereview.webrtc.org/1640053003/diff/440001/webrtc/video/stats_count...
webrtc/video/stats_counter.h:40: class AggregatedCounter {
To make the header even simpler, wdyt of moving this to the cc file instead?

I don't see that the class is used anywhere byt in the cc file. You'd have to
change the private member in StatsCounter to be a unique ptr or something
instead.

[åsapersson, 2016-05-18 11:43:09]

Patchset #12 (id:460001) has been deleted

[åsapersson, 2016-05-18 12:01:34]

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

https://codereview.webrtc.org/1640053003/diff/440001/webrtc/video/stats_count...
webrtc/video/stats_counter.h:40: class AggregatedCounter {
On 2016/05/17 11:56:18, mflodman wrote:
> To make the header even simpler, wdyt of moving this to the cc file instead?
> 
> I don't see that the class is used anywhere byt in the cc file. You'd have to
> change the private member in StatsCounter to be a unique ptr or something
> instead.

Done.

[stefan-webrtc, 2016-05-20 12:56:24]

lgtm

[mflodman, 2016-05-23 05:11:00]

lgtm

[åsapersson, 2016-05-23 09:44:37]

The CQ bit was checked by asapersson@webrtc.org

[commit-bot: I haz the power, 2016-05-23 09:44:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1640053003/480001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1640053003/480001

[commit-bot: I haz the power, 2016-05-23 11:45:28]

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

[commit-bot: I haz the power, 2016-05-23 11:45:30]

Try jobs failed on following builders:
  android_dbg on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[åsapersson, 2016-05-23 13:06:07]

The CQ bit was checked by asapersson@webrtc.org

[commit-bot: I haz the power, 2016-05-23 13:06:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1640053003/480001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1640053003/480001

[commit-bot: I haz the power, 2016-05-23 13:07:59]

Description was changed from

==========
Add class which periodically computes statistics.

During a period (e.g. 2 seconds), different metrics can be computed, e.g.:
- average of samples
- percentage/permille of samples
- units per second

Each periodic metric can be either:
- reported to an observer each period
- aggregated during the call (e.g. min, max, average)


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

to

==========
Add class which periodically computes statistics.

During a period (e.g. 2 seconds), different metrics can be computed, e.g.:
- average of samples
- percentage/permille of samples
- units per second

Each periodic metric can be either:
- reported to an observer each period
- aggregated during the call (e.g. min, max, average)


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

[commit-bot: I haz the power, 2016-05-23 13:08:00]

Committed patchset #12 (id:480001)

[commit-bot: I haz the power, 2016-05-23 13:08:09]

Description was changed from

==========
Add class which periodically computes statistics.

During a period (e.g. 2 seconds), different metrics can be computed, e.g.:
- average of samples
- percentage/permille of samples
- units per second

Each periodic metric can be either:
- reported to an observer each period
- aggregated during the call (e.g. min, max, average)


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

to

==========
Add class which periodically computes statistics.

During a period (e.g. 2 seconds), different metrics can be computed, e.g.:
- average of samples
- percentage/permille of samples
- units per second

Each periodic metric can be either:
- reported to an observer each period
- aggregated during the call (e.g. min, max, average)

BUG=webrtc:5283

Committed: https://crrev.com/0e9d6d9e0c2e553ddd79d497b2e9e47a612d2ee1
Cr-Commit-Position: refs/heads/master@{#12847}
==========

[commit-bot: I haz the power, 2016-05-23 13:08:11]

Patchset 12 (id:??) landed as
https://crrev.com/0e9d6d9e0c2e553ddd79d497b2e9e47a612d2ee1
Cr-Commit-Position: refs/heads/master@{#12847}