Remove dependency on RtpVideoSenderInterface from EncoderRtcpFeedback.

This removes the two step initialization and explicit circular
dependency between the sender and the observer that complicates
construction and making members const that should be.
Moving forward the encoder feedback instance will move to a different
class, so this CL is one part of making that change possible.

Also removing an unnecessary mutex and replacing with a checker.

Bug: webrtc:12840
Change-Id: I21694806b122592de0cd1e1d96f241d339a0860f
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/221108
Commit-Queue: Tommi <tommi@webrtc.org>
Reviewed-by: Markus Handell <handellm@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34214}

video/BUILD.gn

@@ -67,6 +67,7 @@ rtc_library("video") {
     "../api/crypto:options",
     "../api/rtc_event_log",
     "../api/task_queue",
+    "../api/units:time_delta",
     "../api/units:timestamp",
     "../api/video:encoded_image",
     "../api/video:recordable_encoded_frame",

video/encoder_rtcp_feedback.cc

@@ -10,6 +10,9 @@
 
 #include "video/encoder_rtcp_feedback.h"
 
+#include <algorithm>
+#include <utility>
+
 #include "absl/types/optional.h"
 #include "api/video_codecs/video_encoder.h"
 #include "rtc_base/checks.h"
@@ -21,47 +24,36 @@ namespace {
 constexpr int kMinKeyframeSendIntervalMs = 300;
 }  // namespace
 
-EncoderRtcpFeedback::EncoderRtcpFeedback(Clock* clock,
+EncoderRtcpFeedback::EncoderRtcpFeedback(
+    Clock* clock,
     const std::vector<uint32_t>& ssrcs,
-                                         VideoStreamEncoderInterface* encoder)
+    VideoStreamEncoderInterface* encoder,
+    std::function<std::vector<RtpSequenceNumberMap::Info>(
+        uint32_t ssrc,
+        const std::vector<uint16_t>& seq_nums)> get_packet_infos)
     : clock_(clock),
       ssrcs_(ssrcs),
-      rtp_video_sender_(nullptr),
+      get_packet_infos_(std::move(get_packet_infos)),
       video_stream_encoder_(encoder),
-      time_last_intra_request_ms_(-1),
+      time_last_packet_delivery_queue_(Timestamp::Millis(0)),
-      min_keyframe_send_interval_ms_(
+      min_keyframe_send_interval_(
-          KeyframeIntervalSettings::ParseFromFieldTrials()
+          TimeDelta::Millis(KeyframeIntervalSettings::ParseFromFieldTrials()
                                 .MinKeyframeSendIntervalMs()
-              .value_or(kMinKeyframeSendIntervalMs)) {
+                                .value_or(kMinKeyframeSendIntervalMs))) {
   RTC_DCHECK(!ssrcs.empty());
+  packet_delivery_queue_.Detach();
 }
 
-void EncoderRtcpFeedback::SetRtpVideoSender(
+// Called via Call::DeliverRtcp.
-    const RtpVideoSenderInterface* rtp_video_sender) {
-  RTC_DCHECK(rtp_video_sender);
-  RTC_DCHECK(!rtp_video_sender_);
-  rtp_video_sender_ = rtp_video_sender;
-}
-
-bool EncoderRtcpFeedback::HasSsrc(uint32_t ssrc) {
-  for (uint32_t registered_ssrc : ssrcs_) {
-    if (registered_ssrc == ssrc) {
-      return true;
-    }
-  }
-  return false;
-}
-
 void EncoderRtcpFeedback::OnReceivedIntraFrameRequest(uint32_t ssrc) {
-  RTC_DCHECK(HasSsrc(ssrc));
+  RTC_DCHECK_RUN_ON(&packet_delivery_queue_);
-  {
+  RTC_DCHECK(std::find(ssrcs_.begin(), ssrcs_.end(), ssrc) != ssrcs_.end());
-    int64_t now_ms = clock_->TimeInMilliseconds();
+
-    MutexLock lock(&mutex_);
+  const Timestamp now = clock_->CurrentTime();
-    if (time_last_intra_request_ms_ + min_keyframe_send_interval_ms_ > now_ms) {
+  if (time_last_packet_delivery_queue_ + min_keyframe_send_interval_ > now)
     return;
-    }
+
-    time_last_intra_request_ms_ = now_ms;
+  time_last_packet_delivery_queue_ = now;
-  }
 
   // Always produce key frame for all streams.
   video_stream_encoder_->SendKeyFrame();
@@ -72,12 +64,12 @@ void EncoderRtcpFeedback::OnReceivedLossNotification(
     uint16_t seq_num_of_last_decodable,
     uint16_t seq_num_of_last_received,
     bool decodability_flag) {
-  RTC_DCHECK(rtp_video_sender_) << "Object initialization incomplete.";
+  RTC_DCHECK(get_packet_infos_) << "Object initialization incomplete.";
 
   const std::vector<uint16_t> seq_nums = {seq_num_of_last_decodable,
                                           seq_num_of_last_received};
   const std::vector<RtpSequenceNumberMap::Info> infos =
-      rtp_video_sender_->GetSentRtpPacketInfos(ssrc, seq_nums);
+      get_packet_infos_(ssrc, seq_nums);
   if (infos.empty()) {
     return;
   }

video/encoder_rtcp_feedback.h

@@ -10,12 +10,16 @@
 #ifndef VIDEO_ENCODER_RTCP_FEEDBACK_H_
 #define VIDEO_ENCODER_RTCP_FEEDBACK_H_
 
+#include <functional>
 #include <vector>
 
+#include "api/sequence_checker.h"
+#include "api/units/time_delta.h"
+#include "api/units/timestamp.h"
 #include "api/video/video_stream_encoder_interface.h"
 #include "call/rtp_video_sender_interface.h"
 #include "modules/rtp_rtcp/include/rtp_rtcp_defines.h"
-#include "rtc_base/synchronization/mutex.h"
+#include "rtc_base/system/no_unique_address.h"
 #include "system_wrappers/include/clock.h"
 
 namespace webrtc {
@@ -27,13 +31,15 @@ class VideoStreamEncoderInterface;
 class EncoderRtcpFeedback : public RtcpIntraFrameObserver,
                             public RtcpLossNotificationObserver {
  public:
-  EncoderRtcpFeedback(Clock* clock,
+  EncoderRtcpFeedback(
+      Clock* clock,
       const std::vector<uint32_t>& ssrcs,
-                      VideoStreamEncoderInterface* encoder);
+      VideoStreamEncoderInterface* encoder,
+      std::function<std::vector<RtpSequenceNumberMap::Info>(
+          uint32_t ssrc,
+          const std::vector<uint16_t>& seq_nums)> get_packet_infos);
   ~EncoderRtcpFeedback() override = default;
 
-  void SetRtpVideoSender(const RtpVideoSenderInterface* rtp_video_sender);
-
   void OnReceivedIntraFrameRequest(uint32_t ssrc) override;
 
   // Implements RtcpLossNotificationObserver.
@@ -43,17 +49,19 @@ class EncoderRtcpFeedback : public RtcpIntraFrameObserver,
                                   bool decodability_flag) override;
 
  private:
-  bool HasSsrc(uint32_t ssrc);
-
   Clock* const clock_;
   const std::vector<uint32_t> ssrcs_;
-  const RtpVideoSenderInterface* rtp_video_sender_;
+  const std::function<std::vector<RtpSequenceNumberMap::Info>(
+      uint32_t ssrc,
+      const std::vector<uint16_t>& seq_nums)>
+      get_packet_infos_;
   VideoStreamEncoderInterface* const video_stream_encoder_;
 
-  Mutex mutex_;
+  RTC_NO_UNIQUE_ADDRESS SequenceChecker packet_delivery_queue_;
-  int64_t time_last_intra_request_ms_ RTC_GUARDED_BY(mutex_);
+  Timestamp time_last_packet_delivery_queue_
+      RTC_GUARDED_BY(packet_delivery_queue_);
 
-  const int min_keyframe_send_interval_ms_;
+  const TimeDelta min_keyframe_send_interval_;
 };
 
 }  // namespace webrtc

video/encoder_rtcp_feedback_unittest.cc

@@ -26,7 +26,8 @@ class VieKeyRequestTest : public ::testing::Test {
         encoder_rtcp_feedback_(
             &simulated_clock_,
             std::vector<uint32_t>(1, VieKeyRequestTest::kSsrc),
-            &encoder_) {}
+            &encoder_,
+            nullptr) {}
 
  protected:
   const uint32_t kSsrc = 1234;

video/video_send_stream_impl.cc

@@ -223,7 +223,13 @@ VideoSendStreamImpl::VideoSendStreamImpl(
       encoder_bitrate_priority_(initial_encoder_bitrate_priority),
       has_packet_feedback_(false),
       video_stream_encoder_(video_stream_encoder),
-      encoder_feedback_(clock, config_->rtp.ssrcs, video_stream_encoder),
+      encoder_feedback_(
+          clock,
+          config_->rtp.ssrcs,
+          video_stream_encoder,
+          [this](uint32_t ssrc, const std::vector<uint16_t>& seq_nums) {
+            return rtp_video_sender_->GetSentRtpPacketInfos(ssrc, seq_nums);
+          }),
       bandwidth_observer_(transport->GetBandwidthObserver()),
       rtp_video_sender_(
           transport_->CreateRtpVideoSender(suspended_ssrcs,
@@ -245,8 +251,6 @@ VideoSendStreamImpl::VideoSendStreamImpl(
   RTC_LOG(LS_INFO) << "VideoSendStreamInternal: " << config_->ToString();
   weak_ptr_ = weak_ptr_factory_.GetWeakPtr();
 
-  encoder_feedback_.SetRtpVideoSender(rtp_video_sender_);
-
   RTC_DCHECK(!config_->rtp.ssrcs.empty());
   RTC_DCHECK(transport_);
   RTC_DCHECK_NE(initial_encoder_max_bitrate, 0);