RtcpSender: remove lock recursions.

This change removes lock recursions and adds thread annotations.

Bug: webrtc:11567
Change-Id: Idc872bda693776667f3b9126bd79589d74398b34
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/175640
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Commit-Queue: Markus Handell <handellm@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#31308}

modules/rtp_rtcp/source/rtcp_sender.cc

@@ -262,8 +262,8 @@ int32_t RTCPSender::SendLossNotification(const FeedbackState& feedback_state,
     return 0;
   }
 
-  return SendCompoundRTCP(feedback_state,
+  return SendCompoundRTCPLocked(
-                          {RTCPPacketType::kRtcpLossNotification});
+      feedback_state, {RTCPPacketType::kRtcpLossNotification}, 0, nullptr);
 }
 
 void RTCPSender::SetRemb(int64_t bitrate_bps, std::vector<uint32_t> ssrcs) {
@@ -712,76 +712,11 @@ int32_t RTCPSender::SendCompoundRTCP(
 
   {
     rtc::CritScope lock(&critical_section_rtcp_sender_);
-    if (method_ == RtcpMode::kOff) {
+    auto result = ComputeCompoundRTCPPacket(feedback_state, packet_types,
-      RTC_LOG(LS_WARNING) << "Can't send rtcp if it is disabled.";
+                                            nack_size, nack_list, &container);
-      return -1;
+    if (result) {
+      return *result;
     }
-    // Add all flags as volatile. Non volatile entries will not be overwritten.
-    // All new volatile flags added will be consumed by the end of this call.
-    SetFlags(packet_types, true);
-
-    // Prevent sending streams to send SR before any media has been sent.
-    const bool can_calculate_rtp_timestamp = (last_frame_capture_time_ms_ >= 0);
-    if (!can_calculate_rtp_timestamp) {
-      bool consumed_sr_flag = ConsumeFlag(kRtcpSr);
-      bool consumed_report_flag = sending_ && ConsumeFlag(kRtcpReport);
-      bool sender_report = consumed_report_flag || consumed_sr_flag;
-      if (sender_report && AllVolatileFlagsConsumed()) {
-        // This call was for Sender Report and nothing else.
-        return 0;
-      }
-      if (sending_ && method_ == RtcpMode::kCompound) {
-        // Not allowed to send any RTCP packet without sender report.
-        return -1;
-      }
-    }
-
-    if (packet_type_counter_.first_packet_time_ms == -1)
-      packet_type_counter_.first_packet_time_ms = clock_->TimeInMilliseconds();
-
-    // We need to send our NTP even if we haven't received any reports.
-    RtcpContext context(feedback_state, nack_size, nack_list,
-                        clock_->TimeInMicroseconds());
-
-    PrepareReport(feedback_state);
-
-    std::unique_ptr<rtcp::RtcpPacket> packet_bye;
-
-    auto it = report_flags_.begin();
-    while (it != report_flags_.end()) {
-      auto builder_it = builders_.find(it->type);
-      RTC_DCHECK(builder_it != builders_.end())
-          << "Could not find builder for packet type " << it->type;
-      if (it->is_volatile) {
-        report_flags_.erase(it++);
-      } else {
-        ++it;
-      }
-
-      BuilderFunc func = builder_it->second;
-      std::unique_ptr<rtcp::RtcpPacket> packet = (this->*func)(context);
-      if (packet == nullptr)
-        return -1;
-      // If there is a BYE, don't append now - save it and append it
-      // at the end later.
-      if (builder_it->first == kRtcpBye) {
-        packet_bye = std::move(packet);
-      } else {
-        container.Append(packet.release());
-      }
-    }
-
-    // Append the BYE now at the end
-    if (packet_bye) {
-      container.Append(packet_bye.release());
-    }
-
-    if (packet_type_counter_observer_ != nullptr) {
-      packet_type_counter_observer_->RtcpPacketTypesCounterUpdated(
-          remote_ssrc_, packet_type_counter_);
-    }
-
-    RTC_DCHECK(AllVolatileFlagsConsumed());
     max_packet_size = max_packet_size_;
   }
 
@@ -789,6 +724,100 @@ int32_t RTCPSender::SendCompoundRTCP(
   return bytes_sent == 0 ? -1 : 0;
 }
 
+int32_t RTCPSender::SendCompoundRTCPLocked(
+    const FeedbackState& feedback_state,
+    const std::set<RTCPPacketType>& packet_types,
+    int32_t nack_size,
+    const uint16_t* nack_list) {
+  PacketContainer container(transport_, event_log_);
+  auto result = ComputeCompoundRTCPPacket(feedback_state, packet_types,
+                                          nack_size, nack_list, &container);
+  if (result) {
+    return *result;
+  }
+  size_t bytes_sent = container.SendPackets(max_packet_size_);
+  return bytes_sent == 0 ? -1 : 0;
+}
+
+absl::optional<int32_t> RTCPSender::ComputeCompoundRTCPPacket(
+    const FeedbackState& feedback_state,
+    const std::set<RTCPPacketType>& packet_types,
+    int32_t nack_size,
+    const uint16_t* nack_list,
+    rtcp::CompoundPacket* out_packet) {
+  if (method_ == RtcpMode::kOff) {
+    RTC_LOG(LS_WARNING) << "Can't send rtcp if it is disabled.";
+    return -1;
+  }
+  // Add all flags as volatile. Non volatile entries will not be overwritten.
+  // All new volatile flags added will be consumed by the end of this call.
+  SetFlags(packet_types, true);
+
+  // Prevent sending streams to send SR before any media has been sent.
+  const bool can_calculate_rtp_timestamp = (last_frame_capture_time_ms_ >= 0);
+  if (!can_calculate_rtp_timestamp) {
+    bool consumed_sr_flag = ConsumeFlag(kRtcpSr);
+    bool consumed_report_flag = sending_ && ConsumeFlag(kRtcpReport);
+    bool sender_report = consumed_report_flag || consumed_sr_flag;
+    if (sender_report && AllVolatileFlagsConsumed()) {
+      // This call was for Sender Report and nothing else.
+      return 0;
+    }
+    if (sending_ && method_ == RtcpMode::kCompound) {
+      // Not allowed to send any RTCP packet without sender report.
+      return -1;
+    }
+  }
+
+  if (packet_type_counter_.first_packet_time_ms == -1)
+    packet_type_counter_.first_packet_time_ms = clock_->TimeInMilliseconds();
+
+  // We need to send our NTP even if we haven't received any reports.
+  RtcpContext context(feedback_state, nack_size, nack_list,
+                      clock_->TimeInMicroseconds());
+
+  PrepareReport(feedback_state);
+
+  std::unique_ptr<rtcp::RtcpPacket> packet_bye;
+
+  auto it = report_flags_.begin();
+  while (it != report_flags_.end()) {
+    auto builder_it = builders_.find(it->type);
+    RTC_DCHECK(builder_it != builders_.end())
+        << "Could not find builder for packet type " << it->type;
+    if (it->is_volatile) {
+      report_flags_.erase(it++);
+    } else {
+      ++it;
+    }
+
+    BuilderFunc func = builder_it->second;
+    std::unique_ptr<rtcp::RtcpPacket> packet = (this->*func)(context);
+    if (packet == nullptr)
+      return -1;
+    // If there is a BYE, don't append now - save it and append it
+    // at the end later.
+    if (builder_it->first == kRtcpBye) {
+      packet_bye = std::move(packet);
+    } else {
+      out_packet->Append(packet.release());
+    }
+  }
+
+  // Append the BYE now at the end
+  if (packet_bye) {
+    out_packet->Append(packet_bye.release());
+  }
+
+  if (packet_type_counter_observer_ != nullptr) {
+    packet_type_counter_observer_->RtcpPacketTypesCounterUpdated(
+        remote_ssrc_, packet_type_counter_);
+  }
+
+  RTC_DCHECK(AllVolatileFlagsConsumed());
+  return absl::nullopt;
+}
+
 void RTCPSender::PrepareReport(const FeedbackState& feedback_state) {
   bool generate_report;
   if (IsFlagPresent(kRtcpSr) || IsFlagPresent(kRtcpRr)) {

modules/rtp_rtcp/source/rtcp_sender.h

@@ -27,6 +27,7 @@
 #include "modules/rtp_rtcp/include/rtp_rtcp_defines.h"
 #include "modules/rtp_rtcp/source/rtcp_nack_stats.h"
 #include "modules/rtp_rtcp/source/rtcp_packet.h"
+#include "modules/rtp_rtcp/source/rtcp_packet/compound_packet.h"
 #include "modules/rtp_rtcp/source/rtcp_packet/dlrr.h"
 #include "modules/rtp_rtcp/source/rtcp_packet/report_block.h"
 #include "modules/rtp_rtcp/source/rtcp_packet/tmmb_item.h"
@@ -66,84 +67,124 @@ class RTCPSender {
   explicit RTCPSender(const RtpRtcp::Configuration& config);
   virtual ~RTCPSender();
 
-  RtcpMode Status() const;
+  RtcpMode Status() const RTC_LOCKS_EXCLUDED(critical_section_rtcp_sender_);
-  void SetRTCPStatus(RtcpMode method);
+  void SetRTCPStatus(RtcpMode method)
+      RTC_LOCKS_EXCLUDED(critical_section_rtcp_sender_);
 
-  bool Sending() const;
+  bool Sending() const RTC_LOCKS_EXCLUDED(critical_section_rtcp_sender_);
   int32_t SetSendingStatus(const FeedbackState& feedback_state,
-                           bool enabled);  // combine the functions
+                           bool enabled)
+      RTC_LOCKS_EXCLUDED(
+          critical_section_rtcp_sender_);  // combine the functions
 
-  int32_t SetNackStatus(bool enable);
+  int32_t SetNackStatus(bool enable)
+      RTC_LOCKS_EXCLUDED(critical_section_rtcp_sender_);
 
-  void SetTimestampOffset(uint32_t timestamp_offset);
+  void SetTimestampOffset(uint32_t timestamp_offset)
+      RTC_LOCKS_EXCLUDED(critical_section_rtcp_sender_);
 
   // TODO(bugs.webrtc.org/6458): Remove default parameter value when all the
   // depending projects are updated to correctly set payload type.
   void SetLastRtpTime(uint32_t rtp_timestamp,
                       int64_t capture_time_ms,
-                      int8_t payload_type = -1);
+                      int8_t payload_type = -1)
+      RTC_LOCKS_EXCLUDED(critical_section_rtcp_sender_);
 
-  void SetRtpClockRate(int8_t payload_type, int rtp_clock_rate_hz);
+  void SetRtpClockRate(int8_t payload_type, int rtp_clock_rate_hz)
+      RTC_LOCKS_EXCLUDED(critical_section_rtcp_sender_);
 
   uint32_t SSRC() const { return ssrc_; }
 
-  void SetRemoteSSRC(uint32_t ssrc);
+  void SetRemoteSSRC(uint32_t ssrc)
+      RTC_LOCKS_EXCLUDED(critical_section_rtcp_sender_);
 
-  int32_t SetCNAME(const char* cName);
+  int32_t SetCNAME(const char* cName)
+      RTC_LOCKS_EXCLUDED(critical_section_rtcp_sender_);
 
-  int32_t AddMixedCNAME(uint32_t SSRC, const char* c_name);
+  int32_t AddMixedCNAME(uint32_t SSRC, const char* c_name)
+      RTC_LOCKS_EXCLUDED(critical_section_rtcp_sender_);
 
-  int32_t RemoveMixedCNAME(uint32_t SSRC);
+  int32_t RemoveMixedCNAME(uint32_t SSRC)
+      RTC_LOCKS_EXCLUDED(critical_section_rtcp_sender_);
 
-  bool TimeToSendRTCPReport(bool sendKeyframeBeforeRTP = false) const;
+  bool TimeToSendRTCPReport(bool sendKeyframeBeforeRTP = false) const
+      RTC_LOCKS_EXCLUDED(critical_section_rtcp_sender_);
 
   int32_t SendRTCP(const FeedbackState& feedback_state,
                    RTCPPacketType packetType,
                    int32_t nackSize = 0,
-                   const uint16_t* nackList = 0);
+                   const uint16_t* nackList = 0)
+      RTC_LOCKS_EXCLUDED(critical_section_rtcp_sender_);
 
   int32_t SendCompoundRTCP(const FeedbackState& feedback_state,
                            const std::set<RTCPPacketType>& packetTypes,
                            int32_t nackSize = 0,
-                           const uint16_t* nackList = 0);
+                           const uint16_t* nackList = nullptr)
+      RTC_LOCKS_EXCLUDED(critical_section_rtcp_sender_);
 
   int32_t SendLossNotification(const FeedbackState& feedback_state,
                                uint16_t last_decoded_seq_num,
                                uint16_t last_received_seq_num,
                                bool decodability_flag,
-                               bool buffering_allowed);
+                               bool buffering_allowed)
+      RTC_LOCKS_EXCLUDED(critical_section_rtcp_sender_);
 
-  void SetRemb(int64_t bitrate_bps, std::vector<uint32_t> ssrcs);
+  void SetRemb(int64_t bitrate_bps, std::vector<uint32_t> ssrcs)
+      RTC_LOCKS_EXCLUDED(critical_section_rtcp_sender_);
 
-  void UnsetRemb();
+  void UnsetRemb() RTC_LOCKS_EXCLUDED(critical_section_rtcp_sender_);
 
-  bool TMMBR() const;
+  bool TMMBR() const RTC_LOCKS_EXCLUDED(critical_section_rtcp_sender_);
 
-  void SetTMMBRStatus(bool enable);
+  void SetTMMBRStatus(bool enable)
+      RTC_LOCKS_EXCLUDED(critical_section_rtcp_sender_);
 
-  void SetMaxRtpPacketSize(size_t max_packet_size);
+  void SetMaxRtpPacketSize(size_t max_packet_size)
+      RTC_LOCKS_EXCLUDED(critical_section_rtcp_sender_);
 
-  void SetTmmbn(std::vector<rtcp::TmmbItem> bounding_set);
+  void SetTmmbn(std::vector<rtcp::TmmbItem> bounding_set)
+      RTC_LOCKS_EXCLUDED(critical_section_rtcp_sender_);
 
   int32_t SetApplicationSpecificData(uint8_t subType,
                                      uint32_t name,
                                      const uint8_t* data,
-                                     uint16_t length);
+                                     uint16_t length)
+      RTC_LOCKS_EXCLUDED(critical_section_rtcp_sender_);
 
