Add NACK rate throttling for audio channels.

Add NACK rate throttling for audio channels.

Not really used for audio today (already in place for video), but should
still function anyway.

BUG=
R=henrika@webrtc.org, minyue@webrtc.org, stefan@webrtc.org

Committed: https://chromium.googlesource.com/external/webrtc/+/737336d37afb29e20ea406b875c74fbd9f95f1b8

[sprang_webrtc, 2016-07-27 08:14:03]

sprang@webrtc.org changed reviewers:
+ minyue@webrtc.org, stefan@webrtc.org

[sprang_webrtc, 2016-07-27 08:14:04]

ptal
It seems very difficult at the moment to be able to get a reference to the
congestion controller when constructing the voice engine, so I a did a simpler
throttling per channel, similar to the previous behavior.
The test was also difficult to write, as the receive side won't generate NACKs -
I have to inject them manually. If you can think of a cleaner way to do this
plmk...

[stefan-webrtc, 2016-07-28 07:27:38]

I don't think I follow what you meant with congestion controller being hard to
use in voe::Channel. Come by and explain. :)

https://codereview.webrtc.org/2181383002/diff/1/webrtc/video/end_to_end_tests.cc
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/2181383002/diff/1/webrtc/video/end_to_end_tests...
webrtc/video/end_to_end_tests.cc:493: TEST_F(EndToEndTest,
ReceivesNackAndRetransmitsAudio) {
Is there any chance we could merge this test with the one above, or do both need
to be that media type dependent that it's not possible?

https://codereview.webrtc.org/2181383002/diff/1/webrtc/video/end_to_end_tests...
webrtc/video/end_to_end_tests.cc:534:
(*send_channel_)->ReceivedRTCPPacket(buffer.data(), buffer.size()));
Why can't we insert the rtcp packet the regular way via Call::DeliverPacket? I
think ChannelProxy was meant to be an internal  class and not an API.

https://codereview.webrtc.org/2181383002/diff/1/webrtc/voice_engine/channel.cc
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/2181383002/diff/1/webrtc/voice_engine/channel.c...
webrtc/voice_engine/channel.cc:547:
nack_rate_limiter_->SetWindowSize(nack_window_ms);
This seems like a slightly odd place for this. Wouldn't it make more sense to do
this where an rtcp packet is recieved, as that actually may mean that the rtt is
updated?

https://codereview.webrtc.org/2181383002/diff/1/webrtc/voice_engine/channel.c...
webrtc/voice_engine/channel.cc:949: configuration.retransmission_rate_limiter =
nack_rate_limiter_.get();
should we rename it retransmission_rate_limiter_ instead of nack_rate_limiter_?

https://codereview.webrtc.org/2181383002/diff/1/webrtc/voice_engine/channel.c...
webrtc/voice_engine/channel.cc:1369:
nack_rate_limiter_->SetMaxRate(bitrate_bps);
What will the default be if this is never called? Most of the time audio isn't
hooked up to the bitrate allocator.

[minyue-webrtc, 2016-07-28 08:17:27]

generally look good in VoE, (but I am not an owner of VoE, please loop henrika
in)

https://codereview.webrtc.org/2181383002/diff/1/webrtc/voice_engine/channel.cc
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/2181383002/diff/1/webrtc/voice_engine/channel.c...
webrtc/voice_engine/channel.cc:547:
nack_rate_limiter_->SetWindowSize(nack_window_ms);
On 2016/07/28 07:27:38, stefan-webrtc (holmer) wrote:
> This seems like a slightly odd place for this. Wouldn't it make more sense to
do
> this where an rtcp packet is recieved, as that actually may mean that the rtt
is
> updated?

It might be good to put in VoERtcpObserver

[sprang_webrtc, 2016-07-28 13:00:57]

sprang@webrtc.org changed reviewers:
+ henrika@webrtc.org

