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

Delete most use of accessor_lock_ in JsepTransport.

Browse Source 
Most members it used to protect or now either const, or accessed on
network thread only.

Followup to https://webrtc-review.googlesource.com/c/src/+/204801.

Bug: webrtc:11567
Change-Id: I1bc80555885a8d8e9f7282d5adf93a093879cc7e
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/205980
Commit-Queue: Niels Moller <nisse@webrtc.org>
Reviewed-by: Tommi <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#33178}
This commit is contained in:
Niels Möller
2021-02-05 12:34:14 +01:00
committed by Commit Bot
parent 4593047ee1
commit 6a48a1d80b
2 changed files with 95 additions and 137 deletions
Download Patch File Download Diff File Expand all files Collapse all files

42
pc/jsep_transport.cc
View File

@ -168,8 +168,6 @@ webrtc::RTCError JsepTransport::SetLocalJsepTransportDescription(
} }
// If doing SDES, setup the SDES crypto parameters. // If doing SDES, setup the SDES crypto parameters.
{
webrtc::MutexLock lock(&accessor_lock_);
if (sdes_transport_) { if (sdes_transport_) {
RTC_DCHECK(!unencrypted_rtp_transport_); RTC_DCHECK(!unencrypted_rtp_transport_);
RTC_DCHECK(!dtls_srtp_transport_); RTC_DCHECK(!dtls_srtp_transport_);
@ -185,7 +183,6 @@ webrtc::RTCError JsepTransport::SetLocalJsepTransportDescription(
dtls_srtp_transport_->UpdateRecvEncryptedHeaderExtensionIds( dtls_srtp_transport_->UpdateRecvEncryptedHeaderExtensionIds(
jsep_description.encrypted_header_extension_ids); jsep_description.encrypted_header_extension_ids);
} }
}
bool ice_restarting = bool ice_restarting =
local_description_ != nullptr && local_description_ != nullptr &&
IceCredentialsChanged(local_description_->transport_desc.ice_ufrag, IceCredentialsChanged(local_description_->transport_desc.ice_ufrag,
@ -205,12 +202,12 @@ webrtc::RTCError JsepTransport::SetLocalJsepTransportDescription(
return error; return error;
} }
} }
{
webrtc::MutexLock lock(&accessor_lock_);
RTC_DCHECK(rtp_dtls_transport_->internal()); RTC_DCHECK(rtp_dtls_transport_->internal());
rtp_dtls_transport_->internal()->ice_transport()->SetIceParameters( rtp_dtls_transport_->internal()->ice_transport()->SetIceParameters(
ice_parameters); ice_parameters);
{
webrtc::MutexLock lock(&accessor_lock_);
if (rtcp_dtls_transport_) { if (rtcp_dtls_transport_) {
RTC_DCHECK(rtcp_dtls_transport_->internal()); RTC_DCHECK(rtcp_dtls_transport_->internal());
rtcp_dtls_transport_->internal()->ice_transport()->SetIceParameters( rtcp_dtls_transport_->internal()->ice_transport()->SetIceParameters(
@ -260,8 +257,6 @@ webrtc::RTCError JsepTransport::SetRemoteJsepTransportDescription(
} }
// If doing SDES, setup the SDES crypto parameters. // If doing SDES, setup the SDES crypto parameters.
{
webrtc::MutexLock lock(&accessor_lock_);
if (sdes_transport_) { if (sdes_transport_) {
RTC_DCHECK(!unencrypted_rtp_transport_); RTC_DCHECK(!unencrypted_rtp_transport_);
RTC_DCHECK(!dtls_srtp_transport_); RTC_DCHECK(!dtls_srtp_transport_);
@ -281,7 +276,6 @@ webrtc::RTCError JsepTransport::SetRemoteJsepTransportDescription(
dtls_srtp_transport_->CacheRtpAbsSendTimeHeaderExtension( dtls_srtp_transport_->CacheRtpAbsSendTimeHeaderExtension(
jsep_description.rtp_abs_sendtime_extn_id); jsep_description.rtp_abs_sendtime_extn_id);
} }
}
remote_description_.reset(new JsepTransportDescription(jsep_description)); remote_description_.reset(new JsepTransportDescription(jsep_description));
RTC_DCHECK(rtp_dtls_transport()); RTC_DCHECK(rtp_dtls_transport());
@ -341,7 +335,6 @@ void JsepTransport::SetNeedsIceRestartFlag() {
absl::optional<rtc::SSLRole> JsepTransport::GetDtlsRole() const { absl::optional<rtc::SSLRole> JsepTransport::GetDtlsRole() const {
RTC_DCHECK_RUN_ON(network_thread_); RTC_DCHECK_RUN_ON(network_thread_);
webrtc::MutexLock lock(&accessor_lock_);
RTC_DCHECK(rtp_dtls_transport_); RTC_DCHECK(rtp_dtls_transport_);
RTC_DCHECK(rtp_dtls_transport_->internal()); RTC_DCHECK(rtp_dtls_transport_->internal());
rtc::SSLRole dtls_role; rtc::SSLRole dtls_role;
@ -354,14 +347,17 @@ absl::optional<rtc::SSLRole> JsepTransport::GetDtlsRole() const {
bool JsepTransport::GetStats(TransportStats* stats) { bool JsepTransport::GetStats(TransportStats* stats) {
RTC_DCHECK_RUN_ON(network_thread_); RTC_DCHECK_RUN_ON(network_thread_);
webrtc::MutexLock lock(&accessor_lock_);
stats->transport_name = mid(); stats->transport_name = mid();
stats->channel_stats.clear(); stats->channel_stats.clear();
RTC_DCHECK(rtp_dtls_transport_->internal()); RTC_DCHECK(rtp_dtls_transport_->internal());
bool ret = GetTransportStats(rtp_dtls_transport_->internal(), stats); bool ret = GetTransportStats(rtp_dtls_transport_->internal(),
ICE_CANDIDATE_COMPONENT_RTP, stats);
webrtc::MutexLock lock(&accessor_lock_);
if (rtcp_dtls_transport_) { if (rtcp_dtls_transport_) {
RTC_DCHECK(rtcp_dtls_transport_->internal()); RTC_DCHECK(rtcp_dtls_transport_->internal());
ret &= GetTransportStats(rtcp_dtls_transport_->internal(), stats); ret &= GetTransportStats(rtcp_dtls_transport_->internal(),
ICE_CANDIDATE_COMPONENT_RTCP, stats);
} }
return ret; return ret;
} }
@ -396,7 +392,6 @@ webrtc::RTCError JsepTransport::VerifyCertificateFingerprint(
void JsepTransport::SetActiveResetSrtpParams(bool active_reset_srtp_params) { void JsepTransport::SetActiveResetSrtpParams(bool active_reset_srtp_params) {
RTC_DCHECK_RUN_ON(network_thread_); RTC_DCHECK_RUN_ON(network_thread_);
webrtc::MutexLock lock(&accessor_lock_);
if (dtls_srtp_transport_) { if (dtls_srtp_transport_) {
RTC_LOG(INFO) RTC_LOG(INFO)
<< "Setting active_reset_srtp_params of DtlsSrtpTransport to: " << "Setting active_reset_srtp_params of DtlsSrtpTransport to: "
@ -471,14 +466,8 @@ bool JsepTransport::SetRtcpMux(bool enable,
} }
void JsepTransport::ActivateRtcpMux() { void JsepTransport::ActivateRtcpMux() {
{
// Don't hold the network_thread_ lock while calling other functions,
// since they might call other functions that call RTC_DCHECK_RUN_ON.
// TODO(https://crbug.com/webrtc/10318): Simplify when possible.
RTC_DCHECK_RUN_ON(network_thread_); RTC_DCHECK_RUN_ON(network_thread_);
}
{
webrtc::MutexLock lock(&accessor_lock_);
if (unencrypted_rtp_transport_) { if (unencrypted_rtp_transport_) {
RTC_DCHECK(!sdes_transport_); RTC_DCHECK(!sdes_transport_);
RTC_DCHECK(!dtls_srtp_transport_); RTC_DCHECK(!dtls_srtp_transport_);
@ -491,9 +480,11 @@ void JsepTransport::ActivateRtcpMux() {
RTC_DCHECK(dtls_srtp_transport_); RTC_DCHECK(dtls_srtp_transport_);
RTC_DCHECK(!unencrypted_rtp_transport_); RTC_DCHECK(!unencrypted_rtp_transport_);
RTC_DCHECK(!sdes_transport_); RTC_DCHECK(!sdes_transport_);
dtls_srtp_transport_->SetDtlsTransports(rtp_dtls_transport_locked(), dtls_srtp_transport_->SetDtlsTransports(rtp_dtls_transport(),
/*rtcp_dtls_transport=*/nullptr); /*rtcp_dtls_transport=*/nullptr);
} }
{
webrtc::MutexLock lock(&accessor_lock_);
rtcp_dtls_transport_ = nullptr; // Destroy this reference. rtcp_dtls_transport_ = nullptr; // Destroy this reference.
} }
// Notify the JsepTransportController to update the aggregate states. // Notify the JsepTransportController to update the aggregate states.
@ -687,17 +678,12 @@ webrtc::RTCError JsepTransport::NegotiateDtlsRole(
} }
bool JsepTransport::GetTransportStats(DtlsTransportInternal* dtls_transport, bool JsepTransport::GetTransportStats(DtlsTransportInternal* dtls_transport,
int component,
TransportStats* stats) { TransportStats* stats) {
RTC_DCHECK_RUN_ON(network_thread_); RTC_DCHECK_RUN_ON(network_thread_);
RTC_DCHECK(dtls_transport); RTC_DCHECK(dtls_transport);
TransportChannelStats substats; TransportChannelStats substats;
if (rtcp_dtls_transport_) { substats.component = component;
substats.component = dtls_transport == rtcp_dtls_transport_->internal()
? ICE_CANDIDATE_COMPONENT_RTCP
: ICE_CANDIDATE_COMPONENT_RTP;
} else {
substats.component = ICE_CANDIDATE_COMPONENT_RTP;
}
dtls_transport->GetSslVersionBytes(&substats.ssl_version_bytes); dtls_transport->GetSslVersionBytes(&substats.ssl_version_bytes);
dtls_transport->GetSrtpCryptoSuite(&substats.srtp_crypto_suite); dtls_transport->GetSrtpCryptoSuite(&substats.srtp_crypto_suite);
dtls_transport->GetSslCipherSuite(&substats.ssl_cipher_suite); dtls_transport->GetSslCipherSuite(&substats.ssl_cipher_suite);

100
pc/jsep_transport.h
View File

@ -131,9 +131,8 @@ class JsepTransport : public sigslot::has_slots<> {
// that are part of this Transport. // that are part of this Transport.
webrtc::RTCError SetRemoteJsepTransportDescription( webrtc::RTCError SetRemoteJsepTransportDescription(
const JsepTransportDescription& jsep_description, const JsepTransportDescription& jsep_description,
webrtc::SdpType type) RTC_LOCKS_EXCLUDED(accessor_lock_); webrtc::SdpType type);
webrtc::RTCError AddRemoteCandidates(const Candidates& candidates) webrtc::RTCError AddRemoteCandidates(const Candidates& candidates);
RTC_LOCKS_EXCLUDED(accessor_lock_);
// Set the "needs-ice-restart" flag as described in JSEP. After the flag is // Set the "needs-ice-restart" flag as described in JSEP. After the flag is
// set, offers should generate new ufrags/passwords until an ICE restart // set, offers should generate new ufrags/passwords until an ICE restart
@ -152,8 +151,7 @@ class JsepTransport : public sigslot::has_slots<> {
// Returns role if negotiated, or empty absl::optional if it hasn't been // Returns role if negotiated, or empty absl::optional if it hasn't been
// negotiated yet. // negotiated yet.
absl::optional<rtc::SSLRole> GetDtlsRole() const absl::optional<rtc::SSLRole> GetDtlsRole() const;
RTC_LOCKS_EXCLUDED(accessor_lock_);
// TODO(deadbeef): Make this const. See comment in transportcontroller.h. // TODO(deadbeef): Make this const. See comment in transportcontroller.h.
bool GetStats(TransportStats* stats) RTC_LOCKS_EXCLUDED(accessor_lock_); bool GetStats(TransportStats* stats) RTC_LOCKS_EXCLUDED(accessor_lock_);
@ -168,26 +166,32 @@ class JsepTransport : public sigslot::has_slots<> {
return remote_description_.get(); return remote_description_.get();
} }
webrtc::RtpTransportInternal* rtp_transport() const // Returns the rtp transport, if any.
RTC_LOCKS_EXCLUDED(accessor_lock_) { webrtc::RtpTransportInternal* rtp_transport() const {
webrtc::MutexLock lock(&accessor_lock_); if (dtls_srtp_transport_) {
return default_rtp_transport(); return dtls_srtp_transport_.get();
}
if (sdes_transport_) {
return sdes_transport_.get();
}
if (unencrypted_rtp_transport_) {
return unencrypted_rtp_transport_.get();
} }
const DtlsTransportInternal* rtp_dtls_transport() const
RTC_LOCKS_EXCLUDED(accessor_lock_) {
webrtc::MutexLock lock(&accessor_lock_);
if (rtp_dtls_transport_) {
return rtp_dtls_transport_->internal();
} else {
return nullptr; return nullptr;
} }
const DtlsTransportInternal* rtp_dtls_transport() const {
if (rtp_dtls_transport_) {
return rtp_dtls_transport_->internal();
}
return nullptr;
} }
DtlsTransportInternal* rtp_dtls_transport() DtlsTransportInternal* rtp_dtls_transport() {
RTC_LOCKS_EXCLUDED(accessor_lock_) { if (rtp_dtls_transport_) {
webrtc::MutexLock lock(&accessor_lock_); return rtp_dtls_transport_->internal();
return rtp_dtls_transport_locked(); }
return nullptr;
} }
const DtlsTransportInternal* rtcp_dtls_transport() const const DtlsTransportInternal* rtcp_dtls_transport() const
@ -195,9 +199,8 @@ class JsepTransport : public sigslot::has_slots<> {
webrtc::MutexLock lock(&accessor_lock_); webrtc::MutexLock lock(&accessor_lock_);
if (rtcp_dtls_transport_) { if (rtcp_dtls_transport_) {
return rtcp_dtls_transport_->internal(); return rtcp_dtls_transport_->internal();
} else {
return nullptr;
} }
return nullptr;
} }
DtlsTransportInternal* rtcp_dtls_transport() DtlsTransportInternal* rtcp_dtls_transport()
@ -205,28 +208,21 @@ class JsepTransport : public sigslot::has_slots<> {
webrtc::MutexLock lock(&accessor_lock_); webrtc::MutexLock lock(&accessor_lock_);
if (rtcp_dtls_transport_) { if (rtcp_dtls_transport_) {
return rtcp_dtls_transport_->internal(); return rtcp_dtls_transport_->internal();
} else { }
return nullptr; return nullptr;
} }
}
rtc::scoped_refptr<webrtc::DtlsTransport> RtpDtlsTransport() rtc::scoped_refptr<webrtc::DtlsTransport> RtpDtlsTransport() {
RTC_LOCKS_EXCLUDED(accessor_lock_) {
webrtc::MutexLock lock(&accessor_lock_);
return rtp_dtls_transport_; return rtp_dtls_transport_;
} }
rtc::scoped_refptr<webrtc::SctpTransport> SctpTransport() const rtc::scoped_refptr<webrtc::SctpTransport> SctpTransport() const {
RTC_LOCKS_EXCLUDED(accessor_lock_) {
webrtc::MutexLock lock(&accessor_lock_);
return sctp_transport_; return sctp_transport_;
} }
// TODO(bugs.webrtc.org/9719): Delete method, update callers to use // TODO(bugs.webrtc.org/9719): Delete method, update callers to use
// SctpTransport() instead. // SctpTransport() instead.
webrtc::DataChannelTransportInterface* data_channel_transport() const webrtc::DataChannelTransportInterface* data_channel_transport() const {
RTC_LOCKS_EXCLUDED(accessor_lock_) {
webrtc::MutexLock lock(&accessor_lock_);
if (sctp_data_channel_transport_) { if (sctp_data_channel_transport_) {
return sctp_data_channel_transport_.get(); return sctp_data_channel_transport_.get();
} }
@ -248,19 +244,9 @@ class JsepTransport : public sigslot::has_slots<> {
const rtc::RTCCertificate* certificate, const rtc::RTCCertificate* certificate,
const rtc::SSLFingerprint* fingerprint) const; const rtc::SSLFingerprint* fingerprint) const;
void SetActiveResetSrtpParams(bool active_reset_srtp_params) void SetActiveResetSrtpParams(bool active_reset_srtp_params);
RTC_LOCKS_EXCLUDED(accessor_lock_);
private: private:
DtlsTransportInternal* rtp_dtls_transport_locked()
RTC_EXCLUSIVE_LOCKS_REQUIRED(accessor_lock_) {
if (rtp_dtls_transport_) {
return rtp_dtls_transport_->internal();
} else {
return nullptr;
}
}
bool SetRtcpMux(bool enable, webrtc::SdpType type, ContentSource source); bool SetRtcpMux(bool enable, webrtc::SdpType type, ContentSource source);
void ActivateRtcpMux() RTC_LOCKS_EXCLUDED(accessor_lock_); void ActivateRtcpMux() RTC_LOCKS_EXCLUDED(accessor_lock_);
@ -268,8 +254,7 @@ class JsepTransport : public sigslot::has_slots<> {
bool SetSdes(const std::vector<CryptoParams>& cryptos, bool SetSdes(const std::vector<CryptoParams>& cryptos,
const std::vector<int>& encrypted_extension_ids, const std::vector<int>& encrypted_extension_ids,
webrtc::SdpType type, webrtc::SdpType type,
ContentSource source) ContentSource source);
RTC_EXCLUSIVE_LOCKS_REQUIRED(accessor_lock_);
// Negotiates and sets the DTLS parameters based on the current local and // Negotiates and sets the DTLS parameters based on the current local and
// remote transport description, such as the DTLS role to use, and whether // remote transport description, such as the DTLS role to use, and whether
@ -286,8 +271,7 @@ class JsepTransport : public sigslot::has_slots<> {
webrtc::SdpType local_description_type, webrtc::SdpType local_description_type,
ConnectionRole local_connection_role, ConnectionRole local_connection_role,
ConnectionRole remote_connection_role, ConnectionRole remote_connection_role,
absl::optional<rtc::SSLRole>* negotiated_dtls_role) absl::optional<rtc::SSLRole>* negotiated_dtls_role);
RTC_LOCKS_EXCLUDED(accessor_lock_);
// Pushes down the ICE parameters from the remote description. // Pushes down the ICE parameters from the remote description.
void SetRemoteIceParameters(const IceParameters& ice_parameters, void SetRemoteIceParameters(const IceParameters& ice_parameters,
@ -300,22 +284,8 @@ class JsepTransport : public sigslot::has_slots<> {
rtc::SSLFingerprint* remote_fingerprint); rtc::SSLFingerprint* remote_fingerprint);
bool GetTransportStats(DtlsTransportInternal* dtls_transport, bool GetTransportStats(DtlsTransportInternal* dtls_transport,
TransportStats* stats) int component,
RTC_EXCLUSIVE_LOCKS_REQUIRED(accessor_lock_); TransportStats* stats);
// Returns the default (non-datagram) rtp transport, if any.
webrtc::RtpTransportInternal* default_rtp_transport() const
RTC_EXCLUSIVE_LOCKS_REQUIRED(accessor_lock_) {
if (dtls_srtp_transport_) {
return dtls_srtp_transport_.get();
} else if (sdes_transport_) {
return sdes_transport_.get();
} else if (unencrypted_rtp_transport_) {
return unencrypted_rtp_transport_.get();
} else {
return nullptr;
}
}
// Owning thread, for safety checks // Owning thread, for safety checks
const rtc::Thread* const network_thread_; const rtc::Thread* const network_thread_;
@ -345,6 +315,8 @@ class JsepTransport : public sigslot::has_slots<> {
const std::unique_ptr<webrtc::DtlsSrtpTransport> dtls_srtp_transport_; const std::unique_ptr<webrtc::DtlsSrtpTransport> dtls_srtp_transport_;
const rtc::scoped_refptr<webrtc::DtlsTransport> rtp_dtls_transport_; const rtc::scoped_refptr<webrtc::DtlsTransport> rtp_dtls_transport_;
// TODO(tommi): Restrict use to network thread, or make const. And delete the
// `accessor_lock_`.
rtc::scoped_refptr<webrtc::DtlsTransport> rtcp_dtls_transport_ rtc::scoped_refptr<webrtc::DtlsTransport> rtcp_dtls_transport_
RTC_GUARDED_BY(accessor_lock_); RTC_GUARDED_BY(accessor_lock_);