-  void SendRtcpXrReceiverReferenceTime(bool enable);
+  void SendRtcpXrReceiverReferenceTime(bool enable)
+      RTC_LOCKS_EXCLUDED(critical_section_rtcp_sender_);
 
-  bool RtcpXrReceiverReferenceTime() const;
+  bool RtcpXrReceiverReferenceTime() const
+      RTC_LOCKS_EXCLUDED(critical_section_rtcp_sender_);
 
-  void SetCsrcs(const std::vector<uint32_t>& csrcs);
+  void SetCsrcs(const std::vector<uint32_t>& csrcs)
+      RTC_LOCKS_EXCLUDED(critical_section_rtcp_sender_);
 
-  void SetTargetBitrate(unsigned int target_bitrate);
+  void SetTargetBitrate(unsigned int target_bitrate)
-  void SetVideoBitrateAllocation(const VideoBitrateAllocation& bitrate);
+      RTC_LOCKS_EXCLUDED(critical_section_rtcp_sender_);
+  void SetVideoBitrateAllocation(const VideoBitrateAllocation& bitrate)
+      RTC_LOCKS_EXCLUDED(critical_section_rtcp_sender_);
   void SendCombinedRtcpPacket(
-      std::vector<std::unique_ptr<rtcp::RtcpPacket>> rtcp_packets);
+      std::vector<std::unique_ptr<rtcp::RtcpPacket>> rtcp_packets)
+      RTC_LOCKS_EXCLUDED(critical_section_rtcp_sender_);
 
  private:
   class RtcpContext;
 
+  int32_t SendCompoundRTCPLocked(const FeedbackState& feedback_state,
+                                 const std::set<RTCPPacketType>& packet_types,
+                                 int32_t nack_size,
+                                 const uint16_t* nack_list)
+      RTC_EXCLUSIVE_LOCKS_REQUIRED(critical_section_rtcp_sender_);
+
+  absl::optional<int32_t> ComputeCompoundRTCPPacket(
+      const FeedbackState& feedback_state,
+      const std::set<RTCPPacketType>& packet_types,
+      int32_t nack_size,
+      const uint16_t* nack_list,
+      rtcp::CompoundPacket* out_packet)
+      RTC_EXCLUSIVE_LOCKS_REQUIRED(critical_section_rtcp_sender_);
+
   // Determine which RTCP messages should be sent and setup flags.
   void PrepareReport(const FeedbackState& feedback_state)
       RTC_EXCLUSIVE_LOCKS_REQUIRED(critical_section_rtcp_sender_);