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}
diff --git a/video/rtp_video_stream_receiver.cc b/video/rtp_video_stream_receiver.cc
index 87691d4..e1dd736 100644
--- a/video/rtp_video_stream_receiver.cc
+++ b/video/rtp_video_stream_receiver.cc
@@ -136,7 +136,7 @@
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::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();
-}
+ {
+ 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 32d675c..0289f23 100644
--- a/video/rtp_video_stream_receiver.h
+++ b/video/rtp_video_stream_receiver.h
@@ -225,48 +225,22 @@
~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)
- 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<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 @@
// 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 {