From dda5fdcb82e0aab7b9df70fbb7d349e5a230290d Mon Sep 17 00:00:00 2001 From: Ilya Nikolaevskiy Date: Wed, 27 Feb 2019 10:00:06 +0100 Subject: [PATCH] Fix vp8 simulcast screenshare and perf tests for it Simulcast screenshare appears broken due to unrelated changes. It implicitly relied on SimulcastEncoderAdapter fallback, which happened before if streams had same resolution. It's not the case anymore. Thus, this CL adds checks for different frame-rate in simulcast streams. FullStackTests are also updated to use actual parameters. Bug: none Change-Id: I2c1ddb1b39edb96464a0915dfcb9cb4e18844187 Reviewed-on: https://webrtc-review.googlesource.com/c/124494 Reviewed-by: Mirta Dvornicic Commit-Queue: Ilya Nikolaevskiy Cr-Commit-Position: refs/heads/master@{#26869} --- .../codecs/h264/h264_encoder_impl.cc | 6 ++--- .../codecs/vp8/libvpx_vp8_encoder.cc | 4 +-- .../video_coding/utility/simulcast_utility.cc | 20 +++++++++----- .../video_coding/utility/simulcast_utility.h | 6 ++--- video/full_stack_tests.cc | 26 +++++++++---------- 5 files changed, 32 insertions(+), 30 deletions(-) diff --git a/modules/video_coding/codecs/h264/h264_encoder_impl.cc b/modules/video_coding/codecs/h264/h264_encoder_impl.cc index 5987ecbe6f..09a5d02b93 100644 --- a/modules/video_coding/codecs/h264/h264_encoder_impl.cc +++ b/modules/video_coding/codecs/h264/h264_encoder_impl.cc @@ -211,10 +211,8 @@ int32_t H264EncoderImpl::InitEncode(const VideoCodec* inst, int number_of_streams = SimulcastUtility::NumberOfSimulcastStreams(*inst); bool doing_simulcast = (number_of_streams > 1); - if (doing_simulcast && (!SimulcastUtility::ValidSimulcastResolutions( - *inst, number_of_streams) || - !SimulcastUtility::ValidSimulcastTemporalLayers( - *inst, number_of_streams))) { + if (doing_simulcast && + !SimulcastUtility::ValidSimulcastParameters(*inst, number_of_streams)) { return WEBRTC_VIDEO_CODEC_ERR_SIMULCAST_PARAMETERS_NOT_SUPPORTED; } downscaled_buffers_.resize(number_of_streams - 1); diff --git a/modules/video_coding/codecs/vp8/libvpx_vp8_encoder.cc b/modules/video_coding/codecs/vp8/libvpx_vp8_encoder.cc index ea1b994ce5..1dc0837e3d 100644 --- a/modules/video_coding/codecs/vp8/libvpx_vp8_encoder.cc +++ b/modules/video_coding/codecs/vp8/libvpx_vp8_encoder.cc @@ -338,9 +338,7 @@ int LibvpxVp8Encoder::InitEncode(const VideoCodec* inst, int number_of_streams = SimulcastUtility::NumberOfSimulcastStreams(*inst); if (number_of_streams > 1 && - (!SimulcastUtility::ValidSimulcastResolutions(*inst, number_of_streams) || - !SimulcastUtility::ValidSimulcastTemporalLayers(*inst, - number_of_streams))) { + !SimulcastUtility::ValidSimulcastParameters(*inst, number_of_streams)) { return WEBRTC_VIDEO_CODEC_ERR_SIMULCAST_PARAMETERS_NOT_SUPPORTED; } diff --git a/modules/video_coding/utility/simulcast_utility.cc b/modules/video_coding/utility/simulcast_utility.cc index b517455cca..37e736f11e 100644 --- a/modules/video_coding/utility/simulcast_utility.cc +++ b/modules/video_coding/utility/simulcast_utility.cc @@ -10,6 +10,8 @@ #include +#include + #include "common_types.h" // NOLINT(build/include) #include "modules/video_coding/utility/simulcast_utility.h" #include "rtc_base/checks.h" @@ -35,8 +37,9 @@ int SimulcastUtility::NumberOfSimulcastStreams(const VideoCodec& codec) { return streams; } -bool SimulcastUtility::ValidSimulcastResolutions(const VideoCodec& codec, - int num_streams) { +bool SimulcastUtility::ValidSimulcastParameters(const VideoCodec& codec, + int num_streams) { + // Check resolution. if (codec.width != codec.simulcastStream[num_streams - 1].width || codec.height != codec.simulcastStream[num_streams - 1].height) { return false; @@ -63,11 +66,16 @@ bool SimulcastUtility::ValidSimulcastResolutions(const VideoCodec& codec, } } } - return true; -} -bool SimulcastUtility::ValidSimulcastTemporalLayers(const VideoCodec& codec, - int num_streams) { + // Check frame-rate. + for (int i = 1; i < num_streams; ++i) { + if (fabs(codec.simulcastStream[i].maxFramerate - + codec.simulcastStream[i - 1].maxFramerate) > 1e-9) { + return false; + } + } + + // Check temporal layers. for (int i = 0; i < num_streams - 1; ++i) { if (codec.simulcastStream[i].numberOfTemporalLayers != codec.simulcastStream[i + 1].numberOfTemporalLayers) diff --git a/modules/video_coding/utility/simulcast_utility.h b/modules/video_coding/utility/simulcast_utility.h index ab632fdd41..e25a594360 100644 --- a/modules/video_coding/utility/simulcast_utility.h +++ b/modules/video_coding/utility/simulcast_utility.h @@ -21,10 +21,8 @@ class SimulcastUtility { public: static uint32_t SumStreamMaxBitrate(int streams, const VideoCodec& codec); static int NumberOfSimulcastStreams(const VideoCodec& codec); - static bool ValidSimulcastResolutions(const VideoCodec& codec, - int num_streams); - static bool ValidSimulcastTemporalLayers(const VideoCodec& codec, - int num_streams); + static bool ValidSimulcastParameters(const VideoCodec& codec, + int num_streams); static int NumberOfTemporalLayers(const VideoCodec& codec, int spatial_id); // TODO(sprang): Remove this hack when ScreenshareLayers is gone. static bool IsConferenceModeScreenshare(const VideoCodec& codec); diff --git a/video/full_stack_tests.cc b/video/full_stack_tests.cc index c1848a76d6..87f82cc3c8 100644 --- a/video/full_stack_tests.cc +++ b/video/full_stack_tests.cc @@ -724,24 +724,24 @@ const char kScreenshareSimulcastExperiment[] = // TODO(bugs.webrtc.org/9840): Investigate why is this test flaky on Win/Mac. #if !defined(WEBRTC_WIN) -TEST(FullStackTest, ScreenshareSlidesVP8_3TL_Simulcast) { +TEST(FullStackTest, ScreenshareSlidesVP8_2TL_Simulcast) { test::ScopedFieldTrials field_trial( AppendFieldTrials(kScreenshareSimulcastExperiment)); auto fixture = CreateVideoQualityTestFixture(); ParamsWithLogging screenshare; screenshare.call.send_side_bwe = true; screenshare.screenshare[0] = {true, false, 10}; - screenshare.video[0] = {true, 1850, 1110, 5, 800000, - 2500000, 2500000, false, "VP8", 3, - 2, 400000, false, false, false, ""}; + screenshare.video[0] = {true, 1850, 1110, 30, 800000, 2500000, + 2500000, false, "VP8", 3, 2, 400000, + false, false, false, ""}; screenshare.analyzer = {"screenshare_slides_simulcast", 0.0, 0.0, kFullStackTestDurationSecs}; ParamsWithLogging screenshare_params_high; - screenshare_params_high.video[0] = {true, 1850, 1110, 5, 400000, 1000000, - 1000000, false, "VP8", 3, 0, 400000, - false, false, false, ""}; + screenshare_params_high.video[0] = { + true, 1850, 1110, 60, 600000, 1250000, 1250000, false, + "VP8", 2, 0, 400000, false, false, false, ""}; VideoQualityTest::Params screenshare_params_low; - screenshare_params_low.video[0] = {true, 1850, 1110, 5, 50000, 200000, + screenshare_params_low.video[0] = {true, 1850, 1110, 5, 30000, 200000, 1000000, false, "VP8", 2, 0, 400000, false, false, false, ""}; @@ -1249,7 +1249,7 @@ class DualStreamsTest : public ::testing::TestWithParam {}; // TODO(bugs.webrtc.org/9840): Investigate why is this test flaky on MAC. #if !defined(WEBRTC_ANDROID) && !defined(WEBRTC_IOS) && !defined(WEBRTC_MAC) TEST_P(DualStreamsTest, - ModeratelyRestricted_SlidesVp8_3TL_Simulcast_Video_Simulcast_High) { + ModeratelyRestricted_SlidesVp8_2TL_Simulcast_Video_Simulcast_High) { test::ScopedFieldTrials field_trial( AppendFieldTrials(std::string(kPacerPushBackExperiment) + std::string(kScreenshareSimulcastExperiment))); @@ -1264,11 +1264,11 @@ TEST_P(DualStreamsTest, ""}; ParamsWithLogging screenshare_params_high; - screenshare_params_high.video[0] = {true, 1850, 1110, 5, 400000, 1000000, - 1000000, false, "VP8", 3, 0, 400000, - false, false, false, ""}; + screenshare_params_high.video[0] = { + true, 1850, 1110, 60, 600000, 1250000, 1250000, false, + "VP8", 2, 0, 400000, false, false, false, ""}; VideoQualityTest::Params screenshare_params_low; - screenshare_params_low.video[0] = {true, 1850, 1110, 5, 50000, 200000, + screenshare_params_low.video[0] = {true, 1850, 1110, 5, 30000, 200000, 1000000, false, "VP8", 2, 0, 400000, false, false, false, ""}; std::vector screenhsare_streams = {