From 75ea06f0faee397777badaf3568cd2824a8bd983 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Bostr=C3=B6m?= Date: Mon, 20 Mar 2023 12:36:29 +0000 Subject: [PATCH] Revert "Ship ability to opt-in to VP9/AV1 simulcast." MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit 75990b9a8f98ea2d597a31472fb778ec4d55f698. Reason for revert: Breaks downstream, a use case of having three VP9 encodings, scalability mode only specified on the first layer (L2T2_KEY) and the other two layers not having a scalability mode but also being active=false appears to trigger a DCHECK in call/rtp_video_sender.cc:501. More investigation needed Original change's description: > Ship ability to opt-in to VP9/AV1 simulcast. > > With this unflagging, an app can opt-in to simulcast when using multiple > encodings by specifying RTCRtpEncodingParameters.scalabilityMode. This > ensures backwards-compat with apps relying on 3 encodings to mean SVC > who traditionally have not specified scalabilityMode. > > It fixes the spec/API bug of asking for simulcast and not getting > simulcast. The field trial exists only as a kill-switch with a TODO to > remove it. > > This ships initial support, however note that the VP9/AV1 simulcast uses > SimulcastRateAllocator (just like VP8/H264 simulcast). This rate > allocator uses more kbps than SvcRateAllocator. This should be revisited > to avoid significant higher bitrates, for example when comparing VP9 > simulcast to VP9 SVC. > > Shipping the ability for apps to opt-in makes it easier to exercise > these new code paths and allows initial feedback from developers, but > due to the high bitrate (= same bitrate as VP8/H264 simulcast today) > many apps may find that VP9 SVC is still more beneficial for BW reasons. > > Bug: webrtc:14884, webrtc:15005 > Change-Id: I748aae1adb47acc8a6b79b5852cff6aa47a46f5d > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/298046 > Commit-Queue: Henrik Boström > Reviewed-by: Erik Språng > Reviewed-by: Evan Shrubsole > Reviewed-by: Ilya Nikolaevskiy > Cr-Commit-Position: refs/heads/main@{#39601} Bug: webrtc:14884, webrtc:15005 Change-Id: Ic8f77e6a2971f493d6cd8c23faecd435058a8847 No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/298440 Bot-Commit: rubber-stamper@appspot.gserviceaccount.com Reviewed-by: Ilya Nikolaevskiy Commit-Queue: Henrik Boström Cr-Commit-Position: refs/heads/main@{#39605} --- media/engine/webrtc_video_engine.cc | 6 +++--- media/engine/webrtc_video_engine_unittest.cc | 10 ++-------- pc/peer_connection_simulcast_unittest.cc | 14 ++++++++++++++ 3 files changed, 19 insertions(+), 11 deletions(-) diff --git a/media/engine/webrtc_video_engine.cc b/media/engine/webrtc_video_engine.cc index dd91eb2782..b3afbf9fd8 100644 --- a/media/engine/webrtc_video_engine.cc +++ b/media/engine/webrtc_video_engine.cc @@ -2502,9 +2502,9 @@ 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): 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")) { + // 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")) { for (const webrtc::RtpEncodingParameters& encoding : rtp_parameters_.encodings) { if (encoding.scalability_mode.has_value()) { diff --git a/media/engine/webrtc_video_engine_unittest.cc b/media/engine/webrtc_video_engine_unittest.cc index e47f365491..c86ceb6176 100644 --- a/media/engine/webrtc_video_engine_unittest.cc +++ b/media/engine/webrtc_video_engine_unittest.cc @@ -8463,9 +8463,7 @@ TEST_F(WebRtcVideoChannelTest, webrtc::ScalabilityModeFromString(kDefaultScalabilityModeStr); EXPECT_EQ(2, stream->num_encoder_reconfigurations()); webrtc::VideoEncoderConfig encoder_config = stream->GetEncoderConfig().Copy(); - // When `scalability_mode` has been specified on any encoding, we do not - // reduce the number of simulcast streams. - EXPECT_EQ(3u, encoder_config.number_of_streams); + EXPECT_EQ(1u, encoder_config.number_of_streams); EXPECT_THAT(encoder_config.simulcast_layers, ElementsAre(Field(&webrtc::VideoStream::scalability_mode, ScalabilityMode::kL3T3), @@ -8478,11 +8476,7 @@ TEST_F(WebRtcVideoChannelTest, // VideoStreams are created appropriately for the simulcast case. EXPECT_THAT(stream->GetVideoStreams(), ElementsAre(Field(&webrtc::VideoStream::scalability_mode, - ScalabilityMode::kL3T3), - Field(&webrtc::VideoStream::scalability_mode, - kDefaultScalabilityMode), - Field(&webrtc::VideoStream::scalability_mode, - kDefaultScalabilityMode))); + ScalabilityMode::kL3T3))); // GetParameters. parameters = send_channel_->GetRtpSendParameters(last_ssrc_); diff --git a/pc/peer_connection_simulcast_unittest.cc b/pc/peer_connection_simulcast_unittest.cc index c0513b2a6a..1f5f17a517 100644 --- a/pc/peer_connection_simulcast_unittest.cc +++ b/pc/peer_connection_simulcast_unittest.cc @@ -60,6 +60,7 @@ #include "rtc_base/thread.h" #include "rtc_base/unique_id_generator.h" #include "system_wrappers/include/metrics.h" +#include "test/field_trial.h" #include "test/gmock.h" #include "test/gtest.h" @@ -1431,8 +1432,13 @@ 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); @@ -1497,9 +1503,17 @@ TEST_F(PeerConnectionSimulcastWithMediaFlowTests, EXPECT_THAT(*outbound_rtps[2]->scalability_mode, StrEq("L1T3")); } +// 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. if (!HasSenderVideoCodecCapability(local_pc_wrapper, "AV1")) { RTC_LOG(LS_WARNING) << "\n***\nAV1 is not available, skipping test.\n***"; return;