[sprang_webrtc, 2016-07-28 13:00:58]

+henrika for voe

https://codereview.webrtc.org/2181383002/diff/1/webrtc/video/end_to_end_tests.cc
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/2181383002/diff/1/webrtc/video/end_to_end_tests...
webrtc/video/end_to_end_tests.cc:493: TEST_F(EndToEndTest,
ReceivesNackAndRetransmitsAudio) {
On 2016/07/28 07:27:38, stefan-webrtc (holmer) wrote:
> Is there any chance we could merge this test with the one above, or do both
need
> to be that media type dependent that it's not possible?

The big difference here is that for video, we drop rtp packets and check that
the receiver sends nacks. That's not gonna work for audio, I need to manually
inject NACK packets into the sender...

https://codereview.webrtc.org/2181383002/diff/1/webrtc/video/end_to_end_tests...
webrtc/video/end_to_end_tests.cc:534:
(*send_channel_)->ReceivedRTCPPacket(buffer.data(), buffer.size()));
On 2016/07/28 07:27:38, stefan-webrtc (holmer) wrote:
> Why can't we insert the rtcp packet the regular way via Call::DeliverPacket? I
> think ChannelProxy was meant to be an internal  class and not an API.

Yep. I had totally missed the Call::Receiver() method. I tried getting at
internal::AudioSendStream::DeliverRtcp(), but that's an internal class that
can't really be accessed from this class so I tried to get at it via a backdoor
:)

Removed all channelproxy changes and use Call instead. Thanks!

https://codereview.webrtc.org/2181383002/diff/1/webrtc/voice_engine/channel.cc
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/2181383002/diff/1/webrtc/voice_engine/channel.c...
webrtc/voice_engine/channel.cc:547:
nack_rate_limiter_->SetWindowSize(nack_window_ms);
On 2016/07/28 07:27:38, stefan-webrtc (holmer) wrote:
> This seems like a slightly odd place for this. Wouldn't it make more sense to
do
> this where an rtcp packet is recieved, as that actually may mean that the rtt
is
> updated?

Done.

https://codereview.webrtc.org/2181383002/diff/1/webrtc/voice_engine/channel.c...
webrtc/voice_engine/channel.cc:949: configuration.retransmission_rate_limiter =
nack_rate_limiter_.get();
On 2016/07/28 07:27:38, stefan-webrtc (holmer) wrote:
> should we rename it retransmission_rate_limiter_ instead of
nack_rate_limiter_?

Done.

https://codereview.webrtc.org/2181383002/diff/1/webrtc/voice_engine/channel.c...
webrtc/voice_engine/channel.cc:1369:
nack_rate_limiter_->SetMaxRate(bitrate_bps);
On 2016/07/28 07:27:38, stefan-webrtc (holmer) wrote:
> What will the default be if this is never called? Most of the time audio isn't
> hooked up to the bitrate allocator.

Rate limit defaults to maxint, so always allows retransimssion. That sound like
the right thing?

[henrika_webrtc, 2016-07-28 13:03:07]

What effect will this change have on audio?

[sprang_webrtc, 2016-07-28 13:25:05]

On 2016/07/28 13:03:07, henrika_webrtc wrote:
> What effect will this change have on audio?

Main thing, it won't crash if a receiver sends a NACK our way. :)
Throttling of retransmissions should otherwise behave in the same manner as it
did before it was moved from within the RTP modules to an external rate limiter.

[henrika_webrtc, 2016-07-28 13:42:53]

LGTM (voice_engine)

[minyue-webrtc, 2016-07-28 13:50:14]

lgtm

[stefan-webrtc, 2016-07-28 15:06:16]

