ACM: Move NACK functionality inside NetEq

ACM: Move NACK functionality inside NetEq

Negative acknowledgement (NACK) has up to now been implemented in
ACM. But, since NetEq is in charge of the actual packet buffer, it
makes more sense to have the NACK functionlaity in there.

This CL does the following:
- Move nack.{h,cc} and the unit tests from main/acm2 to neteq.
- Move the NACK related code in ACM into NetEq.
- NACK related functions in AcmReceiver are changed to simple
  forwarding APIs.
- Remove unused members in AcmReceiver.
- Remove unused API functions in NetEq.

BUG=webrtc:3520
Committed: https://crrev.com/48ed930975ef7e84023044ed584c4eff914e6c9a
Cr-Commit-Position: refs/heads/master@{#10448}

[hlundin-webrtc, 2015-10-26 14:36:50]

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

[commit-bot: I haz the power, 2015-10-26 14:36:57]

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

[hlundin-webrtc, 2015-10-26 14:52:11]

Description was changed from

==========
ACM: Move NACK functionality inside NetEq

Negative acknowledgement (NACK) has up to now been implemented in
ACM. But, since NetEq is in charge of the actual packet buffer, it
makes more sense to have the NACK functionlaity in there.

This CL does the following:
- Move nack.{h,cc} and the unit tests from main/acm2 to neteq.
- Move the NACK related code in ACM into NetEq.
- NACK related functions in AcmReceiver are changed to simple
  forwarding APIs.
- Remove unused members in AcmReceiver.
- Remove unused API functions in NetEq.

BUG=webrtc:3520
==========

to

==========
ACM: Move NACK functionality inside NetEq

Negative acknowledgement (NACK) has up to now been implemented in
ACM. But, since NetEq is in charge of the actual packet buffer, it
makes more sense to have the NACK functionlaity in there.

This CL does the following:
- Move nack.{h,cc} and the unit tests from main/acm2 to neteq.
- Move the NACK related code in ACM into NetEq.
- NACK related functions in AcmReceiver are changed to simple
  forwarding APIs.
- Remove unused members in AcmReceiver.
- Remove unused API functions in NetEq.

BUG=webrtc:3520
==========

[hlundin-webrtc, 2015-10-26 14:52:11]

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

[hlundin-webrtc, 2015-10-26 15:17:49]

Minyue,

Please, take a look at this CL.

Thanks!

[minyue-webrtc, 2015-10-27 14:35:08]

good and some comments

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

https://codereview.webrtc.org/1410073006/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/nack.cc:142:
nack_list_.begin()->second.time_to_play_ms <= 10)
I know that breaking at ";" can follow the "," rule. But I am not sure about
indentation on breaking line at operators.

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

https://codereview.webrtc.org/1410073006/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/nack_unittest.cc:111:
kSequenceNumberLostPackets[n] +
what's the benefit of breaking line 110

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

https://codereview.webrtc.org/1410073006/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:421:
RTC_CHECK_EQ(nack_->SetMaxNackListSize(max_nack_list_size), 0);
why not make SetMaxNackListSize() a void and move 409 to 412 in to it

https://codereview.webrtc.org/1410073006/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:431:
RTC_CHECK_GE(round_trip_time_ms, 0);
is this necessary to crash?

https://codereview.webrtc.org/1410073006/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:638: RTC_DCHECK(nack_);
nack_.get()?

[hlundin-webrtc, 2015-10-28 15:02:16]

Updates after review

[hlundin-webrtc, 2015-10-28 15:03:36]

Thanks. PTAL again.

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

https://codereview.webrtc.org/1410073006/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/nack.cc:142:
nack_list_.begin()->second.time_to_play_ms <= 10)
On 2015/10/27 14:35:08, minyue-webrtc wrote:
> I know that breaking at ";" can follow the "," rule. But I am not sure about
> indentation on breaking line at operators.

This is what git cl format gives me.

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

https://codereview.webrtc.org/1410073006/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/nack_unittest.cc:111:
kSequenceNumberLostPackets[n] +
On 2015/10/27 14:35:08, minyue-webrtc wrote:
> what's the benefit of breaking line 110

Because git cl format...

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

https://codereview.webrtc.org/1410073006/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:421:
RTC_CHECK_EQ(nack_->SetMaxNackListSize(max_nack_list_size), 0);
On 2015/10/27 14:35:08, minyue-webrtc wrote:
> why not make SetMaxNackListSize() a void and move 409 to 412 in to it

