From b496c3290118a9bd4c38d33220d36720632b2cc7 Mon Sep 17 00:00:00 2001 From: Tomas Gunnarsson Date: Wed, 5 Jan 2022 10:26:36 +0000 Subject: [PATCH] Enhance thread checks in BaseChannel classes. Improve consistency between using DCHECK checkers and compile time. For virtual methods, we were sometimes using both and in other cases we could be using compile time checks but were using runtime. Added annotation for last_send_params_, last_recv_params_ in audio/video channel classes. Also removing redundant logging for when registration with the transport fails. This is already being logged in the demuxer. Bug: webrtc:12230 Change-Id: I48e156c9996dec26a990151301dabc06673541d0 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/244095 Reviewed-by: Niels Moller Commit-Queue: Tomas Gunnarsson Cr-Commit-Position: refs/heads/main@{#35630} --- pc/channel.cc | 34 ++++++++------------------ pc/channel.h | 66 ++++++++++++++++++++++++++++++++------------------- 2 files changed, 51 insertions(+), 49 deletions(-) diff --git a/pc/channel.cc b/pc/channel.cc index eb7219cc84..925d459b41 100644 --- a/pc/channel.cc +++ b/pc/channel.cc @@ -157,14 +157,13 @@ std::string BaseChannel::ToString() const { return sb.Release(); } -bool BaseChannel::ConnectToRtpTransport() { +bool BaseChannel::ConnectToRtpTransport_n() { RTC_DCHECK(rtp_transport_); RTC_DCHECK(media_channel()); // We don't need to call OnDemuxerCriteriaUpdatePending/Complete because // there's no previous criteria to worry about. if (!rtp_transport_->RegisterRtpDemuxerSink(demuxer_criteria_, this)) { - RTC_LOG(LS_ERROR) << "Failed to set up demuxing for " << ToString(); return false; } rtp_transport_->SignalReadyToSend.connect( @@ -178,7 +177,7 @@ bool BaseChannel::ConnectToRtpTransport() { return true; } -void BaseChannel::DisconnectFromRtpTransport() { +void BaseChannel::DisconnectFromRtpTransport_n() { RTC_DCHECK(rtp_transport_); RTC_DCHECK(media_channel()); rtp_transport_->UnregisterRtpDemuxerSink(this); @@ -209,7 +208,7 @@ void BaseChannel::Deinit() { media_channel_->SetInterface(/*iface=*/nullptr); if (rtp_transport_) { - DisconnectFromRtpTransport(); + DisconnectFromRtpTransport_n(); } }); } @@ -222,14 +221,12 @@ bool BaseChannel::SetRtpTransport(webrtc::RtpTransportInternal* rtp_transport) { } if (rtp_transport_) { - DisconnectFromRtpTransport(); + DisconnectFromRtpTransport_n(); } rtp_transport_ = rtp_transport; if (rtp_transport_) { - if (!ConnectToRtpTransport()) { - RTC_LOG(LS_ERROR) << "Failed to connect to the new RtpTransport for " - << ToString() << "."; + if (!ConnectToRtpTransport_n()) { return false; } OnTransportReadyToSend(rtp_transport_->IsReadyToSend()); @@ -297,12 +294,6 @@ bool BaseChannel::SetPayloadTypeDemuxingEnabled(bool enabled) { return SetPayloadTypeDemuxingEnabled_w(enabled); } -bool BaseChannel::IsReadyToReceiveMedia_w() const { - // Receive data if we are enabled and have local content, - return enabled_ && - webrtc::RtpTransceiverDirectionHasRecv(local_content_direction_); -} - bool BaseChannel::IsReadyToSendMedia_w() const { // Send outgoing data if we are enabled, have local and remote content, // and we have had some form of connectivity. @@ -795,24 +786,23 @@ VoiceChannel::~VoiceChannel() { void VoiceChannel::UpdateMediaSendRecvState_w() { // Render incoming data if we're the active call, and we have the local // content. We receive data on the default channel and multiplexed streams. - RTC_DCHECK_RUN_ON(worker_thread()); - bool recv = IsReadyToReceiveMedia_w(); - media_channel()->SetPlayout(recv); + bool ready_to_receive = enabled() && webrtc::RtpTransceiverDirectionHasRecv( + local_content_direction()); + media_channel()->SetPlayout(ready_to_receive); // Send outgoing data if we're the active call, we have the remote content, // and we have had some form of connectivity. bool send = IsReadyToSendMedia_w(); media_channel()->SetSend(send); - RTC_LOG(LS_INFO) << "Changing voice state, recv=" << recv << " send=" << send - << " for " << ToString(); + RTC_LOG(LS_INFO) << "Changing voice state, recv=" << ready_to_receive + << " send=" << send << " for " << ToString(); } bool VoiceChannel::SetLocalContent_w(const MediaContentDescription* content, SdpType type, std::string* error_desc) { TRACE_EVENT0("webrtc", "VoiceChannel::SetLocalContent_w"); - RTC_DCHECK_RUN_ON(worker_thread()); RTC_LOG(LS_INFO) << "Setting local voice description for " << ToString(); RtpHeaderExtensions rtp_header_extensions = @@ -872,7 +862,6 @@ bool VoiceChannel::SetRemoteContent_w(const MediaContentDescription* content, SdpType type, std::string* error_desc) { TRACE_EVENT0("webrtc", "VoiceChannel::SetRemoteContent_w"); - RTC_DCHECK_RUN_ON(worker_thread()); RTC_LOG(LS_INFO) << "Setting remote voice description for " << ToString(); const AudioContentDescription* audio = content->as_audio(); @@ -953,7 +942,6 @@ VideoChannel::~VideoChannel() { void VideoChannel::UpdateMediaSendRecvState_w() { // Send outgoing data if we're the active call, we have the remote content, // and we have had some form of connectivity. - RTC_DCHECK_RUN_ON(worker_thread()); bool send = IsReadyToSendMedia_w(); if (!media_channel()->SetSend(send)) { RTC_LOG(LS_ERROR) << "Failed to SetSend on video channel: " + ToString(); @@ -974,7 +962,6 @@ bool VideoChannel::SetLocalContent_w(const MediaContentDescription* content, SdpType type, std::string* error_desc) { TRACE_EVENT0("webrtc", "VideoChannel::SetLocalContent_w"); - RTC_DCHECK_RUN_ON(worker_thread()); RTC_LOG(LS_INFO) << "Setting local video description for " << ToString(); RtpHeaderExtensions rtp_header_extensions = @@ -1065,7 +1052,6 @@ bool VideoChannel::SetRemoteContent_w(const MediaContentDescription* content, SdpType type, std::string* error_desc) { TRACE_EVENT0("webrtc", "VideoChannel::SetRemoteContent_w"); - RTC_DCHECK_RUN_ON(worker_thread()); RTC_LOG(LS_INFO) << "Setting remote video description for " << ToString(); const VideoContentDescription* video = content->as_video(); diff --git a/pc/channel.h b/pc/channel.h index 9d14db4989..76f6b54f5e 100644 --- a/pc/channel.h +++ b/pc/channel.h @@ -193,27 +193,39 @@ class BaseChannel : public ChannelInterface, } protected: - void set_local_content_direction(webrtc::RtpTransceiverDirection direction) { - RTC_DCHECK_RUN_ON(worker_thread()); + void set_local_content_direction(webrtc::RtpTransceiverDirection direction) + RTC_RUN_ON(worker_thread()) { local_content_direction_ = direction; } - void set_remote_content_direction(webrtc::RtpTransceiverDirection direction) { - RTC_DCHECK_RUN_ON(worker_thread()); + + webrtc::RtpTransceiverDirection local_content_direction() const + RTC_RUN_ON(worker_thread()) { + return local_content_direction_; + } + + void set_remote_content_direction(webrtc::RtpTransceiverDirection direction) + RTC_RUN_ON(worker_thread()) { remote_content_direction_ = direction; } - // These methods verify that: + + webrtc::RtpTransceiverDirection remote_content_direction() const + RTC_RUN_ON(worker_thread()) { + return remote_content_direction_; + } + + bool enabled() const RTC_RUN_ON(worker_thread()) { return enabled_; } + rtc::Thread* signaling_thread() const { return signaling_thread_; } + + // Call to verify that: // * The required content description directions have been set. // * The channel is enabled. - // * And for sending: - // - The SRTP filter is active if it's needed. - // - The transport has been writable before, meaning it should be at least - // possible to succeed in sending a packet. + // * The SRTP filter is active if it's needed. + // * The transport has been writable before, meaning it should be at least + // possible to succeed in sending a packet. // // When any of these properties change, UpdateMediaSendRecvState_w should be // called. - bool IsReadyToReceiveMedia_w() const RTC_RUN_ON(worker_thread()); bool IsReadyToSendMedia_w() const RTC_RUN_ON(worker_thread()); - rtc::Thread* signaling_thread() const { return signaling_thread_; } // NetworkInterface implementation, called by MediaEngine bool SendPacket(rtc::CopyOnWriteBuffer* packet, @@ -291,8 +303,8 @@ class BaseChannel : public ChannelInterface, std::string ToString() const; private: - bool ConnectToRtpTransport() RTC_RUN_ON(network_thread()); - void DisconnectFromRtpTransport() RTC_RUN_ON(network_thread()); + bool ConnectToRtpTransport_n() RTC_RUN_ON(network_thread()); + void DisconnectFromRtpTransport_n() RTC_RUN_ON(network_thread()); void SignalSentPacket_n(const rtc::SentPacket& sent_packet); rtc::Thread* const worker_thread_; @@ -387,20 +399,22 @@ class VoiceChannel : public BaseChannel { private: // overrides from BaseChannel - void UpdateMediaSendRecvState_w() override; + void UpdateMediaSendRecvState_w() RTC_RUN_ON(worker_thread()) override; bool SetLocalContent_w(const MediaContentDescription* content, webrtc::SdpType type, - std::string* error_desc) override; + std::string* error_desc) + RTC_RUN_ON(worker_thread()) override; bool SetRemoteContent_w(const MediaContentDescription* content, webrtc::SdpType type, - std::string* error_desc) override; + std::string* error_desc) + RTC_RUN_ON(worker_thread()) override; // Last AudioSendParameters sent down to the media_channel() via // SetSendParameters. - AudioSendParameters last_send_params_; + AudioSendParameters last_send_params_ RTC_GUARDED_BY(worker_thread()); // Last AudioRecvParameters sent down to the media_channel() via // SetRecvParameters. - AudioRecvParameters last_recv_params_; + AudioRecvParameters last_recv_params_ RTC_GUARDED_BY(worker_thread()); }; // VideoChannel is a specialization for video. @@ -421,28 +435,30 @@ class VideoChannel : public BaseChannel { return static_cast(BaseChannel::media_channel()); } - void FillBitrateInfo(BandwidthEstimationInfo* bwe_info); - cricket::MediaType media_type() const override { return cricket::MEDIA_TYPE_VIDEO; } + void FillBitrateInfo(BandwidthEstimationInfo* bwe_info); + private: // overrides from BaseChannel - void UpdateMediaSendRecvState_w() override; + void UpdateMediaSendRecvState_w() RTC_RUN_ON(worker_thread()) override; bool SetLocalContent_w(const MediaContentDescription* content, webrtc::SdpType type, - std::string* error_desc) override; + std::string* error_desc) + RTC_RUN_ON(worker_thread()) override; bool SetRemoteContent_w(const MediaContentDescription* content, webrtc::SdpType type, - std::string* error_desc) override; + std::string* error_desc) + RTC_RUN_ON(worker_thread()) override; // Last VideoSendParameters sent down to the media_channel() via // SetSendParameters. - VideoSendParameters last_send_params_; + VideoSendParameters last_send_params_ RTC_GUARDED_BY(worker_thread()); // Last VideoRecvParameters sent down to the media_channel() via // SetRecvParameters. - VideoRecvParameters last_recv_params_; + VideoRecvParameters last_recv_params_ RTC_GUARDED_BY(worker_thread()); }; } // namespace cricket