Fix L1Tx target bitrate bug when the standard API is used.

There are now multiple ways to configure VP9 L1Tx:
- Legacy API: configure legacy SVC and disable encodings, this gets
  interpreted as disabling spatial layers (non-standard API hack).
- Standard API: configure scalability_mode. This can be done either
  with a single encoding or multiple encodings. As long as only one
  encoding is active we get a single L1Tx ssrc, same as legacy API.

Due to a bug, the ApplySpatialLayerBitrateLimits() logic which tweaks
bitrates was only applied in the legacy API code path, not the standard
API code path, despite both code paths configuring L1Tx.

The issue is that IsSimulcastOrMultipleSpatialLayers() was checking if
`number_of_streams == 1`. This is true in legacy code path but not
standard code path. The fix is to look at
`numberOfSimulcastStreams == 1` instead, which is set to the correct
value regardless of code path used.

This CL adds comments documenting the difference between
`number_of_streams` and `numberOfSimulcastStreams` to reduce the risk
of more mistakes like this in the future.

Bug: chromium:1455039, b:279161263
Change-Id: I69789b68cc5d45ef1b3becd310687c8dec8e7c87
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/308722
Reviewed-by: Ilya Nikolaevskiy <ilnik@webrtc.org>
Commit-Queue: Henrik Boström <hbos@webrtc.org>
Reviewed-by: Erik Språng <sprang@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#40287}

api/video_codecs/video_codec.h

