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 <crodbro@webrtc.org>
> Reviewed-by: Ying Wang <yinwa@webrtc.org>
> Reviewed-by: Christoffer Rodbro <crodbro@webrtc.org>
> Reviewed-by: Stefan Holmer <stefan@webrtc.org>
> 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 <crodbro@webrtc.org>
Reviewed-by: Stefan Holmer <stefan@webrtc.org>
Commit-Queue: Christoffer Rodbro <crodbro@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#32819}
diff --git a/media/engine/webrtc_video_engine.cc b/media/engine/webrtc_video_engine.cc
index a7ca967..855104a 100644
--- a/media/engine/webrtc_video_engine.cc
+++ b/media/engine/webrtc_video_engine.cc
@@ -67,6 +67,11 @@
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 @@
// VP9, H264 and RED). It will also add default feedback params to the codecs.
std::vector<VideoCodec> AssignPayloadTypesAndDefaultCodecs(
std::vector<webrtc::SdpVideoFormat> input_formats,
- const webrtc::WebRtcKeyValueConfig& trials) {
+ const webrtc::WebRtcKeyValueConfig& trials,
+ bool is_decoder_factory) {
if (input_formats.empty())
return std::vector<VideoCodec>();
// Due to interoperability issues with old Chrome/WebRTC versions only use
@@ -123,7 +129,13 @@
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 @@
// 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 <class T>
std::vector<VideoCodec> GetPayloadTypesAndDefaultCodecs(
@@ -211,7 +225,7 @@
}
return AssignPayloadTypesAndDefaultCodecs(std::move(supported_formats),
- trials);
+ trials, is_decoder_factory);
}
bool IsTemporalLayersSupported(const std::string& codec_name) {
@@ -1532,7 +1546,7 @@
// 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 63d134d..9be59eb 100644
--- a/media/engine/webrtc_video_engine_unittest.cc
+++ b/media/engine/webrtc_video_engine_unittest.cc
@@ -951,26 +951,36 @@
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::test::ScopedFieldTrials>(
"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::test::ScopedFieldTrials>(
+ "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 @@
EXPECT_TRUE(streams.empty());
}
-TEST_F(WebRtcVideoChannelTest, FlexfecRecvCodecWithSsrcNotExposedByDefault) {
+TEST_F(WebRtcVideoChannelTest, FlexfecRecvCodecWithSsrcExposedByDefault) {
AddRecvStream(
CreatePrimaryWithFecFrStreamParams("cname", kSsrcs1[0], kFlexfecSsrc));
const std::vector<FakeFlexfecReceiveStream*>& 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
