Add a flag to control legacy vs spec-compliant scalability mode.

The goal of the VP9 simulcast project is that when `scalability_mode`
is set, multiple encodings are always interpreted as simulcast, even
if VP9 or AV1 is used. This CL makes this so, but only if the flag
"WebRTC-AllowDisablingLegacyScalability" is "/Enabled/". This allows us
to make "SendingThreeEncodings_VP9_Simulcast" EXPECT VP9 simulcast.

When we are ready to ship we will remove the need to use the field
trial, but before we ship this we'll want to revisit if
SvcRateAllocator can be updated to support simulcast. (Today if we use
SvcRateAllocator when VP9 simulcast is used, all encodings except the
first one get bitrate=0, causing the test to fail because media is not
flowing on all layers.) For now, a TODO is added.

Bug: webrtc:14884
Change-Id: Ie20ae748b0c0405162f3a1b015ab94956ef83dae
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/297340
Reviewed-by: Evan Shrubsole <eshr@webrtc.org>
Reviewed-by: Erik Språng <sprang@webrtc.org>
Commit-Queue: Henrik Boström <hbos@webrtc.org>
Reviewed-by: Philip Eliasson <philipel@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#39552}

api/video/builtin_video_bitrate_allocator_factory.cc

@@ -12,6 +12,7 @@
 
 #include <memory>
 
+#include "absl/base/attributes.h"
 #include "absl/base/macros.h"
 #include "api/video/video_bitrate_allocator.h"
 #include "api/video_codecs/video_codec.h"
@@ -33,7 +34,12 @@ class BuiltinVideoBitrateAllocatorFactory
     switch (codec.codecType) {
       case kVideoCodecAV1:
       case kVideoCodecVP9:
-        return std::make_unique<SvcRateAllocator>(codec);
+        // TODO(https://crbug.com/webrtc/14884): Update SvcRateAllocator to
+        // support simulcast and use it for VP9/AV1 simulcast as well.
+        if (codec.numberOfSimulcastStreams <= 1) {
+          return std::make_unique<SvcRateAllocator>(codec);
+        }
+        ABSL_FALLTHROUGH_INTENDED;
       default:
         return std::make_unique<SimulcastRateAllocator>(codec);
     }

media/engine/webrtc_video_engine.cc

@@ -309,14 +309,15 @@ static bool ValidateStreamParams(const StreamParams& sp) {
 }
 
 // Returns true if the given codec is disallowed from doing simulcast.
