diff --git a/api/video/builtin_video_bitrate_allocator_factory.cc b/api/video/builtin_video_bitrate_allocator_factory.cc index 4c24a0e75d..81e24e38bf 100644 --- a/api/video/builtin_video_bitrate_allocator_factory.cc +++ b/api/video/builtin_video_bitrate_allocator_factory.cc @@ -12,6 +12,7 @@ #include +#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(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(codec); + } + ABSL_FALLTHROUGH_INTENDED; default: return std::make_unique(codec); } diff --git a/media/engine/webrtc_video_engine.cc b/media/engine/webrtc_video_engine.cc index 38e0467555..b3afbf9fd8 100644 --- a/media/engine/webrtc_video_engine.cc +++ b/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) || - absl::EqualsIgnoreCase(codec_name, kAv1CodecName)) { + if (legacy_scalability_mode && (codec_type == webrtc::kVideoCodecVP9 || + 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 - // or a screencast (and not in simulcast screenshare experiment), only - // configure a single stream. + // number of negotiated ssrcs but this may be capped below depending on the + // `legacy_scalability_mode` and codec used. 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; } diff --git a/pc/BUILD.gn b/pc/BUILD.gn index 43dd114934..ea1909bc4f 100644 --- a/pc/BUILD.gn +++ b/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", diff --git a/pc/peer_connection_simulcast_unittest.cc b/pc/peer_connection_simulcast_unittest.cc index 7b433b87c2..1e8955be03 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" @@ -1418,10 +1419,13 @@ TEST_F(PeerConnectionSimulcastWithMediaFlowTests, Optional(std::string("L3T3_KEY"))); } -// TODO(https://crbug.com/webrtc/14884): Support VP9 simulcast and update this -// test to EXPECT three RTP streams of L1T3, not the single RTP we get today. +// 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); @@ -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 - // 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 report = GetStats(local_pc_wrapper); - std::vector outbound_rtps = - report->GetStatsOfType(); - 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. + // GetParameters() does not report any fallback. 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 report = GetStats(local_pc_wrapper); + std::vector outbound_rtps = + report->GetStatsOfType(); + 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 diff --git a/video/encoder_bitrate_adjuster.cc b/video/encoder_bitrate_adjuster.cc index 14f73ee442..c672173895 100644 --- a/video/encoder_bitrate_adjuster.cc +++ b/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, - // this code needs to be updated to care about `numberOfSimulcastStreams` even - // in the case of VP9. - // TODO(https://crbug.com/webrtc/14891): This also needs to be updated in - // order to support a mix of simulcast and SVC. - if (codec_settings.codecType == VideoCodecType::kVideoCodecVP9) { + // TODO(https://crbug.com/webrtc/14891): If we want to support simulcast of + // SVC streams, EncoderBitrateAdjuster needs to be updated to care about both + // `simulcastStream` and `spatialLayers` at the same time. + if (codec_settings.codecType == VideoCodecType::kVideoCodecVP9 && + codec_settings.numberOfSimulcastStreams <= 1) { for (size_t si = 0; si < codec_settings.VP9().numberOfSpatialLayers; ++si) { if (codec_settings.spatialLayers[si].active) { min_bitrates_bps_[si] = diff --git a/video/video_stream_encoder.cc b/video/video_stream_encoder.cc index 7e10e778e3..34fa3621f7 100644 --- a/video/video_stream_encoder.cc +++ b/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,