From 9abd74dc1e453a2b0bfd5521b25704db4423ff1e Mon Sep 17 00:00:00 2001 From: Tomas Gunnarsson Date: Tue, 4 Jan 2022 17:21:54 +0000 Subject: [PATCH] Make RtpDemuxerCriteria's mid_ and rsid_ const. Remove unnecessary optimization from BaseChannel, previous_demuxer_criteria_, that I'm not seeing as providing value. Previously it was used to avoid a thread hop if a reconfiguration wasn't needed, but the way that was done, wasn't thread safe. So after addressing that issue, the variable more represents increased complexity in the code than runtime efficiency. Bug: webrtc:11993, webrtc:12230 Change-Id: Ic8e3e1d2f57e669a168cc7b5cf5d407133976e3c Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/244093 Reviewed-by: Harald Alvestrand Commit-Queue: Tomas Gunnarsson Cr-Commit-Position: refs/heads/main@{#35628} --- call/rtp_demuxer.h | 7 ++----- pc/channel.cc | 22 +++++----------------- pc/channel.h | 4 ---- 3 files changed, 7 insertions(+), 26 deletions(-) diff --git a/call/rtp_demuxer.h b/call/rtp_demuxer.h index fa90888d9c..5fd37b4613 100644 --- a/call/rtp_demuxer.h +++ b/call/rtp_demuxer.h @@ -65,11 +65,8 @@ class RtpDemuxerCriteria { private: // Intentionally private member variables to encourage specifying them via the // constructor and consider them to be const as much as possible. - // Post construction, reading the values needs to be done via accessors and if - // changing the value is required, that can still be done via the implicit - // assignment operator (which also reassigns all other member variables). - std::string mid_; - std::string rsid_; + const std::string mid_; + const std::string rsid_; flat_set ssrcs_; flat_set payload_types_; }; diff --git a/pc/channel.cc b/pc/channel.cc index 36900c4cfc..eb7219cc84 100644 --- a/pc/channel.cc +++ b/pc/channel.cc @@ -163,11 +163,7 @@ bool BaseChannel::ConnectToRtpTransport() { // We don't need to call OnDemuxerCriteriaUpdatePending/Complete because // there's no previous criteria to worry about. - bool result = rtp_transport_->RegisterRtpDemuxerSink(demuxer_criteria_, this); - if (result) { - previous_demuxer_criteria_ = demuxer_criteria_; - } else { - previous_demuxer_criteria_ = {}; + if (!rtp_transport_->RegisterRtpDemuxerSink(demuxer_criteria_, this)) { RTC_LOG(LS_ERROR) << "Failed to set up demuxing for " << ToString(); return false; } @@ -493,18 +489,10 @@ bool BaseChannel::RegisterRtpDemuxerSink_w() { bool ret = network_thread_->Invoke( RTC_FROM_HERE, [this, demuxer_criteria = demuxer_criteria_] { RTC_DCHECK_RUN_ON(network_thread()); - RTC_DCHECK(rtp_transport_); - if (demuxer_criteria_ == previous_demuxer_criteria_) - return true; - - bool result = - rtp_transport_->RegisterRtpDemuxerSink(demuxer_criteria, this); - if (result) { - previous_demuxer_criteria_ = demuxer_criteria; - } else { - previous_demuxer_criteria_ = {}; - } - return result; + // Note that RegisterRtpDemuxerSink first unregisters the sink if + // already registered. So this will change the state of the class + // whether the call succeeds or not. + return rtp_transport_->RegisterRtpDemuxerSink(demuxer_criteria, this); }); media_channel_->OnDemuxerCriteriaUpdateComplete(); diff --git a/pc/channel.h b/pc/channel.h index 452fe5a31a..9d14db4989 100644 --- a/pc/channel.h +++ b/pc/channel.h @@ -355,10 +355,6 @@ class BaseChannel : public ChannelInterface, // TODO(bugs.webrtc.org/12239): Modified on worker thread, accessed // on network thread in RegisterRtpDemuxerSink_n (called from Init_w) webrtc::RtpDemuxerCriteria demuxer_criteria_; - // Accessed on the worker thread, modified on the network thread from - // RegisterRtpDemuxerSink_w's Invoke. - webrtc::RtpDemuxerCriteria previous_demuxer_criteria_ - RTC_GUARDED_BY(network_thread()); // This generator is used to generate SSRCs for local streams. // This is needed in cases where SSRCs are not negotiated or set explicitly // like in Simulcast.