Reland of Fixes a bug where a video stream can get stuck in the suspended state. (patchset #1 id:1 of https://codereview.chromium.org/2703393002/ )
Reason for revert:
Downstream fixed
Original issue's description:
> Revert of Fixes a bug where a video stream can get stuck in the suspended state. (patchset #8 id:120001 of https://codereview.webrtc.org/2705603002/ )
>
> Reason for revert:
> Breaks downstream
>
> Original issue's description:
> > Fixes a bug where a video stream can get stuck in the suspended state.
> >
> > This happens if a lot of FEC is allocated when the stream becomes suspended. The required bitrate to unsuspend can then be too high so that the padding bitrate we are allowed to generate is not enough.
> >
> > This CL also switches the tests from using ISAC to OPUS as RampUpTest.UpDownUpAudioVideoTransportSequenceNumberRtx relies on audio BWE to work (which is only compatible with OPUS). I don't know why it didn't fail before.
> >
> > BUG=webrtc:7178
> >
> > Review-Url: https://codereview.webrtc.org/2705603002
> > Cr-Commit-Position: refs/heads/master@{#16739}
> > Committed: https://chromium.googlesource.com/external/webrtc/+/a518a39963d34616d8f0e94991c7f5fbb5affb38
>
> TBR=mflodman@webrtc.org,terelius@webrtc.org,stefan@webrtc.org
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG=webrtc:7178
>
> Review-Url: https://codereview.webrtc.org/2703393002
> Cr-Commit-Position: refs/heads/master@{#16751}
> Committed: https://chromium.googlesource.com/external/webrtc/+/b80bdcafed6c529be140da7d9f3e95a00b94219e
TBR=mflodman@webrtc.org,terelius@webrtc.org,stefan@webrtc.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=webrtc:7178
Review-Url: https://codereview.webrtc.org/2704323003
Cr-Commit-Position: refs/heads/master@{#16753}
diff --git a/webrtc/call/rampup_tests.cc b/webrtc/call/rampup_tests.cc
index 46e97cd..94153b0 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 {
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 @@
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 @@
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 @@
poller_thread_(&BitrateStatsPollingThread,
this,
"BitrateStatsPollingThread") {
+ if (red_)
+ EXPECT_EQ(0u, num_flexfec_streams_);
EXPECT_LE(num_audio_streams_, 1u);
}
@@ -100,6 +105,10 @@
return num_audio_streams_;
}
+size_t RampUpTester::GetNumFlexfecStreams() const {
+ return num_flexfec_streams_;
+}
+
class RampUpTester::VideoStreamFactory
: public VideoEncoderConfig::VideoStreamFactoryInterface {
public:
@@ -209,6 +218,13 @@
}
++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 @@
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 @@
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::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 @@
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 @@
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 @@
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};
+ 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);