Revert "Follow codec preference order for sending codec fallback."
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 <sprang@webrtc.org>
> Reviewed-by: Henrik Boström <hbos@webrtc.org>
> Commit-Queue: Jianlin Qiu <jianlin.qiu@intel.com>
> Reviewed-by: Harald Alvestrand <hta@webrtc.org>
> Reviewed-by: Sergey Silkin <ssilkin@webrtc.org>
> 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 <mbonadei@webrtc.org>
Reviewed-by: Erik Språng <sprang@webrtc.org>
Bot-Commit: rubber-stamper@appspot.gserviceaccount.com <rubber-stamper@appspot.gserviceaccount.com>
Reviewed-by: Sergey Silkin <ssilkin@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#43600}
diff --git a/experiments/field_trials.py b/experiments/field_trials.py
index 0070768..3eea585 100755
--- a/experiments/field_trials.py
+++ b/experiments/field_trials.py
@@ -152,9 +152,6 @@
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 527550a..8822022 100644
--- a/media/engine/webrtc_video_engine.cc
+++ b/media/engine/webrtc_video_engine.cc
@@ -1233,10 +1233,6 @@
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 a541532..91249da 100644
--- a/media/engine/webrtc_video_engine_unittest.cc
+++ b/media/engine/webrtc_video_engine_unittest.cc
@@ -2547,31 +2547,6 @@
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> 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 e527c5a..8620c62 100644
--- a/video/video_stream_encoder.cc
+++ b/video/video_stream_encoder.cc
@@ -80,10 +80,6 @@
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 @@
}
// If encoder selector is available, switch to the encoder it prefers.
+ // Otherwise try switching to VP8 (default WebRTC codec).
std::optional<SdpVideoFormat> 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 @@
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 363e4dd..475a9e6 100644
--- a/video/video_stream_encoder.h
+++ b/video/video_stream_encoder.h
@@ -325,6 +325,8 @@
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 @@
// 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<VideoStreamAdapter> 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 a843a81..dc977f2 100644
--- a/video/video_stream_encoder_unittest.cc
+++ b/video/video_stream_encoder_unittest.cc
@@ -1237,11 +1237,6 @@
return num_set_rates_;
}
- int GetNumEncodes() const {
- MutexLock lock(&local_mutex_);
- return num_encodes_;
- }
-
void SetPreferredPixelFormats(
absl::InlinedVector<VideoFrameBuffer::Type, kMaxPreferredPixelFormats>
pixel_formats) {
@@ -1264,7 +1259,6 @@
const std::vector<VideoFrameType>* 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 @@
std::vector<ResolutionBitrateLimits> 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<VideoFrameBuffer::Type> last_input_pixel_format_
RTC_GUARDED_BY(local_mutex_);
absl::InlinedVector<VideoFrameBuffer::Type, kMaxPreferredPixelFormats>
@@ -8203,7 +8196,9 @@
.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 @@
video_stream_encoder_.reset();
}
-TEST_F(VideoStreamEncoderTest, NoPreferenceDefaultFallbackToVP8Disabled) {
- constexpr int kSufficientBitrateToNotDrop = 1000;
- constexpr int kDontCare = 100;
- constexpr int kNumFrames = 8;
-
- NiceMock<MockVideoEncoder> video_encoder;
- StrictMock<MockEncoderSwitchRequestCallback> switch_callback;
- video_send_config_.encoder_settings.encoder_switch_request_callback =
- &switch_callback;
- auto encoder_factory = std::make_unique<test::VideoEncoderProxyFactory>(
- &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<MockVideoEncoder> video_encoder;
- StrictMock<MockEncoderSwitchRequestCallback> switch_callback;
- video_send_config_.encoder_settings.encoder_switch_request_callback =
- &switch_callback;
- auto encoder_factory = std::make_unique<test::VideoEncoderProxyFactory>(
- &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;