https://codereview.webrtc.org/2181383002/diff/1/webrtc/video/end_to_end_tests.cc
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/2181383002/diff/1/webrtc/video/end_to_end_tests...
webrtc/video/end_to_end_tests.cc:493: TEST_F(EndToEndTest,
ReceivesNackAndRetransmitsAudio) {
On 2016/07/28 13:00:58, språng wrote:
> On 2016/07/28 07:27:38, stefan-webrtc (holmer) wrote:
> > Is there any chance we could merge this test with the one above, or do both
> need
> > to be that media type dependent that it's not possible?
> 
> The big difference here is that for video, we drop rtp packets and check that
> the receiver sends nacks. That's not gonna work for audio, I need to manually
> inject NACK packets into the sender...

Acknowledged.

https://codereview.webrtc.org/2181383002/diff/1/webrtc/voice_engine/channel.cc
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/2181383002/diff/1/webrtc/voice_engine/channel.c...
webrtc/voice_engine/channel.cc:1369:
nack_rate_limiter_->SetMaxRate(bitrate_bps);
On 2016/07/28 13:00:58, språng wrote:
> On 2016/07/28 07:27:38, stefan-webrtc (holmer) wrote:
> > What will the default be if this is never called? Most of the time audio
isn't
> > hooked up to the bitrate allocator.
> 
> Rate limit defaults to maxint, so always allows retransimssion. That sound
like
> the right thing?

Acknowledged.

https://codereview.webrtc.org/2181383002/diff/60001/webrtc/call/rtc_event_log...
File webrtc/call/rtc_event_log_unittest.cc (right):

https://codereview.webrtc.org/2181383002/diff/60001/webrtc/call/rtc_event_log...
webrtc/call/rtc_event_log_unittest.cc:115: RateLimiter
retranmission_rate_limiter(clock, 1000);
retransmission

https://codereview.webrtc.org/2181383002/diff/60001/webrtc/video/end_to_end_t...
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/2181383002/diff/60001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:551: std::unique_ptr<test::PacketTransport>*
const send_transport_;
Doesn't it make more sense to store a test::PacketTransport* here? More clearly
shows that it's not owned by this class IMO.

https://codereview.webrtc.org/2181383002/diff/60001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:1879: retransmission_rate_limiter_(clock_,
1000) {}
Why is this needed in this test now?

https://codereview.webrtc.org/2181383002/diff/60001/webrtc/video/rtp_stream_r...
File webrtc/video/rtp_stream_receiver.h (right):

https://codereview.webrtc.org/2181383002/diff/60001/webrtc/video/rtp_stream_r...
webrtc/video/rtp_stream_receiver.h:66: RateLimiter* retranmission_rate_limiter);
retransmission

[sprang_webrtc, 2016-07-28 15:17:18]

https://codereview.webrtc.org/2181383002/diff/60001/webrtc/call/rtc_event_log...
File webrtc/call/rtc_event_log_unittest.cc (right):

https://codereview.webrtc.org/2181383002/diff/60001/webrtc/call/rtc_event_log...
webrtc/call/rtc_event_log_unittest.cc:115: RateLimiter
retranmission_rate_limiter(clock, 1000);
On 2016/07/28 15:06:15, stefan-webrtc (holmer) wrote:
> retransmission

Done.

https://codereview.webrtc.org/2181383002/diff/60001/webrtc/video/end_to_end_t...
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/2181383002/diff/60001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:551: std::unique_ptr<test::PacketTransport>*
const send_transport_;
On 2016/07/28 15:06:15, stefan-webrtc (holmer) wrote:
> Doesn't it make more sense to store a test::PacketTransport* here? More
clearly
> shows that it's not owned by this class IMO.

This is problematic, because this unique_ptr isn't populated when constructing
|test|; it's set up while running RunBaseTest()...

https://codereview.webrtc.org/2181383002/diff/60001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:1879: retransmission_rate_limiter_(clock_,
1000) {}
On 2016/07/28 15:06:15, stefan-webrtc (holmer) wrote:
> Why is this needed in this test now?

Because I DCHECK in the RtpSender constructor that we always have a
retransmission rate limiter. Just so we don't miss any more cases...

