NetEq: Implement Average Excess Buffer Delay stats

NetEq: Implement Average Excess Buffer Delay stats

This CL adds calculation and logging of excess buffer delay. This is
the average of time spent in the packet buffer for all packets. The
average is calculated for intervals of one minute, and the result is
logged to the UMA stat WebRTC.Audio.AverageExcessBufferDelayMs.

BUG=webrtc:4915, chromium:488124

[hlundin-webrtc, 2015-08-14 21:06:37]

henrik.lundin@webrtc.org changed reviewers:
+ ivoc@webrtc.org, minyue@webrtc.org

[hlundin-webrtc, 2015-08-14 21:06:37]

Minyue, Ivo,

Please, take a look at this CL. This is number three in the series adding new
metrics to NetEq.

Thanks!

[minyue-webrtc, 2015-08-17 13:38:08]

https://codereview.chromium.org/1284303003/diff/1/webrtc/modules/audio_coding...
File webrtc/modules/audio_coding/neteq/statistics_calculator.h (right):

https://codereview.chromium.org/1284303003/diff/1/webrtc/modules/audio_coding...
webrtc/modules/audio_coding/neteq/statistics_calculator.h:116: class
AverageExcessBufferDelayMs {
Since AverageExcessBufferDelayMs and DelayedPacketOutagesPerMinuteCounter and
probably others are similar in implementation, is it possible to create a common
ancestor to share codes, and use a list of the parent class to hold them to
avoid explicitly calling the AdvanceClock() one-by-one, or optionally use a
manager to hold them and put the operations inside the manager?

[hlundin-webrtc, 2015-08-18 07:41:44]

https://codereview.webrtc.org/1284303003/diff/1/webrtc/modules/audio_coding/n...
File webrtc/modules/audio_coding/neteq/statistics_calculator.h (right):

https://codereview.webrtc.org/1284303003/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/statistics_calculator.h:116: class
AverageExcessBufferDelayMs {
On 2015/08/17 13:38:08, minyue-webrtc wrote:
> Since AverageExcessBufferDelayMs and DelayedPacketOutagesPerMinuteCounter and
> probably others are similar in implementation, is it possible to create a
common
> ancestor to share codes, and use a list of the parent class to hold them to
> avoid explicitly calling the AdvanceClock() one-by-one, or optionally use a
> manager to hold them and put the operations inside the manager?

I thought about it at first. And now I tried it, but it gets messy for a number
of reasons. See draft CL: https://codereview.webrtc.org/1289753008/-
- The two use different logging macros; RTC_HISTOGRAM_COUNTS_100 vs
RTC_HISTOGRAM_COUNTS_1000. They both expand to the same underlying macro, so I
could generalize to that, but that means adding another state variable that
keeps track of the range (100 vs 1000).
- They require different input. One just registers that events happen (void
input) while the other registers magnitudes of events (int input).
Specialization needed.
- And since they would require different input parameters for the event
registration, the owner must know the actual class type, and not just the
(presumed) base logger class type. So, we would not really have a list of
objects that can be traversed.

I could add a manager class, but I see little value when the only thing it would
do is batch-call the AdvanceClock method.

[minyue-webrtc, 2015-08-18 12:07:43]

On 2015/08/18 07:41:44, hlundin-webrtc wrote:
>
https://codereview.webrtc.org/1284303003/diff/1/webrtc/modules/audio_coding/n...
> File webrtc/modules/audio_coding/neteq/statistics_calculator.h (right):
> 
>
https://codereview.webrtc.org/1284303003/diff/1/webrtc/modules/audio_coding/n...
> webrtc/modules/audio_coding/neteq/statistics_calculator.h:116: class
> AverageExcessBufferDelayMs {
> On 2015/08/17 13:38:08, minyue-webrtc wrote:
> > Since AverageExcessBufferDelayMs and DelayedPacketOutagesPerMinuteCounter
and
> > probably others are similar in implementation, is it possible to create a
> common
> > ancestor to share codes, and use a list of the parent class to hold them to
> > avoid explicitly calling the AdvanceClock() one-by-one, or optionally use a
> > manager to hold them and put the operations inside the manager?
> 
> I thought about it at first. And now I tried it, but it gets messy for a
number
> of reasons. See draft CL: https://codereview.webrtc.org/1289753008/-
> - The two use different logging macros; RTC_HISTOGRAM_COUNTS_100 vs
> RTC_HISTOGRAM_COUNTS_1000. They both expand to the same underlying macro, so I
> could generalize to that, but that means adding another state variable that
> keeps track of the range (100 vs 1000).

I think https://codereview.webrtc.org/1289753008/ is good. If people want to add
new stats, they can focus on the actual stats.

You may even make RegisterEvent() and RegisterPacketWaitingTime(int) into a
common method by making it a template, i.e. Register(<T>& a), or Register(const
void* a) without template.

> - They require different input. One just registers that events happen (void
> input) while the other registers magnitudes of events (int input).
> Specialization needed.
> - And since they would require different input parameters for the event
> registration, the owner must know the actual class type, and not just the
> (presumed) base logger class type. So, we would not really have a list of
> objects that can be traversed.

Sure

> 
> I could add a manager class, but I see little value when the only thing it
would
> do is batch-call the AdvanceClock method.

True, skip it.

[hlundin-webrtc, 2015-08-18 13:32:02]

On 2015/08/18 12:07:43, minyue-webrtc wrote:
> On 2015/08/18 07:41:44, hlundin-webrtc wrote:
> >
>
https://codereview.webrtc.org/1284303003/diff/1/webrtc/modules/audio_coding/n...
> > File webrtc/modules/audio_coding/neteq/statistics_calculator.h (right):
> > 
> >
>
https://codereview.webrtc.org/1284303003/diff/1/webrtc/modules/audio_coding/n...
> > webrtc/modules/audio_coding/neteq/statistics_calculator.h:116: class
> > AverageExcessBufferDelayMs {
> > On 2015/08/17 13:38:08, minyue-webrtc wrote:
> > > Since AverageExcessBufferDelayMs and DelayedPacketOutagesPerMinuteCounter
> and
> > > probably others are similar in implementation, is it possible to create a
> > common
> > > ancestor to share codes, and use a list of the parent class to hold them
to
> > > avoid explicitly calling the AdvanceClock() one-by-one, or optionally use
a
> > > manager to hold them and put the operations inside the manager?
> > 
> > I thought about it at first. And now I tried it, but it gets messy for a
> number
> > of reasons. See draft CL: https://codereview.webrtc.org/1289753008/-
> > - The two use different logging macros; RTC_HISTOGRAM_COUNTS_100 vs
> > RTC_HISTOGRAM_COUNTS_1000. They both expand to the same underlying macro, so
I
> > could generalize to that, but that means adding another state variable that
> > keeps track of the range (100 vs 1000).
> 
> I think https://codereview.webrtc.org/1289753008/ is good. If people want to
add
> new stats, they can focus on the actual stats.
> 
> You may even make RegisterEvent() and RegisterPacketWaitingTime(int) into a
> common method by making it a template, i.e. Register(<T>& a), or
Register(const
> void* a) without template.
> 
> > - They require different input. One just registers that events happen (void
> > input) while the other registers magnitudes of events (int input).
> > Specialization needed.
> > - And since they would require different input parameters for the event
> > registration, the owner must know the actual class type, and not just the
> > (presumed) base logger class type. So, we would not really have a list of
> > objects that can be traversed.
> 
> Sure
> 
> > 
> > I could add a manager class, but I see little value when the only thing it
> would
> > do is batch-call the AdvanceClock method.
> 
> True, skip it.

This CL is replaced by https://codereview.webrtc.org/1287333005/.
Closing.