Add NACK rate throttling for audio channels.

webrtc/video/end_to_end_tests.cc

Index: webrtc/video/end_to_end_tests.cc
diff --git a/webrtc/video/end_to_end_tests.cc b/webrtc/video/end_to_end_tests.cc
index d0e99ab55df60e457ad2a75bbf09056dd2e83156..d8581ad018922ba0f447e18504d657b2f8b30554 100644
--- a/webrtc/video/end_to_end_tests.cc
+++ b/webrtc/video/end_to_end_tests.cc
@@ -19,12 +19,15 @@
 
 #include "webrtc/base/checks.h"
 #include "webrtc/base/event.h"
+#include "webrtc/base/optional.h"
+#include "webrtc/base/rate_limiter.h"
 #include "webrtc/call.h"
 #include "webrtc/call/transport_adapter.h"
 #include "webrtc/common_video/include/frame_callback.h"
 #include "webrtc/modules/include/module_common_types.h"
 #include "webrtc/modules/rtp_rtcp/include/rtp_rtcp.h"
 #include "webrtc/modules/rtp_rtcp/source/byte_io.h"
+#include "webrtc/modules/rtp_rtcp/source/rtcp_packet/nack.h"
 #include "webrtc/modules/rtp_rtcp/source/rtcp_packet/rapid_resync_request.h"
 #include "webrtc/modules/rtp_rtcp/source/rtcp_utility.h"
 #include "webrtc/modules/rtp_rtcp/source/rtp_utility.h"
@@ -487,6 +490,71 @@ TEST_F(EndToEndTest, ReceivesAndRetransmitsNack) {
   RunBaseTest(&test);
 }
 
+TEST_F(EndToEndTest, ReceivesNackAndRetransmitsAudio) {
+  class NackObserver : public test::EndToEndTest {
+   public:
+    explicit NackObserver(
+        std::unique_ptr<test::PacketTransport>* send_transport)
+        : EndToEndTest(kLongTimeoutMs),
+          local_ssrc_(0),
+          remote_ssrc_(0),
+          send_transport_(send_transport) {}
+
+   private:
+    size_t GetNumVideoStreams() const override { return 0; }
+    size_t GetNumAudioStreams() const override { return 1; }
+
+    Action OnSendRtp(const uint8_t* packet, size_t length) override {
+      RTPHeader header;
+      EXPECT_TRUE(parser_->Parse(packet, length, &header));
+
+      if (!sequence_number_to_retransmit_) {
+        sequence_number_to_retransmit_ =
+            rtc::Optional<uint16_t>(header.sequenceNumber);
+
+        // Don't ask for retransmission straight away, may be deduped in pacer.
+      } else if (header.sequenceNumber == *sequence_number_to_retransmit_) {
+        observation_complete_.Set();
+      } else {
+        // Send a NACK as often as necessary until retransmission is received.
+        rtcp::Nack nack;
+        nack.From(local_ssrc_);
+        nack.To(remote_ssrc_);
+        uint16_t nack_list[] = {*sequence_number_to_retransmit_};
+        nack.WithList(nack_list, 1);
+        rtc::Buffer buffer = nack.Build();
+
+        Transport* transport = static_cast<Transport*>(send_transport_->get());
+        EXPECT_TRUE(transport->SendRtcp(buffer.data(), buffer.size()));
+      }
+
+      return SEND_PACKET;
+    }
+
+    void ModifyAudioConfigs(
+        AudioSendStream::Config* send_config,
+        std::vector<AudioReceiveStream::Config>* receive_configs) override {
+      send_config->rtp.nack.rtp_history_ms = kNackRtpHistoryMs;
+      (*receive_configs)[0].rtp.nack.rtp_history_ms = kNackRtpHistoryMs;
+      local_ssrc_ = (*receive_configs)[0].rtp.local_ssrc;
+      remote_ssrc_ = (*receive_configs)[0].rtp.remote_ssrc;
+    }
+
+    void PerformTest() override {
+      EXPECT_TRUE(Wait())
+          << "Timed out waiting for packets to be NACKed, retransmitted and "
+             "rendered.";
+    }
+
+    uint32_t local_ssrc_;
+    uint32_t remote_ssrc_;
+    std::unique_ptr<test::PacketTransport>* const send_transport_;

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

Doesn't it make more sense to store a test::PacketTransport* here? More clearly
shows that it's not owned by this class IMO.

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

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

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

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_;
    }

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

Done.

+    rtc::Optional<uint16_t> sequence_number_to_retransmit_;
+  } test(&receive_transport_);
+
+  RunBaseTest(&test);
+}
+
 TEST_F(EndToEndTest, CanReceiveFec) {
   class FecRenderObserver : public test::EndToEndTest,
                             public rtc::VideoSinkInterface<VideoFrame> {
@@ -1807,7 +1875,8 @@ TEST_F(EndToEndTest, RembWithSendSideBwe) {
           poller_thread_(&BitrateStatsPollingThread,
                          this,
                          "BitrateStatsPollingThread"),
-          state_(kWaitForFirstRampUp) {}
+          state_(kWaitForFirstRampUp),
+          retransmission_rate_limiter_(clock_, 1000) {}

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

Why is this needed in this test now?

[sprang_webrtc, 2016/07/28 15:17:17]

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

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

I see, that's probably a good idea.

 
     ~BweObserver() {}
 
@@ -1847,6 +1916,7 @@ TEST_F(EndToEndTest, RembWithSendSideBwe) {
       config.receiver_only = true;
       config.clock = clock_;
       config.outgoing_transport = receive_transport_;
+      config.retransmission_rate_limiter = &retransmission_rate_limiter_;
       rtp_rtcp_.reset(RtpRtcp::CreateRtpRtcp(config));
       rtp_rtcp_->SetRemoteSSRC((*receive_configs)[0].rtp.remote_ssrc);
       rtp_rtcp_->SetSSRC((*receive_configs)[0].rtp.local_ssrc);
@@ -1919,6 +1989,7 @@ TEST_F(EndToEndTest, RembWithSendSideBwe) {
     rtc::Event event_;
     rtc::PlatformThread poller_thread_;
     TestState state_;
+    RateLimiter retransmission_rate_limiter_;
   } test;
 
   RunBaseTest(&test);