https://codereview.webrtc.org/2181383002/diff/60001/webrtc/video/rtp_stream_r...
File webrtc/video/rtp_stream_receiver.h (right):

https://codereview.webrtc.org/2181383002/diff/60001/webrtc/video/rtp_stream_r...
webrtc/video/rtp_stream_receiver.h:66: RateLimiter* retranmission_rate_limiter);
On 2016/07/28 15:06:15, stefan-webrtc (holmer) wrote:
> retransmission

Done.

[stefan-webrtc, 2016-07-28 15:24:07]

https://codereview.webrtc.org/2181383002/diff/60001/webrtc/video/end_to_end_t...
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/2181383002/diff/60001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:551: std::unique_ptr<test::PacketTransport>*
const send_transport_;
On 2016/07/28 15:17:18, språng wrote:
> On 2016/07/28 15:06:15, stefan-webrtc (holmer) wrote:
> > Doesn't it make more sense to store a test::PacketTransport* here? More
> clearly
> > shows that it's not owned by this class IMO.
> 
> This is problematic, because this unique_ptr isn't populated when constructing
> |test|; it's set up while running RunBaseTest()...

Ah... In that case I'd suggest creating your own transport instead, see
EndToEndTest.RembWithSendSideBwe for an example of the receive transport
version:

    test::PacketTransport* CreateReceiveTransport() override {
      receive_transport_ = new test::PacketTransport(
          nullptr, this, test::PacketTransport::kReceiver,
          FakeNetworkPipe::Config());
      return receive_transport_;
    }

https://codereview.webrtc.org/2181383002/diff/60001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:1879: retransmission_rate_limiter_(clock_,
1000) {}
On 2016/07/28 15:17:17, språng wrote:
> On 2016/07/28 15:06:15, stefan-webrtc (holmer) wrote:
> > Why is this needed in this test now?
> 
> Because I DCHECK in the RtpSender constructor that we always have a
> retransmission rate limiter. Just so we don't miss any more cases...

I see, that's probably a good idea.

[sprang_webrtc, 2016-07-29 08:00:05]

https://codereview.webrtc.org/2181383002/diff/60001/webrtc/video/end_to_end_t...
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/2181383002/diff/60001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:551: std::unique_ptr<test::PacketTransport>*
const send_transport_;
On 2016/07/28 15:24:07, stefan-webrtc (holmer) wrote:
> On 2016/07/28 15:17:18, språng wrote:
> > On 2016/07/28 15:06:15, stefan-webrtc (holmer) wrote:
> > > Doesn't it make more sense to store a test::PacketTransport* here? More
> > clearly
> > > shows that it's not owned by this class IMO.
> > 
> > This is problematic, because this unique_ptr isn't populated when
constructing
> > |test|; it's set up while running RunBaseTest()...
> 
> Ah... In that case I'd suggest creating your own transport instead, see
> EndToEndTest.RembWithSendSideBwe for an example of the receive transport
> version:
> 
>     test::PacketTransport* CreateReceiveTransport() override {
>       receive_transport_ = new test::PacketTransport(
>           nullptr, this, test::PacketTransport::kReceiver,
>           FakeNetworkPipe::Config());
>       return receive_transport_;
>     }

Done.

[stefan-webrtc, 2016-07-29 08:42:16]

lgtm with nit fixed.

