Reduce VideoSendStream recreations due to FlexFEC.
This CL reduces the number of VideoSendStream recreations during SDP
renegotiation by checking the FlexFEC field trials before, and not after,
the SDP codec diffing logic.
BUG=webrtc:5654
Review-Url: https://codereview.webrtc.org/2882433003
Cr-Original-Commit-Position: refs/heads/master@{#18211}
Cr-Mirrored-From: https://chromium.googlesource.com/external/webrtc
Cr-Mirrored-Commit: 31bd224f356b6dff18664370abd475351de4c683
diff --git a/media/engine/webrtcvideoengine2.cc b/media/engine/webrtcvideoengine2.cc
index 2465aab..4980153 100644
--- a/media/engine/webrtcvideoengine2.cc
+++ b/media/engine/webrtcvideoengine2.cc
@@ -43,14 +43,18 @@
namespace cricket {
namespace {
// If this field trial is enabled, we will enable sending FlexFEC and disable
-// sending ULPFEC whenever the former has been negotiated.
-// FlexFEC can only be negotiated when the "flexfec-03" SDP codec is enabled,
-// which is done by enabling the "WebRTC-FlexFEC-03-Advertised" field trial; see
-// internalencoderfactory.cc.
+// sending ULPFEC whenever the former has been negotiated in the SDPs.
bool IsFlexfecFieldTrialEnabled() {
return webrtc::field_trial::IsEnabled("WebRTC-FlexFEC-03");
}
+// If this field trial is enabled, the "flexfec-03" codec may have been
+// advertised as being supported in the local SDP. That means that we must be
+// ready to receive FlexFEC packets. See internalencoderfactory.cc.
+bool IsFlexfecAdvertisedFieldTrialEnabled() {
+ return webrtc::field_trial::IsEnabled("WebRTC-FlexFEC-03-Advertised");
+}
+
// If this field trial is enabled, we will report VideoContentType RTP extension
// in capabilities (thus, it will end up in the default SDP and extension will
// be sent for all key-frames).
@@ -705,7 +709,7 @@
}
// Select one of the remote codecs that will be used as send codec.
- const rtc::Optional<VideoCodecSettings> selected_send_codec =
+ rtc::Optional<VideoCodecSettings> selected_send_codec =
SelectSendVideoCodec(MapCodecs(params.codecs));
if (!selected_send_codec) {
@@ -713,6 +717,15 @@
return false;
}
+ // Never enable sending FlexFEC, unless we are in the experiment.
+ if (!IsFlexfecFieldTrialEnabled()) {
+ if (selected_send_codec->flexfec_payload_type != -1) {
+ LOG(LS_INFO) << "Remote supports flexfec-03, but we will not send since "
+ << "WebRTC-FlexFEC-03 field trial is not enabled.";
+ }
+ selected_send_codec->flexfec_payload_type = -1;
+ }
+
if (!send_codec_ || *selected_send_codec != *send_codec_)
changed_params->codec = selected_send_codec;
@@ -1270,7 +1283,8 @@
config->rtp.extensions = recv_rtp_extensions_;
// TODO(brandtr): Generalize when we add support for multistream protection.
- if (sp.GetFecFrSsrc(ssrc, &flexfec_config->remote_ssrc)) {
+ if (IsFlexfecAdvertisedFieldTrialEnabled() &&
+ sp.GetFecFrSsrc(ssrc, &flexfec_config->remote_ssrc)) {
flexfec_config->protected_media_ssrcs = {ssrc};
flexfec_config->local_ssrc = config->rtp.local_ssrc;
flexfec_config->rtcp_mode = config->rtp.rtcp_mode;
@@ -1606,7 +1620,7 @@
for (uint32_t primary_ssrc : parameters_.config.rtp.ssrcs) {
if (sp.GetFecFrSsrc(primary_ssrc, &flexfec_ssrc)) {
if (flexfec_enabled) {
- LOG(LS_INFO) << "Multiple FlexFEC streams proposed by remote, but "
+ LOG(LS_INFO) << "Multiple FlexFEC streams in local SDP, but "
"our implementation only supports a single FlexFEC "
"stream. Will not enable FlexFEC for proposed "
"stream with SSRC: "
@@ -1781,10 +1795,8 @@
parameters_.config.encoder_settings.internal_source = false;
}
parameters_.config.rtp.ulpfec = codec_settings.ulpfec;
- if (IsFlexfecFieldTrialEnabled()) {
- parameters_.config.rtp.flexfec.payload_type =
- codec_settings.flexfec_payload_type;
- }
+ parameters_.config.rtp.flexfec.payload_type =
+ codec_settings.flexfec_payload_type;
// Set RTX payload type if RTX is enabled.
if (!parameters_.config.rtp.rtx.ssrcs.empty()) {
diff --git a/media/engine/webrtcvideoengine2_unittest.cc b/media/engine/webrtcvideoengine2_unittest.cc
index 3165be1..46664c4 100644
--- a/media/engine/webrtcvideoengine2_unittest.cc
+++ b/media/engine/webrtcvideoengine2_unittest.cc
@@ -2430,45 +2430,99 @@
// TODO(juberti): Check RTCP, PLI, TMMBR.
}
-// TODO(brandtr): Remove when FlexFEC _is_ exposed by default.
-TEST_F(WebRtcVideoChannel2Test, FlexfecCodecWithoutSsrcNotExposedByDefault) {
+// The following four tests ensures that FlexFEC is not activated by default
+// when the field trials are not enabled.
+// TODO(brandtr): Remove or update these tests when FlexFEC _is_ enabled by
+// default.
+TEST_F(WebRtcVideoChannel2Test,
+ FlexfecSendCodecWithoutSsrcNotExposedByDefault) {
FakeVideoSendStream* stream = AddSendStream();
webrtc::VideoSendStream::Config config = stream->GetConfig().Copy();
EXPECT_EQ(-1, config.rtp.flexfec.payload_type);
+ EXPECT_EQ(0U, config.rtp.flexfec.ssrc);
+ EXPECT_TRUE(config.rtp.flexfec.protected_media_ssrcs.empty());
}
-// TODO(brandtr): Remove when FlexFEC _is_ exposed by default.
-TEST_F(WebRtcVideoChannel2Test, FlexfecCodecWithSsrcNotExposedByDefault) {
+TEST_F(WebRtcVideoChannel2Test, FlexfecSendCodecWithSsrcNotExposedByDefault) {
FakeVideoSendStream* stream = AddSendStream(
CreatePrimaryWithFecFrStreamParams("cname", kSsrcs1[0], kFlexfecSsrc));
webrtc::VideoSendStream::Config config = stream->GetConfig().Copy();
EXPECT_EQ(-1, config.rtp.flexfec.payload_type);
+ EXPECT_EQ(0U, config.rtp.flexfec.ssrc);
+ EXPECT_TRUE(config.rtp.flexfec.protected_media_ssrcs.empty());
+}
+
+TEST_F(WebRtcVideoChannel2Test,
+ FlexfecRecvCodecWithoutSsrcNotExposedByDefault) {
+ AddRecvStream();
+
+ const std::vector<FakeFlexfecReceiveStream*>& streams =
+ fake_call_->GetFlexfecReceiveStreams();
+ EXPECT_TRUE(streams.empty());
+}
+
+TEST_F(WebRtcVideoChannel2Test, FlexfecRecvCodecWithSsrcNotExposedByDefault) {
+ AddRecvStream(
+ CreatePrimaryWithFecFrStreamParams("cname", kSsrcs1[0], kFlexfecSsrc));
+
+ const std::vector<FakeFlexfecReceiveStream*>& streams =
+ fake_call_->GetFlexfecReceiveStreams();
+ EXPECT_TRUE(streams.empty());
}
// TODO(brandtr): When FlexFEC is no longer behind a field trial, merge all
// tests that use this test fixture into the corresponding "non-field trial"
// tests.
-class WebRtcVideoChannel2FlexfecTest : public WebRtcVideoChannel2Test {
+class WebRtcVideoChannel2FlexfecRecvTest : public WebRtcVideoChannel2Test {
public:
- WebRtcVideoChannel2FlexfecTest()
- : WebRtcVideoChannel2Test(
- "WebRTC-FlexFEC-03-Advertised/Enabled/WebRTC-FlexFEC-03/Enabled/") {
- }
+ WebRtcVideoChannel2FlexfecRecvTest()
+ : WebRtcVideoChannel2Test("WebRTC-FlexFEC-03-Advertised/Enabled/") {}
};
-// TODO(brandtr): Merge into "non-field trial" test when FlexFEC is enabled
-// by default.
-TEST_F(WebRtcVideoChannel2FlexfecTest,
+TEST_F(WebRtcVideoChannel2FlexfecRecvTest,
DefaultFlexfecCodecHasTransportCcAndRembFeedbackParam) {
EXPECT_TRUE(cricket::HasTransportCc(GetEngineCodec("flexfec-03")));
EXPECT_TRUE(cricket::HasRemb(GetEngineCodec("flexfec-03")));
}
-// TODO(brandtr): Merge into "non-field trial" test when FlexFEC is enabled
-// by default.
-TEST_F(WebRtcVideoChannel2FlexfecTest, SetDefaultSendCodecsWithoutSsrc) {
+TEST_F(WebRtcVideoChannel2FlexfecRecvTest, SetDefaultRecvCodecsWithoutSsrc) {
+ AddRecvStream();
+
+ const std::vector<FakeFlexfecReceiveStream*>& streams =
+ fake_call_->GetFlexfecReceiveStreams();
+ EXPECT_TRUE(streams.empty());
+}
+
+TEST_F(WebRtcVideoChannel2FlexfecRecvTest, SetDefaultRecvCodecsWithSsrc) {
+ AddRecvStream(
+ CreatePrimaryWithFecFrStreamParams("cname", kSsrcs1[0], kFlexfecSsrc));
+
+ const std::vector<FakeFlexfecReceiveStream*>& streams =
+ fake_call_->GetFlexfecReceiveStreams();
+ ASSERT_EQ(1U, streams.size());
+ const FakeFlexfecReceiveStream* stream = streams.front();
+ const webrtc::FlexfecReceiveStream::Config& config = stream->GetConfig();
+ EXPECT_EQ(GetEngineCodec("flexfec-03").id, config.payload_type);
+ EXPECT_EQ(kFlexfecSsrc, config.remote_ssrc);
+ ASSERT_EQ(1U, config.protected_media_ssrcs.size());
+ EXPECT_EQ(kSsrcs1[0], config.protected_media_ssrcs[0]);
+}
+
+// TODO(brandtr): When FlexFEC is no longer behind a field trial, merge all
+// tests that use this test fixture into the corresponding "non-field trial"
+// tests.
+class WebRtcVideoChannel2FlexfecSendRecvTest : public WebRtcVideoChannel2Test {
+ public:
+ WebRtcVideoChannel2FlexfecSendRecvTest()
+ : WebRtcVideoChannel2Test(
+ "WebRTC-FlexFEC-03-Advertised/Enabled/WebRTC-FlexFEC-03/Enabled/") {
+ }
+};
+
+TEST_F(WebRtcVideoChannel2FlexfecSendRecvTest,
+ SetDefaultSendCodecsWithoutSsrc) {
FakeVideoSendStream* stream = AddSendStream();
webrtc::VideoSendStream::Config config = stream->GetConfig().Copy();
@@ -2477,9 +2531,7 @@
EXPECT_TRUE(config.rtp.flexfec.protected_media_ssrcs.empty());
}
-// TODO(brandtr): Merge into "non-field trial" test when FlexFEC is enabled
-// by default.
-TEST_F(WebRtcVideoChannel2FlexfecTest, SetDefaultSendCodecsWithSsrc) {
+TEST_F(WebRtcVideoChannel2FlexfecSendRecvTest, SetDefaultSendCodecsWithSsrc) {
FakeVideoSendStream* stream = AddSendStream(
CreatePrimaryWithFecFrStreamParams("cname", kSsrcs1[0], kFlexfecSsrc));
webrtc::VideoSendStream::Config config = stream->GetConfig().Copy();
@@ -2502,9 +2554,7 @@
EXPECT_EQ(-1, config.rtp.ulpfec.red_payload_type);
}
-// TODO(brandtr): Merge into "non-field trial" test when FlexFEC is enabled
-// by default.
-TEST_F(WebRtcVideoChannel2FlexfecTest, SetSendCodecsWithoutFec) {
+TEST_F(WebRtcVideoChannel2FlexfecSendRecvTest, SetSendCodecsWithoutFec) {
cricket::VideoSendParameters parameters;
parameters.codecs.push_back(GetEngineCodec("VP8"));
ASSERT_TRUE(channel_->SetSendParameters(parameters));
@@ -2515,7 +2565,7 @@
EXPECT_EQ(-1, config.rtp.flexfec.payload_type);
}
-TEST_F(WebRtcVideoChannel2FlexfecTest, SetRecvCodecsWithFec) {
+TEST_F(WebRtcVideoChannel2FlexfecRecvTest, SetRecvCodecsWithFec) {
AddRecvStream(
CreatePrimaryWithFecFrStreamParams("cname", kSsrcs1[0], kFlexfecSsrc));
@@ -2554,6 +2604,40 @@
flexfec_stream_config.rtp_header_extensions);
}
+// We should not send FlexFEC, even if we advertise it, unless the right
+// field trial is set.
+// TODO(brandtr): Remove when FlexFEC is enabled by default.
+TEST_F(WebRtcVideoChannel2FlexfecRecvTest,
+ SetSendCodecsWithoutSsrcWithFecDoesNotEnableFec) {
+ cricket::VideoSendParameters parameters;
+ parameters.codecs.push_back(GetEngineCodec("VP8"));
+ parameters.codecs.push_back(GetEngineCodec("flexfec-03"));
+ ASSERT_TRUE(channel_->SetSendParameters(parameters));
+
+ FakeVideoSendStream* stream = AddSendStream();
+ webrtc::VideoSendStream::Config config = stream->GetConfig().Copy();
+
+ EXPECT_EQ(-1, config.rtp.flexfec.payload_type);
+ EXPECT_EQ(0, config.rtp.flexfec.ssrc);
+ EXPECT_TRUE(config.rtp.flexfec.protected_media_ssrcs.empty());
+}
+
+TEST_F(WebRtcVideoChannel2FlexfecRecvTest,
+ SetSendCodecsWithSsrcWithFecDoesNotEnableFec) {
+ cricket::VideoSendParameters parameters;
+ parameters.codecs.push_back(GetEngineCodec("VP8"));
+ parameters.codecs.push_back(GetEngineCodec("flexfec-03"));
+ ASSERT_TRUE(channel_->SetSendParameters(parameters));
+
+ FakeVideoSendStream* stream = AddSendStream(
+ CreatePrimaryWithFecFrStreamParams("cname", kSsrcs1[0], kFlexfecSsrc));
+ webrtc::VideoSendStream::Config config = stream->GetConfig().Copy();
+
+ EXPECT_EQ(-1, config.rtp.flexfec.payload_type);
+ EXPECT_EQ(0, config.rtp.flexfec.ssrc);
+ EXPECT_TRUE(config.rtp.flexfec.protected_media_ssrcs.empty());
+}
+
TEST_F(WebRtcVideoChannel2Test,
SetSendCodecRejectsRtxWithoutAssociatedPayloadType) {
const int kUnusedPayloadType = 127;
@@ -2648,9 +2732,8 @@
<< "SetSendCodec without ULPFEC should disable current ULPFEC.";
}
-// TODO(brandtr): Merge into "non-field trial" test when FlexFEC is enabled
-// by default.
-TEST_F(WebRtcVideoChannel2FlexfecTest, SetSendCodecsWithoutFecDisablesFec) {
+TEST_F(WebRtcVideoChannel2FlexfecSendRecvTest,
+ SetSendCodecsWithoutFecDisablesFec) {
cricket::VideoSendParameters parameters;
parameters.codecs.push_back(GetEngineCodec("VP8"));
parameters.codecs.push_back(GetEngineCodec("flexfec-03"));
@@ -3054,14 +3137,7 @@
<< "SetSendCodec without ULPFEC should disable current ULPFEC.";
}
-// TODO(brandtr): Merge into "non-field trial" test when FlexFEC is enabled
-// by default.
-TEST_F(WebRtcVideoChannel2FlexfecTest, SetRecvParamsWithoutFecDisablesFec) {
- cricket::VideoSendParameters send_parameters;
- send_parameters.codecs.push_back(GetEngineCodec("VP8"));
- send_parameters.codecs.push_back(GetEngineCodec("flexfec-03"));
- ASSERT_TRUE(channel_->SetSendParameters(send_parameters));
-
+TEST_F(WebRtcVideoChannel2FlexfecRecvTest, SetRecvParamsWithoutFecDisablesFec) {
AddRecvStream(
CreatePrimaryWithFecFrStreamParams("cname", kSsrcs1[0], kFlexfecSsrc));
const std::vector<FakeFlexfecReceiveStream*>& streams =
@@ -3108,9 +3184,8 @@
<< "ULPFEC should be enabled on the receive stream.";
}
-// TODO(brandtr): Merge into "non-field trial" test when FlexFEC is enabled
-// by default.
-TEST_F(WebRtcVideoChannel2FlexfecTest, SetSendParamsWithFecEnablesFec) {
+TEST_F(WebRtcVideoChannel2FlexfecSendRecvTest,
+ SetSendRecvParamsWithFecEnablesFec) {
AddRecvStream(
CreatePrimaryWithFecFrStreamParams("cname", kSsrcs1[0], kFlexfecSsrc));
const std::vector<FakeFlexfecReceiveStream*>& streams =
@@ -3145,7 +3220,7 @@
stream_with_send_params->GetConfig().protected_media_ssrcs[0]);
}
-TEST_F(WebRtcVideoChannel2Test, SetSendCodecsRejectDuplicateFecPayloads) {
+TEST_F(WebRtcVideoChannel2Test, SetRecvCodecsRejectDuplicateFecPayloads) {
cricket::VideoRecvParameters parameters;
parameters.codecs.push_back(GetEngineCodec("VP8"));
parameters.codecs.push_back(GetEngineCodec("red"));
@@ -3153,10 +3228,8 @@
EXPECT_FALSE(channel_->SetRecvParameters(parameters));
}
-// TODO(brandtr): Merge into "non-field trial" test when FlexFEC is enabled
-// by default.
-TEST_F(WebRtcVideoChannel2FlexfecTest,
- SetSendCodecsRejectDuplicateFecPayloads) {
+TEST_F(WebRtcVideoChannel2FlexfecRecvTest,
+ SetRecvCodecsRejectDuplicateFecPayloads) {
cricket::VideoRecvParameters parameters;
parameters.codecs.push_back(GetEngineCodec("VP8"));
parameters.codecs.push_back(GetEngineCodec("flexfec-03"));
@@ -3809,9 +3882,7 @@
false /* expect_created_receive_stream */);
}
-// TODO(brandtr): Change to "non-field trial" test when FlexFEC is enabled
-// by default.
-TEST_F(WebRtcVideoChannel2FlexfecTest,
+TEST_F(WebRtcVideoChannel2FlexfecRecvTest,
FlexfecPacketDoesntCreateUnsignalledStream) {
TestReceiveUnsignaledSsrcPacket(GetEngineCodec("flexfec-03").id,
false /* expect_created_receive_stream */);
