Add lock to guard rtp packet sequencer.
With deferred packet sequencing, the PacketSequencer instance is called
directly from the RtpRtcp module while before it was called from within
the RTPSender while holding a lock.
Since sequence number assignment happens on the same thread as actual
packet sending, though thought was that locking was no longer needed.
Unfortunately, SetRtpState()/GetRtpState() also exists - and while they
should only be called on creating/destruction there is a possible race
where a delayed packet from the pacer accesses the sequencer while
GetRtpState() is being called.
For now, this CL just adds a lock to guard sequencer. Follow-ups will
make sure get/set state is never called while module is attached to
the packet router. After that, the lock can be removed again.
Bug: webrtc:11340, webrtc:12470
Change-Id: I123c762fb4afd20b3a6bd03b86234eb9ec34a209
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/228430
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Commit-Queue: Erik Språng <sprang@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34723}
diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl2.cc b/modules/rtp_rtcp/source/rtp_rtcp_impl2.cc
index 136c11c..ec42f61 100644
--- a/modules/rtp_rtcp/source/rtp_rtcp_impl2.cc
+++ b/modules/rtp_rtcp/source/rtp_rtcp_impl2.cc
@@ -78,6 +78,7 @@
void ModuleRtpRtcpImpl2::RtpSenderContext::AssignSequenceNumber(
RtpPacketToSend* packet) {
if (deferred_sequencing_) {
+ MutexLock lock(&mutex_sequencer_);
sequencer_.Sequence(*packet);
} else {
packet_generator.AssignSequenceNumber(packet);
@@ -187,6 +188,7 @@
uint16_t ModuleRtpRtcpImpl2::SequenceNumber() const {
if (rtp_sender_->deferred_sequencing_) {
+ MutexLock lock(&rtp_sender_->mutex_sequencer_);
return rtp_sender_->sequencer_.media_sequence_number();
}
return rtp_sender_->packet_generator.SequenceNumber();
@@ -196,6 +198,7 @@
void ModuleRtpRtcpImpl2::SetSequenceNumber(const uint16_t seq_num) {
if (rtp_sender_->deferred_sequencing_) {
RTC_DCHECK_RUN_ON(&pacer_thread_checker_);
+ MutexLock lock(&rtp_sender_->mutex_sequencer_);
if (rtp_sender_->sequencer_.media_sequence_number() != seq_num) {
rtp_sender_->sequencer_.set_media_sequence_number(seq_num);
rtp_sender_->packet_history.Clear();
@@ -208,6 +211,7 @@
void ModuleRtpRtcpImpl2::SetRtpState(const RtpState& rtp_state) {
rtp_sender_->packet_generator.SetRtpState(rtp_state);
if (rtp_sender_->deferred_sequencing_) {
+ MutexLock lock(&rtp_sender_->mutex_sequencer_);
rtp_sender_->sequencer_.SetRtpState(rtp_state);
}
rtcp_sender_.SetTimestampOffset(rtp_state.start_timestamp);
@@ -216,6 +220,7 @@
void ModuleRtpRtcpImpl2::SetRtxState(const RtpState& rtp_state) {
rtp_sender_->packet_generator.SetRtxRtpState(rtp_state);
if (rtp_sender_->deferred_sequencing_) {
+ MutexLock lock(&rtp_sender_->mutex_sequencer_);
rtp_sender_->sequencer_.set_rtx_sequence_number(rtp_state.sequence_number);
}
}
@@ -223,6 +228,7 @@
RtpState ModuleRtpRtcpImpl2::GetRtpState() const {
RtpState state = rtp_sender_->packet_generator.GetRtpState();
if (rtp_sender_->deferred_sequencing_) {
+ MutexLock lock(&rtp_sender_->mutex_sequencer_);
rtp_sender_->sequencer_.PopulateRtpState(state);
}
return state;
@@ -231,6 +237,7 @@
RtpState ModuleRtpRtcpImpl2::GetRtxState() const {
RtpState state = rtp_sender_->packet_generator.GetRtxRtpState();
if (rtp_sender_->deferred_sequencing_) {
+ MutexLock lock(&rtp_sender_->mutex_sequencer_);
state.sequence_number = rtp_sender_->sequencer_.rtx_sequence_number();
}
return state;
@@ -387,6 +394,7 @@
if (!rtp_sender_->packet_generator.SendingMedia()) {
return false;
}
+ MutexLock lock(&rtp_sender_->mutex_sequencer_);
if (packet->packet_type() == RtpPacketMediaType::kPadding &&
packet->Ssrc() == rtp_sender_->packet_generator.SSRC() &&
!rtp_sender_->sequencer_.CanSendPaddingOnMediaSsrc()) {
@@ -452,11 +460,8 @@
std::vector<std::unique_ptr<RtpPacketToSend>>
ModuleRtpRtcpImpl2::GeneratePadding(size_t target_size_bytes) {
RTC_DCHECK(rtp_sender_);
-
- // `can_send_padding_on_media_ssrc` set to false but is ignored at this
- // point, RTPSender will internally query `sequencer_` while holding the
- // send lock.
RTC_DCHECK_RUN_ON(&pacer_thread_checker_);
+ MutexLock lock(&rtp_sender_->mutex_sequencer_);
// `can_send_padding_on_media_ssrc` set to false when deferred sequencing
// is off. It will be ignored in that case, RTPSender will internally query
diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl2.h b/modules/rtp_rtcp/source/rtp_rtcp_impl2.h
index 14d1409..b468bb7 100644
--- a/modules/rtp_rtcp/source/rtp_rtcp_impl2.h
+++ b/modules/rtp_rtcp/source/rtp_rtcp_impl2.h
@@ -269,8 +269,12 @@
// If false, sequencing is owned by `packet_generator` and can happen on
// several threads. If true, sequencing always happens on the pacer thread.
const bool deferred_sequencing_;
+ // TODO(bugs.webrtc.org/11340): Remove lock one we can guarantee that
+ // setting/getting rtp state only happens after removal from packet sending
+ // code path.
+ mutable Mutex mutex_sequencer_;
// Handles sequence number assignment and padding timestamp generation.
- PacketSequencer sequencer_;
+ PacketSequencer sequencer_ RTC_GUARDED_BY(mutex_sequencer_);
// Handles final time timestamping/stats/etc and handover to Transport.
RtpSenderEgress packet_sender;
// If no paced sender configured, this class will be used to pass packets
