From adb946054c7daba852625f3af90698ec0525b1bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Bostr=C3=B6m?= Date: Thu, 23 Mar 2023 17:03:43 +0100 Subject: [PATCH] Ship ability to opt-in to VP9/AV1 simulcast (re-land). MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This makes "WebRTC-AllowDisablingLegacyScalability" enabled-by-default, meaning any app can opt-in to spec-compliant simulcast when scalabilityMode is specified. The opt-in criteria is also made more restricitve: you now have to specify both scalabilityMode and scaleResolutionDownBy to get simulcast, otherwise you continue to get legacy "single stream" path. The reason for this is not to cause any surprises in use cases like [{scalabilityMode:"L1T1", active:true}, {active:false}, {active:false}] In cases like this where scaleResolutionDownBy is not specified, it defaults to 4:2:1 if simulcast is used but the legacy path caps it to one stream, meaning full resolution. By restricing simulcast only to cases that set scaleResolutionDownBy, we remove the risk of an app getting a different resolution than expected due to opt-in. Bug: webrtc:14884, webrtc:15005 Change-Id: I5efb87af60afaeb1e3ff76698d887aaa1f9d63a9 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/298922 Reviewed-by: Erik Språng Commit-Queue: Henrik Boström Reviewed-by: Ilya Nikolaevskiy Reviewed-by: Evan Shrubsole Cr-Commit-Position: refs/heads/main@{#39660} --- media/engine/webrtc_video_engine.cc | 9 +++-- pc/peer_connection_simulcast_unittest.cc | 51 ++++++++++++------------ 2 files changed, 31 insertions(+), 29 deletions(-) diff --git a/media/engine/webrtc_video_engine.cc b/media/engine/webrtc_video_engine.cc index 7c7cc90ed8..a8b58c07e3 100644 --- a/media/engine/webrtc_video_engine.cc +++ b/media/engine/webrtc_video_engine.cc @@ -2507,12 +2507,13 @@ WebRtcVideoChannel::WebRtcVideoSendStream::CreateVideoEncoderConfig( // `legacy_scalability_mode` and codec used. encoder_config.number_of_streams = parameters_.config.rtp.ssrcs.size(); bool legacy_scalability_mode = true; - // TODO(https://crbug.com/webrtc/14884): When simulcast VP9 is ready to ship, - // don't require a field trial to set `legacy_scalability_mode` to false here. - if (call_->trials().IsEnabled("WebRTC-AllowDisablingLegacyScalability")) { + // TODO(https://crbug.com/webrtc/14884): This is only used as a kill-switch + // in case of serious bugs - when this reaches Stable, delete the field trial. + if (!call_->trials().IsDisabled("WebRTC-AllowDisablingLegacyScalability")) { for (const webrtc::RtpEncodingParameters& encoding : rtp_parameters_.encodings) { - if (encoding.scalability_mode.has_value()) { + if (encoding.scalability_mode.has_value() && + encoding.scale_resolution_down_by.has_value()) { legacy_scalability_mode = false; break; } diff --git a/pc/peer_connection_simulcast_unittest.cc b/pc/peer_connection_simulcast_unittest.cc index b8f1a7cdfb..27896a8889 100644 --- a/pc/peer_connection_simulcast_unittest.cc +++ b/pc/peer_connection_simulcast_unittest.cc @@ -1000,6 +1000,20 @@ class PeerConnectionSimulcastWithMediaFlowTests return num_sending_layers == num_active_layers; } + bool HasOutboundRtpWithRidAndScalabilityMode( + rtc::scoped_refptr pc_wrapper, + absl::string_view rid, + absl::string_view expected_scalability_mode) { + rtc::scoped_refptr report = GetStats(pc_wrapper); + std::vector outbound_rtps = + report->GetStatsOfType(); + auto* outbound_rtp = FindOutboundRtpByRid(outbound_rtps, rid); + if (!outbound_rtp || !outbound_rtp->scalability_mode.is_defined()) { + return false; + } + return *outbound_rtp->scalability_mode == expected_scalability_mode; + } + bool OutboundRtpResolutionsAreLessThanOrEqualToExpectations( rtc::scoped_refptr pc_wrapper, std::vector resolutions) { @@ -1412,13 +1426,8 @@ TEST_F(PeerConnectionSimulcastWithMediaFlowTests, Optional(std::string("L3T3_KEY"))); } -// TODO(https://crbug.com/webrtc/14884): A field trial shouldn't be needed to -// get spec-compliant behavior! TEST_F(PeerConnectionSimulcastWithMediaFlowTests, SendingThreeEncodings_VP9_Simulcast) { - test::ScopedFieldTrials field_trials( - "WebRTC-AllowDisablingLegacyScalability/Enabled/"); - rtc::scoped_refptr local_pc_wrapper = CreatePc(); rtc::scoped_refptr remote_pc_wrapper = CreatePc(); ExchangeIceCandidates(local_pc_wrapper, remote_pc_wrapper); @@ -1433,13 +1442,16 @@ TEST_F(PeerConnectionSimulcastWithMediaFlowTests, transceiver->SetCodecPreferences(codecs); // Opt-in to spec-compliant simulcast by explicitly setting the - // `scalability_mode`. + // `scalability_mode` and `scale_resolution_down_by` parameters. rtc::scoped_refptr sender = transceiver->sender(); RtpParameters parameters = sender->GetParameters(); ASSERT_EQ(parameters.encodings.size(), 3u); parameters.encodings[0].scalability_mode = "L1T3"; + parameters.encodings[0].scale_resolution_down_by = 4; parameters.encodings[1].scalability_mode = "L1T3"; + parameters.encodings[1].scale_resolution_down_by = 2; parameters.encodings[2].scalability_mode = "L1T3"; + parameters.encodings[2].scale_resolution_down_by = 1; sender->SetParameters(parameters); NegotiateWithSimulcastTweaks(local_pc_wrapper, remote_pc_wrapper, layers); @@ -1483,11 +1495,6 @@ TEST_F(PeerConnectionSimulcastWithMediaFlowTests, // changes from 1 (legacy SVC) to 3 (standard simulcast). TEST_F(PeerConnectionSimulcastWithMediaFlowTests, SendingThreeEncodings_VP9_FromLegacyToSingleActiveWithScalability) { - // TODO(https://crbug.com/webrtc/14884): A field trial shouldn't be needed to - // get spec-compliant behavior! - test::ScopedFieldTrials field_trials( - "WebRTC-AllowDisablingLegacyScalability/Enabled/"); - rtc::scoped_refptr local_pc_wrapper = CreatePc(); rtc::scoped_refptr remote_pc_wrapper = CreatePc(); ExchangeIceCandidates(local_pc_wrapper, remote_pc_wrapper); @@ -1515,6 +1522,7 @@ TEST_F(PeerConnectionSimulcastWithMediaFlowTests, ASSERT_EQ(parameters.encodings.size(), 3u); parameters.encodings[0].active = true; parameters.encodings[0].scalability_mode = "L2T2_KEY"; + parameters.encodings[0].scale_resolution_down_by = 4.0; parameters.encodings[1].active = false; parameters.encodings[1].scalability_mode = absl::nullopt; parameters.encodings[2].active = false; @@ -1525,14 +1533,10 @@ TEST_F(PeerConnectionSimulcastWithMediaFlowTests, // but only one is active. EXPECT_TRUE_WAIT(HasOutboundRtpBytesSent(local_pc_wrapper, 3u, 1u), kLongTimeoutForRampingUp.ms()); - rtc::scoped_refptr report = GetStats(local_pc_wrapper); - std::vector outbound_rtps = - report->GetStatsOfType(); - ASSERT_THAT(outbound_rtps, SizeIs(3u)); - auto* f_outbound_rtp = FindOutboundRtpByRid(outbound_rtps, "f"); - ASSERT_TRUE(f_outbound_rtp); - ASSERT_TRUE(f_outbound_rtp->scalability_mode.is_defined()); - EXPECT_THAT(*f_outbound_rtp->scalability_mode, StrEq("L2T2_KEY")); + // Reduce risk of flakiness by waiting until the scalability mode is reported. + EXPECT_TRUE_WAIT(HasOutboundRtpWithRidAndScalabilityMode(local_pc_wrapper, + "f", "L2T2_KEY"), + kLongTimeoutForRampingUp.ms()); // GetParameters() does not report any fallback. parameters = sender->GetParameters(); @@ -1543,14 +1547,8 @@ TEST_F(PeerConnectionSimulcastWithMediaFlowTests, EXPECT_FALSE(parameters.encodings[2].scalability_mode.has_value()); } -// TODO(https://crbug.com/webrtc/15005): A field trial shouldn't be needed to -// get spec-compliant behavior! The same field trial is also used for VP9 -// simulcast (https://crbug.com/webrtc/14884). TEST_F(PeerConnectionSimulcastWithMediaFlowTests, SendingThreeEncodings_AV1_Simulcast) { - test::ScopedFieldTrials field_trials( - "WebRTC-AllowDisablingLegacyScalability/Enabled/"); - rtc::scoped_refptr local_pc_wrapper = CreatePc(); // TODO(https://crbug.com/webrtc/15011): Expand testing support for AV1 or // allow compile time checks so that gates like this isn't needed at runtime. @@ -1576,8 +1574,11 @@ TEST_F(PeerConnectionSimulcastWithMediaFlowTests, RtpParameters parameters = sender->GetParameters(); ASSERT_EQ(parameters.encodings.size(), 3u); parameters.encodings[0].scalability_mode = "L1T3"; + parameters.encodings[0].scale_resolution_down_by = 4; parameters.encodings[1].scalability_mode = "L1T3"; + parameters.encodings[1].scale_resolution_down_by = 2; parameters.encodings[2].scalability_mode = "L1T3"; + parameters.encodings[2].scale_resolution_down_by = 1; sender->SetParameters(parameters); NegotiateWithSimulcastTweaks(local_pc_wrapper, remote_pc_wrapper, layers);