Revert "RtpVideoStreamReceiver::RtcpFeedbackBuffer: remove lock recursions."

This reverts commit ef93a26180660eaed00571996bb8e530be89320c.

Reason for revert: Checking if this broke things downstream.

Original change's description:
> 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}

TBR=stefan@webrtc.org,handellm@webrtc.org

Change-Id: I99b622a0f88f3a264f1943f2457b9c9b89962b86
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: webrtc:11567
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/175644
Reviewed-by: Tommi <tommi@webrtc.org>
Commit-Queue: Tommi <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#31315}

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.
-    SendRtcpFeedback(ConsumeRtcpFeedbackLocked());
+    SendBufferedRtcpFeedback();
   }
 }
 
@@ -155,44 +155,34 @@ void RtpVideoStreamReceiver::RtcpFeedbackBuffer::SendLossNotification(
 }
 
 void RtpVideoStreamReceiver::RtcpFeedbackBuffer::SendBufferedRtcpFeedback() {
-  SendRtcpFeedback(ConsumeRtcpFeedback());
+  bool request_key_frame = false;
-}
+  std::vector<uint16_t> nack_sequence_numbers;
+  absl::optional<LossNotificationState> lntf_state;
 
-RtpVideoStreamReceiver::RtcpFeedbackBuffer::ConsumedRtcpFeedback
+  {
-RtpVideoStreamReceiver::RtcpFeedbackBuffer::ConsumeRtcpFeedback() {
     rtc::CritScope lock(&cs_);
-  return ConsumeRtcpFeedbackLocked();
+    std::swap(request_key_frame, request_key_frame_);
-}
+    std::swap(nack_sequence_numbers, nack_sequence_numbers_);
+    std::swap(lntf_state, lntf_state_);
+  }
 
-RtpVideoStreamReceiver::RtcpFeedbackBuffer::ConsumedRtcpFeedback
+  if (lntf_state) {
-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 =
-        feedback.request_key_frame || !feedback.nack_sequence_numbers.empty();
+        request_key_frame || !nack_sequence_numbers.empty();
 
     loss_notification_sender_->SendLossNotification(
-        feedback.lntf_state->last_decoded_seq_num,
+        lntf_state->last_decoded_seq_num, lntf_state->last_received_seq_num,
-        feedback.lntf_state->last_received_seq_num,
+        lntf_state->decodability_flag, buffering_allowed);
-        feedback.lntf_state->decodability_flag, buffering_allowed);
   }
 
-  if (feedback.request_key_frame) {
+  if (request_key_frame) {
     key_frame_request_sender_->RequestKeyFrame();
-  } else if (!feedback.nack_sequence_numbers.empty()) {
+  } else if (!nack_sequence_numbers.empty()) {
-    nack_sender_->SendNack(feedback.nack_sequence_numbers, true);
+    nack_sender_->SendNack(nack_sequence_numbers, true);
   }
 }
 

video/rtp_video_stream_receiver.h

@@ -225,48 +225,22 @@ class RtpVideoStreamReceiver : public LossNotificationSender,
     ~RtcpFeedbackBuffer() override = default;
 
     // KeyFrameRequestSender implementation.
-    void RequestKeyFrame() RTC_LOCKS_EXCLUDED(cs_) override;
+    void RequestKeyFrame() override;
 
     // NackSender implementation.
     void SendNack(const std::vector<uint16_t>& sequence_numbers,
-                  bool buffering_allowed) RTC_LOCKS_EXCLUDED(cs_) override;
+                  bool buffering_allowed) override;
 
     // LossNotificationSender implementation.
     void SendLossNotification(uint16_t last_decoded_seq_num,
                               uint16_t last_received_seq_num,
                               bool decodability_flag,
-                              bool buffering_allowed)
+                              bool buffering_allowed) override;
-        RTC_LOCKS_EXCLUDED(cs_) override;
 
     // Send all RTCP feedback messages buffered thus far.
-    void SendBufferedRtcpFeedback() RTC_LOCKS_EXCLUDED(cs_);
+    void SendBufferedRtcpFeedback();
 
    private:
-    // LNTF-related state.
-    struct LossNotificationState {
-      LossNotificationState(uint16_t last_decoded_seq_num,
-                            uint16_t last_received_seq_num,
-                            bool decodability_flag)
-          : last_decoded_seq_num(last_decoded_seq_num),
-            last_received_seq_num(last_received_seq_num),
-            decodability_flag(decodability_flag) {}
-
-      uint16_t last_decoded_seq_num;
-      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_;
@@ -280,6 +254,19 @@ class RtpVideoStreamReceiver : public LossNotificationSender,
     // 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,
+                            uint16_t last_received_seq_num,
+                            bool decodability_flag)
+          : last_decoded_seq_num(last_decoded_seq_num),
+            last_received_seq_num(last_received_seq_num),
+            decodability_flag(decodability_flag) {}
+
+      uint16_t last_decoded_seq_num;
+      uint16_t last_received_seq_num;
+      bool decodability_flag;
+    };
     absl::optional<LossNotificationState> lntf_state_ RTC_GUARDED_BY(cs_);
   };
   enum ParseGenericDependenciesResult {