NetEq: Use TickTimer in PacketBuffer

NetEq: Use TickTimer in PacketBuffer

This change makes use of the TickTimer::Stopwatch in Packets. When a
packet is inserted into the PacketBuffer, a Stopwatch object is
attached to it. When the packet is extracted from the buffer, the
Stopwatch is read to know how long the packet waited in the buffer.

BUG=webrtc:5608
Committed: https://crrev.com/84f8cd6df467cf78c73d4acd4a10ca702b4ab320
Cr-Commit-Position: refs/heads/master@{#12508}

[hlundin-webrtc, 2016-04-25 17:32:18]

Patchset #1 (id:1) has been deleted

[hlundin-webrtc, 2016-04-25 17:33:34]

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

[hlundin-webrtc, 2016-04-25 17:33:35]

Tina,

CL #3. PTAL.

Thanks!

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

https://codereview.webrtc.org/1917913002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/packet.cc:1: /*
This file is needed to keep clang-style happy.

[tlegrand-webrtc, 2016-04-26 08:32:25]

LGTM, but consider consulting kwiberg to see if there are other options to the
added .cc file.

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

https://codereview.webrtc.org/1917913002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/packet.cc:1: /*
On 2016/04/25 17:33:35, hlundin-webrtc wrote:
> This file is needed to keep clang-style happy.

Maybe ask Karl if he know of a better way of getting around this?

[kwiberg-webrtc, 2016-04-26 09:04:12]

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

[kwiberg-webrtc, 2016-04-26 09:04:14]

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

https://codereview.webrtc.org/1917913002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/packet.cc:1: /*
On 2016/04/26 08:32:25, tlegrand-webrtc wrote:
> On 2016/04/25 17:33:35, hlundin-webrtc wrote:
> > This file is needed to keep clang-style happy.
> 
> Maybe ask Karl if he know of a better way of getting around this?

No; the clang style checks try to ensure that complex constructors and
destructors are non-inline, which means they have to be in a .cc file. (This is
actually for a good reason! It reduces code size by not pasting in a copy of the
code at each call site.) You could put them in an existing .cc file, but it
seems much cleaner and style-guide-compliant to give the .h file its own .cc
file.

(The style checks don't enforce this, but one thing to consider is whether the
other methods could usefully be moved to the .cc file as well.)

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

https://codereview.webrtc.org/1917913002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/packet_buffer.cc:43: :
max_number_of_packets_(max_number_of_packets), tick_timer_(tick_timer) {}
This looks a bit evil; to callers, it looks like they're passing the tick timer
by value, but you're saving a reference to it, which means they end up being
responsible for ensuring that it outlives the packet buffer.

Would it be very inconvenient to pass it by unique_ptr instead?

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

https://codereview.webrtc.org/1917913002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/packet_buffer.h:20: // Forward declarations.
I'm guessing you don't actually need this comment...

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

https://codereview.webrtc.org/1917913002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/packet_buffer_unittest.cc:85: PacketBuffer*
buffer = new PacketBuffer(10, tick_timer);  // 10 packets.
Case in point: It's not obvious to the casual observer why you couldn't simply
do this instead:

  PacketBuffer* buffer = new PacketBuffer(10, TickTimer());

[hlundin-webrtc, 2016-04-26 11:05:54]

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

https://codereview.webrtc.org/1917913002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/packet_buffer.cc:43: :
max_number_of_packets_(max_number_of_packets), tick_timer_(tick_timer) {}
On 2016/04/26 09:04:13, kwiberg-webrtc wrote:
> This looks a bit evil; to callers, it looks like they're passing the tick
timer
> by value, but you're saving a reference to it, which means they end up being
> responsible for ensuring that it outlives the packet buffer.
> 
> Would it be very inconvenient to pass it by unique_ptr instead?

Yes, it would be inconvenient, since the ownership is retained by the caller.
NetEqImpl owns a TickTimer and several of its member classes (like the
PacketBuffer) will need a handle to that one shared TickTimer.

I can make it more explicit by passing a const pointer instead of a const
reference. WDYT?

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

https://codereview.webrtc.org/1917913002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/packet_buffer.h:20: // Forward declarations.
On 2016/04/26 09:04:13, kwiberg-webrtc wrote:
> I'm guessing you don't actually need this comment...

Old habits die hard.

[kwiberg-webrtc, 2016-04-26 11:26:27]

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

https://codereview.webrtc.org/1917913002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/packet_buffer.cc:43: :
max_number_of_packets_(max_number_of_packets), tick_timer_(tick_timer) {}
On 2016/04/26 11:05:54, hlundin-webrtc wrote:
> On 2016/04/26 09:04:13, kwiberg-webrtc wrote:
> > This looks a bit evil; to callers, it looks like they're passing the tick
> timer
> > by value, but you're saving a reference to it, which means they end up being
> > responsible for ensuring that it outlives the packet buffer.
> > 
> > Would it be very inconvenient to pass it by unique_ptr instead?
> 
> Yes, it would be inconvenient, since the ownership is retained by the caller.
> NetEqImpl owns a TickTimer and several of its member classes (like the
> PacketBuffer) will need a handle to that one shared TickTimer.
> 
> I can make it more explicit by passing a const pointer instead of a const
> reference. WDYT?

Yes, that's much better than passing it by const reference.

[hlundin-webrtc, 2016-04-26 11:52:39]

Pass TickTimer as const ptr rather than const ref

[hlundin-webrtc, 2016-04-26 11:55:14]

Remove superfluous comment

[hlundin-webrtc, 2016-04-26 11:56:29]

PTAL again.

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

https://codereview.webrtc.org/1917913002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/packet_buffer.cc:43: :
max_number_of_packets_(max_number_of_packets), tick_timer_(tick_timer) {}
On 2016/04/26 11:26:27, kwiberg-webrtc wrote:
> On 2016/04/26 11:05:54, hlundin-webrtc wrote:
> > On 2016/04/26 09:04:13, kwiberg-webrtc wrote:
> > > This looks a bit evil; to callers, it looks like they're passing the tick
> > timer
> > > by value, but you're saving a reference to it, which means they end up
being
> > > responsible for ensuring that it outlives the packet buffer.
> > > 
> > > Would it be very inconvenient to pass it by unique_ptr instead?
> > 
> > Yes, it would be inconvenient, since the ownership is retained by the
caller.
> > NetEqImpl owns a TickTimer and several of its member classes (like the
> > PacketBuffer) will need a handle to that one shared TickTimer.
> > 
> > I can make it more explicit by passing a const pointer instead of a const
> > reference. WDYT?
> 
> Yes, that's much better than passing it by const reference.

Done.

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

https://codereview.webrtc.org/1917913002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/packet_buffer.h:20: // Forward declarations.
On 2016/04/26 11:05:54, hlundin-webrtc wrote:
> On 2016/04/26 09:04:13, kwiberg-webrtc wrote:
> > I'm guessing you don't actually need this comment...
> 
> Old habits die hard.

Done.

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

https://codereview.webrtc.org/1917913002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/packet_buffer_unittest.cc:85: PacketBuffer*
buffer = new PacketBuffer(10, tick_timer);  // 10 packets.
On 2016/04/26 09:04:13, kwiberg-webrtc wrote:
> Case in point: It's not obvious to the casual observer why you couldn't simply
> do this instead:
> 
>   PacketBuffer* buffer = new PacketBuffer(10, TickTimer());

Acknowledged.

[kwiberg-webrtc, 2016-04-26 12:40:33]

lgtm

[hlundin-webrtc, 2016-04-26 12:47:48]

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

[hlundin-webrtc, 2016-04-26 12:47:49]

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/1917913002/#ps60001 (title:
"Remove superfluous comment")

[commit-bot: I haz the power, 2016-04-26 12:47:56]

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

[commit-bot: I haz the power, 2016-04-26 14:45:21]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-04-26 14:45:31]

Description was changed from

==========
NetEq: Use TickTimer in PacketBuffer

This change makes use of the TickTimer::Stopwatch in Packets. When a
packet is inserted into the PacketBuffer, a Stopwatch object is
attached to it. When the packet is extracted from the buffer, the
Stopwatch is read to know how long the packet waited in the buffer.

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

to

==========
NetEq: Use TickTimer in PacketBuffer

This change makes use of the TickTimer::Stopwatch in Packets. When a
packet is inserted into the PacketBuffer, a Stopwatch object is
attached to it. When the packet is extracted from the buffer, the
Stopwatch is read to know how long the packet waited in the buffer.

BUG=webrtc:5608

Committed: https://crrev.com/84f8cd6df467cf78c73d4acd4a10ca702b4ab320
Cr-Commit-Position: refs/heads/master@{#12508}
==========

[commit-bot: I haz the power, 2016-04-26 14:45:33]

Patchset 3 (id:??) landed as
https://crrev.com/84f8cd6df467cf78c73d4acd4a10ca702b4ab320
Cr-Commit-Position: refs/heads/master@{#12508}