From 064be38380560b92e45bbe21fb0cf07d3bbdbb3f Mon Sep 17 00:00:00 2001 From: Harsh Maniar Date: Tue, 8 Dec 2020 12:02:56 -0800 Subject: [PATCH] Reland "Enable FlexFEC as a receiver video codec by default" This is a reland of f08db1be94e760c201acdc3a121e67453960c970 Original change's description: > Enable FlexFEC as a receiver video codec by default > > - Add Flex FEC format as default supported receive codec > - Disallow advertising FlexFEC as video sender codec by default until implementation is complete > - Toggle field trial "WebRTC-FlexFEC-03-Advertised"s behavior for receiver to use as kill-switch to prevent codec advertising > > Bug: webrtc:8151 > Change-Id: Iff367119263496fb335500e96641669654b45834 > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/191947 > Commit-Queue: Christoffer Rodbro > Reviewed-by: Ying Wang > Reviewed-by: Christoffer Rodbro > Reviewed-by: Stefan Holmer > Cr-Commit-Position: refs/heads/master@{#32639} Bug: webrtc:8151 Change-Id: I36cbe833dc2131d72f1d7e8f96d058d0caa94ff9 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/195363 Reviewed-by: Christoffer Rodbro Reviewed-by: Stefan Holmer Commit-Queue: Christoffer Rodbro Cr-Commit-Position: refs/heads/master@{#32819} --- media/engine/webrtc_video_engine.cc | 24 ++++++++++++---- media/engine/webrtc_video_engine_unittest.cc | 30 +++++++++++++------- 2 files changed, 39 insertions(+), 15 deletions(-) diff --git a/media/engine/webrtc_video_engine.cc b/media/engine/webrtc_video_engine.cc index a7ca96719d..855104a8ca 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, // VP9, H264 and RED). It will also add 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(); // Due to interoperability issues with old Chrome/WebRTC versions only use @@ -123,7 +129,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 @@ -193,7 +205,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( @@ -211,7 +225,7 @@ std::vector GetPayloadTypesAndDefaultCodecs( } return AssignPayloadTypesAndDefaultCodecs(std::move(supported_formats), - trials); + trials, is_decoder_factory); } bool IsTemporalLayersSupported(const std::string& codec_name) { @@ -1532,7 +1546,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 63d134d9b7..9be59ebd1a 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 the FlexFEC "codec" gets assigned to the lower payload type range TEST_F(WebRtcVideoEngineTest, Flexfec03LowerPayloadTypeRange) { encoder_factory_->AddSupportedVideoCodecType("VP8"); @@ -4037,13 +4047,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