Move ownership of decoders to VCMDecoderDatabase

Bug: webrtc:14497
Change-Id: Idf719a1d1605f19fcf46eff7990c61144f2b9e3b
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/277401
Reviewed-by: Philip Eliasson <philipel@webrtc.org>
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#38251}

modules/video_coding/decoder_database.cc

@@ -10,6 +10,9 @@
 
 #include "modules/video_coding/decoder_database.h"
 
+#include <memory>
+#include <utility>
+
 #include "rtc_base/checks.h"
 #include "rtc_base/logging.h"
 
@@ -19,36 +22,34 @@ VCMDecoderDatabase::VCMDecoderDatabase() {
   decoder_sequence_checker_.Detach();
 }
 
-VideoDecoder* VCMDecoderDatabase::DeregisterExternalDecoder(
-    uint8_t payload_type) {
+void VCMDecoderDatabase::DeregisterExternalDecoder(uint8_t payload_type) {
   RTC_DCHECK_RUN_ON(&decoder_sequence_checker_);
   auto it = decoders_.find(payload_type);
   if (it == decoders_.end()) {
-    return nullptr;
+    return;
   }
 
   // We can't use payload_type to check if the decoder is currently in use,
   // because payload type may be out of date (e.g. before we decode the first
   // frame after RegisterReceiveCodec).
-  if (current_decoder_ && current_decoder_->IsSameDecoder(it->second)) {
+  if (current_decoder_ && current_decoder_->IsSameDecoder(it->second.get())) {
     // Release it if it was registered and in use.
     current_decoder_ = absl::nullopt;
   }
-  VideoDecoder* ret = it->second;
   decoders_.erase(it);
-  return ret;
 }
 
 // Add the external decoder object to the list of external decoders.
 // Won't be registered as a receive codec until RegisterReceiveCodec is called.
 void VCMDecoderDatabase::RegisterExternalDecoder(
     uint8_t payload_type,
-    VideoDecoder* external_decoder) {
+    std::unique_ptr<VideoDecoder> external_decoder) {
   RTC_DCHECK_RUN_ON(&decoder_sequence_checker_);
   // If payload value already exists, erase old and insert new.
   DeregisterExternalDecoder(payload_type);
   if (external_decoder) {
-    decoders_[payload_type] = external_decoder;
+    decoders_.emplace(
+        std::make_pair(payload_type, std::move(external_decoder)));
   }
 }
 
@@ -131,7 +132,7 @@ void VCMDecoderDatabase::CreateAndInitDecoder(const VCMEncodedFrame& frame) {
     RTC_LOG(LS_ERROR) << "No decoder of this type exists.";
     return;
   }
-  current_decoder_.emplace(external_dec_item->second);
+  current_decoder_.emplace(external_dec_item->second.get());
 
   // Copy over input resolutions to prevent codec reinitialization due to
   // the first frame being of a different resolution than the database values.

modules/video_coding/decoder_database.h

@@ -14,6 +14,7 @@
 #include <stdint.h>
 
 #include <map>
+#include <memory>
 
 #include "absl/types/optional.h"
 #include "api/sequence_checker.h"
@@ -32,9 +33,9 @@ class VCMDecoderDatabase {
 
   // Returns a pointer to the previously registered decoder or nullptr if none
   // was registered for the `payload_type`.
-  VideoDecoder* DeregisterExternalDecoder(uint8_t payload_type);
+  void DeregisterExternalDecoder(uint8_t payload_type);
   void RegisterExternalDecoder(uint8_t payload_type,
-                               VideoDecoder* external_decoder);
+                               std::unique_ptr<VideoDecoder> external_decoder);
   bool IsExternalDecoderRegistered(uint8_t payload_type) const;
 
   void RegisterReceiveCodec(uint8_t payload_type,
@@ -63,7 +64,7 @@ class VCMDecoderDatabase {
   // Initialization paramaters for decoders keyed by payload type.
   std::map<uint8_t, VideoDecoder::Settings> decoder_settings_;
   // Decoders keyed by payload type.
-  std::map<uint8_t, VideoDecoder*> decoders_
+  std::map<uint8_t, std::unique_ptr<VideoDecoder>> decoders_
       RTC_GUARDED_BY(decoder_sequence_checker_);
 };
 

modules/video_coding/decoder_database_unittest.cc

@@ -10,6 +10,9 @@
 
 #include "modules/video_coding/decoder_database.h"
 
+#include <memory>
+#include <utility>
+
 #include "api/test/mock_video_decoder.h"
 #include "test/gmock.h"
 #include "test/gtest.h"
@@ -25,10 +28,16 @@ TEST(VCMDecoderDatabaseTest, RegisterExternalDecoder) {
   constexpr int kPayloadType = 1;
   ASSERT_FALSE(db.IsExternalDecoderRegistered(kPayloadType));
 
-  NiceMock<MockVideoDecoder> decoder;
-  db.RegisterExternalDecoder(kPayloadType, &decoder);
+  auto decoder = std::make_unique<NiceMock<MockVideoDecoder>>();
+  bool decoder_deleted = false;
+  EXPECT_CALL(*decoder, Destruct).WillOnce([&decoder_deleted] {
+    decoder_deleted = true;
+  });
+
+  db.RegisterExternalDecoder(kPayloadType, std::move(decoder));
   EXPECT_TRUE(db.IsExternalDecoderRegistered(kPayloadType));
-  EXPECT_EQ(db.DeregisterExternalDecoder(kPayloadType), &decoder);
+  db.DeregisterExternalDecoder(kPayloadType);
+  EXPECT_TRUE(decoder_deleted);
   EXPECT_FALSE(db.IsExternalDecoderRegistered(kPayloadType));
 }
 

modules/video_coding/video_receiver2.cc

@@ -62,16 +62,9 @@ void VideoReceiver2::RegisterExternalDecoder(
 
   if (decoder) {
     RTC_DCHECK(!codec_database_.IsExternalDecoderRegistered(payload_type));
-    codec_database_.RegisterExternalDecoder(payload_type, decoder.get());
-    video_decoders_.push_back(std::move(decoder));
+    codec_database_.RegisterExternalDecoder(payload_type, std::move(decoder));
   } else {
-    VideoDecoder* registered =
     codec_database_.DeregisterExternalDecoder(payload_type);
-    if (registered) {
-      video_decoders_.erase(absl::c_find_if(
-          video_decoders_,
-          [registered](const auto& d) { return d.get() == registered; }));
-    }
   }
 }
 

modules/video_coding/video_receiver2.h

@@ -56,10 +56,6 @@ class VideoReceiver2 {
   RTC_NO_UNIQUE_ADDRESS SequenceChecker decoder_sequence_checker_;
   Clock* const clock_;
   VCMDecodedFrameCallback decoded_frame_callback_;
-  // Holds/owns the decoder instances that are registered via
-  // `RegisterExternalDecoder` and referenced by `codec_database_`.
-  std::vector<std::unique_ptr<VideoDecoder>> video_decoders_;
-
   // Callbacks are set before the decoder thread starts.
   // Once the decoder thread has been started, usage of `_codecDataBase` moves
   // over to the decoder thread.