Clean up decoders when stopping video receive stream.

This updates VideoReceiveStream2::Stop() to symmetrically tear down
state that's built up in VideoReceiveStream2::Start().

Bug: webrtc:11993, webrtc:14486
Change-Id: I41f4feea5584e5baaeed2143432136f8b9761321
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/272537
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#38244}

api/test/mock_video_decoder.h

@@ -11,6 +11,8 @@
 #ifndef API_TEST_MOCK_VIDEO_DECODER_H_
 #define API_TEST_MOCK_VIDEO_DECODER_H_
 
+#include <utility>
+
 #include "api/video_codecs/video_decoder.h"
 #include "test/gmock.h"
 
@@ -43,6 +45,8 @@ class MockVideoDecoder : public VideoDecoder {
     ON_CALL(*this, Configure).WillByDefault(testing::Return(true));
   }
 
+  ~MockVideoDecoder() override { Destruct(); }
+
   MOCK_METHOD(bool, Configure, (const Settings& settings), (override));
   MOCK_METHOD(int32_t,
               Decode,
@@ -55,6 +59,10 @@ class MockVideoDecoder : public VideoDecoder {
               (DecodedImageCallback * callback),
               (override));
   MOCK_METHOD(int32_t, Release, (), (override));
+
+  // Special utility method that allows a test to monitor/verify when
+  // destruction of the decoder instance occurs.
+  MOCK_METHOD(void, Destruct, (), ());
 };
 
 }  // namespace webrtc

api/video_codecs/test/video_decoder_factory_template_tests.cc

@@ -37,7 +37,9 @@ struct FooDecoderTemplateAdapter {
 
   static std::unique_ptr<VideoDecoder> CreateDecoder(
       const SdpVideoFormat& format) {
-    return std::make_unique<testing::StrictMock<MockVideoDecoder>>();
+    auto decoder = std::make_unique<testing::StrictMock<MockVideoDecoder>>();
+    EXPECT_CALL(*decoder, Destruct);
+    return decoder;
   }
 };
 
@@ -48,7 +50,9 @@ struct BarDecoderTemplateAdapter {
 
   static std::unique_ptr<VideoDecoder> CreateDecoder(
       const SdpVideoFormat& format) {
-    return std::make_unique<testing::StrictMock<MockVideoDecoder>>();
+    auto decoder = std::make_unique<testing::StrictMock<MockVideoDecoder>>();
+    EXPECT_CALL(*decoder, Destruct);
+    return decoder;
   }
 };
 

modules/video_coding/BUILD.gn

