zqz/platform-external-webrtc
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Prevent concurrent access to AudioSendStream's configuration.

Browse Source 
By design:
 * OnPacketAdded() is meant to be called on pacer thread.
 * Reconfigure()   is meant to be called on worker thread.
Thus we guard against race condition on config_ member.

Possible downside: packet filtering based on ssrc might be slowed down.

Bug: webrtc:9849
Change-Id: I734bb9b34b01db160705897adb1b58e866e12639
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/146980
Commit-Queue: Yves Gerey <yvesg@google.com>
Reviewed-by: Oskar Sundbom <ossu@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#28691}
This commit is contained in:
Yves Gerey
2019-07-26 17:49:52 +02:00
committed by Commit Bot
parent 378cae2543
commit 1704801226
3 changed files with 38 additions and 1 deletions
Download Patch File Download Diff File Expand all files Collapse all files

14
audio/audio_send_stream.cc
View File

@ -236,6 +236,8 @@ void AudioSendStream::ConfigureStream(
const auto& channel_send = stream->channel_send_;
const auto& old_config = stream->config_;
stream->config_cs_.Enter();
// Configuration parameters which cannot be changed.
RTC_DCHECK(first_time ||
old_config.send_transport == new_config.send_transport);
@ -263,6 +265,9 @@ void AudioSendStream::ConfigureStream(
const ExtensionIds old_ids = FindExtensionIds(old_config.rtp.extensions);
const ExtensionIds new_ids = FindExtensionIds(new_config.rtp.extensions);
stream->config_cs_.Leave();
// Audio level indication
if (first_time || new_ids.audio_level != old_ids.audio_level) {
channel_send->SetSendAudioLevelIndicationStatus(new_ids.audio_level != 0,
@ -299,6 +304,7 @@ void AudioSendStream::ConfigureStream(
stream->rtp_transport_, bandwidth_observer);
}
}
stream->config_cs_.Enter();
// MID RTP header extension.
if ((first_time || new_ids.mid != old_ids.mid ||
new_config.rtp.mid != old_config.rtp.mid) &&
@ -321,6 +327,7 @@ void AudioSendStream::ConfigureStream(
ReconfigureBitrateObserver(stream, new_config);
}
stream->config_ = new_config;
stream->config_cs_.Leave();
}
void AudioSendStream::Start() {
@ -487,7 +494,12 @@ uint32_t AudioSendStream::OnBitrateUpdated(BitrateAllocationUpdate update) {
void AudioSendStream::OnPacketAdded(uint32_t ssrc, uint16_t seq_num) {
RTC_DCHECK(pacer_thread_checker_.IsCurrent());
// Only packets that belong to this stream are of interest.
if (ssrc == config_.rtp.ssrc) {
bool same_ssrc;
{
rtc::CritScope lock(&config_cs_);
same_ssrc = ssrc == config_.rtp.ssrc;
}
if (same_ssrc) {
rtc::CritScope lock(&packet_loss_tracker_cs_);
// TODO(eladalon): This function call could potentially reset the window,
// setting both PLR and RPLR to unknown. Consider (during upcoming

1
audio/audio_send_stream.h
View File

@ -153,6 +153,7 @@ class AudioSendStream final : public webrtc::AudioSendStream,
rtc::RaceChecker audio_capture_race_checker_;
rtc::TaskQueue* worker_queue_;
const AudioAllocationSettings allocation_settings_;
rtc::CriticalSection config_cs_;
webrtc::AudioSendStream::Config config_;
rtc::scoped_refptr<webrtc::AudioState> audio_state_;
const std::unique_ptr<voe::ChannelSendInterface> channel_send_;

24
audio/audio_send_stream_unittest.cc
View File

@ -11,6 +11,7 @@
#include "audio/audio_send_stream.h"
#include <string>
#include <thread>
#include <utility>
#include <vector>
@ -679,6 +680,29 @@ TEST(AudioSendStreamTest, DontRecreateEncoder) {
send_stream->Reconfigure(helper.config());
}
// Allow to check for race conditions under tsan.
// This mimicks the situation where 'ModuleProcessThread' (pacer thread) is
// launched by webrtc::RtpTransportControllerSend::RtpTransportControllerSend().
TEST(AudioSendStreamTest, RaceFree) {
ConfigHelper helper(false, false);
// Sanity checks: copy-pasted from DontRecreateEncoder test.
EXPECT_CALL(*helper.channel_send(), SetEncoderForMock(_, _))
.WillOnce(Return());
EXPECT_CALL(*helper.channel_send(), RegisterCngPayloadType(105, 8000));
helper.config().send_codec_spec =
AudioSendStream::Config::SendCodecSpec(9, kG722Format);
helper.config().send_codec_spec->cng_payload_type = 105;
auto send_stream = helper.CreateAudioSendStream();
std::thread pacer([&]() {
send_stream->OnPacketAdded(/*ssrc*/ 0xcafe,
/*seq_num*/ 0xf00d);
});
send_stream->Reconfigure(helper.config());
pacer.join();
}
TEST(AudioSendStreamTest, ReconfigureTransportCcResetsFirst) {
ScopedFieldTrials field_trials("WebRTC-Audio-SendSideBwe/Enabled/");
ConfigHelper helper(false, true);