Move payload_type_demuxing_enabled_ to the network thread Bug: webrtc:42222117 Change-Id: I1414d6dc78e928029de5835f584b22dc5cfd85e2 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/456245 Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org> Reviewed-by: Henrik Boström <hbos@webrtc.org> Cr-Commit-Position: refs/heads/main@{#47151}
diff --git a/pc/channel.cc b/pc/channel.cc index a3f4f40..3f70792 100644 --- a/pc/channel.cc +++ b/pc/channel.cc
@@ -52,6 +52,7 @@ #include "rtc_base/socket.h" #include "rtc_base/strings/string_format.h" #include "rtc_base/thread.h" +#include "rtc_base/thread_annotations.h" #include "rtc_base/trace_event.h" #include "rtc_base/unique_id_generator.h" @@ -361,11 +362,44 @@ // network thread. At the moment there's a workaround for inconsistent state // between the worker and network thread because of this (see // OnDemuxerCriteriaUpdatePending elsewhere in this file) and - // SetPayloadTypeDemuxingEnabled_w has a BlockingCall over to the network + // UpdateRemoteStreams_w has a BlockingCall over to the network // thread to apply state updates. - RTC_DCHECK_RUN_ON(worker_thread()); + RTC_DCHECK_RUN_ON(network_thread()); TRACE_EVENT0("webrtc", "BaseChannel::SetPayloadTypeDemuxingEnabled"); - return SetPayloadTypeDemuxingEnabled_w(enabled); + + if (enabled == payload_type_demuxing_enabled_) { + return true; + } + + payload_type_demuxing_enabled_ = enabled; + + if (!enabled) { + worker_thread_->PostTask(SafeTask(alive_, [this] { + // TODO(crbug.com/11477): This will remove *all* unsignaled streams (those + // without an explicitly signaled SSRC), which may include streams that + // were matched to this channel by MID or RID. Ideally we'd remove only + // the streams that were matched based on payload type alone, but + // currently there is no straightforward way to identify those streams. + media_receive_channel()->ResetUnsignaledRecvStream(); + })); + } + + // TODO: bugs.webrtc.org/42222117 - payload_types_ is guarded by the worker + // thread, but we read it here on the network thread. This is safe because + // this method is called synchronously during SDP signaling, during which no + // tasks that modify payload_types_ can be running on the worker thread. + auto has_payload_types = [this]() RTC_NO_THREAD_SAFETY_ANALYSIS { + return !payload_types_.empty(); + }; + + if (has_payload_types()) { + if (!rtp_transport_) { + return false; + } + return rtp_transport_->RegisterRtpDemuxerSink(demuxer_criteria(), this); + } + + return true; } bool BaseChannel::IsReadyToSendMedia_w() const { @@ -705,33 +739,6 @@ RTC_LOG(LS_INFO) << "Channel not writable (" << ToString() << ")"; } -bool BaseChannel::SetPayloadTypeDemuxingEnabled_w(bool enabled) { - RTC_LOG_THREAD_BLOCK_COUNT(); - - if (enabled == payload_type_demuxing_enabled_) { - return true; - } - - payload_type_demuxing_enabled_ = enabled; - - if (!enabled) { - // TODO(crbug.com/11477): This will remove *all* unsignaled streams (those - // without an explicitly signaled SSRC), which may include streams that - // were matched to this channel by MID or RID. Ideally we'd remove only the - // streams that were matched based on payload type alone, but currently - // there is no straightforward way to identify those streams. - media_receive_channel()->ResetUnsignaledRecvStream(); - if (!payload_types_.empty()) { - return RegisterRtpDemuxerSink_w(); - } - } else if (!payload_types_.empty()) { - return RegisterRtpDemuxerSink_w(); - } - - RTC_DCHECK_BLOCK_COUNT_NO_MORE_THAN(0); - - return true; -} bool BaseChannel::UpdateLocalStreams_w(const std::vector<StreamParams>& streams, SdpType type, @@ -820,8 +827,9 @@ RTC_DLOG(LS_VERBOSE) << "UpdateRemoteStreams_w: remote side will not send " "- disable payload type demuxing for " << ToString(); - if (ClearHandledPayloadTypes()) { - needs_re_registration = payload_type_demuxing_enabled_; + if (!payload_types_.empty()) { + payload_types_.clear(); + needs_re_registration = true; } } @@ -913,15 +921,7 @@ bool BaseChannel::MaybeAddHandledPayloadType(int payload_type) { // Even if payload type demuxing is currently disabled, we need to remember // the payload types in case it's re-enabled later. - bool inserted = - payload_types_.insert(static_cast<uint8_t>(payload_type)).second; - return inserted && payload_type_demuxing_enabled_; -} - -bool BaseChannel::ClearHandledPayloadTypes() { - const bool was_empty = payload_types_.empty(); - payload_types_.clear(); - return !was_empty && payload_type_demuxing_enabled_; + return payload_types_.insert(static_cast<uint8_t>(payload_type)).second; } bool BaseChannel::CheckRtpExtensionValidity(
diff --git a/pc/channel.h b/pc/channel.h index 8a30c46..c53c1b4 100644 --- a/pc/channel.h +++ b/pc/channel.h
@@ -136,7 +136,8 @@ // This method will also remove any existing streams that were bound to this // channel on the basis of payload type, since one of these streams might // actually belong to a new channel. See: crbug.com/webrtc/11477 - bool SetPayloadTypeDemuxingEnabled(bool enabled) override; + bool SetPayloadTypeDemuxingEnabled(bool enabled) override + RTC_RUN_ON(network_thread()); void Enable(bool enable) override; @@ -249,9 +250,6 @@ void ChannelWritable_n() RTC_RUN_ON(network_thread()); void ChannelNotWritable_n() RTC_RUN_ON(network_thread()); - bool SetPayloadTypeDemuxingEnabled_w(bool enabled) - RTC_RUN_ON(worker_thread()); - // Should be called whenever the conditions for // IsReadyToReceiveMedia/IsReadyToSendMedia are satisfied (or unsatisfied). // Updates the send/recv state of the media channel. @@ -286,10 +284,6 @@ // is needed. bool MaybeAddHandledPayloadType(int payload_type) RTC_RUN_ON(worker_thread()); - // Returns true if the demuxer payload type criteria was non-empty before - // clearing. - bool ClearHandledPayloadTypes() RTC_RUN_ON(worker_thread()); - // Checks that the provided RTP header extensions are valid. // This verifies that all extension IDs are within the valid range, // that there are no duplicate IDs, and that no existing extension ID @@ -364,7 +358,7 @@ // call to the worker thread, so it should be safe. bool enabled_ RTC_GUARDED_BY(worker_thread()) = false; bool enabled_s_ RTC_GUARDED_BY(signaling_thread()) = false; - bool payload_type_demuxing_enabled_ RTC_GUARDED_BY(worker_thread()) = true; + bool payload_type_demuxing_enabled_ RTC_GUARDED_BY(network_thread()) = true; std::vector<StreamParams> local_streams_ RTC_GUARDED_BY(worker_thread()); std::vector<StreamParams> remote_streams_ RTC_GUARDED_BY(worker_thread()); RtpTransceiverDirection local_content_direction_
diff --git a/pc/rtp_transceiver.cc b/pc/rtp_transceiver.cc index a36eab4..6357644 100644 --- a/pc/rtp_transceiver.cc +++ b/pc/rtp_transceiver.cc
@@ -1366,7 +1366,7 @@ } bool RtpTransceiver::SetChannelPayloadTypeDemuxingEnabled(bool enabled) { - RTC_DCHECK_RUN_ON(context()->worker_thread()); + RTC_DCHECK_RUN_ON(context()->network_thread()); RTC_DCHECK(channel_); return channel_->SetPayloadTypeDemuxingEnabled(enabled); }
diff --git a/pc/sdp_offer_answer.cc b/pc/sdp_offer_answer.cc index 590a465..6048d98 100644 --- a/pc/sdp_offer_answer.cc +++ b/pc/sdp_offer_answer.cc
@@ -5962,12 +5962,10 @@ return true; } - // TODO(bugs.webrtc.org/11993): This BlockingCall() will also block on the - // network thread for every demuxer sink that needs to be updated. The demuxer - // state needs to be fully (and only) managed on the network thread and once - // that's the case, there's no need to stop by on the worker. Ideally we could - // also do this without blocking. - return context_->worker_thread()->BlockingCall([&channels_to_update]() { + // TODO(bugs.webrtc.org/11993): The demuxer state is now fully managed on + // the network thread. So we do a single blocking call here to the network + // thread. Ideally we could also do this without blocking. + return context_->network_thread()->BlockingCall([&channels_to_update]() { for (const auto& it : channels_to_update) { if (!it.second->SetChannelPayloadTypeDemuxingEnabled(it.first)) { // Note that the state has already been irrevocably changed at this