@@ -1126,6 +1126,7 @@ if (rtc_include_tests) {
       "codecs/vp8/libvpx_vp8_simulcast_test.cc",
       "codecs/vp8/screenshare_layers_unittest.cc",
       "codecs/vp9/svc_config_unittest.cc",
+      "decoder_database_unittest.cc",
       "decoding_state_unittest.cc",
       "fec_controller_unittest.cc",
       "frame_buffer2_unittest.cc",
@@ -1157,6 +1158,7 @@ if (rtc_include_tests) {
       "utility/simulcast_rate_allocator_unittest.cc",
       "utility/vp9_uncompressed_header_parser_unittest.cc",
       "video_codec_initializer_unittest.cc",
+      "video_receiver2_unittest.cc",
       "video_receiver_unittest.cc",
     ]
     if (rtc_use_h264) {

modules/video_coding/decoder_database.cc

@@ -47,14 +47,15 @@ void VCMDecoderDataBase::RegisterExternalDecoder(
   RTC_DCHECK_RUN_ON(&decoder_sequence_checker_);
   // If payload value already exists, erase old and insert new.
   DeregisterExternalDecoder(payload_type);
-  decoders_[payload_type] = external_decoder;
+  if (external_decoder) {
+    decoders_[payload_type] = external_decoder;
+  }
 }
 
 bool VCMDecoderDataBase::IsExternalDecoderRegistered(
     uint8_t payload_type) const {
   RTC_DCHECK_RUN_ON(&decoder_sequence_checker_);
-  return payload_type == current_payload_type_ ||
-         decoders_.find(payload_type) != decoders_.end();
+  return decoders_.find(payload_type) != decoders_.end();
 }
 
 void VCMDecoderDataBase::RegisterReceiveCodec(
@@ -78,6 +79,11 @@ bool VCMDecoderDataBase::DeregisterReceiveCodec(uint8_t payload_type) {
   return true;
 }
 
+void VCMDecoderDataBase::DeregisterReceiveCodecs() {
+  current_payload_type_ = absl::nullopt;
+  decoder_settings_.clear();
+}
+
 VCMGenericDecoder* VCMDecoderDataBase::GetDecoder(
     const VCMEncodedFrame& frame,
     VCMDecodedFrameCallback* decoded_frame_callback) {
@@ -112,8 +118,8 @@ VCMGenericDecoder* VCMDecoderDataBase::GetDecoder(
 
 void VCMDecoderDataBase::CreateAndInitDecoder(const VCMEncodedFrame& frame) {
   uint8_t payload_type = frame.PayloadType();
-  RTC_LOG(LS_INFO) << "Initializing decoder with payload type '"
-                   << int{payload_type} << "'.";
+  RTC_DLOG(LS_INFO) << "Initializing decoder with payload type '"
+                    << int{payload_type} << "'.";
   auto decoder_item = decoder_settings_.find(payload_type);
   if (decoder_item == decoder_settings_.end()) {
     RTC_LOG(LS_ERROR) << "Can't find a decoder associated with payload type: "

modules/video_coding/decoder_database.h

@@ -40,6 +40,7 @@ class VCMDecoderDataBase {
   void RegisterReceiveCodec(uint8_t payload_type,
                             const VideoDecoder::Settings& settings);
   bool DeregisterReceiveCodec(uint8_t payload_type);
+  void DeregisterReceiveCodecs();
 
   // Returns a decoder specified by frame.PayloadType. The decoded frame
   // callback of the decoder is set to `decoded_frame_callback`. If no such

modules/video_coding/decoder_database_unittest.cc

@@ -0,0 +1,75 @@
+/*
+ *  Copyright (c) 2022 The WebRTC project authors. All Rights Reserved.
+ *
+ *  Use of this source code is governed by a BSD-style license
+ *  that can be found in the LICENSE file in the root of the source
+ *  tree. An additional intellectual property rights grant can be found
+ *  in the file PATENTS.  All contributing project authors may
+ *  be found in the AUTHORS file in the root of the source tree.
+ */
+
+#include "modules/video_coding/decoder_database.h"
+
+#include "api/test/mock_video_decoder.h"
+#include "test/gmock.h"
+#include "test/gtest.h"
+
+namespace webrtc {
+namespace {
+
+using ::testing::NiceMock;
+
+// Test registering and unregistering an external decoder instance.
+TEST(VCMDecoderDataBaseTest, RegisterExternalDecoder) {
+  VCMDecoderDataBase db;
+  constexpr int kPayloadType = 1;
+  ASSERT_FALSE(db.IsExternalDecoderRegistered(kPayloadType));
+
+  NiceMock<MockVideoDecoder> decoder;
+  db.RegisterExternalDecoder(kPayloadType, &decoder);
+  EXPECT_TRUE(db.IsExternalDecoderRegistered(kPayloadType));
+  EXPECT_EQ(db.DeregisterExternalDecoder(kPayloadType), &decoder);
+  EXPECT_FALSE(db.IsExternalDecoderRegistered(kPayloadType));
+}
+
+TEST(VCMDecoderDataBaseTest, RegisterReceiveCodec) {
+  VCMDecoderDataBase db;
+  constexpr int kPayloadType = 1;
+  ASSERT_FALSE(db.DeregisterReceiveCodec(kPayloadType));
+
+  VideoDecoder::Settings settings;
+  settings.set_codec_type(kVideoCodecVP8);
+  settings.set_max_render_resolution({10, 10});
+  settings.set_number_of_cores(4);
+  db.RegisterReceiveCodec(kPayloadType, settings);
+
+  EXPECT_TRUE(db.DeregisterReceiveCodec(kPayloadType));
+}
+
+TEST(VCMDecoderDataBaseTest, DeregisterReceiveCodecs) {
+  VCMDecoderDataBase db;
+  constexpr int kPayloadType1 = 1;
+  constexpr int kPayloadType2 = 2;
+  ASSERT_FALSE(db.DeregisterReceiveCodec(kPayloadType1));
+  ASSERT_FALSE(db.DeregisterReceiveCodec(kPayloadType2));
+
+  VideoDecoder::Settings settings1;
+  settings1.set_codec_type(kVideoCodecVP8);
+  settings1.set_max_render_resolution({10, 10});
+  settings1.set_number_of_cores(4);
+
+  VideoDecoder::Settings settings2 = settings1;
+  settings2.set_codec_type(kVideoCodecVP9);
+
+  db.RegisterReceiveCodec(kPayloadType1, settings1);
+  db.RegisterReceiveCodec(kPayloadType2, settings2);
+
+  db.DeregisterReceiveCodecs();
+
+  // All receive codecs must have been removed.
+  EXPECT_FALSE(db.DeregisterReceiveCodec(kPayloadType1));
+  EXPECT_FALSE(db.DeregisterReceiveCodec(kPayloadType2));
+}
+
+}  // namespace
+}  // namespace webrtc

modules/video_coding/packet_buffer.cc

@@ -167,6 +167,10 @@ void PacketBuffer::ForceSpsPpsIdrIsH264Keyframe() {
   sps_pps_idr_is_h264_keyframe_ = true;
 }
 
+void PacketBuffer::ResetSpsPpsIdrIsH264Keyframe() {
+  sps_pps_idr_is_h264_keyframe_ = false;
+}
+
 void PacketBuffer::ClearInternal() {
   for (auto& entry : buffer_) {
     entry = nullptr;

modules/video_coding/packet_buffer.h

@@ -82,6 +82,7 @@ class PacketBuffer {
   void Clear();
 
   void ForceSpsPpsIdrIsH264Keyframe();
+  void ResetSpsPpsIdrIsH264Keyframe();
 
  private:
   void ClearInternal();

modules/video_coding/video_receiver2.cc

@@ -102,4 +102,14 @@ void VideoReceiver2::RegisterReceiveCodec(
   codec_database_.RegisterReceiveCodec(payload_type, settings);
 }
 
+void VideoReceiver2::DeregisterReceiveCodec(uint8_t payload_type) {
+  RTC_DCHECK_RUN_ON(&construction_sequence_checker_);
+  codec_database_.DeregisterReceiveCodec(payload_type);
+}
+
+void VideoReceiver2::DeregisterReceiveCodecs() {
+  RTC_DCHECK_RUN_ON(&construction_sequence_checker_);
+  codec_database_.DeregisterReceiveCodecs();
+}
+
 }  // namespace webrtc

modules/video_coding/video_receiver2.h

@@ -21,6 +21,7 @@
 #include "modules/video_coding/encoded_frame.h"
 #include "modules/video_coding/generic_decoder.h"
 #include "modules/video_coding/timing/timing.h"
+#include "rtc_base/system/no_unique_address.h"
 #include "system_wrappers/include/clock.h"
 
 namespace webrtc {
@@ -39,6 +40,8 @@ class VideoReceiver2 {
 
   void RegisterReceiveCodec(uint8_t payload_type,
                             const VideoDecoder::Settings& decoder_settings);
+  void DeregisterReceiveCodec(uint8_t payload_type);
+  void DeregisterReceiveCodecs();
 
   void RegisterExternalDecoder(std::unique_ptr<VideoDecoder> decoder,
                                uint8_t payload_type);
@@ -49,8 +52,8 @@ class VideoReceiver2 {
   int32_t Decode(const VCMEncodedFrame* frame);
 
  private:
-  SequenceChecker construction_sequence_checker_;
-  SequenceChecker decoder_sequence_checker_;
+  RTC_NO_UNIQUE_ADDRESS SequenceChecker construction_sequence_checker_;
+  RTC_NO_UNIQUE_ADDRESS SequenceChecker decoder_sequence_checker_;
   Clock* const clock_;
   VCMDecodedFrameCallback decoded_frame_callback_;
   // Holds/owns the decoder instances that are registered via

modules/video_coding/video_receiver2_unittest.cc

@@ -0,0 +1,142 @@
+/*
+ *  Copyright (c) 2022 The WebRTC project authors. All Rights Reserved.
+ *
+ *  Use of this source code is governed by a BSD-style license
+ *  that can be found in the LICENSE file in the root of the source
+ *  tree. An additional intellectual property rights grant can be found
+ *  in the file PATENTS.  All contributing project authors may
+ *  be found in the AUTHORS file in the root of the source tree.
+ */
+
+#include "modules/video_coding/video_receiver2.h"
+
+#include <memory>
+#include <utility>
+
+#include "api/test/mock_video_decoder.h"
+#include "api/units/timestamp.h"
+#include "api/video/encoded_frame.h"
+#include "common_video/test/utilities.h"
+#include "modules/video_coding/decoder_database.h"
+#include "modules/video_coding/timing/timing.h"
+#include "system_wrappers/include/clock.h"
+#include "test/gmock.h"
+#include "test/gtest.h"
+#include "test/scoped_key_value_config.h"
+
+namespace webrtc {
+namespace {
+
+using ::testing::_;
+using ::testing::NiceMock;
+using ::testing::Return;
+
+class MockVCMReceiveCallback : public VCMReceiveCallback {
+ public:
+  MockVCMReceiveCallback() = default;
+
+  MOCK_METHOD(
+      int32_t,
+      FrameToRender,
+      (VideoFrame&, absl::optional<uint8_t>, TimeDelta, VideoContentType),
+      (override));
+  MOCK_METHOD(void, OnIncomingPayloadType, (int), (override));
+  MOCK_METHOD(void, OnDecoderImplementationName, (const char*), (override));
+};
+
+class TestEncodedFrame : public EncodedFrame {
+ public:
+  explicit TestEncodedFrame(int payload_type) {
+    _payloadType = payload_type;
+    SetPacketInfos(CreatePacketInfos(3));
+  }
+
+  void SetReceivedTime(webrtc::Timestamp received_time) {
+    received_time_ = received_time;
+  }
+
+  int64_t ReceivedTime() const override { return received_time_.ms(); }
+
+  int64_t RenderTime() const override { return _renderTimeMs; }
+
+ private:
+  webrtc::Timestamp received_time_ = webrtc::Timestamp::Millis(0);
+};
+
+class VideoReceiver2Test : public ::testing::Test {
+ protected:
+  VideoReceiver2Test() {
+    receiver_.RegisterReceiveCallback(&receive_callback_);
+  }
+
+  void RegisterReceiveCodecSettings(
+      int payload_type,
+      VideoCodecType codec_type = kVideoCodecVP8) {
+    VideoDecoder::Settings settings;
+    settings.set_codec_type(codec_type);
+    settings.set_max_render_resolution({10, 10});
+    settings.set_number_of_cores(4);
+    receiver_.RegisterReceiveCodec(payload_type, settings);
+  }
+
+  test::ScopedKeyValueConfig field_trials_;
+  SimulatedClock clock_{Timestamp::Millis(1337)};
+  VCMTiming timing_{&clock_, field_trials_};
+  NiceMock<MockVCMReceiveCallback> receive_callback_;
+  VideoReceiver2 receiver_{&clock_, &timing_, field_trials_};
+};
+
+TEST_F(VideoReceiver2Test, RegisterExternalDecoder) {
+  constexpr int kPayloadType = 1;
+  ASSERT_FALSE(receiver_.IsExternalDecoderRegistered(kPayloadType));
+
+  // Register a decoder, check for correctness, then unregister and check again.
+  auto decoder = std::make_unique<NiceMock<MockVideoDecoder>>();
+  bool decoder_deleted = false;
+  EXPECT_CALL(*decoder, Destruct).WillOnce([&decoder_deleted] {
+    decoder_deleted = true;
+  });
+  receiver_.RegisterExternalDecoder(std::move(decoder), kPayloadType);
+  EXPECT_TRUE(receiver_.IsExternalDecoderRegistered(kPayloadType));
+  receiver_.RegisterExternalDecoder(nullptr, kPayloadType);
+  EXPECT_TRUE(decoder_deleted);
+  EXPECT_FALSE(receiver_.IsExternalDecoderRegistered(kPayloadType));
+}
+
+TEST_F(VideoReceiver2Test, RegisterReceiveCodecs) {
+  constexpr int kPayloadType = 1;
+
+  RegisterReceiveCodecSettings(kPayloadType);
+
+  TestEncodedFrame frame(kPayloadType);
+
+  // A decoder has not been registered yet, so an attempt to decode should fail.
+  EXPECT_EQ(receiver_.Decode(&frame), VCM_NO_CODEC_REGISTERED);
+
+  // Register a decoder that will accept the Decode operation.
+  auto decoder = std::make_unique<NiceMock<MockVideoDecoder>>();
+  EXPECT_CALL(*decoder, RegisterDecodeCompleteCallback)
+      .WillOnce(Return(WEBRTC_VIDEO_CODEC_OK));
+  EXPECT_CALL(*decoder, Decode).WillOnce(Return(WEBRTC_VIDEO_CODEC_OK));
+  EXPECT_CALL(*decoder, Release).WillOnce(Return(WEBRTC_VIDEO_CODEC_OK));
+
+  // Register the decoder. Note that this moves ownership of the mock object
+  // to the `receiver_`.
+  receiver_.RegisterExternalDecoder(std::move(decoder), kPayloadType);
+  EXPECT_TRUE(receiver_.IsExternalDecoderRegistered(kPayloadType));
+
+  EXPECT_CALL(receive_callback_, OnIncomingPayloadType(kPayloadType));
+  EXPECT_CALL(receive_callback_, OnDecoderImplementationName);
+
+  // Call `Decode`. This triggers the above call expectations.
+  EXPECT_EQ(receiver_.Decode(&frame), VCM_OK);
+
+  // Unregister the decoder and verify.
+  receiver_.RegisterExternalDecoder(nullptr, kPayloadType);
+  EXPECT_FALSE(receiver_.IsExternalDecoderRegistered(kPayloadType));
+
+  receiver_.DeregisterReceiveCodec(kPayloadType);
+}
+
+}  // namespace
+}  // namespace webrtc

video/rtp_video_stream_receiver2.cc

@@ -359,7 +359,7 @@ void RtpVideoStreamReceiver2::AddReceiveCodec(
     const std::map<std::string, std::string>& codec_params,
     bool raw_payload) {
   RTC_DCHECK_RUN_ON(&packet_sequence_checker_);
-  if (codec_params.count(cricket::kH264FmtpSpsPpsIdrInKeyframe) ||
+  if (codec_params.count(cricket::kH264FmtpSpsPpsIdrInKeyframe) > 0 ||
       field_trials_.IsEnabled("WebRTC-SpsPpsIdrIsH264Keyframe")) {
     packet_buffer_.ForceSpsPpsIdrIsH264Keyframe();
   }
@@ -369,6 +369,41 @@ void RtpVideoStreamReceiver2::AddReceiveCodec(
   pt_codec_params_.emplace(payload_type, codec_params);
 }
 
+void RtpVideoStreamReceiver2::RemoveReceiveCodec(uint8_t payload_type) {
+  RTC_DCHECK_RUN_ON(&packet_sequence_checker_);
+  auto codec_params_it = pt_codec_params_.find(payload_type);
+  if (codec_params_it == pt_codec_params_.end())
+    return;
+
+  const bool sps_pps_idr_in_key_frame =
+      codec_params_it->second.count(cricket::kH264FmtpSpsPpsIdrInKeyframe) > 0;
+
+  pt_codec_params_.erase(codec_params_it);
+  payload_type_map_.erase(payload_type);
+
+  if (sps_pps_idr_in_key_frame) {
+    bool reset_setting = true;
+    for (auto& [unused, codec_params] : pt_codec_params_) {
+      if (codec_params.count(cricket::kH264FmtpSpsPpsIdrInKeyframe) > 0) {
+        reset_setting = false;
+        break;
+      }
+    }
+
+    if (reset_setting) {
+      packet_buffer_.ResetSpsPpsIdrIsH264Keyframe();
+    }
+  }
+}
+
+void RtpVideoStreamReceiver2::RemoveReceiveCodecs() {
+  RTC_DCHECK_RUN_ON(&packet_sequence_checker_);
+
+  pt_codec_params_.clear();
+  payload_type_map_.clear();
+  packet_buffer_.ResetSpsPpsIdrIsH264Keyframe();
+}
+
 absl::optional<Syncable::Info> RtpVideoStreamReceiver2::GetSyncInfo() const {
   RTC_DCHECK_RUN_ON(&packet_sequence_checker_);
   Syncable::Info info;
@@ -755,7 +790,7 @@ void RtpVideoStreamReceiver2::OnInsertedPacket(
     RTC_DCHECK_EQ(frame_boundary, packet->is_first_packet_in_frame());
     int64_t unwrapped_rtp_seq_num =
         rtp_seq_num_unwrapper_.Unwrap(packet->seq_num);
-    RTC_DCHECK(packet_infos_.count(unwrapped_rtp_seq_num) > 0);
+    RTC_DCHECK_GT(packet_infos_.count(unwrapped_rtp_seq_num), 0);
     RtpPacketInfo& packet_info = packet_infos_[unwrapped_rtp_seq_num];
     if (packet->is_first_packet_in_frame()) {
       first_packet = packet.get();

video/rtp_video_stream_receiver2.h

@@ -104,6 +104,10 @@ class RtpVideoStreamReceiver2 : public LossNotificationSender,
                        VideoCodecType video_codec,
                        const std::map<std::string, std::string>& codec_params,
                        bool raw_payload);
+  void RemoveReceiveCodec(uint8_t payload_type);
+
+  // Clears state for all receive codecs added via `AddReceiveCodec`.
+  void RemoveReceiveCodecs();
 
   void StartReceive();
   void StopReceive();

video/video_receive_stream2.cc

@@ -419,19 +419,19 @@ void VideoReceiveStream2::Start() {
 
 void VideoReceiveStream2::Stop() {
   RTC_DCHECK_RUN_ON(&worker_sequence_checker_);
-  {
-    // TODO(bugs.webrtc.org/11993): Make this call on the network thread.
-    // Also call `GetUniqueFramesSeen()` at the same time (since it's a counter
-    // that's updated on the network thread).
-    RTC_DCHECK_RUN_ON(&packet_sequence_checker_);
-    rtp_video_stream_receiver_.StopReceive();
-  }
+
+  // TODO(bugs.webrtc.org/11993): Make this call on the network thread.
+  // Also call `GetUniqueFramesSeen()` at the same time (since it's a counter
+  // that's updated on the network thread).
+  RTC_DCHECK_RUN_ON(&packet_sequence_checker_);
+  rtp_video_stream_receiver_.StopReceive();
 
   stats_proxy_.OnUniqueFramesCounted(
       rtp_video_stream_receiver_.GetUniqueFramesSeen());
 
   buffer_->Stop();
   call_stats_->DeregisterStatsObserver(this);
+
   if (decoder_running_) {
     rtc::Event done;
     decode_queue_.PostTask([this, &done] {
@@ -453,6 +453,11 @@ void VideoReceiveStream2::Stop() {
     UpdateHistograms();
   }
 
+  // TODO(bugs.webrtc.org/11993): Make these calls on the network thread.
+  RTC_DCHECK_RUN_ON(&packet_sequence_checker_);
+  rtp_video_stream_receiver_.RemoveReceiveCodecs();
+  video_receiver_.DeregisterReceiveCodecs();
+
   video_stream_decoder_.reset();
   incoming_video_stream_.reset();
   transport_adapter_.Disable();