-bool IsCodecDisabledForSimulcast(const std::string& codec_name,
+bool IsCodecDisabledForSimulcast(bool legacy_scalability_mode,
+                                 webrtc::VideoCodecType codec_type,
                                  const webrtc::FieldTrialsView& trials) {
-  if (absl::EqualsIgnoreCase(codec_name, kVp9CodecName) ||
+  if (legacy_scalability_mode && (codec_type == webrtc::kVideoCodecVP9 ||
-      absl::EqualsIgnoreCase(codec_name, kAv1CodecName)) {
+                                  codec_type == webrtc::kVideoCodecAV1)) {
     return true;
   }
 
-  if (absl::EqualsIgnoreCase(codec_name, kH264CodecName)) {
+  if (codec_type == webrtc::kVideoCodecH264) {
     return absl::StartsWith(trials.Lookup("WebRTC-H264Simulcast"), "Disabled");
   }
 
@@ -2497,11 +2498,26 @@ WebRtcVideoChannel::WebRtcVideoSendStream::CreateVideoEncoderConfig(
   }
 
   // By default, the stream count for the codec configuration should match the
-  // number of negotiated ssrcs. But if the codec is disabled for simulcast
+  // number of negotiated ssrcs but this may be capped below depending on the
-  // or a screencast (and not in simulcast screenshare experiment), only
+  // `legacy_scalability_mode` and codec used.
-  // configure a single stream.
   encoder_config.number_of_streams = parameters_.config.rtp.ssrcs.size();
-  if (IsCodecDisabledForSimulcast(codec.name, call_->trials())) {
+  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")) {
+    for (const webrtc::RtpEncodingParameters& encoding :
+         rtp_parameters_.encodings) {
+      if (encoding.scalability_mode.has_value()) {
+        legacy_scalability_mode = false;
+        break;
+      }
+    }
+  }
+  // Maybe limit the number of simulcast layers depending on
+  // `legacy_scalability_mode`, codec types (VP9/AV1) and the
+  // WebRTC-H264Simulcast/Disabled/ field trial.
+  if (IsCodecDisabledForSimulcast(legacy_scalability_mode,
+                                  encoder_config.codec_type, call_->trials())) {
     encoder_config.number_of_streams = 1;
   }
 

pc/BUILD.gn

@@ -2494,6 +2494,7 @@ if (rtc_include_tests && !build_with_chromium) {
       "../rtc_base/third_party/base64",
       "../rtc_base/third_party/sigslot",
       "../system_wrappers:metrics",
+      "../test:field_trial",
       "../test:run_loop",
       "../test:scoped_key_value_config",
       "../test/pc/sctp:fake_sctp_transport",

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"
 
@@ -1418,10 +1419,13 @@ TEST_F(PeerConnectionSimulcastWithMediaFlowTests,
               Optional(std::string("L3T3_KEY")));
 }
 
-// TODO(https://crbug.com/webrtc/14884): Support VP9 simulcast and update this
+// TODO(https://crbug.com/webrtc/14884): A field trial shouldn't be needed to
-// test to EXPECT three RTP streams of L1T3, not the single RTP we get today.
+// get spec-compliant behavior!
 TEST_F(PeerConnectionSimulcastWithMediaFlowTests,
        SendingThreeEncodings_VP9_Simulcast) {
+  test::ScopedFieldTrials field_trials(
+      "WebRTC-AllowDisablingLegacyScalability/Enabled/");
+
   rtc::scoped_refptr<PeerConnectionTestWrapper> local_pc_wrapper = CreatePc();
   rtc::scoped_refptr<PeerConnectionTestWrapper> remote_pc_wrapper = CreatePc();
   ExchangeIceCandidates(local_pc_wrapper, remote_pc_wrapper);
@@ -1449,23 +1453,7 @@ TEST_F(PeerConnectionSimulcastWithMediaFlowTests,
   local_pc_wrapper->WaitForConnection();
   remote_pc_wrapper->WaitForConnection();
 
-  // We want to EXPECT to get 3 "outbound-rtps", but because VP9 simulcast is
+  // GetParameters() does not report any fallback.
-  // not supported yet (webrtc:14884), we expect a single RTP stream. We get
-  // "L1T3" so we do avoid SVC fallback, but the other two layers are dropped
-  // and some parts of the code still assume SVC which could lead to other bugs
-  // such as invalid bitrate configurations.
-  EXPECT_TRUE_WAIT(HasOutboundRtpBytesSent(local_pc_wrapper, 1u),
-                   kLongTimeoutForRampingUp.ms());
-  rtc::scoped_refptr<const RTCStatsReport> report = GetStats(local_pc_wrapper);
-  std::vector<const RTCOutboundRtpStreamStats*> outbound_rtps =
-      report->GetStatsOfType<RTCOutboundRtpStreamStats>();
-  ASSERT_THAT(outbound_rtps, SizeIs(1u));
-  EXPECT_THAT(GetCurrentCodecMimeType(report, *outbound_rtps[0]),
-              StrCaseEq("video/VP9"));
-  // `scalability_mode` in stats does reflect LibvpxVp9Encoder settings!
-  EXPECT_EQ(*outbound_rtps[0]->scalability_mode, "L1T3");
-  // What GetParameters() is returning though is most likely just reflecting
-  // what was set, not what was configured.
   parameters = sender->GetParameters();
   ASSERT_EQ(parameters.encodings.size(), 3u);
   EXPECT_THAT(parameters.encodings[0].scalability_mode,
@@ -1474,6 +1462,32 @@ TEST_F(PeerConnectionSimulcastWithMediaFlowTests,
               Optional(std::string("L1T3")));
   EXPECT_THAT(parameters.encodings[2].scalability_mode,
               Optional(std::string("L1T3")));
+
+  // Wait until media is flowing on all three layers.
+  // Ramp up time is needed before all three layers are sending.
+  EXPECT_TRUE_WAIT(HasOutboundRtpBytesSent(local_pc_wrapper, 3u),
+                   kLongTimeoutForRampingUp.ms());
+  // Sometimes additional ramp up is needed to get the expected resolutions. If
+  // that has not happened yet we log (`log_during_ramp_up=true`).
+  EXPECT_TRUE_WAIT(HasOutboundRtpExpectedResolutions(
+                       local_pc_wrapper,
+                       {{"f", 320, 180}, {"h", 640, 360}, {"q", 1280, 720}},
+                       /*log_during_ramp_up=*/true),
+                   kLongTimeoutForRampingUp.ms());
+  // Verify codec and scalability mode.
+  rtc::scoped_refptr<const RTCStatsReport> report = GetStats(local_pc_wrapper);
+  std::vector<const RTCOutboundRtpStreamStats*> outbound_rtps =
+      report->GetStatsOfType<RTCOutboundRtpStreamStats>();
+  ASSERT_THAT(outbound_rtps, SizeIs(3u));
+  EXPECT_THAT(GetCurrentCodecMimeType(report, *outbound_rtps[0]),
+              StrCaseEq("video/VP9"));
+  EXPECT_THAT(GetCurrentCodecMimeType(report, *outbound_rtps[1]),
+              StrCaseEq("video/VP9"));
+  EXPECT_THAT(GetCurrentCodecMimeType(report, *outbound_rtps[2]),
+              StrCaseEq("video/VP9"));
+  EXPECT_THAT(*outbound_rtps[0]->scalability_mode, StrEq("L1T3"));
+  EXPECT_THAT(*outbound_rtps[1]->scalability_mode, StrEq("L1T3"));
+  EXPECT_THAT(*outbound_rtps[2]->scalability_mode, StrEq("L1T3"));
 }
 
 }  // namespace webrtc

video/encoder_bitrate_adjuster.cc

@@ -48,12 +48,11 @@ EncoderBitrateAdjuster::EncoderBitrateAdjuster(const VideoCodec& codec_settings)
                                       .BitrateAdjusterCanUseNetworkHeadroom()),
       frames_since_layout_change_(0),
       min_bitrates_bps_{} {
-  // TODO(https://crbug.com/webrtc/14884): In order to support simulcast VP9,
+  // TODO(https://crbug.com/webrtc/14891): If we want to support simulcast of
-  // this code needs to be updated to care about `numberOfSimulcastStreams` even
+  // SVC streams, EncoderBitrateAdjuster needs to be updated to care about both
-  // in the case of VP9.
+  // `simulcastStream` and `spatialLayers` at the same time.
-  // TODO(https://crbug.com/webrtc/14891): This also needs to be updated in
+  if (codec_settings.codecType == VideoCodecType::kVideoCodecVP9 &&
-  // order to support a mix of simulcast and SVC.
+      codec_settings.numberOfSimulcastStreams <= 1) {
-  if (codec_settings.codecType == VideoCodecType::kVideoCodecVP9) {
     for (size_t si = 0; si < codec_settings.VP9().numberOfSpatialLayers; ++si) {
       if (codec_settings.spatialLayers[si].active) {
         min_bitrates_bps_[si] =

video/video_stream_encoder.cc

@@ -1376,7 +1376,8 @@ void VideoStreamEncoder::ReconfigureEncoder() {
   bool is_svc = false;
   // Set min_bitrate_bps, max_bitrate_bps, and max padding bit rate for VP9
   // and leave only one stream containing all necessary information.
-  if (encoder_config_.codec_type == kVideoCodecVP9) {
+  if (encoder_config_.codec_type == kVideoCodecVP9 &&
+      encoder_config_.number_of_streams == 1) {
     // Lower max bitrate to the level codec actually can produce.
     streams[0].max_bitrate_bps =
         std::min(streams[0].max_bitrate_bps,