Add a new TickTimer class to NetEq

Add a new TickTimer class to NetEq

The new class is intended to be used as a central time-keeping object
inside NetEq. The actual use of the class will come in subsequent
changes.

BUG=webrtc:5608
Committed: https://crrev.com/8053f79bd9c6ef8b79ee1d5254b42fa3f8647626
Cr-Commit-Position: refs/heads/master@{#12477}

[hlundin-webrtc, 2016-04-21 08:10:27]

Patchset #1 (id:1) has been deleted

[hlundin-webrtc, 2016-04-21 08:11:12]

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

[hlundin-webrtc, 2016-04-21 08:11:12]

Tina,

Please, review this CL.

Thanks!

[tlegrand-webrtc, 2016-04-21 10:37:44]

LG, but consider moving the 10 ms to be an input to the class. By doing that,
you can use the class for "ticking" an arbitrary number of ms.

https://codereview.webrtc.org/1910523003/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/tick_timer.h (right):

https://codereview.webrtc.org/1910523003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/tick_timer.h:41: return elapsed_ticks <
UINT64_MAX / 10 ? elapsed_ticks * 10 : UINT64_MAX;
The class TickTimer might be usable for other time intervals. Maybe we should
take the number of ms as an input then creating the class instead? If not, I
think 10 ms needs to be somewhere in the name of the class.

https://codereview.webrtc.org/1910523003/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/tick_timer_unittest.cc (right):

https://codereview.webrtc.org/1910523003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/tick_timer_unittest.cc:87:
EXPECT_EQ(UINT64_MAX, sw->ElapsedMs());
Is the expected behavior that we cap at UINT64_MAX if running for too long? Is
that what we test here?

[hlundin-webrtc, 2016-04-21 13:50:58]

https://codereview.webrtc.org/1910523003/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/tick_timer.h (right):

https://codereview.webrtc.org/1910523003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/tick_timer.h:41: return elapsed_ticks <
UINT64_MAX / 10 ? elapsed_ticks * 10 : UINT64_MAX;
On 2016/04/21 10:37:44, tlegrand-webrtc wrote:
> The class TickTimer might be usable for other time intervals. Maybe we should
> take the number of ms as an input then creating the class instead? If not, I
> think 10 ms needs to be somewhere in the name of the class.

Acknowledged. I think I'm going to say YAGNI on the variable tick interval, and
just let 10ms be part of the name. Another option is to not have the ElapsedMs()
method, and instead let the caller (NetEq) call ElapsedTicks() and do the
conversion itself. I'll work something out.

https://codereview.webrtc.org/1910523003/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/tick_timer_unittest.cc (right):

https://codereview.webrtc.org/1910523003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/tick_timer_unittest.cc:87:
EXPECT_EQ(UINT64_MAX, sw->ElapsedMs());
On 2016/04/21 10:37:44, tlegrand-webrtc wrote:
> Is the expected behavior that we cap at UINT64_MAX if running for too long? Is
> that what we test here?

ElapsedMs will cap, which is what we test. Since the underlying tick counter is
an uint64_t, any multiple (larger than 1) of the elapsed number of ticks will
reach its maximum before the tick counter does. So, if we want to have
conversion to ms inside the Stopwatch, I think it is nice to have this capping
to avoid the weird situation where ElapsedMs() wraps before ElapsedTicks().

In the end, 2^64 ms is a very long time; 584 million years to be precise.

[hlundin-webrtc, 2016-04-22 07:46:03]

PTAL again.

https://codereview.webrtc.org/1910523003/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/tick_timer.h (right):

https://codereview.webrtc.org/1910523003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/tick_timer.h:41: return elapsed_ticks <
UINT64_MAX / 10 ? elapsed_ticks * 10 : UINT64_MAX;
On 2016/04/21 13:50:58, hlundin-webrtc wrote:
> On 2016/04/21 10:37:44, tlegrand-webrtc wrote:
> > The class TickTimer might be usable for other time intervals. Maybe we
should
> > take the number of ms as an input then creating the class instead? If not, I
> > think 10 ms needs to be somewhere in the name of the class.
> 
> Acknowledged. I think I'm going to say YAGNI on the variable tick interval,
and
> just let 10ms be part of the name. Another option is to not have the
ElapsedMs()
> method, and instead let the caller (NetEq) call ElapsedTicks() and do the
> conversion itself. I'll work something out.

Updated.

[tlegrand-webrtc, 2016-04-22 11:43:22]

Nice! LGTM

https://codereview.webrtc.org/1910523003/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/tick_timer_unittest.cc (right):

https://codereview.webrtc.org/1910523003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/tick_timer_unittest.cc:87:
EXPECT_EQ(UINT64_MAX, sw->ElapsedMs());
On 2016/04/21 13:50:58, hlundin-webrtc wrote:
> On 2016/04/21 10:37:44, tlegrand-webrtc wrote:
> > Is the expected behavior that we cap at UINT64_MAX if running for too long?
Is
> > that what we test here?
> 
> ElapsedMs will cap, which is what we test. Since the underlying tick counter
is
> an uint64_t, any multiple (larger than 1) of the elapsed number of ticks will
> reach its maximum before the tick counter does. So, if we want to have
> conversion to ms inside the Stopwatch, I think it is nice to have this capping
> to avoid the weird situation where ElapsedMs() wraps before ElapsedTicks().
> 
> In the end, 2^64 ms is a very long time; 584 million years to be precise.

Acknowledged.

[hlundin-webrtc, 2016-04-22 13:36:46]

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

[commit-bot: I haz the power, 2016-04-22 13:36:55]

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

[commit-bot: I haz the power, 2016-04-22 13:44:23]

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

[commit-bot: I haz the power, 2016-04-22 13:44:24]

Try jobs failed on following builders:
  presubmit on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/5012)

