From 31c5c9da35209fe65ed15cb3a804823cd2789259 Mon Sep 17 00:00:00 2001 From: Ilya Nikolaevskiy Date: Thu, 11 Mar 2021 15:13:57 +0000 Subject: [PATCH] Revert "Reland "Enable quality scaling when allowed"" This reverts commit 0021fe77937f386e6021a5451e3b0d78d7950815. Reason for revert: Broken on linux_tsan bot: https://ci.chromium.org/ui/p/webrtc/builders/ci/Linux%20Tsan%20v2/25329/overview Original change's description: > Reland "Enable quality scaling when allowed" > > This reverts commit eb449a979bc561a8b256cca434e582f3889375e2. > > Reason for revert: Added QP parsing in https://webrtc.googlesource.com/src/+/8639673f0c098efc294a7593fa3bd98e28ab7508 > > Original change's description: > Before this CL quality scaling was conditioned on scaling settings > provided by encoder. That should not be a requirement since encoder > may not be aware of quality scaling which is a WebRTC feature. In M90 > chromium HW encoders do not provide scaling settings (chromium:1179020). > The default scaling settings provided by these encoders are not correct > (b/181537172). > > This CL adds is_quality_scaling_allowed to VideoEncoderConfig. The flag > is set to true in singlecast with normal video feed (not screen sharing) > mode. If quality scaling is allowed it is enabled no matter whether > scaling settings are present in encoder info or not. Setting from > QualityScalingExperiment are used in case if not provided by encoder. > > Bug: chromium:1179020 > Bug: webrtc:12511 > Change-Id: I97911fde9005ec25028a640a3f007d12f2bbc2e5 > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/211349 > Reviewed-by: Rasmus Brandt > Reviewed-by: Ilya Nikolaevskiy > Commit-Queue: Sergey Silkin > Cr-Commit-Position: refs/heads/master@{#33438} TBR=brandtr@webrtc.org,ilnik@webrtc.org,ssilkin@webrtc.org,rubber-stamper@appspot.gserviceaccount.com Change-Id: Id7633a1e98f95762e81487887f83a0c35f89195c No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: chromium:1179020 Bug: webrtc:12511 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/211352 Reviewed-by: Ilya Nikolaevskiy Commit-Queue: Ilya Nikolaevskiy Cr-Commit-Position: refs/heads/master@{#33439} --- api/video_codecs/video_encoder_config.cc | 3 +- api/video_codecs/video_encoder_config.h | 3 - media/engine/webrtc_video_engine.cc | 6 -- media/engine/webrtc_video_engine_unittest.cc | 42 ------------- .../video_stream_encoder_resource_manager.cc | 10 ++- video/video_stream_encoder_unittest.cc | 61 ------------------- 6 files changed, 5 insertions(+), 120 deletions(-) diff --git a/api/video_codecs/video_encoder_config.cc b/api/video_codecs/video_encoder_config.cc index 0321da24da..5956d60365 100644 --- a/api/video_codecs/video_encoder_config.cc +++ b/api/video_codecs/video_encoder_config.cc @@ -57,8 +57,7 @@ VideoEncoderConfig::VideoEncoderConfig() max_bitrate_bps(0), bitrate_priority(1.0), number_of_streams(0), - legacy_conference_mode(false), - is_quality_scaling_allowed(false) {} + legacy_conference_mode(false) {} VideoEncoderConfig::VideoEncoderConfig(VideoEncoderConfig&&) = default; diff --git a/api/video_codecs/video_encoder_config.h b/api/video_codecs/video_encoder_config.h index 59163743a2..1a061f52f7 100644 --- a/api/video_codecs/video_encoder_config.h +++ b/api/video_codecs/video_encoder_config.h @@ -181,9 +181,6 @@ class VideoEncoderConfig { // Legacy Google conference mode flag for simulcast screenshare bool legacy_conference_mode; - // Indicates whether quality scaling can be used or not. - bool is_quality_scaling_allowed; - private: // Access to the copy constructor is private to force use of the Copy() // method for those exceptional cases where we do use it. diff --git a/media/engine/webrtc_video_engine.cc b/media/engine/webrtc_video_engine.cc index 8fbd5ec148..5808e6fbb2 100644 --- a/media/engine/webrtc_video_engine.cc +++ b/media/engine/webrtc_video_engine.cc @@ -2492,17 +2492,11 @@ WebRtcVideoChannel::WebRtcVideoSendStream::CreateVideoEncoderConfig( encoder_config.legacy_conference_mode = parameters_.conference_mode; - encoder_config.is_quality_scaling_allowed = - !disable_automatic_resize_ && !is_screencast && - (parameters_.config.rtp.ssrcs.size() == 1 || - NumActiveStreams(rtp_parameters_) == 1); - int max_qp = kDefaultQpMax; codec.GetParam(kCodecParamMaxQuantization, &max_qp); encoder_config.video_stream_factory = new rtc::RefCountedObject( codec.name, max_qp, is_screencast, parameters_.conference_mode); - return encoder_config; } diff --git a/media/engine/webrtc_video_engine_unittest.cc b/media/engine/webrtc_video_engine_unittest.cc index cf0349045e..1f7a0eee62 100644 --- a/media/engine/webrtc_video_engine_unittest.cc +++ b/media/engine/webrtc_video_engine_unittest.cc @@ -8302,48 +8302,6 @@ TEST_F(WebRtcVideoChannelTest, ConfiguresLocalSsrcOnExistingReceivers) { TestReceiverLocalSsrcConfiguration(true); } -TEST_F(WebRtcVideoChannelTest, Simulcast_QualityScalingNotAllowed) { - FakeVideoSendStream* stream = SetUpSimulcast(true, true); - EXPECT_FALSE(stream->GetEncoderConfig().is_quality_scaling_allowed); -} - -TEST_F(WebRtcVideoChannelTest, Singlecast_QualityScalingAllowed) { - FakeVideoSendStream* stream = SetUpSimulcast(false, true); - EXPECT_TRUE(stream->GetEncoderConfig().is_quality_scaling_allowed); -} - -TEST_F(WebRtcVideoChannelTest, - SinglecastScreenSharing_QualityScalingNotAllowed) { - SetUpSimulcast(false, true); - - webrtc::test::FrameForwarder frame_forwarder; - VideoOptions options; - options.is_screencast = true; - EXPECT_TRUE(channel_->SetVideoSend(last_ssrc_, &options, &frame_forwarder)); - // Fetch the latest stream since SetVideoSend() may recreate it if the - // screen content setting is changed. - FakeVideoSendStream* stream = fake_call_->GetVideoSendStreams().front(); - - EXPECT_FALSE(stream->GetEncoderConfig().is_quality_scaling_allowed); - EXPECT_TRUE(channel_->SetVideoSend(last_ssrc_, nullptr, nullptr)); -} - -TEST_F(WebRtcVideoChannelTest, - SimulcastSingleActiveStream_QualityScalingAllowed) { - FakeVideoSendStream* stream = SetUpSimulcast(true, false); - - webrtc::RtpParameters rtp_parameters = - channel_->GetRtpSendParameters(last_ssrc_); - ASSERT_EQ(3u, rtp_parameters.encodings.size()); - ASSERT_TRUE(rtp_parameters.encodings[0].active); - ASSERT_TRUE(rtp_parameters.encodings[1].active); - ASSERT_TRUE(rtp_parameters.encodings[2].active); - rtp_parameters.encodings[0].active = false; - rtp_parameters.encodings[1].active = false; - EXPECT_TRUE(channel_->SetRtpSendParameters(last_ssrc_, rtp_parameters).ok()); - EXPECT_TRUE(stream->GetEncoderConfig().is_quality_scaling_allowed); -} - class WebRtcVideoChannelSimulcastTest : public ::testing::Test { public: WebRtcVideoChannelSimulcastTest() diff --git a/video/adaptation/video_stream_encoder_resource_manager.cc b/video/adaptation/video_stream_encoder_resource_manager.cc index 1c2e5839f2..3da534f753 100644 --- a/video/adaptation/video_stream_encoder_resource_manager.cc +++ b/video/adaptation/video_stream_encoder_resource_manager.cc @@ -521,9 +521,7 @@ void VideoStreamEncoderResourceManager::ConfigureQualityScaler( const auto scaling_settings = encoder_info.scaling_settings; const bool quality_scaling_allowed = IsResolutionScalingEnabled(degradation_preference_) && - (scaling_settings.thresholds.has_value() || - (encoder_settings_.has_value() && - encoder_settings_->encoder_config().is_quality_scaling_allowed)); + scaling_settings.thresholds; // TODO(https://crbug.com/webrtc/11222): Should this move to // QualityScalerResource? @@ -537,9 +535,9 @@ void VideoStreamEncoderResourceManager::ConfigureQualityScaler( experimental_thresholds = QualityScalingExperiment::GetQpThresholds( GetVideoCodecTypeOrGeneric(encoder_settings_)); } - UpdateQualityScalerSettings(experimental_thresholds.has_value() - ? experimental_thresholds - : scaling_settings.thresholds); + UpdateQualityScalerSettings(experimental_thresholds + ? *experimental_thresholds + : *(scaling_settings.thresholds)); } } else { UpdateQualityScalerSettings(absl::nullopt); diff --git a/video/video_stream_encoder_unittest.cc b/video/video_stream_encoder_unittest.cc index 1cba0acc79..98e63a7da2 100644 --- a/video/video_stream_encoder_unittest.cc +++ b/video/video_stream_encoder_unittest.cc @@ -7992,65 +7992,4 @@ TEST_F(VideoStreamEncoderTest, QpAbsentParsingDisabled_QpAbsent) { video_stream_encoder_->Stop(); } -TEST_F(VideoStreamEncoderTest, - QualityScalingNotAllowed_QualityScalingDisabled) { - VideoEncoderConfig video_encoder_config = video_encoder_config_.Copy(); - - // Disable scaling settings in encoder info. - fake_encoder_.SetQualityScaling(false); - // Disable quality scaling in encoder config. - video_encoder_config.is_quality_scaling_allowed = false; - ConfigureEncoder(std::move(video_encoder_config)); - - video_stream_encoder_->OnBitrateUpdatedAndWaitForManagedResources( - DataRate::BitsPerSec(kTargetBitrateBps), - DataRate::BitsPerSec(kTargetBitrateBps), - DataRate::BitsPerSec(kTargetBitrateBps), 0, 0, 0); - - test::FrameForwarder source; - video_stream_encoder_->SetSource( - &source, webrtc::DegradationPreference::MAINTAIN_FRAMERATE); - EXPECT_THAT(source.sink_wants(), UnlimitedSinkWants()); - EXPECT_FALSE(stats_proxy_->GetStats().bw_limited_resolution); - - source.IncomingCapturedFrame(CreateFrame(1, 1280, 720)); - WaitForEncodedFrame(1); - video_stream_encoder_->TriggerQualityLow(); - EXPECT_FALSE(stats_proxy_->GetStats().bw_limited_resolution); - - video_stream_encoder_->Stop(); -} - -#if !defined(WEBRTC_IOS) -// TODO(bugs.webrtc.org/12401): Disabled because WebRTC-Video-QualityScaling is -// disabled by default on iOS. -TEST_F(VideoStreamEncoderTest, QualityScalingAllowed_QualityScalingEnabled) { - VideoEncoderConfig video_encoder_config = video_encoder_config_.Copy(); - - // Disable scaling settings in encoder info. - fake_encoder_.SetQualityScaling(false); - // Enable quality scaling in encoder config. - video_encoder_config.is_quality_scaling_allowed = true; - ConfigureEncoder(std::move(video_encoder_config)); - - video_stream_encoder_->OnBitrateUpdatedAndWaitForManagedResources( - DataRate::BitsPerSec(kTargetBitrateBps), - DataRate::BitsPerSec(kTargetBitrateBps), - DataRate::BitsPerSec(kTargetBitrateBps), 0, 0, 0); - - test::FrameForwarder source; - video_stream_encoder_->SetSource( - &source, webrtc::DegradationPreference::MAINTAIN_FRAMERATE); - EXPECT_THAT(source.sink_wants(), UnlimitedSinkWants()); - EXPECT_FALSE(stats_proxy_->GetStats().bw_limited_resolution); - - source.IncomingCapturedFrame(CreateFrame(1, 1280, 720)); - WaitForEncodedFrame(1); - video_stream_encoder_->TriggerQualityLow(); - EXPECT_TRUE(stats_proxy_->GetStats().bw_limited_resolution); - - video_stream_encoder_->Stop(); -} -#endif - } // namespace webrtc