admin/webrtc_m130
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Move AudioSendStream lifetime reporting into destructor

Browse Source 
This avoids a data race in which the lifetime TimeInterval is accessed
by the owning Call objects concurrently with SendRtp calls on the
underlying Channel object.

Bug: webrtc:8794
Change-Id: If53d5680095c0177656b659162457287cb8e45dd
Reviewed-on: https://webrtc-review.googlesource.com/46525
Commit-Queue: Sam Zackrisson <saza@webrtc.org>
Reviewed-by: Fredrik Solenberg <solenberg@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#21853}
This commit is contained in:
Sam Zackrisson 2018-02-01 16:53:16 +01:00 committed by Commit Bot
parent b90a64a449
commit 06953bac6d
4 changed files with 60 additions and 21 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

20
audio/audio_send_stream.cc
View File

@ -61,8 +61,7 @@ std::unique_ptr<voe::ChannelProxy> CreateChannelAndProxy(
}
} // namespace
// TODO(saza): Move this declaration further down when we can use
// std::make_unique.
// Helper class to track the actively sending lifetime of this stream.
class AudioSendStream::TimedTransport : public Transport {
public:
TimedTransport(Transport* transport, TimeInterval* time_interval)
@ -94,7 +93,8 @@ AudioSendStream::AudioSendStream(
BitrateAllocator* bitrate_allocator,
RtcEventLog* event_log,
RtcpRttStats* rtcp_rtt_stats,
const rtc::Optional<RtpState>& suspended_rtp_state)
const rtc::Optional<RtpState>& suspended_rtp_state,
TimeInterval* overall_call_lifetime)
: AudioSendStream(config,
audio_state,
worker_queue,
@ -103,6 +103,7 @@ AudioSendStream::AudioSendStream(
event_log,
rtcp_rtt_stats,
suspended_rtp_state,
overall_call_lifetime,
CreateChannelAndProxy(audio_state.get(),
worker_queue,
module_process_thread)) {}
@ -116,6 +117,7 @@ AudioSendStream::AudioSendStream(
RtcEventLog* event_log,
RtcpRttStats* rtcp_rtt_stats,
const rtc::Optional<RtpState>& suspended_rtp_state,
TimeInterval* overall_call_lifetime,
std::unique_ptr<voe::ChannelProxy> channel_proxy)
: worker_queue_(worker_queue),
config_(Config(nullptr)),
@ -128,7 +130,8 @@ AudioSendStream::AudioSendStream(
kPacketLossRateMinNumAckedPackets,
kRecoverablePacketLossRateMinNumAckedPairs),
rtp_rtcp_module_(nullptr),
suspended_rtp_state_(suspended_rtp_state) {
suspended_rtp_state_(suspended_rtp_state),
overall_call_lifetime_(overall_call_lifetime) {
RTC_LOG(LS_INFO) << "AudioSendStream: " << config.rtp.ssrc;
RTC_DCHECK(worker_queue_);
RTC_DCHECK(audio_state_);
@ -136,6 +139,7 @@ AudioSendStream::AudioSendStream(
RTC_DCHECK(bitrate_allocator_);
RTC_DCHECK(transport);
RTC_DCHECK(transport->send_side_cc());
RTC_DCHECK(overall_call_lifetime_);
channel_proxy_->SetRtcEventLog(event_log_);
channel_proxy_->SetRtcpRttStats(rtcp_rtt_stats);
@ -160,6 +164,10 @@ AudioSendStream::~AudioSendStream() {
channel_proxy_->ResetSenderCongestionControlObjects();
channel_proxy_->SetRtcEventLog(nullptr);
channel_proxy_->SetRtcpRttStats(nullptr);
// Lifetime can only be updated after deregistering
// |timed_send_transport_adapter_| in the underlying channel object to avoid
// data races in |active_lifetime_|.
overall_call_lifetime_->Extend(active_lifetime_);
}
const webrtc::AudioSendStream::Config& AudioSendStream::GetConfig() const {
@ -456,10 +464,6 @@ RtpState AudioSendStream::GetRtpState() const {
return rtp_rtcp_module_->GetRtpState();
}
const TimeInterval& AudioSendStream::GetActiveLifetime() const {
return active_lifetime_;
}
const voe::ChannelProxy& AudioSendStream::GetChannelProxy() const {
RTC_DCHECK(channel_proxy_.get());
return *channel_proxy_.get();

6
audio/audio_send_stream.h
View File

@ -49,7 +49,8 @@ class AudioSendStream final : public webrtc::AudioSendStream,
BitrateAllocator* bitrate_allocator,
RtcEventLog* event_log,
RtcpRttStats* rtcp_rtt_stats,
const rtc::Optional<RtpState>& suspended_rtp_state);
const rtc::Optional<RtpState>& suspended_rtp_state,
TimeInterval* overall_call_lifetime);
// For unit tests, which need to supply a mock channel proxy.
AudioSendStream(const webrtc::AudioSendStream::Config& config,
const rtc::scoped_refptr<webrtc::AudioState>& audio_state,
@ -59,6 +60,7 @@ class AudioSendStream final : public webrtc::AudioSendStream,
RtcEventLog* event_log,
RtcpRttStats* rtcp_rtt_stats,
const rtc::Optional<RtpState>& suspended_rtp_state,
TimeInterval* overall_call_lifetime,
std::unique_ptr<voe::ChannelProxy> channel_proxy);
~AudioSendStream() override;
@ -92,7 +94,6 @@ class AudioSendStream final : public webrtc::AudioSendStream,
void SetTransportOverhead(int transport_overhead_per_packet);
RtpState GetRtpState() const;
const TimeInterval& GetActiveLifetime() const;
const voe::ChannelProxy& GetChannelProxy() const;
private:
@ -148,6 +149,7 @@ class AudioSendStream final : public webrtc::AudioSendStream,
std::unique_ptr<TimedTransport> timed_send_transport_adapter_;
TimeInterval active_lifetime_;
TimeInterval* overall_call_lifetime_ = nullptr;
// RFC 5285: Each distinct extension MUST have a unique ID. The value 0 is
// reserved for padding and MUST NOT be used as a local identifier.

51
audio/audio_send_stream_unittest.cc
View File

@ -28,11 +28,14 @@
#include "modules/pacing/mock/mock_paced_sender.h"
#include "modules/rtp_rtcp/mocks/mock_rtcp_rtt_stats.h"
#include "modules/rtp_rtcp/mocks/mock_rtp_rtcp.h"
#include "rtc_base/fakeclock.h"
#include "rtc_base/ptr_util.h"
#include "rtc_base/task_queue.h"
#include "rtc_base/timedelta.h"
#include "test/gtest.h"
#include "test/mock_audio_encoder.h"
#include "test/mock_audio_encoder_factory.h"
#include "test/mock_transport.h"
namespace webrtc {
namespace test {
@ -168,14 +171,9 @@ struct ConfigHelper {
std::unique_ptr<internal::AudioSendStream> CreateAudioSendStream() {
return std::unique_ptr<internal::AudioSendStream>(
new internal::AudioSendStream(
stream_config_,
audio_state_,
&worker_queue_,
&fake_transport_,
&bitrate_allocator_,
&event_log_,
&rtcp_rtt_stats_,
rtc::nullopt,
stream_config_, audio_state_, &worker_queue_, &fake_transport_,
&bitrate_allocator_, &event_log_, &rtcp_rtt_stats_, rtc::nullopt,
&active_lifetime_,
std::unique_ptr<voe::ChannelProxy>(channel_proxy_)));
}
@ -186,6 +184,7 @@ struct ConfigHelper {
}
MockVoEChannelProxy* channel_proxy() { return channel_proxy_; }
RtpTransportControllerSendInterface* transport() { return &fake_transport_; }
TimeInterval* active_lifetime() { return &active_lifetime_; }
static void AddBweToConfig(AudioSendStream::Config* config) {
config->rtp.extensions.push_back(
@ -223,7 +222,11 @@ struct ConfigHelper {
}
EXPECT_CALL(*channel_proxy_, ResetSenderCongestionControlObjects())
.Times(1);
EXPECT_CALL(*channel_proxy_, RegisterTransport(nullptr)).Times(2);
{
::testing::InSequence unregister_on_destruction;
EXPECT_CALL(*channel_proxy_, RegisterTransport(_)).Times(1);
EXPECT_CALL(*channel_proxy_, RegisterTransport(nullptr)).Times(1);
}
EXPECT_CALL(*channel_proxy_, SetRtcEventLog(testing::NotNull())).Times(1);
EXPECT_CALL(*channel_proxy_, SetRtcEventLog(testing::IsNull()))
.Times(1); // Destructor resets the event log
@ -308,6 +311,7 @@ struct ConfigHelper {
rtc::scoped_refptr<MockAudioProcessing> audio_processing_;
AudioProcessingStats audio_processing_stats_;
SimulatedClock simulated_clock_;
TimeInterval active_lifetime_;
PacketRouter packet_router_;
testing::NiceMock<MockPacedSender> pacer_;
std::unique_ptr<SendSideCongestionController> send_side_cc_;
@ -526,5 +530,34 @@ TEST(AudioSendStreamTest, ReconfigureTransportCcResetsFirst) {
}
send_stream->Reconfigure(new_config);
}
// Checks that AudioSendStream logs the times at which RTP packets are sent
// through its interface.
TEST(AudioSendStreamTest, UpdateLifetime) {
ConfigHelper helper(false, true);
MockTransport mock_transport;
helper.config().send_transport = &mock_transport;
Transport* registered_transport;
ON_CALL(*helper.channel_proxy(), RegisterTransport(_))
.WillByDefault(Invoke([&registered_transport](Transport* transport) {
registered_transport = transport;
}));
rtc::ScopedFakeClock fake_clock;
constexpr int64_t kTimeBetweenSendRtpCallsMs = 100;
{
auto send_stream = helper.CreateAudioSendStream();
EXPECT_CALL(mock_transport, SendRtp(_, _, _)).Times(2);
const PacketOptions options;
registered_transport->SendRtp(nullptr, 0, options);
fake_clock.AdvanceTime(
rtc::TimeDelta::FromMilliseconds(kTimeBetweenSendRtpCallsMs));
registered_transport->SendRtp(nullptr, 0, options);
}
EXPECT_TRUE(!helper.active_lifetime()->Empty());
EXPECT_EQ(helper.active_lifetime()->Length(), kTimeBetweenSendRtpCallsMs);
}
} // namespace test
} // namespace webrtc

4
call/call.cc
View File

@ -618,7 +618,8 @@ webrtc::AudioSendStream* Call::CreateAudioSendStream(
AudioSendStream* send_stream = new AudioSendStream(
config, config_.audio_state, &worker_queue_, module_process_thread_.get(),
transport_send_.get(), bitrate_allocator_.get(), event_log_,
call_stats_->rtcp_rtt_stats(), suspended_rtp_state);
call_stats_->rtcp_rtt_stats(), suspended_rtp_state,
&sent_rtp_audio_timer_ms_);
{
WriteLockScoped write_lock(*send_crit_);
RTC_DCHECK(audio_send_ssrcs_.find(config.rtp.ssrc) ==
@ -663,7 +664,6 @@ void Call::DestroyAudioSendStream(webrtc::AudioSendStream* send_stream) {
}
}
UpdateAggregateNetworkState();
sent_rtp_audio_timer_ms_.Extend(audio_send_stream->GetActiveLifetime());
delete send_stream;
}