From 6cab5c8718d082b3045a08e07e5d47aec833c551 Mon Sep 17 00:00:00 2001 From: Karl Wiberg Date: Tue, 26 Mar 2019 09:57:01 +0100 Subject: [PATCH] Add thread safety annotations for some more PeerConnection members (part 5) Plus all the annotations that were necessary to make things compile again. We needed a special twist for call_. The value it points to is owned by the worker thread, but the signal thread needs to read the pointer. We could have made the pointer const, except that we explicitly reset it in the destructor (in an invoke to the worker thread). Bug: webrtc:9987 Change-Id: I31f024547f4be0e50967133b0d452c80ae38d7ed Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/128863 Commit-Queue: Karl Wiberg Reviewed-by: Steve Anton Cr-Commit-Position: refs/heads/master@{#27278} --- pc/peer_connection.cc | 28 ++++++++++++++++++---------- pc/peer_connection.h | 16 ++++++++++++---- 2 files changed, 30 insertions(+), 14 deletions(-) diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc index 3faacc9238..28cf957481 100644 --- a/pc/peer_connection.cc +++ b/pc/peer_connection.cc @@ -875,7 +875,8 @@ PeerConnection::PeerConnection(PeerConnectionFactory* factory, rtcp_cname_(GenerateRtcpCname()), local_streams_(StreamCollection::Create()), remote_streams_(StreamCollection::Create()), - call_(std::move(call)) {} + call_(std::move(call)), + call_ptr_(call_.get()) {} PeerConnection::~PeerConnection() { TRACE_EVENT0("webrtc", "PeerConnection::~PeerConnection"); @@ -1618,6 +1619,7 @@ PeerConnection::CreateSender( rtc::scoped_refptr track, const std::vector& stream_ids, const std::vector& send_encodings) { + RTC_DCHECK_RUN_ON(signaling_thread()); rtc::scoped_refptr> sender; if (media_type == cricket::MEDIA_TYPE_AUDIO) { RTC_DCHECK(!track || @@ -1793,7 +1795,7 @@ bool PeerConnection::GetStats(StatsObserver* observer, MediaStreamTrackInterface* track, StatsOutputLevel level) { TRACE_EVENT0("webrtc", "PeerConnection::GetStats"); - RTC_DCHECK(signaling_thread()->IsCurrent()); + RTC_DCHECK_RUN_ON(signaling_thread()); if (!observer) { RTC_LOG(LS_ERROR) << "GetStats - observer is NULL."; return false; @@ -1814,6 +1816,7 @@ bool PeerConnection::GetStats(StatsObserver* observer, void PeerConnection::GetStats(RTCStatsCollectorCallback* callback) { TRACE_EVENT0("webrtc", "PeerConnection::GetStats"); + RTC_DCHECK_RUN_ON(signaling_thread()); RTC_DCHECK(stats_collector_); RTC_DCHECK(callback); stats_collector_->GetStatsReport(callback); @@ -1823,6 +1826,7 @@ void PeerConnection::GetStats( rtc::scoped_refptr selector, rtc::scoped_refptr callback) { TRACE_EVENT0("webrtc", "PeerConnection::GetStats"); + RTC_DCHECK_RUN_ON(signaling_thread()); RTC_DCHECK(callback); RTC_DCHECK(stats_collector_); rtc::scoped_refptr internal_sender; @@ -1851,6 +1855,7 @@ void PeerConnection::GetStats( rtc::scoped_refptr selector, rtc::scoped_refptr callback) { TRACE_EVENT0("webrtc", "PeerConnection::GetStats"); + RTC_DCHECK_RUN_ON(signaling_thread()); RTC_DCHECK(callback); RTC_DCHECK(stats_collector_); rtc::scoped_refptr internal_receiver; @@ -3582,6 +3587,7 @@ RTCError PeerConnection::SetBitrate(const BitrateSettings& bitrate) { return worker_thread()->Invoke( RTC_FROM_HERE, [&]() { return SetBitrate(bitrate); }); } + RTC_DCHECK_RUN_ON(worker_thread()); const bool has_min = bitrate.min_bitrate_bps.has_value(); const bool has_start = bitrate.start_bitrate_bps.has_value(); @@ -3621,17 +3627,17 @@ RTCError PeerConnection::SetBitrate(const BitrateSettings& bitrate) { void PeerConnection::SetBitrateAllocationStrategy( std::unique_ptr bitrate_allocation_strategy) { - rtc::Thread* worker_thread = factory_->worker_thread(); - if (!worker_thread->IsCurrent()) { + if (!worker_thread()->IsCurrent()) { rtc::BitrateAllocationStrategy* strategy_raw = bitrate_allocation_strategy.release(); - auto functor = [this, strategy_raw]() { + worker_thread()->Invoke(RTC_FROM_HERE, [this, strategy_raw]() { + RTC_DCHECK_RUN_ON(worker_thread()); call_->SetBitrateAllocationStrategy( absl::WrapUnique(strategy_raw)); - }; - worker_thread->Invoke(RTC_FROM_HERE, functor); + }); return; } + RTC_DCHECK_RUN_ON(worker_thread()); RTC_DCHECK(call_.get()); call_->SetBitrateAllocationStrategy(std::move(bitrate_allocation_strategy)); } @@ -6232,7 +6238,7 @@ cricket::VoiceChannel* PeerConnection::CreateVoiceChannel( } cricket::VoiceChannel* voice_channel = channel_manager()->CreateVoiceChannel( - call_.get(), configuration_.media_config, rtp_transport, media_transport, + call_ptr_, configuration_.media_config, rtp_transport, media_transport, signaling_thread(), mid, SrtpRequired(), GetCryptoOptions(), &ssrc_generator_, audio_options_); if (!voice_channel) { @@ -6257,7 +6263,7 @@ cricket::VideoChannel* PeerConnection::CreateVideoChannel( } cricket::VideoChannel* video_channel = channel_manager()->CreateVideoChannel( - call_.get(), configuration_.media_config, rtp_transport, media_transport, + call_ptr_, configuration_.media_config, rtp_transport, media_transport, signaling_thread(), mid, SrtpRequired(), GetCryptoOptions(), &ssrc_generator_, video_options_); if (!video_channel) { @@ -6326,6 +6332,7 @@ Call::Stats PeerConnection::GetCallStats() { return worker_thread()->Invoke( RTC_FROM_HERE, rtc::Bind(&PeerConnection::GetCallStats, this)); } + RTC_DCHECK_RUN_ON(worker_thread()); if (call_) { return call_->GetStats(); } else { @@ -6963,7 +6970,7 @@ void PeerConnection::ReportNegotiatedCiphers( } void PeerConnection::OnSentPacket_w(const rtc::SentPacket& sent_packet) { - RTC_DCHECK(worker_thread()->IsCurrent()); + RTC_DCHECK_RUN_ON(worker_thread()); RTC_DCHECK(call_); call_->OnSentPacket(sent_packet); } @@ -7089,6 +7096,7 @@ CryptoOptions PeerConnection::GetCryptoOptions() { } void PeerConnection::ClearStatsCache() { + RTC_DCHECK_RUN_ON(signaling_thread()); if (stats_collector_) { stats_collector_->ClearCachedStatsReport(); } diff --git a/pc/peer_connection.h b/pc/peer_connection.h index 5fa8da5811..9c2222ac60 100644 --- a/pc/peer_connection.h +++ b/pc/peer_connection.h @@ -1030,7 +1030,8 @@ class PeerConnection : public PeerConnectionInternal, bool OnTransportChanged(const std::string& mid, RtpTransportInternal* rtp_transport, rtc::scoped_refptr dtls_transport, - MediaTransportInterface* media_transport) override; + MediaTransportInterface* media_transport) override + RTC_RUN_ON(worker_thread()); // Returns the observer. Will crash on CHECK if the observer is removed. PeerConnectionObserver* Observer() const RTC_RUN_ON(signaling_thread()); @@ -1150,9 +1151,16 @@ class PeerConnection : public PeerConnectionInternal, bool remote_peer_supports_msid_ RTC_GUARDED_BY(signaling_thread()) = false; - std::unique_ptr call_; - std::unique_ptr stats_; // A pointer is passed to senders_ - rtc::scoped_refptr stats_collector_; + std::unique_ptr call_ RTC_GUARDED_BY(worker_thread()); + + // Points to the same thing as `call_`. Since it's const, we may read the + // pointer (but not touch the object) from any thread. + Call* const call_ptr_ RTC_PT_GUARDED_BY(worker_thread()); + + std::unique_ptr stats_ + RTC_GUARDED_BY(signaling_thread()); // A pointer is passed to senders_ + rtc::scoped_refptr stats_collector_ + RTC_GUARDED_BY(signaling_thread()); std::vector< rtc::scoped_refptr>>