From 7976b77345ee91b44c06850ddee17cd71a681493 Mon Sep 17 00:00:00 2001 From: Mirko Bonadei Date: Wed, 18 Dec 2024 07:41:08 -0800 Subject: [PATCH] Revert "Follow codec preference order for sending codec fallback." MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit 1ad3e14e9981772554a848c5034c7c555680aef7. Reason for revert: Breaks downstream project. We are investigating into a potential problem when running on mobile platforms. We will get back with info or reland. Original change's description: > Follow codec preference order for sending codec fallback. > > When encoder selector is not enabled, currently we always fallback to > VP8 no matter how the codec preference is setup. Update to follow codec > preference order for the fallback. > > Bug: chromium:378566918 > Change-Id: Ia3fbfc9d407683ef7b3d6246af7e9ec58535dc89 > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/370707 > Reviewed-by: Erik Språng > Reviewed-by: Henrik Boström > Commit-Queue: Jianlin Qiu > Reviewed-by: Harald Alvestrand > Reviewed-by: Sergey Silkin > Cr-Commit-Position: refs/heads/main@{#43566} Bug: chromium:378566918 Change-Id: I09086b5ad100a8f66e87df167e903d0b5fe5b589 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/372080 Commit-Queue: Mirko Bonadei Reviewed-by: Erik Språng Bot-Commit: rubber-stamper@appspot.gserviceaccount.com Reviewed-by: Sergey Silkin Cr-Commit-Position: refs/heads/main@{#43600} --- experiments/field_trials.py | 3 - media/engine/webrtc_video_engine.cc | 4 - media/engine/webrtc_video_engine_unittest.cc | 25 ---- video/video_stream_encoder.cc | 28 ++--- video/video_stream_encoder.h | 4 +- video/video_stream_encoder_unittest.cc | 117 +------------------ 6 files changed, 12 insertions(+), 169 deletions(-) diff --git a/experiments/field_trials.py b/experiments/field_trials.py index 0070768ca3..3eea5853ec 100755 --- a/experiments/field_trials.py +++ b/experiments/field_trials.py @@ -152,9 +152,6 @@ ACTIVE_FIELD_TRIALS: FrozenSet[FieldTrial] = frozenset([ FieldTrial('WebRTC-SrtpRemoveReceiveStream', 42225949, date(2024, 10, 1)), - FieldTrial('WebRTC-SwitchEncoderFollowCodecPreferenceOrder', - 378566918, - date(2025, 5, 1)), FieldTrial('WebRTC-TaskQueue-ReplaceLibeventWithStdlib', 42224654, date(2024, 4, 1)), diff --git a/media/engine/webrtc_video_engine.cc b/media/engine/webrtc_video_engine.cc index 527550a345..8822022103 100644 --- a/media/engine/webrtc_video_engine.cc +++ b/media/engine/webrtc_video_engine.cc @@ -1233,10 +1233,6 @@ void WebRtcVideoSendChannel::RequestEncoderFallback() { RTC_DCHECK_RUN_ON(&thread_checker_); if (negotiated_codecs_.size() <= 1) { RTC_LOG(LS_WARNING) << "Encoder failed but no fallback codec is available"; - // Remove all the send streams since we cannot encode any more. - while (!send_streams_.empty()) { - RemoveSendStream(send_streams_.begin()->first); - } return; } diff --git a/media/engine/webrtc_video_engine_unittest.cc b/media/engine/webrtc_video_engine_unittest.cc index a541532b3c..91249da4bd 100644 --- a/media/engine/webrtc_video_engine_unittest.cc +++ b/media/engine/webrtc_video_engine_unittest.cc @@ -2547,31 +2547,6 @@ TEST_F(WebRtcVideoChannelBaseTest, TwoStreamsSendAndReceive) { TwoStreamsSendAndReceive(codec); } -TEST_F(WebRtcVideoChannelBaseTest, - RequestEncoderFallbackNextCodecFollowNegotiatedOrder) { - cricket::VideoSenderParameters parameters; - parameters.codecs.push_back(GetEngineCodec("VP9")); - parameters.codecs.push_back(GetEngineCodec("AV1")); - parameters.codecs.push_back(GetEngineCodec("VP8")); - EXPECT_TRUE(send_channel_->SetSenderParameters(parameters)); - - std::optional codec = send_channel_->GetSendCodec(); - ASSERT_TRUE(codec); - EXPECT_EQ("VP9", codec->name); - - SendImpl()->RequestEncoderFallback(); - time_controller_.AdvanceTime(kFrameDuration); - codec = send_channel_->GetSendCodec(); - ASSERT_TRUE(codec); - EXPECT_EQ("AV1", codec->name); - - SendImpl()->RequestEncoderFallback(); - time_controller_.AdvanceTime(kFrameDuration); - codec = send_channel_->GetSendCodec(); - ASSERT_TRUE(codec); - EXPECT_EQ("VP8", codec->name); -} - #if defined(RTC_ENABLE_VP9) TEST_F(WebRtcVideoChannelBaseTest, RequestEncoderFallback) { diff --git a/video/video_stream_encoder.cc b/video/video_stream_encoder.cc index e527c5a92b..8620c621dc 100644 --- a/video/video_stream_encoder.cc +++ b/video/video_stream_encoder.cc @@ -80,10 +80,6 @@ constexpr char kFrameDropperFieldTrial[] = "WebRTC-FrameDropper"; constexpr char kSwitchEncoderOnInitializationFailuresFieldTrial[] = "WebRTC-SwitchEncoderOnInitializationFailures"; -// TODO(crbugs.com/378566918): Remove this kill switch after rollout. -constexpr char kSwitchEncoderFollowCodecPreferenceOrderFieldTrial[] = - "WebRTC-SwitchEncoderFollowCodecPreferenceOrder"; - const size_t kDefaultPayloadSize = 1440; const int64_t kParameterUpdateIntervalMs = 1000; @@ -1485,21 +1481,15 @@ void VideoStreamEncoder::RequestEncoderSwitch() { } // If encoder selector is available, switch to the encoder it prefers. + // Otherwise try switching to VP8 (default WebRTC codec). std::optional preferred_fallback_encoder; if (is_encoder_selector_available) { preferred_fallback_encoder = encoder_selector_->OnEncoderBroken(); } if (!preferred_fallback_encoder) { - if (!env_.field_trials().IsDisabled( - kSwitchEncoderFollowCodecPreferenceOrderFieldTrial)) { - encoder_fallback_requested_ = true; - settings_.encoder_switch_request_callback->RequestEncoderFallback(); - return; - } else { - preferred_fallback_encoder = - SdpVideoFormat(CodecTypeToPayloadString(kVideoCodecVP8)); - } + preferred_fallback_encoder = + SdpVideoFormat(CodecTypeToPayloadString(kVideoCodecVP8)); } settings_.encoder_switch_request_callback->RequestEncoderSwitch( @@ -1902,15 +1892,11 @@ void VideoStreamEncoder::EncodeVideoFrame(const VideoFrame& video_frame, RTC_LOG(LS_VERBOSE) << __func__ << " posted " << time_when_posted_us << " ntp time " << video_frame.ntp_time_ms(); - // If encoder fallback is requested, but WebRtcVideoEngine cannot switch codec - // due to negotatied codec list is already exhausted, we don't continue to - // encode frames, and WebRtcVideoEngine will destroy the VideoSendStreams - // along with current VideoStreamEncoder. If the encoder fallback is requested - // and WebRtcVideoEngine responds to the fallback request, the VideoSendStream - // is recreated and current VideoStreamEncoder will no longer be used. - if (encoder_fallback_requested_ || !encoder_initialized_) { + // If the encoder fail we can't continue to encode frames. When this happens + // the WebrtcVideoSender is notified and the whole VideoSendStream is + // recreated. + if (encoder_failed_ || !encoder_initialized_) return; - } // It's possible that EncodeVideoFrame can be called after we've completed // a Stop() operation. Check if the encoder_ is set before continuing. diff --git a/video/video_stream_encoder.h b/video/video_stream_encoder.h index 363e4dd1c3..475a9e65b0 100644 --- a/video/video_stream_encoder.h +++ b/video/video_stream_encoder.h @@ -325,6 +325,8 @@ class VideoStreamEncoder : public VideoStreamEncoderInterface, bool was_encode_called_since_last_initialization_ RTC_GUARDED_BY(encoder_queue_) = false; + bool encoder_failed_ RTC_GUARDED_BY(encoder_queue_) = false; + // Used to make sure incoming time stamp is increasing for every frame. int64_t last_captured_timestamp_ RTC_GUARDED_BY(encoder_queue_) = 0; // Delta used for translating between NTP and internal timestamps. @@ -380,8 +382,6 @@ class VideoStreamEncoder : public VideoStreamEncoderInterface, // Provides video stream input states: current resolution and frame rate. VideoStreamInputStateProvider input_state_provider_; - bool encoder_fallback_requested_ RTC_GUARDED_BY(encoder_queue_) = false; - const std::unique_ptr video_stream_adapter_ RTC_GUARDED_BY(encoder_queue_); // Responsible for adapting input resolution or frame rate to ensure resources diff --git a/video/video_stream_encoder_unittest.cc b/video/video_stream_encoder_unittest.cc index a843a81d57..dc977f2e67 100644 --- a/video/video_stream_encoder_unittest.cc +++ b/video/video_stream_encoder_unittest.cc @@ -1237,11 +1237,6 @@ class VideoStreamEncoderTest : public ::testing::Test { return num_set_rates_; } - int GetNumEncodes() const { - MutexLock lock(&local_mutex_); - return num_encodes_; - } - void SetPreferredPixelFormats( absl::InlinedVector pixel_formats) { @@ -1264,7 +1259,6 @@ class VideoStreamEncoderTest : public ::testing::Test { const std::vector* frame_types) override { { MutexLock lock(&local_mutex_); - num_encodes_++; if (expect_null_frame_) { EXPECT_EQ(input_image.rtp_timestamp(), 0u); EXPECT_EQ(input_image.width(), 1); @@ -1396,7 +1390,6 @@ class VideoStreamEncoderTest : public ::testing::Test { std::vector resolution_bitrate_limits_ RTC_GUARDED_BY(local_mutex_); int num_set_rates_ RTC_GUARDED_BY(local_mutex_) = 0; - int num_encodes_ RTC_GUARDED_BY(local_mutex_) = 0; std::optional last_input_pixel_format_ RTC_GUARDED_BY(local_mutex_); absl::InlinedVector @@ -8203,7 +8196,9 @@ TEST_F(VideoStreamEncoderTest, .WillByDefault(Return(WEBRTC_VIDEO_CODEC_ENCODER_FAILURE)); rtc::Event encode_attempted; - EXPECT_CALL(switch_callback, RequestEncoderFallback()) + EXPECT_CALL(switch_callback, + RequestEncoderSwitch(Field(&SdpVideoFormat::name, "VP8"), + /*allow_default_fallback=*/true)) .WillOnce([&encode_attempted]() { encode_attempted.Set(); }); video_source_.IncomingCapturedFrame(CreateFrame(1, nullptr)); @@ -8271,112 +8266,6 @@ TEST_F(VideoStreamEncoderTest, NullEncoderReturnSwitch) { video_stream_encoder_.reset(); } -TEST_F(VideoStreamEncoderTest, NoPreferenceDefaultFallbackToVP8Disabled) { - constexpr int kSufficientBitrateToNotDrop = 1000; - constexpr int kDontCare = 100; - constexpr int kNumFrames = 8; - - NiceMock video_encoder; - StrictMock switch_callback; - video_send_config_.encoder_settings.encoder_switch_request_callback = - &switch_callback; - auto encoder_factory = std::make_unique( - &video_encoder, /*encoder_selector=*/nullptr); - video_send_config_.encoder_settings.encoder_factory = encoder_factory.get(); - - // Reset encoder for new configuration to take effect. - ConfigureEncoder(video_encoder_config_.Copy()); - - // The VideoStreamEncoder needs some bitrate before it can start encoding, - // setting some bitrate so that subsequent calls to WaitForEncodedFrame does - // not fail. - video_stream_encoder_->OnBitrateUpdatedAndWaitForManagedResources( - /*target_bitrate=*/DataRate::KilobitsPerSec(kSufficientBitrateToNotDrop), - /*stable_target_bitrate=*/ - DataRate::KilobitsPerSec(kSufficientBitrateToNotDrop), - /*link_allocation=*/DataRate::KilobitsPerSec(kSufficientBitrateToNotDrop), - /*fraction_lost=*/0, - /*round_trip_time_ms=*/0, - /*cwnd_reduce_ratio=*/0); - - EXPECT_CALL(video_encoder, Encode) - .WillOnce(Return(WEBRTC_VIDEO_CODEC_ENCODER_FAILURE)); - - EXPECT_CALL(switch_callback, RequestEncoderFallback()); - - VideoFrame frame = CreateFrame(1, kDontCare, kDontCare); - for (int i = 0; i < kNumFrames; ++i) { - int64_t timestamp_ms = CurrentTimeMs(); - frame.set_ntp_time_ms(timestamp_ms); - frame.set_timestamp_us(timestamp_ms * 1000); - video_source_.IncomingCapturedFrame(frame); - time_controller_.AdvanceTime(TimeDelta::Millis(33)); - } - - EXPECT_TRUE_WAIT(fake_encoder_.GetNumEncodes() == 0, 5000); - // After requesting fallback failure, the encoder will be released. - EXPECT_CALL(video_encoder, Release()).Times(1); - - AdvanceTime(TimeDelta::Zero()); - video_stream_encoder_->Stop(); - // The encoders produced by the VideoEncoderProxyFactory have a pointer back - // to it's factory, so in order for the encoder instance in the - // `video_stream_encoder_` to be destroyed before the `encoder_factory` we - // reset the `video_stream_encoder_` here. - video_stream_encoder_.reset(); -} - -TEST_F(VideoStreamEncoderTest, NoPreferenceDefaultFallbackToVP8Enabled) { - constexpr int kSufficientBitrateToNotDrop = 1000; - constexpr int kDontCare = 100; - - webrtc::test::ScopedKeyValueConfig field_trials( - field_trials_, - "WebRTC-SwitchEncoderFollowCodecPreferenceOrder/Disabled/"); - - NiceMock video_encoder; - StrictMock switch_callback; - video_send_config_.encoder_settings.encoder_switch_request_callback = - &switch_callback; - auto encoder_factory = std::make_unique( - &video_encoder, /*encoder_selector=*/nullptr); - video_send_config_.encoder_settings.encoder_factory = encoder_factory.get(); - video_encoder_config_.codec_type = kVideoCodecVP9; - - // Reset encoder for new configuration to take effect. - ConfigureEncoder(video_encoder_config_.Copy()); - - // The VideoStreamEncoder needs some bitrate before it can start encoding, - // setting some bitrate so that subsequent calls to WaitForEncodedFrame does - // not fail. - video_stream_encoder_->OnBitrateUpdatedAndWaitForManagedResources( - /*target_bitrate=*/DataRate::KilobitsPerSec(kSufficientBitrateToNotDrop), - /*stable_target_bitrate=*/ - DataRate::KilobitsPerSec(kSufficientBitrateToNotDrop), - /*link_allocation=*/DataRate::KilobitsPerSec(kSufficientBitrateToNotDrop), - /*fraction_lost=*/0, - /*round_trip_time_ms=*/0, - /*cwnd_reduce_ratio=*/0); - - EXPECT_CALL(video_encoder, Encode) - .WillOnce(Return(WEBRTC_VIDEO_CODEC_ENCODER_FAILURE)); - - // Fallback request will be asking for switching to VP8. - EXPECT_CALL(switch_callback, - RequestEncoderSwitch(Field(&SdpVideoFormat::name, "VP8"), - /*allow_default_fallback=*/true)); - - VideoFrame frame = CreateFrame(1, kDontCare, kDontCare); - video_source_.IncomingCapturedFrame(frame); - - video_stream_encoder_->Stop(); - // The encoders produced by the VideoEncoderProxyFactory have a pointer back - // to it's factory, so in order for the encoder instance in the - // `video_stream_encoder_` to be destroyed before the `encoder_factory` we - // reset the `video_stream_encoder_` here. - video_stream_encoder_.reset(); -} - TEST_F(VideoStreamEncoderTest, AllocationPropagatedToEncoderWhenTargetRateChanged) { const int kFrameWidth = 320;