From d4d254f315c25615765e4f6419d872e4ea304455 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patrik=20H=C3=B6glund?= Date: Thu, 29 Nov 2018 15:09:31 +0000 Subject: [PATCH] Revert "Various VP9 high fps fixes" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit ba2840ce4eba4adebe7c968adcf7689caedccfa9. Reason for revert: Looks like this breaks all VP9 tests on the Chromium level, for Mac: https://ci.chromium.org/buildbot/chromium.webrtc/Mac%20Tester/85866 Search for TIMED OUT in for instance https://logs.chromium.org/logs/chromium/bb/chromium.webrtc/Mac_Tester/85866/+/recipes/steps/browser_tests/0/stdout (it times out because the video is frozen). Original change's description: > Various VP9 high fps fixes > > - Enable flexible mode in loopback tools and quality tests > - Ensure duplicate references are not set by the sender in video header > - Reset first active spatial layer on keyframe in encoder > - Make vp9 encoder to not generate spatial references for first active > layer with external reference control in svc flexible mode > > Bug: webrtc:10049 > Change-Id: If9ff576ea8a1a2fef6116b17b5b5adff08c5f8c6 > Reviewed-on: https://webrtc-review.googlesource.com/c/112080 > Commit-Queue: Ilya Nikolaevskiy > Reviewed-by: Sergey Silkin > Cr-Commit-Position: refs/heads/master@{#25795} TBR=ilnik@webrtc.org,ssilkin@webrtc.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: webrtc:10049 Change-Id: Ie6a7daf6414337173fec38c5ff546d509951cba6 Reviewed-on: https://webrtc-review.googlesource.com/c/112400 Reviewed-by: Patrik Höglund Reviewed-by: Ilya Nikolaevskiy Commit-Queue: Patrik Höglund Cr-Commit-Position: refs/heads/master@{#25842} --- modules/video_coding/codecs/vp9/svc_config.cc | 6 +- .../codecs/vp9/svc_config_unittest.cc | 7 +- .../codecs/vp9/svc_rate_allocator_unittest.cc | 2 +- modules/video_coding/codecs/vp9/vp9_impl.cc | 65 +++++++------------ modules/video_coding/codecs/vp9/vp9_impl.h | 4 +- video/video_quality_test.cc | 4 -- 6 files changed, 31 insertions(+), 57 deletions(-) diff --git a/modules/video_coding/codecs/vp9/svc_config.cc b/modules/video_coding/codecs/vp9/svc_config.cc index 0e76b09d6b..7a79a420a5 100644 --- a/modules/video_coding/codecs/vp9/svc_config.cc +++ b/modules/video_coding/codecs/vp9/svc_config.cc @@ -23,9 +23,9 @@ namespace webrtc { namespace { const size_t kMinVp9SvcBitrateKbps = 30; -const size_t kMaxNumLayersForScreenSharing = 3; -const float kMaxScreenSharingLayerFramerateFps[] = {5.0, 5.0, 30.0}; -const size_t kMaxScreenSharingLayerBitrateKbps[] = {200, 500, 1250}; +const size_t kMaxNumLayersForScreenSharing = 2; +const float kMaxScreenSharingLayerFramerateFps[] = {5.0, 5.0}; +const size_t kMaxScreenSharingLayerBitrateKbps[] = {200, 500}; } // namespace std::vector ConfigureSvcScreenSharing(size_t input_width, diff --git a/modules/video_coding/codecs/vp9/svc_config_unittest.cc b/modules/video_coding/codecs/vp9/svc_config_unittest.cc index 05802eb5b0..683c2d47a0 100644 --- a/modules/video_coding/codecs/vp9/svc_config_unittest.cc +++ b/modules/video_coding/codecs/vp9/svc_config_unittest.cc @@ -48,13 +48,12 @@ TEST(SvcConfig, ScreenSharing) { std::vector spatial_layers = GetSvcConfig(1920, 1080, 30, 3, 3, true); - EXPECT_EQ(spatial_layers.size(), 3UL); + EXPECT_EQ(spatial_layers.size(), 2UL); - for (size_t i = 0; i < 3; ++i) { - const SpatialLayer& layer = spatial_layers[i]; + for (const SpatialLayer& layer : spatial_layers) { EXPECT_EQ(layer.width, 1920); EXPECT_EQ(layer.height, 1080); - EXPECT_EQ(layer.maxFramerate, (i < 2) ? 5 : 30); + EXPECT_EQ(layer.maxFramerate, 5); EXPECT_EQ(layer.numberOfTemporalLayers, 1); EXPECT_LE(layer.minBitrate, layer.maxBitrate); EXPECT_LE(layer.minBitrate, layer.targetBitrate); diff --git a/modules/video_coding/codecs/vp9/svc_rate_allocator_unittest.cc b/modules/video_coding/codecs/vp9/svc_rate_allocator_unittest.cc index e430123909..ac225553ea 100644 --- a/modules/video_coding/codecs/vp9/svc_rate_allocator_unittest.cc +++ b/modules/video_coding/codecs/vp9/svc_rate_allocator_unittest.cc @@ -151,7 +151,7 @@ TEST(SvcRateAllocatorTest, MinBitrateToGetQualityLayer) { const SpatialLayer* layers = codec.spatialLayers; - EXPECT_LE(codec.VP9()->numberOfSpatialLayers, 3U); + EXPECT_LE(codec.VP9()->numberOfSpatialLayers, 2U); VideoBitrateAllocation allocation = allocator.GetAllocation(layers[0].minBitrate * 1000, 30); diff --git a/modules/video_coding/codecs/vp9/vp9_impl.cc b/modules/video_coding/codecs/vp9/vp9_impl.cc index 61542c5082..cec7d9ef72 100644 --- a/modules/video_coding/codecs/vp9/vp9_impl.cc +++ b/modules/video_coding/codecs/vp9/vp9_impl.cc @@ -696,33 +696,30 @@ int VP9EncoderImpl::Encode(const VideoFrame& input_image, } } - size_t first_active_spatial_layer_id = 0; - if (VideoCodecMode::kScreensharing == codec_.mode) { + if (VideoCodecMode::kScreensharing == codec_.mode && !force_key_frame_) { + // Skip encoding spatial layer frames if their target frame rate is lower + // than actual input frame rate. vpx_svc_layer_id_t layer_id = {0}; - if (!force_key_frame_) { - // Skip encoding spatial layer frames if their target frame rate is lower - // than actual input frame rate. - const size_t gof_idx = (pics_since_key_ + 1) % gof_.num_frames_in_gof; - layer_id.temporal_layer_id = gof_.temporal_idx[gof_idx]; + const size_t gof_idx = (pics_since_key_ + 1) % gof_.num_frames_in_gof; + layer_id.temporal_layer_id = gof_.temporal_idx[gof_idx]; - const uint32_t frame_timestamp_ms = - 1000 * input_image.timestamp() / kVideoPayloadTypeFrequency; + const uint32_t frame_timestamp_ms = + 1000 * input_image.timestamp() / kVideoPayloadTypeFrequency; - for (uint8_t sl_idx = 0; sl_idx < num_active_spatial_layers_; ++sl_idx) { - if (framerate_controller_[sl_idx].DropFrame(frame_timestamp_ms)) { - ++layer_id.spatial_layer_id; - } else { - break; - } - } - - RTC_DCHECK_LE(layer_id.spatial_layer_id, num_active_spatial_layers_); - if (layer_id.spatial_layer_id >= num_active_spatial_layers_) { - // Drop entire picture. - return WEBRTC_VIDEO_CODEC_OK; + for (uint8_t sl_idx = 0; sl_idx < num_active_spatial_layers_; ++sl_idx) { + if (framerate_controller_[sl_idx].DropFrame(frame_timestamp_ms)) { + ++layer_id.spatial_layer_id; + } else { + break; } } - first_active_spatial_layer_id = layer_id.spatial_layer_id; + + RTC_DCHECK_LE(layer_id.spatial_layer_id, num_active_spatial_layers_); + if (layer_id.spatial_layer_id >= num_active_spatial_layers_) { + // Drop entire picture. + return WEBRTC_VIDEO_CODEC_OK; + } + vpx_codec_control(encoder_, VP9E_SET_SVC_LAYER_ID, &layer_id); } @@ -783,8 +780,7 @@ int VP9EncoderImpl::Encode(const VideoFrame& input_image, } if (external_ref_control_) { - vpx_svc_ref_frame_config_t ref_config = - SetReferences(force_key_frame_, first_active_spatial_layer_id); + vpx_svc_ref_frame_config_t ref_config = SetReferences(force_key_frame_); if (VideoCodecMode::kScreensharing == codec_.mode) { for (uint8_t sl_idx = 0; sl_idx < num_active_spatial_layers_; ++sl_idx) { @@ -989,8 +985,6 @@ void VP9EncoderImpl::FillReferenceIndices(const vpx_codec_cx_pkt& pkt, size_t max_ref_temporal_layer_id = 0; - std::vector ref_pid_list; - vp9_info->num_ref_pics = 0; for (const RefFrameBuffer& ref_buf : ref_buf_list) { RTC_DCHECK_LE(ref_buf.pic_num, pic_num); @@ -1003,16 +997,6 @@ void VP9EncoderImpl::FillReferenceIndices(const vpx_codec_cx_pkt& pkt, } RTC_DCHECK_LE(ref_buf.temporal_layer_id, layer_id.temporal_layer_id); - // Encoder may reference several spatial layers on the same previous - // frame in case if some spatial layers are skipped on the current frame. - // We shouldn't put duplicate references as it may break some old - // clients and isn't RTP compatible. - if (std::find(ref_pid_list.begin(), ref_pid_list.end(), - ref_buf.pic_num) != ref_pid_list.end()) { - continue; - } - ref_pid_list.push_back(ref_buf.pic_num); - const size_t p_diff = pic_num - ref_buf.pic_num; RTC_DCHECK_LE(p_diff, 127UL); @@ -1077,9 +1061,7 @@ void VP9EncoderImpl::UpdateReferenceBuffers(const vpx_codec_cx_pkt& pkt, } } -vpx_svc_ref_frame_config_t VP9EncoderImpl::SetReferences( - bool is_key_pic, - size_t first_active_spatial_layer_id) { +vpx_svc_ref_frame_config_t VP9EncoderImpl::SetReferences(bool is_key_pic) { // kRefBufIdx, kUpdBufIdx need to be updated to support longer GOFs. RTC_DCHECK_LE(gof_.num_frames_in_gof, 4); @@ -1131,14 +1113,13 @@ vpx_svc_ref_frame_config_t VP9EncoderImpl::SetReferences( } } - if (is_inter_layer_pred_allowed && sl_idx > first_active_spatial_layer_id) { + if (is_inter_layer_pred_allowed && sl_idx > 0) { // Set up spatial reference. RTC_DCHECK(last_updated_buf_idx); ref_config.gld_fb_idx[sl_idx] = *last_updated_buf_idx; ref_config.reference_golden[sl_idx] = 1; } else { - RTC_DCHECK(ref_config.reference_last[sl_idx] != 0 || - sl_idx == first_active_spatial_layer_id || + RTC_DCHECK(ref_config.reference_last[sl_idx] != 0 || sl_idx == 0 || inter_layer_pred_ == InterLayerPredMode::kOff); } diff --git a/modules/video_coding/codecs/vp9/vp9_impl.h b/modules/video_coding/codecs/vp9/vp9_impl.h index 3bfab9ad5f..33f41fd7d4 100644 --- a/modules/video_coding/codecs/vp9/vp9_impl.h +++ b/modules/video_coding/codecs/vp9/vp9_impl.h @@ -70,9 +70,7 @@ class VP9EncoderImpl : public VP9Encoder { CodecSpecificInfoVP9* vp9_info); void UpdateReferenceBuffers(const vpx_codec_cx_pkt& pkt, const size_t pic_num); - vpx_svc_ref_frame_config_t SetReferences( - bool is_key_pic, - size_t first_active_spatial_layer_id); + vpx_svc_ref_frame_config_t SetReferences(bool is_key_pic); bool ExplicitlyConfiguredSpatialLayers() const; bool SetSvcRates(const VideoBitrateAllocation& bitrate_allocation); diff --git a/video/video_quality_test.cc b/video/video_quality_test.cc index d6ccb6522e..3261d41c7a 100644 --- a/video/video_quality_test.cc +++ b/video/video_quality_test.cc @@ -670,10 +670,6 @@ void VideoQualityTest::SetupVideo(Transport* send_transport, vp9_settings.numberOfSpatialLayers = static_cast( params_.ss[video_idx].num_spatial_layers); vp9_settings.interLayerPred = params_.ss[video_idx].inter_layer_pred; - // High FPS vp9 screenshare requires flexible mode. - if (params_.video[video_idx].fps > 5) { - vp9_settings.flexibleMode = true; - } video_encoder_configs_[video_idx].encoder_specific_settings = new rtc::RefCountedObject< VideoEncoderConfig::Vp9EncoderSpecificSettings>(vp9_settings);