RtpVideoStreamReceiver::RtcpFeedbackBuffer: remove lock recursions.

This change removes lock recursions and adds thread annotations.

Bug: webrtc:11567
Change-Id: I68f62d0d62c8ad8dd8276e48f5876b75932bac61
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/175113
Reviewed-by: Stefan Holmer <stefan@webrtc.org>
Commit-Queue: Markus Handell <handellm@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#31314}

video/rtp_video_stream_receiver.cc

@@ -136,7 +136,7 @@ void RtpVideoStreamReceiver::RtcpFeedbackBuffer::SendNack(
   if (!buffering_allowed) {
     // Note that while *buffering* is not allowed, *batching* is, meaning that
     // previously buffered messages may be sent along with the current message.
-    SendBufferedRtcpFeedback();
+    SendRtcpFeedback(ConsumeRtcpFeedbackLocked());
   }
 }
 
@@ -155,34 +155,44 @@ void RtpVideoStreamReceiver::RtcpFeedbackBuffer::SendLossNotification(
 }
 
 void RtpVideoStreamReceiver::RtcpFeedbackBuffer::SendBufferedRtcpFeedback() {
-  bool request_key_frame = false;
+  SendRtcpFeedback(ConsumeRtcpFeedback());
-  std::vector<uint16_t> nack_sequence_numbers;
-  absl::optional<LossNotificationState> lntf_state;
-
-  {
-    rtc::CritScope lock(&cs_);
-    std::swap(request_key_frame, request_key_frame_);
-    std::swap(nack_sequence_numbers, nack_sequence_numbers_);
-    std::swap(lntf_state, lntf_state_);
 }
 
-  if (lntf_state) {
+RtpVideoStreamReceiver::RtcpFeedbackBuffer::ConsumedRtcpFeedback
+RtpVideoStreamReceiver::RtcpFeedbackBuffer::ConsumeRtcpFeedback() {
+  rtc::CritScope lock(&cs_);
+  return ConsumeRtcpFeedbackLocked();
+}
+
+RtpVideoStreamReceiver::RtcpFeedbackBuffer::ConsumedRtcpFeedback
+RtpVideoStreamReceiver::RtcpFeedbackBuffer::ConsumeRtcpFeedbackLocked() {
+  ConsumedRtcpFeedback feedback;
+  std::swap(feedback.request_key_frame, request_key_frame_);
+  std::swap(feedback.nack_sequence_numbers, nack_sequence_numbers_);
+  std::swap(feedback.lntf_state, lntf_state_);
+  return feedback;
+}
+
+void RtpVideoStreamReceiver::RtcpFeedbackBuffer::SendRtcpFeedback(
+    ConsumedRtcpFeedback feedback) {
+  if (feedback.lntf_state) {
     // If either a NACK or a key frame request is sent, we should buffer
     // the LNTF and wait for them (NACK or key frame request) to trigger
     // the compound feedback message.
     // Otherwise, the LNTF should be sent out immediately.
     const bool buffering_allowed =
-        request_key_frame || !nack_sequence_numbers.empty();
+        feedback.request_key_frame || !feedback.nack_sequence_numbers.empty();
 
     loss_notification_sender_->SendLossNotification(
-        lntf_state->last_decoded_seq_num, lntf_state->last_received_seq_num,
+        feedback.lntf_state->last_decoded_seq_num,
-        lntf_state->decodability_flag, buffering_allowed);
+        feedback.lntf_state->last_received_seq_num,
+        feedback.lntf_state->decodability_flag, buffering_allowed);
   }
 
-  if (request_key_frame) {
+  if (feedback.request_key_frame) {
     key_frame_request_sender_->RequestKeyFrame();
-  } else if (!nack_sequence_numbers.empty()) {
+  } else if (!feedback.nack_sequence_numbers.empty()) {
-    nack_sender_->SendNack(nack_sequence_numbers, true);
+    nack_sender_->SendNack(feedback.nack_sequence_numbers, true);
   }
 }
 

video/rtp_video_stream_receiver.h

@@ -225,35 +225,23 @@ class RtpVideoStreamReceiver : public LossNotificationSender,
     ~RtcpFeedbackBuffer() override = default;
 
     // KeyFrameRequestSender implementation.
-    void RequestKeyFrame() override;
+    void RequestKeyFrame() RTC_LOCKS_EXCLUDED(cs_) override;
 
     // NackSender implementation.
     void SendNack(const std::vector<uint16_t>& sequence_numbers,
-                  bool buffering_allowed) override;
+                  bool buffering_allowed) RTC_LOCKS_EXCLUDED(cs_) override;
 
     // LossNotificationSender implementation.
     void SendLossNotification(uint16_t last_decoded_seq_num,
                               uint16_t last_received_seq_num,
                               bool decodability_flag,
-                              bool buffering_allowed) override;
+                              bool buffering_allowed)
+        RTC_LOCKS_EXCLUDED(cs_) override;
 
     // Send all RTCP feedback messages buffered thus far.
-    void SendBufferedRtcpFeedback();
+    void SendBufferedRtcpFeedback() RTC_LOCKS_EXCLUDED(cs_);
 
    private:
-    KeyFrameRequestSender* const key_frame_request_sender_;
-    NackSender* const nack_sender_;
-    LossNotificationSender* const loss_notification_sender_;
-
-    // NACKs are accessible from two threads due to nack_module_ being a module.
-    rtc::CriticalSection cs_;
-
-    // Key-frame-request-related state.
-    bool request_key_frame_ RTC_GUARDED_BY(cs_);
-
-    // NACK-related state.
-    std::vector<uint16_t> nack_sequence_numbers_ RTC_GUARDED_BY(cs_);
-
     // LNTF-related state.
     struct LossNotificationState {
       LossNotificationState(uint16_t last_decoded_seq_num,
@@ -267,6 +255,31 @@ class RtpVideoStreamReceiver : public LossNotificationSender,
       uint16_t last_received_seq_num;
       bool decodability_flag;
     };
+    struct ConsumedRtcpFeedback {
+      bool request_key_frame = false;
+      std::vector<uint16_t> nack_sequence_numbers;
+      absl::optional<LossNotificationState> lntf_state;
+    };
+
+    ConsumedRtcpFeedback ConsumeRtcpFeedback() RTC_LOCKS_EXCLUDED(cs_);
+    ConsumedRtcpFeedback ConsumeRtcpFeedbackLocked()
+        RTC_EXCLUSIVE_LOCKS_REQUIRED(cs_);
+    // This method is called both with and without cs_ held.
+    void SendRtcpFeedback(ConsumedRtcpFeedback feedback);
+
+    KeyFrameRequestSender* const key_frame_request_sender_;
+    NackSender* const nack_sender_;
+    LossNotificationSender* const loss_notification_sender_;
+
+    // NACKs are accessible from two threads due to nack_module_ being a module.
+    rtc::CriticalSection cs_;
+
+    // Key-frame-request-related state.
+    bool request_key_frame_ RTC_GUARDED_BY(cs_);
+
+    // NACK-related state.
+    std::vector<uint16_t> nack_sequence_numbers_ RTC_GUARDED_BY(cs_);
+
     absl::optional<LossNotificationState> lntf_state_ RTC_GUARDED_BY(cs_);
   };
   enum ParseGenericDependenciesResult {