NetEq: Introduce TickTimer in DelayPeakDetector

NetEq: Introduce TickTimer in DelayPeakDetector

Specifically, this change replaces peak_period_counter_ms_ with
peak_period_stopwatch_ which obtains a Stopwatch object from
TickTimer. Necessary plumbing to get the TickTimer through to the
DelayPeakDetector is also included.

BUG=webrtc:5608
NOTRY=True

Committed: https://crrev.com/f393370f02562e9ea89d542978f4b616333715e1
Cr-Commit-Position: refs/heads/master@{#12542}

[hlundin-webrtc, 2016-04-26 06:39:08]

henrik.lundin@webrtc.org changed reviewers:
+ tina.legrand@webrtc.org

[hlundin-webrtc, 2016-04-26 06:39:09]

Tina,

CL #4. Please, take a look when you get a chance.

Thanks!

[hlundin-webrtc, 2016-04-26 12:03:59]

Pass TickTimer as const ptr rather than const ref

[hlundin-webrtc, 2016-04-26 20:25:37]

On 2016/04/26 12:03:59, hlundin-webrtc wrote:
> Pass TickTimer as const ptr rather than const ref

Hold the review for a while. I will have to make a rebase.

[hlundin-webrtc, 2016-04-27 08:46:41]

rebase

[hlundin-webrtc, 2016-04-27 08:49:14]

This is now ready for review again. I suggest you look at the diff between base
and the latest patch set.

[tlegrand-webrtc, 2016-04-27 10:42:31]

Nice and clean set of CLs, well done!
A few comments inline.

https://codereview.webrtc.org/1921163003/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/delay_peak_detector.cc (right):

https://codereview.webrtc.org/1921163003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/delay_peak_detector.cc:37:
peak_period_stopwatch_.reset();
What happens if Reset is called before first call to Update?

https://codereview.webrtc.org/1921163003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/delay_peak_detector.cc:63: uint64_t max_period
= 0;  // Returns 0 for an empty history.
Does it ever happen that we return 0 (before this change, -1)? How does the
change in behavior affect the caller?

https://codereview.webrtc.org/1921163003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/delay_peak_detector.cc:107:
peak_period_stopwatch_->ElapsedMs() <= 2 * MaxPeakPeriod()) {
I guess here is the only place where MaxPeakPeriod is a called, right? If I get
it right, changing the function to return 0 instead of -1 for an empty history
will not result in this being "true", due to "s" being less then
kMinPeaksToTrigger. Right? If this is true, you don't need to care about my
comment above. :)

https://codereview.webrtc.org/1921163003/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/delay_peak_detector.h (right):

https://codereview.webrtc.org/1921163003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/delay_peak_detector.h:42: // Returns -1 if no
delay peaks have been observed recently.
You can remove the -1 comment, since it is now an unsigned value.

https://codereview.webrtc.org/1921163003/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/delay_peak_detector_unittest.cc (right):

https://codereview.webrtc.org/1921163003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/delay_peak_detector_unittest.cc:20: TickTimer
tt;
I prefer tick_timer, but up to you to decide if you're willing to change.

[hlundin-webrtc, 2016-04-27 13:03:10]

Fixing comments after review

[hlundin-webrtc, 2016-04-27 13:04:14]

Thanks, and PTAL again.

https://codereview.webrtc.org/1921163003/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/delay_peak_detector.cc (right):

https://codereview.webrtc.org/1921163003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/delay_peak_detector.cc:37:
peak_period_stopwatch_.reset();
On 2016/04/27 10:42:30, tlegrand-webrtc wrote:
> What happens if Reset is called before first call to Update?

Nothing will happen. It is fine to call reset() on an already empty unique_ptr.

https://codereview.webrtc.org/1921163003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/delay_peak_detector.cc:63: uint64_t max_period
= 0;  // Returns 0 for an empty history.
On 2016/04/27 10:42:30, tlegrand-webrtc wrote:
> Does it ever happen that we return 0 (before this change, -1)? How does the
> change in behavior affect the caller?

We only return 0 when the peak_history_ is empty. All peaks have a strictly
positive period by construction.

I took the opportunity to rewrite this method using std::max_element and a
simple lambda, and then also added a DCHECK to verify that the return is >0 for
non-empty history.

https://codereview.webrtc.org/1921163003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/delay_peak_detector.cc:107:
peak_period_stopwatch_->ElapsedMs() <= 2 * MaxPeakPeriod()) {
On 2016/04/27 10:42:30, tlegrand-webrtc wrote:
> I guess here is the only place where MaxPeakPeriod is a called, right? If I
get
> it right, changing the function to return 0 instead of -1 for an empty history
> will not result in this being "true", due to "s" being less then
> kMinPeaksToTrigger. Right? If this is true, you don't need to care about my
> comment above. :)

Acknowledged.

https://codereview.webrtc.org/1921163003/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/delay_peak_detector.h (right):

https://codereview.webrtc.org/1921163003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/delay_peak_detector.h:42: // Returns -1 if no
delay peaks have been observed recently.
On 2016/04/27 10:42:30, tlegrand-webrtc wrote:
> You can remove the -1 comment, since it is now an unsigned value.

Done.

https://codereview.webrtc.org/1921163003/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/delay_peak_detector_unittest.cc (right):

https://codereview.webrtc.org/1921163003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/delay_peak_detector_unittest.cc:20: TickTimer
tt;
On 2016/04/27 10:42:30, tlegrand-webrtc wrote:
> I prefer tick_timer, but up to you to decide if you're willing to change.