https://codereview.webrtc.org/2181383002/diff/100001/webrtc/video/end_to_end_...
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/2181383002/diff/100001/webrtc/video/end_to_end_...
webrtc/video/end_to_end_tests.cc:507: test::PacketTransport* receive_transport =
new test::PacketTransport(
Directly assign to receive_transport_ here?

[sprang_webrtc, 2016-07-29 09:05:08]

https://codereview.webrtc.org/2181383002/diff/100001/webrtc/video/end_to_end_...
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/2181383002/diff/100001/webrtc/video/end_to_end_...
webrtc/video/end_to_end_tests.cc:507: test::PacketTransport* receive_transport =
new test::PacketTransport(
On 2016/07/29 08:42:16, stefan-webrtc (holmer) wrote:
> Directly assign to receive_transport_ here?

I'd prefer not, as that would mean that I need to change the type of
|receive_transport_| to test::PacketTransport* and then add a
static_cast<Transport*> where I actually call it.

[stefan-webrtc, 2016-07-29 09:20:57]

lgtm

[sprang_webrtc, 2016-07-29 09:25:33]

The CQ bit was checked by sprang@webrtc.org

[sprang_webrtc, 2016-07-29 09:25:33]

The patchset sent to the CQ was uploaded after l-g-t-m from minyue@webrtc.org,
henrika@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2181383002/#ps100001 (title:
"Addressed comments")

[commit-bot: I haz the power, 2016-07-29 09:25:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-07-29 09:26:45]

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

[commit-bot: I haz the power, 2016-07-29 09:26:47]

Try jobs failed on following builders:
  android_compile_x64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x64_dbg...)
  android_compile_x86_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x86_dbg...)
  android_gn_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_gn_dbg/builds/1...)
  android_gn_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_gn_rel/builds/1...)
  ios32_sim_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/9150)
  ios64_gn_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_gn_dbg/builds/1419)
  ios64_gn_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_gn_rel/builds/1443)
  ios64_sim_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/9151)
  ios_api_framework on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_api_framework/build...)
  ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/11468)
  ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/11397)
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/16707)
  ios_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/15353)
  linux_asan on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/16342)
  linux_libfuzzer_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...)
  linux_tsan2 on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/13810)
  linux_ubsan on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan/builds/3685)
  linux_ubsan_vptr on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds...)
  mac_baremetal on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/12969)
  mac_compile_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)
  mac_gn_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_dbg/builds/4865)
  mac_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/16786)

[sprang_webrtc, 2016-07-29 09:31:27]

The CQ bit was checked by sprang@webrtc.org

[sprang_webrtc, 2016-07-29 09:31:27]

The patchset sent to the CQ was uploaded after l-g-t-m from minyue@webrtc.org,
stefan@webrtc.org, henrika@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2181383002/#ps120001 (title:
"Rebase")

[commit-bot: I haz the power, 2016-07-29 09:31:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-07-29 10:06:07]

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

[commit-bot: I haz the power, 2016-07-29 10:06:08]

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

[sprang_webrtc, 2016-07-29 10:59:53]

Description was changed from

==========
Add NACK rate throttling for audio channels.

Not really used for audio today (already in place for video), but should
still function anyway.

BUG=
==========

to

==========
Add NACK rate throttling for audio channels.

Not really used for audio today (already in place for video), but should
still function anyway.

BUG=
R=henrika@webrtc.org, minyue@webrtc.org, stefan@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/737336d37afb29e20ea406b87...
==========

[commit-bot: I haz the power, 2016-07-29 10:59:54]

Description was changed from

==========
Add NACK rate throttling for audio channels.

Not really used for audio today (already in place for video), but should
still function anyway.

BUG=
R=henrika@webrtc.org, minyue@webrtc.org, stefan@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/737336d37afb29e20ea406b87...
==========

to

==========
Add NACK rate throttling for audio channels.

Not really used for audio today (already in place for video), but should
still function anyway.

BUG=
R=henrika@webrtc.org, minyue@webrtc.org, stefan@webrtc.org

Committed: https://crrev.com/737336d37afb29e20ea406b875c74fbd9f95f1b8
Cr-Commit-Position: refs/heads/master@{#13571}
==========

[sprang_webrtc, 2016-07-29 10:59:55]

Committed patchset #7 (id:120001) manually as
737336d37afb29e20ea406b875c74fbd9f95f1b8 (presubmit successful).