diff --git a/video/rtp_video_stream_receiver.cc b/video/rtp_video_stream_receiver.cc index e1dd736be6..87691d4171 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. - SendBufferedRtcpFeedback(); + SendRtcpFeedback(ConsumeRtcpFeedbackLocked()); } } @@ -155,34 +155,44 @@ void RtpVideoStreamReceiver::RtcpFeedbackBuffer::SendLossNotification( } void RtpVideoStreamReceiver::RtcpFeedbackBuffer::SendBufferedRtcpFeedback() { - bool request_key_frame = false; - std::vector nack_sequence_numbers; - absl::optional lntf_state; + SendRtcpFeedback(ConsumeRtcpFeedback()); +} - { - 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::ConsumeRtcpFeedback() { + rtc::CritScope lock(&cs_); + return ConsumeRtcpFeedbackLocked(); +} - if (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 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, - lntf_state->decodability_flag, buffering_allowed); + feedback.lntf_state->last_decoded_seq_num, + 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()) { - nack_sender_->SendNack(nack_sequence_numbers, true); + } else if (!feedback.nack_sequence_numbers.empty()) { + nack_sender_->SendNack(feedback.nack_sequence_numbers, true); } } diff --git a/video/rtp_video_stream_receiver.h b/video/rtp_video_stream_receiver.h index 0289f23a07..32d675c307 100644 --- a/video/rtp_video_stream_receiver.h +++ b/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& 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 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 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_; + + // 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 nack_sequence_numbers_ RTC_GUARDED_BY(cs_); + absl::optional lntf_state_ RTC_GUARDED_BY(cs_); }; enum ParseGenericDependenciesResult {