Fix bug that can cause invalid reset of corruption detection state.
`VideoStreamEncoder` should not recreate the
`FrameInstrumentationGenerator` instace unless the encoder is actually
released. Otherwise it will restart and expect a keyframe the encoder
will likely not produce for a while.
Bug: webrtc:358039777
Change-Id: I111149d5e9b632df9eeb88bbbe8a07969c3e3f1d
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/369740
Auto-Submit: Erik Språng <sprang@webrtc.org>
Reviewed-by: Sergey Silkin <ssilkin@webrtc.org>
Commit-Queue: Erik Språng <sprang@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#43468}
diff --git a/video/video_stream_encoder.cc b/video/video_stream_encoder.cc
index c62a3e1..c19e9ad 100644
--- a/video/video_stream_encoder.cc
+++ b/video/video_stream_encoder.cc
@@ -943,12 +943,6 @@
max_data_payload_length_ = max_data_payload_length;
pending_encoder_reconfiguration_ = true;
- if (settings_.enable_frame_instrumentation_generator) {
- frame_instrumentation_generator_ =
- std::make_unique<FrameInstrumentationGenerator>(
- encoder_config_.codec_type);
- }
-
// Reconfigure the encoder now if the frame resolution is known.
// Otherwise, the reconfiguration is deferred until the next frame to
// minimize the number of reconfigurations. The codec configuration
@@ -1314,6 +1308,11 @@
next_frame_types_.resize(
std::max(static_cast<int>(codec.numberOfSimulcastStreams), 1),
VideoFrameType::kVideoFrameKey);
+ if (settings_.enable_frame_instrumentation_generator) {
+ frame_instrumentation_generator_ =
+ std::make_unique<FrameInstrumentationGenerator>(
+ encoder_config_.codec_type);
+ }
}
frame_encode_metadata_writer_.Reset();
@@ -2495,6 +2494,7 @@
}
encoder_->Release();
encoder_initialized_ = false;
+ frame_instrumentation_generator_ = nullptr;
TRACE_EVENT0("webrtc", "VCMGenericEncoder::Release");
}
diff --git a/video/video_stream_encoder_unittest.cc b/video/video_stream_encoder_unittest.cc
index 08ae5ca..07474fc 100644
--- a/video/video_stream_encoder_unittest.cc
+++ b/video/video_stream_encoder_unittest.cc
@@ -1525,6 +1525,11 @@
return last_frame_instrumentation_data_;
}
+ void ResetLastFrameInstrumentationData() {
+ MutexLock lock(&mutex_);
+ last_frame_instrumentation_data_.reset();
+ }
+
private:
Result OnEncodedImage(
const EncodedImage& encoded_image,
@@ -1776,6 +1781,45 @@
video_stream_encoder_->Stop();
}
+TEST_F(VideoStreamEncoderTest,
+ FrameInstrumentationGeneratorNotResetOnConfigurationUnlessEncoderIsToo) {
+ // Enable frame instrumentation generator and produce the first keyframe.
+ video_send_config_.encoder_settings.enable_frame_instrumentation_generator =
+ true;
+ ConfigureEncoder(video_encoder_config_.Copy());
+ video_stream_encoder_->OnBitrateUpdatedAndWaitForManagedResources(
+ kTargetBitrate, kTargetBitrate, kTargetBitrate, 0, 0, 0);
+
+ // We need a QP for the encoded frame.
+ fake_encoder_.SetEncodedImageData(EncodedImageBuffer::Create(
+ kCodedFrameVp8Qp25, sizeof(kCodedFrameVp8Qp25)));
+ video_source_.IncomingCapturedFrame(
+ CreateFrame(1, codec_width_, codec_height_));
+ WaitForEncodedFrame(1);
+
+ EXPECT_TRUE(sink_.GetLastFrameInstrumentationData().has_value());
+ sink_.ResetLastFrameInstrumentationData();
+
+ // Apply the same configuration again. Encoder should not be reinitilized.
+ video_stream_encoder_->ConfigureEncoder(video_encoder_config_.Copy(),
+ kMaxPayloadLength, nullptr);
+
+ // Insert delta frames until a frame instrumentation should definitely have
+ // been sent.
+ for (int i = 1; i < 40; ++i) {
+ int timestamp = 1 + (33 * i);
+ fake_encoder_.SetEncodedImageData(EncodedImageBuffer::Create(
+ kCodedFrameVp8Qp25, sizeof(kCodedFrameVp8Qp25)));
+ video_source_.IncomingCapturedFrame(
+ CreateFrame(timestamp, codec_width_, codec_height_));
+ WaitForEncodedFrame(timestamp);
+ }
+
+ EXPECT_TRUE(sink_.GetLastFrameInstrumentationData().has_value());
+
+ video_stream_encoder_->Stop();
+}
+
TEST_F(VideoStreamEncoderTest, DropsFramesBeforeFirstOnBitrateUpdated) {
// Dropped since no target bitrate has been set.
rtc::Event frame_destroyed_event;