@@ -148,6 +148,15 @@ class RTC_EXPORT VideoCodec {
   bool active;
 
   unsigned int qpMax;
+  // The actual number of simulcast streams. This is <= 1 in singlecast (it can
+  // be 0 in old code paths), but it is also 1 in the {active,inactive,inactive}
+  // "single RTP simulcast" use case and the legacy kSVC use case. In all other
+  // cases this is the same as the number of encodings (which may include
+  // inactive encodings). In other words:
+  // - `numberOfSimulcastStreams <= 1` in singlecast and singlecast-like setups
+  //   including legacy kSVC (encodings interpreted as spatial layers) or
+  //   standard kSVC (1 active encoding).
+  // - `numberOfSimulcastStreams > 1` in simulcast of 2+ active encodings.
   unsigned char numberOfSimulcastStreams;
   SimulcastStream simulcastStream[kMaxSimulcastStreams];
   SpatialLayer spatialLayers[kMaxSpatialLayers];

pc/BUILD.gn

@@ -2435,6 +2435,7 @@ if (rtc_include_tests && !build_with_chromium) {
       "../api/transport:field_trial_based_config",
       "../api/transport:sctp_transport_factory_interface",
       "../api/transport/rtp:rtp_source",
+      "../api/units:data_rate",
       "../api/units:time_delta",
       "../api/units:timestamp",
       "../api/video:builtin_video_bitrate_allocator_factory",

pc/peer_connection_encodings_integrationtest.cc

@@ -17,6 +17,7 @@
 #include "api/audio_codecs/opus_audio_encoder_factory.h"
 #include "api/rtp_parameters.h"
 #include "api/stats/rtcstats_objects.h"
+#include "api/units/data_rate.h"
 #include "api/video_codecs/video_decoder_factory_template.h"
 #include "api/video_codecs/video_decoder_factory_template_dav1d_adapter.h"
 #include "api/video_codecs/video_decoder_factory_template_libvpx_vp8_adapter.h"
@@ -55,6 +56,12 @@ constexpr TimeDelta kDefaultTimeout = TimeDelta::Seconds(5);
 // using simulated time.
 constexpr TimeDelta kLongTimeoutForRampingUp = TimeDelta::Minutes(1);
 
+// The max bitrate 1500 kbps may be subject to change in the future. What we're
+// interested in here is that all code paths that result in L1T3 result in the
+// same target bitrate which does not exceed this limit.
+constexpr DataRate kVp9ExpectedMaxBitrateForL1T3 =
+    DataRate::KilobitsPerSec(1500);
+
 struct StringParamToString {
   std::string operator()(const ::testing::TestParamInfo<std::string>& info) {
     return info.param;
@@ -792,6 +799,103 @@ TEST_F(PeerConnectionEncodingsIntegrationTest,
   EXPECT_EQ(*outbound_rtps[2]->bytes_sent, 0u);
 }
 
+TEST_F(PeerConnectionEncodingsIntegrationTest, VP9_TargetBitrate_LegacyL1T3) {
+  rtc::scoped_refptr<PeerConnectionTestWrapper> local_pc_wrapper = CreatePc();
+  rtc::scoped_refptr<PeerConnectionTestWrapper> remote_pc_wrapper = CreatePc();
+  ExchangeIceCandidates(local_pc_wrapper, remote_pc_wrapper);
+
+  std::vector<cricket::SimulcastLayer> layers =
+      CreateLayers({"f", "h", "q"}, /*active=*/true);
+  rtc::scoped_refptr<RtpTransceiverInterface> transceiver =
+      AddTransceiverWithSimulcastLayers(local_pc_wrapper, remote_pc_wrapper,
+                                        layers);
+  std::vector<RtpCodecCapability> codecs =
+      GetCapabilitiesAndRestrictToCodec(local_pc_wrapper, "VP9");
+  transceiver->SetCodecPreferences(codecs);
+
+  // In legacy SVC, disabling the bottom two layers encodings is interpreted as
+  // disabling the bottom two spatial layers resulting in L1T3.
+  rtc::scoped_refptr<RtpSenderInterface> sender = transceiver->sender();
+  RtpParameters parameters = sender->GetParameters();
+  parameters.encodings[0].active = false;
+  parameters.encodings[1].active = false;
+  parameters.encodings[2].active = true;
+  sender->SetParameters(parameters);
+
+  NegotiateWithSimulcastTweaks(local_pc_wrapper, remote_pc_wrapper, layers);
+  local_pc_wrapper->WaitForConnection();
+  remote_pc_wrapper->WaitForConnection();
+
+  // Wait until 720p L1T3 has ramped up to 720p. It may take additional time
+  // for the target bitrate to reach its maximum.
+  ASSERT_TRUE_WAIT(HasOutboundRtpWithRidAndScalabilityMode(local_pc_wrapper,
+                                                           "f", "L1T3", 720),
+                   kLongTimeoutForRampingUp.ms());
+
+  // The target bitrate typically reaches `kVp9ExpectedMaxBitrateForL1T3`
+  // in a short period of time. However to reduce risk of flakiness in bot
+  // environments, this test only fails if we we exceed the expected target.
+  rtc::Thread::Current()->SleepMs(1000);
+  rtc::scoped_refptr<const RTCStatsReport> report = GetStats(local_pc_wrapper);
+  std::vector<const RTCOutboundRtpStreamStats*> outbound_rtps =
+      report->GetStatsOfType<RTCOutboundRtpStreamStats>();
+  ASSERT_THAT(outbound_rtps, SizeIs(1));
+  DataRate target_bitrate =
+      DataRate::BitsPerSec(*outbound_rtps[0]->target_bitrate);
+  EXPECT_LE(target_bitrate.kbps(), kVp9ExpectedMaxBitrateForL1T3.kbps());
+}
+
+// Test coverage for https://crbug.com/1455039.
+TEST_F(PeerConnectionEncodingsIntegrationTest, VP9_TargetBitrate_StandardL1T3) {
+  rtc::scoped_refptr<PeerConnectionTestWrapper> local_pc_wrapper = CreatePc();
+  rtc::scoped_refptr<PeerConnectionTestWrapper> remote_pc_wrapper = CreatePc();
+  ExchangeIceCandidates(local_pc_wrapper, remote_pc_wrapper);
+
+  std::vector<cricket::SimulcastLayer> layers =
+      CreateLayers({"f", "h", "q"}, /*active=*/true);
+  rtc::scoped_refptr<RtpTransceiverInterface> transceiver =
+      AddTransceiverWithSimulcastLayers(local_pc_wrapper, remote_pc_wrapper,
+                                        layers);
+  std::vector<RtpCodecCapability> codecs =
+      GetCapabilitiesAndRestrictToCodec(local_pc_wrapper, "VP9");
+  transceiver->SetCodecPreferences(codecs);
+
+  // With standard APIs, L1T3 is explicitly specified and the encodings refers
+  // to the RTP streams, not the spatial layers. The end result should be
+  // equivalent to the legacy L1T3 case.
+  rtc::scoped_refptr<RtpSenderInterface> sender = transceiver->sender();
+  RtpParameters parameters = sender->GetParameters();
+  parameters.encodings[0].active = true;
+  parameters.encodings[0].scale_resolution_down_by = 1.0;
+  parameters.encodings[0].scalability_mode = "L1T3";
+  parameters.encodings[1].active = false;
+  parameters.encodings[2].active = false;
+  sender->SetParameters(parameters);
+
+  NegotiateWithSimulcastTweaks(local_pc_wrapper, remote_pc_wrapper, layers);
+  local_pc_wrapper->WaitForConnection();
+  remote_pc_wrapper->WaitForConnection();
+
+  // Wait until 720p L1T3 has ramped up to 720p. It may take additional time
+  // for the target bitrate to reach its maximum.
+  ASSERT_TRUE_WAIT(HasOutboundRtpWithRidAndScalabilityMode(local_pc_wrapper,
+                                                           "f", "L1T3", 720),
+                   kLongTimeoutForRampingUp.ms());
+
+  // The target bitrate typically reaches `kVp9ExpectedMaxBitrateForL1T3`
+  // in a short period of time. However to reduce risk of flakiness in bot
+  // environments, this test only fails if we we exceed the expected target.
+  rtc::Thread::Current()->SleepMs(1000);
+  rtc::scoped_refptr<const RTCStatsReport> report = GetStats(local_pc_wrapper);
+  std::vector<const RTCOutboundRtpStreamStats*> outbound_rtps =
+      report->GetStatsOfType<RTCOutboundRtpStreamStats>();
+  ASSERT_THAT(outbound_rtps, SizeIs(3));
+  auto* outbound_rtp = FindOutboundRtpByRid(outbound_rtps, "f");
+  ASSERT_TRUE(outbound_rtp);
+  DataRate target_bitrate = DataRate::BitsPerSec(*outbound_rtp->target_bitrate);
+  EXPECT_LE(target_bitrate.kbps(), kVp9ExpectedMaxBitrateForL1T3.kbps());
+}
+
 // Tests that use the standard path (specifying both `scalability_mode` and
 // `scale_resolution_down_by`) should pass for all codecs.
 class PeerConnectionEncodingsIntegrationParameterizedTest

video/adaptation/bitrate_constraint.cc

@@ -58,7 +58,8 @@ bool BitrateConstraint::IsAdaptationUpAllowed(
     }
 
     if (VideoStreamEncoderResourceManager::IsSimulcastOrMultipleSpatialLayers(
-            encoder_settings_->encoder_config())) {
+            encoder_settings_->encoder_config(),
+            encoder_settings_->video_codec())) {
       // Resolution bitrate limits usage is restricted to singlecast.
       return true;
     }

video/adaptation/bitrate_constraint_unittest.cc

@@ -47,7 +47,7 @@ void FillCodecConfig(VideoCodec* video_codec,
                      bool svc) {
   size_t num_layers = params.size();
   video_codec->codecType = kVideoCodecVP8;
-  video_codec->numberOfSimulcastStreams = num_layers;
+  video_codec->numberOfSimulcastStreams = svc ? 1 : num_layers;
 
   encoder_config->number_of_streams = svc ? 1 : num_layers;
   encoder_config->simulcast_layers.resize(num_layers);

video/adaptation/video_stream_encoder_resource_manager.cc

@@ -815,7 +815,8 @@ void VideoStreamEncoderResourceManager::OnQualityRampUp() {
 }
 
 bool VideoStreamEncoderResourceManager::IsSimulcastOrMultipleSpatialLayers(
-    const VideoEncoderConfig& encoder_config) {
+    const VideoEncoderConfig& encoder_config,
+    const VideoCodec& video_codec) {
   const std::vector<VideoStream>& simulcast_layers =
       encoder_config.simulcast_layers;
   if (simulcast_layers.empty()) {
@@ -824,7 +825,7 @@ bool VideoStreamEncoderResourceManager::IsSimulcastOrMultipleSpatialLayers(
 
   absl::optional<int> num_spatial_layers;
   if (simulcast_layers[0].scalability_mode.has_value() &&
-      encoder_config.number_of_streams == 1) {
+      video_codec.numberOfSimulcastStreams == 1) {
     num_spatial_layers = ScalabilityModeToNumSpatialLayers(
         *simulcast_layers[0].scalability_mode);
   }

video/adaptation/video_stream_encoder_resource_manager.h

@@ -153,7 +153,8 @@ class VideoStreamEncoderResourceManager
   void OnQualityRampUp() override;
 
   static bool IsSimulcastOrMultipleSpatialLayers(
-      const VideoEncoderConfig& encoder_config);
+      const VideoEncoderConfig& encoder_config,
+      const VideoCodec& video_codec);
 
  private:
   class InitialFrameDropper;

video/config/video_encoder_config.h

@@ -181,9 +181,15 @@ class VideoEncoderConfig {
   // down to lower layers for the video encoding.
   // `simulcast_layers` is also used for configuring non-simulcast (when there
   // is a single VideoStream).
+  // We have the same number of `simulcast_layers` as we have negotiated
+  // encodings, for example 3 are used in both simulcast and legacy kSVC.
   std::vector<VideoStream> simulcast_layers;
 
   // Max number of encoded VideoStreams to produce.
+  // This is the same as the number of encodings negotiated (i.e. SSRCs),
+  // whether or not those encodings are `active`, except for when legacy kSVC
+  // is used. In this case we have three SSRCs but `number_of_streams` is
+  // changed to 1 to tell lower layers to limit the number of streams.
   size_t number_of_streams;
 
   // Legacy Google conference mode flag for simulcast screenshare

video/video_stream_encoder.cc

@@ -405,7 +405,7 @@ void ApplySpatialLayerBitrateLimits(
     return;
   }
   if (VideoStreamEncoderResourceManager::IsSimulcastOrMultipleSpatialLayers(
-          encoder_config) ||
+          encoder_config, *codec) ||
       encoder_config.simulcast_layers.size() <= 1) {
     // Resolution bitrate limits usage is restricted to singlecast.
     return;