From 51e30837d5d6a32691336c94f4909918f883af6d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Erik=20Spr=C3=A5ng?= Date: Thu, 5 Aug 2021 16:15:14 +0200 Subject: [PATCH] Fix potential race in PacketSequencer. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The race can happen when an encoder thread is packetizing a video frame and is calling RTPSender::AssignSequenceNumber() while the RtpRtcp module is calling GeneratePadding() and querying PacketSequencer::CanSendPaddingOnMediaSsrc(). The solution for now is to simply not call PacketSequencer::CanSendPaddingOnMediaSsrc() from the RtpRtcp module, as that parameter will be ignored anyway - RTPSender will query that method internally while holding the send lock. Once deferred sequencing is implemented, the can_send_padding_on_media_ssrc parameter can be populated safely since it is then always called on the pacer thread. Bug: webrtc:11340, webrtc:12470 Change-Id: I9e90808166453d0e29746df89044e1d3bdffa286 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/227767 Reviewed-by: Tommi Commit-Queue: Erik Språng Cr-Commit-Position: refs/heads/master@{#34655} --- modules/rtp_rtcp/source/rtp_rtcp_impl.cc | 5 ++++- modules/rtp_rtcp/source/rtp_rtcp_impl2.cc | 5 ++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc index 52a16ec36d..f3e7e30c7c 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc @@ -444,9 +444,12 @@ bool ModuleRtpRtcpImpl::SupportsRtxPayloadPadding() const { std::vector> ModuleRtpRtcpImpl::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. return rtp_sender_->packet_generator.GeneratePadding( target_size_bytes, rtp_sender_->packet_sender.MediaHasBeenSent(), - rtp_sender_->sequencer_.CanSendPaddingOnMediaSsrc()); + /*can_send_padding_on_media_ssrc=*/false); } std::vector diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl2.cc b/modules/rtp_rtcp/source/rtp_rtcp_impl2.cc index 38fbf4590d..af64ddb269 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl2.cc +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl2.cc @@ -398,9 +398,12 @@ bool ModuleRtpRtcpImpl2::SupportsRtxPayloadPadding() const { std::vector> 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. return rtp_sender_->packet_generator.GeneratePadding( target_size_bytes, rtp_sender_->packet_sender.MediaHasBeenSent(), - rtp_sender_->sequencer_.CanSendPaddingOnMediaSsrc()); + /*can_send_padding_on_media_ssrc=*/false); } std::vector