Fixes a bug where a video stream can get stuck in the suspended state.

webrtc/call/rampup_tests.cc

Index: webrtc/call/rampup_tests.cc
diff --git a/webrtc/call/rampup_tests.cc b/webrtc/call/rampup_tests.cc
index 46e97cd2a5352ad97b9f6901bb5e7c6bda92fb24..94153b08ae62287978313b3cfcef526f6037e017 100644
--- a/webrtc/call/rampup_tests.cc
+++ b/webrtc/call/rampup_tests.cc
@@ -11,6 +11,7 @@
 #include "webrtc/call/rampup_tests.h"
 
 #include "webrtc/base/checks.h"
+#include "webrtc/base/logging.h"
 #include "webrtc/base/platform_thread.h"
 #include "webrtc/test/encoder_settings.h"
 #include "webrtc/test/gtest.h"
@@ -20,7 +21,7 @@ namespace webrtc {
 namespace {
 
 static const int64_t kPollIntervalMs = 20;
-static const int kExpectedHighVideoBitrateBps = 60000;
+static const int kExpectedHighVideoBitrateBps = 80000;
 static const int kExpectedHighAudioBitrateBps = 30000;
 static const int kLowBandwidthLimitBps = 20000;
 static const int kExpectedLowBitrateBps = 20000;
@@ -35,6 +36,7 @@ std::vector<uint32_t> GenerateSsrcs(size_t num_streams, uint32_t ssrc_offset) {
 
 RampUpTester::RampUpTester(size_t num_video_streams,
                            size_t num_audio_streams,
+                           size_t num_flexfec_streams,
                            unsigned int start_bitrate_bps,
                            int64_t min_run_time_ms,
                            const std::string& extension_type,
@@ -46,6 +48,7 @@ RampUpTester::RampUpTester(size_t num_video_streams,
       clock_(Clock::GetRealTimeClock()),
       num_video_streams_(num_video_streams),
       num_audio_streams_(num_audio_streams),
+      num_flexfec_streams_(num_flexfec_streams),
       rtx_(rtx),
       red_(red),
       sender_call_(nullptr),
@@ -63,6 +66,8 @@ RampUpTester::RampUpTester(size_t num_video_streams,
       poller_thread_(&BitrateStatsPollingThread,
                      this,
                      "BitrateStatsPollingThread") {
+  if (red_)
+    EXPECT_EQ(0u, num_flexfec_streams_);
   EXPECT_LE(num_audio_streams_, 1u);
 }
 
@@ -100,6 +105,10 @@ size_t RampUpTester::GetNumAudioStreams() const {
   return num_audio_streams_;
 }
 
+size_t RampUpTester::GetNumFlexfecStreams() const {
+  return num_flexfec_streams_;
+}
+
 class RampUpTester::VideoStreamFactory
     : public VideoEncoderConfig::VideoStreamFactoryInterface {
  public:
@@ -209,6 +218,13 @@ void RampUpTester::ModifyVideoConfigs(
     }
     ++i;
   }
+
+  RTC_DCHECK_LE(num_flexfec_streams_, 1);
+  if (num_flexfec_streams_ == 1) {
+    send_config->rtp.flexfec.payload_type = test::CallTest::kFlexfecPayloadType;
+    send_config->rtp.flexfec.ssrc = test::CallTest::kFlexfecSendSsrc;
+    send_config->rtp.flexfec.protected_media_ssrcs = {video_ssrcs_[0]};
+  }
 }
 
 void RampUpTester::ModifyAudioConfigs(
@@ -219,6 +235,8 @@ void RampUpTester::ModifyAudioConfigs(
 
   EXPECT_NE(RtpExtension::kTimestampOffsetUri, extension_type_)
       << "Audio BWE not supported with toffset.";
+  EXPECT_NE(RtpExtension::kAbsSendTimeUri, extension_type_)
+      << "Audio BWE not supported with abs-send-time.";
 
   send_config->rtp.ssrc = audio_ssrcs_[0];
   send_config->rtp.extensions.clear();
@@ -227,11 +245,7 @@ void RampUpTester::ModifyAudioConfigs(
   send_config->max_bitrate_bps = 60000;
 
   bool transport_cc = false;
-  if (extension_type_ == RtpExtension::kAbsSendTimeUri) {
-    transport_cc = false;
-    send_config->rtp.extensions.push_back(
-        RtpExtension(extension_type_.c_str(), kAbsSendTimeExtensionId));
-  } else if (extension_type_ == RtpExtension::kTransportSequenceNumberUri) {
+  if (extension_type_ == RtpExtension::kTransportSequenceNumberUri) {
     transport_cc = true;
     send_config->rtp.extensions.push_back(RtpExtension(
         extension_type_.c_str(), kTransportSequenceNumberExtensionId));
@@ -244,6 +258,26 @@ void RampUpTester::ModifyAudioConfigs(
   }
 }
 
+void RampUpTester::ModifyFlexfecConfigs(
+    std::vector<FlexfecReceiveStream::Config>* receive_configs) {
+  if (num_flexfec_streams_ == 0)
+    return;
+  RTC_DCHECK_EQ(1, num_flexfec_streams_);
+  (*receive_configs)[0].payload_type = test::CallTest::kFlexfecPayloadType;
+  (*receive_configs)[0].remote_ssrc = test::CallTest::kFlexfecSendSsrc;
+  (*receive_configs)[0].protected_media_ssrcs = {video_ssrcs_[0]};
+  (*receive_configs)[0].local_ssrc = video_ssrcs_[0];
+  if (extension_type_ == RtpExtension::kAbsSendTimeUri) {
+    (*receive_configs)[0].transport_cc = false;
+    (*receive_configs)[0].rtp_header_extensions.push_back(
+        RtpExtension(extension_type_.c_str(), kAbsSendTimeExtensionId));
+  } else if (extension_type_ == RtpExtension::kTransportSequenceNumberUri) {
+    (*receive_configs)[0].transport_cc = true;
+    (*receive_configs)[0].rtp_header_extensions.push_back(RtpExtension(
+        extension_type_.c_str(), kTransportSequenceNumberExtensionId));
+  }
+}
+
 void RampUpTester::OnCallsCreated(Call* sender_call, Call* receiver_call) {
   sender_call_ = sender_call;
 }
@@ -351,23 +385,32 @@ void RampUpTester::PerformTest() {
 
 RampUpDownUpTester::RampUpDownUpTester(size_t num_video_streams,
                                        size_t num_audio_streams,
+                                       size_t num_flexfec_streams,
                                        unsigned int start_bitrate_bps,
                                        const std::string& extension_type,
                                        bool rtx,
-                                       bool red)
+                                       bool red,
+                                       const std::vector<int>& loss_rates)
     : RampUpTester(num_video_streams,
                    num_audio_streams,
+                   num_flexfec_streams,
                    start_bitrate_bps,
                    0,
                    extension_type,
                    rtx,
                    red,
                    true),
+      link_rates_({GetHighLinkCapacity(), kLowBandwidthLimitBps / 1000,
+                   GetHighLinkCapacity(), 0}),
       test_state_(kFirstRampup),
+      next_state_(kTransitionToNextState),
       state_start_ms_(clock_->TimeInMilliseconds()),
       interval_start_ms_(clock_->TimeInMilliseconds()),
-      sent_bytes_(0) {
-  forward_transport_config_.link_capacity_kbps = GetHighLinkCapacity();
+      sent_bytes_(0),
+      loss_rates_(loss_rates) {
+  forward_transport_config_.link_capacity_kbps = link_rates_[test_state_];
+  forward_transport_config_.queue_delay_ms = 100;
+  forward_transport_config_.loss_percent = loss_rates_[test_state_];
 }
 
 RampUpDownUpTester::~RampUpDownUpTester() {}
@@ -432,57 +475,79 @@ int RampUpDownUpTester::GetHighLinkCapacity() const {
   return 4 * GetExpectedHighBitrate() / (3 * 1000);
 }
 
+size_t RampUpDownUpTester::GetFecBytes() const {
+  size_t flex_fec_bytes = 0;
+  if (num_flexfec_streams_ > 0) {
+    webrtc::VideoSendStream::Stats stats = send_stream_->GetStats();
+    for (const auto& kv : stats.substreams)
+      flex_fec_bytes += kv.second.rtp_stats.fec.TotalBytes();
+  }
+  return flex_fec_bytes;
+}
+
+bool RampUpDownUpTester::ExpectingFec() const {
+  return num_flexfec_streams_ > 0 && forward_transport_config_.loss_percent > 0;
+}
+
 void RampUpDownUpTester::EvolveTestState(int bitrate_bps, bool suspended) {
   int64_t now = clock_->TimeInMilliseconds();
   switch (test_state_) {
-    case kFirstRampup: {
+    case kFirstRampup:
       EXPECT_FALSE(suspended);
       if (bitrate_bps >= GetExpectedHighBitrate()) {
-        // The first ramp-up has reached the target bitrate. Change the
-        // channel limit, and move to the next test state.
-        forward_transport_config_.link_capacity_kbps =
-            kLowBandwidthLimitBps / 1000;
-        send_transport_->SetConfig(forward_transport_config_);
-        test_state_ = kLowRate;
         webrtc::test::PrintResult("ramp_up_down_up", GetModifierString(),
                                   "first_rampup", now - state_start_ms_, "ms",
                                   false);
-        state_start_ms_ = now;
-        interval_start_ms_ = now;
-        sent_bytes_ = 0;
+        // Apply loss during the transition between states if FEC is enabled.
+        forward_transport_config_.loss_percent = loss_rates_[test_state_];
+        test_state_ = kTransitionToNextState;
+        next_state_ = kLowRate;
       }
       break;
-    }
     case kLowRate: {
       // Audio streams are never suspended.
       bool check_suspend_state = num_video_streams_ > 0;
       if (bitrate_bps < kExpectedLowBitrateBps &&
           suspended == check_suspend_state) {
-        // The ramp-down was successful. Change the channel limit back to a
-        // high value, and move to the next test state.
-        forward_transport_config_.link_capacity_kbps = GetHighLinkCapacity();
-        send_transport_->SetConfig(forward_transport_config_);
-        test_state_ = kSecondRampup;
         webrtc::test::PrintResult("ramp_up_down_up", GetModifierString(),
                                   "rampdown", now - state_start_ms_, "ms",
                                   false);
-        state_start_ms_ = now;
-        interval_start_ms_ = now;
-        sent_bytes_ = 0;
+        // Apply loss during the transition between states if FEC is enabled.
+        forward_transport_config_.loss_percent = loss_rates_[test_state_];
+        test_state_ = kTransitionToNextState;
+        next_state_ = kSecondRampup;
       }
       break;
     }
-    case kSecondRampup: {
+    case kSecondRampup:
       if (bitrate_bps >= GetExpectedHighBitrate() && !suspended) {
         webrtc::test::PrintResult("ramp_up_down_up", GetModifierString(),
                                   "second_rampup", now - state_start_ms_, "ms",
                                   false);
         ReportResult("ramp-up-down-up-average-network-latency",
                      send_transport_->GetAverageDelayMs(), "milliseconds");
-        observation_complete_.Set();
+        // Apply loss during the transition between states if FEC is enabled.
+        forward_transport_config_.loss_percent = loss_rates_[test_state_];
+        test_state_ = kTransitionToNextState;
+        next_state_ = kTestEnd;
+      }
+      break;
+    case kTestEnd:
+      observation_complete_.Set();
+      break;
+    case kTransitionToNextState:
+      if (!ExpectingFec() || GetFecBytes() > 0) {
+        test_state_ = next_state_;
+        forward_transport_config_.link_capacity_kbps = link_rates_[test_state_];
+        // No loss while ramping up and down as it may affect the BWE
+        // negatively, making the test flaky.
+        forward_transport_config_.loss_percent = 0;
+        state_start_ms_ = now;
+        interval_start_ms_ = now;
+        sent_bytes_ = 0;
+        send_transport_->SetConfig(forward_transport_config_);
       }
       break;
-    }
   }
 }
 
@@ -499,70 +564,83 @@ class RampUpTest : public test::CallTest {
 static const uint32_t kStartBitrateBps = 60000;
 
 TEST_F(RampUpTest, UpDownUpAbsSendTimeSimulcastRedRtx) {
-  RampUpDownUpTester test(3, 0, kStartBitrateBps, RtpExtension::kAbsSendTimeUri,
-                          true, true);
+  std::vector<int> loss_rates = {0, 0, 0, 0};

[terelius, 2017/02/21 10:29:24]

Would it make sense to set loss rate through a method rather than the
constructor? Positive loss seems to be the exceptional case, so it would be nice
to avoid the all-zero vector in every test case.

[stefan-webrtc, 2017/02/21 10:45:18]

Added a constructor instead.

+  RampUpDownUpTester test(3, 0, 0, kStartBitrateBps,
+                          RtpExtension::kAbsSendTimeUri, true, true,
+                          loss_rates);
   RunBaseTest(&test);
 }
 
 TEST_F(RampUpTest, UpDownUpTransportSequenceNumberRtx) {
-  RampUpDownUpTester test(3, 0, kStartBitrateBps,
+  std::vector<int> loss_rates = {0, 0, 0, 0};
+  RampUpDownUpTester test(3, 0, 0, kStartBitrateBps,
                           RtpExtension::kTransportSequenceNumberUri, true,
-                          false);
+                          false, loss_rates);
+  RunBaseTest(&test);
+}
+
+TEST_F(RampUpTest, UpDownUpTransportSequenceNumberPacketLoss) {
+  std::vector<int> loss_rates = {20, 0, 0, 0};
+  RampUpDownUpTester test(1, 0, 1, kStartBitrateBps,
+                          RtpExtension::kTransportSequenceNumberUri, true,
+                          false, loss_rates);
   RunBaseTest(&test);
 }
 
 TEST_F(RampUpTest, UpDownUpAudioVideoTransportSequenceNumberRtx) {
-  RampUpDownUpTester test(3, 1, kStartBitrateBps,
+  std::vector<int> loss_rates = {0, 0, 0, 0};
+  RampUpDownUpTester test(3, 1, 0, kStartBitrateBps,
                           RtpExtension::kTransportSequenceNumberUri, true,
-                          false);
+                          false, loss_rates);
   RunBaseTest(&test);
 }
 
 TEST_F(RampUpTest, UpDownUpAudioTransportSequenceNumberRtx) {
-  RampUpDownUpTester test(0, 1, kStartBitrateBps,
+  std::vector<int> loss_rates = {0, 0, 0, 0};
+  RampUpDownUpTester test(0, 1, 0, kStartBitrateBps,
                           RtpExtension::kTransportSequenceNumberUri, true,
-                          false);
+                          false, loss_rates);
   RunBaseTest(&test);
 }
 
 TEST_F(RampUpTest, TOffsetSimulcastRedRtx) {
-  RampUpTester test(3, 0, 0, 0, RtpExtension::kTimestampOffsetUri, true, true,
-                    true);
+  RampUpTester test(3, 0, 0, 0, 0, RtpExtension::kTimestampOffsetUri, true,
+                    true, true);
   RunBaseTest(&test);
 }
 
 TEST_F(RampUpTest, AbsSendTime) {
-  RampUpTester test(1, 0, 0, 0, RtpExtension::kAbsSendTimeUri, false, false,
+  RampUpTester test(1, 0, 0, 0, 0, RtpExtension::kAbsSendTimeUri, false, false,
                     true);
   RunBaseTest(&test);
 }
 
 TEST_F(RampUpTest, AbsSendTimeSimulcastRedRtx) {
-  RampUpTester test(3, 0, 0, 0, RtpExtension::kAbsSendTimeUri, true, true,
+  RampUpTester test(3, 0, 0, 0, 0, RtpExtension::kAbsSendTimeUri, true, true,
                     true);
   RunBaseTest(&test);
 }
 
 TEST_F(RampUpTest, TransportSequenceNumber) {
-  RampUpTester test(1, 0, 0, 0, RtpExtension::kTransportSequenceNumberUri,
+  RampUpTester test(1, 0, 0, 0, 0, RtpExtension::kTransportSequenceNumberUri,
                     false, false, true);
   RunBaseTest(&test);
 }
 
 TEST_F(RampUpTest, TransportSequenceNumberSimulcast) {
-  RampUpTester test(3, 0, 0, 0, RtpExtension::kTransportSequenceNumberUri,
+  RampUpTester test(3, 0, 0, 0, 0, RtpExtension::kTransportSequenceNumberUri,
                     false, false, true);
   RunBaseTest(&test);
 }
 
 TEST_F(RampUpTest, TransportSequenceNumberSimulcastRedRtx) {
-  RampUpTester test(3, 0, 0, 0, RtpExtension::kTransportSequenceNumberUri,
+  RampUpTester test(3, 0, 0, 0, 0, RtpExtension::kTransportSequenceNumberUri,
                     true, true, true);
   RunBaseTest(&test);
 }
 
 TEST_F(RampUpTest, AudioTransportSequenceNumber) {
-  RampUpTester test(0, 1, 300000, 10000,
+  RampUpTester test(0, 1, 0, 300000, 10000,
                     RtpExtension::kTransportSequenceNumberUri, false, false,
                     false);
   RunBaseTest(&test);