Reland "RtpVideoStreamReceiver::RtcpFeedbackBuffer: remove lock recursions."
This reverts commit fde94a72d1e4f6bc38f8324bed390a9cebbc091c.
Reason for revert: there was a suspicion it broke the importer, but it was probably something else.
Original change's description:
> 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}
TBR=tommi@webrtc.org,stefan@webrtc.org,handellm@webrtc.org
# Not skipping CQ checks because this is a reland.
Bug: webrtc:11567
Change-Id: I1171127bc4a0520561caf92ddb787ec7e649e7af
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/175651
Reviewed-by: Markus Handell <handellm@webrtc.org>
Commit-Queue: Markus Handell <handellm@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#31322}
diff --git a/video/rtp_video_stream_receiver.cc b/video/rtp_video_stream_receiver.cc
index e1dd736..87691d4 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.
- SendBufferedRtcpFeedback();
+ SendRtcpFeedback(ConsumeRtcpFeedbackLocked());
}
}
@@ -155,34 +155,44 @@
}
void RtpVideoStreamReceiver::RtcpFeedbackBuffer::SendBufferedRtcpFeedback() {
- bool request_key_frame = false;
- std::vector<uint16_t> nack_sequence_numbers;
- absl::optional<LossNotificationState> 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 0289f23..32d675c 100644
--- a/video/rtp_video_stream_receiver.h
+++ b/video/rtp_video_stream_receiver.h
@@ -225,35 +225,23 @@
~RtcpFeedbackBuffer() override = default;
// KeyFrameRequestSender implementation.
- void RequestKeyFrame() override;
+ void RequestKeyFrame() RTC_LOCKS_EXCLUDED(cs_) override;
// NackSender implementation.
void SendNack(const std::vector<uint16_t>& 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<uint16_t> nack_sequence_numbers_ RTC_GUARDED_BY(cs_);
-
// LNTF-related state.
struct LossNotificationState {
LossNotificationState(uint16_t last_decoded_seq_num,
@@ -267,6 +255,31 @@
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_;
+
+ // 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<uint16_t> nack_sequence_numbers_ RTC_GUARDED_BY(cs_);
+
absl::optional<LossNotificationState> lntf_state_ RTC_GUARDED_BY(cs_);
};
enum ParseGenericDependenciesResult {
