From 9a7f650bcd14f241d20f88f4e1ea3b7300de72ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Erik=20Spr=C3=A5ng?= Date: Tue, 21 Apr 2026 13:16:27 +0200 Subject: [PATCH] [M148] Always release VideoEncoders before destruction. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Original change's description: > Always release VideoEncoders before destruction. > > Without a `Release()` call there is a potential for race conditions > between asynchronous encoder callbacks and the destructor sequence. > > Bug: chromium:504620824 > Change-Id: I1fba23ef3a4a238503cdf8d2ea5831e815d7b902 > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/465582 > Auto-Submit: Erik Språng > Reviewed-by: Sergey Silkin > Commit-Queue: Sergey Silkin > Commit-Queue: Erik Språng > Cr-Commit-Position: refs/heads/main@{#47495} (cherry picked from commit 21d9744bd599d106da9171fbb1ed09047d7178ca) Bug: chromium:505251595,chromium:504620824 Change-Id: I1fba23ef3a4a238503cdf8d2ea5831e815d7b902 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/468940 Auto-Submit: Chrome Cherry Picker Commit-Queue: rubber-stamper@appspot.gserviceaccount.com Bot-Commit: rubber-stamper@appspot.gserviceaccount.com Cr-Commit-Position: refs/branch-heads/7778@{#7} Cr-Branched-From: ca896b7ffef011bbf6957c99d413c5aac602c99f-refs/heads/main@{#47319} --- video/video_stream_encoder.cc | 1 + video/video_stream_encoder_unittest.cc | 92 ++++++++++++++++++++++++++ 2 files changed, 93 insertions(+) diff --git a/video/video_stream_encoder.cc b/video/video_stream_encoder.cc index 4faf80cec7..cbed7af318 100644 --- a/video/video_stream_encoder.cc +++ b/video/video_stream_encoder.cc @@ -1079,6 +1079,7 @@ void VideoStreamEncoder::ReconfigureEncoder() { // Destroy existing encoder instance before creating a new one. Otherwise // attempt to create another instance will fail if encoder factory // supports only single instance of encoder of given type. + ReleaseEncoder(); encoder_.reset(); encoder_ = MaybeCreateFrameDumpingEncoderWrapper( diff --git a/video/video_stream_encoder_unittest.cc b/video/video_stream_encoder_unittest.cc index 64468c3448..09c109744e 100644 --- a/video/video_stream_encoder_unittest.cc +++ b/video/video_stream_encoder_unittest.cc @@ -28,6 +28,7 @@ #include "api/adaptation/resource.h" #include "api/environment/environment.h" #include "api/environment/environment_factory.h" +#include "api/fec_controller_override.h" #include "api/field_trials.h" #include "api/field_trials_view.h" #include "api/location.h" @@ -42,6 +43,7 @@ #include "api/test/mock_video_encoder_factory.h" #include "api/test/rtc_error_matchers.h" #include "api/test/time_controller.h" +#include "api/test/video/function_video_encoder_factory.h" #include "api/units/data_rate.h" #include "api/units/data_size.h" #include "api/units/frequency.h" @@ -2377,6 +2379,96 @@ TEST_F(VideoStreamEncoderTest, video_stream_encoder_->Stop(); } +TEST_F(VideoStreamEncoderTest, + EncoderReleasedBeforeDestructionOnReconfiguration) { + class ReleaseCheckEncoderProxy : public VideoEncoder { + public: + ReleaseCheckEncoderProxy(VideoEncoder* encoder, bool& released) + : encoder_(encoder), released_(released) {} + ~ReleaseCheckEncoderProxy() override = default; + + int32_t Encode(const VideoFrame& input_image, + const std::vector* frame_types) override { + return encoder_->Encode(input_image, frame_types); + } + + int32_t InitEncode(const VideoCodec* config, + const Settings& settings) override { + return encoder_->InitEncode(config, settings); + } + + int32_t RegisterEncodeCompleteCallback( + EncodedImageCallback* callback) override { + return encoder_->RegisterEncodeCompleteCallback(callback); + } + + int32_t Release() override { + released_ = true; + return encoder_->Release(); + } + + void SetRates(const RateControlParameters& parameters) override { + encoder_->SetRates(parameters); + } + + VideoEncoder::EncoderInfo GetEncoderInfo() const override { + return encoder_->GetEncoderInfo(); + } + + void SetFecControllerOverride( + FecControllerOverride* fec_controller_override) override { + encoder_->SetFecControllerOverride(fec_controller_override); + } + + private: + VideoEncoder* const encoder_; + bool& released_; + }; + + bool released[2] = {false, false}; + int instance_count = 0; + + test::FunctionVideoEncoderFactory factory( + [this, &instance_count, &released](const Environment& env, + const SdpVideoFormat& format) { + RTC_CHECK_LT(instance_count, 2); + bool& r = released[instance_count++]; + return std::make_unique(&fake_encoder_, r); + }); + + video_send_config_.encoder_settings.encoder_factory = &factory; + ConfigureEncoder(video_encoder_config_.Copy()); + + video_stream_encoder_->OnBitrateUpdatedAndWaitForManagedResources( + kTargetBitrate, kTargetBitrate, 0, 0, 0); + + // Capture a frame and wait for it to synchronize with the encoder thread. + video_source_.IncomingCapturedFrame(CreateFrame(1, nullptr)); + WaitForEncodedFrame(1); + + EXPECT_EQ(instance_count, 1); + EXPECT_FALSE(released[0]); + + VideoEncoderConfig video_encoder_config = video_encoder_config_.Copy(); + // Changing the max payload data length recreates encoder. + video_stream_encoder_->ConfigureEncoder(std::move(video_encoder_config), + kMaxPayloadLength / 2); + + // Capture a frame and wait for it to synchronize with the encoder thread. + video_source_.IncomingCapturedFrame(CreateFrame(2, nullptr)); + WaitForEncodedFrame(2); + + // We expect that two encoders were created. + EXPECT_EQ(instance_count, 2); + // The first encoder should have been released before destruction. + EXPECT_TRUE(released[0]); + + video_stream_encoder_->Stop(); + + // The second encoder should also be released after Stop(). + EXPECT_TRUE(released[1]); +} + TEST_F(VideoStreamEncoderTest, BitrateLimitsChangeReconfigureRateAllocator) { video_stream_encoder_->OnBitrateUpdatedAndWaitForManagedResources( kTargetBitrate, kTargetBitrate, 0, 0, 0);