Destroy existing encoder instance before creating a new one.
Before this change, an attempt to recreate video encoder would fail if
video encoder factory supports only single instance of an encoder.
Added tracking of max number of existed simultaneously encoder
instances to VideoEncoderProxyFactory.
Bug: webrtc:10776
Change-Id: I317cbdf1af94dfb4c72bf99c5cd4ce7b454188fa
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/144044
Commit-Queue: Sergey Silkin <ssilkin@webrtc.org>
Reviewed-by: Åsa Persson <asapersson@webrtc.org>
Reviewed-by: Niels Moller <nisse@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#28457}
diff --git a/test/video_encoder_proxy_factory.h b/test/video_encoder_proxy_factory.h
index d09e6ff..ac19e52 100644
--- a/test/video_encoder_proxy_factory.h
+++ b/test/video_encoder_proxy_factory.h
@@ -30,7 +30,10 @@
// a proxy for the same encoder, typically an instance of FakeEncoder.
class VideoEncoderProxyFactory final : public VideoEncoderFactory {
public:
- explicit VideoEncoderProxyFactory(VideoEncoder* encoder) : encoder_(encoder) {
+ explicit VideoEncoderProxyFactory(VideoEncoder* encoder)
+ : encoder_(encoder),
+ num_simultaneous_encoder_instances_(0),
+ max_num_simultaneous_encoder_instances_(0) {
codec_info_.is_hardware_accelerated = false;
codec_info_.has_internal_source = false;
}
@@ -47,7 +50,11 @@
std::unique_ptr<VideoEncoder> CreateVideoEncoder(
const SdpVideoFormat& format) override {
- return absl::make_unique<EncoderProxy>(encoder_);
+ ++num_simultaneous_encoder_instances_;
+ max_num_simultaneous_encoder_instances_ =
+ std::max(max_num_simultaneous_encoder_instances_,
+ num_simultaneous_encoder_instances_);
+ return absl::make_unique<EncoderProxy>(encoder_, this);
}
void SetIsHardwareAccelerated(bool is_hardware_accelerated) {
@@ -57,12 +64,24 @@
codec_info_.has_internal_source = has_internal_source;
}
+ int GetMaxNumberOfSimultaneousEncoderInstances() {
+ return max_num_simultaneous_encoder_instances_;
+ }
+
private:
+ void OnDestroyVideoEncoder() {
+ RTC_CHECK_GT(num_simultaneous_encoder_instances_, 0);
+ --num_simultaneous_encoder_instances_;
+ }
+
// Wrapper class, since CreateVideoEncoder needs to surrender
// ownership to the object it returns.
class EncoderProxy final : public VideoEncoder {
public:
- explicit EncoderProxy(VideoEncoder* encoder) : encoder_(encoder) {}
+ explicit EncoderProxy(VideoEncoder* encoder,
+ VideoEncoderProxyFactory* encoder_factory)
+ : encoder_(encoder), encoder_factory_(encoder_factory) {}
+ ~EncoderProxy() { encoder_factory_->OnDestroyVideoEncoder(); }
private:
void SetFecControllerOverride(
@@ -96,10 +115,14 @@
}
VideoEncoder* const encoder_;
+ VideoEncoderProxyFactory* const encoder_factory_;
};
VideoEncoder* const encoder_;
CodecInfo codec_info_;
+
+ int num_simultaneous_encoder_instances_;
+ int max_num_simultaneous_encoder_instances_;
};
} // namespace test
diff --git a/video/video_stream_encoder.cc b/video/video_stream_encoder.cc
index 9b2f0f2..3dba91e 100644
--- a/video/video_stream_encoder.cc
+++ b/video/video_stream_encoder.cc
@@ -754,6 +754,11 @@
if (pending_encoder_creation_ || reset_required) {
ReleaseEncoder();
if (pending_encoder_creation_) {
+ // Destroy existing encoder instance before creating a new one. Otherwise
+ // attempt to create another instance will fail if encoder factory
+ // supports only single encoder instance.
+ encoder_.reset();
+
encoder_ = settings_.encoder_factory->CreateVideoEncoder(
encoder_config_.video_format);
// TODO(nisse): What to do if creating the encoder fails? Crash,
diff --git a/video/video_stream_encoder_unittest.cc b/video/video_stream_encoder_unittest.cc
index c973e87..b80acc2 100644
--- a/video/video_stream_encoder_unittest.cc
+++ b/video/video_stream_encoder_unittest.cc
@@ -1264,6 +1264,29 @@
video_stream_encoder_->Stop();
}
+TEST_F(VideoStreamEncoderTest,
+ EncoderInstanceDestroyedBeforeAnotherInstanceCreated) {
+ video_stream_encoder_->OnBitrateUpdated(
+ DataRate::bps(kTargetBitrateBps), DataRate::bps(kTargetBitrateBps), 0, 0);
+
+ // Capture a frame and wait for it to synchronize with the encoder thread.
+ video_source_.IncomingCapturedFrame(CreateFrame(1, nullptr));
+ WaitForEncodedFrame(1);
+
+ VideoEncoderConfig video_encoder_config;
+ test::FillEncoderConfiguration(kVideoCodecVP8, 1, &video_encoder_config);
+ // 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);
+ EXPECT_EQ(1, encoder_factory_.GetMaxNumberOfSimultaneousEncoderInstances());
+
+ video_stream_encoder_->Stop();
+}
+
TEST_F(VideoStreamEncoderTest, BitrateLimitsChangeReconfigureRateAllocator) {
video_stream_encoder_->OnBitrateUpdated(
DataRate::bps(kTargetBitrateBps), DataRate::bps(kTargetBitrateBps), 0, 0);
