Configure VoE NACK through AudioSendStream::Config.

Configure VoE NACK through AudioSendStream::Config, for send streams.

BUG=webrtc:4690
Committed: https://crrev.com/971cab0d93823af9a991aa8901eda75f574e0974
Cr-Commit-Position: refs/heads/master@{#13136}

[the sun, 2016-06-13 09:10:28]

Description was changed from

==========
Configure VoE NACK through Audio[Receive|Send]Stream::Config.

BUG=webrtc:4690
==========

to

==========
Configure VoE NACK through AudioSendStream::Config, for send streams.

BUG=webrtc:4690
==========

[the sun, 2016-06-13 09:10:28]

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

[kwiberg-webrtc, 2016-06-13 13:30:31]

https://codereview.webrtc.org/1955363003/diff/40001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right):

https://codereview.webrtc.org/1955363003/diff/40001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine_unittest.cc:52: const int kRtpHistoryMs =
5000;
constexpr

https://codereview.webrtc.org/1955363003/diff/40001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine_unittest.cc:1368: EXPECT_EQ(kRtpHistoryMs,
GetSendStreamConfig(kSsrc1).rtp.nack.rtp_history_ms);
Why do you replace some GetNACK calls, but not all of them?

[kwiberg-webrtc, 2016-06-13 13:35:30]

https://codereview.webrtc.org/1955363003/diff/40001/webrtc/audio_receive_stre...
File webrtc/audio_receive_stream.h (right):

https://codereview.webrtc.org/1955363003/diff/40001/webrtc/audio_receive_stre...
webrtc/audio_receive_stream.h:83: NackConfig nack;
Does this belong in this CL?

[the sun, 2016-06-13 15:51:44]

https://codereview.webrtc.org/1955363003/diff/40001/webrtc/audio_receive_stre...
File webrtc/audio_receive_stream.h (right):

https://codereview.webrtc.org/1955363003/diff/40001/webrtc/audio_receive_stre...
webrtc/audio_receive_stream.h:83: NackConfig nack;
On 2016/06/13 13:35:30, kwiberg-webrtc wrote:
> Does this belong in this CL?

Not after I split the receive stream changes into a separate CL.

https://codereview.webrtc.org/1955363003/diff/40001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right):

https://codereview.webrtc.org/1955363003/diff/40001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine_unittest.cc:52: const int kRtpHistoryMs =
5000;
On 2016/06/13 13:30:31, kwiberg-webrtc wrote:
> constexpr

Changed here, and in webrtcvoiceengince.cc.

https://codereview.webrtc.org/1955363003/diff/40001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine_unittest.cc:1368: EXPECT_EQ(kRtpHistoryMs,
GetSendStreamConfig(kSsrc1).rtp.nack.rtp_history_ms);
On 2016/06/13 13:30:31, kwiberg-webrtc wrote:
> Why do you replace some GetNACK calls, but not all of them?

Because some of them are for receive streams, which I made a separate CL for.

[kwiberg-webrtc, 2016-06-14 13:07:43]

lgtm

The diff between patch set #3 and #4 is hard to read, because you've rebased in
addition to fixing comments. The required changes were very simple, though, so I
trust you got them right.

https://codereview.webrtc.org/1955363003/diff/40001/webrtc/audio_receive_stre...
File webrtc/audio_receive_stream.h (right):

https://codereview.webrtc.org/1955363003/diff/40001/webrtc/audio_receive_stre...
webrtc/audio_receive_stream.h:83: NackConfig nack;
On 2016/06/13 15:51:43, the sun wrote:
> On 2016/06/13 13:35:30, kwiberg-webrtc wrote:
> > Does this belong in this CL?
> 
> Not after I split the receive stream changes into a separate CL.

Acknowledged.

https://codereview.webrtc.org/1955363003/diff/40001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right):

https://codereview.webrtc.org/1955363003/diff/40001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine_unittest.cc:1368: EXPECT_EQ(kRtpHistoryMs,
GetSendStreamConfig(kSsrc1).rtp.nack.rtp_history_ms);
On 2016/06/13 15:51:43, the sun wrote:
> On 2016/06/13 13:30:31, kwiberg-webrtc wrote:
> > Why do you replace some GetNACK calls, but not all of them?
> 
> Because some of them are for receive streams, which I made a separate CL for.

Acknowledged.

[the sun, 2016-06-14 14:08:06]

The CQ bit was checked by solenberg@webrtc.org

[the sun, 2016-06-14 14:08:07]

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

[commit-bot: I haz the power, 2016-06-14 14:08:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1955363003/100001

[the sun, 2016-06-14 15:42:35]

The CQ bit was checked by solenberg@webrtc.org

[the sun, 2016-06-14 15:42:35]

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

[commit-bot: I haz the power, 2016-06-14 15:42:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1955363003/120001

[the sun, 2016-06-14 15:43:16]

On 2016/06/14 13:07:43, kwiberg-webrtc wrote:
> lgtm
> 
> The diff between patch set #3 and #4 is hard to read, because you've rebased
in
> addition to fixing comments. The required changes were very simple, though, so
I
> trust you got them right.

Sorry about that.

[commit-bot: I haz the power, 2016-06-14 17:02:44]

Description was changed from

==========
Configure VoE NACK through AudioSendStream::Config, for send streams.

BUG=webrtc:4690
==========

to

==========
Configure VoE NACK through AudioSendStream::Config, for send streams.

BUG=webrtc:4690
==========

[commit-bot: I haz the power, 2016-06-14 17:02:45]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-06-14 17:02:52]

Description was changed from

==========
Configure VoE NACK through AudioSendStream::Config, for send streams.

BUG=webrtc:4690
==========

to

==========
Configure VoE NACK through AudioSendStream::Config, for send streams.

BUG=webrtc:4690

Committed: https://crrev.com/971cab0d93823af9a991aa8901eda75f574e0974
Cr-Commit-Position: refs/heads/master@{#13136}
==========

[commit-bot: I haz the power, 2016-06-14 17:02:54]

Patchset 7 (id:??) landed as
https://crrev.com/971cab0d93823af9a991aa8901eda75f574e0974
Cr-Commit-Position: refs/heads/master@{#13136}