From 75990b9a8f98ea2d597a31472fb778ec4d55f698 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Bostr=C3=B6m?= Date: Mon, 20 Mar 2023 10:09:04 +0100 Subject: [PATCH] Ship ability to opt-in to VP9/AV1 simulcast. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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} --- 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, 11 insertions(+), 19 deletions(-) diff --git a/media/engine/webrtc_video_engine.cc b/media/engine/webrtc_video_engine.cc index b3afbf9fd8..dd91eb2782 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): 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()) { diff --git a/media/engine/webrtc_video_engine_unittest.cc b/media/engine/webrtc_video_engine_unittest.cc index c86ceb6176..e47f365491 100644 --- a/media/engine/webrtc_video_engine_unittest.cc +++ b/media/engine/webrtc_video_engine_unittest.cc @@ -8463,7 +8463,9 @@ TEST_F(WebRtcVideoChannelTest, webrtc::ScalabilityModeFromString(kDefaultScalabilityModeStr); EXPECT_EQ(2, stream->num_encoder_reconfigurations()); webrtc::VideoEncoderConfig encoder_config = stream->GetEncoderConfig().Copy(); - EXPECT_EQ(1u, encoder_config.number_of_streams); + // 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_THAT(encoder_config.simulcast_layers, ElementsAre(Field(&webrtc::VideoStream::scalability_mode, ScalabilityMode::kL3T3), @@ -8476,7 +8478,11 @@ TEST_F(WebRtcVideoChannelTest, // VideoStreams are created appropriately for the simulcast case. EXPECT_THAT(stream->GetVideoStreams(), ElementsAre(Field(&webrtc::VideoStream::scalability_mode, - ScalabilityMode::kL3T3))); + ScalabilityMode::kL3T3), + Field(&webrtc::VideoStream::scalability_mode, + kDefaultScalabilityMode), + Field(&webrtc::VideoStream::scalability_mode, + kDefaultScalabilityMode))); // GetParameters. parameters = send_channel_->GetRtpSendParameters(last_ssrc_); diff --git a/pc/peer_connection_simulcast_unittest.cc b/pc/peer_connection_simulcast_unittest.cc index 1f5f17a517..c0513b2a6a 100644 --- a/pc/peer_connection_simulcast_unittest.cc +++ b/pc/peer_connection_simulcast_unittest.cc @@ -60,7 +60,6 @@ #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" @@ -1432,13 +1431,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); @@ -1503,17 +1497,9 @@ 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;