[hlundin-webrtc, 2016-04-22 14:03:57]

Description was changed from

==========
Add a new TickTimer class to NetEq

The new class is intended to be used as a central time-keeping object
inside NetEq. The actual use of the class will come in subsequent
changes.

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

to

==========
Add a new TickTimer class to NetEq

The new class is intended to be used as a central time-keeping object
inside NetEq. The actual use of the class will come in subsequent
changes.

BUG=webrtc:5608
NOPRESUBMIT=True
==========

[hlundin-webrtc, 2016-04-22 19:48:42]

Description was changed from

==========
Add a new TickTimer class to NetEq

The new class is intended to be used as a central time-keeping object
inside NetEq. The actual use of the class will come in subsequent
changes.

BUG=webrtc:5608
NOPRESUBMIT=True
==========

to

==========
Add a new TickTimer class to NetEq

The new class is intended to be used as a central time-keeping object
inside NetEq. The actual use of the class will come in subsequent
changes.

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

[hlundin-webrtc, 2016-04-22 19:48:53]

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

[commit-bot: I haz the power, 2016-04-22 19:49:06]

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

[commit-bot: I haz the power, 2016-04-22 20:21:46]

Description was changed from

==========
Add a new TickTimer class to NetEq

The new class is intended to be used as a central time-keeping object
inside NetEq. The actual use of the class will come in subsequent
changes.

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

to

==========
Add a new TickTimer class to NetEq

The new class is intended to be used as a central time-keeping object
inside NetEq. The actual use of the class will come in subsequent
changes.

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

[commit-bot: I haz the power, 2016-04-22 20:21:47]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2016-04-22 20:21:54]

Description was changed from

==========
Add a new TickTimer class to NetEq

The new class is intended to be used as a central time-keeping object
inside NetEq. The actual use of the class will come in subsequent
changes.

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

to

==========
Add a new TickTimer class to NetEq

The new class is intended to be used as a central time-keeping object
inside NetEq. The actual use of the class will come in subsequent
changes.

BUG=webrtc:5608

Committed: https://crrev.com/8053f79bd9c6ef8b79ee1d5254b42fa3f8647626
Cr-Commit-Position: refs/heads/master@{#12477}
==========

[commit-bot: I haz the power, 2016-04-22 20:21:55]

Patchset 2 (id:??) landed as
https://crrev.com/8053f79bd9c6ef8b79ee1d5254b42fa3f8647626
Cr-Commit-Position: refs/heads/master@{#12477}