Good idea. Done.

https://codereview.webrtc.org/1410073006/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:431:
RTC_CHECK_GE(round_trip_time_ms, 0);
On 2015/10/27 14:35:08, minyue-webrtc wrote:
> is this necessary to crash?

No, you are right. Changed to DCHECK. But I also moved it into
Nack::GetNackList.

https://codereview.webrtc.org/1410073006/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:638: RTC_DCHECK(nack_);
On 2015/10/27 14:35:08, minyue-webrtc wrote:
> nack_.get()?

Not needed. This expands "in the obvious way" through some clever tricks in
scoped_ptr.

[minyue-webrtc, 2015-10-29 10:36:10]

nice, lgtm if you considered a last comment.

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

https://codereview.webrtc.org/1410073006/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/nack.cc:142:
nack_list_.begin()->second.time_to_play_ms <= 10)
On 2015/10/28 15:03:36, hlundin-webrtc wrote:
> On 2015/10/27 14:35:08, minyue-webrtc wrote:
> > I know that breaking at ";" can follow the "," rule. But I am not sure about
> > indentation on breaking line at operators.
> 
> This is what git cl format gives me.

Acknowledged.

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

https://codereview.webrtc.org/1410073006/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/nack_unittest.cc:111:
kSequenceNumberLostPackets[n] +
On 2015/10/28 15:03:36, hlundin-webrtc wrote:
> On 2015/10/27 14:35:08, minyue-webrtc wrote:
> > what's the benefit of breaking line 110
> 
> Because git cl format...

Acknowledged.

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

https://codereview.webrtc.org/1410073006/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:638: RTC_DCHECK(nack_);
On 2015/10/28 15:03:36, hlundin-webrtc wrote:
> On 2015/10/27 14:35:08, minyue-webrtc wrote:
> > nack_.get()?
> 
> Not needed. This expands "in the obvious way" through some clever tricks in
> scoped_ptr.

Ack, but we use get() elsewhere a lot, see line 436. I let you decide.

[hlundin-webrtc, 2015-10-29 10:38:18]

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

https://codereview.webrtc.org/1410073006/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:638: RTC_DCHECK(nack_);
On 2015/10/29 10:36:10, minyue-webrtc wrote:
> On 2015/10/28 15:03:36, hlundin-webrtc wrote:
> > On 2015/10/27 14:35:08, minyue-webrtc wrote:
> > > nack_.get()?
> > 
> > Not needed. This expands "in the obvious way" through some clever tricks in
> > scoped_ptr.
> 
> Ack, but we use get() elsewhere a lot, see line 436. I let you decide.

I know, but I'm trying to do better now. :)

[hlundin-webrtc, 2015-10-29 10:44:01]

rebase

[hlundin-webrtc, 2015-10-29 10:45:25]

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

[hlundin-webrtc, 2015-10-29 10:45:26]

The patchset sent to the CQ was uploaded after l-g-t-m from minyue@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1410073006/#ps60001 (title:
"rebase")

[commit-bot: I haz the power, 2015-10-29 10:45:35]

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

[kwiberg-webrtc, 2015-10-29 10:46:00]

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

[kwiberg-webrtc, 2015-10-29 10:46:01]

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

https://codereview.webrtc.org/1410073006/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:638: RTC_DCHECK(nack_);
On 2015/10/29 10:38:18, hlundin-webrtc wrote:
> On 2015/10/29 10:36:10, minyue-webrtc wrote:
> > On 2015/10/28 15:03:36, hlundin-webrtc wrote:
> > > On 2015/10/27 14:35:08, minyue-webrtc wrote:
> > > > nack_.get()?
> > > 
> > > Not needed. This expands "in the obvious way" through some clever tricks
in
> > > scoped_ptr.
> > 
> > Ack, but we use get() elsewhere a lot, see line 436. I let you decide.
> 
> I know, but I'm trying to do better now. :)

+1

[commit-bot: I haz the power, 2015-10-29 12:36:28]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-10-29 12:36:36]

Patchset 4 (id:??) landed as
https://crrev.com/48ed930975ef7e84023044ed584c4eff914e6c9a
Cr-Commit-Position: refs/heads/master@{#10448}