From 3791d332c53b283d76d2d3f2fc7e2e74ae28ba1e Mon Sep 17 00:00:00 2001 From: Jiwon Jung Date: Fri, 8 Oct 2021 18:22:57 +0900 Subject: [PATCH] Proper assignment of FEC setting to encoders in SoftwareFallbackWrapper. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When using VideoEncoderSoftwareFallbackWrapper, releasing and initialization of encoder_ (H/W) and fallback_encoder_(S/W) happen repeatedly as reconfiguration procedure is called from higher layer. Below problems would occur when our encoder_(H/W) fails to initialize or encode. Firstly, some encoders' SetFecControllerOverride() functions will fail during repeated calls since they have checks like RTC_DCHECK(!fec_controller_override_) to avoid repeated assignment of fec_controller_override_. (see : LibvpxVp8Encoder::SetFecControllerOverride()) Secondly, if main_ encoder fails to initialize at first attempt, FEC setting (fec_controller_override) will not set until reconfiguration procedure is called again. This CL comes with two changes to fix above problems. 1. Sets fec_controller_override to both encoders when SoftwareFallbackWrapper::SetFecController() is called. 2. Removes the current_encoder()->SetFecControllerOverride() in PrimeEncoder() to avoid redundant calls which may involve fatal error. Bug: webrtc:13184 Change-Id: I082c93de552bc9ec3141c6490d35acfcee2f8935 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/234301 Reviewed-by: Ilya Nikolaevskiy Reviewed-by: Niels Moller Reviewed-by: Åsa Persson Commit-Queue: Ilya Nikolaevskiy Cr-Commit-Position: refs/heads/main@{#35231} --- ...deo_encoder_software_fallback_wrapper_unittest.cc | 8 ++++---- .../video_encoder_software_fallback_wrapper.cc | 12 ++++-------- 2 files changed, 8 insertions(+), 12 deletions(-) diff --git a/api/video_codecs/test/video_encoder_software_fallback_wrapper_unittest.cc b/api/video_codecs/test/video_encoder_software_fallback_wrapper_unittest.cc index a63a7c27fd..1db1908dc9 100644 --- a/api/video_codecs/test/video_encoder_software_fallback_wrapper_unittest.cc +++ b/api/video_codecs/test/video_encoder_software_fallback_wrapper_unittest.cc @@ -982,7 +982,7 @@ TEST_F(PreferTemporalLayersFallbackTest, PrimesEncoderOnSwitch) { wrapper_->RegisterEncodeCompleteCallback(&callback1); EXPECT_CALL(*hw_, SetFecControllerOverride(&fec_controller_override1)); - EXPECT_CALL(*sw_, SetFecControllerOverride).Times(0); + EXPECT_CALL(*sw_, SetFecControllerOverride).Times(1); wrapper_->SetFecControllerOverride(&fec_controller_override1); EXPECT_CALL(*hw_, SetRates(rate_params1)); @@ -1007,7 +1007,7 @@ TEST_F(PreferTemporalLayersFallbackTest, PrimesEncoderOnSwitch) { EXPECT_CALL(*sw_, RegisterEncodeCompleteCallback(&callback1)); EXPECT_CALL(*hw_, RegisterEncodeCompleteCallback).Times(0); - EXPECT_CALL(*sw_, SetFecControllerOverride(&fec_controller_override1)); + EXPECT_CALL(*sw_, SetFecControllerOverride).Times(0); EXPECT_CALL(*hw_, SetFecControllerOverride).Times(0); // Rate control parameters are cleared on InitEncode. @@ -1041,7 +1041,7 @@ TEST_F(PreferTemporalLayersFallbackTest, PrimesEncoderOnSwitch) { wrapper_->RegisterEncodeCompleteCallback(&callback2); EXPECT_CALL(*sw_, SetFecControllerOverride(&fec_controller_override2)); - EXPECT_CALL(*hw_, SetFecControllerOverride).Times(0); + EXPECT_CALL(*hw_, SetFecControllerOverride).Times(1); wrapper_->SetFecControllerOverride(&fec_controller_override2); EXPECT_CALL(*sw_, SetRates(rate_params2)); @@ -1066,7 +1066,7 @@ TEST_F(PreferTemporalLayersFallbackTest, PrimesEncoderOnSwitch) { EXPECT_CALL(*hw_, RegisterEncodeCompleteCallback(&callback2)); EXPECT_CALL(*sw_, RegisterEncodeCompleteCallback).Times(0); - EXPECT_CALL(*hw_, SetFecControllerOverride(&fec_controller_override2)); + EXPECT_CALL(*hw_, SetFecControllerOverride).Times(0); EXPECT_CALL(*sw_, SetFecControllerOverride).Times(0); // Rate control parameters are cleared on InitEncode. diff --git a/api/video_codecs/video_encoder_software_fallback_wrapper.cc b/api/video_codecs/video_encoder_software_fallback_wrapper.cc index e95c088811..8202217880 100644 --- a/api/video_codecs/video_encoder_software_fallback_wrapper.cc +++ b/api/video_codecs/video_encoder_software_fallback_wrapper.cc @@ -180,7 +180,6 @@ class VideoEncoderSoftwareFallbackWrapper final : public VideoEncoder { // The last channel parameters set. absl::optional packet_loss_; absl::optional rtt_; - FecControllerOverride* fec_controller_override_; absl::optional loss_notification_; enum class EncoderState { @@ -205,8 +204,7 @@ VideoEncoderSoftwareFallbackWrapper::VideoEncoderSoftwareFallbackWrapper( std::unique_ptr sw_encoder, std::unique_ptr hw_encoder, bool prefer_temporal_support) - : fec_controller_override_(nullptr), - encoder_state_(EncoderState::kUninitialized), + : encoder_state_(EncoderState::kUninitialized), encoder_(std::move(hw_encoder)), fallback_encoder_(std::move(sw_encoder)), callback_(nullptr), @@ -234,9 +232,7 @@ void VideoEncoderSoftwareFallbackWrapper::PrimeEncoder( if (packet_loss_.has_value()) { encoder->OnPacketLossRateUpdate(packet_loss_.value()); } - if (fec_controller_override_) { - encoder->SetFecControllerOverride(fec_controller_override_); - } + if (loss_notification_.has_value()) { encoder->OnLossNotification(loss_notification_.value()); } @@ -277,8 +273,8 @@ void VideoEncoderSoftwareFallbackWrapper::SetFecControllerOverride( // `fec_controller_override` at a given time. This is the responsibility // of `this` to maintain. - fec_controller_override_ = fec_controller_override; - current_encoder()->SetFecControllerOverride(fec_controller_override); + encoder_->SetFecControllerOverride(fec_controller_override); + fallback_encoder_->SetFecControllerOverride(fec_controller_override); } int32_t VideoEncoderSoftwareFallbackWrapper::InitEncode(