diff --git a/webrtc/media/engine/webrtcvideoengine2.cc b/webrtc/media/engine/webrtcvideoengine2.cc index 2465aab778..4980153490 100644 --- a/webrtc/media/engine/webrtcvideoengine2.cc +++ b/webrtc/media/engine/webrtcvideoengine2.cc @@ -43,14 +43,18 @@ using DegradationPreference = webrtc::VideoSendStream::DegradationPreference; 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 @@ bool WebRtcVideoChannel2::GetChangedSendParameters( } // Select one of the remote codecs that will be used as send codec. - const rtc::Optional selected_send_codec = + rtc::Optional selected_send_codec = SelectSendVideoCodec(MapCodecs(params.codecs)); if (!selected_send_codec) { @@ -713,6 +717,15 @@ bool WebRtcVideoChannel2::GetChangedSendParameters( 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 @@ void WebRtcVideoChannel2::ConfigureReceiverRtp( 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 @@ WebRtcVideoChannel2::WebRtcVideoSendStream::WebRtcVideoSendStream( 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 @@ void WebRtcVideoChannel2::WebRtcVideoSendStream::SetCodec( 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/webrtc/media/engine/webrtcvideoengine2_unittest.cc b/webrtc/media/engine/webrtcvideoengine2_unittest.cc index 3165be1e2d..46664c4d94 100644 --- a/webrtc/media/engine/webrtcvideoengine2_unittest.cc +++ b/webrtc/media/engine/webrtcvideoengine2_unittest.cc @@ -2430,45 +2430,99 @@ TEST_F(WebRtcVideoChannel2Test, SetDefaultSendCodecs) { // 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& streams = + fake_call_->GetFlexfecReceiveStreams(); + EXPECT_TRUE(streams.empty()); +} + +TEST_F(WebRtcVideoChannel2Test, FlexfecRecvCodecWithSsrcNotExposedByDefault) { + AddRecvStream( + CreatePrimaryWithFecFrStreamParams("cname", kSsrcs1[0], kFlexfecSsrc)); + + const std::vector& 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& streams = + fake_call_->GetFlexfecReceiveStreams(); + EXPECT_TRUE(streams.empty()); +} + +TEST_F(WebRtcVideoChannel2FlexfecRecvTest, SetDefaultRecvCodecsWithSsrc) { + AddRecvStream( + CreatePrimaryWithFecFrStreamParams("cname", kSsrcs1[0], kFlexfecSsrc)); + + const std::vector& 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 @@ TEST_F(WebRtcVideoChannel2FlexfecTest, SetDefaultSendCodecsWithoutSsrc) { 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 @@ TEST_F(WebRtcVideoChannel2Test, SetSendCodecsWithoutFec) { 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 @@ TEST_F(WebRtcVideoChannel2FlexfecTest, SetSendCodecsWithoutFec) { 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 @@ TEST_F(WebRtcVideoChannel2FlexfecTest, SetRecvCodecsWithFec) { 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 @@ TEST_F(WebRtcVideoChannel2Test, SetSendCodecsWithoutFecDisablesFec) { << "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 @@ TEST_F(WebRtcVideoChannel2Test, SetRecvCodecsWithoutFecDisablesFec) { << "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& streams = @@ -3108,9 +3184,8 @@ TEST_F(WebRtcVideoChannel2Test, SetSendParamsWithFecEnablesFec) { << "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& streams = @@ -3145,7 +3220,7 @@ TEST_F(WebRtcVideoChannel2FlexfecTest, SetSendParamsWithFecEnablesFec) { 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 @@ TEST_F(WebRtcVideoChannel2Test, SetSendCodecsRejectDuplicateFecPayloads) { 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 @@ TEST_F(WebRtcVideoChannel2Test, UlpfecPacketDoesntCreateUnsignalledStream) { 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 */);