From fde94a72d1e4f6bc38f8324bed390a9cebbc091c Mon Sep 17 00:00:00 2001 From: Tommi Date: Tue, 19 May 2020 05:48:33 +0000 Subject: [PATCH] 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 > Commit-Queue: Markus Handell > 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 Commit-Queue: Tommi Cr-Commit-Position: refs/heads/master@{#31315} --- video/rtp_video_stream_receiver.cc | 44 +++++++++++----------------- video/rtp_video_stream_receiver.h | 47 +++++++++++------------------- 2 files changed, 34 insertions(+), 57 deletions(-) diff --git a/video/rtp_video_stream_receiver.cc b/video/rtp_video_stream_receiver.cc index 87691d4171..e1dd736be6 100644 --- a/video/rtp_video_stream_receiver.cc +++ b/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 nack_sequence_numbers; + absl::optional lntf_state; -RtpVideoStreamReceiver::RtcpFeedbackBuffer::ConsumedRtcpFeedback -RtpVideoStreamReceiver::RtcpFeedbackBuffer::ConsumeRtcpFeedback() { - rtc::CritScope lock(&cs_); - return ConsumeRtcpFeedbackLocked(); -} + { + 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_); + } -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 (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, - feedback.lntf_state->last_received_seq_num, - feedback.lntf_state->decodability_flag, buffering_allowed); + lntf_state->last_decoded_seq_num, lntf_state->last_received_seq_num, + 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()) { - nack_sender_->SendNack(feedback.nack_sequence_numbers, true); + } else if (!nack_sequence_numbers.empty()) { + nack_sender_->SendNack(nack_sequence_numbers, true); } } diff --git a/video/rtp_video_stream_receiver.h b/video/rtp_video_stream_receiver.h index 32d675c307..0289f23a07 100644 --- a/video/rtp_video_stream_receiver.h +++ b/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& 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) - RTC_LOCKS_EXCLUDED(cs_) override; + bool buffering_allowed) 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 nack_sequence_numbers; - absl::optional 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 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 lntf_state_ RTC_GUARDED_BY(cs_); }; enum ParseGenericDependenciesResult {