From e6f0c2fd23f6038048085dc4943accedd083f512 Mon Sep 17 00:00:00 2001 From: Evan Shrubsole Date: Wed, 6 Nov 2024 13:33:21 +0000 Subject: [PATCH] SEA discards inactive encoders in implementation name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Inactive encoders are included in the string when they are paused due to bitrate allocation being 0 for that simulcast layer. #rtc_ktlo Bug: webrtc:376804631 Change-Id: I4234b452b60fee58981907380df41962fda5bf40 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/367660 Reviewed-by: Henrik Boström Commit-Queue: Evan Shrubsole Reviewed-by: Erik Språng Cr-Commit-Position: refs/heads/main@{#43367} --- media/BUILD.gn | 3 + media/engine/simulcast_encoder_adapter.cc | 26 ++-- .../simulcast_encoder_adapter_unittest.cc | 57 +++++++- .../utility/simulcast_test_fixture_impl.cc | 19 ++- ...er_connection_encodings_integrationtest.cc | 134 +++++++++++++++++- 5 files changed, 220 insertions(+), 19 deletions(-) diff --git a/media/BUILD.gn b/media/BUILD.gn index 1b57100ab3..0afc9f89ae 100644 --- a/media/BUILD.gn +++ b/media/BUILD.gn @@ -473,6 +473,7 @@ rtc_library("rtc_simulcast_encoder_adapter") { "../modules/video_coding:video_coding_utility", "../rtc_base:checks", "../rtc_base:logging", + "../rtc_base:stringutils", "../rtc_base/experiments:encoder_info_settings", "../rtc_base/experiments:rate_control_settings", "../rtc_base/system:no_unique_address", @@ -952,12 +953,14 @@ if (rtc_include_tests) { "../api/transport:bitrate_settings", "../api/transport:field_trial_based_config", "../api/transport/rtp:rtp_source", + "../api/units:data_rate", "../api/units:time_delta", "../api/units:timestamp", "../api/video:builtin_video_bitrate_allocator_factory", "../api/video:recordable_encoded_frame", "../api/video:resolution", "../api/video:video_bitrate_allocation", + "../api/video:video_bitrate_allocator", "../api/video:video_bitrate_allocator_factory", "../api/video:video_codec_constants", "../api/video:video_frame", diff --git a/media/engine/simulcast_encoder_adapter.cc b/media/engine/simulcast_encoder_adapter.cc index 7195a39957..50a1e03fb2 100644 --- a/media/engine/simulcast_encoder_adapter.cc +++ b/media/engine/simulcast_encoder_adapter.cc @@ -58,6 +58,8 @@ #include "rtc_base/checks.h" #include "rtc_base/experiments/rate_control_settings.h" #include "rtc_base/logging.h" +#include "rtc_base/strings/str_join.h" +#include "rtc_base/strings/string_builder.h" namespace webrtc { namespace { @@ -393,6 +395,9 @@ int SimulcastEncoderAdapter::InitEncode( << WebRtcVideoCodecErrorToString(ret); return ret; } + RTC_LOG(LS_WARNING) << "[SEA] InitEncode: failed with error code: " + << WebRtcVideoCodecErrorToString(ret) + << ". Falling back to multi-encoder mode."; } // Multi-encoder simulcast or singlecast (deactivated layers). @@ -945,15 +950,17 @@ VideoEncoder::EncoderInfo SimulcastEncoderAdapter::GetEncoderInfo() const { } encoder_info.scaling_settings = VideoEncoder::ScalingSettings::kOff; + std::vector encoder_names; for (size_t i = 0; i < stream_contexts_.size(); ++i) { VideoEncoder::EncoderInfo encoder_impl_info = stream_contexts_[i].encoder().GetEncoderInfo(); - if (i == 0) { - // Encoder name indicates names of all sub-encoders. - encoder_info.implementation_name += " ("; - encoder_info.implementation_name += encoder_impl_info.implementation_name; + // Encoder name indicates names of all active sub-encoders. + if (!stream_contexts_[i].is_paused()) { + encoder_names.push_back(encoder_impl_info.implementation_name); + } + if (i == 0) { encoder_info.supports_native_handle = encoder_impl_info.supports_native_handle; encoder_info.has_trusted_rate_controller = @@ -962,9 +969,6 @@ VideoEncoder::EncoderInfo SimulcastEncoderAdapter::GetEncoderInfo() const { encoder_impl_info.is_hardware_accelerated; encoder_info.is_qp_trusted = encoder_impl_info.is_qp_trusted; } else { - encoder_info.implementation_name += ", "; - encoder_info.implementation_name += encoder_impl_info.implementation_name; - // Native handle supported if any encoder supports it. encoder_info.supports_native_handle |= encoder_impl_info.supports_native_handle; @@ -999,7 +1003,13 @@ VideoEncoder::EncoderInfo SimulcastEncoderAdapter::GetEncoderInfo() const { encoder_info.apply_alignment_to_all_simulcast_layers = true; } } - encoder_info.implementation_name += ")"; + + if (!encoder_names.empty()) { + rtc::StringBuilder implementation_name_builder(" ("); + implementation_name_builder << StrJoin(encoder_names, ", "); + implementation_name_builder << ")"; + encoder_info.implementation_name += implementation_name_builder.Release(); + } OverrideFromFieldTrial(&encoder_info); diff --git a/media/engine/simulcast_encoder_adapter_unittest.cc b/media/engine/simulcast_encoder_adapter_unittest.cc index b5c4cd1a5c..e54654f132 100644 --- a/media/engine/simulcast_encoder_adapter_unittest.cc +++ b/media/engine/simulcast_encoder_adapter_unittest.cc @@ -21,7 +21,10 @@ #include "api/test/simulcast_test_fixture.h" #include "api/test/video/function_video_decoder_factory.h" #include "api/test/video/function_video_encoder_factory.h" +#include "api/units/data_rate.h" +#include "api/video/video_bitrate_allocator.h" #include "api/video/video_codec_constants.h" +#include "api/video_codecs/scalability_mode.h" #include "api/video_codecs/sdp_video_format.h" #include "api/video_codecs/video_encoder.h" #include "api/video_codecs/video_encoder_factory.h" @@ -992,15 +995,15 @@ TEST_F(TestSimulcastEncoderAdapterFake, SupportsImplementationName) { SimulcastTestFixtureImpl::DefaultSettings( &codec_, static_cast(kTestTemporalLayerProfile), kVideoCodecVP8); + codec_.numberOfSimulcastStreams = 2; std::vector encoder_names; encoder_names.push_back("codec1"); encoder_names.push_back("codec2"); - encoder_names.push_back("codec3"); helper_->factory()->SetEncoderNames(encoder_names); EXPECT_EQ("SimulcastEncoderAdapter", adapter_->GetEncoderInfo().implementation_name); EXPECT_EQ(0, adapter_->InitEncode(&codec_, kSettings)); - EXPECT_EQ("SimulcastEncoderAdapter (codec1, codec2, codec3)", + EXPECT_EQ("SimulcastEncoderAdapter (codec1, codec2)", adapter_->GetEncoderInfo().implementation_name); // Single streams should not expose "SimulcastEncoderAdapter" in name. @@ -1022,13 +1025,59 @@ TEST_F(TestSimulcastEncoderAdapterFake, RuntimeEncoderInfoUpdate) { encoder_names.push_back("codec3"); helper_->factory()->SetEncoderNames(encoder_names); EXPECT_EQ(0, adapter_->InitEncode(&codec_, kSettings)); - EXPECT_EQ("SimulcastEncoderAdapter (codec1, codec2, codec3)", + EXPECT_EQ("SimulcastEncoderAdapter (codec1, codec2)", adapter_->GetEncoderInfo().implementation_name); // Change name of first encoder to indicate it has done a fallback to another // implementation. helper_->factory()->encoders().front()->set_implementation_name("fallback1"); - EXPECT_EQ("SimulcastEncoderAdapter (fallback1, codec2, codec3)", + EXPECT_EQ("SimulcastEncoderAdapter (fallback1, codec2)", + adapter_->GetEncoderInfo().implementation_name); +} + +TEST_F(TestSimulcastEncoderAdapterFake, EncoderInfoDeactiveLayersUpdatesName) { + SimulcastTestFixtureImpl::DefaultSettings( + &codec_, static_cast(kTestTemporalLayerProfile), + kVideoCodecVP8); + const DataRate target_bitrate = + DataRate::KilobitsPerSec(codec_.simulcastStream[0].targetBitrate + + codec_.simulcastStream[1].targetBitrate + + codec_.simulcastStream[2].targetBitrate); + const DataRate bandwidth_allocation = + target_bitrate + DataRate::KilobitsPerSec(600); + const DataRate target_bitrate_without_layer3 = + target_bitrate - + DataRate::KilobitsPerSec(codec_.simulcastStream[2].targetBitrate); + const DataRate bandwidth_allocation_without_layer3 = + target_bitrate + DataRate::KilobitsPerSec(300); + + std::vector encoder_names = {"codec1", "codec2", "codec3"}; + helper_->factory()->SetEncoderNames(encoder_names); + rate_allocator_ = std::make_unique(env_, codec_); + + EXPECT_EQ(0, adapter_->InitEncode(&codec_, kSettings)); + adapter_->SetRates(VideoEncoder::RateControlParameters( + rate_allocator_->Allocate( + VideoBitrateAllocationParameters(target_bitrate, 30)), + 30.0, bandwidth_allocation)); + EXPECT_EQ("SimulcastEncoderAdapter (codec1, codec2, codec3)", + adapter_->GetEncoderInfo().implementation_name); + + // Disable the third encoder using bitrate allocation. + adapter_->SetRates(VideoEncoder::RateControlParameters( + rate_allocator_->Allocate( + VideoBitrateAllocationParameters(target_bitrate_without_layer3, 30)), + 30.0, bandwidth_allocation_without_layer3)); + EXPECT_EQ("SimulcastEncoderAdapter (codec1, codec2)", + adapter_->GetEncoderInfo().implementation_name); + + // Enable the third encoder again using bitrate allocation. + rate_allocator_ = std::make_unique(env_, codec_); + adapter_->SetRates(VideoEncoder::RateControlParameters( + rate_allocator_->Allocate( + VideoBitrateAllocationParameters(target_bitrate, 30)), + 30.0, bandwidth_allocation)); + EXPECT_EQ("SimulcastEncoderAdapter (codec1, codec2, codec3)", adapter_->GetEncoderInfo().implementation_name); } diff --git a/modules/video_coding/utility/simulcast_test_fixture_impl.cc b/modules/video_coding/utility/simulcast_test_fixture_impl.cc index 7cba7e6d77..13635e5fbe 100644 --- a/modules/video_coding/utility/simulcast_test_fixture_impl.cc +++ b/modules/video_coding/utility/simulcast_test_fixture_impl.cc @@ -246,12 +246,19 @@ void SimulcastTestFixtureImpl::DefaultSettings( &settings->simulcastStream[layer_order[2]], temporal_layer_profile[2]); settings->SetFrameDropEnabled(true); - if (codec_type == kVideoCodecVP8) { - settings->VP8()->denoisingOn = true; - settings->VP8()->automaticResizeOn = false; - settings->VP8()->keyFrameInterval = 3000; - } else { - settings->H264()->keyFrameInterval = 3000; + switch (codec_type) { + case kVideoCodecVP8: + settings->VP8()->denoisingOn = true; + settings->VP8()->automaticResizeOn = false; + settings->VP8()->keyFrameInterval = 3000; + break; + case kVideoCodecH264: + settings->H264()->keyFrameInterval = 3000; + break; + case kVideoCodecVP9: + break; + default: + RTC_CHECK_NOTREACHED(); } } diff --git a/pc/peer_connection_encodings_integrationtest.cc b/pc/peer_connection_encodings_integrationtest.cc index fd86173cd3..6ebc247a46 100644 --- a/pc/peer_connection_encodings_integrationtest.cc +++ b/pc/peer_connection_encodings_integrationtest.cc @@ -11,6 +11,7 @@ #include #include #include +#include #include #include #include @@ -55,6 +56,7 @@ #include "test/scoped_key_value_config.h" using ::testing::Eq; +using ::testing::Field; using ::testing::Optional; using ::testing::SizeIs; using ::testing::StrCaseEq; @@ -78,6 +80,22 @@ constexpr TimeDelta kLongTimeoutForRampingUp = TimeDelta::Minutes(1); constexpr DataRate kVp9ExpectedMaxBitrateForL1T3 = DataRate::KilobitsPerSec(1500); +auto HasEncoderImplementation(absl::string_view impl) { + return Field("encoder_implementation", + &RTCOutboundRtpStreamStats::encoder_implementation, + Optional(StrEq(impl))); +} + +auto HasScalabilityMode(absl::string_view mode) { + return Field("scalability_mode", &RTCOutboundRtpStreamStats::scalability_mode, + Optional(StrEq(mode))); +} + +auto HasEmptyScalabilityMode() { + return Field("scalability_mode", &RTCOutboundRtpStreamStats::scalability_mode, + Eq(std::nullopt)); +} + struct StringParamToString { std::string operator()(const ::testing::TestParamInfo& info) { return info.param; @@ -320,6 +338,9 @@ class PeerConnectionEncodingsIntegrationTest : public ::testing::Test { report->GetStatsOfType(); for (const auto* outbound_rtp : outbound_rtps) { if (outbound_rtp->rid.value_or("") == rid) { + RTC_LOG(LS_INFO) << "Encoding " << rid << " is " + << (outbound_rtp->active.value_or(false) ? "active" + : "inactive"); return *outbound_rtp->active; } } @@ -327,6 +348,19 @@ class PeerConnectionEncodingsIntegrationTest : public ::testing::Test { return false; } + std::string GetEncoderImplementationName( + rtc::scoped_refptr pc_wrapper) { + rtc::scoped_refptr report = GetStats(pc_wrapper); + std::vector outbound_rtps = + report->GetStatsOfType(); + for (const auto* outbound_rtp : outbound_rtps) { + if (outbound_rtp->encoder_implementation.has_value()) { + return *outbound_rtp->encoder_implementation; + } + } + return "unknown"; + } + Resolution GetEncodingResolution( rtc::scoped_refptr pc_wrapper, std::string_view rid = "") { @@ -828,7 +862,7 @@ TEST_F(PeerConnectionEncodingsIntegrationTest, } // Exercise common path where `scalability_mode` is not specified until after -// negotiation, requring us to recreate the stream when the number of streams +// negotiation, requiring us to recreate the stream when the number of streams // changes from 1 (legacy SVC) to 3 (standard simulcast). TEST_F(PeerConnectionEncodingsIntegrationTest, VP9_SwitchFromLegacySvcToStandardSingleActiveEncodingSvc) { @@ -885,6 +919,98 @@ TEST_F(PeerConnectionEncodingsIntegrationTest, EXPECT_FALSE(parameters.encodings[2].scalability_mode.has_value()); } +TEST_F(PeerConnectionEncodingsIntegrationTest, + VP9_SimulcastDeactiveActiveLayer_StandardSvc) { + rtc::scoped_refptr local_pc_wrapper = CreatePc(); + rtc::scoped_refptr remote_pc_wrapper = CreatePc(); + ExchangeIceCandidates(local_pc_wrapper, remote_pc_wrapper); + + std::vector layers = + CreateLayers({"q", "h", "f"}, /*active=*/true); + rtc::scoped_refptr transceiver = + AddTransceiverWithSimulcastLayers(local_pc_wrapper, remote_pc_wrapper, + layers); + constexpr absl::string_view kCodec = "VP9"; + std::vector codecs = + GetCapabilitiesAndRestrictToCodec(remote_pc_wrapper, kCodec); + transceiver->SetCodecPreferences(codecs); + + // Switch to the standard mode. Despite only having a single active stream in + // both cases, this internally reconfigures from 1 stream to 3 streams. + // Test coverage for https://crbug.com/webrtc/15016. + rtc::scoped_refptr sender = transceiver->sender(); + RtpParameters parameters = sender->GetParameters(); + ASSERT_THAT(parameters.encodings, SizeIs(3)); + parameters.encodings[0].active = true; + parameters.encodings[0].scalability_mode = "L1T3"; + parameters.encodings[0].scale_resolution_down_by = 4.0; + parameters.encodings[1].active = true; + parameters.encodings[1].scalability_mode = "L1T1"; + parameters.encodings[1].scale_resolution_down_by = 2.0; + parameters.encodings[2].active = true; + parameters.encodings[2].scalability_mode = "L1T1"; + parameters.encodings[2].scale_resolution_down_by = 1.0; + EXPECT_TRUE(sender->SetParameters(parameters).ok()); + + // The original negotiation triggers legacy SVC because we didn't specify + // any scalability mode. + NegotiateWithSimulcastTweaks(local_pc_wrapper, remote_pc_wrapper); + local_pc_wrapper->WaitForConnection(); + remote_pc_wrapper->WaitForConnection(); + + // Since the standard API is configuring simulcast we get three outbound-rtps, + // and two are active. + ASSERT_TRUE_WAIT(HasOutboundRtpBytesSent(local_pc_wrapper, /*num_layers=*/3u, + /*num_active_layers=*/3u), + kDefaultTimeout.ms()); + // Wait until scalability mode is reported and expected resolution reached. + // Ramp up time may be significant. + EXPECT_TRUE_WAIT(OutboundRtpResolutionsAreLessThanOrEqualToExpectations( + local_pc_wrapper, + {{"q", 320, 180}, {"h", 640, 360}, {"f", 1280, 720}}), + kLongTimeoutForRampingUp.ms() / 2); + rtc::scoped_refptr report = GetStats(local_pc_wrapper); + ASSERT_TRUE(report); + std::vector outbound_rtps = + report->GetStatsOfType(); + EXPECT_THAT(outbound_rtps, + Each(HasEncoderImplementation( + "SimulcastEncoderAdapter (libvpx, libvpx, libvpx)"))); + + // GetParameters() does not report any fallback. + parameters = sender->GetParameters(); + ASSERT_THAT(parameters.encodings, SizeIs(3)); + EXPECT_THAT(parameters.encodings[0].scalability_mode, + Optional(StrEq("L1T3"))); + EXPECT_THAT(parameters.encodings[1].scalability_mode, + Optional(StrEq("L1T1"))); + EXPECT_THAT(parameters.encodings[2].scalability_mode, + Optional(StrEq("L1T1"))); + EXPECT_THAT(parameters.encodings[2].scale_resolution_down_by, Eq(1.0)); + EXPECT_THAT(parameters.encodings[1].scale_resolution_down_by, Eq(2.0)); + EXPECT_THAT(parameters.encodings[0].scale_resolution_down_by, Eq(4.0)); + + // Deactivate the active layer. + parameters.encodings[2].active = false; + EXPECT_TRUE(sender->SetParameters(parameters).ok()); + // Ensure that we are getting VGA at L1T1 from the "f" rid. + ASSERT_TRUE_WAIT(!EncodingIsActive(local_pc_wrapper, "f"), + kDefaultTimeout.ms()); + + EXPECT_EQ_WAIT(GetEncoderImplementationName(local_pc_wrapper), + "SimulcastEncoderAdapter (libvpx, libvpx)", + kDefaultTimeout.ms()); + + report = GetStats(local_pc_wrapper); + ASSERT_TRUE(report); + outbound_rtps = report->GetStatsOfType(); + EXPECT_THAT(outbound_rtps, Each(HasEncoderImplementation( + "SimulcastEncoderAdapter (libvpx, libvpx)"))); + EXPECT_THAT(outbound_rtps, UnorderedElementsAre(HasScalabilityMode("L1T3"), + HasScalabilityMode("L1T1"), + HasEmptyScalabilityMode())); +} + TEST_F(PeerConnectionEncodingsIntegrationTest, VP9_SimulcastMultiplLayersActive_StandardSvc) { rtc::scoped_refptr local_pc_wrapper = CreatePc(); @@ -935,6 +1061,12 @@ TEST_F(PeerConnectionEncodingsIntegrationTest, ASSERT_TRUE_WAIT(HasOutboundRtpWithRidAndScalabilityMode( local_pc_wrapper, "h", "L1T1", 720 / 2), kLongTimeoutForRampingUp.ms() / 2); + rtc::scoped_refptr report = GetStats(local_pc_wrapper); + ASSERT_TRUE(report); + std::vector outbound_rtps = + report->GetStatsOfType(); + EXPECT_THAT(outbound_rtps, Each(HasEncoderImplementation( + "SimulcastEncoderAdapter (libvpx, libvpx)"))); // GetParameters() does not report any fallback. parameters = sender->GetParameters();