Move decoder instance ownership to VideoReceiver2

This moves the ownership away from VideoReceiveStream2 and closer to
VCMDecoderDataBase. That facilitates unregistration (upcoming change)
without recreating receive streams.

Bug: none
Change-Id: I812175134730a0ffbf7077fd149c8489481c73d8
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/272481
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#37866}

media/engine/webrtc_video_engine.cc

@@ -3012,7 +3012,6 @@ bool WebRtcVideoChannel::WebRtcVideoReceiveStream::ReconfigureCodecs(
   }
 
   if (codec.ulpfec.red_rtx_payload_type != -1) {
-    // TODO(tommi): Look into if/when this happens in practice.
     rtx_associated_payload_types[codec.ulpfec.red_rtx_payload_type] =
         codec.ulpfec.red_payload_type;
   }

modules/video_coding/decoder_database.cc

@@ -19,13 +19,14 @@ VCMDecoderDataBase::VCMDecoderDataBase() {
   decoder_sequence_checker_.Detach();
 }
 
-bool VCMDecoderDataBase::DeregisterExternalDecoder(uint8_t payload_type) {
+VideoDecoder* VCMDecoderDataBase::DeregisterExternalDecoder(
+    uint8_t payload_type) {
   RTC_DCHECK_RUN_ON(&decoder_sequence_checker_);
   auto it = decoders_.find(payload_type);
   if (it == decoders_.end()) {
-    // Not found.
+    return nullptr;
-    return false;
   }
+
   // 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).
@@ -33,8 +34,9 @@ bool VCMDecoderDataBase::DeregisterExternalDecoder(uint8_t payload_type) {
     // Release it if it was registered and in use.
     current_decoder_ = absl::nullopt;
   }
+  VideoDecoder* ret = it->second;
   decoders_.erase(it);
-  return true;
+  return ret;
 }
 
 // Add the external decoder object to the list of external decoders.

modules/video_coding/decoder_database.h

@@ -30,7 +30,9 @@ class VCMDecoderDataBase {
   VCMDecoderDataBase& operator=(const VCMDecoderDataBase&) = delete;
   ~VCMDecoderDataBase() = default;
 
-  bool DeregisterExternalDecoder(uint8_t payload_type);
+  // 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 RegisterExternalDecoder(uint8_t payload_type,
                                VideoDecoder* external_decoder);
   bool IsExternalDecoderRegistered(uint8_t payload_type) const;

modules/video_coding/video_receiver2.cc

@@ -13,8 +13,10 @@
 #include <stddef.h>
 
 #include <cstdint>
+#include <utility>
 #include <vector>
 
+#include "absl/algorithm/container.h"
 #include "api/video_codecs/video_codec.h"
 #include "api/video_codecs/video_decoder.h"
 #include "modules/video_coding/decoder_database.h"
@@ -32,9 +34,8 @@ VideoReceiver2::VideoReceiver2(Clock* clock,
                                VCMTiming* timing,
                                const FieldTrialsView& field_trials)
     : clock_(clock),
-      timing_(timing),
+      decoded_frame_callback_(timing, clock_, field_trials),
-      decodedFrameCallback_(timing_, clock_, field_trials),
+      codec_database_() {
-      codecDataBase_() {
   decoder_sequence_checker_.Detach();
 }
 
@@ -45,29 +46,38 @@ VideoReceiver2::~VideoReceiver2() {
 // Register a receive callback. Will be called whenever there is a new frame
 // ready for rendering.
 int32_t VideoReceiver2::RegisterReceiveCallback(
-    VCMReceiveCallback* receiveCallback) {
+    VCMReceiveCallback* receive_callback) {
   RTC_DCHECK_RUN_ON(&construction_sequence_checker_);
   // This value is set before the decoder thread starts and unset after
   // the decoder thread has been stopped.
-  decodedFrameCallback_.SetUserReceiveCallback(receiveCallback);
+  decoded_frame_callback_.SetUserReceiveCallback(receive_callback);
   return VCM_OK;
 }
 
-void VideoReceiver2::RegisterExternalDecoder(VideoDecoder* externalDecoder,
+void VideoReceiver2::RegisterExternalDecoder(
-                                             uint8_t payloadType) {
+    std::unique_ptr<VideoDecoder> decoder,
+    uint8_t payload_type) {
   RTC_DCHECK_RUN_ON(&decoder_sequence_checker_);
-  RTC_DCHECK(decodedFrameCallback_.UserReceiveCallback());
+  RTC_DCHECK(decoded_frame_callback_.UserReceiveCallback());
 
-  if (externalDecoder == nullptr) {
+  if (decoder) {
-    codecDataBase_.DeregisterExternalDecoder(payloadType);
+    RTC_DCHECK(!codec_database_.IsExternalDecoderRegistered(payload_type));
+    codec_database_.RegisterExternalDecoder(payload_type, decoder.get());
+    video_decoders_.push_back(std::move(decoder));
   } else {
-    codecDataBase_.RegisterExternalDecoder(payloadType, externalDecoder);
+    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; }));
+    }
   }
 }
 
-bool VideoReceiver2::IsExternalDecoderRegistered(uint8_t payloadType) const {
+bool VideoReceiver2::IsExternalDecoderRegistered(uint8_t payload_type) const {
   RTC_DCHECK_RUN_ON(&decoder_sequence_checker_);
-  return codecDataBase_.IsExternalDecoderRegistered(payloadType);
+  return codec_database_.IsExternalDecoderRegistered(payload_type);
 }
 
 // Must be called from inside the receive side critical section.
@@ -76,7 +86,7 @@ int32_t VideoReceiver2::Decode(const VCMEncodedFrame* frame) {
   TRACE_EVENT0("webrtc", "VideoReceiver2::Decode");
   // Change decoder if payload type has changed.
   VCMGenericDecoder* decoder =
-      codecDataBase_.GetDecoder(*frame, &decodedFrameCallback_);
+      codec_database_.GetDecoder(*frame, &decoded_frame_callback_);
   if (decoder == nullptr) {
     return VCM_NO_CODEC_REGISTERED;
   }
@@ -89,7 +99,7 @@ void VideoReceiver2::RegisterReceiveCodec(
     uint8_t payload_type,
     const VideoDecoder::Settings& settings) {
   RTC_DCHECK_RUN_ON(&construction_sequence_checker_);
-  codecDataBase_.RegisterReceiveCodec(payload_type, settings);
+  codec_database_.RegisterReceiveCodec(payload_type, settings);
 }
 
 }  // namespace webrtc

modules/video_coding/video_receiver2.h

@@ -11,6 +11,9 @@
 #ifndef MODULES_VIDEO_CODING_VIDEO_RECEIVER2_H_
 #define MODULES_VIDEO_CODING_VIDEO_RECEIVER2_H_
 
+#include <memory>
+#include <vector>
+
 #include "api/field_trials_view.h"
 #include "api/sequence_checker.h"
 #include "api/video_codecs/video_decoder.h"
@@ -37,24 +40,27 @@ class VideoReceiver2 {
   void RegisterReceiveCodec(uint8_t payload_type,
                             const VideoDecoder::Settings& decoder_settings);
 
-  void RegisterExternalDecoder(VideoDecoder* externalDecoder,
+  void RegisterExternalDecoder(std::unique_ptr<VideoDecoder> decoder,
-                               uint8_t payloadType);
+                               uint8_t payload_type);
-  bool IsExternalDecoderRegistered(uint8_t payloadType) const;
-  int32_t RegisterReceiveCallback(VCMReceiveCallback* receiveCallback);
 
-  int32_t Decode(const webrtc::VCMEncodedFrame* frame);
+  bool IsExternalDecoderRegistered(uint8_t payload_type) const;
+  int32_t RegisterReceiveCallback(VCMReceiveCallback* receive_callback);
+
+  int32_t Decode(const VCMEncodedFrame* frame);
 
  private:
   SequenceChecker construction_sequence_checker_;
   SequenceChecker decoder_sequence_checker_;
   Clock* const clock_;
-  VCMTiming* timing_;
+  VCMDecodedFrameCallback decoded_frame_callback_;
-  VCMDecodedFrameCallback decodedFrameCallback_;
+  // 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.
-  VCMDecoderDataBase codecDataBase_;
+  VCMDecoderDataBase codec_database_;
 };
 
 }  // namespace webrtc

video/video_receive_stream2.cc

@@ -602,8 +602,7 @@ void VideoReceiveStream2::CreateAndRegisterExternalDecoder(
         std::move(video_decoder), FileWrapper::OpenWriteOnly(ssb.str()));
   }
 
-  video_decoders_.push_back(std::move(video_decoder));
+  video_receiver_.RegisterExternalDecoder(std::move(video_decoder),
-  video_receiver_.RegisterExternalDecoder(video_decoders_.back().get(),
                                           decoder.payload_type);
 }
 

video/video_receive_stream2.h

@@ -276,10 +276,6 @@ class VideoReceiveStream2
   std::unique_ptr<VideoStreamDecoder> video_stream_decoder_;
   RtpStreamsSynchronizer rtp_stream_sync_;
 
-  // TODO(nisse, philipel): Creation and ownership of video encoders should be
-  // moved to the new VideoStreamDecoder.
-  std::vector<std::unique_ptr<VideoDecoder>> video_decoders_;
-
   std::unique_ptr<VideoStreamBufferController> buffer_;
 
   std::unique_ptr<RtpStreamReceiverInterface> media_receiver_