From 3df1d5d2fbe0923b0804ca93cd92ca5d32b0cf62 Mon Sep 17 00:00:00 2001 From: Ilya Nikolaevskiy Date: Wed, 22 Aug 2018 09:26:51 +0200 Subject: [PATCH] Revert removal of simulcast screenshare experimental code (killswitch checks) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit a3df0f2d05c7b0973c31fe171507e97e588671a5. Reason for revert: We decided to keep a killswitch in M70 just in case. Original reviewed at: https://webrtc-review.googlesource.com/c/src/+/90251 Bug: chromium:690537 Change-Id: Ieb0eb8d5487e03fc55a221f10366ed9768a6eb16 Reviewed-on: https://webrtc-review.googlesource.com/95061 Commit-Queue: Erik Språng Reviewed-by: Stefan Holmer Reviewed-by: Ilya Nikolaevskiy Reviewed-by: Erik Språng Cr-Commit-Position: refs/heads/master@{#24385} --- media/base/videocapturer.cc | 7 ++++++- media/engine/simulcast.cc | 17 ++++++++++++---- media/engine/simulcast.h | 4 ++++ media/engine/simulcast_unittest.cc | 23 ++++++++++++++++++++++ media/engine/webrtcvideoengine.cc | 8 ++++++-- media/engine/webrtcvideoengine_unittest.cc | 19 ++++++++++++++++++ video/full_stack_tests.cc | 13 ++++++++++++ 7 files changed, 84 insertions(+), 7 deletions(-) diff --git a/media/base/videocapturer.cc b/media/base/videocapturer.cc index f8420dc1ad..643134c546 100644 --- a/media/base/videocapturer.cc +++ b/media/base/videocapturer.cc @@ -27,6 +27,8 @@ static const int64_t kMaxDistance = ~(static_cast(1) << 63); #ifdef WEBRTC_LINUX static const int kYU12Penalty = 16; // Needs to be higher than MJPG index. #endif +static const char* kSimulcastScreenshareFieldTrialName = + "WebRTC-SimulcastScreenshare"; } // namespace @@ -182,7 +184,10 @@ bool VideoCapturer::AdaptFrame(int width, return false; } - if (enable_video_adapter_) { + bool simulcast_screenshare_enabled = + !webrtc::field_trial::IsDisabled(kSimulcastScreenshareFieldTrialName); + if (enable_video_adapter_ && + (!IsScreencast() || simulcast_screenshare_enabled)) { if (!video_adapter_.AdaptFrameResolution( width, height, camera_time_us * rtc::kNumNanosecsPerMicrosec, crop_width, crop_height, out_width, out_height)) { diff --git a/media/engine/simulcast.cc b/media/engine/simulcast.cc index a2802a4998..9c4b233ec4 100644 --- a/media/engine/simulcast.cc +++ b/media/engine/simulcast.cc @@ -32,6 +32,8 @@ constexpr int kScreenshareDefaultTl1BitrateKbps = 1000; // Max bitrate for the higher one of the two simulcast stream used for screen // content. constexpr int kScreenshareHighStreamMaxBitrateBps = 1250000; +static const char* kSimulcastScreenshareFieldTrialName = + "WebRTC-SimulcastScreenshare"; } // namespace @@ -196,9 +198,9 @@ std::vector GetSimulcastConfig( bool is_screenshare, bool temporal_layers_supported) { if (is_screenshare) { - return GetScreenshareLayers(max_layers, width, height, bitrate_priority, - max_qp, max_framerate, - temporal_layers_supported); + return GetScreenshareLayers( + max_layers, width, height, bitrate_priority, max_qp, max_framerate, + ScreenshareSimulcastFieldTrialEnabled(), temporal_layers_supported); } else { return GetNormalSimulcastLayers(max_layers, width, height, bitrate_priority, max_qp, max_framerate, @@ -297,9 +299,12 @@ std::vector GetScreenshareLayers( double bitrate_priority, int max_qp, int max_framerate, + bool screenshare_simulcast_enabled, bool temporal_layers_supported) { + auto max_screenshare_layers = + screenshare_simulcast_enabled ? kMaxScreenshareSimulcastLayers : 1; size_t num_simulcast_layers = - std::min(max_layers, kMaxScreenshareSimulcastLayers); + std::min(max_layers, max_screenshare_layers); std::vector layers(num_simulcast_layers); // For legacy screenshare in conference mode, tl0 and tl1 bitrates are @@ -362,4 +367,8 @@ std::vector GetScreenshareLayers( return layers; } +bool ScreenshareSimulcastFieldTrialEnabled() { + return !webrtc::field_trial::IsDisabled(kSimulcastScreenshareFieldTrialName); +} + } // namespace cricket diff --git a/media/engine/simulcast.h b/media/engine/simulcast.h index 84583bca36..7a396a45fa 100644 --- a/media/engine/simulcast.h +++ b/media/engine/simulcast.h @@ -56,7 +56,11 @@ std::vector GetScreenshareLayers( double bitrate_priority, int max_qp, int max_framerate, + bool screenshare_simulcast_enabled, bool temporal_layers_supported = true); + +bool ScreenshareSimulcastFieldTrialEnabled(); + } // namespace cricket #endif // MEDIA_ENGINE_SIMULCAST_H_ diff --git a/media/engine/simulcast_unittest.cc b/media/engine/simulcast_unittest.cc index 29226d1211..f3fa854c82 100644 --- a/media/engine/simulcast_unittest.cc +++ b/media/engine/simulcast_unittest.cc @@ -134,6 +134,27 @@ TEST(SimulcastTest, GetConfigWithNormalizedResolution) { EXPECT_EQ(360u, streams[1].height); } +TEST(SimulcastTest, GetConfigForScreenshare) { + test::ScopedFieldTrials field_trials("WebRTC-SimulcastScreenshare/Disabled/"); + + const size_t kMaxLayers = 3; + std::vector streams = cricket::GetSimulcastConfig( + kMaxLayers, 1400, 800, kMaxBitrateBps, kBitratePriority, kQpMax, kMaxFps, + kScreenshare); + + EXPECT_EQ(1u, streams.size()) << "No simulcast."; + EXPECT_EQ(1400u, streams[0].width); + EXPECT_EQ(800u, streams[0].height); + EXPECT_EQ(kQpMax, streams[0].max_qp); + EXPECT_EQ(kBitratePriority, streams[0].bitrate_priority); + EXPECT_TRUE(streams[0].active); + EXPECT_GT(streams[0].num_temporal_layers, size_t{1}); + EXPECT_GT(streams[0].max_framerate, 0); + EXPECT_EQ(cricket::kMinVideoBitrateBps, streams[0].min_bitrate_bps); + EXPECT_GT(streams[0].target_bitrate_bps, streams[0].min_bitrate_bps); + EXPECT_GT(streams[0].max_bitrate_bps, streams[0].target_bitrate_bps); +} + TEST(SimulcastTest, GetConfigForScreenshareSimulcast) { const size_t kMaxLayers = 3; std::vector streams = cricket::GetSimulcastConfig( @@ -155,6 +176,8 @@ TEST(SimulcastTest, GetConfigForScreenshareSimulcast) { } TEST(SimulcastTest, GetConfigForScreenshareSimulcastWithLimitedMaxLayers) { + test::ScopedFieldTrials field_trials("WebRTC-SimulcastScreenshare/Enabled/"); + const size_t kMaxLayers = 1; std::vector streams = cricket::GetSimulcastConfig( kMaxLayers, 1400, 800, kMaxBitrateBps, kBitratePriority, kQpMax, kMaxFps, diff --git a/media/engine/webrtcvideoengine.cc b/media/engine/webrtcvideoengine.cc index e518682bb6..e7da4220c8 100644 --- a/media/engine/webrtcvideoengine.cc +++ b/media/engine/webrtcvideoengine.cc @@ -1957,7 +1957,8 @@ WebRtcVideoChannel::WebRtcVideoSendStream::CreateVideoEncoderConfig( // configure a single stream. encoder_config.number_of_streams = parameters_.config.rtp.ssrcs.size(); if (IsCodecBlacklistedForSimulcast(codec.name) || - (is_screencast && !parameters_.conference_mode)) { + (is_screencast && (!ScreenshareSimulcastFieldTrialEnabled() || + !parameters_.conference_mode))) { encoder_config.number_of_streams = 1; } @@ -2736,7 +2737,10 @@ std::vector EncoderStreamFactory::CreateEncoderStreams( int width, int height, const webrtc::VideoEncoderConfig& encoder_config) { - if (is_screenshare_ && !screenshare_config_explicitly_enabled_) { + bool screenshare_simulcast_enabled = + screenshare_config_explicitly_enabled_ && + cricket::ScreenshareSimulcastFieldTrialEnabled(); + if (is_screenshare_ && !screenshare_simulcast_enabled) { RTC_DCHECK_EQ(1, encoder_config.number_of_streams); } RTC_DCHECK_GT(encoder_config.number_of_streams, 0); diff --git a/media/engine/webrtcvideoengine_unittest.cc b/media/engine/webrtcvideoengine_unittest.cc index 3192866a93..41b68c98d2 100644 --- a/media/engine/webrtcvideoengine_unittest.cc +++ b/media/engine/webrtcvideoengine_unittest.cc @@ -6521,6 +6521,25 @@ TEST_F(WebRtcVideoChannelSimulcastTest, SetSendCodecsWithOddSizeInSimulcast) { true); } +// TODO(ilnik): Remove this test once Simulcast Screenshare is launched. +TEST_F(WebRtcVideoChannelSimulcastTest, SetSendCodecsForScreenshare) { + webrtc::test::ScopedFieldTrials override_field_trials_( + "WebRTC-SimulcastScreenshare/Disabled/"); + + VerifySimulcastSettings(cricket::VideoCodec("VP8"), 1280, 720, 3, 1, true, + false); +} + +// TODO(ilnik): Remove this test once Simulcast Screenshare is launched. +TEST_F(WebRtcVideoChannelSimulcastTest, + SetSendCodecsForConferenceModeScreenshare) { + webrtc::test::ScopedFieldTrials override_field_trials_( + "WebRTC-SimulcastScreenshare/Disabled/"); + + VerifySimulcastSettings(cricket::VideoCodec("VP8"), 1280, 720, 3, 1, true, + true); +} + TEST_F(WebRtcVideoChannelSimulcastTest, SetSendCodecsForSimulcastScreenshare) { VerifySimulcastSettings(cricket::VideoCodec("VP8"), 1280, 720, 3, 2, true, true); diff --git a/video/full_stack_tests.cc b/video/full_stack_tests.cc index 8135697728..9c320d795f 100644 --- a/video/full_stack_tests.cc +++ b/video/full_stack_tests.cc @@ -46,6 +46,8 @@ namespace webrtc { namespace { static const int kFullStackTestDurationSecs = 45; +const char kNotScreenshareSimulcastExperiment[] = + "WebRTC-SimulcastScreenshare/Disabled/"; const char kRoundRobinPacingQueueExperiment[] = "WebRTC-RoundRobinPacing/Enabled/"; const char kPacerPushBackExperiment[] = @@ -596,6 +598,7 @@ TEST(FullStackTest, ConferenceMotionHd2000kbps100msLimitedQueueVP9) { #endif TEST(FullStackTest, ScreenshareSlidesVP8_2TL) { + test::ScopedFieldTrials field_trial(kNotScreenshareSimulcastExperiment); auto fixture = CreateVideoQualityTestFixture(); ParamsWithLogging screenshare; screenshare.call.send_side_bwe = true; @@ -637,6 +640,8 @@ TEST(FullStackTest, ScreenshareSlidesVP8_3TL_Simulcast) { } TEST(FullStackTest, ScreenshareSlidesVP8_2TL_Scroll) { + test::ScopedFieldTrials field_trial(kNotScreenshareSimulcastExperiment); + auto fixture = CreateVideoQualityTestFixture(); ParamsWithLogging config; config.call.send_side_bwe = true; @@ -650,6 +655,8 @@ TEST(FullStackTest, ScreenshareSlidesVP8_2TL_Scroll) { } TEST(FullStackTest, ScreenshareSlidesVP8_2TL_LossyNet) { + test::ScopedFieldTrials field_trial(kNotScreenshareSimulcastExperiment); + auto fixture = CreateVideoQualityTestFixture(); ParamsWithLogging screenshare; screenshare.call.send_side_bwe = true; @@ -666,6 +673,8 @@ TEST(FullStackTest, ScreenshareSlidesVP8_2TL_LossyNet) { } TEST(FullStackTest, ScreenshareSlidesVP8_2TL_VeryLossyNet) { + test::ScopedFieldTrials field_trial(kNotScreenshareSimulcastExperiment); + auto fixture = CreateVideoQualityTestFixture(); ParamsWithLogging screenshare; screenshare.call.send_side_bwe = true; @@ -682,6 +691,8 @@ TEST(FullStackTest, ScreenshareSlidesVP8_2TL_VeryLossyNet) { } TEST(FullStackTest, ScreenshareSlidesVP8_2TL_LossyNetRestrictedQueue) { + test::ScopedFieldTrials field_trial(kNotScreenshareSimulcastExperiment); + auto fixture = CreateVideoQualityTestFixture(); ParamsWithLogging screenshare; screenshare.call.send_side_bwe = true; @@ -699,6 +710,8 @@ TEST(FullStackTest, ScreenshareSlidesVP8_2TL_LossyNetRestrictedQueue) { } TEST(FullStackTest, ScreenshareSlidesVP8_2TL_ModeratelyRestricted) { + test::ScopedFieldTrials field_trial(kNotScreenshareSimulcastExperiment); + auto fixture = CreateVideoQualityTestFixture(); ParamsWithLogging screenshare; screenshare.call.send_side_bwe = true;