From b4d09df6c20da1cb9ad7cdccf7299daddd60e6a0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Erik=20Spr=C3=A5ng?= <sprang@webrtc.org>
Date: Thu, 28 Nov 2024 15:05:19 +0100
Subject: [PATCH] Fix bug that can cause invalid reset of corruption detection
 state.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

`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}
---
 video/video_stream_encoder.cc          | 12 +++----
 video/video_stream_encoder_unittest.cc | 44 ++++++++++++++++++++++++++
 2 files changed, 50 insertions(+), 6 deletions(-)

diff --git a/video/video_stream_encoder.cc b/video/video_stream_encoder.cc
index c62a3e1c90..c19e9ad187 100644
--- a/video/video_stream_encoder.cc
+++ b/video/video_stream_encoder.cc
@@ -943,12 +943,6 @@ void VideoStreamEncoder::ConfigureEncoder(VideoEncoderConfig config,
     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 @@ void VideoStreamEncoder::ReconfigureEncoder() {
       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 @@ void VideoStreamEncoder::ReleaseEncoder() {
   }
   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 08ae5cae95..07474fc069 100644
--- a/video/video_stream_encoder_unittest.cc
+++ b/video/video_stream_encoder_unittest.cc
@@ -1525,6 +1525,11 @@ class VideoStreamEncoderTest : public ::testing::Test {
       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 @@ TEST_F(VideoStreamEncoderTest,
   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;