From ce4be1e64050943f47d5944dc0dead3875615184 Mon Sep 17 00:00:00 2001 From: Mirko Bonadei Date: Fri, 20 Nov 2020 19:21:05 +0000 Subject: [PATCH] Revert "Enable FlexFEC as a receiver video codec by default" This reverts commit f08db1be94e760c201acdc3a121e67453960c970. Reason for revert: It looks like this breaks Chromium FYI Windows bots. See https://ci.chromium.org/p/chromium/builders/webrtc.fyi/WebRTC%20Chromium%20FYI%20Win10%20Tester/6988. If this is not the culprit I will reland. 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} TBR=brandtr@webrtc.org,tommi@webrtc.org,stefan@webrtc.org,crodbro@webrtc.org,crodbro@google.com,yinwa@webrtc.org,philipp.hancke@googlemail.com,hmaniar@nvidia.com # Not skipping CQ checks because original CL landed > 1 day ago. Bug: webrtc:8151 Change-Id: Ia1788a1cf34e0fc9500a081552f6ed03d0995d5b Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/194334 Reviewed-by: Mirko Bonadei Commit-Queue: Mirko Bonadei Cr-Commit-Position: refs/heads/master@{#32657} --- media/engine/webrtc_video_engine.cc | 24 ++++------------ media/engine/webrtc_video_engine_unittest.cc | 30 +++++++------------- pc/peer_connection_end_to_end_unittest.cc | 6 ---- pc/peer_connection_integrationtest.cc | 4 --- 4 files changed, 15 insertions(+), 49 deletions(-) diff --git a/media/engine/webrtc_video_engine.cc b/media/engine/webrtc_video_engine.cc index 2485256248..8a916c4c7e 100644 --- a/media/engine/webrtc_video_engine.cc +++ b/media/engine/webrtc_video_engine.cc @@ -67,11 +67,6 @@ 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); } @@ -112,8 +107,7 @@ void AddDefaultFeedbackParams(VideoCodec* codec, // default feedback params to the codecs. std::vector AssignPayloadTypesAndDefaultCodecs( std::vector input_formats, - const webrtc::WebRtcKeyValueConfig& trials, - bool is_decoder_factory) { + const webrtc::WebRtcKeyValueConfig& trials) { if (input_formats.empty()) return std::vector(); static const int kFirstDynamicPayloadType = 96; @@ -123,13 +117,7 @@ std::vector AssignPayloadTypesAndDefaultCodecs( input_formats.push_back(webrtc::SdpVideoFormat(kRedCodecName)); input_formats.push_back(webrtc::SdpVideoFormat(kUlpfecCodecName)); - // 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"))) { + if (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 @@ -172,9 +160,7 @@ 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. -// 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 +// TODO(kron): Perhaps it better to move the implcit knowledge to the place // where codecs are negotiated. template std::vector GetPayloadTypesAndDefaultCodecs( @@ -192,7 +178,7 @@ std::vector GetPayloadTypesAndDefaultCodecs( } return AssignPayloadTypesAndDefaultCodecs(std::move(supported_formats), - trials, is_decoder_factory); + trials); } bool IsTemporalLayersSupported(const std::string& codec_name) { @@ -1513,7 +1499,7 @@ void WebRtcVideoChannel::ConfigureReceiverRtp( // TODO(brandtr): Generalize when we add support for multistream protection. flexfec_config->payload_type = recv_flexfec_payload_type_; - if (!IsDisabled(call_->trials(), "WebRTC-FlexFEC-03-Advertised") && + if (IsEnabled(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 d5b5435e5c..44984c5ed4 100644 --- a/media/engine/webrtc_video_engine_unittest.cc +++ b/media/engine/webrtc_video_engine_unittest.cc @@ -951,36 +951,26 @@ TEST_F(WebRtcVideoEngineTest, SimulcastEnabledForH264BehindFieldTrial) { EXPECT_TRUE(channel->SetVideoSend(ssrcs[0], nullptr, nullptr)); } -// 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) { +// 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) { 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"); @@ -4027,13 +4017,13 @@ TEST_F(WebRtcVideoChannelTest, FlexfecRecvCodecWithoutSsrcNotExposedByDefault) { EXPECT_TRUE(streams.empty()); } -TEST_F(WebRtcVideoChannelTest, FlexfecRecvCodecWithSsrcExposedByDefault) { +TEST_F(WebRtcVideoChannelTest, FlexfecRecvCodecWithSsrcNotExposedByDefault) { AddRecvStream( CreatePrimaryWithFecFrStreamParams("cname", kSsrcs1[0], kFlexfecSsrc)); const std::vector& streams = fake_call_->GetFlexfecReceiveStreams(); - EXPECT_EQ(1U, streams.size()); + EXPECT_TRUE(streams.empty()); } // 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 aafcd5e26d..24ef69c111 100644 --- a/pc/peer_connection_end_to_end_unittest.cc +++ b/pc/peer_connection_end_to_end_unittest.cc @@ -21,7 +21,6 @@ #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" @@ -429,11 +428,6 @@ 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 364e5d6f1b..53e0f6d7c9 100644 --- a/pc/peer_connection_integrationtest.cc +++ b/pc/peer_connection_integrationtest.cc @@ -2021,10 +2021,6 @@ 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();