diff --git a/media/engine/webrtc_video_engine.cc b/media/engine/webrtc_video_engine.cc index 8a916c4c7e..2485256248 100644 --- a/media/engine/webrtc_video_engine.cc +++ b/media/engine/webrtc_video_engine.cc @@ -67,6 +67,11 @@ bool IsEnabled(const webrtc::WebRtcKeyValueConfig& trials, return absl::StartsWith(trials.Lookup(name), "Enabled"); } +bool IsDisabled(const webrtc::WebRtcKeyValueConfig& trials, + absl::string_view name) { + return absl::StartsWith(trials.Lookup(name), "Disabled"); +} + bool PowerOfTwo(int value) { return (value > 0) && ((value & (value - 1)) == 0); } @@ -107,7 +112,8 @@ void AddDefaultFeedbackParams(VideoCodec* codec, // default feedback params to the codecs. std::vector AssignPayloadTypesAndDefaultCodecs( std::vector input_formats, - const webrtc::WebRtcKeyValueConfig& trials) { + const webrtc::WebRtcKeyValueConfig& trials, + bool is_decoder_factory) { if (input_formats.empty()) return std::vector(); static const int kFirstDynamicPayloadType = 96; @@ -117,7 +123,13 @@ std::vector AssignPayloadTypesAndDefaultCodecs( input_formats.push_back(webrtc::SdpVideoFormat(kRedCodecName)); input_formats.push_back(webrtc::SdpVideoFormat(kUlpfecCodecName)); - if (IsEnabled(trials, "WebRTC-FlexFEC-03-Advertised")) { + // flexfec-03 is supported as + // - receive codec unless WebRTC-FlexFEC-03-Advertised is disabled + // - send codec if WebRTC-FlexFEC-03-Advertised is enabled + if ((is_decoder_factory && + !IsDisabled(trials, "WebRTC-FlexFEC-03-Advertised")) || + (!is_decoder_factory && + IsEnabled(trials, "WebRTC-FlexFEC-03-Advertised"))) { webrtc::SdpVideoFormat flexfec_format(kFlexfecCodecName); // This value is currently arbitrarily set to 10 seconds. (The unit // is microseconds.) This parameter MUST be present in the SDP, but @@ -160,7 +172,9 @@ std::vector AssignPayloadTypesAndDefaultCodecs( // is_decoder_factory is needed to keep track of the implict assumption that any // H264 decoder also supports constrained base line profile. -// TODO(kron): Perhaps it better to move the implcit knowledge to the place +// Also, is_decoder_factory is used to decide whether FlexFEC video format +// should be advertised as supported. +// TODO(kron): Perhaps it is better to move the implicit knowledge to the place // where codecs are negotiated. template std::vector GetPayloadTypesAndDefaultCodecs( @@ -178,7 +192,7 @@ std::vector GetPayloadTypesAndDefaultCodecs( } return AssignPayloadTypesAndDefaultCodecs(std::move(supported_formats), - trials); + trials, is_decoder_factory); } bool IsTemporalLayersSupported(const std::string& codec_name) { @@ -1499,7 +1513,7 @@ void WebRtcVideoChannel::ConfigureReceiverRtp( // TODO(brandtr): Generalize when we add support for multistream protection. flexfec_config->payload_type = recv_flexfec_payload_type_; - if (IsEnabled(call_->trials(), "WebRTC-FlexFEC-03-Advertised") && + if (!IsDisabled(call_->trials(), "WebRTC-FlexFEC-03-Advertised") && sp.GetFecFrSsrc(ssrc, &flexfec_config->remote_ssrc)) { flexfec_config->protected_media_ssrcs = {ssrc}; flexfec_config->local_ssrc = config->rtp.local_ssrc; diff --git a/media/engine/webrtc_video_engine_unittest.cc b/media/engine/webrtc_video_engine_unittest.cc index 44984c5ed4..d5b5435e5c 100644 --- a/media/engine/webrtc_video_engine_unittest.cc +++ b/media/engine/webrtc_video_engine_unittest.cc @@ -951,26 +951,36 @@ TEST_F(WebRtcVideoEngineTest, SimulcastEnabledForH264BehindFieldTrial) { EXPECT_TRUE(channel->SetVideoSend(ssrcs[0], nullptr, nullptr)); } -// Test that the FlexFEC field trial properly alters the output of -// WebRtcVideoEngine::codecs(), for an existing |engine_| object. -// -// TODO(brandtr): Remove this test, when the FlexFEC field trial is gone. -TEST_F(WebRtcVideoEngineTest, - Flexfec03SupportedAsInternalCodecBehindFieldTrial) { +// Test that FlexFEC is not supported as a send video codec by default. +// Only enabling field trial should allow advertising FlexFEC send codec. +TEST_F(WebRtcVideoEngineTest, Flexfec03SendCodecEnablesWithFieldTrial) { encoder_factory_->AddSupportedVideoCodecType("VP8"); auto flexfec = Field("name", &VideoCodec::name, "flexfec-03"); - // FlexFEC is not active without field trial. EXPECT_THAT(engine_.send_codecs(), Not(Contains(flexfec))); - // FlexFEC is active with field trial. RTC_DCHECK(!override_field_trials_); override_field_trials_ = std::make_unique( "WebRTC-FlexFEC-03-Advertised/Enabled/"); EXPECT_THAT(engine_.send_codecs(), Contains(flexfec)); } +// Test that FlexFEC is supported as a receive video codec by default. +// Disabling field trial should prevent advertising FlexFEC receive codec. +TEST_F(WebRtcVideoEngineTest, Flexfec03ReceiveCodecDisablesWithFieldTrial) { + decoder_factory_->AddSupportedVideoCodecType("VP8"); + + auto flexfec = Field("name", &VideoCodec::name, "flexfec-03"); + + EXPECT_THAT(engine_.recv_codecs(), Contains(flexfec)); + + RTC_DCHECK(!override_field_trials_); + override_field_trials_ = std::make_unique( + "WebRTC-FlexFEC-03-Advertised/Disabled/"); + EXPECT_THAT(engine_.recv_codecs(), Not(Contains(flexfec))); +} + // Test that codecs are added in the order they are reported from the factory. TEST_F(WebRtcVideoEngineTest, ReportSupportedCodecs) { encoder_factory_->AddSupportedVideoCodecType("VP8"); @@ -4017,13 +4027,13 @@ TEST_F(WebRtcVideoChannelTest, FlexfecRecvCodecWithoutSsrcNotExposedByDefault) { EXPECT_TRUE(streams.empty()); } -TEST_F(WebRtcVideoChannelTest, FlexfecRecvCodecWithSsrcNotExposedByDefault) { +TEST_F(WebRtcVideoChannelTest, FlexfecRecvCodecWithSsrcExposedByDefault) { AddRecvStream( CreatePrimaryWithFecFrStreamParams("cname", kSsrcs1[0], kFlexfecSsrc)); const std::vector& streams = fake_call_->GetFlexfecReceiveStreams(); - EXPECT_TRUE(streams.empty()); + EXPECT_EQ(1U, streams.size()); } // TODO(brandtr): When FlexFEC is no longer behind a field trial, merge all diff --git a/pc/peer_connection_end_to_end_unittest.cc b/pc/peer_connection_end_to_end_unittest.cc index 24ef69c111..aafcd5e26d 100644 --- a/pc/peer_connection_end_to_end_unittest.cc +++ b/pc/peer_connection_end_to_end_unittest.cc @@ -21,6 +21,7 @@ #include "media/sctp/sctp_transport_internal.h" #include "rtc_base/gunit.h" #include "rtc_base/logging.h" +#include "test/field_trial.h" #ifdef WEBRTC_ANDROID #include "pc/test/android_test_initializer.h" @@ -428,6 +429,11 @@ TEST_P(PeerConnectionEndToEndTest, CallWithCustomCodec) { std::vector* const codec_ids_; }; + // Disable advertising FlexFEC as receive codec to avoid running out of unique + // payload types. See bugs.webrtc.org/12194 + webrtc::test::ScopedFieldTrials field_trials( + "WebRTC-FlexFEC-03-Advertised/Disabled/"); + std::vector encoder_id1, encoder_id2, decoder_id1, decoder_id2; CreatePcs(rtc::scoped_refptr( diff --git a/pc/peer_connection_integrationtest.cc b/pc/peer_connection_integrationtest.cc index 53e0f6d7c9..364e5d6f1b 100644 --- a/pc/peer_connection_integrationtest.cc +++ b/pc/peer_connection_integrationtest.cc @@ -2021,6 +2021,10 @@ TEST_P(PeerConnectionIntegrationTest, EndToEndCallWithSendOnlyVideo) { // Tests that receive only works without the caller having an encoder factory // and the callee having a decoder factory. TEST_P(PeerConnectionIntegrationTest, EndToEndCallWithReceiveOnlyVideo) { + // Disable advertising FlexFEC as receive codec to avoid running out of unique + // payload types. See bugs.webrtc.org/12194 + webrtc::test::ScopedFieldTrials field_trials( + "WebRTC-FlexFEC-03-Advertised/Disabled/"); ASSERT_TRUE( CreateOneDirectionalPeerConnectionWrappers(/*caller_to_callee=*/false)); ConnectFakeSignaling();