Done.

[hlundin-webrtc, 2016-04-27 14:03:45]

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

[commit-bot: I haz the power, 2016-04-27 14:03:54]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1921163003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1921163003/60001

[commit-bot: I haz the power, 2016-04-27 16:04:24]

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

[commit-bot: I haz the power, 2016-04-27 16:04:25]

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

[tlegrand-webrtc, 2016-04-28 08:32:32]

LGTM with one nit.

https://codereview.webrtc.org/1921163003/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/delay_peak_detector.cc (right):

https://codereview.webrtc.org/1921163003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/delay_peak_detector.cc:37:
peak_period_stopwatch_.reset();
On 2016/04/27 13:04:14, hlundin-webrtc wrote:
> On 2016/04/27 10:42:30, tlegrand-webrtc wrote:
> > What happens if Reset is called before first call to Update?
> 
> Nothing will happen. It is fine to call reset() on an already empty
unique_ptr.

Acknowledged.

https://codereview.webrtc.org/1921163003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/delay_peak_detector.cc:63: uint64_t max_period
= 0;  // Returns 0 for an empty history.
On 2016/04/27 13:04:14, hlundin-webrtc wrote:
> On 2016/04/27 10:42:30, tlegrand-webrtc wrote:
> > Does it ever happen that we return 0 (before this change, -1)? How does the
> > change in behavior affect the caller?
> 
> We only return 0 when the peak_history_ is empty. All peaks have a strictly
> positive period by construction.
> 
> I took the opportunity to rewrite this method using std::max_element and a
> simple lambda, and then also added a DCHECK to verify that the return is >0
for
> non-empty history.

Acknowledged.

https://codereview.webrtc.org/1921163003/diff/60001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/delay_peak_detector.h (right):

https://codereview.webrtc.org/1921163003/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/delay_peak_detector.h:43: // Returns -1 if no
delay peaks have been observed recently.
You missed the last comment which says "Returns -1..."

[hlundin-webrtc, 2016-04-28 08:41:19]

Correcting the function comments

[hlundin-webrtc, 2016-04-28 08:44:02]

https://codereview.webrtc.org/1921163003/diff/60001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/delay_peak_detector.h (right):

https://codereview.webrtc.org/1921163003/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/delay_peak_detector.h:43: // Returns -1 if no
delay peaks have been observed recently.
On 2016/04/28 08:32:32, tlegrand-webrtc wrote:
> You missed the last comment which says "Returns -1..."

My bad. I modified the comment for MaxPeakHeight() instead of this one. Thanks
for finding.

[hlundin-webrtc, 2016-04-28 08:44:50]

Description was changed from

==========
NetEq: Introduce TickTimer in DelayPeakDetector

Specifically, this change replaces peak_period_counter_ms_ with
peak_period_stopwatch_ which obtains a Stopwatch object from
TickTimer. Necessary plumbing to get the TickTimer through to the
DelayPeakDetector is also included.

BUG=webrtc:5608
==========

to

==========
NetEq: Introduce TickTimer in DelayPeakDetector

Specifically, this change replaces peak_period_counter_ms_ with
peak_period_stopwatch_ which obtains a Stopwatch object from
TickTimer. Necessary plumbing to get the TickTimer through to the
DelayPeakDetector is also included.

BUG=webrtc:5608
NOTRY=True
==========

[hlundin-webrtc, 2016-04-28 08:47:25]

The CQ bit was checked by henrik.lundin@webrtc.org

[hlundin-webrtc, 2016-04-28 08:47:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
tina.legrand@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1921163003/#ps80001 (title:
"Correcting the function comments")

[commit-bot: I haz the power, 2016-04-28 08:47:34]

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

[commit-bot: I haz the power, 2016-04-28 08:54:29]

Description was changed from

==========
NetEq: Introduce TickTimer in DelayPeakDetector

Specifically, this change replaces peak_period_counter_ms_ with
peak_period_stopwatch_ which obtains a Stopwatch object from
TickTimer. Necessary plumbing to get the TickTimer through to the
DelayPeakDetector is also included.

BUG=webrtc:5608
NOTRY=True
==========

to

==========
NetEq: Introduce TickTimer in DelayPeakDetector

Specifically, this change replaces peak_period_counter_ms_ with
peak_period_stopwatch_ which obtains a Stopwatch object from
TickTimer. Necessary plumbing to get the TickTimer through to the
DelayPeakDetector is also included.

BUG=webrtc:5608
NOTRY=True
==========

[commit-bot: I haz the power, 2016-04-28 08:54:30]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-05-01 22:01:10]

Description was changed from

==========
NetEq: Introduce TickTimer in DelayPeakDetector

Specifically, this change replaces peak_period_counter_ms_ with
peak_period_stopwatch_ which obtains a Stopwatch object from
TickTimer. Necessary plumbing to get the TickTimer through to the
DelayPeakDetector is also included.

BUG=webrtc:5608
NOTRY=True
==========

to

==========
NetEq: Introduce TickTimer in DelayPeakDetector

Specifically, this change replaces peak_period_counter_ms_ with
peak_period_stopwatch_ which obtains a Stopwatch object from
TickTimer. Necessary plumbing to get the TickTimer through to the
DelayPeakDetector is also included.

BUG=webrtc:5608
NOTRY=True

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

[commit-bot: I haz the power, 2016-05-01 22:01:11]

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